Hi Hanjun!

W dniu 10.09.2013 14:16, Hanjun Guo pisze:

Hi Graeme,

After some deep thoughts about introducing CONFIG_HARDWARE_REDUCED_ACPI, I think it is not a good idea to do that, so I drop this idea, and try to fix it as this patch did.

Comments are welcomed :)

ACPI spec states that fixed feature like ACPI specific registers do not fall within reduced profile. So each portion of ACPICA code that operate on ACPI specific hardware should be secured by: if (acpi_gbl_reduced_hardware) { return_ACPI_STATUS(AE_NOT_IMPLEMENTED); } IMHO, your patch does it in the right way. Bug should be reported to ACPICA along with this patch as solution proposal.

Regards, Tomasz

Thanks Hanjun

On 2013-9-10 20:03, Hanjun Guo wrote:

...

On hardware reduced platforms, there is no support for the following:

• PM Event and Control registers
• SCI interrupt (and handler)
• Fixed Events
• General Purpose Events (GPEs)
• Global Lock
• ACPI PM timer
• FACS table (Waking vectors and Global Lock)

but when FACP Hardware Reduced flag is set, ACPI drivers will still access power management registers directly which will cause oops when system booted.

acpi_gbl_reduced_hardware is a global flag and can be used to prevent misbehavior on hardware reduced platforms, and this flag is initialized at the very beginning of the system boot when FADT is parsed, so we can use it to prevent accessing PM registers.

There are still lots of other places to be fixed, this is the first patch to fix hte bug we met when --cores=1;

I will finish a patch set and send to upstream if you guys have no objections. I think we should start a discussion in ACPI mail list for this bug on hardware reduced platforms now.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

drivers/acpi/acpica/hwregs.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c index 8d2e866..4c270c3 100644 --- a/drivers/acpi/acpica/hwregs.c +++ b/drivers/acpi/acpica/hwregs.c @@ -265,6 +265,11 @@ acpi_status acpi_hw_clear_acpi_status(void)

ACPI_FUNCTION_TRACE(hw_clear_acpi_status);

• /* If Hardware Reduced flag is set, there are no GPEs */
• if (acpi_gbl_reduced_hardware) {

• return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }
• ACPI_DEBUG_PRINT((ACPI_DB_IO, "About to write %04X to %8.8X%8.8X\n", ACPI_BITMASK_ALL_FIXED_STATUS, ACPI_FORMAT_UINT64(acpi_gbl_xpm1a_status.address)));

@@ -305,6 +310,10 @@ struct acpi_bit_register_info *acpi_hw_get_bit_register_info(u32 register_id) { ACPI_FUNCTION_ENTRY();

• if (acpi_gbl_reduced_hardware) {

• return (NULL);
  

• }
• if (register_id > ACPI_BITREG_MAX) { ACPI_ERROR((AE_INFO, "Invalid BitRegister ID: 0x%X", register_id));

@@ -337,6 +346,11 @@ acpi_status acpi_hw_write_pm1_control(u32 pm1a_control, u32 pm1b_control)

ACPI_FUNCTION_TRACE(hw_write_pm1_control);

• /* If Hardware Reduced flag is set, there are no PM Control registers */
• if (acpi_gbl_reduced_hardware) {

• return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }
• status = acpi_hw_write(pm1a_control, &acpi_gbl_FADT.xpm1a_control_block); if (ACPI_FAILURE(status)) {

@@ -370,6 +384,11 @@ acpi_status acpi_hw_register_read(u32 register_id, u32 *return_value)

ACPI_FUNCTION_TRACE(hw_register_read);

• /* If Hardware Reduced flag is set, there are no PM Control registers */
• if (acpi_gbl_reduced_hardware) {

• return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }
• switch (register_id) { case ACPI_REGISTER_PM1_STATUS: /* PM1 A/B: 16-bit access each */

@@ -465,6 +484,11 @@ acpi_status acpi_hw_register_write(u32 register_id, u32 value)

ACPI_FUNCTION_TRACE(hw_register_write);

• /* If Hardware Reduced flag is set, there are no PM Control registers */
• if (acpi_gbl_reduced_hardware) {

• return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }
• switch (register_id) { case ACPI_REGISTER_PM1_STATUS: /* PM1 A/B: 16-bit access each */ /*

---

Linaro-acpi mailing list Linaro-acpi@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-acpi

On 09/11/2013 05:06 AM, Hanjun Guo wrote:

On 2013-9-10 23:15, Al Stone wrote:

...

On 09/10/2013 06:03 AM, Hanjun Guo wrote:

...

On hardware reduced platforms, there is no support for the following:

• PM Event and Control registers
• SCI interrupt (and handler)
• Fixed Events
• General Purpose Events (GPEs)
• Global Lock
• ACPI PM timer
• FACS table (Waking vectors and Global Lock)

but when FACP Hardware Reduced flag is set, ACPI drivers will still access power management registers directly which will cause oops when system booted.

acpi_gbl_reduced_hardware is a global flag and can be used to prevent misbehavior on hardware reduced platforms, and this flag is initialized at the very beginning of the system boot when FADT is parsed, so we can use it to prevent accessing PM registers.

There are still lots of other places to be fixed, this is the first patch to fix hte bug we met when --cores=1;

I will finish a patch set and send to upstream if you guys have no objections. I think we should start a discussion in ACPI mail list for this bug on hardware reduced platforms now.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

drivers/acpi/acpica/hwregs.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c index 8d2e866..4c270c3 100644 --- a/drivers/acpi/acpica/hwregs.c +++ b/drivers/acpi/acpica/hwregs.c @@ -265,6 +265,11 @@ acpi_status acpi_hw_clear_acpi_status(void)

   ACPI_FUNCTION_TRACE(hw_clear_acpi_status);

• /* If Hardware Reduced flag is set, there are no GPEs */
• if (acpi_gbl_reduced_hardware) {

•    return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }

•  ACPI_DEBUG_PRINT((ACPI_DB_IO, "About to write %04X to %8.8X%8.8X\n",
             ACPI_BITMASK_ALL_FIXED_STATUS,
             ACPI_FORMAT_UINT64(acpi_gbl_xpm1a_status.address)));
  

@@ -305,6 +310,10 @@ struct acpi_bit_register_info *acpi_hw_get_bit_register_info(u32 register_id) { ACPI_FUNCTION_ENTRY();

• if (acpi_gbl_reduced_hardware) {

•    return (NULL);
  

• }

•  if (register_id > ACPI_BITREG_MAX) {
       ACPI_ERROR((AE_INFO, "Invalid BitRegister ID: 0x%X",
               register_id));
  

@@ -337,6 +346,11 @@ acpi_status acpi_hw_write_pm1_control(u32 pm1a_control, u32 pm1b_control)

   ACPI_FUNCTION_TRACE(hw_write_pm1_control);

• /* If Hardware Reduced flag is set, there are no PM Control registers */
• if (acpi_gbl_reduced_hardware) {

•    return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }

•  status =
       acpi_hw_write(pm1a_control, &acpi_gbl_FADT.xpm1a_control_block);
   if (ACPI_FAILURE(status)) {
  

@@ -370,6 +384,11 @@ acpi_status acpi_hw_register_read(u32 register_id, u32 *return_value)

   ACPI_FUNCTION_TRACE(hw_register_read);

• /* If Hardware Reduced flag is set, there are no PM Control registers */
• if (acpi_gbl_reduced_hardware) {

•    return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }

•  switch (register_id) {
   case ACPI_REGISTER_PM1_STATUS:    /* PM1 A/B: 16-bit access each */
  
  

@@ -465,6 +484,11 @@ acpi_status acpi_hw_register_write(u32 register_id, u32 value)

   ACPI_FUNCTION_TRACE(hw_register_write);

• /* If Hardware Reduced flag is set, there are no PM Control registers */
• if (acpi_gbl_reduced_hardware) {

•    return_ACPI_STATUS(AE_NOT_IMPLEMENTED);
  

• }

•  switch (register_id) {
   case ACPI_REGISTER_PM1_STATUS:    /* PM1 A/B: 16-bit access each */
       /*
  
  

I'm still thinking this patch through. Functional Fixed HW is _not_ precluded by the reduced HW profile, unfortunately (at least according to Dong Wei, the spec chair for 5.0). I am still trying to figure out

Something is illogical but we should follow and fire bugs on ARM platform?

Oh! No, that was not what I meant to say. All I am saying is that I am puzzled about what the spec is saying. The patch itself is a good one, I think.

...

what that really means for these registers -- on the one hand, it makes no sense at all to use them in reduced HW profile, but on the other hand they are not prohibited.

do you mean I should suspend the patches to upstream until we get this clear?

No, I think the better thing to do is what you have already suggested: submit these upstream and see if there is some sort of feedback that can clarify things.

...

It makes my head hurt :(.

It makes my head hurt too :(

The next time we meet, I'll bring aspirin :).