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 basedon 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.