Re: [PATCH] clk: imx: gate2: Fix the is_enabled op

[Abel Vesa]

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@xxxxxxx>
> > 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
>