| Message ID | 1542809234-539-1-git-send-email-aisheng.dong@nxp.com |
|---|---|
| Headers | show |
| Series | clk: imx: add imx8qxp clock support | expand |
Gently Ping... > -----Original Message----- > From: Aisheng DONG > Sent: Wednesday, November 21, 2018 10:12 PM > To: linux-clk@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org; sboyd@kernel.org; > mturquette@baylibre.com; shawnguo@kernel.org; Fabio Estevam > <fabio.estevam@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; > kernel@pengutronix.de; Aisheng DONG <aisheng.dong@nxp.com> > Subject: [PATCH V8 0/7] clk: imx: add imx8qxp clock support > > This patch series adds i.MX8QXP clock support which is based on the clock > service provided by SCU firmware. > > Note: It depends on SCU driver which has already been merged by Shawn. > So this patch series could go through Shawn's tree as well. > > ChangeLog: > v7->v8: > * add more kernel doc for lpcg and scu clock structures > * remove one unneccessry reg checking > v6->v7: > * use struct_size() > * remove MODULE_ macros > * add more kernel docs/code comments > * other small improvements > * update reviewed-by tags > * add the missing PATCH 1 in V6 > v5->v6: > * simply the driver a lot by re-orgnizing the driver into a few clock types: > scu clock (merge scu divider/gate/mux) and scu gpr lock which accessing is > through SCU protocol and LPCG clock which is directly accessible by CPU. > * LPCG clock is separate from SCU clock, gpr clock is still not used > and will be added later. > * remove old year license as the code is totally rewritten > * scu mux support will be added later as it's also still not used. > v4->v5: > Address all Stephen and Sascha's review comments, see details in each patch > v3->v4: > * scu headfile path update > * no functionality change > v2->v3: > * structures/enums name update with imx_sc prefix > * no functionality change > v1->v2: > * structure and enums name update > * api usage update due to api change > * no functionality change > > Dong Aisheng (7): > clk: imx: add configuration option for mmio clks > clk: imx: add scu clock common part > dt-bindings: clock: imx8qxp: add SCU clock IDs > clk: imx: add imx8qxp clk driver > dt-bindings: clock: add imx8qxp lpcg clock binding > clk: imx: add lpcg clock support > clk: imx: add imx8qxp lpcg driver > > .../devicetree/bindings/clock/imx8qxp-lpcg.txt | 51 ++++ > arch/arm/mach-imx/Kconfig | 11 + > drivers/clk/Kconfig | 1 + > drivers/clk/imx/Kconfig | 9 + > drivers/clk/imx/Makefile | 8 +- > drivers/clk/imx/clk-imx8qxp-lpcg.c | 216 > +++++++++++++++ > drivers/clk/imx/clk-imx8qxp-lpcg.h | 102 ++++++++ > drivers/clk/imx/clk-imx8qxp.c | 153 +++++++++++ > drivers/clk/imx/clk-lpcg-scu.c | 113 ++++++++ > drivers/clk/imx/clk-scu.c | 267 > +++++++++++++++++++ > drivers/clk/imx/clk-scu.h | 24 ++ > include/dt-bindings/clock/imx8qxp-clock.h | 289 > +++++++++++++++++++++ > 12 files changed, 1243 insertions(+), 1 deletion(-) create mode 100644 > Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > create mode 100644 drivers/clk/imx/Kconfig create mode 100644 > drivers/clk/imx/clk-imx8qxp-lpcg.c > create mode 100644 drivers/clk/imx/clk-imx8qxp-lpcg.h > create mode 100644 drivers/clk/imx/clk-imx8qxp.c create mode 100644 > drivers/clk/imx/clk-lpcg-scu.c create mode 100644 drivers/clk/imx/clk-scu.c > create mode 100644 drivers/clk/imx/clk-scu.h create mode 100644 > include/dt-bindings/clock/imx8qxp-clock.h > > -- > 2.7.4
Quoting Aisheng DONG (2018-11-21 06:12:23) > 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). So a new configuration option > CONFIG_MXC_CLK_SCU is added. Why are they different? Can you add more details on what has changed here? I hope to see something like "it's a firmware interface now". > > 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> > diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c > new file mode 100644 > index 0000000..5f9c1f6 > --- /dev/null > +++ b/drivers/clk/imx/clk-scu.c > @@ -0,0 +1,267 @@ > +// 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 > + * > + * This structure describes one SCU clock Drop this sentence, it duplicates the first line of this kernel-doc. > + */ > +struct clk_scu { > + struct clk_hw hw; > + u32 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; > + u32 rate; > + u16 resource; These should be __le32 and __le16 presumably because the firmware interface we're talking to is little endian? > + u8 clk; > +} __packed; > + > +struct req_get_clock_rate { > + u16 resource; Same. > + u8 clk; > +} __packed; > + > +struct resp_get_clock_rate { > + u32 rate; Same. > +}; > + > +/* > + * 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; > +} __packed; Does this need __packed? struct imx_sc_rpc_msg is naturally aligned so I think we don't need this. > + > +/* > + * 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; > + u16 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; I really hope msg isn't DMAed to the firmware, because the response is in there! > + 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 = 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 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 = rate; > + msg.resource = clk->rsrc_id; > + msg.clk = clk->clk_type; > + > + return imx_scu_call_rpc(ccm_ipc_handle, &msg, true); This function returns normal error codes? Or some firmware special error code? Please make sure we return normal linux errno values from this driver. > +} > + > +static int sc_pm_clock_enable(struct imx_sc_ipc *ipc, u32 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 = 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) Is this supposed to return EPROBE_DEFER or something? Who calls this? > +{ > + return imx_scu_get_handle(&ccm_ipc_handle); > +} > + > +struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type); > +
Quoting Aisheng DONG (2018-11-28 09:18:47) > Gently Ping... > FYI, when you ping the thread, it floats to the end of the queue for me, and then I don't review it for some time. Sorry, this is how it mostly works. The bigger topics come to the front queue and then I try to get to those sooner and let the stuff churning as newest in the queue become lower priority because "someone else is doing something". I do sweeps over the new stuff every day or two to pick out the easy one liners and simple things because they may be critical, etc.
Quoting Aisheng DONG (2018-11-21 06:12:38) > diff --git a/drivers/clk/imx/clk-lpcg-scu.c b/drivers/clk/imx/clk-lpcg-scu.c > new file mode 100644 > index 0000000..15b7d94 > --- /dev/null > +++ b/drivers/clk/imx/clk-lpcg-scu.c > @@ -0,0 +1,113 @@ > +// 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/io.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > + > +static DEFINE_SPINLOCK(imx_lpcg_scu_lock); > + > +#define CLK_GATE_SCU_LPCG_MASK 0x3 > +#define CLK_GATE_SCU_LPCG_HW_SEL BIT(0) > +#define CLK_GATE_SCU_LPCG_SW_SEL BIT(1) > + > +/* > + * struct clk_lpcg_scu - Description of LPCG clock > + * > + * @hw: clk_hw of this LPCG > + * @reg: register of this LPCG clock > + * @bit_idx: bit index of this LPCG clock > + * @hw_gate: HW auto gate enable > + * > + * This structure describes one LPCG clock > + */ > +struct clk_lpcg_scu { > + struct clk_hw hw; > + void __iomem *reg; > + u8 bit_idx; > + bool hw_gate; > +}; > + > +#define to_clk_lpcg_scu(_hw) container_of(_hw, struct clk_lpcg_scu, hw) > + > +static int clk_lpcg_scu_enable(struct clk_hw *hw) > +{ > + struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw); > + unsigned long flags; > + u32 reg; > + > + spin_lock_irqsave(&imx_lpcg_scu_lock, flags); > + > + reg = readl(clk->reg); Use readl_relaxed() here? > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx); > + if (clk->hw_gate) > + reg |= (CLK_GATE_SCU_LPCG_HW_SEL | > + CLK_GATE_SCU_LPCG_SW_SEL) << clk->bit_idx; > + else > + reg |= (CLK_GATE_SCU_LPCG_SW_SEL << clk->bit_idx); Write this as: u32 val; val = CLK_GATE_SCU_LPCG_SW_SEL; if (...) val |= CLK_GATE_SCU_LPCG_HW_SEL; reg |= val << clk->bit_idx; > + writel(reg, clk->reg); This can stay "stronger", so no writel_relaxed(). > + > + spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags); > + > + return 0; > +} > + > +static void clk_lpcg_scu_disable(struct clk_hw *hw) > +{ > + struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw); > + unsigned long flags; > + u32 reg; > + > + spin_lock_irqsave(&imx_lpcg_scu_lock, flags); > + > + reg = readl(clk->reg); readl_relaxed()? > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
Quoting Aisheng DONG (2018-11-21 06:12:16) > This patch series adds i.MX8QXP clock support which is based > on the clock service provided by SCU firmware. > > Note: It depends on SCU driver which has already been merged by Shawn. > So this patch series could go through Shawn's tree as well. Any patches I didn't make comments on look good to go. Only a few comments to address and otherwise it looks good.
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Tuesday, December 4, 2018 2:34 AM > To: linux-clk@vger.kernel.org; Aisheng DONG <aisheng.dong@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com; > shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; Aisheng DONG > <aisheng.dong@nxp.com> > Subject: Re: [PATCH V8 2/7] clk: imx: add scu clock common part > > Quoting Aisheng DONG (2018-11-21 06:12:23) > > 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). So a new configuration option > > CONFIG_MXC_CLK_SCU is added. > > Why are they different? Can you add more details on what has changed here? I > hope to see something like "it's a firmware interface now". > Good suggestion. I will add it in commit message. > > > > 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> diff --git > > a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c new file mode > > 100644 index 0000000..5f9c1f6 > > --- /dev/null > > +++ b/drivers/clk/imx/clk-scu.c > > @@ -0,0 +1,267 @@ > > +// 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 > > + * > > + * This structure describes one SCU clock > > Drop this sentence, it duplicates the first line of this kernel-doc. > Got it > > + */ > > +struct clk_scu { > > + struct clk_hw hw; > > + u32 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; > > + u32 rate; > > + u16 resource; > > These should be __le32 and __le16 presumably because the firmware interface > we're talking to is little endian? > Yes, I think we can do it. > > + u8 clk; > > +} __packed; > > + > > +struct req_get_clock_rate { > > + u16 resource; > > Same. > > > + u8 clk; > > +} __packed; > > + > > +struct resp_get_clock_rate { > > + u32 rate; > > Same. > > > +}; > > + > > +/* > > + * 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; > > +} __packed; > > Does this need __packed? struct imx_sc_rpc_msg is naturally aligned so I think > we don't need this. > I think this is more safe way. But struct imx_sc_rpc_msg is unlikely to change. So I guess it's safe to remove it. > > + > > +/* > > + * 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; > > + u16 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; > > I really hope msg isn't DMAed to the firmware, because the response is > in there! No, not DMAed. It's sent over MU messages. > > > + 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 = 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 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 = rate; > > + msg.resource = clk->rsrc_id; > > + msg.clk = clk->clk_type; > > + > > + return imx_scu_call_rpc(ccm_ipc_handle, &msg, true); > > This function returns normal error codes? Or some firmware special error > code? Please make sure we return normal linux errno values from this > driver. The error code returned by imx_scu_call_rpc is internally converted to Linux Errno. So the caller don't have to care about it. > > > +} > > + > > +static int sc_pm_clock_enable(struct imx_sc_ipc *ipc, u32 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 = 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) > > Is this supposed to return EPROBE_DEFER or something? Who calls this? > The SCU clock client driver will call it. E.g. MX8QXP or MX8QM clock drivers. It might return EPROBE_DEFER or normal Linux errno by imx_scu_get_handle(). Regards Dong Aisheng > > +{ > > + return imx_scu_get_handle(&ccm_ipc_handle); > > +} > > + > > +struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type); > > +
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Tuesday, December 4, 2018 3:45 AM > To: linux-clk@vger.kernel.org; Aisheng DONG <aisheng.dong@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com; > shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; Aisheng DONG > <aisheng.dong@nxp.com> > Subject: Re: [PATCH V8 6/7] clk: imx: add lpcg clock support > > Quoting Aisheng DONG (2018-11-21 06:12:38) > > diff --git a/drivers/clk/imx/clk-lpcg-scu.c > > b/drivers/clk/imx/clk-lpcg-scu.c new file mode 100644 index > > 0000000..15b7d94 > > --- /dev/null > > +++ b/drivers/clk/imx/clk-lpcg-scu.c > > @@ -0,0 +1,113 @@ > > +// 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/io.h> > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > + > > +static DEFINE_SPINLOCK(imx_lpcg_scu_lock); > > + > > +#define CLK_GATE_SCU_LPCG_MASK 0x3 > > +#define CLK_GATE_SCU_LPCG_HW_SEL BIT(0) > > +#define CLK_GATE_SCU_LPCG_SW_SEL BIT(1) > > + > > +/* > > + * struct clk_lpcg_scu - Description of LPCG clock > > + * > > + * @hw: clk_hw of this LPCG > > + * @reg: register of this LPCG clock > > + * @bit_idx: bit index of this LPCG clock > > + * @hw_gate: HW auto gate enable > > + * > > + * This structure describes one LPCG clock */ struct clk_lpcg_scu { > > + struct clk_hw hw; > > + void __iomem *reg; > > + u8 bit_idx; > > + bool hw_gate; > > +}; > > + > > +#define to_clk_lpcg_scu(_hw) container_of(_hw, struct clk_lpcg_scu, > > +hw) > > + > > +static int clk_lpcg_scu_enable(struct clk_hw *hw) { > > + struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw); > > + unsigned long flags; > > + u32 reg; > > + > > + spin_lock_irqsave(&imx_lpcg_scu_lock, flags); > > + > > + reg = readl(clk->reg); > > Use readl_relaxed() here? > Yes, we can do it. > > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx); > > + if (clk->hw_gate) > > + reg |= (CLK_GATE_SCU_LPCG_HW_SEL | > > + CLK_GATE_SCU_LPCG_SW_SEL) << clk->bit_idx; > > + else > > + reg |= (CLK_GATE_SCU_LPCG_SW_SEL << clk->bit_idx); > > Write this as: > > u32 val; > > val = CLK_GATE_SCU_LPCG_SW_SEL; > if (...) > val |= CLK_GATE_SCU_LPCG_HW_SEL; > reg |= val << clk->bit_idx; > Yes, a smarter way. > > + writel(reg, clk->reg); > > This can stay "stronger", so no writel_relaxed(). > Got it. > > + > > + spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags); > > + > > + return 0; > > +} > > + > > +static void clk_lpcg_scu_disable(struct clk_hw *hw) { > > + struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw); > > + unsigned long flags; > > + u32 reg; > > + > > + spin_lock_irqsave(&imx_lpcg_scu_lock, flags); > > + > > + reg = readl(clk->reg); > > readl_relaxed()? Got it. Regards Dong Aisheng > > > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Tuesday, December 4, 2018 3:46 AM > To: linux-clk@vger.kernel.org; Aisheng DONG <aisheng.dong@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com; > shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; Aisheng DONG > <aisheng.dong@nxp.com> > Subject: Re: [PATCH V8 0/7] clk: imx: add imx8qxp clock support > > Quoting Aisheng DONG (2018-11-21 06:12:16) > > This patch series adds i.MX8QXP clock support which is based on the > > clock service provided by SCU firmware. > > > > Note: It depends on SCU driver which has already been merged by Shawn. > > So this patch series could go through Shawn's tree as well. > > Any patches I didn't make comments on look good to go. Only a few > comments to address and otherwise it looks good. Great, I will address all your comments and re-send soon. Thanks for the carefully review. Regards Dong Aisheng
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Tuesday, December 4, 2018 2:39 AM > To: linux-clk@vger.kernel.org; Aisheng DONG <aisheng.dong@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com; > shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de > Subject: RE: [PATCH V8 0/7] clk: imx: add imx8qxp clock support > > Quoting Aisheng DONG (2018-11-28 09:18:47) > > Gently Ping... > > > > FYI, when you ping the thread, it floats to the end of the queue for me, and > then I don't review it for some time. Sorry, this is how it mostly works. The > bigger topics come to the front queue and then I try to get to those sooner and > let the stuff churning as newest in the queue become lower priority because > "someone else is doing something". I do sweeps over the new stuff every day > or two to pick out the easy one liners and simple things because they may be > critical, etc. I fully understood. Thanks for your effort and help. Regards Dong Aisheng
Quoting Aisheng DONG (2018-12-03 17:26:01) > > > +}; > > > + > > > +/* > > > + * 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; > > > +} __packed; > > > > Does this need __packed? struct imx_sc_rpc_msg is naturally aligned so I think > > we don't need this. > > > > I think this is more safe way. But struct imx_sc_rpc_msg is unlikely to change. > So I guess it's safe to remove it. If the struct is naturally aligned then I would say remove __packed and only add the marking if it is really needed. It makes more work now, but is cleaner to do it this way so we don't have to think about it later and we may be able to generate slightly better code if the compiler doesn't have to worry about packed either.
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Quoting Aisheng DONG (2018-12-03 17:26:01) > > > > +}; > > > > + > > > > +/* > > > > + * 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; > > > > +} __packed; > > > > > > Does this need __packed? struct imx_sc_rpc_msg is naturally aligned > > > so I think we don't need this. > > > > > > > I think this is more safe way. But struct imx_sc_rpc_msg is unlikely to change. > > So I guess it's safe to remove it. > > If the struct is naturally aligned then I would say remove __packed and only > add the marking if it is really needed. It makes more work now, but is cleaner > to do it this way so we don't have to think about it later and we may be able > to generate slightly better code if the compiler doesn't have to worry about > packed either. Thanks for the detailed explanation and it helps a further understanding. I've removed it and sent a updated patch series with all your suggestions addressed. https://patchwork.kernel.org/cover/10712007/ Please have a look at it. Regards Dong Aisheng