| Message ID | 20171203091713.22029-4-b.galvani@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 03/12/2017 10:17, Beniamino Galvani wrote: > Introduce a basic clock driver for Amlogic Meson SoCs which supports > enabling/disabling clock gates and getting their frequency. > > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> > --- > arch/arm/mach-meson/Kconfig | 2 + > drivers/clk/Makefile | 1 + > drivers/clk/clk_meson.c | 196 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 199 insertions(+) > create mode 100644 drivers/clk/clk_meson.c > > diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig > index d4bd230be3..7acee3bc5c 100644 > --- a/arch/arm/mach-meson/Kconfig > +++ b/arch/arm/mach-meson/Kconfig > @@ -3,6 +3,7 @@ if ARCH_MESON > config MESON_GXBB > bool "Support Meson GXBaby" > select ARM64 > + select CLK > select DM > select DM_SERIAL > help > @@ -12,6 +13,7 @@ config MESON_GXBB > config MESON_GXL > bool "Support Meson GXL" > select ARM64 > + select CLK > select DM > select DM_SERIAL > help > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index bcc8f82fb6..67da27873d 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -7,6 +7,7 @@ > > obj-$(CONFIG_$(SPL_TPL_)CLK) += clk-uclass.o clk_fixed_rate.o > obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip/ > +obj-$(CONFIG_ARCH_MESON) += clk_meson.o > obj-$(CONFIG_SANDBOX) += clk_sandbox.o > obj-$(CONFIG_SANDBOX) += clk_sandbox_test.o > obj-$(CONFIG_MACH_PIC32) += clk_pic32.o > diff --git a/drivers/clk/clk_meson.c b/drivers/clk/clk_meson.c > new file mode 100644 > index 0000000000..3cf9372e05 > --- /dev/null > +++ b/drivers/clk/clk_meson.c > @@ -0,0 +1,196 @@ > +/* > + * (C) Copyright 2017 - Beniamino Galvani <b.galvani@gmail.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <asm/arch/clock.h> > +#include <asm/io.h> > +#include <clk-uclass.h> > +#include <dm.h> > +#include <dt-bindings/clock/gxbb-clkc.h> > + > +struct meson_clk { > + void __iomem *addr; > + ulong rate; > +}; > + > +struct meson_gate { > + unsigned int reg; > + unsigned int bit; > +}; > + > +#define MESON_GATE(id, _reg, _bit) \ > + [id] = { \ > + .reg = (_reg), \ > + .bit = (_bit), \ > + } > + > +struct meson_gate gates[] = { > + /* Everything Else (EE) domain gates */ > + MESON_GATE(CLKID_DDR, HHI_GCLK_MPEG0, 0), > + MESON_GATE(CLKID_DOS, HHI_GCLK_MPEG0, 1), > + MESON_GATE(CLKID_ISA, HHI_GCLK_MPEG0, 5), > + MESON_GATE(CLKID_PL301, HHI_GCLK_MPEG0, 6), > + MESON_GATE(CLKID_PERIPHS, HHI_GCLK_MPEG0, 7), > + MESON_GATE(CLKID_SPICC, HHI_GCLK_MPEG0, 8), > + MESON_GATE(CLKID_I2C, HHI_GCLK_MPEG0, 9), > + MESON_GATE(CLKID_SAR_ADC, HHI_GCLK_MPEG0, 10), > + MESON_GATE(CLKID_SMART_CARD, HHI_GCLK_MPEG0, 11), > + MESON_GATE(CLKID_RNG0, HHI_GCLK_MPEG0, 12), > + MESON_GATE(CLKID_UART0, HHI_GCLK_MPEG0, 13), > + MESON_GATE(CLKID_SDHC, HHI_GCLK_MPEG0, 14), > + MESON_GATE(CLKID_STREAM, HHI_GCLK_MPEG0, 15), > + MESON_GATE(CLKID_ASYNC_FIFO, HHI_GCLK_MPEG0, 16), > + MESON_GATE(CLKID_SDIO, HHI_GCLK_MPEG0, 17), > + MESON_GATE(CLKID_ABUF, HHI_GCLK_MPEG0, 18), > + MESON_GATE(CLKID_HIU_IFACE, HHI_GCLK_MPEG0, 19), > + MESON_GATE(CLKID_ASSIST_MISC, HHI_GCLK_MPEG0, 23), > + MESON_GATE(CLKID_SD_EMMC_A, HHI_GCLK_MPEG0, 24), > + MESON_GATE(CLKID_SD_EMMC_B, HHI_GCLK_MPEG0, 25), > + MESON_GATE(CLKID_SD_EMMC_C, HHI_GCLK_MPEG0, 26), > + MESON_GATE(CLKID_SPI, HHI_GCLK_MPEG0, 30), > + > + MESON_GATE(CLKID_I2S_SPDIF, HHI_GCLK_MPEG1, 2), > + MESON_GATE(CLKID_ETH, HHI_GCLK_MPEG1, 3), > + MESON_GATE(CLKID_DEMUX, HHI_GCLK_MPEG1, 4), > + MESON_GATE(CLKID_AIU_GLUE, HHI_GCLK_MPEG1, 6), > + MESON_GATE(CLKID_IEC958, HHI_GCLK_MPEG1, 7), > + MESON_GATE(CLKID_I2S_OUT, HHI_GCLK_MPEG1, 8), > + MESON_GATE(CLKID_AMCLK, HHI_GCLK_MPEG1, 9), > + MESON_GATE(CLKID_AIFIFO2, HHI_GCLK_MPEG1, 10), > + MESON_GATE(CLKID_MIXER, HHI_GCLK_MPEG1, 11), > + MESON_GATE(CLKID_MIXER_IFACE, HHI_GCLK_MPEG1, 12), > + MESON_GATE(CLKID_ADC, HHI_GCLK_MPEG1, 13), > + MESON_GATE(CLKID_BLKMV, HHI_GCLK_MPEG1, 14), > + MESON_GATE(CLKID_AIU, HHI_GCLK_MPEG1, 15), > + MESON_GATE(CLKID_UART1, HHI_GCLK_MPEG1, 16), > + MESON_GATE(CLKID_G2D, HHI_GCLK_MPEG1, 20), > + MESON_GATE(CLKID_USB0, HHI_GCLK_MPEG1, 21), > + MESON_GATE(CLKID_USB1, HHI_GCLK_MPEG1, 22), > + MESON_GATE(CLKID_RESET, HHI_GCLK_MPEG1, 23), > + MESON_GATE(CLKID_NAND, HHI_GCLK_MPEG1, 24), > + MESON_GATE(CLKID_DOS_PARSER, HHI_GCLK_MPEG1, 25), > + MESON_GATE(CLKID_USB, HHI_GCLK_MPEG1, 26), > + MESON_GATE(CLKID_VDIN1, HHI_GCLK_MPEG1, 28), > + MESON_GATE(CLKID_AHB_ARB0, HHI_GCLK_MPEG1, 29), > + MESON_GATE(CLKID_EFUSE, HHI_GCLK_MPEG1, 30), > + MESON_GATE(CLKID_BOOT_ROM, HHI_GCLK_MPEG1, 31), > + > + MESON_GATE(CLKID_AHB_DATA_BUS, HHI_GCLK_MPEG2, 1), > + MESON_GATE(CLKID_AHB_CTRL_BUS, HHI_GCLK_MPEG2, 2), > + MESON_GATE(CLKID_HDMI_INTR_SYNC, HHI_GCLK_MPEG2, 3), > + MESON_GATE(CLKID_HDMI_PCLK, HHI_GCLK_MPEG2, 4), > + MESON_GATE(CLKID_USB1_DDR_BRIDGE, HHI_GCLK_MPEG2, 8), > + MESON_GATE(CLKID_USB0_DDR_BRIDGE, HHI_GCLK_MPEG2, 9), > + MESON_GATE(CLKID_MMC_PCLK, HHI_GCLK_MPEG2, 11), > + MESON_GATE(CLKID_DVIN, HHI_GCLK_MPEG2, 12), > + MESON_GATE(CLKID_UART2, HHI_GCLK_MPEG2, 15), > + MESON_GATE(CLKID_SANA, HHI_GCLK_MPEG2, 22), > + MESON_GATE(CLKID_VPU_INTR, HHI_GCLK_MPEG2, 25), > + MESON_GATE(CLKID_SEC_AHB_AHB3_BRIDGE, HHI_GCLK_MPEG2, 26), > + MESON_GATE(CLKID_CLK81_A53, HHI_GCLK_MPEG2, 29), > + > + MESON_GATE(CLKID_VCLK2_VENCI0, HHI_GCLK_OTHER, 1), > + MESON_GATE(CLKID_VCLK2_VENCI1, HHI_GCLK_OTHER, 2), > + MESON_GATE(CLKID_VCLK2_VENCP0, HHI_GCLK_OTHER, 3), > + MESON_GATE(CLKID_VCLK2_VENCP1, HHI_GCLK_OTHER, 4), > + MESON_GATE(CLKID_GCLK_VENCI_INT0, HHI_GCLK_OTHER, 8), > + MESON_GATE(CLKID_DAC_CLK, HHI_GCLK_OTHER, 10), > + MESON_GATE(CLKID_AOCLK_GATE, HHI_GCLK_OTHER, 14), > + MESON_GATE(CLKID_IEC958_GATE, HHI_GCLK_OTHER, 16), > + MESON_GATE(CLKID_ENC480P, HHI_GCLK_OTHER, 20), > + MESON_GATE(CLKID_RNG1, HHI_GCLK_OTHER, 21), > + MESON_GATE(CLKID_GCLK_VENCI_INT1, HHI_GCLK_OTHER, 22), > + MESON_GATE(CLKID_VCLK2_VENCLMCC, HHI_GCLK_OTHER, 24), > + MESON_GATE(CLKID_VCLK2_VENCL, HHI_GCLK_OTHER, 25), > + MESON_GATE(CLKID_VCLK_OTHER, HHI_GCLK_OTHER, 26), > + MESON_GATE(CLKID_EDP, HHI_GCLK_OTHER, 31), > + > + /* Always On (AO) domain gates */ > + MESON_GATE(CLKID_AO_MEDIA_CPU, HHI_GCLK_AO, 0), > + MESON_GATE(CLKID_AO_AHB_SRAM, HHI_GCLK_AO, 1), > + MESON_GATE(CLKID_AO_AHB_BUS, HHI_GCLK_AO, 2), > + MESON_GATE(CLKID_AO_IFACE, HHI_GCLK_AO, 3), > + MESON_GATE(CLKID_AO_I2C, HHI_GCLK_AO, 4), > +}; > + > +static int meson_set_gate(struct clk *clk, bool on) > +{ > + struct meson_clk *priv = dev_get_priv(clk->dev); > + struct meson_gate *gate; > + > + if (clk->id >= ARRAY_SIZE(gates)) > + return -ENOENT; > + > + gate = &gates[clk->id]; > + > + if (gate->reg == 0) > + return -ENOENT; > + > + clrsetbits_le32(priv->addr + gate->reg, > + BIT(gate->bit), on ? BIT(gate->bit) : 0); > + return 0; > +} > + > +static int meson_clk_enable(struct clk *clk) > +{ > + return meson_set_gate(clk, true); > +} > + > +static int meson_clk_disable(struct clk *clk) > +{ > + return meson_set_gate(clk, false); > +} > + > +static ulong meson_clk_get_rate(struct clk *clk) > +{ > + struct meson_clk *priv = dev_get_priv(clk->dev); > + > + if (clk->id != CLKID_CLK81) { > + if (clk->id >= ARRAY_SIZE(gates)) > + return -ENOENT; > + if (gates[clk->id].reg == 0) > + return -ENOENT; > + } > + > + /* Use cached value if available */ > + if (priv->rate) > + return priv->rate; > + > + priv->rate = meson_measure_clk_rate(CLK_81); > + > + return priv->rate; > +} > + > +static int meson_clk_probe(struct udevice *dev) > +{ > + struct meson_clk *priv = dev_get_priv(dev); > + > + priv->addr = dev_read_addr_ptr(dev); > + debug("meson-clk: probed at addr %p\n", priv->addr); > + > + return 0; > +} > + > +static struct clk_ops meson_clk_ops = { > + .disable = meson_clk_disable, > + .enable = meson_clk_enable, > + .get_rate = meson_clk_get_rate, > +}; > + > +static const struct udevice_id meson_clk_ids[] = { > + { .compatible = "amlogic,gxbb-clkc" }, > + { .compatible = "amlogic,gxl-clkc" }, > + { } > +}; > + > +U_BOOT_DRIVER(meson_clk) = { > + .name = "meson_clk", > + .id = UCLASS_CLK, > + .of_match = meson_clk_ids, > + .priv_auto_alloc_size = sizeof(struct meson_clk), > + .ops = &meson_clk_ops, > + .probe = meson_clk_probe, > +}; > Ok for now to only handle the gates with the clock measure function. Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Hi Benjamin, On 3 December 2017 at 02:17, Beniamino Galvani <b.galvani@gmail.com> wrote: > Introduce a basic clock driver for Amlogic Meson SoCs which supports > enabling/disabling clock gates and getting their frequency. > > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> > --- > arch/arm/mach-meson/Kconfig | 2 + > drivers/clk/Makefile | 1 + > drivers/clk/clk_meson.c | 196 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 199 insertions(+) > create mode 100644 drivers/clk/clk_meson.c Reviewed-by: Simon Glass <sjg@chromium.org>
Hi Beniamino, On 03/12/2017 10:17, Beniamino Galvani wrote: > Introduce a basic clock driver for Amlogic Meson SoCs which supports > enabling/disabling clock gates and getting their frequency. > > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> > --- > arch/arm/mach-meson/Kconfig | 2 + > drivers/clk/Makefile | 1 + > drivers/clk/clk_meson.c | 196 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 199 insertions(+) > create mode 100644 drivers/clk/clk_meson.c > > diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig > index d4bd230be3..7acee3bc5c 100644 > --- a/arch/arm/mach-meson/Kconfig > +++ b/arch/arm/mach-meson/Kconfig [...] > + > +static int meson_set_gate(struct clk *clk, bool on) > +{ > + struct meson_clk *priv = dev_get_priv(clk->dev); > + struct meson_gate *gate; > + > + if (clk->id >= ARRAY_SIZE(gates)) > + return -ENOENT; This should be -ENOSYS, otherwise it breaks the ethernet driver since it waits -ENOSYS if clock cannot be enabled. > + > + gate = &gates[clk->id]; > + > + if (gate->reg == 0) > + return -ENOENT; Same here -ENOSYS > + > + clrsetbits_le32(priv->addr + gate->reg, > + BIT(gate->bit), on ? BIT(gate->bit) : 0); > + return 0; > +} > + > +static int meson_clk_enable(struct clk *clk) > +{ > + return meson_set_gate(clk, true); > +} > + > +static int meson_clk_disable(struct clk *clk) > +{ > + return meson_set_gate(clk, false); > +} > + > +static ulong meson_clk_get_rate(struct clk *clk) > +{ > + struct meson_clk *priv = dev_get_priv(clk->dev); > + > + if (clk->id != CLKID_CLK81) { > + if (clk->id >= ARRAY_SIZE(gates)) > + return -ENOENT; Same here -ENOSYS > + if (gates[clk->id].reg == 0) > + return -ENOENT; Same here -ENOSYS > + } > + > + /* Use cached value if available */ > + if (priv->rate) > + return priv->rate; > + > + priv->rate = meson_measure_clk_rate(CLK_81); > + > + return priv->rate; > +} > + [...] Neil
Hi Neil, On 29 March 2018 at 16:42, Neil Armstrong <narmstrong@baylibre.com> wrote: > Hi Beniamino, > > On 03/12/2017 10:17, Beniamino Galvani wrote: >> Introduce a basic clock driver for Amlogic Meson SoCs which supports >> enabling/disabling clock gates and getting their frequency. >> >> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> >> --- >> arch/arm/mach-meson/Kconfig | 2 + >> drivers/clk/Makefile | 1 + >> drivers/clk/clk_meson.c | 196 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 199 insertions(+) >> create mode 100644 drivers/clk/clk_meson.c >> >> diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig >> index d4bd230be3..7acee3bc5c 100644 >> --- a/arch/arm/mach-meson/Kconfig >> +++ b/arch/arm/mach-meson/Kconfig > > [...] >> + >> +static int meson_set_gate(struct clk *clk, bool on) >> +{ >> + struct meson_clk *priv = dev_get_priv(clk->dev); >> + struct meson_gate *gate; >> + >> + if (clk->id >= ARRAY_SIZE(gates)) >> + return -ENOENT; > > This should be -ENOSYS, otherwise it breaks the ethernet driver since it waits -ENOSYS if clock cannot be enabled. Perhaps, but this is a genuine error, so it is OK to break the Ethernet driver, isn't it? We don't want errors to be silently ignored. Not having a device is one thing, but having a device that does not work is bad. Also I have tried to keep -ENOSYS for cases where the driver does not support the operation. We should be very clear in clk-uclass.h as to what errors are returned. At present I don't see ENOSYS mentioned. At the very least we should update the docs if certain behaviour is expected. I would also expect us to have a sandbox test for it. > >> + >> + gate = &gates[clk->id]; >> + >> + if (gate->reg == 0) >> + return -ENOENT; > > Same here -ENOSYS > >> + >> + clrsetbits_le32(priv->addr + gate->reg, >> + BIT(gate->bit), on ? BIT(gate->bit) : 0); >> + return 0; >> +} >> + >> +static int meson_clk_enable(struct clk *clk) >> +{ >> + return meson_set_gate(clk, true); >> +} >> + >> +static int meson_clk_disable(struct clk *clk) >> +{ >> + return meson_set_gate(clk, false); >> +} >> + >> +static ulong meson_clk_get_rate(struct clk *clk) >> +{ >> + struct meson_clk *priv = dev_get_priv(clk->dev); >> + >> + if (clk->id != CLKID_CLK81) { >> + if (clk->id >= ARRAY_SIZE(gates)) >> + return -ENOENT; > > Same here -ENOSYS > >> + if (gates[clk->id].reg == 0) >> + return -ENOENT; > > Same here -ENOSYS > >> + } >> + >> + /* Use cached value if available */ >> + if (priv->rate) >> + return priv->rate; >> + >> + priv->rate = meson_measure_clk_rate(CLK_81); >> + >> + return priv->rate; >> +} >> + > > [...] > > Neil Regards, Simon
On 30/03/2018 00:41, Simon Glass wrote: > Hi Neil, > > On 29 March 2018 at 16:42, Neil Armstrong <narmstrong@baylibre.com> wrote: >> Hi Beniamino, >> >> On 03/12/2017 10:17, Beniamino Galvani wrote: >>> Introduce a basic clock driver for Amlogic Meson SoCs which supports >>> enabling/disabling clock gates and getting their frequency. >>> >>> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> >>> --- >>> arch/arm/mach-meson/Kconfig | 2 + >>> drivers/clk/Makefile | 1 + >>> drivers/clk/clk_meson.c | 196 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 199 insertions(+) >>> create mode 100644 drivers/clk/clk_meson.c >>> >>> diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig >>> index d4bd230be3..7acee3bc5c 100644 >>> --- a/arch/arm/mach-meson/Kconfig >>> +++ b/arch/arm/mach-meson/Kconfig >> >> [...] >>> + >>> +static int meson_set_gate(struct clk *clk, bool on) >>> +{ >>> + struct meson_clk *priv = dev_get_priv(clk->dev); >>> + struct meson_gate *gate; >>> + >>> + if (clk->id >= ARRAY_SIZE(gates)) >>> + return -ENOENT; >> >> This should be -ENOSYS, otherwise it breaks the ethernet driver since it waits -ENOSYS if clock cannot be enabled. > > Perhaps, but this is a genuine error, so it is OK to break the > Ethernet driver, isn't it? We don't want errors to be silently > ignored. > > Not having a device is one thing, but having a device that does not work is bad. > The driver only manages the gates, enabling and disabling them and getting their freq. The missing clocks of the ethernet driver in this case are dividers of the PLL, thus we don't need to enable them, and for now the driver don't need the freq. > Also I have tried to keep -ENOSYS for cases where the driver does not > support the operation. We should be very clear in clk-uclass.h as to > what errors are returned. At present I don't see ENOSYS mentioned. At > the very least we should update the docs if certain behaviour is > expected. I would also expect us to have a sandbox test for it. In this case, the driver does not support the operation for these clocks, but maybe it would be better to add them with reg=0 and return -ENOSYS in the following return : > >> >>> + >>> + gate = &gates[clk->id]; >>> + >>> + if (gate->reg == 0) >>> + return -ENOENT; >> >> Same here -ENOSYS Here thsi means it's not a gate. >> >>> + >>> + clrsetbits_le32(priv->addr + gate->reg, >>> + BIT(gate->bit), on ? BIT(gate->bit) : 0); >>> + return 0; >>> +} >>> + >>> +static int meson_clk_enable(struct clk *clk) >>> +{ >>> + return meson_set_gate(clk, true); >>> +} >>> + >>> +static int meson_clk_disable(struct clk *clk) >>> +{ >>> + return meson_set_gate(clk, false); >>> +} >>> + >>> +static ulong meson_clk_get_rate(struct clk *clk) >>> +{ >>> + struct meson_clk *priv = dev_get_priv(clk->dev); >>> + >>> + if (clk->id != CLKID_CLK81) { >>> + if (clk->id >= ARRAY_SIZE(gates)) >>> + return -ENOENT; >> >> Same here -ENOSYS >> >>> + if (gates[clk->id].reg == 0) >>> + return -ENOENT; >> >> Same here -ENOSYS >> >>> + } >>> + >>> + /* Use cached value if available */ >>> + if (priv->rate) >>> + return priv->rate; >>> + >>> + priv->rate = meson_measure_clk_rate(CLK_81); >>> + >>> + return priv->rate; >>> +} >>> + >> >> [...] >> >> Neil > > Regards, > Simon >
Hi Neil, On 30 March 2018 at 15:53, Neil Armstrong <narmstrong@baylibre.com> wrote: > On 30/03/2018 00:41, Simon Glass wrote: >> Hi Neil, >> >> On 29 March 2018 at 16:42, Neil Armstrong <narmstrong@baylibre.com> wrote: >>> Hi Beniamino, >>> >>> On 03/12/2017 10:17, Beniamino Galvani wrote: >>>> Introduce a basic clock driver for Amlogic Meson SoCs which supports >>>> enabling/disabling clock gates and getting their frequency. >>>> >>>> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> >>>> --- >>>> arch/arm/mach-meson/Kconfig | 2 + >>>> drivers/clk/Makefile | 1 + >>>> drivers/clk/clk_meson.c | 196 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 199 insertions(+) >>>> create mode 100644 drivers/clk/clk_meson.c >>>> >>>> diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig >>>> index d4bd230be3..7acee3bc5c 100644 >>>> --- a/arch/arm/mach-meson/Kconfig >>>> +++ b/arch/arm/mach-meson/Kconfig >>> >>> [...] >>>> + >>>> +static int meson_set_gate(struct clk *clk, bool on) >>>> +{ >>>> + struct meson_clk *priv = dev_get_priv(clk->dev); >>>> + struct meson_gate *gate; >>>> + >>>> + if (clk->id >= ARRAY_SIZE(gates)) >>>> + return -ENOENT; >>> >>> This should be -ENOSYS, otherwise it breaks the ethernet driver since it waits -ENOSYS if clock cannot be enabled. >> >> Perhaps, but this is a genuine error, so it is OK to break the >> Ethernet driver, isn't it? We don't want errors to be silently >> ignored. >> >> Not having a device is one thing, but having a device that does not work is bad. >> > > The driver only manages the gates, enabling and disabling them and getting their freq. > The missing clocks of the ethernet driver in this case are dividers of the PLL, thus > we don't need to enable them, and for now the driver don't need the freq. Yes but -ENOSYS means that the driver does not support the operation at all. Put it another way: you can't return -ENOSYS for some parameters and not for others. > >> Also I have tried to keep -ENOSYS for cases where the driver does not >> support the operation. We should be very clear in clk-uclass.h as to >> what errors are returned. At present I don't see ENOSYS mentioned. At >> the very least we should update the docs if certain behaviour is >> expected. I would also expect us to have a sandbox test for it. > > In this case, the driver does not support the operation for these clocks, but maybe it would be > better to add them with reg=0 and return -ENOSYS in the following return : I don't like that - ENOENT seems better. > >> >>> >>>> + >>>> + gate = &gates[clk->id]; >>>> + >>>> + if (gate->reg == 0) >>>> + return -ENOENT; >>> >>> Same here -ENOSYS > > Here thsi means it's not a gate. Yes, but the driver still supports the operation. The problem is that its inputs are invalid. So use -ENOENT to mean that. > >>> >>>> + >>>> + clrsetbits_le32(priv->addr + gate->reg, >>>> + BIT(gate->bit), on ? BIT(gate->bit) : 0); >>>> + return 0; >>>> +} >>>> + >>>> +static int meson_clk_enable(struct clk *clk) >>>> +{ >>>> + return meson_set_gate(clk, true); >>>> +} >>>> + >>>> +static int meson_clk_disable(struct clk *clk) >>>> +{ >>>> + return meson_set_gate(clk, false); >>>> +} >>>> + >>>> +static ulong meson_clk_get_rate(struct clk *clk) >>>> +{ >>>> + struct meson_clk *priv = dev_get_priv(clk->dev); >>>> + >>>> + if (clk->id != CLKID_CLK81) { >>>> + if (clk->id >= ARRAY_SIZE(gates)) >>>> + return -ENOENT; >>> >>> Same here -ENOSYS >>> >>>> + if (gates[clk->id].reg == 0) >>>> + return -ENOENT; >>> >>> Same here -ENOSYS >>> >>>> + } >>>> + >>>> + /* Use cached value if available */ >>>> + if (priv->rate) >>>> + return priv->rate; >>>> + >>>> + priv->rate = meson_measure_clk_rate(CLK_81); >>>> + >>>> + return priv->rate; >>>> +} >>>> + >>> >>> [...] >>> >>> Neil >> >> Regards, >> Simon >> > Regards, Simon
Hi guys, Am 30.03.2018 um 10:41 schrieb Simon Glass: > On 30 March 2018 at 15:53, Neil Armstrong <narmstrong@baylibre.com> wrote: >> On 30/03/2018 00:41, Simon Glass wrote: >>> On 29 March 2018 at 16:42, Neil Armstrong <narmstrong@baylibre.com> wrote: >>>> On 03/12/2017 10:17, Beniamino Galvani wrote: >>>>> + >>>>> + gate = &gates[clk->id]; >>>>> + >>>>> + if (gate->reg == 0) >>>>> + return -ENOENT; >>>> >>>> Same here -ENOSYS >> >> Here thsi means it's not a gate. > > Yes, but the driver still supports the operation. The problem is that > its inputs are invalid. So use -ENOENT to mean that. Isn't that the definition of -EINVAL? Cheers, Andreas
Hi Andreas, On 30 March 2018 at 22:27, Andreas Färber <afaerber@suse.de> wrote: > Hi guys, > > Am 30.03.2018 um 10:41 schrieb Simon Glass: >> On 30 March 2018 at 15:53, Neil Armstrong <narmstrong@baylibre.com> wrote: >>> On 30/03/2018 00:41, Simon Glass wrote: >>>> On 29 March 2018 at 16:42, Neil Armstrong <narmstrong@baylibre.com> wrote: >>>>> On 03/12/2017 10:17, Beniamino Galvani wrote: >>>>>> + >>>>>> + gate = &gates[clk->id]; >>>>>> + >>>>>> + if (gate->reg == 0) >>>>>> + return -ENOENT; >>>>> >>>>> Same here -ENOSYS >>> >>> Here thsi means it's not a gate. >> >> Yes, but the driver still supports the operation. The problem is that >> its inputs are invalid. So use -ENOENT to mean that. > > Isn't that the definition of -EINVAL? We tend to use that to indicate a failure to read the DT config. I should probably not have said 'invalid'. The input value is reasonable (it isn't -ve, for example) but the selected item does not exist. Regards, Simon
diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig index d4bd230be3..7acee3bc5c 100644 --- a/arch/arm/mach-meson/Kconfig +++ b/arch/arm/mach-meson/Kconfig @@ -3,6 +3,7 @@ if ARCH_MESON config MESON_GXBB bool "Support Meson GXBaby" select ARM64 + select CLK select DM select DM_SERIAL help @@ -12,6 +13,7 @@ config MESON_GXBB config MESON_GXL bool "Support Meson GXL" select ARM64 + select CLK select DM select DM_SERIAL help diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index bcc8f82fb6..67da27873d 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_$(SPL_TPL_)CLK) += clk-uclass.o clk_fixed_rate.o obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip/ +obj-$(CONFIG_ARCH_MESON) += clk_meson.o obj-$(CONFIG_SANDBOX) += clk_sandbox.o obj-$(CONFIG_SANDBOX) += clk_sandbox_test.o obj-$(CONFIG_MACH_PIC32) += clk_pic32.o diff --git a/drivers/clk/clk_meson.c b/drivers/clk/clk_meson.c new file mode 100644 index 0000000000..3cf9372e05 --- /dev/null +++ b/drivers/clk/clk_meson.c @@ -0,0 +1,196 @@ +/* + * (C) Copyright 2017 - Beniamino Galvani <b.galvani@gmail.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <asm/arch/clock.h> +#include <asm/io.h> +#include <clk-uclass.h> +#include <dm.h> +#include <dt-bindings/clock/gxbb-clkc.h> + +struct meson_clk { + void __iomem *addr; + ulong rate; +}; + +struct meson_gate { + unsigned int reg; + unsigned int bit; +}; + +#define MESON_GATE(id, _reg, _bit) \ + [id] = { \ + .reg = (_reg), \ + .bit = (_bit), \ + } + +struct meson_gate gates[] = { + /* Everything Else (EE) domain gates */ + MESON_GATE(CLKID_DDR, HHI_GCLK_MPEG0, 0), + MESON_GATE(CLKID_DOS, HHI_GCLK_MPEG0, 1), + MESON_GATE(CLKID_ISA, HHI_GCLK_MPEG0, 5), + MESON_GATE(CLKID_PL301, HHI_GCLK_MPEG0, 6), + MESON_GATE(CLKID_PERIPHS, HHI_GCLK_MPEG0, 7), + MESON_GATE(CLKID_SPICC, HHI_GCLK_MPEG0, 8), + MESON_GATE(CLKID_I2C, HHI_GCLK_MPEG0, 9), + MESON_GATE(CLKID_SAR_ADC, HHI_GCLK_MPEG0, 10), + MESON_GATE(CLKID_SMART_CARD, HHI_GCLK_MPEG0, 11), + MESON_GATE(CLKID_RNG0, HHI_GCLK_MPEG0, 12), + MESON_GATE(CLKID_UART0, HHI_GCLK_MPEG0, 13), + MESON_GATE(CLKID_SDHC, HHI_GCLK_MPEG0, 14), + MESON_GATE(CLKID_STREAM, HHI_GCLK_MPEG0, 15), + MESON_GATE(CLKID_ASYNC_FIFO, HHI_GCLK_MPEG0, 16), + MESON_GATE(CLKID_SDIO, HHI_GCLK_MPEG0, 17), + MESON_GATE(CLKID_ABUF, HHI_GCLK_MPEG0, 18), + MESON_GATE(CLKID_HIU_IFACE, HHI_GCLK_MPEG0, 19), + MESON_GATE(CLKID_ASSIST_MISC, HHI_GCLK_MPEG0, 23), + MESON_GATE(CLKID_SD_EMMC_A, HHI_GCLK_MPEG0, 24), + MESON_GATE(CLKID_SD_EMMC_B, HHI_GCLK_MPEG0, 25), + MESON_GATE(CLKID_SD_EMMC_C, HHI_GCLK_MPEG0, 26), + MESON_GATE(CLKID_SPI, HHI_GCLK_MPEG0, 30), + + MESON_GATE(CLKID_I2S_SPDIF, HHI_GCLK_MPEG1, 2), + MESON_GATE(CLKID_ETH, HHI_GCLK_MPEG1, 3), + MESON_GATE(CLKID_DEMUX, HHI_GCLK_MPEG1, 4), + MESON_GATE(CLKID_AIU_GLUE, HHI_GCLK_MPEG1, 6), + MESON_GATE(CLKID_IEC958, HHI_GCLK_MPEG1, 7), + MESON_GATE(CLKID_I2S_OUT, HHI_GCLK_MPEG1, 8), + MESON_GATE(CLKID_AMCLK, HHI_GCLK_MPEG1, 9), + MESON_GATE(CLKID_AIFIFO2, HHI_GCLK_MPEG1, 10), + MESON_GATE(CLKID_MIXER, HHI_GCLK_MPEG1, 11), + MESON_GATE(CLKID_MIXER_IFACE, HHI_GCLK_MPEG1, 12), + MESON_GATE(CLKID_ADC, HHI_GCLK_MPEG1, 13), + MESON_GATE(CLKID_BLKMV, HHI_GCLK_MPEG1, 14), + MESON_GATE(CLKID_AIU, HHI_GCLK_MPEG1, 15), + MESON_GATE(CLKID_UART1, HHI_GCLK_MPEG1, 16), + MESON_GATE(CLKID_G2D, HHI_GCLK_MPEG1, 20), + MESON_GATE(CLKID_USB0, HHI_GCLK_MPEG1, 21), + MESON_GATE(CLKID_USB1, HHI_GCLK_MPEG1, 22), + MESON_GATE(CLKID_RESET, HHI_GCLK_MPEG1, 23), + MESON_GATE(CLKID_NAND, HHI_GCLK_MPEG1, 24), + MESON_GATE(CLKID_DOS_PARSER, HHI_GCLK_MPEG1, 25), + MESON_GATE(CLKID_USB, HHI_GCLK_MPEG1, 26), + MESON_GATE(CLKID_VDIN1, HHI_GCLK_MPEG1, 28), + MESON_GATE(CLKID_AHB_ARB0, HHI_GCLK_MPEG1, 29), + MESON_GATE(CLKID_EFUSE, HHI_GCLK_MPEG1, 30), + MESON_GATE(CLKID_BOOT_ROM, HHI_GCLK_MPEG1, 31), + + MESON_GATE(CLKID_AHB_DATA_BUS, HHI_GCLK_MPEG2, 1), + MESON_GATE(CLKID_AHB_CTRL_BUS, HHI_GCLK_MPEG2, 2), + MESON_GATE(CLKID_HDMI_INTR_SYNC, HHI_GCLK_MPEG2, 3), + MESON_GATE(CLKID_HDMI_PCLK, HHI_GCLK_MPEG2, 4), + MESON_GATE(CLKID_USB1_DDR_BRIDGE, HHI_GCLK_MPEG2, 8), + MESON_GATE(CLKID_USB0_DDR_BRIDGE, HHI_GCLK_MPEG2, 9), + MESON_GATE(CLKID_MMC_PCLK, HHI_GCLK_MPEG2, 11), + MESON_GATE(CLKID_DVIN, HHI_GCLK_MPEG2, 12), + MESON_GATE(CLKID_UART2, HHI_GCLK_MPEG2, 15), + MESON_GATE(CLKID_SANA, HHI_GCLK_MPEG2, 22), + MESON_GATE(CLKID_VPU_INTR, HHI_GCLK_MPEG2, 25), + MESON_GATE(CLKID_SEC_AHB_AHB3_BRIDGE, HHI_GCLK_MPEG2, 26), + MESON_GATE(CLKID_CLK81_A53, HHI_GCLK_MPEG2, 29), + + MESON_GATE(CLKID_VCLK2_VENCI0, HHI_GCLK_OTHER, 1), + MESON_GATE(CLKID_VCLK2_VENCI1, HHI_GCLK_OTHER, 2), + MESON_GATE(CLKID_VCLK2_VENCP0, HHI_GCLK_OTHER, 3), + MESON_GATE(CLKID_VCLK2_VENCP1, HHI_GCLK_OTHER, 4), + MESON_GATE(CLKID_GCLK_VENCI_INT0, HHI_GCLK_OTHER, 8), + MESON_GATE(CLKID_DAC_CLK, HHI_GCLK_OTHER, 10), + MESON_GATE(CLKID_AOCLK_GATE, HHI_GCLK_OTHER, 14), + MESON_GATE(CLKID_IEC958_GATE, HHI_GCLK_OTHER, 16), + MESON_GATE(CLKID_ENC480P, HHI_GCLK_OTHER, 20), + MESON_GATE(CLKID_RNG1, HHI_GCLK_OTHER, 21), + MESON_GATE(CLKID_GCLK_VENCI_INT1, HHI_GCLK_OTHER, 22), + MESON_GATE(CLKID_VCLK2_VENCLMCC, HHI_GCLK_OTHER, 24), + MESON_GATE(CLKID_VCLK2_VENCL, HHI_GCLK_OTHER, 25), + MESON_GATE(CLKID_VCLK_OTHER, HHI_GCLK_OTHER, 26), + MESON_GATE(CLKID_EDP, HHI_GCLK_OTHER, 31), + + /* Always On (AO) domain gates */ + MESON_GATE(CLKID_AO_MEDIA_CPU, HHI_GCLK_AO, 0), + MESON_GATE(CLKID_AO_AHB_SRAM, HHI_GCLK_AO, 1), + MESON_GATE(CLKID_AO_AHB_BUS, HHI_GCLK_AO, 2), + MESON_GATE(CLKID_AO_IFACE, HHI_GCLK_AO, 3), + MESON_GATE(CLKID_AO_I2C, HHI_GCLK_AO, 4), +}; + +static int meson_set_gate(struct clk *clk, bool on) +{ + struct meson_clk *priv = dev_get_priv(clk->dev); + struct meson_gate *gate; + + if (clk->id >= ARRAY_SIZE(gates)) + return -ENOENT; + + gate = &gates[clk->id]; + + if (gate->reg == 0) + return -ENOENT; + + clrsetbits_le32(priv->addr + gate->reg, + BIT(gate->bit), on ? BIT(gate->bit) : 0); + return 0; +} + +static int meson_clk_enable(struct clk *clk) +{ + return meson_set_gate(clk, true); +} + +static int meson_clk_disable(struct clk *clk) +{ + return meson_set_gate(clk, false); +} + +static ulong meson_clk_get_rate(struct clk *clk) +{ + struct meson_clk *priv = dev_get_priv(clk->dev); + + if (clk->id != CLKID_CLK81) { + if (clk->id >= ARRAY_SIZE(gates)) + return -ENOENT; + if (gates[clk->id].reg == 0) + return -ENOENT; + } + + /* Use cached value if available */ + if (priv->rate) + return priv->rate; + + priv->rate = meson_measure_clk_rate(CLK_81); + + return priv->rate; +} + +static int meson_clk_probe(struct udevice *dev) +{ + struct meson_clk *priv = dev_get_priv(dev); + + priv->addr = dev_read_addr_ptr(dev); + debug("meson-clk: probed at addr %p\n", priv->addr); + + return 0; +} + +static struct clk_ops meson_clk_ops = { + .disable = meson_clk_disable, + .enable = meson_clk_enable, + .get_rate = meson_clk_get_rate, +}; + +static const struct udevice_id meson_clk_ids[] = { + { .compatible = "amlogic,gxbb-clkc" }, + { .compatible = "amlogic,gxl-clkc" }, + { } +}; + +U_BOOT_DRIVER(meson_clk) = { + .name = "meson_clk", + .id = UCLASS_CLK, + .of_match = meson_clk_ids, + .priv_auto_alloc_size = sizeof(struct meson_clk), + .ops = &meson_clk_ops, + .probe = meson_clk_probe, +};
Introduce a basic clock driver for Amlogic Meson SoCs which supports enabling/disabling clock gates and getting their frequency. Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> --- arch/arm/mach-meson/Kconfig | 2 + drivers/clk/Makefile | 1 + drivers/clk/clk_meson.c | 196 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 199 insertions(+) create mode 100644 drivers/clk/clk_meson.c