diff mbox series

[05/10] dt-bindings: interconnect: Add sm8350, sc8280xp and generic OSM L3 compatibles

Message ID 20221028034155.5580-6-quic_bjorande@quicinc.com
State New
Headers
Series interconnect: osm-l3: SC8280XP L3 and DDR scaling |

Commit Message

Bjorn Andersson Oct. 28, 2022, 3:41 a.m. UTC 
  Add EPSS L3 compatibles for sm8350 and sc8280xp, but while at it also
introduce generic compatible for both qcom,osm-l3 and qcom,epss-l3.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 .../bindings/interconnect/qcom,osm-l3.yaml    | 22 +++++++++++++------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Rob Herring Oct. 28, 2022, 12:20 p.m. UTC | #1 
On Thu, 27 Oct 2022 20:41:50 -0700, Bjorn Andersson wrote:
> Add EPSS L3 compatibles for sm8350 and sc8280xp, but while at it also
> introduce generic compatible for both qcom,osm-l3 and qcom,epss-l3.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  .../bindings/interconnect/qcom,osm-l3.yaml    | 22 +++++++++++++------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 

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:
./Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml:27:7: [error] duplication of key "items" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/interconnect/qcom,osm-l3.example.dts'
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml:27:7: found duplicate key "items" with value "[]" (original value: "[]")
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/interconnect/qcom,osm-l3.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml:27:7: found duplicate key "items" with value "[]" (original value: "[]")
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml: ignoring, error parsing file
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

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.
Krzysztof Kozlowski Oct. 28, 2022, 10:12 p.m. UTC | #2 
On 27/10/2022 23:41, Bjorn Andersson wrote:
> Add EPSS L3 compatibles for sm8350 and sc8280xp, but while at it also
> introduce generic compatible for both qcom,osm-l3 and qcom,epss-l3.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  .../bindings/interconnect/qcom,osm-l3.yaml    | 22 +++++++++++++------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> index bf538c0c5a81..ae0995341a78 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> @@ -16,13 +16,21 @@ description:
>  
>  properties:
>    compatible:
> -    enum:
> -      - qcom,sc7180-osm-l3
> -      - qcom,sc7280-epss-l3
> -      - qcom,sc8180x-osm-l3
> -      - qcom,sdm845-osm-l3
> -      - qcom,sm8150-osm-l3
> -      - qcom,sm8250-epss-l3
> +    oneOf:
> +      items:

oneOf expects a list, so this should be "    - items"

> +        - enum:
> +            - qcom,sc7180-osm-l3
> +            - qcom,sc8180x-osm-l3
> +            - qcom,sdm845-osm-l3
> +            - qcom,sm8150-osm-l3
> +        - const: qcom,osm-l3

The concept is good, but are you sure all SoCs will be compatible with
generic osm-l3? Why not using dedicated compatible of one soc, e.g. the
oldest here? We already did like that for BWMON, DMA and few others.

> +      items:
> +        - enum:
> +            - qcom,sc7280-epss-l3
> +            - qcom,sc8280xp-epss-l3
> +            - qcom,sm8250-epss-l3
> +            - qcom,sm8350-epss-l3
> +        - const: qcom,epss-l3
>  
>    reg:
>      maxItems: 1

Best regards,
Krzysztof
Bjorn Andersson Nov. 3, 2022, 3:44 a.m. UTC | #3 
On Fri, Oct 28, 2022 at 06:12:29PM -0400, Krzysztof Kozlowski wrote:
> On 27/10/2022 23:41, Bjorn Andersson wrote:
> > Add EPSS L3 compatibles for sm8350 and sc8280xp, but while at it also
> > introduce generic compatible for both qcom,osm-l3 and qcom,epss-l3.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  .../bindings/interconnect/qcom,osm-l3.yaml    | 22 +++++++++++++------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> > index bf538c0c5a81..ae0995341a78 100644
> > --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> > +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> > @@ -16,13 +16,21 @@ description:
> >  
> >  properties:
> >    compatible:
> > -    enum:
> > -      - qcom,sc7180-osm-l3
> > -      - qcom,sc7280-epss-l3
> > -      - qcom,sc8180x-osm-l3
> > -      - qcom,sdm845-osm-l3
> > -      - qcom,sm8150-osm-l3
> > -      - qcom,sm8250-epss-l3
> > +    oneOf:
> > +      items:
> 
> oneOf expects a list, so this should be "    - items"
> 

Ahh, thanks. Must have missed running the dt_binding_check on this one.

> > +        - enum:
> > +            - qcom,sc7180-osm-l3
> > +            - qcom,sc8180x-osm-l3
> > +            - qcom,sdm845-osm-l3
> > +            - qcom,sm8150-osm-l3
> > +        - const: qcom,osm-l3
> 
> The concept is good, but are you sure all SoCs will be compatible with
> generic osm-l3?

Per the current implementation yes, worst case if one or more of them isn't the
more specific compatible can be used to alter the behavior of that platform.

> Why not using dedicated compatible of one soc, e.g. the
> oldest here? We already did like that for BWMON, DMA and few others.
> 

Because if we say compatible = "qcom,sc8180x-osm-l3", "qcom,sdm845-osm-l3" and
there is a quirk needed for "qcom,sdm845-osm-l3" we're forced to add a "special
case" every other *-osm-l3 in the driver.

This way we can have a generic implementation for the qcom,osm-l3 and if we
realize that we need to quirk something for the oldest platform, we can do so
without affecting the others.

Regards,
Bjorn

> > +      items:
> > +        - enum:
> > +            - qcom,sc7280-epss-l3
> > +            - qcom,sc8280xp-epss-l3
> > +            - qcom,sm8250-epss-l3
> > +            - qcom,sm8350-epss-l3
> > +        - const: qcom,epss-l3
> >  
> >    reg:
> >      maxItems: 1
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Nov. 3, 2022, 12:25 p.m. UTC | #4 
On 02/11/2022 23:44, Bjorn Andersson wrote:
> On Fri, Oct 28, 2022 at 06:12:29PM -0400, Krzysztof Kozlowski wrote:
>> On 27/10/2022 23:41, Bjorn Andersson wrote:
>>> Add EPSS L3 compatibles for sm8350 and sc8280xp, but while at it also
>>> introduce generic compatible for both qcom,osm-l3 and qcom,epss-l3.
>>>
>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> ---
>>>  .../bindings/interconnect/qcom,osm-l3.yaml    | 22 +++++++++++++------
>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>> index bf538c0c5a81..ae0995341a78 100644
>>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>> @@ -16,13 +16,21 @@ description:
>>>  
>>>  properties:
>>>    compatible:
>>> -    enum:
>>> -      - qcom,sc7180-osm-l3
>>> -      - qcom,sc7280-epss-l3
>>> -      - qcom,sc8180x-osm-l3
>>> -      - qcom,sdm845-osm-l3
>>> -      - qcom,sm8150-osm-l3
>>> -      - qcom,sm8250-epss-l3
>>> +    oneOf:
>>> +      items:
>>
>> oneOf expects a list, so this should be "    - items"
>>
> 
> Ahh, thanks. Must have missed running the dt_binding_check on this one.
> 
>>> +        - enum:
>>> +            - qcom,sc7180-osm-l3
>>> +            - qcom,sc8180x-osm-l3
>>> +            - qcom,sdm845-osm-l3
>>> +            - qcom,sm8150-osm-l3
>>> +        - const: qcom,osm-l3
>>
>> The concept is good, but are you sure all SoCs will be compatible with
>> generic osm-l3?
> 
> Per the current implementation yes, worst case if one or more of them isn't the
> more specific compatible can be used to alter the behavior of that platform.
> 
>> Why not using dedicated compatible of one soc, e.g. the
>> oldest here? We already did like that for BWMON, DMA and few others.
>>
> 
> Because if we say compatible = "qcom,sc8180x-osm-l3", "qcom,sdm845-osm-l3" and
> there is a quirk needed for "qcom,sdm845-osm-l3" we're forced to add a "special
> case" every other *-osm-l3 in the driver.
> 
> This way we can have a generic implementation for the qcom,osm-l3 and if we
> realize that we need to quirk something for the oldest platform, we can do so
> without affecting the others.

True. This also means we do not really know which one is the generic
implementation :)


Best regards,
Krzysztof
Bjorn Andersson Nov. 3, 2022, 3:46 p.m. UTC | #5 
On Thu, Nov 03, 2022 at 08:25:17AM -0400, Krzysztof Kozlowski wrote:
> On 02/11/2022 23:44, Bjorn Andersson wrote:
> > On Fri, Oct 28, 2022 at 06:12:29PM -0400, Krzysztof Kozlowski wrote:
> >> On 27/10/2022 23:41, Bjorn Andersson wrote:
> >>> Add EPSS L3 compatibles for sm8350 and sc8280xp, but while at it also
> >>> introduce generic compatible for both qcom,osm-l3 and qcom,epss-l3.
> >>>
> >>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> >>> ---
> >>>  .../bindings/interconnect/qcom,osm-l3.yaml    | 22 +++++++++++++------
> >>>  1 file changed, 15 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> >>> index bf538c0c5a81..ae0995341a78 100644
> >>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> >>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> >>> @@ -16,13 +16,21 @@ description:
> >>>  
> >>>  properties:
> >>>    compatible:
> >>> -    enum:
> >>> -      - qcom,sc7180-osm-l3
> >>> -      - qcom,sc7280-epss-l3
> >>> -      - qcom,sc8180x-osm-l3
> >>> -      - qcom,sdm845-osm-l3
> >>> -      - qcom,sm8150-osm-l3
> >>> -      - qcom,sm8250-epss-l3
> >>> +    oneOf:
> >>> +      items:
> >>
> >> oneOf expects a list, so this should be "    - items"
> >>
> > 
> > Ahh, thanks. Must have missed running the dt_binding_check on this one.
> > 
> >>> +        - enum:
> >>> +            - qcom,sc7180-osm-l3
> >>> +            - qcom,sc8180x-osm-l3
> >>> +            - qcom,sdm845-osm-l3
> >>> +            - qcom,sm8150-osm-l3
> >>> +        - const: qcom,osm-l3
> >>
> >> The concept is good, but are you sure all SoCs will be compatible with
> >> generic osm-l3?
> > 
> > Per the current implementation yes, worst case if one or more of them isn't the
> > more specific compatible can be used to alter the behavior of that platform.
> > 
> >> Why not using dedicated compatible of one soc, e.g. the
> >> oldest here? We already did like that for BWMON, DMA and few others.
> >>
> > 
> > Because if we say compatible = "qcom,sc8180x-osm-l3", "qcom,sdm845-osm-l3" and
> > there is a quirk needed for "qcom,sdm845-osm-l3" we're forced to add a "special
> > case" every other *-osm-l3 in the driver.
> > 
> > This way we can have a generic implementation for the qcom,osm-l3 and if we
> > realize that we need to quirk something for the oldest platform, we can do so
> > without affecting the others.
> 
> True. This also means we do not really know which one is the generic
> implementation :)
> 

There currently is an implementation without platform specific quirks, I
call that the generic implementation and suggest that we refer to that
using "qcom,osm-l3".

If we instead were to use sdm845 as the generic compatible, and there
turns out to be a need for a quirk for this platform, you:

1) no longer have a generic implementation, but 4 platform-specific
   implementations

2) have 3 platforms claiming to be compatible with the quirked (now
   specialized) implementation, which they clearly aren't anymore

Therefor I favor using generic names for generic compatibles.

Regards,
Bjorn
Krzysztof Kozlowski Nov. 3, 2022, 3:56 p.m. UTC | #6 
On 03/11/2022 11:46, Bjorn Andersson wrote:
> On Thu, Nov 03, 2022 at 08:25:17AM -0400, Krzysztof Kozlowski wrote:
>> On 02/11/2022 23:44, Bjorn Andersson wrote:
>>> On Fri, Oct 28, 2022 at 06:12:29PM -0400, Krzysztof Kozlowski wrote:
>>>> On 27/10/2022 23:41, Bjorn Andersson wrote:
>>>>> Add EPSS L3 compatibles for sm8350 and sc8280xp, but while at it also
>>>>> introduce generic compatible for both qcom,osm-l3 and qcom,epss-l3.
>>>>>
>>>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>>>> ---
>>>>>  .../bindings/interconnect/qcom,osm-l3.yaml    | 22 +++++++++++++------
>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>>>> index bf538c0c5a81..ae0995341a78 100644
>>>>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>>>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>>>> @@ -16,13 +16,21 @@ description:
>>>>>  
>>>>>  properties:
>>>>>    compatible:
>>>>> -    enum:
>>>>> -      - qcom,sc7180-osm-l3
>>>>> -      - qcom,sc7280-epss-l3
>>>>> -      - qcom,sc8180x-osm-l3
>>>>> -      - qcom,sdm845-osm-l3
>>>>> -      - qcom,sm8150-osm-l3
>>>>> -      - qcom,sm8250-epss-l3
>>>>> +    oneOf:
>>>>> +      items:
>>>>
>>>> oneOf expects a list, so this should be "    - items"
>>>>
>>>
>>> Ahh, thanks. Must have missed running the dt_binding_check on this one.
>>>
>>>>> +        - enum:
>>>>> +            - qcom,sc7180-osm-l3
>>>>> +            - qcom,sc8180x-osm-l3
>>>>> +            - qcom,sdm845-osm-l3
>>>>> +            - qcom,sm8150-osm-l3
>>>>> +        - const: qcom,osm-l3
>>>>
>>>> The concept is good, but are you sure all SoCs will be compatible with
>>>> generic osm-l3?
>>>
>>> Per the current implementation yes, worst case if one or more of them isn't the
>>> more specific compatible can be used to alter the behavior of that platform.
>>>
>>>> Why not using dedicated compatible of one soc, e.g. the
>>>> oldest here? We already did like that for BWMON, DMA and few others.
>>>>
>>>
>>> Because if we say compatible = "qcom,sc8180x-osm-l3", "qcom,sdm845-osm-l3" and
>>> there is a quirk needed for "qcom,sdm845-osm-l3" we're forced to add a "special
>>> case" every other *-osm-l3 in the driver.
>>>
>>> This way we can have a generic implementation for the qcom,osm-l3 and if we
>>> realize that we need to quirk something for the oldest platform, we can do so
>>> without affecting the others.
>>
>> True. This also means we do not really know which one is the generic
>> implementation :)
>>
> 
> There currently is an implementation without platform specific quirks, I
> call that the generic implementation and suggest that we refer to that
> using "qcom,osm-l3".>
> If we instead were to use sdm845 as the generic compatible, and there
> turns out to be a need for a quirk for this platform, you:
> 
> 1) no longer have a generic implementation, but 4 platform-specific
>    implementations

It's okay because there is no such thing anymore as "generic
implementation". If this happened, your generic compatible is not
specific enough. It does not represent any specific hardware.

Adding generic compatibles just to make driver binding easier, is a bit
in contrast with DT which should focus on hardware description.

> 
> 2) have 3 platforms claiming to be compatible with the quirked (now
>    specialized) implementation, which they clearly aren't anymore

Yes, that's the problem and this is why I mentioned that we do not know
the generic implementation. If we knew that sdm845 is the generic, we
would not expect specific quirks for it.

If you cannot make sdm845 generic because of unknown quirk, then you
just do not know which one is generic implementation and that compatible
is not specific enough... Or it is specific only to current Linux driver.

> Therefor I favor using generic names for generic compatibles.

They make driver development easier but they hide the real match between
hardware and compatible.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
index bf538c0c5a81..ae0995341a78 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
@@ -16,13 +16,21 @@  description:
 
 properties:
   compatible:
-    enum:
-      - qcom,sc7180-osm-l3
-      - qcom,sc7280-epss-l3
-      - qcom,sc8180x-osm-l3
-      - qcom,sdm845-osm-l3
-      - qcom,sm8150-osm-l3
-      - qcom,sm8250-epss-l3
+    oneOf:
+      items:
+        - enum:
+            - qcom,sc7180-osm-l3
+            - qcom,sc8180x-osm-l3
+            - qcom,sdm845-osm-l3
+            - qcom,sm8150-osm-l3
+        - const: qcom,osm-l3
+      items:
+        - enum:
+            - qcom,sc7280-epss-l3
+            - qcom,sc8280xp-epss-l3
+            - qcom,sm8250-epss-l3
+            - qcom,sm8350-epss-l3
+        - const: qcom,epss-l3
 
   reg:
     maxItems: 1