[PATCH RFC] i2c: rk3x: handle dynamic clock rate changes correctly

[Heiko Stübner]

Am Montag, 15. September 2014, 10:29:21 schrieb Max Schwarz:
> Hi Addy,
> 
> Am Montag, 15. September 2014, 11:11:24 schrieb addy ke:
> > On 2014/9/13 19:26, Max Schwarz wrote:
> > > On Wednesday 10 September 2014 at 16:16:10, Addy wrote:
> > >>> @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device
> > >>> *pdev)
> > >>> 
> > >>>        return ret;
> > >>>    
> > >>>    }
> > >>> 
> > >>> +    i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb;
> > >>> +    ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb);
> > >>> +    if (ret != 0) {
> > >>> +        dev_err(&pdev->dev, "Unable to register clock notifier\n");
> > >>> +        goto err_clk;
> > >>> +    }
> > >>> +
> > >>> +    i2c->clk_freq = clk_get_rate(i2c->clk);
> > >>> +    rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> > >> 
> > >> I have tested on rk3288-pinky board:
> > >> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate()
> > >> to write div value to CLKDIV register. If not, system will crash.
> > >> 
> > >> So we need call clk_enable() before rk3x_i2c_set_scl_rate().
> > > 
> > > Interesting. It works without problems on RK3188. Do you know if the
> > > clock
> > > has to be kept enabled for some time? Or is it okay to do it like this?
> > > 
> > > clk_enable(i2c->clk);
> > > rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> > > clk_disable(i2c->clk);
> > > 
> > > Cheers,
> > > 
> > >   Max
> > 
> > Before write value to i2c register, it is necessary to enable i2c's clock
> > and parent clock!
> 
> The parent clock is enabled by the clk framework when we enable the child
> clock. No need to do it explicitly.

Correct.

To make it even more explicit, kernel/driver code should _never_ rely on a 
clock magically just being on, but instead must handle it properly.

CLK_IGNORE_UNUSED and friends are just an intermediate measures when parts of 
the clock tree are still unhandled.


Heiko

> My question was whether we can immediately *disable* the clock after writing
> the divider register. Could you test the above snippet in the i2c probe
> function? I don't have access to a RK3288 board.
>
> > In the linux-rockchip branch code, the i2c's clock and parent clock are
> > enabled by default. So there are no problems on it.
> 
> Yes, I saw that. I still think it's nice that we disable the clock when we
> don't need it. I'd like to keep that if possible.
> 
> > But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed.
> > (Maybe its reference count drops to 0 and it is disabled by clk driver)
> 
> Yes, that's probably because no one is using childs of the pclk_peri clock
> yet.
> 
> Cheers,
>   Max