| Message ID | 1603738248-8193-1-git-send-email-abel.vesa@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: imx: gate2: Fix the is_enabled op | expand |
Hi Abel, On Mon, Oct 26, 2020 at 08:50:48PM +0200, Abel Vesa wrote: > The clock is considered to be enabled only if the controlling bits > match the cgr_val mask. Also make sure the is_enabled returns the > correct vaule by locking the access to the register. > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock") > --- > drivers/clk/imx/clk-gate2.c | 60 ++++++++++++++++++++------------------------- > drivers/clk/imx/clk.h | 8 ++---- > 2 files changed, 29 insertions(+), 39 deletions(-) > > diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c > index 7eed708..f320bd2b 100644 > --- a/drivers/clk/imx/clk-gate2.c > +++ b/drivers/clk/imx/clk-gate2.c > @@ -37,10 +37,22 @@ struct clk_gate2 { > > #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw) > > +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable) > +{ > + struct clk_gate2 *gate = to_clk_gate2(hw); > + u32 reg; > + > + reg = readl(gate->reg); > + if (enable) > + reg |= gate->cgr_val << gate->bit_idx; > + else > + reg &= ~(gate->cgr_val << gate->bit_idx); Shouldn't this be: reg &= ~(3 << gate->bit_idx); if (enable) reg |= gate->cgr_val << gate->bit_idx; At least that's how it was without this patch and that's how it makes sense to me with cgr_val != 3. Sascha
On 20-10-28 09:24:12, Sascha Hauer wrote: > Hi Abel, > > On Mon, Oct 26, 2020 at 08:50:48PM +0200, Abel Vesa wrote: > > The clock is considered to be enabled only if the controlling bits > > match the cgr_val mask. Also make sure the is_enabled returns the > > correct vaule by locking the access to the register. > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock") > > --- > > drivers/clk/imx/clk-gate2.c | 60 ++++++++++++++++++++------------------------- > > drivers/clk/imx/clk.h | 8 ++---- > > 2 files changed, 29 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c > > index 7eed708..f320bd2b 100644 > > --- a/drivers/clk/imx/clk-gate2.c > > +++ b/drivers/clk/imx/clk-gate2.c > > @@ -37,10 +37,22 @@ struct clk_gate2 { > > > > #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw) > > > > +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable) > > +{ > > + struct clk_gate2 *gate = to_clk_gate2(hw); > > + u32 reg; > > + > > + reg = readl(gate->reg); > > + if (enable) > > + reg |= gate->cgr_val << gate->bit_idx; > > + else > > + reg &= ~(gate->cgr_val << gate->bit_idx); > > Shouldn't this be: > > reg &= ~(3 << gate->bit_idx); > if (enable) > reg |= gate->cgr_val << gate->bit_idx; > > At least that's how it was without this patch and that's how it makes > sense to me with cgr_val != 3. > Well, that's the actual problem. The value 3 forces all the clocks that register with this clock type to have 2 bits for controlling the gate. My patch (though now I think I should split it into 2 separate patches) allows two HW gates to be controlled by as many bits necessary. For example, there could be multiple HW gates that are controled by the same bit. By passing the cgr_val when registering the clocks you can specify how many bits (as a mask) control all those HW gates that share their control bits. > Sascha >
On Wed, Oct 28, 2020 at 11:50:57AM +0200, Abel Vesa wrote: > On 20-10-28 09:24:12, Sascha Hauer wrote: > > Hi Abel, > > > > On Mon, Oct 26, 2020 at 08:50:48PM +0200, Abel Vesa wrote: > > > The clock is considered to be enabled only if the controlling bits > > > match the cgr_val mask. Also make sure the is_enabled returns the > > > correct vaule by locking the access to the register. > > > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > > Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock") > > > --- > > > drivers/clk/imx/clk-gate2.c | 60 ++++++++++++++++++++------------------------- > > > drivers/clk/imx/clk.h | 8 ++---- > > > 2 files changed, 29 insertions(+), 39 deletions(-) > > > > > > diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c > > > index 7eed708..f320bd2b 100644 > > > --- a/drivers/clk/imx/clk-gate2.c > > > +++ b/drivers/clk/imx/clk-gate2.c > > > @@ -37,10 +37,22 @@ struct clk_gate2 { > > > > > > #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw) > > > > > > +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable) > > > +{ > > > + struct clk_gate2 *gate = to_clk_gate2(hw); > > > + u32 reg; > > > + > > > + reg = readl(gate->reg); > > > + if (enable) > > > + reg |= gate->cgr_val << gate->bit_idx; > > > + else > > > + reg &= ~(gate->cgr_val << gate->bit_idx); > > > > Shouldn't this be: > > > > reg &= ~(3 << gate->bit_idx); > > if (enable) > > reg |= gate->cgr_val << gate->bit_idx; > > > > At least that's how it was without this patch and that's how it makes > > sense to me with cgr_val != 3. > > > > Well, that's the actual problem. The value 3 forces all the clocks > that register with this clock type to have 2 bits for controlling the gate. > > My patch (though now I think I should split it into 2 separate patches) allows > two HW gates to be controlled by as many bits necessary. For example, there > could be multiple HW gates that are controled by the same bit. By passing > the cgr_val when registering the clocks you can specify how many bits (as a mask) > control all those HW gates that share their control bits. cgr_val is not a mask, it's a value that shall be written to the two bits to enable the clock. cgr_val could also be 0b10, see imx_clk_gate2_cgr(). Sascha
On 20-10-28 11:15:52, Sascha Hauer wrote: > On Wed, Oct 28, 2020 at 11:50:57AM +0200, Abel Vesa wrote: > > On 20-10-28 09:24:12, Sascha Hauer wrote: > > > Hi Abel, > > > > > > On Mon, Oct 26, 2020 at 08:50:48PM +0200, Abel Vesa wrote: > > > > The clock is considered to be enabled only if the controlling bits > > > > match the cgr_val mask. Also make sure the is_enabled returns the > > > > correct vaule by locking the access to the register. > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > > > Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock") > > > > --- > > > > drivers/clk/imx/clk-gate2.c | 60 ++++++++++++++++++++------------------------- > > > > drivers/clk/imx/clk.h | 8 ++---- > > > > 2 files changed, 29 insertions(+), 39 deletions(-) > > > > > > > > diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c > > > > index 7eed708..f320bd2b 100644 > > > > --- a/drivers/clk/imx/clk-gate2.c > > > > +++ b/drivers/clk/imx/clk-gate2.c > > > > @@ -37,10 +37,22 @@ struct clk_gate2 { > > > > > > > > #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw) > > > > > > > > +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable) > > > > +{ > > > > + struct clk_gate2 *gate = to_clk_gate2(hw); > > > > + u32 reg; > > > > + > > > > + reg = readl(gate->reg); > > > > + if (enable) > > > > + reg |= gate->cgr_val << gate->bit_idx; > > > > + else > > > > + reg &= ~(gate->cgr_val << gate->bit_idx); > > > > > > Shouldn't this be: > > > > > > reg &= ~(3 << gate->bit_idx); > > > if (enable) > > > reg |= gate->cgr_val << gate->bit_idx; > > > > > > At least that's how it was without this patch and that's how it makes > > > sense to me with cgr_val != 3. > > > > > > > Well, that's the actual problem. The value 3 forces all the clocks > > that register with this clock type to have 2 bits for controlling the gate. > > > > My patch (though now I think I should split it into 2 separate patches) allows > > two HW gates to be controlled by as many bits necessary. For example, there > > could be multiple HW gates that are controled by the same bit. By passing > > the cgr_val when registering the clocks you can specify how many bits (as a mask) > > control all those HW gates that share their control bits. > > cgr_val is not a mask, it's a value that shall be written to the two > bits to enable the clock. cgr_val could also be 0b10, see imx_clk_gate2_cgr(). > Oh, I see your point now. Maybe we should add another member called cgr_mask. That should fix the issue. > Sascha > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&data=04%7C01%7Cabel.vesa%40nxp.com%7C03a2a0c430384a43db0708d87b2a762c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637394769581294045%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=HiJ25Kyi3TS0XJx24G2VExgTntmCT2eKFbzBUe6GBH0%3D&reserved=0 | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c index 7eed708..f320bd2b 100644 --- a/drivers/clk/imx/clk-gate2.c +++ b/drivers/clk/imx/clk-gate2.c @@ -37,10 +37,22 @@ struct clk_gate2 { #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw) +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable) +{ + struct clk_gate2 *gate = to_clk_gate2(hw); + u32 reg; + + reg = readl(gate->reg); + if (enable) + reg |= gate->cgr_val << gate->bit_idx; + else + reg &= ~(gate->cgr_val << gate->bit_idx); + writel(reg, gate->reg); +} + static int clk_gate2_enable(struct clk_hw *hw) { struct clk_gate2 *gate = to_clk_gate2(hw); - u32 reg; unsigned long flags; int ret = 0; @@ -49,15 +61,7 @@ static int clk_gate2_enable(struct clk_hw *hw) if (gate->share_count && (*gate->share_count)++ > 0) goto out; - if (gate->flags & IMX_CLK_GATE2_SINGLE_BIT) { - ret = clk_gate_ops.enable(hw); - } else { - reg = readl(gate->reg); - reg &= ~(3 << gate->bit_idx); - reg |= gate->cgr_val << gate->bit_idx; - writel(reg, gate->reg); - } - + clk_gate2_do_shared_clks(hw, true); out: spin_unlock_irqrestore(gate->lock, flags); @@ -67,7 +71,6 @@ static int clk_gate2_enable(struct clk_hw *hw) static void clk_gate2_disable(struct clk_hw *hw) { struct clk_gate2 *gate = to_clk_gate2(hw); - u32 reg; unsigned long flags; spin_lock_irqsave(gate->lock, flags); @@ -79,23 +82,16 @@ static void clk_gate2_disable(struct clk_hw *hw) goto out; } - if (gate->flags & IMX_CLK_GATE2_SINGLE_BIT) { - clk_gate_ops.disable(hw); - } else { - reg = readl(gate->reg); - reg &= ~(3 << gate->bit_idx); - writel(reg, gate->reg); - } - + clk_gate2_do_shared_clks(hw, false); out: spin_unlock_irqrestore(gate->lock, flags); } -static int clk_gate2_reg_is_enabled(void __iomem *reg, u8 bit_idx) +static int clk_gate2_reg_is_enabled(void __iomem *reg, u8 bit_idx, u8 cgr_val) { u32 val = readl(reg); - if (((val >> bit_idx) & 1) == 1) + if (((val >> bit_idx) & cgr_val) == cgr_val) return 1; return 0; @@ -104,29 +100,27 @@ static int clk_gate2_reg_is_enabled(void __iomem *reg, u8 bit_idx) static int clk_gate2_is_enabled(struct clk_hw *hw) { struct clk_gate2 *gate = to_clk_gate2(hw); + unsigned long flags; + int ret; - if (gate->flags & IMX_CLK_GATE2_SINGLE_BIT) - return clk_gate_ops.is_enabled(hw); + spin_lock_irqsave(gate->lock, flags); - return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx); + ret = clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx, gate->cgr_val); + + spin_unlock_irqrestore(gate->lock, flags); + + return ret; } static void clk_gate2_disable_unused(struct clk_hw *hw) { struct clk_gate2 *gate = to_clk_gate2(hw); unsigned long flags; - u32 reg; - - if (gate->flags & IMX_CLK_GATE2_SINGLE_BIT) - return; spin_lock_irqsave(gate->lock, flags); - if (!gate->share_count || *gate->share_count == 0) { - reg = readl(gate->reg); - reg &= ~(3 << gate->bit_idx); - writel(reg, gate->reg); - } + if (!gate->share_count || *gate->share_count == 0) + clk_gate2_do_shared_clks(hw, false); spin_unlock_irqrestore(gate->lock, flags); } diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index 3b796b3..069b07d 100644 --- a/drivers/clk/imx/clk.h +++ b/drivers/clk/imx/clk.h @@ -6,8 +6,6 @@ #include <linux/spinlock.h> #include <linux/clk-provider.h> -#define IMX_CLK_GATE2_SINGLE_BIT 1 - extern spinlock_t imx_ccm_lock; void imx_check_clocks(struct clk *clks[], unsigned int count); @@ -383,10 +381,8 @@ static inline struct clk_hw *imx_dev_clk_hw_gate_shared(struct device *dev, void __iomem *reg, u8 shift, unsigned int *share_count) { - return clk_hw_register_gate2(NULL, name, parent, CLK_SET_RATE_PARENT | - CLK_OPS_PARENT_ENABLE, reg, shift, 0x3, - IMX_CLK_GATE2_SINGLE_BIT, - &imx_ccm_lock, share_count); + return clk_hw_register_gate2(dev, name, parent, CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE, reg, + shift, 0x1, 0, &imx_ccm_lock, share_count); } static inline struct clk *imx_clk_gate2_cgr(const char *name,
The clock is considered to be enabled only if the controlling bits match the cgr_val mask. Also make sure the is_enabled returns the correct vaule by locking the access to the register. Signed-off-by: Abel Vesa <abel.vesa@nxp.com> Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock") --- drivers/clk/imx/clk-gate2.c | 60 ++++++++++++++++++++------------------------- drivers/clk/imx/clk.h | 8 ++---- 2 files changed, 29 insertions(+), 39 deletions(-)