Re: [PATCH v2 1/9] dt-bindings: arm-smmu: Allow up to 3 power-domains

[Robin Murphy]

On 2022-11-15 13:06, Konrad Dybcio wrote:
> On 15/11/2022 14:00, Krzysztof Kozlowski wrote:
> > On 15/11/2022 13:54, Konrad Dybcio wrote:
> > > On 14/11/2022 17:58, Krzysztof Kozlowski wrote:
> > > > On 14/11/2022 16:53, Konrad Dybcio wrote:
> > > > > On 14/11/2022 14:00, Krzysztof Kozlowski wrote:
> > > > > > On 14/11/2022 12:17, Konrad Dybcio wrote:
> > > > > > > On 14/11/2022 12:01, Krzysztof Kozlowski wrote:
> > > > > > > > On 14/11/2022 11:42, Konrad Dybcio wrote:
> > > > > > > > > Some SMMUs require that a vote is held on as much as 3 separate 
> > > > > > > > > PDs
> > > > > > > > > (hello Qualcomm). Allow it in bindings.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> > > > > > > > > ---
> > > > > > > > > Changes since v1:
> > > > > > > > > - Add minItems
> > > > > > > > >
> > > > > > > > >      Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 3 ++-
> > > > > > > > >      1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git 
> > > > > > > > > a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml 
> > > > > > > > > b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > > > > > > > index 9066e6df1ba1..82bc696de662 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > > > > > > > @@ -159,7 +159,8 @@ properties:
> > > > > > > > >                through the TCU's programming interface.
> > > > > > > > >        power-domains:
> > > > > > > > > -    maxItems: 1
> > > > > > > > > +    minItems: 0
> > > > > > > > It cannot be 0.
> > > > > > > >
> > > > > > > > minItems: 1
> > > > > > > >
> > > > > > > > Anyway you still need to restrict it per variant, as I said in 
> > > > > > > > previous
> > > > > > > > version.
> > > > > > > Hm.. I'm not entirely sure what you mean.. Should I add a list of
> > > > > > > compatibles
> > > > > > Yes and limit it to maxItems: 1 for "else".
> > > > >
> > > > > I tried adding:
> > > > >
> > > > >
> > > > >
> > > > >      - if:
> > > > >          properties:
> > > > >            compatible:
> > > > >              contains:
> > > > >                enum:
> > > > >                  - qcom,sm6375-smmu-500
> > > > >        then:
> > > > >          properties:
> > > > >            power-domains:
> > > > >              minItems: 3
> > > > >              maxItems: 3
> > > > >        else:
> > > > >          properties:
> > > > >            power-domains:
> > > > >              maxItems: 1
> > > > >
> > > > >
> > > > > Right under the nvidia reg if-else in the allOf, but dtbs_check throws
> > > > > errors like:
> > > > >
> > > > >
> > > > > /home/konrad/linux/arch/arm64/boot/dts/qcom/msm8998-sony-xperia-yoshino-poplar.dtb:
> > > > > iommu@5040000: 'power-domains' does not match any of the regexes:
> > > > > 'pinctrl-[0-9]+'
> > > > >
> > > > >
> > > > > Any clues as to why?
> > > >
> > > > I don't know what code do you have there, but generic pattern is:
> > > >
> > > > https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L38
> > > I tried many things, but I still don't seem to get a hang of it.. Here's
> > > my current diff rebased on top of Dmitry's recent cleanups (available at
> > > [1])
> > >
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > index 28f5720824cd..55759aebc4a0 100644
> > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > @@ -200,7 +200,7 @@ properties:
> > >        maxItems: 7
> > >
> > >      power-domains:
> >
> > As I mentioned before - minItems: 1.
> But not all SMMUs require a power domain :/

Right, so it's not a required property. However if it *is* present, then 
it needs to reference at least one power domain, because having an empty 
property, i.e.:

	power-domains = <>;

or
	power-domains;

makes no sense whatsoever.

Thanks,
Robin.

> > Just like the link I gave you.
> >
> > > -    maxItems: 1
> > > +    maxItems: 3
> > >
> > >      nvidia,memory-controller:
> > >        description: |
> > > @@ -364,6 +364,26 @@ allOf:
> > >                - description: interface clock required to access smmu's
> > > registers
> > >                    through the TCU's programming interface.
> > >
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: qcom,sm6375-smmu-500
> > > +    then:
> > > +      properties:
> > > +        power-domains:
> > > +          items:
> > > +            - description: SNoC MMU TBU RT GDSC
> > > +            - description: SNoC MMU TBU NRT GDSC
> > > +            - description: SNoC TURING MMU TBU0 GDSC
> > > +
> > > +      required:
> > > +        - power-domains
> > > +    else:
> > > +      properties:
> > > +        power-domains:
> > > +          maxItems: 1
> > > +
> > >    examples:
> > >      - |+
> > >        /* SMMU with stream matching or stream indexing */
> > >
> > >
> > > In my eyes, this should work, but I still get errors like:
> > >
> > > /home/konrad/linux/arch/arm64/boot/dts/qcom/sm8250-hdk.dtb:
> > > iommu@3da0000: power-domains: [[108, 0]] is too short
> > >
> > > as if the else: path was never taken..
> >
> > It was, but the top-level property said that minItems=3 (implicitly), so
> > it is too short.
> So the top-level properties take precedence over the ones that come from 
> the if-then-else?? Ugh.
>
> Konrad
> > Best regards,
> > Krzysztof