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
