| Message ID | 20241025105620.1891596-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [RFC] clk: sunxi-ng: h616: Reparent CPU clock during frequency changes | expand |
On Fri, Oct 25, 2024 at 6:56 PM Andre Przywara <andre.przywara@arm.com> wrote: > > The H616 user manual recommends to re-parent the CPU clock during > frequency changes of the PLL, and recommends PLL_PERI0(1X), which runs > at 600 MHz. Also it asks to disable and then re-enable the PLL lock bit, > after the factor changes have been applied. > > Add clock notifiers for the PLL and the CPU mux clock, using the existing > notifier callbacks, and tell them to use mux 4 (the PLL_PERI0(1X) source), > and bit 29 (the LOCK_ENABLE) bit. The existing code already follows the > correct algorithms. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > the manual states that those changes would be needed to safely change > the CPU_PLL frequency during DVFS operation. On my H618 boards it works > fine without them, but Philippe reported problems on his H700 board. > Posting this for reference at this point, to see if it helps people. > I am not sure we should change this without it fixing any real issues. IIRC we do this for all the other SoCs. But if you want to be cautious, we can wait for Philippe to give a Tested-by? ChenYu > The same algorithm would apply to the A100/A133 (and the upcoming A523) > as well. > > Cheers, > Andre > > drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 28 ++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c > index 84e406ddf9d12..85eea196f25e3 100644 > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c > @@ -1095,11 +1095,24 @@ static const u32 usb2_clk_regs[] = { > SUN50I_H616_USB3_CLK_REG, > }; > > +static struct ccu_mux_nb sun50i_h616_cpu_nb = { > + .common = &cpux_clk.common, > + .cm = &cpux_clk.mux, > + .delay_us = 1, /* manual doesn't really say */ > + .bypass_index = 4, /* PLL_PERI0@600MHz, as recommended by manual */ > +}; > + > +static struct ccu_pll_nb sun50i_h616_pll_cpu_nb = { > + .common = &pll_cpux_clk.common, > + .enable = BIT(29), /* LOCK_ENABLE */ > + .lock = BIT(28), > +}; > + > static int sun50i_h616_ccu_probe(struct platform_device *pdev) > { > void __iomem *reg; > u32 val; > - int i; > + int ret, i; > > reg = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(reg)) > @@ -1152,7 +1165,18 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev) > val |= BIT(24); > writel(val, reg + SUN50I_H616_HDMI_CEC_CLK_REG); > > - return devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc); > + ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc); > + if (ret) > + return ret; > + > + /* Reparent CPU during CPU PLL rate changes */ > + ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk, > + &sun50i_h616_cpu_nb); > + > + /* Re-lock the CPU PLL after any rate changes */ > + ccu_pll_notifier_register(&sun50i_h616_pll_cpu_nb); > + > + return 0; > } > > static const struct of_device_id sun50i_h616_ccu_ids[] = { > -- > 2.25.1 >
On Fri, 25 Oct 2024 22:49:27 +0800 Chen-Yu Tsai <wens@csie.org> wrote: Hi, > On Fri, Oct 25, 2024 at 6:56 PM Andre Przywara <andre.przywara@arm.com> wrote: > > > > The H616 user manual recommends to re-parent the CPU clock during > > frequency changes of the PLL, and recommends PLL_PERI0(1X), which runs > > at 600 MHz. Also it asks to disable and then re-enable the PLL lock bit, > > after the factor changes have been applied. > > > > Add clock notifiers for the PLL and the CPU mux clock, using the existing > > notifier callbacks, and tell them to use mux 4 (the PLL_PERI0(1X) source), > > and bit 29 (the LOCK_ENABLE) bit. The existing code already follows the > > correct algorithms. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > Hi, > > > > the manual states that those changes would be needed to safely change > > the CPU_PLL frequency during DVFS operation. On my H618 boards it works > > fine without them, but Philippe reported problems on his H700 board. > > Posting this for reference at this point, to see if it helps people. > > I am not sure we should change this without it fixing any real issues. > > IIRC we do this for all the other SoCs. But if you want to be cautious, > we can wait for Philippe to give a Tested-by? Yes, I copied this code from the A64 CCU, but IIRC this was desperately needed there. But so far I didn't hear many complaints on the H616, and I ran through like 100,000 transistions in a matter on minutes without any issues yesterday. And apparently this patch doesn't fix Philippe's immediate problem, so I would like to hold it back for now, until we have either more testing, with or without this patch. Thanks, Andre > ChenYu > > > The same algorithm would apply to the A100/A133 (and the upcoming A523) > > as well. > > > > Cheers, > > Andre > > > > drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 28 ++++++++++++++++++++++++-- > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c > > index 84e406ddf9d12..85eea196f25e3 100644 > > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c > > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c > > @@ -1095,11 +1095,24 @@ static const u32 usb2_clk_regs[] = { > > SUN50I_H616_USB3_CLK_REG, > > }; > > > > +static struct ccu_mux_nb sun50i_h616_cpu_nb = { > > + .common = &cpux_clk.common, > > + .cm = &cpux_clk.mux, > > + .delay_us = 1, /* manual doesn't really say */ > > + .bypass_index = 4, /* PLL_PERI0@600MHz, as recommended by manual */ > > +}; > > + > > +static struct ccu_pll_nb sun50i_h616_pll_cpu_nb = { > > + .common = &pll_cpux_clk.common, > > + .enable = BIT(29), /* LOCK_ENABLE */ > > + .lock = BIT(28), > > +}; > > + > > static int sun50i_h616_ccu_probe(struct platform_device *pdev) > > { > > void __iomem *reg; > > u32 val; > > - int i; > > + int ret, i; > > > > reg = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(reg)) > > @@ -1152,7 +1165,18 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev) > > val |= BIT(24); > > writel(val, reg + SUN50I_H616_HDMI_CEC_CLK_REG); > > > > - return devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc); > > + ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc); > > + if (ret) > > + return ret; > > + > > + /* Reparent CPU during CPU PLL rate changes */ > > + ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk, > > + &sun50i_h616_cpu_nb); > > + > > + /* Re-lock the CPU PLL after any rate changes */ > > + ccu_pll_notifier_register(&sun50i_h616_pll_cpu_nb); > > + > > + return 0; > > } > > > > static const struct of_device_id sun50i_h616_ccu_ids[] = { > > -- > > 2.25.1 > >
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c index 84e406ddf9d12..85eea196f25e3 100644 --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c @@ -1095,11 +1095,24 @@ static const u32 usb2_clk_regs[] = { SUN50I_H616_USB3_CLK_REG, }; +static struct ccu_mux_nb sun50i_h616_cpu_nb = { + .common = &cpux_clk.common, + .cm = &cpux_clk.mux, + .delay_us = 1, /* manual doesn't really say */ + .bypass_index = 4, /* PLL_PERI0@600MHz, as recommended by manual */ +}; + +static struct ccu_pll_nb sun50i_h616_pll_cpu_nb = { + .common = &pll_cpux_clk.common, + .enable = BIT(29), /* LOCK_ENABLE */ + .lock = BIT(28), +}; + static int sun50i_h616_ccu_probe(struct platform_device *pdev) { void __iomem *reg; u32 val; - int i; + int ret, i; reg = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(reg)) @@ -1152,7 +1165,18 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev) val |= BIT(24); writel(val, reg + SUN50I_H616_HDMI_CEC_CLK_REG); - return devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc); + ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc); + if (ret) + return ret; + + /* Reparent CPU during CPU PLL rate changes */ + ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk, + &sun50i_h616_cpu_nb); + + /* Re-lock the CPU PLL after any rate changes */ + ccu_pll_notifier_register(&sun50i_h616_pll_cpu_nb); + + return 0; } static const struct of_device_id sun50i_h616_ccu_ids[] = {
The H616 user manual recommends to re-parent the CPU clock during frequency changes of the PLL, and recommends PLL_PERI0(1X), which runs at 600 MHz. Also it asks to disable and then re-enable the PLL lock bit, after the factor changes have been applied. Add clock notifiers for the PLL and the CPU mux clock, using the existing notifier callbacks, and tell them to use mux 4 (the PLL_PERI0(1X) source), and bit 29 (the LOCK_ENABLE) bit. The existing code already follows the correct algorithms. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- Hi, the manual states that those changes would be needed to safely change the CPU_PLL frequency during DVFS operation. On my H618 boards it works fine without them, but Philippe reported problems on his H700 board. Posting this for reference at this point, to see if it helps people. I am not sure we should change this without it fixing any real issues. The same algorithm would apply to the A100/A133 (and the upcoming A523) as well. Cheers, Andre drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 28 ++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)