diff mbox series

[v6,10/18] clk: imx: add hw API imx8m_anatop_get_clk_hw

Message ID 20241222170534.3621453-11-dario.binacchi@amarulasolutions.com
State New
Headers
Series Support spread spectrum clocking for i.MX8N PLLs |

Commit Message

Dario Binacchi Dec. 22, 2024, 5:04 p.m. UTC 
Get the hw of a clock registered by the anatop module. This function is
preparatory for future developments.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

(no changes since v5)

Changes in v5:
- Consider CONFIG_CLK_IMX8M{M,N,P,Q}_MODULE to fix compilation errors

Changes in v4:
- New

 drivers/clk/imx/clk.c | 28 ++++++++++++++++++++++++++++
 drivers/clk/imx/clk.h |  7 +++++++
 2 files changed, 35 insertions(+)

Comments

Peng Fan Dec. 24, 2024, 3:22 a.m. UTC | #1 
On Sun, Dec 22, 2024 at 06:04:25PM +0100, Dario Binacchi wrote:
>Get the hw of a clock registered by the anatop module. This function is
>preparatory for future developments.
>
>Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
>---
>
>(no changes since v5)
>
>Changes in v5:
>- Consider CONFIG_CLK_IMX8M{M,N,P,Q}_MODULE to fix compilation errors
>
>Changes in v4:
>- New
>
> drivers/clk/imx/clk.c | 28 ++++++++++++++++++++++++++++
> drivers/clk/imx/clk.h |  7 +++++++
> 2 files changed, 35 insertions(+)
>
>diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
>index df83bd939492..9a21f233e105 100644
>--- a/drivers/clk/imx/clk.c
>+++ b/drivers/clk/imx/clk.c
>@@ -128,6 +128,34 @@ struct clk_hw *imx_get_clk_hw_by_name(struct device_node *np, const char *name)
> }
> EXPORT_SYMBOL_GPL(imx_get_clk_hw_by_name);
> 
>+#if defined(CONFIG_CLK_IMX8MM) || defined(CONFIG_CLK_IMX8MM_MODULE) || \
>+	defined(CONFIG_CLK_IMX8MN) || defined(CONFIG_CLK_IMX8MN_MODULE) || \
>+	defined(CONFIG_CLK_IMX8MP) || defined(CONFIG_CLK_IMX8MP_MODULE) || \
>+	defined(CONFIG_CLK_IMX8MQ) || defined(CONFIG_CLK_IMX8MQ_MODULE)

drop the guarding macros, then this could be reused by i.MX9.

>+struct clk_hw *imx8m_anatop_get_clk_hw(int id)

how about change to
struct clk_hw *imx_anatop_get_clk_hw(int id, struct device_node *np)?

>+{
>+#if defined(CONFIG_CLK_IMX8MQ) || defined(CONFIG_CLK_IMX8MQ_MODULE)
>+	const char *compatible = "fsl,imx8mq-anatop";
>+#else
>+	const char *compatible = "fsl,imx8mm-anatop";
>+#endif
>+	struct device_node *np;
>+	struct of_phandle_args args;
>+	struct clk_hw *hw;
>+
>+	np = of_find_compatible_node(NULL, NULL, compatible);

Then no need to find the compatible for every function call.

>+	args.np = np;
>+	args.args_count = 1;
>+	args.args[0] = id;
>+	of_node_put(np);
>+

Thanks,
Peng

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Dario Binacchi Dec. 24, 2024, 12:18 p.m. UTC | #2 
Hi Peng,

On Tue, Dec 24, 2024 at 3:17 AM Peng Fan <peng.fan@oss.nxp.com> wrote:
>
> On Sun, Dec 22, 2024 at 06:04:25PM +0100, Dario Binacchi wrote:
> >Get the hw of a clock registered by the anatop module. This function is
> >preparatory for future developments.
> >
> >Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> >---
> >
> >(no changes since v5)
> >
> >Changes in v5:
> >- Consider CONFIG_CLK_IMX8M{M,N,P,Q}_MODULE to fix compilation errors
> >
> >Changes in v4:
> >- New
> >
> > drivers/clk/imx/clk.c | 28 ++++++++++++++++++++++++++++
> > drivers/clk/imx/clk.h |  7 +++++++
> > 2 files changed, 35 insertions(+)
> >
> >diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> >index df83bd939492..9a21f233e105 100644
> >--- a/drivers/clk/imx/clk.c
> >+++ b/drivers/clk/imx/clk.c
> >@@ -128,6 +128,34 @@ struct clk_hw *imx_get_clk_hw_by_name(struct device_node *np, const char *name)
> > }
> > EXPORT_SYMBOL_GPL(imx_get_clk_hw_by_name);
> >
> >+#if defined(CONFIG_CLK_IMX8MM) || defined(CONFIG_CLK_IMX8MM_MODULE) || \
> >+      defined(CONFIG_CLK_IMX8MN) || defined(CONFIG_CLK_IMX8MN_MODULE) || \
> >+      defined(CONFIG_CLK_IMX8MP) || defined(CONFIG_CLK_IMX8MP_MODULE) || \
> >+      defined(CONFIG_CLK_IMX8MQ) || defined(CONFIG_CLK_IMX8MQ_MODULE)
>
> drop the guarding macros, then this could be reused by i.MX9.
>
> >+struct clk_hw *imx8m_anatop_get_clk_hw(int id)
>
> how about change to
> struct clk_hw *imx_anatop_get_clk_hw(int id, struct device_node *np)?

I designed this function so that the caller, i.e., the CCM driver,
would no longer need to
reference the anatop compatible in any way, but I agree with you that
it would be better
 to add the np parameter. Do you think it would then make sense to add
a phandle to the
 anatop node in the clk node?

clk: clock-controller@30380000 {
    compatible = "fsl,imx8mn-ccm";
    ...
    fsl,anatop = <&anatop>
}

So that we can call
anatop_np = of_parse_phandle(np, "fsl,anatop", 0);
instead of
anatop_np = of_find_compatible_node(NULL, NULL, "fsl,imx8mn-anatop");
This would require an additional patch to
Documentation/devicetree/bindings/clock/imx8m-clock.yaml,
but it would make the CCM driver code more generic.

What do you think? I’m waiting for your response before sending version 7.

Thanks and regards,
Dario





>
> >+{
> >+#if defined(CONFIG_CLK_IMX8MQ) || defined(CONFIG_CLK_IMX8MQ_MODULE)
> >+      const char *compatible = "fsl,imx8mq-anatop";
> >+#else
> >+      const char *compatible = "fsl,imx8mm-anatop";
> >+#endif
> >+      struct device_node *np;
> >+      struct of_phandle_args args;
> >+      struct clk_hw *hw;
> >+
> >+      np = of_find_compatible_node(NULL, NULL, compatible);
>
> Then no need to find the compatible for every function call.
>
> >+      args.np = np;
> >+      args.args_count = 1;
> >+      args.args[0] = id;
> >+      of_node_put(np);
> >+
>
> Thanks,
> Peng
Peng Fan Dec. 25, 2024, 12:59 a.m. UTC | #3 
> Subject: Re: [PATCH v6 10/18] clk: imx: add hw API
> imx8m_anatop_get_clk_hw
> 
> Hi Peng,
> 
> On Tue, Dec 24, 2024 at 3:17 AM Peng Fan <peng.fan@oss.nxp.com>
> wrote:
> >
> > On Sun, Dec 22, 2024 at 06:04:25PM +0100, Dario Binacchi wrote:
> > >Get the hw of a clock registered by the anatop module. This function
> > >is preparatory for future developments.
> > >
> > >Signed-off-by: Dario Binacchi
> <dario.binacchi@amarulasolutions.com>
> > >
> > >---
> > >
> > >(no changes since v5)
> > >
> > >Changes in v5:
> > >- Consider CONFIG_CLK_IMX8M{M,N,P,Q}_MODULE to fix
> compilation errors
> > >
> > >Changes in v4:
> > >- New
> > >
> > > drivers/clk/imx/clk.c | 28 ++++++++++++++++++++++++++++
> > > drivers/clk/imx/clk.h |  7 +++++++
> > > 2 files changed, 35 insertions(+)
> > >
> > >diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index
> > >df83bd939492..9a21f233e105 100644
> > >--- a/drivers/clk/imx/clk.c
> > >+++ b/drivers/clk/imx/clk.c
> > >@@ -128,6 +128,34 @@ struct clk_hw
> *imx_get_clk_hw_by_name(struct
> > >device_node *np, const char *name)  }
> > >EXPORT_SYMBOL_GPL(imx_get_clk_hw_by_name);
> > >
> > >+#if defined(CONFIG_CLK_IMX8MM) ||
> defined(CONFIG_CLK_IMX8MM_MODULE) || \
> > >+      defined(CONFIG_CLK_IMX8MN) ||
> defined(CONFIG_CLK_IMX8MN_MODULE) || \
> > >+      defined(CONFIG_CLK_IMX8MP) ||
> defined(CONFIG_CLK_IMX8MP_MODULE) || \
> > >+      defined(CONFIG_CLK_IMX8MQ) ||
> > >+defined(CONFIG_CLK_IMX8MQ_MODULE)
> >
> > drop the guarding macros, then this could be reused by i.MX9.
> >
> > >+struct clk_hw *imx8m_anatop_get_clk_hw(int id)
> >
> > how about change to
> > struct clk_hw *imx_anatop_get_clk_hw(int id, struct device_node
> *np)?
> 
> I designed this function so that the caller, i.e., the CCM driver, would no
> longer need to reference the anatop compatible in any way, but I agree
> with you that it would be better  to add the np parameter. Do you
> think it would then make sense to add a phandle to the  anatop node
> in the clk node?
> 
> clk: clock-controller@30380000 {
>     compatible = "fsl,imx8mn-ccm";
>     ...
>     fsl,anatop = <&anatop>
> }

This needs binding update, so needs DT maintainers to approve.
I am fine with it.

Regards,
Peng

> 
> So that we can call
> anatop_np = of_parse_phandle(np, "fsl,anatop", 0); instead of
> anatop_np = of_find_compatible_node(NULL, NULL, "fsl,imx8mn-
> anatop"); This would require an additional patch to
> Documentation/devicetree/bindings/clock/imx8m-clock.yaml,
> but it would make the CCM driver code more generic.
> 
> What do you think? I’m waiting for your response before sending
> version 7.
> 
> Thanks and regards,
> Dario
> 
> 
> 
> 
> 
> >
> > >+{
> > >+#if defined(CONFIG_CLK_IMX8MQ) ||
> defined(CONFIG_CLK_IMX8MQ_MODULE)
> > >+      const char *compatible = "fsl,imx8mq-anatop"; #else
> > >+      const char *compatible = "fsl,imx8mm-anatop"; #endif
> > >+      struct device_node *np;
> > >+      struct of_phandle_args args;
> > >+      struct clk_hw *hw;
> > >+
> > >+      np = of_find_compatible_node(NULL, NULL, compatible);
> >
> > Then no need to find the compatible for every function call.
> >
> > >+      args.np = np;
> > >+      args.args_count = 1;
> > >+      args.args[0] = id;
> > >+      of_node_put(np);
> > >+
> >
> > Thanks,
> > Peng
> 
> 
> 
> --
> 
> Dario Binacchi
> 
> Senior Embedded Linux Developer
> 
> dario.binacchi@amarulasolutions.com
> 
> __________________________________
> 
> 
> Amarula Solutions SRL
> 
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> 
> T. +39 042 243 5310
> info@amarulasolutions.com
> 
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
> com%7C4e8243a5b9f94a532c8d08dd24152922%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C0%7C638706395491848166%7CUnkn
> own%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMD
> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7
> C%7C%7C&sdata=0hMc9UwgFpa8VhOJvUVl3I3krgeFSzcB149zLRmHTx
> g%3D&reserved=0

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
diff mbox series

Patch

diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index df83bd939492..9a21f233e105 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -128,6 +128,34 @@  struct clk_hw *imx_get_clk_hw_by_name(struct device_node *np, const char *name)
 }
 EXPORT_SYMBOL_GPL(imx_get_clk_hw_by_name);
 
+#if defined(CONFIG_CLK_IMX8MM) || defined(CONFIG_CLK_IMX8MM_MODULE) || \
+	defined(CONFIG_CLK_IMX8MN) || defined(CONFIG_CLK_IMX8MN_MODULE) || \
+	defined(CONFIG_CLK_IMX8MP) || defined(CONFIG_CLK_IMX8MP_MODULE) || \
+	defined(CONFIG_CLK_IMX8MQ) || defined(CONFIG_CLK_IMX8MQ_MODULE)
+struct clk_hw *imx8m_anatop_get_clk_hw(int id)
+{
+#if defined(CONFIG_CLK_IMX8MQ) || defined(CONFIG_CLK_IMX8MQ_MODULE)
+	const char *compatible = "fsl,imx8mq-anatop";
+#else
+	const char *compatible = "fsl,imx8mm-anatop";
+#endif
+	struct device_node *np;
+	struct of_phandle_args args;
+	struct clk_hw *hw;
+
+	np = of_find_compatible_node(NULL, NULL, compatible);
+	args.np = np;
+	args.args_count = 1;
+	args.args[0] = id;
+	of_node_put(np);
+
+	hw = __clk_get_hw(of_clk_get_from_provider(&args));
+	pr_debug("%s: got clk: %s\n", __func__, clk_hw_get_name(hw));
+	return hw;
+}
+EXPORT_SYMBOL_GPL(imx8m_anatop_get_clk_hw);
+#endif
+
 /*
  * This fixups the register CCM_CSCMR1 write value.
  * The write/read/divider values of the aclk_podf field
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index aa5202f284f3..52055fda3058 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -487,4 +487,11 @@  struct clk_hw *imx_clk_gpr_mux(const char *name, const char *compatible,
 			       u32 reg, const char **parent_names,
 			       u8 num_parents, const u32 *mux_table, u32 mask);
 
+#if defined(CONFIG_CLK_IMX8MM) || defined(CONFIG_CLK_IMX8MM_MODULE) || \
+	defined(CONFIG_CLK_IMX8MN) || defined(CONFIG_CLK_IMX8MN_MODULE) || \
+	defined(CONFIG_CLK_IMX8MP) || defined(CONFIG_CLK_IMX8MP_MODULE) || \
+	defined(CONFIG_CLK_IMX8MQ) || defined(CONFIG_CLK_IMX8MQ_MODULE)
+struct clk_hw *imx8m_anatop_get_clk_hw(int id);
+#endif
+
 #endif