| Message ID | 1521104071-19595-1-git-send-email-hl@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Huang, On 2018/3/15 16:54, Lin Huang wrote: > Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can > assign frequency for them in dts. I'm curious under which condition that we need assign frequency for hclk_sd? > > Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee > Signed-off-by: Lin Huang <hl@rock-chips.com> > --- > drivers/clk/rockchip/clk-rk3399.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c > index 6847120..a29c99e 100644 > --- a/drivers/clk/rockchip/clk-rk3399.c > +++ b/drivers/clk/rockchip/clk-rk3399.c > @@ -670,7 +670,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = { > RK3399_CLKGATE_CON(9), 7, GFLAGS, > &rk3399_uart3_fracmux), > > - COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED, > + COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED, > RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS, > RK3399_CLKGATE_CON(3), 4, GFLAGS), > > @@ -886,7 +886,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = { > RK3399_CLKGATE_CON(31), 8, GFLAGS), > > /* sdio & sdmmc */ > - COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, > + COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, > RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS, > RK3399_CLKGATE_CON(12), 13, GFLAGS), > GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0, >
Hi Shawn, On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote: > Hi Huang, > > On 2018/3/15 16:54, Lin Huang wrote: >> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can >> assign frequency for them in dts. > > I'm curious under which condition that we need assign frequency for > hclk_sd? We may set CPLL frequency higher than 800MHz, with that we need assign clock to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru register value to get hclk_sd for now. you can check https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 > >> >> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee >> Signed-off-by: Lin Huang <hl@rock-chips.com> >> --- >> drivers/clk/rockchip/clk-rk3399.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/rockchip/clk-rk3399.c >> b/drivers/clk/rockchip/clk-rk3399.c >> index 6847120..a29c99e 100644 >> --- a/drivers/clk/rockchip/clk-rk3399.c >> +++ b/drivers/clk/rockchip/clk-rk3399.c >> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch >> rk3399_clk_branches[] __initdata = { >> RK3399_CLKGATE_CON(9), 7, GFLAGS, >> &rk3399_uart3_fracmux), >> - COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, >> CLK_IGNORE_UNUSED, >> + COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, >> CLK_IGNORE_UNUSED, >> RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS, >> RK3399_CLKGATE_CON(3), 4, GFLAGS), >> @@ -886,7 +886,7 @@ static struct rockchip_clk_branch >> rk3399_clk_branches[] __initdata = { >> RK3399_CLKGATE_CON(31), 8, GFLAGS), >> /* sdio & sdmmc */ >> - COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, >> + COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, >> RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS, >> RK3399_CLKGATE_CON(12), 13, GFLAGS), >> GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0, >> > > >
[Correct Doug's email address] On 2018/3/15 17:12, hl wrote: > Hi Shawn, > > > On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote: >> Hi Huang, >> >> On 2018/3/15 16:54, Lin Huang wrote: >>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can >>> assign frequency for them in dts. >> >> I'm curious under which condition that we need assign frequency for >> hclk_sd? > We may set CPLL frequency higher than 800MHz, with that we need assign > clock > to hclk_sd, otherwise it will exceed 200MHz, since we use the default > cru register value to get hclk_sd for now. > you can check > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 > Okay, thanks for pointing me to the actual user case. I'm fine with that, however, what I am thinking now is: (1) It's worth elaborating more in the commit msg. (2) The reason you need set hclk_sd is that it depends on the defualt settings but lack real owner to explicitly set it to <=200MHz. So my another question is if that is more about aspect of power consumption, then it looks okay to me. But if that is a SoC limitation, then we are under risk for that. Either we should never rely on the default settings, or we should set its rate range after registering this clock provider. Of course, I'm not a clk expert, but just want to learn more bits from you guys. :) >> >>> >>> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee >>> Signed-off-by: Lin Huang <hl@rock-chips.com> >>> --- >>> drivers/clk/rockchip/clk-rk3399.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/clk/rockchip/clk-rk3399.c >>> b/drivers/clk/rockchip/clk-rk3399.c >>> index 6847120..a29c99e 100644 >>> --- a/drivers/clk/rockchip/clk-rk3399.c >>> +++ b/drivers/clk/rockchip/clk-rk3399.c >>> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch >>> rk3399_clk_branches[] __initdata = { >>> RK3399_CLKGATE_CON(9), 7, GFLAGS, >>> &rk3399_uart3_fracmux), >>> - COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, >>> CLK_IGNORE_UNUSED, >>> + COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, >>> CLK_IGNORE_UNUSED, >>> RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS, >>> RK3399_CLKGATE_CON(3), 4, GFLAGS), >>> @@ -886,7 +886,7 @@ static struct rockchip_clk_branch >>> rk3399_clk_branches[] __initdata = { >>> RK3399_CLKGATE_CON(31), 8, GFLAGS), >>> /* sdio & sdmmc */ >>> - COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, >>> + COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, >>> RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS, >>> RK3399_CLKGATE_CON(12), 13, GFLAGS), >>> GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0, >>> >> >> >> > > > >
Hi, On Thu, Mar 15, 2018 at 6:40 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > [Correct Doug's email address] > > On 2018/3/15 17:12, hl wrote: >> >> Hi Shawn, >> >> >> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote: >>> >>> Hi Huang, >>> >>> On 2018/3/15 16:54, Lin Huang wrote: >>>> >>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can >>>> assign frequency for them in dts. >>> >>> >>> I'm curious under which condition that we need assign frequency for >>> hclk_sd? >> >> We may set CPLL frequency higher than 800MHz, with that we need assign >> clock >> to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru >> register value to get hclk_sd for now. >> you can check >> >> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 > > > Okay, thanks for pointing me to the actual user case. > I'm fine with that, however, what I am thinking now is: > > (1) It's worth elaborating more in the commit msg. Seems like a nice idea to me. > (2) The reason you need set hclk_sd is that it depends on the > defualt settings but lack real owner to explicitly set it to <=200MHz. Actually, in the case of hclk_sd there _is_ an owner to set it to <= 200 MHz. I'll comment on the gerrit review, but the assigned clock for hclk_sd probably belongs in the node "sdmmc: dwmmc@fe320000". ...but in any case, you still need the clock ID to do that. > So my another question is if that is more about aspect of power > consumption, then it looks okay to me. But if that is a SoC limitation, > then we are under risk for that. Either we should never rely on the > default settings, or we should set its rate range after registering this > clock provider. I have always wondered about perhaps encoding the min/max clock rate for each clock somewhere in the source code. Then whenever we change clock rates we could enforce that we don't ever go past this min/max. In think, in theory, this is possible by registering for all the right callbacks / notifications. ...but I suspect that doing in a generic / performant way might be very difficult. Specifically, whenever a clock changes you may need to make all sorts of decisions about re-parenting and also checking whether all your children are out fo spec. So without encoding the min/max and having it magically auto-resolve, the best we can do is just to assign a sane clock rate. If there's no subsystem owning this clock then doing so at clock init time is sane (and that's what we do with many clocks). If a subsystem owns it, doing it when the subsystem is probed is sane. So, to summarize, I'm happy with this change. I wouldn't mind a little more justification in the CL description but personally I won't make a big deal about it. Reviewed-by: Douglas Anderson <dianders@chromium.org> -Doug
Hi Doug, On 2018/3/16 0:20, Doug Anderson wrote: > Hi, > > On Thu, Mar 15, 2018 at 6:40 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> [Correct Doug's email address] >> >> On 2018/3/15 17:12, hl wrote: >>> >>> Hi Shawn, >>> >>> >>> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote: >>>> >>>> Hi Huang, >>>> >>>> On 2018/3/15 16:54, Lin Huang wrote: >>>>> >>>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can >>>>> assign frequency for them in dts. >>>> >>>> >>>> I'm curious under which condition that we need assign frequency for >>>> hclk_sd? >>> >>> We may set CPLL frequency higher than 800MHz, with that we need assign >>> clock >>> to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru >>> register value to get hclk_sd for now. >>> you can check >>> >>> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 >> >> >> Okay, thanks for pointing me to the actual user case. >> I'm fine with that, however, what I am thinking now is: >> >> (1) It's worth elaborating more in the commit msg. > > Seems like a nice idea to me. > > >> (2) The reason you need set hclk_sd is that it depends on the >> defualt settings but lack real owner to explicitly set it to <=200MHz. > > Actually, in the case of hclk_sd there _is_ an owner to set it to <= > 200 MHz. I'll comment on the gerrit review, but the assigned clock > for hclk_sd probably belongs in the node "sdmmc: dwmmc@fe320000". Agreed. hclk_sd is the parent for hclk_sdmmc and hclk_sdmmc_noc, so it makes more sense to me for sdmmc node to pick it up in rk3399.dtsi > > ...but in any case, you still need the clock ID to do that. > yep. > >> So my another question is if that is more about aspect of power >> consumption, then it looks okay to me. But if that is a SoC limitation, >> then we are under risk for that. Either we should never rely on the >> default settings, or we should set its rate range after registering this >> clock provider. > > I have always wondered about perhaps encoding the min/max clock rate > for each clock somewhere in the source code. Then whenever we change > clock rates we could enforce that we don't ever go past this min/max. > In think, in theory, this is possible by registering for all the right > callbacks / notifications. ...but I suspect that doing in a generic / > performant way might be very difficult. Specifically, whenever a > clock changes you may need to make all sorts of decisions about > re-parenting and also checking whether all your children are out fo > spec. Agreed. > > So without encoding the min/max and having it magically auto-resolve, > the best we can do is just to assign a sane clock rate. If there's no > subsystem owning this clock then doing so at clock init time is sane > (and that's what we do with many clocks). If a subsystem owns it, > doing it when the subsystem is probed is sane. > I'm not convinced it was invented to abuse encoding the rate range for each clock, but just enclose the the hardware limitation within the clock framework and per-SoC clock providers. That being said, the author of per-SoC clock providers is more fimilar with the hardware limitation than BSP guys, so it's less prone to make fatal mistake by encoding this kinda limitation somewhere in clk provider drivers. But yes, I didn't see any hardware limitation here for hclk_sd, so my best guess is 200MHz is a tradeoff for perf and power. This looks sane to me by doing it at clock init time, so FWIW for this patch, Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com> However, I'm incline to assigne hclk_sd by sdmmc node in Huang's CL. Then we may never warry about leaving it along again when we randomly adjust clk hierarchy again. It applies to all this kinda clk providers. > > So, to summarize, I'm happy with this change. I wouldn't mind a > little more justification in the CL description but personally I won't > make a big deal about it. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > > -Doug > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > > >
diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c index 6847120..a29c99e 100644 --- a/drivers/clk/rockchip/clk-rk3399.c +++ b/drivers/clk/rockchip/clk-rk3399.c @@ -670,7 +670,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = { RK3399_CLKGATE_CON(9), 7, GFLAGS, &rk3399_uart3_fracmux), - COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED, + COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED, RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS, RK3399_CLKGATE_CON(3), 4, GFLAGS), @@ -886,7 +886,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = { RK3399_CLKGATE_CON(31), 8, GFLAGS), /* sdio & sdmmc */ - COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, + COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0, RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS, RK3399_CLKGATE_CON(12), 13, GFLAGS), GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can assign frequency for them in dts. Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee Signed-off-by: Lin Huang <hl@rock-chips.com> --- drivers/clk/rockchip/clk-rk3399.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)