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

[PATCH 00/14] 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, we can just target 10-14 for following merge window
after 1-9 is merged.

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>
---
Huisong Li (1):
      mailbox: pcc: Fix the possible race in updation of chan_in_use flag

Sudeep Holla (13):
      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: Move pcc_mbox_ioremap() before pcc_mbox_request_channel()
      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                  | 128 ++++++++++++++++++---------------
 drivers/soc/hisilicon/kunpeng_hccs.c   |  38 ++++------
 drivers/soc/hisilicon/kunpeng_hccs.h   |   2 -
 include/acpi/pcc.h                     |   5 --
 8 files changed, 97 insertions(+), 184 deletions(-)
---
base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
change-id: 20250303-pcc_fixes_updates-55a17fd28e76

Best regards,
-- 
Regards,
Sudeep

[PATCH 01/14] 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

[PATCH 02/14] 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.

        Platform Firmware              OSPM/Linux PCC driver
------------------------------------------------------------------------
                                     build message in shmem
                                     ring type3 channel doorbell
receives the doorbell interrupt
  process the message from OSPM
  build response for the message
ring the platform ack interrupt to OSPM
				--->
build notification in type4 channel
                                     start processing in pcc_mbox_irq()
                                      enter pcc handler for type4 chan
                                         command complete cleared
			        	 read the notification
                                <---     clear platform ack irq
  		* no effect from above as platform ack irq *
		* not yet triggered on this channel *
ring the platform ack irq on type4 channel
				--->
                                      leave pcc handler for type4 chan
                                      enter pcc handler for type3 chan
                                         command complete set
					 read the response
                                <---     clear platform ack irq
                                      leave pcc handler for type3 chan
                                     leave pcc_mbox_irq() handler
                                     start processing in pcc_mbox_irq()
                                      enter pcc handler for type4 chan
                                      leave pcc handler for type4 chan
                                      enter pcc handler for type3 chan
                                      leave pcc handler for type3 chan
                                     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>
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

[PATCH 03/14] 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.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 4c582fa2b8bf4c9a9368dba8220f567555dba963..c87a5b7fa6eaf7bcabe0d55f844961c499376938 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_ACK_FLAG_MASK)
 		pcc_send_data(chan, NULL);
 	else
 		pcc_chan_reg_read_modify_write(&pchan->cmd_update);

-- 
2.34.1

[PATCH 04/14] 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.

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 c87a5b7fa6eaf7bcabe0d55f844961c499376938..98c99f0e24c4a654a8f4835063f5a479a433c9a0 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

[PATCH 05/14] 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 98c99f0e24c4a654a8f4835063f5a479a433c9a0..a0fdafc3ef71d20c73ff58ef065201e6dc911396 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

[PATCH 06/14] 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 a0fdafc3ef71d20c73ff58ef065201e6dc911396..e693675ce1fbd8d01d0640b3053a5c1882bdbce7 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

[PATCH 07/14] mailbox: pcc: Move pcc_mbox_ioremap() before pcc_mbox_request_channel()

[Sudeep Holla]

In order to add support of mapping the generic communication shared
memory region in the PCC mailbox driver when the PCC channel is requested,
we need to move pcc_mbox_ioremap() before pcc_mbox_request_channel().

No functional change.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index e693675ce1fbd8d01d0640b3053a5c1882bdbce7..f230e512c29b79fc03e429145180ff049a250d2d 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -357,6 +357,25 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+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_mbox_request_channel - PCC clients call this function to
  *		request a pointer to their PCC subspace, from which they
@@ -419,25 +438,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

-- 
2.34.1

[PATCH 08/14] 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 headers in those
memory to manage the transport.

Since there are no users of pcc_chan_ioremap() and also it is mapped
by default, we can stop exporting it and make it static function.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index f230e512c29b79fc03e429145180ff049a250d2d..5f2e2b727d99f07c44e87e44c11ba0aefe3a2318 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -357,7 +357,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
-int pcc_mbox_ioremap(struct mbox_chan *chan)
+static int pcc_mbox_ioremap(struct mbox_chan *chan)
 {
 	struct pcc_chan_info *pchan_info;
 	struct pcc_mbox_chan *pcc_mbox_chan;
@@ -374,7 +374,6 @@ int pcc_mbox_ioremap(struct mbox_chan *chan)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(pcc_mbox_ioremap);
 
 /**
  * pcc_mbox_request_channel - PCC clients call this function to
@@ -409,7 +408,12 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	if (rc)
 		return ERR_PTR(rc);
 
-	return &pchan->chan;
+	rc = pcc_mbox_ioremap(chan);
+	if (!rc)
+		return &pchan->chan;
+
+	mbox_free_channel(chan);
+	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
 
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 699c1a37b8e7846362bae35477eb5736be15d79e..0462bb9da6513a241f3b652b8e203299a1d990c7 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -38,7 +38,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)
@@ -46,10 +45,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

[PATCH 09/14] 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 | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 5f2e2b727d99f07c44e87e44c11ba0aefe3a2318..17500d4122af3194011eb47bc91efa4317cd8a32 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,25 @@ 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_ACK_FLAG_MASK)
-		pcc_send_data(chan, NULL);
-	else
-		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
+	if (ioread32(&pcc_hdr->flags) & PCC_ACK_FLAG_MASK)
+		pcc_chan_reg_read_modify_write(&pchan->db);
+
 }
 
 /**
@@ -352,7 +342,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

[PATCH 10/14] 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>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 38 ++++++++++++------------------------
 drivers/soc/hisilicon/kunpeng_hccs.h |  2 --
 2 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 8aa8dec14911cdcdc2a2d11606bf6159144e9489..3b57189cd778f507afaa89bf47f0fa834043c244 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) {
+	if (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);
 		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 11/14] 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 12/14] 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
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 13/14] 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 14/14] 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>
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 14/14] ACPI: CPPC: Simplify PCC shared memory region handling

[Rafael J. Wysocki]

On Mon, Mar 3, 2025 at 11:53 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> 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>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Acked-by: "Rafael J. Wysocki" <rafael@kernel.org>

> ---
>  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 12/14] hwmon: (xgene-hwmon) Simplify PCC shared memory region handling

[Guenter Roeck]

On 3/3/25 02:51, Sudeep Holla wrote:
> 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
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

Note that I'll apply a fix this week which will cause a context conflict.
See below.

> ---
>   drivers/hwmon/xgene-hwmon.c | 40 ++++------------------------------------
...
> @@ -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) {

This needed to be IS_ERR_OR_NULL() since devm_memremap() returns an ERR_PTR.

Thanks,
Guenter

Re: [PATCH 12/14] hwmon: (xgene-hwmon) Simplify PCC shared memory region handling

[Sudeep Holla]

On Mon, Mar 03, 2025 at 05:55:57AM -0800, Guenter Roeck wrote:
> On 3/3/25 02:51, Sudeep Holla wrote:
> > 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
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> Note that I'll apply a fix this week which will cause a context conflict.
> See below.
>

Sure, I was planning to repost these individually after the mailbox changes
are merged. I am not sure if I can get some testing on this xgene platform
as I am changing the mapping attributes which currently is different for
different versions of firmware and both are incorrect in terms of ACPI spec.

In short, no rush. The idea of posting these is to show how duplicate code
can be removed. I will post it independently sometime in the future.

>
> This needed to be IS_ERR_OR_NULL() since devm_memremap() returns an ERR_PTR.
>

True. IIRC, there are few valid iomem related sparse warnings which I plan
to fix and post it together once the PCC changes are merged.

Thanks for the review.

-- 
Regards,
Sudeep

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

[lihuisong (C)]

在 2025/3/3 18:51, 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.
>
>          Platform Firmware              OSPM/Linux PCC driver
> ------------------------------------------------------------------------
>                                       build message in shmem
>                                       ring type3 channel doorbell
> receives the doorbell interrupt
>    process the message from OSPM
>    build response for the message
> ring the platform ack interrupt to OSPM
> 				--->
> build notification in type4 channel
>                                       start processing in pcc_mbox_irq()
>                                        enter pcc handler for type4 chan
>                                           command complete cleared
> 			        	 read the notification
>                                  <---     clear platform ack irq
>    		* no effect from above as platform ack irq *
> 		* not yet triggered on this channel *
> ring the platform ack irq on type4 channel
> 				--->
>                                        leave pcc handler for type4 chan
>                                        enter pcc handler for type3 chan
>                                           command complete set
> 					 read the response
>                                  <---     clear platform ack irq
>                                        leave pcc handler for type3 chan
>                                       leave pcc_mbox_irq() handler
>                                       start processing in pcc_mbox_irq()
>                                        enter pcc handler for type4 chan
>                                        leave pcc handler for type4 chan
>                                        enter pcc handler for type3 chan
>                                        leave pcc handler for type3 chan
>                                       leave pcc_mbox_irq() handler
This is not easy to understand to me.
The issue as below described is already very clear to me.
So suggest remove above flow graph.
> 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.
Has this issue been confired? It's more better if has the log.😁
But it seems a valid issue.
>
> To resolve this issue, the platform acknowledgment interrupt should
> always be cleared before processing the interrupt for any notifications
> or response.
AFAIC，always clearing the platform ack interrupt first which is also the 
communication flow as ACPI spec described.
I am not sure if it is ok when triggering interrupt and clearing 
interrupt occur concurrently.
But this scenario is always possible. I think It doesn't matter with 
this patch. It's just my confusion.
>
> Reported-by: Robbie King <robbiek@xsightlabs.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Lgtm,
Reviewed-by: Huisong Li <lihuisong@huawei.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
>

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

[lihuisong (C)]

在 2025/3/3 18:51, 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.
How to confirm ACPI works only on little-endian?
>
> 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.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 4c582fa2b8bf4c9a9368dba8220f567555dba963..c87a5b7fa6eaf7bcabe0d55f844961c499376938 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_ACK_FLAG_MASK)
It's recommanded to delete PCC_ACK_FLAG_MASK and use 
PCC_CMD_COMPLETION_NOTIFY.
They are from the same place, namely, 'Initiator Responder 
Communications Channel Flags'.
>   		pcc_send_data(chan, NULL);
>   	else
>   		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>

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

[lihuisong (C)]

在 2025/3/3 18:51, 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.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
Acked-by: Huisong Li <lihuisong@huawei.com>

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

[lihuisong (C)]

在 2025/3/3 18:51, 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 a0fdafc3ef71d20c73ff58ef065201e6dc911396..e693675ce1fbd8d01d0640b3053a5c1882bdbce7 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;

Here should be like below code, right? 0 on success.

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 07/14] mailbox: pcc: Move pcc_mbox_ioremap() before pcc_mbox_request_channel()

[lihuisong (C)]

在 2025/3/3 18:51, Sudeep Holla 写道:
> In order to add support of mapping the generic communication shared
> memory region in the PCC mailbox driver when the PCC channel is requested,
> we need to move pcc_mbox_ioremap() before pcc_mbox_request_channel().
This patch is supposed to merge into patch 8/14 because it depend on 
this moving.
>
> No functional change.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 38 +++++++++++++++++++-------------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index e693675ce1fbd8d01d0640b3053a5c1882bdbce7..f230e512c29b79fc03e429145180ff049a250d2d 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -357,6 +357,25 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	return IRQ_HANDLED;
>   }
>   
> +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_mbox_request_channel - PCC clients call this function to
>    *		request a pointer to their PCC subspace, from which they
> @@ -419,25 +438,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
>

Re: [PATCH 08/14] mailbox: pcc: Always map the shared memory communication address

[lihuisong (C)]

在 2025/3/3 18:51, 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 headers in those
> memory to manage the transport.
>
> Since there are no users of pcc_chan_ioremap() and also it is mapped
> by default, we can stop exporting it and make it static function.

If pcc_chan_ioremap is static function, I think we can delete this 
function which just has one line if put this to pcc_mbox_request_channel().

>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/mailbox/pcc.c | 10 +++++++---
>   include/acpi/pcc.h    |  5 -----
>   2 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index f230e512c29b79fc03e429145180ff049a250d2d..5f2e2b727d99f07c44e87e44c11ba0aefe3a2318 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -357,7 +357,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	return IRQ_HANDLED;
>   }
>   
> -int pcc_mbox_ioremap(struct mbox_chan *chan)
> +static int pcc_mbox_ioremap(struct mbox_chan *chan)
>   {
>   	struct pcc_chan_info *pchan_info;
>   	struct pcc_mbox_chan *pcc_mbox_chan;
> @@ -374,7 +374,6 @@ int pcc_mbox_ioremap(struct mbox_chan *chan)
>   
>   	return 0;
>   }
> -EXPORT_SYMBOL_GPL(pcc_mbox_ioremap);
>   
>   /**
>    * pcc_mbox_request_channel - PCC clients call this function to
> @@ -409,7 +408,12 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	if (rc)
>   		return ERR_PTR(rc);
>   
> -	return &pchan->chan;
> +	rc = pcc_mbox_ioremap(chan);
> +	if (!rc)
> +		return &pchan->chan;
> +
> +	mbox_free_channel(chan);
> +	return ERR_PTR(rc);
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
>   
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 699c1a37b8e7846362bae35477eb5736be15d79e..0462bb9da6513a241f3b652b8e203299a1d990c7 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -38,7 +38,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)
> @@ -46,10 +45,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 */
>

Re: [PATCH 10/14] soc: hisilicon: kunpeng_hccs: Simplify PCC shared memory region handling

[lihuisong (C)]

在 2025/3/3 18:51, 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>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
With belows to change,
Reviewed-by: Huisong Li <lihuisong@huawei.com>
> ---
>   drivers/soc/hisilicon/kunpeng_hccs.c | 38 ++++++++++++------------------------
>   drivers/soc/hisilicon/kunpeng_hccs.h |  2 --
>   2 files changed, 13 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index 8aa8dec14911cdcdc2a2d11606bf6159144e9489..3b57189cd778f507afaa89bf47f0fa834043c244 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) {
> +	if (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);

Now the check of shared base address is not here. The log about this 
address no need to be printed.

Can you help me fix it like:

dev_err(dev, "The base size (%llu) of PCC communication region must be %d Byte.\n",
  			pcc_chan->shmem_size, HCCS_PCC_SHARE_MEM_BYTES
);

>   		rc = -EINVAL;
>   		goto err_mbx_channel_free;
>   	}
>   

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

[Sudeep Holla]

On Wed, Mar 05, 2025 at 12:02:13PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/3 18:51, 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.
> How to confirm ACPI works only on little-endian?
> > 
> > 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.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >   drivers/mailbox/pcc.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > index 4c582fa2b8bf4c9a9368dba8220f567555dba963..c87a5b7fa6eaf7bcabe0d55f844961c499376938 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_ACK_FLAG_MASK)
> It's recommanded to delete PCC_ACK_FLAG_MASK and use
> PCC_CMD_COMPLETION_NOTIFY.
> They are from the same place, namely, 'Initiator Responder Communications
> Channel Flags'.

Good point, I will use PCC_CMD_COMPLETION_NOTIFY and drop even the definition
of PCC_ACK_FLAG_MASK as it got added for no good reason.

--
Regards,
Sudeep

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

[Sudeep Holla]

On Wed, Mar 05, 2025 at 12:02:13PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/3 18:51, 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.
> How to confirm ACPI works only on little-endian?

Sorry I didn't notice this question. ACPI depends on ARCH_SUPPORTS_ACPI
and it is selected only from EFI which is disabled if CPU_BIG_ENDIAN=y

--
Regards,
Sudeep

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

[Sudeep Holla]

On Wed, Mar 05, 2025 at 02:09:03PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/3 18:51, 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 a0fdafc3ef71d20c73ff58ef065201e6dc911396..e693675ce1fbd8d01d0640b3053a5c1882bdbce7 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;
> 
> Here should be like below code, right? 0 on success.
> 
> if (pcc_mbox_error_check_and_clear(pchan))
>  		return IRQ_NONE;

Spot on, nice catch. Copied it from pcc_mbox_cmd_complete_check(), will fix
it.

-- 
Regards,
Sudeep

Re: [PATCH 07/14] mailbox: pcc: Move pcc_mbox_ioremap() before pcc_mbox_request_channel()

[Sudeep Holla]

On Wed, Mar 05, 2025 at 02:48:13PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/3 18:51, Sudeep Holla 写道:
> > In order to add support of mapping the generic communication shared
> > memory region in the PCC mailbox driver when the PCC channel is requested,
> > we need to move pcc_mbox_ioremap() before pcc_mbox_request_channel().
> This patch is supposed to merge into patch 8/14 because it depend on this
> moving.

Not sure what exactly you mean. This is 7/14 and 8/14 depends on it which
falls in natural order. Or do you mean just merge the patch into one. I
can do that as well, just kept it separate to easy the review. Let me
know. I thought keeping it separate is good for bisectibility in case
I break anything with these change 😉.

-- 
Regards,
Sudeep

Re: [PATCH 08/14] mailbox: pcc: Always map the shared memory communication address

[Sudeep Holla]

On Wed, Mar 05, 2025 at 02:54:39PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/3 18:51, 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 headers in those
> > memory to manage the transport.
> > 
> > Since there are no users of pcc_chan_ioremap() and also it is mapped
> > by default, we can stop exporting it and make it static function.
> 
> If pcc_chan_ioremap is static function, I think we can delete this function
> which just has one line if put this to pcc_mbox_request_channel().
> 

Makes sense, didn't notice while I was cleaning other things around it.

-- 
Regards,
Sudeep

Re: [PATCH 10/14] soc: hisilicon: kunpeng_hccs: Simplify PCC shared memory region handling

[Sudeep Holla]

On Wed, Mar 05, 2025 at 03:14:50PM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/3 18:51, 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>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> With belows to change,
> Reviewed-by: Huisong Li <lihuisong@huawei.com>

Thanks!

[...]

> > -	if (!pcc_chan->shmem_base_addr ||
> > -	    pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
> > +	if (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);
> 
> Now the check of shared base address is not here. The log about this address
> no need to be printed.
> 
> Can you help me fix it like:
> 
> dev_err(dev, "The base size (%llu) of PCC communication region must be %d Byte.\n",
>  			pcc_chan->shmem_size, HCCS_PCC_SHARE_MEM_BYTES
> );

Sure.

Did you get a chance to validate this driver and any other users of PCC
on your platform with these changes + the error handling fix you pointed
out ? That would be very useful as I don't have any set up to test.

--
Regards,
Sudeep

Re: [PATCH 07/14] mailbox: pcc: Move pcc_mbox_ioremap() before pcc_mbox_request_channel()

[Sudeep Holla]

On Wed, Mar 05, 2025 at 10:56:57AM +0000, Sudeep Holla wrote:
> On Wed, Mar 05, 2025 at 02:48:13PM +0800, lihuisong (C) wrote:
> > 
> > 在 2025/3/3 18:51, Sudeep Holla 写道:
> > > In order to add support of mapping the generic communication shared
> > > memory region in the PCC mailbox driver when the PCC channel is requested,
> > > we need to move pcc_mbox_ioremap() before pcc_mbox_request_channel().
> > This patch is supposed to merge into patch 8/14 because it depend on this
> > moving.
> 
> Not sure what exactly you mean. This is 7/14 and 8/14 depends on it which
> falls in natural order. Or do you mean just merge the patch into one. I
> can do that as well, just kept it separate to easy the review. Let me
> know. I thought keeping it separate is good for bisectibility in case
> I break anything with these change 😉.
> 

Scratch that, after reworking 8/14 as per your suggestion made me realise
what you meant. Sorry for that. Dropping this patch entirely now.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On Wed, Mar 05, 2025 at 11:45:35AM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/3 18:51, 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.
> > 
> >          Platform Firmware              OSPM/Linux PCC driver
> > ------------------------------------------------------------------------
> >                                       build message in shmem
> >                                       ring type3 channel doorbell
> > receives the doorbell interrupt
> >    process the message from OSPM
> >    build response for the message
> > ring the platform ack interrupt to OSPM
> > 				--->
> > build notification in type4 channel
> >                                       start processing in pcc_mbox_irq()
> >                                        enter pcc handler for type4 chan
> >                                           command complete cleared
> > 			        	 read the notification
> >                                  <---     clear platform ack irq
> >    		* no effect from above as platform ack irq *
> > 		* not yet triggered on this channel *
> > ring the platform ack irq on type4 channel
> > 				--->
> >                                        leave pcc handler for type4 chan
> >                                        enter pcc handler for type3 chan
> >                                           command complete set
> > 					 read the response
> >                                  <---     clear platform ack irq
> >                                        leave pcc handler for type3 chan
> >                                       leave pcc_mbox_irq() handler
> >                                       start processing in pcc_mbox_irq()
> >                                        enter pcc handler for type4 chan
> >                                        leave pcc handler for type4 chan
> >                                        enter pcc handler for type3 chan
> >                                        leave pcc handler for type3 chan
> >                                       leave pcc_mbox_irq() handler
> This is not easy to understand to me.
> The issue as below described is already very clear to me.
> So suggest remove above flow graph.

I understood it with the graph similar to the one above, though I simplified
it in terms of PCC rather than specific IP reference.

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

> Has this issue been confired? It's more better if has the log.😁
> But it seems a valid issue.

Yes Robbie reported this. He is away and can't test or respond until next
week. The log just says there was loads of spurious interrupts and nobody
cared log as you got in the first patch of yours fixing similar race.

> >
> > To resolve this issue, the platform acknowledgment interrupt should
> > always be cleared before processing the interrupt for any notifications
> > or response.
> >
> AFAIC，always clearing the platform ack interrupt first which is also the
> communication flow as ACPI spec described.

Indeed, not sure how we missed it so far.

> I am not sure if it is ok when triggering interrupt and clearing interrupt
> occur concurrently.

Should be OK as we start clearing all the channels that share, if the
handler doesn't clear any source, the interrupt must remain asserted.

> But this scenario is always possible. I think It doesn't matter with this
> patch. It's just my confusion.

Indeed, it can happen any time as you mentioned. No worries better to ask
and clarify than assume. Thanks for your time and review.

--
Regards,
Sudeep

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

[lihuisong (C)]

在 2025/3/5 22:29, Sudeep Holla 写道:
> On Wed, Mar 05, 2025 at 11:45:35AM +0800, lihuisong (C) wrote:
>> 在 2025/3/3 18:51, 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.
>>>
>>>           Platform Firmware              OSPM/Linux PCC driver
>>> ------------------------------------------------------------------------
>>>                                        build message in shmem
>>>                                        ring type3 channel doorbell
>>> receives the doorbell interrupt
>>>     process the message from OSPM
>>>     build response for the message
>>> ring the platform ack interrupt to OSPM
>>> 				--->
>>> build notification in type4 channel
>>>                                        start processing in pcc_mbox_irq()
>>>                                         enter pcc handler for type4 chan
>>>                                            command complete cleared
>>> 			        	 read the notification
>>>                                   <---     clear platform ack irq
>>>     		* no effect from above as platform ack irq *
>>> 		* not yet triggered on this channel *
>>> ring the platform ack irq on type4 channel
>>> 				--->
>>>                                         leave pcc handler for type4 chan
>>>                                         enter pcc handler for type3 chan
>>>                                            command complete set
>>> 					 read the response
>>>                                   <---     clear platform ack irq
>>>                                         leave pcc handler for type3 chan
>>>                                        leave pcc_mbox_irq() handler
>>>                                        start processing in pcc_mbox_irq()
>>>                                         enter pcc handler for type4 chan
>>>                                         leave pcc handler for type4 chan
>>>                                         enter pcc handler for type3 chan
>>>                                         leave pcc handler for type3 chan
>>>                                        leave pcc_mbox_irq() handler
>> This is not easy to understand to me.
>> The issue as below described is already very clear to me.
>> So suggest remove above flow graph.
> I understood it with the graph similar to the one above, though I simplified
> it in terms of PCC rather than specific IP reference.
>
>>> 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.
>>>
>> Has this issue been confired? It's more better if has the log.😁
>> But it seems a valid issue.
> Yes Robbie reported this. He is away and can't test or respond until next
> week. The log just says there was loads of spurious interrupts and nobody
> cared log as you got in the first patch of yours fixing similar race.
Yeah
>
>>> To resolve this issue, the platform acknowledgment interrupt should
>>> always be cleared before processing the interrupt for any notifications
>>> or response.
>>>
>> AFAIC，always clearing the platform ack interrupt first which is also the
>> communication flow as ACPI spec described.
> Indeed, not sure how we missed it so far.
>
>> I am not sure if it is ok when triggering interrupt and clearing interrupt
>> occur concurrently.
> Should be OK as we start clearing all the channels that share, if the
> handler doesn't clear any source, the interrupt must remain asserted.
ok, thank you for clarifying to me.
>
>> But this scenario is always possible. I think It doesn't matter with this
>> patch. It's just my confusion.
> Indeed, it can happen any time as you mentioned. No worries better to ask
> and clarify than assume. Thanks for your time and review.
>
> --
> Regards,
> Sudeep
>
>
> .

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

[lihuisong (C)]

在 2025/3/5 18:36, Sudeep Holla 写道:
> On Wed, Mar 05, 2025 at 12:02:13PM +0800, lihuisong (C) wrote:
>> 在 2025/3/3 18:51, 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.
>> How to confirm ACPI works only on little-endian?
> Sorry I didn't notice this question. ACPI depends on ARCH_SUPPORTS_ACPI
> and it is selected only from EFI which is disabled if CPU_BIG_ENDIAN=y
>
got it. I found it.
> .

Re: [PATCH 10/14] soc: hisilicon: kunpeng_hccs: Simplify PCC shared memory region handling

[lihuisong (C)]

在 2025/3/5 19:34, Sudeep Holla 写道:
> On Wed, Mar 05, 2025 at 03:14:50PM +0800, lihuisong (C) wrote:
>> 在 2025/3/3 18:51, 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>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> With belows to change,
>> Reviewed-by: Huisong Li <lihuisong@huawei.com>
> Thanks!
>
> [...]
>
>>> -	if (!pcc_chan->shmem_base_addr ||
>>> -	    pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
>>> +	if (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);
>> Now the check of shared base address is not here. The log about this address
>> no need to be printed.
>>
>> Can you help me fix it like:
>>
>> dev_err(dev, "The base size (%llu) of PCC communication region must be %d Byte.\n",
>>   			pcc_chan->shmem_size, HCCS_PCC_SHARE_MEM_BYTES
>> );
> Sure.
>
> Did you get a chance to validate this driver and any other users of PCC
> on your platform with these changes + the error handling fix you pointed
> out ? That would be very useful as I don't have any set up to test.
Sure, I'll test this series.
>
> .

Re: [PATCH 10/14] soc: hisilicon: kunpeng_hccs: Simplify PCC shared memory region handling

[Sudeep Holla]

On Thu, Mar 06, 2025 at 11:55:41AM +0800, lihuisong (C) wrote:
> 
> 在 2025/3/5 19:34, Sudeep Holla 写道:
> > On Wed, Mar 05, 2025 at 03:14:50PM +0800, lihuisong (C) wrote:
> > > 在 2025/3/3 18:51, 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>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > With belows to change,
> > > Reviewed-by: Huisong Li <lihuisong@huawei.com>
> > Thanks!
> > 
> > [...]
> > 
> > > > -	if (!pcc_chan->shmem_base_addr ||
> > > > -	    pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
> > > > +	if (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);
> > > Now the check of shared base address is not here. The log about this address
> > > no need to be printed.
> > > 
> > > Can you help me fix it like:
> > > 
> > > dev_err(dev, "The base size (%llu) of PCC communication region must be %d Byte.\n",
> > >   			pcc_chan->shmem_size, HCCS_PCC_SHARE_MEM_BYTES
> > > );
> > Sure.
> > 
> > Did you get a chance to validate this driver and any other users of PCC
> > on your platform with these changes + the error handling fix you pointed
> > out ? That would be very useful as I don't have any set up to test.
> Sure, I'll test this series.
> >

I have posted v2 [1] with your feedback incorporated. Please use the same.

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/r/20250305-pcc_fixes_updates-v2-0-1b1822bc8746@arm.com