[PATCH 3/8] clk: mxs: add clock support for imx28

[Sascha Hauer]

On Wed, Apr 25, 2012 at 04:02:45PM +0800, Shawn Guo wrote:
> On Wed, Apr 25, 2012 at 09:17:47AM +0200, Sascha Hauer wrote:
> > Hi Shawn,
> > 
> > I realized that you have no register lock which means that you rely on
> > the locking of the clock framework. This may not be sufficient, see the
> > following example:
> > 
> > > +	mxs_clk_div("ssp0_div", "ssp0_sel", SSP0, 0, 9, 29);
> > > +	clk = mxs_clk_gate("ssp0", "ssp0_div", SSP0, 31);
> > 
> > You have both a divider and a gate in the same register here. The gate
> > implements clk_enable which is protected by a spinlock in the clock
> > framework. The divider implements clk_set_rate which is protected by a
> > mutex in the clock framework. This means that during a read-modify-write
> > operation for a rate change the clk_enable call can come in between.
> 
> We have SET and CLR register on mxs clocks, so do not really have to
> go through read-modify-write sequence.

You have the SET and CLR registers but you don't use them. You could use
SET/CLR in the gate, but even then:

- clk_set_rate starts and reads the divider register
- clk_enable comes in and writes to SET/CLR for the same register
- clk_set_rate continues and writes to the divider register not knowing
  that it has been modified in between.

You could try and clear all divider bits in the divider using the CLR
register and then set the wanted bits using the SET register, but this
would mean writing reserved values to the divider in between.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |