diff mbox series

[V7,3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P

Message ID 20250111161429.51-4-quic_rlaggysh@quicinc.com
State New
Headers show
Series Add EPSS L3 provider support on SA8775P SoC | expand

Commit Message

Raviteja Laggyshetty Jan. 11, 2025, 4:14 p.m. UTC
Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
SA8775P SoCs.

Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
 Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml | 1 +
 1 file changed, 1 insertion(+)

Comments

Krzysztof Kozlowski Jan. 12, 2025, 9:31 a.m. UTC | #1
On Sat, Jan 11, 2025 at 04:14:27PM +0000, Raviteja Laggyshetty wrote:
> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
> SA8775P SoCs.

1. And why is this not compatible with sm8250? There was lengthy
discussion and no outcome of it managed to get to commit msg. Really, so
we are going to repeat everything again and you will not get any acks.

You have entire commit msg to explain things but instead you repeat what
the patch does. We can read the diff for that.

2. Binding *ALWAYS* comes before the user.

> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> index 21dae0b92819..94f7f283787a 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> @@ -33,6 +33,7 @@ properties:
>                - qcom,sm6375-cpucp-l3
>                - qcom,sm8250-epss-l3
>                - qcom,sm8350-epss-l3
> +              - qcom,sa8775p-epss-l3
>            - const: qcom,epss-l3

Your driver suggests this is not really true - it is not compatible with
qcom,epss-l3. Maybe it is, maybe not, no clue, commit explains nothing.

Best regards,
Krzysztof
Raviteja Laggyshetty Jan. 23, 2025, 12:07 p.m. UTC | #2
On 1/12/2025 3:01 PM, Krzysztof Kozlowski wrote:
> On Sat, Jan 11, 2025 at 04:14:27PM +0000, Raviteja Laggyshetty wrote:
>> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
>> SA8775P SoCs.
> 
> 1. And why is this not compatible with sm8250? There was lengthy
> discussion and no outcome of it managed to get to commit msg. Really, so
> we are going to repeat everything again and you will not get any acks.
> 
sa8775p is compatible with sm8250, but only difference is sa8775p has
two instances of L3 EPSS. Initial patches were posted with two
compatibles for supporting two instances.
Later there was a comment to reuse the existing and avoid growing the
compatibles in the driver.

Initially we thought to have separate generic compatible
"qcom,epss-l3-perf" for SoCs which support L3 voting through
EPSS_L3_PERF state register. But actually, it is already supported in
the driver for sm8250 and sc7280. We can reuse sm8250 or sc7280
compatible for sa8775p.

I will update the commit text with details in the next patch revision.


> You have entire commit msg to explain things but instead you repeat what
> the patch does. We can read the diff for that.
> 
> 2. Binding *ALWAYS* comes before the user.

Sure, I will update the patch order to bindings, driver and dt in next
patch revision and will follow the same for future patchsets.
> 
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> index 21dae0b92819..94f7f283787a 100644
>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> @@ -33,6 +33,7 @@ properties:
>>                - qcom,sm6375-cpucp-l3
>>                - qcom,sm8250-epss-l3
>>                - qcom,sm8350-epss-l3
>> +              - qcom,sa8775p-epss-l3
>>            - const: qcom,epss-l3
> 
> Your driver suggests this is not really true - it is not compatible with
> qcom,epss-l3. Maybe it is, maybe not, no clue, commit explains nothing.

"qcom,epss-l3" is generic compatible introduced for EPSS H/W from sm8250
SoC.  sm8250, sa8775p and sc7280 SoCs have same EPSS H/W and use
EPSS_L3_PERF register for configuring the perf level. We thought to add
generic compatible "qcom,epss-l3-perf" for configuring perf level
through EPSS_L3_PERF register.
 Later, as suggested by reviewers, looks like using a different register
cannot be a reason to have different generic compatible as the EPSS H/W
is still same on all the SoCs.
> 
> 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 21dae0b92819..94f7f283787a 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
@@ -33,6 +33,7 @@  properties:
               - qcom,sm6375-cpucp-l3
               - qcom,sm8250-epss-l3
               - qcom,sm8350-epss-l3
+              - qcom,sa8775p-epss-l3
           - const: qcom,epss-l3
 
   reg: