Skip to contentSkip to conversation

: spmi: pmic-arb: support for V5 HW and bug fixes

PATCH Superseded Kiran Gunda 2 participants 7 messages Jul 11, 2017 – Jul 18, 2017

Subsystems (2)
Arm Qualcomm mailing list SPMI

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Kiran Gunda

author
#0 lore raw

v3 cover : spmi: pmic-arb: support for V5 HW and bug fixes

v3:
* spmi: pmic-arb: add support for HW version 5
Modified #define INVALID (-1) to
#define INVALID_EE 0xFF.

v2:
* spmi: pmic-arb: return __iomem pointer instead of offset Added Stephen's reviewed-by tag.

* spmi: pmic-arb: fix a possible null pointer dereference Added Stephen's reviewed-by tag.

* spmi: pmic-arb: add support for HW version 5
Modified the pmic_arb_offset_v5 function to return the offset instead of passed by a pointer.

* spmi: pmic-arb: Remove checking opc value not less than 0 Added Stephen's reviewed-by tag.
Added my sign-off tag.

v1:

This patch series add the support for pmic arbiter hardware v5 along with the few bug fixes and code cleanup.

This patch series is dependent on the below patches and can be merged cleanly only after picking the below patches in to the tree.

https://patchwork.kernel.org/patch/9810723/
https://patchwork.kernel.org/patch/9822597/
https://patchwork.kernel.org/patch/9822601/
https://patchwork.kernel.org/patch/9822607/
https://patchwork.kernel.org/patch/9822609/
https://patchwork.kernel.org/patch/9822613/

David Collins (1):
spmi: pmic-arb: add support for HW version 5

Fenglin Wu (1):
spmi: pmic-arb: Remove checking opc value not less than 0

Kiran Gunda (2):
spmi: pmic-arb: return __iomem pointer instead of offset
spmi: pmic-arb: fix a possible null pointer dereference

 drivers/spmi/spmi-pmic-arb.c | 328 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 268 insertions(+), 60 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH v3 1/4 spmi: pmic-arb: return __iomem pointer instead of offset

Modify the pmic_arb version ops to return an __iomem pointer to the address instead of an offset. That way we do not need to care about the base address changes in the new HW version.

Signed-off-by Kiran Gunda <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 87 ++++++++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 39 deletions(-)
drivers/spmi/spmi-pmic-arb.c +48 -39
index 25589d6..4853758 100644
@@ -167,14 +167,14 @@ struct spmi_pmic_arb {
  * @offset:		on v1 offset of per-ee channel.
  *			on v2 offset of per-ee and per-ppid channel.
  * @fmt_cmd:		formats a GENI/SPMI command.
- * @owner_acc_status:	on v1 offset of PMIC_ARB_SPMI_PIC_OWNERm_ACC_STATUSn
- *			on v2 offset of SPMI_PIC_OWNERm_ACC_STATUSn.
- * @acc_enable:		on v1 offset of PMIC_ARB_SPMI_PIC_ACC_ENABLEn
- *			on v2 offset of SPMI_PIC_ACC_ENABLEn.
- * @irq_status:		on v1 offset of PMIC_ARB_SPMI_PIC_IRQ_STATUSn
- *			on v2 offset of SPMI_PIC_IRQ_STATUSn.
- * @irq_clear:		on v1 offset of PMIC_ARB_SPMI_PIC_IRQ_CLEARn
- *			on v2 offset of SPMI_PIC_IRQ_CLEARn.
+ * @owner_acc_status:	on v1 address of PMIC_ARB_SPMI_PIC_OWNERm_ACC_STATUSn
+ *			on v2 address of SPMI_PIC_OWNERm_ACC_STATUSn.
+ * @acc_enable:		on v1 address of PMIC_ARB_SPMI_PIC_ACC_ENABLEn
+ *			on v2 address of SPMI_PIC_ACC_ENABLEn.
+ * @irq_status:		on v1 address of PMIC_ARB_SPMI_PIC_IRQ_STATUSn
+ *			on v2 address of SPMI_PIC_IRQ_STATUSn.
+ * @irq_clear:		on v1 address of PMIC_ARB_SPMI_PIC_IRQ_CLEARn
+ *			on v2 address of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
 	const char *ver_str;

@@ -184,10 +184,11 @@ struct pmic_arb_ver_ops {
 	u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
 	int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
 	/* Interrupts controller functionality (offset of PIC registers) */
-	u32 (*owner_acc_status)(u8 m, u16 n);
-	u32 (*acc_enable)(u16 n);
-	u32 (*irq_status)(u16 n);
-	u32 (*irq_clear)(u16 n);
+	void __iomem *(*owner_acc_status)(struct spmi_pmic_arb *pmic_arb, u8 m,
+					  u16 n);
+	void __iomem *(*acc_enable)(struct spmi_pmic_arb *pmic_arb, u16 n);
+	void __iomem *(*irq_status)(struct spmi_pmic_arb *pmic_arb, u16 n);
+	void __iomem *(*irq_clear)(struct spmi_pmic_arb *pmic_arb, u16 n);
 };
 
 static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb,

@@ -476,7 +477,7 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
 	u8 per = ppid & 0xFF;
 	u8 irq_mask = BIT(id);
 
-	writel_relaxed(irq_mask, pmic_arb->intr + ver_ops->irq_clear(apid));
+	writel_relaxed(irq_mask, ver_ops->irq_clear(pmic_arb, apid));
 
 	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
 			(per << 8) + QPNPINT_REG_LATCHED_CLR, &irq_mask, 1))

@@ -498,7 +499,7 @@ static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
 	u32 status;
 	int id;
 
-	status = readl_relaxed(pmic_arb->intr + ver_ops->irq_status(apid));
+	status = readl_relaxed(ver_ops->irq_status(pmic_arb, apid));
 	while (status) {
 		id = ffs(status) - 1;
 		status &= ~BIT(id);

@@ -517,23 +518,21 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 	struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
 	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	void __iomem *intr = pmic_arb->intr;
 	int first = pmic_arb->min_apid >> 5;
 	int last = pmic_arb->max_apid >> 5;
+	u8 ee = pmic_arb->ee;
 	u32 status, enable;
 	int i, id, apid;
 
 	chained_irq_enter(chip, desc);
 
 	for (i = first; i <= last; ++i) {
-		status = readl_relaxed(intr +
-				    ver_ops->owner_acc_status(pmic_arb->ee, i));
+		status = readl_relaxed(ver_ops->owner_acc_status(pmic_arb, ee, i));
 		while (status) {
 			id = ffs(status) - 1;
 			status &= ~BIT(id);
 			apid = id + i * 32;
-			enable = readl_relaxed(intr +
-					pmic_arb->ver_ops->acc_enable(apid));
+			enable = readl_relaxed(ver_ops->acc_enable(pmic_arb, apid));
 			if (enable & SPMI_PIC_ACC_ENABLE_BIT)
 				periph_interrupt(pmic_arb, apid);
 		}

@@ -550,7 +549,7 @@ static void qpnpint_irq_ack(struct irq_data *d)
 	u16 apid = hwirq_to_apid(d->hwirq);
 	u8 data;
 
-	writel_relaxed(BIT(irq), pmic_arb->intr + ver_ops->irq_clear(apid));
+	writel_relaxed(BIT(irq), ver_ops->irq_clear(pmic_arb, apid));
 
 	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);

@@ -567,12 +566,13 @@ static void qpnpint_irq_mask(struct irq_data *d)
 static void qpnpint_irq_unmask(struct irq_data *d)
 {
 	struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
 	u8 irq = hwirq_to_irq(d->hwirq);
 	u16 apid = hwirq_to_apid(d->hwirq);
 	u8 buf[2];
 
 	writel_relaxed(SPMI_PIC_ACC_ENABLE_BIT,
-		pmic_arb->intr + pmic_arb->ver_ops->acc_enable(apid));
+			ver_ops->acc_enable(pmic_arb, apid));
 
 	qpnpint_spmi_read(d, QPNPINT_REG_EN_SET, &buf[0], 1);
 	if (!(buf[0] & BIT(irq))) {

@@ -843,49 +843,58 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 	return (opc << 27) | ((addr & 0xff) << 4) | (bc & 0x7);
 }
 
-static u32 pmic_arb_owner_acc_status_v1(u8 m, u16 n)
+static void __iomem *
+pmic_arb_owner_acc_status_v1(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
 {
-	return 0x20 * m + 0x4 * n;
+	return pmic_arb->intr + 0x20 * m + 0x4 * n;
 }
 
-static u32 pmic_arb_owner_acc_status_v2(u8 m, u16 n)
+static void __iomem *
+pmic_arb_owner_acc_status_v2(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
 {
-	return 0x100000 + 0x1000 * m + 0x4 * n;
+	return pmic_arb->intr + 0x100000 + 0x1000 * m + 0x4 * n;
 }
 
-static u32 pmic_arb_owner_acc_status_v3(u8 m, u16 n)
+static void __iomem *
+pmic_arb_owner_acc_status_v3(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
 {
-	return 0x200000 + 0x1000 * m + 0x4 * n;
+	return pmic_arb->intr + 0x200000 + 0x1000 * m + 0x4 * n;
 }
 
-static u32 pmic_arb_acc_enable_v1(u16 n)
+static void __iomem *
+pmic_arb_acc_enable_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
-	return 0x200 + 0x4 * n;
+	return pmic_arb->intr + 0x200 + 0x4 * n;
 }
 
-static u32 pmic_arb_acc_enable_v2(u16 n)
+static void __iomem *
+pmic_arb_acc_enable_v2(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
-	return 0x1000 * n;
+	return pmic_arb->intr + 0x1000 * n;
 }
 
-static u32 pmic_arb_irq_status_v1(u16 n)
+static void __iomem *
+pmic_arb_irq_status_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
-	return 0x600 + 0x4 * n;
+	return pmic_arb->intr + 0x600 + 0x4 * n;
 }
 
-static u32 pmic_arb_irq_status_v2(u16 n)
+static void __iomem *
+pmic_arb_irq_status_v2(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
-	return 0x4 + 0x1000 * n;
+	return pmic_arb->intr + 0x4 + 0x1000 * n;
 }
 
-static u32 pmic_arb_irq_clear_v1(u16 n)
+static void __iomem *
+pmic_arb_irq_clear_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
-	return 0xA00 + 0x4 * n;
+	return pmic_arb->intr + 0xA00 + 0x4 * n;
 }
 
-static u32 pmic_arb_irq_clear_v2(u16 n)
+static void __iomem *
+pmic_arb_irq_clear_v2(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
-	return 0x8 + 0x1000 * n;
+	return pmic_arb->intr + 0x8 + 0x1000 * n;
 }
 
 static const struct pmic_arb_ver_ops pmic_arb_v1 = {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project
author
#2 lore raw

PATCH v3 2/4 spmi: pmic-arb: fix a possible null pointer dereference

In reply to Kiran Gunda

If "core" memory resource is not specified, then the driver could end up dereferencing a null pointer. Fix this issue.

Signed-off-by Kiran Gunda <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spmi/spmi-pmic-arb.c +2 -2
index 4853758..c7b54a6 100644
@@ -956,14 +956,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	pmic_arb->spmic = ctrl;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
-	pmic_arb->core_size = resource_size(res);
-
 	core = devm_ioremap_resource(&ctrl->dev, res);
 	if (IS_ERR(core)) {
 		err = PTR_ERR(core);
 		goto err_put_ctrl;
 	}
 
+	pmic_arb->core_size = resource_size(res);
+
 	pmic_arb->ppid_to_apid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PPID,
 					      sizeof(*pmic_arb->ppid_to_apid),
 					      GFP_KERNEL);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project
author
#3 lore raw

PATCH v3 3/4 spmi: pmic-arb: add support for HW version 5

In reply to Kiran Gunda

From: David Collins <[email protected]>

Add support for version 5 of the SPMI PMIC arbiter. It utilizes different offsets for registers than those found on version 3. Also, the procedure to determine if writing and IRQ access is allowed for a given PPID changes for version 5.

Signed-off-by David Collins <[email protected]>
Signed-off-by Kiran Gunda <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 235 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 217 insertions(+), 18 deletions(-)
drivers/spmi/spmi-pmic-arb.c +217 -18
index c7b54a6..5297aa4 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2017, The Linux Foundation. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and

@@ -29,6 +29,7 @@
 #define PMIC_ARB_VERSION		0x0000
 #define PMIC_ARB_VERSION_V2_MIN		0x20010000
 #define PMIC_ARB_VERSION_V3_MIN		0x30000000
+#define PMIC_ARB_VERSION_V5_MIN		0x50000000
 #define PMIC_ARB_INT_EN			0x0004
 
 /* PMIC Arbiter channel registers offsets */

@@ -39,7 +40,6 @@
 #define PMIC_ARB_WDATA1			0x14
 #define PMIC_ARB_RDATA0			0x18
 #define PMIC_ARB_RDATA1			0x1C
-#define PMIC_ARB_REG_CHNL(N)		(0x800 + 0x4 * (N))
 
 /* Mapping Table */
 #define SPMI_MAPPING_TABLE_REG(N)	(0x0B00 + (4 * (N)))

@@ -52,6 +52,8 @@
 #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
 #define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
 #define PMIC_ARB_APID_VALID		BIT(15)
+#define PMIC_ARB_CHAN_IS_IRQ_OWNER(reg)	((reg) & BIT(24))
+#define INVALID_EE				0xFF
 
 /* Ownership Table */
 #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))

@@ -86,6 +88,15 @@ enum pmic_arb_cmd_op_code {
 	PMIC_ARB_OP_ZERO_WRITE = 16,
 };
 
+/*
+ * PMIC arbiter version 5 uses different register offsets for read/write vs
+ * observer channels.
+ */
+enum pmic_arb_channel {
+	PMIC_ARB_CHANNEL_RW,
+	PMIC_ARB_CHANNEL_OBS,
+};
+
 /* Maximum number of support PMIC peripherals */
 #define PMIC_ARB_MAX_PERIPHS		512
 #define PMIC_ARB_TIMEOUT_US		100

@@ -112,7 +123,8 @@ enum pmic_arb_cmd_op_code {
 
 struct apid_data {
 	u16		ppid;
-	u8		owner;
+	u8		write_ee;
+	u8		irq_ee;
 };
 
 /**

@@ -175,12 +187,14 @@ struct spmi_pmic_arb {
  *			on v2 address of SPMI_PIC_IRQ_STATUSn.
  * @irq_clear:		on v1 address of PMIC_ARB_SPMI_PIC_IRQ_CLEARn
  *			on v2 address of SPMI_PIC_IRQ_CLEARn.
+ * @apid_map_offset:	offset of PMIC_ARB_REG_CHNLn
  */
 struct pmic_arb_ver_ops {
 	const char *ver_str;
 	int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-	int (*offset)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr);
+	int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
+			enum pmic_arb_channel ch_type);
 	u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
 	int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
 	/* Interrupts controller functionality (offset of PIC registers) */

@@ -189,6 +203,7 @@ struct pmic_arb_ver_ops {
 	void __iomem *(*acc_enable)(struct spmi_pmic_arb *pmic_arb, u16 n);
 	void __iomem *(*irq_status)(struct spmi_pmic_arb *pmic_arb, u16 n);
 	void __iomem *(*irq_clear)(struct spmi_pmic_arb *pmic_arb, u16 n);
+	u32 (*apid_map_offset)(u16 n);
 };
 
 static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb,

@@ -233,7 +248,8 @@ static void pmic_arb_write_data(struct spmi_pmic_arb *pmic_arb, const u8 *buf,
 }
 
 static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
-				  void __iomem *base, u8 sid, u16 addr)
+				  void __iomem *base, u8 sid, u16 addr,
+				  enum pmic_arb_channel ch_type)
 {
 	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	u32 status = 0;

@@ -241,7 +257,7 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 	u32 offset;
 	int rc;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, ch_type);
 	if (rc < 0)
 		return rc;
 

@@ -289,7 +305,7 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, 0);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, 0, PMIC_ARB_CHANNEL_RW);
 	if (rc < 0)
 		return rc;
 

@@ -298,7 +314,8 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 
 	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
 	pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, 0);
+	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, 0,
+				    PMIC_ARB_CHANNEL_RW);
 	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
 
 	return rc;

@@ -334,7 +351,8 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr,
+				       PMIC_ARB_CHANNEL_OBS);
 	if (rc < 0)
 		return rc;
 

@@ -359,7 +377,8 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 
 	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
 	pmic_arb_set_rd_cmd(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->rd_base, sid, addr);
+	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->rd_base, sid, addr,
+				    PMIC_ARB_CHANNEL_OBS);
 	if (rc)
 		goto done;
 

@@ -385,7 +404,8 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr,
+					PMIC_ARB_CHANNEL_RW);
 	if (rc < 0)
 		return rc;
 

@@ -420,7 +440,8 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 
 	/* Start the transaction */
 	pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr);
+	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr,
+				    PMIC_ARB_CHANNEL_RW);
 	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
 
 	return rc;

@@ -681,12 +702,19 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	ppid = intspec[0] << 8 | intspec[1];
 	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
 	if (rc < 0) {
-		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
+		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
 		intspec[0], intspec[1], intspec[2], rc);
 		return rc;
 	}
 
 	apid = rc;
+	if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
+		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u: ee=%u but owner=%u\n",
+			intspec[0], intspec[1], intspec[2], pmic_arb->ee,
+			pmic_arb->apid_data[apid].irq_ee);
+		return -ENODEV;
+	}
+
 	/* Keep track of {max,min}_apid for bounding search during interrupt */
 	if (apid > pmic_arb->max_apid)
 		pmic_arb->max_apid = apid;

@@ -763,7 +791,8 @@ static int pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 }
 
 /* v1 offset per ee */
-static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr)
+static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
+			enum pmic_arb_channel ch_type)
 {
 	return 0x800 + 0x80 * pmic_arb->channel;
 }

@@ -779,13 +808,14 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 	 * ppid_to_apid is an in-memory invert of that table.
 	 */
 	for (i = pmic_arb->last_apid; ; i++, apid_info++) {
-		offset = PMIC_ARB_REG_CHNL(i);
+		offset = pmic_arb->ver_ops->apid_map_offset(i);
 		if (offset >= pmic_arb->core_size)
 			break;
 
 		regval = readl_relaxed(pmic_arb->cnfg +
 					SPMI_OWNERSHIP_TABLE_REG(i));
-		apid_info->owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+		apid_info->irq_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+		apid_info->write_ee = apid_info->irq_ee;
 
 		regval = readl_relaxed(pmic_arb->core + offset);
 		if (!regval)

@@ -817,8 +847,87 @@ static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 	return apid_valid & ~PMIC_ARB_APID_VALID;
 }
 
+static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
+{
+	struct apid_data *apid_info = pmic_arb->apid_data;
+	struct apid_data *prev_apid_info;
+	u16 i, j, ppid;
+	bool valid, is_irq_ee;
+	u32 regval, offset;
+
+	/*
+	 * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to PPID.
+	 * ppid_to_apid is an in-memory invert of that table.  In order to allow
+	 * multiple EEs to write to a single PPID in arbiter version 5, there
+	 * is 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 for a given PPID will
+	 * receive interrupts from the PPID.
+	 */
+	for (i = 0; ; i++, apid_info++) {
+		offset = pmic_arb->ver_ops->apid_map_offset(i);
+		if (offset >= pmic_arb->core_size)
+			break;
+
+		regval = readl_relaxed(pmic_arb->core + offset);
+		if (!regval)
+			continue;
+		ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
+		is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
+
+		regval = readl_relaxed(pmic_arb->cnfg +
+				      SPMI_OWNERSHIP_TABLE_REG(i));
+		apid_info->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+
+		apid_info->irq_ee = is_irq_ee ?
+					apid_info->write_ee : INVALID_EE;
+
+		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
+		j = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
+		prev_apid_info = &pmic_arb->apid_data[j];
+
+		if (valid && is_irq_ee &&
+				prev_apid_info->write_ee == pmic_arb->ee) {
+			/*
+			 * Duplicate PPID mapping after the one for this EE;
+			 * override the irq owner
+			 */
+			prev_apid_info->irq_ee = apid_info->irq_ee;
+		} else if (!valid || is_irq_ee) {
+			/* First PPID mapping or duplicate for another EE */
+			pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
+		}
+
+		apid_info->ppid = ppid;
+		pmic_arb->last_apid = i;
+	}
+
+	/* Dump the mapping table for debug purposes. */
+	dev_dbg(&pmic_arb->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
+	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
+		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
+		i = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
+		if (valid) {
+			apid_info = &pmic_arb->apid_data[i];
+			dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
+			      ppid, i, apid_info->write_ee, apid_info->irq_ee);
+		}
+	}
+
+	return 0;
+}
+
+static int pmic_arb_ppid_to_apid_v5(struct spmi_pmic_arb *pmic_arb, u16 ppid)
+{
+	if (!(pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID))
+		return -ENODEV;
+
+	return pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
+}
+
 /* v2 offset per ppid and per ee */
-static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr)
+static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
+			   enum pmic_arb_channel ch_type)
 {
 	u16 apid;
 	u16 ppid;

@@ -833,6 +942,35 @@ static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr)
 	return 0x1000 * pmic_arb->ee + 0x8000 * apid;
 }
 
+/*
+ * v5 offset per ee and per apid for observer channels and per apid for
+ * read/write channels.
+ */
+static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
+			   enum pmic_arb_channel ch_type)
+{
+	u16 apid;
+	int rc;
+	u32 offset = 0;
+	u16 ppid = (sid << 8) | (addr >> 8);
+
+	rc = pmic_arb_ppid_to_apid_v5(pmic_arb, ppid);
+	if (rc < 0)
+		return rc;
+
+	apid = rc;
+	switch (ch_type) {
+	case PMIC_ARB_CHANNEL_OBS:
+		offset = 0x10000 * pmic_arb->ee + 0x80 * apid;
+		break;
+	case PMIC_ARB_CHANNEL_RW:
+		offset = 0x10000 * 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);

@@ -862,6 +1000,12 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 }
 
 static void __iomem *
+pmic_arb_owner_acc_status_v5(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
+{
+	return pmic_arb->intr + 0x10000 * m + 0x4 * n;
+}
+
+static void __iomem *
 pmic_arb_acc_enable_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
 	return pmic_arb->intr + 0x200 + 0x4 * n;

@@ -874,6 +1018,12 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 }
 
 static void __iomem *
+pmic_arb_acc_enable_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
+{
+	return pmic_arb->wr_base + 0x100 + 0x10000 * n;
+}
+
+static void __iomem *
 pmic_arb_irq_status_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
 	return pmic_arb->intr + 0x600 + 0x4 * n;

@@ -886,6 +1036,12 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 }
 
 static void __iomem *
+pmic_arb_irq_status_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
+{
+	return pmic_arb->wr_base + 0x104 + 0x10000 * n;
+}
+
+static void __iomem *
 pmic_arb_irq_clear_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
 {
 	return pmic_arb->intr + 0xA00 + 0x4 * n;

@@ -897,6 +1053,22 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 	return pmic_arb->intr + 0x8 + 0x1000 * n;
 }
 
+static void __iomem *
+pmic_arb_irq_clear_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
+{
+	return pmic_arb->wr_base + 0x108 + 0x10000 * n;
+}
+
+static u32 pmic_arb_apid_map_offset_v2(u16 n)
+{
+	return 0x800 + 0x4 * n;
+}
+
+static u32 pmic_arb_apid_map_offset_v5(u16 n)
+{
+	return 0x900 + 0x4 * n;
+}
+
 static const struct pmic_arb_ver_ops pmic_arb_v1 = {
 	.ver_str		= "v1",
 	.ppid_to_apid		= pmic_arb_ppid_to_apid_v1,

@@ -907,6 +1079,7 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 	.acc_enable		= pmic_arb_acc_enable_v1,
 	.irq_status		= pmic_arb_irq_status_v1,
 	.irq_clear		= pmic_arb_irq_clear_v1,
+	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
 };
 
 static const struct pmic_arb_ver_ops pmic_arb_v2 = {

@@ -919,6 +1092,7 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 	.acc_enable		= pmic_arb_acc_enable_v2,
 	.irq_status		= pmic_arb_irq_status_v2,
 	.irq_clear		= pmic_arb_irq_clear_v2,
+	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
 };
 
 static const struct pmic_arb_ver_ops pmic_arb_v3 = {

@@ -931,6 +1105,20 @@ static u32 pmic_arb_fmt_cmd_v2(u8 opc, u8 sid, u16 addr, u8 bc)
 	.acc_enable		= pmic_arb_acc_enable_v2,
 	.irq_status		= pmic_arb_irq_status_v2,
 	.irq_clear		= pmic_arb_irq_clear_v2,
+	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
+};
+
+static const struct pmic_arb_ver_ops pmic_arb_v5 = {
+	.ver_str		= "v5",
+	.ppid_to_apid		= pmic_arb_ppid_to_apid_v5,
+	.non_data_cmd		= pmic_arb_non_data_cmd_v2,
+	.offset			= pmic_arb_offset_v5,
+	.fmt_cmd		= pmic_arb_fmt_cmd_v2,
+	.owner_acc_status	= pmic_arb_owner_acc_status_v5,
+	.acc_enable		= pmic_arb_acc_enable_v5,
+	.irq_status		= pmic_arb_irq_status_v5,
+	.irq_clear		= pmic_arb_irq_clear_v5,
+	.apid_map_offset	= pmic_arb_apid_map_offset_v5,
 };
 
 static const struct irq_domain_ops pmic_arb_irq_domain_ops = {

@@ -983,8 +1171,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 		if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
 			pmic_arb->ver_ops = &pmic_arb_v2;
-		else
+		else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
 			pmic_arb->ver_ops = &pmic_arb_v3;
+		else
+			pmic_arb->ver_ops = &pmic_arb_v5;
 
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 						   "obsrvr");

@@ -1074,6 +1264,15 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	ctrl->read_cmd = pmic_arb_read_cmd;
 	ctrl->write_cmd = pmic_arb_write_cmd;
 
+	if (hw_ver >= PMIC_ARB_VERSION_V5_MIN) {
+		err = pmic_arb_read_apid_map_v5(pmic_arb);
+		if (err) {
+			dev_err(&pdev->dev, "could not read APID->PPID mapping table, rc= %d\n",
+				err);
+			goto err_put_ctrl;
+		}
+	}
+
 	dev_dbg(&pdev->dev, "adding irq domain\n");
 	pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
 					 &pmic_arb_irq_domain_ops, pmic_arb);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project
#4 lore raw

Re: [PATCH V3 3/4] spmi: pmic-arb: add support for HW version 5

Review static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, · line 440

Quoted 2 lines · Kiran Gunda
(quoted diff) +0 -0
@@ -420,7 +440,8 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,

Mostly style nitpicks!

Quoted 9 lines
drivers/spmi/spmi-pmic-arb.c +2 -1
 	/* Start the transaction */
 	pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr);
+	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr,
+				    PMIC_ARB_CHANNEL_RW);
 	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
 
 	return rc;

drivers/spmi/spmi-pmic-arb.c static int qpnpint_irq_domain_dt_translate(struct irq_domain *d, · line 706

Quoted 6 lines
drivers/spmi/spmi-pmic-arb.c +1 -1
@@ -681,12 +702,19 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	ppid = intspec[0] << 8 | intspec[1];
 	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
 	if (rc < 0) {
-		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
+		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
 		intspec[0], intspec[1], intspec[2], rc);

Unrelated change?

Quoted 17 lines
drivers/spmi/spmi-pmic-arb.c +7 -0
 		return rc;
 	}
 
 	apid = rc;
+	if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
+		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u: ee=%u but owner=%u\n",
+			intspec[0], intspec[1], intspec[2], pmic_arb->ee,
+			pmic_arb->apid_data[apid].irq_ee);
+		return -ENODEV;
+	}
+
 	/* Keep track of {max,min}_apid for bounding search during interrupt */
 	if (apid > pmic_arb->max_apid)
 		pmic_arb->max_apid = apid;
drivers/spmi/spmi-pmic-arb.c +0 -0
 	return apid_valid & ~PMIC_ARB_APID_VALID;
 }

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) · line 859

Quoted 10 lines
drivers/spmi/spmi-pmic-arb.c +10 -0
+static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
+{
+	struct apid_data *apid_info = pmic_arb->apid_data;
+	struct apid_data *prev_apid_info;
+	u16 i, j, ppid;
+	bool valid, is_irq_ee;
+	u32 regval, offset;
+
+	/*
+	 * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to PPID.

Is this comment stale? PMIC_ARB_REG_CHNL macro was deleted?

Quoted 14 lines
drivers/spmi/spmi-pmic-arb.c +14 -0
+	 * ppid_to_apid is an in-memory invert of that table.  In order to allow
+	 * multiple EEs to write to a single PPID in arbiter version 5, there
+	 * is 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 for a given PPID will
+	 * receive interrupts from the PPID.
+	 */
+	for (i = 0; ; i++, apid_info++) {
+		offset = pmic_arb->ver_ops->apid_map_offset(i);
+		if (offset >= pmic_arb->core_size)
+			break;
+
+		regval = readl_relaxed(pmic_arb->core + offset);
+		if (!regval)

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) · line 883

Quoted 10 lines
drivers/spmi/spmi-pmic-arb.c +10 -0
+			continue;
+		ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
+		is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
+
+		regval = readl_relaxed(pmic_arb->cnfg +
+				      SPMI_OWNERSHIP_TABLE_REG(i));
+		apid_info->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+
+		apid_info->irq_ee = is_irq_ee ?
+					apid_info->write_ee : INVALID_EE;

Perhaps apid_info can be renamed to apidd (for apid descriptor) or apidi (for apid info) and then this line is short enough to fit on one line?

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) · line 886

Quoted 3 lines
drivers/spmi/spmi-pmic-arb.c +3 -0
+
+		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
+		j = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;

Maybe j can be 'apid'. Slightly more informative and usually 'j' is reserved for iterating, which in this case we're not doing. We're just directly indexing an apid into a table.

Quoted 18 lines
drivers/spmi/spmi-pmic-arb.c +18 -0
+		prev_apid_info = &pmic_arb->apid_data[j];
+
+		if (valid && is_irq_ee &&
+				prev_apid_info->write_ee == pmic_arb->ee) {
+			/*
+			 * Duplicate PPID mapping after the one for this EE;
+			 * override the irq owner
+			 */
+			prev_apid_info->irq_ee = apid_info->irq_ee;
+		} else if (!valid || is_irq_ee) {
+			/* First PPID mapping or duplicate for another EE */
+			pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
+		}
+
+		apid_info->ppid = ppid;
+		pmic_arb->last_apid = i;
+	}
+

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) · line 914

Quoted 10 lines
drivers/spmi/spmi-pmic-arb.c +10 -0
+	/* Dump the mapping table for debug purposes. */
+	dev_dbg(&pmic_arb->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
+	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
+		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
+		i = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
+		if (valid) {
+			apid_info = &pmic_arb->apid_data[i];
+			dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
+			      ppid, i, apid_info->write_ee, apid_info->irq_ee);
+		}

Could be

	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
		apid = pmic_arb->ppid_to_apid[ppid];
		if (apid & PMIC_ARB_APID_VALID) {
			apid &= ~PMIC_ARB_VALID;
			apidd = &pmic_arb->apid_data[apid];
			dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
			      ppid, apid, apidd->write_ee, apidd->irq_ee);
		}
	}

Which maybe is clearer because it uses less local variables that don't get used more than once.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#5 lore raw

Re: [PATCH V3 3/4] spmi: pmic-arb: add support for HW version 5

Quoted 7 lines · Stephen Boyd

Review static int pmic_arb_write_cmd(struct · line 441

Quoted 3 lines · Kiran Gunda
(quoted diff) +0 -0
@@ -420,7 +440,8 @@ static int pmic_arb_write_cmd(struct 
 spmi_controller *ctrl, u8 opc, u8 sid,

Mostly style nitpicks!

Will check and address in the next patch.

Quoted 7 lines
Quoted 7 lines
drivers/spmi/spmi-pmic-arb.c +2 -1
 	/* Start the transaction */
 	pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr);
+	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr,
+				    PMIC_ARB_CHANNEL_RW);
 	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
Quoted 14 lines

Review static int · line 709

Quoted 11 lines
(quoted diff) +1 -1
 	return rc;

@@ -681,12 +702,19 @@ static int 
 qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	ppid = intspec[0] << 8 | intspec[1];
 	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
 	if (rc < 0) {
-		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = 0x%x, periph 
 = 0x%x, irq = %x rc = %d\n",
+		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = 
 %#x, irq = %u rc = %d\n",
 		intspec[0], intspec[1], intspec[2], rc);

Unrelated change?

This just to print the irq number in decimal format.

Quoted 19 lines
Quoted 19 lines
drivers/spmi/spmi-pmic-arb.c +2 -0
 		return rc;
 	}
 
 	apid = rc;
+	if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
+		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph =

%#x, irq = %u: ee=%u but owner=%u\n",

drivers/spmi/spmi-pmic-arb.c +5 -0
+			intspec[0], intspec[1], intspec[2], pmic_arb->ee,
+			pmic_arb->apid_data[apid].irq_ee);
+		return -ENODEV;
+	}
+
 	/* Keep track of {max,min}_apid for bounding search during interrupt

*/

drivers/spmi/spmi-pmic-arb.c +0 -0
 	if (apid > pmic_arb->max_apid)
 		pmic_arb->max_apid = apid;
 	return apid_valid & ~PMIC_ARB_APID_VALID;
 }
Quoted 14 lines
Quoted 11 lines
drivers/spmi/spmi-pmic-arb.c +10 -0
+static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
+{
+	struct apid_data *apid_info = pmic_arb->apid_data;
+	struct apid_data *prev_apid_info;
+	u16 i, j, ppid;
+	bool valid, is_irq_ee;
+	u32 regval, offset;
+
+	/*
+	 * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to

PPID.

Is this comment stale? PMIC_ARB_REG_CHNL macro was deleted?

will remove this line in the next patch.

Quoted 24 lines
Quoted 24 lines
(quoted diff) +1 -0
+	 * ppid_to_apid is an in-memory invert of that table.  In order to

allow

(quoted diff) +1 -0
+	 * multiple EEs to write to a single PPID in arbiter version 5,

there

(quoted diff) +1 -0
+	 * is more than one APID mapped to each PPID.  The owner field for

each

(quoted diff) +1 -0
+	 * of these mappings specifies the EE which is allowed to write to

the

drivers/spmi/spmi-pmic-arb.c +16 -0
+	 * APID.  The owner of the last (highest) APID for a given PPID will
+	 * receive interrupts from the PPID.
+	 */
+	for (i = 0; ; i++, apid_info++) {
+		offset = pmic_arb->ver_ops->apid_map_offset(i);
+		if (offset >= pmic_arb->core_size)
+			break;
+
+		regval = readl_relaxed(pmic_arb->core + offset);
+		if (!regval)
+			continue;
+		ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
+		is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
+
+		regval = readl_relaxed(pmic_arb->cnfg +
+				      SPMI_OWNERSHIP_TABLE_REG(i));
Quoted 9 lines

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) · line 883

Quoted 4 lines
drivers/spmi/spmi-pmic-arb.c +4 -0
+		apid_info->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+
+		apid_info->irq_ee = is_irq_ee ?
+					apid_info->write_ee : INVALID_EE;

Perhaps apid_info can be renamed to apidd (for apid descriptor) or apidi (for apid info) and then this line is short enough to fit on one line?

ok. will change it in next patch.

Quoted 8 lines

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) · line 886

Quoted 3 lines
drivers/spmi/spmi-pmic-arb.c +3 -0
+
+		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
+		j = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;

Maybe j can be 'apid'. Slightly more informative and usually 'j' is reserved for iterating, which in this case we're not doing. We're just directly indexing an apid into a table.

Will change in the next patch.

Quoted 41 lines
Quoted 18 lines
drivers/spmi/spmi-pmic-arb.c +18 -0
+		prev_apid_info = &pmic_arb->apid_data[j];
+
+		if (valid && is_irq_ee &&
+				prev_apid_info->write_ee == pmic_arb->ee) {
+			/*
+			 * Duplicate PPID mapping after the one for this EE;
+			 * override the irq owner
+			 */
+			prev_apid_info->irq_ee = apid_info->irq_ee;
+		} else if (!valid || is_irq_ee) {
+			/* First PPID mapping or duplicate for another EE */
+			pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
+		}
+
+		apid_info->ppid = ppid;
+		pmic_arb->last_apid = i;
+	}
+

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) · line 914

Quoted 10 lines
drivers/spmi/spmi-pmic-arb.c +10 -0
+	/* Dump the mapping table for debug purposes. */
+	dev_dbg(&pmic_arb->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
+	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
+		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
+		i = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
+		if (valid) {
+			apid_info = &pmic_arb->apid_data[i];
+			dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
+			      ppid, i, apid_info->write_ee, apid_info->irq_ee);
+		}

Could be

	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
		apid = pmic_arb->ppid_to_apid[ppid];
		if (apid & PMIC_ARB_APID_VALID) {
			apid &= ~PMIC_ARB_VALID;
			apidd = &pmic_arb->apid_data[apid];
			dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
			      ppid, apid, apidd->write_ee, apidd->irq_ee);
		}
	}
Quoted 2 lines

Which maybe is clearer because it uses less local variables that don't get used more than once.

yes. Will change it in the next patch.

author
#6 lore raw

PATCH v3 4/4 spmi: pmic-arb: Remove checking opc value not less than 0

In reply to Kiran Gunda

From: Fenglin Wu <[email protected]>

The opc parameter in pmic_arb_write_cmd() function is defined with type u8 and it's always greater than or equal to 0. Checking that it's not less than 0 is redundant and it can cause a forbidden warning during compilation. Remove the check.

Signed-off-by Fenglin Wu <[email protected]>
Signed-off-by Kiran Gunda <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spmi/spmi-pmic-arb.c +2 -2
index 5297aa4..c2dd001 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012-2017, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2015, 2017, The Linux Foundation. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and

@@ -419,7 +419,7 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	/* Check the opcode */
 	if (opc >= 0x40 && opc <= 0x5F)
 		opc = PMIC_ARB_OP_WRITE;
-	else if (opc >= 0x00 && opc <= 0x0F)
+	else if (opc <= 0x0F)
 		opc = PMIC_ARB_OP_EXT_WRITE;
 	else if (opc >= 0x30 && opc <= 0x37)
 		opc = PMIC_ARB_OP_EXT_WRITEL;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project