lkml.org 
[lkml]   [2019]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288
From
Date
hi,

在 2019/4/10 下午11:25, Doug Anderson 写道:
> Hi,
>
> On Tue, Apr 9, 2019 at 11:42 PM elaine.zhang <zhangqing@rock-chips.com> wrote:
>> hi,
>>
>> 在 2019/4/10 上午4:47, Douglas Anderson 写道:
>>> Most rk3288-based boards are derived from the EVB and thus use a PWM
>>> regulator for the logic rail. However, most rk3288-based boards don't
>>> specify the PWM regulator in their device tree. We'll deal with that
>>> by making it critical.
>>>
>>> NOTE: it's important to make it critical and not just IGNORE_UNUSED
>>> because all PWMs in the system share the same clock. We don't want
>>> another PWM user to turn the clock on and off and kill the logic rail.
>>>
>>> This change is in preparation for actually having the PWMs in the
>>> rk3288 device tree actually point to the proper PWM clock. Up until
>>> now they've all pointed to the clock for the old IP block and they've
>>> all worked due to the fact that rkpwm was IGNORE_UNUSED and that the
>>> clock rates for both clocks were the same.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> drivers/clk/rockchip/clk-rk3288.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
>>> index 06287810474e..c3321eade23e 100644
>>> --- a/drivers/clk/rockchip/clk-rk3288.c
>>> +++ b/drivers/clk/rockchip/clk-rk3288.c
>>> @@ -697,7 +697,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
>>> GATE(PCLK_TZPC, "pclk_tzpc", "pclk_cpu", 0, RK3288_CLKGATE_CON(11), 3, GFLAGS),
>>> GATE(PCLK_UART2, "pclk_uart2", "pclk_cpu", 0, RK3288_CLKGATE_CON(11), 9, GFLAGS),
>>> GATE(PCLK_EFUSE256, "pclk_efuse_256", "pclk_cpu", 0, RK3288_CLKGATE_CON(11), 10, GFLAGS),
>>> - GATE(PCLK_RKPWM, "pclk_rkpwm", "pclk_cpu", CLK_IGNORE_UNUSED, RK3288_CLKGATE_CON(11), 11, GFLAGS),
>>> + GATE(PCLK_RKPWM, "pclk_rkpwm", "pclk_cpu", 0, RK3288_CLKGATE_CON(11), 11, GFLAGS),
>>>
>>> /* ddrctrl [DDR Controller PHY clock] gates */
>>> GATE(0, "nclk_ddrupctl0", "ddrphy", CLK_IGNORE_UNUSED, RK3288_CLKGATE_CON(11), 4, GFLAGS),
>>> @@ -837,6 +837,7 @@ static const char *const rk3288_critical_clocks[] __initconst = {
>>> "pclk_alive_niu",
>>> "pclk_pd_pmu",
>>> "pclk_pmu_niu",
>>> + "pclk_rkpwm",
>> pwm have device node, can enable and disable it in the pwm drivers.
>>
>> pwm regulator use pwm node as:
>>
>> pwms = <&pwm2 0 25000 1>
>>
>> when set Logic voltage:
>>
>> pwm_regulator_set_voltage()
>>
>> --> pwm_apply_state()
>>
>> -->clk_enable()
>>
>> -->pwm_enable()
>>
>> -->pwm_config()
>>
>> -->pinctrl_select()
>>
>> --....
>>
>> For mark pclk_rkpwm as critical,do you have any questions, or provides
>> some log or more information.
> Right, if we actually specify the PWM used for the PWM regulator in
> the device tree then there is no need to mark it as a critical clock.
> In fact rk3288-veyron devices boot absolutely fine without marking
> this clock as critical. Actually, it seems like the way the PWM
> framework works (IIRC it was designed this way specifically to support
> PWM regulators) is that even just specifying that pwm1 is "okay" is
> enough to keep the clock on even if the PWM regulator isn't specified.
>
> ...however...
>
> Take a look at, for instance, the rk3288-evb device tree file.
> Nowhere in there does it specify that the PWM used for the PWM
> regulator should be on. Presumably that means that if we don't mark
> the clock as critical then rk3288-evb will fail to boot. That's easy
> for me to fix since I have the rk3288-evb schematics, but what about
> other rk3288 boards? We could make educated guesses about each of
> them and/or fix things are we hear about breakages.
>
> ...but...
>
> All the above would only be worth doing if we thought someone would
> get some benefit out of it. I'd bet that pretty much all rk3288-based
> boards use a PWM regulator. Thus, in reality, everyone will want the
> rkpwm clock on all the time anyway. In that case going through all
> that extra work / potentially breaking other boards doesn't seem worth
> it. Just mark the clock as critical.

I have no problem with changing it like this, but I think it is better
to modify dts:

vdd_log: vdd-log {
        compatible = "pwm-regulator";
        rockchip,pwm_id = <2>; //for rk uboot
        rockchip,pwm_voltage = <900000>; // for rk uboot
        pwms = <&pwm2 0 25000 1>;
        regulator-name = "vdd_log";
        regulator-min-microvolt = <800000>;//hw logic min voltage
        regulator-max-microvolt = <1400000>;//hw logic max voltage
        regulator-always-on;
        regulator-boot-on;
    };

Maybe we did not push the modification of this part in rk3288-evb, I
will push to deal with this.(rk3229-evb.dts and rk3399 has been already
pushed)
>
>
>
>
> -Doug
>
>
>


\
 
 \ /
  Last update: 2019-04-11 05:43    [W:0.095 / U:2.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site