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

[addy ke]

On 2014/9/15 16:29, Max Schwarz wrote:
> 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.
> 
> 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.
> 
sure, we can.
I have comfirmed on rk3288-pinky 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
> 
> 
>