[PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8

Jishnu Prakash posted 3 patches 3 months, 2 weeks ago
There is a newer version of this series
[PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8
Posted by Jishnu Prakash 3 months, 2 weeks ago
From: David Collins <david.collins@oss.qualcomm.com>

PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
peripherals.  Its register map differs from v7 as several fields
increased in size. Add support for PMIC arbiter version 8.

Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
---
 drivers/spmi/spmi-pmic-arb.c | 324 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 294 insertions(+), 30 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 91581974ef84..612736973e4b 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2012-2015, 2017, 2021, The Linux Foundation. All rights reserved.
  */
 #include <linux/bitmap.h>
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -25,10 +26,12 @@
 #define PMIC_ARB_VERSION_V3_MIN		0x30000000
 #define PMIC_ARB_VERSION_V5_MIN		0x50000000
 #define PMIC_ARB_VERSION_V7_MIN		0x70000000
+#define PMIC_ARB_VERSION_V8_MIN		0x80000000
 #define PMIC_ARB_INT_EN			0x0004
 
 #define PMIC_ARB_FEATURES		0x0004
 #define PMIC_ARB_FEATURES_PERIPH_MASK	GENMASK(10, 0)
+#define PMIC_ARB_FEATURES_V8_PERIPH_MASK	GENMASK(12, 0)
 
 #define PMIC_ARB_FEATURES1		0x0008
 
@@ -50,9 +53,10 @@
 #define SPMI_MAPPING_BIT_IS_1_RESULT(X)	(((X) >> 0) & 0xFF)
 
 #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
-#define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
+#define PMIC_ARB_MAX_PPID		BIT(13)
 #define PMIC_ARB_APID_VALID		BIT(15)
 #define PMIC_ARB_CHAN_IS_IRQ_OWNER(reg)	((reg) & BIT(24))
+#define PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(reg)	((reg) & BIT(31))
 #define INVALID_EE				0xFF
 
 /* Ownership Table */
@@ -96,30 +100,33 @@ enum pmic_arb_channel {
 	PMIC_ARB_CHANNEL_OBS,
 };
 
-#define PMIC_ARB_MAX_BUSES		2
+#define PMIC_ARB_MAX_BUSES		4
+#define PMIC_ARB_MAX_BUSES_V8		4
 
 /* Maximum number of support PMIC peripherals */
 #define PMIC_ARB_MAX_PERIPHS		512
 #define PMIC_ARB_MAX_PERIPHS_V7		1024
+#define PMIC_ARB_MAX_PERIPHS_V8		8192
 #define PMIC_ARB_TIMEOUT_US		1000
 #define PMIC_ARB_MAX_TRANS_BYTES	(8)
 
 #define PMIC_ARB_APID_MASK		0xFF
 #define PMIC_ARB_PPID_MASK		0xFFF
+#define PMIC_ARB_V8_PPID_MASK		0x1FFF
 
 /* interrupt enable bit */
 #define SPMI_PIC_ACC_ENABLE_BIT		BIT(0)
 
 #define spec_to_hwirq(slave_id, periph_id, irq_id, apid) \
-	((((slave_id) & 0xF)   << 28) | \
-	(((periph_id) & 0xFF)  << 20) | \
-	(((irq_id)    & 0x7)   << 16) | \
-	(((apid)      & 0x3FF) << 0))
+	(FIELD_PREP(GENMASK(28, 24), (slave_id))  | \
+	FIELD_PREP(GENMASK(23, 16), (periph_id)) | \
+	FIELD_PREP(GENMASK(15, 13), (irq_id))    | \
+	FIELD_PREP(GENMASK(12, 0),  (apid)))
 
-#define hwirq_to_sid(hwirq)  (((hwirq) >> 28) & 0xF)
-#define hwirq_to_per(hwirq)  (((hwirq) >> 20) & 0xFF)
-#define hwirq_to_irq(hwirq)  (((hwirq) >> 16) & 0x7)
-#define hwirq_to_apid(hwirq) (((hwirq) >> 0)  & 0x3FF)
+#define hwirq_to_sid(hwirq)  FIELD_GET(GENMASK(28, 24), (hwirq))
+#define hwirq_to_per(hwirq)  FIELD_GET(GENMASK(23, 16), (hwirq))
+#define hwirq_to_irq(hwirq)  FIELD_GET(GENMASK(15, 13), (hwirq))
+#define hwirq_to_apid(hwirq) FIELD_GET(GENMASK(12, 0), (hwirq))
 
 struct pmic_arb_ver_ops;
 
@@ -138,11 +145,12 @@ struct spmi_pmic_arb;
  * @domain:		irq domain object for PMIC IRQ domain
  * @intr:		address of the SPMI interrupt control registers.
  * @cnfg:		address of the PMIC Arbiter configuration registers.
+ * @apid_owner:		on v8: address of APID owner mapping table registers
  * @spmic:		spmi controller registered for this bus
  * @lock:		lock to synchronize accesses.
- * @base_apid:		on v7: minimum APID associated with the particular SPMI
- *			bus instance
- * @apid_count:		on v5 and v7: number of APIDs associated with the
+ * @base_apid:		on v7 and v8: minimum APID associated with the
+ *			particular SPMI bus instance
+ * @apid_count:		on v5, v7 and v8: number of APIDs associated with the
  *			particular SPMI bus instance
  * @mapping_table:	in-memory copy of PPID -> APID mapping table.
  * @mapping_table_valid:bitmap containing valid-only periphs
@@ -159,6 +167,7 @@ struct spmi_pmic_arb_bus {
 	struct irq_domain	*domain;
 	void __iomem		*intr;
 	void __iomem		*cnfg;
+	void __iomem		*apid_owner;
 	struct spmi_controller	*spmic;
 	raw_spinlock_t		lock;
 	u16			base_apid;
@@ -181,6 +190,7 @@ struct spmi_pmic_arb_bus {
  * @wr_base:		on v1 "core", on v2 "chnls"    register base off DT.
  * @core:		core register base for v2 and above only (see above)
  * @core_size:		core register base size
+ * @apid_map:		on v8, APID mapping table register base
  * @channel:		execution environment channel to use for accesses.
  * @ee:			the current Execution Environment
  * @ver_ops:		version dependent operations.
@@ -193,6 +203,7 @@ struct spmi_pmic_arb {
 	void __iomem		*wr_base;
 	void __iomem		*core;
 	resource_size_t		core_size;
+	void __iomem		*apid_map;
 	u8			channel;
 	u8			ee;
 	const struct pmic_arb_ver_ops *ver_ops;
@@ -206,6 +217,7 @@ struct spmi_pmic_arb {
  *
  * @ver_str:		version string.
  * @get_core_resources:	initializes the core, observer and channels
+ * @get_bus_resources:	requests per-SPMI bus register resources
  * @init_apid:		finds the apid base and count
  * @ppid_to_apid:	finds the apid for a given ppid.
  * @non_data_cmd:	on v1 issues an spmi non-data command.
@@ -227,6 +239,9 @@ struct spmi_pmic_arb {
 struct pmic_arb_ver_ops {
 	const char *ver_str;
 	int (*get_core_resources)(struct platform_device *pdev, void __iomem *core);
+	int (*get_bus_resources)(struct platform_device *pdev,
+				 struct device_node *node,
+				 struct spmi_pmic_arb_bus *bus);
 	int (*init_apid)(struct spmi_pmic_arb_bus *bus, int index);
 	int (*ppid_to_apid)(struct spmi_pmic_arb_bus *bus, u16 ppid);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
@@ -656,7 +671,7 @@ static int periph_interrupt(struct spmi_pmic_arb_bus *bus, u16 apid)
 	unsigned int irq;
 	u32 status, id;
 	int handled = 0;
-	u8 sid = (bus->apid_data[apid].ppid >> 8) & 0xF;
+	u8 sid = (bus->apid_data[apid].ppid >> 8) & 0x1F;
 	u8 per = bus->apid_data[apid].ppid & 0xFF;
 
 	status = readl_relaxed(pmic_arb->ver_ops->irq_status(bus, apid));
@@ -686,7 +701,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 	int last = bus->max_apid;
 	/*
 	 * acc_offset will be non-zero for the secondary SPMI bus instance on
-	 * v7 controllers.
+	 * v7 and v8 controllers.
 	 */
 	int acc_offset = bus->base_apid >> 5;
 	u8 ee = pmic_arb->ee;
@@ -913,7 +928,7 @@ static int qpnpint_irq_domain_translate(struct irq_domain *d,
 		return -EINVAL;
 	if (fwspec->param_count != 4)
 		return -EINVAL;
-	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
+	if (intspec[0] > 0x1F || intspec[1] > 0xFF || intspec[2] > 0x7)
 		return -EINVAL;
 
 	ppid = intspec[0] << 8 | intspec[1];
@@ -1160,6 +1175,24 @@ static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb_bus *bus, u16 ppid)
 	return apid_valid & ~PMIC_ARB_APID_VALID;
 }
 
+static void pmic_arb_dump_apid_map(struct spmi_pmic_arb_bus *bus)
+{
+	struct apid_data *apidd;
+	u16 apid, ppid;
+
+	/* Dump the mapping table for debug purposes. */
+	dev_dbg(&bus->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
+	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
+		apid = bus->ppid_to_apid[ppid];
+		if (apid & PMIC_ARB_APID_VALID) {
+			apid &= ~PMIC_ARB_APID_VALID;
+			apidd = &bus->apid_data[apid];
+			dev_dbg(&bus->spmic->dev, "%#03X %3u %2u %2u\n",
+				ppid, apid, apidd->write_ee, apidd->irq_ee);
+		}
+	}
+}
+
 static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb_bus *bus)
 {
 	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
@@ -1222,17 +1255,7 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb_bus *bus)
 		bus->last_apid = i;
 	}
 
-	/* Dump the mapping table for debug purposes. */
-	dev_dbg(&bus->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
-	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
-		apid = bus->ppid_to_apid[ppid];
-		if (apid & PMIC_ARB_APID_VALID) {
-			apid &= ~PMIC_ARB_APID_VALID;
-			apidd = &bus->apid_data[apid];
-			dev_dbg(&bus->spmic->dev, "%#03X %3u %2u %2u\n",
-				ppid, apid, apidd->write_ee, apidd->irq_ee);
-		}
-	}
+	pmic_arb_dump_apid_map(bus);
 
 	return 0;
 }
@@ -1346,7 +1369,7 @@ static int pmic_arb_get_core_resources_v7(struct platform_device *pdev,
 }
 
 /*
- * Only v7 supports 2 buses. Each bus will get a different apid count, read
+ * Arbiter v7 supports 2 buses. Each bus will get a different apid count, read
  * from different registers.
  */
 static int pmic_arb_init_apid_v7(struct spmi_pmic_arb_bus *bus, int index)
@@ -1424,6 +1447,185 @@ static int pmic_arb_offset_v7(struct spmi_pmic_arb_bus *bus, u8 sid, u16 addr,
 	return offset;
 }
 
+static int pmic_arb_get_core_resources_v8(struct platform_device *pdev,
+					  void __iomem *core)
+{
+	struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
+
+	pmic_arb->apid_map = devm_platform_ioremap_resource_byname(pdev,
+								   "chnl_map");
+	if (IS_ERR(pmic_arb->apid_map))
+		return PTR_ERR(pmic_arb->apid_map);
+
+	pmic_arb->core = core;
+
+	pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V8;
+
+	return pmic_arb_get_obsrvr_chnls_v2(pdev);
+}
+
+static int pmic_arb_get_bus_resources_v8(struct platform_device *pdev,
+					 struct device_node *node,
+					 struct spmi_pmic_arb_bus *bus)
+{
+	int index;
+
+	index = of_property_match_string(node, "reg-names", "chnl_owner");
+	if (index < 0) {
+		dev_err(&pdev->dev, "chnl_owner reg region missing\n");
+		return -EINVAL;
+	}
+
+	bus->apid_owner = devm_of_iomap(&pdev->dev, node, index, NULL);
+
+	return PTR_ERR_OR_ZERO(bus->apid_owner);
+}
+
+static int pmic_arb_read_apid_map_v8(struct spmi_pmic_arb_bus *bus)
+{
+	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
+	struct apid_data *apidd;
+	struct apid_data *prev_apidd;
+	u16 i, apid, ppid, apid_max;
+	bool valid, is_irq_ee;
+	u32 regval, offset;
+
+	/*
+	 * In order to allow multiple EEs to write to a single PPID in arbiter
+	 * version 8, there can be more than one APID mapped to each PPID.  The
+	 * owner field for each of these mappings specifies the EE which is
+	 * allowed to write to the APID.  The owner of the last (highest) APID
+	 * which has the IRQ owner bit set for a given PPID will receive
+	 * interrupts from the PPID.
+	 *
+	 * In arbiter version 8, the APID numbering space is divided between
+	 * the SPMI buses according to this mapping:
+	 * APID = 0     to N-1       --> bus 0
+	 * APID = N     to N+M-1     --> bus 1
+	 * APID = N+M   to N+M+P-1   --> bus 2
+	 * APID = N+M+P to N+M+P+Q-1 --> bus 3
+	 * where N = number of APIDs supported by bus 0
+	 *       M = number of APIDs supported by bus 1
+	 *       P = number of APIDs supported by bus 2
+	 *       Q = number of APIDs supported by bus 3
+	 */
+	apidd = &bus->apid_data[bus->base_apid];
+	apid_max = bus->base_apid + bus->apid_count;
+	for (i = bus->base_apid; i < apid_max; i++, apidd++) {
+		offset = pmic_arb->ver_ops->apid_map_offset(i);
+		regval = readl_relaxed(pmic_arb->apid_map + offset);
+		if (!regval)
+			continue;
+		ppid = regval & PMIC_ARB_V8_PPID_MASK;
+		is_irq_ee = PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(regval);
+
+		regval = readl_relaxed(pmic_arb->ver_ops->apid_owner(bus, i));
+		apidd->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+
+		apidd->irq_ee = is_irq_ee ? apidd->write_ee : INVALID_EE;
+
+		valid = bus->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
+		apid = bus->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
+		prev_apidd = &bus->apid_data[apid];
+
+		if (!valid || apidd->write_ee == pmic_arb->ee) {
+			/* First PPID mapping or one for this EE */
+			bus->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
+		} else if (valid && is_irq_ee &&
+			   prev_apidd->write_ee == pmic_arb->ee) {
+			/*
+			 * Duplicate PPID mapping after the one for this EE;
+			 * override the irq owner
+			 */
+			prev_apidd->irq_ee = apidd->irq_ee;
+		}
+
+		apidd->ppid = ppid;
+		bus->last_apid = i;
+	}
+
+	pmic_arb_dump_apid_map(bus);
+
+	return 0;
+}
+
+static int pmic_arb_init_apid_v8(struct spmi_pmic_arb_bus *bus, int index)
+{
+	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
+	int ret, i;
+
+	if (index < 0 || index >= PMIC_ARB_MAX_BUSES_V8) {
+		dev_err(&bus->spmic->dev, "Unsupported bus index %d detected\n",
+			index);
+		return -EINVAL;
+	}
+
+	bus->base_apid = 0;
+	bus->apid_count = 0;
+	for (i = 0; i <= index; i++) {
+		bus->base_apid += bus->apid_count;
+		bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES + i * 4) &
+						PMIC_ARB_FEATURES_V8_PERIPH_MASK;
+	}
+
+	if (bus->apid_count == 0) {
+		dev_err(&bus->spmic->dev, "Bus %d not implemented\n", index);
+		return -EINVAL;
+	} else if (bus->base_apid + bus->apid_count > pmic_arb->max_periphs) {
+		dev_err(&bus->spmic->dev, "Unsupported max APID %d detected\n",
+			bus->base_apid + bus->apid_count);
+		return -EINVAL;
+	}
+
+	ret = pmic_arb_init_apid_min_max(bus);
+	if (ret)
+		return ret;
+
+	ret = pmic_arb_read_apid_map_v8(bus);
+	if (ret) {
+		dev_err(&bus->spmic->dev, "could not read APID->PPID mapping table, rc= %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * v8 offset per ee and per apid for observer channels and per apid for
+ * read/write channels.
+ */
+static int pmic_arb_offset_v8(struct spmi_pmic_arb_bus *bus, u8 sid, u16 addr,
+			      enum pmic_arb_channel ch_type)
+{
+	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
+	u16 apid;
+	int rc;
+	u32 offset = 0;
+	u16 ppid = (sid << 8) | (addr >> 8);
+
+	rc = pmic_arb->ver_ops->ppid_to_apid(bus, ppid);
+	if (rc < 0)
+		return rc;
+
+	apid = rc;
+	switch (ch_type) {
+	case PMIC_ARB_CHANNEL_OBS:
+		offset = 0x40000 * pmic_arb->ee + 0x20 * apid;
+		break;
+	case PMIC_ARB_CHANNEL_RW:
+		if (bus->apid_data[apid].write_ee != pmic_arb->ee) {
+			dev_err(&bus->spmic->dev, "disallowed SPMI write to sid=%u, addr=0x%04X\n",
+				sid, addr);
+			return -EPERM;
+		}
+		offset = 0x200 * apid;
+		break;
+	}
+
+	return offset;
+}
+
 static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc)
 {
 	return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
@@ -1490,6 +1692,14 @@ pmic_arb_acc_enable_v7(struct spmi_pmic_arb_bus *bus, u16 n)
 	return pmic_arb->wr_base + 0x100 + 0x1000 * n;
 }
 
+static void __iomem *
+pmic_arb_acc_enable_v8(struct spmi_pmic_arb_bus *bus, u16 n)
+{
+	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
+
+	return pmic_arb->wr_base + 0x100 + 0x200 * n;
+}
+
 static void __iomem *
 pmic_arb_irq_status_v1(struct spmi_pmic_arb_bus *bus, u16 n)
 {
@@ -1516,6 +1726,14 @@ pmic_arb_irq_status_v7(struct spmi_pmic_arb_bus *bus, u16 n)
 	return pmic_arb->wr_base + 0x104 + 0x1000 * n;
 }
 
+static void __iomem *
+pmic_arb_irq_status_v8(struct spmi_pmic_arb_bus *bus, u16 n)
+{
+	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
+
+	return pmic_arb->wr_base + 0x104 + 0x200 * n;
+}
+
 static void __iomem *
 pmic_arb_irq_clear_v1(struct spmi_pmic_arb_bus *bus, u16 n)
 {
@@ -1542,6 +1760,14 @@ pmic_arb_irq_clear_v7(struct spmi_pmic_arb_bus *bus, u16 n)
 	return pmic_arb->wr_base + 0x108 + 0x1000 * n;
 }
 
+static void __iomem *
+pmic_arb_irq_clear_v8(struct spmi_pmic_arb_bus *bus, u16 n)
+{
+	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
+
+	return pmic_arb->wr_base + 0x108 + 0x200 * n;
+}
+
 static u32 pmic_arb_apid_map_offset_v2(u16 n)
 {
 	return 0x800 + 0x4 * n;
@@ -1557,6 +1783,12 @@ static u32 pmic_arb_apid_map_offset_v7(u16 n)
 	return 0x2000 + 0x4 * n;
 }
 
+static u32 pmic_arb_apid_map_offset_v8(u16 n)
+{
+	/* For v8, offset is from "chnl_map" base register, not "core". */
+	return 0x4 * n;
+}
+
 static void __iomem *
 pmic_arb_apid_owner_v2(struct spmi_pmic_arb_bus *bus, u16 n)
 {
@@ -1564,7 +1796,7 @@ pmic_arb_apid_owner_v2(struct spmi_pmic_arb_bus *bus, u16 n)
 }
 
 /*
- * For arbiter version 7, APID ownership table registers have independent
+ * For arbiter version 7 and 8, APID ownership table registers have independent
  * numbering space for each SPMI bus instance, so each is indexed starting from
  * 0.
  */
@@ -1574,6 +1806,12 @@ pmic_arb_apid_owner_v7(struct spmi_pmic_arb_bus *bus, u16 n)
 	return bus->cnfg + 0x4 * (n - bus->base_apid);
 }
 
+static void __iomem *
+pmic_arb_apid_owner_v8(struct spmi_pmic_arb_bus *bus, u16 n)
+{
+	return bus->apid_owner + 0x4 * (n - bus->base_apid);
+}
+
 static const struct pmic_arb_ver_ops pmic_arb_v1 = {
 	.ver_str		= "v1",
 	.get_core_resources	= pmic_arb_get_core_resources_v1,
@@ -1654,6 +1892,23 @@ static const struct pmic_arb_ver_ops pmic_arb_v7 = {
 	.apid_owner		= pmic_arb_apid_owner_v7,
 };
 
+static const struct pmic_arb_ver_ops pmic_arb_v8 = {
+	.ver_str		= "v8",
+	.get_core_resources	= pmic_arb_get_core_resources_v8,
+	.get_bus_resources	= pmic_arb_get_bus_resources_v8,
+	.init_apid		= pmic_arb_init_apid_v8,
+	.ppid_to_apid		= pmic_arb_ppid_to_apid_v5,
+	.non_data_cmd		= pmic_arb_non_data_cmd_v2,
+	.offset			= pmic_arb_offset_v8,
+	.fmt_cmd		= pmic_arb_fmt_cmd_v2,
+	.owner_acc_status	= pmic_arb_owner_acc_status_v7,
+	.acc_enable		= pmic_arb_acc_enable_v8,
+	.irq_status		= pmic_arb_irq_status_v8,
+	.irq_clear		= pmic_arb_irq_clear_v8,
+	.apid_map_offset	= pmic_arb_apid_map_offset_v8,
+	.apid_owner		= pmic_arb_apid_owner_v8,
+};
+
 static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
 	.activate = qpnpint_irq_domain_activate,
 	.alloc = qpnpint_irq_domain_alloc,
@@ -1731,6 +1986,12 @@ static int spmi_pmic_arb_bus_init(struct platform_device *pdev,
 	bus->spmic = ctrl;
 	bus->id = bus_index;
 
+	if (pmic_arb->ver_ops->get_bus_resources) {
+		ret = pmic_arb->ver_ops->get_bus_resources(pdev, node, bus);
+		if (ret)
+			return ret;
+	}
+
 	ret = pmic_arb->ver_ops->init_apid(bus, bus_index);
 	if (ret)
 		return ret;
@@ -1825,8 +2086,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		pmic_arb->ver_ops = &pmic_arb_v3;
 	else if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
 		pmic_arb->ver_ops = &pmic_arb_v5;
-	else
+	else if (hw_ver < PMIC_ARB_VERSION_V8_MIN)
 		pmic_arb->ver_ops = &pmic_arb_v7;
+	else
+		pmic_arb->ver_ops = &pmic_arb_v8;
 
 	err = pmic_arb->ver_ops->get_core_resources(pdev, core);
 	if (err)
@@ -1875,6 +2138,7 @@ static void spmi_pmic_arb_remove(struct platform_device *pdev)
 static const struct of_device_id spmi_pmic_arb_match_table[] = {
 	{ .compatible = "qcom,spmi-pmic-arb", },
 	{ .compatible = "qcom,x1e80100-spmi-pmic-arb", },
+	{ .compatible = "qcom,glymur-spmi-pmic-arb", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, spmi_pmic_arb_match_table);

-- 
2.25.1
Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8
Posted by Konrad Dybcio 3 months, 2 weeks ago
On 10/24/25 11:33 AM, Jishnu Prakash wrote:
> From: David Collins <david.collins@oss.qualcomm.com>
> 
> PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
> peripherals.  Its register map differs from v7 as several fields
> increased in size. Add support for PMIC arbiter version 8.
> 
> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 324 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 294 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 91581974ef84..612736973e4b 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2012-2015, 2017, 2021, The Linux Foundation. All rights reserved.
>   */
>  #include <linux/bitmap.h>
> +#include <linux/bitfield.h>

bit'f'ield < bit'm'ap

[...]

>  #define spec_to_hwirq(slave_id, periph_id, irq_id, apid) \
> -	((((slave_id) & 0xF)   << 28) | \
> -	(((periph_id) & 0xFF)  << 20) | \
> -	(((irq_id)    & 0x7)   << 16) | \
> -	(((apid)      & 0x3FF) << 0))
> +	(FIELD_PREP(GENMASK(28, 24), (slave_id))  | \
> +	FIELD_PREP(GENMASK(23, 16), (periph_id)) | \
> +	FIELD_PREP(GENMASK(15, 13), (irq_id))    | \
> +	FIELD_PREP(GENMASK(12, 0),  (apid)))

I think this could be further improved by:

#define SOMETHING_SLAVE_ID_SOMETHING	GENMASK(28, 24)

and then below:

[...]

> -	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
> +	if (intspec[0] > 0x1F || intspec[1] > 0xFF || intspec[2] > 0x7)
>  		return -EINVAL;

we can use FIELD_MAX(SOMETHING...)

[...]

> +static int pmic_arb_get_core_resources_v8(struct platform_device *pdev,
> +					  void __iomem *core)
> +{
> +	struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
> +
> +	pmic_arb->apid_map = devm_platform_ioremap_resource_byname(pdev,
> +								   "chnl_map");

Feel free to unwrap this line

> +	if (IS_ERR(pmic_arb->apid_map))
> +		return PTR_ERR(pmic_arb->apid_map);
> +
> +	pmic_arb->core = core;
> +
> +	pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V8;
> +
> +	return pmic_arb_get_obsrvr_chnls_v2(pdev);
> +}
> +
> +static int pmic_arb_get_bus_resources_v8(struct platform_device *pdev,
> +					 struct device_node *node,
> +					 struct spmi_pmic_arb_bus *bus)
> +{
> +	int index;
> +
> +	index = of_property_match_string(node, "reg-names", "chnl_owner");
> +	if (index < 0) {
> +		dev_err(&pdev->dev, "chnl_owner reg region missing\n");
> +		return -EINVAL;
> +	}
> +
> +	bus->apid_owner = devm_of_iomap(&pdev->dev, node, index, NULL);
> +
> +	return PTR_ERR_OR_ZERO(bus->apid_owner);

Is this any different chan using devm_platform_ioremap_resource_byname()
like you did above?


> +}
> +
> +static int pmic_arb_read_apid_map_v8(struct spmi_pmic_arb_bus *bus)
> +{
> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
> +	struct apid_data *apidd;
> +	struct apid_data *prev_apidd;
> +	u16 i, apid, ppid, apid_max;
> +	bool valid, is_irq_ee;
> +	u32 regval, offset;
> +
> +	/*
> +	 * In order to allow multiple EEs to write to a single PPID in arbiter
> +	 * version 8, there can be more than one APID mapped to each PPID.  The
> +	 * owner field for each of these mappings specifies the EE which is
> +	 * allowed to write to the APID.  The owner of the last (highest) APID
> +	 * which has the IRQ owner bit set for a given PPID will receive
> +	 * interrupts from the PPID.
> +	 *
> +	 * In arbiter version 8, the APID numbering space is divided between
> +	 * the SPMI buses according to this mapping:
> +	 * APID = 0     to N-1       --> bus 0
> +	 * APID = N     to N+M-1     --> bus 1
> +	 * APID = N+M   to N+M+P-1   --> bus 2
> +	 * APID = N+M+P to N+M+P+Q-1 --> bus 3
> +	 * where N = number of APIDs supported by bus 0
> +	 *       M = number of APIDs supported by bus 1
> +	 *       P = number of APIDs supported by bus 2
> +	 *       Q = number of APIDs supported by bus 3
> +	 */
> +	apidd = &bus->apid_data[bus->base_apid];
> +	apid_max = bus->base_apid + bus->apid_count;
> +	for (i = bus->base_apid; i < apid_max; i++, apidd++) {
> +		offset = pmic_arb->ver_ops->apid_map_offset(i);
> +		regval = readl_relaxed(pmic_arb->apid_map + offset);
> +		if (!regval)
> +			continue;
> +		ppid = regval & PMIC_ARB_V8_PPID_MASK;
> +		is_irq_ee = PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(regval);

[...]


If you parametrize the masks, the diff against pmic_arb_read_apid_map_v5
is 3 lines of code instead


> +
> +	return 0;
> +}
> +
> +static int pmic_arb_init_apid_v8(struct spmi_pmic_arb_bus *bus, int index)
> +{
> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
> +	int ret, i;
> +
> +	if (index < 0 || index >= PMIC_ARB_MAX_BUSES_V8) {
> +		dev_err(&bus->spmic->dev, "Unsupported bus index %d detected\n",
> +			index);
> +		return -EINVAL;
> +	}
> +
> +	bus->base_apid = 0;
> +	bus->apid_count = 0;
> +	for (i = 0; i <= index; i++) {
> +		bus->base_apid += bus->apid_count;
> +		bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES + i * 4) &
> +						PMIC_ARB_FEATURES_V8_PERIPH_MASK;

You can almost replace pmic_arb_init_apid_v7 with this impl

[...]

> +static void __iomem *
> +pmic_arb_apid_owner_v8(struct spmi_pmic_arb_bus *bus, u16 n)
> +{
> +	return bus->apid_owner + 0x4 * (n - bus->base_apid);
> +}

This is identical to pmic_arb_apid_owner_v7

Konrad
Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8
Posted by Jishnu Prakash 3 months, 1 week ago
Hi Konrad,

On 10/27/2025 5:47 PM, Konrad Dybcio wrote:
> On 10/24/25 11:33 AM, Jishnu Prakash wrote:
>> From: David Collins <david.collins@oss.qualcomm.com>
>>
>> PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
>> peripherals.  Its register map differs from v7 as several fields
>> increased in size. Add support for PMIC arbiter version 8.
>>
>> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>> ---
>>  drivers/spmi/spmi-pmic-arb.c | 324 +++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 294 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 91581974ef84..612736973e4b 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -3,6 +3,7 @@
>>   * Copyright (c) 2012-2015, 2017, 2021, The Linux Foundation. All rights reserved.
>>   */
>>  #include <linux/bitmap.h>
>> +#include <linux/bitfield.h>
> 
> bit'f'ield < bit'm'ap
> 
> [...]
> 
>>  #define spec_to_hwirq(slave_id, periph_id, irq_id, apid) \
>> -	((((slave_id) & 0xF)   << 28) | \
>> -	(((periph_id) & 0xFF)  << 20) | \
>> -	(((irq_id)    & 0x7)   << 16) | \
>> -	(((apid)      & 0x3FF) << 0))
>> +	(FIELD_PREP(GENMASK(28, 24), (slave_id))  | \
>> +	FIELD_PREP(GENMASK(23, 16), (periph_id)) | \
>> +	FIELD_PREP(GENMASK(15, 13), (irq_id))    | \
>> +	FIELD_PREP(GENMASK(12, 0),  (apid)))
> 
> I think this could be further improved by:
> 
> #define SOMETHING_SLAVE_ID_SOMETHING	GENMASK(28, 24)
> 
> and then below:
> 
> [...]
> 
>> -	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
>> +	if (intspec[0] > 0x1F || intspec[1] > 0xFF || intspec[2] > 0x7)
>>  		return -EINVAL;
> 
> we can use FIELD_MAX(SOMETHING...)
> 
> [...]
> 
>> +static int pmic_arb_get_core_resources_v8(struct platform_device *pdev,
>> +					  void __iomem *core)
>> +{
>> +	struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
>> +
>> +	pmic_arb->apid_map = devm_platform_ioremap_resource_byname(pdev,
>> +								   "chnl_map");
> 
> Feel free to unwrap this line

I'll make all the changes you have suggested above in the next series.

> 
>> +	if (IS_ERR(pmic_arb->apid_map))
>> +		return PTR_ERR(pmic_arb->apid_map);
>> +
>> +	pmic_arb->core = core;
>> +
>> +	pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V8;
>> +
>> +	return pmic_arb_get_obsrvr_chnls_v2(pdev);
>> +}
>> +
>> +static int pmic_arb_get_bus_resources_v8(struct platform_device *pdev,
>> +					 struct device_node *node,
>> +					 struct spmi_pmic_arb_bus *bus)
>> +{
>> +	int index;
>> +
>> +	index = of_property_match_string(node, "reg-names", "chnl_owner");
>> +	if (index < 0) {
>> +		dev_err(&pdev->dev, "chnl_owner reg region missing\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	bus->apid_owner = devm_of_iomap(&pdev->dev, node, index, NULL);
>> +
>> +	return PTR_ERR_OR_ZERO(bus->apid_owner);
> 
> Is this any different chan using devm_platform_ioremap_resource_byname()
> like you did above?

devm_platform_ioremap_resource_byname() would work for mapping resources
directly under the main device node, like"chnl_map" , but in this case, the
resource "chnl_owner" is under a child node of the the main node, so we
need to use devm_of_iomap() here.

> 
> 
>> +}
>> +
>> +static int pmic_arb_read_apid_map_v8(struct spmi_pmic_arb_bus *bus)
>> +{
>> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
>> +	struct apid_data *apidd;
>> +	struct apid_data *prev_apidd;
>> +	u16 i, apid, ppid, apid_max;
>> +	bool valid, is_irq_ee;
>> +	u32 regval, offset;
>> +
>> +	/*
>> +	 * In order to allow multiple EEs to write to a single PPID in arbiter
>> +	 * version 8, there can be more than one APID mapped to each PPID.  The
>> +	 * owner field for each of these mappings specifies the EE which is
>> +	 * allowed to write to the APID.  The owner of the last (highest) APID
>> +	 * which has the IRQ owner bit set for a given PPID will receive
>> +	 * interrupts from the PPID.
>> +	 *
>> +	 * In arbiter version 8, the APID numbering space is divided between
>> +	 * the SPMI buses according to this mapping:
>> +	 * APID = 0     to N-1       --> bus 0
>> +	 * APID = N     to N+M-1     --> bus 1
>> +	 * APID = N+M   to N+M+P-1   --> bus 2
>> +	 * APID = N+M+P to N+M+P+Q-1 --> bus 3
>> +	 * where N = number of APIDs supported by bus 0
>> +	 *       M = number of APIDs supported by bus 1
>> +	 *       P = number of APIDs supported by bus 2
>> +	 *       Q = number of APIDs supported by bus 3
>> +	 */
>> +	apidd = &bus->apid_data[bus->base_apid];
>> +	apid_max = bus->base_apid + bus->apid_count;
>> +	for (i = bus->base_apid; i < apid_max; i++, apidd++) {
>> +		offset = pmic_arb->ver_ops->apid_map_offset(i);
>> +		regval = readl_relaxed(pmic_arb->apid_map + offset);
>> +		if (!regval)
>> +			continue;
>> +		ppid = regval & PMIC_ARB_V8_PPID_MASK;
>> +		is_irq_ee = PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(regval);
> 
> [...]
> 
> 
> If you parametrize the masks, the diff against pmic_arb_read_apid_map_v5
> is 3 lines of code instead

Are you suggesting we should try to have a common helper function for v5/v7
and v8, which does the bulk of these actions and can be given different
input arguments to distinguish the versions?

There is at least one more difference which I don't think we can easily
accommodate with parameters:

In pmic_arb_read_apid_map_v5(), we have:
regval = readl_relaxed(pmic_arb->core + offset);

In pmic_arb_read_apid_map_v8(), we have:
regval = readl_relaxed(pmic_arb->apid_map + offset);


But in any case, we would have to add `if` checks to distinguish arbiter
versions to use a helper function. This itself might not be a good idea as it
would break the abstraction already implemented in the probe, by having
PMIC version checking happen only in the probe, to select the correct set
of ver_ops functions for a particular version.


> 
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int pmic_arb_init_apid_v8(struct spmi_pmic_arb_bus *bus, int index)
>> +{
>> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
>> +	int ret, i;
>> +
>> +	if (index < 0 || index >= PMIC_ARB_MAX_BUSES_V8) {
>> +		dev_err(&bus->spmic->dev, "Unsupported bus index %d detected\n",
>> +			index);
>> +		return -EINVAL;
>> +	}
>> +
>> +	bus->base_apid = 0;
>> +	bus->apid_count = 0;
>> +	for (i = 0; i <= index; i++) {
>> +		bus->base_apid += bus->apid_count;
>> +		bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES + i * 4) &
>> +						PMIC_ARB_FEATURES_V8_PERIPH_MASK;
> 
> You can almost replace pmic_arb_init_apid_v7 with this impl

They are not exactly the same - v7 arbiter supports at most 2 buses,
so pmic_arb_init_apid_v7 needs to restrict the max value of index to 2.

Here too, using a common helper function would require us to add more
`if` checks for the version, which leads to the same kind of issue as above.


> 
> [...]
> 
>> +static void __iomem *
>> +pmic_arb_apid_owner_v8(struct spmi_pmic_arb_bus *bus, u16 n)
>> +{
>> +	return bus->apid_owner + 0x4 * (n - bus->base_apid);
>> +}
> 
> This is identical to pmic_arb_apid_owner_v7

This is not exactly right, pmic_arb_apid_owner_v7 uses bus->cnfg
and pmic_arb_apid_owner_v8 uses bus->apid_owner at the same place.
They are both already single line functions, would it really help
to try to simplify them further?

Thanks,
Jishnu

> 
> Konrad
Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8
Posted by Konrad Dybcio 3 months, 1 week ago
On 11/1/25 3:22 AM, Jishnu Prakash wrote:
> Hi Konrad,
> 
> On 10/27/2025 5:47 PM, Konrad Dybcio wrote:
>> On 10/24/25 11:33 AM, Jishnu Prakash wrote:
>>> From: David Collins <david.collins@oss.qualcomm.com>
>>>
>>> PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
>>> peripherals.  Its register map differs from v7 as several fields
>>> increased in size. Add support for PMIC arbiter version 8.
>>>
>>> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
>>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>>> ---

[...]

>>> +static int pmic_arb_get_bus_resources_v8(struct platform_device *pdev,
>>> +					 struct device_node *node,
>>> +					 struct spmi_pmic_arb_bus *bus)
>>> +{
>>> +	int index;
>>> +
>>> +	index = of_property_match_string(node, "reg-names", "chnl_owner");
>>> +	if (index < 0) {
>>> +		dev_err(&pdev->dev, "chnl_owner reg region missing\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	bus->apid_owner = devm_of_iomap(&pdev->dev, node, index, NULL);
>>> +
>>> +	return PTR_ERR_OR_ZERO(bus->apid_owner);
>>
>> Is this any different chan using devm_platform_ioremap_resource_byname()
>> like you did above?
> 
> devm_platform_ioremap_resource_byname() would work for mapping resources
> directly under the main device node, like"chnl_map" , but in this case, the
> resource "chnl_owner" is under a child node of the the main node, so we
> need to use devm_of_iomap() here.

Oh, you're right

[...]

>>> +	apidd = &bus->apid_data[bus->base_apid];
>>> +	apid_max = bus->base_apid + bus->apid_count;
>>> +	for (i = bus->base_apid; i < apid_max; i++, apidd++) {
>>> +		offset = pmic_arb->ver_ops->apid_map_offset(i);
>>> +		regval = readl_relaxed(pmic_arb->apid_map + offset);
>>> +		if (!regval)
>>> +			continue;
>>> +		ppid = regval & PMIC_ARB_V8_PPID_MASK;
>>> +		is_irq_ee = PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(regval);
>>
>> [...]
>>
>>
>> If you parametrize the masks, the diff against pmic_arb_read_apid_map_v5
>> is 3 lines of code instead
> 
> Are you suggesting we should try to have a common helper function for v5/v7
> and v8, which does the bulk of these actions and can be given different
> input arguments to distinguish the versions?
> 
> There is at least one more difference which I don't think we can easily
> accommodate with parameters:
> 
> In pmic_arb_read_apid_map_v5(), we have:
> regval = readl_relaxed(pmic_arb->core + offset);
> 
> In pmic_arb_read_apid_map_v8(), we have:
> regval = readl_relaxed(pmic_arb->apid_map + offset);

You can make the function accept this region base as a parameter, rename
it to something like _pmic_arb_ppid_to_apid_v5()..

> But in any case, we would have to add `if` checks to distinguish arbiter
> versions to use a helper function. This itself might not be a good idea as it
> would break the abstraction already implemented in the probe, by having
> PMIC version checking happen only in the probe, to select the correct set
> of ver_ops functions for a particular version.

..and then create a much thinner pmic_arb_ppid_to_apid_v5/v8() wrappers
that will call _pmic_arb_ppid_to_apid_v5()

>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int pmic_arb_init_apid_v8(struct spmi_pmic_arb_bus *bus, int index)
>>> +{
>>> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
>>> +	int ret, i;
>>> +
>>> +	if (index < 0 || index >= PMIC_ARB_MAX_BUSES_V8) {
>>> +		dev_err(&bus->spmic->dev, "Unsupported bus index %d detected\n",
>>> +			index);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	bus->base_apid = 0;
>>> +	bus->apid_count = 0;
>>> +	for (i = 0; i <= index; i++) {
>>> +		bus->base_apid += bus->apid_count;
>>> +		bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES + i * 4) &
>>> +						PMIC_ARB_FEATURES_V8_PERIPH_MASK;
>>
>> You can almost replace pmic_arb_init_apid_v7 with this impl
> 
> They are not exactly the same - v7 arbiter supports at most 2 buses,
> so pmic_arb_init_apid_v7 needs to restrict the max value of index to 2.
> 
> Here too, using a common helper function would require us to add more
> `if` checks for the version, which leads to the same kind of issue as above.

Add pmic_arb_ver_ops.max_buses, set it to 1/2/1234 as needed. I really
don't want you to add a copy of an existing function with a single
comparison argument changed

>> [...]
>>
>>> +static void __iomem *
>>> +pmic_arb_apid_owner_v8(struct spmi_pmic_arb_bus *bus, u16 n)
>>> +{
>>> +	return bus->apid_owner + 0x4 * (n - bus->base_apid);
>>> +}
>>
>> This is identical to pmic_arb_apid_owner_v7
> 
> This is not exactly right, pmic_arb_apid_owner_v7 uses bus->cnfg
> and pmic_arb_apid_owner_v8 uses bus->apid_owner at the same place.
> They are both already single line functions, would it really help
> to try to simplify them further?

Sorry, I misread and thought they were *actually the same*, scratch
that

Konrad
Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8
Posted by Neil Armstrong 3 months, 2 weeks ago
Hi,

On 10/24/25 11:33, Jishnu Prakash wrote:
> From: David Collins <david.collins@oss.qualcomm.com>
> 
> PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
> peripherals.  Its register map differs from v7 as several fields
> increased in size. Add support for PMIC arbiter version 8.
> 
> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> ---
>   drivers/spmi/spmi-pmic-arb.c | 324 +++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 294 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 91581974ef84..612736973e4b 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -3,6 +3,7 @@
>    * Copyright (c) 2012-2015, 2017, 2021, The Linux Foundation. All rights reserved.
>    */
>   #include <linux/bitmap.h>
> +#include <linux/bitfield.h>
>   #include <linux/delay.h>
>   #include <linux/err.h>
>   #include <linux/interrupt.h>
> @@ -25,10 +26,12 @@
>   #define PMIC_ARB_VERSION_V3_MIN		0x30000000
>   #define PMIC_ARB_VERSION_V5_MIN		0x50000000
>   #define PMIC_ARB_VERSION_V7_MIN		0x70000000
> +#define PMIC_ARB_VERSION_V8_MIN		0x80000000
>   #define PMIC_ARB_INT_EN			0x0004
>   
>   #define PMIC_ARB_FEATURES		0x0004
>   #define PMIC_ARB_FEATURES_PERIPH_MASK	GENMASK(10, 0)
> +#define PMIC_ARB_FEATURES_V8_PERIPH_MASK	GENMASK(12, 0)
>   
>   #define PMIC_ARB_FEATURES1		0x0008
>   
> @@ -50,9 +53,10 @@
>   #define SPMI_MAPPING_BIT_IS_1_RESULT(X)	(((X) >> 0) & 0xFF)
>   
>   #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
> -#define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
> +#define PMIC_ARB_MAX_PPID		BIT(13)
>   #define PMIC_ARB_APID_VALID		BIT(15)
>   #define PMIC_ARB_CHAN_IS_IRQ_OWNER(reg)	((reg) & BIT(24))
> +#define PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(reg)	((reg) & BIT(31))
>   #define INVALID_EE				0xFF
>   
>   /* Ownership Table */
> @@ -96,30 +100,33 @@ enum pmic_arb_channel {
>   	PMIC_ARB_CHANNEL_OBS,
>   };
>   
> -#define PMIC_ARB_MAX_BUSES		2
> +#define PMIC_ARB_MAX_BUSES		4

Why PMIC_ARB_MAX_BUSES is changed to 4 ?

Neil

<snip>
Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8
Posted by Jishnu Prakash 3 months, 2 weeks ago
Hi Neil,

On 10/24/2025 3:48 PM, Neil Armstrong wrote:
> Hi,
> 
> On 10/24/25 11:33, Jishnu Prakash wrote:
>> From: David Collins <david.collins@oss.qualcomm.com>
>>
>> PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
>> peripherals.  Its register map differs from v7 as several fields
>> increased in size. Add support for PMIC arbiter version 8.
>>
>> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>> ---
>>   drivers/spmi/spmi-pmic-arb.c | 324 +++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 294 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 91581974ef84..612736973e4b 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -3,6 +3,7 @@
>>    * Copyright (c) 2012-2015, 2017, 2021, The Linux Foundation. All rights reserved.
>>    */
>>   #include <linux/bitmap.h>
>> +#include <linux/bitfield.h>
>>   #include <linux/delay.h>
>>   #include <linux/err.h>
>>   #include <linux/interrupt.h>
>> @@ -25,10 +26,12 @@
>>   #define PMIC_ARB_VERSION_V3_MIN        0x30000000
>>   #define PMIC_ARB_VERSION_V5_MIN        0x50000000
>>   #define PMIC_ARB_VERSION_V7_MIN        0x70000000
>> +#define PMIC_ARB_VERSION_V8_MIN        0x80000000
>>   #define PMIC_ARB_INT_EN            0x0004
>>     #define PMIC_ARB_FEATURES        0x0004
>>   #define PMIC_ARB_FEATURES_PERIPH_MASK    GENMASK(10, 0)
>> +#define PMIC_ARB_FEATURES_V8_PERIPH_MASK    GENMASK(12, 0)
>>     #define PMIC_ARB_FEATURES1        0x0008
>>   @@ -50,9 +53,10 @@
>>   #define SPMI_MAPPING_BIT_IS_1_RESULT(X)    (((X) >> 0) & 0xFF)
>>     #define SPMI_MAPPING_TABLE_TREE_DEPTH    16    /* Maximum of 16-bits */
>> -#define PMIC_ARB_MAX_PPID        BIT(12) /* PPID is 12bit */
>> +#define PMIC_ARB_MAX_PPID        BIT(13)
>>   #define PMIC_ARB_APID_VALID        BIT(15)
>>   #define PMIC_ARB_CHAN_IS_IRQ_OWNER(reg)    ((reg) & BIT(24))
>> +#define PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(reg)    ((reg) & BIT(31))
>>   #define INVALID_EE                0xFF
>>     /* Ownership Table */
>> @@ -96,30 +100,33 @@ enum pmic_arb_channel {
>>       PMIC_ARB_CHANNEL_OBS,
>>   };
>>   -#define PMIC_ARB_MAX_BUSES        2
>> +#define PMIC_ARB_MAX_BUSES        4
> 
> Why PMIC_ARB_MAX_BUSES is changed to 4 ?

PMIC_ARB_MAX_BUSES is used only in the definition of
struct spmi_pmic_arb, for this member:

	struct spmi_pmic_arb_bus *buses[PMIC_ARB_MAX_BUSES];

The PMIC Arbiter v8 is capable of supporting up to 4
SPMI buses, so this change is needed to support it.

Thanks,
Jishnu

> 
> Neil
> 
> <snip>
>