[PATCH v2 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[PATCH v2 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Sudeep Holla]

Here is a summary of the changes in this patch series:

1. Fix for race condition in updating of the chan_in_use flag

   Ensures correct updating of the chan_in_use flag to avoid potential race
   conditions.

2. Interrupt handling fix

   Ensures platform acknowledgment interrupts are always cleared to avoid
   leaving the interrupt asserted forever.

3. Endian conversion cleanup

   Removes unnecessary endianness conversion in the PCC mailbox driver.

4. Memory mapping improvements

   Uses acpi_os_ioremap() instead of direct mapping methods for better ACPI
   compatibility.

5. Return early if the command complete register is absent

   Ensures that if no GAS (Generic Address Structure) register is available,
   the function exits early.

6. Refactor IRQ handler and move error handling to a separate function

   Improves readability of error handling in the PCC mailbox driver’s
   interrupt handler.

7. Code restructuring to avoid unnecessary forward declaration

   Moves pcc_mbox_ioremap() function to a more appropriate location with
   no functional impact.

8. Shared memory mapping refactoring/enhancements

   Ensures the shared memory is always mapped and unmapped in the PCC
   mailbox driver when the PCC channel is requested and release.

9. Refactored check_and_ack() Function

   Simplifies and improves the logic for handling type4 platform notification
   acknowledgments.

10-14. Shared memory handling simplifications across multiple drivers

    Simplifies shared memory handling in:
        Kunpeng HCCS driver (soc: hisilicon)
        Apm X-Gene Slimpro I2C driver
        X-Gene hardware monitoring driver (hwmon)
        ACPI PCC driver
        ACPI CPPC driver

The X-gene related changes now change the mapping attributes to align
with ACPI specification. There are possibilities for more cleanups on
top of these changes around how the shmem is accessed within these
driver.

Also, my main aim is to get 1-8 merged first and target 9-13 for
following merge window through respective tree.

Overall, the patch series focuses on improving correctness, efficiency, and
maintainability of the PCC mailbox driver and related components by fixing
race conditions, optimizing memory handling, simplifying shared memory
interactions, and refactoring code for clarity.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
Adam, Robbie, Huisong,

Please test this in your setup as you are the ones reporting/fixing the
issues or last modified the code that I am changing here.

Changes in v2:
- Improved time vs flow graph for the platform ack interrupt
  acknowledgment issue in patch 2
- Replaced PCC_ACK_FLAG_MASK with PCC_CMD_COMPLETION_NOTIFY in patch 3
- Fixed return value check from pcc_mbox_error_check_and_clear() in patch 6
- Dropped the change moving the function pcc_mbox_ioremap()
- Adjusted error message in kunpeng_hccs driver after the change
- Added the received ack/review tags
- Link to v1: https://lore.kernel.org/r/20250303-pcc_fixes_updates-v1-0-3b44f3d134b1@arm.com

---
Huisong Li (1):
      mailbox: pcc: Fix the possible race in updation of chan_in_use flag

Sudeep Holla (12):
      mailbox: pcc: Always clear the platform ack interrupt first
      mailbox: pcc: Drop unnecessary endianness conversion of pcc_hdr.flags
      mailbox: pcc: Return early if no GAS register from pcc_mbox_cmd_complete_check
      mailbox: pcc: Use acpi_os_ioremap() instead of ioremap()
      mailbox: pcc: Refactor error handling in irq handler into separate function
      mailbox: pcc: Always map the shared memory communication address
      mailbox: pcc: Refactor and simplify check_and_ack()
      soc: hisilicon: kunpeng_hccs: Simplify PCC shared memory region handling
      i2c: xgene-slimpro: Simplify PCC shared memory region handling
      hwmon: (xgene-hwmon) Simplify PCC shared memory region handling
      ACPI: PCC: Simplify PCC shared memory region handling
      ACPI: CPPC: Simplify PCC shared memory region handling

 drivers/acpi/acpi_pcc.c                |  13 +---
 drivers/acpi/cppc_acpi.c               |  16 +----
 drivers/hwmon/xgene-hwmon.c            |  40 ++----------
 drivers/i2c/busses/i2c-xgene-slimpro.c |  39 ++----------
 drivers/mailbox/pcc.c                  | 112 ++++++++++++++++-----------------
 drivers/soc/hisilicon/kunpeng_hccs.c   |  42 +++++--------
 drivers/soc/hisilicon/kunpeng_hccs.h   |   2 -
 include/acpi/pcc.h                     |   6 --
 8 files changed, 83 insertions(+), 187 deletions(-)
---
base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
change-id: 20250303-pcc_fixes_updates-55a17fd28e76

Best regards,
-- 
Regards,
Sudeep

[PATCH v2 01/13] mailbox: pcc: Fix the possible race in updation of chan_in_use flag

[Sudeep Holla]

From: Huisong Li <lihuisong@huawei.com>

The function mbox_chan_received_data() calls the Rx callback of the
mailbox client driver. The callback might set chan_in_use flag from
pcc_send_data(). This flag's status determines whether the PCC channel
is in use.

However, there is a potential race condition where chan_in_use is
updated incorrectly due to concurrency between the interrupt handler
(pcc_mbox_irq()) and the command sender(pcc_send_data()).

The 'chan_in_use' flag of a channel is set to true after sending a
command. And the flag of the new command may be cleared erroneous by
the interrupt handler afer mbox_chan_received_data() returns,

As a result, the interrupt being level triggered can't be cleared in
pcc_mbox_irq() and it will be disabled after the number of handled times
exceeds the specified value. The error log is as follows:

  |  kunpeng_hccs HISI04B2:00: PCC command executed timeout!
  |  kunpeng_hccs HISI04B2:00: get port link status info failed, ret = -110
  |  irq 13: nobody cared (try booting with the "irqpoll" option)
  |  Call trace:
  |   dump_backtrace+0x0/0x210
  |   show_stack+0x1c/0x2c
  |   dump_stack+0xec/0x130
  |   __report_bad_irq+0x50/0x190
  |   note_interrupt+0x1e4/0x260
  |   handle_irq_event+0x144/0x17c
  |   handle_fasteoi_irq+0xd0/0x240
  |   __handle_domain_irq+0x80/0xf0
  |   gic_handle_irq+0x74/0x2d0
  |   el1_irq+0xbc/0x140
  |   mnt_clone_write+0x0/0x70
  |   file_update_time+0xcc/0x160
  |   fault_dirty_shared_page+0xe8/0x150
  |   do_shared_fault+0x80/0x1d0
  |   do_fault+0x118/0x1a4
  |   handle_pte_fault+0x154/0x230
  |   __handle_mm_fault+0x1ac/0x390
  |   handle_mm_fault+0xf0/0x250
  |   do_page_fault+0x184/0x454
  |   do_translation_fault+0xac/0xd4
  |   do_mem_abort+0x44/0xb4
  |   el0_da+0x40/0x74
  |   el0_sync_handler+0x60/0xb4
  |   el0_sync+0x168/0x180
  |  handlers:
  |   pcc_mbox_irq
  |  Disabling IRQ #13

To solve this issue, pcc_mbox_irq() must clear 'chan_in_use' flag before
the call to mbox_chan_received_data().

Signed-off-by: Huisong Li <lihuisong@huawei.com>
(sudeep.holla: Minor updates to the subject and commit message)
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 82102a4c5d68839170238540a6fed61afa5185a0..f2e4087281c70eeb5b9b33371596613a371dff4f 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -333,10 +333,15 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
 		return IRQ_NONE;
 
+	/*
+	 * Clear this flag immediately after updating interrupt ack register
+	 * to avoid possible race in updatation of the flag from
+	 * pcc_send_data() that could execute from mbox_chan_received_data()
+	 */
+	pchan->chan_in_use = false;
 	mbox_chan_received_data(chan, NULL);
 
 	check_and_ack(pchan, chan);
-	pchan->chan_in_use = false;
 
 	return IRQ_HANDLED;
 }

-- 
2.34.1

Re: [PATCH v2 01/13] mailbox: pcc: Fix the possible race in updation of chan_in_use flag

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> From: Huisong Li <lihuisong@huawei.com>
>
> The function mbox_chan_received_data() calls the Rx callback of the
> mailbox client driver. The callback might set chan_in_use flag from
> pcc_send_data(). This flag's status determines whether the PCC channel
> is in use.
>
> However, there is a potential race condition where chan_in_use is
> updated incorrectly due to concurrency between the interrupt handler
> (pcc_mbox_irq()) and the command sender(pcc_send_data()).
>
> The 'chan_in_use' flag of a channel is set to true after sending a
> command. And the flag of the new command may be cleared erroneous by
> the interrupt handler afer mbox_chan_received_data() returns,
>
> As a result, the interrupt being level triggered can't be cleared in
> pcc_mbox_irq() and it will be disabled after the number of handled times
> exceeds the specified value. The error log is as follows:
>
>    |  kunpeng_hccs HISI04B2:00: PCC command executed timeout!
>    |  kunpeng_hccs HISI04B2:00: get port link status info failed, ret = -110
>    |  irq 13: nobody cared (try booting with the "irqpoll" option)
>    |  Call trace:
>    |   dump_backtrace+0x0/0x210
>    |   show_stack+0x1c/0x2c
>    |   dump_stack+0xec/0x130
>    |   __report_bad_irq+0x50/0x190
>    |   note_interrupt+0x1e4/0x260
>    |   handle_irq_event+0x144/0x17c
>    |   handle_fasteoi_irq+0xd0/0x240
>    |   __handle_domain_irq+0x80/0xf0
>    |   gic_handle_irq+0x74/0x2d0
>    |   el1_irq+0xbc/0x140
>    |   mnt_clone_write+0x0/0x70
>    |   file_update_time+0xcc/0x160
>    |   fault_dirty_shared_page+0xe8/0x150
>    |   do_shared_fault+0x80/0x1d0
>    |   do_fault+0x118/0x1a4
>    |   handle_pte_fault+0x154/0x230
>    |   __handle_mm_fault+0x1ac/0x390
>    |   handle_mm_fault+0xf0/0x250
>    |   do_page_fault+0x184/0x454
>    |   do_translation_fault+0xac/0xd4
>    |   do_mem_abort+0x44/0xb4
>    |   el0_da+0x40/0x74
>    |   el0_sync_handler+0x60/0xb4
>    |   el0_sync+0x168/0x180
>    |  handlers:
>    |   pcc_mbox_irq
>    |  Disabling IRQ #13
>
> To solve this issue, pcc_mbox_irq() must clear 'chan_in_use' flag before
> the call to mbox_chan_received_data().
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> (sudeep.holla: Minor updates to the subject and commit message)
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 82102a4c5d68839170238540a6fed61afa5185a0..f2e4087281c70eeb5b9b33371596613a371dff4f 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -333,10 +333,15 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>   		return IRQ_NONE;
>   
> +	/*
> +	 * Clear this flag immediately after updating interrupt ack register
> +	 * to avoid possible race in updatation of the flag from
> +	 * pcc_send_data() that could execute from mbox_chan_received_data()
This comment may be inappropriate becuase of the moving of clearing 
interrupt ack register in patch 2/13.
I suggested that fix it in this patch or patch 2/13.
> +	 */
> +	pchan->chan_in_use = false;
>   	mbox_chan_received_data(chan, NULL);
>   
>   	check_and_ack(pchan, chan);
> -	pchan->chan_in_use = false;
>   
>   	return IRQ_HANDLED;
>   }
>

Re: [PATCH v2 01/13] mailbox: pcc: Fix the possible race in updation of chan_in_use flag

[Sudeep Holla]

On Tue, Mar 11, 2025 at 07:40:53PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/6 0:38, Sudeep Holla 写道:
> > From: Huisong Li <lihuisong@huawei.com>
> > 
> > The function mbox_chan_received_data() calls the Rx callback of the
> > mailbox client driver. The callback might set chan_in_use flag from
> > pcc_send_data(). This flag's status determines whether the PCC channel
> > is in use.
> > 
> > However, there is a potential race condition where chan_in_use is
> > updated incorrectly due to concurrency between the interrupt handler
> > (pcc_mbox_irq()) and the command sender(pcc_send_data()).
> > 
> > The 'chan_in_use' flag of a channel is set to true after sending a
> > command. And the flag of the new command may be cleared erroneous by
> > the interrupt handler afer mbox_chan_received_data() returns,
> > 
> > As a result, the interrupt being level triggered can't be cleared in
> > pcc_mbox_irq() and it will be disabled after the number of handled times
> > exceeds the specified value. The error log is as follows:
> > 
> >    |  kunpeng_hccs HISI04B2:00: PCC command executed timeout!
> >    |  kunpeng_hccs HISI04B2:00: get port link status info failed, ret = -110
> >    |  irq 13: nobody cared (try booting with the "irqpoll" option)
> >    |  Call trace:
> >    |   dump_backtrace+0x0/0x210
> >    |   show_stack+0x1c/0x2c
> >    |   dump_stack+0xec/0x130
> >    |   __report_bad_irq+0x50/0x190
> >    |   note_interrupt+0x1e4/0x260
> >    |   handle_irq_event+0x144/0x17c
> >    |   handle_fasteoi_irq+0xd0/0x240
> >    |   __handle_domain_irq+0x80/0xf0
> >    |   gic_handle_irq+0x74/0x2d0
> >    |   el1_irq+0xbc/0x140
> >    |   mnt_clone_write+0x0/0x70
> >    |   file_update_time+0xcc/0x160
> >    |   fault_dirty_shared_page+0xe8/0x150
> >    |   do_shared_fault+0x80/0x1d0
> >    |   do_fault+0x118/0x1a4
> >    |   handle_pte_fault+0x154/0x230
> >    |   __handle_mm_fault+0x1ac/0x390
> >    |   handle_mm_fault+0xf0/0x250
> >    |   do_page_fault+0x184/0x454
> >    |   do_translation_fault+0xac/0xd4
> >    |   do_mem_abort+0x44/0xb4
> >    |   el0_da+0x40/0x74
> >    |   el0_sync_handler+0x60/0xb4
> >    |   el0_sync+0x168/0x180
> >    |  handlers:
> >    |   pcc_mbox_irq
> >    |  Disabling IRQ #13
> > 
> > To solve this issue, pcc_mbox_irq() must clear 'chan_in_use' flag before
> > the call to mbox_chan_received_data().
> > 
> > Signed-off-by: Huisong Li <lihuisong@huawei.com>
> > (sudeep.holla: Minor updates to the subject and commit message)
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >   drivers/mailbox/pcc.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > index 82102a4c5d68839170238540a6fed61afa5185a0..f2e4087281c70eeb5b9b33371596613a371dff4f 100644
> > --- a/drivers/mailbox/pcc.c
> > +++ b/drivers/mailbox/pcc.c
> > @@ -333,10 +333,15 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> >   	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
> >   		return IRQ_NONE;
> > +	/*
> > +	 * Clear this flag immediately after updating interrupt ack register
> > +	 * to avoid possible race in updatation of the flag from
> > +	 * pcc_send_data() that could execute from mbox_chan_received_data()
> This comment may be inappropriate becuase of the moving of clearing
> interrupt ack register in patch 2/13.
> I suggested that fix it in this patch or patch 2/13.

Right, I did think of updating or did update and missed to commit.
I wanted to update something like:
"
Clear this flag after updating interrupt ack register and just before
mbox_chan_received_data() which might call pcc_send_data() where the
flag is set again to start new transfer
"

Hope that is more apt to the current situation.

-- 
Regards,
Sudeep

Re: [PATCH v2 01/13] mailbox: pcc: Fix the possible race in updation of chan_in_use flag

[lihuisong (C)]

在 2025/3/11 20:02, Sudeep Holla 写道:
> On Tue, Mar 11, 2025 at 07:40:53PM +0800, lihuisong (C) wrote:
>> 在 2025/3/6 0:38, Sudeep Holla 写道:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> The function mbox_chan_received_data() calls the Rx callback of the
>>> mailbox client driver. The callback might set chan_in_use flag from
>>> pcc_send_data(). This flag's status determines whether the PCC channel
>>> is in use.
>>>
>>> However, there is a potential race condition where chan_in_use is
>>> updated incorrectly due to concurrency between the interrupt handler
>>> (pcc_mbox_irq()) and the command sender(pcc_send_data()).
>>>
>>> The 'chan_in_use' flag of a channel is set to true after sending a
>>> command. And the flag of the new command may be cleared erroneous by
>>> the interrupt handler afer mbox_chan_received_data() returns,
>>>
>>> As a result, the interrupt being level triggered can't be cleared in
>>> pcc_mbox_irq() and it will be disabled after the number of handled times
>>> exceeds the specified value. The error log is as follows:
>>>
>>>     |  kunpeng_hccs HISI04B2:00: PCC command executed timeout!
>>>     |  kunpeng_hccs HISI04B2:00: get port link status info failed, ret = -110
>>>     |  irq 13: nobody cared (try booting with the "irqpoll" option)
>>>     |  Call trace:
>>>     |   dump_backtrace+0x0/0x210
>>>     |   show_stack+0x1c/0x2c
>>>     |   dump_stack+0xec/0x130
>>>     |   __report_bad_irq+0x50/0x190
>>>     |   note_interrupt+0x1e4/0x260
>>>     |   handle_irq_event+0x144/0x17c
>>>     |   handle_fasteoi_irq+0xd0/0x240
>>>     |   __handle_domain_irq+0x80/0xf0
>>>     |   gic_handle_irq+0x74/0x2d0
>>>     |   el1_irq+0xbc/0x140
>>>     |   mnt_clone_write+0x0/0x70
>>>     |   file_update_time+0xcc/0x160
>>>     |   fault_dirty_shared_page+0xe8/0x150
>>>     |   do_shared_fault+0x80/0x1d0
>>>     |   do_fault+0x118/0x1a4
>>>     |   handle_pte_fault+0x154/0x230
>>>     |   __handle_mm_fault+0x1ac/0x390
>>>     |   handle_mm_fault+0xf0/0x250
>>>     |   do_page_fault+0x184/0x454
>>>     |   do_translation_fault+0xac/0xd4
>>>     |   do_mem_abort+0x44/0xb4
>>>     |   el0_da+0x40/0x74
>>>     |   el0_sync_handler+0x60/0xb4
>>>     |   el0_sync+0x168/0x180
>>>     |  handlers:
>>>     |   pcc_mbox_irq
>>>     |  Disabling IRQ #13
>>>
>>> To solve this issue, pcc_mbox_irq() must clear 'chan_in_use' flag before
>>> the call to mbox_chan_received_data().
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> (sudeep.holla: Minor updates to the subject and commit message)
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>    drivers/mailbox/pcc.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index 82102a4c5d68839170238540a6fed61afa5185a0..f2e4087281c70eeb5b9b33371596613a371dff4f 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -333,10 +333,15 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>>    	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>>>    		return IRQ_NONE;
>>> +	/*
>>> +	 * Clear this flag immediately after updating interrupt ack register
>>> +	 * to avoid possible race in updatation of the flag from
>>> +	 * pcc_send_data() that could execute from mbox_chan_received_data()
>> This comment may be inappropriate becuase of the moving of clearing
>> interrupt ack register in patch 2/13.
>> I suggested that fix it in this patch or patch 2/13.
> Right, I did think of updating or did update and missed to commit.
> I wanted to update something like:
> "
> Clear this flag after updating interrupt ack register and just before
> mbox_chan_received_data() which might call pcc_send_data() where the
> flag is set again to start new transfer
> "
Ack
>
> Hope that is more apt to the current situation.
>

Re: [PATCH v2 01/13] mailbox: pcc: Fix the possible race in updation of chan_in_use flag

[Robbie King]

On 3/5/2025 11:38 AM, Sudeep Holla wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> The function mbox_chan_received_data() calls the Rx callback of the
> mailbox client driver. The callback might set chan_in_use flag from
> pcc_send_data(). This flag's status determines whether the PCC channel
> is in use.
> 
> However, there is a potential race condition where chan_in_use is
> updated incorrectly due to concurrency between the interrupt handler
> (pcc_mbox_irq()) and the command sender(pcc_send_data()).
> 
> The 'chan_in_use' flag of a channel is set to true after sending a
> command. And the flag of the new command may be cleared erroneous by
> the interrupt handler afer mbox_chan_received_data() returns,
> 
> As a result, the interrupt being level triggered can't be cleared in
> pcc_mbox_irq() and it will be disabled after the number of handled times
> exceeds the specified value. The error log is as follows:
> 
>   |  kunpeng_hccs HISI04B2:00: PCC command executed timeout!
>   |  kunpeng_hccs HISI04B2:00: get port link status info failed, ret = -110
>   |  irq 13: nobody cared (try booting with the "irqpoll" option)
>   |  Call trace:
>   |   dump_backtrace+0x0/0x210
>   |   show_stack+0x1c/0x2c
>   |   dump_stack+0xec/0x130
>   |   __report_bad_irq+0x50/0x190
>   |   note_interrupt+0x1e4/0x260
>   |   handle_irq_event+0x144/0x17c
>   |   handle_fasteoi_irq+0xd0/0x240
>   |   __handle_domain_irq+0x80/0xf0
>   |   gic_handle_irq+0x74/0x2d0
>   |   el1_irq+0xbc/0x140
>   |   mnt_clone_write+0x0/0x70
>   |   file_update_time+0xcc/0x160
>   |   fault_dirty_shared_page+0xe8/0x150
>   |   do_shared_fault+0x80/0x1d0
>   |   do_fault+0x118/0x1a4
>   |   handle_pte_fault+0x154/0x230
>   |   __handle_mm_fault+0x1ac/0x390
>   |   handle_mm_fault+0xf0/0x250
>   |   do_page_fault+0x184/0x454
>   |   do_translation_fault+0xac/0xd4
>   |   do_mem_abort+0x44/0xb4
>   |   el0_da+0x40/0x74
>   |   el0_sync_handler+0x60/0xb4
>   |   el0_sync+0x168/0x180
>   |  handlers:
>   |   pcc_mbox_irq
>   |  Disabling IRQ #13
> 
> To solve this issue, pcc_mbox_irq() must clear 'chan_in_use' flag before
> the call to mbox_chan_received_data().
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> (sudeep.holla: Minor updates to the subject and commit message)
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Tested-by: Robbie King <robbiek@xsightlabs.com>

[PATCH v2 02/13] mailbox: pcc: Always clear the platform ack interrupt first

[Sudeep Holla]

The PCC mailbox interrupt handler (pcc_mbox_irq()) currently checks
for command completion flags and any error status before clearing the
interrupt.

The below sequence highlights an issue in the handling of PCC mailbox
interrupts, specifically when dealing with doorbell notifications and
acknowledgment between the OSPM and the platform where type3 and type4
channels are sharing the interrupt.

-------------------------------------------------------------------------
| T |       Platform Firmware         |    OSPM/Linux PCC driver        |
|---|---------------------------------|---------------------------------|
| 1 |                                 | Build message in shmem          |
| 2 |                                 | Ring Type3 chan doorbell        |
| 3 | Receives the doorbell interrupt |                                 |
| 4 | Process the message from OSPM   |                                 |
| 5 | Build response for the message  |                                 |
| 6 | Ring Platform ACK interrupt on  |                                 |
|   |  Type3 chan to OSPM             | Received the interrupt          |
| 7 | Build Notification in Type4 Chan|                                 |
| 8 |                                 | Start processing interrupt in   |
|   |                                 |  pcc_mbox_irq() handler         |
| 9 |                                 | Enter PCC handler for Type4 chan|
|10 |                                 | Check command complete cleared  |
|11 |                                 | Read the notification           |
|12 |                                 | Clear Platform ACK interrupt    |
|   | No effect from the previous step yet as the Platform ACK          |
|   |  interrupt has not yet been triggered for this channel            |
|13 | Ring Platform ACK interrupt on  |                                 |
|   | Type4 chan to OSPM              |                                 |
|14 |                                 | Enter PCC handler for Type3 chan|
|15 |                                 | Command complete is set.        |
|16 |                                 | Read the response.              |
|17 |                                 | Clear Platform ACK interrupt    |
|18 |                                 | Leave PCC handler for Type3     |
|19 |                                 | Leave pcc_mbox_irq() handler    |
|20 |                                 | Re-enter pcc_mbox_irq() handler |
|21 |                                 | Enter PCC handler for Type4 chan|
|22 |                                 | Leave PCC handler for Type4 chan|
|23 |                                 | Enter PCC handler for Type3 chan|
|24 |                                 | Leave PCC handler for Type3 chan|
|25 |                                 | Leave pcc_mbox_irq() handler    |
-------------------------------------------------------------------------

The key issue occurs when OSPM tries to acknowledge platform ack
interrupt for a notification which is ready to be read and processed
but the interrupt itself is not yet triggered by the platform.

This ineffective acknowledgment leads to an issue later in time where
the interrupt remains pending as we exit the interrupt handler without
clearing the platform ack interrupt as there is no pending response or
notification. The interrupt acknowledgment order is incorrect.

To resolve this issue, the platform acknowledgment interrupt should
always be cleared before processing the interrupt for any notifications
or response.

Reported-by: Robbie King <robbiek@xsightlabs.com>
Reviewed-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index f2e4087281c70eeb5b9b33371596613a371dff4f..4c582fa2b8bf4c9a9368dba8220f567555dba963 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -313,6 +313,10 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	int ret;
 
 	pchan = chan->con_priv;
+
+	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
+		return IRQ_NONE;
+
 	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
 	    !pchan->chan_in_use)
 		return IRQ_NONE;
@@ -330,9 +334,6 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 		return IRQ_NONE;
 	}
 
-	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
-		return IRQ_NONE;
-
 	/*
 	 * Clear this flag immediately after updating interrupt ack register
 	 * to avoid possible race in updatation of the flag from

-- 
2.34.1

Re: [PATCH v2 02/13] mailbox: pcc: Always clear the platform ack interrupt first

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> The PCC mailbox interrupt handler (pcc_mbox_irq()) currently checks
> for command completion flags and any error status before clearing the
> interrupt.
>
> The below sequence highlights an issue in the handling of PCC mailbox
> interrupts, specifically when dealing with doorbell notifications and
> acknowledgment between the OSPM and the platform where type3 and type4
> channels are sharing the interrupt.
>
> -------------------------------------------------------------------------
> | T |       Platform Firmware         |    OSPM/Linux PCC driver        |
> |---|---------------------------------|---------------------------------|
> | 1 |                                 | Build message in shmem          |
> | 2 |                                 | Ring Type3 chan doorbell        |
> | 3 | Receives the doorbell interrupt |                                 |
> | 4 | Process the message from OSPM   |                                 |
> | 5 | Build response for the message  |                                 |
> | 6 | Ring Platform ACK interrupt on  |                                 |
> |   |  Type3 chan to OSPM             | Received the interrupt          |
> | 7 | Build Notification in Type4 Chan|                                 |
> | 8 |                                 | Start processing interrupt in   |
> |   |                                 |  pcc_mbox_irq() handler         |
> | 9 |                                 | Enter PCC handler for Type4 chan|
> |10 |                                 | Check command complete cleared  |
> |11 |                                 | Read the notification           |
> |12 |                                 | Clear Platform ACK interrupt    |
> |   | No effect from the previous step yet as the Platform ACK          |
> |   |  interrupt has not yet been triggered for this channel            |
> |13 | Ring Platform ACK interrupt on  |                                 |
> |   | Type4 chan to OSPM              |                                 |
> |14 |                                 | Enter PCC handler for Type3 chan|
> |15 |                                 | Command complete is set.        |
> |16 |                                 | Read the response.              |
> |17 |                                 | Clear Platform ACK interrupt    |
> |18 |                                 | Leave PCC handler for Type3     |
> |19 |                                 | Leave pcc_mbox_irq() handler    |
> |20 |                                 | Re-enter pcc_mbox_irq() handler |
> |21 |                                 | Enter PCC handler for Type4 chan|
> |22 |                                 | Leave PCC handler for Type4 chan|
> |23 |                                 | Enter PCC handler for Type3 chan|
> |24 |                                 | Leave PCC handler for Type3 chan|
> |25 |                                 | Leave pcc_mbox_irq() handler    |
> -------------------------------------------------------------------------
>
> The key issue occurs when OSPM tries to acknowledge platform ack
> interrupt for a notification which is ready to be read and processed
> but the interrupt itself is not yet triggered by the platform.
>
> This ineffective acknowledgment leads to an issue later in time where
> the interrupt remains pending as we exit the interrupt handler without
> clearing the platform ack interrupt as there is no pending response or
> notification. The interrupt acknowledgment order is incorrect.
>
> To resolve this issue, the platform acknowledgment interrupt should
> always be cleared before processing the interrupt for any notifications
> or response.
>
> Reported-by: Robbie King <robbiek@xsightlabs.com>
> Reviewed-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Tested-by: Huisong Li <lihuisong@huawei.com>
>

Re: [PATCH v2 02/13] mailbox: pcc: Always clear the platform ack interrupt first

[Adam Young]

On 3/5/25 11:38, Sudeep Holla wrote:
> The PCC mailbox interrupt handler (pcc_mbox_irq()) currently checks
> for command completion flags and any error status before clearing the
> interrupt.
>
> The below sequence highlights an issue in the handling of PCC mailbox
> interrupts, specifically when dealing with doorbell notifications and
> acknowledgment between the OSPM and the platform where type3 and type4
> channels are sharing the interrupt.
>
> -------------------------------------------------------------------------
> | T |       Platform Firmware         |    OSPM/Linux PCC driver        |
> |---|---------------------------------|---------------------------------|
> | 1 |                                 | Build message in shmem          |
> | 2 |                                 | Ring Type3 chan doorbell        |
> | 3 | Receives the doorbell interrupt |                                 |
> | 4 | Process the message from OSPM   |                                 |
> | 5 | Build response for the message  |                                 |
> | 6 | Ring Platform ACK interrupt on  |                                 |
> |   |  Type3 chan to OSPM             | Received the interrupt          |
> | 7 | Build Notification in Type4 Chan|                                 |
> | 8 |                                 | Start processing interrupt in   |
> |   |                                 |  pcc_mbox_irq() handler         |
> | 9 |                                 | Enter PCC handler for Type4 chan|
> |10 |                                 | Check command complete cleared  |
> |11 |                                 | Read the notification           |
> |12 |                                 | Clear Platform ACK interrupt    |
> |   | No effect from the previous step yet as the Platform ACK          |
> |   |  interrupt has not yet been triggered for this channel            |
> |13 | Ring Platform ACK interrupt on  |                                 |
> |   | Type4 chan to OSPM              |                                 |
> |14 |                                 | Enter PCC handler for Type3 chan|
> |15 |                                 | Command complete is set.        |
> |16 |                                 | Read the response.              |
> |17 |                                 | Clear Platform ACK interrupt    |
> |18 |                                 | Leave PCC handler for Type3     |
> |19 |                                 | Leave pcc_mbox_irq() handler    |
> |20 |                                 | Re-enter pcc_mbox_irq() handler |
> |21 |                                 | Enter PCC handler for Type4 chan|
> |22 |                                 | Leave PCC handler for Type4 chan|
> |23 |                                 | Enter PCC handler for Type3 chan|
> |24 |                                 | Leave PCC handler for Type3 chan|
> |25 |                                 | Leave pcc_mbox_irq() handler    |
> -------------------------------------------------------------------------
>
> The key issue occurs when OSPM tries to acknowledge platform ack
> interrupt for a notification which is ready to be read and processed
> but the interrupt itself is not yet triggered by the platform.
>
> This ineffective acknowledgment leads to an issue later in time where
> the interrupt remains pending as we exit the interrupt handler without
> clearing the platform ack interrupt as there is no pending response or
> notification. The interrupt acknowledgment order is incorrect.
>
> To resolve this issue, the platform acknowledgment interrupt should
> always be cleared before processing the interrupt for any notifications
> or response.
>
> Reported-by: Robbie King <robbiek@xsightlabs.com>
> Reviewed-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index f2e4087281c70eeb5b9b33371596613a371dff4f..4c582fa2b8bf4c9a9368dba8220f567555dba963 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -313,6 +313,10 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	int ret;
>   
>   	pchan = chan->con_priv;
> +
> +	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
> +		return IRQ_NONE;
> +
>   	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
>   	    !pchan->chan_in_use)
>   		return IRQ_NONE;
> @@ -330,9 +334,6 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   		return IRQ_NONE;
>   	}
>   
> -	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
> -		return IRQ_NONE;
> -
>   	/*
>   	 * Clear this flag immediately after updating interrupt ack register
>   	 * to avoid possible race in updatation of the flag from
>

tested-by: admiyo@os.amperecomputing.com

Re: [PATCH v2 02/13] mailbox: pcc: Always clear the platform ack interrupt first

[Robbie King]

On 3/5/2025 11:38 AM, Sudeep Holla wrote:
> The PCC mailbox interrupt handler (pcc_mbox_irq()) currently checks
> for command completion flags and any error status before clearing the
> interrupt.
> 
> The below sequence highlights an issue in the handling of PCC mailbox
> interrupts, specifically when dealing with doorbell notifications and
> acknowledgment between the OSPM and the platform where type3 and type4
> channels are sharing the interrupt.
> 
> -------------------------------------------------------------------------
> | T |       Platform Firmware         |    OSPM/Linux PCC driver        |
> |---|---------------------------------|---------------------------------|
> | 1 |                                 | Build message in shmem          |
> | 2 |                                 | Ring Type3 chan doorbell        |
> | 3 | Receives the doorbell interrupt |                                 |
> | 4 | Process the message from OSPM   |                                 |
> | 5 | Build response for the message  |                                 |
> | 6 | Ring Platform ACK interrupt on  |                                 |
> |   |  Type3 chan to OSPM             | Received the interrupt          |
> | 7 | Build Notification in Type4 Chan|                                 |
> | 8 |                                 | Start processing interrupt in   |
> |   |                                 |  pcc_mbox_irq() handler         |
> | 9 |                                 | Enter PCC handler for Type4 chan|
> |10 |                                 | Check command complete cleared  |
> |11 |                                 | Read the notification           |
> |12 |                                 | Clear Platform ACK interrupt    |
> |   | No effect from the previous step yet as the Platform ACK          |
> |   |  interrupt has not yet been triggered for this channel            |
> |13 | Ring Platform ACK interrupt on  |                                 |
> |   | Type4 chan to OSPM              |                                 |
> |14 |                                 | Enter PCC handler for Type3 chan|
> |15 |                                 | Command complete is set.        |
> |16 |                                 | Read the response.              |
> |17 |                                 | Clear Platform ACK interrupt    |
> |18 |                                 | Leave PCC handler for Type3     |
> |19 |                                 | Leave pcc_mbox_irq() handler    |
> |20 |                                 | Re-enter pcc_mbox_irq() handler |
> |21 |                                 | Enter PCC handler for Type4 chan|
> |22 |                                 | Leave PCC handler for Type4 chan|
> |23 |                                 | Enter PCC handler for Type3 chan|
> |24 |                                 | Leave PCC handler for Type3 chan|
> |25 |                                 | Leave pcc_mbox_irq() handler    |
> -------------------------------------------------------------------------
> 
> The key issue occurs when OSPM tries to acknowledge platform ack
> interrupt for a notification which is ready to be read and processed
> but the interrupt itself is not yet triggered by the platform.
> 
> This ineffective acknowledgment leads to an issue later in time where
> the interrupt remains pending as we exit the interrupt handler without
> clearing the platform ack interrupt as there is no pending response or
> notification. The interrupt acknowledgment order is incorrect.
> 
> To resolve this issue, the platform acknowledgment interrupt should
> always be cleared before processing the interrupt for any notifications
> or response.
> 
> Reported-by: Robbie King <robbiek@xsightlabs.com>
> Reviewed-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Tested-by: Robbie King <robbiek@xsightlabs.com>

Re: [PATCH v2 02/13] mailbox: pcc: Always clear the platform ack interrupt first

[Sudeep Holla]

On Thu, Mar 13, 2025 at 11:08:04AM -0400, Robbie King wrote:
> On 3/5/2025 11:38 AM, Sudeep Holla wrote:
> > The PCC mailbox interrupt handler (pcc_mbox_irq()) currently checks
> > for command completion flags and any error status before clearing the
> > interrupt.
> > 
> > The below sequence highlights an issue in the handling of PCC mailbox
> > interrupts, specifically when dealing with doorbell notifications and
> > acknowledgment between the OSPM and the platform where type3 and type4
> > channels are sharing the interrupt.
> > 
> > -------------------------------------------------------------------------
> > | T |       Platform Firmware         |    OSPM/Linux PCC driver        |
> > |---|---------------------------------|---------------------------------|
> > | 1 |                                 | Build message in shmem          |
> > | 2 |                                 | Ring Type3 chan doorbell        |
> > | 3 | Receives the doorbell interrupt |                                 |
> > | 4 | Process the message from OSPM   |                                 |
> > | 5 | Build response for the message  |                                 |
> > | 6 | Ring Platform ACK interrupt on  |                                 |
> > |   |  Type3 chan to OSPM             | Received the interrupt          |
> > | 7 | Build Notification in Type4 Chan|                                 |
> > | 8 |                                 | Start processing interrupt in   |
> > |   |                                 |  pcc_mbox_irq() handler         |
> > | 9 |                                 | Enter PCC handler for Type4 chan|
> > |10 |                                 | Check command complete cleared  |
> > |11 |                                 | Read the notification           |
> > |12 |                                 | Clear Platform ACK interrupt    |
> > |   | No effect from the previous step yet as the Platform ACK          |
> > |   |  interrupt has not yet been triggered for this channel            |
> > |13 | Ring Platform ACK interrupt on  |                                 |
> > |   | Type4 chan to OSPM              |                                 |
> > |14 |                                 | Enter PCC handler for Type3 chan|
> > |15 |                                 | Command complete is set.        |
> > |16 |                                 | Read the response.              |
> > |17 |                                 | Clear Platform ACK interrupt    |
> > |18 |                                 | Leave PCC handler for Type3     |
> > |19 |                                 | Leave pcc_mbox_irq() handler    |
> > |20 |                                 | Re-enter pcc_mbox_irq() handler |
> > |21 |                                 | Enter PCC handler for Type4 chan|
> > |22 |                                 | Leave PCC handler for Type4 chan|
> > |23 |                                 | Enter PCC handler for Type3 chan|
> > |24 |                                 | Leave PCC handler for Type3 chan|
> > |25 |                                 | Leave pcc_mbox_irq() handler    |
> > -------------------------------------------------------------------------
> > 
> > The key issue occurs when OSPM tries to acknowledge platform ack
> > interrupt for a notification which is ready to be read and processed
> > but the interrupt itself is not yet triggered by the platform.
> > 
> > This ineffective acknowledgment leads to an issue later in time where
> > the interrupt remains pending as we exit the interrupt handler without
> > clearing the platform ack interrupt as there is no pending response or
> > notification. The interrupt acknowledgment order is incorrect.
> > 
> > To resolve this issue, the platform acknowledgment interrupt should
> > always be cleared before processing the interrupt for any notifications
> > or response.
> > 
> > Reported-by: Robbie King <robbiek@xsightlabs.com>
> > Reviewed-by: Huisong Li <lihuisong@huawei.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Tested-by: Robbie King <robbiek@xsightlabs.com>
> 

Thanks Robbie for reporting the issue and testing the fix. Much appreciated!

-- 
Regards,
Sudeep

[PATCH v2 03/13] mailbox: pcc: Drop unnecessary endianness conversion of pcc_hdr.flags

[Sudeep Holla]

The Sparse static checker flags a type mismatch warning related to
endianness conversion:

  |  warning: incorrect type in argument 1 (different base types)
  |     expected restricted __le32 const [usertype] *p
  |     got unsigned int *

This is because an explicit endianness conversion (le32_to_cpu()) was
applied unnecessarily to a pcc_hdr.flags field that is already in
little-endian format.

The PCC driver is only enabled on little-endian kernels due to its
dependency on ACPI and EFI, making the explicit conversion unnecessary.

The redundant conversion occurs in pcc_chan_check_and_ack() for the
pcc_hdr.flags field. Drop this unnecessary endianness conversion of
pcc_hdr.flags.

Also drop the redundant PCC_ACK_FLAG_MASK definition and use the
more appropriate and already defined PCC_CMD_COMPLETION_NOTIFY.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 2 +-
 include/acpi/pcc.h    | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 4c582fa2b8bf4c9a9368dba8220f567555dba963..42dd405482e407cf90e66917a46fb8e350e0eeaf 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -292,7 +292,7 @@ static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
 	 *
 	 * The PCC master subspace channel clears chan_in_use to free channel.
 	 */
-	if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
+	if (pcc_hdr.flags & PCC_CMD_COMPLETION_NOTIFY)
 		pcc_send_data(chan, NULL);
 	else
 		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 699c1a37b8e7846362bae35477eb5736be15d79e..d1e506f041c5a80857d4a025fa3c1803746ba4b9 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -32,7 +32,6 @@ struct pcc_mbox_chan {
 #define PCC_CMD_COMPLETION_NOTIFY	BIT(0)
 
 #define MAX_PCC_SUBSPACES	256
-#define PCC_ACK_FLAG_MASK	0x1
 
 #ifdef CONFIG_PCC
 extern struct pcc_mbox_chan *

-- 
2.34.1

Re: [PATCH v2 03/13] mailbox: pcc: Drop unnecessary endianness conversion of pcc_hdr.flags

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> The Sparse static checker flags a type mismatch warning related to
> endianness conversion:
>
>    |  warning: incorrect type in argument 1 (different base types)
>    |     expected restricted __le32 const [usertype] *p
>    |     got unsigned int *
>
> This is because an explicit endianness conversion (le32_to_cpu()) was
> applied unnecessarily to a pcc_hdr.flags field that is already in
> little-endian format.
>
> The PCC driver is only enabled on little-endian kernels due to its
> dependency on ACPI and EFI, making the explicit conversion unnecessary.
>
> The redundant conversion occurs in pcc_chan_check_and_ack() for the
> pcc_hdr.flags field. Drop this unnecessary endianness conversion of
> pcc_hdr.flags.
>
> Also drop the redundant PCC_ACK_FLAG_MASK definition and use the
> more appropriate and already defined PCC_CMD_COMPLETION_NOTIFY.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 2 +-
>   include/acpi/pcc.h    | 1 -
>   2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 4c582fa2b8bf4c9a9368dba8220f567555dba963..42dd405482e407cf90e66917a46fb8e350e0eeaf 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -292,7 +292,7 @@ static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
>   	 *
>   	 * The PCC master subspace channel clears chan_in_use to free channel.
>   	 */
> -	if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
> +	if (pcc_hdr.flags & PCC_CMD_COMPLETION_NOTIFY)
>   		pcc_send_data(chan, NULL);
>   	else
>   		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 699c1a37b8e7846362bae35477eb5736be15d79e..d1e506f041c5a80857d4a025fa3c1803746ba4b9 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -32,7 +32,6 @@ struct pcc_mbox_chan {
>   #define PCC_CMD_COMPLETION_NOTIFY	BIT(0)
>   
>   #define MAX_PCC_SUBSPACES	256
> -#define PCC_ACK_FLAG_MASK	0x1
>   
>   #ifdef CONFIG_PCC
>   extern struct pcc_mbox_chan *
Acked-by: Huisong Li <lihuisong@huawei.com>
>

Re: [PATCH v2 03/13] mailbox: pcc: Drop unnecessary endianness conversion of pcc_hdr.flags

[Adam Young]

On 3/5/25 11:38, Sudeep Holla wrote:
> The Sparse static checker flags a type mismatch warning related to
> endianness conversion:
>
>    |  warning: incorrect type in argument 1 (different base types)
>    |     expected restricted __le32 const [usertype] *p
>    |     got unsigned int *
>
> This is because an explicit endianness conversion (le32_to_cpu()) was
> applied unnecessarily to a pcc_hdr.flags field that is already in
> little-endian format.
>
> The PCC driver is only enabled on little-endian kernels due to its
> dependency on ACPI and EFI, making the explicit conversion unnecessary.
>
> The redundant conversion occurs in pcc_chan_check_and_ack() for the
> pcc_hdr.flags field. Drop this unnecessary endianness conversion of
> pcc_hdr.flags.
>
> Also drop the redundant PCC_ACK_FLAG_MASK definition and use the
> more appropriate and already defined PCC_CMD_COMPLETION_NOTIFY.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 2 +-
>   include/acpi/pcc.h    | 1 -
>   2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 4c582fa2b8bf4c9a9368dba8220f567555dba963..42dd405482e407cf90e66917a46fb8e350e0eeaf 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -292,7 +292,7 @@ static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
>   	 *
>   	 * The PCC master subspace channel clears chan_in_use to free channel.
>   	 */
> -	if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
> +	if (pcc_hdr.flags & PCC_CMD_COMPLETION_NOTIFY)
>   		pcc_send_data(chan, NULL);
>   	else
>   		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 699c1a37b8e7846362bae35477eb5736be15d79e..d1e506f041c5a80857d4a025fa3c1803746ba4b9 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -32,7 +32,6 @@ struct pcc_mbox_chan {
>   #define PCC_CMD_COMPLETION_NOTIFY	BIT(0)
>   
>   #define MAX_PCC_SUBSPACES	256
> -#define PCC_ACK_FLAG_MASK	0x1
>   
>   #ifdef CONFIG_PCC
>   extern struct pcc_mbox_chan *
>
Make sense.  The endianess was due to this coming from a network driver 
and should not be here.

tested-by: admiyo@os.amperecomputing.com

[PATCH v2 04/13] mailbox: pcc: Return early if no GAS register from pcc_mbox_cmd_complete_check

[Sudeep Holla]

pcc_mbox_cmd_complete_check() accesses pchan->cmd_complete.gas to check
command completion status. Even if GAS is NULL, pcc_chan_reg_read() gets
called which returns success doing nothing and then we return.

Add an early return if pchan->cmd_complete.gas == NULL before performing
any operations.

Acked-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 42dd405482e407cf90e66917a46fb8e350e0eeaf..5a9ae67f5c50a3e43d30aa368c31c80b81db01f7 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -245,13 +245,13 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
 	u64 val;
 	int ret;
 
+	if (!pchan->cmd_complete.gas)
+		return true;
+
 	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
 	if (ret)
 		return false;
 
-	if (!pchan->cmd_complete.gas)
-		return true;
-
 	/*
 	 * Judge if the channel respond the interrupt based on the value of
 	 * command complete.

-- 
2.34.1

Re: [PATCH v2 04/13] mailbox: pcc: Return early if no GAS register from pcc_mbox_cmd_complete_check

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> pcc_mbox_cmd_complete_check() accesses pchan->cmd_complete.gas to check
> command completion status. Even if GAS is NULL, pcc_chan_reg_read() gets
> called which returns success doing nothing and then we return.
>
> Add an early return if pchan->cmd_complete.gas == NULL before performing
> any operations.
>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
Tested-by: Huisong Li <lihuisong@huawei.com>
>

Re: [PATCH v2 04/13] mailbox: pcc: Return early if no GAS register from pcc_mbox_cmd_complete_check

[Adam Young]

On 3/5/25 11:38, Sudeep Holla wrote:
> pcc_mbox_cmd_complete_check() accesses pchan->cmd_complete.gas to check
> command completion status. Even if GAS is NULL, pcc_chan_reg_read() gets
> called which returns success doing nothing and then we return.
>
> Add an early return if pchan->cmd_complete.gas == NULL before performing
> any operations.
>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 42dd405482e407cf90e66917a46fb8e350e0eeaf..5a9ae67f5c50a3e43d30aa368c31c80b81db01f7 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -245,13 +245,13 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>   	u64 val;
>   	int ret;
>   
> +	if (!pchan->cmd_complete.gas)
> +		return true;
> +
>   	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>   	if (ret)
>   		return false;
>   
> -	if (!pchan->cmd_complete.gas)
> -		return true;
> -
>   	/*
>   	 * Judge if the channel respond the interrupt based on the value of
>   	 * command complete.
>
tested-by: admiyo@os.amperecomputing.com

[PATCH v2 05/13] mailbox: pcc: Use acpi_os_ioremap() instead of ioremap()

[Sudeep Holla]

The Platform Communication Channel (PCC) mailbox driver currently uses
ioremap() to map channel shared memory regions. However it is preferred
to use acpi_os_ioremap(), which is mapping function specific to EFI/ACPI
defined memory regions. It ensures that the correct memory attributes
are applied when mapping ACPI-provided regions.

While at it, also add checks for handling any errors with the mapping.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 5a9ae67f5c50a3e43d30aa368c31c80b81db01f7..b1b8223b5da7002fc522523dbc82f6124215439a 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -418,8 +418,12 @@ int pcc_mbox_ioremap(struct mbox_chan *chan)
 		return -1;
 	pchan_info = chan->con_priv;
 	pcc_mbox_chan = &pchan_info->chan;
-	pcc_mbox_chan->shmem = ioremap(pcc_mbox_chan->shmem_base_addr,
-				       pcc_mbox_chan->shmem_size);
+
+	pcc_mbox_chan->shmem = acpi_os_ioremap(pcc_mbox_chan->shmem_base_addr,
+					       pcc_mbox_chan->shmem_size);
+	if (!pcc_mbox_chan->shmem)
+		return -ENXIO;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_ioremap);

-- 
2.34.1

Re: [PATCH v2 05/13] mailbox: pcc: Use acpi_os_ioremap() instead of ioremap()

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> The Platform Communication Channel (PCC) mailbox driver currently uses
> ioremap() to map channel shared memory regions. However it is preferred
> to use acpi_os_ioremap(), which is mapping function specific to EFI/ACPI
> defined memory regions. It ensures that the correct memory attributes
> are applied when mapping ACPI-provided regions.
>
> While at it, also add checks for handling any errors with the mapping.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 5a9ae67f5c50a3e43d30aa368c31c80b81db01f7..b1b8223b5da7002fc522523dbc82f6124215439a 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -418,8 +418,12 @@ int pcc_mbox_ioremap(struct mbox_chan *chan)
>   		return -1;
>   	pchan_info = chan->con_priv;
>   	pcc_mbox_chan = &pchan_info->chan;
> -	pcc_mbox_chan->shmem = ioremap(pcc_mbox_chan->shmem_base_addr,
> -				       pcc_mbox_chan->shmem_size);
> +
> +	pcc_mbox_chan->shmem = acpi_os_ioremap(pcc_mbox_chan->shmem_base_addr,
> +					       pcc_mbox_chan->shmem_size);
> +	if (!pcc_mbox_chan->shmem)
> +		return -ENXIO;
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_ioremap);
LGTM,
Acked-by: Huisong Li <lihuisong@huawei.com>
Tested-by: Huisong Li <lihuisong@huawei.com>

Re: [PATCH v2 05/13] mailbox: pcc: Use acpi_os_ioremap() instead of ioremap()

[Adam Young]

On 3/5/25 11:38, Sudeep Holla wrote:
> The Platform Communication Channel (PCC) mailbox driver currently uses
> ioremap() to map channel shared memory regions. However it is preferred
> to use acpi_os_ioremap(), which is mapping function specific to EFI/ACPI
> defined memory regions. It ensures that the correct memory attributes
> are applied when mapping ACPI-provided regions.
>
> While at it, also add checks for handling any errors with the mapping.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 5a9ae67f5c50a3e43d30aa368c31c80b81db01f7..b1b8223b5da7002fc522523dbc82f6124215439a 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -418,8 +418,12 @@ int pcc_mbox_ioremap(struct mbox_chan *chan)
>   		return -1;
>   	pchan_info = chan->con_priv;
>   	pcc_mbox_chan = &pchan_info->chan;
> -	pcc_mbox_chan->shmem = ioremap(pcc_mbox_chan->shmem_base_addr,
> -				       pcc_mbox_chan->shmem_size);
> +
> +	pcc_mbox_chan->shmem = acpi_os_ioremap(pcc_mbox_chan->shmem_base_addr,
> +					       pcc_mbox_chan->shmem_size);
> +	if (!pcc_mbox_chan->shmem)
> +		return -ENXIO;
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_ioremap);
>

tested-by: admiyo@os.amperecomputing.com

[PATCH v2 06/13] mailbox: pcc: Refactor error handling in irq handler into separate function

[Sudeep Holla]

The existing error handling logic in pcc_mbox_irq() is intermixed with the
main flow of the function. The command complete check and the complete
complete update/acknowledgment are nicely factored into separate functions.

Moves error detection and clearing logic into a separate function called:
pcc_mbox_error_check_and_clear() by extracting error-handling logic from
pcc_mbox_irq().

This ensures error checking and clearing are handled separately and it
improves maintainability by keeping the IRQ handler focused on processing
events.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index b1b8223b5da7002fc522523dbc82f6124215439a..41bd14851216e8c4f03052c81aaf938a5e5c5343 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -269,6 +269,25 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
 	return !!val;
 }
 
+static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
+{
+	u64 val;
+	int ret;
+
+	ret = pcc_chan_reg_read(&pchan->error, &val);
+	if (ret)
+		return ret;
+
+	val &= pchan->error.status_mask;
+	if (val) {
+		val &= ~pchan->error.status_mask;
+		pcc_chan_reg_write(&pchan->error, val);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
 {
 	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
@@ -309,8 +328,6 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 {
 	struct pcc_chan_info *pchan;
 	struct mbox_chan *chan = p;
-	u64 val;
-	int ret;
 
 	pchan = chan->con_priv;
 
@@ -324,15 +341,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	if (!pcc_mbox_cmd_complete_check(pchan))
 		return IRQ_NONE;
 
-	ret = pcc_chan_reg_read(&pchan->error, &val);
-	if (ret)
+	if (pcc_mbox_error_check_and_clear(pchan))
 		return IRQ_NONE;
-	val &= pchan->error.status_mask;
-	if (val) {
-		val &= ~pchan->error.status_mask;
-		pcc_chan_reg_write(&pchan->error, val);
-		return IRQ_NONE;
-	}
 
 	/*
 	 * Clear this flag immediately after updating interrupt ack register

-- 
2.34.1

Re: [PATCH v2 06/13] mailbox: pcc: Refactor error handling in irq handler into separate function

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> The existing error handling logic in pcc_mbox_irq() is intermixed with the
> main flow of the function. The command complete check and the complete
> complete update/acknowledgment are nicely factored into separate functions.
>
> Moves error detection and clearing logic into a separate function called:
> pcc_mbox_error_check_and_clear() by extracting error-handling logic from
> pcc_mbox_irq().
>
> This ensures error checking and clearing are handled separately and it
> improves maintainability by keeping the IRQ handler focused on processing
> events.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Acked-by: Huisong Li <lihuisong@huawei.com>
Tested-by: Huisong Li <lihuisong@huawei.com>
> ---
>   drivers/mailbox/pcc.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index b1b8223b5da7002fc522523dbc82f6124215439a..41bd14851216e8c4f03052c81aaf938a5e5c5343 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -269,6 +269,25 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>   	return !!val;
>   }
>   
> +static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
> +{
> +	u64 val;
> +	int ret;
> +
> +	ret = pcc_chan_reg_read(&pchan->error, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= pchan->error.status_mask;
> +	if (val) {
> +		val &= ~pchan->error.status_mask;
> +		pcc_chan_reg_write(&pchan->error, val);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>   static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
>   {
>   	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
> @@ -309,8 +328,6 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   {
>   	struct pcc_chan_info *pchan;
>   	struct mbox_chan *chan = p;
> -	u64 val;
> -	int ret;
>   
>   	pchan = chan->con_priv;
>   
> @@ -324,15 +341,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	if (!pcc_mbox_cmd_complete_check(pchan))
>   		return IRQ_NONE;
>   
> -	ret = pcc_chan_reg_read(&pchan->error, &val);
> -	if (ret)
> +	if (pcc_mbox_error_check_and_clear(pchan))
>   		return IRQ_NONE;
> -	val &= pchan->error.status_mask;
> -	if (val) {
> -		val &= ~pchan->error.status_mask;
> -		pcc_chan_reg_write(&pchan->error, val);
> -		return IRQ_NONE;
> -	}
>   
>   	/*
>   	 * Clear this flag immediately after updating interrupt ack register
>

Re: [PATCH v2 06/13] mailbox: pcc: Refactor error handling in irq handler into separate function

[Adam Young]

On 3/5/25 11:38, Sudeep Holla wrote:
> The existing error handling logic in pcc_mbox_irq() is intermixed with the
> main flow of the function. The command complete check and the complete
> complete update/acknowledgment are nicely factored into separate functions.
>
> Moves error detection and clearing logic into a separate function called:
> pcc_mbox_error_check_and_clear() by extracting error-handling logic from
> pcc_mbox_irq().
>
> This ensures error checking and clearing are handled separately and it
> improves maintainability by keeping the IRQ handler focused on processing
> events.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index b1b8223b5da7002fc522523dbc82f6124215439a..41bd14851216e8c4f03052c81aaf938a5e5c5343 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -269,6 +269,25 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>   	return !!val;
>   }
>   
> +static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
> +{
> +	u64 val;
> +	int ret;
> +
> +	ret = pcc_chan_reg_read(&pchan->error, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= pchan->error.status_mask;
> +	if (val) {
> +		val &= ~pchan->error.status_mask;
> +		pcc_chan_reg_write(&pchan->error, val);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>   static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
>   {
>   	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
> @@ -309,8 +328,6 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   {
>   	struct pcc_chan_info *pchan;
>   	struct mbox_chan *chan = p;
> -	u64 val;
> -	int ret;
>   
>   	pchan = chan->con_priv;
>   
> @@ -324,15 +341,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	if (!pcc_mbox_cmd_complete_check(pchan))
>   		return IRQ_NONE;
>   
> -	ret = pcc_chan_reg_read(&pchan->error, &val);
> -	if (ret)
> +	if (pcc_mbox_error_check_and_clear(pchan))
>   		return IRQ_NONE;
> -	val &= pchan->error.status_mask;
> -	if (val) {
> -		val &= ~pchan->error.status_mask;
> -		pcc_chan_reg_write(&pchan->error, val);
> -		return IRQ_NONE;
> -	}
>   
>   	/*
>   	 * Clear this flag immediately after updating interrupt ack register
>

tested-by: Adam Young <admiyo@os.amperecomputing.com>

[PATCH v2 07/13] mailbox: pcc: Always map the shared memory communication address

[Sudeep Holla]

Currently the shared memory communication address was mapped by the
mailbox client drivers leading to all sorts of inconsistencies.

It also has resulted in the inconsistent attributes used while mapping
the shared memory regions.

In order to remove/eliminate any issues, let us ensures the shared
memory address is always mapped and unmapped when the PCC channels are
requested and release.

We need to map them as the ACPI PCCT associates these shared memory
with each channel subspace and may need use the status or the flags in
the headers of those shared memory communication address regions to
manage the transport/channel.

Since there are no users of pcc_chan_ioremap() and also it is mapped
by default, we can stop exporting it and merge the functionality into
pcc_mbox_request_channel().

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 29 +++++++++--------------------
 include/acpi/pcc.h    |  5 -----
 2 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 41bd14851216e8c4f03052c81aaf938a5e5c5343..b3d133170aac7f8acfd1999564c69b7fe4f6d582 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,6 +372,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 struct pcc_mbox_chan *
 pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 {
+	struct pcc_mbox_chan *pcc_mchan;
 	struct pcc_chan_info *pchan;
 	struct mbox_chan *chan;
 	int rc;
@@ -390,7 +391,14 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	if (rc)
 		return ERR_PTR(rc);
 
-	return &pchan->chan;
+	pcc_mchan = &pchan->chan;
+	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
+					   pcc_mchan->shmem_size);
+	if (pcc_mchan->shmem)
+		return pcc_mchan;
+
+	mbox_free_channel(chan);
+	return ERR_PTR(-ENXIO);
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
 
@@ -419,25 +427,6 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
 
-int pcc_mbox_ioremap(struct mbox_chan *chan)
-{
-	struct pcc_chan_info *pchan_info;
-	struct pcc_mbox_chan *pcc_mbox_chan;
-
-	if (!chan || !chan->cl)
-		return -1;
-	pchan_info = chan->con_priv;
-	pcc_mbox_chan = &pchan_info->chan;
-
-	pcc_mbox_chan->shmem = acpi_os_ioremap(pcc_mbox_chan->shmem_base_addr,
-					       pcc_mbox_chan->shmem_size);
-	if (!pcc_mbox_chan->shmem)
-		return -ENXIO;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pcc_mbox_ioremap);
-
 /**
  * pcc_send_data - Called from Mailbox Controller code. Used
  *		here only to ring the channel doorbell. The PCC client
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index d1e506f041c5a80857d4a025fa3c1803746ba4b9..840bfc95bae3329605da5f66cf37b7d2ca183f48 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -37,7 +37,6 @@ struct pcc_mbox_chan {
 extern struct pcc_mbox_chan *
 pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);
 extern void pcc_mbox_free_channel(struct pcc_mbox_chan *chan);
-extern int pcc_mbox_ioremap(struct mbox_chan *chan);
 #else
 static inline struct pcc_mbox_chan *
 pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
@@ -45,10 +44,6 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	return ERR_PTR(-ENODEV);
 }
 static inline void pcc_mbox_free_channel(struct pcc_mbox_chan *chan) { }
-static inline int pcc_mbox_ioremap(struct mbox_chan *chan)
-{
-	return 0;
-};
 #endif
 
 #endif /* _PCC_H */

-- 
2.34.1

Re: [PATCH v2 07/13] mailbox: pcc: Always map the shared memory communication address

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> Currently the shared memory communication address was mapped by the
> mailbox client drivers leading to all sorts of inconsistencies.
>
> It also has resulted in the inconsistent attributes used while mapping
> the shared memory regions.
>
> In order to remove/eliminate any issues, let us ensures the shared
> memory address is always mapped and unmapped when the PCC channels are
> requested and release.
>
> We need to map them as the ACPI PCCT associates these shared memory
> with each channel subspace and may need use the status or the flags in
> the headers of those shared memory communication address regions to
> manage the transport/channel.
>
> Since there are no users of pcc_chan_ioremap() and also it is mapped
> by default, we can stop exporting it and merge the functionality into
> pcc_mbox_request_channel().
There are two ioremap for the existing mbox client driver after this patch.
The existing mbox client driver would not use this variable, and no one 
else uses it. So it is safe, right?
Do we need to make a statement that the two iommaps have no impact on 
the existing mbox client drivers?
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
Acked-by: Huisong Li <lihuisong@huawei.com>
Tested-by: Huisong Li <lihuisong@huawei.com>
>

Re: [PATCH v2 07/13] mailbox: pcc: Always map the shared memory communication address

[Sudeep Holla]

On Tue, Mar 11, 2025 at 07:32:34PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/6 0:38, Sudeep Holla 写道:
> > Currently the shared memory communication address was mapped by the
> > mailbox client drivers leading to all sorts of inconsistencies.
> > 
> > It also has resulted in the inconsistent attributes used while mapping
> > the shared memory regions.
> > 
> > In order to remove/eliminate any issues, let us ensures the shared
> > memory address is always mapped and unmapped when the PCC channels are
> > requested and release.
> > 
> > We need to map them as the ACPI PCCT associates these shared memory
> > with each channel subspace and may need use the status or the flags in
> > the headers of those shared memory communication address regions to
> > manage the transport/channel.
> > 
> > Since there are no users of pcc_chan_ioremap() and also it is mapped
> > by default, we can stop exporting it and merge the functionality into
> > pcc_mbox_request_channel().
> There are two ioremap for the existing mbox client driver after this patch.
> The existing mbox client driver would not use this variable, and no one else
> uses it. So it is safe, right?

IIUC yes, it should be fine.

> Do we need to make a statement that the two iommaps have no impact on the
> existing mbox client drivers?

We can add one, but again it will be become obsolete once we change driver
to use this new mapping. That's why I didn't add it. We can merge all the
changes together if that is the concern. I am fine either way.

Thanks a lot for all the review and testing.

-- 
Regards,
Sudeep

Re: [PATCH v2 07/13] mailbox: pcc: Always map the shared memory communication address

[lihuisong (C)]

在 2025/3/11 19:56, Sudeep Holla 写道:
> On Tue, Mar 11, 2025 at 07:32:34PM +0800, lihuisong (C) wrote:
>> 在 2025/3/6 0:38, Sudeep Holla 写道:
>>> Currently the shared memory communication address was mapped by the
>>> mailbox client drivers leading to all sorts of inconsistencies.
>>>
>>> It also has resulted in the inconsistent attributes used while mapping
>>> the shared memory regions.
>>>
>>> In order to remove/eliminate any issues, let us ensures the shared
>>> memory address is always mapped and unmapped when the PCC channels are
>>> requested and release.
>>>
>>> We need to map them as the ACPI PCCT associates these shared memory
>>> with each channel subspace and may need use the status or the flags in
>>> the headers of those shared memory communication address regions to
>>> manage the transport/channel.
>>>
>>> Since there are no users of pcc_chan_ioremap() and also it is mapped
>>> by default, we can stop exporting it and merge the functionality into
>>> pcc_mbox_request_channel().
>> There are two ioremap for the existing mbox client driver after this patch.
>> The existing mbox client driver would not use this variable, and no one else
>> uses it. So it is safe, right?
> IIUC yes, it should be fine.
>
>> Do we need to make a statement that the two iommaps have no impact on the
>> existing mbox client drivers?
> We can add one, but again it will be become obsolete once we change driver
> to use this new mapping. That's why I didn't add it. We can merge all the
> changes together if that is the concern. I am fine either way.
>
I also tested the case with this patch and no modification of driver.
I didn't find any other issue.
IMO, we should make a statement for this anyway.

/Huisong
>

Re: [PATCH v2 07/13] mailbox: pcc: Always map the shared memory communication address

[Adam Young]

On 3/5/25 11:38, Sudeep Holla wrote:
> Currently the shared memory communication address was mapped by the
> mailbox client drivers leading to all sorts of inconsistencies.
>
> It also has resulted in the inconsistent attributes used while mapping
> the shared memory regions.
>
> In order to remove/eliminate any issues, let us ensures the shared
> memory address is always mapped and unmapped when the PCC channels are
> requested and release.
>
> We need to map them as the ACPI PCCT associates these shared memory
> with each channel subspace and may need use the status or the flags in
> the headers of those shared memory communication address regions to
> manage the transport/channel.
>
> Since there are no users of pcc_chan_ioremap() and also it is mapped
> by default, we can stop exporting it and merge the functionality into
> pcc_mbox_request_channel().
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 29 +++++++++--------------------
>   include/acpi/pcc.h    |  5 -----
>   2 files changed, 9 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 41bd14851216e8c4f03052c81aaf938a5e5c5343..b3d133170aac7f8acfd1999564c69b7fe4f6d582 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -372,6 +372,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   struct pcc_mbox_chan *
>   pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   {
> +	struct pcc_mbox_chan *pcc_mchan;
>   	struct pcc_chan_info *pchan;
>   	struct mbox_chan *chan;
>   	int rc;
> @@ -390,7 +391,14 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	if (rc)
>   		return ERR_PTR(rc);
>   
> -	return &pchan->chan;
> +	pcc_mchan = &pchan->chan;
> +	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
> +					   pcc_mchan->shmem_size);
> +	if (pcc_mchan->shmem)
> +		return pcc_mchan;
> +
> +	mbox_free_channel(chan);
> +	return ERR_PTR(-ENXIO);
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
>   
> @@ -419,25 +427,6 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>   
> -int pcc_mbox_ioremap(struct mbox_chan *chan)
> -{
> -	struct pcc_chan_info *pchan_info;
> -	struct pcc_mbox_chan *pcc_mbox_chan;
> -
> -	if (!chan || !chan->cl)
> -		return -1;
> -	pchan_info = chan->con_priv;
> -	pcc_mbox_chan = &pchan_info->chan;
> -
> -	pcc_mbox_chan->shmem = acpi_os_ioremap(pcc_mbox_chan->shmem_base_addr,
> -					       pcc_mbox_chan->shmem_size);
> -	if (!pcc_mbox_chan->shmem)
> -		return -ENXIO;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(pcc_mbox_ioremap);
> -
>   /**
>    * pcc_send_data - Called from Mailbox Controller code. Used
>    *		here only to ring the channel doorbell. The PCC client
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index d1e506f041c5a80857d4a025fa3c1803746ba4b9..840bfc95bae3329605da5f66cf37b7d2ca183f48 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -37,7 +37,6 @@ struct pcc_mbox_chan {
>   extern struct pcc_mbox_chan *
>   pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);
>   extern void pcc_mbox_free_channel(struct pcc_mbox_chan *chan);
> -extern int pcc_mbox_ioremap(struct mbox_chan *chan);
>   #else
>   static inline struct pcc_mbox_chan *
>   pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
> @@ -45,10 +44,6 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	return ERR_PTR(-ENODEV);
>   }
>   static inline void pcc_mbox_free_channel(struct pcc_mbox_chan *chan) { }
> -static inline int pcc_mbox_ioremap(struct mbox_chan *chan)
> -{
> -	return 0;
> -};
>   #endif
>   
>   #endif /* _PCC_H */
>
tested-by: Adam Young <admiyo@os.amperecomputing.com>

[PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and_ack()

[Sudeep Holla]

The existing check_and_ack() function had unnecessary complexity. The
logic could be streamlined to improve code readability and maintainability.

The command update register needs to be updated in order to acknowledge
the platform notification through type 4 channel. So it can be done
unconditionally. Currently it is complicated just to make use of
pcc_send_data() which also executes the same updation.

In order to simplify, let us just ring the doorbell directly from
check_and_ack() instead of calling into pcc_send_data(). While at it,
rename it into pcc_chan_check_and_ack() to maintain consistency in the
driver.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -117,8 +117,6 @@ struct pcc_chan_info {
 static struct pcc_chan_info *chan_info;
 static int pcc_chan_count;
 
-static int pcc_send_data(struct mbox_chan *chan, void *data);
-
 /*
  * PCC can be used with perf critical drivers such as CPPC
  * So it makes sense to locally cache the virtual address and
@@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
 	return 0;
 }
 
-static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
+static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
 {
-	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
+	struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_hdr;
 
 	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
 		return;
-	/* If the memory region has not been mapped, we cannot
-	 * determine if we need to send the message, but we still
-	 * need to set the cmd_update flag before returning.
-	 */
-	if (pchan->chan.shmem == NULL) {
-		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
-		return;
-	}
-	memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
-		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
+
+	pcc_chan_reg_read_modify_write(&pchan->cmd_update);
+
+	pcc_hdr = pchan->chan.shmem;
+
 	/*
-	 * The PCC slave subspace channel needs to set the command complete bit
-	 * after processing message. If the PCC_ACK_FLAG is set, it should also
-	 * ring the doorbell.
-	 *
-	 * The PCC master subspace channel clears chan_in_use to free channel.
+	 * The PCC slave subspace channel needs to set the command
+	 * complete bit after processing message. If the PCC_ACK_FLAG
+	 * is set, it should also ring the doorbell.
 	 */
-	if (pcc_hdr.flags & PCC_CMD_COMPLETION_NOTIFY)
-		pcc_send_data(chan, NULL);
-	else
-		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
+	if (ioread32(&pcc_hdr->flags) & PCC_CMD_COMPLETION_NOTIFY)
+		pcc_chan_reg_read_modify_write(&pchan->db);
 }
 
 /**
@@ -352,7 +341,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	pchan->chan_in_use = false;
 	mbox_chan_received_data(chan, NULL);
 
-	check_and_ack(pchan, chan);
+	pcc_chan_check_and_ack(pchan);
 
 	return IRQ_HANDLED;
 }

-- 
2.34.1

Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and_ack()

[lihuisong (C)]

在 2025/3/6 0:38, Sudeep Holla 写道:
> The existing check_and_ack() function had unnecessary complexity. The
> logic could be streamlined to improve code readability and maintainability.
>
> The command update register needs to be updated in order to acknowledge
> the platform notification through type 4 channel. So it can be done
> unconditionally. Currently it is complicated just to make use of
> pcc_send_data() which also executes the same updation.
>
> In order to simplify, let us just ring the doorbell directly from
> check_and_ack() instead of calling into pcc_send_data(). While at it,
> rename it into pcc_chan_check_and_ack() to maintain consistency in the
> driver.
LGTM except for some trivial,
Acked-by: Huisong Li <lihuisong@huawei.com>
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
>   1 file changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -117,8 +117,6 @@ struct pcc_chan_info {
>   static struct pcc_chan_info *chan_info;
>   static int pcc_chan_count;
>   
> -static int pcc_send_data(struct mbox_chan *chan, void *data);
> -
>   /*
>    * PCC can be used with perf critical drivers such as CPPC
>    * So it makes sense to locally cache the virtual address and
> @@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
>   	return 0;
>   }
>   
> -static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> +static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
How about use pcc_chan_ack?
>   {
> -	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
> +	struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_hdr;
>   
>   	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>   		return;
> -	/* If the memory region has not been mapped, we cannot
> -	 * determine if we need to send the message, but we still
> -	 * need to set the cmd_update flag before returning.
> -	 */
> -	if (pchan->chan.shmem == NULL) {
> -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> -		return;
> -	}
> -	memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
> -		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
> +
> +	pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> +
> +	pcc_hdr = pchan->chan.shmem;

Should use the original code?

memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));

> +
>   	/*
> -	 * The PCC slave subspace channel needs to set the command complete bit
> -	 * after processing message. If the PCC_ACK_FLAG is set, it should also
> -	 * ring the doorbell.
> -	 *
> -	 * The PCC master subspace channel clears chan_in_use to free channel.
> +	 * The PCC slave subspace channel needs to set the command
> +	 * complete bit after processing message. If the PCC_ACK_FLAG
> +	 * is set, it should also ring the doorbell.
>   	 */
> -	if (pcc_hdr.flags & PCC_CMD_COMPLETION_NOTIFY)
> -		pcc_send_data(chan, NULL);
> -	else
> -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> +	if (ioread32(&pcc_hdr->flags) & PCC_CMD_COMPLETION_NOTIFY)
> +		pcc_chan_reg_read_modify_write(&pchan->db);
>   }
>   
>   /**
> @@ -352,7 +341,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	pchan->chan_in_use = false;
>   	mbox_chan_received_data(chan, NULL);
>   
> -	check_and_ack(pchan, chan);
> +	pcc_chan_check_and_ack(pchan);
>   
>   	return IRQ_HANDLED;
>   }
>

Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and_ack()

[Sudeep Holla]

On Tue, Mar 11, 2025 at 07:47:39PM +0800, lihuisong (C) wrote:
>
> 在 2025/3/6 0:38, Sudeep Holla 写道:
> > The existing check_and_ack() function had unnecessary complexity. The
> > logic could be streamlined to improve code readability and maintainability.
> >
> > The command update register needs to be updated in order to acknowledge
> > the platform notification through type 4 channel. So it can be done
> > unconditionally. Currently it is complicated just to make use of
> > pcc_send_data() which also executes the same updation.
> >
> > In order to simplify, let us just ring the doorbell directly from
> > check_and_ack() instead of calling into pcc_send_data(). While at it,
> > rename it into pcc_chan_check_and_ack() to maintain consistency in the
> > driver.
> LGTM except for some trivial,
> Acked-by: Huisong Li <lihuisong@huawei.com>
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >   drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
> >   1 file changed, 13 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
> > --- a/drivers/mailbox/pcc.c
> > +++ b/drivers/mailbox/pcc.c
> > @@ -117,8 +117,6 @@ struct pcc_chan_info {
> >   static struct pcc_chan_info *chan_info;
> >   static int pcc_chan_count;
> > -static int pcc_send_data(struct mbox_chan *chan, void *data);
> > -
> >   /*
> >    * PCC can be used with perf critical drivers such as CPPC
> >    * So it makes sense to locally cache the virtual address and
> > @@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
> >   	return 0;
> >   }
> > -static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> > +static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
> How about use pcc_chan_ack?
> >   {
> > -	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
> > +	struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_hdr;
> >   	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> >   		return;
> > -	/* If the memory region has not been mapped, we cannot
> > -	 * determine if we need to send the message, but we still
> > -	 * need to set the cmd_update flag before returning.
> > -	 */
> > -	if (pchan->chan.shmem == NULL) {
> > -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> > -		return;
> > -	}
> > -	memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
> > -		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
> > +
> > +	pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> > +
> > +	pcc_hdr = pchan->chan.shmem;
>
> Should use the original code?
>
> memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
> 		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>

ioread32(&pcc_hdr->flags) just reads 4 byte flag instead of copying entire
header for no reason. It should be same as memcpy_fromio(.., .., 4)

--
Regards,
Sudeep

Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and_ack()

[lihuisong (C)]

在 2025/3/11 20:08, Sudeep Holla 写道:
> On Tue, Mar 11, 2025 at 07:47:39PM +0800, lihuisong (C) wrote:
>> 在 2025/3/6 0:38, Sudeep Holla 写道:
>>> The existing check_and_ack() function had unnecessary complexity. The
>>> logic could be streamlined to improve code readability and maintainability.
>>>
>>> The command update register needs to be updated in order to acknowledge
>>> the platform notification through type 4 channel. So it can be done
>>> unconditionally. Currently it is complicated just to make use of
>>> pcc_send_data() which also executes the same updation.
>>>
>>> In order to simplify, let us just ring the doorbell directly from
>>> check_and_ack() instead of calling into pcc_send_data(). While at it,
>>> rename it into pcc_chan_check_and_ack() to maintain consistency in the
>>> driver.
>> LGTM except for some trivial,
>> Acked-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>    drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
>>>    1 file changed, 13 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -117,8 +117,6 @@ struct pcc_chan_info {
>>>    static struct pcc_chan_info *chan_info;
>>>    static int pcc_chan_count;
>>> -static int pcc_send_data(struct mbox_chan *chan, void *data);
>>> -
>>>    /*
>>>     * PCC can be used with perf critical drivers such as CPPC
>>>     * So it makes sense to locally cache the virtual address and
>>> @@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
>>>    	return 0;
>>>    }
>>> -static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
>>> +static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
>> How about use pcc_chan_ack?
What do you think of this?
>>>    {
>>> -	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
>>> +	struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_hdr;
>>>    	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>>>    		return;
>>> -	/* If the memory region has not been mapped, we cannot
>>> -	 * determine if we need to send the message, but we still
>>> -	 * need to set the cmd_update flag before returning.
>>> -	 */
>>> -	if (pchan->chan.shmem == NULL) {
>>> -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>> -		return;
>>> -	}
>>> -	memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
>>> -		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>>> +
>>> +	pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>> +
>>> +	pcc_hdr = pchan->chan.shmem;
>> Should use the original code?
>>
>> memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
>> 		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>>
> ioread32(&pcc_hdr->flags) just reads 4 byte flag instead of copying entire
> header for no reason. It should be same as memcpy_fromio(.., .., 4)
>
get it.
>
> .

Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and_ack()

[Sudeep Holla]

On Tue, Mar 11, 2025 at 08:19:32PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/11 20:08, Sudeep Holla 写道:
> > On Tue, Mar 11, 2025 at 07:47:39PM +0800, lihuisong (C) wrote:
> > > 在 2025/3/6 0:38, Sudeep Holla 写道:
> > > > The existing check_and_ack() function had unnecessary complexity. The
> > > > logic could be streamlined to improve code readability and maintainability.
> > > > 
> > > > The command update register needs to be updated in order to acknowledge
> > > > the platform notification through type 4 channel. So it can be done
> > > > unconditionally. Currently it is complicated just to make use of
> > > > pcc_send_data() which also executes the same updation.
> > > > 
> > > > In order to simplify, let us just ring the doorbell directly from
> > > > check_and_ack() instead of calling into pcc_send_data(). While at it,
> > > > rename it into pcc_chan_check_and_ack() to maintain consistency in the
> > > > driver.
> > > LGTM except for some trivial,
> > > Acked-by: Huisong Li <lihuisong@huawei.com>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > > >    drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
> > > >    1 file changed, 13 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > > index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
> > > > --- a/drivers/mailbox/pcc.c
> > > > +++ b/drivers/mailbox/pcc.c
> > > > @@ -117,8 +117,6 @@ struct pcc_chan_info {
> > > >    static struct pcc_chan_info *chan_info;
> > > >    static int pcc_chan_count;
> > > > -static int pcc_send_data(struct mbox_chan *chan, void *data);
> > > > -
> > > >    /*
> > > >     * PCC can be used with perf critical drivers such as CPPC
> > > >     * So it makes sense to locally cache the virtual address and
> > > > @@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
> > > >    	return 0;
> > > >    }
> > > > -static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> > > > +static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
> > > How about use pcc_chan_ack?
> What do you think of this?

Sure I can update, just retained what was there and made it consistent
with other functions.

-- 
Regards,
Sudeep

Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and_ack()

[Adam Young]

On 3/5/25 11:38, Sudeep Holla wrote:
> The existing check_and_ack() function had unnecessary complexity. The
> logic could be streamlined to improve code readability and maintainability.
>
> The command update register needs to be updated in order to acknowledge
> the platform notification through type 4 channel. So it can be done
> unconditionally. Currently it is complicated just to make use of
> pcc_send_data() which also executes the same updation.
>
> In order to simplify, let us just ring the doorbell directly from
> check_and_ack() instead of calling into pcc_send_data(). While at it,
> rename it into pcc_chan_check_and_ack() to maintain consistency in the
> driver.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
>   1 file changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -117,8 +117,6 @@ struct pcc_chan_info {
>   static struct pcc_chan_info *chan_info;
>   static int pcc_chan_count;
>   
> -static int pcc_send_data(struct mbox_chan *chan, void *data);
> -
>   /*
>    * PCC can be used with perf critical drivers such as CPPC
>    * So it makes sense to locally cache the virtual address and
> @@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
>   	return 0;
>   }
>   
> -static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> +static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
>   {
> -	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
> +	struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_hdr;
>   
>   	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>   		return;
> -	/* If the memory region has not been mapped, we cannot
> -	 * determine if we need to send the message, but we still
> -	 * need to set the cmd_update flag before returning.
> -	 */
> -	if (pchan->chan.shmem == NULL) {
> -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> -		return;
> -	}
> -	memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
> -		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
> +
> +	pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> +
> +	pcc_hdr = pchan->chan.shmem;
> +
>   	/*
> -	 * The PCC slave subspace channel needs to set the command complete bit
> -	 * after processing message. If the PCC_ACK_FLAG is set, it should also
> -	 * ring the doorbell.
> -	 *
> -	 * The PCC master subspace channel clears chan_in_use to free channel.
> +	 * The PCC slave subspace channel needs to set the command
> +	 * complete bit after processing message. If the PCC_ACK_FLAG
> +	 * is set, it should also ring the doorbell.
>   	 */
> -	if (pcc_hdr.flags & PCC_CMD_COMPLETION_NOTIFY)
> -		pcc_send_data(chan, NULL);
> -	else
> -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> +	if (ioread32(&pcc_hdr->flags) & PCC_CMD_COMPLETION_NOTIFY)
> +		pcc_chan_reg_read_modify_write(&pchan->db);
>   }
>   
>   /**
> @@ -352,7 +341,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	pchan->chan_in_use = false;
>   	mbox_chan_received_data(chan, NULL);
>   
> -	check_and_ack(pchan, chan);
> +	pcc_chan_check_and_ack(pchan);
>   
>   	return IRQ_HANDLED;
>   }
>

tested-by: Adam Young <admiyo@os.amperecomputing.com>

[PATCH v2 09/13] soc: hisilicon: kunpeng_hccs: Simplify PCC shared memory region handling

[Sudeep Holla]

The PCC driver now handles mapping and unmapping of shared memory
areas as part of pcc_mbox_{request,free}_channel(). Without these before,
this Kunpeng HCCS driver did handling of those mappings like several
other PCC mailbox client drivers.

There were redundant operations, leading to unnecessary code. Maintaining
the consistency across these driver was harder due to scattered handling
of shmem.

Just use the mapped shmem and remove all redundant operations from this
driver.

Cc: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 42 +++++++++++++-----------------------
 drivers/soc/hisilicon/kunpeng_hccs.h |  2 --
 2 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 8aa8dec14911cdcdc2a2d11606bf6159144e9489..02b2e5ce40b313c8c3cf462c5c2f1d0c53f612f6 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -167,10 +167,6 @@ static void hccs_pcc_rx_callback(struct mbox_client *cl, void *mssg)
 
 static void hccs_unregister_pcc_channel(struct hccs_dev *hdev)
 {
-	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
-
-	if (cl_info->pcc_comm_addr)
-		iounmap(cl_info->pcc_comm_addr);
 	pcc_mbox_free_channel(hdev->cl_info.pcc_chan);
 }
 
@@ -179,6 +175,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
 	struct mbox_client *cl = &cl_info->client;
 	struct pcc_mbox_chan *pcc_chan;
+	struct mbox_chan *mbox_chan;
 	struct device *dev = hdev->dev;
 	int rc;
 
@@ -196,7 +193,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 		goto out;
 	}
 	cl_info->pcc_chan = pcc_chan;
-	cl_info->mbox_chan = pcc_chan->mchan;
+	mbox_chan = pcc_chan->mchan;
 
 	/*
 	 * pcc_chan->latency is just a nominal value. In reality the remote
@@ -206,34 +203,24 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	cl_info->deadline_us =
 			HCCS_PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
 	if (!hdev->verspec_data->has_txdone_irq &&
-	    cl_info->mbox_chan->mbox->txdone_irq) {
+	    mbox_chan->mbox->txdone_irq) {
 		dev_err(dev, "PCC IRQ in PCCT is enabled.\n");
 		rc = -EINVAL;
 		goto err_mbx_channel_free;
 	} else if (hdev->verspec_data->has_txdone_irq &&
-		   !cl_info->mbox_chan->mbox->txdone_irq) {
+		   !mbox_chan->mbox->txdone_irq) {
 		dev_err(dev, "PCC IRQ in PCCT isn't supported.\n");
 		rc = -EINVAL;
 		goto err_mbx_channel_free;
 	}
 
-	if (!pcc_chan->shmem_base_addr ||
-	    pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
-		dev_err(dev, "The base address or size (%llu) of PCC communication region is invalid.\n",
-			pcc_chan->shmem_size);
+	if (pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
+		dev_err(dev, "Base size (%llu) of PCC communication region must be %d bytes.\n",
+			pcc_chan->shmem_size, HCCS_PCC_SHARE_MEM_BYTES);
 		rc = -EINVAL;
 		goto err_mbx_channel_free;
 	}
 
-	cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr,
-					 pcc_chan->shmem_size);
-	if (!cl_info->pcc_comm_addr) {
-		dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
-			hdev->chan_id);
-		rc = -ENOMEM;
-		goto err_mbx_channel_free;
-	}
-
 	return 0;
 
 err_mbx_channel_free:
@@ -246,7 +233,7 @@ static int hccs_wait_cmd_complete_by_poll(struct hccs_dev *hdev)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
 	struct acpi_pcct_shared_memory __iomem *comm_base =
-							cl_info->pcc_comm_addr;
+							cl_info->pcc_chan->shmem;
 	u16 status;
 	int ret;
 
@@ -289,7 +276,7 @@ static inline void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev,
 		.status = 0,
 	};
 
-	memcpy_toio(hdev->cl_info.pcc_comm_addr, (void *)&tmp,
+	memcpy_toio(hdev->cl_info.pcc_chan->shmem, (void *)&tmp,
 		    sizeof(struct acpi_pcct_shared_memory));
 
 	/* Copy the message to the PCC comm space */
@@ -309,7 +296,7 @@ static inline void hccs_fill_ext_pcc_shared_mem_region(struct hccs_dev *hdev,
 		.command = cmd,
 	};
 
-	memcpy_toio(hdev->cl_info.pcc_comm_addr, (void *)&tmp,
+	memcpy_toio(hdev->cl_info.pcc_chan->shmem, (void *)&tmp,
 		    sizeof(struct acpi_pcct_ext_pcc_shared_memory));
 
 	/* Copy the message to the PCC comm space */
@@ -321,12 +308,13 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 {
 	const struct hccs_verspecific_data *verspec_data = hdev->verspec_data;
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
+	struct mbox_chan *mbox_chan = cl_info->pcc_chan->mchan;
 	struct hccs_fw_inner_head *fw_inner_head;
 	void __iomem *comm_space;
 	u16 space_size;
 	int ret;
 
-	comm_space = cl_info->pcc_comm_addr + verspec_data->shared_mem_size;
+	comm_space = cl_info->pcc_chan->shmem + verspec_data->shared_mem_size;
 	space_size = HCCS_PCC_SHARE_MEM_BYTES - verspec_data->shared_mem_size;
 	verspec_data->fill_pcc_shared_mem(hdev, cmd, desc,
 					  comm_space, space_size);
@@ -334,7 +322,7 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 		reinit_completion(&cl_info->done);
 
 	/* Ring doorbell */
-	ret = mbox_send_message(cl_info->mbox_chan, &cmd);
+	ret = mbox_send_message(mbox_chan, &cmd);
 	if (ret < 0) {
 		dev_err(hdev->dev, "Send PCC mbox message failed, ret = %d.\n",
 			ret);
@@ -356,9 +344,9 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 
 end:
 	if (verspec_data->has_txdone_irq)
-		mbox_chan_txdone(cl_info->mbox_chan, ret);
+		mbox_chan_txdone(mbox_chan, ret);
 	else
-		mbox_client_txdone(cl_info->mbox_chan, ret);
+		mbox_client_txdone(mbox_chan, ret);
 	return ret;
 }
 
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
index dc267136919b7bf3ecc0deb8cf7291267dd91789..f0a9a5618d9735e959633059192449b10d5bbf16 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.h
+++ b/drivers/soc/hisilicon/kunpeng_hccs.h
@@ -60,10 +60,8 @@ struct hccs_chip_info {
 
 struct hccs_mbox_client_info {
 	struct mbox_client client;
-	struct mbox_chan *mbox_chan;
 	struct pcc_mbox_chan *pcc_chan;
 	u64 deadline_us;
-	void __iomem *pcc_comm_addr;
 	struct completion done;
 };
 

-- 
2.34.1

[PATCH v2 10/13] i2c: xgene-slimpro: Simplify PCC shared memory region handling

[Sudeep Holla]

The PCC driver now handles mapping and unmapping of shared memory
areas as part of pcc_mbox_{request,free}_channel(). Without these before,
this xgene-slimpro I2C driver did handling of those mappings like several
other PCC mailbox client drivers.

There were redundant operations, leading to unnecessary code. Maintaining
the consistency across these driver was harder due to scattered handling
of shmem.

Just use the mapped shmem and remove all redundant operations from this
driver.

Cc: Andi Shyti <andi.shyti@kernel.org>
Cc: linux-i2c@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/i2c/busses/i2c-xgene-slimpro.c | 39 ++++------------------------------
 1 file changed, 4 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
index 663fe5604dd64b80e57f906e1f7430dcf6d5e95b..a0880f4a056d2b8abbac9f58416215a7fc9b130e 100644
--- a/drivers/i2c/busses/i2c-xgene-slimpro.c
+++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
@@ -101,8 +101,6 @@ struct slimpro_i2c_dev {
 	struct completion rd_complete;
 	u8 dma_buffer[I2C_SMBUS_BLOCK_MAX + 1]; /* dma_buffer[0] is used for length */
 	u32 *resp_msg;
-	phys_addr_t comm_base_addr;
-	void *pcc_comm_addr;
 };
 
 #define to_slimpro_i2c_dev(cl)	\
@@ -148,7 +146,8 @@ static void slimpro_i2c_rx_cb(struct mbox_client *cl, void *mssg)
 static void slimpro_i2c_pcc_rx_cb(struct mbox_client *cl, void *msg)
 {
 	struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl);
-	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+							ctx->pcc_chan->shmem;
 
 	/* Check if platform sends interrupt */
 	if (!xgene_word_tst_and_clr(&generic_comm_base->status,
@@ -169,7 +168,8 @@ static void slimpro_i2c_pcc_rx_cb(struct mbox_client *cl, void *msg)
 
 static void slimpro_i2c_pcc_tx_prepare(struct slimpro_i2c_dev *ctx, u32 *msg)
 {
-	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+							ctx->pcc_chan->shmem;
 	u32 *ptr = (void *)(generic_comm_base + 1);
 	u16 status;
 	int i;
@@ -464,15 +464,12 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
 	} else {
 		struct pcc_mbox_chan *pcc_chan;
 		const struct acpi_device_id *acpi_id;
-		int version = XGENE_SLIMPRO_I2C_V1;
 
 		acpi_id = acpi_match_device(pdev->dev.driver->acpi_match_table,
 					    &pdev->dev);
 		if (!acpi_id)
 			return -EINVAL;
 
-		version = (int)acpi_id->driver_data;
-
 		if (device_property_read_u32(&pdev->dev, "pcc-channel",
 					     &ctx->mbox_idx))
 			ctx->mbox_idx = MAILBOX_I2C_INDEX;
@@ -494,34 +491,6 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
 			goto mbox_err;
 		}
 
-		/*
-		 * This is the shared communication region
-		 * for the OS and Platform to communicate over.
-		 */
-		ctx->comm_base_addr = pcc_chan->shmem_base_addr;
-		if (ctx->comm_base_addr) {
-			if (version == XGENE_SLIMPRO_I2C_V2)
-				ctx->pcc_comm_addr = memremap(
-							ctx->comm_base_addr,
-							pcc_chan->shmem_size,
-							MEMREMAP_WT);
-			else
-				ctx->pcc_comm_addr = memremap(
-							ctx->comm_base_addr,
-							pcc_chan->shmem_size,
-							MEMREMAP_WB);
-		} else {
-			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
-			rc = -ENOENT;
-			goto mbox_err;
-		}
-
-		if (!ctx->pcc_comm_addr) {
-			dev_err(&pdev->dev,
-				"Failed to ioremap PCC comm region\n");
-			rc = -ENOMEM;
-			goto mbox_err;
-		}
 	}
 	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
 	if (rc)

-- 
2.34.1

[PATCH v2 11/13] hwmon: (xgene-hwmon) Simplify PCC shared memory region handling

[Sudeep Holla]

The PCC driver now handles mapping and unmapping of shared memory
areas as part of pcc_mbox_{request,free}_channel(). Without these before,
this xgene hwmon driver did handling of those mappings like several
other PCC mailbox client drivers.

There were redundant operations, leading to unnecessary code. Maintaining
the consistency across these driver was harder due to scattered handling
of shmem.

Just use the mapped shmem and remove all redundant operations from this
driver.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/hwmon/xgene-hwmon.c | 40 ++++------------------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index 1e3bd129a922d25ff25142d864503377773304a8..ea350d4de902c4e6fc4de1cd54a8b75edfad1119 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -102,9 +102,6 @@ struct xgene_hwmon_dev {
 
 	struct device		*hwmon_dev;
 	bool			temp_critical_alarm;
-
-	phys_addr_t		comm_base_addr;
-	void			*pcc_comm_addr;
 	u64			usecs_lat;
 };
 
@@ -125,7 +122,8 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
 
 static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 {
-	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+							ctx->pcc_chan->shmem;
 	u32 *ptr = (void *)(generic_comm_base + 1);
 	int rc, i;
 	u16 val;
@@ -523,7 +521,8 @@ static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg)
 static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
 {
 	struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
-	struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+							ctx->pcc_chan->shmem;
 	struct slimpro_resp_msg amsg;
 
 	/*
@@ -649,7 +648,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 	} else {
 		struct pcc_mbox_chan *pcc_chan;
 		const struct acpi_device_id *acpi_id;
-		int version;
 
 		acpi_id = acpi_match_device(pdev->dev.driver->acpi_match_table,
 					    &pdev->dev);
@@ -658,8 +656,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 			goto out_mbox_free;
 		}
 
-		version = (int)acpi_id->driver_data;
-
 		if (device_property_read_u32(&pdev->dev, "pcc-channel",
 					     &ctx->mbox_idx)) {
 			dev_err(&pdev->dev, "no pcc-channel property\n");
@@ -685,34 +681,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 			goto out;
 		}
 
-		/*
-		 * This is the shared communication region
-		 * for the OS and Platform to communicate over.
-		 */
-		ctx->comm_base_addr = pcc_chan->shmem_base_addr;
-		if (ctx->comm_base_addr) {
-			if (version == XGENE_HWMON_V2)
-				ctx->pcc_comm_addr = (void __force *)devm_ioremap(&pdev->dev,
-								  ctx->comm_base_addr,
-								  pcc_chan->shmem_size);
-			else
-				ctx->pcc_comm_addr = devm_memremap(&pdev->dev,
-								   ctx->comm_base_addr,
-								   pcc_chan->shmem_size,
-								   MEMREMAP_WB);
-		} else {
-			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
-			rc = -ENODEV;
-			goto out;
-		}
-
-		if (!ctx->pcc_comm_addr) {
-			dev_err(&pdev->dev,
-				"Failed to ioremap PCC comm region\n");
-			rc = -ENOMEM;
-			goto out;
-		}
-
 		/*
 		 * pcc_chan->latency is just a Nominal value. In reality
 		 * the remote processor could be much slower to reply.

-- 
2.34.1

[PATCH v2 12/13] ACPI: PCC: Simplify PCC shared memory region handling

[Sudeep Holla]

The PCC driver now handles mapping and unmapping of shared memory
areas as part of pcc_mbox_{request,free}_channel(). Without these before,
this ACPI PCC opregion driver did handling of those mappings like several
other PCC mailbox client drivers.

There were redundant operations, leading to unnecessary code. Maintaining
the consistency across these driver was harder due to scattered handling
of shmem.

Just use the mapped shmem and remove all redundant operations from this
driver.

Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/acpi_pcc.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index 07a034a53acac1e8307265bcc5572054d34d971f..97064e943768ad9f1704effa13dddbc0876a9452 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -31,7 +31,6 @@
 
 struct pcc_data {
 	struct pcc_mbox_chan *pcc_chan;
-	void __iomem *pcc_comm_addr;
 	struct completion done;
 	struct mbox_client cl;
 	struct acpi_pcc_info ctx;
@@ -81,14 +80,6 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
 		ret = AE_SUPPORT;
 		goto err_free_channel;
 	}
-	data->pcc_comm_addr = acpi_os_ioremap(pcc_chan->shmem_base_addr,
-					      pcc_chan->shmem_size);
-	if (!data->pcc_comm_addr) {
-		pr_err("Failed to ioremap PCC comm region mem for %d\n",
-		       ctx->subspace_id);
-		ret = AE_NO_MEMORY;
-		goto err_free_channel;
-	}
 
 	*region_context = data;
 	return AE_OK;
@@ -113,7 +104,7 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 	reinit_completion(&data->done);
 
 	/* Write to Shared Memory */
-	memcpy_toio(data->pcc_comm_addr, (void *)value, data->ctx.length);
+	memcpy_toio(data->pcc_chan->shmem, (void *)value, data->ctx.length);
 
 	ret = mbox_send_message(data->pcc_chan->mchan, NULL);
 	if (ret < 0)
@@ -134,7 +125,7 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 
 	mbox_chan_txdone(data->pcc_chan->mchan, ret);
 
-	memcpy_fromio(value, data->pcc_comm_addr, data->ctx.length);
+	memcpy_fromio(value, data->pcc_chan->shmem, data->ctx.length);
 
 	return AE_OK;
 }

-- 
2.34.1

[PATCH v2 13/13] ACPI: CPPC: Simplify PCC shared memory region handling

[Sudeep Holla]

The PCC driver now handles mapping and unmapping of shared memory
areas as part of pcc_mbox_{request,free}_channel(). Without these before,
this ACPI CPPC driver did handling of those mappings like several
other PCC mailbox client drivers.

There were redundant operations, leading to unnecessary code. Maintaining
the consistency across these driver was harder due to scattered handling
of shmem.

Just use the mapped shmem and remove all redundant operations from this
driver.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/cppc_acpi.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index f193e713825ac24203ece5f94d6cf99dd4724ce4..d972157a79b6ade2f3738c90128e8692141b3ee5 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -47,7 +47,6 @@
 
 struct cppc_pcc_data {
 	struct pcc_mbox_chan *pcc_channel;
-	void __iomem *pcc_comm_addr;
 	bool pcc_channel_acquired;
 	unsigned int deadline_us;
 	unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
@@ -95,7 +94,7 @@ static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
 static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 
 /* pcc mapped address + header size + offset within PCC subspace */
-#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
+#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_channel->shmem + \
 						0x8 + (offs))
 
 /* Check if a CPC register is in PCC */
@@ -223,7 +222,7 @@ static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
 	int ret, status;
 	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
-		pcc_ss_data->pcc_comm_addr;
+					pcc_ss_data->pcc_channel->shmem;
 
 	if (!pcc_ss_data->platform_owns_pcc)
 		return 0;
@@ -258,7 +257,7 @@ static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
 	int ret = -EIO, i;
 	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
-		pcc_ss_data->pcc_comm_addr;
+					pcc_ss_data->pcc_channel->shmem;
 	unsigned int time_delta;
 
 	/*
@@ -571,15 +570,6 @@ static int register_pcc_channel(int pcc_ss_idx)
 		pcc_data[pcc_ss_idx]->pcc_mpar = pcc_chan->max_access_rate;
 		pcc_data[pcc_ss_idx]->pcc_nominal = pcc_chan->latency;
 
-		pcc_data[pcc_ss_idx]->pcc_comm_addr =
-			acpi_os_ioremap(pcc_chan->shmem_base_addr,
-					pcc_chan->shmem_size);
-		if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
-			pr_err("Failed to ioremap PCC comm region mem for %d\n",
-			       pcc_ss_idx);
-			return -ENOMEM;
-		}
-
 		/* Set flag so that we don't come here for each CPU. */
 		pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
 	}

-- 
2.34.1

Re: [PATCH v2 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Sudeep Holla]

On Wed, Mar 05, 2025 at 04:38:04PM +0000, Sudeep Holla wrote:
> Adam, Robbie, Huisong,
>
> Please test this in your setup as you are the ones reporting/fixing the
> issues or last modified the code that I am changing here.
>
Huisong,

Thanks a lot for all the testing and review.

Adam, Robbie,

Can you please help me with the testing on your platforms ?

-- 
Regards,
Sudeep

Re: [PATCH v2 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Adam Young]

The XGene patch did not apply on top of Linus's current tree. The other 
patches applied OK.


I only had to make one modification to my patch to remove the call to 
‘pcc_mbox_ioremap’,  as it is performed in the pcc_mbox_request_channel 
call instead. With that change, my driver continues to work. I will 
submit another version here shortly.

I like the direction that this change is pushing, making the mailbox 
layer the owner for other drivers.


On 3/11/25 08:10, Sudeep Holla wrote:
> On Wed, Mar 05, 2025 at 04:38:04PM +0000, Sudeep Holla wrote:
>> Adam, Robbie, Huisong,
>>
>> Please test this in your setup as you are the ones reporting/fixing the
>> issues or last modified the code that I am changing here.
>>
> Huisong,
>
> Thanks a lot for all the testing and review.
>
> Adam, Robbie,
>
> Can you please help me with the testing on your platforms ?
>

Re: [PATCH v2 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Sudeep Holla]

On Wed, Mar 12, 2025 at 02:04:51PM -0400, Adam Young wrote:
> The XGene patch did not apply on top of Linus's current tree. The other
> patches applied OK.
>

Yes Guenter had mentioned it in his review. I have it rebased locally [1]
but yet to push out v3 on the list.

> I only had to make one modification to my patch to remove the call to
> ‘pcc_mbox_ioremap’,  as it is performed in the pcc_mbox_request_channel call
> instead. With that change, my driver continues to work. I will submit
> another version here shortly.
>

Nice, I wasn't aware of the Ampere driver using ioremap. Is it posted on
the list ? Or are you saying you will post it soon.

Thanks for testing. Please provide tested-by for patch 1-8 if you are
happy with it.

> I like the direction that this change is pushing, making the mailbox layer
> the owner for other drivers.
>

Yes it was long due. I had changes in my WIP but was away when you changes
got merged. Otherwise I would have asked you to do some of the changes in
this series. My bad, couldn't review your patches unfortunately.

-- 
Regards,
Sudeep

[1] ihttps://git.kernel.org/sudeep.holla/h/b4/pcc_fixes_updates

Re: [PATCH v2 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Adam Young]

On 3/12/25 16:05, Sudeep Holla wrote:
> On Wed, Mar 12, 2025 at 02:04:51PM -0400, Adam Young wrote:
>> The XGene patch did not apply on top of Linus's current tree. The other
>> patches applied OK.
>>
> Yes Guenter had mentioned it in his review. I have it rebased locally [1]
> but yet to push out v3 on the list.
>
>> I only had to make one modification to my patch to remove the call to
>> ‘pcc_mbox_ioremap’,  as it is performed in the pcc_mbox_request_channel call
>> instead. With that change, my driver continues to work. I will submit
>> another version here shortly.
>>
> Nice, I wasn't aware of the Ampere driver using ioremap. Is it posted on
> the list ? Or are you saying you will post it soon.

It is posted to net-next.

https://lore.kernel.org/lkml/20250224181117.21ad7ab1@kernel.org/T/

I will post an updated version once this series goes in.  I don't expect 
it to merge for this kernel due to the dependency, but the code will be 
better for this change.

>
> Thanks for testing. Please provide tested-by for patch 1-8 if you are
> happy with it.
Happy to do so.
>
>> I like the direction that this change is pushing, making the mailbox layer
>> the owner for other drivers.
>>
> Yes it was long due. I had changes in my WIP but was away when you changes
> got merged. Otherwise I would have asked you to do some of the changes in
> this series. My bad, couldn't review your patches unfortunately.
>

Re: [PATCH v2 00/13] mailbox: pcc: Fixes and cleanup/refactoring

[Sudeep Holla]

On Wed, Mar 12, 2025 at 04:37:35PM -0400, Adam Young wrote:
> 
> On 3/12/25 16:05, Sudeep Holla wrote:
> > On Wed, Mar 12, 2025 at 02:04:51PM -0400, Adam Young wrote:
> > > The XGene patch did not apply on top of Linus's current tree. The other
> > > patches applied OK.
> > > 
> > Yes Guenter had mentioned it in his review. I have it rebased locally [1]
> > but yet to push out v3 on the list.
> > 
> > > I only had to make one modification to my patch to remove the call to
> > > ‘pcc_mbox_ioremap’,  as it is performed in the pcc_mbox_request_channel call
> > > instead. With that change, my driver continues to work. I will submit
> > > another version here shortly.
> > > 
> > Nice, I wasn't aware of the Ampere driver using ioremap. Is it posted on
> > the list ? Or are you saying you will post it soon.
> 
> It is posted to net-next.
> 
> https://lore.kernel.org/lkml/20250224181117.21ad7ab1@kernel.org/T/
> 
> I will post an updated version once this series goes in.  I don't expect it
> to merge for this kernel due to the dependency, but the code will be better
> for this change.
> 
> > 
> > Thanks for testing. Please provide tested-by for patch 1-8 if you are
> > happy with it.
> Happy to do so.

Thanks a lot for your time and testing. Much appreciated!

-- 
Regards,
Sudeep