Changes in v4:
In [v4 02/11], separated spurious interrupt handling.
In [v4 03/11], added Fixes tag for ("spmi: pmic-arb: do not ack and clear peripheral").
In [v4 11/11], updated the binding to address few warnings in "make dtbs_check"
Changes in v3:
Drop [v2 07/10] as this is no longer needed after this change:
50fc4c8cd240 ("spmi: spmi-pmic-arb: fix irq_set_type race condition")
In [v3 07/10], updated the author email to match with Signed-off-by.
In [v3 10/10], added the binding change in this series, and addressed issues in "make dt_binding_check"
Changes in v2:
In [v2 01/10], added code to handle spurious interrupt.
In [v2 03/10], adressed minor comments to update the code logic.
In [v2 04/10], minor update to detect spurious interrupt.
In [v2 05/10], added Fixes tag.
In [v2 07/10], added Fixes tag and updated commit text to explain the problem.
In [v2 08/10], added binding change to make interrupt properties as optional.
In [v2 09/10], updated to check presence of "interrupt-controller" property.
Abhijeet Dharmapurikar (1):
spmi: pmic-arb: add a print in cleanup_irq
Ashay Jaiswal (1):
spmi: pmic-arb: add support to dispatch interrupt based on IRQ status
David Collins (6):
spmi: pmic-arb: check apid against limits before calling irq handler
spmi: pmic-arb: correct duplicate APID to PPID mapping logic
spmi: pmic-arb: block access for invalid PMIC arbiter v5 SPMI writes
bindings: spmi: spmi-pmic-arb: mark interrupt properties as optional
spmi: pmic-arb: make interrupt support optional
spmi: pmic-arb: increase SPMI transaction timeout delay
Fenglin Wu (2):
spmi: pmic-arb: handle spurious interrupt
dt-bindings: convert qcom,spmi-pmic-arb binding to YAML format
Subbaraman Narayanamurthy (1):
spmi: pmic-arb: do not ack and clear peripheral interrupts in
cleanup_irq
.../bindings/spmi/qcom,spmi-pmic-arb.txt | 65 ---------
.../bindings/spmi/qcom,spmi-pmic-arb.yaml | 146 +++++++++++++++++++++
drivers/spmi/spmi-pmic-arb.c | 136 +++++++++++++------
3 files changed, 242 insertions(+), 105 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
--
2.7.4
From: Abhijeet Dharmapurikar <[email protected]>
The cleanup_irq() was meant to clear and mask interrupts that were
left enabled in the hardware but there was no interrupt handler
registered for it. Add an error print when it gets invoked.
Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 2113be4..5a99723 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -590,6 +590,8 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
u8 per = ppid & 0xFF;
u8 irq_mask = BIT(id);
+ dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d sid=0x%x per=0x%x irq=%d\n",
+ __func__, apid, sid, per, id);
writel_relaxed(irq_mask, pmic_arb->ver_ops->irq_clear(pmic_arb, apid));
if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
--
2.7.4
Call handle_bad_irq() when the summary interrupt is fired spuriously.
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 5a99723..719bd73 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -605,10 +605,11 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
irq_mask, ppid);
}
-static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
+static int periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
{
unsigned int irq;
u32 status, id;
+ int handled = 0;
u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;
@@ -623,7 +624,10 @@ static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
continue;
}
generic_handle_irq(irq);
+ handled++;
}
+
+ return handled;
}
static void pmic_arb_chained_irq(struct irq_desc *desc)
@@ -634,7 +638,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
int first = pmic_arb->min_apid >> 5;
int last = pmic_arb->max_apid >> 5;
u8 ee = pmic_arb->ee;
- u32 status, enable;
+ u32 status, enable, handled = 0;
int i, id, apid;
chained_irq_enter(chip, desc);
@@ -649,10 +653,14 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
enable = readl_relaxed(
ver_ops->acc_enable(pmic_arb, apid));
if (enable & SPMI_PIC_ACC_ENABLE_BIT)
- periph_interrupt(pmic_arb, apid);
+ if (periph_interrupt(pmic_arb, apid) != 0)
+ handled++;
}
}
+ if (handled == 0)
+ handle_bad_irq(desc);
+
chained_irq_exit(chip, desc);
}
--
2.7.4
From: Subbaraman Narayanamurthy <[email protected]>
Currently, cleanup_irq() is invoked when a peripheral's interrupt
fires and there is no mapping present in the interrupt domain of
spmi interrupt controller.
The cleanup_irq clears the arbiter bit, clears the pmic interrupt
and disables it at the pmic in that order. The last disable in
cleanup_irq races with request_irq() in that it stomps over the
enable issued by request_irq. Fix this by not writing to the pmic
in cleanup_irq. The latched bit will be left set in the pmic,
which will not send us more interrupts even if the enable bit
stays enabled.
When a client wants to request an interrupt, use the activate
callback on the irq_domain to clear latched bit. This ensures
that the latched, if set due to the above changes in cleanup_irq
or when the bootloader leaves it set, gets cleaned up, paving way
for upcoming interrupts to trigger.
With this, there is a possibility of unwanted triggering of
interrupt right after the latched bit is cleared - the interrupt
may be left enabled too. To avoid that, clear the enable first
followed by clearing the latched bit in the activate callback.
Fixes: 6bc546e71e50 ("spmi: pmic-arb: cleanup unrequested irqs")
Fixes: 02abec3616c1 ("spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other cleanup")
Signed-off-by: Subbaraman Narayanamurthy <[email protected]>
[[email protected]: fix merge conflict]
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 719bd73..2bc3b88 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -593,16 +593,6 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d sid=0x%x per=0x%x irq=%d\n",
__func__, apid, sid, per, id);
writel_relaxed(irq_mask, pmic_arb->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))
- dev_err_ratelimited(&pmic_arb->spmic->dev, "failed to ack irq_mask = 0x%x for ppid = %x\n",
- irq_mask, ppid);
-
- if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
- (per << 8) + QPNPINT_REG_EN_CLR, &irq_mask, 1))
- dev_err_ratelimited(&pmic_arb->spmic->dev, "failed to ack irq_mask = 0x%x for ppid = %x\n",
- irq_mask, ppid);
}
static int periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
@@ -780,6 +770,7 @@ static int qpnpint_irq_domain_activate(struct irq_domain *domain,
u16 apid = hwirq_to_apid(d->hwirq);
u16 sid = hwirq_to_sid(d->hwirq);
u16 irq = hwirq_to_irq(d->hwirq);
+ u8 buf;
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",
@@ -788,6 +779,10 @@ static int qpnpint_irq_domain_activate(struct irq_domain *domain,
return -ENODEV;
}
+ buf = BIT(irq);
+ qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &buf, 1);
+ qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &buf, 1);
+
return 0;
}
--
2.7.4
From: David Collins <[email protected]>
Check that the apid for an SPMI interrupt falls between the
min_apid and max_apid that can be handled by the APPS processor
before invoking the per-apid interrupt handler:
periph_interrupt().
This avoids an access violation in rare cases where the status
bit is set for an interrupt that is not owned by the APPS
processor.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 2bc3b88..e19eaec 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -625,21 +625,26 @@ 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);
- int first = pmic_arb->min_apid >> 5;
- int last = pmic_arb->max_apid >> 5;
+ int first = pmic_arb->min_apid;
+ int last = pmic_arb->max_apid;
u8 ee = pmic_arb->ee;
u32 status, enable, handled = 0;
int i, id, apid;
chained_irq_enter(chip, desc);
- for (i = first; i <= last; ++i) {
+ for (i = first >> 5; i <= last >> 5; ++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;
+ if (apid < first || apid > last) {
+ WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
+ apid);
+ continue;
+ }
enable = readl_relaxed(
ver_ops->acc_enable(pmic_arb, apid));
if (enable & SPMI_PIC_ACC_ENABLE_BIT)
--
2.7.4
From: Ashay Jaiswal <[email protected]>
Current implementation of SPMI arbiter dispatches interrupt based on the
Arbiter's accumulator status, in some cases the accumulator status may
remain zero and the interrupt remains un-handled. Add logic to dispatch
interrupts based Arbiter's IRQ status if the accumulator status is zero.
Signed-off-by: Ashay Jaiswal <[email protected]>
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index e19eaec..56f2294 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -630,12 +630,18 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
u8 ee = pmic_arb->ee;
u32 status, enable, handled = 0;
int i, id, apid;
+ /* status based dispatch */
+ bool acc_valid = false;
+ u32 irq_status = 0;
chained_irq_enter(chip, desc);
for (i = first >> 5; i <= last >> 5; ++i) {
status = readl_relaxed(
ver_ops->owner_acc_status(pmic_arb, ee, i));
+ if (status)
+ acc_valid = true;
+
while (status) {
id = ffs(status) - 1;
status &= ~BIT(id);
@@ -653,6 +659,29 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
}
}
+ /* ACC_STATUS is empty but IRQ fired check IRQ_STATUS */
+ if (!acc_valid) {
+ for (i = first; i <= last; i++) {
+ /* skip if APPS is not irq owner */
+ if (pmic_arb->apid_data[i].irq_ee != pmic_arb->ee)
+ continue;
+
+ irq_status = readl_relaxed(
+ ver_ops->irq_status(pmic_arb, i));
+ if (irq_status) {
+ enable = readl_relaxed(
+ ver_ops->acc_enable(pmic_arb, i));
+ if (enable & SPMI_PIC_ACC_ENABLE_BIT) {
+ dev_dbg(&pmic_arb->spmic->dev,
+ "Dispatching IRQ for apid=%d status=%x\n",
+ i, irq_status);
+ if (periph_interrupt(pmic_arb, i) != 0)
+ handled++;
+ }
+ }
+ }
+ }
+
if (handled == 0)
handle_bad_irq(desc);
--
2.7.4
From: David Collins <[email protected]>
Correct the way that duplicate PPID mappings are handled for PMIC
arbiter v5. The final APID mapped to a given PPID should be the
one which has write owner = APPS EE, if it exists, or if not
that, then the first APID mapped to the PPID, if it exists.
Fixes: 40f318f0ed67 ("spmi: pmic-arb: add support for HW version 5")
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 56f2294..cf92abc 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1031,7 +1031,8 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
* 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.
+ * which has the IRQ owner bit set for a given PPID will receive
+ * interrupts from the PPID.
*/
for (i = 0; ; i++, apidd++) {
offset = pmic_arb->ver_ops->apid_map_offset(i);
@@ -1054,16 +1055,16 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
apid = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
prev_apidd = &pmic_arb->apid_data[apid];
- if (valid && is_irq_ee &&
- prev_apidd->write_ee == pmic_arb->ee) {
+ if (!valid || apidd->write_ee == pmic_arb->ee) {
+ /* First PPID mapping or one for this EE */
+ pmic_arb->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;
- } 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;
}
apidd->ppid = ppid;
--
2.7.4
From: David Collins <[email protected]>
Mark all interrupt related properties as optional instead of
required. Some boards do not required PMIC IRQ support and it
isn't needed to handle SPMI bus transactions, so specify it as
optional.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
index ca645e2..6332507 100644
--- a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
+++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
@@ -29,6 +29,8 @@ Required properties:
- #size-cells : must be set to 0
- qcom,ee : indicates the active Execution Environment identifier (0-5)
- qcom,channel : which of the PMIC Arb provided channels to use for accesses (0-5)
+
+Optional properties:
- interrupts : interrupt list for the PMIC Arb controller, must contain a
single interrupt entry for the peripheral interrupt
- interrupt-names : corresponding interrupt names for the interrupts
--
2.7.4
From: David Collins <[email protected]>
Make the support of PMIC peripheral interrupts optional for
spmi-pmic-arb devices. This is useful in situations where
SPMI address mapping is required without the need for IRQ
support.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 39f25bc..0496e5d 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1386,10 +1386,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
goto err_put_ctrl;
}
- pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
- if (pmic_arb->irq < 0) {
- err = pmic_arb->irq;
- goto err_put_ctrl;
+ if (of_find_property(pdev->dev.of_node, "interrupt-controller", NULL)) {
+ pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
+ if (pmic_arb->irq < 0) {
+ err = pmic_arb->irq;
+ goto err_put_ctrl;
+ }
}
err = of_property_read_u32(pdev->dev.of_node, "qcom,channel", &channel);
@@ -1449,17 +1451,22 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
}
}
- 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);
- if (!pmic_arb->domain) {
- dev_err(&pdev->dev, "unable to create irq_domain\n");
- err = -ENOMEM;
- goto err_put_ctrl;
+ if (pmic_arb->irq > 0) {
+ 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);
+ if (!pmic_arb->domain) {
+ dev_err(&pdev->dev, "unable to create irq_domain\n");
+ err = -ENOMEM;
+ goto err_put_ctrl;
+ }
+
+ irq_set_chained_handler_and_data(pmic_arb->irq,
+ pmic_arb_chained_irq, pmic_arb);
+ } else {
+ dev_dbg(&pdev->dev, "not supporting PMIC interrupts\n");
}
- irq_set_chained_handler_and_data(pmic_arb->irq, pmic_arb_chained_irq,
- pmic_arb);
err = spmi_controller_add(ctrl);
if (err)
goto err_domain_remove;
@@ -1467,8 +1474,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
return 0;
err_domain_remove:
- irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
- irq_domain_remove(pmic_arb->domain);
+ if (pmic_arb->irq > 0) {
+ irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+ irq_domain_remove(pmic_arb->domain);
+ }
err_put_ctrl:
spmi_controller_put(ctrl);
return err;
@@ -1479,8 +1488,10 @@ static int spmi_pmic_arb_remove(struct platform_device *pdev)
struct spmi_controller *ctrl = platform_get_drvdata(pdev);
struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
spmi_controller_remove(ctrl);
- irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
- irq_domain_remove(pmic_arb->domain);
+ if (pmic_arb->irq > 0) {
+ irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+ irq_domain_remove(pmic_arb->domain);
+ }
spmi_controller_put(ctrl);
return 0;
}
--
2.7.4
From: David Collins <[email protected]>
The system crashes due to an access permission violation when
writing to a PMIC peripheral which is not owned by the current
ee. Add a check for PMIC arbiter version 5 for such invalid
write requests and return an error instead of crashing the
system.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index cf92abc..39f25bc 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1133,6 +1133,11 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
offset = 0x10000 * pmic_arb->ee + 0x80 * apid;
break;
case PMIC_ARB_CHANNEL_RW:
+ if (pmic_arb->apid_data[apid].write_ee != pmic_arb->ee) {
+ dev_err(&pmic_arb->spmic->dev, "disallowed SPMI write to sid=%u, addr=0x%04X\n",
+ sid, addr);
+ return -EPERM;
+ }
offset = 0x10000 * apid;
break;
}
--
2.7.4
From: David Collins <[email protected]>
Increase the SPMI transaction timeout delay from 100 us to
1000 us in order to account for the slower execution time
found on some simulator targets.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 0496e5d..45f9344 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -91,7 +91,7 @@ enum pmic_arb_channel {
/* Maximum number of support PMIC peripherals */
#define PMIC_ARB_MAX_PERIPHS 512
-#define PMIC_ARB_TIMEOUT_US 100
+#define PMIC_ARB_TIMEOUT_US 1000
#define PMIC_ARB_MAX_TRANS_BYTES (8)
#define PMIC_ARB_APID_MASK 0xFF
--
2.7.4
Convert the SPMI PMIC arbiter documentation to JSON/yaml.
Signed-off-by: Fenglin Wu <[email protected]>
---
.../bindings/spmi/qcom,spmi-pmic-arb.txt | 67 ----------
.../bindings/spmi/qcom,spmi-pmic-arb.yaml | 146 +++++++++++++++++++++
2 files changed, 146 insertions(+), 67 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
deleted file mode 100644
index 6332507..0000000
--- a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
+++ /dev/null
@@ -1,67 +0,0 @@
-Qualcomm SPMI Controller (PMIC Arbiter)
-
-The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
-controller with wrapping arbitration logic to allow for multiple on-chip
-devices to control a single SPMI master.
-
-The PMIC Arbiter can also act as an interrupt controller, providing interrupts
-to slave devices.
-
-See Documentation/devicetree/bindings/spmi/spmi.yaml for the generic SPMI
-controller binding requirements for child nodes.
-
-See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt for
-generic interrupt controller binding documentation.
-
-Required properties:
-- compatible : should be "qcom,spmi-pmic-arb".
-- reg-names : must contain:
- "core" - core registers
- "intr" - interrupt controller registers
- "cnfg" - configuration registers
- Registers used only for V2 PMIC Arbiter:
- "chnls" - tx-channel per virtual slave registers.
- "obsrvr" - rx-channel (called observer) per virtual slave registers.
-
-- reg : address + size pairs describing the PMIC arb register sets; order must
- correspond with the order of entries in reg-names
-- #address-cells : must be set to 2
-- #size-cells : must be set to 0
-- qcom,ee : indicates the active Execution Environment identifier (0-5)
-- qcom,channel : which of the PMIC Arb provided channels to use for accesses (0-5)
-
-Optional properties:
-- interrupts : interrupt list for the PMIC Arb controller, must contain a
- single interrupt entry for the peripheral interrupt
-- interrupt-names : corresponding interrupt names for the interrupts
- listed in the 'interrupts' property, must contain:
- "periph_irq" - summary interrupt for PMIC peripherals
-- interrupt-controller : boolean indicator that the PMIC arbiter is an interrupt controller
-- #interrupt-cells : must be set to 4. Interrupts are specified as a 4-tuple:
- cell 1: slave ID for the requested interrupt (0-15)
- cell 2: peripheral ID for requested interrupt (0-255)
- cell 3: the requested peripheral interrupt (0-7)
- cell 4: interrupt flags indicating level-sense information, as defined in
- dt-bindings/interrupt-controller/irq.h
-
-Example:
-
- spmi {
- compatible = "qcom,spmi-pmic-arb";
- reg-names = "core", "intr", "cnfg";
- reg = <0xfc4cf000 0x1000>,
- <0xfc4cb000 0x1000>,
- <0xfc4ca000 0x1000>;
-
- interrupt-names = "periph_irq";
- interrupts = <0 190 0>;
-
- qcom,ee = <0>;
- qcom,channel = <0>;
-
- #address-cells = <2>;
- #size-cells = <0>;
-
- interrupt-controller;
- #interrupt-cells = <4>;
- };
diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
new file mode 100644
index 0000000..df8cfb7
--- /dev/null
+++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spmi/qcom,spmi-pmic-arb.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SPMI PMIC Arbiter
+
+maintainers:
+ - Fenglin Wu <[email protected]>
+ - Subbaraman Narayanamurthy <[email protected]>
+
+description: |
+ The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
+ controller with wrapping arbitration logic to allow for multiple
+ on-chip devices to control a single SPMI master.
+
+ The PMIC Arbiter can also act as an interrupt controller, providing
+ interrupts to slave devices.
+
+ See Documentation/devicetree/bindings/spmi/spmi.yaml for the generic
+ SPMI controller binding requirements for child nodes.
+
+allOf:
+ - $ref: spmi.yaml#
+
+properties:
+ $nodename:
+ pattern: "^spmi@.*"
+
+ compatible:
+ const: qcom,spmi-pmic-arb
+
+ reg-names:
+ $ref: /schemas/types.yaml#/definitions/string-array
+ anyOf:
+ - minItems: 3
+ - maxItems: 3
+ - enum: ["core", "intr", "cnfg"]
+
+ - minItems: 5
+ - maxItems: 5
+ - enum: ["core", "intr", "cnfg", "chnls", "obsrvr"]
+
+ reg:
+ minItems: 3
+ maxItems: 5
+ description: |
+ Specifies base physical address and size of the registers in SPMI PMIC
+ Arbiter HW module, with the following order.
+ - SPMI PMIC arbiter core registers (core)
+ - SPMI PMIC arbiter interrupt controller registers (intr)
+ - SPMI PMIC arbiter configuration registers (cnfg)
+ - SPMI PMIC arbiter tx-channel per virtual slave registers (chnls)
+ - SPMI PMIC arbiter rx-channel per virtual slave registers (obsrvr).
+ Register for "chnls" and "obsrvr" are only applicable for PMIC arbiter
+ with HW version greater than V2.
+
+ "#address-cells":
+ const: 2
+
+ "#size-cells":
+ const: 0
+
+ interrupts:
+ description: The summary interrupt for the PMIC Arb controller.
+ maxItems: 1
+
+ interrupt-names:
+ const: periph_irq
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 4
+ description: |
+ Specifies the number of cells needed to encode any interrupt source.
+ The 1st cell is the slave ID for the requested interrupt, its valid
+ range is [0-15].
+ The 2nd cell is the peripheral ID for requested interrupt, its valid
+ range is [0-255].
+ The 3rd cell is the requested peripheral interrupt, its valid range
+ is [0-7].
+ The 4th cell is interrupt flags indicating level-sense information,
+ as defined in dt-bindings/interrupt-controller/irq.h
+
+ qcom,ee:
+ description: the active Execution Environment identifier
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2, 3, 4, 5]
+
+ qcom,channel:
+ description: which of the PMIC Arbiter provided channels to use for accesses
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2, 3, 4, 5]
+
+patternProperties:
+ "@[0-9a-f]$":
+ description: up to 16 child PMIC nodes
+ type: object
+
+ properties:
+ reg:
+ items:
+ - minItems: 1
+ items:
+ - minimum: 0
+ maximum: 0xf
+ - enum: [ 0 ]
+ description:
+ 0 means user ID address. 1 is reserved for group ID
+ address.
+
+ required:
+ - reg
+
+required:
+ - compatible
+ - reg-names
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+ - qcom,ee
+ - qcom,channel
+
+additionalProperties: false
+
+examples:
+ - |
+ spmi@fc4cf000 {
+ compatible = "qcom,spmi-pmic-arb";
+ reg-names = "core", "intr", "cnfg";
+ reg = <0xfc4cf000 0x1000>,
+ <0xfc4cb000 0x1000>,
+ <0xfc4ca000 0x1000>;
+ interrupt-names = "periph_irq";
+ interrupts = <0 190 0>;
+ interrupt-controller;
+ #interrupt-cells = <4>;
+
+ qcom,ee = <0>;
+ qcom,channel = <0>;
+
+ #address-cells = <2>;
+ #size-cells = <0>;
+ };
--
2.7.4
On Tue, 21 Dec 2021 15:20:09 +0800, Fenglin Wu wrote:
> Convert the SPMI PMIC arbiter documentation to JSON/yaml.
>
> Signed-off-by: Fenglin Wu <[email protected]>
> ---
> .../bindings/spmi/qcom,spmi-pmic-arb.txt | 67 ----------
> .../bindings/spmi/qcom,spmi-pmic-arb.yaml | 146 +++++++++++++++++++++
> 2 files changed, 146 insertions(+), 67 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.example.dt.yaml: spmi@fc4cf000: reg: [[4232900608, 4096], [4232884224, 4096], [4232880128, 4096]] is too long
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/spmi.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.example.dt.yaml: spmi@fc4cf000: reg: [[4232900608, 4096], [4232884224, 4096], [4232880128, 4096]] is too long
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
doc reference errors (make refcheckdocs):
Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt: Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
See https://patchwork.ozlabs.org/patch/1571409
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
On Tue, Dec 21, 2021 at 03:20:09PM +0800, Fenglin Wu wrote:
> Convert the SPMI PMIC arbiter documentation to JSON/yaml.
>
> Signed-off-by: Fenglin Wu <[email protected]>
> ---
> .../bindings/spmi/qcom,spmi-pmic-arb.txt | 67 ----------
> .../bindings/spmi/qcom,spmi-pmic-arb.yaml | 146 +++++++++++++++++++++
> 2 files changed, 146 insertions(+), 67 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>
> diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
> new file mode 100644
> index 0000000..df8cfb7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
> @@ -0,0 +1,146 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spmi/qcom,spmi-pmic-arb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SPMI PMIC Arbiter
> +
> +maintainers:
> + - Fenglin Wu <[email protected]>
> + - Subbaraman Narayanamurthy <[email protected]>
> +
> +description: |
> + The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
> + controller with wrapping arbitration logic to allow for multiple
> + on-chip devices to control a single SPMI master.
> +
> + The PMIC Arbiter can also act as an interrupt controller, providing
> + interrupts to slave devices.
> +
> + See Documentation/devicetree/bindings/spmi/spmi.yaml for the generic
> + SPMI controller binding requirements for child nodes.
> +
> +allOf:
> + - $ref: spmi.yaml#
> +
> +properties:
> + $nodename:
> + pattern: "^spmi@.*"
> +
> + compatible:
> + const: qcom,spmi-pmic-arb
> +
> + reg-names:
> + $ref: /schemas/types.yaml#/definitions/string-array
reg-names already has a type defined.
> + anyOf:
> + - minItems: 3
> + - maxItems: 3
> + - enum: ["core", "intr", "cnfg"]
> +
> + - minItems: 5
> + - maxItems: 5
> + - enum: ["core", "intr", "cnfg", "chnls", "obsrvr"]
I think you want something like this:
minItems: 3
items:
- const: core
- const: intr
- const: cnfg
- const: chnls
- const: obsrvr
> +
> + reg:
> + minItems: 3
> + maxItems: 5
> + description: |
> + Specifies base physical address and size of the registers in SPMI PMIC
> + Arbiter HW module, with the following order.
> + - SPMI PMIC arbiter core registers (core)
> + - SPMI PMIC arbiter interrupt controller registers (intr)
> + - SPMI PMIC arbiter configuration registers (cnfg)
> + - SPMI PMIC arbiter tx-channel per virtual slave registers (chnls)
> + - SPMI PMIC arbiter rx-channel per virtual slave registers (obsrvr).
> + Register for "chnls" and "obsrvr" are only applicable for PMIC arbiter
> + with HW version greater than V2.
> +
> + "#address-cells":
> + const: 2
> +
> + "#size-cells":
> + const: 0
> +
> + interrupts:
> + description: The summary interrupt for the PMIC Arb controller.
> + maxItems: 1
> +
> + interrupt-names:
> + const: periph_irq
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 4
> + description: |
> + Specifies the number of cells needed to encode any interrupt source.
> + The 1st cell is the slave ID for the requested interrupt, its valid
> + range is [0-15].
> + The 2nd cell is the peripheral ID for requested interrupt, its valid
> + range is [0-255].
> + The 3rd cell is the requested peripheral interrupt, its valid range
> + is [0-7].
> + The 4th cell is interrupt flags indicating level-sense information,
> + as defined in dt-bindings/interrupt-controller/irq.h
> +
> + qcom,ee:
> + description: the active Execution Environment identifier
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5]
> +
> + qcom,channel:
> + description: which of the PMIC Arbiter provided channels to use for accesses
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5]
> +
> +patternProperties:
> + "@[0-9a-f]$":
> + description: up to 16 child PMIC nodes
> + type: object
> +
> + properties:
> + reg:
> + items:
> + - minItems: 1
> + items:
> + - minimum: 0
> + maximum: 0xf
> + - enum: [ 0 ]
> + description:
> + 0 means user ID address. 1 is reserved for group ID
> + address.
> +
> + required:
> + - reg
All this should be covered by spmi.yaml
> +
> +required:
> + - compatible
> + - reg-names
> + - reg
> + - "#address-cells"
> + - "#size-cells"
> + - qcom,ee
> + - qcom,channel
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + spmi@fc4cf000 {
> + compatible = "qcom,spmi-pmic-arb";
> + reg-names = "core", "intr", "cnfg";
> + reg = <0xfc4cf000 0x1000>,
> + <0xfc4cb000 0x1000>,
> + <0xfc4ca000 0x1000>;
> + interrupt-names = "periph_irq";
> + interrupts = <0 190 0>;
> + interrupt-controller;
> + #interrupt-cells = <4>;
> +
> + qcom,ee = <0>;
> + qcom,channel = <0>;
> +
> + #address-cells = <2>;
> + #size-cells = <0>;
> + };
> --
> 2.7.4
>
>
On Tue, 21 Dec 2021 15:20:06 +0800, Fenglin Wu wrote:
> From: David Collins <[email protected]>
>
> Mark all interrupt related properties as optional instead of
> required. Some boards do not required PMIC IRQ support and it
> isn't needed to handle SPMI bus transactions, so specify it as
> optional.
>
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Fenglin Wu <[email protected]>
> ---
> Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.
If a tag was not added on purpose, please state why and what changed.
On 2021/12/21 19:11, Rob Herring wrote:
> On Tue, 21 Dec 2021 15:20:09 +0800, Fenglin Wu wrote:
>> Convert the SPMI PMIC arbiter documentation to JSON/yaml.
>>
>> Signed-off-by: Fenglin Wu <[email protected]>
>> ---
>> .../bindings/spmi/qcom,spmi-pmic-arb.txt | 67 ----------
>> .../bindings/spmi/qcom,spmi-pmic-arb.yaml | 146 +++++++++++++++++++++
>> 2 files changed, 146 insertions(+), 67 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
>> create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.example.dt.yaml: spmi@fc4cf000: reg: [[4232900608, 4096], [4232884224, 4096], [4232880128, 4096]] is too long
> From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/spmi.yaml
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.example.dt.yaml: spmi@fc4cf000: reg: [[4232900608, 4096], [4232884224, 4096], [4232880128, 4096]] is too long
> From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
I re-based the change on the tip of spmi-next project which has this
change included:
https://lore.kernel.org/all/[email protected]/
With it, the constraint should be removed and this warning/error won't
be seen.
> doc reference errors (make refcheckdocs):
> Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt: Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
>
> See https://patchwork.ozlabs.org/patch/1571409
>
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit.
>
resend with plain text
On 2021/12/21 22:42, Rob Herring wrote:
> On Tue, Dec 21, 2021 at 03:20:09PM +0800, Fenglin Wu wrote:
>> Convert the SPMI PMIC arbiter documentation to JSON/yaml.
>>
>> Signed-off-by: Fenglin Wu <[email protected]>
>> ---
>> .../bindings/spmi/qcom,spmi-pmic-arb.txt | 67 ----------
>> .../bindings/spmi/qcom,spmi-pmic-arb.yaml | 146 +++++++++++++++++++++
>> 2 files changed, 146 insertions(+), 67 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
>> create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>>
>
>> diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>> new file mode 100644
>> index 0000000..df8cfb7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>> @@ -0,0 +1,146 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spmi/qcom,spmi-pmic-arb.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm SPMI PMIC Arbiter
>> +
>> +maintainers:
>> + - Fenglin Wu <[email protected]>
>> + - Subbaraman Narayanamurthy <[email protected]>
>> +
>> +description: |
>> + The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
>> + controller with wrapping arbitration logic to allow for multiple
>> + on-chip devices to control a single SPMI master.
>> +
>> + The PMIC Arbiter can also act as an interrupt controller, providing
>> + interrupts to slave devices.
>> +
>> + See Documentation/devicetree/bindings/spmi/spmi.yaml for the generic
>> + SPMI controller binding requirements for child nodes.
>> +
>> +allOf:
>> + - $ref: spmi.yaml#
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^spmi@.*"
>> +
>> + compatible:
>> + const: qcom,spmi-pmic-arb
>> +
>> + reg-names:
>> + $ref: /schemas/types.yaml#/definitions/string-array
>
> reg-names already has a type defined.
I understand there is a pattern property defined in dt-core.yaml and it
defines ".*-names" as a "non-unique-string-array" type. But here, the
strings in "reg-names" needs to be unique and it has to be ["core",
"intr", "cnfg"] or ["core", "intr", "cnfg", "chnls", "obsrvr"] , that's
why I redefined it as "string-array" type which requires each string to
be unique. Otherwise, if any dtsi nodes define the "reg-name" as
["core", "core", "core"] will not be caught as a fault.
>
>> + anyOf:
>> + - minItems: 3
>> + - maxItems: 3
>> + - enum: ["core", "intr", "cnfg"]
>> +
>> + - minItems: 5
>> + - maxItems: 5
>> + - enum: ["core", "intr", "cnfg", "chnls", "obsrvr"]
>
> I think you want something like this:
>
> minItems: 3
> items:
> - const: core
> - const: intr
> - const: cnfg
> - const: chnls
> - const: obsrvr
>
>
As I said, the content for "reg-names" here only has two options ,
either ["core", "intr", "cnfg"] or ["core", "intr", "cnfg", "chnls",
"obsrvr"]. In patch V3, I defined it as below and "make dtbs_check"
threw out warnings because some of existing nodes defined "reg-names"
with these strings are not having the same order as I defined here (I
understood from the warnings that const items need to be followed
strictly even in order wise, is this correct?), and I guess the order of
the strings doesn't matter here and the schema here shouldn't have such
limitation, so I updated it as the "array-string" type and specified the
tuples can only be one of the strings defined in the enum. With this,
the previous warning regarding "reg-names" in "make dtbs_check" are all
fixed.
reg-names:
oneOf:
- items:
- const: core
- const: intr
- const: cnfg
- items:
- const: core
- const: intr
- const: cnfg
- const: chnls
- const: obsrvr
>> +
>> + reg:
>> + minItems: 3
>> + maxItems: 5
>> + description: |
>> + Specifies base physical address and size of the registers in SPMI PMIC
>> + Arbiter HW module, with the following order.
>> + - SPMI PMIC arbiter core registers (core)
>> + - SPMI PMIC arbiter interrupt controller registers (intr)
>> + - SPMI PMIC arbiter configuration registers (cnfg)
>> + - SPMI PMIC arbiter tx-channel per virtual slave registers (chnls)
>> + - SPMI PMIC arbiter rx-channel per virtual slave registers (obsrvr).
>> + Register for "chnls" and "obsrvr" are only applicable for PMIC arbiter
>> + with HW version greater than V2.
>> +
>> + "#address-cells":
>> + const: 2
>> +
>> + "#size-cells":
>> + const: 0
>> +
>> + interrupts:
>> + description: The summary interrupt for the PMIC Arb controller.
>> + maxItems: 1
>> +
>> + interrupt-names:
>> + const: periph_irq
>> +
>> + interrupt-controller: true
>> +
>> + "#interrupt-cells":
>> + const: 4
>> + description: |
>> + Specifies the number of cells needed to encode any interrupt source.
>> + The 1st cell is the slave ID for the requested interrupt, its valid
>> + range is [0-15].
>> + The 2nd cell is the peripheral ID for requested interrupt, its valid
>> + range is [0-255].
>> + The 3rd cell is the requested peripheral interrupt, its valid range
>> + is [0-7].
>> + The 4th cell is interrupt flags indicating level-sense information,
>> + as defined in dt-bindings/interrupt-controller/irq.h
>> +
>> + qcom,ee:
>> + description: the active Execution Environment identifier
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1, 2, 3, 4, 5]
>> +
>> + qcom,channel:
>> + description: which of the PMIC Arbiter provided channels to use for accesses
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1, 2, 3, 4, 5]
>> +
>
>> +patternProperties:
>> + "@[0-9a-f]$":
>> + description: up to 16 child PMIC nodes
>> + type: object
>> +
>> + properties:
>> + reg:
>> + items:
>> + - minItems: 1
>> + items:
>> + - minimum: 0
>> + maximum: 0xf
>> + - enum: [ 0 ]
>> + description:
>> + 0 means user ID address. 1 is reserved for group ID
>> + address.
>> +
>> + required:
>> + - reg
>
> All this should be covered by spmi.yaml
>
>> +
>> +required:
>> + - compatible
>> + - reg-names
>> + - reg
>> + - "#address-cells"
>> + - "#size-cells"
>> + - qcom,ee
>> + - qcom,channel
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + spmi@fc4cf000 {
>> + compatible = "qcom,spmi-pmic-arb";
>> + reg-names = "core", "intr", "cnfg";
>> + reg = <0xfc4cf000 0x1000>,
>> + <0xfc4cb000 0x1000>,
>> + <0xfc4ca000 0x1000>;
>> + interrupt-names = "periph_irq";
>> + interrupts = <0 190 0>;
>> + interrupt-controller;
>> + #interrupt-cells = <4>;
>> +
>> + qcom,ee = <0>;
>> + qcom,channel = <0>;
>> +
>> + #address-cells = <2>;
>> + #size-cells = <0>;
>> + };
>> --
>> 2.7.4
>>
>>
On 2021/12/21 23:13, Rob Herring wrote:
> On Tue, 21 Dec 2021 15:20:06 +0800, Fenglin Wu wrote:
>> From: David Collins <[email protected]>
>>
>> Mark all interrupt related properties as optional instead of
>> required. Some boards do not required PMIC IRQ support and it
>> isn't needed to handle SPMI bus transactions, so specify it as
>> optional.
>>
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Fenglin Wu <[email protected]>
>> ---
>> Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>
>
> Please add Acked-by/Reviewed-by tags when posting new versions. However,
> there's no need to repost patches *only* to add the tags. The upstream
> maintainer will do that for acks received on the version they apply.
>
> If a tag was not added on purpose, please state why and what changed.
>
I will remember to add the Acked-by/Reviewed-by tags next time when
sending any update in this series. Thanks for the notice!
On 2021/12/22 8:45, Fenglin Wu wrote:
> resend with plain text
>
>
> On 2021/12/21 22:42, Rob Herring wrote:
>> On Tue, Dec 21, 2021 at 03:20:09PM +0800, Fenglin Wu wrote:
>>> Convert the SPMI PMIC arbiter documentation to JSON/yaml.
>>>
>>> Signed-off-by: Fenglin Wu <[email protected]>
>>> ---
>>> .../bindings/spmi/qcom,spmi-pmic-arb.txt | 67 ----------
>>> .../bindings/spmi/qcom,spmi-pmic-arb.yaml | 146
>>> +++++++++++++++++++++
>>> 2 files changed, 146 insertions(+), 67 deletions(-)
>>> delete mode 100644
>>> Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
>>> create mode 100644
>>> Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>>>
>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>>> b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>>> new file mode 100644
>>> index 0000000..df8cfb7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.yaml
>>> @@ -0,0 +1,146 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/spmi/qcom,spmi-pmic-arb.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm SPMI PMIC Arbiter
>>> +
>>> +maintainers:
>>> + - Fenglin Wu <[email protected]>
>>> + - Subbaraman Narayanamurthy <[email protected]>
>>> +
>>> +description: |
>>> + The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
>>> + controller with wrapping arbitration logic to allow for multiple
>>> + on-chip devices to control a single SPMI master.
>>> +
>>> + The PMIC Arbiter can also act as an interrupt controller, providing
>>> + interrupts to slave devices.
>>> +
>>> + See Documentation/devicetree/bindings/spmi/spmi.yaml for the generic
>>> + SPMI controller binding requirements for child nodes.
>>> +
>>> +allOf:
>>> + - $ref: spmi.yaml#
>>> +
>>> +properties:
>>> + $nodename:
>>> + pattern: "^spmi@.*"
>>> +
>>> + compatible:
>>> + const: qcom,spmi-pmic-arb
>>> +
>>> + reg-names:
>>> + $ref: /schemas/types.yaml#/definitions/string-array
>>
>> reg-names already has a type defined.
> I understand there is a pattern property defined in dt-core.yaml and it
> defines ".*-names" as a "non-unique-string-array" type. But here, the
> strings in "reg-names" needs to be unique and it has to be ["core",
> "intr", "cnfg"] or ["core", "intr", "cnfg", "chnls", "obsrvr"] , that's
> why I redefined it as "string-array" type which requires each string to
> be unique. Otherwise, if any dtsi nodes define the "reg-name" as
> ["core", "core", "core"] will not be caught as a fault.
>
>>
>>> + anyOf:
>>> + - minItems: 3
>>> + - maxItems: 3
>>> + - enum: ["core", "intr", "cnfg"]
>>> +
>>> + - minItems: 5
>>> + - maxItems: 5
>>> + - enum: ["core", "intr", "cnfg", "chnls", "obsrvr"]
>>
>> I think you want something like this:
>>
>> minItems: 3
>> items:
>> - const: core
>> - const: intr
>> - const: cnfg
>> - const: chnls
>> - const: obsrvr
>>
>>
> As I said, the content for "reg-names" here only has two options ,
> either ["core", "intr", "cnfg"] or ["core", "intr", "cnfg", "chnls",
> "obsrvr"]. In patch V3, I defined it as below and "make dtbs_check"
> threw out warnings because some of existing nodes defined "reg-names"
> with these strings are not having the same order as I defined here (I
> understood from the warnings that const items need to be followed
> strictly even in order wise, is this correct?), and I guess the order of
> the strings doesn't matter here and the schema here shouldn't have such
> limitation, so I updated it as the "array-string" type and specified the
> tuples can only be one of the strings defined in the enum. With this,
> the previous warning regarding "reg-names" in "make dtbs_check" are all
> fixed.
>
> reg-names:
> oneOf:
> - items:
> - const: core
> - const: intr
> - const: cnfg
> - items:
> - const: core
> - const: intr
> - const: cnfg
> - const: chnls
> - const: obsrvr
>
Can you help to confirm if I need to change this back to what has been
defined in PATCH v3 but just ignore those "make dtbs_check" warnings?
Thanks
>
>>> +
>>> + reg:
>>> + minItems: 3
>>> + maxItems: 5
>>> + description: |
>>> + Specifies base physical address and size of the registers in
>>> SPMI PMIC
>>> + Arbiter HW module, with the following order.
>>> + - SPMI PMIC arbiter core registers (core)
>>> + - SPMI PMIC arbiter interrupt controller registers (intr)
>>> + - SPMI PMIC arbiter configuration registers (cnfg)
>>> + - SPMI PMIC arbiter tx-channel per virtual slave registers
>>> (chnls)
>>> + - SPMI PMIC arbiter rx-channel per virtual slave registers
>>> (obsrvr).
>>> + Register for "chnls" and "obsrvr" are only applicable for PMIC
>>> arbiter
>>> + with HW version greater than V2.
>>> +
>>> + "#address-cells":
>>> + const: 2
>>> +
>>> + "#size-cells":
>>> + const: 0
>>> +
>>> + interrupts:
>>> + description: The summary interrupt for the PMIC Arb controller.
>>> + maxItems: 1
>>> +
>>> + interrupt-names:
>>> + const: periph_irq
>>> +
>>> + interrupt-controller: true
>>> +
>>> + "#interrupt-cells":
>>> + const: 4
>>> + description: |
>>> + Specifies the number of cells needed to encode any interrupt
>>> source.
>>> + The 1st cell is the slave ID for the requested interrupt, its
>>> valid
>>> + range is [0-15].
>>> + The 2nd cell is the peripheral ID for requested interrupt,
>>> its valid
>>> + range is [0-255].
>>> + The 3rd cell is the requested peripheral interrupt, its valid
>>> range
>>> + is [0-7].
>>> + The 4th cell is interrupt flags indicating level-sense
>>> information,
>>> + as defined in dt-bindings/interrupt-controller/irq.h
>>> +
>>> + qcom,ee:
>>> + description: the active Execution Environment identifier
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + enum: [0, 1, 2, 3, 4, 5]
>>> +
>>> + qcom,channel:
>>> + description: which of the PMIC Arbiter provided channels to use
>>> for accesses
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + enum: [0, 1, 2, 3, 4, 5]
>>> +
>>
>>> +patternProperties:
>>> + "@[0-9a-f]$":
>>> + description: up to 16 child PMIC nodes
>>> + type: object
>>> +
>>> + properties:
>>> + reg:
>>> + items:
>>> + - minItems: 1
>>> + items:
>>> + - minimum: 0
>>> + maximum: 0xf
>>> + - enum: [ 0 ]
>>> + description:
>>> + 0 means user ID address. 1 is reserved for group ID
>>> + address.
>>> +
>>> + required:
>>> + - reg
>>
>> All this should be covered by spmi.yaml
>>
>>> +
>>> +required:
>>> + - compatible
>>> + - reg-names
>>> + - reg
>>> + - "#address-cells"
>>> + - "#size-cells"
>>> + - qcom,ee
>>> + - qcom,channel
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + spmi@fc4cf000 {
>>> + compatible = "qcom,spmi-pmic-arb";
>>> + reg-names = "core", "intr", "cnfg";
>>> + reg = <0xfc4cf000 0x1000>,
>>> + <0xfc4cb000 0x1000>,
>>> + <0xfc4ca000 0x1000>;
>>> + interrupt-names = "periph_irq";
>>> + interrupts = <0 190 0>;
>>> + interrupt-controller;
>>> + #interrupt-cells = <4>;
>>> +
>>> + qcom,ee = <0>;
>>> + qcom,channel = <0>;
>>> +
>>> + #address-cells = <2>;
>>> + #size-cells = <0>;
>>> + };
>>> --
>>> 2.7.4
>>>
>>>