[06/15] dt-bindings: display/msm: sc7180-dpu: Describe SM6125

  SM6125 is identical to SM6375 except that while downstream also defines
a throttle clock, its presence results in timeouts whereas SM6375
requires it to not observe any timeouts.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml | 1 +
 1 file changed, 1 insertion(+)
  

On 24/06/2023 02:41, Marijn Suijten wrote:
> SM6125 is identical to SM6375 except that while downstream also defines
> a throttle clock, its presence results in timeouts whereas SM6375
> requires it to not observe any timeouts.

Then it should not be allowed, so you need either "else:" block or
another "if: properties: compatible:" to disallow it. Because in current
patch it would be allowed.

Best regards,
Krzysztof
  

On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote:
> On 24/06/2023 02:41, Marijn Suijten wrote:
> > SM6125 is identical to SM6375 except that while downstream also defines
> > a throttle clock, its presence results in timeouts whereas SM6375
> > requires it to not observe any timeouts.
> 
> Then it should not be allowed, so you need either "else:" block or
> another "if: properties: compatible:" to disallow it. Because in current
> patch it would be allowed.

That means this binding is wrong/incomplete for all other SoCs then.
clock(-name)s has 6 items, and sets `minItems: 6`.  Only for sm6375-dpu
does it set `minItems: 7`, but an else case is missing.

Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm:
sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems:
6 be the default under clock(-name)s or in an else:?

- Marijn
  

On 24/06/2023 03:41, Marijn Suijten wrote:
> SM6125 is identical to SM6375 except that while downstream also defines
> a throttle clock, its presence results in timeouts whereas SM6375
> requires it to not observe any timeouts.

I see that the vendor DTS still references this clock.

Abhinav, Tanya, do possibly know what can be wrong here?

> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> index 630b11480496..6d2ba9a1cca1 100644
> --- a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> @@ -15,6 +15,7 @@ properties:
>     compatible:
>       enum:
>         - qcom,sc7180-dpu
> +      - qcom,sm6125-dpu
>         - qcom,sm6350-dpu
>         - qcom,sm6375-dpu
>   
>
  

On 25/06/2023 21:52, Marijn Suijten wrote:
> On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote:
>> On 24/06/2023 02:41, Marijn Suijten wrote:
>>> SM6125 is identical to SM6375 except that while downstream also defines
>>> a throttle clock, its presence results in timeouts whereas SM6375
>>> requires it to not observe any timeouts.
>>
>> Then it should not be allowed, so you need either "else:" block or
>> another "if: properties: compatible:" to disallow it. Because in current
>> patch it would be allowed.
> 
> That means this binding is wrong/incomplete for all other SoCs then.
> clock(-name)s has 6 items, and sets `minItems: 6`.  Only for sm6375-dpu
> does it set `minItems: 7`, but an else case is missing.

Ask the author why it is done like this.

> 
> Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm:
> sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems:
> 6 be the default under clock(-name)s or in an else:?

There is no bug to fix. Or at least it is not yet known. Whether other
devices should be constrained as well - sure, sounds reasonable, but I
did not check the code exactly.

We talk here about this patch.

Best regards,
Krzysztof
  

On 2023-06-26 18:16:58, Krzysztof Kozlowski wrote:
> On 25/06/2023 21:52, Marijn Suijten wrote:
> > On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote:
> >> On 24/06/2023 02:41, Marijn Suijten wrote:
> >>> SM6125 is identical to SM6375 except that while downstream also defines
> >>> a throttle clock, its presence results in timeouts whereas SM6375
> >>> requires it to not observe any timeouts.
> >>
> >> Then it should not be allowed, so you need either "else:" block or
> >> another "if: properties: compatible:" to disallow it. Because in current
> >> patch it would be allowed.
> > 
> > That means this binding is wrong/incomplete for all other SoCs then.
> > clock(-name)s has 6 items, and sets `minItems: 6`.  Only for sm6375-dpu

Of course meant to say that clock(-name)s has **7** items, not 6.

> > does it set `minItems: 7`, but an else case is missing.
> 
> Ask the author why it is done like this.

Konrad, can you clarify why other 

> > Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm:
> > sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems:
> > 6 be the default under clock(-name)s or in an else:?
> 
> There is no bug to fix. Or at least it is not yet known. Whether other
> devices should be constrained as well - sure, sounds reasonable, but I
> did not check the code exactly.

I don't know either, but we need this information to decide whether to
use `maxItems: 6`:

1. Directly on the property;
2. In an `else:` case on the current `if: sm6375-dpu` (should have the
   same effect as 1., afaik);
3. In a second `if:` case that lists all SoCS explicitly.

Since we don't have this information, I think option 3. is the right way
to go, setting `maxItems: 6` for qcom,sm6125-dpu.

However, it is not yet understood why downstream is able to use the
throttle clock without repercussions.

> We talk here about this patch.

We used this patch to discover that other SoCs are similarly
unconstrained.  But if you don't want me to look into it, by all means!
Saves me a lot of time.  So I will go with option 3.

- Marijn
  

On 26.06.2023 19:54, Marijn Suijten wrote:
> On 2023-06-26 18:16:58, Krzysztof Kozlowski wrote:
>> On 25/06/2023 21:52, Marijn Suijten wrote:
>>> On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote:
>>>> On 24/06/2023 02:41, Marijn Suijten wrote:
>>>>> SM6125 is identical to SM6375 except that while downstream also defines
>>>>> a throttle clock, its presence results in timeouts whereas SM6375
>>>>> requires it to not observe any timeouts.
>>>>
>>>> Then it should not be allowed, so you need either "else:" block or
>>>> another "if: properties: compatible:" to disallow it. Because in current
>>>> patch it would be allowed.
>>>
>>> That means this binding is wrong/incomplete for all other SoCs then.
>>> clock(-name)s has 6 items, and sets `minItems: 6`.  Only for sm6375-dpu
> 
> Of course meant to say that clock(-name)s has **7** items, not 6.
> 
>>> does it set `minItems: 7`, but an else case is missing.
>>
>> Ask the author why it is done like this.
> 
> Konrad, can you clarify why other 
6375 needs the throttle clk and the clock(-names) are strongly ordered
so having minItems: 6 discards the last entry

Konrad
> 
>>> Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm:
>>> sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems:
>>> 6 be the default under clock(-name)s or in an else:?
>>
>> There is no bug to fix. Or at least it is not yet known. Whether other
>> devices should be constrained as well - sure, sounds reasonable, but I
>> did not check the code exactly.
> 
> I don't know either, but we need this information to decide whether to
> use `maxItems: 6`:
> 
> 1. Directly on the property;
> 2. In an `else:` case on the current `if: sm6375-dpu` (should have the
>    same effect as 1., afaik);
> 3. In a second `if:` case that lists all SoCS explicitly.
> 
> Since we don't have this information, I think option 3. is the right way
> to go, setting `maxItems: 6` for qcom,sm6125-dpu.
> 
> However, it is not yet understood why downstream is able to use the
> throttle clock without repercussions.
> 
>> We talk here about this patch.
> 
> We used this patch to discover that other SoCs are similarly
> unconstrained.  But if you don't want me to look into it, by all means!
> Saves me a lot of time.  So I will go with option 3.
> 
> - Marijn
  

On 2023-06-26 20:57:51, Konrad Dybcio wrote:
> On 26.06.2023 19:54, Marijn Suijten wrote:
> > On 2023-06-26 18:16:58, Krzysztof Kozlowski wrote:
> >> On 25/06/2023 21:52, Marijn Suijten wrote:
> >>> On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote:
> >>>> On 24/06/2023 02:41, Marijn Suijten wrote:
> >>>>> SM6125 is identical to SM6375 except that while downstream also defines
> >>>>> a throttle clock, its presence results in timeouts whereas SM6375
> >>>>> requires it to not observe any timeouts.
> >>>>
> >>>> Then it should not be allowed, so you need either "else:" block or
> >>>> another "if: properties: compatible:" to disallow it. Because in current
> >>>> patch it would be allowed.
> >>>
> >>> That means this binding is wrong/incomplete for all other SoCs then.
> >>> clock(-name)s has 6 items, and sets `minItems: 6`.  Only for sm6375-dpu
> > 
> > Of course meant to say that clock(-name)s has **7** items, not 6.
> > 
> >>> does it set `minItems: 7`, but an else case is missing.
> >>
> >> Ask the author why it is done like this.
> > 
> > Konrad, can you clarify why other 

(Looks like I forgot to complete this sentence before sending,
apologies)

> 6375 needs the throttle clk and the clock(-names) are strongly ordered
> so having minItems: 6 discards the last entry

The question is whether or not we should have maxItems: 6 to disallow
the clock from being passed: right now it is optional and either is
allowed for any !6375 SoC.

- Marijn

> 
> Konrad
> > 
> >>> Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm:
> >>> sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems:
> >>> 6 be the default under clock(-name)s or in an else:?
> >>
> >> There is no bug to fix. Or at least it is not yet known. Whether other
> >> devices should be constrained as well - sure, sounds reasonable, but I
> >> did not check the code exactly.
> > 
> > I don't know either, but we need this information to decide whether to
> > use `maxItems: 6`:
> > 
> > 1. Directly on the property;
> > 2. In an `else:` case on the current `if: sm6375-dpu` (should have the
> >    same effect as 1., afaik);
> > 3. In a second `if:` case that lists all SoCS explicitly.
> > 
> > Since we don't have this information, I think option 3. is the right way
> > to go, setting `maxItems: 6` for qcom,sm6125-dpu.
> > 
> > However, it is not yet understood why downstream is able to use the
> > throttle clock without repercussions.
> > 
> >> We talk here about this patch.
> > 
> > We used this patch to discover that other SoCs are similarly
> > unconstrained.  But if you don't want me to look into it, by all means!
> > Saves me a lot of time.  So I will go with option 3.
> > 
> > - Marijn
  

On 26.06.2023 22:28, Marijn Suijten wrote:
> On 2023-06-26 20:57:51, Konrad Dybcio wrote:
>> On 26.06.2023 19:54, Marijn Suijten wrote:
>>> On 2023-06-26 18:16:58, Krzysztof Kozlowski wrote:
>>>> On 25/06/2023 21:52, Marijn Suijten wrote:
>>>>> On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote:
>>>>>> On 24/06/2023 02:41, Marijn Suijten wrote:
>>>>>>> SM6125 is identical to SM6375 except that while downstream also defines
>>>>>>> a throttle clock, its presence results in timeouts whereas SM6375
>>>>>>> requires it to not observe any timeouts.
>>>>>>
>>>>>> Then it should not be allowed, so you need either "else:" block or
>>>>>> another "if: properties: compatible:" to disallow it. Because in current
>>>>>> patch it would be allowed.
>>>>>
>>>>> That means this binding is wrong/incomplete for all other SoCs then.
>>>>> clock(-name)s has 6 items, and sets `minItems: 6`.  Only for sm6375-dpu
>>>
>>> Of course meant to say that clock(-name)s has **7** items, not 6.
>>>
>>>>> does it set `minItems: 7`, but an else case is missing.
>>>>
>>>> Ask the author why it is done like this.
>>>
>>> Konrad, can you clarify why other 
> 
> (Looks like I forgot to complete this sentence before sending,
> apologies)
> 
>> 6375 needs the throttle clk and the clock(-names) are strongly ordered
>> so having minItems: 6 discards the last entry
> 
> The question is whether or not we should have maxItems: 6 to disallow
> the clock from being passed: right now it is optional and either is
> allowed for any !6375 SoC.
That's a very good question. I don't have a 7180 to test, but for
you it seems to cause inexplicable issues on 6125..

Konrad
> 
> - Marijn
> 
>>
>> Konrad
>>>
>>>>> Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm:
>>>>> sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems:
>>>>> 6 be the default under clock(-name)s or in an else:?
>>>>
>>>> There is no bug to fix. Or at least it is not yet known. Whether other
>>>> devices should be constrained as well - sure, sounds reasonable, but I
>>>> did not check the code exactly.
>>>
>>> I don't know either, but we need this information to decide whether to
>>> use `maxItems: 6`:
>>>
>>> 1. Directly on the property;
>>> 2. In an `else:` case on the current `if: sm6375-dpu` (should have the
>>>    same effect as 1., afaik);
>>> 3. In a second `if:` case that lists all SoCS explicitly.
>>>
>>> Since we don't have this information, I think option 3. is the right way
>>> to go, setting `maxItems: 6` for qcom,sm6125-dpu.
>>>
>>> However, it is not yet understood why downstream is able to use the
>>> throttle clock without repercussions.
>>>
>>>> We talk here about this patch.
>>>
>>> We used this patch to discover that other SoCs are similarly
>>> unconstrained.  But if you don't want me to look into it, by all means!
>>> Saves me a lot of time.  So I will go with option 3.
>>>
>>> - Marijn
  

On 6/26/2023 7:04 AM, Dmitry Baryshkov wrote:
> On 24/06/2023 03:41, Marijn Suijten wrote:
>> SM6125 is identical to SM6375 except that while downstream also defines
>> a throttle clock, its presence results in timeouts whereas SM6375
>> requires it to not observe any timeouts.
> 
> I see that the vendor DTS still references this clock.
> 
> Abhinav, Tanya, do possibly know what can be wrong here?
> 

 From display side, we just enable it without any specific vote. Seeing 
timeouts without it makes sense but not with it.

I dont have experience with this family of chipsets and this clock is 
specific to this family of chipsets.

Will reach out to folks who might have a better idea about this clock 
and update with possible suggestions.

Unless ... tanya has more suggestions.

>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> ---
>>   Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml | 
>> 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml 
>> b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
>> index 630b11480496..6d2ba9a1cca1 100644
>> --- a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
>> @@ -15,6 +15,7 @@ properties:
>>     compatible:
>>       enum:
>>         - qcom,sc7180-dpu
>> +      - qcom,sm6125-dpu
>>         - qcom,sm6350-dpu
>>         - qcom,sm6375-dpu
>>
>