[PATCH 1/6] clk: qcom: clk-rcg2: add rcg2 mux ops

[PATCH 1/6] clk: qcom: clk-rcg2: add rcg2 mux ops

[Robert Marko]

From: Christian Marangi <ansuelsmth@gmail.com>

An RCG may act as a mux that switch between 2 parents.
This is the case on IPQ6018 and IPQ8074 where the APCS core clk that feeds
the CPU cluster clock just switches between XO and the PLL that feeds it.

Add the required ops to add support for this special configuration and use
the generic mux function to determine the rate.

This way we dont have to keep a essentially dummy frequency table to use
RCG2 as a mux.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/clk/qcom/clk-rcg.h  | 1 +
 drivers/clk/qcom/clk-rcg2.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index 012e745794fd..01581f4d2c39 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -167,6 +167,7 @@ struct clk_rcg2_gfx3d {
 
 extern const struct clk_ops clk_rcg2_ops;
 extern const struct clk_ops clk_rcg2_floor_ops;
+extern const struct clk_ops clk_rcg2_mux_closest_ops;
 extern const struct clk_ops clk_edp_pixel_ops;
 extern const struct clk_ops clk_byte_ops;
 extern const struct clk_ops clk_byte2_ops;
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 28019edd2a50..609c10f8d0d9 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -509,6 +509,13 @@ const struct clk_ops clk_rcg2_floor_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_rcg2_floor_ops);
 
+const struct clk_ops clk_rcg2_mux_closest_ops = {
+	.determine_rate = __clk_mux_determine_rate_closest,
+	.get_parent = clk_rcg2_get_parent,
+	.set_parent = clk_rcg2_set_parent,
+};
+EXPORT_SYMBOL_GPL(clk_rcg2_mux_closest_ops);
+
 struct frac_entry {
 	int num;
 	int den;
-- 
2.36.1

[PATCH 2/6] clk: qcom: apss-ipq6018: fix apcs_alias0_clk_src

[Robert Marko]

While working on IPQ8074 APSS driver it was discovered that IPQ6018 and
IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is
currently broken.

More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux
clock.
However after debugging why it was always stuck at 800Mhz, it was figured
out that its not regmap_mux compatible at all.
It is a simple mux but it uses RCG2 register layout and control bits, so
utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not
having to provide a dummy frequency table.

While we are here, use ARRAY_SIZE for number of parents.

Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards.

Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller")
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/clk/qcom/apss-ipq6018.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
index d78ff2f310bf..be952d417ded 100644
--- a/drivers/clk/qcom/apss-ipq6018.c
+++ b/drivers/clk/qcom/apss-ipq6018.c
@@ -16,7 +16,7 @@
 #include "clk-regmap.h"
 #include "clk-branch.h"
 #include "clk-alpha-pll.h"
-#include "clk-regmap-mux.h"
+#include "clk-rcg.h"
 
 enum {
 	P_XO,
@@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
 	{ P_APSS_PLL_EARLY, 5 },
 };
 
-static struct clk_regmap_mux apcs_alias0_clk_src = {
-	.reg = 0x0050,
-	.width = 3,
-	.shift = 7,
+static struct clk_rcg2 apcs_alias0_clk_src = {
+	.cmd_rcgr = 0x0050,
+	.hid_width = 5,
 	.parent_map = parents_apcs_alias0_clk_src_map,
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "apcs_alias0_clk_src",
 		.parent_data = parents_apcs_alias0_clk_src,
-		.num_parents = 2,
-		.ops = &clk_regmap_mux_closest_ops,
+		.num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
+		.ops = &clk_rcg2_mux_closest_ops,
 		.flags = CLK_SET_RATE_PARENT,
 	},
 };
-- 
2.36.1

Re: [PATCH 2/6] clk: qcom: apss-ipq6018: fix apcs_alias0_clk_src

[Dmitry Baryshkov]

On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
>
> While working on IPQ8074 APSS driver it was discovered that IPQ6018 and
> IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is
> currently broken.
>
> More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux
> clock.
> However after debugging why it was always stuck at 800Mhz, it was figured
> out that its not regmap_mux compatible at all.
> It is a simple mux but it uses RCG2 register layout and control bits, so

To utilize control bits, you probably should also use

> utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not
> having to provide a dummy frequency table.

Could you please clarify this. Your new rcg2 ops seems to be literally
equivalent to the clk_regmap_mux_closest_ops provided the shift and
width are set correctly..

> While we are here, use ARRAY_SIZE for number of parents.
>
> Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards.
>
> Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller")
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/clk/qcom/apss-ipq6018.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> index d78ff2f310bf..be952d417ded 100644
> --- a/drivers/clk/qcom/apss-ipq6018.c
> +++ b/drivers/clk/qcom/apss-ipq6018.c
> @@ -16,7 +16,7 @@
>  #include "clk-regmap.h"
>  #include "clk-branch.h"
>  #include "clk-alpha-pll.h"
> -#include "clk-regmap-mux.h"
> +#include "clk-rcg.h"
>
>  enum {
>         P_XO,
> @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
>         { P_APSS_PLL_EARLY, 5 },
>  };
>
> -static struct clk_regmap_mux apcs_alias0_clk_src = {
> -       .reg = 0x0050,
> -       .width = 3,
> -       .shift = 7,

Judging from rcg2 ops, .shift should be set to 8.

> +static struct clk_rcg2 apcs_alias0_clk_src = {
> +       .cmd_rcgr = 0x0050,
> +       .hid_width = 5,
>         .parent_map = parents_apcs_alias0_clk_src_map,
>         .clkr.hw.init = &(struct clk_init_data){
>                 .name = "apcs_alias0_clk_src",
>                 .parent_data = parents_apcs_alias0_clk_src,
> -               .num_parents = 2,
> -               .ops = &clk_regmap_mux_closest_ops,
> +               .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
> +               .ops = &clk_rcg2_mux_closest_ops,
>                 .flags = CLK_SET_RATE_PARENT,
>         },
>  };
> --
> 2.36.1
>


--
With best wishes
Dmitry

Re: [PATCH 2/6] clk: qcom: apss-ipq6018: fix apcs_alias0_clk_src

[Robert Marko]

On Mon, 11 Jul 2022 at 14:48, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
> >
> > While working on IPQ8074 APSS driver it was discovered that IPQ6018 and
> > IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is
> > currently broken.
> >
> > More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux
> > clock.
> > However after debugging why it was always stuck at 800Mhz, it was figured
> > out that its not regmap_mux compatible at all.
> > It is a simple mux but it uses RCG2 register layout and control bits, so
>
> To utilize control bits, you probably should also use

Hi,
I am not really sure what you mean here?

>
> > utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not
> > having to provide a dummy frequency table.
>
> Could you please clarify this. Your new rcg2 ops seems to be literally
> equivalent to the clk_regmap_mux_closest_ops provided the shift and
> width are set correctly..

Well, I have tried playing with the clk_regmap_mux_closest_ops but I
just cannot get it
to work.

The width like you pointed out should be 8, register offset is
currently pointing at the RCG control
register and not the CFG one, so it obviously does not work.

Setting the register to 0x54 and shift to 8 will just fail silently,
leaving the shift at 7 and correcting
the register won't work as RCG control bits are not utilized at all
with regmap_mux and DIRTY_CFG
is active when I manually look at the register.

So, I am really not sure how clk_regmap_mux_closest_ops are supposed
to work here at all.

Regards,
Robert
>
> > While we are here, use ARRAY_SIZE for number of parents.
> >
> > Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards.
> >
> > Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller")
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> >  drivers/clk/qcom/apss-ipq6018.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> > index d78ff2f310bf..be952d417ded 100644
> > --- a/drivers/clk/qcom/apss-ipq6018.c
> > +++ b/drivers/clk/qcom/apss-ipq6018.c
> > @@ -16,7 +16,7 @@
> >  #include "clk-regmap.h"
> >  #include "clk-branch.h"
> >  #include "clk-alpha-pll.h"
> > -#include "clk-regmap-mux.h"
> > +#include "clk-rcg.h"
> >
> >  enum {
> >         P_XO,
> > @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
> >         { P_APSS_PLL_EARLY, 5 },
> >  };
> >
> > -static struct clk_regmap_mux apcs_alias0_clk_src = {
> > -       .reg = 0x0050,
> > -       .width = 3,
> > -       .shift = 7,
>
> Judging from rcg2 ops, .shift should be set to 8.
>
> > +static struct clk_rcg2 apcs_alias0_clk_src = {
> > +       .cmd_rcgr = 0x0050,
> > +       .hid_width = 5,
> >         .parent_map = parents_apcs_alias0_clk_src_map,
> >         .clkr.hw.init = &(struct clk_init_data){
> >                 .name = "apcs_alias0_clk_src",
> >                 .parent_data = parents_apcs_alias0_clk_src,
> > -               .num_parents = 2,
> > -               .ops = &clk_regmap_mux_closest_ops,
> > +               .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
> > +               .ops = &clk_rcg2_mux_closest_ops,
> >                 .flags = CLK_SET_RATE_PARENT,
> >         },
> >  };
> > --
> > 2.36.1
> >
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH 2/6] clk: qcom: apss-ipq6018: fix apcs_alias0_clk_src

[Dmitry Baryshkov]

On Mon, 11 Jul 2022 at 16:23, Robert Marko <robimarko@gmail.com> wrote:
>
> On Mon, 11 Jul 2022 at 14:48, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
> > >
> > > While working on IPQ8074 APSS driver it was discovered that IPQ6018 and
> > > IPQ8074 use almost the same PLL and APSS clocks, however APSS driver is
> > > currently broken.
> > >
> > > More precisely apcs_alias0_clk_src is broken, it was added as regmap_mux
> > > clock.
> > > However after debugging why it was always stuck at 800Mhz, it was figured
> > > out that its not regmap_mux compatible at all.
> > > It is a simple mux but it uses RCG2 register layout and control bits, so
> >
> > To utilize control bits, you probably should also use
>
> Hi,
> I am not really sure what you mean here?

Ugh, excuse me. Sent the message too early.
I mean that to utilize RCG2 control bits, you probably also need to
use clk_rcg2_is_enabled, etc.

>
> >
> > > utilize the new clk_rcg2_mux_closest_ops to correctly drive it while not
> > > having to provide a dummy frequency table.
> >
> > Could you please clarify this. Your new rcg2 ops seems to be literally
> > equivalent to the clk_regmap_mux_closest_ops provided the shift and
> > width are set correctly..
>
> Well, I have tried playing with the clk_regmap_mux_closest_ops but I
> just cannot get it
> to work.
>
> The width like you pointed out should be 8, register offset is
> currently pointing at the RCG control
> register and not the CFG one, so it obviously does not work.
>
> Setting the register to 0x54 and shift to 8 will just fail silently,
> leaving the shift at 7 and correcting
> the register won't work as RCG control bits are not utilized at all
> with regmap_mux and DIRTY_CFG
> is active when I manually look at the register.

Ok, I missed the update_cfg part. So, yes, this looks correct.

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

>
> So, I am really not sure how clk_regmap_mux_closest_ops are supposed
> to work here at all.
>
> Regards,
> Robert
> >
> > > While we are here, use ARRAY_SIZE for number of parents.
> > >
> > > Tested on IPQ6018-CP01-C1 reference board and multiple IPQ8074 boards.
> > >
> > > Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller")
> > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > ---
> > >  drivers/clk/qcom/apss-ipq6018.c | 13 ++++++-------
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> > > index d78ff2f310bf..be952d417ded 100644
> > > --- a/drivers/clk/qcom/apss-ipq6018.c
> > > +++ b/drivers/clk/qcom/apss-ipq6018.c
> > > @@ -16,7 +16,7 @@
> > >  #include "clk-regmap.h"
> > >  #include "clk-branch.h"
> > >  #include "clk-alpha-pll.h"
> > > -#include "clk-regmap-mux.h"
> > > +#include "clk-rcg.h"
> > >
> > >  enum {
> > >         P_XO,
> > > @@ -33,16 +33,15 @@ static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
> > >         { P_APSS_PLL_EARLY, 5 },
> > >  };
> > >
> > > -static struct clk_regmap_mux apcs_alias0_clk_src = {
> > > -       .reg = 0x0050,
> > > -       .width = 3,
> > > -       .shift = 7,
> >
> > Judging from rcg2 ops, .shift should be set to 8.
> >
> > > +static struct clk_rcg2 apcs_alias0_clk_src = {
> > > +       .cmd_rcgr = 0x0050,
> > > +       .hid_width = 5,
> > >         .parent_map = parents_apcs_alias0_clk_src_map,
> > >         .clkr.hw.init = &(struct clk_init_data){
> > >                 .name = "apcs_alias0_clk_src",
> > >                 .parent_data = parents_apcs_alias0_clk_src,
> > > -               .num_parents = 2,
> > > -               .ops = &clk_regmap_mux_closest_ops,
> > > +               .num_parents = ARRAY_SIZE(parents_apcs_alias0_clk_src),
> > > +               .ops = &clk_rcg2_mux_closest_ops,
> > >                 .flags = CLK_SET_RATE_PARENT,
> > >         },
> > >  };
> > > --
> > > 2.36.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

[PATCH 3/6] clk: qcom: apss-ipq6018: mark apcs_alias0_core_clk as critical

[Robert Marko]

While fixing up the driver I noticed that my IPQ8074 board was hanging
after CPUFreq switched the frequency during boot, WDT would eventually
reset it.

So mark apcs_alias0_core_clk as critical since its the clock feeding the
CPU cluster and must never be disabled.

Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller")
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/clk/qcom/apss-ipq6018.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
index be952d417ded..f2f502e2d5a4 100644
--- a/drivers/clk/qcom/apss-ipq6018.c
+++ b/drivers/clk/qcom/apss-ipq6018.c
@@ -56,7 +56,7 @@ static struct clk_branch apcs_alias0_core_clk = {
 			.parent_hws = (const struct clk_hw *[]){
 				&apcs_alias0_clk_src.clkr.hw },
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
 			.ops = &clk_branch2_ops,
 		},
 	},
-- 
2.36.1

Re: [PATCH 3/6] clk: qcom: apss-ipq6018: mark apcs_alias0_core_clk as critical

[Dmitry Baryshkov]

On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
>
> While fixing up the driver I noticed that my IPQ8074 board was hanging
> after CPUFreq switched the frequency during boot, WDT would eventually
> reset it.
>
> So mark apcs_alias0_core_clk as critical since its the clock feeding the
> CPU cluster and must never be disabled.

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

>
> Fixes: 5e77b4ef1b19 ("clk: qcom: Add ipq6018 apss clock controller")
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/clk/qcom/apss-ipq6018.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> index be952d417ded..f2f502e2d5a4 100644
> --- a/drivers/clk/qcom/apss-ipq6018.c
> +++ b/drivers/clk/qcom/apss-ipq6018.c
> @@ -56,7 +56,7 @@ static struct clk_branch apcs_alias0_core_clk = {
>                         .parent_hws = (const struct clk_hw *[]){
>                                 &apcs_alias0_clk_src.clkr.hw },
>                         .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
> +                       .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> --
> 2.36.1
>


-- 
With best wishes
Dmitry

[PATCH 4/6] clk: qcom: apss-ipq6018: add MODULE_ALIAS

[Robert Marko]

Add MODULE_ALIAS so that driver will be autoloaded if built as a module.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/clk/qcom/apss-ipq6018.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
index f2f502e2d5a4..963c69f2c0c2 100644
--- a/drivers/clk/qcom/apss-ipq6018.c
+++ b/drivers/clk/qcom/apss-ipq6018.c
@@ -101,5 +101,6 @@ static struct platform_driver apss_ipq6018_driver = {
 
 module_platform_driver(apss_ipq6018_driver);
 
+MODULE_ALIAS("platform:qcom,apss-ipq6018-clk");
 MODULE_DESCRIPTION("QCOM APSS IPQ 6018 CLK Driver");
 MODULE_LICENSE("GPL v2");
-- 
2.36.1

Re: [PATCH 4/6] clk: qcom: apss-ipq6018: add MODULE_ALIAS

[Krzysztof Kozlowski]

On 11/07/2022 12:47, Robert Marko wrote:
> Add MODULE_ALIAS so that driver will be autoloaded if built as a module.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/clk/qcom/apss-ipq6018.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> index f2f502e2d5a4..963c69f2c0c2 100644
> --- a/drivers/clk/qcom/apss-ipq6018.c
> +++ b/drivers/clk/qcom/apss-ipq6018.c
> @@ -101,5 +101,6 @@ static struct platform_driver apss_ipq6018_driver = {
>  
>  module_platform_driver(apss_ipq6018_driver);
>  
> +MODULE_ALIAS("platform:qcom,apss-ipq6018-clk");

That's not correct alias (no commas) and usually alias is not needed at
all. If you need one, please explain why it is needed. Module
autoloading works fine without aliases...

Best regards,
Krzysztof

Re: [PATCH 4/6] clk: qcom: apss-ipq6018: add MODULE_ALIAS

[Robert Marko]

On Mon, 11 Jul 2022 at 13:05, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/07/2022 12:47, Robert Marko wrote:
> > Add MODULE_ALIAS so that driver will be autoloaded if built as a module.
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> >  drivers/clk/qcom/apss-ipq6018.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> > index f2f502e2d5a4..963c69f2c0c2 100644
> > --- a/drivers/clk/qcom/apss-ipq6018.c
> > +++ b/drivers/clk/qcom/apss-ipq6018.c
> > @@ -101,5 +101,6 @@ static struct platform_driver apss_ipq6018_driver = {
> >
> >  module_platform_driver(apss_ipq6018_driver);
> >
> > +MODULE_ALIAS("platform:qcom,apss-ipq6018-clk");
>
> That's not correct alias (no commas) and usually alias is not needed at
> all. If you need one, please explain why it is needed. Module
> autoloading works fine without aliases...

Hi Krzysztof,
alias is required here as the driver does not use a DT compatible but
is registered
by the APCS driver, if built as a module, it won't get autoloaded
without an alias.

I can only fix up the driver name here and in APCS first to have an
alias without commas.

Regards,
Robert
>
> Best regards,
> Krzysztof

Re: [PATCH 4/6] clk: qcom: apss-ipq6018: add MODULE_ALIAS

[Krzysztof Kozlowski]

On 11/07/2022 13:46, Robert Marko wrote:
> On Mon, 11 Jul 2022 at 13:05, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/07/2022 12:47, Robert Marko wrote:
>>> Add MODULE_ALIAS so that driver will be autoloaded if built as a module.
>>>
>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>> ---
>>>  drivers/clk/qcom/apss-ipq6018.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
>>> index f2f502e2d5a4..963c69f2c0c2 100644
>>> --- a/drivers/clk/qcom/apss-ipq6018.c
>>> +++ b/drivers/clk/qcom/apss-ipq6018.c
>>> @@ -101,5 +101,6 @@ static struct platform_driver apss_ipq6018_driver = {
>>>
>>>  module_platform_driver(apss_ipq6018_driver);
>>>
>>> +MODULE_ALIAS("platform:qcom,apss-ipq6018-clk");
>>
>> That's not correct alias (no commas) and usually alias is not needed at
>> all. If you need one, please explain why it is needed. Module
>> autoloading works fine without aliases...
> 
> Hi Krzysztof,
> alias is required here as the driver does not use a DT compatible but
> is registered
> by the APCS driver, if built as a module, it won't get autoloaded
> without an alias.

Instead you need device ID table. Aliases are not a workaround for
missing core driver elements.

> 
> I can only fix up the driver name here and in APCS first to have an
> alias without commas.

I see that the comma is used in driver name, so this is an independent
issue. Maybe change it to '-' in separate commit?


Best regards,
Krzysztof

Re: [PATCH 4/6] clk: qcom: apss-ipq6018: add MODULE_ALIAS

[Robert Marko]

On Mon, 11 Jul 2022 at 14:02, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/07/2022 13:46, Robert Marko wrote:
> > On Mon, 11 Jul 2022 at 13:05, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/07/2022 12:47, Robert Marko wrote:
> >>> Add MODULE_ALIAS so that driver will be autoloaded if built as a module.
> >>>
> >>> Signed-off-by: Robert Marko <robimarko@gmail.com>
> >>> ---
> >>>  drivers/clk/qcom/apss-ipq6018.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> >>> index f2f502e2d5a4..963c69f2c0c2 100644
> >>> --- a/drivers/clk/qcom/apss-ipq6018.c
> >>> +++ b/drivers/clk/qcom/apss-ipq6018.c
> >>> @@ -101,5 +101,6 @@ static struct platform_driver apss_ipq6018_driver = {
> >>>
> >>>  module_platform_driver(apss_ipq6018_driver);
> >>>
> >>> +MODULE_ALIAS("platform:qcom,apss-ipq6018-clk");
> >>
> >> That's not correct alias (no commas) and usually alias is not needed at
> >> all. If you need one, please explain why it is needed. Module
> >> autoloading works fine without aliases...
> >
> > Hi Krzysztof,
> > alias is required here as the driver does not use a DT compatible but
> > is registered
> > by the APCS driver, if built as a module, it won't get autoloaded
> > without an alias.
>
> Instead you need device ID table. Aliases are not a workaround for
> missing core driver elements.

Thanks for pointing this out, it looks like a proper solution for this.
I will drop this patch and fix up autoloading after this series gets merged
as APCS also requires a fixup, especially since the name in the platform
table is limited to 20 characters and the current name does not fit.
>
> >
> > I can only fix up the driver name here and in APCS first to have an
> > alias without commas.
>
> I see that the comma is used in driver name, so this is an independent
> issue. Maybe change it to '-' in separate commit?

Like with the previous point, I will drop this patch and fix it after
this series gets merged.

Regards,
Robert
>
>
> Best regards,
> Krzysztof

[PATCH 5/6] dt-bindings: clock: qcom,a53pll: add IPQ8074 compatible

[Robert Marko]

Add IPQ8074 compatible to A53 PLL bindings.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 Documentation/devicetree/bindings/clock/qcom,a53pll.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
index fbd758470b88..76830816982e 100644
--- a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     enum:
       - qcom,ipq6018-a53pll
+      - qcom,ipq8074-a53pll
       - qcom,msm8916-a53pll
       - qcom,msm8939-a53pll
 
-- 
2.36.1

Re: [PATCH 5/6] dt-bindings: clock: qcom,a53pll: add IPQ8074 compatible

[Krzysztof Kozlowski]

On 11/07/2022 12:47, Robert Marko wrote:
> Add IPQ8074 compatible to A53 PLL bindings.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH 6/6] clk: qcom: apss-ipq-pll: add support for IPQ8074

[Robert Marko]

Add support for IPQ8074 since it uses the same PLL setup, however it does
not require the Alpha PLL to be reconfigured.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/clk/qcom/apss-ipq-pll.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index bef7899ad0d6..acfb3ec4f142 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -55,6 +55,7 @@ static const struct regmap_config ipq_pll_regmap_config = {
 static int apss_ipq_pll_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
 	struct regmap *regmap;
 	void __iomem *base;
 	int ret;
@@ -67,7 +68,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
-	clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
+	if (of_device_is_compatible(node, "qcom,ipq6018-a53pll"))
+		clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
 
 	ret = devm_clk_register_regmap(dev, &ipq_pll.clkr);
 	if (ret)
@@ -79,6 +81,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
 
 static const struct of_device_id apss_ipq_pll_match_table[] = {
 	{ .compatible = "qcom,ipq6018-a53pll" },
+	{ .compatible = "qcom,ipq8074-a53pll" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
-- 
2.36.1

Re: [PATCH 6/6] clk: qcom: apss-ipq-pll: add support for IPQ8074

[Krzysztof Kozlowski]

On 11/07/2022 12:47, Robert Marko wrote:
> Add support for IPQ8074 since it uses the same PLL setup, however it does
> not require the Alpha PLL to be reconfigured.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/clk/qcom/apss-ipq-pll.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> index bef7899ad0d6..acfb3ec4f142 100644
> --- a/drivers/clk/qcom/apss-ipq-pll.c
> +++ b/drivers/clk/qcom/apss-ipq-pll.c
> @@ -55,6 +55,7 @@ static const struct regmap_config ipq_pll_regmap_config = {
>  static int apss_ipq_pll_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
>  	struct regmap *regmap;
>  	void __iomem *base;
>  	int ret;
> @@ -67,7 +68,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>  	if (IS_ERR(regmap))
>  		return PTR_ERR(regmap);
>  
> -	clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
> +	if (of_device_is_compatible(node, "qcom,ipq6018-a53pll"))

Use match data instead with quirks. Better not to encode compatibles all
through the code.

Best regards,
Krzysztof

Re: [PATCH 6/6] clk: qcom: apss-ipq-pll: add support for IPQ8074

[Dmitry Baryshkov]

On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
>
> Add support for IPQ8074 since it uses the same PLL setup, however it does
> not require the Alpha PLL to be reconfigured.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/clk/qcom/apss-ipq-pll.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> index bef7899ad0d6..acfb3ec4f142 100644
> --- a/drivers/clk/qcom/apss-ipq-pll.c
> +++ b/drivers/clk/qcom/apss-ipq-pll.c
> @@ -55,6 +55,7 @@ static const struct regmap_config ipq_pll_regmap_config = {
>  static int apss_ipq_pll_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> +       struct device_node *node = dev->of_node;
>         struct regmap *regmap;
>         void __iomem *base;
>         int ret;
> @@ -67,7 +68,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>         if (IS_ERR(regmap))
>                 return PTR_ERR(regmap);
>
> -       clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
> +       if (of_device_is_compatible(node, "qcom,ipq6018-a53pll"))
> +               clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);

I'd suggest having the 8074 config here too. It seems logical to me to
make sure that the pll is configured correctly.

>
>         ret = devm_clk_register_regmap(dev, &ipq_pll.clkr);
>         if (ret)
> @@ -79,6 +81,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>
>  static const struct of_device_id apss_ipq_pll_match_table[] = {
>         { .compatible = "qcom,ipq6018-a53pll" },
> +       { .compatible = "qcom,ipq8074-a53pll" },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
> --
> 2.36.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 6/6] clk: qcom: apss-ipq-pll: add support for IPQ8074

[Robert Marko]

On Mon, 11 Jul 2022 at 14:51, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
> >
> > Add support for IPQ8074 since it uses the same PLL setup, however it does
> > not require the Alpha PLL to be reconfigured.
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> >  drivers/clk/qcom/apss-ipq-pll.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> > index bef7899ad0d6..acfb3ec4f142 100644
> > --- a/drivers/clk/qcom/apss-ipq-pll.c
> > +++ b/drivers/clk/qcom/apss-ipq-pll.c
> > @@ -55,6 +55,7 @@ static const struct regmap_config ipq_pll_regmap_config = {
> >  static int apss_ipq_pll_probe(struct platform_device *pdev)
> >  {
> >         struct device *dev = &pdev->dev;
> > +       struct device_node *node = dev->of_node;
> >         struct regmap *regmap;
> >         void __iomem *base;
> >         int ret;
> > @@ -67,7 +68,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
> >         if (IS_ERR(regmap))
> >                 return PTR_ERR(regmap);
> >
> > -       clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
> > +       if (of_device_is_compatible(node, "qcom,ipq6018-a53pll"))
> > +               clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
>
> I'd suggest having the 8074 config here too. It seems logical to me to
> make sure that the pll is configured correctly.

Hi,

I have reworked the driver to use match data so it can be easily provided,
However, I dont have it as the downstream QCA kernel does not
reconfigure the PLL, unlike IPQ6018.
I can probably read the registers from a running board and provide that?

Regards,
Robert
>
> >
> >         ret = devm_clk_register_regmap(dev, &ipq_pll.clkr);
> >         if (ret)
> > @@ -79,6 +81,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
> >
> >  static const struct of_device_id apss_ipq_pll_match_table[] = {
> >         { .compatible = "qcom,ipq6018-a53pll" },
> > +       { .compatible = "qcom,ipq8074-a53pll" },
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
> > --
> > 2.36.1
> >
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH 6/6] clk: qcom: apss-ipq-pll: add support for IPQ8074

[Dmitry Baryshkov]

On Mon, 11 Jul 2022 at 15:55, Robert Marko <robimarko@gmail.com> wrote:
>
> On Mon, 11 Jul 2022 at 14:51, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
> > >
> > > Add support for IPQ8074 since it uses the same PLL setup, however it does
> > > not require the Alpha PLL to be reconfigured.
> > >
> > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > ---
> > >  drivers/clk/qcom/apss-ipq-pll.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> > > index bef7899ad0d6..acfb3ec4f142 100644
> > > --- a/drivers/clk/qcom/apss-ipq-pll.c
> > > +++ b/drivers/clk/qcom/apss-ipq-pll.c
> > > @@ -55,6 +55,7 @@ static const struct regmap_config ipq_pll_regmap_config = {
> > >  static int apss_ipq_pll_probe(struct platform_device *pdev)
> > >  {
> > >         struct device *dev = &pdev->dev;
> > > +       struct device_node *node = dev->of_node;
> > >         struct regmap *regmap;
> > >         void __iomem *base;
> > >         int ret;
> > > @@ -67,7 +68,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
> > >         if (IS_ERR(regmap))
> > >                 return PTR_ERR(regmap);
> > >
> > > -       clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
> > > +       if (of_device_is_compatible(node, "qcom,ipq6018-a53pll"))
> > > +               clk_alpha_pll_configure(&ipq_pll, regmap, &ipq_pll_config);
> >
> > I'd suggest having the 8074 config here too. It seems logical to me to
> > make sure that the pll is configured correctly.
>
> Hi,
>
> I have reworked the driver to use match data so it can be easily provided,
> However, I dont have it as the downstream QCA kernel does not
> reconfigure the PLL, unlike IPQ6018.
> I can probably read the registers from a running board and provide that?

Yes, please try that.

>
> Regards,
> Robert
> >
> > >
> > >         ret = devm_clk_register_regmap(dev, &ipq_pll.clkr);
> > >         if (ret)
> > > @@ -79,6 +81,7 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
> > >
> > >  static const struct of_device_id apss_ipq_pll_match_table[] = {
> > >         { .compatible = "qcom,ipq6018-a53pll" },
> > > +       { .compatible = "qcom,ipq8074-a53pll" },
> > >         { }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, apss_ipq_pll_match_table);
> > > --
> > > 2.36.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

Re: [PATCH 1/6] clk: qcom: clk-rcg2: add rcg2 mux ops

[Dmitry Baryshkov]

On Mon, 11 Jul 2022 at 14:22, Robert Marko <robimarko@gmail.com> wrote:
>
> From: Christian Marangi <ansuelsmth@gmail.com>
>
> An RCG may act as a mux that switch between 2 parents.
> This is the case on IPQ6018 and IPQ8074 where the APCS core clk that feeds
> the CPU cluster clock just switches between XO and the PLL that feeds it.
>
> Add the required ops to add support for this special configuration and use
> the generic mux function to determine the rate.
>
> This way we dont have to keep a essentially dummy frequency table to use
> RCG2 as a mux.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Robert Marko <robimarko@gmail.com>

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

> ---
>  drivers/clk/qcom/clk-rcg.h  | 1 +
>  drivers/clk/qcom/clk-rcg2.c | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> index 012e745794fd..01581f4d2c39 100644
> --- a/drivers/clk/qcom/clk-rcg.h
> +++ b/drivers/clk/qcom/clk-rcg.h
> @@ -167,6 +167,7 @@ struct clk_rcg2_gfx3d {
>
>  extern const struct clk_ops clk_rcg2_ops;
>  extern const struct clk_ops clk_rcg2_floor_ops;
> +extern const struct clk_ops clk_rcg2_mux_closest_ops;
>  extern const struct clk_ops clk_edp_pixel_ops;
>  extern const struct clk_ops clk_byte_ops;
>  extern const struct clk_ops clk_byte2_ops;
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 28019edd2a50..609c10f8d0d9 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -509,6 +509,13 @@ const struct clk_ops clk_rcg2_floor_ops = {
>  };
>  EXPORT_SYMBOL_GPL(clk_rcg2_floor_ops);
>
> +const struct clk_ops clk_rcg2_mux_closest_ops = {
> +       .determine_rate = __clk_mux_determine_rate_closest,
> +       .get_parent = clk_rcg2_get_parent,
> +       .set_parent = clk_rcg2_set_parent,
> +};
> +EXPORT_SYMBOL_GPL(clk_rcg2_mux_closest_ops);
> +
>  struct frac_entry {
>         int num;
>         int den;
> --
> 2.36.1
>


-- 
With best wishes
Dmitry