diff mbox series

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

Message ID 20190409204707.150347-3-dianders@chromium.org (mailing list archive)
State New, archived
Headers show
Series rockchip: A few clock cleanups for rk3288 | expand

Commit Message

Doug Anderson April 9, 2019, 8:47 p.m. UTC
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(-)

Comments

Doug Anderson April 10, 2019, 3:25 p.m. UTC | #1
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.




-Doug
Elaine Zhang April 11, 2019, 3:42 a.m. UTC | #2
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
>
>
>
Doug Anderson April 11, 2019, 2:42 p.m. UTC | #3
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
Heiko Stuebner April 11, 2019, 7:20 p.m. UTC | #4
Hi Doug, Elaine,

Am Donnerstag, 11. April 2019, 16:42:23 CEST schrieb Doug Anderson:
> On Wed, Apr 10, 2019 at 8:42 PM elaine.zhang <zhangqing@rock-chips.com> wrote:
> > 在 2019/4/10 下午11:25, Doug Anderson 写道:
> > > On Tue, Apr 9, 2019 at 11:42 PM elaine.zhang <zhangqing@rock-chips.com> wrote:
> > >> 在 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.

So I've skimmed through a number of rk3288 device-schematics and
at least there pwm-based logic-regulator is only part of some of the
rk808-based designs. All the act8846-based boards use one of its
regulator-outputs for the logic supply. So this matters for at least the
veyron-boards and the rk3288-evb ... but not the phycore.

But it also is part of a vital system for veyron and they won't really like
if somewhere during probe the pwm-clock gets turned off ;-) .

So I tend to agree with Doug and will just apply the make-critical patch.
If Stephen's "handoff" clock-type [critical until a driver claims it]
materializes some-time, we can switch to that.


Heiko
Heiko Stuebner April 11, 2019, 7:27 p.m. UTC | #5
Am Dienstag, 9. April 2019, 22:47:06 CEST schrieb 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>

applied for 5.2
I've added a comment line describing the pwm-reg reason.


Heiko
diff mbox series

Patch

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",
 };
 
 static void __iomem *rk3288_cru_base;