| Message ID | 20221028034155.5580-6-quic_bjorande@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | interconnect: osm-l3: SC8280XP L3 and DDR scaling | expand |
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 >
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
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
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 --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
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(-)