sunxi: Add initial CLK, RESET support

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +121 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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" },
+	{ }
+};
+

Quoted 177 lines · Jagan Teki

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +129 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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,

drivers/clk/sunxi/clk_h3.c line 131

Quoted 2 lines

drivers/clk/sunxi/clk_h3.c new file +2 -0

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

Hi,

Quoted 178 lines · Maxime Ripard

Quoted 177 lines · Jagan Teki

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +129 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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,

Quoted 8 lines

drivers/clk/sunxi/clk_h3.c line 131

Quoted 2 lines

drivers/clk/sunxi/clk_h3.c new file +2 -0

+    .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));

Hi,

Quoted 178 lines · Maxime Ripard

Quoted 177 lines · Jagan Teki

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +129 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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,

Quoted 8 lines

drivers/clk/sunxi/clk_h3.c line 131

Quoted 2 lines

drivers/clk/sunxi/clk_h3.c new file +2 -0

+	.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));

Quoted 225 lines · Andre Przywara

Hi,

Quoted 178 lines · Maxime Ripard

Quoted 177 lines · Jagan Teki

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +129 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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,

Quoted 8 lines

drivers/clk/sunxi/clk_h3.c line 131

Quoted 2 lines

drivers/clk/sunxi/clk_h3.c new file +2 -0

+	.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)

Quoted 15 lines

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

Hi,

Quoted 242 lines · Maxime Ripard

Quoted 225 lines · Andre Przywara

Hi,

Quoted 178 lines · Maxime Ripard

Quoted 177 lines · Jagan Teki

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +129 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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,

Quoted 8 lines

drivers/clk/sunxi/clk_h3.c line 131

Quoted 2 lines

drivers/clk/sunxi/clk_h3.c new file +2 -0

+	.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)

Quoted 15 lines

{
	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));

Quoted 3 lines

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.

Quoted 225 lines · Andre Przywara

Hi,

Quoted 178 lines · Maxime Ripard

Quoted 177 lines · Jagan Teki

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +129 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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,

Quoted 8 lines

drivers/clk/sunxi/clk_h3.c line 131

Quoted 2 lines

drivers/clk/sunxi/clk_h3.c new file +2 -0

+  .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)

Quoted 15 lines

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

Quoted 242 lines · Maxime Ripard

Quoted 240 lines · Andre Przywara

Hi,

Quoted 178 lines · Maxime Ripard

Quoted 177 lines · Jagan Teki

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 <[email protected]>

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

drivers/clk/sunxi/Kconfig +7 -0

index 3a86c91e75..065cadf2fe 100644
@@ -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

drivers/clk/sunxi/Makefile +2 -0

index 860bb6dfea..37e6bcb147 100644
@@ -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

drivers/clk/sunxi/clk_h3.c new file +129 -0

new file mode 100644
index 0000000000..e924017717
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions B.V.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#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,

Quoted 8 lines

drivers/clk/sunxi/clk_h3.c line 131

Quoted 2 lines

drivers/clk/sunxi/clk_h3.c new file +2 -0

+  .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));

Quoted 3 lines

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) },