| Message ID | 1640071211-31462-1-git-send-email-quic_fenglinw@quicinc.com |
|---|---|
| Headers | show |
| Series | A bunch of fix and optimization patches in spmi-pmic-arb.c | expand |
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 <quic_fenglinw@quicinc.com> > --- > .../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 <quic_fenglinw@quicinc.com> > --- > .../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 <quic_fenglinw@quicinc.com> > + - Subbaraman Narayanamurthy <quic_subbaram@quicinc.com> > + > +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 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 <quic_fenglinw@quicinc.com> >> --- >> .../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/20211119034613.32489-2-james.lo@mediatek.com/ 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 <quic_fenglinw@quicinc.com> >> --- >> .../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 <quic_fenglinw@quicinc.com> >> + - Subbaraman Narayanamurthy <quic_subbaram@quicinc.com> >> + >> +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 <collinsd@codeaurora.org> >> >> 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 <collinsd@codeaurora.org> >> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com> >> --- >> 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!
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