diff mbox series

[V9,2/7] clk: imx: add scu clock common part

Message ID 1543934041-12572-3-git-send-email-aisheng.dong@nxp.com (mailing list archive)
State New, archived
Headers show
Series clk: imx: add imx8qxp clock support | expand

Commit Message

Dong Aisheng Dec. 4, 2018, 2:39 p.m. UTC 
Add SCU clock common part which will be used by client clock drivers.
SCU clocks are totally different from the legacy clocks (No much
legacy things can be reused), it's using a firmware interface now based
on SCU protocol. So a new configuration option CONFIG_MXC_CLK_SCU is added.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/clk/imx/Kconfig   |   4 +
 drivers/clk/imx/Makefile  |   3 +
 drivers/clk/imx/clk-scu.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/clk-scu.h |  21 ++++
 4 files changed, 293 insertions(+)
 create mode 100644 drivers/clk/imx/clk-scu.c
 create mode 100644 drivers/clk/imx/clk-scu.h

Comments

Stephen Boyd Dec. 10, 2018, 9:56 p.m. UTC | #1 
Quoting Aisheng DONG (2018-12-04 06:39:25)
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> index 43a3ecc..63e7b01 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -3,3 +3,7 @@
>  config MXC_CLK
>         bool
>         depends on ARCH_MXC
> +
> +config MXC_CLK_SCU

Is there any reason to make this a hidden option instead of making it a
selectable option? It can still depend on ARCH_MXC and ARM64, but
otherwise it should be compilable as long as CONFIG_IMX_SCU is defined
(this should also be a config we depend on here).

> +       bool
> +       depends on ARCH_MXC && ARM64
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index f850424..4abed37 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_CLK) += \
>         clk-pllv4.o \
>         clk-sccg-pll.o
>  
> +obj-$(CONFIG_MXC_CLK_SCU) += \
> +       clk-scu.o
> +
>  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
>  obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
>  obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> new file mode 100644
> index 0000000..ec8a471
> --- /dev/null
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 NXP
> + *   Dong Aisheng <aisheng.dong@nxp.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +#include "clk-scu.h"
> +
> +struct imx_sc_ipc *ccm_ipc_handle;

Why does this need to be a global? Can it be in each clk_scu instance
instead?

> +
> diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h
> new file mode 100644
> index 0000000..09f381b
> --- /dev/null
> +++ b/drivers/clk/imx/clk-scu.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2018 NXP
> + *   Dong Aisheng <aisheng.dong@nxp.com>
> + */
> +
> +#ifndef __IMX_CLK_SCU_H
> +#define __IMX_CLK_SCU_H
> +
> +#include <linux/firmware/imx/sci.h>
> +
> +extern struct imx_sc_ipc *ccm_ipc_handle;
> +
> +static inline int imx_clk_scu_init(void)
> +{
> +       return imx_scu_get_handle(&ccm_ipc_handle);

And then this can be implemented in the C driver so that ccm_ipc_handle
doesn't need to be exported into the header file?
Dong Aisheng Dec. 11, 2018, 3:35 a.m. UTC | #2 
> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Quoting Aisheng DONG (2018-12-04 06:39:25)
> > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > 43a3ecc..63e7b01 100644
> > --- a/drivers/clk/imx/Kconfig
> > +++ b/drivers/clk/imx/Kconfig
> > @@ -3,3 +3,7 @@
> >  config MXC_CLK
> >         bool
> >         depends on ARCH_MXC
> > +
> > +config MXC_CLK_SCU
> 
> Is there any reason to make this a hidden option instead of making it a
> selectable option? It can still depend on ARCH_MXC and ARM64, but otherwise
> it should be compilable as long as CONFIG_IMX_SCU is defined (this should
> also be a config we depend on here).
> 

This is mostly following the exist using that CLK is selected by SoC config option.
https://patchwork.kernel.org/patch/10677309/

As CLK usually is required for platform to run well, so we did not make it selectable.

> > +       bool
> > +       depends on ARCH_MXC && ARM64
> > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > f850424..4abed37 100644
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_CLK) += \
> >         clk-pllv4.o \
> >         clk-sccg-pll.o
> >
> > +obj-$(CONFIG_MXC_CLK_SCU) += \
> > +       clk-scu.o
> > +
> >  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> >  obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> >  obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o diff --git
> > a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c new file mode
> > 100644 index 0000000..ec8a471
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-scu.c
> > @@ -0,0 +1,265 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018 NXP
> > + *   Dong Aisheng <aisheng.dong@nxp.com>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +
> > +#include "clk-scu.h"
> > +
> > +struct imx_sc_ipc *ccm_ipc_handle;
> 
> Why does this need to be a global? Can it be in each clk_scu instance instead?
> 

No, no need to be in each clk_scu instance.
There's only one handler.

> > +
> > diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h new
> > file mode 100644 index 0000000..09f381b
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-scu.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2018 NXP
> > + *   Dong Aisheng <aisheng.dong@nxp.com>
> > + */
> > +
> > +#ifndef __IMX_CLK_SCU_H
> > +#define __IMX_CLK_SCU_H
> > +
> > +#include <linux/firmware/imx/sci.h>
> > +
> > +extern struct imx_sc_ipc *ccm_ipc_handle;
> > +
> > +static inline int imx_clk_scu_init(void) {
> > +       return imx_scu_get_handle(&ccm_ipc_handle);
> 
> And then this can be implemented in the C driver so that ccm_ipc_handle
> doesn't need to be exported into the header file?

Looks like a good suggestion.
I will take this as there's no other users of ccm_ipc_handle now, so no need in headfile.

Thanks

Regards
Dong Aisheng
Lothar Waßmann Dec. 17, 2018, 10:27 a.m. UTC | #3 
Hi,

On Tue, 11 Dec 2018 03:35:48 +0000 Aisheng Dong wrote:
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Quoting Aisheng DONG (2018-12-04 06:39:25)  
> > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > > 43a3ecc..63e7b01 100644
> > > --- a/drivers/clk/imx/Kconfig
> > > +++ b/drivers/clk/imx/Kconfig
> > > @@ -3,3 +3,7 @@
> > >  config MXC_CLK
> > >         bool
> > >         depends on ARCH_MXC
> > > +
> > > +config MXC_CLK_SCU  
> > 
> > Is there any reason to make this a hidden option instead of making it a
> > selectable option? It can still depend on ARCH_MXC and ARM64, but otherwise
> > it should be compilable as long as CONFIG_IMX_SCU is defined (this should
> > also be a config we depend on here).
> >   
> 
> This is mostly following the exist using that CLK is selected by SoC config option.
> https://patchwork.kernel.org/patch/10677309/
> 
> As CLK usually is required for platform to run well, so we did not make it selectable.
> 
> > > +       bool
> > > +       depends on ARCH_MXC && ARM64
> > > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > > f850424..4abed37 100644
> > > --- a/drivers/clk/imx/Makefile
> > > +++ b/drivers/clk/imx/Makefile
> > > @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_CLK) += \
> > >         clk-pllv4.o \
> > >         clk-sccg-pll.o
> > >
> > > +obj-$(CONFIG_MXC_CLK_SCU) += \
> > > +       clk-scu.o
> > > +
> > >  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> > >  obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> > >  obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o diff --git
> > > a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c new file mode
> > > 100644 index 0000000..ec8a471
> > > --- /dev/null
> > > +++ b/drivers/clk/imx/clk-scu.c
> > > @@ -0,0 +1,265 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2018 NXP
> > > + *   Dong Aisheng <aisheng.dong@nxp.com>
> > > + */
> > > +
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/err.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#include "clk-scu.h"
> > > +
> > > +struct imx_sc_ipc *ccm_ipc_handle;  
> > 
> > Why does this need to be a global? Can it be in each clk_scu instance instead?
> >   
> 
> No, no need to be in each clk_scu instance.
> There's only one handler.
> 
Shouldn't it be 'static'?


Lothar Waßmann
Dong Aisheng Dec. 17, 2018, 10:30 a.m. UTC | #4 
[...]
> > > > +#include "clk-scu.h"
> > > > +
> > > > +struct imx_sc_ipc *ccm_ipc_handle;
> > >
> > > Why does this need to be a global? Can it be in each clk_scu instance
> instead?
> > >
> >
> > No, no need to be in each clk_scu instance.
> > There's only one handler.
> >
> Shouldn't it be 'static'?
> 

This has been fixed in CLK tree.

Regards
Dong Aisheng

> 
> Lothar Waßmann
diff mbox series

Patch

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index 43a3ecc..63e7b01 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -3,3 +3,7 @@ 
 config MXC_CLK
 	bool
 	depends on ARCH_MXC
+
+config MXC_CLK_SCU
+	bool
+	depends on ARCH_MXC && ARM64
diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index f850424..4abed37 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -20,6 +20,9 @@  obj-$(CONFIG_MXC_CLK) += \
 	clk-pllv4.o \
 	clk-sccg-pll.o
 
+obj-$(CONFIG_MXC_CLK_SCU) += \
+	clk-scu.o
+
 obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
 obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
 obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
new file mode 100644
index 0000000..ec8a471
--- /dev/null
+++ b/drivers/clk/imx/clk-scu.c
@@ -0,0 +1,265 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP
+ *   Dong Aisheng <aisheng.dong@nxp.com>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#include "clk-scu.h"
+
+struct imx_sc_ipc *ccm_ipc_handle;
+
+/*
+ * struct clk_scu - Description of one SCU clock
+ * @hw: the common clk_hw
+ * @rsrc_id: resource ID of this SCU clock
+ * @clk_type: type of this clock resource
+ */
+struct clk_scu {
+	struct clk_hw hw;
+	u16 rsrc_id;
+	u8 clk_type;
+};
+
+/*
+ * struct imx_sc_msg_req_set_clock_rate - clock set rate protocol
+ * @hdr: SCU protocol header
+ * @rate: rate to set
+ * @resource: clock resource to set rate
+ * @clk: clk type of this resource
+ *
+ * This structure describes the SCU protocol of clock rate set
+ */
+struct imx_sc_msg_req_set_clock_rate {
+	struct imx_sc_rpc_msg hdr;
+	__le32 rate;
+	__le16 resource;
+	u8 clk;
+} __packed;
+
+struct req_get_clock_rate {
+	__le16 resource;
+	u8 clk;
+} __packed;
+
+struct resp_get_clock_rate {
+	__le32 rate;
+};
+
+/*
+ * struct imx_sc_msg_get_clock_rate - clock get rate protocol
+ * @hdr: SCU protocol header
+ * @req: get rate request protocol
+ * @resp: get rate response protocol
+ *
+ * This structure describes the SCU protocol of clock rate get
+ */
+struct imx_sc_msg_get_clock_rate {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct req_get_clock_rate req;
+		struct resp_get_clock_rate resp;
+	} data;
+};
+
+/*
+ * struct imx_sc_msg_req_clock_enable - clock gate protocol
+ * @hdr: SCU protocol header
+ * @resource: clock resource to gate
+ * @clk: clk type of this resource
+ * @enable: whether gate off the clock
+ * @autog: HW auto gate enable
+ *
+ * This structure describes the SCU protocol of clock gate
+ */
+struct imx_sc_msg_req_clock_enable {
+	struct imx_sc_rpc_msg hdr;
+	__le16 resource;
+	u8 clk;
+	u8 enable;
+	u8 autog;
+} __packed;
+
+static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
+{
+	return container_of(hw, struct clk_scu, hw);
+}
+
+/*
+ * clk_scu_recalc_rate - Get clock rate for a SCU clock
+ * @hw: clock to get rate for
+ * @parent_rate: parent rate provided by common clock framework, not used
+ *
+ * Gets the current clock rate of a SCU clock. Returns the current
+ * clock rate, or zero in failure.
+ */
+static unsigned long clk_scu_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct clk_scu *clk = to_clk_scu(hw);
+	struct imx_sc_msg_get_clock_rate msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	int ret;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_PM;
+	hdr->func = IMX_SC_PM_FUNC_GET_CLOCK_RATE;
+	hdr->size = 2;
+
+	msg.data.req.resource = cpu_to_le16(clk->rsrc_id);
+	msg.data.req.clk = clk->clk_type;
+
+	ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true);
+	if (ret) {
+		pr_err("%s: failed to get clock rate %d\n",
+		       clk_hw_get_name(hw), ret);
+		return 0;
+	}
+
+	return le32_to_cpu(msg.data.resp.rate);
+}
+
+/*
+ * clk_scu_round_rate - Round clock rate for a SCU clock
+ * @hw: clock to round rate for
+ * @rate: rate to round
+ * @parent_rate: parent rate provided by common clock framework, not used
+ *
+ * Returns the current clock rate, or zero in failure.
+ */
+static long clk_scu_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	/*
+	 * Assume we support all the requested rate and let the SCU firmware
+	 * to handle the left work
+	 */
+	return rate;
+}
+
+/*
+ * clk_scu_set_rate - Set rate for a SCU clock
+ * @hw: clock to change rate for
+ * @rate: target rate for the clock
+ * @parent_rate: rate of the clock parent, not used for SCU clocks
+ *
+ * Sets a clock frequency for a SCU clock. Returns the SCU
+ * protocol status.
+ */
+static int clk_scu_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct clk_scu *clk = to_clk_scu(hw);
+	struct imx_sc_msg_req_set_clock_rate msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_PM;
+	hdr->func = IMX_SC_PM_FUNC_SET_CLOCK_RATE;
+	hdr->size = 3;
+
+	msg.rate = cpu_to_le32(rate);
+	msg.resource = cpu_to_le16(clk->rsrc_id);
+	msg.clk = clk->clk_type;
+
+	return imx_scu_call_rpc(ccm_ipc_handle, &msg, true);
+}
+
+static int sc_pm_clock_enable(struct imx_sc_ipc *ipc, u16 resource,
+			      u8 clk, bool enable, bool autog)
+{
+	struct imx_sc_msg_req_clock_enable msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_PM;
+	hdr->func = IMX_SC_PM_FUNC_CLOCK_ENABLE;
+	hdr->size = 3;
+
+	msg.resource = cpu_to_le16(resource);
+	msg.clk = clk;
+	msg.enable = enable;
+	msg.autog = autog;
+
+	return imx_scu_call_rpc(ccm_ipc_handle, &msg, true);
+}
+
+/*
+ * clk_scu_prepare - Enable a SCU clock
+ * @hw: clock to enable
+ *
+ * Enable the clock at the DSC slice level
+ */
+static int clk_scu_prepare(struct clk_hw *hw)
+{
+	struct clk_scu *clk = to_clk_scu(hw);
+
+	return sc_pm_clock_enable(ccm_ipc_handle, clk->rsrc_id,
+				  clk->clk_type, true, false);
+}
+
+/*
+ * clk_scu_unprepare - Disable a SCU clock
+ * @hw: clock to enable
+ *
+ * Disable the clock at the DSC slice level
+ */
+static void clk_scu_unprepare(struct clk_hw *hw)
+{
+	struct clk_scu *clk = to_clk_scu(hw);
+	int ret;
+
+	ret = sc_pm_clock_enable(ccm_ipc_handle, clk->rsrc_id,
+				 clk->clk_type, false, false);
+	if (ret)
+		pr_warn("%s: clk unprepare failed %d\n", clk_hw_get_name(hw),
+			ret);
+}
+
+static const struct clk_ops clk_scu_ops = {
+	.recalc_rate = clk_scu_recalc_rate,
+	.round_rate = clk_scu_round_rate,
+	.set_rate = clk_scu_set_rate,
+	.prepare = clk_scu_prepare,
+	.unprepare = clk_scu_unprepare,
+};
+
+struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type)
+{
+	struct clk_init_data init;
+	struct clk_scu *clk;
+	struct clk_hw *hw;
+	int ret;
+
+	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	clk->rsrc_id = rsrc_id;
+	clk->clk_type = clk_type;
+
+	init.name = name;
+	init.ops = &clk_scu_ops;
+	init.num_parents = 0;
+	/*
+	 * Note on MX8, the clocks are tightly coupled with power domain
+	 * that once the power domain is off, the clock status may be
+	 * lost. So we make it NOCACHE to let user to retrieve the real
+	 * clock status from HW instead of using the possible invalid
+	 * cached rate.
+	 */
+	init.flags = CLK_GET_RATE_NOCACHE;
+	clk->hw.init = &init;
+
+	hw = &clk->hw;
+	ret = clk_hw_register(NULL, hw);
+	if (ret) {
+		kfree(clk);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h
new file mode 100644
index 0000000..09f381b
--- /dev/null
+++ b/drivers/clk/imx/clk-scu.h
@@ -0,0 +1,21 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2018 NXP
+ *   Dong Aisheng <aisheng.dong@nxp.com>
+ */
+
+#ifndef __IMX_CLK_SCU_H
+#define __IMX_CLK_SCU_H
+
+#include <linux/firmware/imx/sci.h>
+
+extern struct imx_sc_ipc *ccm_ipc_handle;
+
+static inline int imx_clk_scu_init(void)
+{
+	return imx_scu_get_handle(&ccm_ipc_handle);
+}
+
+struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type);
+
+#endif