| Message ID | 20180716112850.3961-18-jagan@amarulasolutions.com |
|---|---|
| State | RFC |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | sunxi: Add initial CLK, RESET support | expand |
On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: > Add initial clock driver Allwinner for H3_H5. > > Implemented clock enable and disable functions for > USB OHCI, EHCI, OTG and PHY gate and clock registers. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > drivers/clk/sunxi/Kconfig | 7 ++ > drivers/clk/sunxi/Makefile | 2 + > drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 140 insertions(+) > create mode 100644 drivers/clk/sunxi/clk_h3.c > > diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig > index 3a86c91e75..065cadf2fe 100644 > --- a/drivers/clk/sunxi/Kconfig > +++ b/drivers/clk/sunxi/Kconfig > @@ -8,6 +8,13 @@ config CLK_SUNXI > > if CLK_SUNXI > > +config CLK_SUN8I_H3 > + bool "Clock driver for Allwinner H3/H5" > + default MACH_SUNXI_H3_H5 > + help > + This enables common clock driver support for platforms based > + on Allwinner H3/H5 SoC. > + > config CLK_SUN50I_A64 > bool "Clock driver for Allwinner A64" > default MACH_SUN50I > diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile > index 860bb6dfea..37e6bcb147 100644 > --- a/drivers/clk/sunxi/Makefile > +++ b/drivers/clk/sunxi/Makefile > @@ -5,4 +5,6 @@ > # > > obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o > + > +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o > obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o > diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c > new file mode 100644 > index 0000000000..e924017717 > --- /dev/null > +++ b/drivers/clk/sunxi/clk_h3.c > @@ -0,0 +1,131 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Copyright (C) 2018 Amarula Solutions B.V. > + * Author: Jagan Teki <jagan@amarulasolutions.com> > + */ > + > +#include <common.h> > +#include <clk-uclass.h> > +#include <dm.h> > +#include <errno.h> > +#include <asm/io.h> > +#include <asm/arch/clock.h> > +#include <dt-bindings/clock/sun8i-h3-ccu.h> > + > +struct h3_clk_priv { > + void *base; > +}; > + > +static int h3_clk_enable(struct clk *clk) > +{ > + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > + > + debug("%s(#%ld)\n", __func__, clk->id); > + > + switch (clk->id) { > + case CLK_BUS_OTG: > + case CLK_BUS_EHCI0: > + case CLK_BUS_EHCI1: > + case CLK_BUS_EHCI2: > + case CLK_BUS_EHCI3: > + case CLK_BUS_OHCI0: > + case CLK_BUS_OHCI1: > + case CLK_BUS_OHCI2: > + case CLK_BUS_OHCI3: > + setbits_le32(priv->base + 0x60, > + BIT(23 + (clk->id - CLK_BUS_OTG))); > + return 0; > + case CLK_USB_PHY0: > + case CLK_USB_PHY1: > + case CLK_USB_PHY2: > + case CLK_USB_PHY3: > + setbits_le32(priv->base + 0xcc, > + BIT(8 + (clk->id - CLK_USB_PHY0))); > + return 0; > + case CLK_USB_OHCI0: > + case CLK_USB_OHCI1: > + case CLK_USB_OHCI2: > + case CLK_USB_OHCI3: > + setbits_le32(priv->base + 0xcc, > + BIT(16 + (clk->id - CLK_USB_OHCI0))); > + return 0; > + default: > + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > + return -ENODEV; > + } > +} > + > +static int h3_clk_disable(struct clk *clk) > +{ > + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > + > + debug("%s(#%ld)\n", __func__, clk->id); > + > + switch (clk->id) { > + case CLK_BUS_OTG: > + case CLK_BUS_EHCI0: > + case CLK_BUS_EHCI1: > + case CLK_BUS_EHCI2: > + case CLK_BUS_EHCI3: > + case CLK_BUS_OHCI0: > + case CLK_BUS_OHCI1: > + case CLK_BUS_OHCI2: > + case CLK_BUS_OHCI3: > + clrbits_le32(priv->base + 0x60, > + BIT(23 + (clk->id - CLK_BUS_OTG))); > + return 0; > + case CLK_USB_PHY0: > + case CLK_USB_PHY1: > + case CLK_USB_PHY2: > + case CLK_USB_PHY3: > + clrbits_le32(priv->base + 0xcc, > + BIT(8 + (clk->id - CLK_USB_PHY0))); > + return 0; > + case CLK_USB_OHCI0: > + case CLK_USB_OHCI1: > + case CLK_USB_OHCI2: > + case CLK_USB_OHCI3: > + clrbits_le32(priv->base + 0xcc, > + BIT(16 + (clk->id - CLK_USB_OHCI0))); > + return 0; > + default: > + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > + return -ENODEV; > + } > +} > + > +static struct clk_ops h3_clk_ops = { > + .enable = h3_clk_enable, > + .disable = h3_clk_disable, > +}; > + > +static int h3_clk_probe(struct udevice *dev) > +{ > + return 0; > +} > + > +static int h3_clk_ofdata_to_platdata(struct udevice *dev) > +{ > + struct h3_clk_priv *priv = dev_get_priv(dev); > + > + priv->base = dev_read_addr_ptr(dev); > + > + return 0; > +} > + > +static const struct udevice_id h3_clk_ids[] = { > + { .compatible = "allwinner,sun8i-h3-ccu" }, > + { .compatible = "allwinner,sun50i-h5-ccu" }, > + { } > +}; > + > +U_BOOT_DRIVER(clk_sun8i_h3) = { > + .name = "sun8i_h3_ccu", > + .id = UCLASS_CLK, > + .of_match = h3_clk_ids, > + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), > + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, > + .ops = &h3_clk_ops, > + .probe = h3_clk_probe, > + .bind = sunxi_clk_bind, > +}; Speaking from experience, you do not want to have separate implementations for each and every SoCs. This might be enough for enabling / disabling the clocks, but as soon as you'll throw the set_rate / get_rate callbacks into the mix it's going to turn into a nightmare. Maxime
Hi, On 16/07/18 13:59, Maxime Ripard wrote: > On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: >> Add initial clock driver Allwinner for H3_H5. >> >> Implemented clock enable and disable functions for >> USB OHCI, EHCI, OTG and PHY gate and clock registers. >> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >> --- >> drivers/clk/sunxi/Kconfig | 7 ++ >> drivers/clk/sunxi/Makefile | 2 + >> drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 140 insertions(+) >> create mode 100644 drivers/clk/sunxi/clk_h3.c >> >> diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig >> index 3a86c91e75..065cadf2fe 100644 >> --- a/drivers/clk/sunxi/Kconfig >> +++ b/drivers/clk/sunxi/Kconfig >> @@ -8,6 +8,13 @@ config CLK_SUNXI >> >> if CLK_SUNXI >> >> +config CLK_SUN8I_H3 >> + bool "Clock driver for Allwinner H3/H5" >> + default MACH_SUNXI_H3_H5 >> + help >> + This enables common clock driver support for platforms based >> + on Allwinner H3/H5 SoC. >> + >> config CLK_SUN50I_A64 >> bool "Clock driver for Allwinner A64" >> default MACH_SUN50I >> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile >> index 860bb6dfea..37e6bcb147 100644 >> --- a/drivers/clk/sunxi/Makefile >> +++ b/drivers/clk/sunxi/Makefile >> @@ -5,4 +5,6 @@ >> # >> >> obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o >> + >> +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o >> obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o >> diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c >> new file mode 100644 >> index 0000000000..e924017717 >> --- /dev/null >> +++ b/drivers/clk/sunxi/clk_h3.c >> @@ -0,0 +1,131 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Copyright (C) 2018 Amarula Solutions B.V. >> + * Author: Jagan Teki <jagan@amarulasolutions.com> >> + */ >> + >> +#include <common.h> >> +#include <clk-uclass.h> >> +#include <dm.h> >> +#include <errno.h> >> +#include <asm/io.h> >> +#include <asm/arch/clock.h> >> +#include <dt-bindings/clock/sun8i-h3-ccu.h> >> + >> +struct h3_clk_priv { >> + void *base; >> +}; >> + >> +static int h3_clk_enable(struct clk *clk) >> +{ >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >> + >> + debug("%s(#%ld)\n", __func__, clk->id); >> + >> + switch (clk->id) { >> + case CLK_BUS_OTG: >> + case CLK_BUS_EHCI0: >> + case CLK_BUS_EHCI1: >> + case CLK_BUS_EHCI2: >> + case CLK_BUS_EHCI3: >> + case CLK_BUS_OHCI0: >> + case CLK_BUS_OHCI1: >> + case CLK_BUS_OHCI2: >> + case CLK_BUS_OHCI3: >> + setbits_le32(priv->base + 0x60, >> + BIT(23 + (clk->id - CLK_BUS_OTG))); >> + return 0; >> + case CLK_USB_PHY0: >> + case CLK_USB_PHY1: >> + case CLK_USB_PHY2: >> + case CLK_USB_PHY3: >> + setbits_le32(priv->base + 0xcc, >> + BIT(8 + (clk->id - CLK_USB_PHY0))); >> + return 0; >> + case CLK_USB_OHCI0: >> + case CLK_USB_OHCI1: >> + case CLK_USB_OHCI2: >> + case CLK_USB_OHCI3: >> + setbits_le32(priv->base + 0xcc, >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >> + return 0; >> + default: >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >> + return -ENODEV; >> + } >> +} >> + >> +static int h3_clk_disable(struct clk *clk) >> +{ >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >> + >> + debug("%s(#%ld)\n", __func__, clk->id); >> + >> + switch (clk->id) { >> + case CLK_BUS_OTG: >> + case CLK_BUS_EHCI0: >> + case CLK_BUS_EHCI1: >> + case CLK_BUS_EHCI2: >> + case CLK_BUS_EHCI3: >> + case CLK_BUS_OHCI0: >> + case CLK_BUS_OHCI1: >> + case CLK_BUS_OHCI2: >> + case CLK_BUS_OHCI3: >> + clrbits_le32(priv->base + 0x60, >> + BIT(23 + (clk->id - CLK_BUS_OTG))); >> + return 0; >> + case CLK_USB_PHY0: >> + case CLK_USB_PHY1: >> + case CLK_USB_PHY2: >> + case CLK_USB_PHY3: >> + clrbits_le32(priv->base + 0xcc, >> + BIT(8 + (clk->id - CLK_USB_PHY0))); >> + return 0; >> + case CLK_USB_OHCI0: >> + case CLK_USB_OHCI1: >> + case CLK_USB_OHCI2: >> + case CLK_USB_OHCI3: >> + clrbits_le32(priv->base + 0xcc, >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >> + return 0; >> + default: >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >> + return -ENODEV; >> + } >> +} >> + >> +static struct clk_ops h3_clk_ops = { >> + .enable = h3_clk_enable, >> + .disable = h3_clk_disable, >> +}; >> + >> +static int h3_clk_probe(struct udevice *dev) >> +{ >> + return 0; >> +} >> + >> +static int h3_clk_ofdata_to_platdata(struct udevice *dev) >> +{ >> + struct h3_clk_priv *priv = dev_get_priv(dev); >> + >> + priv->base = dev_read_addr_ptr(dev); >> + >> + return 0; >> +} >> + >> +static const struct udevice_id h3_clk_ids[] = { >> + { .compatible = "allwinner,sun8i-h3-ccu" }, >> + { .compatible = "allwinner,sun50i-h5-ccu" }, >> + { } >> +}; >> + >> +U_BOOT_DRIVER(clk_sun8i_h3) = { >> + .name = "sun8i_h3_ccu", >> + .id = UCLASS_CLK, >> + .of_match = h3_clk_ids, >> + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), >> + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, >> + .ops = &h3_clk_ops, >> + .probe = h3_clk_probe, >> + .bind = sunxi_clk_bind, >> +}; > > Speaking from experience, you do not want to have separate > implementations for each and every SoCs. This might be enough for > enabling / disabling the clocks, but as soon as you'll throw the > set_rate / get_rate callbacks into the mix it's going to turn into a > nightmare. I agree, but I guess it won't be too pretty anyway: The CLK_BUS_[EO]HCIx definitions are different for each SoC, but share the same symbol. So we can't use a nicely readable switch/case anymore. Unless we translate the values to a common namespace? But I support that we should share as much code as possible, maybe by using macros to instantiate the driver boilerplates and by using a shared file with the gist of the clock programming code and then just have shim layers to connect the bits? In case it's just bit and register offsets differing, we could define a structure holding register and bit offsets, filling this for the various SoCs, then tie those together with the compatible strings: struct sunxi_clk_defs { uint16_t clk_bus_usb_offset; uint16_t clk_bus_usb_bit; ... } sun8i_h3_h5_clk_defs = { .clk_bus_usb_offset = 0x60; .clk_bus_usb_bit = 23; }; ... case CLK_BUS_OHCI3: clrbits_le32(priv->base + priv->clk_bus_usb_offset, BIT(priv->clk_bus_usb_bit + (clk->id - CLK_BUS_OTG))); .... static const struct udevice_id sunxi_clk_ids[] = { { .compatible = "allwinner,sun8i-h3-ccu", .data = sun8i_h3_h5_clk_defs }, }; Just an example, not sure we are actually much different in those bits there. Or we put the DT clock numbers into that struct and look those up: int sunxi_clk_bus_usb_idx (struct sunxi_clk_defs *priv, int clkid) { if (clkid >= priv->first_bus_usb && clkid <= priv->last_bus_usb) return clkid - priv->first_bus_usb; return -1; } static int h3_clk_enable(struct clk *clk) { ... idx = sunxi_clk_bus_usb_idx(priv, clk->id)); if (idx >= 0) setbits_le32(priv->base + 0x60, BIT(23 + idx)); idx = sunxi_clk_usb_phy_idx(priv, clk->id)); if (idx >= 0) setbits_le32(priv->base + 0xcc, BIT(8 + idx)); Cheers, Andre.
On Mon, Jul 16, 2018 at 10:25 PM, Andre Przywara <andre.przywara@arm.com> wrote: > Hi, > > On 16/07/18 13:59, Maxime Ripard wrote: >> On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: >>> Add initial clock driver Allwinner for H3_H5. >>> >>> Implemented clock enable and disable functions for >>> USB OHCI, EHCI, OTG and PHY gate and clock registers. >>> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>> --- >>> drivers/clk/sunxi/Kconfig | 7 ++ >>> drivers/clk/sunxi/Makefile | 2 + >>> drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 140 insertions(+) >>> create mode 100644 drivers/clk/sunxi/clk_h3.c >>> >>> diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig >>> index 3a86c91e75..065cadf2fe 100644 >>> --- a/drivers/clk/sunxi/Kconfig >>> +++ b/drivers/clk/sunxi/Kconfig >>> @@ -8,6 +8,13 @@ config CLK_SUNXI >>> >>> if CLK_SUNXI >>> >>> +config CLK_SUN8I_H3 >>> + bool "Clock driver for Allwinner H3/H5" >>> + default MACH_SUNXI_H3_H5 >>> + help >>> + This enables common clock driver support for platforms based >>> + on Allwinner H3/H5 SoC. >>> + >>> config CLK_SUN50I_A64 >>> bool "Clock driver for Allwinner A64" >>> default MACH_SUN50I >>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile >>> index 860bb6dfea..37e6bcb147 100644 >>> --- a/drivers/clk/sunxi/Makefile >>> +++ b/drivers/clk/sunxi/Makefile >>> @@ -5,4 +5,6 @@ >>> # >>> >>> obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o >>> + >>> +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o >>> obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o >>> diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c >>> new file mode 100644 >>> index 0000000000..e924017717 >>> --- /dev/null >>> +++ b/drivers/clk/sunxi/clk_h3.c >>> @@ -0,0 +1,131 @@ >>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >>> +/* >>> + * Copyright (C) 2018 Amarula Solutions B.V. >>> + * Author: Jagan Teki <jagan@amarulasolutions.com> >>> + */ >>> + >>> +#include <common.h> >>> +#include <clk-uclass.h> >>> +#include <dm.h> >>> +#include <errno.h> >>> +#include <asm/io.h> >>> +#include <asm/arch/clock.h> >>> +#include <dt-bindings/clock/sun8i-h3-ccu.h> >>> + >>> +struct h3_clk_priv { >>> + void *base; >>> +}; >>> + >>> +static int h3_clk_enable(struct clk *clk) >>> +{ >>> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >>> + >>> + debug("%s(#%ld)\n", __func__, clk->id); >>> + >>> + switch (clk->id) { >>> + case CLK_BUS_OTG: >>> + case CLK_BUS_EHCI0: >>> + case CLK_BUS_EHCI1: >>> + case CLK_BUS_EHCI2: >>> + case CLK_BUS_EHCI3: >>> + case CLK_BUS_OHCI0: >>> + case CLK_BUS_OHCI1: >>> + case CLK_BUS_OHCI2: >>> + case CLK_BUS_OHCI3: >>> + setbits_le32(priv->base + 0x60, >>> + BIT(23 + (clk->id - CLK_BUS_OTG))); >>> + return 0; >>> + case CLK_USB_PHY0: >>> + case CLK_USB_PHY1: >>> + case CLK_USB_PHY2: >>> + case CLK_USB_PHY3: >>> + setbits_le32(priv->base + 0xcc, >>> + BIT(8 + (clk->id - CLK_USB_PHY0))); >>> + return 0; >>> + case CLK_USB_OHCI0: >>> + case CLK_USB_OHCI1: >>> + case CLK_USB_OHCI2: >>> + case CLK_USB_OHCI3: >>> + setbits_le32(priv->base + 0xcc, >>> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >>> + return 0; >>> + default: >>> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >>> + return -ENODEV; >>> + } >>> +} >>> + >>> +static int h3_clk_disable(struct clk *clk) >>> +{ >>> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >>> + >>> + debug("%s(#%ld)\n", __func__, clk->id); >>> + >>> + switch (clk->id) { >>> + case CLK_BUS_OTG: >>> + case CLK_BUS_EHCI0: >>> + case CLK_BUS_EHCI1: >>> + case CLK_BUS_EHCI2: >>> + case CLK_BUS_EHCI3: >>> + case CLK_BUS_OHCI0: >>> + case CLK_BUS_OHCI1: >>> + case CLK_BUS_OHCI2: >>> + case CLK_BUS_OHCI3: >>> + clrbits_le32(priv->base + 0x60, >>> + BIT(23 + (clk->id - CLK_BUS_OTG))); >>> + return 0; >>> + case CLK_USB_PHY0: >>> + case CLK_USB_PHY1: >>> + case CLK_USB_PHY2: >>> + case CLK_USB_PHY3: >>> + clrbits_le32(priv->base + 0xcc, >>> + BIT(8 + (clk->id - CLK_USB_PHY0))); >>> + return 0; >>> + case CLK_USB_OHCI0: >>> + case CLK_USB_OHCI1: >>> + case CLK_USB_OHCI2: >>> + case CLK_USB_OHCI3: >>> + clrbits_le32(priv->base + 0xcc, >>> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >>> + return 0; >>> + default: >>> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >>> + return -ENODEV; >>> + } >>> +} >>> + >>> +static struct clk_ops h3_clk_ops = { >>> + .enable = h3_clk_enable, >>> + .disable = h3_clk_disable, >>> +}; >>> + >>> +static int h3_clk_probe(struct udevice *dev) >>> +{ >>> + return 0; >>> +} >>> + >>> +static int h3_clk_ofdata_to_platdata(struct udevice *dev) >>> +{ >>> + struct h3_clk_priv *priv = dev_get_priv(dev); >>> + >>> + priv->base = dev_read_addr_ptr(dev); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct udevice_id h3_clk_ids[] = { >>> + { .compatible = "allwinner,sun8i-h3-ccu" }, >>> + { .compatible = "allwinner,sun50i-h5-ccu" }, >>> + { } >>> +}; >>> + >>> +U_BOOT_DRIVER(clk_sun8i_h3) = { >>> + .name = "sun8i_h3_ccu", >>> + .id = UCLASS_CLK, >>> + .of_match = h3_clk_ids, >>> + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), >>> + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, >>> + .ops = &h3_clk_ops, >>> + .probe = h3_clk_probe, >>> + .bind = sunxi_clk_bind, >>> +}; >> >> Speaking from experience, you do not want to have separate >> implementations for each and every SoCs. This might be enough for >> enabling / disabling the clocks, but as soon as you'll throw the >> set_rate / get_rate callbacks into the mix it's going to turn into a >> nightmare. > > I agree, but I guess it won't be too pretty anyway: > The CLK_BUS_[EO]HCIx definitions are different for each SoC, but share > the same symbol. So we can't use a nicely readable switch/case anymore. > Unless we translate the values to a common namespace? > > But I support that we should share as much code as possible, maybe by > using macros to instantiate the driver boilerplates and by using a > shared file with the gist of the clock programming code and then just > have shim layers to connect the bits? > > In case it's just bit and register offsets differing, we could define a > structure holding register and bit offsets, filling this for the various > SoCs, then tie those together with the compatible strings: > struct sunxi_clk_defs { > uint16_t clk_bus_usb_offset; > uint16_t clk_bus_usb_bit; > ... > } sun8i_h3_h5_clk_defs = { > .clk_bus_usb_offset = 0x60; > .clk_bus_usb_bit = 23; > }; > ... case CLK_BUS_OHCI3: > clrbits_le32(priv->base + priv->clk_bus_usb_offset, > BIT(priv->clk_bus_usb_bit + (clk->id - CLK_BUS_OTG))); > .... > static const struct udevice_id sunxi_clk_ids[] = { > { .compatible = "allwinner,sun8i-h3-ccu", > .data = sun8i_h3_h5_clk_defs }, > }; I tried this boilerplates via driver data, this would ended-ed big structure for all SoC's and afraid to move further since we have other IP's to add in future. may be in separate file for each IP not sure. > > Just an example, not sure we are actually much different in those bits > there. > > Or we put the DT clock numbers into that struct and look those up: > int sunxi_clk_bus_usb_idx (struct sunxi_clk_defs *priv, int clkid) > { > if (clkid >= priv->first_bus_usb && > clkid <= priv->last_bus_usb) > return clkid - priv->first_bus_usb; > return -1; > } > static int h3_clk_enable(struct clk *clk) > { > ... > idx = sunxi_clk_bus_usb_idx(priv, clk->id)); > if (idx >= 0) > setbits_le32(priv->base + 0x60, BIT(23 + idx)); This look interesting, but the issue with bit positions are different between SoC's even we have bit position change between EHCI to OHCI clk_bus like this case CLK_AHB1_OTG: setbits_le32(priv->base + 0x60, BIT(24)); return 0; case CLK_AHB1_EHCI0: case CLK_AHB1_EHCI1: setbits_le32(priv->base + 0x60, BIT(26 + (clk->id - CLK_AHB1_EHCI0))); return 0; case CLK_AHB1_OHCI0: case CLK_AHB1_OHCI1: case CLK_AHB1_OHCI2: setbits_le32(priv->base + 0x60, BIT(29 + (clk->id - CLK_AHB1_OHCI0))); On the other-side, may be we can think of common implementation for set/get_rate instead of bit enable/disable because bit enable/disable need many IP's than rate by keeping enable code simple.
On Mon, Jul 16, 2018 at 05:55:25PM +0100, Andre Przywara wrote: > Hi, > > On 16/07/18 13:59, Maxime Ripard wrote: > > On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: > >> Add initial clock driver Allwinner for H3_H5. > >> > >> Implemented clock enable and disable functions for > >> USB OHCI, EHCI, OTG and PHY gate and clock registers. > >> > >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >> --- > >> drivers/clk/sunxi/Kconfig | 7 ++ > >> drivers/clk/sunxi/Makefile | 2 + > >> drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 140 insertions(+) > >> create mode 100644 drivers/clk/sunxi/clk_h3.c > >> > >> diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig > >> index 3a86c91e75..065cadf2fe 100644 > >> --- a/drivers/clk/sunxi/Kconfig > >> +++ b/drivers/clk/sunxi/Kconfig > >> @@ -8,6 +8,13 @@ config CLK_SUNXI > >> > >> if CLK_SUNXI > >> > >> +config CLK_SUN8I_H3 > >> + bool "Clock driver for Allwinner H3/H5" > >> + default MACH_SUNXI_H3_H5 > >> + help > >> + This enables common clock driver support for platforms based > >> + on Allwinner H3/H5 SoC. > >> + > >> config CLK_SUN50I_A64 > >> bool "Clock driver for Allwinner A64" > >> default MACH_SUN50I > >> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile > >> index 860bb6dfea..37e6bcb147 100644 > >> --- a/drivers/clk/sunxi/Makefile > >> +++ b/drivers/clk/sunxi/Makefile > >> @@ -5,4 +5,6 @@ > >> # > >> > >> obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o > >> + > >> +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o > >> obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o > >> diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c > >> new file mode 100644 > >> index 0000000000..e924017717 > >> --- /dev/null > >> +++ b/drivers/clk/sunxi/clk_h3.c > >> @@ -0,0 +1,131 @@ > >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > >> +/* > >> + * Copyright (C) 2018 Amarula Solutions B.V. > >> + * Author: Jagan Teki <jagan@amarulasolutions.com> > >> + */ > >> + > >> +#include <common.h> > >> +#include <clk-uclass.h> > >> +#include <dm.h> > >> +#include <errno.h> > >> +#include <asm/io.h> > >> +#include <asm/arch/clock.h> > >> +#include <dt-bindings/clock/sun8i-h3-ccu.h> > >> + > >> +struct h3_clk_priv { > >> + void *base; > >> +}; > >> + > >> +static int h3_clk_enable(struct clk *clk) > >> +{ > >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > >> + > >> + debug("%s(#%ld)\n", __func__, clk->id); > >> + > >> + switch (clk->id) { > >> + case CLK_BUS_OTG: > >> + case CLK_BUS_EHCI0: > >> + case CLK_BUS_EHCI1: > >> + case CLK_BUS_EHCI2: > >> + case CLK_BUS_EHCI3: > >> + case CLK_BUS_OHCI0: > >> + case CLK_BUS_OHCI1: > >> + case CLK_BUS_OHCI2: > >> + case CLK_BUS_OHCI3: > >> + setbits_le32(priv->base + 0x60, > >> + BIT(23 + (clk->id - CLK_BUS_OTG))); > >> + return 0; > >> + case CLK_USB_PHY0: > >> + case CLK_USB_PHY1: > >> + case CLK_USB_PHY2: > >> + case CLK_USB_PHY3: > >> + setbits_le32(priv->base + 0xcc, > >> + BIT(8 + (clk->id - CLK_USB_PHY0))); > >> + return 0; > >> + case CLK_USB_OHCI0: > >> + case CLK_USB_OHCI1: > >> + case CLK_USB_OHCI2: > >> + case CLK_USB_OHCI3: > >> + setbits_le32(priv->base + 0xcc, > >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); > >> + return 0; > >> + default: > >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > >> + return -ENODEV; > >> + } > >> +} > >> + > >> +static int h3_clk_disable(struct clk *clk) > >> +{ > >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > >> + > >> + debug("%s(#%ld)\n", __func__, clk->id); > >> + > >> + switch (clk->id) { > >> + case CLK_BUS_OTG: > >> + case CLK_BUS_EHCI0: > >> + case CLK_BUS_EHCI1: > >> + case CLK_BUS_EHCI2: > >> + case CLK_BUS_EHCI3: > >> + case CLK_BUS_OHCI0: > >> + case CLK_BUS_OHCI1: > >> + case CLK_BUS_OHCI2: > >> + case CLK_BUS_OHCI3: > >> + clrbits_le32(priv->base + 0x60, > >> + BIT(23 + (clk->id - CLK_BUS_OTG))); > >> + return 0; > >> + case CLK_USB_PHY0: > >> + case CLK_USB_PHY1: > >> + case CLK_USB_PHY2: > >> + case CLK_USB_PHY3: > >> + clrbits_le32(priv->base + 0xcc, > >> + BIT(8 + (clk->id - CLK_USB_PHY0))); > >> + return 0; > >> + case CLK_USB_OHCI0: > >> + case CLK_USB_OHCI1: > >> + case CLK_USB_OHCI2: > >> + case CLK_USB_OHCI3: > >> + clrbits_le32(priv->base + 0xcc, > >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); > >> + return 0; > >> + default: > >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > >> + return -ENODEV; > >> + } > >> +} > >> + > >> +static struct clk_ops h3_clk_ops = { > >> + .enable = h3_clk_enable, > >> + .disable = h3_clk_disable, > >> +}; > >> + > >> +static int h3_clk_probe(struct udevice *dev) > >> +{ > >> + return 0; > >> +} > >> + > >> +static int h3_clk_ofdata_to_platdata(struct udevice *dev) > >> +{ > >> + struct h3_clk_priv *priv = dev_get_priv(dev); > >> + > >> + priv->base = dev_read_addr_ptr(dev); > >> + > >> + return 0; > >> +} > >> + > >> +static const struct udevice_id h3_clk_ids[] = { > >> + { .compatible = "allwinner,sun8i-h3-ccu" }, > >> + { .compatible = "allwinner,sun50i-h5-ccu" }, > >> + { } > >> +}; > >> + > >> +U_BOOT_DRIVER(clk_sun8i_h3) = { > >> + .name = "sun8i_h3_ccu", > >> + .id = UCLASS_CLK, > >> + .of_match = h3_clk_ids, > >> + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), > >> + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, > >> + .ops = &h3_clk_ops, > >> + .probe = h3_clk_probe, > >> + .bind = sunxi_clk_bind, > >> +}; > > > > Speaking from experience, you do not want to have separate > > implementations for each and every SoCs. This might be enough for > > enabling / disabling the clocks, but as soon as you'll throw the > > set_rate / get_rate callbacks into the mix it's going to turn into a > > nightmare. > > I agree, but I guess it won't be too pretty anyway: > The CLK_BUS_[EO]HCIx definitions are different for each SoC, but share > the same symbol. So we can't use a nicely readable switch/case anymore. > Unless we translate the values to a common namespace? > > But I support that we should share as much code as possible, maybe by > using macros to instantiate the driver boilerplates and by using a > shared file with the gist of the clock programming code and then just > have shim layers to connect the bits? > > In case it's just bit and register offsets differing, we could define a > structure holding register and bit offsets, filling this for the various > SoCs, then tie those together with the compatible strings: > struct sunxi_clk_defs { > uint16_t clk_bus_usb_offset; > uint16_t clk_bus_usb_bit; > ... > } sun8i_h3_h5_clk_defs = { > .clk_bus_usb_offset = 0x60; > .clk_bus_usb_bit = 23; > }; > ... case CLK_BUS_OHCI3: > clrbits_le32(priv->base + priv->clk_bus_usb_offset, > BIT(priv->clk_bus_usb_bit + (clk->id - CLK_BUS_OTG))); > .... > static const struct udevice_id sunxi_clk_ids[] = { > { .compatible = "allwinner,sun8i-h3-ccu", > .data = sun8i_h3_h5_clk_defs }, > }; > > Just an example, not sure we are actually much different in those bits > there. > > Or we put the DT clock numbers into that struct and look those up: > int sunxi_clk_bus_usb_idx (struct sunxi_clk_defs *priv, int clkid) > { > if (clkid >= priv->first_bus_usb && > clkid <= priv->last_bus_usb) > return clkid - priv->first_bus_usb; > return -1; > } > static int h3_clk_enable(struct clk *clk) > { > ... > idx = sunxi_clk_bus_usb_idx(priv, clk->id)); > if (idx >= 0) > setbits_le32(priv->base + 0x60, BIT(23 + idx)); > idx = sunxi_clk_usb_phy_idx(priv, clk->id)); > if (idx >= 0) > setbits_le32(priv->base + 0xcc, BIT(8 + idx)); I guess we could also give to a common code a key / value (register - offset) pair that would be SoC specific, a simpler version of what we're doing in Linux. Maxime
Hi, On 17/07/18 13:36, Maxime Ripard wrote: > On Mon, Jul 16, 2018 at 05:55:25PM +0100, Andre Przywara wrote: >> Hi, >> >> On 16/07/18 13:59, Maxime Ripard wrote: >>> On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: >>>> Add initial clock driver Allwinner for H3_H5. >>>> >>>> Implemented clock enable and disable functions for >>>> USB OHCI, EHCI, OTG and PHY gate and clock registers. >>>> >>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>> --- >>>> drivers/clk/sunxi/Kconfig | 7 ++ >>>> drivers/clk/sunxi/Makefile | 2 + >>>> drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 140 insertions(+) >>>> create mode 100644 drivers/clk/sunxi/clk_h3.c >>>> >>>> diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig >>>> index 3a86c91e75..065cadf2fe 100644 >>>> --- a/drivers/clk/sunxi/Kconfig >>>> +++ b/drivers/clk/sunxi/Kconfig >>>> @@ -8,6 +8,13 @@ config CLK_SUNXI >>>> >>>> if CLK_SUNXI >>>> >>>> +config CLK_SUN8I_H3 >>>> + bool "Clock driver for Allwinner H3/H5" >>>> + default MACH_SUNXI_H3_H5 >>>> + help >>>> + This enables common clock driver support for platforms based >>>> + on Allwinner H3/H5 SoC. >>>> + >>>> config CLK_SUN50I_A64 >>>> bool "Clock driver for Allwinner A64" >>>> default MACH_SUN50I >>>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile >>>> index 860bb6dfea..37e6bcb147 100644 >>>> --- a/drivers/clk/sunxi/Makefile >>>> +++ b/drivers/clk/sunxi/Makefile >>>> @@ -5,4 +5,6 @@ >>>> # >>>> >>>> obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o >>>> + >>>> +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o >>>> obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o >>>> diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c >>>> new file mode 100644 >>>> index 0000000000..e924017717 >>>> --- /dev/null >>>> +++ b/drivers/clk/sunxi/clk_h3.c >>>> @@ -0,0 +1,131 @@ >>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >>>> +/* >>>> + * Copyright (C) 2018 Amarula Solutions B.V. >>>> + * Author: Jagan Teki <jagan@amarulasolutions.com> >>>> + */ >>>> + >>>> +#include <common.h> >>>> +#include <clk-uclass.h> >>>> +#include <dm.h> >>>> +#include <errno.h> >>>> +#include <asm/io.h> >>>> +#include <asm/arch/clock.h> >>>> +#include <dt-bindings/clock/sun8i-h3-ccu.h> >>>> + >>>> +struct h3_clk_priv { >>>> + void *base; >>>> +}; >>>> + >>>> +static int h3_clk_enable(struct clk *clk) >>>> +{ >>>> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >>>> + >>>> + debug("%s(#%ld)\n", __func__, clk->id); >>>> + >>>> + switch (clk->id) { >>>> + case CLK_BUS_OTG: >>>> + case CLK_BUS_EHCI0: >>>> + case CLK_BUS_EHCI1: >>>> + case CLK_BUS_EHCI2: >>>> + case CLK_BUS_EHCI3: >>>> + case CLK_BUS_OHCI0: >>>> + case CLK_BUS_OHCI1: >>>> + case CLK_BUS_OHCI2: >>>> + case CLK_BUS_OHCI3: >>>> + setbits_le32(priv->base + 0x60, >>>> + BIT(23 + (clk->id - CLK_BUS_OTG))); >>>> + return 0; >>>> + case CLK_USB_PHY0: >>>> + case CLK_USB_PHY1: >>>> + case CLK_USB_PHY2: >>>> + case CLK_USB_PHY3: >>>> + setbits_le32(priv->base + 0xcc, >>>> + BIT(8 + (clk->id - CLK_USB_PHY0))); >>>> + return 0; >>>> + case CLK_USB_OHCI0: >>>> + case CLK_USB_OHCI1: >>>> + case CLK_USB_OHCI2: >>>> + case CLK_USB_OHCI3: >>>> + setbits_le32(priv->base + 0xcc, >>>> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >>>> + return 0; >>>> + default: >>>> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >>>> + return -ENODEV; >>>> + } >>>> +} >>>> + >>>> +static int h3_clk_disable(struct clk *clk) >>>> +{ >>>> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >>>> + >>>> + debug("%s(#%ld)\n", __func__, clk->id); >>>> + >>>> + switch (clk->id) { >>>> + case CLK_BUS_OTG: >>>> + case CLK_BUS_EHCI0: >>>> + case CLK_BUS_EHCI1: >>>> + case CLK_BUS_EHCI2: >>>> + case CLK_BUS_EHCI3: >>>> + case CLK_BUS_OHCI0: >>>> + case CLK_BUS_OHCI1: >>>> + case CLK_BUS_OHCI2: >>>> + case CLK_BUS_OHCI3: >>>> + clrbits_le32(priv->base + 0x60, >>>> + BIT(23 + (clk->id - CLK_BUS_OTG))); >>>> + return 0; >>>> + case CLK_USB_PHY0: >>>> + case CLK_USB_PHY1: >>>> + case CLK_USB_PHY2: >>>> + case CLK_USB_PHY3: >>>> + clrbits_le32(priv->base + 0xcc, >>>> + BIT(8 + (clk->id - CLK_USB_PHY0))); >>>> + return 0; >>>> + case CLK_USB_OHCI0: >>>> + case CLK_USB_OHCI1: >>>> + case CLK_USB_OHCI2: >>>> + case CLK_USB_OHCI3: >>>> + clrbits_le32(priv->base + 0xcc, >>>> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >>>> + return 0; >>>> + default: >>>> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >>>> + return -ENODEV; >>>> + } >>>> +} >>>> + >>>> +static struct clk_ops h3_clk_ops = { >>>> + .enable = h3_clk_enable, >>>> + .disable = h3_clk_disable, >>>> +}; >>>> + >>>> +static int h3_clk_probe(struct udevice *dev) >>>> +{ >>>> + return 0; >>>> +} >>>> + >>>> +static int h3_clk_ofdata_to_platdata(struct udevice *dev) >>>> +{ >>>> + struct h3_clk_priv *priv = dev_get_priv(dev); >>>> + >>>> + priv->base = dev_read_addr_ptr(dev); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct udevice_id h3_clk_ids[] = { >>>> + { .compatible = "allwinner,sun8i-h3-ccu" }, >>>> + { .compatible = "allwinner,sun50i-h5-ccu" }, >>>> + { } >>>> +}; >>>> + >>>> +U_BOOT_DRIVER(clk_sun8i_h3) = { >>>> + .name = "sun8i_h3_ccu", >>>> + .id = UCLASS_CLK, >>>> + .of_match = h3_clk_ids, >>>> + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), >>>> + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, >>>> + .ops = &h3_clk_ops, >>>> + .probe = h3_clk_probe, >>>> + .bind = sunxi_clk_bind, >>>> +}; >>> >>> Speaking from experience, you do not want to have separate >>> implementations for each and every SoCs. This might be enough for >>> enabling / disabling the clocks, but as soon as you'll throw the >>> set_rate / get_rate callbacks into the mix it's going to turn into a >>> nightmare. >> >> I agree, but I guess it won't be too pretty anyway: >> The CLK_BUS_[EO]HCIx definitions are different for each SoC, but share >> the same symbol. So we can't use a nicely readable switch/case anymore. >> Unless we translate the values to a common namespace? >> >> But I support that we should share as much code as possible, maybe by >> using macros to instantiate the driver boilerplates and by using a >> shared file with the gist of the clock programming code and then just >> have shim layers to connect the bits? >> >> In case it's just bit and register offsets differing, we could define a >> structure holding register and bit offsets, filling this for the various >> SoCs, then tie those together with the compatible strings: >> struct sunxi_clk_defs { >> uint16_t clk_bus_usb_offset; >> uint16_t clk_bus_usb_bit; >> ... >> } sun8i_h3_h5_clk_defs = { >> .clk_bus_usb_offset = 0x60; >> .clk_bus_usb_bit = 23; >> }; >> ... case CLK_BUS_OHCI3: >> clrbits_le32(priv->base + priv->clk_bus_usb_offset, >> BIT(priv->clk_bus_usb_bit + (clk->id - CLK_BUS_OTG))); >> .... >> static const struct udevice_id sunxi_clk_ids[] = { >> { .compatible = "allwinner,sun8i-h3-ccu", >> .data = sun8i_h3_h5_clk_defs }, >> }; >> >> Just an example, not sure we are actually much different in those bits >> there. >> >> Or we put the DT clock numbers into that struct and look those up: >> int sunxi_clk_bus_usb_idx (struct sunxi_clk_defs *priv, int clkid) >> { >> if (clkid >= priv->first_bus_usb && >> clkid <= priv->last_bus_usb) >> return clkid - priv->first_bus_usb; >> return -1; >> } >> static int h3_clk_enable(struct clk *clk) >> { >> ... >> idx = sunxi_clk_bus_usb_idx(priv, clk->id)); >> if (idx >= 0) >> setbits_le32(priv->base + 0x60, BIT(23 + idx)); >> idx = sunxi_clk_usb_phy_idx(priv, clk->id)); >> if (idx >= 0) >> setbits_le32(priv->base + 0xcc, BIT(8 + idx)); > > I guess we could also give to a common code a key / value (register - > offset) pair that would be SoC specific, a simpler version of what > we're doing in Linux. Yeah, something like that. I wonder if it would be useful to implement *two* clocks (USB and MMC or UART) for *two* SoCs, to get a feeling what would be useful and feasible. Definitely having one clock for *all* SoCs (like here) might be a lot of work to potentially throw away, and might not reveal everything we need. Cheers, Andre.
On Tue, Jul 17, 2018 at 01:43:45PM +0100, Andre Przywara wrote: > Hi, > > On 17/07/18 13:36, Maxime Ripard wrote: > > On Mon, Jul 16, 2018 at 05:55:25PM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 16/07/18 13:59, Maxime Ripard wrote: > >>> On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: > >>>> Add initial clock driver Allwinner for H3_H5. > >>>> > >>>> Implemented clock enable and disable functions for > >>>> USB OHCI, EHCI, OTG and PHY gate and clock registers. > >>>> > >>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >>>> --- > >>>> drivers/clk/sunxi/Kconfig | 7 ++ > >>>> drivers/clk/sunxi/Makefile | 2 + > >>>> drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ > >>>> 3 files changed, 140 insertions(+) > >>>> create mode 100644 drivers/clk/sunxi/clk_h3.c > >>>> > >>>> diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig > >>>> index 3a86c91e75..065cadf2fe 100644 > >>>> --- a/drivers/clk/sunxi/Kconfig > >>>> +++ b/drivers/clk/sunxi/Kconfig > >>>> @@ -8,6 +8,13 @@ config CLK_SUNXI > >>>> > >>>> if CLK_SUNXI > >>>> > >>>> +config CLK_SUN8I_H3 > >>>> + bool "Clock driver for Allwinner H3/H5" > >>>> + default MACH_SUNXI_H3_H5 > >>>> + help > >>>> + This enables common clock driver support for platforms based > >>>> + on Allwinner H3/H5 SoC. > >>>> + > >>>> config CLK_SUN50I_A64 > >>>> bool "Clock driver for Allwinner A64" > >>>> default MACH_SUN50I > >>>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile > >>>> index 860bb6dfea..37e6bcb147 100644 > >>>> --- a/drivers/clk/sunxi/Makefile > >>>> +++ b/drivers/clk/sunxi/Makefile > >>>> @@ -5,4 +5,6 @@ > >>>> # > >>>> > >>>> obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o > >>>> + > >>>> +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o > >>>> obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o > >>>> diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c > >>>> new file mode 100644 > >>>> index 0000000000..e924017717 > >>>> --- /dev/null > >>>> +++ b/drivers/clk/sunxi/clk_h3.c > >>>> @@ -0,0 +1,131 @@ > >>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > >>>> +/* > >>>> + * Copyright (C) 2018 Amarula Solutions B.V. > >>>> + * Author: Jagan Teki <jagan@amarulasolutions.com> > >>>> + */ > >>>> + > >>>> +#include <common.h> > >>>> +#include <clk-uclass.h> > >>>> +#include <dm.h> > >>>> +#include <errno.h> > >>>> +#include <asm/io.h> > >>>> +#include <asm/arch/clock.h> > >>>> +#include <dt-bindings/clock/sun8i-h3-ccu.h> > >>>> + > >>>> +struct h3_clk_priv { > >>>> + void *base; > >>>> +}; > >>>> + > >>>> +static int h3_clk_enable(struct clk *clk) > >>>> +{ > >>>> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > >>>> + > >>>> + debug("%s(#%ld)\n", __func__, clk->id); > >>>> + > >>>> + switch (clk->id) { > >>>> + case CLK_BUS_OTG: > >>>> + case CLK_BUS_EHCI0: > >>>> + case CLK_BUS_EHCI1: > >>>> + case CLK_BUS_EHCI2: > >>>> + case CLK_BUS_EHCI3: > >>>> + case CLK_BUS_OHCI0: > >>>> + case CLK_BUS_OHCI1: > >>>> + case CLK_BUS_OHCI2: > >>>> + case CLK_BUS_OHCI3: > >>>> + setbits_le32(priv->base + 0x60, > >>>> + BIT(23 + (clk->id - CLK_BUS_OTG))); > >>>> + return 0; > >>>> + case CLK_USB_PHY0: > >>>> + case CLK_USB_PHY1: > >>>> + case CLK_USB_PHY2: > >>>> + case CLK_USB_PHY3: > >>>> + setbits_le32(priv->base + 0xcc, > >>>> + BIT(8 + (clk->id - CLK_USB_PHY0))); > >>>> + return 0; > >>>> + case CLK_USB_OHCI0: > >>>> + case CLK_USB_OHCI1: > >>>> + case CLK_USB_OHCI2: > >>>> + case CLK_USB_OHCI3: > >>>> + setbits_le32(priv->base + 0xcc, > >>>> + BIT(16 + (clk->id - CLK_USB_OHCI0))); > >>>> + return 0; > >>>> + default: > >>>> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > >>>> + return -ENODEV; > >>>> + } > >>>> +} > >>>> + > >>>> +static int h3_clk_disable(struct clk *clk) > >>>> +{ > >>>> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > >>>> + > >>>> + debug("%s(#%ld)\n", __func__, clk->id); > >>>> + > >>>> + switch (clk->id) { > >>>> + case CLK_BUS_OTG: > >>>> + case CLK_BUS_EHCI0: > >>>> + case CLK_BUS_EHCI1: > >>>> + case CLK_BUS_EHCI2: > >>>> + case CLK_BUS_EHCI3: > >>>> + case CLK_BUS_OHCI0: > >>>> + case CLK_BUS_OHCI1: > >>>> + case CLK_BUS_OHCI2: > >>>> + case CLK_BUS_OHCI3: > >>>> + clrbits_le32(priv->base + 0x60, > >>>> + BIT(23 + (clk->id - CLK_BUS_OTG))); > >>>> + return 0; > >>>> + case CLK_USB_PHY0: > >>>> + case CLK_USB_PHY1: > >>>> + case CLK_USB_PHY2: > >>>> + case CLK_USB_PHY3: > >>>> + clrbits_le32(priv->base + 0xcc, > >>>> + BIT(8 + (clk->id - CLK_USB_PHY0))); > >>>> + return 0; > >>>> + case CLK_USB_OHCI0: > >>>> + case CLK_USB_OHCI1: > >>>> + case CLK_USB_OHCI2: > >>>> + case CLK_USB_OHCI3: > >>>> + clrbits_le32(priv->base + 0xcc, > >>>> + BIT(16 + (clk->id - CLK_USB_OHCI0))); > >>>> + return 0; > >>>> + default: > >>>> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > >>>> + return -ENODEV; > >>>> + } > >>>> +} > >>>> + > >>>> +static struct clk_ops h3_clk_ops = { > >>>> + .enable = h3_clk_enable, > >>>> + .disable = h3_clk_disable, > >>>> +}; > >>>> + > >>>> +static int h3_clk_probe(struct udevice *dev) > >>>> +{ > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int h3_clk_ofdata_to_platdata(struct udevice *dev) > >>>> +{ > >>>> + struct h3_clk_priv *priv = dev_get_priv(dev); > >>>> + > >>>> + priv->base = dev_read_addr_ptr(dev); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static const struct udevice_id h3_clk_ids[] = { > >>>> + { .compatible = "allwinner,sun8i-h3-ccu" }, > >>>> + { .compatible = "allwinner,sun50i-h5-ccu" }, > >>>> + { } > >>>> +}; > >>>> + > >>>> +U_BOOT_DRIVER(clk_sun8i_h3) = { > >>>> + .name = "sun8i_h3_ccu", > >>>> + .id = UCLASS_CLK, > >>>> + .of_match = h3_clk_ids, > >>>> + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), > >>>> + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, > >>>> + .ops = &h3_clk_ops, > >>>> + .probe = h3_clk_probe, > >>>> + .bind = sunxi_clk_bind, > >>>> +}; > >>> > >>> Speaking from experience, you do not want to have separate > >>> implementations for each and every SoCs. This might be enough for > >>> enabling / disabling the clocks, but as soon as you'll throw the > >>> set_rate / get_rate callbacks into the mix it's going to turn into a > >>> nightmare. > >> > >> I agree, but I guess it won't be too pretty anyway: > >> The CLK_BUS_[EO]HCIx definitions are different for each SoC, but share > >> the same symbol. So we can't use a nicely readable switch/case anymore. > >> Unless we translate the values to a common namespace? > >> > >> But I support that we should share as much code as possible, maybe by > >> using macros to instantiate the driver boilerplates and by using a > >> shared file with the gist of the clock programming code and then just > >> have shim layers to connect the bits? > >> > >> In case it's just bit and register offsets differing, we could define a > >> structure holding register and bit offsets, filling this for the various > >> SoCs, then tie those together with the compatible strings: > >> struct sunxi_clk_defs { > >> uint16_t clk_bus_usb_offset; > >> uint16_t clk_bus_usb_bit; > >> ... > >> } sun8i_h3_h5_clk_defs = { > >> .clk_bus_usb_offset = 0x60; > >> .clk_bus_usb_bit = 23; > >> }; > >> ... case CLK_BUS_OHCI3: > >> clrbits_le32(priv->base + priv->clk_bus_usb_offset, > >> BIT(priv->clk_bus_usb_bit + (clk->id - CLK_BUS_OTG))); > >> .... > >> static const struct udevice_id sunxi_clk_ids[] = { > >> { .compatible = "allwinner,sun8i-h3-ccu", > >> .data = sun8i_h3_h5_clk_defs }, > >> }; > >> > >> Just an example, not sure we are actually much different in those bits > >> there. > >> > >> Or we put the DT clock numbers into that struct and look those up: > >> int sunxi_clk_bus_usb_idx (struct sunxi_clk_defs *priv, int clkid) > >> { > >> if (clkid >= priv->first_bus_usb && > >> clkid <= priv->last_bus_usb) > >> return clkid - priv->first_bus_usb; > >> return -1; > >> } > >> static int h3_clk_enable(struct clk *clk) > >> { > >> ... > >> idx = sunxi_clk_bus_usb_idx(priv, clk->id)); > >> if (idx >= 0) > >> setbits_le32(priv->base + 0x60, BIT(23 + idx)); > >> idx = sunxi_clk_usb_phy_idx(priv, clk->id)); > >> if (idx >= 0) > >> setbits_le32(priv->base + 0xcc, BIT(8 + idx)); > > > > I guess we could also give to a common code a key / value (register - > > offset) pair that would be SoC specific, a simpler version of what > > we're doing in Linux. > > Yeah, something like that. > I wonder if it would be useful to implement *two* clocks (USB and MMC or > UART) for *two* SoCs, to get a feeling what would be useful and > feasible. Definitely having one clock for *all* SoCs (like here) might > be a lot of work to potentially throw away, and might not reveal > everything we need. That definitely makes sense yes. Maxime
On Tue, Jul 17, 2018 at 6:06 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > On Mon, Jul 16, 2018 at 05:55:25PM +0100, Andre Przywara wrote: >> Hi, >> >> On 16/07/18 13:59, Maxime Ripard wrote: >> > On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: >> >> Add initial clock driver Allwinner for H3_H5. >> >> >> >> Implemented clock enable and disable functions for >> >> USB OHCI, EHCI, OTG and PHY gate and clock registers. >> >> >> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >> >> --- >> >> drivers/clk/sunxi/Kconfig | 7 ++ >> >> drivers/clk/sunxi/Makefile | 2 + >> >> drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 140 insertions(+) >> >> create mode 100644 drivers/clk/sunxi/clk_h3.c >> >> >> >> diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig >> >> index 3a86c91e75..065cadf2fe 100644 >> >> --- a/drivers/clk/sunxi/Kconfig >> >> +++ b/drivers/clk/sunxi/Kconfig >> >> @@ -8,6 +8,13 @@ config CLK_SUNXI >> >> >> >> if CLK_SUNXI >> >> >> >> +config CLK_SUN8I_H3 >> >> + bool "Clock driver for Allwinner H3/H5" >> >> + default MACH_SUNXI_H3_H5 >> >> + help >> >> + This enables common clock driver support for platforms based >> >> + on Allwinner H3/H5 SoC. >> >> + >> >> config CLK_SUN50I_A64 >> >> bool "Clock driver for Allwinner A64" >> >> default MACH_SUN50I >> >> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile >> >> index 860bb6dfea..37e6bcb147 100644 >> >> --- a/drivers/clk/sunxi/Makefile >> >> +++ b/drivers/clk/sunxi/Makefile >> >> @@ -5,4 +5,6 @@ >> >> # >> >> >> >> obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o >> >> + >> >> +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o >> >> obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o >> >> diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c >> >> new file mode 100644 >> >> index 0000000000..e924017717 >> >> --- /dev/null >> >> +++ b/drivers/clk/sunxi/clk_h3.c >> >> @@ -0,0 +1,131 @@ >> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> >> +/* >> >> + * Copyright (C) 2018 Amarula Solutions B.V. >> >> + * Author: Jagan Teki <jagan@amarulasolutions.com> >> >> + */ >> >> + >> >> +#include <common.h> >> >> +#include <clk-uclass.h> >> >> +#include <dm.h> >> >> +#include <errno.h> >> >> +#include <asm/io.h> >> >> +#include <asm/arch/clock.h> >> >> +#include <dt-bindings/clock/sun8i-h3-ccu.h> >> >> + >> >> +struct h3_clk_priv { >> >> + void *base; >> >> +}; >> >> + >> >> +static int h3_clk_enable(struct clk *clk) >> >> +{ >> >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >> >> + >> >> + debug("%s(#%ld)\n", __func__, clk->id); >> >> + >> >> + switch (clk->id) { >> >> + case CLK_BUS_OTG: >> >> + case CLK_BUS_EHCI0: >> >> + case CLK_BUS_EHCI1: >> >> + case CLK_BUS_EHCI2: >> >> + case CLK_BUS_EHCI3: >> >> + case CLK_BUS_OHCI0: >> >> + case CLK_BUS_OHCI1: >> >> + case CLK_BUS_OHCI2: >> >> + case CLK_BUS_OHCI3: >> >> + setbits_le32(priv->base + 0x60, >> >> + BIT(23 + (clk->id - CLK_BUS_OTG))); >> >> + return 0; >> >> + case CLK_USB_PHY0: >> >> + case CLK_USB_PHY1: >> >> + case CLK_USB_PHY2: >> >> + case CLK_USB_PHY3: >> >> + setbits_le32(priv->base + 0xcc, >> >> + BIT(8 + (clk->id - CLK_USB_PHY0))); >> >> + return 0; >> >> + case CLK_USB_OHCI0: >> >> + case CLK_USB_OHCI1: >> >> + case CLK_USB_OHCI2: >> >> + case CLK_USB_OHCI3: >> >> + setbits_le32(priv->base + 0xcc, >> >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >> >> + return 0; >> >> + default: >> >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >> >> + return -ENODEV; >> >> + } >> >> +} >> >> + >> >> +static int h3_clk_disable(struct clk *clk) >> >> +{ >> >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); >> >> + >> >> + debug("%s(#%ld)\n", __func__, clk->id); >> >> + >> >> + switch (clk->id) { >> >> + case CLK_BUS_OTG: >> >> + case CLK_BUS_EHCI0: >> >> + case CLK_BUS_EHCI1: >> >> + case CLK_BUS_EHCI2: >> >> + case CLK_BUS_EHCI3: >> >> + case CLK_BUS_OHCI0: >> >> + case CLK_BUS_OHCI1: >> >> + case CLK_BUS_OHCI2: >> >> + case CLK_BUS_OHCI3: >> >> + clrbits_le32(priv->base + 0x60, >> >> + BIT(23 + (clk->id - CLK_BUS_OTG))); >> >> + return 0; >> >> + case CLK_USB_PHY0: >> >> + case CLK_USB_PHY1: >> >> + case CLK_USB_PHY2: >> >> + case CLK_USB_PHY3: >> >> + clrbits_le32(priv->base + 0xcc, >> >> + BIT(8 + (clk->id - CLK_USB_PHY0))); >> >> + return 0; >> >> + case CLK_USB_OHCI0: >> >> + case CLK_USB_OHCI1: >> >> + case CLK_USB_OHCI2: >> >> + case CLK_USB_OHCI3: >> >> + clrbits_le32(priv->base + 0xcc, >> >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); >> >> + return 0; >> >> + default: >> >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); >> >> + return -ENODEV; >> >> + } >> >> +} >> >> + >> >> +static struct clk_ops h3_clk_ops = { >> >> + .enable = h3_clk_enable, >> >> + .disable = h3_clk_disable, >> >> +}; >> >> + >> >> +static int h3_clk_probe(struct udevice *dev) >> >> +{ >> >> + return 0; >> >> +} >> >> + >> >> +static int h3_clk_ofdata_to_platdata(struct udevice *dev) >> >> +{ >> >> + struct h3_clk_priv *priv = dev_get_priv(dev); >> >> + >> >> + priv->base = dev_read_addr_ptr(dev); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static const struct udevice_id h3_clk_ids[] = { >> >> + { .compatible = "allwinner,sun8i-h3-ccu" }, >> >> + { .compatible = "allwinner,sun50i-h5-ccu" }, >> >> + { } >> >> +}; >> >> + >> >> +U_BOOT_DRIVER(clk_sun8i_h3) = { >> >> + .name = "sun8i_h3_ccu", >> >> + .id = UCLASS_CLK, >> >> + .of_match = h3_clk_ids, >> >> + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), >> >> + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, >> >> + .ops = &h3_clk_ops, >> >> + .probe = h3_clk_probe, >> >> + .bind = sunxi_clk_bind, >> >> +}; >> > >> > Speaking from experience, you do not want to have separate >> > implementations for each and every SoCs. This might be enough for >> > enabling / disabling the clocks, but as soon as you'll throw the >> > set_rate / get_rate callbacks into the mix it's going to turn into a >> > nightmare. >> >> I agree, but I guess it won't be too pretty anyway: >> The CLK_BUS_[EO]HCIx definitions are different for each SoC, but share >> the same symbol. So we can't use a nicely readable switch/case anymore. >> Unless we translate the values to a common namespace? >> >> But I support that we should share as much code as possible, maybe by >> using macros to instantiate the driver boilerplates and by using a >> shared file with the gist of the clock programming code and then just >> have shim layers to connect the bits? >> >> In case it's just bit and register offsets differing, we could define a >> structure holding register and bit offsets, filling this for the various >> SoCs, then tie those together with the compatible strings: >> struct sunxi_clk_defs { >> uint16_t clk_bus_usb_offset; >> uint16_t clk_bus_usb_bit; >> ... >> } sun8i_h3_h5_clk_defs = { >> .clk_bus_usb_offset = 0x60; >> .clk_bus_usb_bit = 23; >> }; >> ... case CLK_BUS_OHCI3: >> clrbits_le32(priv->base + priv->clk_bus_usb_offset, >> BIT(priv->clk_bus_usb_bit + (clk->id - CLK_BUS_OTG))); >> .... >> static const struct udevice_id sunxi_clk_ids[] = { >> { .compatible = "allwinner,sun8i-h3-ccu", >> .data = sun8i_h3_h5_clk_defs }, >> }; >> >> Just an example, not sure we are actually much different in those bits >> there. >> >> Or we put the DT clock numbers into that struct and look those up: >> int sunxi_clk_bus_usb_idx (struct sunxi_clk_defs *priv, int clkid) >> { >> if (clkid >= priv->first_bus_usb && >> clkid <= priv->last_bus_usb) >> return clkid - priv->first_bus_usb; >> return -1; >> } >> static int h3_clk_enable(struct clk *clk) >> { >> ... >> idx = sunxi_clk_bus_usb_idx(priv, clk->id)); >> if (idx >= 0) >> setbits_le32(priv->base + 0x60, BIT(23 + idx)); >> idx = sunxi_clk_usb_phy_idx(priv, clk->id)); >> if (idx >= 0) >> setbits_le32(priv->base + 0xcc, BIT(8 + idx)); > > I guess we could also give to a common code a key / value (register - > offset) pair that would be SoC specific, a simpler version of what > we're doing in Linux. is it something like ccu_reset_map struct with reg and bit members? [RST_USB_PHY0] = { 0x0cc, BIT(0) },
On Tue, Jul 17, 2018 at 10:20:42PM +0530, Jagan Teki wrote: > On Tue, Jul 17, 2018 at 6:06 PM, Maxime Ripard > <maxime.ripard@bootlin.com> wrote: > > On Mon, Jul 16, 2018 at 05:55:25PM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 16/07/18 13:59, Maxime Ripard wrote: > >> > On Mon, Jul 16, 2018 at 04:58:32PM +0530, Jagan Teki wrote: > >> >> Add initial clock driver Allwinner for H3_H5. > >> >> > >> >> Implemented clock enable and disable functions for > >> >> USB OHCI, EHCI, OTG and PHY gate and clock registers. > >> >> > >> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >> >> --- > >> >> drivers/clk/sunxi/Kconfig | 7 ++ > >> >> drivers/clk/sunxi/Makefile | 2 + > >> >> drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ > >> >> 3 files changed, 140 insertions(+) > >> >> create mode 100644 drivers/clk/sunxi/clk_h3.c > >> >> > >> >> diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig > >> >> index 3a86c91e75..065cadf2fe 100644 > >> >> --- a/drivers/clk/sunxi/Kconfig > >> >> +++ b/drivers/clk/sunxi/Kconfig > >> >> @@ -8,6 +8,13 @@ config CLK_SUNXI > >> >> > >> >> if CLK_SUNXI > >> >> > >> >> +config CLK_SUN8I_H3 > >> >> + bool "Clock driver for Allwinner H3/H5" > >> >> + default MACH_SUNXI_H3_H5 > >> >> + help > >> >> + This enables common clock driver support for platforms based > >> >> + on Allwinner H3/H5 SoC. > >> >> + > >> >> config CLK_SUN50I_A64 > >> >> bool "Clock driver for Allwinner A64" > >> >> default MACH_SUN50I > >> >> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile > >> >> index 860bb6dfea..37e6bcb147 100644 > >> >> --- a/drivers/clk/sunxi/Makefile > >> >> +++ b/drivers/clk/sunxi/Makefile > >> >> @@ -5,4 +5,6 @@ > >> >> # > >> >> > >> >> obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o > >> >> + > >> >> +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o > >> >> obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o > >> >> diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c > >> >> new file mode 100644 > >> >> index 0000000000..e924017717 > >> >> --- /dev/null > >> >> +++ b/drivers/clk/sunxi/clk_h3.c > >> >> @@ -0,0 +1,131 @@ > >> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > >> >> +/* > >> >> + * Copyright (C) 2018 Amarula Solutions B.V. > >> >> + * Author: Jagan Teki <jagan@amarulasolutions.com> > >> >> + */ > >> >> + > >> >> +#include <common.h> > >> >> +#include <clk-uclass.h> > >> >> +#include <dm.h> > >> >> +#include <errno.h> > >> >> +#include <asm/io.h> > >> >> +#include <asm/arch/clock.h> > >> >> +#include <dt-bindings/clock/sun8i-h3-ccu.h> > >> >> + > >> >> +struct h3_clk_priv { > >> >> + void *base; > >> >> +}; > >> >> + > >> >> +static int h3_clk_enable(struct clk *clk) > >> >> +{ > >> >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > >> >> + > >> >> + debug("%s(#%ld)\n", __func__, clk->id); > >> >> + > >> >> + switch (clk->id) { > >> >> + case CLK_BUS_OTG: > >> >> + case CLK_BUS_EHCI0: > >> >> + case CLK_BUS_EHCI1: > >> >> + case CLK_BUS_EHCI2: > >> >> + case CLK_BUS_EHCI3: > >> >> + case CLK_BUS_OHCI0: > >> >> + case CLK_BUS_OHCI1: > >> >> + case CLK_BUS_OHCI2: > >> >> + case CLK_BUS_OHCI3: > >> >> + setbits_le32(priv->base + 0x60, > >> >> + BIT(23 + (clk->id - CLK_BUS_OTG))); > >> >> + return 0; > >> >> + case CLK_USB_PHY0: > >> >> + case CLK_USB_PHY1: > >> >> + case CLK_USB_PHY2: > >> >> + case CLK_USB_PHY3: > >> >> + setbits_le32(priv->base + 0xcc, > >> >> + BIT(8 + (clk->id - CLK_USB_PHY0))); > >> >> + return 0; > >> >> + case CLK_USB_OHCI0: > >> >> + case CLK_USB_OHCI1: > >> >> + case CLK_USB_OHCI2: > >> >> + case CLK_USB_OHCI3: > >> >> + setbits_le32(priv->base + 0xcc, > >> >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); > >> >> + return 0; > >> >> + default: > >> >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > >> >> + return -ENODEV; > >> >> + } > >> >> +} > >> >> + > >> >> +static int h3_clk_disable(struct clk *clk) > >> >> +{ > >> >> + struct h3_clk_priv *priv = dev_get_priv(clk->dev); > >> >> + > >> >> + debug("%s(#%ld)\n", __func__, clk->id); > >> >> + > >> >> + switch (clk->id) { > >> >> + case CLK_BUS_OTG: > >> >> + case CLK_BUS_EHCI0: > >> >> + case CLK_BUS_EHCI1: > >> >> + case CLK_BUS_EHCI2: > >> >> + case CLK_BUS_EHCI3: > >> >> + case CLK_BUS_OHCI0: > >> >> + case CLK_BUS_OHCI1: > >> >> + case CLK_BUS_OHCI2: > >> >> + case CLK_BUS_OHCI3: > >> >> + clrbits_le32(priv->base + 0x60, > >> >> + BIT(23 + (clk->id - CLK_BUS_OTG))); > >> >> + return 0; > >> >> + case CLK_USB_PHY0: > >> >> + case CLK_USB_PHY1: > >> >> + case CLK_USB_PHY2: > >> >> + case CLK_USB_PHY3: > >> >> + clrbits_le32(priv->base + 0xcc, > >> >> + BIT(8 + (clk->id - CLK_USB_PHY0))); > >> >> + return 0; > >> >> + case CLK_USB_OHCI0: > >> >> + case CLK_USB_OHCI1: > >> >> + case CLK_USB_OHCI2: > >> >> + case CLK_USB_OHCI3: > >> >> + clrbits_le32(priv->base + 0xcc, > >> >> + BIT(16 + (clk->id - CLK_USB_OHCI0))); > >> >> + return 0; > >> >> + default: > >> >> + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); > >> >> + return -ENODEV; > >> >> + } > >> >> +} > >> >> + > >> >> +static struct clk_ops h3_clk_ops = { > >> >> + .enable = h3_clk_enable, > >> >> + .disable = h3_clk_disable, > >> >> +}; > >> >> + > >> >> +static int h3_clk_probe(struct udevice *dev) > >> >> +{ > >> >> + return 0; > >> >> +} > >> >> + > >> >> +static int h3_clk_ofdata_to_platdata(struct udevice *dev) > >> >> +{ > >> >> + struct h3_clk_priv *priv = dev_get_priv(dev); > >> >> + > >> >> + priv->base = dev_read_addr_ptr(dev); > >> >> + > >> >> + return 0; > >> >> +} > >> >> + > >> >> +static const struct udevice_id h3_clk_ids[] = { > >> >> + { .compatible = "allwinner,sun8i-h3-ccu" }, > >> >> + { .compatible = "allwinner,sun50i-h5-ccu" }, > >> >> + { } > >> >> +}; > >> >> + > >> >> +U_BOOT_DRIVER(clk_sun8i_h3) = { > >> >> + .name = "sun8i_h3_ccu", > >> >> + .id = UCLASS_CLK, > >> >> + .of_match = h3_clk_ids, > >> >> + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), > >> >> + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, > >> >> + .ops = &h3_clk_ops, > >> >> + .probe = h3_clk_probe, > >> >> + .bind = sunxi_clk_bind, > >> >> +}; > >> > > >> > Speaking from experience, you do not want to have separate > >> > implementations for each and every SoCs. This might be enough for > >> > enabling / disabling the clocks, but as soon as you'll throw the > >> > set_rate / get_rate callbacks into the mix it's going to turn into a > >> > nightmare. > >> > >> I agree, but I guess it won't be too pretty anyway: > >> The CLK_BUS_[EO]HCIx definitions are different for each SoC, but share > >> the same symbol. So we can't use a nicely readable switch/case anymore. > >> Unless we translate the values to a common namespace? > >> > >> But I support that we should share as much code as possible, maybe by > >> using macros to instantiate the driver boilerplates and by using a > >> shared file with the gist of the clock programming code and then just > >> have shim layers to connect the bits? > >> > >> In case it's just bit and register offsets differing, we could define a > >> structure holding register and bit offsets, filling this for the various > >> SoCs, then tie those together with the compatible strings: > >> struct sunxi_clk_defs { > >> uint16_t clk_bus_usb_offset; > >> uint16_t clk_bus_usb_bit; > >> ... > >> } sun8i_h3_h5_clk_defs = { > >> .clk_bus_usb_offset = 0x60; > >> .clk_bus_usb_bit = 23; > >> }; > >> ... case CLK_BUS_OHCI3: > >> clrbits_le32(priv->base + priv->clk_bus_usb_offset, > >> BIT(priv->clk_bus_usb_bit + (clk->id - CLK_BUS_OTG))); > >> .... > >> static const struct udevice_id sunxi_clk_ids[] = { > >> { .compatible = "allwinner,sun8i-h3-ccu", > >> .data = sun8i_h3_h5_clk_defs }, > >> }; > >> > >> Just an example, not sure we are actually much different in those bits > >> there. > >> > >> Or we put the DT clock numbers into that struct and look those up: > >> int sunxi_clk_bus_usb_idx (struct sunxi_clk_defs *priv, int clkid) > >> { > >> if (clkid >= priv->first_bus_usb && > >> clkid <= priv->last_bus_usb) > >> return clkid - priv->first_bus_usb; > >> return -1; > >> } > >> static int h3_clk_enable(struct clk *clk) > >> { > >> ... > >> idx = sunxi_clk_bus_usb_idx(priv, clk->id)); > >> if (idx >= 0) > >> setbits_le32(priv->base + 0x60, BIT(23 + idx)); > >> idx = sunxi_clk_usb_phy_idx(priv, clk->id)); > >> if (idx >= 0) > >> setbits_le32(priv->base + 0xcc, BIT(8 + idx)); > > > > I guess we could also give to a common code a key / value (register - > > offset) pair that would be SoC specific, a simpler version of what > > we're doing in Linux. > > is it something like ccu_reset_map struct with reg and bit members? > [RST_USB_PHY0] = { 0x0cc, BIT(0) }, Yes, something along those lines. Maxime
diff --git a/drivers/clk/sunxi/Kconfig b/drivers/clk/sunxi/Kconfig index 3a86c91e75..065cadf2fe 100644 --- a/drivers/clk/sunxi/Kconfig +++ b/drivers/clk/sunxi/Kconfig @@ -8,6 +8,13 @@ config CLK_SUNXI if CLK_SUNXI +config CLK_SUN8I_H3 + bool "Clock driver for Allwinner H3/H5" + default MACH_SUNXI_H3_H5 + help + This enables common clock driver support for platforms based + on Allwinner H3/H5 SoC. + config CLK_SUN50I_A64 bool "Clock driver for Allwinner A64" default MACH_SUN50I diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile index 860bb6dfea..37e6bcb147 100644 --- a/drivers/clk/sunxi/Makefile +++ b/drivers/clk/sunxi/Makefile @@ -5,4 +5,6 @@ # obj-$(CONFIG_CLK_SUNXI) += clk_sunxi.o + +obj-$(CONFIG_CLK_SUN8I_H3) += clk_h3.o obj-$(CONFIG_CLK_SUN50I_A64) += clk_a64.o diff --git a/drivers/clk/sunxi/clk_h3.c b/drivers/clk/sunxi/clk_h3.c new file mode 100644 index 0000000000..e924017717 --- /dev/null +++ b/drivers/clk/sunxi/clk_h3.c @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (C) 2018 Amarula Solutions B.V. + * Author: Jagan Teki <jagan@amarulasolutions.com> + */ + +#include <common.h> +#include <clk-uclass.h> +#include <dm.h> +#include <errno.h> +#include <asm/io.h> +#include <asm/arch/clock.h> +#include <dt-bindings/clock/sun8i-h3-ccu.h> + +struct h3_clk_priv { + void *base; +}; + +static int h3_clk_enable(struct clk *clk) +{ + struct h3_clk_priv *priv = dev_get_priv(clk->dev); + + debug("%s(#%ld)\n", __func__, clk->id); + + switch (clk->id) { + case CLK_BUS_OTG: + case CLK_BUS_EHCI0: + case CLK_BUS_EHCI1: + case CLK_BUS_EHCI2: + case CLK_BUS_EHCI3: + case CLK_BUS_OHCI0: + case CLK_BUS_OHCI1: + case CLK_BUS_OHCI2: + case CLK_BUS_OHCI3: + setbits_le32(priv->base + 0x60, + BIT(23 + (clk->id - CLK_BUS_OTG))); + return 0; + case CLK_USB_PHY0: + case CLK_USB_PHY1: + case CLK_USB_PHY2: + case CLK_USB_PHY3: + setbits_le32(priv->base + 0xcc, + BIT(8 + (clk->id - CLK_USB_PHY0))); + return 0; + case CLK_USB_OHCI0: + case CLK_USB_OHCI1: + case CLK_USB_OHCI2: + case CLK_USB_OHCI3: + setbits_le32(priv->base + 0xcc, + BIT(16 + (clk->id - CLK_USB_OHCI0))); + return 0; + default: + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); + return -ENODEV; + } +} + +static int h3_clk_disable(struct clk *clk) +{ + struct h3_clk_priv *priv = dev_get_priv(clk->dev); + + debug("%s(#%ld)\n", __func__, clk->id); + + switch (clk->id) { + case CLK_BUS_OTG: + case CLK_BUS_EHCI0: + case CLK_BUS_EHCI1: + case CLK_BUS_EHCI2: + case CLK_BUS_EHCI3: + case CLK_BUS_OHCI0: + case CLK_BUS_OHCI1: + case CLK_BUS_OHCI2: + case CLK_BUS_OHCI3: + clrbits_le32(priv->base + 0x60, + BIT(23 + (clk->id - CLK_BUS_OTG))); + return 0; + case CLK_USB_PHY0: + case CLK_USB_PHY1: + case CLK_USB_PHY2: + case CLK_USB_PHY3: + clrbits_le32(priv->base + 0xcc, + BIT(8 + (clk->id - CLK_USB_PHY0))); + return 0; + case CLK_USB_OHCI0: + case CLK_USB_OHCI1: + case CLK_USB_OHCI2: + case CLK_USB_OHCI3: + clrbits_le32(priv->base + 0xcc, + BIT(16 + (clk->id - CLK_USB_OHCI0))); + return 0; + default: + debug("%s (CLK#%ld) unhandled\n", __func__, clk->id); + return -ENODEV; + } +} + +static struct clk_ops h3_clk_ops = { + .enable = h3_clk_enable, + .disable = h3_clk_disable, +}; + +static int h3_clk_probe(struct udevice *dev) +{ + return 0; +} + +static int h3_clk_ofdata_to_platdata(struct udevice *dev) +{ + struct h3_clk_priv *priv = dev_get_priv(dev); + + priv->base = dev_read_addr_ptr(dev); + + return 0; +} + +static const struct udevice_id h3_clk_ids[] = { + { .compatible = "allwinner,sun8i-h3-ccu" }, + { .compatible = "allwinner,sun50i-h5-ccu" }, + { } +}; + +U_BOOT_DRIVER(clk_sun8i_h3) = { + .name = "sun8i_h3_ccu", + .id = UCLASS_CLK, + .of_match = h3_clk_ids, + .priv_auto_alloc_size = sizeof(struct h3_clk_priv), + .ofdata_to_platdata = h3_clk_ofdata_to_platdata, + .ops = &h3_clk_ops, + .probe = h3_clk_probe, + .bind = sunxi_clk_bind, +};
Add initial clock driver Allwinner for H3_H5. Implemented clock enable and disable functions for USB OHCI, EHCI, OTG and PHY gate and clock registers. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/clk/sunxi/Kconfig | 7 ++ drivers/clk/sunxi/Makefile | 2 + drivers/clk/sunxi/clk_h3.c | 131 +++++++++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+) create mode 100644 drivers/clk/sunxi/clk_h3.c