Re: [linux-sunxi] Re: [PATCH 5/8] clk: sunxi-ng: Add support for the Allwinner H616 CCU
在 2020-12-02星期三的 23:06 +，André Przywara写道：
> On 02/12/2020 21:03, Jernej Škrabec wrote:
> > Dne sreda, 02. december 2020 ob 14:54:06 CET je Andre Przywara
> > napisal(a):
> > > While the clocks are fairly similar to the H6, many differ in
> > > tiny
> > > details, so a separate clock driver seems indicated.
> > >
> > > Derived from the H6 clock driver, and adjusted according to the
> > > manual.
> > >
> > > Signed-off-by: Andre Przywara
> > > ---
> > > drivers/clk/sunxi-ng/Kconfig|7 +-
> > > drivers/clk/sunxi-ng/Makefile |1 +
> > > drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 1134
> > > +++
> > > drivers/clk/sunxi-ng/ccu-sun50i-h616.h | 58 +
> > > include/dt-bindings/clock/sun50i-h616-ccu.h | 110 ++
> > > include/dt-bindings/reset/sun50i-h616-ccu.h | 67 ++
> > > 6 files changed, 1376 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > > create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.h
> > > create mode 100644 include/dt-bindings/clock/sun50i-h616-ccu.h
> > > create mode 100644 include/dt-bindings/reset/sun50i-h616-ccu.h
> > >
> > > diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-
> > > ng/Kconfig
> > > index ce5f5847d5d3..cd46d8853876 100644
> > > --- a/drivers/clk/sunxi-ng/Kconfig
> > > +++ b/drivers/clk/sunxi-ng/Kconfig
>
>
>
> > > +static int sun50i_h616_ccu_probe(struct platform_device *pdev)
> > > +{
> > > + struct resource *res;
> > > + void __iomem *reg;
> > > + u32 val;
> > > + int i;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + reg = devm_ioremap_resource(>dev, res);
> > > + if (IS_ERR(reg))
> > > + return PTR_ERR(reg);
> > > +
> > > + /* Enable the lock bits on all PLLs */
> > > + for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
> > > + val = readl(reg + pll_regs[i]);
> > > + val |= BIT(29);
> >
> > You should also add BIT(27) here. It enables PLL output (new
> > functionality in
> > H616). Without this only clocks which are child of PLL_CPUX and
> > PLL_PERIPH0
> > would work (set up by U-Boot). I'm pretty sure that's not intended
> > usage but
> > until we know better, it's ok imo.
>
> Ah right, the output enable bit. I was wondering if the A100 solution
> would better here: use bit 27 as the .enable value, and actually
> enable
Why not .enable = BIT(27) | BIT(31) ?
> (bit31) all PLLs here.
> Or we add another field, maybe a flag, to allow the kernel to decide
> which bit to use. Clement suggested something like that on IRC.
> But for now I can surely just set bit 27 here as well.
>
> > > + writel(val, reg + pll_regs[i]);
> > > + }
> > > +
> > > + /*
> > > + * Force the output divider of video PLLs to 0.
> > > + *
> > > + * See the comment before pll-video0 definition for the reason.
> > > + */
> > > + for (i = 0; i < ARRAY_SIZE(pll_video_regs); i++) {
> > > + val = readl(reg + pll_video_regs[i]);
> > > + val &= ~BIT(0);
> > > + writel(val, reg + pll_video_regs[i]);
> > > + }
> > > +
> > > + /*
> > > + * Force OHCI 12M clock sources to 00 (12MHz divided from
> > > 48MHz)
> > > + *
> > > + * This clock mux is still mysterious, and the code just
> > > enforces
> > > + * it to have a valid clock parent.
> > > + */
> > > + for (i = 0; i < ARRAY_SIZE(usb2_clk_regs); i++) {
> > > + val = readl(reg + usb2_clk_regs[i]);
> > > + val &= ~GENMASK(25, 24);
> > > + writel (val, reg + usb2_clk_regs[i]);
> > > + }
> > > +
> > > + /*
> > > + * Force the post-divider of pll-audio to 12 and the output
> > > divider
> > > + * of it to 2, so 24576000 and 22579200 rates can be set
> > > exactly.
> > > + */
> > > + val = readl(reg + SUN50I_H616_PLL_AUDIO_REG);
> > > + val &= ~(GENMASK(21, 16) | BIT(0));
> > > + writel(val | (11 << 16) | BIT(0), reg +
> > > SUN50I_H616_PLL_AUDIO_REG);
> > > +
> > > + /*
> > > + * First clock parent (osc32K) is unusable for CEC. But since
> > > there
> > > + * is no good way to force parent switch (both run with same
> > > frequency),
> > > + * just set second clock parent here.
> > > + */
> > > + val = readl(reg + SUN50I_H616_HDMI_CEC_CLK_REG);
> > > + val |= BIT(24);
> > > + writel(val, reg + SUN50I_H616_HDMI_CEC_CLK_REG);
> > > +
> > > + return sunxi_ccu_probe(pdev->dev.of_node, reg,
> > > _h616_ccu_desc);
> > > +}
> > > +
> > > +static const struct of_device_id sun50i_h616_ccu_ids[] = {
> > > + { .compatible = "allwinner,sun50i-h616-ccu",
> > > + .data = _h616_ccu_desc },
> > > + { }
> > > +};
> > > +
> > > +static struct platform_driver sun50i_h616_ccu_driver = {
> > > + .probe = sun50i_h616_ccu_probe,
> > > + .driver = {
> > > + .name = "sun50i-h616-ccu",
> > > + .of_match_table = sun50i_h616_ccu_ids,
> > > + },
> > > +};
> > > +builtin_platform_driver(sun50i_h616_ccu_driver);
> >
> > Please use CLK_OF_DECLARE() instead. That way clocks will be
> > initialized
>
[linux-sunxi] Re: [PATCH 5/8] clk: sunxi-ng: Add support for the Allwinner H616 CCU
On 02/12/2020 21:03, Jernej Škrabec wrote:
> Dne sreda, 02. december 2020 ob 14:54:06 CET je Andre Przywara napisal(a):
>> While the clocks are fairly similar to the H6, many differ in tiny
>> details, so a separate clock driver seems indicated.
>>
>> Derived from the H6 clock driver, and adjusted according to the manual.
>>
>> Signed-off-by: Andre Przywara
>> ---
>> drivers/clk/sunxi-ng/Kconfig|7 +-
>> drivers/clk/sunxi-ng/Makefile |1 +
>> drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 1134 +++
>> drivers/clk/sunxi-ng/ccu-sun50i-h616.h | 58 +
>> include/dt-bindings/clock/sun50i-h616-ccu.h | 110 ++
>> include/dt-bindings/reset/sun50i-h616-ccu.h | 67 ++
>> 6 files changed, 1376 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.c
>> create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.h
>> create mode 100644 include/dt-bindings/clock/sun50i-h616-ccu.h
>> create mode 100644 include/dt-bindings/reset/sun50i-h616-ccu.h
>>
>> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
>> index ce5f5847d5d3..cd46d8853876 100644
>> --- a/drivers/clk/sunxi-ng/Kconfig
>> +++ b/drivers/clk/sunxi-ng/Kconfig
>> +static int sun50i_h616_ccu_probe(struct platform_device *pdev)
>> +{
>> +struct resource *res;
>> +void __iomem *reg;
>> +u32 val;
>> +int i;
>> +
>> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +reg = devm_ioremap_resource(>dev, res);
>> +if (IS_ERR(reg))
>> +return PTR_ERR(reg);
>> +
>> +/* Enable the lock bits on all PLLs */
>> +for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
>> +val = readl(reg + pll_regs[i]);
>> +val |= BIT(29);
>
> You should also add BIT(27) here. It enables PLL output (new functionality in
> H616). Without this only clocks which are child of PLL_CPUX and PLL_PERIPH0
> would work (set up by U-Boot). I'm pretty sure that's not intended usage but
> until we know better, it's ok imo.
Ah right, the output enable bit. I was wondering if the A100 solution
would better here: use bit 27 as the .enable value, and actually enable
(bit31) all PLLs here.
Or we add another field, maybe a flag, to allow the kernel to decide
which bit to use. Clement suggested something like that on IRC.
But for now I can surely just set bit 27 here as well.
>> +writel(val, reg + pll_regs[i]);
>> +}
>> +
>> +/*
>> + * Force the output divider of video PLLs to 0.
>> + *
>> + * See the comment before pll-video0 definition for the reason.
>> + */
>> +for (i = 0; i < ARRAY_SIZE(pll_video_regs); i++) {
>> +val = readl(reg + pll_video_regs[i]);
>> +val &= ~BIT(0);
>> +writel(val, reg + pll_video_regs[i]);
>> +}
>> +
>> +/*
>> + * Force OHCI 12M clock sources to 00 (12MHz divided from 48MHz)
>> + *
>> + * This clock mux is still mysterious, and the code just enforces
>> + * it to have a valid clock parent.
>> + */
>> +for (i = 0; i < ARRAY_SIZE(usb2_clk_regs); i++) {
>> +val = readl(reg + usb2_clk_regs[i]);
>> +val &= ~GENMASK(25, 24);
>> +writel (val, reg + usb2_clk_regs[i]);
>> +}
>> +
>> +/*
>> + * Force the post-divider of pll-audio to 12 and the output divider
>> + * of it to 2, so 24576000 and 22579200 rates can be set exactly.
>> + */
>> +val = readl(reg + SUN50I_H616_PLL_AUDIO_REG);
>> +val &= ~(GENMASK(21, 16) | BIT(0));
>> +writel(val | (11 << 16) | BIT(0), reg + SUN50I_H616_PLL_AUDIO_REG);
>> +
>> +/*
>> + * First clock parent (osc32K) is unusable for CEC. But since there
>> + * is no good way to force parent switch (both run with same frequency),
>> + * just set second clock parent here.
>> + */
>> +val = readl(reg + SUN50I_H616_HDMI_CEC_CLK_REG);
>> +val |= BIT(24);
>> +writel(val, reg + SUN50I_H616_HDMI_CEC_CLK_REG);
>> +
>> +return sunxi_ccu_probe(pdev->dev.of_node, reg, _h616_ccu_desc);
>> +}
>> +
>> +static const struct of_device_id sun50i_h616_ccu_ids[] = {
>> +{ .compatible = "allwinner,sun50i-h616-ccu",
>> +.data = _h616_ccu_desc },
>> +{ }
>> +};
>> +
>> +static struct platform_driver sun50i_h616_ccu_driver = {
>> +.probe = sun50i_h616_ccu_probe,
>> +.driver = {
>> +.name = "sun50i-h616-ccu",
>> +.of_match_table = sun50i_h616_ccu_ids,
>> +},
>> +};
>> +builtin_platform_driver(sun50i_h616_ccu_driver);
>
> Please use CLK_OF_DECLARE() instead. That way clocks will be initialized
> earlier and it will be actually possible to register both timer peripherals
> (once DT nodes are added). If pdev or dev is ever needed, two stage
> initialization can be made later.
Sure, will do.
Thanks for having a look!
Andre
>
> Best regards,
> Jernej
>
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.h
>>
[linux-sunxi] Re: [PATCH 5/8] clk: sunxi-ng: Add support for the Allwinner H616 CCU
Dne sreda, 02. december 2020 ob 14:54:06 CET je Andre Przywara napisal(a):
> While the clocks are fairly similar to the H6, many differ in tiny
> details, so a separate clock driver seems indicated.
>
> Derived from the H6 clock driver, and adjusted according to the manual.
>
> Signed-off-by: Andre Przywara
> ---
> drivers/clk/sunxi-ng/Kconfig|7 +-
> drivers/clk/sunxi-ng/Makefile |1 +
> drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 1134 +++
> drivers/clk/sunxi-ng/ccu-sun50i-h616.h | 58 +
> include/dt-bindings/clock/sun50i-h616-ccu.h | 110 ++
> include/dt-bindings/reset/sun50i-h616-ccu.h | 67 ++
> 6 files changed, 1376 insertions(+), 1 deletion(-)
> create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> create mode 100644 drivers/clk/sunxi-ng/ccu-sun50i-h616.h
> create mode 100644 include/dt-bindings/clock/sun50i-h616-ccu.h
> create mode 100644 include/dt-bindings/reset/sun50i-h616-ccu.h
>
> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
> index ce5f5847d5d3..cd46d8853876 100644
> --- a/drivers/clk/sunxi-ng/Kconfig
> +++ b/drivers/clk/sunxi-ng/Kconfig
> @@ -32,8 +32,13 @@ config SUN50I_H6_CCU
> default ARM64 && ARCH_SUNXI
> depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
>
> +config SUN50I_H616_CCU
> + bool "Support for the Allwinner H616 CCU"
> + default ARM64 && ARCH_SUNXI
> + depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +
> config SUN50I_H6_R_CCU
> - bool "Support for the Allwinner H6 PRCM CCU"
> + bool "Support for the Allwinner H6 and H616 PRCM CCU"
> default ARM64 && ARCH_SUNXI
> depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
>
> diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
> index 3eb5cff40eac..96c324306d97 100644
> --- a/drivers/clk/sunxi-ng/Makefile
> +++ b/drivers/clk/sunxi-ng/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_SUN50I_A64_CCU)+= ccu-sun50i-a64.o
> obj-$(CONFIG_SUN50I_A100_CCU)+= ccu-sun50i-a100.o
> obj-$(CONFIG_SUN50I_A100_R_CCU) += ccu-sun50i-a100-r.o
> obj-$(CONFIG_SUN50I_H6_CCU) += ccu-sun50i-h6.o
> +obj-$(CONFIG_SUN50I_H616_CCU)+= ccu-sun50i-h616.o
> obj-$(CONFIG_SUN50I_H6_R_CCU)+= ccu-sun50i-h6-r.o
> obj-$(CONFIG_SUN4I_A10_CCU) += ccu-sun4i-a10.o
> obj-$(CONFIG_SUN5I_CCU) += ccu-sun5i.o
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/
ccu-sun50i-h616.c
> new file mode 100644
> index ..3fbb258f0354
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> @@ -0,0 +1,1134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Arm Ltd.
> + * Based on the H6 CCU driver, which is:
> + * Copyright (c) 2017 Icenowy Zheng
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "ccu_common.h"
> +#include "ccu_reset.h"
> +
> +#include "ccu_div.h"
> +#include "ccu_gate.h"
> +#include "ccu_mp.h"
> +#include "ccu_mult.h"
> +#include "ccu_nk.h"
> +#include "ccu_nkm.h"
> +#include "ccu_nkmp.h"
> +#include "ccu_nm.h"
> +
> +#include "ccu-sun50i-h616.h"
> +
> +/*
> + * The CPU PLL is actually NP clock, with P being /1, /2 or /4. However
> + * P should only be used for output frequencies lower than 288 MHz.
> + *
> + * For now we can just model it as a multiplier clock, and force P to /1.
> + *
> + * The M factor is present in the register's description, but not in the
> + * frequency formula, and it's documented as "M is only used for backdoor
> + * testing", so it's not modelled and then force to 0.
> + */
> +#define SUN50I_H616_PLL_CPUX_REG 0x000
> +static struct ccu_mult pll_cpux_clk = {
> + .enable = BIT(31),
> + .lock = BIT(28),
> + .mult = _SUNXI_CCU_MULT_MIN(8, 8, 12),
> + .common = {
> + .reg= 0x000,
> + .hw.init= CLK_HW_INIT("pll-cpux", "osc24M",
> + _mult_ops,
> +
CLK_SET_RATE_UNGATE),
> + },
> +};
> +
> +/* Some PLLs are input * N / div1 / P. Model them as NKMP with no K */
> +#define SUN50I_H616_PLL_DDR0_REG 0x010
> +static struct ccu_nkmp pll_ddr0_clk = {
> + .enable = BIT(31),
> + .lock = BIT(28),
> + .n = _SUNXI_CCU_MULT_MIN(8, 8, 12),
> + .m = _SUNXI_CCU_DIV(1, 1), /* input divider */
> + .p = _SUNXI_CCU_DIV(0, 1), /* output divider */
> + .common = {
> + .reg= 0x010,
> + .hw.init= CLK_HW_INIT("pll-ddr0", "osc24M",
> + _nkmp_ops,
> +
CLK_SET_RATE_UNGATE),
> + },
> +};
> +
> +#define SUN50I_H616_PLL_DDR1_REG 0x018
> +static struct ccu_nkmp pll_ddr1_clk = {
> + .enable = BIT(31),
> + .lock = BIT(28),
> +
[linux-sunxi] Re: [PATCH 5/8] clk: sunxi-ng: Add support for the Allwinner H616 CCU
Hi On Wed, Dec 02, 2020 at 01:54:06PM +, Andre Przywara wrote: > While the clocks are fairly similar to the H6, many differ in tiny > details, so a separate clock driver seems indicated. > > Derived from the H6 clock driver, and adjusted according to the manual. > > Signed-off-by: Andre Przywara The patch itself looks ok, but there's a bunch of checkpatch warning you'll want to fix. Maxime -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20201202155651.vgbkrrrxlw55yq7x%40gilmour. signature.asc Description: PGP signature
