diff mbox series

[v9,1/7] dt-bindings: spmi: Add X1E80100 SPMI PMIC ARB schema

Message ID 20240407-spmi-multi-master-support-v9-1-fa151c1391f3@linaro.org (mailing list archive)
State Superseded
Headers show
Series spmi: pmic-arb: Add support for multiple buses | expand

Commit Message

Abel Vesa April 7, 2024, 4:23 p.m. UTC
Add dedicated schema for X1E80100 PMIC ARB (v7) as it allows multiple
buses by declaring them as child nodes.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 .../bindings/spmi/qcom,x1e80100-spmi-pmic-arb.yaml | 136 +++++++++++++++++++++
 1 file changed, 136 insertions(+)

Comments

Bjorn Andersson April 8, 2024, 12:07 a.m. UTC | #1
On Sun, Apr 07, 2024 at 07:23:21PM +0300, Abel Vesa wrote:
> Add dedicated schema for X1E80100 PMIC ARB (v7) as it allows multiple
> buses by declaring them as child nodes.
> 

But is this really a "dedicated schema for X1E80100"? Isn't it "the
schema for all multi-bus controllers"?

I.e. isn't this a "dedicated schema for all platforms starting with
SM8450"?

Can you please use the commit message to document the actual reason why
you choose to create a dedicated schema for this? Is it simply to avoid
having to schema with either pmics or multiple buses as children?

Regards,
Bjorn
Abel Vesa April 8, 2024, 6:04 a.m. UTC | #2
On 24-04-07 19:07:03, Bjorn Andersson wrote:
> On Sun, Apr 07, 2024 at 07:23:21PM +0300, Abel Vesa wrote:
> > Add dedicated schema for X1E80100 PMIC ARB (v7) as it allows multiple
> > buses by declaring them as child nodes.
> > 
> 
> But is this really a "dedicated schema for X1E80100"? Isn't it "the
> schema for all multi-bus controllers"?
> 
> I.e. isn't this a "dedicated schema for all platforms starting with
> SM8450"?

Suggestion was from Krzysztof to add platform specific comaptible (and
therefore schema). Since the first platform that will support in
upstream proper multi bus is the x1e80100, the schema needs to bear the
same name as the compatible. When support for multi bus will be added to
the other platforms (including the SM8450), they will use the fallback
compatible of the x1e80100 and will be documented in this newly added
schema. We did the same thing with some PHYs drivers, IIRC.

> 
> Can you please use the commit message to document the actual reason why
> you choose to create a dedicated schema for this? Is it simply to avoid
> having to schema with either pmics or multiple buses as children?

I can re-send the patchset with such a phrase in commit message.

One of the early versions of this patchset was actually submitting a
generic compatible for multi bus, but I remember that there was a
request for following the platform dedicated approach.

Krzysztof, can you please provide here the argument for why that is
preferred?

> 
> Regards,
> Bjorn
Krzysztof Kozlowski April 8, 2024, 6:30 a.m. UTC | #3
On 08/04/2024 08:04, Abel Vesa wrote:
> On 24-04-07 19:07:03, Bjorn Andersson wrote:
>> On Sun, Apr 07, 2024 at 07:23:21PM +0300, Abel Vesa wrote:
>>> Add dedicated schema for X1E80100 PMIC ARB (v7) as it allows multiple
>>> buses by declaring them as child nodes.
>>>
>>
>> But is this really a "dedicated schema for X1E80100"? Isn't it "the
>> schema for all multi-bus controllers"?
>>
>> I.e. isn't this a "dedicated schema for all platforms starting with
>> SM8450"?
> 
> Suggestion was from Krzysztof to add platform specific comaptible (and
> therefore schema). Since the first platform that will support in
> upstream proper multi bus is the x1e80100, the schema needs to bear the
> same name as the compatible. When support for multi bus will be added to
> the other platforms (including the SM8450), they will use the fallback
> compatible of the x1e80100 and will be documented in this newly added
> schema. We did the same thing with some PHYs drivers, IIRC.
> 
>>
>> Can you please use the commit message to document the actual reason why
>> you choose to create a dedicated schema for this? Is it simply to avoid
>> having to schema with either pmics or multiple buses as children?
> 
> I can re-send the patchset with such a phrase in commit message.
> 
> One of the early versions of this patchset was actually submitting a
> generic compatible for multi bus, but I remember that there was a
> request for following the platform dedicated approach.
> 
> Krzysztof, can you please provide here the argument for why that is
> preferred?

I could not find such suggestions from my side in the archives, except:
https://lore.kernel.org/all/dd86117e-0196-499b-b8b3-efe4013cbc07@linaro.org/

where I want SoC specific compatibles to be used, not versions.

Now about this binding, it is not a schema for all platforms starting
with sm8450, but only for x1e. I do not understand why this would be a
problem?

If you ask why this is not a schema for all platforms, then because:
1. maybe no one tested other SoCs?
2. maybe no one cares?
3. maybe other boards need some quirks, so this would be applicable but
not fully?

I don't know... since when do we add "generic schemas"?

However maybe the question is different: why other devices are not
described here, while they should? Then probably Abel can answer what he
wants and what he does not want to describe. There is no requirement to
model all possible hardware in a binding, but instead describe one
hardware, so x1e, fully.

Best regards,
Krzysztof
Abel Vesa April 12, 2024, 11:04 a.m. UTC | #4
On 24-04-08 08:30:56, Krzysztof Kozlowski wrote:
> On 08/04/2024 08:04, Abel Vesa wrote:
> > On 24-04-07 19:07:03, Bjorn Andersson wrote:
> >> On Sun, Apr 07, 2024 at 07:23:21PM +0300, Abel Vesa wrote:
> >>> Add dedicated schema for X1E80100 PMIC ARB (v7) as it allows multiple
> >>> buses by declaring them as child nodes.
> >>>
> >>
> >> But is this really a "dedicated schema for X1E80100"? Isn't it "the
> >> schema for all multi-bus controllers"?
> >>
> >> I.e. isn't this a "dedicated schema for all platforms starting with
> >> SM8450"?
> > 
> > Suggestion was from Krzysztof to add platform specific comaptible (and
> > therefore schema). Since the first platform that will support in
> > upstream proper multi bus is the x1e80100, the schema needs to bear the
> > same name as the compatible. When support for multi bus will be added to
> > the other platforms (including the SM8450), they will use the fallback
> > compatible of the x1e80100 and will be documented in this newly added
> > schema. We did the same thing with some PHYs drivers, IIRC.
> > 
> >>
> >> Can you please use the commit message to document the actual reason why
> >> you choose to create a dedicated schema for this? Is it simply to avoid
> >> having to schema with either pmics or multiple buses as children?
> > 
> > I can re-send the patchset with such a phrase in commit message.
> > 
> > One of the early versions of this patchset was actually submitting a
> > generic compatible for multi bus, but I remember that there was a
> > request for following the platform dedicated approach.
> > 
> > Krzysztof, can you please provide here the argument for why that is
> > preferred?
> 
> I could not find such suggestions from my side in the archives, except:
> https://lore.kernel.org/all/dd86117e-0196-499b-b8b3-efe4013cbc07@linaro.org/
> 
> where I want SoC specific compatibles to be used, not versions.
> 
> Now about this binding, it is not a schema for all platforms starting
> with sm8450, but only for x1e. I do not understand why this would be a
> problem?
> 

I agree, I don't think there is a problem with that. At some point,
all platforms starting with sm8450 will be added and then we can even
make the sm8450 compatible as the fallback comaptible.

> If you ask why this is not a schema for all platforms, then because:
> 1. maybe no one tested other SoCs?
> 2. maybe no one cares?
> 3. maybe other boards need some quirks, so this would be applicable but
> not fully?
> 
> I don't know... since when do we add "generic schemas"?
> 

The focus of this patchset is support on X Elite which implicitly needs
multi bus support. Again, we can do the other ones later on. I don't
think we should extend the focus of this patchset more that it already
is.

> However maybe the question is different: why other devices are not
> described here, while they should? Then probably Abel can answer what he
> wants and what he does not want to describe. There is no requirement to
> model all possible hardware in a binding, but instead describe one
> hardware, so x1e, fully.
> 

I'll switch the older platforms as well in a separate patchset, I promise.
But let's not delay this any longer.

> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spmi/qcom,x1e80100-spmi-pmic-arb.yaml b/Documentation/devicetree/bindings/spmi/qcom,x1e80100-spmi-pmic-arb.yaml
new file mode 100644
index 000000000000..a28b70fb330a
--- /dev/null
+++ b/Documentation/devicetree/bindings/spmi/qcom,x1e80100-spmi-pmic-arb.yaml
@@ -0,0 +1,136 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spmi/qcom,x1e80100-spmi-pmic-arb.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm X1E80100 SPMI Controller (PMIC Arbiter v7)
+
+maintainers:
+  - Stephen Boyd <sboyd@kernel.org>
+
+description: |
+  The X1E80100 SPMI PMIC Arbiter implements HW version 7 and it's an SPMI
+  controller with wrapping arbitration logic to allow for multiple on-chip
+  devices to control up to 2 SPMI separate buses.
+
+  The PMIC Arbiter can also act as an interrupt controller, providing interrupts
+  to slave devices.
+
+properties:
+  compatible:
+    const: qcom,x1e80100-spmi-pmic-arb
+
+  reg:
+    items:
+      - description: core registers
+      - description: tx-channel per virtual slave registers
+      - description: rx-channel (called observer) per virtual slave registers
+
+  reg-names:
+    items:
+      - const: core
+      - const: chnls
+      - const: obsrvr
+
+  ranges: true
+
+  '#address-cells':
+    const: 2
+
+  '#size-cells':
+    const: 2
+
+  qcom,ee:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 5
+    description: >
+      indicates the active Execution Environment identifier
+
+  qcom,channel:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 5
+    description: >
+      which of the PMIC Arb provided channels to use for accesses
+
+patternProperties:
+  "^spmi@[a-f0-9]+$":
+    type: object
+    $ref: /schemas/spmi/spmi.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        items:
+          - description: configuration registers
+          - description: interrupt controller registers
+
+      reg-names:
+        items:
+          - const: cnfg
+          - const: intr
+
+      interrupts:
+        maxItems: 1
+
+      interrupt-names:
+        const: periph_irq
+
+      interrupt-controller: true
+
+      '#interrupt-cells':
+        const: 4
+        description: |
+          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
+
+required:
+  - compatible
+  - reg-names
+  - qcom,ee
+  - qcom,channel
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      spmi: arbiter@c400000 {
+        compatible = "qcom,x1e80100-spmi-pmic-arb";
+        reg = <0 0x0c400000 0 0x3000>,
+              <0 0x0c500000 0 0x4000000>,
+              <0 0x0c440000 0 0x80000>;
+        reg-names = "core", "chnls", "obsrvr";
+
+        qcom,ee = <0>;
+        qcom,channel = <0>;
+
+        #address-cells = <2>;
+        #size-cells = <2>;
+        ranges;
+
+        spmi_bus0: spmi@c42d000 {
+          reg = <0 0x0c42d000 0 0x4000>,
+                <0 0x0c4c0000 0 0x10000>;
+          reg-names = "cnfg", "intr";
+
+          interrupt-names = "periph_irq";
+          interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>;
+          interrupt-controller;
+          #interrupt-cells = <4>;
+
+          #address-cells = <2>;
+          #size-cells = <0>;
+        };
+      };
+    };