[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 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

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