| Message ID | 20231122-phy-qualcomm-edp-x1e80100-v3-2-576fc4e9559d@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: qcom: edp: Add support for X1E80100 | expand |
On 07/12/2023 11:52, Abel Vesa wrote: > 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(+) > > 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 Please put it after allOf: block (IOW, allOf: before additonalProperties:) > > +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. Best regards, Krzysztof
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>; }; ------------------ 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 Konrad
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
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.
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
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.
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
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.
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
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 --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 {
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(+)