[U-Boot] [RFC 17/35] clk: sunxi: Add initial CLK driver for H3_H5
Maxime Ripard
maxime.ripard at bootlin.com
Tue Jul 17 15:20:44 UTC 2018
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 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 },
> >> };
> >>
> >> 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
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180717/c68a88a5/attachment.sig>
More information about the U-Boot
mailing list