| Message ID | 20240308000432.32369-1-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present | expand |
On Thu, Mar 7, 2024 at 6:04 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Sometimes clocks provided to a consumer might not have .set_rate > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > set. In that case it's usually possible to find a parent up the tree > which is capable of setting the rate (div, pll, etc). Implement a simple > lookup procedure for such cases, to traverse the clock tree until > .set_rate capable parent is found, and use that parent to actually > change the rate. The search will stop once the first .set_rate capable > clock is found, which is usually enough to handle most cases. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- Hi Lukasz, Sean, Tom, If this patch looks good to you and there are no outstanding comments, can you please apply it? It's needed as a part of eMMC enablement for E850-96 board, as eMMC gate (leaf) clock is specified as ciu clock in dts, which requires further clock rate change propagation up to divider clock. Thanks! > drivers/clk/clk-uclass.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index ed6e60bc4841..755f05f34669 100644 > --- a/drivers/clk/clk-uclass.c > +++ b/drivers/clk/clk-uclass.c > @@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate) > return 0; > ops = clk_dev_ops(clk->dev); > > - if (!ops->set_rate) > - return -ENOSYS; > + /* Try to find parents which can set rate */ > + while (!ops->set_rate) { > + struct clk *parent; > + > + if (!(clk->flags & CLK_SET_RATE_PARENT)) > + return -ENOSYS; > + > + parent = clk_get_parent(clk); > + if (IS_ERR_OR_NULL(parent) || !clk_valid(parent)) > + return -ENODEV; > + > + clk = parent; > + ops = clk_dev_ops(clk->dev); > + } > > /* get private clock struct used for cache */ > clk_get_priv(clk, &clkp); > -- > 2.39.2 >
On 3/20/2024 2:42 AM, Sam Protsenko wrote: > On Thu, Mar 7, 2024 at 6:04 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: >> Sometimes clocks provided to a consumer might not have .set_rate >> operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag >> set. In that case it's usually possible to find a parent up the tree >> which is capable of setting the rate (div, pll, etc). Implement a simple >> lookup procedure for such cases, to traverse the clock tree until >> .set_rate capable parent is found, and use that parent to actually >> change the rate. The search will stop once the first .set_rate capable >> clock is found, which is usually enough to handle most cases. >> >> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >> --- > Hi Lukasz, Sean, Tom, > > If this patch looks good to you and there are no outstanding comments, > can you please apply it? It's needed as a part of eMMC enablement for > E850-96 board, as eMMC gate (leaf) clock is specified as ciu clock in > dts, which requires further clock rate change propagation up to > divider clock. > > Thanks! Please be patient. The release cycle is quite long. My patch set for HiSilicon clk framework has been ignored for ~2months already. > >> drivers/clk/clk-uclass.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c >> index ed6e60bc4841..755f05f34669 100644 >> --- a/drivers/clk/clk-uclass.c >> +++ b/drivers/clk/clk-uclass.c >> @@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate) >> return 0; >> ops = clk_dev_ops(clk->dev); >> >> - if (!ops->set_rate) >> - return -ENOSYS; >> + /* Try to find parents which can set rate */ >> + while (!ops->set_rate) { >> + struct clk *parent; >> + >> + if (!(clk->flags & CLK_SET_RATE_PARENT)) >> + return -ENOSYS; >> + >> + parent = clk_get_parent(clk); >> + if (IS_ERR_OR_NULL(parent) || !clk_valid(parent)) >> + return -ENODEV; >> + >> + clk = parent; >> + ops = clk_dev_ops(clk->dev); >> + } >> >> /* get private clock struct used for cache */ >> clk_get_priv(clk, &clkp); >> -- >> 2.39.2 >>
On 3/20/24 10:02, Yang Xiwen wrote: > On 3/20/2024 2:42 AM, Sam Protsenko wrote: >> On Thu, Mar 7, 2024 at 6:04 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: >>> Sometimes clocks provided to a consumer might not have .set_rate >>> operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag >>> set. In that case it's usually possible to find a parent up the tree >>> which is capable of setting the rate (div, pll, etc). Implement a simple >>> lookup procedure for such cases, to traverse the clock tree until >>> .set_rate capable parent is found, and use that parent to actually >>> change the rate. The search will stop once the first .set_rate capable >>> clock is found, which is usually enough to handle most cases. >>> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >>> --- >> Hi Lukasz, Sean, Tom, >> >> If this patch looks good to you and there are no outstanding comments, >> can you please apply it? It's needed as a part of eMMC enablement for >> E850-96 board, as eMMC gate (leaf) clock is specified as ciu clock in >> dts, which requires further clock rate change propagation up to >> divider clock. >> >> Thanks! > > > Please be patient. The release cycle is quite long. My patch set for HiSilicon clk framework has been ignored for ~2months already. Sorry, I've been busy IRL recently. I will try to review patches in the backlog, but it might be a few more weeks. --Sean
On 3/7/24 19:04, Sam Protsenko wrote: > Sometimes clocks provided to a consumer might not have .set_rate > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > set. In that case it's usually possible to find a parent up the tree > which is capable of setting the rate (div, pll, etc). Implement a simple > lookup procedure for such cases, to traverse the clock tree until > .set_rate capable parent is found, and use that parent to actually > change the rate. The search will stop once the first .set_rate capable > clock is found, which is usually enough to handle most cases. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/clk/clk-uclass.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index ed6e60bc4841..755f05f34669 100644 > --- a/drivers/clk/clk-uclass.c > +++ b/drivers/clk/clk-uclass.c > @@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate) > return 0; > ops = clk_dev_ops(clk->dev); > > - if (!ops->set_rate) > - return -ENOSYS; > + /* Try to find parents which can set rate */ > + while (!ops->set_rate) { > + struct clk *parent; > + > + if (!(clk->flags & CLK_SET_RATE_PARENT)) > + return -ENOSYS; > + > + parent = clk_get_parent(clk); > + if (IS_ERR_OR_NULL(parent) || !clk_valid(parent)) > + return -ENODEV; > + > + clk = parent; > + ops = clk_dev_ops(clk->dev); > + } > > /* get private clock struct used for cache */ > clk_get_priv(clk, &clkp); Can you give an example of where this is needed? --Sean
On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> wrote: > > On 3/7/24 19:04, Sam Protsenko wrote: > > Sometimes clocks provided to a consumer might not have .set_rate > > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > > set. In that case it's usually possible to find a parent up the tree > > which is capable of setting the rate (div, pll, etc). Implement a simple > > lookup procedure for such cases, to traverse the clock tree until > > .set_rate capable parent is found, and use that parent to actually > > change the rate. The search will stop once the first .set_rate capable > > clock is found, which is usually enough to handle most cases. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- [snip] > > Can you give an example of where this is needed? > Sure. In my case it's needed for eMMC enablement on E850-96 board. eMMC node in the device tree [1] is a gate clock (CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to change its rate. But that clock actually has CLK_SET_RATE_PARENT flag set in the clock driver [2]. So the right thing to do in this case (and that's basically how it's done in Linux kernel too) is to traverse the clock tree upwards, and try to find the parent capable to do .set_rate (which is usually a divider clock), and use it to change the clock rate. I'm working on exynos_dw_mmc driver [3] right now, making it use CCF clocks, but I can't do that before this patch is applied. Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag is also used in various IMX clock drivers and STM32MP13 clock driver. I guess without this change those flags will be basically ignored. Thanks! [1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/exynos850.dtsi#L408 [2] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/clk/exynos/clk-exynos850.c#L353 [3] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/exynos_dw_mmc.c#L117 > --Sean
On Wed, May 1, 2024 at 4:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> wrote: > > > > On 3/7/24 19:04, Sam Protsenko wrote: > > > Sometimes clocks provided to a consumer might not have .set_rate > > > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > > > set. In that case it's usually possible to find a parent up the tree > > > which is capable of setting the rate (div, pll, etc). Implement a simple > > > lookup procedure for such cases, to traverse the clock tree until > > > .set_rate capable parent is found, and use that parent to actually > > > change the rate. The search will stop once the first .set_rate capable > > > clock is found, which is usually enough to handle most cases. > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > --- > > [snip] > > > > > Can you give an example of where this is needed? > > > > Sure. In my case it's needed for eMMC enablement on E850-96 board. > eMMC node in the device tree [1] is a gate clock > (CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to change > its rate. But that clock actually has CLK_SET_RATE_PARENT flag set in > the clock driver [2]. So the right thing to do in this case (and > that's basically how it's done in Linux kernel too) is to traverse the > clock tree upwards, and try to find the parent capable to do .set_rate > (which is usually a divider clock), and use it to change the clock > rate. I'm working on exynos_dw_mmc driver [3] right now, making it use > CCF clocks, but I can't do that before this patch is applied. > > Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag is > also used in various IMX clock drivers and STM32MP13 clock driver. I > guess without this change those flags will be basically ignored. > > Thanks! > Hi Sean, Just wanted to check if you think my explanation above is ok and the patch can be applied? I'm finishing my new patch series for enabling MMC on E850-96, but this patch has to be applied first. Thanks! > [1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/exynos850.dtsi#L408 > [2] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/clk/exynos/clk-exynos850.c#L353 > [3] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/exynos_dw_mmc.c#L117 > > > --Sean
On Tue, May 14, 2024 at 3:26 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, May 1, 2024 at 4:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> wrote: > > > > > > On 3/7/24 19:04, Sam Protsenko wrote: > > > > Sometimes clocks provided to a consumer might not have .set_rate > > > > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > > > > set. In that case it's usually possible to find a parent up the tree > > > > which is capable of setting the rate (div, pll, etc). Implement a simple > > > > lookup procedure for such cases, to traverse the clock tree until > > > > .set_rate capable parent is found, and use that parent to actually > > > > change the rate. The search will stop once the first .set_rate capable > > > > clock is found, which is usually enough to handle most cases. > > > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > > --- > > > > [snip] > > > > > > > > Can you give an example of where this is needed? > > > > > > > Sure. In my case it's needed for eMMC enablement on E850-96 board. > > eMMC node in the device tree [1] is a gate clock > > (CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to change > > its rate. But that clock actually has CLK_SET_RATE_PARENT flag set in > > the clock driver [2]. So the right thing to do in this case (and > > that's basically how it's done in Linux kernel too) is to traverse the > > clock tree upwards, and try to find the parent capable to do .set_rate > > (which is usually a divider clock), and use it to change the clock > > rate. I'm working on exynos_dw_mmc driver [3] right now, making it use > > CCF clocks, but I can't do that before this patch is applied. > > > > Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag is > > also used in various IMX clock drivers and STM32MP13 clock driver. I > > guess without this change those flags will be basically ignored. > > > > Thanks! > > > > Hi Sean, > > Just wanted to check if you think my explanation above is ok and the > patch can be applied? I'm finishing my new patch series for enabling > MMC on E850-96, but this patch has to be applied first. > > Thanks! > Hi Tom, Would it be reasonable to ask you to take care of this patch? It's been ignored for 3 months now, but it's needed for eMMC enablement on E850-96 board. Thanks! > > [1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/exynos850.dtsi#L408 > > [2] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/clk/exynos/clk-exynos850.c#L353 > > [3] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/exynos_dw_mmc.c#L117 > > > > > --Sean
On Mon, Jun 10, 2024 at 02:43:17PM -0500, Sam Protsenko wrote: > On Tue, May 14, 2024 at 3:26 PM Sam Protsenko > <semen.protsenko@linaro.org> wrote: > > > > On Wed, May 1, 2024 at 4:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> wrote: > > > > > > > > On 3/7/24 19:04, Sam Protsenko wrote: > > > > > Sometimes clocks provided to a consumer might not have .set_rate > > > > > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > > > > > set. In that case it's usually possible to find a parent up the tree > > > > > which is capable of setting the rate (div, pll, etc). Implement a simple > > > > > lookup procedure for such cases, to traverse the clock tree until > > > > > .set_rate capable parent is found, and use that parent to actually > > > > > change the rate. The search will stop once the first .set_rate capable > > > > > clock is found, which is usually enough to handle most cases. > > > > > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > > > --- > > > > > > [snip] > > > > > > > > > > > Can you give an example of where this is needed? > > > > > > > > > > Sure. In my case it's needed for eMMC enablement on E850-96 board. > > > eMMC node in the device tree [1] is a gate clock > > > (CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to change > > > its rate. But that clock actually has CLK_SET_RATE_PARENT flag set in > > > the clock driver [2]. So the right thing to do in this case (and > > > that's basically how it's done in Linux kernel too) is to traverse the > > > clock tree upwards, and try to find the parent capable to do .set_rate > > > (which is usually a divider clock), and use it to change the clock > > > rate. I'm working on exynos_dw_mmc driver [3] right now, making it use > > > CCF clocks, but I can't do that before this patch is applied. > > > > > > Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag is > > > also used in various IMX clock drivers and STM32MP13 clock driver. I > > > guess without this change those flags will be basically ignored. > > > > > > Thanks! > > > > > > > Hi Sean, > > > > Just wanted to check if you think my explanation above is ok and the > > patch can be applied? I'm finishing my new patch series for enabling > > MMC on E850-96, but this patch has to be applied first. > > > > Thanks! > > > > Hi Tom, > > Would it be reasonable to ask you to take care of this patch? It's > been ignored for 3 months now, but it's needed for eMMC enablement on > E850-96 board. Any comments at this point Sean?
Hi all Working on the clock now. I have done a different implementation clk-mux, clk-gate I have added a clk_generic_set_rate that is a stub that propagate to the parent, I think that should work the same but wihout this while and even on preparation on more changes I need. Can be work on your case too? Michael On Thu, Jun 13, 2024 at 5:33 PM Tom Rini <trini@konsulko.com> wrote: > > On Mon, Jun 10, 2024 at 02:43:17PM -0500, Sam Protsenko wrote: > > On Tue, May 14, 2024 at 3:26 PM Sam Protsenko > > <semen.protsenko@linaro.org> wrote: > > > > > > On Wed, May 1, 2024 at 4:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > > > On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> wrote: > > > > > > > > > > On 3/7/24 19:04, Sam Protsenko wrote: > > > > > > Sometimes clocks provided to a consumer might not have .set_rate > > > > > > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > > > > > > set. In that case it's usually possible to find a parent up the tree > > > > > > which is capable of setting the rate (div, pll, etc). Implement a simple > > > > > > lookup procedure for such cases, to traverse the clock tree until > > > > > > .set_rate capable parent is found, and use that parent to actually > > > > > > change the rate. The search will stop once the first .set_rate capable > > > > > > clock is found, which is usually enough to handle most cases. > > > > > > > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > > > > --- > > > > > > > > [snip] > > > > > > > > > > > > > > Can you give an example of where this is needed? > > > > > > > > > > > > > Sure. In my case it's needed for eMMC enablement on E850-96 board. > > > > eMMC node in the device tree [1] is a gate clock > > > > (CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to change > > > > its rate. But that clock actually has CLK_SET_RATE_PARENT flag set in > > > > the clock driver [2]. So the right thing to do in this case (and > > > > that's basically how it's done in Linux kernel too) is to traverse the > > > > clock tree upwards, and try to find the parent capable to do .set_rate > > > > (which is usually a divider clock), and use it to change the clock > > > > rate. I'm working on exynos_dw_mmc driver [3] right now, making it use > > > > CCF clocks, but I can't do that before this patch is applied. > > > > > > > > Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag is > > > > also used in various IMX clock drivers and STM32MP13 clock driver. I > > > > guess without this change those flags will be basically ignored. > > > > > > > > Thanks! > > > > > > > > > > Hi Sean, > > > > > > Just wanted to check if you think my explanation above is ok and the > > > patch can be applied? I'm finishing my new patch series for enabling > > > MMC on E850-96, but this patch has to be applied first. > > > > > > Thanks! > > > > > > > Hi Tom, > > > > Would it be reasonable to ask you to take care of this patch? It's > > been ignored for 3 months now, but it's needed for eMMC enablement on > > E850-96 board. > > Any comments at this point Sean? > > -- > Tom
On Wed, Jul 3, 2024 at 10:00 AM Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote: > > Hi all > > Working on the clock now. I have done a different implementation > > clk-mux, clk-gate I have added a clk_generic_set_rate that is a stub > that propagate to the parent, I think that should work the same but > wihout this while and even on preparation on more > changes I need. Can be work on your case too? > Hi Michael, I've provided some thoughts on your gate/mux implementation here: [1]. As you've submitted your (different) implementation of the same feature, but 4 months later, I guess my patch doesn't cover your cases, or perhaps it has some design flaws? Please let me know. If anything, that shows there is a rising need in this propagation feature. I'd like to encourage the maintainers to take a look at both implementations, and maybe at least comment on why there is hesitance to accept this patch. It's been almost 5 months that I'm trying to push that through, and my MMC work (DDR mode on E850-96 board to be precise) still very much depends on this. Thanks! [1] https://lists.denx.de/pipermail/u-boot/2024-July/559753.html > Michael > > On Thu, Jun 13, 2024 at 5:33 PM Tom Rini <trini@konsulko.com> wrote: > > > > On Mon, Jun 10, 2024 at 02:43:17PM -0500, Sam Protsenko wrote: > > > On Tue, May 14, 2024 at 3:26 PM Sam Protsenko > > > <semen.protsenko@linaro.org> wrote: > > > > > > > > On Wed, May 1, 2024 at 4:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > > > > > On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> wrote: > > > > > > > > > > > > On 3/7/24 19:04, Sam Protsenko wrote: > > > > > > > Sometimes clocks provided to a consumer might not have .set_rate > > > > > > > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag > > > > > > > set. In that case it's usually possible to find a parent up the tree > > > > > > > which is capable of setting the rate (div, pll, etc). Implement a simple > > > > > > > lookup procedure for such cases, to traverse the clock tree until > > > > > > > .set_rate capable parent is found, and use that parent to actually > > > > > > > change the rate. The search will stop once the first .set_rate capable > > > > > > > clock is found, which is usually enough to handle most cases. > > > > > > > > > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > > > > > --- > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > Can you give an example of where this is needed? > > > > > > > > > > > > > > > > Sure. In my case it's needed for eMMC enablement on E850-96 board. > > > > > eMMC node in the device tree [1] is a gate clock > > > > > (CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to change > > > > > its rate. But that clock actually has CLK_SET_RATE_PARENT flag set in > > > > > the clock driver [2]. So the right thing to do in this case (and > > > > > that's basically how it's done in Linux kernel too) is to traverse the > > > > > clock tree upwards, and try to find the parent capable to do .set_rate > > > > > (which is usually a divider clock), and use it to change the clock > > > > > rate. I'm working on exynos_dw_mmc driver [3] right now, making it use > > > > > CCF clocks, but I can't do that before this patch is applied. > > > > > > > > > > Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag is > > > > > also used in various IMX clock drivers and STM32MP13 clock driver. I > > > > > guess without this change those flags will be basically ignored. > > > > > > > > > > Thanks! > > > > > > > > > > > > > Hi Sean, > > > > > > > > Just wanted to check if you think my explanation above is ok and the > > > > patch can be applied? I'm finishing my new patch series for enabling > > > > MMC on E850-96, but this patch has to be applied first. > > > > > > > > Thanks! > > > > > > > > > > Hi Tom, > > > > > > Would it be reasonable to ask you to take care of this patch? It's > > > been ignored for 3 months now, but it's needed for eMMC enablement on > > > E850-96 board. > > > > Any comments at this point Sean? > > > > -- > > Tom > > > > -- > Michael Nazzareno Trimarchi > Co-Founder & Chief Executive Officer > M. +39 347 913 2170 > michael@amarulasolutions.com > __________________________________ > > Amarula Solutions BV > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > T. +31 (0)85 111 9172 > info@amarulasolutions.com > www.amarulasolutions.com
Hi Sam Il mar 23 lug 2024, 22:53 Sam Protsenko <semen.protsenko@linaro.org> ha scritto: > On Wed, Jul 3, 2024 at 10:00 AM Michael Nazzareno Trimarchi > <michael@amarulasolutions.com> wrote: > > > > Hi all > > > > Working on the clock now. I have done a different implementation > > > > clk-mux, clk-gate I have added a clk_generic_set_rate that is a stub > > that propagate to the parent, I think that should work the same but > > wihout this while and even on preparation on more > > changes I need. Can be work on your case too? > > > > Hi Michael, > > I've provided some thoughts on your gate/mux implementation here: [1]. > As you've submitted your (different) implementation of the same > feature, but 4 months later, I guess my patch doesn't cover your > cases, or perhaps it has some design flaws? Please let me know. > > If anything, that shows there is a rising need in this propagation > feature. I'd like to encourage the maintainers to take a look at both > implementations, and maybe at least comment on why there is hesitance > to accept this patch. It's been almost 5 months that I'm trying to > push that through, and my MMC work (DDR mode on E850-96 board to be > precise) still very much depends on this. > My series is still not ready and I have no problem to go with your while for now Michael > > > > Thanks! > > [1] https://lists.denx.de/pipermail/u-boot/2024-July/559753.html > > > > Michael > > > > On Thu, Jun 13, 2024 at 5:33 PM Tom Rini <trini@konsulko.com> wrote: > > > > > > On Mon, Jun 10, 2024 at 02:43:17PM -0500, Sam Protsenko wrote: > > > > On Tue, May 14, 2024 at 3:26 PM Sam Protsenko > > > > <semen.protsenko@linaro.org> wrote: > > > > > > > > > > On Wed, May 1, 2024 at 4:12 PM Sam Protsenko < > semen.protsenko@linaro.org> wrote: > > > > > > > > > > > > On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> > wrote: > > > > > > > > > > > > > > On 3/7/24 19:04, Sam Protsenko wrote: > > > > > > > > Sometimes clocks provided to a consumer might not have > .set_rate > > > > > > > > operation (like gate or mux clocks), but have > CLK_SET_PARENT_RATE flag > > > > > > > > set. In that case it's usually possible to find a parent up > the tree > > > > > > > > which is capable of setting the rate (div, pll, etc). > Implement a simple > > > > > > > > lookup procedure for such cases, to traverse the clock tree > until > > > > > > > > .set_rate capable parent is found, and use that parent to > actually > > > > > > > > change the rate. The search will stop once the first > .set_rate capable > > > > > > > > clock is found, which is usually enough to handle most cases. > > > > > > > > > > > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > > > > > > --- > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > Can you give an example of where this is needed? > > > > > > > > > > > > > > > > > > > Sure. In my case it's needed for eMMC enablement on E850-96 > board. > > > > > > eMMC node in the device tree [1] is a gate clock > > > > > > (CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to > change > > > > > > its rate. But that clock actually has CLK_SET_RATE_PARENT flag > set in > > > > > > the clock driver [2]. So the right thing to do in this case (and > > > > > > that's basically how it's done in Linux kernel too) is to > traverse the > > > > > > clock tree upwards, and try to find the parent capable to do > .set_rate > > > > > > (which is usually a divider clock), and use it to change the > clock > > > > > > rate. I'm working on exynos_dw_mmc driver [3] right now, making > it use > > > > > > CCF clocks, but I can't do that before this patch is applied. > > > > > > > > > > > > Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag > is > > > > > > also used in various IMX clock drivers and STM32MP13 clock > driver. I > > > > > > guess without this change those flags will be basically ignored. > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > Hi Sean, > > > > > > > > > > Just wanted to check if you think my explanation above is ok and > the > > > > > patch can be applied? I'm finishing my new patch series for > enabling > > > > > MMC on E850-96, but this patch has to be applied first. > > > > > > > > > > Thanks! > > > > > > > > > > > > > Hi Tom, > > > > > > > > Would it be reasonable to ask you to take care of this patch? It's > > > > been ignored for 3 months now, but it's needed for eMMC enablement on > > > > E850-96 board. > > > > > > Any comments at this point Sean? > > > > > > -- > > > Tom > > > > > > > > -- > > Michael Nazzareno Trimarchi > > Co-Founder & Chief Executive Officer > > M. +39 347 913 2170 > > michael@amarulasolutions.com > > __________________________________ > > > > Amarula Solutions BV > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > > T. +31 (0)85 111 9172 > > info@amarulasolutions.com > > www.amarulasolutions.com >
[snip] >> >> If anything, that shows there is a rising need in this propagation >> feature. I'd like to encourage the maintainers to take a look at both >> implementations, and maybe at least comment on why there is hesitance >> to accept this patch. It's been almost 5 months that I'm trying to >> push that through, and my MMC work (DDR mode on E850-96 board to be >> precise) still very much depends on this. > > My series is still not ready and I have no problem to go with your while for now > Hi Sean, Can you please apply this patch, if it's ok with you? Thanks! [snip]
On Thu, Jul 25, 2024 at 3:57 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > [snip] > > >> > >> If anything, that shows there is a rising need in this propagation > >> feature. I'd like to encourage the maintainers to take a look at both > >> implementations, and maybe at least comment on why there is hesitance > >> to accept this patch. It's been almost 5 months that I'm trying to > >> push that through, and my MMC work (DDR mode on E850-96 board to be > >> precise) still very much depends on this. > > > > My series is still not ready and I have no problem to go with your while for now > > > > Hi Sean, > > Can you please apply this patch, if it's ok with you? > Gentle reminder. > Thanks! > > [snip]
Hi Sean, This series has been pending for 5 months now. If there are no objections, can you please apply it? Thanks! On Wed, Jul 31, 2024 at 3:05 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Thu, Jul 25, 2024 at 3:57 PM Sam Protsenko > <semen.protsenko@linaro.org> wrote: > > > > [snip] > > > > >> > > >> If anything, that shows there is a rising need in this propagation > > >> feature. I'd like to encourage the maintainers to take a look at both > > >> implementations, and maybe at least comment on why there is hesitance > > >> to accept this patch. It's been almost 5 months that I'm trying to > > >> push that through, and my MMC work (DDR mode on E850-96 board to be > > >> precise) still very much depends on this. > > > > > > My series is still not ready and I have no problem to go with your while for now > > > > > > > Hi Sean, > > > > Can you please apply this patch, if it's ok with you? > > > > Gentle reminder. > > > Thanks! > > > > [snip]
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index ed6e60bc4841..755f05f34669 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate) return 0; ops = clk_dev_ops(clk->dev); - if (!ops->set_rate) - return -ENOSYS; + /* Try to find parents which can set rate */ + while (!ops->set_rate) { + struct clk *parent; + + if (!(clk->flags & CLK_SET_RATE_PARENT)) + return -ENOSYS; + + parent = clk_get_parent(clk); + if (IS_ERR_OR_NULL(parent) || !clk_valid(parent)) + return -ENODEV; + + clk = parent; + ops = clk_dev_ops(clk->dev); + } /* get private clock struct used for cache */ clk_get_priv(clk, &clkp);
Sometimes clocks provided to a consumer might not have .set_rate operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag set. In that case it's usually possible to find a parent up the tree which is capable of setting the rate (div, pll, etc). Implement a simple lookup procedure for such cases, to traverse the clock tree until .set_rate capable parent is found, and use that parent to actually change the rate. The search will stop once the first .set_rate capable clock is found, which is usually enough to handle most cases. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/clk/clk-uclass.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)