diff mbox series

[3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT

Message ID 20210504052844.21096-4-shawn.guo@linaro.org
State New
Headers show
Series Add MSM8939 APCS/A53PLL clock support | expand

Commit Message

Shawn Guo May 4, 2021, 5:28 a.m. UTC
Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
retrieve the name from DT, so that multiple APCS clocks can be
registered.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/a53-pll.c      | 5 ++++-
 drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Stephen Boyd June 28, 2021, 12:28 a.m. UTC | #1
Quoting Shawn Guo (2021-05-03 22:28:42)
> Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for

> Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent

> Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,

> retrieve the name from DT, so that multiple APCS clocks can be

> registered.

> 

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

> ---

>  drivers/clk/qcom/a53-pll.c      | 5 ++++-

>  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-

>  2 files changed, 8 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c

> index 8614b0b0e82c..964f5ab7d02f 100644

> --- a/drivers/clk/qcom/a53-pll.c

> +++ b/drivers/clk/qcom/a53-pll.c

> @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)

>         struct clk_pll *pll;

>         void __iomem *base;

>         struct clk_init_data init = { };

> +       const char *clk_name = NULL;

>         int ret;

>  

>         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);

> @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)

>         pll->status_bit = 16;

>         pll->freq_tbl = a53pll_freq;

>  

> -       init.name = "a53pll";

> +       of_property_read_string(pdev->dev.of_node, "clock-output-names",

> +                               &clk_name);


Please no? Is there any use for this? Why not just generate the name as
a53pll@<MMIO ADDRESS>?

> +       init.name = clk_name ? clk_name : "a53pll";

>         init.parent_names = (const char *[]){ "xo" };

>         init.num_parents = 1;

>         init.ops = &clk_pll_sr2_ops;
Shawn Guo June 29, 2021, 1:36 p.m. UTC | #2
On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:
> Quoting Shawn Guo (2021-05-03 22:28:42)
> > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> > retrieve the name from DT, so that multiple APCS clocks can be
> > registered.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/clk/qcom/a53-pll.c      | 5 ++++-
> >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> > index 8614b0b0e82c..964f5ab7d02f 100644
> > --- a/drivers/clk/qcom/a53-pll.c
> > +++ b/drivers/clk/qcom/a53-pll.c
> > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> >         struct clk_pll *pll;
> >         void __iomem *base;
> >         struct clk_init_data init = { };
> > +       const char *clk_name = NULL;
> >         int ret;
> >  
> >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> >         pll->status_bit = 16;
> >         pll->freq_tbl = a53pll_freq;
> >  
> > -       init.name = "a53pll";
> > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> > +                               &clk_name);
> 
> Please no? Is there any use for this? Why not just generate the name as
> a53pll@<MMIO ADDRESS>?

There is no other use for this than getting different names.  I will do
what you suggest here.  Thanks!

Shawn

> 
> > +       init.name = clk_name ? clk_name : "a53pll";
> >         init.parent_names = (const char *[]){ "xo" };
> >         init.num_parents = 1;
> >         init.ops = &clk_pll_sr2_ops;
Bjorn Andersson June 29, 2021, 3:57 p.m. UTC | #3
On Tue 29 Jun 08:36 CDT 2021, Shawn Guo wrote:

> On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:

> > Quoting Shawn Guo (2021-05-03 22:28:42)

> > > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for

> > > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent

> > > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,

> > > retrieve the name from DT, so that multiple APCS clocks can be

> > > registered.

> > > 

> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

> > > ---

> > >  drivers/clk/qcom/a53-pll.c      | 5 ++++-

> > >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-

> > >  2 files changed, 8 insertions(+), 2 deletions(-)

> > > 

> > > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c

> > > index 8614b0b0e82c..964f5ab7d02f 100644

> > > --- a/drivers/clk/qcom/a53-pll.c

> > > +++ b/drivers/clk/qcom/a53-pll.c

> > > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)

> > >         struct clk_pll *pll;

> > >         void __iomem *base;

> > >         struct clk_init_data init = { };

> > > +       const char *clk_name = NULL;

> > >         int ret;

> > >  

> > >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);

> > > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)

> > >         pll->status_bit = 16;

> > >         pll->freq_tbl = a53pll_freq;

> > >  

> > > -       init.name = "a53pll";

> > > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",

> > > +                               &clk_name);

> > 

> > Please no? Is there any use for this? Why not just generate the name as

> > a53pll@<MMIO ADDRESS>?

> 

> There is no other use for this than getting different names.  I will do

> what you suggest here.  Thanks!

> 


I have exactly the same problem with my two DP PHYs (in
phy_dp_clks_register()), so I'm in favor of us setting some sort of
standard for this (not for anyone to rely on, but to avoid everyone
coming up with their own scheme).

But unfortunately I don't have easy access to the phy block's base
address in phy_dp_clks_register().

Regards,
Bjorn

> Shawn

> 

> > 

> > > +       init.name = clk_name ? clk_name : "a53pll";

> > >         init.parent_names = (const char *[]){ "xo" };

> > >         init.num_parents = 1;

> > >         init.ops = &clk_pll_sr2_ops;
Stephen Boyd June 29, 2021, 8:23 p.m. UTC | #4
Quoting Bjorn Andersson (2021-06-29 08:57:29)
> On Tue 29 Jun 08:36 CDT 2021, Shawn Guo wrote:
> 
> > On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:
> > > Quoting Shawn Guo (2021-05-03 22:28:42)
> > > > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> > > > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> > > > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> > > > retrieve the name from DT, so that multiple APCS clocks can be
> > > > registered.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > ---
> > > >  drivers/clk/qcom/a53-pll.c      | 5 ++++-
> > > >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
> > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> > > > index 8614b0b0e82c..964f5ab7d02f 100644
> > > > --- a/drivers/clk/qcom/a53-pll.c
> > > > +++ b/drivers/clk/qcom/a53-pll.c
> > > > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > >         struct clk_pll *pll;
> > > >         void __iomem *base;
> > > >         struct clk_init_data init = { };
> > > > +       const char *clk_name = NULL;
> > > >         int ret;
> > > >  
> > > >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> > > > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > >         pll->status_bit = 16;
> > > >         pll->freq_tbl = a53pll_freq;
> > > >  
> > > > -       init.name = "a53pll";
> > > > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> > > > +                               &clk_name);
> > > 
> > > Please no? Is there any use for this? Why not just generate the name as
> > > a53pll@<MMIO ADDRESS>?
> > 
> > There is no other use for this than getting different names.  I will do
> > what you suggest here.  Thanks!
> > 
> 
> I have exactly the same problem with my two DP PHYs (in
> phy_dp_clks_register()), so I'm in favor of us setting some sort of
> standard for this (not for anyone to rely on, but to avoid everyone
> coming up with their own scheme).
> 
> But unfortunately I don't have easy access to the phy block's base
> address in phy_dp_clks_register().

It really doesn't matter what name you use as it's basically only for
debugging. The problem is uniqueness. I've wondered if leaving the name
as NULL and then passing in a dev would be sufficient to generate a clk
name at runtime. Basically dev_name() plus an incrementing global
numberspace would probably work fine. Debugging would be annoying in
that case, but maybe it wouldn't matter.
Bjorn Andersson June 29, 2021, 8:39 p.m. UTC | #5
On Tue 29 Jun 15:23 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-29 08:57:29)
> > On Tue 29 Jun 08:36 CDT 2021, Shawn Guo wrote:
> > 
> > > On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:
> > > > Quoting Shawn Guo (2021-05-03 22:28:42)
> > > > > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> > > > > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> > > > > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> > > > > retrieve the name from DT, so that multiple APCS clocks can be
> > > > > registered.
> > > > > 
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > ---
> > > > >  drivers/clk/qcom/a53-pll.c      | 5 ++++-
> > > > >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
> > > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> > > > > index 8614b0b0e82c..964f5ab7d02f 100644
> > > > > --- a/drivers/clk/qcom/a53-pll.c
> > > > > +++ b/drivers/clk/qcom/a53-pll.c
> > > > > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > > >         struct clk_pll *pll;
> > > > >         void __iomem *base;
> > > > >         struct clk_init_data init = { };
> > > > > +       const char *clk_name = NULL;
> > > > >         int ret;
> > > > >  
> > > > >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> > > > > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > > >         pll->status_bit = 16;
> > > > >         pll->freq_tbl = a53pll_freq;
> > > > >  
> > > > > -       init.name = "a53pll";
> > > > > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> > > > > +                               &clk_name);
> > > > 
> > > > Please no? Is there any use for this? Why not just generate the name as
> > > > a53pll@<MMIO ADDRESS>?
> > > 
> > > There is no other use for this than getting different names.  I will do
> > > what you suggest here.  Thanks!
> > > 
> > 
> > I have exactly the same problem with my two DP PHYs (in
> > phy_dp_clks_register()), so I'm in favor of us setting some sort of
> > standard for this (not for anyone to rely on, but to avoid everyone
> > coming up with their own scheme).
> > 
> > But unfortunately I don't have easy access to the phy block's base
> > address in phy_dp_clks_register().
> 
> It really doesn't matter what name you use as it's basically only for
> debugging. The problem is uniqueness. I've wondered if leaving the name
> as NULL and then passing in a dev would be sufficient to generate a clk
> name at runtime. Basically dev_name() plus an incrementing global
> numberspace would probably work fine. Debugging would be annoying in
> that case, but maybe it wouldn't matter.

Something like "%s:link" and "%s:vco_div" based on dev_name() would be
quite nice in the case of DP. Probably more enjoyable to read than
dev_name():N and dev_name():N+1.

It comes with a cost of a few extra lines of code in each driver, but if
it's only a few drivers I don't think it warrants the extra logic in the
core.

Regards,
Bjorn
diff mbox series

Patch

diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
index 8614b0b0e82c..964f5ab7d02f 100644
--- a/drivers/clk/qcom/a53-pll.c
+++ b/drivers/clk/qcom/a53-pll.c
@@ -42,6 +42,7 @@  static int qcom_a53pll_probe(struct platform_device *pdev)
 	struct clk_pll *pll;
 	void __iomem *base;
 	struct clk_init_data init = { };
+	const char *clk_name = NULL;
 	int ret;
 
 	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
@@ -66,7 +67,9 @@  static int qcom_a53pll_probe(struct platform_device *pdev)
 	pll->status_bit = 16;
 	pll->freq_tbl = a53pll_freq;
 
-	init.name = "a53pll";
+	of_property_read_string(pdev->dev.of_node, "clock-output-names",
+				&clk_name);
+	init.name = clk_name ? clk_name : "a53pll";
 	init.parent_names = (const char *[]){ "xo" };
 	init.num_parents = 1;
 	init.ops = &clk_pll_sr2_ops;
diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index d7ac6d6b15b6..b8bbfe9622e1 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -49,6 +49,7 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	struct clk_regmap_mux_div *a53cc;
 	struct regmap *regmap;
 	struct clk_init_data init = { };
+	const char *clk_name = NULL;
 	int ret = -ENODEV;
 
 	regmap = dev_get_regmap(parent, NULL);
@@ -61,7 +62,9 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	if (!a53cc)
 		return -ENOMEM;
 
-	init.name = "a53mux";
+	of_property_read_string(parent->of_node, "clock-output-names",
+				&clk_name);
+	init.name = clk_name ? clk_name : "a53mux";
 	init.parent_data = pdata;
 	init.num_parents = ARRAY_SIZE(pdata);
 	init.ops = &clk_regmap_mux_div_ops;