* [PATCH 0/3] rockchip: A few clock cleanups for rk3288 @ 2019-04-09 20:47 Douglas Anderson 2019-04-09 20:47 ` [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" Douglas Anderson ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Douglas Anderson @ 2019-04-09 20:47 UTC (permalink / raw) To: Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, Elaine Zhang, linux-clk, Douglas Anderson, devicetree, linux-kernel, Rob Herring, Mark Rutland, linux-arm-kernel This series contains a few misc clock cleanups for Rockchip rk3288 found by comparing to what's in the downstream Chrome OS 3.14 kernel. NOTES: * The PWM patches could go in separately from the revert but that would cause a merge conflict which is why they're together in a series. * Having the PWM marked as a critical clock _definitely_ needs to land before switching the clock in the device tree. Caesar Wang (1): ARM: dts: rockchip: fix PWM clock found on RK3288 Socs Douglas Anderson (2): Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" clk: rockchip: Make rkpwm a critical clock on rk3288 arch/arm/boot/dts/rk3288.dtsi | 8 ++++---- drivers/clk/rockchip/clk-rk3288.c | 17 ++++++----------- 2 files changed, 10 insertions(+), 15 deletions(-) -- 2.21.0.392.gf8f6787159e-goog ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" 2019-04-09 20:47 [PATCH 0/3] rockchip: A few clock cleanups for rk3288 Douglas Anderson @ 2019-04-09 20:47 ` Douglas Anderson 2019-04-10 6:23 ` elaine.zhang 2019-04-09 20:47 ` [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 Douglas Anderson 2019-04-09 20:47 ` [PATCH 3/3] ARM: dts: rockchip: fix PWM clock found on RK3288 Socs Douglas Anderson 2 siblings, 1 reply; 17+ messages in thread From: Douglas Anderson @ 2019-04-09 20:47 UTC (permalink / raw) To: Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, Elaine Zhang, linux-clk, Douglas Anderson, linux-kernel, linux-arm-kernel This reverts commit 55bb6a633c33caf68ab470907ecf945289cb733d. The clocks that were enabled by that patch are pretty questionable. Specifically looking at what has been shipping on rk3288-veyron Chromebooks almost all of these clocks are safely turned off and cause no apparent problems. If some boards need these clocks turned on for some reason then it seems like we should figure out how to do that at a board level. NOTE: turning these clocks off doesn't seem to do a whole lot in terms of power savings (checking the power on the logic rail). It appears to save maybe 1-2mW. ...but still it seems like we should turn the clocks off if they aren't needed. Digging into the clocks here to describe why they shouldn't need to be left on: atclk: No documentation about this clock other than that it goes to the CPU. CPU functions fine without it on. jtag: Presumably this clock is only needed if you're debugging with JTAG. It doesn't seem like it makes sense to waste power for every rk3288 user. Perhaps this could be turned on with a CONFIG option? pclk_dbg, pclk_core_niu: On veyron Chromebooks we turn these two clocks on only during kernel panics in order to access some coresight registers. Since nothing in the upstream kernel does this we should be able to leave them off safely. hsicphy12m_xin12m: There is no indication of why this clock would need to be turned on for boards that don't use HSIC. pclk_ddrupctl[0-1], pclk_publ0[0-1]: On veyron Chromebooks we turn these 4 clocks on only when doing DDR transitions and they are off otherwise. I see no reason why they'd need to be on in the upstream kernel which doesn't support DDRFreq. pmu_hclk_otg0: A "chip design defect" is mentioned in the original patch but no details. This clock has always been gated in shipping veyron Chromebooks so presumably this chip defect doesn't affect all boards. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/clk/rockchip/clk-rk3288.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c index 5a67b7869960..06287810474e 100644 --- a/drivers/clk/rockchip/clk-rk3288.c +++ b/drivers/clk/rockchip/clk-rk3288.c @@ -313,13 +313,13 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { COMPOSITE_NOMUX(0, "aclk_core_mp", "armclk", CLK_IGNORE_UNUSED, RK3288_CLKSEL_CON(0), 4, 4, DFLAGS | CLK_DIVIDER_READ_ONLY, RK3288_CLKGATE_CON(12), 6, GFLAGS), - COMPOSITE_NOMUX(0, "atclk", "armclk", CLK_IGNORE_UNUSED, + COMPOSITE_NOMUX(0, "atclk", "armclk", 0, RK3288_CLKSEL_CON(37), 4, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, RK3288_CLKGATE_CON(12), 7, GFLAGS), COMPOSITE_NOMUX(0, "pclk_dbg_pre", "armclk", CLK_IGNORE_UNUSED, RK3288_CLKSEL_CON(37), 9, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, RK3288_CLKGATE_CON(12), 8, GFLAGS), - GATE(0, "pclk_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, + GATE(0, "pclk_dbg", "pclk_dbg_pre", 0, RK3288_CLKGATE_CON(12), 9, GFLAGS), GATE(0, "cs_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, RK3288_CLKGATE_CON(12), 10, GFLAGS), @@ -647,7 +647,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { INVERTER(SCLK_HSADC, "sclk_hsadc", "sclk_hsadc_out", RK3288_CLKSEL_CON(22), 7, IFLAGS), - GATE(0, "jtag", "ext_jtag", CLK_IGNORE_UNUSED, + GATE(0, "jtag", "ext_jtag", 0, RK3288_CLKGATE_CON(4), 14, GFLAGS), COMPOSITE_NODIV(SCLK_USBPHY480M_SRC, "usbphy480m_src", mux_usbphy480m_p, 0, @@ -656,7 +656,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { COMPOSITE_NODIV(SCLK_HSICPHY480M, "sclk_hsicphy480m", mux_hsicphy480m_p, 0, RK3288_CLKSEL_CON(29), 0, 2, MFLAGS, RK3288_CLKGATE_CON(3), 6, GFLAGS), - GATE(0, "hsicphy12m_xin12m", "xin12m", CLK_IGNORE_UNUSED, + GATE(0, "hsicphy12m_xin12m", "xin12m", 0, RK3288_CLKGATE_CON(13), 9, GFLAGS), DIV(0, "hsicphy12m_usbphy", "sclk_hsicphy480m", 0, RK3288_CLKSEL_CON(11), 8, 6, DFLAGS), @@ -837,12 +837,6 @@ static const char *const rk3288_critical_clocks[] __initconst = { "pclk_alive_niu", "pclk_pd_pmu", "pclk_pmu_niu", - "pclk_core_niu", - "pclk_ddrupctl0", - "pclk_publ0", - "pclk_ddrupctl1", - "pclk_publ1", - "pmu_hclk_otg0", }; static void __iomem *rk3288_cru_base; -- 2.21.0.392.gf8f6787159e-goog ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" 2019-04-09 20:47 ` [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" Douglas Anderson @ 2019-04-10 6:23 ` elaine.zhang 2019-04-10 15:34 ` Doug Anderson 0 siblings, 1 reply; 17+ messages in thread From: elaine.zhang @ 2019-04-10 6:23 UTC (permalink / raw) To: Douglas Anderson, Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, linux-clk, linux-kernel, linux-arm-kernel hi, 在 2019/4/10 上午4:47, Douglas Anderson 写道: > This reverts commit 55bb6a633c33caf68ab470907ecf945289cb733d. > > The clocks that were enabled by that patch are pretty questionable. > Specifically looking at what has been shipping on rk3288-veyron > Chromebooks almost all of these clocks are safely turned off and cause > no apparent problems. If some boards need these clocks turned on for > some reason then it seems like we should figure out how to do that at > a board level. > > NOTE: turning these clocks off doesn't seem to do a whole lot in terms > of power savings (checking the power on the logic rail). It appears > to save maybe 1-2mW. ...but still it seems like we should turn the > clocks off if they aren't needed. > > Digging into the clocks here to describe why they shouldn't need to be > left on: > > atclk: No documentation about this clock other than that it goes to > the CPU. CPU functions fine without it on. > > jtag: Presumably this clock is only needed if you're debugging with > JTAG. It doesn't seem like it makes sense to waste power for every > rk3288 user. Perhaps this could be turned on with a CONFIG option? > > pclk_dbg, pclk_core_niu: On veyron Chromebooks we turn these two > clocks on only during kernel panics in order to access some coresight > registers. Since nothing in the upstream kernel does this we should > be able to leave them off safely. > > hsicphy12m_xin12m: There is no indication of why this clock would need > to be turned on for boards that don't use HSIC. > > pclk_ddrupctl[0-1], pclk_publ0[0-1]: On veyron Chromebooks we turn > these 4 clocks on only when doing DDR transitions and they are off > otherwise. I see no reason why they'd need to be on in the upstream > kernel which doesn't support DDRFreq. > > pmu_hclk_otg0: A "chip design defect" is mentioned in the original > patch but no details. This clock has always been gated in shipping > veyron Chromebooks so presumably this chip defect doesn't affect all > boards. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/clk/rockchip/clk-rk3288.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c > index 5a67b7869960..06287810474e 100644 > --- a/drivers/clk/rockchip/clk-rk3288.c > +++ b/drivers/clk/rockchip/clk-rk3288.c > @@ -313,13 +313,13 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > COMPOSITE_NOMUX(0, "aclk_core_mp", "armclk", CLK_IGNORE_UNUSED, > RK3288_CLKSEL_CON(0), 4, 4, DFLAGS | CLK_DIVIDER_READ_ONLY, > RK3288_CLKGATE_CON(12), 6, GFLAGS), > - COMPOSITE_NOMUX(0, "atclk", "armclk", CLK_IGNORE_UNUSED, > + COMPOSITE_NOMUX(0, "atclk", "armclk", 0, > RK3288_CLKSEL_CON(37), 4, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > RK3288_CLKGATE_CON(12), 7, GFLAGS), > COMPOSITE_NOMUX(0, "pclk_dbg_pre", "armclk", CLK_IGNORE_UNUSED, > RK3288_CLKSEL_CON(37), 9, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > RK3288_CLKGATE_CON(12), 8, GFLAGS), > - GATE(0, "pclk_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > + GATE(0, "pclk_dbg", "pclk_dbg_pre", 0, > RK3288_CLKGATE_CON(12), 9, GFLAGS), > GATE(0, "cs_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > RK3288_CLKGATE_CON(12), 10, GFLAGS), > @@ -647,7 +647,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > INVERTER(SCLK_HSADC, "sclk_hsadc", "sclk_hsadc_out", > RK3288_CLKSEL_CON(22), 7, IFLAGS), > > - GATE(0, "jtag", "ext_jtag", CLK_IGNORE_UNUSED, > + GATE(0, "jtag", "ext_jtag", 0, > RK3288_CLKGATE_CON(4), 14, GFLAGS), CLK_IGNORE_UNUSED: Whether to close the unused clk after clk init complete. not affect there own enable/disable. JTAG is not have device node, when need jtag to debug, may be the system is crashed, there is no way to turn on this clk. > > COMPOSITE_NODIV(SCLK_USBPHY480M_SRC, "usbphy480m_src", mux_usbphy480m_p, 0, > @@ -656,7 +656,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > COMPOSITE_NODIV(SCLK_HSICPHY480M, "sclk_hsicphy480m", mux_hsicphy480m_p, 0, > RK3288_CLKSEL_CON(29), 0, 2, MFLAGS, > RK3288_CLKGATE_CON(3), 6, GFLAGS), > - GATE(0, "hsicphy12m_xin12m", "xin12m", CLK_IGNORE_UNUSED, > + GATE(0, "hsicphy12m_xin12m", "xin12m", 0, > RK3288_CLKGATE_CON(13), 9, GFLAGS), > DIV(0, "hsicphy12m_usbphy", "sclk_hsicphy480m", 0, > RK3288_CLKSEL_CON(11), 8, 6, DFLAGS), > @@ -837,12 +837,6 @@ static const char *const rk3288_critical_clocks[] __initconst = { > "pclk_alive_niu", > "pclk_pd_pmu", > "pclk_pmu_niu", > - "pclk_core_niu", > - "pclk_ddrupctl0", > - "pclk_publ0", > - "pclk_ddrupctl1", > - "pclk_publ1", These clks needed enable, device node not use this clk, so we mark it as critical. > - "pmu_hclk_otg0", It's a soc bug, pmu_hclk_otg0 must always on. > }; > > static void __iomem *rk3288_cru_base; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" 2019-04-10 6:23 ` elaine.zhang @ 2019-04-10 15:34 ` Doug Anderson [not found] ` <1491b5f1-e9f9-5718-76e5-0a49814ed76d@rock-chips.com> 0 siblings, 1 reply; 17+ messages in thread From: Doug Anderson @ 2019-04-10 15:34 UTC (permalink / raw) To: elaine.zhang Cc: Heiko Stuebner, Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM Hi, On Tue, Apr 9, 2019 at 11:23 PM elaine.zhang <zhangqing@rock-chips.com> wrote: > > hi, > > 在 2019/4/10 上午4:47, Douglas Anderson 写道: > > This reverts commit 55bb6a633c33caf68ab470907ecf945289cb733d. > > > > The clocks that were enabled by that patch are pretty questionable. > > Specifically looking at what has been shipping on rk3288-veyron > > Chromebooks almost all of these clocks are safely turned off and cause > > no apparent problems. If some boards need these clocks turned on for > > some reason then it seems like we should figure out how to do that at > > a board level. > > > > NOTE: turning these clocks off doesn't seem to do a whole lot in terms > > of power savings (checking the power on the logic rail). It appears > > to save maybe 1-2mW. ...but still it seems like we should turn the > > clocks off if they aren't needed. > > > > Digging into the clocks here to describe why they shouldn't need to be > > left on: > > > > atclk: No documentation about this clock other than that it goes to > > the CPU. CPU functions fine without it on. > > > > jtag: Presumably this clock is only needed if you're debugging with > > JTAG. It doesn't seem like it makes sense to waste power for every > > rk3288 user. Perhaps this could be turned on with a CONFIG option? > > > > pclk_dbg, pclk_core_niu: On veyron Chromebooks we turn these two > > clocks on only during kernel panics in order to access some coresight > > registers. Since nothing in the upstream kernel does this we should > > be able to leave them off safely. > > > > hsicphy12m_xin12m: There is no indication of why this clock would need > > to be turned on for boards that don't use HSIC. > > > > pclk_ddrupctl[0-1], pclk_publ0[0-1]: On veyron Chromebooks we turn > > these 4 clocks on only when doing DDR transitions and they are off > > otherwise. I see no reason why they'd need to be on in the upstream > > kernel which doesn't support DDRFreq. > > > > pmu_hclk_otg0: A "chip design defect" is mentioned in the original > > patch but no details. This clock has always been gated in shipping > > veyron Chromebooks so presumably this chip defect doesn't affect all > > boards. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/clk/rockchip/clk-rk3288.c | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c > > index 5a67b7869960..06287810474e 100644 > > --- a/drivers/clk/rockchip/clk-rk3288.c > > +++ b/drivers/clk/rockchip/clk-rk3288.c > > @@ -313,13 +313,13 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > > COMPOSITE_NOMUX(0, "aclk_core_mp", "armclk", CLK_IGNORE_UNUSED, > > RK3288_CLKSEL_CON(0), 4, 4, DFLAGS | CLK_DIVIDER_READ_ONLY, > > RK3288_CLKGATE_CON(12), 6, GFLAGS), > > - COMPOSITE_NOMUX(0, "atclk", "armclk", CLK_IGNORE_UNUSED, > > + COMPOSITE_NOMUX(0, "atclk", "armclk", 0, > > RK3288_CLKSEL_CON(37), 4, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > > RK3288_CLKGATE_CON(12), 7, GFLAGS), > > COMPOSITE_NOMUX(0, "pclk_dbg_pre", "armclk", CLK_IGNORE_UNUSED, > > RK3288_CLKSEL_CON(37), 9, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > > RK3288_CLKGATE_CON(12), 8, GFLAGS), > > - GATE(0, "pclk_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > > + GATE(0, "pclk_dbg", "pclk_dbg_pre", 0, > > RK3288_CLKGATE_CON(12), 9, GFLAGS), > > GATE(0, "cs_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > > RK3288_CLKGATE_CON(12), 10, GFLAGS), > > @@ -647,7 +647,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > > INVERTER(SCLK_HSADC, "sclk_hsadc", "sclk_hsadc_out", > > RK3288_CLKSEL_CON(22), 7, IFLAGS), > > > > - GATE(0, "jtag", "ext_jtag", CLK_IGNORE_UNUSED, > > + GATE(0, "jtag", "ext_jtag", 0, > > RK3288_CLKGATE_CON(4), 14, GFLAGS), > CLK_IGNORE_UNUSED: > Whether to close the unused clk after clk init complete. not affect > there own enable/disable. > JTAG is not have device node, when need jtag to debug, may be the system > is crashed, there is no way to turn on this clk. As I mentioned in the commit message this seems like the kind of thing that should be controlled by a CONFIG_ option. It's a debug option that's fine to have on all the time but consumes resources so some people might want to turn it off. > > COMPOSITE_NODIV(SCLK_USBPHY480M_SRC, "usbphy480m_src", mux_usbphy480m_p, 0, > > @@ -656,7 +656,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > > COMPOSITE_NODIV(SCLK_HSICPHY480M, "sclk_hsicphy480m", mux_hsicphy480m_p, 0, > > RK3288_CLKSEL_CON(29), 0, 2, MFLAGS, > > RK3288_CLKGATE_CON(3), 6, GFLAGS), > > - GATE(0, "hsicphy12m_xin12m", "xin12m", CLK_IGNORE_UNUSED, > > + GATE(0, "hsicphy12m_xin12m", "xin12m", 0, > > RK3288_CLKGATE_CON(13), 9, GFLAGS), > > DIV(0, "hsicphy12m_usbphy", "sclk_hsicphy480m", 0, > > RK3288_CLKSEL_CON(11), 8, 6, DFLAGS), > > @@ -837,12 +837,6 @@ static const char *const rk3288_critical_clocks[] __initconst = { > > "pclk_alive_niu", > > "pclk_pd_pmu", > > "pclk_pmu_niu", > > - "pclk_core_niu", > > - "pclk_ddrupctl0", > > - "pclk_publ0", > > - "pclk_ddrupctl1", > > - "pclk_publ1", > These clks needed enable, device node not use this clk, so we mark it as > critical. What breaks if you don't enable these clocks? As far as I can tell these clocks only need to be enabled while touching memory controller registers (deep suspend/resume and DDRFreq transitions). On Chromebooks everything works fine with these clocks only turned on when needed. > > - "pmu_hclk_otg0", > It's a soc bug, pmu_hclk_otg0 must always on. So you said in your previous commit message. However we've shipped lots and lots of Chromebooks with this clock off. Can you explain what is broken? Is this only needed for gadget mode (which we don't use), for instance? -Doug ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <1491b5f1-e9f9-5718-76e5-0a49814ed76d@rock-chips.com>]
* Re: [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" [not found] ` <1491b5f1-e9f9-5718-76e5-0a49814ed76d@rock-chips.com> @ 2019-04-11 15:26 ` Doug Anderson 2019-04-11 22:05 ` Heiko Stübner 0 siblings, 1 reply; 17+ messages in thread From: Doug Anderson @ 2019-04-11 15:26 UTC (permalink / raw) To: elaine.zhang, Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM Hi, On Wed, Apr 10, 2019 at 8:27 PM elaine.zhang <zhangqing@rock-chips.com> wrote: > > hi, > > 在 2019/4/10 下午11:34, Doug Anderson 写道: > > Hi, > > On Tue, Apr 9, 2019 at 11:23 PM elaine.zhang <zhangqing@rock-chips.com> wrote: > > hi, > > 在 2019/4/10 上午4:47, Douglas Anderson 写道: > > This reverts commit 55bb6a633c33caf68ab470907ecf945289cb733d. > > The clocks that were enabled by that patch are pretty questionable. > Specifically looking at what has been shipping on rk3288-veyron > Chromebooks almost all of these clocks are safely turned off and cause > no apparent problems. If some boards need these clocks turned on for > some reason then it seems like we should figure out how to do that at > a board level. > > NOTE: turning these clocks off doesn't seem to do a whole lot in terms > of power savings (checking the power on the logic rail). It appears > to save maybe 1-2mW. ...but still it seems like we should turn the > clocks off if they aren't needed. > > Digging into the clocks here to describe why they shouldn't need to be > left on: > > atclk: No documentation about this clock other than that it goes to > the CPU. CPU functions fine without it on. > > jtag: Presumably this clock is only needed if you're debugging with > JTAG. It doesn't seem like it makes sense to waste power for every > rk3288 user. Perhaps this could be turned on with a CONFIG option? > > pclk_dbg, pclk_core_niu: On veyron Chromebooks we turn these two > clocks on only during kernel panics in order to access some coresight > registers. Since nothing in the upstream kernel does this we should > be able to leave them off safely. > > hsicphy12m_xin12m: There is no indication of why this clock would need > to be turned on for boards that don't use HSIC. > > pclk_ddrupctl[0-1], pclk_publ0[0-1]: On veyron Chromebooks we turn > these 4 clocks on only when doing DDR transitions and they are off > otherwise. I see no reason why they'd need to be on in the upstream > kernel which doesn't support DDRFreq. > > pmu_hclk_otg0: A "chip design defect" is mentioned in the original > patch but no details. This clock has always been gated in shipping > veyron Chromebooks so presumably this chip defect doesn't affect all > boards. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/clk/rockchip/clk-rk3288.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c > index 5a67b7869960..06287810474e 100644 > --- a/drivers/clk/rockchip/clk-rk3288.c > +++ b/drivers/clk/rockchip/clk-rk3288.c > @@ -313,13 +313,13 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > COMPOSITE_NOMUX(0, "aclk_core_mp", "armclk", CLK_IGNORE_UNUSED, > RK3288_CLKSEL_CON(0), 4, 4, DFLAGS | CLK_DIVIDER_READ_ONLY, > RK3288_CLKGATE_CON(12), 6, GFLAGS), > - COMPOSITE_NOMUX(0, "atclk", "armclk", CLK_IGNORE_UNUSED, > + COMPOSITE_NOMUX(0, "atclk", "armclk", 0, > RK3288_CLKSEL_CON(37), 4, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > RK3288_CLKGATE_CON(12), 7, GFLAGS), > COMPOSITE_NOMUX(0, "pclk_dbg_pre", "armclk", CLK_IGNORE_UNUSED, > RK3288_CLKSEL_CON(37), 9, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > RK3288_CLKGATE_CON(12), 8, GFLAGS), > - GATE(0, "pclk_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > + GATE(0, "pclk_dbg", "pclk_dbg_pre", 0, > RK3288_CLKGATE_CON(12), 9, GFLAGS), > GATE(0, "cs_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > RK3288_CLKGATE_CON(12), 10, GFLAGS), > @@ -647,7 +647,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > INVERTER(SCLK_HSADC, "sclk_hsadc", "sclk_hsadc_out", > RK3288_CLKSEL_CON(22), 7, IFLAGS), > > - GATE(0, "jtag", "ext_jtag", CLK_IGNORE_UNUSED, > + GATE(0, "jtag", "ext_jtag", 0, > RK3288_CLKGATE_CON(4), 14, GFLAGS), > > CLK_IGNORE_UNUSED: > Whether to close the unused clk after clk init complete. not affect > there own enable/disable. > JTAG is not have device node, when need jtag to debug, may be the system > is crashed, there is no way to turn on this clk. > > As I mentioned in the commit message this seems like the kind of thing > that should be controlled by a CONFIG_ option. It's a debug option > that's fine to have on all the time but consumes resources so some > people might want to turn it off. > > Currently, CONFIG_ option is not implemented. We will refer to this suggestion. > > For debug, I don't hope to remove the flag CLK_IGNORE_UNUSED.(The clk static power is very small) I'll leave it up to Heiko for what to do here. I agree that it's not tons of power (this whole patch saved 1-2 mW on the INT rail) but I'd still prefer for clocks to be off if they can. In general one could also argue that keeping JTAG off by default could be good for security. ...but I guess I have to wonder how you're doing JTAG on upstream kernels without any extra patches anyway. Automatic JTAG switching is turned off in "drivers/soc/rockchip/grf.c". ...and by default the JTAG signals are muxed as the SD Card. So presumably you've already got extra patches in order to make JTAG work. I assume that the clocks that are important for JTAG are "atclk", "jtag", "pclk_dbg", and "pclk_core_niu". > COMPOSITE_NODIV(SCLK_USBPHY480M_SRC, "usbphy480m_src", mux_usbphy480m_p, 0, > @@ -656,7 +656,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > COMPOSITE_NODIV(SCLK_HSICPHY480M, "sclk_hsicphy480m", mux_hsicphy480m_p, 0, > RK3288_CLKSEL_CON(29), 0, 2, MFLAGS, > RK3288_CLKGATE_CON(3), 6, GFLAGS), > - GATE(0, "hsicphy12m_xin12m", "xin12m", CLK_IGNORE_UNUSED, > + GATE(0, "hsicphy12m_xin12m", "xin12m", 0, > RK3288_CLKGATE_CON(13), 9, GFLAGS), > DIV(0, "hsicphy12m_usbphy", "sclk_hsicphy480m", 0, > RK3288_CLKSEL_CON(11), 8, 6, DFLAGS), > @@ -837,12 +837,6 @@ static const char *const rk3288_critical_clocks[] __initconst = { > "pclk_alive_niu", > "pclk_pd_pmu", > "pclk_pmu_niu", > - "pclk_core_niu", > - "pclk_ddrupctl0", > - "pclk_publ0", > - "pclk_ddrupctl1", > - "pclk_publ1", > > These clks needed enable, device node not use this clk, so we mark it as > critical. > > What breaks if you don't enable these clocks? As far as I can tell > these clocks only need to be enabled while touching memory controller > registers (deep suspend/resume and DDRFreq transitions). On > Chromebooks everything works fine with these clocks only turned on > when needed. > > If need to enable these clks, ddrupctl0/ddrupctl1/publ0/publ1 clks no driver to handle them, so mark these clk as critical. Perhaps it's just a typo, but you say "If need to enable". I see no evidence that these clocks need to be enabled unless there is code touching memory controller registers. Thus we don't need to enable them so they don't need to be critical. > - "pmu_hclk_otg0", > > It's a soc bug, pmu_hclk_otg0 must always on. > > So you said in your previous commit message. However we've shipped > lots and lots of Chromebooks with this clock off. Can you explain > what is broken? Is this only needed for gadget mode (which we don't > use), for instance? > > test case: > > recovery test, < 1 hour , system crash. > > log: > > [ 127.569629] I[0: swapper/0: 0] GOTGCTL @0xFFFFFF8000B80000 : 0x00400010 > [ 127.569644] I[0: swapper/0: 0] GOTGINT @0xFFFFFF8000B80004 : 0x00400010 > [ 127.569659] I[0: swapper/0: 0] GAHBCFG @0xFFFFFF8000B80008 : 0x00400010 > [ 127.569673] I[0: swapper/0: 0] GUSBCFG @0xFFFFFF8000B8000C : 0x00400010 > [ 127.569688] I[0: swapper/0: 0] GRSTCTL @0xFFFFFF8000B80010 : 0x00400010 > [ 127.569702] I[0: swapper/0: 0] GINTSTS @0xFFFFFF8000B80014 : 0x00400010 > [ 127.569718] I[0: swapper/0: 0] GINTMSK @0xFFFFFF8000B80018 : 0x00400010 > [ 127.569733] I[0: swapper/0: 0] GRXSTSR @0xFFFFFF8000B8001C : 0x00400010 > [ 127.569748] I[0: swapper/0: 0] GRXFSIZ @0xFFFFFF8000B80024 : 0x00400010 I don't know what a "recovery test" is and I don't understand your logs. Can you explain we do not run into this on Chromebooks? > reason: > > USB OTG controller supports turning off most logic power, and then only one PMU module is left. This clock cannot be turned off, which is similar to the always on module in USB OTG. Can't you just add a patch to the dwc2 driver to have it grab this clock? I assume this clock doesn't need to be turned on unless you're using the OTG contoller in a certain way? NOTE: nowhere in your responses did you talk about why "hsicphy12m_xin12m" needed to be marked as IGNORE_UNUSED. I guess you're OK with removing that? -Doug ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" 2019-04-11 15:26 ` Doug Anderson @ 2019-04-11 22:05 ` Heiko Stübner 2019-04-12 1:43 ` elaine.zhang 0 siblings, 1 reply; 17+ messages in thread From: Heiko Stübner @ 2019-04-11 22:05 UTC (permalink / raw) To: Doug Anderson Cc: elaine.zhang, Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM Hi, Am Donnerstag, 11. April 2019, 17:26:41 CEST schrieb Doug Anderson: > On Wed, Apr 10, 2019 at 8:27 PM elaine.zhang <zhangqing@rock-chips.com> wrote: > > 在 2019/4/10 下午11:34, Doug Anderson 写道: > > On Tue, Apr 9, 2019 at 11:23 PM elaine.zhang <zhangqing@rock-chips.com> wrote: > > 在 2019/4/10 上午4:47, Douglas Anderson 写道: > > > > This reverts commit 55bb6a633c33caf68ab470907ecf945289cb733d. > > > > The clocks that were enabled by that patch are pretty questionable. > > Specifically looking at what has been shipping on rk3288-veyron > > Chromebooks almost all of these clocks are safely turned off and cause > > no apparent problems. If some boards need these clocks turned on for > > some reason then it seems like we should figure out how to do that at > > a board level. > > > > NOTE: turning these clocks off doesn't seem to do a whole lot in terms > > of power savings (checking the power on the logic rail). It appears > > to save maybe 1-2mW. ...but still it seems like we should turn the > > clocks off if they aren't needed. > > > > Digging into the clocks here to describe why they shouldn't need to be > > left on: > > > > atclk: No documentation about this clock other than that it goes to > > the CPU. CPU functions fine without it on. > > > > jtag: Presumably this clock is only needed if you're debugging with > > JTAG. It doesn't seem like it makes sense to waste power for every > > rk3288 user. Perhaps this could be turned on with a CONFIG option? > > > > pclk_dbg, pclk_core_niu: On veyron Chromebooks we turn these two > > clocks on only during kernel panics in order to access some coresight > > registers. Since nothing in the upstream kernel does this we should > > be able to leave them off safely. > > > > hsicphy12m_xin12m: There is no indication of why this clock would need > > to be turned on for boards that don't use HSIC. > > > > pclk_ddrupctl[0-1], pclk_publ0[0-1]: On veyron Chromebooks we turn > > these 4 clocks on only when doing DDR transitions and they are off > > otherwise. I see no reason why they'd need to be on in the upstream > > kernel which doesn't support DDRFreq. > > > > pmu_hclk_otg0: A "chip design defect" is mentioned in the original > > patch but no details. This clock has always been gated in shipping > > veyron Chromebooks so presumably this chip defect doesn't affect all > > boards. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/clk/rockchip/clk-rk3288.c | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c > > index 5a67b7869960..06287810474e 100644 > > --- a/drivers/clk/rockchip/clk-rk3288.c > > +++ b/drivers/clk/rockchip/clk-rk3288.c > > @@ -313,13 +313,13 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > > COMPOSITE_NOMUX(0, "aclk_core_mp", "armclk", CLK_IGNORE_UNUSED, > > RK3288_CLKSEL_CON(0), 4, 4, DFLAGS | CLK_DIVIDER_READ_ONLY, > > RK3288_CLKGATE_CON(12), 6, GFLAGS), > > - COMPOSITE_NOMUX(0, "atclk", "armclk", CLK_IGNORE_UNUSED, > > + COMPOSITE_NOMUX(0, "atclk", "armclk", 0, > > RK3288_CLKSEL_CON(37), 4, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > > RK3288_CLKGATE_CON(12), 7, GFLAGS), > > COMPOSITE_NOMUX(0, "pclk_dbg_pre", "armclk", CLK_IGNORE_UNUSED, > > RK3288_CLKSEL_CON(37), 9, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, > > RK3288_CLKGATE_CON(12), 8, GFLAGS), > > - GATE(0, "pclk_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > > + GATE(0, "pclk_dbg", "pclk_dbg_pre", 0, > > RK3288_CLKGATE_CON(12), 9, GFLAGS), > > GATE(0, "cs_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, > > RK3288_CLKGATE_CON(12), 10, GFLAGS), > > @@ -647,7 +647,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > > INVERTER(SCLK_HSADC, "sclk_hsadc", "sclk_hsadc_out", > > RK3288_CLKSEL_CON(22), 7, IFLAGS), > > > > - GATE(0, "jtag", "ext_jtag", CLK_IGNORE_UNUSED, > > + GATE(0, "jtag", "ext_jtag", 0, > > RK3288_CLKGATE_CON(4), 14, GFLAGS), > > > > CLK_IGNORE_UNUSED: > > Whether to close the unused clk after clk init complete. not affect > > there own enable/disable. > > JTAG is not have device node, when need jtag to debug, may be the system > > is crashed, there is no way to turn on this clk. > > > > As I mentioned in the commit message this seems like the kind of thing > > that should be controlled by a CONFIG_ option. It's a debug option > > that's fine to have on all the time but consumes resources so some > > people might want to turn it off. > > > > Currently, CONFIG_ option is not implemented. We will refer to this suggestion. > > > > For debug, I don't hope to remove the flag CLK_IGNORE_UNUSED.(The clk static power is very small) > > I'll leave it up to Heiko for what to do here. I agree that it's not > tons of power (this whole patch saved 1-2 mW on the INT rail) but I'd > still prefer for clocks to be off if they can. In general one could > also argue that keeping JTAG off by default could be good for > security. > > ...but I guess I have to wonder how you're doing JTAG on upstream > kernels without any extra patches anyway. Automatic JTAG switching is > turned off in "drivers/soc/rockchip/grf.c". ...and by default the > JTAG signals are muxed as the SD Card. So presumably you've already > got extra patches in order to make JTAG work. > > I assume that the clocks that are important for JTAG are "atclk", > "jtag", "pclk_dbg", and "pclk_core_niu". Isn't JTAG part of the coresight package anyway? The whole coresight stuff, so in an upstream-context I'd somehow expect it to get activated through that. Or is it actually separate and only in the debug-diagram in the TRM mixed into coresight? In any case I'm with Doug here, for jtag you need to modify your kernel anyway for pinctrl settings, so could also enable the clock in that case. > > COMPOSITE_NODIV(SCLK_USBPHY480M_SRC, "usbphy480m_src", mux_usbphy480m_p, 0, > > @@ -656,7 +656,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { > > COMPOSITE_NODIV(SCLK_HSICPHY480M, "sclk_hsicphy480m", mux_hsicphy480m_p, 0, > > RK3288_CLKSEL_CON(29), 0, 2, MFLAGS, > > RK3288_CLKGATE_CON(3), 6, GFLAGS), > > - GATE(0, "hsicphy12m_xin12m", "xin12m", CLK_IGNORE_UNUSED, > > + GATE(0, "hsicphy12m_xin12m", "xin12m", 0, > > RK3288_CLKGATE_CON(13), 9, GFLAGS), > > DIV(0, "hsicphy12m_usbphy", "sclk_hsicphy480m", 0, > > RK3288_CLKSEL_CON(11), 8, 6, DFLAGS), > > @@ -837,12 +837,6 @@ static const char *const rk3288_critical_clocks[] __initconst = { > > "pclk_alive_niu", > > "pclk_pd_pmu", > > "pclk_pmu_niu", > > - "pclk_core_niu", > > - "pclk_ddrupctl0", > > - "pclk_publ0", > > - "pclk_ddrupctl1", > > - "pclk_publ1", > > > > These clks needed enable, device node not use this clk, so we mark it as > > critical. > > > > What breaks if you don't enable these clocks? As far as I can tell > > these clocks only need to be enabled while touching memory controller > > registers (deep suspend/resume and DDRFreq transitions). On > > Chromebooks everything works fine with these clocks only turned on > > when needed. > > > > If need to enable these clks, ddrupctl0/ddrupctl1/publ0/publ1 clks no driver to handle them, so mark these clk as critical. > > Perhaps it's just a typo, but you say "If need to enable". I see no > evidence that these clocks need to be enabled unless there is code > touching memory controller registers. Thus we don't need to enable > them so they don't need to be critical. yep ... maybe Elaine can clarify in what other activities these clocks are needed ... I guess any ddr-scaling code would activate them before touching memory-controller-registers. > > - "pmu_hclk_otg0", > > > > It's a soc bug, pmu_hclk_otg0 must always on. > > > > So you said in your previous commit message. However we've shipped > > lots and lots of Chromebooks with this clock off. Can you explain > > what is broken? Is this only needed for gadget mode (which we don't > > use), for instance? > > > > test case: > > > > recovery test, < 1 hour , system crash. > > > > log: > > > > [ 127.569629] I[0: swapper/0: 0] GOTGCTL @0xFFFFFF8000B80000 : 0x00400010 > > [ 127.569644] I[0: swapper/0: 0] GOTGINT @0xFFFFFF8000B80004 : 0x00400010 > > [ 127.569659] I[0: swapper/0: 0] GAHBCFG @0xFFFFFF8000B80008 : 0x00400010 > > [ 127.569673] I[0: swapper/0: 0] GUSBCFG @0xFFFFFF8000B8000C : 0x00400010 > > [ 127.569688] I[0: swapper/0: 0] GRSTCTL @0xFFFFFF8000B80010 : 0x00400010 > > [ 127.569702] I[0: swapper/0: 0] GINTSTS @0xFFFFFF8000B80014 : 0x00400010 > > [ 127.569718] I[0: swapper/0: 0] GINTMSK @0xFFFFFF8000B80018 : 0x00400010 > > [ 127.569733] I[0: swapper/0: 0] GRXSTSR @0xFFFFFF8000B8001C : 0x00400010 > > [ 127.569748] I[0: swapper/0: 0] GRXFSIZ @0xFFFFFF8000B80024 : 0x00400010 > > I don't know what a "recovery test" is and I don't understand your logs. > > Can you explain we do not run into this on Chromebooks? > > > > reason: > > > > USB OTG controller supports turning off most logic power, and then only one PMU module is left. This clock cannot be turned off, which is similar to the always on module in USB OTG. > > Can't you just add a patch to the dwc2 driver to have it grab this > clock? I assume this clock doesn't need to be turned on unless you're > using the OTG contoller in a certain way? So far we don't really know where the clock in question is sitting in the clock hirarchy. For example the kernel got a new interconnect framework recently, so handling non-device clocks in a device may haunt us later on. @Elaine: could you elaborate what pmu_hclk_otg0 actually is for please? > NOTE: nowhere in your responses did you talk about why > "hsicphy12m_xin12m" needed to be marked as IGNORE_UNUSED. I guess > you're OK with removing that? Thanks Heiko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" 2019-04-11 22:05 ` Heiko Stübner @ 2019-04-12 1:43 ` elaine.zhang 2019-04-12 15:41 ` Doug Anderson 0 siblings, 1 reply; 17+ messages in thread From: elaine.zhang @ 2019-04-12 1:43 UTC (permalink / raw) To: Heiko Stübner, Doug Anderson Cc: Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM hi, 在 2019/4/12 上午6:05, Heiko Stübner 写道: > Hi, > > Am Donnerstag, 11. April 2019, 17:26:41 CEST schrieb Doug Anderson: >> On Wed, Apr 10, 2019 at 8:27 PM elaine.zhang <zhangqing@rock-chips.com> wrote: >>> 在 2019/4/10 下午11:34, Doug Anderson 写道: >>> On Tue, Apr 9, 2019 at 11:23 PM elaine.zhang <zhangqing@rock-chips.com> wrote: >>> 在 2019/4/10 上午4:47, Douglas Anderson 写道: >>> >>> This reverts commit 55bb6a633c33caf68ab470907ecf945289cb733d. >>> >>> The clocks that were enabled by that patch are pretty questionable. >>> Specifically looking at what has been shipping on rk3288-veyron >>> Chromebooks almost all of these clocks are safely turned off and cause >>> no apparent problems. If some boards need these clocks turned on for >>> some reason then it seems like we should figure out how to do that at >>> a board level. >>> >>> NOTE: turning these clocks off doesn't seem to do a whole lot in terms >>> of power savings (checking the power on the logic rail). It appears >>> to save maybe 1-2mW. ...but still it seems like we should turn the >>> clocks off if they aren't needed. >>> >>> Digging into the clocks here to describe why they shouldn't need to be >>> left on: >>> >>> atclk: No documentation about this clock other than that it goes to >>> the CPU. CPU functions fine without it on. >>> >>> jtag: Presumably this clock is only needed if you're debugging with >>> JTAG. It doesn't seem like it makes sense to waste power for every >>> rk3288 user. Perhaps this could be turned on with a CONFIG option? >>> >>> pclk_dbg, pclk_core_niu: On veyron Chromebooks we turn these two >>> clocks on only during kernel panics in order to access some coresight >>> registers. Since nothing in the upstream kernel does this we should >>> be able to leave them off safely. >>> >>> hsicphy12m_xin12m: There is no indication of why this clock would need >>> to be turned on for boards that don't use HSIC. >>> >>> pclk_ddrupctl[0-1], pclk_publ0[0-1]: On veyron Chromebooks we turn >>> these 4 clocks on only when doing DDR transitions and they are off >>> otherwise. I see no reason why they'd need to be on in the upstream >>> kernel which doesn't support DDRFreq. >>> >>> pmu_hclk_otg0: A "chip design defect" is mentioned in the original >>> patch but no details. This clock has always been gated in shipping >>> veyron Chromebooks so presumably this chip defect doesn't affect all >>> boards. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> --- >>> >>> drivers/clk/rockchip/clk-rk3288.c | 14 ++++---------- >>> 1 file changed, 4 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c >>> index 5a67b7869960..06287810474e 100644 >>> --- a/drivers/clk/rockchip/clk-rk3288.c >>> +++ b/drivers/clk/rockchip/clk-rk3288.c >>> @@ -313,13 +313,13 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { >>> COMPOSITE_NOMUX(0, "aclk_core_mp", "armclk", CLK_IGNORE_UNUSED, >>> RK3288_CLKSEL_CON(0), 4, 4, DFLAGS | CLK_DIVIDER_READ_ONLY, >>> RK3288_CLKGATE_CON(12), 6, GFLAGS), >>> - COMPOSITE_NOMUX(0, "atclk", "armclk", CLK_IGNORE_UNUSED, >>> + COMPOSITE_NOMUX(0, "atclk", "armclk", 0, >>> RK3288_CLKSEL_CON(37), 4, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, >>> RK3288_CLKGATE_CON(12), 7, GFLAGS), >>> COMPOSITE_NOMUX(0, "pclk_dbg_pre", "armclk", CLK_IGNORE_UNUSED, >>> RK3288_CLKSEL_CON(37), 9, 5, DFLAGS | CLK_DIVIDER_READ_ONLY, >>> RK3288_CLKGATE_CON(12), 8, GFLAGS), >>> - GATE(0, "pclk_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, >>> + GATE(0, "pclk_dbg", "pclk_dbg_pre", 0, >>> RK3288_CLKGATE_CON(12), 9, GFLAGS), >>> GATE(0, "cs_dbg", "pclk_dbg_pre", CLK_IGNORE_UNUSED, >>> RK3288_CLKGATE_CON(12), 10, GFLAGS), >>> @@ -647,7 +647,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { >>> INVERTER(SCLK_HSADC, "sclk_hsadc", "sclk_hsadc_out", >>> RK3288_CLKSEL_CON(22), 7, IFLAGS), >>> >>> - GATE(0, "jtag", "ext_jtag", CLK_IGNORE_UNUSED, >>> + GATE(0, "jtag", "ext_jtag", 0, >>> RK3288_CLKGATE_CON(4), 14, GFLAGS), >>> >>> CLK_IGNORE_UNUSED: >>> Whether to close the unused clk after clk init complete. not affect >>> there own enable/disable. >>> JTAG is not have device node, when need jtag to debug, may be the system >>> is crashed, there is no way to turn on this clk. >>> >>> As I mentioned in the commit message this seems like the kind of thing >>> that should be controlled by a CONFIG_ option. It's a debug option >>> that's fine to have on all the time but consumes resources so some >>> people might want to turn it off. >>> >>> Currently, CONFIG_ option is not implemented. We will refer to this suggestion. >>> >>> For debug, I don't hope to remove the flag CLK_IGNORE_UNUSED.(The clk static power is very small) >> I'll leave it up to Heiko for what to do here. I agree that it's not >> tons of power (this whole patch saved 1-2 mW on the INT rail) but I'd >> still prefer for clocks to be off if they can. In general one could >> also argue that keeping JTAG off by default could be good for >> security. >> >> ...but I guess I have to wonder how you're doing JTAG on upstream >> kernels without any extra patches anyway. Automatic JTAG switching is >> turned off in "drivers/soc/rockchip/grf.c". ...and by default the >> JTAG signals are muxed as the SD Card. So presumably you've already >> got extra patches in order to make JTAG work. >> >> I assume that the clocks that are important for JTAG are "atclk", >> "jtag", "pclk_dbg", and "pclk_core_niu". > Isn't JTAG part of the coresight package anyway? The whole coresight > stuff, so in an upstream-context I'd somehow expect it to get activated > through that. Or is it actually separate and only in the debug-diagram > in the TRM mixed into coresight? > > In any case I'm with Doug here, for jtag you need to modify your kernel > anyway for pinctrl settings, so could also enable the clock in that case. OK, I agree. > > >>> COMPOSITE_NODIV(SCLK_USBPHY480M_SRC, "usbphy480m_src", mux_usbphy480m_p, 0, >>> @@ -656,7 +656,7 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = { >>> COMPOSITE_NODIV(SCLK_HSICPHY480M, "sclk_hsicphy480m", mux_hsicphy480m_p, 0, >>> RK3288_CLKSEL_CON(29), 0, 2, MFLAGS, >>> RK3288_CLKGATE_CON(3), 6, GFLAGS), >>> - GATE(0, "hsicphy12m_xin12m", "xin12m", CLK_IGNORE_UNUSED, >>> + GATE(0, "hsicphy12m_xin12m", "xin12m", 0, >>> RK3288_CLKGATE_CON(13), 9, GFLAGS), >>> DIV(0, "hsicphy12m_usbphy", "sclk_hsicphy480m", 0, >>> RK3288_CLKSEL_CON(11), 8, 6, DFLAGS), >>> @@ -837,12 +837,6 @@ static const char *const rk3288_critical_clocks[] __initconst = { >>> "pclk_alive_niu", >>> "pclk_pd_pmu", >>> "pclk_pmu_niu", >>> - "pclk_core_niu", >>> - "pclk_ddrupctl0", >>> - "pclk_publ0", >>> - "pclk_ddrupctl1", >>> - "pclk_publ1", >>> >>> These clks needed enable, device node not use this clk, so we mark it as >>> critical. >>> >>> What breaks if you don't enable these clocks? As far as I can tell >>> these clocks only need to be enabled while touching memory controller >>> registers (deep suspend/resume and DDRFreq transitions). On >>> Chromebooks everything works fine with these clocks only turned on >>> when needed. >>> >>> If need to enable these clks, ddrupctl0/ddrupctl1/publ0/publ1 clks no driver to handle them, so mark these clk as critical. >> Perhaps it's just a typo, but you say "If need to enable". I see no >> evidence that these clocks need to be enabled unless there is code >> touching memory controller registers. Thus we don't need to enable >> them so they don't need to be critical. > yep ... maybe Elaine can clarify in what other activities these clocks > are needed ... I guess any ddr-scaling code would activate them > before touching memory-controller-registers. OK, I agree. I rechecked the clocks.It does get turned on before touching memory-controller-registers. The old DDR drivers are not going to control these clks, so we marked it as critical. The latest DDR drivers are already supported activate them before touching memory-controller-registers. Sorry. it's my fault. > > >>> - "pmu_hclk_otg0", >>> >>> It's a soc bug, pmu_hclk_otg0 must always on. >>> >>> So you said in your previous commit message. However we've shipped >>> lots and lots of Chromebooks with this clock off. Can you explain >>> what is broken? Is this only needed for gadget mode (which we don't >>> use), for instance? >>> >>> test case: >>> >>> recovery test, < 1 hour , system crash. >>> >>> log: >>> >>> [ 127.569629] I[0: swapper/0: 0] GOTGCTL @0xFFFFFF8000B80000 : 0x00400010 >>> [ 127.569644] I[0: swapper/0: 0] GOTGINT @0xFFFFFF8000B80004 : 0x00400010 >>> [ 127.569659] I[0: swapper/0: 0] GAHBCFG @0xFFFFFF8000B80008 : 0x00400010 >>> [ 127.569673] I[0: swapper/0: 0] GUSBCFG @0xFFFFFF8000B8000C : 0x00400010 >>> [ 127.569688] I[0: swapper/0: 0] GRSTCTL @0xFFFFFF8000B80010 : 0x00400010 >>> [ 127.569702] I[0: swapper/0: 0] GINTSTS @0xFFFFFF8000B80014 : 0x00400010 >>> [ 127.569718] I[0: swapper/0: 0] GINTMSK @0xFFFFFF8000B80018 : 0x00400010 >>> [ 127.569733] I[0: swapper/0: 0] GRXSTSR @0xFFFFFF8000B8001C : 0x00400010 >>> [ 127.569748] I[0: swapper/0: 0] GRXFSIZ @0xFFFFFF8000B80024 : 0x00400010 >> I don't know what a "recovery test" is and I don't understand your logs. >> >> Can you explain we do not run into this on Chromebooks? >> >> >>> reason: >>> >>> USB OTG controller supports turning off most logic power, and then only one PMU module is left. This clock cannot be turned off, which is similar to the always on module in USB OTG. >> Can't you just add a patch to the dwc2 driver to have it grab this >> clock? I assume this clock doesn't need to be turned on unless you're >> using the OTG contoller in a certain way? > So far we don't really know where the clock in question is sitting > in the clock hirarchy. For example the kernel got a new interconnect > framework recently, so handling non-device clocks in a device may haunt > us later on. > > @Elaine: could you elaborate what pmu_hclk_otg0 actually is for please? Doug: Recovry test: Regular factory tests, including restart, adb debugging, clear data/factory Settings, and clear cache. I'm not clear whether the test was added by chromebooks. Heiko: pmu_hclk_otg0: pmu ahb clock Function: Clock to pmu module when hibernation and/or ADP is enabled.Must be greater than or equal to 30 MHz. If the SOC design does not support hibernation/ADP function, only have hclk_otg, this clk can be switched according to the usage of otg. If the SOC design support hibernation/ADP, has two clocks, hclk_otg and pmu_hclk_otg0. Hclk_otg belongs to the closed part of otg logic, which can be switched according to the use of otg. pmu_hclk_otg0 belongs to the always on part. As for whether pmu_hclk_otg0 can be turned off when otg is not in use, we have not tested. IC suggest make pmu_hclk_otg0 always on. > > >> NOTE: nowhere in your responses did you talk about why >> "hsicphy12m_xin12m" needed to be marked as IGNORE_UNUSED. I guess >> you're OK with removing that? Yes, I agree. > > Thanks > Heiko > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" 2019-04-12 1:43 ` elaine.zhang @ 2019-04-12 15:41 ` Doug Anderson 0 siblings, 0 replies; 17+ messages in thread From: Doug Anderson @ 2019-04-12 15:41 UTC (permalink / raw) To: elaine.zhang Cc: Heiko Stübner, Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM Hi, On Thu, Apr 11, 2019 at 6:43 PM elaine.zhang <zhangqing@rock-chips.com> wrote: > >>> - "pmu_hclk_otg0", > >>> > >>> It's a soc bug, pmu_hclk_otg0 must always on. > >>> > >>> So you said in your previous commit message. However we've shipped > >>> lots and lots of Chromebooks with this clock off. Can you explain > >>> what is broken? Is this only needed for gadget mode (which we don't > >>> use), for instance? > >>> > >>> test case: > >>> > >>> recovery test, < 1 hour , system crash. > >>> > >>> log: > >>> > >>> [ 127.569629] I[0: swapper/0: 0] GOTGCTL @0xFFFFFF8000B80000 : 0x00400010 > >>> [ 127.569644] I[0: swapper/0: 0] GOTGINT @0xFFFFFF8000B80004 : 0x00400010 > >>> [ 127.569659] I[0: swapper/0: 0] GAHBCFG @0xFFFFFF8000B80008 : 0x00400010 > >>> [ 127.569673] I[0: swapper/0: 0] GUSBCFG @0xFFFFFF8000B8000C : 0x00400010 > >>> [ 127.569688] I[0: swapper/0: 0] GRSTCTL @0xFFFFFF8000B80010 : 0x00400010 > >>> [ 127.569702] I[0: swapper/0: 0] GINTSTS @0xFFFFFF8000B80014 : 0x00400010 > >>> [ 127.569718] I[0: swapper/0: 0] GINTMSK @0xFFFFFF8000B80018 : 0x00400010 > >>> [ 127.569733] I[0: swapper/0: 0] GRXSTSR @0xFFFFFF8000B8001C : 0x00400010 > >>> [ 127.569748] I[0: swapper/0: 0] GRXFSIZ @0xFFFFFF8000B80024 : 0x00400010 > >> I don't know what a "recovery test" is and I don't understand your logs. > >> > >> Can you explain we do not run into this on Chromebooks? > >> > >> > >>> reason: > >>> > >>> USB OTG controller supports turning off most logic power, and then only one PMU module is left. This clock cannot be turned off, which is similar to the always on module in USB OTG. > >> Can't you just add a patch to the dwc2 driver to have it grab this > >> clock? I assume this clock doesn't need to be turned on unless you're > >> using the OTG contoller in a certain way? > > So far we don't really know where the clock in question is sitting > > in the clock hirarchy. For example the kernel got a new interconnect > > framework recently, so handling non-device clocks in a device may haunt > > us later on. > > > > @Elaine: could you elaborate what pmu_hclk_otg0 actually is for please? > > Doug: > > Recovry test: Regular factory tests, including restart, adb debugging, > clear data/factory Settings, and clear cache. > I'm not clear whether the test was added by chromebooks. > > Heiko: > > pmu_hclk_otg0: pmu ahb clock > > Function: Clock to pmu module when hibernation and/or ADP is > enabled.Must be greater than or equal to 30 MHz. Does this mean we can enable hibernation in dwc2 once we turn this clock on? I think right now hibernation doesn't work for dwc2 on rk3288. Not that I know a ton about dwc2's hibernation modes, but I've certainly bumped up against it when enabling power down modes. In fact I'm planning to post some patches soon... I'll CC you. > If the SOC design does not support hibernation/ADP function, only have > hclk_otg, this clk can be switched according to the usage of otg. > If the SOC design support hibernation/ADP, has two clocks, hclk_otg and > pmu_hclk_otg0. > Hclk_otg belongs to the closed part of otg logic, which can be switched > according to the use of otg. > > pmu_hclk_otg0 belongs to the always on part. > > As for whether pmu_hclk_otg0 can be turned off when otg is not in use, > we have not tested. IC suggest make pmu_hclk_otg0 always on. OK. I'm fine with this clock staying as a critical clock for now. I'll send out a v2 shortly. -Doug ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 2019-04-09 20:47 [PATCH 0/3] rockchip: A few clock cleanups for rk3288 Douglas Anderson 2019-04-09 20:47 ` [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" Douglas Anderson @ 2019-04-09 20:47 ` Douglas Anderson 2019-04-10 6:42 ` elaine.zhang 2019-04-11 19:27 ` Heiko Stübner 2019-04-09 20:47 ` [PATCH 3/3] ARM: dts: rockchip: fix PWM clock found on RK3288 Socs Douglas Anderson 2 siblings, 2 replies; 17+ messages in thread From: Douglas Anderson @ 2019-04-09 20:47 UTC (permalink / raw) To: Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, Elaine Zhang, linux-clk, Douglas Anderson, linux-kernel, linux-arm-kernel 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", }; static void __iomem *rk3288_cru_base; -- 2.21.0.392.gf8f6787159e-goog ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 2019-04-09 20:47 ` [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 Douglas Anderson @ 2019-04-10 6:42 ` elaine.zhang 2019-04-10 15:25 ` Doug Anderson 2019-04-11 19:27 ` Heiko Stübner 1 sibling, 1 reply; 17+ messages in thread From: elaine.zhang @ 2019-04-10 6:42 UTC (permalink / raw) To: Douglas Anderson, Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, linux-clk, linux-kernel, linux-arm-kernel 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; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 2019-04-10 6:42 ` elaine.zhang @ 2019-04-10 15:25 ` Doug Anderson 2019-04-11 3:42 ` elaine.zhang 0 siblings, 1 reply; 17+ messages in thread From: Doug Anderson @ 2019-04-10 15:25 UTC (permalink / raw) To: elaine.zhang Cc: Heiko Stuebner, Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 2019-04-10 15:25 ` Doug Anderson @ 2019-04-11 3:42 ` elaine.zhang 2019-04-11 14:42 ` Doug Anderson 0 siblings, 1 reply; 17+ messages in thread From: elaine.zhang @ 2019-04-11 3:42 UTC (permalink / raw) To: Doug Anderson Cc: Heiko Stuebner, Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM 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 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 2019-04-11 3:42 ` elaine.zhang @ 2019-04-11 14:42 ` Doug Anderson 2019-04-11 19:20 ` Heiko Stübner 0 siblings, 1 reply; 17+ messages in thread From: Doug Anderson @ 2019-04-11 14:42 UTC (permalink / raw) To: elaine.zhang, Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 2019-04-11 14:42 ` Doug Anderson @ 2019-04-11 19:20 ` Heiko Stübner 0 siblings, 0 replies; 17+ messages in thread From: Heiko Stübner @ 2019-04-11 19:20 UTC (permalink / raw) To: Doug Anderson Cc: elaine.zhang, Michael Turquette, Stephen Boyd, Caesar Wang, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, linux-clk, LKML, Linux ARM 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 2019-04-09 20:47 ` [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 Douglas Anderson 2019-04-10 6:42 ` elaine.zhang @ 2019-04-11 19:27 ` Heiko Stübner 1 sibling, 0 replies; 17+ messages in thread From: Heiko Stübner @ 2019-04-11 19:27 UTC (permalink / raw) To: Douglas Anderson Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, Elaine Zhang, linux-clk, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] ARM: dts: rockchip: fix PWM clock found on RK3288 Socs 2019-04-09 20:47 [PATCH 0/3] rockchip: A few clock cleanups for rk3288 Douglas Anderson 2019-04-09 20:47 ` [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" Douglas Anderson 2019-04-09 20:47 ` [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 Douglas Anderson @ 2019-04-09 20:47 ` Douglas Anderson 2019-04-11 19:29 ` Heiko Stübner 2 siblings, 1 reply; 17+ messages in thread From: Douglas Anderson @ 2019-04-09 20:47 UTC (permalink / raw) To: Heiko Stuebner Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, Elaine Zhang, linux-clk, Douglas Anderson, devicetree, linux-kernel, Rob Herring, Mark Rutland, linux-arm-kernel From: Caesar Wang <caesar.wang@rock-chips.com> We use the new PWM IP on RK3288, but the PWM's clock indeed incorrect. Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- arch/arm/boot/dts/rk3288.dtsi | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi index 8bce2fc49b24..efdfa3737561 100644 --- a/arch/arm/boot/dts/rk3288.dtsi +++ b/arch/arm/boot/dts/rk3288.dtsi @@ -677,7 +677,7 @@ #pwm-cells = <3>; pinctrl-names = "default"; pinctrl-0 = <&pwm0_pin>; - clocks = <&cru PCLK_PWM>; + clocks = <&cru PCLK_RKPWM>; clock-names = "pwm"; status = "disabled"; }; @@ -688,7 +688,7 @@ #pwm-cells = <3>; pinctrl-names = "default"; pinctrl-0 = <&pwm1_pin>; - clocks = <&cru PCLK_PWM>; + clocks = <&cru PCLK_RKPWM>; clock-names = "pwm"; status = "disabled"; }; @@ -699,7 +699,7 @@ #pwm-cells = <3>; pinctrl-names = "default"; pinctrl-0 = <&pwm2_pin>; - clocks = <&cru PCLK_PWM>; + clocks = <&cru PCLK_RKPWM>; clock-names = "pwm"; status = "disabled"; }; @@ -710,7 +710,7 @@ #pwm-cells = <2>; pinctrl-names = "default"; pinctrl-0 = <&pwm3_pin>; - clocks = <&cru PCLK_PWM>; + clocks = <&cru PCLK_RKPWM>; clock-names = "pwm"; status = "disabled"; }; -- 2.21.0.392.gf8f6787159e-goog ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] ARM: dts: rockchip: fix PWM clock found on RK3288 Socs 2019-04-09 20:47 ` [PATCH 3/3] ARM: dts: rockchip: fix PWM clock found on RK3288 Socs Douglas Anderson @ 2019-04-11 19:29 ` Heiko Stübner 0 siblings, 0 replies; 17+ messages in thread From: Heiko Stübner @ 2019-04-11 19:29 UTC (permalink / raw) To: Douglas Anderson Cc: Michael Turquette, Stephen Boyd, Caesar Wang, linux-rockchip, mka, ryandcase, Elaine Zhang, linux-clk, devicetree, linux-kernel, Rob Herring, Mark Rutland, linux-arm-kernel Am Dienstag, 9. April 2019, 22:47:07 CEST schrieb Douglas Anderson: > From: Caesar Wang <caesar.wang@rock-chips.com> > > We use the new PWM IP on RK3288, but the PWM's clock indeed incorrect. > > Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> applied into a branch for 5._3_ ... as it requires a change coming through a different tree, the patch gets a "quarantine"-release in between. Thanks Heiko ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-04-12 15:41 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-09 20:47 [PATCH 0/3] rockchip: A few clock cleanups for rk3288 Douglas Anderson 2019-04-09 20:47 ` [PATCH 1/3] Revert "clk: rockchip: mark noc and some special clk as critical on rk3288" Douglas Anderson 2019-04-10 6:23 ` elaine.zhang 2019-04-10 15:34 ` Doug Anderson [not found] ` <1491b5f1-e9f9-5718-76e5-0a49814ed76d@rock-chips.com> 2019-04-11 15:26 ` Doug Anderson 2019-04-11 22:05 ` Heiko Stübner 2019-04-12 1:43 ` elaine.zhang 2019-04-12 15:41 ` Doug Anderson 2019-04-09 20:47 ` [PATCH 2/3] clk: rockchip: Make rkpwm a critical clock on rk3288 Douglas Anderson 2019-04-10 6:42 ` elaine.zhang 2019-04-10 15:25 ` Doug Anderson 2019-04-11 3:42 ` elaine.zhang 2019-04-11 14:42 ` Doug Anderson 2019-04-11 19:20 ` Heiko Stübner 2019-04-11 19:27 ` Heiko Stübner 2019-04-09 20:47 ` [PATCH 3/3] ARM: dts: rockchip: fix PWM clock found on RK3288 Socs Douglas Anderson 2019-04-11 19:29 ` Heiko Stübner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).