Message ID | 1539504194-28289-1-git-send-email-aisheng.dong@nxp.com |
---|---|
Headers | show |
Series | clk: imx: add imx8qxp clock support | expand |
On Sun, Oct 14, 2018 at 08:07:56AM +0000, A.s. Dong wrote: > +/* Write to the LPCG bits. */ > +static int clk_gate_scu_enable(struct clk_hw *hw) > +{ > + struct clk_gate_scu *gate = to_clk_gate_scu(hw); > + u32 reg; > + > + if (gate->reg) { > + reg = readl(gate->reg); > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << gate->bit_idx); > + if (gate->hw_gate) > + reg |= (CLK_GATE_SCU_LPCG_HW_SEL | > + CLK_GATE_SCU_LPCG_SW_SEL) << gate->bit_idx; > + else > + reg |= (CLK_GATE_SCU_LPCG_SW_SEL << gate->bit_idx); > + writel(reg, gate->reg); > + } These register manipulations look like they need locking. > + > + return 0; > +} > + > +struct clk_hw *clk_register_gate_scu(const char *name, const char *parent_name, > + unsigned long flags, u32 rsrc_id, > + u8 clk_type, void __iomem *reg, > + u8 bit_idx, bool hw_gate) > +{ > + struct clk_gate_scu *gate; > + struct clk_init_data init; > + struct clk_hw *hw; > + int ret; > + > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > + if (!gate) > + return ERR_PTR(-ENOMEM); > + > + gate->rsrc_id = rsrc_id; > + gate->clk_type = clk_type; > + if (reg) { > + gate->reg = ioremap((phys_addr_t)reg, SZ_64K); ioremap never takes a void __iomem * as argument. Given that you have to cast it here to another type and to void __iomem * when calling this function is a clear sign that the type of this variable is poorly chosen. > + if (!gate->reg) { > + kfree(gate); > + return ERR_PTR(-ENOMEM); > + } > + } > + > + gate->bit_idx = bit_idx; > + gate->hw_gate = hw_gate; > + > + init.name = name; > + init.ops = &clk_gate_scu_ops; > + init.flags = flags; > + init.parent_names = parent_name ? &parent_name : NULL; > + init.num_parents = parent_name ? 1 : 0; > + > + gate->hw.init = &init; > + > + hw = &gate->hw; > + ret = clk_hw_register(NULL, hw); > + if (ret) { > + iounmap(gate->reg); Is iounmap on a NULL pointer allowed? Otherwise the error path is wrong here. Sascha
> -----Original Message----- > From: Sascha Hauer [mailto:s.hauer@pengutronix.de] > Sent: Monday, October 15, 2018 3:32 PM > To: A.s. Dong <aisheng.dong@nxp.com> > Cc: linux-clk@vger.kernel.org; sboyd@kernel.org; mturquette@baylibre.com; > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam > <fabio.estevam@nxp.com>; shawnguo@kernel.org; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH V4 05/11] clk: imx: scu: add scu clock gate > > On Sun, Oct 14, 2018 at 08:07:56AM +0000, A.s. Dong wrote: > > +/* Write to the LPCG bits. */ > > +static int clk_gate_scu_enable(struct clk_hw *hw) { > > + struct clk_gate_scu *gate = to_clk_gate_scu(hw); > > + u32 reg; > > + > > + if (gate->reg) { > > + reg = readl(gate->reg); > > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << gate->bit_idx); > > + if (gate->hw_gate) > > + reg |= (CLK_GATE_SCU_LPCG_HW_SEL | > > + CLK_GATE_SCU_LPCG_SW_SEL) << gate->bit_idx; > > + else > > + reg |= (CLK_GATE_SCU_LPCG_SW_SEL << gate->bit_idx); > > + writel(reg, gate->reg); > > + } > > These register manipulations look like they need locking. > Unlike the legacy MX6&7 SoCs, each clock has a separate LPCG register. Do we still need locking? > > + > > + return 0; > > +} > > + > > +struct clk_hw *clk_register_gate_scu(const char *name, const char > *parent_name, > > + unsigned long flags, u32 rsrc_id, > > + u8 clk_type, void __iomem *reg, > > + u8 bit_idx, bool hw_gate) > > +{ > > + struct clk_gate_scu *gate; > > + struct clk_init_data init; > > + struct clk_hw *hw; > > + int ret; > > + > > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > > + if (!gate) > > + return ERR_PTR(-ENOMEM); > > + > > + gate->rsrc_id = rsrc_id; > > + gate->clk_type = clk_type; > > + if (reg) { > > + gate->reg = ioremap((phys_addr_t)reg, SZ_64K); > > ioremap never takes a void __iomem * as argument. Given that you have to > cast it here to another type and to void __iomem * when calling this function > is a clear sign that the type of this variable is poorly chosen. > Good catch. I missed it. Thanks for pointing it out. Looks like I should use struct phy_addr_t for it by default. > > + if (!gate->reg) { > > + kfree(gate); > > + return ERR_PTR(-ENOMEM); > > + } > > + } > > + > > + gate->bit_idx = bit_idx; > > + gate->hw_gate = hw_gate; > > + > > + init.name = name; > > + init.ops = &clk_gate_scu_ops; > > + init.flags = flags; > > + init.parent_names = parent_name ? &parent_name : NULL; > > + init.num_parents = parent_name ? 1 : 0; > > + > > + gate->hw.init = &init; > > + > > + hw = &gate->hw; > > + ret = clk_hw_register(NULL, hw); > > + if (ret) { > > + iounmap(gate->reg); > > Is iounmap on a NULL pointer allowed? Otherwise the error path is wrong > here. > If gate->reg is NULL, the execution seems can't reach here. Am I missing something? Regards Dong Aisheng > Sascha > > -- > Pengutronix e.K. | > | > Industrial Linux Solutions | > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7Ca8 > dc9d70c490491d116b08d6327050dc%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35%7C0%7C0%7C636751855312147344&sdata=RkcPJ0TNLnFP%2BBbTh > VrApN3Z%2B7MRR8%2FxJO1TSnqBv40%3D&reserved=0 | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
On Mon, Oct 15, 2018 at 09:17:14AM +0000, A.s. Dong wrote: > > -----Original Message----- > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de] > > Sent: Monday, October 15, 2018 3:32 PM > > To: A.s. Dong <aisheng.dong@nxp.com> > > Cc: linux-clk@vger.kernel.org; sboyd@kernel.org; mturquette@baylibre.com; > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam > > <fabio.estevam@nxp.com>; shawnguo@kernel.org; > > linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCH V4 05/11] clk: imx: scu: add scu clock gate > > > > On Sun, Oct 14, 2018 at 08:07:56AM +0000, A.s. Dong wrote: > > > +/* Write to the LPCG bits. */ > > > +static int clk_gate_scu_enable(struct clk_hw *hw) { > > > + struct clk_gate_scu *gate = to_clk_gate_scu(hw); > > > + u32 reg; > > > + > > > + if (gate->reg) { > > > + reg = readl(gate->reg); > > > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << gate->bit_idx); > > > + if (gate->hw_gate) > > > + reg |= (CLK_GATE_SCU_LPCG_HW_SEL | > > > + CLK_GATE_SCU_LPCG_SW_SEL) << gate->bit_idx; > > > + else > > > + reg |= (CLK_GATE_SCU_LPCG_SW_SEL << gate->bit_idx); > > > + writel(reg, gate->reg); > > > + } > > > > These register manipulations look like they need locking. > > > > Unlike the legacy MX6&7 SoCs, each clock has a separate LPCG register. > Do we still need locking? Let's take PWM_0_LPCG as an example: + clks[IMX8QXP_LSIO_PWM0_IPG_S_CLK] = imx_clk_gate_scu("pwm_0_ipg_s_clk", "pwm_0_div", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 0x10, 0); + clks[IMX8QXP_LSIO_PWM0_IPG_SLV_CLK] = imx_clk_gate_scu("pwm_0_ipg_slv_clk", "pwm_0_ipg_s_clk", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 0x14, 0); + clks[IMX8QXP_LSIO_PWM0_IPG_MSTR_CLK] = imx_clk_gate2_scu("pwm_0_ipg_mstr_clk", "lsio_bus_clk_root", (void __iomem *)(PWM_0_LPCG), 0x18, 0); + clks[IMX8QXP_LSIO_PWM0_HF_CLK] = imx_clk_gate_scu("pwm_0_hf_clk", "pwm_0_ipg_slv_clk", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 4, 0); + clks[IMX8QXP_LSIO_PWM0_CLK] = imx_clk_gate_scu("pwm_0_clk", "pwm_0_ipg_slv_clk", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 0, 0); This register is used in five different clocks. > > > > + > > > + return 0; > > > +} > > > + > > > +struct clk_hw *clk_register_gate_scu(const char *name, const char > > *parent_name, > > > + unsigned long flags, u32 rsrc_id, > > > + u8 clk_type, void __iomem *reg, > > > + u8 bit_idx, bool hw_gate) > > > +{ > > > + struct clk_gate_scu *gate; > > > + struct clk_init_data init; > > > + struct clk_hw *hw; > > > + int ret; > > > + > > > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > > > + if (!gate) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + gate->rsrc_id = rsrc_id; > > > + gate->clk_type = clk_type; > > > + if (reg) { > > > + gate->reg = ioremap((phys_addr_t)reg, SZ_64K); > > > > ioremap never takes a void __iomem * as argument. Given that you have to > > cast it here to another type and to void __iomem * when calling this function > > is a clear sign that the type of this variable is poorly chosen. > > > > Good catch. I missed it. > Thanks for pointing it out. > Looks like I should use struct phy_addr_t for it by default. > > > > + if (!gate->reg) { > > > + kfree(gate); > > > + return ERR_PTR(-ENOMEM); > > > + } > > > + } > > > + > > > + gate->bit_idx = bit_idx; > > > + gate->hw_gate = hw_gate; > > > + > > > + init.name = name; > > > + init.ops = &clk_gate_scu_ops; > > > + init.flags = flags; > > > + init.parent_names = parent_name ? &parent_name : NULL; > > > + init.num_parents = parent_name ? 1 : 0; > > > + > > > + gate->hw.init = &init; > > > + > > > + hw = &gate->hw; > > > + ret = clk_hw_register(NULL, hw); > > > + if (ret) { > > > + iounmap(gate->reg); > > > > Is iounmap on a NULL pointer allowed? Otherwise the error path is wrong > > here. > > > > If gate->reg is NULL, the execution seems can't reach here. > Am I missing something? Yes. gate->reg is only valid when the input parameter reg in non NULL. Sascha
> -----Original Message----- > From: Sascha Hauer [mailto:s.hauer@pengutronix.de] > Sent: Monday, October 15, 2018 5:54 PM > To: A.s. Dong <aisheng.dong@nxp.com> > Cc: linux-clk@vger.kernel.org; sboyd@kernel.org; mturquette@baylibre.com; > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam > <fabio.estevam@nxp.com>; shawnguo@kernel.org; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH V4 05/11] clk: imx: scu: add scu clock gate > > On Mon, Oct 15, 2018 at 09:17:14AM +0000, A.s. Dong wrote: > > > -----Original Message----- > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de] > > > Sent: Monday, October 15, 2018 3:32 PM > > > To: A.s. Dong <aisheng.dong@nxp.com> > > > Cc: linux-clk@vger.kernel.org; sboyd@kernel.org; > > > mturquette@baylibre.com; dl-linux-imx <linux-imx@nxp.com>; > > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>; > > > shawnguo@kernel.org; linux-arm-kernel@lists.infradead.org > > > Subject: Re: [PATCH V4 05/11] clk: imx: scu: add scu clock gate > > > > > > On Sun, Oct 14, 2018 at 08:07:56AM +0000, A.s. Dong wrote: > > > > +/* Write to the LPCG bits. */ > > > > +static int clk_gate_scu_enable(struct clk_hw *hw) { > > > > + struct clk_gate_scu *gate = to_clk_gate_scu(hw); > > > > + u32 reg; > > > > + > > > > + if (gate->reg) { > > > > + reg = readl(gate->reg); > > > > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << gate->bit_idx); > > > > + if (gate->hw_gate) > > > > + reg |= (CLK_GATE_SCU_LPCG_HW_SEL | > > > > + CLK_GATE_SCU_LPCG_SW_SEL) << gate->bit_idx; > > > > + else > > > > + reg |= (CLK_GATE_SCU_LPCG_SW_SEL << gate->bit_idx); > > > > + writel(reg, gate->reg); > > > > + } > > > > > > These register manipulations look like they need locking. > > > > > > > Unlike the legacy MX6&7 SoCs, each clock has a separate LPCG register. > > Do we still need locking? > > Let's take PWM_0_LPCG as an example: > > + clks[IMX8QXP_LSIO_PWM0_IPG_S_CLK] = > imx_clk_gate_scu("pwm_0_ipg_s_clk", "pwm_0_div", IMX_SC_R_PWM_0, > IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 0x10, 0); > + clks[IMX8QXP_LSIO_PWM0_IPG_SLV_CLK] = > imx_clk_gate_scu("pwm_0_ipg_slv_clk", "pwm_0_ipg_s_clk", > IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), > 0x14, 0); > + clks[IMX8QXP_LSIO_PWM0_IPG_MSTR_CLK] = > imx_clk_gate2_scu("pwm_0_ipg_mstr_clk", "lsio_bus_clk_root", (void > __iomem *)(PWM_0_LPCG), 0x18, 0); > + clks[IMX8QXP_LSIO_PWM0_HF_CLK] = > imx_clk_gate_scu("pwm_0_hf_clk", "pwm_0_ipg_slv_clk", IMX_SC_R_PWM_0, > IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 4, 0); > + clks[IMX8QXP_LSIO_PWM0_CLK] = > imx_clk_gate_scu("pwm_0_clk", "pwm_0_ipg_slv_clk", IMX_SC_R_PWM_0, > IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 0, 0); > > This register is used in five different clocks. > Good catch. BTW, it seems for the same clk group, we may still not need lock as the clock framework already defined the global enable/disable lock for the same group. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/clk.rst " Drivers don't need to manually protect resources shared between the operations of one group, regardless of whether those resources are shared by multiple clocks or not. However, access to resources that are shared between operations of the two groups needs to be protected by the drivers." Do you think it's okay to drop it? Regards Dong Aisheng > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +struct clk_hw *clk_register_gate_scu(const char *name, const char > > > *parent_name, > > > > + unsigned long flags, u32 rsrc_id, > > > > + u8 clk_type, void __iomem *reg, > > > > + u8 bit_idx, bool hw_gate) { > > > > + struct clk_gate_scu *gate; > > > > + struct clk_init_data init; > > > > + struct clk_hw *hw; > > > > + int ret; > > > > + > > > > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > > > > + if (!gate) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + gate->rsrc_id = rsrc_id; > > > > + gate->clk_type = clk_type; > > > > + if (reg) { > > > > + gate->reg = ioremap((phys_addr_t)reg, SZ_64K); > > > > > > ioremap never takes a void __iomem * as argument. Given that you > > > have to cast it here to another type and to void __iomem * when > > > calling this function is a clear sign that the type of this variable is poorly > chosen. > > > > > > > Good catch. I missed it. > > Thanks for pointing it out. > > Looks like I should use struct phy_addr_t for it by default. > > > > > > + if (!gate->reg) { > > > > + kfree(gate); > > > > + return ERR_PTR(-ENOMEM); > > > > + } > > > > + } > > > > + > > > > + gate->bit_idx = bit_idx; > > > > + gate->hw_gate = hw_gate; > > > > + > > > > + init.name = name; > > > > + init.ops = &clk_gate_scu_ops; > > > > + init.flags = flags; > > > > + init.parent_names = parent_name ? &parent_name : NULL; > > > > + init.num_parents = parent_name ? 1 : 0; > > > > + > > > > + gate->hw.init = &init; > > > > + > > > > + hw = &gate->hw; > > > > + ret = clk_hw_register(NULL, hw); > > > > + if (ret) { > > > > + iounmap(gate->reg); > > > > > > Is iounmap on a NULL pointer allowed? Otherwise the error path is > > > wrong here. > > > > > > > If gate->reg is NULL, the execution seems can't reach here. > > Am I missing something? > > Yes. gate->reg is only valid when the input parameter reg in non NULL. > > Sascha > > > -- > Pengutronix e.K. | > | > Industrial Linux Solutions | > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C11 > c8f62f729749de7b8708d63284183f%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636751940255777834&sdata=tsICGc0z9Rcfgu4YcKWy5 > hu8sFHXeRYSy3lP8CjtxO8%3D&reserved=0 | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
Quoting A.s. Dong (2018-10-15 08:30:45) > > -----Original Message----- > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de] > > Sent: Monday, October 15, 2018 5:54 PM > > To: A.s. Dong <aisheng.dong@nxp.com> > > Cc: linux-clk@vger.kernel.org; sboyd@kernel.org; mturquette@baylibre.com; > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam > > <fabio.estevam@nxp.com>; shawnguo@kernel.org; > > linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCH V4 05/11] clk: imx: scu: add scu clock gate > > > > On Mon, Oct 15, 2018 at 09:17:14AM +0000, A.s. Dong wrote: > > > > -----Original Message----- > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de] > > > > Sent: Monday, October 15, 2018 3:32 PM > > > > To: A.s. Dong <aisheng.dong@nxp.com> > > > > Cc: linux-clk@vger.kernel.org; sboyd@kernel.org; > > > > mturquette@baylibre.com; dl-linux-imx <linux-imx@nxp.com>; > > > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>; > > > > shawnguo@kernel.org; linux-arm-kernel@lists.infradead.org > > > > Subject: Re: [PATCH V4 05/11] clk: imx: scu: add scu clock gate > > > > > > > > On Sun, Oct 14, 2018 at 08:07:56AM +0000, A.s. Dong wrote: > > > > > +/* Write to the LPCG bits. */ > > > > > +static int clk_gate_scu_enable(struct clk_hw *hw) { > > > > > + struct clk_gate_scu *gate = to_clk_gate_scu(hw); > > > > > + u32 reg; > > > > > + > > > > > + if (gate->reg) { > > > > > + reg = readl(gate->reg); > > > > > + reg &= ~(CLK_GATE_SCU_LPCG_MASK << gate->bit_idx); > > > > > + if (gate->hw_gate) > > > > > + reg |= (CLK_GATE_SCU_LPCG_HW_SEL | > > > > > + CLK_GATE_SCU_LPCG_SW_SEL) << gate->bit_idx; > > > > > + else > > > > > + reg |= (CLK_GATE_SCU_LPCG_SW_SEL << gate->bit_idx); > > > > > + writel(reg, gate->reg); > > > > > + } > > > > > > > > These register manipulations look like they need locking. > > > > > > > > > > Unlike the legacy MX6&7 SoCs, each clock has a separate LPCG register. > > > Do we still need locking? > > > > Let's take PWM_0_LPCG as an example: > > > > + clks[IMX8QXP_LSIO_PWM0_IPG_S_CLK] = > > imx_clk_gate_scu("pwm_0_ipg_s_clk", "pwm_0_div", IMX_SC_R_PWM_0, > > IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 0x10, 0); > > + clks[IMX8QXP_LSIO_PWM0_IPG_SLV_CLK] = > > imx_clk_gate_scu("pwm_0_ipg_slv_clk", "pwm_0_ipg_s_clk", > > IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), > > 0x14, 0); > > + clks[IMX8QXP_LSIO_PWM0_IPG_MSTR_CLK] = > > imx_clk_gate2_scu("pwm_0_ipg_mstr_clk", "lsio_bus_clk_root", (void > > __iomem *)(PWM_0_LPCG), 0x18, 0); > > + clks[IMX8QXP_LSIO_PWM0_HF_CLK] = > > imx_clk_gate_scu("pwm_0_hf_clk", "pwm_0_ipg_slv_clk", IMX_SC_R_PWM_0, > > IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 4, 0); > > + clks[IMX8QXP_LSIO_PWM0_CLK] = > > imx_clk_gate_scu("pwm_0_clk", "pwm_0_ipg_slv_clk", IMX_SC_R_PWM_0, > > IMX_SC_PM_CLK_PER, (void __iomem *)(PWM_0_LPCG), 0, 0); > > > > This register is used in five different clocks. > > > > Good catch. > BTW, it seems for the same clk group, we may still not need lock > as the clock framework already defined the global enable/disable lock > for the same group. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/clk.rst > " Drivers don't need to manually protect resources shared between the operations > of one group, regardless of whether those resources are shared by multiple > clocks or not. However, access to resources that are shared between operations > of the two groups needs to be protected by the drivers." > > Do you think it's okay to drop it? > No it's not OK. We prefer that clk drivers don't assume the global locks in the clk framework are going to protect them from concurrent access to the same resource between different clks. Drivers can assume that a clk op won't be called in parallel for the same clk, but they shouldn't assume that everything is protected otherwise. If they did, we would have to go find all the drivers that make this assumption and then fix them when we eventually split the lock into smaller pieces. Long story short, if you have something shared (i.e. a register) and you plan to write to it and read from it for multiple clks, add a lock around it.
Quoting A.s. Dong (2018-10-14 01:07:49) > diff --git a/drivers/clk/imx/scu/clk-divider-scu.c b/drivers/clk/imx/scu/clk-divider-scu.c > new file mode 100644 > index 0000000..51cb816 > --- /dev/null > +++ b/drivers/clk/imx/scu/clk-divider-scu.c > @@ -0,0 +1,176 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > + * Copyright 2017~2018 NXP > + * Dong Aisheng <aisheng.dong@nxp.com> > + * > + */ > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/jiffies.h> Is this used? > +#include <linux/slab.h> > + > +#include "clk-scu.h" > + > +struct clk_divider_scu { > + struct clk_hw hw; Nitpick: No idea why this is spaced out from clk_hw. > + u32 rsrc_id; > + u8 clk_type; > +}; > + > +/* SCU Clock Protocol definitions */ > +struct imx_sc_msg_req_set_clock_rate { > + struct imx_sc_rpc_msg hdr; > + u32 rate; > + u16 resource; > + u8 clk; > +} __packed; > + > +struct imx_sc_msg_req_get_clock_rate { > + struct imx_sc_rpc_msg hdr; > + u16 resource; > + u8 clk; > +} __packed; > + > +struct imx_sc_msg_resp_get_clock_rate { > + struct imx_sc_rpc_msg hdr; > + u32 rate; > +} __packed; > + > + > +static inline struct clk_divider_scu *to_clk_divider_scu(struct clk_hw *hw) > +{ > + return container_of(hw, struct clk_divider_scu, hw); > +} > + > +/* > + * clk_divider_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_divider_scu_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct clk_divider_scu *div = to_clk_divider_scu(hw); > + struct imx_sc_msg_req_get_clock_rate msg; > + struct imx_sc_msg_resp_get_clock_rate *resp; > + struct imx_sc_rpc_msg *hdr = &msg.hdr; > + int ret; > + > + hdr->ver = IMX_SC_RPC_VERSION; > + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM; > + hdr->func = (uint8_t)IMX_SC_PM_FUNC_GET_CLOCK_RATE; > + hdr->size = 2; > + > + msg.resource = div->rsrc_id; > + msg.clk = div->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; > + } > + > + resp = (struct imx_sc_msg_resp_get_clock_rate *)&msg; Does the response get written to the same place that the message is written? If so, it would be better to combine the different structs into a union that's always large enough to handle this? For example, it looks like there are only 16 + 8 bytes for the get_clock_rate structure, but then the response is returning the rate in 32 bytes. When we cast that here we're now getting an extra 8 bytes of stack, aren't we? Combining the different structures into one bigger structure would alleviate this and avoid the need for casting. > + > + return resp->rate; > +} > + > +/* > + * clk_divider_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 > + * > + * Gets the current clock rate of a SCU clock. Returns the current > + * clock rate, or zero in failure. > + */ > +static long clk_divider_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_divider_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_divider_scu_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_divider_scu *div = to_clk_divider_scu(hw); > + struct imx_sc_msg_req_set_clock_rate msg; > + struct imx_sc_rpc_msg *hdr = &msg.hdr; > + int ret; > + > + hdr->ver = IMX_SC_RPC_VERSION; > + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM; > + hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_CLOCK_RATE; Are these casts necessary? > + hdr->size = 3; > + > + msg.rate = rate; > + msg.resource = div->rsrc_id; > + msg.clk = div->clk_type; > + > + ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true); > + if (ret) > + pr_err("%s: failed to set clock rate %ld : ret %d\n", > + clk_hw_get_name(hw), rate, ret); > + > + return 0; > +} > + > +static const struct clk_ops clk_divider_scu_ops = { > + .recalc_rate = clk_divider_scu_recalc_rate, > + .round_rate = clk_divider_scu_round_rate, > + .set_rate = clk_divider_scu_set_rate, > +}; > + > +struct clk_hw *imx_clk_register_divider_scu(const char *name, > + const char *parent_name, > + u32 rsrc_id, > + u8 clk_type) > +{ > + struct clk_divider_scu *div; > + struct clk_init_data init; > + struct clk_hw *hw; > + int ret; > + > + div = kzalloc(sizeof(*div), GFP_KERNEL); > + if (!div) > + return ERR_PTR(-ENOMEM); > + > + div->rsrc_id = rsrc_id; > + div->clk_type = clk_type; > + > + init.name = name; > + init.ops = &clk_divider_scu_ops; > + init.flags = CLK_GET_RATE_NOCACHE; Why nocache? Please have a good reason and add a comment indicating why. > + init.parent_names = parent_name ? &parent_name : NULL; > + init.num_parents = parent_name ? 1 : 0; > + div->hw.init = &init; > + > + hw = &div->hw; > + ret = clk_hw_register(NULL, hw); > + if (ret) { > + kfree(div); > + hw = ERR_PTR(ret); > + } > + > + return hw; > +}
Quoting A.s. Dong (2018-10-14 01:07:52) > +/* > + * clk_divider_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 > + * > + * Gets the current clock rate of a SCU clock. Returns the current > + * clock rate, or zero in failure. > + */ > +static unsigned long clk_divider_gpr_scu_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct clk_divider_gpr_scu *clk = to_clk_divider_gpr_scu(hw); > + u32 val; > + int ret; > + > + ret = imx_sc_misc_get_control(ccm_ipc_handle, clk->rsrc_id, > + clk->gpr_id, &val); > + if (ret) { > + pr_err("%s: failed to get clock rate %d\n", > + clk_hw_get_name(hw), ret); > + return 0; > + } > + > + return val ? parent_rate / 2 : parent_rate; I hope parent_rate can't be zero here. > +} > + > +/* > + * clk_divider_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 > + * > + * Round clock rate for a SCU clock according to parent rate > + */ > +static long clk_divider_gpr_scu_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *prate) > +{ > + if (rate < *prate) > + rate = *prate / 2; > + else > + rate = *prate; > + > + return rate; > +} > + > +/* > + * clk_divider_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 > + * > + * Sets a clock frequency for a SCU clock. Returns the SCU > + * protocol status. > + */ > +static int clk_divider_gpr_scu_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_divider_gpr_scu *clk = to_clk_divider_gpr_scu(hw); > + uint32_t val; > + > + val = (rate < parent_rate) ? 1 : 0; Nitpick: Write it out as val = 0; if (rate < parent_rate) val = 1; > + > + return imx_sc_misc_set_control(ccm_ipc_handle, clk->rsrc_id, > + clk->gpr_id, val); > +} > + > +static const struct clk_ops clk_divider_gpr_scu_ops = { > + .recalc_rate = clk_divider_gpr_scu_recalc_rate, > + .round_rate = clk_divider_gpr_scu_round_rate, > + .set_rate = clk_divider_gpr_scu_set_rate, > +}; > + > +struct clk_hw *imx_clk_divider_gpr_scu(const char *name, const char *parent_name, > + u32 rsrc_id, u8 gpr_id) > +{ > + struct clk_divider_gpr_scu *div; > + struct clk_init_data init; > + struct clk_hw *hw; > + int ret; > + > + div = kzalloc(sizeof(*div), GFP_KERNEL); > + if (!div) > + return ERR_PTR(-ENOMEM); > + > + div->rsrc_id = rsrc_id; > + div->gpr_id = gpr_id; > + > + init.name = name; > + init.ops = &clk_divider_gpr_scu_ops; > + init.flags = CLK_GET_RATE_NOCACHE; Same NOCACHE comment. > + init.parent_names = parent_name ? &parent_name : NULL; > + init.num_parents = parent_name ? 1 : 0; > + > + div->hw.init = &init; > + > + hw = &div->hw; > + ret = clk_hw_register(NULL, hw); > + if (ret) { > + kfree(div); > + hw = ERR_PTR(ret); > + } > + > + return hw; > +}
Quoting A.s. Dong (2018-10-14 01:08:06) > diff --git a/drivers/clk/imx/scu/clk-mux-gpr-scu.c b/drivers/clk/imx/scu/clk-mux-gpr-scu.c > new file mode 100644 > index 0000000..2ad7a80 > --- /dev/null > +++ b/drivers/clk/imx/scu/clk-mux-gpr-scu.c > @@ -0,0 +1,90 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > + * Copyright 2017~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 "clk-scu.h" > + > +struct clk_mux_gpr_scu { > + struct clk_hw hw; > + u32 rsrc_id; > + u8 gpr_id; > +}; > + > +#define to_clk_mux_gpr_scu(_hw) container_of(_hw, struct clk_mux_gpr_scu, hw) > + > +static u8 clk_mux_gpr_scu_get_parent(struct clk_hw *hw) > +{ > + struct clk_mux_gpr_scu *gpr_mux = to_clk_mux_gpr_scu(hw); > + u32 val = 0; > + int ret; > + > + ret = imx_sc_misc_get_control(ccm_ipc_handle, gpr_mux->rsrc_id, > + gpr_mux->gpr_id, &val); > + if (ret) { > + pr_err("%s: failed to get clock parent %d\n", > + clk_hw_get_name(hw), ret); > + return 0; > + } > + > + return (u8)val; Nitpick: Please drop explicit casts. > +} > + > +static int clk_mux_gpr_scu_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct clk_mux_gpr_scu *gpr_mux = to_clk_mux_gpr_scu(hw); > + int ret; > + > + ret = imx_sc_misc_set_control(ccm_ipc_handle, gpr_mux->rsrc_id, > + gpr_mux->gpr_id, index); > + if (ret) > + pr_err("%s: failed to set clock parent %d\n", > + clk_hw_get_name(hw), ret); Nitpick: These printks are cluttering the code, any chance they can be removed and you can rely on callers to care if something failed? > + > + return ret; > +} > + > +static const struct clk_ops clk_mux_gpr_scu_ops = { > + .get_parent = clk_mux_gpr_scu_get_parent, > + .set_parent = clk_mux_gpr_scu_set_parent, > +}; > + > +struct clk_hw *clk_register_mux_gpr_scu(const char *name, const char * const *parents,
Quoting A.s. Dong (2018-10-14 01:07:45) > Add scu clock common part which will be used by client clock drivers. > > 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> > --- > ChangeLog: > v3->v4: > * scu headfile path change > v2->v3: > * no changes > v1->v2: > * update function call name > --- > drivers/clk/imx/Kconfig | 2 ++ > drivers/clk/imx/Makefile | 2 ++ > drivers/clk/imx/scu/Kconfig | 5 +++++ > drivers/clk/imx/scu/Makefile | 4 ++++ > drivers/clk/imx/scu/clk-scu.c | 17 +++++++++++++++++ > drivers/clk/imx/scu/clk-scu.h | 18 ++++++++++++++++++ Why is scu nested one level deeper in the imx directory? Can't we just keep them all within imx toplevel directory? > 6 files changed, 48 insertions(+) > create mode 100644 drivers/clk/imx/scu/Kconfig > create mode 100644 drivers/clk/imx/scu/Makefile > create mode 100644 drivers/clk/imx/scu/clk-scu.c > create mode 100644 drivers/clk/imx/scu/clk-scu.h >
Quoting A.s. Dong (2018-10-14 01:08:09) > This may be used by both mmio and scu clks. So let's put it > into a common file. > > 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/clk-common.h | 16 ++++++++++++++++ > drivers/clk/imx/scu/clk-scu.h | 2 ++ > 2 files changed, 18 insertions(+) > create mode 100644 drivers/clk/imx/clk-common.h > > diff --git a/drivers/clk/imx/clk-common.h b/drivers/clk/imx/clk-common.h > new file mode 100644 > index 0000000..e3634a5 > --- /dev/null > +++ b/drivers/clk/imx/clk-common.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright 2018 NXP > + */ > + > +#ifndef __IMX_CLK_COMMON_H > +#define __IMX_CLK_COMMON_H > + > +#include <linux/clk-provider.h> > + > +static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int rate) > +{ > + return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate); > +} Why do we need this wrapper file? Just call the registration function directly with the right arguments instead of wrapping them in another function please.
Quoting A.s. Dong (2018-10-14 01:08:13) > Add imx_check_clk_hws helper function > > 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/clk-common.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/clk/imx/clk-common.h b/drivers/clk/imx/clk-common.h > index e3634a5..01550fd 100644 > --- a/drivers/clk/imx/clk-common.h > +++ b/drivers/clk/imx/clk-common.h > @@ -13,4 +13,15 @@ static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int rate) > return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate); > } > > +static inline void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count) > +{ > + unsigned int i; > + > + for (i = 0; i < count; i++) { > + if (IS_ERR(clks[i])) > + pr_err("i.MX clk %u: register failed with %ld\n", > + i, PTR_ERR(clks[i])); > + } > +} And get rid of this too? I don't see the need for layers on top of code snippets. Just write them many times in the same driver, and then decide to consolidate that logic behind something larger than a few helper functions. > +
Quoting A.s. Dong (2018-10-14 01:08:16) > diff --git a/drivers/clk/imx/scu/clk-imx8qxp.c b/drivers/clk/imx/scu/clk-imx8qxp.c > new file mode 100644 > index 0000000..958c26d > --- /dev/null > +++ b/drivers/clk/imx/scu/clk-imx8qxp.c > @@ -0,0 +1,425 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > + * Copyright 2017~2018 NXP > + * Dong Aisheng <aisheng.dong@nxp.com> > + */ > + > +#include <dt-bindings/clock/imx8qxp-clock.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > + > +#include <soc/imx/imx8qxp/lpcg.h> > + > +#include "clk-scu.h" > + > +static struct clk_hw_onecell_data *clk_data; Please give this static global variable a more unique name as opposed to 'clk_data'. > + > +static const char * const enet_sels[] = { "enet_25MHz", "enet_125MHz", }; > +static const char * const enet0_rmii_tx_sels[] = { "enet0_ref_div", "dummy", }; > +static const char * const enet1_rmii_tx_sels[] = { "enet1_ref_div", "dummy", }; > + > +static int imx8qxp_clk_probe(struct platform_device *pdev) > +{ > + struct device_node *ccm_node = pdev->dev.of_node; > + struct clk_hw **clks; > + int ret; > + > + ret = imx_clk_scu_init(); > + if (ret) > + return ret; > + > + clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) + > + sizeof(*clk_data->hws) * IMX8QXP_CLK_END, > + GFP_KERNEL); > + if (!clk_data) > + return -ENOMEM; > + > + clk_data->num = IMX8QXP_CLK_END; > + clks = clk_data->hws; > + > + /* Fixed clocks */ > + clks[IMX8QXP_CLK_DUMMY] = imx_clk_hw_fixed("dummy", 0); > + clks[IMX8QXP_24MHZ] = imx_clk_hw_fixed("xtal_24MHz", 24000000); > + clks[IMX8QXP_GPT_3M] = imx_clk_hw_fixed("gpt_3m", 3000000); > + clks[IMX8QXP_32KHZ] = imx_clk_hw_fixed("xtal_32KHz", 32768); > + > + /* ARM core */ > + clks[IMX8QXP_A35_DIV] = imx_clk_divider_scu("a35_div", IMX_SC_R_A35, IMX_SC_PM_CLK_CPU); > + > + clks[IMX8QXP_IPG_DMA_CLK_ROOT] = imx_clk_hw_fixed("ipg_dma_clk_root", 120000000); > + clks[IMX8QXP_AXI_CONN_CLK_ROOT] = imx_clk_hw_fixed("axi_conn_clk_root", 333333333); > + clks[IMX8QXP_AHB_CONN_CLK_ROOT] = imx_clk_hw_fixed("ahb_conn_clk_root", 166666666); > + clks[IMX8QXP_IPG_CONN_CLK_ROOT] = imx_clk_hw_fixed("ipg_conn_clk_root", 83333333); > + clks[IMX8QXP_DC_AXI_EXT_CLK] = imx_clk_hw_fixed("axi_ext_dc_clk_root", 800000000); > + clks[IMX8QXP_DC_AXI_INT_CLK] = imx_clk_hw_fixed("axi_int_dc_clk_root", 400000000); > + clks[IMX8QXP_DC_CFG_CLK] = imx_clk_hw_fixed("cfg_dc_clk_root", 100000000); > + clks[IMX8QXP_MIPI_IPG_CLK] = imx_clk_hw_fixed("ipg_mipi_clk_root", 120000000); > + clks[IMX8QXP_IMG_AXI_CLK] = imx_clk_hw_fixed("axi_img_clk_root", 400000000); > + clks[IMX8QXP_IMG_IPG_CLK] = imx_clk_hw_fixed("ipg_img_clk_root", 200000000); > + clks[IMX8QXP_IMG_PXL_CLK] = imx_clk_hw_fixed("pxl_img_clk_root", 600000000); > + clks[IMX8QXP_HSIO_AXI_CLK] = imx_clk_hw_fixed("axi_hsio_clk_root", 400000000); > + clks[IMX8QXP_HSIO_PER_CLK] = imx_clk_hw_fixed("per_hsio_clk_root", 133333333); Can these fixed rate clks come from DT? Or what's going on here? > + > + clks[IMX8QXP_UART0_DIV] = imx_clk_divider_scu("uart0_div", IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER); > + clks[IMX8QXP_UART0_IPG_CLK] = imx_clk_gate2_scu("uart0_ipg_clk", "ipg_dma_clk_root", (void __iomem *)(LPUART_0_LPCG), 16, 0); > + clks[IMX8QXP_UART0_CLK] = imx_clk_gate_scu("uart0_clk", "uart0_div", IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER, (void __iomem *)(LPUART_0_LPCG), 0, 0); > + [...] > + clks[IMX8QXP_IMG_PDMA_2_CLK] = imx_clk_gate2_scu("img_pdma2_clk", "pxl_img_clk_root", (void __iomem *)(IMG_PDMA_2_LPCG), 0, 0); > + clks[IMX8QXP_IMG_PDMA_3_CLK] = imx_clk_gate2_scu("img_pdma3_clk", "pxl_img_clk_root", (void __iomem *)(IMG_PDMA_3_LPCG), 0, 0); > + clks[IMX8QXP_IMG_PDMA_4_CLK] = imx_clk_gate2_scu("img_pdma4_clk", "pxl_img_clk_root", (void __iomem *)(IMG_PDMA_4_LPCG), 0, 0); > + clks[IMX8QXP_IMG_PDMA_5_CLK] = imx_clk_gate2_scu("img_pdma5_clk", "pxl_img_clk_root", (void __iomem *)(IMG_PDMA_5_LPCG), 0, 0); > + clks[IMX8QXP_IMG_PDMA_6_CLK] = imx_clk_gate2_scu("img_pdma6_clk", "pxl_img_clk_root", (void __iomem *)(IMG_PDMA_6_LPCG), 0, 0); > + clks[IMX8QXP_IMG_PDMA_7_CLK] = imx_clk_gate2_scu("img_pdma7_clk", "pxl_img_clk_root", (void __iomem *)(IMG_PDMA_7_LPCG), 0, 0); All the casts in here shouldn't be necessary, see below. > + > + /* MIPI CSI SS */ > + clks[IMX8QXP_CSI0_I2C0_DIV] = imx_clk_divider_scu("mipi_csi0_i2c0_div", IMX_SC_R_CSI_0_I2C_0, IMX_SC_PM_CLK_PER); > + clks[IMX8QXP_CSI0_PWM0_DIV] = imx_clk_divider_scu("mipi_csi0_pwm0_div", IMX_SC_R_CSI_0_PWM_0, IMX_SC_PM_CLK_PER); > + clks[IMX8QXP_CSI0_CORE_DIV] = imx_clk_divider_scu("mipi_csi0_core_div", IMX_SC_R_CSI_0, IMX_SC_PM_CLK_PER); > + clks[IMX8QXP_CSI0_ESC_DIV] = imx_clk_divider_scu("mipi_csi0_esc_div", IMX_SC_R_CSI_0, IMX_SC_PM_CLK_MISC); > + clks[IMX8QXP_CSI0_IPG_CLK_S] = imx_clk_gate2_scu("mipi_csi0_ipg_s", "ipg_mipi_csi_clk_root", (void __iomem *)(MIPI_CSI_0_LPCG + 0x8), 16, 0); > + clks[IMX8QXP_CSI0_IPG_CLK] = imx_clk_gate2_scu("mipi_csi0_ipg", "mipi_csi0_ipg_s", (void __iomem *)(MIPI_CSI_0_LPCG), 16, 0); > + clks[IMX8QXP_CSI0_APB_CLK] = imx_clk_gate2_scu("mipi_csi0_apb_clk", "ipg_mipi_csi_clk_root", (void __iomem *)(MIPI_CSI_0_LPCG + 0x4), 16, 0); > + clks[IMX8QXP_CSI0_I2C0_IPG_CLK] = imx_clk_gate2_scu("mipi_csi0_i2c0_ipg_s", "ipg_mipi_csi_clk_root", (void __iomem *)(MIPI_CSI_0_LPCG + 0x14), 16, 0); > + clks[IMX8QXP_CSI0_I2C0_CLK] = imx_clk_gate_scu("mipi_csi0_i2c0_clk", "mipi_csi0_i2c0_div", IMX_SC_R_CSI_0_I2C_0, IMX_SC_PM_CLK_PER, (void __iomem *)(MIPI_CSI_0_LPCG + 0x14), 0, 0); > + clks[IMX8QXP_CSI0_PWM0_IPG_CLK] = imx_clk_gate2_scu("mipi_csi0_pwm0_ipg_s", "ipg_mipi_csi_clk_root", (void __iomem *)(MIPI_CSI_0_LPCG + 0x10), 16, 0); > + clks[IMX8QXP_CSI0_PWM0_CLK] = imx_clk_gate_scu("mipi_csi0_pwm0_clk", "mipi_csi0_pwm0_div", IMX_SC_R_CSI_0_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem *)(MIPI_CSI_0_LPCG + 0x10), 0, 0); > + clks[IMX8QXP_CSI0_CORE_CLK] = imx_clk_gate_scu("mipi_csi0_core_clk", "mipi_csi0_core_div", IMX_SC_R_CSI_0, IMX_SC_PM_CLK_PER, (void __iomem *)(MIPI_CSI_0_LPCG + 0x18), 16, 0); > + clks[IMX8QXP_CSI0_ESC_CLK] = imx_clk_gate_scu("mipi_csi0_esc_clk", "mipi_csi0_esc_div", IMX_SC_R_CSI_0, IMX_SC_PM_CLK_MISC, (void __iomem *)(MIPI_CSI_0_LPCG + 0x1C), 16, 0); > + > + /* HSIO SS */ > + clks[IMX8QXP_HSIO_PCIE_MSTR_AXI_CLK] = imx_clk_gate2_scu("hsio_pcie_mstr_axi_clk", "axi_hsio_clk_root", (void __iomem *)(HSIO_PCIE_X1_LPCG), 16, 0); > + clks[IMX8QXP_HSIO_PCIE_SLV_AXI_CLK] = imx_clk_gate2_scu("hsio_pcie_slv_axi_clk", "axi_hsio_clk_root", (void __iomem *)(HSIO_PCIE_X1_LPCG), 20, 0); > + clks[IMX8QXP_HSIO_PCIE_DBI_AXI_CLK] = imx_clk_gate2_scu("hsio_pcie_dbi_axi_clk", "axi_hsio_clk_root", (void __iomem *)(HSIO_PCIE_X1_LPCG), 24, 0); > + clks[IMX8QXP_HSIO_PCIE_X1_PER_CLK] = imx_clk_gate2_scu("hsio_pcie_x1_per_clk", "per_hsio_clk_root", (void __iomem *)(HSIO_PCIE_X1_CRR3_LPCG), 16, 0); > + clks[IMX8QXP_HSIO_PHY_X1_PER_CLK] = imx_clk_gate2_scu("hsio_phy_x1_per_clk", "per_hsio_clk_root", (void __iomem *)(HSIO_PHY_X1_CRR1_LPCG), 16, 0); > + clks[IMX8QXP_HSIO_MISC_PER_CLK] = imx_clk_gate2_scu("hsio_misc_per_clk", "per_hsio_clk_root", (void __iomem *)(HSIO_MISC_LPCG), 16, 0); > + clks[IMX8QXP_HSIO_PHY_X1_APB_CLK] = imx_clk_gate2_scu("hsio_phy_x1_apb_clk", "per_hsio_clk_root", (void __iomem *)(HSIO_PHY_X1_LPCG), 16, 0); > + clks[IMX8QXP_HSIO_GPIO_CLK] = imx_clk_gate2_scu("hsio_gpio_clk", "per_hsio_clk_root", (void __iomem *)(HSIO_GPIO_LPCG), 16, 0); > + clks[IMX8QXP_HSIO_PHY_X1_PCLK] = imx_clk_gate2_scu("hsio_phy_x1_pclk", "dummy", (void __iomem *)(HSIO_PHY_X1_LPCG), 0, 0); > + > + imx_check_clk_hws(clks, clk_data->num); > + > + of_clk_add_hw_provider(ccm_node, of_clk_hw_onecell_get, clk_data); > + > + pr_info("i.MX8QXP clock tree init done.\n"); Please no "I'm alive!" messages. > + > + return 0; > +} > + > +static const struct of_device_id imx8qxp_match[] = { > + { .compatible = "fsl,imx8qxp-clk", }, > + { /* sentinel */ } > +}; > + > +static struct platform_driver imx8qxp_clk_driver = { > + .driver = { > + .name = "imx8qxp-clk", > + .of_match_table = imx8qxp_match, > + }, > + .probe = imx8qxp_clk_probe, > +}; > + > +static int __init imx8qxp_clk_init(void) > +{ > + return platform_driver_register(&imx8qxp_clk_driver); > +} > +core_initcall(imx8qxp_clk_init); > diff --git a/include/soc/imx/imx8qxp/lpcg.h b/include/soc/imx/imx8qxp/lpcg.h > new file mode 100644 > index 0000000..afbb5da > --- /dev/null > +++ b/include/soc/imx/imx8qxp/lpcg.h > @@ -0,0 +1,186 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > + * Copyright 2017~2018 NXP > + * > + */ > + > +#ifndef _SC_LPCG_H > +#define _SC_LPCG_H > + > +/*LSIO SS */ > +#define PWM_0_LPCG 0x5D400000 Are these virtual or physical addresses? Because up above they're casted right into void __iomem pointers, which makes me thing they're virtual addresses, but then that would mean there's some sort of static iomap being used, but I'm not aware of anything like that.
> From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:19 AM [...] > > " Drivers don't need to manually protect resources shared between the > > operations of one group, regardless of whether those resources are > > shared by multiple clocks or not. However, access to resources that > > are shared between operations of the two groups needs to be protected by > the drivers." > > > > Do you think it's okay to drop it? > > > > No it's not OK. We prefer that clk drivers don't assume the global locks in the > clk framework are going to protect them from concurrent access to the same > resource between different clks. Drivers can assume that a clk op won't be > called in parallel for the same clk, but they shouldn't assume that everything is > protected otherwise. If they did, we would have to go find all the drivers that > make this assumption and then fix them when we eventually split the lock into > smaller pieces. > > Long story short, if you have something shared (i.e. a register) and you plan to > write to it and read from it for multiple clks, add a lock around it. Okay, got it. Appreciated for the detailed explanation. Regards Dong Aisheng
> From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:26 AM > Quoting A.s. Dong (2018-10-14 01:07:49) > > diff --git a/drivers/clk/imx/scu/clk-divider-scu.c > > b/drivers/clk/imx/scu/clk-divider-scu.c > > new file mode 100644 > > index 0000000..51cb816 > > --- /dev/null > > +++ b/drivers/clk/imx/scu/clk-divider-scu.c > > @@ -0,0 +1,176 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > + * Copyright 2017~2018 NXP > > + * Dong Aisheng <aisheng.dong@nxp.com> > > + * > > + */ > > +#include <linux/clk.h> > > +#include <linux/clk-provider.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/jiffies.h> > > Is this used? > Good catch, it's introduced by test code and should be removed. > > +#include <linux/slab.h> > > + > > +#include "clk-scu.h" > > + > > +struct clk_divider_scu { > > + struct clk_hw hw; > > Nitpick: No idea why this is spaced out from clk_hw. > Yes, better aligned. > > + u32 rsrc_id; > > + u8 clk_type; > > +}; > > + > > +/* SCU Clock Protocol definitions */ > > +struct imx_sc_msg_req_set_clock_rate { > > + struct imx_sc_rpc_msg hdr; > > + u32 rate; > > + u16 resource; > > + u8 clk; > > +} __packed; > > + > > +struct imx_sc_msg_req_get_clock_rate { > > + struct imx_sc_rpc_msg hdr; > > + u16 resource; > > + u8 clk; > > +} __packed; > > + > > +struct imx_sc_msg_resp_get_clock_rate { > > + struct imx_sc_rpc_msg hdr; > > + u32 rate; > > +} __packed; > > + > > + > > +static inline struct clk_divider_scu *to_clk_divider_scu(struct > > +clk_hw *hw) { > > + return container_of(hw, struct clk_divider_scu, hw); } > > + > > +/* > > + * clk_divider_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_divider_scu_recalc_rate(struct clk_hw *hw, > > + unsigned long > > +parent_rate) { > > + struct clk_divider_scu *div = to_clk_divider_scu(hw); > > + struct imx_sc_msg_req_get_clock_rate msg; > > + struct imx_sc_msg_resp_get_clock_rate *resp; > > + struct imx_sc_rpc_msg *hdr = &msg.hdr; > > + int ret; > > + > > + hdr->ver = IMX_SC_RPC_VERSION; > > + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM; > > + hdr->func = (uint8_t)IMX_SC_PM_FUNC_GET_CLOCK_RATE; > > + hdr->size = 2; > > + > > + msg.resource = div->rsrc_id; > > + msg.clk = div->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; > > + } > > + > > + resp = (struct imx_sc_msg_resp_get_clock_rate *)&msg; > > Does the response get written to the same place that the message is written? If Yes > so, it would be better to combine the different structs into a union that's > always large enough to handle this? For example, it looks like there are only 16 > + 8 bytes for the get_clock_rate structure, but then the response is returning > the rate in 32 bytes. When we cast that here we're now getting an extra 8 > bytes of stack, aren't we? Combining the different structures into one bigger > structure would alleviate this and avoid the need for casting. > Good catch. Thanks for the professional explanation. How about do something like below? struct req_get_clock_rate { u16 resource; u8 clk; } __packed; struct resp_get_clock_rate { u32 rate; } __packed; 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; > > + > > + return resp->rate; > > +} > > + > > +/* > > + * clk_divider_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 > > + * > > + * Gets the current clock rate of a SCU clock. Returns the current > > + * clock rate, or zero in failure. > > + */ > > +static long clk_divider_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_divider_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_divider_scu_set_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long parent_rate) { > > + struct clk_divider_scu *div = to_clk_divider_scu(hw); > > + struct imx_sc_msg_req_set_clock_rate msg; > > + struct imx_sc_rpc_msg *hdr = &msg.hdr; > > + int ret; > > + > > + hdr->ver = IMX_SC_RPC_VERSION; > > + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM; > > + hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_CLOCK_RATE; > > Are these casts necessary? > I guess the explicit case may not need. Will double check. > > + hdr->size = 3; > > + > > + msg.rate = rate; > > + msg.resource = div->rsrc_id; > > + msg.clk = div->clk_type; > > + > > + ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true); > > + if (ret) > > + pr_err("%s: failed to set clock rate %ld : ret %d\n", > > + clk_hw_get_name(hw), rate, ret); > > + > > + return 0; > > +} > > + > > +static const struct clk_ops clk_divider_scu_ops = { > > + .recalc_rate = clk_divider_scu_recalc_rate, > > + .round_rate = clk_divider_scu_round_rate, > > + .set_rate = clk_divider_scu_set_rate, }; > > + > > +struct clk_hw *imx_clk_register_divider_scu(const char *name, > > + const char > *parent_name, > > + u32 rsrc_id, > > + u8 clk_type) { > > + struct clk_divider_scu *div; > > + struct clk_init_data init; > > + struct clk_hw *hw; > > + int ret; > > + > > + div = kzalloc(sizeof(*div), GFP_KERNEL); > > + if (!div) > > + return ERR_PTR(-ENOMEM); > > + > > + div->rsrc_id = rsrc_id; > > + div->clk_type = clk_type; > > + > > + init.name = name; > > + init.ops = &clk_divider_scu_ops; > > + init.flags = CLK_GET_RATE_NOCACHE; > > Why nocache? Please have a good reason and add a comment indicating why. > Because on MX8, the clocks are tightly couple with power domain that once the power domain is off, the clock status will be lost. Making it NOCACHE helps user to retrieve the real clock status from HW Instead of using the possible invalid cached rate. Is that reasonable enough to specify it? Regards Dong Aisheng > > + init.parent_names = parent_name ? &parent_name : NULL; > > + init.num_parents = parent_name ? 1 : 0; > > + div->hw.init = &init; > > + > > + hw = &div->hw; > > + ret = clk_hw_register(NULL, hw); > > + if (ret) { > > + kfree(div); > > + hw = ERR_PTR(ret); > > + } > > + > > + return hw; > > +}
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:28 AM [...] > Quoting A.s. Dong (2018-10-14 01:07:52) > > +/* > > + * clk_divider_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 > > + * > > + * Gets the current clock rate of a SCU clock. Returns the current > > + * clock rate, or zero in failure. > > + */ > > +static unsigned long clk_divider_gpr_scu_recalc_rate(struct clk_hw *hw, > > + unsigned long > > +parent_rate) { > > + struct clk_divider_gpr_scu *clk = to_clk_divider_gpr_scu(hw); > > + u32 val; > > + int ret; > > + > > + ret = imx_sc_misc_get_control(ccm_ipc_handle, clk->rsrc_id, > > + clk->gpr_id, &val); > > + if (ret) { > > + pr_err("%s: failed to get clock rate %d\n", > > + clk_hw_get_name(hw), ret); > > + return 0; > > + } > > + > > + return val ? parent_rate / 2 : parent_rate; > > I hope parent_rate can't be zero here. > It can be zero, I guess. But zero seems safe to me, am I wrong? > > +} > > + > > +/* > > + * clk_divider_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 > > + * > > + * Round clock rate for a SCU clock according to parent rate */ > > +static long clk_divider_gpr_scu_round_rate(struct clk_hw *hw, unsigned > long rate, > > + unsigned long *prate) { > > + if (rate < *prate) > > + rate = *prate / 2; > > + else > > + rate = *prate; > > + > > + return rate; > > +} > > + > > +/* > > + * clk_divider_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 > > + * > > + * Sets a clock frequency for a SCU clock. Returns the SCU > > + * protocol status. > > + */ > > +static int clk_divider_gpr_scu_set_rate(struct clk_hw *hw, unsigned long > rate, > > + unsigned long parent_rate) { > > + struct clk_divider_gpr_scu *clk = to_clk_divider_gpr_scu(hw); > > + uint32_t val; > > + > > + val = (rate < parent_rate) ? 1 : 0; > > Nitpick: Write it out as > > val = 0; > if (rate < parent_rate) > val = 1; > Good suggestion > > + > > + return imx_sc_misc_set_control(ccm_ipc_handle, clk->rsrc_id, > > + clk->gpr_id, val); } > > + > > +static const struct clk_ops clk_divider_gpr_scu_ops = { > > + .recalc_rate = clk_divider_gpr_scu_recalc_rate, > > + .round_rate = clk_divider_gpr_scu_round_rate, > > + .set_rate = clk_divider_gpr_scu_set_rate, }; > > + > > +struct clk_hw *imx_clk_divider_gpr_scu(const char *name, const char > *parent_name, > > + u32 rsrc_id, u8 gpr_id) { > > + struct clk_divider_gpr_scu *div; > > + struct clk_init_data init; > > + struct clk_hw *hw; > > + int ret; > > + > > + div = kzalloc(sizeof(*div), GFP_KERNEL); > > + if (!div) > > + return ERR_PTR(-ENOMEM); > > + > > + div->rsrc_id = rsrc_id; > > + div->gpr_id = gpr_id; > > + > > + init.name = name; > > + init.ops = &clk_divider_gpr_scu_ops; > > + init.flags = CLK_GET_RATE_NOCACHE; > > Same NOCACHE comment. > Replied in another mail. Regards Dong Aisheng > > + init.parent_names = parent_name ? &parent_name : NULL; > > + init.num_parents = parent_name ? 1 : 0; > > + > > + div->hw.init = &init; > > + > > + hw = &div->hw; > > + ret = clk_hw_register(NULL, hw); > > + if (ret) { > > + kfree(div); > > + hw = ERR_PTR(ret); > > + } > > + > > + return hw; > > +}
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:31 AM > Quoting A.s. Dong (2018-10-14 01:08:06) > > diff --git a/drivers/clk/imx/scu/clk-mux-gpr-scu.c > > b/drivers/clk/imx/scu/clk-mux-gpr-scu.c > > new file mode 100644 > > index 0000000..2ad7a80 > > --- /dev/null > > +++ b/drivers/clk/imx/scu/clk-mux-gpr-scu.c > > @@ -0,0 +1,90 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > + * Copyright 2017~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 "clk-scu.h" > > + > > +struct clk_mux_gpr_scu { > > + struct clk_hw hw; > > + u32 rsrc_id; > > + u8 gpr_id; > > +}; > > + > > +#define to_clk_mux_gpr_scu(_hw) container_of(_hw, struct > > +clk_mux_gpr_scu, hw) > > + > > +static u8 clk_mux_gpr_scu_get_parent(struct clk_hw *hw) { > > + struct clk_mux_gpr_scu *gpr_mux = to_clk_mux_gpr_scu(hw); > > + u32 val = 0; > > + int ret; > > + > > + ret = imx_sc_misc_get_control(ccm_ipc_handle, gpr_mux->rsrc_id, > > + gpr_mux->gpr_id, &val); > > + if (ret) { > > + pr_err("%s: failed to get clock parent %d\n", > > + clk_hw_get_name(hw), ret); > > + return 0; > > + } > > + > > + return (u8)val; > > Nitpick: Please drop explicit casts. > Compiler does it for us already, right? Will drop it. > > +} > > + > > +static int clk_mux_gpr_scu_set_parent(struct clk_hw *hw, u8 index) { > > + struct clk_mux_gpr_scu *gpr_mux = to_clk_mux_gpr_scu(hw); > > + int ret; > > + > > + ret = imx_sc_misc_set_control(ccm_ipc_handle, gpr_mux->rsrc_id, > > + gpr_mux->gpr_id, index); > > + if (ret) > > + pr_err("%s: failed to set clock parent %d\n", > > + clk_hw_get_name(hw), ret); > > Nitpick: These printks are cluttering the code, any chance they can be removed > and you can rely on callers to care if something failed? > Yes, good suggestion. Regards Dong Aisheng > > + > > + return ret; > > +} > > + > > +static const struct clk_ops clk_mux_gpr_scu_ops = { > > + .get_parent = clk_mux_gpr_scu_get_parent, > > + .set_parent = clk_mux_gpr_scu_set_parent, }; > > + > > +struct clk_hw *clk_register_mux_gpr_scu(const char *name, const char > > +* const *parents,
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:32 AM > Quoting A.s. Dong (2018-10-14 01:07:45) > > Add scu clock common part which will be used by client clock drivers. > > > > 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> > > --- > > ChangeLog: > > v3->v4: > > * scu headfile path change > > v2->v3: > > * no changes > > v1->v2: > > * update function call name > > --- > > drivers/clk/imx/Kconfig | 2 ++ > > drivers/clk/imx/Makefile | 2 ++ > > drivers/clk/imx/scu/Kconfig | 5 +++++ > > drivers/clk/imx/scu/Makefile | 4 ++++ > > drivers/clk/imx/scu/clk-scu.c | 17 +++++++++++++++++ > > drivers/clk/imx/scu/clk-scu.h | 18 ++++++++++++++++++ > > Why is scu nested one level deeper in the imx directory? Can't we just keep > them all within imx toplevel directory? > Mostly because SCU clocks are totally different from the legacy clocks. (No much legacy things can be reused) So organized them together in a deeper folder for clear separation and better maintenance Do you think we still should put them within imx toplevel directory and mix them with Legacy clocks? Regards Dong Aisheng > > 6 files changed, 48 insertions(+) > > create mode 100644 drivers/clk/imx/scu/Kconfig create mode 100644 > > drivers/clk/imx/scu/Makefile create mode 100644 > > drivers/clk/imx/scu/clk-scu.c create mode 100644 > > drivers/clk/imx/scu/clk-scu.h > >
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:33 AM > Quoting A.s. Dong (2018-10-14 01:08:09) > > This may be used by both mmio and scu clks. So let's put it into a > > common file. > > > > 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/clk-common.h | 16 ++++++++++++++++ > > drivers/clk/imx/scu/clk-scu.h | 2 ++ > > 2 files changed, 18 insertions(+) > > create mode 100644 drivers/clk/imx/clk-common.h > > > > diff --git a/drivers/clk/imx/clk-common.h > > b/drivers/clk/imx/clk-common.h new file mode 100644 index > > 0000000..e3634a5 > > --- /dev/null > > +++ b/drivers/clk/imx/clk-common.h > > @@ -0,0 +1,16 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * Copyright 2018 NXP > > + */ > > + > > +#ifndef __IMX_CLK_COMMON_H > > +#define __IMX_CLK_COMMON_H > > + > > +#include <linux/clk-provider.h> > > + > > +static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int > > +rate) { > > + return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate); > > +} > > Why do we need this wrapper file? This file clk-common.h is used to put the common bits between legacy clocks and scu clocks. Only imx_clk_hw_fixed() is added in this patch. > Just call the registration function directly > with the right arguments instead of wrapping them in another function please. That's legacy way and widely used before in order to save unnecessary parameters for imx clocks. e.g. drivers/clk/imx/clk.h static inline struct clk *imx_clk_fixed(const char *name, int rate) { return clk_register_fixed_rate(NULL, name, NULL, 0, rate); } static inline struct clk *imx_clk_divider(const char *name, const char *parent, void __iomem *reg, u8 shift, u8 width) { return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT, reg, shift, width, 0, &imx_ccm_lock); } I can remove it if you dislike it. Just one question, in later patch, another common function will also be added here: static inline void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count) { unsigned int i; for (i = 0; i < count; i++) { if (IS_ERR(clks[i])) pr_err("i.MX clk %u: register failed with %ld\n", i, PTR_ERR(clks[i])); } } Should we remove it as well? Regards Dong Aisheng
> From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:35 AM > Quoting A.s. Dong (2018-10-14 01:08:13) > > Add imx_check_clk_hws helper function > > > > 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/clk-common.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/clk/imx/clk-common.h > > b/drivers/clk/imx/clk-common.h index e3634a5..01550fd 100644 > > --- a/drivers/clk/imx/clk-common.h > > +++ b/drivers/clk/imx/clk-common.h > > @@ -13,4 +13,15 @@ static inline struct clk_hw *imx_clk_hw_fixed(const > char *name, int rate) > > return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate); > > } > > > > +static inline void imx_check_clk_hws(struct clk_hw *clks[], unsigned > > +int count) { > > + unsigned int i; > > + > > + for (i = 0; i < count; i++) { > > + if (IS_ERR(clks[i])) > > + pr_err("i.MX clk %u: register failed with %ld\n", > > + i, PTR_ERR(clks[i])); > > + } > > +} > > And get rid of this too? I don't see the need for layers on top of code snippets. > Just write them many times in the same driver, and then decide to consolidate > that logic behind something larger than a few helper functions. Okay, just see this, then forget my former question in another email talking about this function. Will remove them all. Thanks for the suggestion. Regards Dong Aisheng > > > +
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 5:39 AM > To: A.s. Dong <aisheng.dong@nxp.com>; linux-clk@vger.kernel.org > 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; A.s. Dong > <aisheng.dong@nxp.com> > Subject: Re: [PATCH V4 11/11] clk: imx: add imx8qxp clk driver > > Quoting A.s. Dong (2018-10-14 01:08:16) > > diff --git a/drivers/clk/imx/scu/clk-imx8qxp.c > > b/drivers/clk/imx/scu/clk-imx8qxp.c > > new file mode 100644 > > index 0000000..958c26d > > --- /dev/null > > +++ b/drivers/clk/imx/scu/clk-imx8qxp.c > > @@ -0,0 +1,425 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > + * Copyright 2017~2018 NXP > > + * Dong Aisheng <aisheng.dong@nxp.com> > > + */ > > + > > +#include <dt-bindings/clock/imx8qxp-clock.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > + > > +#include <soc/imx/imx8qxp/lpcg.h> > > + > > +#include "clk-scu.h" > > + > > +static struct clk_hw_onecell_data *clk_data; > > Please give this static global variable a more unique name as opposed to > 'clk_data'. > Got it. > > > + > > +static const char * const enet_sels[] = { "enet_25MHz", > > +"enet_125MHz", }; static const char * const enet0_rmii_tx_sels[] = { > > +"enet0_ref_div", "dummy", }; static const char * const > > +enet1_rmii_tx_sels[] = { "enet1_ref_div", "dummy", }; > > + > > +static int imx8qxp_clk_probe(struct platform_device *pdev) { > > + struct device_node *ccm_node = pdev->dev.of_node; > > + struct clk_hw **clks; > > + int ret; > > + > > + ret = imx_clk_scu_init(); > > + if (ret) > > + return ret; > > + > > + clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) + > > + sizeof(*clk_data->hws) * > IMX8QXP_CLK_END, > > + GFP_KERNEL); > > + if (!clk_data) > > + return -ENOMEM; > > + > > + clk_data->num = IMX8QXP_CLK_END; > > + clks = clk_data->hws; > > + > > + /* Fixed clocks */ > > + clks[IMX8QXP_CLK_DUMMY] = > imx_clk_hw_fixed("dummy", 0); > > + clks[IMX8QXP_24MHZ] = > imx_clk_hw_fixed("xtal_24MHz", 24000000); > > + clks[IMX8QXP_GPT_3M] = > imx_clk_hw_fixed("gpt_3m", 3000000); > > + clks[IMX8QXP_32KHZ] = > imx_clk_hw_fixed("xtal_32KHz", 32768); > > + These are external clocks and should be put in Devicetree. > > + /* ARM core */ > > + clks[IMX8QXP_A35_DIV] = > imx_clk_divider_scu("a35_div", IMX_SC_R_A35, IMX_SC_PM_CLK_CPU); > > + > > + clks[IMX8QXP_IPG_DMA_CLK_ROOT] = > imx_clk_hw_fixed("ipg_dma_clk_root", 120000000); > > + clks[IMX8QXP_AXI_CONN_CLK_ROOT] = > imx_clk_hw_fixed("axi_conn_clk_root", 333333333); > > + clks[IMX8QXP_AHB_CONN_CLK_ROOT] = > imx_clk_hw_fixed("ahb_conn_clk_root", 166666666); > > + clks[IMX8QXP_IPG_CONN_CLK_ROOT] = > imx_clk_hw_fixed("ipg_conn_clk_root", 83333333); > > + clks[IMX8QXP_DC_AXI_EXT_CLK] = > imx_clk_hw_fixed("axi_ext_dc_clk_root", 800000000); > > + clks[IMX8QXP_DC_AXI_INT_CLK] = > imx_clk_hw_fixed("axi_int_dc_clk_root", 400000000); > > + clks[IMX8QXP_DC_CFG_CLK] = > imx_clk_hw_fixed("cfg_dc_clk_root", 100000000); > > + clks[IMX8QXP_MIPI_IPG_CLK] = > imx_clk_hw_fixed("ipg_mipi_clk_root", 120000000); > > + clks[IMX8QXP_IMG_AXI_CLK] = > imx_clk_hw_fixed("axi_img_clk_root", 400000000); > > + clks[IMX8QXP_IMG_IPG_CLK] = > imx_clk_hw_fixed("ipg_img_clk_root", 200000000); > > + clks[IMX8QXP_IMG_PXL_CLK] = > imx_clk_hw_fixed("pxl_img_clk_root", 600000000); > > + clks[IMX8QXP_HSIO_AXI_CLK] = > imx_clk_hw_fixed("axi_hsio_clk_root", 400000000); > > + clks[IMX8QXP_HSIO_PER_CLK] = > imx_clk_hw_fixed("per_hsio_clk_root", 133333333); > > Can these fixed rate clks come from DT? Or what's going on here? > Those are not external clocks. Do we have to put them in device tree as well? > > + > > + clks[IMX8QXP_UART0_DIV] = > imx_clk_divider_scu("uart0_div", IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER); > > + clks[IMX8QXP_UART0_IPG_CLK] = > imx_clk_gate2_scu("uart0_ipg_clk", "ipg_dma_clk_root", (void __iomem > *)(LPUART_0_LPCG), 16, 0); > > + clks[IMX8QXP_UART0_CLK] = > imx_clk_gate_scu("uart0_clk", "uart0_div", IMX_SC_R_UART_0, > IMX_SC_PM_CLK_PER, (void __iomem *)(LPUART_0_LPCG), 0, 0); > > + > [...] > > + clks[IMX8QXP_IMG_PDMA_2_CLK] = > imx_clk_gate2_scu("img_pdma2_clk", "pxl_img_clk_root", (void __iomem > *)(IMG_PDMA_2_LPCG), 0, 0); > > + clks[IMX8QXP_IMG_PDMA_3_CLK] = > imx_clk_gate2_scu("img_pdma3_clk", "pxl_img_clk_root", (void __iomem > *)(IMG_PDMA_3_LPCG), 0, 0); > > + clks[IMX8QXP_IMG_PDMA_4_CLK] = > imx_clk_gate2_scu("img_pdma4_clk", "pxl_img_clk_root", (void __iomem > *)(IMG_PDMA_4_LPCG), 0, 0); > > + clks[IMX8QXP_IMG_PDMA_5_CLK] = > imx_clk_gate2_scu("img_pdma5_clk", "pxl_img_clk_root", (void __iomem > *)(IMG_PDMA_5_LPCG), 0, 0); > > + clks[IMX8QXP_IMG_PDMA_6_CLK] = > imx_clk_gate2_scu("img_pdma6_clk", "pxl_img_clk_root", (void __iomem > *)(IMG_PDMA_6_LPCG), 0, 0); > > + clks[IMX8QXP_IMG_PDMA_7_CLK] = > imx_clk_gate2_scu("img_pdma7_clk", "pxl_img_clk_root", (void __iomem > *)(IMG_PDMA_7_LPCG), 0, 0); > > All the casts in here shouldn't be necessary, see below. > > > + > > + /* MIPI CSI SS */ > > + clks[IMX8QXP_CSI0_I2C0_DIV] = > imx_clk_divider_scu("mipi_csi0_i2c0_div", IMX_SC_R_CSI_0_I2C_0, > IMX_SC_PM_CLK_PER); > > + clks[IMX8QXP_CSI0_PWM0_DIV] = > imx_clk_divider_scu("mipi_csi0_pwm0_div", IMX_SC_R_CSI_0_PWM_0, > IMX_SC_PM_CLK_PER); > > + clks[IMX8QXP_CSI0_CORE_DIV] = > imx_clk_divider_scu("mipi_csi0_core_div", IMX_SC_R_CSI_0, > IMX_SC_PM_CLK_PER); > > + clks[IMX8QXP_CSI0_ESC_DIV] = > imx_clk_divider_scu("mipi_csi0_esc_div", IMX_SC_R_CSI_0, > IMX_SC_PM_CLK_MISC); > > + clks[IMX8QXP_CSI0_IPG_CLK_S] = > imx_clk_gate2_scu("mipi_csi0_ipg_s", "ipg_mipi_csi_clk_root", (void __iomem > *)(MIPI_CSI_0_LPCG + 0x8), 16, 0); > > + clks[IMX8QXP_CSI0_IPG_CLK] = > imx_clk_gate2_scu("mipi_csi0_ipg", "mipi_csi0_ipg_s", (void __iomem > *)(MIPI_CSI_0_LPCG), 16, 0); > > + clks[IMX8QXP_CSI0_APB_CLK] = > imx_clk_gate2_scu("mipi_csi0_apb_clk", "ipg_mipi_csi_clk_root", (void > __iomem *)(MIPI_CSI_0_LPCG + 0x4), 16, 0); > > + clks[IMX8QXP_CSI0_I2C0_IPG_CLK] = > imx_clk_gate2_scu("mipi_csi0_i2c0_ipg_s", "ipg_mipi_csi_clk_root", (void > __iomem *)(MIPI_CSI_0_LPCG + 0x14), 16, 0); > > + clks[IMX8QXP_CSI0_I2C0_CLK] = > imx_clk_gate_scu("mipi_csi0_i2c0_clk", "mipi_csi0_i2c0_div", > IMX_SC_R_CSI_0_I2C_0, IMX_SC_PM_CLK_PER, (void __iomem > *)(MIPI_CSI_0_LPCG + 0x14), 0, 0); > > + clks[IMX8QXP_CSI0_PWM0_IPG_CLK] = > imx_clk_gate2_scu("mipi_csi0_pwm0_ipg_s", "ipg_mipi_csi_clk_root", (void > __iomem *)(MIPI_CSI_0_LPCG + 0x10), 16, 0); > > + clks[IMX8QXP_CSI0_PWM0_CLK] = > imx_clk_gate_scu("mipi_csi0_pwm0_clk", "mipi_csi0_pwm0_div", > IMX_SC_R_CSI_0_PWM_0, IMX_SC_PM_CLK_PER, (void __iomem > *)(MIPI_CSI_0_LPCG + 0x10), 0, 0); > > + clks[IMX8QXP_CSI0_CORE_CLK] = > imx_clk_gate_scu("mipi_csi0_core_clk", "mipi_csi0_core_div", > IMX_SC_R_CSI_0, IMX_SC_PM_CLK_PER, (void __iomem *)(MIPI_CSI_0_LPCG + > 0x18), 16, 0); > > + clks[IMX8QXP_CSI0_ESC_CLK] = > imx_clk_gate_scu("mipi_csi0_esc_clk", "mipi_csi0_esc_div", IMX_SC_R_CSI_0, > IMX_SC_PM_CLK_MISC, (void __iomem *)(MIPI_CSI_0_LPCG + 0x1C), 16, 0); > > + > > + /* HSIO SS */ > > + clks[IMX8QXP_HSIO_PCIE_MSTR_AXI_CLK] = > imx_clk_gate2_scu("hsio_pcie_mstr_axi_clk", "axi_hsio_clk_root", (void > __iomem *)(HSIO_PCIE_X1_LPCG), 16, 0); > > + clks[IMX8QXP_HSIO_PCIE_SLV_AXI_CLK] = > imx_clk_gate2_scu("hsio_pcie_slv_axi_clk", "axi_hsio_clk_root", (void > __iomem *)(HSIO_PCIE_X1_LPCG), 20, 0); > > + clks[IMX8QXP_HSIO_PCIE_DBI_AXI_CLK] = > imx_clk_gate2_scu("hsio_pcie_dbi_axi_clk", "axi_hsio_clk_root", (void > __iomem *)(HSIO_PCIE_X1_LPCG), 24, 0); > > + clks[IMX8QXP_HSIO_PCIE_X1_PER_CLK] = > imx_clk_gate2_scu("hsio_pcie_x1_per_clk", "per_hsio_clk_root", (void > __iomem *)(HSIO_PCIE_X1_CRR3_LPCG), 16, 0); > > + clks[IMX8QXP_HSIO_PHY_X1_PER_CLK] = > imx_clk_gate2_scu("hsio_phy_x1_per_clk", "per_hsio_clk_root", (void > __iomem *)(HSIO_PHY_X1_CRR1_LPCG), 16, 0); > > + clks[IMX8QXP_HSIO_MISC_PER_CLK] = > imx_clk_gate2_scu("hsio_misc_per_clk", "per_hsio_clk_root", (void __iomem > *)(HSIO_MISC_LPCG), 16, 0); > > + clks[IMX8QXP_HSIO_PHY_X1_APB_CLK] = > imx_clk_gate2_scu("hsio_phy_x1_apb_clk", "per_hsio_clk_root", (void > __iomem *)(HSIO_PHY_X1_LPCG), 16, 0); > > + clks[IMX8QXP_HSIO_GPIO_CLK] = > imx_clk_gate2_scu("hsio_gpio_clk", "per_hsio_clk_root", (void __iomem > *)(HSIO_GPIO_LPCG), 16, 0); > > + clks[IMX8QXP_HSIO_PHY_X1_PCLK] = > imx_clk_gate2_scu("hsio_phy_x1_pclk", "dummy", (void __iomem > *)(HSIO_PHY_X1_LPCG), 0, 0); > > + > > + imx_check_clk_hws(clks, clk_data->num); > > + > > + of_clk_add_hw_provider(ccm_node, of_clk_hw_onecell_get, > > + clk_data); > > + > > + pr_info("i.MX8QXP clock tree init done.\n"); > > Please no "I'm alive!" messages. > Got it > > + > > + return 0; > > +} > > + > > +static const struct of_device_id imx8qxp_match[] = { > > + { .compatible = "fsl,imx8qxp-clk", }, > > + { /* sentinel */ } > > +}; > > + > > +static struct platform_driver imx8qxp_clk_driver = { > > + .driver = { > > + .name = "imx8qxp-clk", > > + .of_match_table = imx8qxp_match, > > + }, > > + .probe = imx8qxp_clk_probe, > > +}; > > + > > +static int __init imx8qxp_clk_init(void) { > > + return platform_driver_register(&imx8qxp_clk_driver); > > +} > > +core_initcall(imx8qxp_clk_init); > > diff --git a/include/soc/imx/imx8qxp/lpcg.h > > b/include/soc/imx/imx8qxp/lpcg.h new file mode 100644 index > > 0000000..afbb5da > > --- /dev/null > > +++ b/include/soc/imx/imx8qxp/lpcg.h > > @@ -0,0 +1,186 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > + * Copyright 2017~2018 NXP > > + * > > + */ > > + > > +#ifndef _SC_LPCG_H > > +#define _SC_LPCG_H > > + > > +/*LSIO SS */ > > +#define PWM_0_LPCG 0x5D400000 > > Are these virtual or physical addresses? Because up above they're casted right > into void __iomem pointers, which makes me thing they're virtual addresses, > but then that would mean there's some sort of static iomap being used, but > I'm not aware of anything like that. Yes, you're right. Those should be physical address and iomem cast is not necessary. Sorry about missing that. Regards Dong Aisheng
Quoting A.s. Dong (2018-10-17 02:11:03) > > -----Original Message----- > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Wednesday, October 17, 2018 5:32 AM > > Quoting A.s. Dong (2018-10-14 01:07:45) > > > Add scu clock common part which will be used by client clock drivers. > > > > > > 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> > > > --- > > > ChangeLog: > > > v3->v4: > > > * scu headfile path change > > > v2->v3: > > > * no changes > > > v1->v2: > > > * update function call name > > > --- > > > drivers/clk/imx/Kconfig | 2 ++ > > > drivers/clk/imx/Makefile | 2 ++ > > > drivers/clk/imx/scu/Kconfig | 5 +++++ > > > drivers/clk/imx/scu/Makefile | 4 ++++ > > > drivers/clk/imx/scu/clk-scu.c | 17 +++++++++++++++++ > > > drivers/clk/imx/scu/clk-scu.h | 18 ++++++++++++++++++ > > > > Why is scu nested one level deeper in the imx directory? Can't we just keep > > them all within imx toplevel directory? > > > > Mostly because SCU clocks are totally different from the legacy clocks. > (No much legacy things can be reused) So organized them together in a deeper folder > for clear separation and better maintenance > > Do you think we still should put them within imx toplevel directory and mix them with > Legacy clocks? > Yes. Or make a new imx-foo directory for your new fangled SoC clk driver code in drivers/clk/.
Quoting A.s. Dong (2018-10-17 01:56:35) > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Wednesday, October 17, 2018 5:26 AM > > Quoting A.s. Dong (2018-10-14 01:07:49) > > > diff --git a/drivers/clk/imx/scu/clk-divider-scu.c > > > b/drivers/clk/imx/scu/clk-divider-scu.c > > > new file mode 100644 > > > index 0000000..51cb816 > > > --- /dev/null > > > +++ b/drivers/clk/imx/scu/clk-divider-scu.c > > > @@ -0,0 +1,176 @@ > > > + msg.clk = div->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; > > > + } > > > + > > > + resp = (struct imx_sc_msg_resp_get_clock_rate *)&msg; > > > > Does the response get written to the same place that the message is written? If > > Yes > > > so, it would be better to combine the different structs into a union that's > > always large enough to handle this? For example, it looks like there are only 16 > > + 8 bytes for the get_clock_rate structure, but then the response is returning > > the rate in 32 bytes. When we cast that here we're now getting an extra 8 > > bytes of stack, aren't we? Combining the different structures into one bigger > > structure would alleviate this and avoid the need for casting. > > > > Good catch. > Thanks for the professional explanation. > > How about do something like below? > struct req_get_clock_rate { > u16 resource; > u8 clk; > } __packed; > > struct resp_get_clock_rate { > u32 rate; > } __packed; This doesn't need __packed because it's a single u32. > > 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; > Yes something like this would be best. And now I wonder if imx_scu_call_rpc() is doing endianness swapping? Or does it copy data into these response structures? I'm saying that the u32/16/8 may need to be __le32/16/8 and then have the proper accessors. > > > > + hdr->size = 3; > > > + > > > + msg.rate = rate; > > > + msg.resource = div->rsrc_id; > > > + msg.clk = div->clk_type; > > > + > > > + ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true); > > > + if (ret) > > > + pr_err("%s: failed to set clock rate %ld : ret %d\n", > > > + clk_hw_get_name(hw), rate, ret); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct clk_ops clk_divider_scu_ops = { > > > + .recalc_rate = clk_divider_scu_recalc_rate, > > > + .round_rate = clk_divider_scu_round_rate, > > > + .set_rate = clk_divider_scu_set_rate, }; > > > + > > > +struct clk_hw *imx_clk_register_divider_scu(const char *name, > > > + const char > > *parent_name, > > > + u32 rsrc_id, > > > + u8 clk_type) { > > > + struct clk_divider_scu *div; > > > + struct clk_init_data init; > > > + struct clk_hw *hw; > > > + int ret; > > > + > > > + div = kzalloc(sizeof(*div), GFP_KERNEL); > > > + if (!div) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + div->rsrc_id = rsrc_id; > > > + div->clk_type = clk_type; > > > + > > > + init.name = name; > > > + init.ops = &clk_divider_scu_ops; > > > + init.flags = CLK_GET_RATE_NOCACHE; > > > > Why nocache? Please have a good reason and add a comment indicating why. > > > > Because on MX8, the clocks are tightly couple with power domain that > once the power domain is off, the clock status will be lost. > Making it NOCACHE helps user to retrieve the real clock status from HW > Instead of using the possible invalid cached rate. > > Is that reasonable enough to specify it? Yes, you can add a comment like that. Or the clk rate can be restored when the power domain is enabled again?
Quoting A.s. Dong (2018-10-17 02:03:28) > > -----Original Message----- > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Wednesday, October 17, 2018 5:28 AM > [...] > > Quoting A.s. Dong (2018-10-14 01:07:52) > > > +/* > > > + * clk_divider_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 > > > + * > > > + * Gets the current clock rate of a SCU clock. Returns the current > > > + * clock rate, or zero in failure. > > > + */ > > > +static unsigned long clk_divider_gpr_scu_recalc_rate(struct clk_hw *hw, > > > + unsigned long > > > +parent_rate) { > > > + struct clk_divider_gpr_scu *clk = to_clk_divider_gpr_scu(hw); > > > + u32 val; > > > + int ret; > > > + > > > + ret = imx_sc_misc_get_control(ccm_ipc_handle, clk->rsrc_id, > > > + clk->gpr_id, &val); > > > + if (ret) { > > > + pr_err("%s: failed to get clock rate %d\n", > > > + clk_hw_get_name(hw), ret); > > > + return 0; > > > + } > > > + > > > + return val ? parent_rate / 2 : parent_rate; > > > > I hope parent_rate can't be zero here. > > > > It can be zero, I guess. > But zero seems safe to me, am I wrong? No nothing goes wrong so it's fine.
Quoting A.s. Dong (2018-10-17 02:07:00) > > -----Original Message----- > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Wednesday, October 17, 2018 5:31 AM > > Quoting A.s. Dong (2018-10-14 01:08:06) > > > diff --git a/drivers/clk/imx/scu/clk-mux-gpr-scu.c > > > b/drivers/clk/imx/scu/clk-mux-gpr-scu.c > > > new file mode 100644 > > > index 0000000..2ad7a80 > > > --- /dev/null > > > +++ b/drivers/clk/imx/scu/clk-mux-gpr-scu.c > > > @@ -0,0 +1,90 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > > + * Copyright 2017~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 "clk-scu.h" > > > + > > > +struct clk_mux_gpr_scu { > > > + struct clk_hw hw; > > > + u32 rsrc_id; > > > + u8 gpr_id; > > > +}; > > > + > > > +#define to_clk_mux_gpr_scu(_hw) container_of(_hw, struct > > > +clk_mux_gpr_scu, hw) > > > + > > > +static u8 clk_mux_gpr_scu_get_parent(struct clk_hw *hw) { > > > + struct clk_mux_gpr_scu *gpr_mux = to_clk_mux_gpr_scu(hw); > > > + u32 val = 0; > > > + int ret; > > > + > > > + ret = imx_sc_misc_get_control(ccm_ipc_handle, gpr_mux->rsrc_id, > > > + gpr_mux->gpr_id, &val); > > > + if (ret) { > > > + pr_err("%s: failed to get clock parent %d\n", > > > + clk_hw_get_name(hw), ret); > > > + return 0; > > > + } > > > + > > > + return (u8)val; > > > > Nitpick: Please drop explicit casts. > > > > Compiler does it for us already, right? > Will drop it. Yes and my brain is a tired compiler :)
Quoting A.s. Dong (2018-10-17 02:21:57) > > -----Original Message----- > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Wednesday, October 17, 2018 5:33 AM > > Quoting A.s. Dong (2018-10-14 01:08:09) > > > This may be used by both mmio and scu clks. So let's put it into a > > > common file. > > > > > > 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/clk-common.h | 16 ++++++++++++++++ > > > drivers/clk/imx/scu/clk-scu.h | 2 ++ > > > 2 files changed, 18 insertions(+) > > > create mode 100644 drivers/clk/imx/clk-common.h > > > > > > diff --git a/drivers/clk/imx/clk-common.h > > > b/drivers/clk/imx/clk-common.h new file mode 100644 index > > > 0000000..e3634a5 > > > --- /dev/null > > > +++ b/drivers/clk/imx/clk-common.h > > > @@ -0,0 +1,16 @@ > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > +/* > > > + * Copyright 2018 NXP > > > + */ > > > + > > > +#ifndef __IMX_CLK_COMMON_H > > > +#define __IMX_CLK_COMMON_H > > > + > > > +#include <linux/clk-provider.h> > > > + > > > +static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int > > > +rate) { > > > + return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate); > > > +} > > > > Why do we need this wrapper file? > > This file clk-common.h is used to put the common bits between legacy clocks and scu clocks. > Only imx_clk_hw_fixed() is added in this patch. > > > Just call the registration function directly > > with the right arguments instead of wrapping them in another function please. > > That's legacy way and widely used before in order to save unnecessary parameters > for imx clocks. > e.g. > drivers/clk/imx/clk.h > static inline struct clk *imx_clk_fixed(const char *name, int rate) > { > return clk_register_fixed_rate(NULL, name, NULL, 0, rate); > } > > static inline struct clk *imx_clk_divider(const char *name, const char *parent, > void __iomem *reg, u8 shift, u8 width) > { > return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT, > reg, shift, width, 0, &imx_ccm_lock); > } > > I can remove it if you dislike it. > > Just one question, in later patch, another common function will also be added here: > static inline void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count) > { > unsigned int i; > > for (i = 0; i < count; i++) { > if (IS_ERR(clks[i])) > pr_err("i.MX clk %u: register failed with %ld\n", > i, PTR_ERR(clks[i])); > } > } > > Should we remove it as well? > Yes please remove this whole file.
Quoting A.s. Dong (2018-10-17 02:43:05) > > -----Original Message----- > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Wednesday, October 17, 2018 5:39 AM > > To: A.s. Dong <aisheng.dong@nxp.com>; linux-clk@vger.kernel.org > > 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; A.s. Dong > > <aisheng.dong@nxp.com> > > Subject: Re: [PATCH V4 11/11] clk: imx: add imx8qxp clk driver > > > > Quoting A.s. Dong (2018-10-14 01:08:16) > > > diff --git a/drivers/clk/imx/scu/clk-imx8qxp.c > > > b/drivers/clk/imx/scu/clk-imx8qxp.c > > > new file mode 100644 > > > index 0000000..958c26d > > > --- /dev/null > > > +++ b/drivers/clk/imx/scu/clk-imx8qxp.c > > > > > + > > > +static const char * const enet_sels[] = { "enet_25MHz", > > > +"enet_125MHz", }; static const char * const enet0_rmii_tx_sels[] = { > > > +"enet0_ref_div", "dummy", }; static const char * const > > > +enet1_rmii_tx_sels[] = { "enet1_ref_div", "dummy", }; > > > + > > > +static int imx8qxp_clk_probe(struct platform_device *pdev) { > > > + struct device_node *ccm_node = pdev->dev.of_node; > > > + struct clk_hw **clks; > > > + int ret; > > > + > > > + ret = imx_clk_scu_init(); > > > + if (ret) > > > + return ret; > > > + > > > + clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) + > > > + sizeof(*clk_data->hws) * > > IMX8QXP_CLK_END, > > > + GFP_KERNEL); > > > + if (!clk_data) > > > + return -ENOMEM; > > > + > > > + clk_data->num = IMX8QXP_CLK_END; > > > + clks = clk_data->hws; > > > + > > > + /* Fixed clocks */ > > > + clks[IMX8QXP_CLK_DUMMY] = > > imx_clk_hw_fixed("dummy", 0); > > > + clks[IMX8QXP_24MHZ] = > > imx_clk_hw_fixed("xtal_24MHz", 24000000); > > > + clks[IMX8QXP_GPT_3M] = > > imx_clk_hw_fixed("gpt_3m", 3000000); > > > + clks[IMX8QXP_32KHZ] = > > imx_clk_hw_fixed("xtal_32KHz", 32768); > > > + > > These are external clocks and should be put in Devicetree. Ok, great! > > > > + /* ARM core */ > > > + clks[IMX8QXP_A35_DIV] = > > imx_clk_divider_scu("a35_div", IMX_SC_R_A35, IMX_SC_PM_CLK_CPU); > > > + > > > + clks[IMX8QXP_IPG_DMA_CLK_ROOT] = > > imx_clk_hw_fixed("ipg_dma_clk_root", 120000000); > > > + clks[IMX8QXP_AXI_CONN_CLK_ROOT] = > > imx_clk_hw_fixed("axi_conn_clk_root", 333333333); > > > + clks[IMX8QXP_AHB_CONN_CLK_ROOT] = > > imx_clk_hw_fixed("ahb_conn_clk_root", 166666666); > > > + clks[IMX8QXP_IPG_CONN_CLK_ROOT] = > > imx_clk_hw_fixed("ipg_conn_clk_root", 83333333); > > > + clks[IMX8QXP_DC_AXI_EXT_CLK] = > > imx_clk_hw_fixed("axi_ext_dc_clk_root", 800000000); > > > + clks[IMX8QXP_DC_AXI_INT_CLK] = > > imx_clk_hw_fixed("axi_int_dc_clk_root", 400000000); > > > + clks[IMX8QXP_DC_CFG_CLK] = > > imx_clk_hw_fixed("cfg_dc_clk_root", 100000000); > > > + clks[IMX8QXP_MIPI_IPG_CLK] = > > imx_clk_hw_fixed("ipg_mipi_clk_root", 120000000); > > > + clks[IMX8QXP_IMG_AXI_CLK] = > > imx_clk_hw_fixed("axi_img_clk_root", 400000000); > > > + clks[IMX8QXP_IMG_IPG_CLK] = > > imx_clk_hw_fixed("ipg_img_clk_root", 200000000); > > > + clks[IMX8QXP_IMG_PXL_CLK] = > > imx_clk_hw_fixed("pxl_img_clk_root", 600000000); > > > + clks[IMX8QXP_HSIO_AXI_CLK] = > > imx_clk_hw_fixed("axi_hsio_clk_root", 400000000); > > > + clks[IMX8QXP_HSIO_PER_CLK] = > > imx_clk_hw_fixed("per_hsio_clk_root", 133333333); > > > > Can these fixed rate clks come from DT? Or what's going on here? > > > > Those are not external clocks. > Do we have to put them in device tree as well? No if they're not external clks then it's fine to keep specifying them in this driver. > > > > + > > > + return 0; > > > +} > > > + > > > +static const struct of_device_id imx8qxp_match[] = { > > > + { .compatible = "fsl,imx8qxp-clk", }, > > > + { /* sentinel */ } > > > +}; > > > + > > > +static struct platform_driver imx8qxp_clk_driver = { > > > + .driver = { > > > + .name = "imx8qxp-clk", > > > + .of_match_table = imx8qxp_match, > > > + }, > > > + .probe = imx8qxp_clk_probe, > > > +}; > > > + > > > +static int __init imx8qxp_clk_init(void) { > > > + return platform_driver_register(&imx8qxp_clk_driver); > > > +} > > > +core_initcall(imx8qxp_clk_init); > > > diff --git a/include/soc/imx/imx8qxp/lpcg.h > > > b/include/soc/imx/imx8qxp/lpcg.h new file mode 100644 index > > > 0000000..afbb5da > > > --- /dev/null > > > +++ b/include/soc/imx/imx8qxp/lpcg.h > > > @@ -0,0 +1,186 @@ > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > +/* > > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > > + * Copyright 2017~2018 NXP > > > + * > > > + */ > > > + > > > +#ifndef _SC_LPCG_H > > > +#define _SC_LPCG_H > > > + > > > +/*LSIO SS */ > > > +#define PWM_0_LPCG 0x5D400000 > > > > Are these virtual or physical addresses? Because up above they're casted right > > into void __iomem pointers, which makes me thing they're virtual addresses, > > but then that would mean there's some sort of static iomap being used, but > > I'm not aware of anything like that. > > Yes, you're right. Those should be physical address and iomem cast is not necessary. > Sorry about missing that. > Ok. Thanks for fixing it.
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 11:07 PM > Quoting A.s. Dong (2018-10-17 02:11:03) > > > -----Original Message----- > > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > > Sent: Wednesday, October 17, 2018 5:32 AM Quoting A.s. Dong > > > (2018-10-14 01:07:45) > > > > Add scu clock common part which will be used by client clock drivers. > > > > > > > > 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> > > > > --- > > > > ChangeLog: > > > > v3->v4: > > > > * scu headfile path change > > > > v2->v3: > > > > * no changes > > > > v1->v2: > > > > * update function call name > > > > --- > > > > drivers/clk/imx/Kconfig | 2 ++ > > > > drivers/clk/imx/Makefile | 2 ++ > > > > drivers/clk/imx/scu/Kconfig | 5 +++++ > > > > drivers/clk/imx/scu/Makefile | 4 ++++ > > > > drivers/clk/imx/scu/clk-scu.c | 17 +++++++++++++++++ > > > > drivers/clk/imx/scu/clk-scu.h | 18 ++++++++++++++++++ > > > > > > Why is scu nested one level deeper in the imx directory? Can't we > > > just keep them all within imx toplevel directory? > > > > > > > Mostly because SCU clocks are totally different from the legacy clocks. > > (No much legacy things can be reused) So organized them together in a > > deeper folder for clear separation and better maintenance > > > > Do you think we still should put them within imx toplevel directory > > and mix them with Legacy clocks? > > > > Yes. Or make a new imx-foo directory for your new fangled SoC clk driver code > in drivers/clk/. Okay, then let's still put them in the same imx directory. Regards Dong Aisheng
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Wednesday, October 17, 2018 11:17 PM > Quoting A.s. Dong (2018-10-17 01:56:35) > > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > > Sent: Wednesday, October 17, 2018 5:26 AM Quoting A.s. Dong > > > (2018-10-14 01:07:49) > > > > diff --git a/drivers/clk/imx/scu/clk-divider-scu.c > > > > b/drivers/clk/imx/scu/clk-divider-scu.c > > > > new file mode 100644 > > > > index 0000000..51cb816 > > > > --- /dev/null > > > > +++ b/drivers/clk/imx/scu/clk-divider-scu.c > > > > @@ -0,0 +1,176 @@ > > > > + msg.clk = div->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; > > > > + } > > > > + > > > > + resp = (struct imx_sc_msg_resp_get_clock_rate *)&msg; > > > > > > Does the response get written to the same place that the message is > > > written? If > > > > Yes > > > > > so, it would be better to combine the different structs into a union > > > that's always large enough to handle this? For example, it looks > > > like there are only 16 > > > + 8 bytes for the get_clock_rate structure, but then the response is > > > + returning > > > the rate in 32 bytes. When we cast that here we're now getting an > > > extra 8 bytes of stack, aren't we? Combining the different > > > structures into one bigger structure would alleviate this and avoid the need > for casting. > > > > > > > Good catch. > > Thanks for the professional explanation. > > > > How about do something like below? > > struct req_get_clock_rate { > > u16 resource; > > u8 clk; > > } __packed; > > > > struct resp_get_clock_rate { > > u32 rate; > > } __packed; > > This doesn't need __packed because it's a single u32. > It's a safe writing, but yes, can be removed. > > > > 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; > > > > Yes something like this would be best. And now I wonder if > imx_scu_call_rpc() is doing endianness swapping? Or does it copy data into > these response structures? I'm saying that the u32/16/8 may need to be > __le32/16/8 and then have the proper accessors. > No endianness swapping. It's fixed little endian. SCU protocol isn't aware of these structures. The structures are defined according to SCU protocol definition to make sure each field position and size are correct. Then SCU IPC driver just send and receive them sequentially. Client driver uses the structures to retrieve the responding filed values. We do this like drivers/firmware/ti_sci.h Do you think we still need specify __le32/16/8 for this case? > > > > > > + hdr->size = 3; > > > > + > > > > + msg.rate = rate; > > > > + msg.resource = div->rsrc_id; > > > > + msg.clk = div->clk_type; > > > > + > > > > + ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true); > > > > + if (ret) > > > > + pr_err("%s: failed to set clock rate %ld : ret %d\n", > > > > + clk_hw_get_name(hw), rate, ret); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static const struct clk_ops clk_divider_scu_ops = { > > > > + .recalc_rate = clk_divider_scu_recalc_rate, > > > > + .round_rate = clk_divider_scu_round_rate, > > > > + .set_rate = clk_divider_scu_set_rate, }; > > > > + > > > > +struct clk_hw *imx_clk_register_divider_scu(const char *name, > > > > + const char > > > *parent_name, > > > > + u32 rsrc_id, > > > > + u8 clk_type) { > > > > + struct clk_divider_scu *div; > > > > + struct clk_init_data init; > > > > + struct clk_hw *hw; > > > > + int ret; > > > > + > > > > + div = kzalloc(sizeof(*div), GFP_KERNEL); > > > > + if (!div) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + div->rsrc_id = rsrc_id; > > > > + div->clk_type = clk_type; > > > > + > > > > + init.name = name; > > > > + init.ops = &clk_divider_scu_ops; > > > > + init.flags = CLK_GET_RATE_NOCACHE; > > > > > > Why nocache? Please have a good reason and add a comment indicating > why. > > > > > > > Because on MX8, the clocks are tightly couple with power domain that > > once the power domain is off, the clock status will be lost. > > Making it NOCACHE helps user to retrieve the real clock status from HW > > Instead of using the possible invalid cached rate. > > > > Is that reasonable enough to specify it? > > Yes, you can add a comment like that. Or the clk rate can be restored when the > power domain is enabled again? No restore when power domain is enabled again. That needs driver to do special handling. Can we add the comment in commit message as there're still many other places using this? Regards Dong Aisheng
Quoting A.s. Dong (2018-10-17 08:45:05) > > -----Original Message----- > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > Sent: Wednesday, October 17, 2018 11:17 PM > > Quoting A.s. Dong (2018-10-17 01:56:35) > > > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > > > Sent: Wednesday, October 17, 2018 5:26 AM Quoting A.s. Dong > > > > (2018-10-14 01:07:49) > > > > > > How about do something like below? > > > struct req_get_clock_rate { > > > u16 resource; > > > u8 clk; > > > } __packed; > > > > > > struct resp_get_clock_rate { > > > u32 rate; > > > } __packed; > > > > This doesn't need __packed because it's a single u32. > > > > It's a safe writing, but yes, can be removed. > > > > > > > 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; > > > > > > > Yes something like this would be best. And now I wonder if > > imx_scu_call_rpc() is doing endianness swapping? Or does it copy data into > > these response structures? I'm saying that the u32/16/8 may need to be > > __le32/16/8 and then have the proper accessors. > > > > No endianness swapping. It's fixed little endian. > SCU protocol isn't aware of these structures. The structures are defined > according to SCU protocol definition to make sure each field position and size > are correct. Then SCU IPC driver just send and receive them sequentially. > Client driver uses the structures to retrieve the responding filed values. > > We do this like drivers/firmware/ti_sci.h > Do you think we still need specify __le32/16/8 for this case? Probably, because the CPU in linux could be big endian or little endian. It doesn't hurt to do it right to begin with and then you get the support for free if it's ever used later on. > > > > > > > > > + hdr->size = 3; > > > > > + > > > > > + msg.rate = rate; > > > > > + msg.resource = div->rsrc_id; > > > > > + msg.clk = div->clk_type; > > > > > + > > > > > + ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true); > > > > > + if (ret) > > > > > + pr_err("%s: failed to set clock rate %ld : ret %d\n", > > > > > + clk_hw_get_name(hw), rate, ret); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static const struct clk_ops clk_divider_scu_ops = { > > > > > + .recalc_rate = clk_divider_scu_recalc_rate, > > > > > + .round_rate = clk_divider_scu_round_rate, > > > > > + .set_rate = clk_divider_scu_set_rate, }; > > > > > + > > > > > +struct clk_hw *imx_clk_register_divider_scu(const char *name, > > > > > + const char > > > > *parent_name, > > > > > + u32 rsrc_id, > > > > > + u8 clk_type) { > > > > > + struct clk_divider_scu *div; > > > > > + struct clk_init_data init; > > > > > + struct clk_hw *hw; > > > > > + int ret; > > > > > + > > > > > + div = kzalloc(sizeof(*div), GFP_KERNEL); > > > > > + if (!div) > > > > > + return ERR_PTR(-ENOMEM); > > > > > + > > > > > + div->rsrc_id = rsrc_id; > > > > > + div->clk_type = clk_type; > > > > > + > > > > > + init.name = name; > > > > > + init.ops = &clk_divider_scu_ops; > > > > > + init.flags = CLK_GET_RATE_NOCACHE; > > > > > > > > Why nocache? Please have a good reason and add a comment indicating > > why. > > > > > > > > > > Because on MX8, the clocks are tightly couple with power domain that > > > once the power domain is off, the clock status will be lost. > > > Making it NOCACHE helps user to retrieve the real clock status from HW > > > Instead of using the possible invalid cached rate. > > > > > > Is that reasonable enough to specify it? > > > > Yes, you can add a comment like that. Or the clk rate can be restored when the > > power domain is enabled again? > > No restore when power domain is enabled again. That needs driver to do special > handling. > > Can we add the comment in commit message as there're still many other places > using this? > Sure. Please also add the comment into the code so we don't have to dig it out of commit text.
> -----Original Message----- > From: Stephen Boyd [mailto:sboyd@kernel.org] > Sent: Thursday, October 18, 2018 12:06 AM > Quoting A.s. Dong (2018-10-17 08:45:05) > > > -----Original Message----- > > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > > Sent: Wednesday, October 17, 2018 11:17 PM Quoting A.s. Dong > > > (2018-10-17 01:56:35) > > > > > From: Stephen Boyd [mailto:sboyd@kernel.org] > > > > > Sent: Wednesday, October 17, 2018 5:26 AM Quoting A.s. Dong > > > > > (2018-10-14 01:07:49) > > > > > > > > How about do something like below? > > > > struct req_get_clock_rate { > > > > u16 resource; > > > > u8 clk; > > > > } __packed; > > > > > > > > struct resp_get_clock_rate { > > > > u32 rate; > > > > } __packed; > > > > > > This doesn't need __packed because it's a single u32. > > > > > > > It's a safe writing, but yes, can be removed. > > > > > > > > > > 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; > > > > > > > > > > Yes something like this would be best. And now I wonder if > > > imx_scu_call_rpc() is doing endianness swapping? Or does it copy > > > data into these response structures? I'm saying that the u32/16/8 > > > may need to be > > > __le32/16/8 and then have the proper accessors. > > > > > > > No endianness swapping. It's fixed little endian. > > SCU protocol isn't aware of these structures. The structures are > > defined according to SCU protocol definition to make sure each field > > position and size are correct. Then SCU IPC driver just send and receive them > sequentially. > > Client driver uses the structures to retrieve the responding filed values. > > > > We do this like drivers/firmware/ti_sci.h Do you think we still need > > specify __le32/16/8 for this case? > > Probably, because the CPU in linux could be big endian or little endian. > It doesn't hurt to do it right to begin with and then you get the support for free > if it's ever used later on. > All right for me. Thanks for the suggestion. Regards Dong Aisheng