mbox series

[0/9] dt-bindings: msm/dp: cleanup Qualcomm DP and eDP bidndings

Message ID 20220707213204.2605816-1-dmitry.baryshkov@linaro.org
Headers show
Series dt-bindings: msm/dp: cleanup Qualcomm DP and eDP bidndings | expand

Message

Dmitry Baryshkov July 7, 2022, 9:31 p.m. UTC
Fix several issues with the DP and eDP bindings on the Qualcomm
platforms. While we are at it, fix several small issues with platform
files declaring these controllers.

Dmitry Baryshkov (9):
  dt-bindings: msm/dp: drop extra p1 region
  dt-bindings: msm/dp: bring back support for legacy DP reg property
  dt-bindings: msm/dp: mark vdda supplies as deprecated
  dt-bindings: msm/dp: add missing properties
  dt-bindings: msm/dp: account for clocks specific for qcom,sc7280-edp
  dt-bindings: msm/dp: handle DP vs eDP difference
  arm64: dts: qcom: sc7180: drop #clock-cells from
    displayport-controller
  arm64: dts: qcom: sc7280: drop #clock-cells from
    displayport-controller
  arm64: dts: qcom: sc7280: drop address/size-cells from eDP node

 .../bindings/display/msm/dp-controller.yaml   | 115 ++++++++++++++----
 arch/arm64/boot/dts/qcom/sc7180.dtsi          |   1 -
 arch/arm64/boot/dts/qcom/sc7280.dtsi          |   5 -
 3 files changed, 91 insertions(+), 30 deletions(-)

Comments

Stephen Boyd July 8, 2022, 1:28 a.m. UTC | #1
Quoting Dmitry Baryshkov (2022-07-07 14:31:56)
> The p1 region was probably added by mistake, none of the DTS files
> provides one (and the driver source code also doesn't use one). Drop it
> now.

Yes, looks like the driver doesn't use it.

>
> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 94bc6e1b6451..d6bbe58ef9e8 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -29,7 +29,6 @@ properties:
>        - description: aux register block
>        - description: link register block
>        - description: p0 register block
> -      - description: p1 register block

The p1 registers exist on sc7180. They start where the example starts,
at 0xae91400.
Stephen Boyd July 8, 2022, 1:30 a.m. UTC | #2
Quoting Dmitry Baryshkov (2022-07-07 14:31:59)
> Document missing definitions for opp-table (DP controller OPPs), aux-bus
> (eDP AUX BUS) and data-lanes (DP/eDP lanes mapping) properties.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Dmitry Baryshkov July 8, 2022, 3:46 a.m. UTC | #3
On 08/07/2022 04:28, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-07-07 14:31:56)
>> The p1 region was probably added by mistake, none of the DTS files
>> provides one (and the driver source code also doesn't use one). Drop it
>> now.
> 
> Yes, looks like the driver doesn't use it.
> 
>>
>> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> index 94bc6e1b6451..d6bbe58ef9e8 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -29,7 +29,6 @@ properties:
>>         - description: aux register block
>>         - description: link register block
>>         - description: p0 register block
>> -      - description: p1 register block
> 
> The p1 registers exist on sc7180. They start where the example starts,
> at 0xae91400.

Do they exist on e.g. sc7280? In other words, should we add the region 
to the DTS? For now I'm going to mark it as optional.
Dmitry Baryshkov July 8, 2022, 3:59 a.m. UTC | #4
On 08/07/2022 04:32, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-07-07 14:32:00)
>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> index f00eae66196f..1ef845005b14 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -119,6 +111,50 @@ required:
>>     - power-domains
>>     - ports
>>
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,sc7280-edp
>> +    then:
>> +      properties:
>> +        clocks:
>> +          items:
>> +            - description: XO clock
> 
> What is this for? I would guess it's for the eDP phy, but that isn't
> part of the eDP controller, so probably it can be removed.

Good question. I was documenting what is present in the sc7280-edp 
controller DT entry. Could you please check if we can drop them? I don't 
have the hardware at hand.

> 
>> +            - description: eDP reference clock
> 
> Same for this one, looking at the binding for qcom,sc7280-edp-phy. Can
> we simply remove these two clks from sc7280? I think it will be fine.
Stephen Boyd July 8, 2022, 7:24 p.m. UTC | #5
Quoting Dmitry Baryshkov (2022-07-07 20:59:02)
> On 08/07/2022 04:32, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2022-07-07 14:32:00)
> >> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> index f00eae66196f..1ef845005b14 100644
> >> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> @@ -119,6 +111,50 @@ required:
> >>     - power-domains
> >>     - ports
> >>
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >> +              - qcom,sc7280-edp
> >> +    then:
> >> +      properties:
> >> +        clocks:
> >> +          items:
> >> +            - description: XO clock
> >
> > What is this for? I would guess it's for the eDP phy, but that isn't
> > part of the eDP controller, so probably it can be removed.
>
> Good question. I was documenting what is present in the sc7280-edp
> controller DT entry. Could you please check if we can drop them? I don't
> have the hardware at hand.
>

eDP works fine without those two clks on CRD (hoglin). They can be
dropped from the dtsi file.
Stephen Boyd July 8, 2022, 7:27 p.m. UTC | #6
Quoting Dmitry Baryshkov (2022-07-07 20:46:43)
> On 08/07/2022 04:28, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2022-07-07 14:31:56)
> >> The p1 region was probably added by mistake, none of the DTS files
> >> provides one (and the driver source code also doesn't use one). Drop it
> >> now.
> >
> > Yes, looks like the driver doesn't use it.
> >
> >>
> >> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition")
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> >>   Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 -
> >>   1 file changed, 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> index 94bc6e1b6451..d6bbe58ef9e8 100644
> >> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> @@ -29,7 +29,6 @@ properties:
> >>         - description: aux register block
> >>         - description: link register block
> >>         - description: p0 register block
> >> -      - description: p1 register block
> >
> > The p1 registers exist on sc7180. They start where the example starts,
> > at 0xae91400.
>
> Do they exist on e.g. sc7280? In other words, should we add the region
> to the DTS? For now I'm going to mark it as optional.
>

Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav,
can you confirm?
Abhinav Kumar July 8, 2022, 7:38 p.m. UTC | #7
+ kuogee

On 7/8/2022 12:27 PM, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-07-07 20:46:43)
>> On 08/07/2022 04:28, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-07-07 14:31:56)
>>>> The p1 region was probably added by mistake, none of the DTS files
>>>> provides one (and the driver source code also doesn't use one). Drop it
>>>> now.
>>>
>>> Yes, looks like the driver doesn't use it.
>>>
>>>>
>>>> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>    Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 -
>>>>    1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> index 94bc6e1b6451..d6bbe58ef9e8 100644
>>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> @@ -29,7 +29,6 @@ properties:
>>>>          - description: aux register block
>>>>          - description: link register block
>>>>          - description: p0 register block
>>>> -      - description: p1 register block
>>>
>>> The p1 registers exist on sc7180. They start where the example starts,
>>> at 0xae91400.
>>
>> Do they exist on e.g. sc7280? In other words, should we add the region
>> to the DTS? For now I'm going to mark it as optional.
>>
> 
> Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav,
> can you confirm?

P1 block does exist on sc7280 and yes its address is same as the address 
mentioned in sc7180. So its not a typo.

Yes, we are not programming this today but I would prefer to keep this 
as optional.

I did sync up with Kuogee on this change this morning, we will check a 
few things internally on the P1 block's usage as to which use-cases we 
need to program it for and update here.

The idea behind having this register space listed in the yaml is thats 
how the software documents have the blocks listed so dropping P1 block 
just because its unused seemed wrong to me. Optional seems more appropriate.

Thanks

Abhinav
Stephen Boyd July 8, 2022, 7:51 p.m. UTC | #8
Quoting Abhinav Kumar (2022-07-08 12:38:09)
> + kuogee
>
> On 7/8/2022 12:27 PM, Stephen Boyd wrote:
> >
> > Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav,
> > can you confirm?
>
> P1 block does exist on sc7280 and yes its address is same as the address
> mentioned in sc7180. So its not a typo.

Thanks!

>
> Yes, we are not programming this today but I would prefer to keep this
> as optional.
>
> I did sync up with Kuogee on this change this morning, we will check a
> few things internally on the P1 block's usage as to which use-cases we
> need to program it for and update here.
>
> The idea behind having this register space listed in the yaml is thats
> how the software documents have the blocks listed so dropping P1 block
> just because its unused seemed wrong to me. Optional seems more appropriate.
>

It doesn't sound optional on sc7180 or sc7280. It exists in the
hardware, so we should list the reg property. My understanding of
optional properties is for the case where something could be different
in the hardware design, like an optionally connected pin on a device.
Rob Herring July 8, 2022, 9:32 p.m. UTC | #9
On Fri, 08 Jul 2022 00:31:56 +0300, Dmitry Baryshkov wrote:
> The p1 region was probably added by mistake, none of the DTS files
> provides one (and the driver source code also doesn't use one). Drop it
> now.
> 
> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dp-controller.example.dtb: displayport-controller@ae90000: reg: [[183042048, 512], [183042560, 512], [183043072, 3072], [183046144, 1024], [183047168, 1024]] is too long
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dp-controller.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.