| Message ID | 20240407-spmi-multi-master-support-v9-1-fa151c1391f3@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spmi: pmic-arb: Add support for multiple buses | expand |
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
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
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
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 --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>; + }; + }; + };