Re: [U-Boot] [RFC 17/35] clk: sunxi: Add initial CLK driver for H3_H5

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