| Message ID | 20230624-sm6125-dpu-v1-6-1d5a638cebf2@somainline.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm: Add SM6125 MDSS/DPU hardware and enable Sony Xperia 10 II panel | expand |
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 >> >
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
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(+)