[PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Konrad Dybcio]

The DP PHY needs different settings when an eDP display is used.
Make sure these apply on the X13s.

FWIW
I could not notice any user-facing change stemming from this commit.

Fixes: f48c70b111b4 ("arm64: dts: qcom: sc8280xp-x13s: enable eDP display")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
I have no idea whether DP3 is hardwired to be eDP, like it
seems to be on the last DP controller of SC7280. In that
case this would be moved to the SoC DTSI.
---
 arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 38edaf51aa34..6a4c6cc19c09 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -601,6 +601,7 @@ mdss0_dp3_out: endpoint {
 };
 
 &mdss0_dp3_phy {
+	compatible = "qcom,sc8280xp-edp-phy";
 	vdda-phy-supply = <&vreg_l6b>;
 	vdda-pll-supply = <&vreg_l3b>;
 

---
base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
change-id: 20230929-topic-x13s_edpphy-0e172498c432

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Johan Hovold]

On Fri, Sep 29, 2023 at 06:02:57PM +0200, Konrad Dybcio wrote:
> The DP PHY needs different settings when an eDP display is used.
> Make sure these apply on the X13s.

Good catch. This looks to be more in line with what Bjorn intended.

You should fix up sc8280xp-crd and sa8295p-adp.dts as well however.

> FWIW
> I could not notice any user-facing change stemming from this commit.

I've seen some infrequent link-training failures (e.g. on resume) even if
it's been a while since last time now.

> Fixes: f48c70b111b4 ("arm64: dts: qcom: sc8280xp-x13s: enable eDP display")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> I have no idea whether DP3 is hardwired to be eDP, like it
> seems to be on the last DP controller of SC7280. In that
> case this would be moved to the SoC DTSI.

sa8295p-adp appears to use mdss[01]_dp[23] for eDP.

> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 38edaf51aa34..6a4c6cc19c09 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -601,6 +601,7 @@ mdss0_dp3_out: endpoint {
>  };
>  
>  &mdss0_dp3_phy {
> +	compatible = "qcom,sc8280xp-edp-phy";

Nit: Can you add a newline here after the compatible, please?

>  	vdda-phy-supply = <&vreg_l6b>;
>  	vdda-pll-supply = <&vreg_l3b>;

Johan

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Johan Hovold]

On Mon, Oct 02, 2023 at 10:54:57AM +0200, Johan Hovold wrote:
> On Fri, Sep 29, 2023 at 06:02:57PM +0200, Konrad Dybcio wrote:
> > The DP PHY needs different settings when an eDP display is used.
> > Make sure these apply on the X13s.
> 
> Good catch. This looks to be more in line with what Bjorn intended.
> 
> You should fix up sc8280xp-crd and sa8295p-adp.dts as well however.

As we discussed off-list, some of the ADP DP PHYs are apparently just
confusingly named "eDP" for some reason so that does not need fixing.

I sent a fix for the CRD here:

	https://lore.kernel.org/all/20231016080658.6667-1-johan+linaro@kernel.org/

> >  &mdss0_dp3_phy {
> > +	compatible = "qcom,sc8280xp-edp-phy";
> 
> Nit: Can you add a newline here after the compatible, please?

Would you mind respinning this one with an added newline here for
consistency, though?

> >  	vdda-phy-supply = <&vreg_l6b>;
> >  	vdda-pll-supply = <&vreg_l3b>;
 
Johan

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Dmitry Baryshkov]

On Fri, 29 Sept 2023 at 19:03, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> The DP PHY needs different settings when an eDP display is used.
> Make sure these apply on the X13s.

Could you please clarify, is it the same PHY type, just being
repurposed for eDP or is it a different PHY type?

If the former is the case (and the same PHY can be used for both DP
and eDP), it should carry the same compatible string and use software
mechanisms (e.g. phy_set_mode_ext()) to be programmed for the correct
operation mode.

>
> FWIW
> I could not notice any user-facing change stemming from this commit.
>
> Fixes: f48c70b111b4 ("arm64: dts: qcom: sc8280xp-x13s: enable eDP display")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> I have no idea whether DP3 is hardwired to be eDP, like it
> seems to be on the last DP controller of SC7280. In that
> case this would be moved to the SoC DTSI.
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 38edaf51aa34..6a4c6cc19c09 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -601,6 +601,7 @@ mdss0_dp3_out: endpoint {
>  };
>
>  &mdss0_dp3_phy {
> +       compatible = "qcom,sc8280xp-edp-phy";
>         vdda-phy-supply = <&vreg_l6b>;
>         vdda-pll-supply = <&vreg_l3b>;
>
>
> ---
> base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
> change-id: 20230929-topic-x13s_edpphy-0e172498c432
>
> Best regards,
> --
> Konrad Dybcio <konrad.dybcio@linaro.org>
>


-- 
With best wishes
Dmitry

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Johan Hovold]

On Mon, Oct 16, 2023 at 11:51:33AM +0300, Dmitry Baryshkov wrote:
> On Fri, 29 Sept 2023 at 19:03, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >
> > The DP PHY needs different settings when an eDP display is used.
> > Make sure these apply on the X13s.
> 
> Could you please clarify, is it the same PHY type, just being
> repurposed for eDP or is it a different PHY type?

Same PHY, just different settings AFAIK.

> If the former is the case (and the same PHY can be used for both DP
> and eDP), it should carry the same compatible string and use software
> mechanisms (e.g. phy_set_mode_ext()) to be programmed for the correct
> operation mode.

Possibly, but that's not how the current binding and implementation
works:

	6993c079cd58 ("dt-bindings: phy: qcom-edp: Add SC8280XP PHY compatibles")
	2300d1cb24b3 ("phy: qcom: edp: Introduce support for DisplayPort")
	3b7267dec445 ("phy: qcom: edp: Add SC8280XP eDP and DP PHYs")

	https://lore.kernel.org/lkml/20220810040745.3582985-1-bjorn.andersson@linaro.org/

And you'd still need to infer the mode from DT somehow.

Johan

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Dmitry Baryshkov]

On Mon, 16 Oct 2023 at 12:01, Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Oct 16, 2023 at 11:51:33AM +0300, Dmitry Baryshkov wrote:
> > On Fri, 29 Sept 2023 at 19:03, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > >
> > > The DP PHY needs different settings when an eDP display is used.
> > > Make sure these apply on the X13s.
> >
> > Could you please clarify, is it the same PHY type, just being
> > repurposed for eDP or is it a different PHY type?
>
> Same PHY, just different settings AFAIK.
>
> > If the former is the case (and the same PHY can be used for both DP
> > and eDP), it should carry the same compatible string and use software
> > mechanisms (e.g. phy_set_mode_ext()) to be programmed for the correct
> > operation mode.
>
> Possibly, but that's not how the current binding and implementation
> works:
>
>         6993c079cd58 ("dt-bindings: phy: qcom-edp: Add SC8280XP PHY compatibles")
>         2300d1cb24b3 ("phy: qcom: edp: Introduce support for DisplayPort")
>         3b7267dec445 ("phy: qcom: edp: Add SC8280XP eDP and DP PHYs")
>
>         https://lore.kernel.org/lkml/20220810040745.3582985-1-bjorn.andersson@linaro.org/
>
> And you'd still need to infer the mode from DT somehow.

If it is the same hardware block, it seems incorrect to have two
different compat entries. For example, for PCIe RC vs PCIe EP we
specify the PHY mode from the host controller driver.
I'd say, we need to fix the bindings for both DP/eDP controller and
the PHY.  See the `phy-mode` DT property for example.


-- 
With best wishes
Dmitry

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Bjorn Andersson]

On Mon, Oct 16, 2023 at 12:10:18PM +0300, Dmitry Baryshkov wrote:
> On Mon, 16 Oct 2023 at 12:01, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Mon, Oct 16, 2023 at 11:51:33AM +0300, Dmitry Baryshkov wrote:
> > > On Fri, 29 Sept 2023 at 19:03, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > >
> > > > The DP PHY needs different settings when an eDP display is used.
> > > > Make sure these apply on the X13s.
> > >
> > > Could you please clarify, is it the same PHY type, just being
> > > repurposed for eDP or is it a different PHY type?
> >
> > Same PHY, just different settings AFAIK.
> >
> > > If the former is the case (and the same PHY can be used for both DP
> > > and eDP), it should carry the same compatible string and use software
> > > mechanisms (e.g. phy_set_mode_ext()) to be programmed for the correct
> > > operation mode.
> >
> > Possibly, but that's not how the current binding and implementation
> > works:
> >
> >         6993c079cd58 ("dt-bindings: phy: qcom-edp: Add SC8280XP PHY compatibles")
> >         2300d1cb24b3 ("phy: qcom: edp: Introduce support for DisplayPort")
> >         3b7267dec445 ("phy: qcom: edp: Add SC8280XP eDP and DP PHYs")
> >
> >         https://lore.kernel.org/lkml/20220810040745.3582985-1-bjorn.andersson@linaro.org/
> >
> > And you'd still need to infer the mode from DT somehow.
> 
> If it is the same hardware block, it seems incorrect to have two
> different compat entries. For example, for PCIe RC vs PCIe EP we
> specify the PHY mode from the host controller driver.
> I'd say, we need to fix the bindings for both DP/eDP controller and
> the PHY.  See the `phy-mode` DT property for example.
> 

It is one hardware block, supporting both eDP and DP, so I like your
suggestion of having a single compatible instead and using some other
means of defining the configuration. I just wasn't able to find a
better way to do so back when I wrote the binding/driver...

Regards,
Bjorn

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Konrad Dybcio]

On 10/17/23 05:28, Bjorn Andersson wrote:
> On Mon, Oct 16, 2023 at 12:10:18PM +0300, Dmitry Baryshkov wrote:
>> On Mon, 16 Oct 2023 at 12:01, Johan Hovold <johan@kernel.org> wrote:
>>>
>>> On Mon, Oct 16, 2023 at 11:51:33AM +0300, Dmitry Baryshkov wrote:
>>>> On Fri, 29 Sept 2023 at 19:03, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>>
>>>>> The DP PHY needs different settings when an eDP display is used.
>>>>> Make sure these apply on the X13s.
>>>>
>>>> Could you please clarify, is it the same PHY type, just being
>>>> repurposed for eDP or is it a different PHY type?
>>>
>>> Same PHY, just different settings AFAIK.
>>>
>>>> If the former is the case (and the same PHY can be used for both DP
>>>> and eDP), it should carry the same compatible string and use software
>>>> mechanisms (e.g. phy_set_mode_ext()) to be programmed for the correct
>>>> operation mode.
>>>
>>> Possibly, but that's not how the current binding and implementation
>>> works:
>>>
>>>          6993c079cd58 ("dt-bindings: phy: qcom-edp: Add SC8280XP PHY compatibles")
>>>          2300d1cb24b3 ("phy: qcom: edp: Introduce support for DisplayPort")
>>>          3b7267dec445 ("phy: qcom: edp: Add SC8280XP eDP and DP PHYs")
>>>
>>>          https://lore.kernel.org/lkml/20220810040745.3582985-1-bjorn.andersson@linaro.org/
>>>
>>> And you'd still need to infer the mode from DT somehow.
>>
>> If it is the same hardware block, it seems incorrect to have two
>> different compat entries. For example, for PCIe RC vs PCIe EP we
>> specify the PHY mode from the host controller driver.
>> I'd say, we need to fix the bindings for both DP/eDP controller and
>> the PHY.  See the `phy-mode` DT property for example.
>>
> 
> It is one hardware block, supporting both eDP and DP, so I like your
> suggestion of having a single compatible instead and using some other
> means of defining the configuration. I just wasn't able to find a
> better way to do so back when I wrote the binding/driver...
Since this one is still unused, we can deprecate it (not sure if remove, 
but deprecate) and add phy-type instead. I was quite surprised to see 
that a new compatible was added as well :/

Konrad

Re: [PATCH] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible

[Bjorn Andersson]

On Fri, 29 Sep 2023 18:02:57 +0200, Konrad Dybcio wrote:
> The DP PHY needs different settings when an eDP display is used.
> Make sure these apply on the X13s.
> 
> FWIW
> I could not notice any user-facing change stemming from this commit.
> 
> 
> [...]

Applied, thanks!

[1/1] arm64: dts: qcom: sc8280xp-x13s: Use the correct DP PHY compatible
      commit: 0cd080dd6d08817c9980d2069197b066636b0f23

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>