[PATCH v2 0/2] Use "ref" clocks from firmware for DSI PLL VCO parent

[PATCH v2 0/2] Use "ref" clocks from firmware for DSI PLL 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/

Changes since v1:
  - Dropped "arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL
    reference" which has made its way into 5.15-fixes in advance of this
    patchset landing in 5.16.
  - Added Fixes: tags for commits that added missing "ref" clocks to DT
    while this firmware clock was never used (until this patchset).
  - Documented missing/wrong and later-added clocks (by aforementioned
    patches) in patch 1/2 more clearly.

Dmitry:
  I have not added the .name="xo" fallback to the 28nm-hpm driver for
  the missing "ref" clock in msm8974 yet.  This patch is supposed to
  make it in for 5.16 while the missing clock should be added in 5.15,
  is that enough time?
  If not I'll gladly respin a v3 with that fallback, but I hope everyone
  can update their DT firmware before that time.  Likewise Bjorn
  acknowledged that there is enough time for the same to happen on
  apq8064.

Marijn Suijten (2):
  drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent
  clk: qcom: gcc-sdm660: Remove transient global "xo" clock

 drivers/clk/qcom/gcc-sdm660.c                   | 14 --------------
 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 +++-
 6 files changed, 15 insertions(+), 19 deletions(-)

-- 
2.33.0

[PATCH v2 1/2] 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.

Note that msm8974 is the only board not providing this clock, and
apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
pxo).  Both have been been addressed in separate patches that are
supposed to land well in advance of this patchset.

Furthermore not all board-DTs provided this clock initially but that
deficiency has been addressed in followup patches (see the Fixes:
below).  Those commits seem to assume that the clock was used, while
nothing in history indicates that this "ref" clock was ever retrieved.

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

Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY")
Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")
Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs")
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

[PATCH v2 2/2] 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

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

[Stephen Boyd]

Quoting Marijn Suijten (2021-08-30 11:24:44)
> 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.
> 
> Note that msm8974 is the only board not providing this clock, and
> apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
> pxo).  Both have been been addressed in separate patches that are
> supposed to land well in advance of this patchset.
> 
> Furthermore not all board-DTs provided this clock initially but that
> deficiency has been addressed in followup patches (see the Fixes:
> below).  Those commits seem to assume that the clock was used, while
> nothing in history indicates that this "ref" clock was ever retrieved.
> 
> [1]: https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/
> 
> Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY")
> Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")
> Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs")
> 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",

Please also add .name as the old parent_names value so that newer
kernels can be used without having to use new DT.

> +               },
>                 .num_parents = 1,
>                 .name = vco_name,
>                 .flags = CLK_IGNORE_UNUSED,

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

[Marijn Suijten]

Hi Stephen,

On 2021-08-30 15:16:13, Stephen Boyd wrote:
> Quoting Marijn Suijten (2021-08-30 11:24:44)
> > 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.
> > 
> > Note that msm8974 is the only board not providing this clock, and
> > apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
> > pxo).  Both have been been addressed in separate patches that are
> > supposed to land well in advance of this patchset.
> > 
> > Furthermore not all board-DTs provided this clock initially but that
> > deficiency has been addressed in followup patches (see the Fixes:
> > below).  Those commits seem to assume that the clock was used, while
> > nothing in history indicates that this "ref" clock was ever retrieved.
> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/
> > 
> > Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY")
> > Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")
> > Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs")
> > 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",
> 
> Please also add .name as the old parent_names value so that newer
> kernels can be used without having to use new DT.

We discussed that only msm8974 misses this "ref" clock at the time of
writing.  Aforementioned Fixes: patches have all been merged about 3
years ago, are those DTs still in use with a newer kernel?  I suppose
this patch is only backported to kernels including those DT patches, is
it reasonable to assume that at least that DT is in use there?

Besides, not all clock trees provide this global "xo" or "bi_tcxo" clock
in the first place.

- Marijn

> > +               },
> >                 .num_parents = 1,
> >                 .name = vco_name,
> >                 .flags = CLK_IGNORE_UNUSED,

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

[Stephen Boyd]

Quoting Marijn Suijten (2021-08-30 15:45:42)
> Hi Stephen,
> 
> On 2021-08-30 15:16:13, Stephen Boyd wrote:
> > Quoting Marijn Suijten (2021-08-30 11:24:44)
> > > 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.
> > > 
> > > Note that msm8974 is the only board not providing this clock, and
> > > apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
> > > pxo).  Both have been been addressed in separate patches that are
> > > supposed to land well in advance of this patchset.
> > > 
> > > Furthermore not all board-DTs provided this clock initially but that
> > > deficiency has been addressed in followup patches (see the Fixes:
> > > below).  Those commits seem to assume that the clock was used, while
> > > nothing in history indicates that this "ref" clock was ever retrieved.
> > > 
> > > [1]: https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/
> > > 
> > > Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY")
> > > Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")
> > > Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs")
> > > 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",
> > 
> > Please also add .name as the old parent_names value so that newer
> > kernels can be used without having to use new DT.
> 
> We discussed that only msm8974 misses this "ref" clock at the time of
> writing.  Aforementioned Fixes: patches have all been merged about 3
> years ago, are those DTs still in use with a newer kernel?  I suppose
> this patch is only backported to kernels including those DT patches, is
> it reasonable to assume that at least that DT is in use there?

I have no idea.

> 
> Besides, not all clock trees provide this global "xo" or "bi_tcxo" clock
> in the first place.
> 

It doesn't hurt to also specify a .name to help migrate anything else
over. Unless you're confident it won't cause problems to rely on proper
DT being used?

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

[Marijn Suijten]

On 2021-08-30 15:53:10, Stephen Boyd wrote:
> Quoting Marijn Suijten (2021-08-30 15:45:42)
> > Hi Stephen,
> > 
> > On 2021-08-30 15:16:13, Stephen Boyd wrote:
> > > Quoting Marijn Suijten (2021-08-30 11:24:44)
> > > > 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.
> > > > 
> > > > Note that msm8974 is the only board not providing this clock, and
> > > > apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
> > > > pxo).  Both have been been addressed in separate patches that are
> > > > supposed to land well in advance of this patchset.
> > > > 
> > > > Furthermore not all board-DTs provided this clock initially but that
> > > > deficiency has been addressed in followup patches (see the Fixes:
> > > > below).  Those commits seem to assume that the clock was used, while
> > > > nothing in history indicates that this "ref" clock was ever retrieved.
> > > > 
> > > > [1]: https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/
> > > > 
> > > > Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY")
> > > > Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")
> > > > Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs")
> > > > 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",
> > > 
> > > Please also add .name as the old parent_names value so that newer
> > > kernels can be used without having to use new DT.
> > 
> > We discussed that only msm8974 misses this "ref" clock at the time of
> > writing.  Aforementioned Fixes: patches have all been merged about 3
> > years ago, are those DTs still in use with a newer kernel?  I suppose
> > this patch is only backported to kernels including those DT patches, is
> > it reasonable to assume that at least that DT is in use there?
> 
> I have no idea.
> 
> > 
> > Besides, not all clock trees provide this global "xo" or "bi_tcxo" clock
> > in the first place.
> > 
> 
> It doesn't hurt to also specify a .name to help migrate anything else
> over. Unless you're confident it won't cause problems to rely on proper
> DT being used?

I'm 95% sure this shouldn't cause any problems given current DTs and
their history, but that's probably not enough.  This might also impact
DTs that have not yet been upstreamed, but afaik the general stance is
to not care and actually serve as a fair hint/warning before new DTs
make it to the list.

If there is a protocol in place to deprecate, warn, and eventually
remove this reliance on global clock names I'm more than happy to add
.name as a temporary fallback, even if likely unneeded.  Otherwise we
might never get rid of it.

- Marijn

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

[Stephen Boyd]

Quoting Marijn Suijten (2021-08-30 16:10:26)
> 
> I'm 95% sure this shouldn't cause any problems given current DTs and
> their history, but that's probably not enough.  This might also impact
> DTs that have not yet been upstreamed, but afaik the general stance is
> to not care and actually serve as a fair hint/warning before new DTs
> make it to the list.
> 
> If there is a protocol in place to deprecate, warn, and eventually
> remove this reliance on global clock names I'm more than happy to add
> .name as a temporary fallback, even if likely unneeded.  Otherwise we
> might never get rid of it.

I'm not aware of any protocol to deprecate, warn, and remove the
fallback name. It's a fallback because it can't ever really be removed.

Anyway, if you're not willing to add the .name then that's fine. We can
deal with the problem easily by adding a .name in the future if someone
complains that things aren't working. Sound like a plan? If so, it's
probably good to add some sort of note in the commit text so that when
the bisector lands on this patch they can realize that this
intentionally broke them.

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

[Stephen Boyd]

Quoting Marijn Suijten (2021-08-30 11:24:45)
> 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>
> ---

Presumably this wants to go with the first one.

Acked-by: Stephen Boyd <sboyd@kernel.org>

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

[Marijn Suijten]

Hi Stephen,

On 2021-08-31 22:35:12, Stephen Boyd wrote:
> Quoting Marijn Suijten (2021-08-30 16:10:26)
> > 
> > I'm 95% sure this shouldn't cause any problems given current DTs and
> > their history, but that's probably not enough.  This might also impact
> > DTs that have not yet been upstreamed, but afaik the general stance is
> > to not care and actually serve as a fair hint/warning before new DTs
> > make it to the list.
> > 
> > If there is a protocol in place to deprecate, warn, and eventually
> > remove this reliance on global clock names I'm more than happy to add
> > .name as a temporary fallback, even if likely unneeded.  Otherwise we
> > might never get rid of it.
> 
> I'm not aware of any protocol to deprecate, warn, and remove the
> fallback name. It's a fallback because it can't ever really be removed.

That is unfortunate, I was hoping for a breaking "kernel release" at
some point where we could say "no more, update your DTs first".  But
that may not be possible in every scenario?

> Anyway, if you're not willing to add the .name then that's fine.

I feel like .name has caused more problems for us than it solves, but in
a fallback position it might be fine.  My main gripe is that I don't
want DT to rely on the clock to also be discoverable through the clock
tree, which we've seen on many occasions (not sure if the former was
done upstream, but: "xo" being renamed to "xo_board", and DSI PLL clocks
loosing +1 causing a naming mismatch with what mmcc expects, to name
some examples).  Omitting .name is the only way to enforce that.

> We can
> deal with the problem easily by adding a .name in the future if someone
> complains that things aren't working. Sound like a plan? If so, it's
> probably good to add some sort of note in the commit text so that when
> the bisector lands on this patch they can realize that this
> intentionally broke them.

I'm all for this but lack the industrial knowledge to sign off on the
approach.  Bjorn and Dmitry should ack/agree before going ahead (you may
wonder why I'm worrying about getting clock drivers and DT in sync on
platforms I don't own...):

We have the following situations:
- apq8064 used the wrong clock.  Bjorn acknowledged that landing the DT
  fix in 5.15, and this patch in 5.16 should give enough time for DT to
  be updated (this is nothing we can fix with .name anyway).
- msm8974 doesn't have the clock at all.  Dmitry recommended to add
  .name for this specific case, but I'm wondering if the 5.15 -> 5.16
  window is enough to update DTs too?
- msm8916 and sdm845 had the missing "ref" clock added three years ago.
  Would a 5.16 kernel still work in any meaningful way with a DT that
  old?

Should we decide on a case-by-case basis whether to add .name, ie. only
for (some/all) of the aforementioned SoCs?

Thanks!

- Marijn

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

[Marijn Suijten]

On 2021-08-31 22:35:56, Stephen Boyd wrote:
> Quoting Marijn Suijten (2021-08-30 11:24:45)
> > 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>
> > ---
> 
> Presumably this wants to go with the first one.

What are you referring to with "the first one"?  This patch can only go
in after patch 1/2 of this series, unless you are suggesting to squash
it with Bjorns cleanup and making sure that lands after the fix in the
DSI?

> Acked-by: Stephen Boyd <sboyd@kernel.org>

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

[Stephen Boyd]

Quoting Marijn Suijten (2021-09-01 01:49:10)
> Hi Stephen,
> 
> On 2021-08-31 22:35:12, Stephen Boyd wrote:
> > Quoting Marijn Suijten (2021-08-30 16:10:26)
> > > 
> > > I'm 95% sure this shouldn't cause any problems given current DTs and
> > > their history, but that's probably not enough.  This might also impact
> > > DTs that have not yet been upstreamed, but afaik the general stance is
> > > to not care and actually serve as a fair hint/warning before new DTs
> > > make it to the list.
> > > 
> > > If there is a protocol in place to deprecate, warn, and eventually
> > > remove this reliance on global clock names I'm more than happy to add
> > > .name as a temporary fallback, even if likely unneeded.  Otherwise we
> > > might never get rid of it.
> > 
> > I'm not aware of any protocol to deprecate, warn, and remove the
> > fallback name. It's a fallback because it can't ever really be removed.
> 
> That is unfortunate, I was hoping for a breaking "kernel release" at
> some point where we could say "no more, update your DTs first".  But
> that may not be possible in every scenario?
> 
> > Anyway, if you're not willing to add the .name then that's fine.
> 
> I feel like .name has caused more problems for us than it solves, but in
> a fallback position it might be fine.  My main gripe is that I don't
> want DT to rely on the clock to also be discoverable through the clock
> tree, which we've seen on many occasions (not sure if the former was
> done upstream, but: "xo" being renamed to "xo_board", and DSI PLL clocks
> loosing +1 causing a naming mismatch with what mmcc expects, to name
> some examples).  Omitting .name is the only way to enforce that.

The simple approach to take is anything new should use fw_name. The name
member is only there for the backup mode when the DT isn't properly
setup to describe connections between clk controllers. If the code is
new then name can be omitted and everything is OK. Otherwise, if
parent_names was already being used, then most likely it will need to be
set as .name in the clk_parent_data struct to maintain compatibility. If
parent_names was wrong, then it was all broken to begin with and .name
can be omitted.

> 
> > We can
> > deal with the problem easily by adding a .name in the future if someone
> > complains that things aren't working. Sound like a plan? If so, it's
> > probably good to add some sort of note in the commit text so that when
> > the bisector lands on this patch they can realize that this
> > intentionally broke them.
> 
> I'm all for this but lack the industrial knowledge to sign off on the
> approach.  Bjorn and Dmitry should ack/agree before going ahead (you may
> wonder why I'm worrying about getting clock drivers and DT in sync on
> platforms I don't own...):
> 
> We have the following situations:
> - apq8064 used the wrong clock.  Bjorn acknowledged that landing the DT
>   fix in 5.15, and this patch in 5.16 should give enough time for DT to
>   be updated (this is nothing we can fix with .name anyway).
> - msm8974 doesn't have the clock at all.  Dmitry recommended to add
>   .name for this specific case, but I'm wondering if the 5.15 -> 5.16
>   window is enough to update DTs too?
> - msm8916 and sdm845 had the missing "ref" clock added three years ago.
>   Would a 5.16 kernel still work in any meaningful way with a DT that
>   old?
> 
> Should we decide on a case-by-case basis whether to add .name, ie. only
> for (some/all) of the aforementioned SoCs?
> 

I sort of glossed over this, sorry. Hopefully what I wrote above can
guide you and then we shouldn't really need to worry about anything?

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

[Stephen Boyd]

Quoting Marijn Suijten (2021-09-01 01:57:15)
> On 2021-08-31 22:35:56, Stephen Boyd wrote:
> > Quoting Marijn Suijten (2021-08-30 11:24:45)
> > > 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>
> > > ---
> > 
> > Presumably this wants to go with the first one.
> 
> What are you referring to with "the first one"?  This patch can only go
> in after patch 1/2 of this series, unless you are suggesting to squash
> it with Bjorns cleanup and making sure that lands after the fix in the
> DSI?

The first patch in this series.

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

[AngeloGioacchino Del Regno]

Il 30/08/21 20:24, Marijn Suijten ha scritto:
> 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.
> 
> Note that msm8974 is the only board not providing this clock, and
> apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
> pxo).  Both have been been addressed in separate patches that are
> supposed to land well in advance of this patchset.
> 
> Furthermore not all board-DTs provided this clock initially but that
> deficiency has been addressed in followup patches (see the Fixes:
> below).  Those commits seem to assume that the clock was used, while
> nothing in history indicates that this "ref" clock was ever retrieved.
> 
> [1]: https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1be96@somainline.org/
> 
> Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY")
> Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY")
> Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@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,
> 

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

[AngeloGioacchino Del Regno]

Il 30/08/21 20:24, Marijn Suijten ha scritto:
> 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>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@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,
>   };
> 

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

[Marijn Suijten]

On 2021-09-01 20:46:34, Stephen Boyd wrote:
> Quoting Marijn Suijten (2021-09-01 01:57:15)
> > On 2021-08-31 22:35:56, Stephen Boyd wrote:
> > > Quoting Marijn Suijten (2021-08-30 11:24:45)
> > > > 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>
> > > > ---
> > > 
> > > Presumably this wants to go with the first one.
> > 
> > What are you referring to with "the first one"?  This patch can only go
> > in after patch 1/2 of this series, unless you are suggesting to squash
> > it with Bjorns cleanup and making sure that lands after the fix in the
> > DSI?
> 
> The first patch in this series.

Are you suggesting to squash this patch into the first patch in this
series?  Note that the first patch touches drm/msm/dsi, the second
(this) patch touches clk/qcom.

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

[Stephen Boyd]

Quoting Marijn Suijten (2021-09-02 06:05:34)
> On 2021-09-01 20:46:34, Stephen Boyd wrote:
> > Quoting Marijn Suijten (2021-09-01 01:57:15)
> > > On 2021-08-31 22:35:56, Stephen Boyd wrote:
> > > > Quoting Marijn Suijten (2021-08-30 11:24:45)
> > > > > 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>
> > > > > ---
> > > > 
> > > > Presumably this wants to go with the first one.
> > > 
> > > What are you referring to with "the first one"?  This patch can only go
> > > in after patch 1/2 of this series, unless you are suggesting to squash
> > > it with Bjorns cleanup and making sure that lands after the fix in the
> > > DSI?
> > 
> > The first patch in this series.
> 
> Are you suggesting to squash this patch into the first patch in this
> series?  Note that the first patch touches drm/msm/dsi, the second
> (this) patch touches clk/qcom.

No.