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

[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