linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces
@ 2021-07-08 18:08 Sudeep Holla
  2021-07-08 18:08 ` [PATCH 01/13] mailbox: pcc: Fix doxygen comments Sudeep Holla
                   ` (12 more replies)
  0 siblings, 13 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Hi,

Though the series is about adding support for PCCT extended PCC subspaces,
except one patch, remaining are either preparatory or clean up to add
the PCCT extended PCC subspaces. Only patch 12 adds the support of extended
PCC type3/4 subspaces.

The main change affecting your is the change in pcc_mbox_request_channel
to avoid clien driver using con_priv member which is designed for controller
private pointer rather than for client's to use that.

Shared memory region accesses could be consolidated but I am planning to
take that up later as some drivers are using different types of mappings,
yet to figure out on how to consolidate that aspect.

Regards,
Sudeep

Sudeep Holla (13):
  mailbox: pcc: Fix doxygen comments
  ACPI: CPPC: Fix doxygen comments
  mailbox: pcc: Refactor all PCC channel information into a structure
  mailbox: pcc: Consolidate subspace interrupt information parsing
  mailbox: pcc: Consolidate subspace doorbell register parsing
  mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info
  mailbox: pcc: Use PCC mailbox channel pointer instead of standard
  mailbox: pcc: Rename doorbell ack to platform interrupt ack register
  mailbox: pcc: Add PCC register bundle and associated accessor functions
  mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq
  mailbox: pcc: Drop handling invalid bit-width in {read,write}_register
  mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4)
  mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe

 drivers/acpi/cppc_acpi.c               |  50 +--
 drivers/hwmon/xgene-hwmon.c            |  35 +-
 drivers/i2c/busses/i2c-xgene-slimpro.c |  33 +-
 drivers/mailbox/pcc.c                  | 590 +++++++++++++++----------
 include/acpi/pcc.h                     |  21 +-
 5 files changed, 420 insertions(+), 309 deletions(-)

--
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 01/13] mailbox: pcc: Fix doxygen comments
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-08 18:51   ` Joe Perches
  2021-07-08 18:08 ` [PATCH 02/13] ACPI: CPPC: " Sudeep Holla
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Clang complains about doxygen comments too with W=1 in the build.

  |  drivers/mailbox/pcc.c:179: warning: Function parameter or member 'irq'
  |	not described in 'pcc_mbox_irq'
  |  drivers/mailbox/pcc.c:179: warning: Function parameter or member 'p'
  |	not described in 'pcc_mbox_irq'
  |  drivers/mailbox/pcc.c:378: warning: expecting prototype for
  |	parse_pcc_subspaces(). Prototype was for parse_pcc_subspace() instead

Fix it.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 0296558f9e22..23391e224a68 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -174,6 +174,10 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
 
 /**
  * pcc_mbox_irq - PCC mailbox interrupt handler
+ * @irq:	interrupt number
+ * @p: data/cookie passed from the caller to identify the channel
+ *
+ * Returns: IRQ_HANDLED if interrupt is handled or IRQ_NONE if not
  */
 static irqreturn_t pcc_mbox_irq(int irq, void *p)
 {
@@ -364,7 +368,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
 };
 
 /**
- * parse_pcc_subspaces -- Count PCC subspaces defined
+ * parse_pcc_subspace - Count PCC subspaces defined
  * @header: Pointer to the ACPI subtable header under the PCCT.
  * @end: End of subtable entry.
  *
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 02/13] ACPI: CPPC: Fix doxygen comments
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
  2021-07-08 18:08 ` [PATCH 01/13] mailbox: pcc: Fix doxygen comments Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-14 12:20   ` Rafael J. Wysocki
  2021-07-08 18:08 ` [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure Sudeep Holla
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Clang complains about doxygen comments too with W=1 in the build.

  | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member
  |	'pcc_ss_id' not described in 'pcc_data_alloc'
  | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member
  |	'cpu_num' not described in 'cppc_get_transition_latency'

Fix it.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/cppc_acpi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index a4d4eebba1da..eb5685167d19 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)
 /**
  * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
  *
+ * @pcc_ss_id: PCC Subspace channel identifier
+ *
  * Check and allocate the cppc_pcc_data memory.
  * In some processor configurations it is possible that same subspace
  * is shared between multiple CPUs. This is seen especially in CPUs
@@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
 /**
  * cppc_get_transition_latency - returns frequency transition latency in ns
  *
+ * @cpu_num: Logical index of the CPU for which latencty is requested
+ *
  * ACPI CPPC does not explicitly specify how a platform can specify the
  * transition latency for performance change requests. The closest we have
  * is the timing information from the PCCT tables which provides the info
  * on the number and frequency of PCC commands the platform can handle.
+ *
+ * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on
+ * failure
  */
 unsigned int cppc_get_transition_latency(int cpu_num)
 {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
  2021-07-08 18:08 ` [PATCH 01/13] mailbox: pcc: Fix doxygen comments Sudeep Holla
  2021-07-08 18:08 ` [PATCH 02/13] ACPI: CPPC: " Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-14 16:54   ` Cristian Marussi
  2021-07-08 18:08 ` [PATCH 04/13] mailbox: pcc: Consolidate subspace interrupt information parsing Sudeep Holla
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Currently all the PCC channel specific information are stored/maintained
in global individual arrays for each of those information. It is not
scalable and not clean if we have to stash more channel specific
information. Couple of reasons to stash more information are to extend
the support to Type 3/4 PCCT subspace and also to avoid accessing the
PCCT table entries themselves each time we need the information.

This patch moves all those PCC channel specific information into a
separate structure pcc_chan_info.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 23391e224a68..c5f481a615b0 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -64,12 +64,20 @@
 
 static struct mbox_chan *pcc_mbox_channels;
 
-/* Array of cached virtual address for doorbell registers */
-static void __iomem **pcc_doorbell_vaddr;
-/* Array of cached virtual address for doorbell ack registers */
-static void __iomem **pcc_doorbell_ack_vaddr;
-/* Array of doorbell interrupts */
-static int *pcc_doorbell_irq;
+/**
+ * struct pcc_chan_info - PCC channel specific information
+ *
+ * @db_vaddr: cached virtual address for doorbell register
+ * @db_ack_vaddr: cached virtual address for doorbell ack register
+ * @db_irq: doorbell interrupt
+ */
+struct pcc_chan_info {
+	void __iomem *db_vaddr;
+	void __iomem *db_ack_vaddr;
+	int db_irq;
+};
+
+static struct pcc_chan_info *chan_info;
 
 static struct mbox_controller pcc_mbox_ctrl = {};
 /**
@@ -183,6 +191,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 {
 	struct acpi_generic_address *doorbell_ack;
 	struct acpi_pcct_hw_reduced *pcct_ss;
+	struct pcc_chan_info *pchan;
 	struct mbox_chan *chan = p;
 	u64 doorbell_ack_preserve;
 	u64 doorbell_ack_write;
@@ -197,17 +206,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = chan->con_priv;
 		u32 id = chan - pcc_mbox_channels;
 
+		pchan = chan_info + id;
 		doorbell_ack = &pcct2_ss->platform_ack_register;
 		doorbell_ack_preserve = pcct2_ss->ack_preserve_mask;
 		doorbell_ack_write = pcct2_ss->ack_write_mask;
 
-		ret = read_register(pcc_doorbell_ack_vaddr[id],
-				    &doorbell_ack_val,
-				    doorbell_ack->bit_width);
+		ret = read_register(pchan->db_ack_vaddr,
+				    &doorbell_ack_val, doorbell_ack->bit_width);
 		if (ret)
 			return IRQ_NONE;
 
-		ret = write_register(pcc_doorbell_ack_vaddr[id],
+		ret = write_register(pchan->db_ack_vaddr,
 				     (doorbell_ack_val & doorbell_ack_preserve)
 					| doorbell_ack_write,
 				     doorbell_ack->bit_width);
@@ -232,8 +241,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
  *		ERR_PTR.
  */
 struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
-		int subspace_id)
+					   int subspace_id)
 {
+	struct pcc_chan_info *pchan = chan_info + subspace_id;
 	struct device *dev = pcc_mbox_ctrl.dev;
 	struct mbox_chan *chan;
 	unsigned long flags;
@@ -264,14 +274,14 @@ struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
 
 	spin_unlock_irqrestore(&chan->lock, flags);
 
-	if (pcc_doorbell_irq[subspace_id] > 0) {
+	if (pchan->db_irq > 0) {
 		int rc;
 
-		rc = devm_request_irq(dev, pcc_doorbell_irq[subspace_id],
-				      pcc_mbox_irq, 0, MBOX_IRQ_NAME, chan);
+		rc = devm_request_irq(dev, pchan->db_irq, pcc_mbox_irq, 0,
+				      MBOX_IRQ_NAME, chan);
 		if (unlikely(rc)) {
 			dev_err(dev, "failed to register PCC interrupt %d\n",
-				pcc_doorbell_irq[subspace_id]);
+				pchan->db_irq);
 			pcc_mbox_free_channel(chan);
 			chan = ERR_PTR(rc);
 		}
@@ -290,6 +300,7 @@ EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
 void pcc_mbox_free_channel(struct mbox_chan *chan)
 {
 	u32 id = chan - pcc_mbox_channels;
+	struct pcc_chan_info *pchan;
 	unsigned long flags;
 
 	if (!chan || !chan->cl)
@@ -300,8 +311,9 @@ void pcc_mbox_free_channel(struct mbox_chan *chan)
 		return;
 	}
 
-	if (pcc_doorbell_irq[id] > 0)
-		devm_free_irq(chan->mbox->dev, pcc_doorbell_irq[id], chan);
+	pchan = chan_info + id;
+	if (pchan->db_irq > 0)
+		devm_free_irq(chan->mbox->dev, pchan->db_irq, chan);
 
 	spin_lock_irqsave(&chan->lock, flags);
 	chan->cl = NULL;
@@ -329,6 +341,7 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
 {
 	struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
 	struct acpi_generic_address *doorbell;
+	struct pcc_chan_info *pchan;
 	u64 doorbell_preserve;
 	u64 doorbell_val;
 	u64 doorbell_write;
@@ -340,19 +353,20 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
 		return -ENOENT;
 	}
 
+	pchan = chan_info + id;
 	doorbell = &pcct_ss->doorbell_register;
 	doorbell_preserve = pcct_ss->preserve_mask;
 	doorbell_write = pcct_ss->write_mask;
 
 	/* Sync notification from OS to Platform. */
-	if (pcc_doorbell_vaddr[id]) {
-		ret = read_register(pcc_doorbell_vaddr[id], &doorbell_val,
-			doorbell->bit_width);
+	if (pchan->db_vaddr) {
+		ret = read_register(pchan->db_vaddr, &doorbell_val,
+				    doorbell->bit_width);
 		if (ret)
 			return ret;
-		ret = write_register(pcc_doorbell_vaddr[id],
-			(doorbell_val & doorbell_preserve) | doorbell_write,
-			doorbell->bit_width);
+		ret = write_register(pchan->db_vaddr,
+				     (doorbell_val & doorbell_preserve)
+				      | doorbell_write, doorbell->bit_width);
 	} else {
 		ret = acpi_read(&doorbell_val, doorbell);
 		if (ret)
@@ -398,12 +412,13 @@ static int parse_pcc_subspace(union acpi_subtable_headers *header,
  *
  * This gets called for each entry in the PCC table.
  */
-static int pcc_parse_subspace_irq(int id,
-				  struct acpi_pcct_hw_reduced *pcct_ss)
+static int pcc_parse_subspace_irq(int id, struct acpi_pcct_hw_reduced *pcct_ss)
 {
-	pcc_doorbell_irq[id] = pcc_map_interrupt(pcct_ss->platform_interrupt,
-						 (u32)pcct_ss->flags);
-	if (pcc_doorbell_irq[id] <= 0) {
+	struct pcc_chan_info *pchan = chan_info + id;
+
+	pchan->db_irq = pcc_map_interrupt(pcct_ss->platform_interrupt,
+					  (u32)pcct_ss->flags);
+	if (pchan->db_irq <= 0) {
 		pr_err("PCC GSI %d not registered\n",
 		       pcct_ss->platform_interrupt);
 		return -EINVAL;
@@ -413,10 +428,10 @@ static int pcc_parse_subspace_irq(int id,
 		== ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
 		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
 
-		pcc_doorbell_ack_vaddr[id] = acpi_os_ioremap(
-				pcct2_ss->platform_ack_register.address,
-				pcct2_ss->platform_ack_register.bit_width / 8);
-		if (!pcc_doorbell_ack_vaddr[id]) {
+		pchan->db_ack_vaddr =
+			acpi_os_ioremap(pcct2_ss->platform_ack_register.address,
+					pcct2_ss->platform_ack_register.bit_width / 8);
+		if (!pchan->db_ack_vaddr) {
 			pr_err("Failed to ioremap PCC ACK register\n");
 			return -ENOMEM;
 		}
@@ -474,24 +489,12 @@ static int __init acpi_pcc_probe(void)
 		goto err_put_pcct;
 	}
 
-	pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
-	if (!pcc_doorbell_vaddr) {
+	chan_info = kcalloc(count, sizeof(*chan_info), GFP_KERNEL);
+	if (!chan_info) {
 		rc = -ENOMEM;
 		goto err_free_mbox;
 	}
 
-	pcc_doorbell_ack_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
-	if (!pcc_doorbell_ack_vaddr) {
-		rc = -ENOMEM;
-		goto err_free_db_vaddr;
-	}
-
-	pcc_doorbell_irq = kcalloc(count, sizeof(int), GFP_KERNEL);
-	if (!pcc_doorbell_irq) {
-		rc = -ENOMEM;
-		goto err_free_db_ack_vaddr;
-	}
-
 	/* Point to the first PCC subspace entry */
 	pcct_entry = (struct acpi_subtable_header *) (
 		(unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
@@ -501,6 +504,7 @@ static int __init acpi_pcc_probe(void)
 		pcc_mbox_ctrl.txdone_irq = true;
 
 	for (i = 0; i < count; i++) {
+		struct pcc_chan_info *pchan = chan_info + i;
 		struct acpi_generic_address *db_reg;
 		struct acpi_pcct_subspace *pcct_ss;
 		pcc_mbox_channels[i].con_priv = pcct_entry;
@@ -522,8 +526,8 @@ static int __init acpi_pcc_probe(void)
 		/* If doorbell is in system memory cache the virt address */
 		db_reg = &pcct_ss->doorbell_register;
 		if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			pcc_doorbell_vaddr[i] = acpi_os_ioremap(db_reg->address,
-							db_reg->bit_width/8);
+			pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
+							  db_reg->bit_width / 8);
 		pcct_entry = (struct acpi_subtable_header *)
 			((unsigned long) pcct_entry + pcct_entry->length);
 	}
@@ -535,11 +539,7 @@ static int __init acpi_pcc_probe(void)
 	return 0;
 
 err:
-	kfree(pcc_doorbell_irq);
-err_free_db_ack_vaddr:
-	kfree(pcc_doorbell_ack_vaddr);
-err_free_db_vaddr:
-	kfree(pcc_doorbell_vaddr);
+	kfree(chan_info);
 err_free_mbox:
 	kfree(pcc_mbox_channels);
 err_put_pcct:
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 04/13] mailbox: pcc: Consolidate subspace interrupt information parsing
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (2 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-14 17:44   ` Cristian Marussi
  2021-07-08 18:08 ` [PATCH 05/13] mailbox: pcc: Consolidate subspace doorbell register parsing Sudeep Holla
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Extended PCC subspaces(Type 3 and 4) differs from generic(Type 0) and
HW-Reduced Communication(Type 1 and 2) subspace structures. However some
fields share same offsets and same type of structure can be use to extract
the fields. In order to simplify that, let us move all the IRQ related
information parsing into pcc_parse_subspace_irq and consolidate there.
It will be easier to extend it if required within the same.

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 c5f481a615b0..55866676a508 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -404,18 +404,26 @@ static int parse_pcc_subspace(union acpi_subtable_headers *header,
 
 /**
  * pcc_parse_subspace_irq - Parse the PCC IRQ and PCC ACK register
- *		There should be one entry per PCC client.
- * @id: PCC subspace index.
- * @pcct_ss: Pointer to the ACPI subtable header under the PCCT.
+ *
+ * @pchan: Pointer to the PCC channel info structure.
+ * @pcct_entry: Pointer to the ACPI subtable header.
  *
  * Return: 0 for Success, else errno.
  *
- * This gets called for each entry in the PCC table.
+ * There should be one entry per PCC channel. This gets called for each
+ * entry in the PCC table. This uses PCCY Type1 structure for all applicable
+ * types(Type 1 -4) to fetch irq
  */
-static int pcc_parse_subspace_irq(int id, struct acpi_pcct_hw_reduced *pcct_ss)
+static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
+				  struct acpi_subtable_header *pcct_entry)
 {
-	struct pcc_chan_info *pchan = chan_info + id;
+	struct acpi_pcct_hw_reduced *pcct_ss;
 
+	if (pcct_entry->type < ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
+	    pcct_entry->type > ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+		return 0;
+
+	pcct_ss = (struct acpi_pcct_hw_reduced *)pcct_entry;
 	pchan->db_irq = pcc_map_interrupt(pcct_ss->platform_interrupt,
 					  (u32)pcct_ss->flags);
 	if (pchan->db_irq <= 0) {
@@ -424,8 +432,7 @@ static int pcc_parse_subspace_irq(int id, struct acpi_pcct_hw_reduced *pcct_ss)
 		return -EINVAL;
 	}
 
-	if (pcct_ss->header.type
-		== ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
 		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
 
 		pchan->db_ack_vaddr =
@@ -509,17 +516,10 @@ static int __init acpi_pcc_probe(void)
 		struct acpi_pcct_subspace *pcct_ss;
 		pcc_mbox_channels[i].con_priv = pcct_entry;
 
-		if (pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
-		    pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
-			struct acpi_pcct_hw_reduced *pcct_hrss;
-
-			pcct_hrss = (struct acpi_pcct_hw_reduced *) pcct_entry;
-
-			if (pcc_mbox_ctrl.txdone_irq) {
-				rc = pcc_parse_subspace_irq(i, pcct_hrss);
-				if (rc < 0)
-					goto err;
-			}
+		if (pcc_mbox_ctrl.txdone_irq) {
+			rc = pcc_parse_subspace_irq(pchan, pcct_entry);
+			if (rc < 0)
+				goto err;
 		}
 		pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 05/13] mailbox: pcc: Consolidate subspace doorbell register parsing
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (3 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 04/13] mailbox: pcc: Consolidate subspace interrupt information parsing Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-14 18:09   ` Cristian Marussi
  2021-07-08 18:08 ` [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info Sudeep Holla
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Extended PCC subspaces(Type 3 and 4) differs from generic(Type 0) and
HW-Reduced Communication(Type 1 and 2) subspace structures. However some
fields share same offsets and same type of structure can be use to
extract the fields. In order to simplify that, let us move all the doorbell
register parsing into pcc_parse_subspace_db_reg and consolidate there.
It will be easier to extend it if required within the same.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 55866676a508..5f19bee71c04 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -447,6 +447,28 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
 	return 0;
 }
 
+/**
+ * pcc_parse_subspace_db_reg - Parse the PCC doorbell register
+ *
+ * @pchan: Pointer to the PCC channel info structure.
+ * @pcct_entry: Pointer to the ACPI subtable header.
+ *
+ */
+static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
+				      struct acpi_subtable_header *pcct_entry)
+{
+	struct acpi_pcct_subspace *pcct_ss;
+	struct acpi_generic_address *db_reg;
+
+	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
+
+	/* If doorbell is in system memory cache the virt address */
+	db_reg = &pcct_ss->doorbell_register;
+	if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
+						  db_reg->bit_width / 8);
+}
+
 /**
  * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
  *
@@ -512,8 +534,6 @@ static int __init acpi_pcc_probe(void)
 
 	for (i = 0; i < count; i++) {
 		struct pcc_chan_info *pchan = chan_info + i;
-		struct acpi_generic_address *db_reg;
-		struct acpi_pcct_subspace *pcct_ss;
 		pcc_mbox_channels[i].con_priv = pcct_entry;
 
 		if (pcc_mbox_ctrl.txdone_irq) {
@@ -521,13 +541,8 @@ static int __init acpi_pcc_probe(void)
 			if (rc < 0)
 				goto err;
 		}
-		pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
+		pcc_parse_subspace_db_reg(pchan, pcct_entry);
 
-		/* If doorbell is in system memory cache the virt address */
-		db_reg = &pcct_ss->doorbell_register;
-		if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
-			pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
-							  db_reg->bit_width / 8);
 		pcct_entry = (struct acpi_subtable_header *)
 			((unsigned long) pcct_entry + pcct_entry->length);
 	}
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (4 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 05/13] mailbox: pcc: Consolidate subspace doorbell register parsing Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-14 18:18   ` Cristian Marussi
  2021-07-08 18:08 ` [PATCH 07/13] mailbox: pcc: Use PCC mailbox channel pointer instead of standard Sudeep Holla
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Currently PCC mailbox controller sets con_priv in each channel to hold
the pointer to pcct subspace entry it corresponds to. The mailbox uses
will then fetch this pointer from the channel descriptor they get when
they request for the channel. Using that pointer they then parse the
pcct entry again to fetch all the information about shared memory region.

In order to remove individual users of PCC mailbox parsing the PCCT
subspace entries to fetch same information, let us consolidate the same
in pcc mailbox controller by parsing all the shared memory region
information into a structure that can also hold the mbox_chan pointer it
represent.

This can then be used as main PCC mailbox channel pointer that we can
return as part of pcc_mbox_request_channel instead of standard mailbox
channel pointer.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 5f19bee71c04..affde0995d52 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -67,11 +67,13 @@ static struct mbox_chan *pcc_mbox_channels;
 /**
  * struct pcc_chan_info - PCC channel specific information
  *
+ * @chan: PCC channel information with Shared Memory Region info
  * @db_vaddr: cached virtual address for doorbell register
  * @db_ack_vaddr: cached virtual address for doorbell ack register
  * @db_irq: doorbell interrupt
  */
 struct pcc_chan_info {
+	struct pcc_mbox_chan chan;
 	void __iomem *db_vaddr;
 	void __iomem *db_ack_vaddr;
 	int db_irq;
@@ -469,6 +471,27 @@ static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
 						  db_reg->bit_width / 8);
 }
 
+/**
+ * pcc_parse_subspace_shmem - Parse the PCC Shared Memory Region information
+ *
+ * @pchan: Pointer to the PCC channel info structure.
+ * @pcct_entry: Pointer to the ACPI subtable header.
+ *
+ */
+static void pcc_parse_subspace_shmem(struct pcc_chan_info *pchan,
+				     struct acpi_subtable_header *pcct_entry)
+{
+	struct acpi_pcct_subspace *pcct_ss;
+
+	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
+
+	pchan->chan.shmem_base_addr = pcct_ss->base_address;
+	pchan->chan.shmem_size = pcct_ss->length;
+	pchan->chan.latency = pcct_ss->latency;
+	pchan->chan.max_access_rate = pcct_ss->max_access_rate;
+	pchan->chan.min_turnaround_time = pcct_ss->min_turnaround_time;
+}
+
 /**
  * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
  *
@@ -536,6 +559,8 @@ static int __init acpi_pcc_probe(void)
 		struct pcc_chan_info *pchan = chan_info + i;
 		pcc_mbox_channels[i].con_priv = pcct_entry;
 
+		pchan->chan.mchan = &pcc_mbox_channels[i];
+
 		if (pcc_mbox_ctrl.txdone_irq) {
 			rc = pcc_parse_subspace_irq(pchan, pcct_entry);
 			if (rc < 0)
@@ -543,6 +568,8 @@ static int __init acpi_pcc_probe(void)
 		}
 		pcc_parse_subspace_db_reg(pchan, pcct_entry);
 
+		pcc_parse_subspace_shmem(pchan, pcct_entry);
+
 		pcct_entry = (struct acpi_subtable_header *)
 			((unsigned long) pcct_entry + pcct_entry->length);
 	}
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 4dec4ed138cd..5e510a6b8052 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -9,6 +9,15 @@
 #include <linux/mailbox_controller.h>
 #include <linux/mailbox_client.h>
 
+struct pcc_mbox_chan {
+	struct mbox_chan *mchan;
+	u64 shmem_base_addr;
+	u64 shmem_size;
+	u32 latency;
+	u32 max_access_rate;
+	u16 min_turnaround_time;
+};
+
 #define MAX_PCC_SUBSPACES	256
 #ifdef CONFIG_PCC
 extern struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 07/13] mailbox: pcc: Use PCC mailbox channel pointer instead of standard
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (5 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-08 18:08 ` [PATCH 08/13] mailbox: pcc: Rename doorbell ack to platform interrupt ack register Sudeep Holla
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar,
	Jean Delvare, Guenter Roeck, Wolfram Sang, linux-hwmon,
	linux-i2c

Now that we have all the shared memory region information populated in
the pcc_mbox_chan, let us propagate the pointer to the same as the
return value to pcc_mbox_request channel.

This eliminates the need for the individual users of PCC mailbox to
parse the PCCT subspace entries and fetch the shmem information. This
also eliminates the need for PCC mailbox controller to set con_priv to
PCCT subspace entries. This is required as con_priv is private to the
controller driver to attach private data associated with the channel and
not meant to be used by the mailbox client/users.

Let us convert all the users of pcc_mbox_{request,free}_channel to use
new interface.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Wolfram Sang <wsa@kernel.org>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-i2c@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/cppc_acpi.c               | 43 +++++++-----------
 drivers/hwmon/xgene-hwmon.c            | 35 ++++++---------
 drivers/i2c/busses/i2c-xgene-slimpro.c | 33 +++++---------
 drivers/mailbox/pcc.c                  | 61 +++++++-------------------
 include/acpi/pcc.h                     | 12 ++---
 5 files changed, 61 insertions(+), 123 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index eb5685167d19..dad6c0c1dd3d 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -43,7 +43,7 @@
 #include <acpi/cppc_acpi.h>
 
 struct cppc_pcc_data {
-	struct mbox_chan *pcc_channel;
+	struct pcc_mbox_chan *pcc_channel;
 	void __iomem *pcc_comm_addr;
 	bool pcc_channel_acquired;
 	unsigned int deadline_us;
@@ -295,7 +295,7 @@ static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
 	pcc_ss_data->platform_owns_pcc = true;
 
 	/* Ring doorbell */
-	ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
+	ret = mbox_send_message(pcc_ss_data->pcc_channel->mchan, &cmd);
 	if (ret < 0) {
 		pr_err("Err sending PCC mbox message. ss: %d cmd:%d, ret:%d\n",
 		       pcc_ss_id, cmd, ret);
@@ -308,10 +308,10 @@ static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
 	if (pcc_ss_data->pcc_mrtt)
 		pcc_ss_data->last_cmd_cmpl_time = ktime_get();
 
-	if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
-		mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
+	if (pcc_ss_data->pcc_channel->mchan->mbox->txdone_irq)
+		mbox_chan_txdone(pcc_ss_data->pcc_channel->mchan, ret);
 	else
-		mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
+		mbox_client_txdone(pcc_ss_data->pcc_channel->mchan, ret);
 
 end:
 	if (cmd == CMD_WRITE) {
@@ -493,46 +493,33 @@ EXPORT_SYMBOL_GPL(acpi_get_psd_map);
 
 static int register_pcc_channel(int pcc_ss_idx)
 {
-	struct acpi_pcct_hw_reduced *cppc_ss;
+	struct pcc_mbox_chan *pcc_chan;
 	u64 usecs_lat;
 
 	if (pcc_ss_idx >= 0) {
-		pcc_data[pcc_ss_idx]->pcc_channel =
-			pcc_mbox_request_channel(&cppc_mbox_cl,	pcc_ss_idx);
+		pcc_chan = pcc_mbox_request_channel(&cppc_mbox_cl, pcc_ss_idx);
 
-		if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
+		if (IS_ERR(pcc_chan)) {
 			pr_err("Failed to find PCC channel for subspace %d\n",
 			       pcc_ss_idx);
 			return -ENODEV;
 		}
 
-		/*
-		 * The PCC mailbox controller driver should
-		 * have parsed the PCCT (global table of all
-		 * PCC channels) and stored pointers to the
-		 * subspace communication region in con_priv.
-		 */
-		cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
-
-		if (!cppc_ss) {
-			pr_err("No PCC subspace found for %d CPPC\n",
-			       pcc_ss_idx);
-			return -ENODEV;
-		}
-
+		pcc_data[pcc_ss_idx]->pcc_channel = pcc_chan;
 		/*
 		 * cppc_ss->latency is just a Nominal value. In reality
 		 * the remote processor could be much slower to reply.
 		 * So add an arbitrary amount of wait on top of Nominal.
 		 */
-		usecs_lat = NUM_RETRIES * cppc_ss->latency;
+		usecs_lat = NUM_RETRIES * pcc_chan->latency;
 		pcc_data[pcc_ss_idx]->deadline_us = usecs_lat;
-		pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
-		pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
-		pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
+		pcc_data[pcc_ss_idx]->pcc_mrtt = pcc_chan->min_turnaround_time;
+		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(cppc_ss->base_address, cppc_ss->length);
+			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);
diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index 382ef0395d8e..30aae8642069 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -93,6 +93,7 @@ struct slimpro_resp_msg {
 struct xgene_hwmon_dev {
 	struct device		*dev;
 	struct mbox_chan	*mbox_chan;
+	struct pcc_mbox_chan	*pcc_chan;
 	struct mbox_client	mbox_client;
 	int			mbox_idx;
 
@@ -652,7 +653,7 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 			goto out_mbox_free;
 		}
 	} else {
-		struct acpi_pcct_hw_reduced *cppc_ss;
+		struct pcc_mbox_chan *pcc_chan;
 		const struct acpi_device_id *acpi_id;
 		int version;
 
@@ -671,26 +672,16 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		}
 
 		cl->rx_callback = xgene_hwmon_pcc_rx_cb;
-		ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx);
-		if (IS_ERR(ctx->mbox_chan)) {
+		pcc_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx);
+		if (IS_ERR(pcc_chan)) {
 			dev_err(&pdev->dev,
 				"PPC channel request failed\n");
 			rc = -ENODEV;
 			goto out_mbox_free;
 		}
 
-		/*
-		 * The PCC mailbox controller driver should
-		 * have parsed the PCCT (global table of all
-		 * PCC channels) and stored pointers to the
-		 * subspace communication region in con_priv.
-		 */
-		cppc_ss = ctx->mbox_chan->con_priv;
-		if (!cppc_ss) {
-			dev_err(&pdev->dev, "PPC subspace not found\n");
-			rc = -ENODEV;
-			goto out;
-		}
+		ctx->pcc_chan = pcc_chan;
+		ctx->mbox_chan = pcc_chan->mchan;
 
 		if (!ctx->mbox_chan->mbox->txdone_irq) {
 			dev_err(&pdev->dev, "PCC IRQ not supported\n");
@@ -702,16 +693,16 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		 * This is the shared communication region
 		 * for the OS and Platform to communicate over.
 		 */
-		ctx->comm_base_addr = cppc_ss->base_address;
+		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 *)ioremap(
 							ctx->comm_base_addr,
-							cppc_ss->length);
+							pcc_chan->shmem_size);
 			else
 				ctx->pcc_comm_addr = memremap(
 							ctx->comm_base_addr,
-							cppc_ss->length,
+							pcc_chan->shmem_size,
 							MEMREMAP_WB);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
@@ -727,11 +718,11 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 		}
 
 		/*
-		 * cppc_ss->latency is just a Nominal value. In reality
+		 * pcc_chan->latency is just a Nominal value. In reality
 		 * the remote processor could be much slower to reply.
 		 * So add an arbitrary amount of wait on top of Nominal.
 		 */
-		ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency;
+		ctx->usecs_lat = PCC_NUM_RETRIES * pcc_chan->latency;
 	}
 
 	ctx->hwmon_dev = hwmon_device_register_with_groups(ctx->dev,
@@ -757,7 +748,7 @@ static int xgene_hwmon_probe(struct platform_device *pdev)
 	if (acpi_disabled)
 		mbox_free_channel(ctx->mbox_chan);
 	else
-		pcc_mbox_free_channel(ctx->mbox_chan);
+		pcc_mbox_free_channel(ctx->pcc_chan);
 out_mbox_free:
 	kfifo_free(&ctx->async_msg_fifo);
 
@@ -773,7 +764,7 @@ static int xgene_hwmon_remove(struct platform_device *pdev)
 	if (acpi_disabled)
 		mbox_free_channel(ctx->mbox_chan);
 	else
-		pcc_mbox_free_channel(ctx->mbox_chan);
+		pcc_mbox_free_channel(ctx->pcc_chan);
 
 	return 0;
 }
diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
index bba08cbce6e1..1a19ebad60ad 100644
--- a/drivers/i2c/busses/i2c-xgene-slimpro.c
+++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
@@ -103,6 +103,7 @@ struct slimpro_i2c_dev {
 	struct i2c_adapter adapter;
 	struct device *dev;
 	struct mbox_chan *mbox_chan;
+	struct pcc_mbox_chan *pcc_chan;
 	struct mbox_client mbox_client;
 	int mbox_idx;
 	struct completion rd_complete;
@@ -466,7 +467,7 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->mbox_chan);
 		}
 	} else {
-		struct acpi_pcct_hw_reduced *cppc_ss;
+		struct pcc_mbox_chan *pcc_chan;
 		const struct acpi_device_id *acpi_id;
 		int version = XGENE_SLIMPRO_I2C_V1;
 
@@ -483,24 +484,14 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
 
 		cl->tx_block = false;
 		cl->rx_callback = slimpro_i2c_pcc_rx_cb;
-		ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx);
-		if (IS_ERR(ctx->mbox_chan)) {
+		pcc_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx);
+		if (IS_ERR(pcc_chan)) {
 			dev_err(&pdev->dev, "PCC mailbox channel request failed\n");
-			return PTR_ERR(ctx->mbox_chan);
+			return PTR_ERR(ctx->pcc_chan);
 		}
 
-		/*
-		 * The PCC mailbox controller driver should
-		 * have parsed the PCCT (global table of all
-		 * PCC channels) and stored pointers to the
-		 * subspace communication region in con_priv.
-		 */
-		cppc_ss = ctx->mbox_chan->con_priv;
-		if (!cppc_ss) {
-			dev_err(&pdev->dev, "PPC subspace not found\n");
-			rc = -ENOENT;
-			goto mbox_err;
-		}
+		ctx->pcc_chan = pcc_chan;
+		ctx->mbox_chan = pcc_chan->mchan;
 
 		if (!ctx->mbox_chan->mbox->txdone_irq) {
 			dev_err(&pdev->dev, "PCC IRQ not supported\n");
@@ -512,17 +503,17 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
 		 * This is the shared communication region
 		 * for the OS and Platform to communicate over.
 		 */
-		ctx->comm_base_addr = cppc_ss->base_address;
+		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,
-							cppc_ss->length,
+							pcc_chan->shmem_size,
 							MEMREMAP_WT);
 			else
 				ctx->pcc_comm_addr = memremap(
 							ctx->comm_base_addr,
-							cppc_ss->length,
+							pcc_chan->shmem_size,
 							MEMREMAP_WB);
 		} else {
 			dev_err(&pdev->dev, "Failed to get PCC comm region\n");
@@ -561,7 +552,7 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
 	if (acpi_disabled)
 		mbox_free_channel(ctx->mbox_chan);
 	else
-		pcc_mbox_free_channel(ctx->mbox_chan);
+		pcc_mbox_free_channel(ctx->pcc_chan);
 
 	return rc;
 }
@@ -575,7 +566,7 @@ static int xgene_slimpro_i2c_remove(struct platform_device *pdev)
 	if (acpi_disabled)
 		mbox_free_channel(ctx->mbox_chan);
 	else
-		pcc_mbox_free_channel(ctx->mbox_chan);
+		pcc_mbox_free_channel(ctx->pcc_chan);
 
 	return 0;
 }
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index affde0995d52..49971e007e40 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -79,24 +79,9 @@ struct pcc_chan_info {
 	int db_irq;
 };
 
+#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
 static struct pcc_chan_info *chan_info;
-
 static struct mbox_controller pcc_mbox_ctrl = {};
-/**
- * get_pcc_channel - Given a PCC subspace idx, get
- *	the respective mbox_channel.
- * @id: PCC subspace index.
- *
- * Return: ERR_PTR(errno) if error, else pointer
- *	to mbox channel.
- */
-static struct mbox_chan *get_pcc_channel(int id)
-{
-	if (id < 0 || id >= pcc_mbox_ctrl.num_chans)
-		return ERR_PTR(-ENOENT);
-
-	return &pcc_mbox_channels[id];
-}
 
 /*
  * PCC can be used with perf critical drivers such as CPPC
@@ -239,26 +224,16 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
  *		ACPI package. This is used to lookup the array of PCC
  *		subspaces as parsed by the PCC Mailbox controller.
  *
- * Return: Pointer to the Mailbox Channel if successful or
- *		ERR_PTR.
+ * Return: Pointer to the PCC Mailbox Channel if successful or ERR_PTR.
  */
-struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
-					   int subspace_id)
+struct pcc_mbox_chan *
+pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 {
 	struct pcc_chan_info *pchan = chan_info + subspace_id;
 	struct device *dev = pcc_mbox_ctrl.dev;
-	struct mbox_chan *chan;
+	struct mbox_chan *chan = pchan->chan.mchan;
 	unsigned long flags;
 
-	/*
-	 * Each PCC Subspace is a Mailbox Channel.
-	 * The PCC Clients get their PCC Subspace ID
-	 * from their own tables and pass it here.
-	 * This returns a pointer to the PCC subspace
-	 * for the Client to operate on.
-	 */
-	chan = get_pcc_channel(subspace_id);
-
 	if (IS_ERR(chan) || chan->cl) {
 		dev_err(dev, "Channel not found for idx: %d\n", subspace_id);
 		return ERR_PTR(-EBUSY);
@@ -284,38 +259,32 @@ struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
 		if (unlikely(rc)) {
 			dev_err(dev, "failed to register PCC interrupt %d\n",
 				pchan->db_irq);
-			pcc_mbox_free_channel(chan);
-			chan = ERR_PTR(rc);
+			pcc_mbox_free_channel(&pchan->chan);
+			return ERR_PTR(rc);
 		}
 	}
 
-	return chan;
+	return &pchan->chan;
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
 
 /**
  * pcc_mbox_free_channel - Clients call this to free their Channel.
  *
- * @chan: Pointer to the mailbox channel as returned by
- *		pcc_mbox_request_channel()
+ * @pchan: Pointer to the PCC mailbox channel as returned by
+ *	   pcc_mbox_request_channel()
  */
-void pcc_mbox_free_channel(struct mbox_chan *chan)
+void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
 {
-	u32 id = chan - pcc_mbox_channels;
-	struct pcc_chan_info *pchan;
+	struct pcc_chan_info *pchan_info = to_pcc_chan_info(pchan);
+	struct mbox_chan *chan = pchan->mchan;
 	unsigned long flags;
 
 	if (!chan || !chan->cl)
 		return;
 
-	if (id >= pcc_mbox_ctrl.num_chans) {
-		pr_debug("pcc_mbox_free_channel: Invalid mbox_chan passed\n");
-		return;
-	}
-
-	pchan = chan_info + id;
-	if (pchan->db_irq > 0)
-		devm_free_irq(chan->mbox->dev, pchan->db_irq, chan);
+	if (pchan_info->db_irq > 0)
+		devm_free_irq(chan->mbox->dev, pchan_info->db_irq, chan);
 
 	spin_lock_irqsave(&chan->lock, flags);
 	chan->cl = NULL;
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 5e510a6b8052..73e806fe7ce7 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -20,16 +20,16 @@ struct pcc_mbox_chan {
 
 #define MAX_PCC_SUBSPACES	256
 #ifdef CONFIG_PCC
-extern struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
-						  int subspace_id);
-extern void pcc_mbox_free_channel(struct mbox_chan *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);
 #else
-static inline struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
-							 int subspace_id)
+static inline struct pcc_mbox_chan *
+pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 {
 	return ERR_PTR(-ENODEV);
 }
-static inline void pcc_mbox_free_channel(struct mbox_chan *chan) { }
+static inline void pcc_mbox_free_channel(struct pcc_mbox_chan *chan) { }
 #endif
 
 #endif /* _PCC_H */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 08/13] mailbox: pcc: Rename doorbell ack to platform interrupt ack register
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (6 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 07/13] mailbox: pcc: Use PCC mailbox channel pointer instead of standard Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-08 18:08 ` [PATCH 09/13] mailbox: pcc: Add PCC register bundle and associated accessor functions Sudeep Holla
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

The specification refers this register and associated bitmask as platform
interrupt acknowledge register. Let us rename it so that it is easier to
map and understand.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 49971e007e40..c4eecccac4b8 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -69,13 +69,14 @@ static struct mbox_chan *pcc_mbox_channels;
  *
  * @chan: PCC channel information with Shared Memory Region info
  * @db_vaddr: cached virtual address for doorbell register
- * @db_ack_vaddr: cached virtual address for doorbell ack register
+ * @plat_irq_ack_vaddr: cached virtual address for platform interrupt
+ *	acknowledge register
  * @db_irq: doorbell interrupt
  */
 struct pcc_chan_info {
 	struct pcc_mbox_chan chan;
 	void __iomem *db_vaddr;
-	void __iomem *db_ack_vaddr;
+	void __iomem *plat_irq_ack_vaddr;
 	int db_irq;
 };
 
@@ -198,12 +199,12 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 		doorbell_ack_preserve = pcct2_ss->ack_preserve_mask;
 		doorbell_ack_write = pcct2_ss->ack_write_mask;
 
-		ret = read_register(pchan->db_ack_vaddr,
+		ret = read_register(pchan->plat_irq_ack_vaddr,
 				    &doorbell_ack_val, doorbell_ack->bit_width);
 		if (ret)
 			return IRQ_NONE;
 
-		ret = write_register(pchan->db_ack_vaddr,
+		ret = write_register(pchan->plat_irq_ack_vaddr,
 				     (doorbell_ack_val & doorbell_ack_preserve)
 					| doorbell_ack_write,
 				     doorbell_ack->bit_width);
@@ -406,10 +407,10 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
 	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
 		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
 
-		pchan->db_ack_vaddr =
+		pchan->plat_irq_ack_vaddr =
 			acpi_os_ioremap(pcct2_ss->platform_ack_register.address,
 					pcct2_ss->platform_ack_register.bit_width / 8);
-		if (!pchan->db_ack_vaddr) {
+		if (!pchan->plat_irq_ack_vaddr) {
 			pr_err("Failed to ioremap PCC ACK register\n");
 			return -ENOMEM;
 		}
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 09/13] mailbox: pcc: Add PCC register bundle and associated accessor functions
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (7 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 08/13] mailbox: pcc: Rename doorbell ack to platform interrupt ack register Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-08 18:08 ` [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq Sudeep Holla
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Extended PCC subspaces introduces more registers into the PCCT. In order
to consolidate access to these registers and to keep all the details
contained in one place, let us introduce PCC register bundle that holds
the ACPI Generic Address Structure as well as the virtual address for
the same if it is mapped in the OS.

It also contains the various masks used to access the register and
the associated read, write and read-modify-write accessors.

We can also clean up the initialisations by having a helper function
for the same.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index c4eecccac4b8..7d48e5c1ac52 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -52,6 +52,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/log2.h>
 #include <linux/platform_device.h>
 #include <linux/mailbox_controller.h>
 #include <linux/mailbox_client.h>
@@ -64,6 +65,23 @@
 
 static struct mbox_chan *pcc_mbox_channels;
 
+/**
+ * struct pcc_chan_reg - PCC register bundle
+ *
+ * @vaddr: cached virtual address for this register
+ * @gas: pointer to the generic address structure for this register
+ * @preserve_mask: bitmask to preserve when writing to this register
+ * @set_mask: bitmask to set when writing to this register
+ * @status_mask: bitmask to determine and/or update the status for this register
+ */
+struct pcc_chan_reg {
+	void __iomem *vaddr;
+	struct acpi_generic_address *gas;
+	u64 preserve_mask;
+	u64 set_mask;
+	u64 status_mask;
+};
+
 /**
  * struct pcc_chan_info - PCC channel specific information
  *
@@ -144,6 +162,53 @@ static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
 	return ret_val;
 }
 
+static int pcc_chan_reg_read(struct pcc_chan_reg *reg, u64 *val)
+{
+	int ret = 0;
+
+	if (!reg->gas) {
+		*val = 0;
+		return 0;
+	}
+
+	if (reg->vaddr)
+		ret = read_register(reg->vaddr, val, reg->gas->bit_width);
+	else
+		ret = acpi_read(val, reg->gas);
+
+	return ret;
+}
+
+static int pcc_chan_reg_write(struct pcc_chan_reg *reg, u64 val)
+{
+	int ret = 0;
+
+	if (!reg->gas)
+		return 0;
+
+	if (reg->vaddr)
+		ret = write_register(reg->vaddr, val, reg->gas->bit_width);
+	else
+		ret = acpi_write(val, reg->gas);
+
+	return ret;
+}
+
+static int pcc_chan_reg_read_modify_write(struct pcc_chan_reg *reg)
+{
+	int ret = 0;
+	u64 val;
+
+	ret = pcc_chan_reg_read(reg, &val);
+	if (ret)
+		return ret;
+
+	val &= reg->preserve_mask;
+	val |= reg->set_mask;
+
+	return pcc_chan_reg_write(reg, val);
+}
+
 /**
  * pcc_map_interrupt - Map a PCC subspace GSI to a linux IRQ number
  * @interrupt: GSI number.
@@ -374,6 +439,31 @@ static int parse_pcc_subspace(union acpi_subtable_headers *header,
 	return -EINVAL;
 }
 
+static int
+pcc_chan_reg_init(struct pcc_chan_reg *reg, struct acpi_generic_address *gas,
+		  u64 preserve_mask, u64 set_mask, u64 status_mask, char *name)
+{
+	if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+		if (!(gas->bit_width >= 8 && gas->bit_width <= 64 &&
+		      is_power_of_2(gas->bit_width))) {
+			pr_err("Error: Cannot access register of %u bit width",
+			       gas->bit_width);
+			return -EFAULT;
+		}
+
+		reg->vaddr = acpi_os_ioremap(gas->address, gas->bit_width / 8);
+		if (!reg->vaddr) {
+			pr_err("Failed to ioremap PCC %s register\n", name);
+			return -ENOMEM;
+		}
+	}
+	reg->gas = gas;
+	reg->preserve_mask = preserve_mask;
+	reg->set_mask = set_mask;
+	reg->status_mask = status_mask;
+	return 0;
+}
+
 /**
  * pcc_parse_subspace_irq - Parse the PCC IRQ and PCC ACK register
  *
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (8 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 09/13] mailbox: pcc: Add PCC register bundle and associated accessor functions Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-14 18:37   ` Cristian Marussi
  2021-07-15 13:38   ` Cristian Marussi
  2021-07-08 18:08 ` [PATCH 11/13] mailbox: pcc: Drop handling invalid bit-width in {read,write}_register Sudeep Holla
                   ` (2 subsequent siblings)
  12 siblings, 2 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Now that the con_priv is availvale solely for PCC mailbox controller
driver, let us use the same to save the channel specific information
in it so that we can it whenever required instead of parsing the PCCT
table entries every time in both pcc_send_data and pcc_mbox_irq.

We can now use the newly introduces PCC register bundle to simplify both
saving of channel specific information and accessing them without repeated
checks for the subspace type.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 7d48e5c1ac52..237dba9cb445 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -86,15 +86,15 @@ struct pcc_chan_reg {
  * struct pcc_chan_info - PCC channel specific information
  *
  * @chan: PCC channel information with Shared Memory Region info
- * @db_vaddr: cached virtual address for doorbell register
- * @plat_irq_ack_vaddr: cached virtual address for platform interrupt
- *	acknowledge register
+ * @db: PCC register bundle for the doorbell register
+ * @plat_irq_ack: PCC register bundle for the platform interrupt acknowledge
+ *	register
  * @db_irq: doorbell interrupt
  */
 struct pcc_chan_info {
 	struct pcc_mbox_chan chan;
-	void __iomem *db_vaddr;
-	void __iomem *plat_irq_ack_vaddr;
+	struct pcc_chan_reg db;
+	struct pcc_chan_reg plat_irq_ack;
 	int db_irq;
 };
 
@@ -242,40 +242,15 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
  */
 static irqreturn_t pcc_mbox_irq(int irq, void *p)
 {
-	struct acpi_generic_address *doorbell_ack;
-	struct acpi_pcct_hw_reduced *pcct_ss;
 	struct pcc_chan_info *pchan;
 	struct mbox_chan *chan = p;
-	u64 doorbell_ack_preserve;
-	u64 doorbell_ack_write;
-	u64 doorbell_ack_val;
-	int ret;
 
-	pcct_ss = chan->con_priv;
+	pchan = chan->con_priv;
 
 	mbox_chan_received_data(chan, NULL);
 
-	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
-		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = chan->con_priv;
-		u32 id = chan - pcc_mbox_channels;
-
-		pchan = chan_info + id;
-		doorbell_ack = &pcct2_ss->platform_ack_register;
-		doorbell_ack_preserve = pcct2_ss->ack_preserve_mask;
-		doorbell_ack_write = pcct2_ss->ack_write_mask;
-
-		ret = read_register(pchan->plat_irq_ack_vaddr,
-				    &doorbell_ack_val, doorbell_ack->bit_width);
-		if (ret)
-			return IRQ_NONE;
-
-		ret = write_register(pchan->plat_irq_ack_vaddr,
-				     (doorbell_ack_val & doorbell_ack_preserve)
-					| doorbell_ack_write,
-				     doorbell_ack->bit_width);
-		if (ret)
-			return IRQ_NONE;
-	}
+	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
+		return IRQ_NONE;
 
 	return IRQ_HANDLED;
 }
@@ -376,42 +351,9 @@ EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
  */
 static int pcc_send_data(struct mbox_chan *chan, void *data)
 {
-	struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
-	struct acpi_generic_address *doorbell;
-	struct pcc_chan_info *pchan;
-	u64 doorbell_preserve;
-	u64 doorbell_val;
-	u64 doorbell_write;
-	u32 id = chan - pcc_mbox_channels;
-	int ret = 0;
-
-	if (id >= pcc_mbox_ctrl.num_chans) {
-		pr_debug("pcc_send_data: Invalid mbox_chan passed\n");
-		return -ENOENT;
-	}
+	struct pcc_chan_info *pchan = chan->con_priv;
 
-	pchan = chan_info + id;
-	doorbell = &pcct_ss->doorbell_register;
-	doorbell_preserve = pcct_ss->preserve_mask;
-	doorbell_write = pcct_ss->write_mask;
-
-	/* Sync notification from OS to Platform. */
-	if (pchan->db_vaddr) {
-		ret = read_register(pchan->db_vaddr, &doorbell_val,
-				    doorbell->bit_width);
-		if (ret)
-			return ret;
-		ret = write_register(pchan->db_vaddr,
-				     (doorbell_val & doorbell_preserve)
-				      | doorbell_write, doorbell->bit_width);
-	} else {
-		ret = acpi_read(&doorbell_val, doorbell);
-		if (ret)
-			return ret;
-		ret = acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
-			doorbell);
-	}
-	return ret;
+	return pcc_chan_reg_read_modify_write(&pchan->db);
 }
 
 static const struct mbox_chan_ops pcc_chan_ops = {
@@ -479,6 +421,7 @@ pcc_chan_reg_init(struct pcc_chan_reg *reg, struct acpi_generic_address *gas,
 static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
 				  struct acpi_subtable_header *pcct_entry)
 {
+	int ret = 0;
 	struct acpi_pcct_hw_reduced *pcct_ss;
 
 	if (pcct_entry->type < ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
@@ -497,16 +440,14 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
 	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
 		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
 
-		pchan->plat_irq_ack_vaddr =
-			acpi_os_ioremap(pcct2_ss->platform_ack_register.address,
-					pcct2_ss->platform_ack_register.bit_width / 8);
-		if (!pchan->plat_irq_ack_vaddr) {
-			pr_err("Failed to ioremap PCC ACK register\n");
-			return -ENOMEM;
-		}
+		ret = pcc_chan_reg_init(&pchan->plat_irq_ack,
+					&pcct2_ss->platform_ack_register,
+					pcct2_ss->ack_preserve_mask,
+					pcct2_ss->ack_write_mask, 0,
+					"PLAT IRQ ACK");
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -516,19 +457,20 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
  * @pcct_entry: Pointer to the ACPI subtable header.
  *
  */
-static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
-				      struct acpi_subtable_header *pcct_entry)
+static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
+				     struct acpi_subtable_header *pcct_entry)
 {
+	int ret = 0;
+
 	struct acpi_pcct_subspace *pcct_ss;
-	struct acpi_generic_address *db_reg;
 
 	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
 
-	/* If doorbell is in system memory cache the virt address */
-	db_reg = &pcct_ss->doorbell_register;
-	if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
-		pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
-						  db_reg->bit_width / 8);
+	ret = pcc_chan_reg_init(&pchan->db,
+				&pcct_ss->doorbell_register,
+				pcct_ss->preserve_mask,
+				pcct_ss->write_mask, 0,	"Doorbell");
+	return ret;
 }
 
 /**
@@ -617,8 +559,8 @@ static int __init acpi_pcc_probe(void)
 
 	for (i = 0; i < count; i++) {
 		struct pcc_chan_info *pchan = chan_info + i;
-		pcc_mbox_channels[i].con_priv = pcct_entry;
 
+		pcc_mbox_channels[i].con_priv = pchan;
 		pchan->chan.mchan = &pcc_mbox_channels[i];
 
 		if (pcc_mbox_ctrl.txdone_irq) {
@@ -626,7 +568,9 @@ static int __init acpi_pcc_probe(void)
 			if (rc < 0)
 				goto err;
 		}
-		pcc_parse_subspace_db_reg(pchan, pcct_entry);
+		rc = pcc_parse_subspace_db_reg(pchan, pcct_entry);
+		if (rc < 0)
+			goto err;
 
 		pcc_parse_subspace_shmem(pchan, pcct_entry);
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 11/13] mailbox: pcc: Drop handling invalid bit-width in {read,write}_register
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (9 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-08 18:08 ` [PATCH 12/13] mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4) Sudeep Holla
  2021-07-08 18:08 ` [PATCH 13/13] mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe Sudeep Holla
  12 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

pcc_chan_reg_init now checks if the register bit width is within the
list [8, 16, 32, 64] and flags error if that is not the case. Therefore
there is no need to handling invalid bit-width in both read_register
and write_register. We can drop that along with the return values for
these 2 functions.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 237dba9cb445..99ad3429f174 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -110,10 +110,8 @@ static struct mbox_controller pcc_mbox_ctrl = {};
  * The below read_register and write_registers are used to read and
  * write from perf critical registers such as PCC doorbell register
  */
-static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
+static void read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
 {
-	int ret_val = 0;
-
 	switch (bit_width) {
 	case 8:
 		*val = readb(vaddr);
@@ -127,19 +125,11 @@ static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
 	case 64:
 		*val = readq(vaddr);
 		break;
-	default:
-		pr_debug("Error: Cannot read register of %u bit width",
-			bit_width);
-		ret_val = -EFAULT;
-		break;
 	}
-	return ret_val;
 }
 
-static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
+static void write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
 {
-	int ret_val = 0;
-
 	switch (bit_width) {
 	case 8:
 		writeb(val, vaddr);
@@ -153,13 +143,7 @@ static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
 	case 64:
 		writeq(val, vaddr);
 		break;
-	default:
-		pr_debug("Error: Cannot write register of %u bit width",
-			bit_width);
-		ret_val = -EFAULT;
-		break;
 	}
-	return ret_val;
 }
 
 static int pcc_chan_reg_read(struct pcc_chan_reg *reg, u64 *val)
@@ -172,7 +156,7 @@ static int pcc_chan_reg_read(struct pcc_chan_reg *reg, u64 *val)
 	}
 
 	if (reg->vaddr)
-		ret = read_register(reg->vaddr, val, reg->gas->bit_width);
+		read_register(reg->vaddr, val, reg->gas->bit_width);
 	else
 		ret = acpi_read(val, reg->gas);
 
@@ -187,7 +171,7 @@ static int pcc_chan_reg_write(struct pcc_chan_reg *reg, u64 val)
 		return 0;
 
 	if (reg->vaddr)
-		ret = write_register(reg->vaddr, val, reg->gas->bit_width);
+		write_register(reg->vaddr, val, reg->gas->bit_width);
 	else
 		ret = acpi_write(val, reg->gas);
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 12/13] mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4)
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (10 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 11/13] mailbox: pcc: Drop handling invalid bit-width in {read,write}_register Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  2021-07-14 18:52   ` Cristian Marussi
  2021-07-08 18:08 ` [PATCH 13/13] mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe Sudeep Holla
  12 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

With all the plumbing in place to avoid accessing PCCT type and other
fields directly from the PCCT table all the time, let us now add the
support for extended PCC subspaces(type 3 and 4).

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 99ad3429f174..22d1c7691887 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -89,12 +89,18 @@ struct pcc_chan_reg {
  * @db: PCC register bundle for the doorbell register
  * @plat_irq_ack: PCC register bundle for the platform interrupt acknowledge
  *	register
+ * @cmd_complete: PCC register bundle for the command complete check register
+ * @cmd_update: PCC register bundle for the command complete update register
+ * @error: PCC register bundle for the error status register
  * @db_irq: doorbell interrupt
  */
 struct pcc_chan_info {
 	struct pcc_mbox_chan chan;
 	struct pcc_chan_reg db;
 	struct pcc_chan_reg plat_irq_ack;
+	struct pcc_chan_reg cmd_complete;
+	struct pcc_chan_reg cmd_update;
+	struct pcc_chan_reg error;
 	int db_irq;
 };
 
@@ -228,9 +234,29 @@ 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;
 
+	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
+	if (ret)
+		return IRQ_NONE;
+
+	val &= pchan->cmd_complete.status_mask;
+	if (!val)
+		return IRQ_NONE;
+
+	ret = pcc_chan_reg_read(&pchan->error, &val);
+	if (ret)
+		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;
+	}
+
 	mbox_chan_received_data(chan, NULL);
 
 	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
@@ -335,8 +361,13 @@ EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
  */
 static int pcc_send_data(struct mbox_chan *chan, void *data)
 {
+	int ret;
 	struct pcc_chan_info *pchan = chan->con_priv;
 
+	ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
+	if (ret)
+		return ret;
+
 	return pcc_chan_reg_read_modify_write(&pchan->db);
 }
 
@@ -429,6 +460,16 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
 					pcct2_ss->ack_preserve_mask,
 					pcct2_ss->ack_write_mask, 0,
 					"PLAT IRQ ACK");
+
+	} else if (pcct_ss->header.type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE ||
+		   pcct_ss->header.type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) {
+		struct acpi_pcct_ext_pcc_master *pcct_ext = (void *)pcct_ss;
+
+		ret = pcc_chan_reg_init(&pchan->plat_irq_ack,
+					&pcct_ext->platform_ack_register,
+					pcct_ext->ack_preserve_mask,
+					pcct_ext->ack_set_mask, 0,
+					"PLAT IRQ ACK");
 	}
 
 	return ret;
@@ -446,14 +487,48 @@ static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
 {
 	int ret = 0;
 
-	struct acpi_pcct_subspace *pcct_ss;
-
-	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
-
-	ret = pcc_chan_reg_init(&pchan->db,
-				&pcct_ss->doorbell_register,
-				pcct_ss->preserve_mask,
-				pcct_ss->write_mask, 0,	"Doorbell");
+	if (pcct_entry->type <= ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+		struct acpi_pcct_subspace *pcct_ss;
+
+		pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
+
+		ret = pcc_chan_reg_init(&pchan->db,
+					&pcct_ss->doorbell_register,
+					pcct_ss->preserve_mask,
+					pcct_ss->write_mask, 0,	"Doorbell");
+
+	} else {
+		struct acpi_pcct_ext_pcc_master *pcct_ext;
+
+		pcct_ext = (struct acpi_pcct_ext_pcc_master *)pcct_entry;
+
+		ret = pcc_chan_reg_init(&pchan->db,
+					&pcct_ext->doorbell_register,
+					pcct_ext->preserve_mask,
+					pcct_ext->write_mask, 0, "Doorbell");
+		if (ret)
+			return ret;
+
+		ret = pcc_chan_reg_init(&pchan->cmd_complete,
+					&pcct_ext->cmd_complete_register,
+					0, 0, pcct_ext->cmd_complete_mask,
+					"Command Complete Check");
+		if (ret)
+			return ret;
+
+		ret = pcc_chan_reg_init(&pchan->cmd_update,
+					&pcct_ext->cmd_update_register,
+					pcct_ext->cmd_update_preserve_mask,
+					pcct_ext->cmd_update_set_mask, 0,
+					"Command Complete Update");
+		if (ret)
+			return ret;
+
+		ret = pcc_chan_reg_init(&pchan->error,
+					&pcct_ext->error_status_register,
+					0, 0, pcct_ext->error_status_mask,
+					"Error Status");
+	}
 	return ret;
 }
 
@@ -467,15 +542,25 @@ static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
 static void pcc_parse_subspace_shmem(struct pcc_chan_info *pchan,
 				     struct acpi_subtable_header *pcct_entry)
 {
-	struct acpi_pcct_subspace *pcct_ss;
-
-	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
-
-	pchan->chan.shmem_base_addr = pcct_ss->base_address;
-	pchan->chan.shmem_size = pcct_ss->length;
-	pchan->chan.latency = pcct_ss->latency;
-	pchan->chan.max_access_rate = pcct_ss->max_access_rate;
-	pchan->chan.min_turnaround_time = pcct_ss->min_turnaround_time;
+	if (pcct_entry->type <= ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+		struct acpi_pcct_subspace *pcct_ss =
+			(struct acpi_pcct_subspace *)pcct_entry;
+
+		pchan->chan.shmem_base_addr = pcct_ss->base_address;
+		pchan->chan.shmem_size = pcct_ss->length;
+		pchan->chan.latency = pcct_ss->latency;
+		pchan->chan.max_access_rate = pcct_ss->max_access_rate;
+		pchan->chan.min_turnaround_time = pcct_ss->min_turnaround_time;
+	} else {
+		struct acpi_pcct_ext_pcc_master *pcct_ext =
+			(struct acpi_pcct_ext_pcc_master *)pcct_entry;
+
+		pchan->chan.shmem_base_addr = pcct_ext->base_address;
+		pchan->chan.shmem_size = pcct_ext->length;
+		pchan->chan.latency = pcct_ext->latency;
+		pchan->chan.max_access_rate = pcct_ext->max_access_rate;
+		pchan->chan.min_turnaround_time = pcct_ext->min_turnaround_time;
+	}
 }
 
 /**
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 13/13] mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe
  2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
                   ` (11 preceding siblings ...)
  2021-07-08 18:08 ` [PATCH 12/13] mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4) Sudeep Holla
@ 2021-07-08 18:08 ` Sudeep Holla
  12 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 18:08 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: Sudeep Holla, Cristian Marussi, Rafael J . Wysocki, Jassi Brar

Move the PCCT subspace parsing and allocation into pcc_mbox_probe so
that we can get rid of global PCC channel and mailbox controller data.
It also helps to make use of devm_* APIs for all the allocations.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 22d1c7691887..c915b915e039 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -63,8 +63,6 @@
 
 #define MBOX_IRQ_NAME		"pcc-mbox"
 
-static struct mbox_chan *pcc_mbox_channels;
-
 /**
  * struct pcc_chan_reg - PCC register bundle
  *
@@ -106,7 +104,7 @@ struct pcc_chan_info {
 
 #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
 static struct pcc_chan_info *chan_info;
-static struct mbox_controller pcc_mbox_ctrl = {};
+static int pcc_chan_count;
 
 /*
  * PCC can be used with perf critical drivers such as CPPC
@@ -281,8 +279,8 @@ struct pcc_mbox_chan *
 pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 {
 	struct pcc_chan_info *pchan = chan_info + subspace_id;
-	struct device *dev = pcc_mbox_ctrl.dev;
 	struct mbox_chan *chan = pchan->chan.mchan;
+	struct device *dev = chan->mbox->dev;
 	unsigned long flags;
 
 	if (IS_ERR(chan) || chan->cl) {
@@ -570,16 +568,12 @@ static void pcc_parse_subspace_shmem(struct pcc_chan_info *pchan,
  */
 static int __init acpi_pcc_probe(void)
 {
+	int count, i, rc = 0;
+	acpi_status status;
 	struct acpi_table_header *pcct_tbl;
-	struct acpi_subtable_header *pcct_entry;
-	struct acpi_table_pcct *acpi_pcct_tbl;
 	struct acpi_subtable_proc proc[ACPI_PCCT_TYPE_RESERVED];
-	int count, i, rc;
-	acpi_status status = AE_OK;
 
-	/* Search for PCCT */
 	status = acpi_get_table(ACPI_SIG_PCCT, 0, &pcct_tbl);
-
 	if (ACPI_FAILURE(status) || !pcct_tbl)
 		return -ENODEV;
 
@@ -601,21 +595,60 @@ static int __init acpi_pcc_probe(void)
 			pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
 
 		rc = -EINVAL;
-		goto err_put_pcct;
+	} else {
+		pcc_chan_count = count;
 	}
 
-	pcc_mbox_channels = kcalloc(count, sizeof(struct mbox_chan),
-				    GFP_KERNEL);
+	acpi_put_table(pcct_tbl);
+
+	return rc;
+}
+
+/**
+ * pcc_mbox_probe - Called when we find a match for the
+ *	PCCT platform device. This is purely used to represent
+ *	the PCCT as a virtual device for registering with the
+ *	generic Mailbox framework.
+ *
+ * @pdev: Pointer to platform device returned when a match
+ *	is found.
+ *
+ *	Return: 0 for Success, else errno.
+ */
+static int pcc_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mbox_controller *pcc_mbox_ctrl;
+	struct mbox_chan *pcc_mbox_channels;
+	struct acpi_table_header *pcct_tbl;
+	struct acpi_subtable_header *pcct_entry;
+	struct acpi_table_pcct *acpi_pcct_tbl;
+	acpi_status status = AE_OK;
+	int i, rc, count = pcc_chan_count;
+
+	/* Search for PCCT */
+	status = acpi_get_table(ACPI_SIG_PCCT, 0, &pcct_tbl);
+
+	if (ACPI_FAILURE(status) || !pcct_tbl)
+		return -ENODEV;
+
+	pcc_mbox_channels = devm_kcalloc(dev, count, sizeof(*pcc_mbox_channels),
+					 GFP_KERNEL);
 	if (!pcc_mbox_channels) {
-		pr_err("Could not allocate space for PCC mbox channels\n");
 		rc = -ENOMEM;
-		goto err_put_pcct;
+		goto err;
 	}
 
-	chan_info = kcalloc(count, sizeof(*chan_info), GFP_KERNEL);
+	chan_info = devm_kcalloc(dev, count, sizeof(*chan_info), GFP_KERNEL);
 	if (!chan_info) {
 		rc = -ENOMEM;
-		goto err_free_mbox;
+		goto err;
+	}
+
+	pcc_mbox_ctrl = devm_kmalloc(dev, sizeof(*pcc_mbox_ctrl), GFP_KERNEL);
+	if (!pcc_mbox_ctrl) {
+		rc = -ENOMEM;
+		goto err;
 	}
 
 	/* Point to the first PCC subspace entry */
@@ -624,7 +657,7 @@ static int __init acpi_pcc_probe(void)
 
 	acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
 	if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
-		pcc_mbox_ctrl.txdone_irq = true;
+		pcc_mbox_ctrl->txdone_irq = true;
 
 	for (i = 0; i < count; i++) {
 		struct pcc_chan_info *pchan = chan_info + i;
@@ -632,7 +665,7 @@ static int __init acpi_pcc_probe(void)
 		pcc_mbox_channels[i].con_priv = pchan;
 		pchan->chan.mchan = &pcc_mbox_channels[i];
 
-		if (pcc_mbox_ctrl.txdone_irq) {
+		if (pcc_mbox_ctrl->txdone_irq) {
 			rc = pcc_parse_subspace_irq(pchan, pcct_entry);
 			if (rc < 0)
 				goto err;
@@ -647,51 +680,25 @@ static int __init acpi_pcc_probe(void)
 			((unsigned long) pcct_entry + pcct_entry->length);
 	}
 
-	pcc_mbox_ctrl.num_chans = count;
+	pcc_mbox_ctrl->num_chans = count;
 
-	pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
+	pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl->num_chans);
 
-	return 0;
+	pcc_mbox_ctrl->chans = pcc_mbox_channels;
+	pcc_mbox_ctrl->ops = &pcc_chan_ops;
+	pcc_mbox_ctrl->dev = dev;
 
+	pr_info("Registering PCC driver as Mailbox controller\n");
+	rc = mbox_controller_register(pcc_mbox_ctrl);
+	if (rc)
+		pr_err("Err registering PCC as Mailbox controller: %d\n", rc);
+	else
+		return 0;
 err:
-	kfree(chan_info);
-err_free_mbox:
-	kfree(pcc_mbox_channels);
-err_put_pcct:
 	acpi_put_table(pcct_tbl);
 	return rc;
 }
 
-/**
- * pcc_mbox_probe - Called when we find a match for the
- *	PCCT platform device. This is purely used to represent
- *	the PCCT as a virtual device for registering with the
- *	generic Mailbox framework.
- *
- * @pdev: Pointer to platform device returned when a match
- *	is found.
- *
- *	Return: 0 for Success, else errno.
- */
-static int pcc_mbox_probe(struct platform_device *pdev)
-{
-	int ret = 0;
-
-	pcc_mbox_ctrl.chans = pcc_mbox_channels;
-	pcc_mbox_ctrl.ops = &pcc_chan_ops;
-	pcc_mbox_ctrl.dev = &pdev->dev;
-
-	pr_info("Registering PCC driver as Mailbox controller\n");
-	ret = mbox_controller_register(&pcc_mbox_ctrl);
-
-	if (ret) {
-		pr_err("Err registering PCC as Mailbox controller: %d\n", ret);
-		ret = -ENODEV;
-	}
-
-	return ret;
-}
-
 static struct platform_driver pcc_mbox_driver = {
 	.probe = pcc_mbox_probe,
 	.driver = {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 01/13] mailbox: pcc: Fix doxygen comments
  2021-07-08 18:08 ` [PATCH 01/13] mailbox: pcc: Fix doxygen comments Sudeep Holla
@ 2021-07-08 18:51   ` Joe Perches
  2021-07-08 21:03     ` Sudeep Holla
  0 siblings, 1 reply; 32+ messages in thread
From: Joe Perches @ 2021-07-08 18:51 UTC (permalink / raw)
  To: Sudeep Holla, linux-acpi, linux-kernel
  Cc: Cristian Marussi, Rafael J . Wysocki, Jassi Brar

On Thu, 2021-07-08 at 19:08 +0100, Sudeep Holla wrote:
> Clang complains about doxygen comments too with W=1 in the build.

just fyi: it's not a clang message, it's kernel-doc that complains.

./scripts/kernel-doc -none drivers/mailbox/pcc.c



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 01/13] mailbox: pcc: Fix doxygen comments
  2021-07-08 18:51   ` Joe Perches
@ 2021-07-08 21:03     ` Sudeep Holla
  0 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-08 21:03 UTC (permalink / raw)
  To: Joe Perches
  Cc: linux-acpi, linux-kernel, Cristian Marussi, Rafael J . Wysocki,
	Sudeep Holla, Jassi Brar

On Thu, Jul 08, 2021 at 11:51:45AM -0700, Joe Perches wrote:
> On Thu, 2021-07-08 at 19:08 +0100, Sudeep Holla wrote:
> > Clang complains about doxygen comments too with W=1 in the build.
> 
> just fyi: it's not a clang message, it's kernel-doc that complains.
> 
> ./scripts/kernel-doc -none drivers/mailbox/pcc.c
>

Ah OK. Thanks for the info. I may have missed to observe this with gcc or
the kernel-doc is not built by default with gcc unlike clang.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 02/13] ACPI: CPPC: Fix doxygen comments
  2021-07-08 18:08 ` [PATCH 02/13] ACPI: CPPC: " Sudeep Holla
@ 2021-07-14 12:20   ` Rafael J. Wysocki
  2021-07-14 15:12     ` Sudeep Holla
  0 siblings, 1 reply; 32+ messages in thread
From: Rafael J. Wysocki @ 2021-07-14 12:20 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: ACPI Devel Maling List, Linux Kernel Mailing List,
	Cristian Marussi, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Clang complains about doxygen comments too with W=1 in the build.
>
>   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member
>   |     'pcc_ss_id' not described in 'pcc_data_alloc'
>   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member
>   |     'cpu_num' not described in 'cppc_get_transition_latency'
>
> Fix it.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/acpi/cppc_acpi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index a4d4eebba1da..eb5685167d19 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)
>  /**
>   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
>   *

I would drop this empty line (and analogously below).

> + * @pcc_ss_id: PCC Subspace channel identifier
> + *
>   * Check and allocate the cppc_pcc_data memory.
>   * In some processor configurations it is possible that same subspace
>   * is shared between multiple CPUs. This is seen especially in CPUs
> @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
>  /**
>   * cppc_get_transition_latency - returns frequency transition latency in ns
>   *
> + * @cpu_num: Logical index of the CPU for which latencty is requested
> + *
>   * ACPI CPPC does not explicitly specify how a platform can specify the
>   * transition latency for performance change requests. The closest we have
>   * is the timing information from the PCCT tables which provides the info
>   * on the number and frequency of PCC commands the platform can handle.
> + *
> + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on
> + * failure

Is this change needed?  The one-line summary already says this.

>   */
>  unsigned int cppc_get_transition_latency(int cpu_num)
>  {
> --

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 02/13] ACPI: CPPC: Fix doxygen comments
  2021-07-14 12:20   ` Rafael J. Wysocki
@ 2021-07-14 15:12     ` Sudeep Holla
  2021-07-14 16:07       ` Cristian Marussi
  0 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-14 15:12 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, Linux Kernel Mailing List,
	Cristian Marussi, Rafael J . Wysocki, Jassi Brar

On Wed, Jul 14, 2021 at 02:20:05PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Clang complains about doxygen comments too with W=1 in the build.
> >
> >   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member
> >   |     'pcc_ss_id' not described in 'pcc_data_alloc'
> >   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member
> >   |     'cpu_num' not described in 'cppc_get_transition_latency'
> >
> > Fix it.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/acpi/cppc_acpi.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index a4d4eebba1da..eb5685167d19 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)
> >  /**
> >   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
> >   *
>
> I would drop this empty line (and analogously below).
>

Sure

> > + * @pcc_ss_id: PCC Subspace channel identifier
> > + *
> >   * Check and allocate the cppc_pcc_data memory.
> >   * In some processor configurations it is possible that same subspace
> >   * is shared between multiple CPUs. This is seen especially in CPUs
> > @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
> >  /**
> >   * cppc_get_transition_latency - returns frequency transition latency in ns
> >   *
> > + * @cpu_num: Logical index of the CPU for which latencty is requested
> > + *
> >   * ACPI CPPC does not explicitly specify how a platform can specify the
> >   * transition latency for performance change requests. The closest we have
> >   * is the timing information from the PCCT tables which provides the info
> >   * on the number and frequency of PCC commands the platform can handle.
> > + *
> > + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on
> > + * failure
> 
> Is this change needed?  The one-line summary already says this.
>

Right, not required. I must have got confused with other place that expected
return summary.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 02/13] ACPI: CPPC: Fix doxygen comments
  2021-07-14 15:12     ` Sudeep Holla
@ 2021-07-14 16:07       ` Cristian Marussi
  2021-07-14 16:14         ` Sudeep Holla
  0 siblings, 1 reply; 32+ messages in thread
From: Cristian Marussi @ 2021-07-14 16:07 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Rafael J. Wysocki, ACPI Devel Maling List,
	Linux Kernel Mailing List, Rafael J . Wysocki, Jassi Brar

On Wed, Jul 14, 2021 at 04:12:10PM +0100, Sudeep Holla wrote:
> On Wed, Jul 14, 2021 at 02:20:05PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > Clang complains about doxygen comments too with W=1 in the build.
> > >
> > >   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member
> > >   |     'pcc_ss_id' not described in 'pcc_data_alloc'
> > >   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member
> > >   |     'cpu_num' not described in 'cppc_get_transition_latency'
> > >
> > > Fix it.
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >  drivers/acpi/cppc_acpi.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > index a4d4eebba1da..eb5685167d19 100644
> > > --- a/drivers/acpi/cppc_acpi.c
> > > +++ b/drivers/acpi/cppc_acpi.c
> > > @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)
> > >  /**
> > >   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
> > >   *
> >
> > I would drop this empty line (and analogously below).
> >
> 
> Sure
> 
> > > + * @pcc_ss_id: PCC Subspace channel identifier
> > > + *
> > >   * Check and allocate the cppc_pcc_data memory.
> > >   * In some processor configurations it is possible that same subspace
> > >   * is shared between multiple CPUs. This is seen especially in CPUs
> > > @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
> > >  /**
> > >   * cppc_get_transition_latency - returns frequency transition latency in ns
> > >   *
> > > + * @cpu_num: Logical index of the CPU for which latencty is requested
> > > + *
> > >   * ACPI CPPC does not explicitly specify how a platform can specify the
> > >   * transition latency for performance change requests. The closest we have
> > >   * is the timing information from the PCCT tables which provides the info
> > >   * on the number and frequency of PCC commands the platform can handle.
> > > + *
> > > + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on
> > > + * failure
> > 
> > Is this change needed?  The one-line summary already says this.
> >
> 
> Right, not required. I must have got confused with other place that expected
> return summary.
> 
I think kernel-doc complains if no Return: (not Returns:) doxygen clause
is provided while describing a function which do return some values.
(..even though the info is clearly duplicated as it is now in the
one-line summary)

Thanks,
Cristian

> -- 
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 02/13] ACPI: CPPC: Fix doxygen comments
  2021-07-14 16:07       ` Cristian Marussi
@ 2021-07-14 16:14         ` Sudeep Holla
  0 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-14 16:14 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: Rafael J. Wysocki, ACPI Devel Maling List,
	Linux Kernel Mailing List, Rafael J . Wysocki, Sudeep Holla,
	Jassi Brar

On Wed, Jul 14, 2021 at 05:07:02PM +0100, Cristian Marussi wrote:
> On Wed, Jul 14, 2021 at 04:12:10PM +0100, Sudeep Holla wrote:
> > On Wed, Jul 14, 2021 at 02:20:05PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Jul 8, 2021 at 8:09 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > Clang complains about doxygen comments too with W=1 in the build.
> > > >
> > > >   | drivers/acpi/cppc_acpi.c:560: warning: Function parameter or member
> > > >   |     'pcc_ss_id' not described in 'pcc_data_alloc'
> > > >   | drivers/acpi/cppc_acpi.c:1343: warning: Function parameter or member
> > > >   |     'cpu_num' not described in 'cppc_get_transition_latency'
> > > >
> > > > Fix it.
> > > >
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > > >  drivers/acpi/cppc_acpi.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > > index a4d4eebba1da..eb5685167d19 100644
> > > > --- a/drivers/acpi/cppc_acpi.c
> > > > +++ b/drivers/acpi/cppc_acpi.c
> > > > @@ -562,6 +562,8 @@ bool __weak cpc_ffh_supported(void)
> > > >  /**
> > > >   * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
> > > >   *
> > >
> > > I would drop this empty line (and analogously below).
> > >
> > 
> > Sure
> > 
> > > > + * @pcc_ss_id: PCC Subspace channel identifier
> > > > + *
> > > >   * Check and allocate the cppc_pcc_data memory.
> > > >   * In some processor configurations it is possible that same subspace
> > > >   * is shared between multiple CPUs. This is seen especially in CPUs
> > > > @@ -1347,10 +1349,15 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
> > > >  /**
> > > >   * cppc_get_transition_latency - returns frequency transition latency in ns
> > > >   *
> > > > + * @cpu_num: Logical index of the CPU for which latencty is requested
> > > > + *
> > > >   * ACPI CPPC does not explicitly specify how a platform can specify the
> > > >   * transition latency for performance change requests. The closest we have
> > > >   * is the timing information from the PCCT tables which provides the info
> > > >   * on the number and frequency of PCC commands the platform can handle.
> > > > + *
> > > > + * Returns: frequency transition latency on success or CPUFREQ_ETERNAL on
> > > > + * failure
> > > 
> > > Is this change needed?  The one-line summary already says this.
> > >
> >
> > Right, not required. I must have got confused with other place that expected
> > return summary.
> >
> I think kernel-doc complains if no Return: (not Returns:) doxygen clause
> is provided while describing a function which do return some values.
> (..even though the info is clearly duplicated as it is now in the
> one-line summary)
>

Thanks Cristian, just noticed the same. I was convinced that I did see the
warning before but couldn't recollect the details quickly.

$ ./scripts/kernel-doc -none drivers/acpi/cppc_acpi.c
(no warnings)

$ ./scripts/kernel-doc -v -none drivers/acpi/cppc_acpi.c 
drivers/acpi/cppc_acpi.c:1345: warning: No description found for return value of 'cppc_get_transition_latency'

The build with W=1 may not be using -v. That explains why I got confused as
I initially started with W=1 build but did switch to ./scripts/kernel-doc -v
after Joe pointed out its existence.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure
  2021-07-08 18:08 ` [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure Sudeep Holla
@ 2021-07-14 16:54   ` Cristian Marussi
  2021-07-15 11:27     ` Sudeep Holla
  0 siblings, 1 reply; 32+ messages in thread
From: Cristian Marussi @ 2021-07-14 16:54 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote:
> Currently all the PCC channel specific information are stored/maintained
> in global individual arrays for each of those information. It is not
> scalable and not clean if we have to stash more channel specific
> information. Couple of reasons to stash more information are to extend
> the support to Type 3/4 PCCT subspace and also to avoid accessing the
> PCCT table entries themselves each time we need the information.
> 
> This patch moves all those PCC channel specific information into a
> separate structure pcc_chan_info.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Hi Sudeep,

>  drivers/mailbox/pcc.c | 106 +++++++++++++++++++++---------------------
>  1 file changed, 53 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 23391e224a68..c5f481a615b0 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -64,12 +64,20 @@
>  
>  static struct mbox_chan *pcc_mbox_channels;
>  
> -/* Array of cached virtual address for doorbell registers */
> -static void __iomem **pcc_doorbell_vaddr;
> -/* Array of cached virtual address for doorbell ack registers */
> -static void __iomem **pcc_doorbell_ack_vaddr;
> -/* Array of doorbell interrupts */
> -static int *pcc_doorbell_irq;
> +/**
> + * struct pcc_chan_info - PCC channel specific information
> + *
> + * @db_vaddr: cached virtual address for doorbell register
> + * @db_ack_vaddr: cached virtual address for doorbell ack register
> + * @db_irq: doorbell interrupt
> + */
> +struct pcc_chan_info {
> +	void __iomem *db_vaddr;
> +	void __iomem *db_ack_vaddr;
> +	int db_irq;
> +};

Given that this db_irq represents the optional completion interrupt that is
used platform-->OSPM to signal command completions and/or notifications/
delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform
Interrupt" and also referred in this driver as such somewherelse, is it not
misleading to call it then here "doorbell interrupt" since the "doorbell" in
this context is usually the interrupt that goes the other way around
OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell
registers ? (that are indeed called Doorbell throughout this driver down below
...but I understand this was the nomenclature used also before in this driver)

Thanks,
Cristian


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 04/13] mailbox: pcc: Consolidate subspace interrupt information parsing
  2021-07-08 18:08 ` [PATCH 04/13] mailbox: pcc: Consolidate subspace interrupt information parsing Sudeep Holla
@ 2021-07-14 17:44   ` Cristian Marussi
  0 siblings, 0 replies; 32+ messages in thread
From: Cristian Marussi @ 2021-07-14 17:44 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 08, 2021 at 07:08:42PM +0100, Sudeep Holla wrote:
> Extended PCC subspaces(Type 3 and 4) differs from generic(Type 0) and

nit: s/differs/differ

> HW-Reduced Communication(Type 1 and 2) subspace structures. However some
> fields share same offsets and same type of structure can be use to extract
> the fields. In order to simplify that, let us move all the IRQ related
> information parsing into pcc_parse_subspace_irq and consolidate there.
> It will be easier to extend it if required within the same.
> 
> 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 c5f481a615b0..55866676a508 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -404,18 +404,26 @@ static int parse_pcc_subspace(union acpi_subtable_headers *header,
>  
>  /**
>   * pcc_parse_subspace_irq - Parse the PCC IRQ and PCC ACK register
> - *		There should be one entry per PCC client.
> - * @id: PCC subspace index.
> - * @pcct_ss: Pointer to the ACPI subtable header under the PCCT.
> + *
> + * @pchan: Pointer to the PCC channel info structure.
> + * @pcct_entry: Pointer to the ACPI subtable header.
>   *
>   * Return: 0 for Success, else errno.
>   *
> - * This gets called for each entry in the PCC table.
> + * There should be one entry per PCC channel. This gets called for each
> + * entry in the PCC table. This uses PCCY Type1 structure for all applicable
> + * types(Type 1 -4) to fetch irq
>   */
> -static int pcc_parse_subspace_irq(int id, struct acpi_pcct_hw_reduced *pcct_ss)
> +static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
> +				  struct acpi_subtable_header *pcct_entry)
>  {
> -	struct pcc_chan_info *pchan = chan_info + id;
> +	struct acpi_pcct_hw_reduced *pcct_ss;
>  
> +	if (pcct_entry->type < ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
> +	    pcct_entry->type > ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		return 0;
> +
> +	pcct_ss = (struct acpi_pcct_hw_reduced *)pcct_entry;
>  	pchan->db_irq = pcc_map_interrupt(pcct_ss->platform_interrupt,
>  					  (u32)pcct_ss->flags);
>  	if (pchan->db_irq <= 0) {
> @@ -424,8 +432,7 @@ static int pcc_parse_subspace_irq(int id, struct acpi_pcct_hw_reduced *pcct_ss)
>  		return -EINVAL;
>  	}
>  
> -	if (pcct_ss->header.type
> -		== ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> +	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
>  		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
>  
>  		pchan->db_ack_vaddr =
> @@ -509,17 +516,10 @@ static int __init acpi_pcc_probe(void)
>  		struct acpi_pcct_subspace *pcct_ss;
>  		pcc_mbox_channels[i].con_priv = pcct_entry;
>  
> -		if (pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
> -		    pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> -			struct acpi_pcct_hw_reduced *pcct_hrss;
> -
> -			pcct_hrss = (struct acpi_pcct_hw_reduced *) pcct_entry;
> -
> -			if (pcc_mbox_ctrl.txdone_irq) {
> -				rc = pcc_parse_subspace_irq(i, pcct_hrss);
> -				if (rc < 0)
> -					goto err;
> -			}
> +		if (pcc_mbox_ctrl.txdone_irq) {
> +			rc = pcc_parse_subspace_irq(pchan, pcct_entry);
> +			if (rc < 0)
> +				goto err;
>  		}
>  		pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
>  
> -- 
> 2.25.1

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 05/13] mailbox: pcc: Consolidate subspace doorbell register parsing
  2021-07-08 18:08 ` [PATCH 05/13] mailbox: pcc: Consolidate subspace doorbell register parsing Sudeep Holla
@ 2021-07-14 18:09   ` Cristian Marussi
  0 siblings, 0 replies; 32+ messages in thread
From: Cristian Marussi @ 2021-07-14 18:09 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 08, 2021 at 07:08:43PM +0100, Sudeep Holla wrote:
> Extended PCC subspaces(Type 3 and 4) differs from generic(Type 0) and
nit: s/differs/differ

> HW-Reduced Communication(Type 1 and 2) subspace structures. However some
> fields share same offsets and same type of structure can be use to
> extract the fields. In order to simplify that, let us move all the doorbell
> register parsing into pcc_parse_subspace_db_reg and consolidate there.
> It will be easier to extend it if required within the same.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/mailbox/pcc.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 55866676a508..5f19bee71c04 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -447,6 +447,28 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>  	return 0;
>  }
>  
> +/**
> + * pcc_parse_subspace_db_reg - Parse the PCC doorbell register
> + *
> + * @pchan: Pointer to the PCC channel info structure.
> + * @pcct_entry: Pointer to the ACPI subtable header.
> + *
> + */
> +static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> +				      struct acpi_subtable_header *pcct_entry)
> +{
> +	struct acpi_pcct_subspace *pcct_ss;
> +	struct acpi_generic_address *db_reg;
> +
> +	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
> +
> +	/* If doorbell is in system memory cache the virt address */
> +	db_reg = &pcct_ss->doorbell_register;
> +	if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +		pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
> +						  db_reg->bit_width / 8);
> +}
> +
>  /**
>   * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
>   *
> @@ -512,8 +534,6 @@ static int __init acpi_pcc_probe(void)
>  
>  	for (i = 0; i < count; i++) {
>  		struct pcc_chan_info *pchan = chan_info + i;
> -		struct acpi_generic_address *db_reg;
> -		struct acpi_pcct_subspace *pcct_ss;
>  		pcc_mbox_channels[i].con_priv = pcct_entry;
>  
>  		if (pcc_mbox_ctrl.txdone_irq) {
> @@ -521,13 +541,8 @@ static int __init acpi_pcc_probe(void)
>  			if (rc < 0)
>  				goto err;
>  		}
> -		pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
> +		pcc_parse_subspace_db_reg(pchan, pcct_entry);
>  
> -		/* If doorbell is in system memory cache the virt address */
> -		db_reg = &pcct_ss->doorbell_register;
> -		if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> -			pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
> -							  db_reg->bit_width / 8);
>  		pcct_entry = (struct acpi_subtable_header *)
>  			((unsigned long) pcct_entry + pcct_entry->length);
>  	}
> -- 
> 2.25.1
> 

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info
  2021-07-08 18:08 ` [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info Sudeep Holla
@ 2021-07-14 18:18   ` Cristian Marussi
  2021-07-15 11:22     ` Sudeep Holla
  0 siblings, 1 reply; 32+ messages in thread
From: Cristian Marussi @ 2021-07-14 18:18 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 08, 2021 at 07:08:44PM +0100, Sudeep Holla wrote:
> Currently PCC mailbox controller sets con_priv in each channel to hold
> the pointer to pcct subspace entry it corresponds to. The mailbox uses

nit: s/uses/users

> will then fetch this pointer from the channel descriptor they get when
> they request for the channel. Using that pointer they then parse the
> pcct entry again to fetch all the information about shared memory region.
> 
> In order to remove individual users of PCC mailbox parsing the PCCT
> subspace entries to fetch same information, let us consolidate the same
> in pcc mailbox controller by parsing all the shared memory region
> information into a structure that can also hold the mbox_chan pointer it
> represent.
> 
> This can then be used as main PCC mailbox channel pointer that we can
> return as part of pcc_mbox_request_channel instead of standard mailbox
> channel pointer.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/mailbox/pcc.c | 27 +++++++++++++++++++++++++++
>  include/acpi/pcc.h    |  9 +++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 5f19bee71c04..affde0995d52 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -67,11 +67,13 @@ static struct mbox_chan *pcc_mbox_channels;
>  /**
>   * struct pcc_chan_info - PCC channel specific information
>   *
> + * @chan: PCC channel information with Shared Memory Region info
>   * @db_vaddr: cached virtual address for doorbell register
>   * @db_ack_vaddr: cached virtual address for doorbell ack register
>   * @db_irq: doorbell interrupt
>   */
>  struct pcc_chan_info {
> +	struct pcc_mbox_chan chan;
>  	void __iomem *db_vaddr;
>  	void __iomem *db_ack_vaddr;
>  	int db_irq;
> @@ -469,6 +471,27 @@ static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
>  						  db_reg->bit_width / 8);
>  }
>  
> +/**
> + * pcc_parse_subspace_shmem - Parse the PCC Shared Memory Region information
> + *
> + * @pchan: Pointer to the PCC channel info structure.
> + * @pcct_entry: Pointer to the ACPI subtable header.
> + *
> + */
> +static void pcc_parse_subspace_shmem(struct pcc_chan_info *pchan,
> +				     struct acpi_subtable_header *pcct_entry)
> +{
> +	struct acpi_pcct_subspace *pcct_ss;
> +
> +	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
> +
> +	pchan->chan.shmem_base_addr = pcct_ss->base_address;
> +	pchan->chan.shmem_size = pcct_ss->length;
> +	pchan->chan.latency = pcct_ss->latency;
> +	pchan->chan.max_access_rate = pcct_ss->max_access_rate;
> +	pchan->chan.min_turnaround_time = pcct_ss->min_turnaround_time;
> +}
> +

Out of curiosity this ACPI provided latency/max_access/turnaround_time
are supposed to be considered and/or enforced where ? by the clients
using this controller ?

>  /**
>   * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
>   *
> @@ -536,6 +559,8 @@ static int __init acpi_pcc_probe(void)
>  		struct pcc_chan_info *pchan = chan_info + i;
>  		pcc_mbox_channels[i].con_priv = pcct_entry;
>  
> +		pchan->chan.mchan = &pcc_mbox_channels[i];
> +
>  		if (pcc_mbox_ctrl.txdone_irq) {
>  			rc = pcc_parse_subspace_irq(pchan, pcct_entry);
>  			if (rc < 0)
> @@ -543,6 +568,8 @@ static int __init acpi_pcc_probe(void)
>  		}
>  		pcc_parse_subspace_db_reg(pchan, pcct_entry);
>  
> +		pcc_parse_subspace_shmem(pchan, pcct_entry);
> +
>  		pcct_entry = (struct acpi_subtable_header *)
>  			((unsigned long) pcct_entry + pcct_entry->length);
>  	}
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 4dec4ed138cd..5e510a6b8052 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -9,6 +9,15 @@
>  #include <linux/mailbox_controller.h>
>  #include <linux/mailbox_client.h>
>  
> +struct pcc_mbox_chan {
> +	struct mbox_chan *mchan;
> +	u64 shmem_base_addr;
> +	u64 shmem_size;
> +	u32 latency;
> +	u32 max_access_rate;
> +	u16 min_turnaround_time;
> +};
> +
>  #define MAX_PCC_SUBSPACES	256
>  #ifdef CONFIG_PCC
>  extern struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
> -- 
> 2.25.1


Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq
  2021-07-08 18:08 ` [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq Sudeep Holla
@ 2021-07-14 18:37   ` Cristian Marussi
  2021-07-15 13:38   ` Cristian Marussi
  1 sibling, 0 replies; 32+ messages in thread
From: Cristian Marussi @ 2021-07-14 18:37 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 08, 2021 at 07:08:48PM +0100, Sudeep Holla wrote:
> Now that the con_priv is availvale solely for PCC mailbox controller
nit: s/availvale/available

> driver, let us use the same to save the channel specific information
> in it so that we can it whenever required instead of parsing the PCCT
> table entries every time in both pcc_send_data and pcc_mbox_irq.
> 
> We can now use the newly introduces PCC register bundle to simplify both
> saving of channel specific information and accessing them without repeated
> checks for the subspace type.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

>  drivers/mailbox/pcc.c | 116 +++++++++++-------------------------------
>  1 file changed, 30 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 7d48e5c1ac52..237dba9cb445 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -86,15 +86,15 @@ struct pcc_chan_reg {
>   * struct pcc_chan_info - PCC channel specific information
>   *
>   * @chan: PCC channel information with Shared Memory Region info
> - * @db_vaddr: cached virtual address for doorbell register
> - * @plat_irq_ack_vaddr: cached virtual address for platform interrupt
> - *	acknowledge register
> + * @db: PCC register bundle for the doorbell register
> + * @plat_irq_ack: PCC register bundle for the platform interrupt acknowledge
> + *	register
>   * @db_irq: doorbell interrupt
>   */
>  struct pcc_chan_info {
>  	struct pcc_mbox_chan chan;
> -	void __iomem *db_vaddr;
> -	void __iomem *plat_irq_ack_vaddr;
> +	struct pcc_chan_reg db;
> +	struct pcc_chan_reg plat_irq_ack;
>  	int db_irq;
>  };
>  
> @@ -242,40 +242,15 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>   */
>  static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  {
> -	struct acpi_generic_address *doorbell_ack;
> -	struct acpi_pcct_hw_reduced *pcct_ss;
>  	struct pcc_chan_info *pchan;
>  	struct mbox_chan *chan = p;
> -	u64 doorbell_ack_preserve;
> -	u64 doorbell_ack_write;
> -	u64 doorbell_ack_val;
> -	int ret;
>  
> -	pcct_ss = chan->con_priv;
> +	pchan = chan->con_priv;
>  
>  	mbox_chan_received_data(chan, NULL);
>  
> -	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> -		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = chan->con_priv;
> -		u32 id = chan - pcc_mbox_channels;
> -
> -		pchan = chan_info + id;
> -		doorbell_ack = &pcct2_ss->platform_ack_register;
> -		doorbell_ack_preserve = pcct2_ss->ack_preserve_mask;
> -		doorbell_ack_write = pcct2_ss->ack_write_mask;
> -
> -		ret = read_register(pchan->plat_irq_ack_vaddr,
> -				    &doorbell_ack_val, doorbell_ack->bit_width);
> -		if (ret)
> -			return IRQ_NONE;
> -
> -		ret = write_register(pchan->plat_irq_ack_vaddr,
> -				     (doorbell_ack_val & doorbell_ack_preserve)
> -					| doorbell_ack_write,
> -				     doorbell_ack->bit_width);
> -		if (ret)
> -			return IRQ_NONE;
> -	}
> +	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
> +		return IRQ_NONE;
>  
>  	return IRQ_HANDLED;
>  }
> @@ -376,42 +351,9 @@ EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>   */
>  static int pcc_send_data(struct mbox_chan *chan, void *data)
>  {
> -	struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
> -	struct acpi_generic_address *doorbell;
> -	struct pcc_chan_info *pchan;
> -	u64 doorbell_preserve;
> -	u64 doorbell_val;
> -	u64 doorbell_write;
> -	u32 id = chan - pcc_mbox_channels;
> -	int ret = 0;
> -
> -	if (id >= pcc_mbox_ctrl.num_chans) {
> -		pr_debug("pcc_send_data: Invalid mbox_chan passed\n");
> -		return -ENOENT;
> -	}
> +	struct pcc_chan_info *pchan = chan->con_priv;
>  
> -	pchan = chan_info + id;
> -	doorbell = &pcct_ss->doorbell_register;
> -	doorbell_preserve = pcct_ss->preserve_mask;
> -	doorbell_write = pcct_ss->write_mask;
> -
> -	/* Sync notification from OS to Platform. */
> -	if (pchan->db_vaddr) {
> -		ret = read_register(pchan->db_vaddr, &doorbell_val,
> -				    doorbell->bit_width);
> -		if (ret)
> -			return ret;
> -		ret = write_register(pchan->db_vaddr,
> -				     (doorbell_val & doorbell_preserve)
> -				      | doorbell_write, doorbell->bit_width);
> -	} else {
> -		ret = acpi_read(&doorbell_val, doorbell);
> -		if (ret)
> -			return ret;
> -		ret = acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
> -			doorbell);
> -	}
> -	return ret;
> +	return pcc_chan_reg_read_modify_write(&pchan->db);
>  }
>  
>  static const struct mbox_chan_ops pcc_chan_ops = {
> @@ -479,6 +421,7 @@ pcc_chan_reg_init(struct pcc_chan_reg *reg, struct acpi_generic_address *gas,
>  static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>  				  struct acpi_subtable_header *pcct_entry)
>  {
> +	int ret = 0;
>  	struct acpi_pcct_hw_reduced *pcct_ss;
>  
>  	if (pcct_entry->type < ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
> @@ -497,16 +440,14 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>  	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
>  		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
>  
> -		pchan->plat_irq_ack_vaddr =
> -			acpi_os_ioremap(pcct2_ss->platform_ack_register.address,
> -					pcct2_ss->platform_ack_register.bit_width / 8);
> -		if (!pchan->plat_irq_ack_vaddr) {
> -			pr_err("Failed to ioremap PCC ACK register\n");
> -			return -ENOMEM;
> -		}
> +		ret = pcc_chan_reg_init(&pchan->plat_irq_ack,
> +					&pcct2_ss->platform_ack_register,
> +					pcct2_ss->ack_preserve_mask,
> +					pcct2_ss->ack_write_mask, 0,
> +					"PLAT IRQ ACK");
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -516,19 +457,20 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>   * @pcct_entry: Pointer to the ACPI subtable header.
>   *
>   */
> -static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> -				      struct acpi_subtable_header *pcct_entry)
> +static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> +				     struct acpi_subtable_header *pcct_entry)
>  {
> +	int ret = 0;
> +
>  	struct acpi_pcct_subspace *pcct_ss;
> -	struct acpi_generic_address *db_reg;
>  
>  	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
>  
> -	/* If doorbell is in system memory cache the virt address */
> -	db_reg = &pcct_ss->doorbell_register;
> -	if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> -		pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
> -						  db_reg->bit_width / 8);
> +	ret = pcc_chan_reg_init(&pchan->db,
> +				&pcct_ss->doorbell_register,
> +				pcct_ss->preserve_mask,
> +				pcct_ss->write_mask, 0,	"Doorbell");
> +	return ret;
>  }
>  
>  /**
> @@ -617,8 +559,8 @@ static int __init acpi_pcc_probe(void)
>  
>  	for (i = 0; i < count; i++) {
>  		struct pcc_chan_info *pchan = chan_info + i;
> -		pcc_mbox_channels[i].con_priv = pcct_entry;
>  
> +		pcc_mbox_channels[i].con_priv = pchan;
>  		pchan->chan.mchan = &pcc_mbox_channels[i];
>  
>  		if (pcc_mbox_ctrl.txdone_irq) {
> @@ -626,7 +568,9 @@ static int __init acpi_pcc_probe(void)
>  			if (rc < 0)
>  				goto err;
>  		}
> -		pcc_parse_subspace_db_reg(pchan, pcct_entry);
> +		rc = pcc_parse_subspace_db_reg(pchan, pcct_entry);
> +		if (rc < 0)
> +			goto err;
>  
>  		pcc_parse_subspace_shmem(pchan, pcct_entry);
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 12/13] mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4)
  2021-07-08 18:08 ` [PATCH 12/13] mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4) Sudeep Holla
@ 2021-07-14 18:52   ` Cristian Marussi
  2021-07-15 11:31     ` Sudeep Holla
  0 siblings, 1 reply; 32+ messages in thread
From: Cristian Marussi @ 2021-07-14 18:52 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 08, 2021 at 07:08:50PM +0100, Sudeep Holla wrote:
> With all the plumbing in place to avoid accessing PCCT type and other
> fields directly from the PCCT table all the time, let us now add the
> support for extended PCC subspaces(type 3 and 4).
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Hi Sudeep,

just a few general observation on Type 3/4 subspaces from the spec
Table 14.7:

- "If a slave-subspace is present in the PCCT, then the platform interrupt flag must be set to 1." table 14.7

  Maybe is worth to WARN on this if this assumption is violated by the
  ACPI table we found.

- "Note that if interrupts are edge triggered, then each subspace must have its own unique interrupt."

  Same here, maybe it's worth also to check this, so that after all the
  pchan->db_irq has been obtained no edge triggered irqs are duplicated
  before requesting them.

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info
  2021-07-14 18:18   ` Cristian Marussi
@ 2021-07-15 11:22     ` Sudeep Holla
  0 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-15 11:22 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Sudeep Holla, Jassi Brar

On Wed, Jul 14, 2021 at 07:18:43PM +0100, Cristian Marussi wrote:
> On Thu, Jul 08, 2021 at 07:08:44PM +0100, Sudeep Holla wrote:
> > Currently PCC mailbox controller sets con_priv in each channel to hold
> > the pointer to pcct subspace entry it corresponds to. The mailbox uses
> 
> nit: s/uses/users
> 
> > will then fetch this pointer from the channel descriptor they get when
> > they request for the channel. Using that pointer they then parse the
> > pcct entry again to fetch all the information about shared memory region.
> > 
> > In order to remove individual users of PCC mailbox parsing the PCCT
> > subspace entries to fetch same information, let us consolidate the same
> > in pcc mailbox controller by parsing all the shared memory region
> > information into a structure that can also hold the mbox_chan pointer it
> > represent.
> > 
> > This can then be used as main PCC mailbox channel pointer that we can
> > return as part of pcc_mbox_request_channel instead of standard mailbox
> > channel pointer.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/mailbox/pcc.c | 27 +++++++++++++++++++++++++++
> >  include/acpi/pcc.h    |  9 +++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > index 5f19bee71c04..affde0995d52 100644
> > --- a/drivers/mailbox/pcc.c
> > +++ b/drivers/mailbox/pcc.c
> > @@ -67,11 +67,13 @@ static struct mbox_chan *pcc_mbox_channels;
> >  /**
> >   * struct pcc_chan_info - PCC channel specific information
> >   *
> > + * @chan: PCC channel information with Shared Memory Region info
> >   * @db_vaddr: cached virtual address for doorbell register
> >   * @db_ack_vaddr: cached virtual address for doorbell ack register
> >   * @db_irq: doorbell interrupt
> >   */
> >  struct pcc_chan_info {
> > +	struct pcc_mbox_chan chan;
> >  	void __iomem *db_vaddr;
> >  	void __iomem *db_ack_vaddr;
> >  	int db_irq;
> > @@ -469,6 +471,27 @@ static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> >  						  db_reg->bit_width / 8);
> >  }
> >  
> > +/**
> > + * pcc_parse_subspace_shmem - Parse the PCC Shared Memory Region information
> > + *
> > + * @pchan: Pointer to the PCC channel info structure.
> > + * @pcct_entry: Pointer to the ACPI subtable header.
> > + *
> > + */
> > +static void pcc_parse_subspace_shmem(struct pcc_chan_info *pchan,
> > +				     struct acpi_subtable_header *pcct_entry)
> > +{
> > +	struct acpi_pcct_subspace *pcct_ss;
> > +
> > +	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
> > +
> > +	pchan->chan.shmem_base_addr = pcct_ss->base_address;
> > +	pchan->chan.shmem_size = pcct_ss->length;
> > +	pchan->chan.latency = pcct_ss->latency;
> > +	pchan->chan.max_access_rate = pcct_ss->max_access_rate;
> > +	pchan->chan.min_turnaround_time = pcct_ss->min_turnaround_time;
> > +}
> > +
>
> Out of curiosity this ACPI provided latency/max_access/turnaround_time
> are supposed to be considered and/or enforced where ? by the clients
> using this controller ?
>

Ideally enforced, if not at-least considered by the clients using this
PCC channel. I do agree it results in duplication and at the discretion
of the clients. I am thinking if we can provide some kind of helpers/
accessors to achieve that. Very rough idea still as mentioned in the
cover letter, not looked at all users in details. Some drivers map the
shared memory as normal/cached while majority expects it to be device/IO.
That is one difference to consider while we try to consolidate shmem
accesses. Happy to hear opinions here 😄.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure
  2021-07-14 16:54   ` Cristian Marussi
@ 2021-07-15 11:27     ` Sudeep Holla
  2021-07-15 12:50       ` Cristian Marussi
  0 siblings, 1 reply; 32+ messages in thread
From: Sudeep Holla @ 2021-07-15 11:27 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Sudeep Holla, Jassi Brar

On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote:
> On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote:
> > Currently all the PCC channel specific information are stored/maintained
> > in global individual arrays for each of those information. It is not
> > scalable and not clean if we have to stash more channel specific
> > information. Couple of reasons to stash more information are to extend
> > the support to Type 3/4 PCCT subspace and also to avoid accessing the
> > PCCT table entries themselves each time we need the information.
> > 
> > This patch moves all those PCC channel specific information into a
> > separate structure pcc_chan_info.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> 
> Hi Sudeep,
> 
> >  drivers/mailbox/pcc.c | 106 +++++++++++++++++++++---------------------
> >  1 file changed, 53 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > index 23391e224a68..c5f481a615b0 100644
> > --- a/drivers/mailbox/pcc.c
> > +++ b/drivers/mailbox/pcc.c
> > @@ -64,12 +64,20 @@
> >  
> >  static struct mbox_chan *pcc_mbox_channels;
> >  
> > -/* Array of cached virtual address for doorbell registers */
> > -static void __iomem **pcc_doorbell_vaddr;
> > -/* Array of cached virtual address for doorbell ack registers */
> > -static void __iomem **pcc_doorbell_ack_vaddr;
> > -/* Array of doorbell interrupts */
> > -static int *pcc_doorbell_irq;
> > +/**
> > + * struct pcc_chan_info - PCC channel specific information
> > + *
> > + * @db_vaddr: cached virtual address for doorbell register
> > + * @db_ack_vaddr: cached virtual address for doorbell ack register
> > + * @db_irq: doorbell interrupt
> > + */
> > +struct pcc_chan_info {
> > +	void __iomem *db_vaddr;
> > +	void __iomem *db_ack_vaddr;
> > +	int db_irq;
> > +};
>
> Given that this db_irq represents the optional completion interrupt that is
> used platform-->OSPM to signal command completions and/or notifications/
> delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform
> Interrupt" and also referred in this driver as such somewherelse, is it not
> misleading to call it then here "doorbell interrupt" since the "doorbell" in
> this context is usually the interrupt that goes the other way around
> OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell
> registers ? (that are indeed called Doorbell throughout this driver down below
> ...but I understand this was the nomenclature used also before in this driver)
>

Exactly, I share your thoughts and I completely agree. I didn't want to change
it in this patch as that would mix 2 different change and makes it hard to
review. I assume you might have already seen the 8/13 which renames this
before we add more such registers in later patches.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 12/13] mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4)
  2021-07-14 18:52   ` Cristian Marussi
@ 2021-07-15 11:31     ` Sudeep Holla
  0 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-15 11:31 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Sudeep Holla, Jassi Brar

On Wed, Jul 14, 2021 at 07:52:16PM +0100, Cristian Marussi wrote:
> On Thu, Jul 08, 2021 at 07:08:50PM +0100, Sudeep Holla wrote:
> > With all the plumbing in place to avoid accessing PCCT type and other
> > fields directly from the PCCT table all the time, let us now add the
> > support for extended PCC subspaces(type 3 and 4).
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
>
> Hi Sudeep,
>
> just a few general observation on Type 3/4 subspaces from the spec
> Table 14.7:
>
> - "If a slave-subspace is present in the PCCT, then the platform interrupt flag must be set to 1." table 14.7
>
>   Maybe is worth to WARN on this if this assumption is violated by the
>   ACPI table we found.
>
> - "Note that if interrupts are edge triggered, then each subspace must have its own unique interrupt."
>
>   Same here, maybe it's worth also to check this, so that after all the
>   pchan->db_irq has been obtained no edge triggered irqs are duplicated
>   before requesting them.
>

Both valid points and in my TODO list, just forgot to address this as I
didn't have a setup to check/flag these up. I will address them in next
version.

Thanks for reviewing these patches.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure
  2021-07-15 11:27     ` Sudeep Holla
@ 2021-07-15 12:50       ` Cristian Marussi
  2021-07-15 13:24         ` Sudeep Holla
  0 siblings, 1 reply; 32+ messages in thread
From: Cristian Marussi @ 2021-07-15 12:50 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 15, 2021 at 12:27:10PM +0100, Sudeep Holla wrote:
> On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote:
> > On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote:
> > > Currently all the PCC channel specific information are stored/maintained
> > > in global individual arrays for each of those information. It is not
> > > scalable and not clean if we have to stash more channel specific
> > > information. Couple of reasons to stash more information are to extend
> > > the support to Type 3/4 PCCT subspace and also to avoid accessing the
> > > PCCT table entries themselves each time we need the information.
> > > 
> > > This patch moves all those PCC channel specific information into a
> > > separate structure pcc_chan_info.
> > > 
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > 
> > Hi Sudeep,
> > 
> > >  drivers/mailbox/pcc.c | 106 +++++++++++++++++++++---------------------
> > >  1 file changed, 53 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > index 23391e224a68..c5f481a615b0 100644
> > > --- a/drivers/mailbox/pcc.c
> > > +++ b/drivers/mailbox/pcc.c
> > > @@ -64,12 +64,20 @@
> > >  
> > >  static struct mbox_chan *pcc_mbox_channels;
> > >  
> > > -/* Array of cached virtual address for doorbell registers */
> > > -static void __iomem **pcc_doorbell_vaddr;
> > > -/* Array of cached virtual address for doorbell ack registers */
> > > -static void __iomem **pcc_doorbell_ack_vaddr;
> > > -/* Array of doorbell interrupts */
> > > -static int *pcc_doorbell_irq;
> > > +/**
> > > + * struct pcc_chan_info - PCC channel specific information
> > > + *
> > > + * @db_vaddr: cached virtual address for doorbell register
> > > + * @db_ack_vaddr: cached virtual address for doorbell ack register
> > > + * @db_irq: doorbell interrupt
> > > + */
> > > +struct pcc_chan_info {
> > > +	void __iomem *db_vaddr;
> > > +	void __iomem *db_ack_vaddr;
> > > +	int db_irq;
> > > +};
> >
> > Given that this db_irq represents the optional completion interrupt that is
> > used platform-->OSPM to signal command completions and/or notifications/
> > delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform
> > Interrupt" and also referred in this driver as such somewherelse, is it not
> > misleading to call it then here "doorbell interrupt" since the "doorbell" in
> > this context is usually the interrupt that goes the other way around
> > OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell
> > registers ? (that are indeed called Doorbell throughout this driver down below
> > ...but I understand this was the nomenclature used also before in this driver)
> >
> 
> Exactly, I share your thoughts and I completely agree. I didn't want to change
> it in this patch as that would mix 2 different change and makes it hard to
> review. I assume you might have already seen the 8/13 which renames this
> before we add more such registers in later patches.
> 

Yes indeed, but db_irq is not renamed even later when db_vaddr/db_ack_addr are
consolidated and renamed. So that confused me even more :D

Thanks,
Cristian
> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure
  2021-07-15 12:50       ` Cristian Marussi
@ 2021-07-15 13:24         ` Sudeep Holla
  0 siblings, 0 replies; 32+ messages in thread
From: Sudeep Holla @ 2021-07-15 13:24 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Sudeep Holla, Jassi Brar

On Thu, Jul 15, 2021 at 01:50:48PM +0100, Cristian Marussi wrote:
> On Thu, Jul 15, 2021 at 12:27:10PM +0100, Sudeep Holla wrote:
> > On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote:
> > > On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote:
> > > > Currently all the PCC channel specific information are stored/maintained
> > > > in global individual arrays for each of those information. It is not
> > > > scalable and not clean if we have to stash more channel specific
> > > > information. Couple of reasons to stash more information are to extend
> > > > the support to Type 3/4 PCCT subspace and also to avoid accessing the
> > > > PCCT table entries themselves each time we need the information.
> > > >
> > > > This patch moves all those PCC channel specific information into a
> > > > separate structure pcc_chan_info.
> > > >
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > >
> > > Hi Sudeep,
> > >
> > > >  drivers/mailbox/pcc.c | 106 +++++++++++++++++++++---------------------
> > > >  1 file changed, 53 insertions(+), 53 deletions(-)
> > > >
> > > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > > index 23391e224a68..c5f481a615b0 100644
> > > > --- a/drivers/mailbox/pcc.c
> > > > +++ b/drivers/mailbox/pcc.c
> > > > @@ -64,12 +64,20 @@
> > > >
> > > >  static struct mbox_chan *pcc_mbox_channels;
> > > >
> > > > -/* Array of cached virtual address for doorbell registers */
> > > > -static void __iomem **pcc_doorbell_vaddr;
> > > > -/* Array of cached virtual address for doorbell ack registers */
> > > > -static void __iomem **pcc_doorbell_ack_vaddr;
> > > > -/* Array of doorbell interrupts */
> > > > -static int *pcc_doorbell_irq;
> > > > +/**
> > > > + * struct pcc_chan_info - PCC channel specific information
> > > > + *
> > > > + * @db_vaddr: cached virtual address for doorbell register
> > > > + * @db_ack_vaddr: cached virtual address for doorbell ack register
> > > > + * @db_irq: doorbell interrupt
> > > > + */
> > > > +struct pcc_chan_info {
> > > > +	void __iomem *db_vaddr;
> > > > +	void __iomem *db_ack_vaddr;
> > > > +	int db_irq;
> > > > +};
> > >
> > > Given that this db_irq represents the optional completion interrupt that is
> > > used platform-->OSPM to signal command completions and/or notifications/
> > > delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform
> > > Interrupt" and also referred in this driver as such somewherelse, is it not
> > > misleading to call it then here "doorbell interrupt" since the "doorbell" in
> > > this context is usually the interrupt that goes the other way around
> > > OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell
> > > registers ? (that are indeed called Doorbell throughout this driver down below
> > > ...but I understand this was the nomenclature used also before in this driver)
> > >
> >
> > Exactly, I share your thoughts and I completely agree. I didn't want to change
> > it in this patch as that would mix 2 different change and makes it hard to
> > review. I assume you might have already seen the 8/13 which renames this
> > before we add more such registers in later patches.
> >
>
> Yes indeed, but db_irq is not renamed even later when db_vaddr/db_ack_addr are
> consolidated and renamed. So that confused me even more :D
>

Yikes! That was not intentional. Since I re-ordered some of the changes
after making them originally, I relied on regex to get it right and ease
the rebasing which I know I failed terribly. I will fix that too.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq
  2021-07-08 18:08 ` [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq Sudeep Holla
  2021-07-14 18:37   ` Cristian Marussi
@ 2021-07-15 13:38   ` Cristian Marussi
  1 sibling, 0 replies; 32+ messages in thread
From: Cristian Marussi @ 2021-07-15 13:38 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-acpi, linux-kernel, Rafael J . Wysocki, Jassi Brar

On Thu, Jul 08, 2021 at 07:08:48PM +0100, Sudeep Holla wrote:
> Now that the con_priv is availvale solely for PCC mailbox controller
> driver, let us use the same to save the channel specific information
> in it so that we can it whenever required instead of parsing the PCCT
> table entries every time in both pcc_send_data and pcc_mbox_irq.
> 
> We can now use the newly introduces PCC register bundle to simplify both
> saving of channel specific information and accessing them without repeated
> checks for the subspace type.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---o
[snip]
>

Sorry missed this small doxygen break along the way early:

>  /**
> @@ -516,19 +457,20 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>   * @pcct_entry: Pointer to the ACPI subtable header.
>   *
>   */
> -static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> -				      struct acpi_subtable_header *pcct_entry)
> +static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> +				     struct acpi_subtable_header *pcct_entry)
>  {
> +	int ret = 0;
> +


drivers/mailbox/pcc.c:485: warning: No description found for return value of 'pcc_parse_subspace_db_reg'

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2021-07-15 13:38 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08 18:08 [PATCH 00/13] mailbox: pcc: Add support for PCCT extended PCC subspaces Sudeep Holla
2021-07-08 18:08 ` [PATCH 01/13] mailbox: pcc: Fix doxygen comments Sudeep Holla
2021-07-08 18:51   ` Joe Perches
2021-07-08 21:03     ` Sudeep Holla
2021-07-08 18:08 ` [PATCH 02/13] ACPI: CPPC: " Sudeep Holla
2021-07-14 12:20   ` Rafael J. Wysocki
2021-07-14 15:12     ` Sudeep Holla
2021-07-14 16:07       ` Cristian Marussi
2021-07-14 16:14         ` Sudeep Holla
2021-07-08 18:08 ` [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure Sudeep Holla
2021-07-14 16:54   ` Cristian Marussi
2021-07-15 11:27     ` Sudeep Holla
2021-07-15 12:50       ` Cristian Marussi
2021-07-15 13:24         ` Sudeep Holla
2021-07-08 18:08 ` [PATCH 04/13] mailbox: pcc: Consolidate subspace interrupt information parsing Sudeep Holla
2021-07-14 17:44   ` Cristian Marussi
2021-07-08 18:08 ` [PATCH 05/13] mailbox: pcc: Consolidate subspace doorbell register parsing Sudeep Holla
2021-07-14 18:09   ` Cristian Marussi
2021-07-08 18:08 ` [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info Sudeep Holla
2021-07-14 18:18   ` Cristian Marussi
2021-07-15 11:22     ` Sudeep Holla
2021-07-08 18:08 ` [PATCH 07/13] mailbox: pcc: Use PCC mailbox channel pointer instead of standard Sudeep Holla
2021-07-08 18:08 ` [PATCH 08/13] mailbox: pcc: Rename doorbell ack to platform interrupt ack register Sudeep Holla
2021-07-08 18:08 ` [PATCH 09/13] mailbox: pcc: Add PCC register bundle and associated accessor functions Sudeep Holla
2021-07-08 18:08 ` [PATCH 10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq Sudeep Holla
2021-07-14 18:37   ` Cristian Marussi
2021-07-15 13:38   ` Cristian Marussi
2021-07-08 18:08 ` [PATCH 11/13] mailbox: pcc: Drop handling invalid bit-width in {read,write}_register Sudeep Holla
2021-07-08 18:08 ` [PATCH 12/13] mailbox: pcc: Add support for PCCT extended PCC subspaces(type 3/4) Sudeep Holla
2021-07-14 18:52   ` Cristian Marussi
2021-07-15 11:31     ` Sudeep Holla
2021-07-08 18:08 ` [PATCH 13/13] mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).