[PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Marijn Suijten]

The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
clock and should hence use PXO, not CXO which runs at 19.2MHz.

Note that none of the DSI PHY/PLL drivers currently use this "ref"
clock; they all rely on (sometimes inexistant) global clock names and
usually function normally without a parent clock.  This discrepancy will
be corrected in a future patch, for which this change needs to be in
place first.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 arch/arm/boot/dts/qcom-apq8064.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 2687c4e890ba..77659b783759 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -198,7 +198,7 @@ cxo_board: cxo_board {
 			clock-frequency = <19200000>;
 		};
 
-		pxo_board {
+		pxo_board: pxo_board {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
 			clock-frequency = <27000000>;
@@ -1306,7 +1306,7 @@ dsi0_phy: dsi-phy@4700200 {
 			reg-names = "dsi_pll", "dsi_phy", "dsi_phy_regulator";
 			clock-names = "iface_clk", "ref";
 			clocks = <&mmcc DSI_M_AHB_CLK>,
-				 <&cxo_board>;
+				 <&pxo_board>;
 		};
 
 
-- 
2.33.0

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Dmitry Baryshkov]

On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> clock and should hence use PXO, not CXO which runs at 19.2MHz.
>
> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> clock; they all rely on (sometimes inexistant) global clock names and
> usually function normally without a parent clock.  This discrepancy will
> be corrected in a future patch, for which this change needs to be in
> place first.
>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Checked the downstream driver, it always uses 27 MHz clock in calculations.

> ---
>  arch/arm/boot/dts/qcom-apq8064.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
> index 2687c4e890ba..77659b783759 100644
> --- a/arch/arm/boot/dts/qcom-apq8064.dtsi
> +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
> @@ -198,7 +198,7 @@ cxo_board: cxo_board {
>                         clock-frequency = <19200000>;
>                 };
>
> -               pxo_board {
> +               pxo_board: pxo_board {
>                         compatible = "fixed-clock";
>                         #clock-cells = <0>;
>                         clock-frequency = <27000000>;
> @@ -1306,7 +1306,7 @@ dsi0_phy: dsi-phy@4700200 {
>                         reg-names = "dsi_pll", "dsi_phy", "dsi_phy_regulator";
>                         clock-names = "iface_clk", "ref";
>                         clocks = <&mmcc DSI_M_AHB_CLK>,
> -                                <&cxo_board>;
> +                                <&pxo_board>;
>                 };
>
>
> --
> 2.33.0
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Marijn Suijten]

Hi Dmitry,

On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
>>
>> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
>> clock and should hence use PXO, not CXO which runs at 19.2MHz.
>>
>> Note that none of the DSI PHY/PLL drivers currently use this "ref"
>> clock; they all rely on (sometimes inexistant) global clock names and
>> usually function normally without a parent clock.  This discrepancy will
>> be corrected in a future patch, for which this change needs to be in
>> place first.
>>
>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Checked the downstream driver, it always uses 27 MHz clock in calculations.


Given our concerns for msm8974 not updating DT in parallel with the 
kernel (hence the need for a global-name fallback because "ref" is 
missing from the DT), should we worry about the same for apq8064?  That 
is, is there a chance that the kernel but not the firmware is upgraded 
leading to the wrong parent clock being used?  The msm8960 variant of 
the 28nm PLL driver uses parent_rate in a few places and might read 
cxo's 19.2MHz erroneously instead of using pxo's 27MHz.

- Marijn

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Dmitry Baryshkov]

On Mon, 30 Aug 2021 at 11:28, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Hi Dmitry,
>
> On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> >>
> >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> >> clock and should hence use PXO, not CXO which runs at 19.2MHz.
> >>
> >> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> >> clock; they all rely on (sometimes inexistant) global clock names and
> >> usually function normally without a parent clock.  This discrepancy will
> >> be corrected in a future patch, for which this change needs to be in
> >> place first.
> >>
> >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> > Checked the downstream driver, it always uses 27 MHz clock in calculations.
>
>
> Given our concerns for msm8974 not updating DT in parallel with the
> kernel (hence the need for a global-name fallback because "ref" is
> missing from the DT), should we worry about the same for apq8064?  That
> is, is there a chance that the kernel but not the firmware is upgraded
> leading to the wrong parent clock being used?  The msm8960 variant of
> the 28nm PLL driver uses parent_rate in a few places and might read
> cxo's 19.2MHz erroneously instead of using pxo's 27MHz.

Checked the code. It uses .parent_names =  "pxo", so changing ref
clock should not matter. We'd need to fix ref clocks and after that we
can switch parent names to fw_name.

>
> - Marijn



-- 
With best wishes
Dmitry

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Marijn Suijten]

On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote:
> On Mon, 30 Aug 2021 at 11:28, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Hi Dmitry,
> >
> > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > > <marijn.suijten@somainline.org> wrote:
> > >>
> > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> > >> clock and should hence use PXO, not CXO which runs at 19.2MHz.
> > >>
> > >> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> > >> clock; they all rely on (sometimes inexistant) global clock names and
> > >> usually function normally without a parent clock.  This discrepancy will
> > >> be corrected in a future patch, for which this change needs to be in
> > >> place first.
> > >>
> > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > >
> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
> > > Checked the downstream driver, it always uses 27 MHz clock in calculations.
> >
> >
> > Given our concerns for msm8974 not updating DT in parallel with the
> > kernel (hence the need for a global-name fallback because "ref" is
> > missing from the DT), should we worry about the same for apq8064?  That
> > is, is there a chance that the kernel but not the firmware is upgraded
> > leading to the wrong parent clock being used?  The msm8960 variant of
> > the 28nm PLL driver uses parent_rate in a few places and might read
> > cxo's 19.2MHz erroneously instead of using pxo's 27MHz.
> 
> Checked the code. It uses .parent_names =  "pxo", so changing ref
> clock should not matter. We'd need to fix ref clocks and after that we
> can switch parent names to fw_name.

Correct, hence why this patch is ordered before the switch to .fw_name.
These patches can't go in the same series if apq8064 doesn't update its
firmware in parallel with the kernel just like msm8974.  Do you know if
this is the case?  If so, how much time do you think should be between
the DT fix (this patch) and migrating the drivers?

- Marijn

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Dmitry Baryshkov]

On Mon, 30 Aug 2021 at 17:14, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote:
> > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > Hi Dmitry,
> > >
> > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > > > <marijn.suijten@somainline.org> wrote:
> > > >>
> > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz.
> > > >>
> > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> > > >> clock; they all rely on (sometimes inexistant) global clock names and
> > > >> usually function normally without a parent clock.  This discrepancy will
> > > >> be corrected in a future patch, for which this change needs to be in
> > > >> place first.
> > > >>
> > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > >
> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > >
> > > > Checked the downstream driver, it always uses 27 MHz clock in calculations.
> > >
> > >
> > > Given our concerns for msm8974 not updating DT in parallel with the
> > > kernel (hence the need for a global-name fallback because "ref" is
> > > missing from the DT), should we worry about the same for apq8064?  That
> > > is, is there a chance that the kernel but not the firmware is upgraded
> > > leading to the wrong parent clock being used?  The msm8960 variant of
> > > the 28nm PLL driver uses parent_rate in a few places and might read
> > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz.
> >
> > Checked the code. It uses .parent_names =  "pxo", so changing ref
> > clock should not matter. We'd need to fix ref clocks and after that we
> > can switch parent names to fw_name.
>
> Correct, hence why this patch is ordered before the switch to .fw_name.
> These patches can't go in the same series if apq8064 doesn't update its
> firmware in parallel with the kernel just like msm8974.  Do you know if
> this is the case?  If so, how much time do you think should be between
> the DT fix (this patch) and migrating the drivers?

You can have parent_data with .fw_name and .name in it.  .name will be
used as a fallback if .fw_name doesn't match.


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Marijn Suijten]

On Mon, Aug 30, 2021 at 05:18:37PM +0300, Dmitry Baryshkov wrote:
> On Mon, 30 Aug 2021 at 17:14, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten
> > > <marijn.suijten@somainline.org> wrote:
> > > >
> > > > Hi Dmitry,
> > > >
> > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > > > > <marijn.suijten@somainline.org> wrote:
> > > > >>
> > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz.
> > > > >>
> > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> > > > >> clock; they all rely on (sometimes inexistant) global clock names and
> > > > >> usually function normally without a parent clock.  This discrepancy will
> > > > >> be corrected in a future patch, for which this change needs to be in
> > > > >> place first.
> > > > >>
> > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > > >
> > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > >
> > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations.
> > > >
> > > >
> > > > Given our concerns for msm8974 not updating DT in parallel with the
> > > > kernel (hence the need for a global-name fallback because "ref" is
> > > > missing from the DT), should we worry about the same for apq8064?  That
> > > > is, is there a chance that the kernel but not the firmware is upgraded
> > > > leading to the wrong parent clock being used?  The msm8960 variant of
> > > > the 28nm PLL driver uses parent_rate in a few places and might read
> > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz.
> > >
> > > Checked the code. It uses .parent_names =  "pxo", so changing ref
> > > clock should not matter. We'd need to fix ref clocks and after that we
> > > can switch parent names to fw_name.
> >
> > Correct, hence why this patch is ordered before the switch to .fw_name.
> > These patches can't go in the same series if apq8064 doesn't update its
> > firmware in parallel with the kernel just like msm8974.  Do you know if
> > this is the case?  If so, how much time do you think should be between
> > the DT fix (this patch) and migrating the drivers?
> 
> You can have parent_data with .fw_name and .name in it.  .name will be
> used as a fallback if .fw_name doesn't match.

The problem is that it will always find the "ref" clock which references
&cxo_board until the DT is updated with this patch to use &pxo_board
instead.  Question is, will the kernel and DT usually/always be updated
in parallel?

- Marijn

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Bjorn Andersson]

On Mon 30 Aug 09:25 CDT 2021, Marijn Suijten wrote:

> On Mon, Aug 30, 2021 at 05:18:37PM +0300, Dmitry Baryshkov wrote:
> > On Mon, 30 Aug 2021 at 17:14, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote:
> > > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten
> > > > <marijn.suijten@somainline.org> wrote:
> > > > >
> > > > > Hi Dmitry,
> > > > >
> > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> > > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > > > > > <marijn.suijten@somainline.org> wrote:
> > > > > >>
> > > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> > > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz.
> > > > > >>
> > > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> > > > > >> clock; they all rely on (sometimes inexistant) global clock names and
> > > > > >> usually function normally without a parent clock.  This discrepancy will
> > > > > >> be corrected in a future patch, for which this change needs to be in
> > > > > >> place first.
> > > > > >>
> > > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > > > >
> > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > >
> > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations.
> > > > >
> > > > >
> > > > > Given our concerns for msm8974 not updating DT in parallel with the
> > > > > kernel (hence the need for a global-name fallback because "ref" is
> > > > > missing from the DT), should we worry about the same for apq8064?  That
> > > > > is, is there a chance that the kernel but not the firmware is upgraded
> > > > > leading to the wrong parent clock being used?  The msm8960 variant of
> > > > > the 28nm PLL driver uses parent_rate in a few places and might read
> > > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz.
> > > >
> > > > Checked the code. It uses .parent_names =  "pxo", so changing ref
> > > > clock should not matter. We'd need to fix ref clocks and after that we
> > > > can switch parent names to fw_name.
> > >
> > > Correct, hence why this patch is ordered before the switch to .fw_name.
> > > These patches can't go in the same series if apq8064 doesn't update its
> > > firmware in parallel with the kernel just like msm8974.  Do you know if
> > > this is the case?  If so, how much time do you think should be between
> > > the DT fix (this patch) and migrating the drivers?
> > 
> > You can have parent_data with .fw_name and .name in it.  .name will be
> > used as a fallback if .fw_name doesn't match.
> 
> The problem is that it will always find the "ref" clock which references
> &cxo_board until the DT is updated with this patch to use &pxo_board
> instead.  Question is, will the kernel and DT usually/always be updated
> in parallel?
> 

Afaik these devices all boots off a boot.img, which means that it's
unlikely that a new kernel is installed on a device with an older DT.
None of them would boot mainline off the DT that shipped with the
original product.

As such, if I pick this patch up as a fix for 5.15 you can respin the
other two patches and they can land in 5.16 and I would be surprised if
anyone will run into any issues with it.


I.e. I've applied this patch.

Regards,
Bjorn

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Marijn Suijten]

On Mon, Aug 30, 2021 at 10:17:37AM -0500, Bjorn Andersson wrote:
> On Mon 30 Aug 09:25 CDT 2021, Marijn Suijten wrote:
> 
> > On Mon, Aug 30, 2021 at 05:18:37PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, 30 Aug 2021 at 17:14, Marijn Suijten
> > > <marijn.suijten@somainline.org> wrote:
> > > >
> > > > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote:
> > > > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten
> > > > > <marijn.suijten@somainline.org> wrote:
> > > > > >
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> > > > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > > > > > > <marijn.suijten@somainline.org> wrote:
> > > > > > >>
> > > > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> > > > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz.
> > > > > > >>
> > > > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> > > > > > >> clock; they all rely on (sometimes inexistant) global clock names and
> > > > > > >> usually function normally without a parent clock.  This discrepancy will
> > > > > > >> be corrected in a future patch, for which this change needs to be in
> > > > > > >> place first.
> > > > > > >>
> > > > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > > > > >
> > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > >
> > > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations.
> > > > > >
> > > > > >
> > > > > > Given our concerns for msm8974 not updating DT in parallel with the
> > > > > > kernel (hence the need for a global-name fallback because "ref" is
> > > > > > missing from the DT), should we worry about the same for apq8064?  That
> > > > > > is, is there a chance that the kernel but not the firmware is upgraded
> > > > > > leading to the wrong parent clock being used?  The msm8960 variant of
> > > > > > the 28nm PLL driver uses parent_rate in a few places and might read
> > > > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz.
> > > > >
> > > > > Checked the code. It uses .parent_names =  "pxo", so changing ref
> > > > > clock should not matter. We'd need to fix ref clocks and after that we
> > > > > can switch parent names to fw_name.
> > > >
> > > > Correct, hence why this patch is ordered before the switch to .fw_name.
> > > > These patches can't go in the same series if apq8064 doesn't update its
> > > > firmware in parallel with the kernel just like msm8974.  Do you know if
> > > > this is the case?  If so, how much time do you think should be between
> > > > the DT fix (this patch) and migrating the drivers?
> > > 
> > > You can have parent_data with .fw_name and .name in it.  .name will be
> > > used as a fallback if .fw_name doesn't match.
> > 
> > The problem is that it will always find the "ref" clock which references
> > &cxo_board until the DT is updated with this patch to use &pxo_board
> > instead.  Question is, will the kernel and DT usually/always be updated
> > in parallel?
> > 
> 
> Afaik these devices all boots off a boot.img, which means that it's
> unlikely that a new kernel is installed on a device with an older DT.
> None of them would boot mainline off the DT that shipped with the
> original product.

That was my understanding as well, DT overlays are a "new thing" afaik
and most devices (at least all Sony's that I'm working with) use an
appended DTB that's always in-sync with the kernel image.

> As such, if I pick this patch up as a fix for 5.15 you can respin the
> other two patches and they can land in 5.16 and I would be surprised if
> anyone will run into any issues with it.
> 
> I.e. I've applied this patch.

Sounds good, I'll leave this patch out from v2.  Should it have a Fixes:
tag to get backported too?

Since most review seems to be in I'll respin v2 shortly with the
addition of the "ref" clock to msm8974, that should probably get the
same treatment (added to 5.15 fixes) then we can land this patchset in
5.16 (maybe without .name= fallback if Dmitry is okay with that).

- Marijn

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Bjorn Andersson]

On Mon 30 Aug 10:37 CDT 2021, Marijn Suijten wrote:

> On Mon, Aug 30, 2021 at 10:17:37AM -0500, Bjorn Andersson wrote:
> > On Mon 30 Aug 09:25 CDT 2021, Marijn Suijten wrote:
> > 
> > > On Mon, Aug 30, 2021 at 05:18:37PM +0300, Dmitry Baryshkov wrote:
> > > > On Mon, 30 Aug 2021 at 17:14, Marijn Suijten
> > > > <marijn.suijten@somainline.org> wrote:
> > > > >
> > > > > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote:
> > > > > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten
> > > > > > <marijn.suijten@somainline.org> wrote:
> > > > > > >
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote:
> > > > > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > > > > > > > <marijn.suijten@somainline.org> wrote:
> > > > > > > >>
> > > > > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference
> > > > > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz.
> > > > > > > >>
> > > > > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref"
> > > > > > > >> clock; they all rely on (sometimes inexistant) global clock names and
> > > > > > > >> usually function normally without a parent clock.  This discrepancy will
> > > > > > > >> be corrected in a future patch, for which this change needs to be in
> > > > > > > >> place first.
> > > > > > > >>
> > > > > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > > > > > >
> > > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > >
> > > > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations.
> > > > > > >
> > > > > > >
> > > > > > > Given our concerns for msm8974 not updating DT in parallel with the
> > > > > > > kernel (hence the need for a global-name fallback because "ref" is
> > > > > > > missing from the DT), should we worry about the same for apq8064?  That
> > > > > > > is, is there a chance that the kernel but not the firmware is upgraded
> > > > > > > leading to the wrong parent clock being used?  The msm8960 variant of
> > > > > > > the 28nm PLL driver uses parent_rate in a few places and might read
> > > > > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz.
> > > > > >
> > > > > > Checked the code. It uses .parent_names =  "pxo", so changing ref
> > > > > > clock should not matter. We'd need to fix ref clocks and after that we
> > > > > > can switch parent names to fw_name.
> > > > >
> > > > > Correct, hence why this patch is ordered before the switch to .fw_name.
> > > > > These patches can't go in the same series if apq8064 doesn't update its
> > > > > firmware in parallel with the kernel just like msm8974.  Do you know if
> > > > > this is the case?  If so, how much time do you think should be between
> > > > > the DT fix (this patch) and migrating the drivers?
> > > > 
> > > > You can have parent_data with .fw_name and .name in it.  .name will be
> > > > used as a fallback if .fw_name doesn't match.
> > > 
> > > The problem is that it will always find the "ref" clock which references
> > > &cxo_board until the DT is updated with this patch to use &pxo_board
> > > instead.  Question is, will the kernel and DT usually/always be updated
> > > in parallel?
> > > 
> > 
> > Afaik these devices all boots off a boot.img, which means that it's
> > unlikely that a new kernel is installed on a device with an older DT.
> > None of them would boot mainline off the DT that shipped with the
> > original product.
> 
> That was my understanding as well, DT overlays are a "new thing" afaik
> and most devices (at least all Sony's that I'm working with) use an
> appended DTB that's always in-sync with the kernel image.
> 

I think that with the introduction of DT overlays the system becomes
more flexible and as such more susceptible for bugs caused by unexpected
DT versions.

I think in practice the real issues comes when the DTB is delivered
separately (i.e. not by boot.img) or inbetween two kernel releases where
the Qualcomm tree might not be in sync with the driver tree.

> > As such, if I pick this patch up as a fix for 5.15 you can respin the
> > other two patches and they can land in 5.16 and I would be surprised if
> > anyone will run into any issues with it.
> > 
> > I.e. I've applied this patch.
> 
> Sounds good, I'll leave this patch out from v2.  Should it have a Fixes:
> tag to get backported too?
> 

Sounds good, I added to this:

Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")

> Since most review seems to be in I'll respin v2 shortly with the
> addition of the "ref" clock to msm8974, that should probably get the
> same treatment (added to 5.15 fixes) then we can land this patchset in
> 5.16 (maybe without .name= fallback if Dmitry is okay with that).
> 

Sounds good.

Regards,
Bjorn

Re: [PATCH 1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference

[Marijn Suijten]

On 2021-08-30 10:58:09, Bjorn Andersson wrote:
[...]
> > > 
> > > Afaik these devices all boots off a boot.img, which means that it's
> > > unlikely that a new kernel is installed on a device with an older DT.
> > > None of them would boot mainline off the DT that shipped with the
> > > original product.
> > 
> > That was my understanding as well, DT overlays are a "new thing" afaik
> > and most devices (at least all Sony's that I'm working with) use an
> > appended DTB that's always in-sync with the kernel image.
> > 
> 
> I think that with the introduction of DT overlays the system becomes
> more flexible and as such more susceptible for bugs caused by unexpected
> DT versions.

Offtopic: We have some problems with this on newer Sony devices where
the BL indeed tries to overlay this DTBO on the DT, which is usually a
downstream DT not fitting on top of a mainline kernel+appended-DTB.  The
solution is to simply wipe DTBO, and afaik it should be fine to compile
the overlay bits directly inside the appended-DTB anyway.  Leads to
unsuspecting problems at times, but it is manageable.

> I think in practice the real issues comes when the DTB is delivered
> separately (i.e. not by boot.img) or inbetween two kernel releases where
> the Qualcomm tree might not be in sync with the driver tree.

Dmitry sees this as a problem for msm8974 but I'm not familiar enough
with the board.  I take it this doesn't use appended DTBs then?

> > > As such, if I pick this patch up as a fix for 5.15 you can respin the
> > > other two patches and they can land in 5.16 and I would be surprised if
> > > anyone will run into any issues with it.
> > > 
> > > I.e. I've applied this patch.
> > 
> > Sounds good, I'll leave this patch out from v2.  Should it have a Fixes:
> > tag to get backported too?
> > 
> 
> Sounds good, I added to this:
> 
> Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")

Did the same on the v2 respin since it seems like those patches were
incomplete without the driver change.

> > Since most review seems to be in I'll respin v2 shortly with the
> > addition of the "ref" clock to msm8974, that should probably get the
> > same treatment (added to 5.15 fixes) then we can land this patchset in
> > 5.16 (maybe without .name= fallback if Dmitry is okay with that).
> > 
> 
> Sounds good.

Sent the msm8974 patch separately and re-spun a v2.  I haven't added the
.name="xo" fallback yet while awaiting Dmitry to see if that counts as
enough time for firmware to be delivered between kernel 5.15 and 5.16.

- Marijn

[PATCH 2/3] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent

[Marijn Suijten]

All DSI PHY/PLL drivers were referencing their VCO parent clock by a
global name, most of which don't exist or have been renamed.  These
clock drivers seem to function fine without that except the 14nm driver
for the sdm6xx [1].

At the same time all DTs provide a "ref" clock as per the requirements
of dsi-phy-common.yaml, but the clock is never used.  This patchset puts
that clock to use without relying on a global clock name, so that all
dependencies are explicitly defined in DT (the firmware) in the end.

[1]: https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c      | 4 +++-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c      | 4 +++-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c      | 4 +++-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 +++-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c       | 4 +++-
 5 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index e46b10fc793a..3cbb1f1475e8 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -562,7 +562,9 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm, struct clk_hw **prov
 	char clk_name[32], parent[32], vco_name[32];
 	char parent2[32], parent3[32], parent4[32];
 	struct clk_init_data vco_init = {
-		.parent_names = (const char *[]){ "xo" },
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "ref",
+		},
 		.num_parents = 1,
 		.name = vco_name,
 		.flags = CLK_IGNORE_UNUSED,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
index bb31230721bd..406470265408 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
@@ -804,7 +804,9 @@ static int pll_14nm_register(struct dsi_pll_14nm *pll_14nm, struct clk_hw **prov
 {
 	char clk_name[32], parent[32], vco_name[32];
 	struct clk_init_data vco_init = {
-		.parent_names = (const char *[]){ "xo" },
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "ref",
+		},
 		.num_parents = 1,
 		.name = vco_name,
 		.flags = CLK_IGNORE_UNUSED,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
index 2da673a2add6..8ee9c9c0548d 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -521,7 +521,9 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **prov
 {
 	char clk_name[32], parent1[32], parent2[32], vco_name[32];
 	struct clk_init_data vco_init = {
-		.parent_names = (const char *[]){ "xo" },
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "ref",
+		},
 		.num_parents = 1,
 		.name = vco_name,
 		.flags = CLK_IGNORE_UNUSED,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
index aaa37456f4ee..9662cb236468 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
@@ -385,7 +385,9 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **prov
 {
 	char *clk_name, *parent_name, *vco_name;
 	struct clk_init_data vco_init = {
-		.parent_names = (const char *[]){ "pxo" },
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "ref",
+		},
 		.num_parents = 1,
 		.flags = CLK_IGNORE_UNUSED,
 		.ops = &clk_ops_dsi_pll_28nm_vco,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 7c23d4c47338..c77c30628cca 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -590,7 +590,9 @@ static int pll_7nm_register(struct dsi_pll_7nm *pll_7nm, struct clk_hw **provide
 	char clk_name[32], parent[32], vco_name[32];
 	char parent2[32], parent3[32], parent4[32];
 	struct clk_init_data vco_init = {
-		.parent_names = (const char *[]){ "bi_tcxo" },
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "ref",
+		},
 		.num_parents = 1,
 		.name = vco_name,
 		.flags = CLK_IGNORE_UNUSED,
-- 
2.33.0

Re: [PATCH 2/3] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent

[Dmitry Baryshkov]

Hi,

On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> All DSI PHY/PLL drivers were referencing their VCO parent clock by a
> global name, most of which don't exist or have been renamed.  These
> clock drivers seem to function fine without that except the 14nm driver
> for the sdm6xx [1].
>
> At the same time all DTs provide a "ref" clock as per the requirements
> of dsi-phy-common.yaml, but the clock is never used.  This patchset puts
> that clock to use without relying on a global clock name, so that all
> dependencies are explicitly defined in DT (the firmware) in the end.

msm8974 (28nm-hpm) does not define the "ref" clock. So you'd have to:
1) add ref clock to the dtsi (should come in a separate patch).
2) add .name = "xo" as a fallback to the 28nm driver (to be compatible
with older devices)

Other than that this looks good to me.

>
> [1]: https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/
>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c      | 4 +++-
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c      | 4 +++-
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c      | 4 +++-
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 +++-
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c       | 4 +++-
>  5 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> index e46b10fc793a..3cbb1f1475e8 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> @@ -562,7 +562,9 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm, struct clk_hw **prov
>         char clk_name[32], parent[32], vco_name[32];
>         char parent2[32], parent3[32], parent4[32];
>         struct clk_init_data vco_init = {
> -               .parent_names = (const char *[]){ "xo" },
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .fw_name = "ref",
> +               },
>                 .num_parents = 1,
>                 .name = vco_name,
>                 .flags = CLK_IGNORE_UNUSED,
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> index bb31230721bd..406470265408 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> @@ -804,7 +804,9 @@ static int pll_14nm_register(struct dsi_pll_14nm *pll_14nm, struct clk_hw **prov
>  {
>         char clk_name[32], parent[32], vco_name[32];
>         struct clk_init_data vco_init = {
> -               .parent_names = (const char *[]){ "xo" },
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .fw_name = "ref",
> +               },
>                 .num_parents = 1,
>                 .name = vco_name,
>                 .flags = CLK_IGNORE_UNUSED,
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
> index 2da673a2add6..8ee9c9c0548d 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
> @@ -521,7 +521,9 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **prov
>  {
>         char clk_name[32], parent1[32], parent2[32], vco_name[32];
>         struct clk_init_data vco_init = {
> -               .parent_names = (const char *[]){ "xo" },
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .fw_name = "ref",
> +               },
>                 .num_parents = 1,
>                 .name = vco_name,
>                 .flags = CLK_IGNORE_UNUSED,
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
> index aaa37456f4ee..9662cb236468 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
> @@ -385,7 +385,9 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **prov
>  {
>         char *clk_name, *parent_name, *vco_name;
>         struct clk_init_data vco_init = {
> -               .parent_names = (const char *[]){ "pxo" },
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .fw_name = "ref",
> +               },
>                 .num_parents = 1,
>                 .flags = CLK_IGNORE_UNUSED,
>                 .ops = &clk_ops_dsi_pll_28nm_vco,
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> index 7c23d4c47338..c77c30628cca 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> @@ -590,7 +590,9 @@ static int pll_7nm_register(struct dsi_pll_7nm *pll_7nm, struct clk_hw **provide
>         char clk_name[32], parent[32], vco_name[32];
>         char parent2[32], parent3[32], parent4[32];
>         struct clk_init_data vco_init = {
> -               .parent_names = (const char *[]){ "bi_tcxo" },
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .fw_name = "ref",
> +               },
>                 .num_parents = 1,
>                 .name = vco_name,
>                 .flags = CLK_IGNORE_UNUSED,
> --
> 2.33.0
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent

[Marijn Suijten]

Hi Dmitry,

On 8/29/21 10:39 PM, Dmitry Baryshkov wrote:
> Hi,
> 
> On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
>>
>> All DSI PHY/PLL drivers were referencing their VCO parent clock by a
>> global name, most of which don't exist or have been renamed.  These
>> clock drivers seem to function fine without that except the 14nm driver
>> for the sdm6xx [1].
>>
>> At the same time all DTs provide a "ref" clock as per the requirements
>> of dsi-phy-common.yaml, but the clock is never used.  This patchset puts
>> that clock to use without relying on a global clock name, so that all
>> dependencies are explicitly defined in DT (the firmware) in the end.
> 
> msm8974 (28nm-hpm) does not define the "ref" clock. So you'd have to:
> 1) add ref clock to the dtsi (should come in a separate patch).


Thanks for double-checking and noticing this!  I've queued up this patch 
for v2.

> 2) add .name = "xo" as a fallback to the 28nm driver (to be compatible
> with older devices)


Are there msm8974 devices out there that might upgrade kernels, but not 
firmware (DT)?  On other boards (sdm630) I'm removing these from various 
drivers as to not have any possibility of relying on global names, in 
favour of having the clock dependencies fully specified in the DT.

> Other than that this looks good to me.


Any r-b/a-b/t-b I can pick up for the next round?

- Marijn

Re: [PATCH 2/3] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent

[Dmitry Baryshkov]

On Mon, 30 Aug 2021 at 00:53, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Hi Dmitry,
>
> On 8/29/21 10:39 PM, Dmitry Baryshkov wrote:
> > Hi,
> >
> > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> >>
> >> All DSI PHY/PLL drivers were referencing their VCO parent clock by a
> >> global name, most of which don't exist or have been renamed.  These
> >> clock drivers seem to function fine without that except the 14nm driver
> >> for the sdm6xx [1].
> >>
> >> At the same time all DTs provide a "ref" clock as per the requirements
> >> of dsi-phy-common.yaml, but the clock is never used.  This patchset puts
> >> that clock to use without relying on a global clock name, so that all
> >> dependencies are explicitly defined in DT (the firmware) in the end.
> >
> > msm8974 (28nm-hpm) does not define the "ref" clock. So you'd have to:
> > 1) add ref clock to the dtsi (should come in a separate patch).
>
>
> Thanks for double-checking and noticing this!  I've queued up this patch
> for v2.
>
> > 2) add .name = "xo" as a fallback to the 28nm driver (to be compatible
> > with older devices)
>
>
> Are there msm8974 devices out there that might upgrade kernels, but not
> firmware (DT)?  On other boards (sdm630) I'm removing these from various
> drivers as to not have any possibility of relying on global names, in
> favour of having the clock dependencies fully specified in the DT.

IIUC it is a general policy of trying to be (somewhat)
backwards-compatible. For example because your dts might come from a
different source/be a part of different build process/etc.

>
> > Other than that this looks good to me.
>
>
> Any r-b/a-b/t-b I can pick up for the next round?

Let's get those issues fixed and I'll respond with R-B tags


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent

[Marijn Suijten]

Hi Dmitry,

On Mon, Aug 30, 2021 at 04:17:32AM +0300, Dmitry Baryshkov wrote:
> On Mon, 30 Aug 2021 at 00:53, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Hi Dmitry,
> >
> > On 8/29/21 10:39 PM, Dmitry Baryshkov wrote:
> > > Hi,
> > >
> > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten
> > > <marijn.suijten@somainline.org> wrote:
> > >>
> > >> All DSI PHY/PLL drivers were referencing their VCO parent clock by a
> > >> global name, most of which don't exist or have been renamed.  These
> > >> clock drivers seem to function fine without that except the 14nm driver
> > >> for the sdm6xx [1].
> > >>
> > >> At the same time all DTs provide a "ref" clock as per the requirements
> > >> of dsi-phy-common.yaml, but the clock is never used.  This patchset puts
> > >> that clock to use without relying on a global clock name, so that all
> > >> dependencies are explicitly defined in DT (the firmware) in the end.
> > >
> > > msm8974 (28nm-hpm) does not define the "ref" clock. So you'd have to:
> > > 1) add ref clock to the dtsi (should come in a separate patch).
> >
> >
> > Thanks for double-checking and noticing this!  I've queued up this patch
> > for v2.
> >
> > > 2) add .name = "xo" as a fallback to the 28nm driver (to be compatible
> > > with older devices)
> >
> >
> > Are there msm8974 devices out there that might upgrade kernels, but not
> > firmware (DT)?  On other boards (sdm630) I'm removing these from various
> > drivers as to not have any possibility of relying on global names, in
> > favour of having the clock dependencies fully specified in the DT.
> 
> IIUC it is a general policy of trying to be (somewhat)
> backwards-compatible. For example because your dts might come from a
> different source/be a part of different build process/etc.

Good thinking; DT was after all intended to be used as firmware shipping
on the device, when we're usually modifying and shipping it with the
kernel in the end.

Just to make sure other platforms aren't affected by these changes,
every board currently providing a "ref" clock has done so since the DSI
node was added, except these for these three patches that added them
after the fact:

    79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY")
    6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")
    0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs")

Their commit-messages confuse me.  They make it seem like the "ref"
clock was previously used when this doesn't seem to be the case (hence
my patch).  Has there possibly been a patchset like mine that removed
the mentioned hardcoded clock, but ended up never being merged?

Either way, perhaps it's worth mentioning those patches with Fixes: so
that this commit can be backported (have to be careful that DT changes
for the other drivers are also backported, or this patch is split per
PHY file), and maybe it's worth cc-ing the original authors to ask for
clarification or at least make them aware?

- Marijn

[PATCH 3/3] clk: qcom: gcc-sdm660: Remove transient global "xo" clock

[Marijn Suijten]

The DSI PHY/PLL was relying on a global "xo" clock to be found, but the
real clock is named "xo_board" in the DT.  The standard nowadays is to
never use global clock names anymore but require the firmware (DT) to
provide every clock binding explicitly with .fw_name.  The DSI PLLs have
since been converted to this mechanism (specifically 14nm for SDM660)
and this transient clock can now be removed.

This issue was originally discovered in:
https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/
and prevented the removal of "xo" at that time.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/clk/qcom/gcc-sdm660.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c
index 9b97425008ce..16fd16351f95 100644
--- a/drivers/clk/qcom/gcc-sdm660.c
+++ b/drivers/clk/qcom/gcc-sdm660.c
@@ -37,19 +37,6 @@ enum {
 	P_GPLL1_EARLY_DIV,
 };
 
-static struct clk_fixed_factor xo = {
-	.mult = 1,
-	.div = 1,
-	.hw.init = &(struct clk_init_data){
-		.name = "xo",
-		.parent_data = &(const struct clk_parent_data) {
-			.fw_name = "xo"
-		},
-		.num_parents = 1,
-		.ops = &clk_fixed_factor_ops,
-	},
-};
-
 static struct clk_alpha_pll gpll0_early = {
 	.offset = 0x0,
 	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
@@ -2281,7 +2268,6 @@ static struct gdsc pcie_0_gdsc = {
 };
 
 static struct clk_hw *gcc_sdm660_hws[] = {
-	&xo.hw,
 	&gpll0_early_div.hw,
 	&gpll1_early_div.hw,
 };
-- 
2.33.0