| Message ID | 20190409204707.150347-3-dianders@chromium.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
| Series | rockchip: A few clock cleanups for rk3288 | expand |
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. > }; > > static void __iomem *rk3288_cru_base;
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
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 > > >
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
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
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 --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;
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(-)