Re: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288

[Doug Anderson <dianders@chromium.org>]

Hi,

On Wed, Apr 10, 2019 at 8:42 PM elaine.zhang <zhangqing@rock-chips.com> wrote:
>
> 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)

Heiko: do you have advice for how you'd like this to proceed?  We
could certainly land patch #3 in this series without patch #2 and then
pick up the pieces as we find boards that no longer boot.  As I've
argued I'm not sure that's worth the effort, but I'll leave it up to
you.


-Doug