[U-Boot] [RFC 17/35] clk: sunxi: Add initial CLK driver for H3_H5
Jagan Teki
jagan at amarulasolutions.com
Mon Jul 16 18:13:36 UTC 2018
On Mon, Jul 16, 2018 at 10:25 PM, Andre Przywara <andre.przywara at 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 at 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 at 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.
More information about the U-Boot
mailing list