diff mbox series

[v3,2/3] dt-bindings: phy: qcom-edp: Add X1E80100 PHY compatibles

Message ID 20231122-phy-qualcomm-edp-x1e80100-v3-2-576fc4e9559d@linaro.org
State New
Headers show
Series phy: qcom: edp: Add support for X1E80100 | expand

Commit Message

Abel Vesa Dec. 7, 2023, 10:52 a.m. UTC
The Qualcomm X1E80100 platform has multiple PHYs that can work in both
eDP or DP mode, add compatibles for these. New platforms can use the
phy-type property to specify which mode the PHY should be configured in.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Krzysztof Kozlowski Dec. 8, 2023, 7:47 a.m. UTC | #1
On 07/12/2023 20:16, Konrad Dybcio wrote:
> 
> 
> On 12/7/23 17:51, Krzysztof Kozlowski wrote:
> 
> [...]
> 
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,x1e80100-dp-phy
>>> +    then:
>>> +      properties:
>>> +        phy-type:
>>> +          description: DP (default) or eDP type
>>
>> Properties must be defined in top-level "properties:" block. In
>> allOf:if:then you only disallow them for other variants.
>>
>>> +          enum: [ 6, 13 ]
>>> +          default: 6
>>
>> Anyway, I was thinking this should be rather argument to phy-cells.
> I'm not sure I'm for this, because the results would be:
> 
> --- device.dts ---
> &dp_controller0 {
>      phys = <&dp_phy0 PHY_EDP>;
> };
> 
> &dp_controller1 {
>      phys = <&dp_phy1 PHY_DP>;
> };
> ------------------
> 
> as opposed to:
> 
> --- device.dts ---
> &dp_phy0 {
>      phy-type <PHY_EDP>;
> };
> 
> &dp_phy1 {
>      phy-type = <PHY_DP>;
> };
> ------------------

Which is exactly what I proposed/wanted to see.

> 
> i.e., we would be saying "this board is connected to this phy
> instead" vs "this phy is of this type on this board".
> 
> While none of them really fit the "same hw, different config"
> situation, I'd vote for the latter one being closer to the
> truth

Then maybe I miss the bigger picture, but commit msg clearly says:
"multiple PHYs that can work in both eDP or DP mode"

If this is not the case, describe the hardware correctly in the commit
msg, so people will not ask stupid questions...

Best regards,
Krzysztof
Dmitry Baryshkov Dec. 8, 2023, 11:04 a.m. UTC | #2
On Fri, 8 Dec 2023 at 09:47, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 07/12/2023 20:16, Konrad Dybcio wrote:
> >
> >
> > On 12/7/23 17:51, Krzysztof Kozlowski wrote:
> >
> > [...]
> >
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - qcom,x1e80100-dp-phy
> >>> +    then:
> >>> +      properties:
> >>> +        phy-type:
> >>> +          description: DP (default) or eDP type
> >>
> >> Properties must be defined in top-level "properties:" block. In
> >> allOf:if:then you only disallow them for other variants.
> >>
> >>> +          enum: [ 6, 13 ]
> >>> +          default: 6
> >>
> >> Anyway, I was thinking this should be rather argument to phy-cells.
> > I'm not sure I'm for this, because the results would be:
> >
> > --- device.dts ---
> > &dp_controller0 {
> >      phys = <&dp_phy0 PHY_EDP>;
> > };
> >
> > &dp_controller1 {
> >      phys = <&dp_phy1 PHY_DP>;
> > };
> > ------------------
> >
> > as opposed to:
> >
> > --- device.dts ---
> > &dp_phy0 {
> >      phy-type <PHY_EDP>;
> > };
> >
> > &dp_phy1 {
> >      phy-type = <PHY_DP>;
> > };
> > ------------------
>
> Which is exactly what I proposed/wanted to see.
>
> >
> > i.e., we would be saying "this board is connected to this phy
> > instead" vs "this phy is of this type on this board".
> >
> > While none of them really fit the "same hw, different config"
> > situation, I'd vote for the latter one being closer to the
> > truth
>
> Then maybe I miss the bigger picture, but commit msg clearly says:
> "multiple PHYs that can work in both eDP or DP mode"
>
> If this is not the case, describe the hardware correctly in the commit
> msg, so people will not ask stupid questions...

There are multiple PHYs (each of them at its own address space). Each
of the PHYs in question can be used either for the DisplayPort output
(directly or through the USB-C) or to drive the eDP panel.

Same applies to the displayport-controller. It can either drive the DP
or eDP output, hardware-wise it is the same.
Krzysztof Kozlowski Dec. 8, 2023, 11:45 a.m. UTC | #3
On 08/12/2023 12:04, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 09:47, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 07/12/2023 20:16, Konrad Dybcio wrote:
>>>
>>>
>>> On 12/7/23 17:51, Krzysztof Kozlowski wrote:
>>>
>>> [...]
>>>
>>>>> +allOf:
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            enum:
>>>>> +              - qcom,x1e80100-dp-phy
>>>>> +    then:
>>>>> +      properties:
>>>>> +        phy-type:
>>>>> +          description: DP (default) or eDP type
>>>>
>>>> Properties must be defined in top-level "properties:" block. In
>>>> allOf:if:then you only disallow them for other variants.
>>>>
>>>>> +          enum: [ 6, 13 ]
>>>>> +          default: 6
>>>>
>>>> Anyway, I was thinking this should be rather argument to phy-cells.
>>> I'm not sure I'm for this, because the results would be:
>>>
>>> --- device.dts ---
>>> &dp_controller0 {
>>>      phys = <&dp_phy0 PHY_EDP>;
>>> };
>>>
>>> &dp_controller1 {
>>>      phys = <&dp_phy1 PHY_DP>;
>>> };
>>> ------------------
>>>
>>> as opposed to:
>>>
>>> --- device.dts ---
>>> &dp_phy0 {
>>>      phy-type <PHY_EDP>;
>>> };
>>>
>>> &dp_phy1 {
>>>      phy-type = <PHY_DP>;
>>> };
>>> ------------------
>>
>> Which is exactly what I proposed/wanted to see.
>>
>>>
>>> i.e., we would be saying "this board is connected to this phy
>>> instead" vs "this phy is of this type on this board".
>>>
>>> While none of them really fit the "same hw, different config"
>>> situation, I'd vote for the latter one being closer to the
>>> truth
>>
>> Then maybe I miss the bigger picture, but commit msg clearly says:
>> "multiple PHYs that can work in both eDP or DP mode"
>>
>> If this is not the case, describe the hardware correctly in the commit
>> msg, so people will not ask stupid questions...
> 
> There are multiple PHYs (each of them at its own address space). Each
> of the PHYs in question can be used either for the DisplayPort output
> (directly or through the USB-C) or to drive the eDP panel.
> 
> Same applies to the displayport-controller. It can either drive the DP
> or eDP output, hardware-wise it is the same.

Therefore what I proposed was correct - the block which uses the phy
configures its mode. Because this part:
  "this phy is of this type on this board".
is not true. The phy is both types.

Best regards,
Krzysztof
Dmitry Baryshkov Dec. 8, 2023, 12:17 p.m. UTC | #4
On Fri, 8 Dec 2023 at 13:45, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/12/2023 12:04, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 09:47, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 07/12/2023 20:16, Konrad Dybcio wrote:
> >>>
> >>>
> >>> On 12/7/23 17:51, Krzysztof Kozlowski wrote:
> >>>
> >>> [...]
> >>>
> >>>>> +allOf:
> >>>>> +  - if:
> >>>>> +      properties:
> >>>>> +        compatible:
> >>>>> +          contains:
> >>>>> +            enum:
> >>>>> +              - qcom,x1e80100-dp-phy
> >>>>> +    then:
> >>>>> +      properties:
> >>>>> +        phy-type:
> >>>>> +          description: DP (default) or eDP type
> >>>>
> >>>> Properties must be defined in top-level "properties:" block. In
> >>>> allOf:if:then you only disallow them for other variants.
> >>>>
> >>>>> +          enum: [ 6, 13 ]
> >>>>> +          default: 6
> >>>>
> >>>> Anyway, I was thinking this should be rather argument to phy-cells.
> >>> I'm not sure I'm for this, because the results would be:
> >>>
> >>> --- device.dts ---
> >>> &dp_controller0 {
> >>>      phys = <&dp_phy0 PHY_EDP>;
> >>> };
> >>>
> >>> &dp_controller1 {
> >>>      phys = <&dp_phy1 PHY_DP>;
> >>> };
> >>> ------------------
> >>>
> >>> as opposed to:
> >>>
> >>> --- device.dts ---
> >>> &dp_phy0 {
> >>>      phy-type <PHY_EDP>;
> >>> };
> >>>
> >>> &dp_phy1 {
> >>>      phy-type = <PHY_DP>;
> >>> };
> >>> ------------------
> >>
> >> Which is exactly what I proposed/wanted to see.
> >>
> >>>
> >>> i.e., we would be saying "this board is connected to this phy
> >>> instead" vs "this phy is of this type on this board".
> >>>
> >>> While none of them really fit the "same hw, different config"
> >>> situation, I'd vote for the latter one being closer to the
> >>> truth
> >>
> >> Then maybe I miss the bigger picture, but commit msg clearly says:
> >> "multiple PHYs that can work in both eDP or DP mode"
> >>
> >> If this is not the case, describe the hardware correctly in the commit
> >> msg, so people will not ask stupid questions...
> >
> > There are multiple PHYs (each of them at its own address space). Each
> > of the PHYs in question can be used either for the DisplayPort output
> > (directly or through the USB-C) or to drive the eDP panel.
> >
> > Same applies to the displayport-controller. It can either drive the DP
> > or eDP output, hardware-wise it is the same.
>
> Therefore what I proposed was correct - the block which uses the phy
> configures its mode. Because this part:
>   "this phy is of this type on this board".
> is not true. The phy is both types.

But hopefully you don't mean using #phy-cells here. There are no
sub-PHYs or anything like that.
Krzysztof Kozlowski Dec. 8, 2023, 12:20 p.m. UTC | #5
On 08/12/2023 13:17, Dmitry Baryshkov wrote:
>>>>>> Anyway, I was thinking this should be rather argument to phy-cells.
>>>>> I'm not sure I'm for this, because the results would be:
>>>>>
>>>>> --- device.dts ---
>>>>> &dp_controller0 {
>>>>>      phys = <&dp_phy0 PHY_EDP>;
>>>>> };
>>>>>
>>>>> &dp_controller1 {
>>>>>      phys = <&dp_phy1 PHY_DP>;
>>>>> };
>>>>> ------------------
>>>>>
>>>>> as opposed to:
>>>>>
>>>>> --- device.dts ---
>>>>> &dp_phy0 {
>>>>>      phy-type <PHY_EDP>;
>>>>> };
>>>>>
>>>>> &dp_phy1 {
>>>>>      phy-type = <PHY_DP>;
>>>>> };
>>>>> ------------------
>>>>
>>>> Which is exactly what I proposed/wanted to see.
>>>>
>>>>>
>>>>> i.e., we would be saying "this board is connected to this phy
>>>>> instead" vs "this phy is of this type on this board".
>>>>>
>>>>> While none of them really fit the "same hw, different config"
>>>>> situation, I'd vote for the latter one being closer to the
>>>>> truth
>>>>
>>>> Then maybe I miss the bigger picture, but commit msg clearly says:
>>>> "multiple PHYs that can work in both eDP or DP mode"
>>>>
>>>> If this is not the case, describe the hardware correctly in the commit
>>>> msg, so people will not ask stupid questions...
>>>
>>> There are multiple PHYs (each of them at its own address space). Each
>>> of the PHYs in question can be used either for the DisplayPort output
>>> (directly or through the USB-C) or to drive the eDP panel.
>>>
>>> Same applies to the displayport-controller. It can either drive the DP
>>> or eDP output, hardware-wise it is the same.
>>
>> Therefore what I proposed was correct - the block which uses the phy
>> configures its mode. Because this part:
>>   "this phy is of this type on this board".
>> is not true. The phy is both types.
> 
> But hopefully you don't mean using #phy-cells here. There are no
> sub-PHYs or anything like that.

I am exactly talking about phy-cells. Look at first example from Abel's
code.

Best regards,
Krzysztof
Dmitry Baryshkov Dec. 8, 2023, 12:35 p.m. UTC | #6
On Fri, 8 Dec 2023 at 14:21, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/12/2023 13:17, Dmitry Baryshkov wrote:
> >>>>>> Anyway, I was thinking this should be rather argument to phy-cells.
> >>>>> I'm not sure I'm for this, because the results would be:
> >>>>>
> >>>>> --- device.dts ---
> >>>>> &dp_controller0 {
> >>>>>      phys = <&dp_phy0 PHY_EDP>;
> >>>>> };
> >>>>>
> >>>>> &dp_controller1 {
> >>>>>      phys = <&dp_phy1 PHY_DP>;
> >>>>> };
> >>>>> ------------------
> >>>>>
> >>>>> as opposed to:
> >>>>>
> >>>>> --- device.dts ---
> >>>>> &dp_phy0 {
> >>>>>      phy-type <PHY_EDP>;
> >>>>> };
> >>>>>
> >>>>> &dp_phy1 {
> >>>>>      phy-type = <PHY_DP>;
> >>>>> };
> >>>>> ------------------
> >>>>
> >>>> Which is exactly what I proposed/wanted to see.
> >>>>
> >>>>>
> >>>>> i.e., we would be saying "this board is connected to this phy
> >>>>> instead" vs "this phy is of this type on this board".
> >>>>>
> >>>>> While none of them really fit the "same hw, different config"
> >>>>> situation, I'd vote for the latter one being closer to the
> >>>>> truth
> >>>>
> >>>> Then maybe I miss the bigger picture, but commit msg clearly says:
> >>>> "multiple PHYs that can work in both eDP or DP mode"
> >>>>
> >>>> If this is not the case, describe the hardware correctly in the commit
> >>>> msg, so people will not ask stupid questions...
> >>>
> >>> There are multiple PHYs (each of them at its own address space). Each
> >>> of the PHYs in question can be used either for the DisplayPort output
> >>> (directly or through the USB-C) or to drive the eDP panel.
> >>>
> >>> Same applies to the displayport-controller. It can either drive the DP
> >>> or eDP output, hardware-wise it is the same.
> >>
> >> Therefore what I proposed was correct - the block which uses the phy
> >> configures its mode. Because this part:
> >>   "this phy is of this type on this board".
> >> is not true. The phy is both types.
> >
> > But hopefully you don't mean using #phy-cells here. There are no
> > sub-PHYs or anything like that.
>
> I am exactly talking about phy-cells. Look at first example from Abel's
> code.

I always had an impression that #foo-cells means that there are
different units within the major handler. I.e. #clock-cells mean that
there are several different clocks, #reset-cells mean that there are
several resets, etc.
Ok, maybe this is not a perfect description. We need cells to identify
a particular instance within the major block. Maybe that sounds more
correct.

For the USB+DP PHY we use #phy-cells to select between USB3 and DP
PHYs. But for these PHYs we do not have sub-devices, sub-blocks, etc.
There is a single PHY which works in either of the modes.

Last, but not least, using #phy-cells in this way would create
asymmetry with all the other PHYs (and especially other QMP PHYs)
present on these platforms.

If you feel that phy-type is not an appropriate solution, I'd vote for
not having the type in DT at all, letting the DP controller determine
the proper mode on its own.
Krzysztof Kozlowski Dec. 8, 2023, 12:46 p.m. UTC | #7
On 08/12/2023 13:35, Dmitry Baryshkov wrote:
>>>>> Same applies to the displayport-controller. It can either drive the DP
>>>>> or eDP output, hardware-wise it is the same.
>>>>
>>>> Therefore what I proposed was correct - the block which uses the phy
>>>> configures its mode. Because this part:
>>>>   "this phy is of this type on this board".
>>>> is not true. The phy is both types.
>>>
>>> But hopefully you don't mean using #phy-cells here. There are no
>>> sub-PHYs or anything like that.
>>
>> I am exactly talking about phy-cells. Look at first example from Abel's
>> code.
> 
> I always had an impression that #foo-cells means that there are
> different units within the major handler. I.e. #clock-cells mean that
> there are several different clocks, #reset-cells mean that there are
> several resets, etc.
> Ok, maybe this is not a perfect description. We need cells to identify
> a particular instance within the major block. Maybe that sounds more
> correct.

No, the cells have also meaning of additional arguments. See usage of
phy-type (not the one here, but the correct one) and PWMs.

> 
> For the USB+DP PHY we use #phy-cells to select between USB3 and DP
> PHYs. But for these PHYs we do not have sub-devices, sub-blocks, etc.
> There is a single PHY which works in either of the modes.

Is it different than case here?

> 
> Last, but not least, using #phy-cells in this way would create
> asymmetry with all the other PHYs (and especially other QMP PHYs)
> present on these platforms.

OK. Is phy-type not something different?

> 
> If you feel that phy-type is not an appropriate solution, I'd vote for
> not having the type in DT at all, letting the DP controller determine
> the proper mode on its own.

Can we do it? That's BTW the best option.

Best regards,
Krzysztof
Dmitry Baryshkov Dec. 8, 2023, 1:09 p.m. UTC | #8
On Fri, 8 Dec 2023 at 14:47, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/12/2023 13:35, Dmitry Baryshkov wrote:
> >>>>> Same applies to the displayport-controller. It can either drive the DP
> >>>>> or eDP output, hardware-wise it is the same.
> >>>>
> >>>> Therefore what I proposed was correct - the block which uses the phy
> >>>> configures its mode. Because this part:
> >>>>   "this phy is of this type on this board".
> >>>> is not true. The phy is both types.
> >>>
> >>> But hopefully you don't mean using #phy-cells here. There are no
> >>> sub-PHYs or anything like that.
> >>
> >> I am exactly talking about phy-cells. Look at first example from Abel's
> >> code.
> >
> > I always had an impression that #foo-cells means that there are
> > different units within the major handler. I.e. #clock-cells mean that
> > there are several different clocks, #reset-cells mean that there are
> > several resets, etc.
> > Ok, maybe this is not a perfect description. We need cells to identify
> > a particular instance within the major block. Maybe that sounds more
> > correct.
>
> No, the cells have also meaning of additional arguments. See usage of
> phy-type (not the one here, but the correct one) and PWMs.

phy-type being used for the 7nm DSI PHY, where it specify exactly the
same thing: whether the connected device uses D-PHY or C-PHY modes of
the PHY.
cdns,phy-type - selecs between PCIe, DP, USB3 or other modes of the PHY
ti/emif.txt: phy-type specifies which PHY is attached / used in the controller
xlnx,phy-type: deprecated in favour of phy-mode, selects MII mode for the PHY
marvell,xenon-phy-type: I _think_ this specifies the actual PHY
attached to the controller in hardware.

> > For the USB+DP PHY we use #phy-cells to select between USB3 and DP
> > PHYs. But for these PHYs we do not have sub-devices, sub-blocks, etc.
> > There is a single PHY which works in either of the modes.
>
> Is it different than case here?

Hmm, I was not clear enough.

USB+DP = two different PHYs in the same hardware block.
DP-eDP = single PHY, working in one of the modes.

>
> >
> > Last, but not least, using #phy-cells in this way would create
> > asymmetry with all the other PHYs (and especially other QMP PHYs)
> > present on these platforms.
>
> OK. Is phy-type not something different?

No. It doesn't redefine what we already have for other QMP PHYs, it
defines new property.

>
> >
> > If you feel that phy-type is not an appropriate solution, I'd vote for
> > not having the type in DT at all, letting the DP controller determine
> > the proper mode on its own.
>
> Can we do it? That's BTW the best option.

That's a good question. We have separate -dp and -edp compatibles for
the DP controller, but I think those also should go, at least for
newer platforms. And the reason is the same, there is a single
hardware block, just two modes of operation. See mdss_dp3 in the
X13s's DT file.

I had a thought of using aux-bus presence to determine whether the
controller is working in the DP or eDP modes. But this might need
additional care for older DT files.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml
index 6566353f1a02..3341283577ce 100644
--- a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml
@@ -21,6 +21,7 @@  properties:
       - qcom,sc8180x-edp-phy
       - qcom,sc8280xp-dp-phy
       - qcom,sc8280xp-edp-phy
+      - qcom,x1e80100-dp-phy
 
   reg:
     items:
@@ -59,6 +60,20 @@  required:
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,x1e80100-dp-phy
+    then:
+      properties:
+        phy-type:
+          description: DP (default) or eDP type
+          enum: [ 6, 13 ]
+          default: 6
+
 examples:
   - |
     phy@aec2a00 {