| Message ID | 20241125174511.45-4-quic_rlaggysh@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add EPSS L3 provider support on SA8775P SoC | expand |
On 25/11/2024 18:45, Raviteja Laggyshetty wrote: > EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of This should explain that these devices are actually 100% compatible. > REG_L3_VOTE to scale L3 clocks, hence adding a new generic > compatible "qcom,epss-l3-perf" for these targets. Not the best reason... You add one more generic compatible, because devices are different? With such reason answer is: no, do not add generic compatibles, because they are not generic. The entire point of having generic compatibles is that they really are generic. Here you prove that they are not generic, so why creating one more generic compatible which might not be generic at all? I already expressed above concerns multiple times: 1. In previous versions of this patchset 2. In other threads I am not against this change, but I am not going to Ack it. Get acks from other maintainers, I am not happy with multiple generic-but-actually-not-generic compatibles. I think that is poor approach. Best regards, Krzysztof
On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: > EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of > REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible > "qcom,epss-l3-perf" for these targets. Is this a h/w difference from prior blocks or you just want to use B instead of A while the h/w has both A and B? The latter sounds like driver policy. It is also an ABI break for s/w that didn't understand qcom,epss-l3-perf. > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > --- > .../devicetree/bindings/interconnect/qcom,osm-l3.yaml | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > index 21dae0b92819..e24399ca110f 100644 > --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > @@ -28,12 +28,15 @@ properties: > - const: qcom,osm-l3 > - items: > - enum: > - - qcom,sc7280-epss-l3 > - qcom,sc8280xp-epss-l3 > - qcom,sm6375-cpucp-l3 > - - qcom,sm8250-epss-l3 > - qcom,sm8350-epss-l3 > - const: qcom,epss-l3 > + - items: > + - enum: > + - qcom,sc7280-epss-l3 > + - qcom,sm8250-epss-l3 > + - const: qcom,epss-l3-perf > > reg: > maxItems: 1 > -- > 2.39.2 >
On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: > On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: > > EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of > > REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible > > "qcom,epss-l3-perf" for these targets. > > Is this a h/w difference from prior blocks or you just want to use B > instead of A while the h/w has both A and B? The latter sounds like > driver policy. > > It is also an ABI break for s/w that didn't understand > qcom,epss-l3-perf. As the bindings keep old compatible strings in addition to the new qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, newer can use either of those. > > > > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > > --- > > .../devicetree/bindings/interconnect/qcom,osm-l3.yaml | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > > index 21dae0b92819..e24399ca110f 100644 > > --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > > +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > > @@ -28,12 +28,15 @@ properties: > > - const: qcom,osm-l3 > > - items: > > - enum: > > - - qcom,sc7280-epss-l3 > > - qcom,sc8280xp-epss-l3 > > - qcom,sm6375-cpucp-l3 > > - - qcom,sm8250-epss-l3 > > - qcom,sm8350-epss-l3 > > - const: qcom,epss-l3 > > + - items: > > + - enum: > > + - qcom,sc7280-epss-l3 > > + - qcom,sm8250-epss-l3 > > + - const: qcom,epss-l3-perf > > > > reg: > > maxItems: 1 > > -- > > 2.39.2 > >
On 27/11/2024 17:53, Dmitry Baryshkov wrote: > On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>> "qcom,epss-l3-perf" for these targets. >> >> Is this a h/w difference from prior blocks or you just want to use B >> instead of A while the h/w has both A and B? The latter sounds like >> driver policy. >> >> It is also an ABI break for s/w that didn't understand >> qcom,epss-l3-perf. > > As the bindings keep old compatible strings in addition to the new > qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, > newer can use either of those. No, this change drops qcom,epss-l3 and adds new fallback. How old software can work in such case? It's broken. Best regards, Krzysztof
On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >On 27/11/2024 17:53, Dmitry Baryshkov wrote: >> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>>> "qcom,epss-l3-perf" for these targets. >>> >>> Is this a h/w difference from prior blocks or you just want to use B >>> instead of A while the h/w has both A and B? The latter sounds like >>> driver policy. >>> >>> It is also an ABI break for s/w that didn't understand >>> qcom,epss-l3-perf. >> >> As the bindings keep old compatible strings in addition to the new >> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, >> newer can use either of those. >No, this change drops qcom,epss-l3 and adds new fallback. How old >software can work in such case? It's broken. Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". > >Best regards, >Krzysztof
On 27/11/2024 19:49, Dmitry Baryshkov wrote: > On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 27/11/2024 17:53, Dmitry Baryshkov wrote: >>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>>>> "qcom,epss-l3-perf" for these targets. >>>> >>>> Is this a h/w difference from prior blocks or you just want to use B >>>> instead of A while the h/w has both A and B? The latter sounds like >>>> driver policy. >>>> >>>> It is also an ABI break for s/w that didn't understand >>>> qcom,epss-l3-perf. >>> >>> As the bindings keep old compatible strings in addition to the new >>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, >>> newer can use either of those. >> No, this change drops qcom,epss-l3 and adds new fallback. How old >> software can work in such case? It's broken. > > Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". It depends for example whether epss-l3 is valid at all. ABI is not broken if nothing was working in the first place, assuming it is explained in commit msg (not the case here). Best regards, Krzysztof
On 27 November 2024 21:22:02 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >On 27/11/2024 19:49, Dmitry Baryshkov wrote: >> On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> On 27/11/2024 17:53, Dmitry Baryshkov wrote: >>>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >>>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>>>>> "qcom,epss-l3-perf" for these targets. >>>>> >>>>> Is this a h/w difference from prior blocks or you just want to use B >>>>> instead of A while the h/w has both A and B? The latter sounds like >>>>> driver policy. >>>>> >>>>> It is also an ABI break for s/w that didn't understand >>>>> qcom,epss-l3-perf. >>>> >>>> As the bindings keep old compatible strings in addition to the new >>>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, >>>> newer can use either of those. >>> No, this change drops qcom,epss-l3 and adds new fallback. How old >>> software can work in such case? It's broken. >> >> Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". > >It depends for example whether epss-l3 is valid at all. ABI is not >broken if nothing was working in the first place, assuming it is >explained in commit msg (not the case here). Judging by the current schema, epss-l3 is defined as new HW block of aka not OSM L3, no matter which register is used for programming. > >Best regards, >Krzysztof
diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml index 21dae0b92819..e24399ca110f 100644 --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml @@ -28,12 +28,15 @@ properties: - const: qcom,osm-l3 - items: - enum: - - qcom,sc7280-epss-l3 - qcom,sc8280xp-epss-l3 - qcom,sm6375-cpucp-l3 - - qcom,sm8250-epss-l3 - qcom,sm8350-epss-l3 - const: qcom,epss-l3 + - items: + - enum: + - qcom,sc7280-epss-l3 + - qcom,sm8250-epss-l3 + - const: qcom,epss-l3-perf reg: maxItems: 1
EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible "qcom,epss-l3-perf" for these targets. Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> --- .../devicetree/bindings/interconnect/qcom,osm-l3.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)