[V4,05/11] clk: imx: scu: add scu clock gate

Commit Message

Aisheng Dong Oct. 14, 2018, 8:07 a.m. UTC

Add scu based clock gate.

Cc: Shawn Guo <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Michael Turquette <[e-mail]>
Signed-off-by: Dong Aisheng <[e-mail]>
---
ChangeLog:
v3->v4:
 * scu headfile path update
v2->v3:
 * structure names and api usage update
v1->v2:
 * move SCU clock API implementation into driver
---
 drivers/clk/imx/scu/Makefile       |   3 +-
 drivers/clk/imx/scu/clk-gate-scu.c | 222 +++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/scu/clk-scu.h      |  23 ++++
 3 files changed, 247 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/imx/scu/clk-gate-scu.c

Comments

Sascha Hauer Oct. 15, 2018, 7:32 a.m. UTC | #1

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

Aisheng Dong Oct. 15, 2018, 9:17 a.m. UTC | #2

> -----Original Message-----
> From: Sascha Hauer [mailto:[e-mail]]
> Sent: Monday, October 15, 2018 3:32 PM
> To: A.s. Dong <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>; [e-mail]; Fabio Estevam
> <[e-mail]>; [e-mail];
> [e-mail]
> 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&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7Ca8
> dc9d70c490491d116b08d6327050dc%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636751855312147344&amp;sdata=RkcPJ0TNLnFP%2BBbTh
> VrApN3Z%2B7MRR8%2FxJO1TSnqBv40%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Sascha Hauer Oct. 15, 2018, 9:53 a.m. UTC | #3

On Mon, Oct 15, 2018 at 09:17:14AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:[e-mail]]
> > Sent: Monday, October 15, 2018 3:32 PM
> > To: A.s. Dong <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > dl-linux-imx <[e-mail]>; [e-mail]; Fabio Estevam
> > <[e-mail]>; [e-mail];
> > [e-mail]
> > 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

Aisheng Dong Oct. 15, 2018, 3:30 p.m. UTC | #4

> -----Original Message-----
> From: Sascha Hauer [mailto:[e-mail]]
> Sent: Monday, October 15, 2018 5:54 PM
> To: A.s. Dong <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>; [e-mail]; Fabio Estevam
> <[e-mail]>; [e-mail];
> [e-mail]
> 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:[e-mail]]
> > > Sent: Monday, October 15, 2018 3:32 PM
> > > To: A.s. Dong <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; dl-linux-imx <[e-mail]>;
> > > [e-mail]; Fabio Estevam <[e-mail]>;
> > > [e-mail]; [e-mail]
> > > 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&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7C11
> c8f62f729749de7b8708d63284183f%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636751940255777834&amp;sdata=tsICGc0z9Rcfgu4YcKWy5
> hu8sFHXeRYSy3lP8CjtxO8%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Stephen Boyd Oct. 16, 2018, 9:18 p.m. UTC | #5

Quoting A.s. Dong (2018-10-15 08:30:45)
> > -----Original Message-----
> > From: Sascha Hauer [mailto:[e-mail]]
> > Sent: Monday, October 15, 2018 5:54 PM
> > To: A.s. Dong <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > dl-linux-imx <[e-mail]>; [e-mail]; Fabio Estevam
> > <[e-mail]>; [e-mail];
> > [e-mail]
> > 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:[e-mail]]
> > > > Sent: Monday, October 15, 2018 3:32 PM
> > > > To: A.s. Dong <[e-mail]>
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; dl-linux-imx <[e-mail]>;
> > > > [e-mail]; Fabio Estevam <[e-mail]>;
> > > > [e-mail]; [e-mail]
> > > > 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.

Aisheng Dong Oct. 17, 2018, 7:28 a.m. UTC | #6

> From: Stephen Boyd [mailto:[e-mail]]
> 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

Patch

diff --git a/drivers/clk/imx/scu/Makefile b/drivers/clk/imx/scu/Makefile
index 9e7f4aa..2abed17 100644
--- a/drivers/clk/imx/scu/Makefile
+++ b/drivers/clk/imx/scu/Makefile
@@ -3,4 +3,5 @@ 
 obj-$(CONFIG_MXC_CLK_SCU) += \
 	clk-scu.o \
 	clk-divider-scu.o \
-	clk-divider-gpr-scu.o
+	clk-divider-gpr-scu.o \
+	clk-gate-scu.o
diff --git a/drivers/clk/imx/scu/clk-gate-scu.c b/drivers/clk/imx/scu/clk-gate-scu.c
new file mode 100644
index 0000000..d86d2ee
--- /dev/null
+++ b/drivers/clk/imx/scu/clk-gate-scu.c
@@ -0,0 +1,222 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *	Dong Aisheng <[e-mail]>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+
+#include "clk-scu.h"
+
+/*
+ * basic gatable clock which can gate and ungate it's output
+ *
+ * Traits of this clock:
+ * prepare - clk_(un)prepare only ensures parent is (un)prepared
+ * enable - clk_enable and clk_disable are functional & control gating
+ * rate - inherits rate from parent.  No clk_set_rate support
+ * parent - fixed parent.  No clk_set_parent support
+ */
+
+#define CLK_GATE_SCU_LPCG_MASK		0x3
+#define CLK_GATE_SCU_LPCG_HW_SEL	BIT(0)
+#define CLK_GATE_SCU_LPCG_SW_SEL	BIT(1)
+
+struct clk_gate_scu {
+	struct clk_hw	hw;
+	void __iomem	*reg;
+	u8	bit_idx;
+	bool	hw_gate;
+	u32	rsrc_id;
+	u8	clk_type;
+};
+
+#define to_clk_gate_scu(_hw) container_of(_hw, struct clk_gate_scu, hw)
+
+/* SCU Clock Protocol definitions */
+struct imx_sc_msg_req_clock_enable {
+	struct imx_sc_rpc_msg hdr;
+	u16	resource;
+	u8	clk;
+	u8	enable;
+	u8	autog;
+} __packed;
+
+/* 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);
+	}
+
+	return 0;
+}
+
+static void clk_gate_scu_disable(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);
+		writel(reg, gate->reg);
+	}
+}
+
+static int sc_pm_clock_enable(struct imx_sc_ipc *ipc, u32 resource,
+				   u8 clk, bool enable, bool autog)
+{
+	struct imx_sc_msg_req_clock_enable msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
+	hdr->func = (uint8_t)IMX_SC_PM_FUNC_CLOCK_ENABLE;
+	hdr->size = 3;
+
+	msg.resource = resource;
+	msg.clk = clk;
+	msg.enable = (uint8_t)enable;
+	msg.autog = (uint8_t)autog;
+
+	return imx_scu_call_rpc(ccm_ipc_handle, &msg, true);
+}
+
+static int clk_gate_scu_prepare(struct clk_hw *hw)
+{
+	struct clk_gate_scu *gate = to_clk_gate_scu(hw);
+	int ret;
+
+	/* Enable the clock at the DSC slice level */
+	ret = sc_pm_clock_enable(ccm_ipc_handle, gate->rsrc_id,
+				 gate->clk_type, true, gate->hw_gate);
+	if (ret)
+		pr_err("%s: clk prepare failed %d\n", clk_hw_get_name(hw), ret);
+
+	return ret;
+}
+
+static void clk_gate_scu_unprepare(struct clk_hw *hw)
+{
+	struct clk_gate_scu *gate = to_clk_gate_scu(hw);
+	int ret;
+
+	ret = sc_pm_clock_enable(ccm_ipc_handle, gate->rsrc_id,
+				     gate->clk_type, false, false);
+	if (ret)
+		pr_err("%s: clk unprepare failed %d\n", clk_hw_get_name(hw),
+			ret);
+}
+
+static const struct clk_ops clk_gate_scu_ops = {
+	.prepare = clk_gate_scu_prepare,
+	.unprepare = clk_gate_scu_unprepare,
+	.enable = clk_gate_scu_enable,
+	.disable = clk_gate_scu_disable,
+};
+
+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);
+		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);
+		kfree(gate);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
+
+static const struct clk_ops clk_gate2_scu_ops = {
+	.enable = clk_gate_scu_enable,
+	.disable = clk_gate_scu_disable,
+};
+
+struct clk_hw *clk_register_gate2_scu(const char *name, const char *parent_name,
+				      unsigned long flags, 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->reg = ioremap((phys_addr_t)reg, SZ_64K);
+	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_gate2_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);
+		kfree(gate);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
diff --git a/drivers/clk/imx/scu/clk-scu.h b/drivers/clk/imx/scu/clk-scu.h
index f0796f3..4ff7837 100644
--- a/drivers/clk/imx/scu/clk-scu.h
+++ b/drivers/clk/imx/scu/clk-scu.h
@@ -36,4 +36,27 @@  static inline struct clk_hw *imx_clk_divider2_scu(const char *name,
 struct clk_hw *imx_clk_divider_gpr_scu(const char *name, const char *parent_name,
 				u32 rsrc_id, u8 gpr_id);
 
+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_hw *clk_register_gate2_scu(const char *name, const char *parent_name,
+				unsigned long flags, void __iomem *reg,
+				u8 bit_idx, bool hw_gate);
+
+static inline struct clk_hw *imx_clk_gate_scu(const char *name, const char *parent,
+				u32 rsrc_id, u8 clk_type,
+				void __iomem *reg, u8 bit_idx, bool hw_gate)
+{
+	return clk_register_gate_scu(name, parent, CLK_SET_RATE_PARENT,
+				     rsrc_id, clk_type, reg, bit_idx, hw_gate);
+}
+
+static inline struct clk_hw *imx_clk_gate2_scu(const char *name, const char *parent,
+				void __iomem *reg, u8 bit_idx, bool hw_gate)
+{
+	return clk_register_gate2_scu(name, parent, 0, reg, bit_idx, hw_gate);
+}
+
 #endif