Re: [PATCH v4 3/6] clk: ralink: add clock driver for mt7621 SoC
Quoting Sergio Paracuellos (2020-12-17 01:54:18)
>
> On Thu, Dec 17, 2020 at 10:09 AM Stephen Boyd wrote:
> >
> > Quoting Sergio Paracuellos (2020-11-22 01:55:53)
> > > diff --git a/drivers/clk/ralink/Makefile b/drivers/clk/ralink/Makefile
> > > new file mode 100644
> > > index ..cf6f9216379d
> > > --- /dev/null
> > > +++ b/drivers/clk/ralink/Makefile
> > > @@ -0,0 +1,2 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +obj-$(CONFIG_CLK_MT7621) += clk-mt7621.o
> > > diff --git a/drivers/clk/ralink/clk-mt7621.c
> > > b/drivers/clk/ralink/clk-mt7621.c
> > > new file mode 100644
> > > index ..4e929f13fe7c
> > > --- /dev/null
> > > +++ b/drivers/clk/ralink/clk-mt7621.c
> > > @@ -0,0 +1,435 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Mediatek MT7621 Clock Driver
> > > + * Author: Sergio Paracuellos
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> >
> > Is it possible to drop this include? Doing so would make this portable
> > and compilable on more architectures so us cross compilers can check
> > build stuff and make changes easily.
>
> No, this is not possible. This old arch makes some global functions
> there to properly access different registers in the palmbus. It is not
> also well documented so it is really difficult to make something
> better with this.
> This is needed to use 'rt_memc_r32'
> (arch/mips/include/asm/mach-ralink/ralink_regs.h) for reading
> MEMC_REG_CPU_PLL.
>
> This is a not documented register and is not in the syscon related
> part and we need it to derive the clock frequency for the XTAL clock.
Ok.
> > > +static int mt7621_gate_ops_init(struct device_node *np,
> > > +struct mt7621_gate *sclk)
> > > +{
> > > + struct clk_init_data init = {
> > > + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> >
> > Why ignore unused? Are they CLK_IS_CRITICAL? Can they be enabled at
> > driver probe instead of here? Or left out of the kernel entirely if they
> > shouldn't be turned off?
>
> Because all the platform drivers are not changed to use this gates yet
> and all gates are enabled by default (related registers are set to all
> ones), kernel disables all the stuff because they are not being
> referenced, but yes, you are right, I think I can call
> clk_prepare_enable for all of them at init time and avoid this
> 'CLK_IGNORE_UNUSED' flag to don't break anything of the current other
> upstream code.
Does something crash if they're turned off? We have CLK_IS_CRITICAL for
that. The CLK_IGNORE_UNUSED flag is sort of deprecated now.
> > > +
> > > +#define CLK_BASE(_name, _parent, _recalc) {\
> > > + .init = &(struct clk_init_data) { \
> > > + .name = _name, \
> > > + .ops = &(const struct clk_ops) {\
> > > + .recalc_rate = _recalc, \
> > > + }, \
> > > + .parent_names = (const char *const[]) { _parent }, \
> >
> > Please use clk_parent_data instead
>
> parent can also be NULL here and num_parents zero, but I will search
> what do you really mean with this 'clk_parent_data' :).
Heh, 'git grep clk_parent_data -- drivers/clk/' should give some clues.
> > > +free_clk_prov:
> > > + kfree(clk_prov);
> > > +}
> > > +
> > > +CLK_OF_DECLARE(mt7621_clk, "mediatek,mt7621-clk", mt7621_clk_init);
> >
> > Any reason to use this vs. a platform driver?
>
> We need clocks available in 'plat_time_init' before setting up the
> timer for the GIC, so to maintain all the clock driver in a simple
> file and using only one device tree node and no separate the gates
> into another platform driver, I think this is the only way to go, but
> please correct me if I am wrong.
We can register the few clks like that early with
CLK_OF_DECLARE_DRIVER() and then have a platform driver register the
rest of the clks that aren't required early. This lets us hook into the
driver framework better while still getting those few clks to turn on
early enough for the timers.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/6] clk: ralink: add clock driver for mt7621 SoC
Hi Stephen, Thanks for the review. On Thu, Dec 17, 2020 at 10:09 AM Stephen Boyd wrote: > > Quoting Sergio Paracuellos (2020-11-22 01:55:53) > > The documentation for this SOC only talks about two > > registers regarding to the clocks: > > * SYSC_REG_CPLL_CLKCFG0 - provides some information about > > boostrapped refclock. PLL and dividers used for CPU and some > > sort of BUS. > > * SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable > > clocks for all or some ip cores. > > > > Looking into driver code, and some openWRT patched there are > > another frequences which are used in some drivers (uart, sd...). > > s/frequences/frequencies/ Ok! > > > According to all of this information the clock plan for this > > SoC is set as follows: > > - Main top clock "xtal" from where all the rest of the world is > > derived. > > - CPU clock "cpu" derived from "xtal" frequencies and a bunch of > > register reads and predividers. > > - BUS clock "bus" derived from "cpu" and with (cpu / 4) MHz. > > - Fixed clocks from "xtal": > > * "50m": 50 MHz. > > * "125m": 125 MHz. > > * "150m": 150 MHz. > > * "250m": 250 MHz. > > * "270m": 270 MHz. > > > > We also have a buch of gate clocks with their parents: > > * "hsdma": "150m" > > * "fe": "250m" > > * "sp_divtx": "270m" > > * "timer": "50m" > > * "pcm": "270m" > > * "pio": "50m" > > * "gdma": "bus" > > * "nand": "125m" > > * "i2c": "50m" > > * "i2s": "270m" > > * "spi": "bus" > > * "uart1": "50m" > > * "uart2": "50m" > > * "uart3": "50m" > > * "eth": "50m" > > * "pcie0": "125m" > > * "pcie1": "125m" > > * "pcie2": "125m" > > * "crypto": "250m" > > * "shxc": "50m" > > > > With this information the clk driver will provide clock and gates > > functionality from a a set of hardcoded clocks allowing to define > > a nice device tree without fixed clocks. > > > > Signed-off-by: Sergio Paracuellos > > --- > > drivers/clk/Kconfig | 1 + > > drivers/clk/Makefile| 1 + > > drivers/clk/ralink/Kconfig | 14 + > > drivers/clk/ralink/Makefile | 2 + > > drivers/clk/ralink/clk-mt7621.c | 435 > > 5 files changed, 453 insertions(+) > > create mode 100644 drivers/clk/ralink/Kconfig > > create mode 100644 drivers/clk/ralink/Makefile > > create mode 100644 drivers/clk/ralink/clk-mt7621.c > > > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > > index c715d4681a0b..5f94c4329033 100644 > > --- a/drivers/clk/Kconfig > > +++ b/drivers/clk/Kconfig > > @@ -372,6 +372,7 @@ source "drivers/clk/mediatek/Kconfig" > > source "drivers/clk/meson/Kconfig" > > source "drivers/clk/mvebu/Kconfig" > > source "drivers/clk/qcom/Kconfig" > > +source "drivers/clk/ralink/Kconfig" > > source "drivers/clk/renesas/Kconfig" > > source "drivers/clk/rockchip/Kconfig" > > source "drivers/clk/samsung/Kconfig" > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > index da8fcf147eb1..6578e167b047 100644 > > --- a/drivers/clk/Makefile > > +++ b/drivers/clk/Makefile > > @@ -100,6 +100,7 @@ obj-$(CONFIG_COMMON_CLK_NXP)+= nxp/ > > obj-$(CONFIG_MACH_PISTACHIO) += pistachio/ > > obj-$(CONFIG_COMMON_CLK_PXA) += pxa/ > > obj-$(CONFIG_COMMON_CLK_QCOM) += qcom/ > > +obj-y += ralink/ > > obj-y += renesas/ > > obj-$(CONFIG_ARCH_ROCKCHIP)+= rockchip/ > > obj-$(CONFIG_COMMON_CLK_SAMSUNG) += samsung/ > > Thanks for keeping it sorted! It was so clean sorted so I just followed the style there. > > > diff --git a/drivers/clk/ralink/Kconfig b/drivers/clk/ralink/Kconfig > > new file mode 100644 > > index ..7e8697327e0c > > --- /dev/null > > +++ b/drivers/clk/ralink/Kconfig > > @@ -0,0 +1,14 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +# > > +# MediaTek Mt7621 Clock Driver > > +# > > +menu "Clock driver for mediatek mt7621 SoC" > > Capitalize MediaTek? Will do. > > > + depends on SOC_MT7621 || COMPILE_TEST > > + > > +config CLK_MT7621 > > + bool "Clock driver for MediaTek MT7621" > > Like it is done here. Yes, both should be properly capitalized. > > > + depends on SOC_MT7621 || COMPILE_TEST > > + default SOC_MT7621 > > + help > > + This driver supports MediaTek MT7621 basic clocks. > > +endmenu > > diff --git a/drivers/clk/ralink/Makefile b/drivers/clk/ralink/Makefile > > new file mode 100644 > > index ..cf6f9216379d > > --- /dev/null > > +++ b/drivers/clk/ralink/Makefile > > @@ -0,0 +1,2 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +obj-$(CONFIG_CLK_MT7621) += clk-mt7621.o > > diff --git a/drivers/clk/ralink/clk-mt7621.c > > b/drivers/clk/ralink/clk-mt7621.c > > new file mode 100644 > > index ..4e929f13fe7c > > --- /dev/null > > +++ b/drivers/clk/ralink/clk-mt7621.c > > @@ -0,0 +1,435 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Mediatek MT76
Re: [PATCH v4 3/6] clk: ralink: add clock driver for mt7621 SoC
Quoting Sergio Paracuellos (2020-11-22 01:55:53) > The documentation for this SOC only talks about two > registers regarding to the clocks: > * SYSC_REG_CPLL_CLKCFG0 - provides some information about > boostrapped refclock. PLL and dividers used for CPU and some > sort of BUS. > * SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable > clocks for all or some ip cores. > > Looking into driver code, and some openWRT patched there are > another frequences which are used in some drivers (uart, sd...). s/frequences/frequencies/ > According to all of this information the clock plan for this > SoC is set as follows: > - Main top clock "xtal" from where all the rest of the world is > derived. > - CPU clock "cpu" derived from "xtal" frequencies and a bunch of > register reads and predividers. > - BUS clock "bus" derived from "cpu" and with (cpu / 4) MHz. > - Fixed clocks from "xtal": > * "50m": 50 MHz. > * "125m": 125 MHz. > * "150m": 150 MHz. > * "250m": 250 MHz. > * "270m": 270 MHz. > > We also have a buch of gate clocks with their parents: > * "hsdma": "150m" > * "fe": "250m" > * "sp_divtx": "270m" > * "timer": "50m" > * "pcm": "270m" > * "pio": "50m" > * "gdma": "bus" > * "nand": "125m" > * "i2c": "50m" > * "i2s": "270m" > * "spi": "bus" > * "uart1": "50m" > * "uart2": "50m" > * "uart3": "50m" > * "eth": "50m" > * "pcie0": "125m" > * "pcie1": "125m" > * "pcie2": "125m" > * "crypto": "250m" > * "shxc": "50m" > > With this information the clk driver will provide clock and gates > functionality from a a set of hardcoded clocks allowing to define > a nice device tree without fixed clocks. > > Signed-off-by: Sergio Paracuellos > --- > drivers/clk/Kconfig | 1 + > drivers/clk/Makefile| 1 + > drivers/clk/ralink/Kconfig | 14 + > drivers/clk/ralink/Makefile | 2 + > drivers/clk/ralink/clk-mt7621.c | 435 > 5 files changed, 453 insertions(+) > create mode 100644 drivers/clk/ralink/Kconfig > create mode 100644 drivers/clk/ralink/Makefile > create mode 100644 drivers/clk/ralink/clk-mt7621.c > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index c715d4681a0b..5f94c4329033 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -372,6 +372,7 @@ source "drivers/clk/mediatek/Kconfig" > source "drivers/clk/meson/Kconfig" > source "drivers/clk/mvebu/Kconfig" > source "drivers/clk/qcom/Kconfig" > +source "drivers/clk/ralink/Kconfig" > source "drivers/clk/renesas/Kconfig" > source "drivers/clk/rockchip/Kconfig" > source "drivers/clk/samsung/Kconfig" > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index da8fcf147eb1..6578e167b047 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -100,6 +100,7 @@ obj-$(CONFIG_COMMON_CLK_NXP)+= nxp/ > obj-$(CONFIG_MACH_PISTACHIO) += pistachio/ > obj-$(CONFIG_COMMON_CLK_PXA) += pxa/ > obj-$(CONFIG_COMMON_CLK_QCOM) += qcom/ > +obj-y += ralink/ > obj-y += renesas/ > obj-$(CONFIG_ARCH_ROCKCHIP)+= rockchip/ > obj-$(CONFIG_COMMON_CLK_SAMSUNG) += samsung/ Thanks for keeping it sorted! > diff --git a/drivers/clk/ralink/Kconfig b/drivers/clk/ralink/Kconfig > new file mode 100644 > index ..7e8697327e0c > --- /dev/null > +++ b/drivers/clk/ralink/Kconfig > @@ -0,0 +1,14 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# > +# MediaTek Mt7621 Clock Driver > +# > +menu "Clock driver for mediatek mt7621 SoC" Capitalize MediaTek? > + depends on SOC_MT7621 || COMPILE_TEST > + > +config CLK_MT7621 > + bool "Clock driver for MediaTek MT7621" Like it is done here. > + depends on SOC_MT7621 || COMPILE_TEST > + default SOC_MT7621 > + help > + This driver supports MediaTek MT7621 basic clocks. > +endmenu > diff --git a/drivers/clk/ralink/Makefile b/drivers/clk/ralink/Makefile > new file mode 100644 > index ..cf6f9216379d > --- /dev/null > +++ b/drivers/clk/ralink/Makefile > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_CLK_MT7621) += clk-mt7621.o > diff --git a/drivers/clk/ralink/clk-mt7621.c b/drivers/clk/ralink/clk-mt7621.c > new file mode 100644 > index ..4e929f13fe7c > --- /dev/null > +++ b/drivers/clk/ralink/clk-mt7621.c > @@ -0,0 +1,435 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Mediatek MT7621 Clock Driver > + * Author: Sergio Paracuellos > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include Is it possible to drop this include? Doing so would make this portable and compilable on more architectures so us cross compilers can check build stuff and make changes easily. > +#include > + > +/* Configuration registers */ > +#define SYSC_REG_SYSTEM_CONFIG0 0x10 > +#define SYSC_REG_SYSTEM_CONFIG1 0x14 >
[PATCH v4 3/6] clk: ralink: add clock driver for mt7621 SoC
The documentation for this SOC only talks about two registers regarding to the clocks: * SYSC_REG_CPLL_CLKCFG0 - provides some information about boostrapped refclock. PLL and dividers used for CPU and some sort of BUS. * SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for all or some ip cores. Looking into driver code, and some openWRT patched there are another frequences which are used in some drivers (uart, sd...). According to all of this information the clock plan for this SoC is set as follows: - Main top clock "xtal" from where all the rest of the world is derived. - CPU clock "cpu" derived from "xtal" frequencies and a bunch of register reads and predividers. - BUS clock "bus" derived from "cpu" and with (cpu / 4) MHz. - Fixed clocks from "xtal": * "50m": 50 MHz. * "125m": 125 MHz. * "150m": 150 MHz. * "250m": 250 MHz. * "270m": 270 MHz. We also have a buch of gate clocks with their parents: * "hsdma": "150m" * "fe": "250m" * "sp_divtx": "270m" * "timer": "50m" * "pcm": "270m" * "pio": "50m" * "gdma": "bus" * "nand": "125m" * "i2c": "50m" * "i2s": "270m" * "spi": "bus" * "uart1": "50m" * "uart2": "50m" * "uart3": "50m" * "eth": "50m" * "pcie0": "125m" * "pcie1": "125m" * "pcie2": "125m" * "crypto": "250m" * "shxc": "50m" With this information the clk driver will provide clock and gates functionality from a a set of hardcoded clocks allowing to define a nice device tree without fixed clocks. Signed-off-by: Sergio Paracuellos --- drivers/clk/Kconfig | 1 + drivers/clk/Makefile| 1 + drivers/clk/ralink/Kconfig | 14 + drivers/clk/ralink/Makefile | 2 + drivers/clk/ralink/clk-mt7621.c | 435 5 files changed, 453 insertions(+) create mode 100644 drivers/clk/ralink/Kconfig create mode 100644 drivers/clk/ralink/Makefile create mode 100644 drivers/clk/ralink/clk-mt7621.c diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index c715d4681a0b..5f94c4329033 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -372,6 +372,7 @@ source "drivers/clk/mediatek/Kconfig" source "drivers/clk/meson/Kconfig" source "drivers/clk/mvebu/Kconfig" source "drivers/clk/qcom/Kconfig" +source "drivers/clk/ralink/Kconfig" source "drivers/clk/renesas/Kconfig" source "drivers/clk/rockchip/Kconfig" source "drivers/clk/samsung/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index da8fcf147eb1..6578e167b047 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -100,6 +100,7 @@ obj-$(CONFIG_COMMON_CLK_NXP)+= nxp/ obj-$(CONFIG_MACH_PISTACHIO) += pistachio/ obj-$(CONFIG_COMMON_CLK_PXA) += pxa/ obj-$(CONFIG_COMMON_CLK_QCOM) += qcom/ +obj-y += ralink/ obj-y += renesas/ obj-$(CONFIG_ARCH_ROCKCHIP)+= rockchip/ obj-$(CONFIG_COMMON_CLK_SAMSUNG) += samsung/ diff --git a/drivers/clk/ralink/Kconfig b/drivers/clk/ralink/Kconfig new file mode 100644 index ..7e8697327e0c --- /dev/null +++ b/drivers/clk/ralink/Kconfig @@ -0,0 +1,14 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# MediaTek Mt7621 Clock Driver +# +menu "Clock driver for mediatek mt7621 SoC" + depends on SOC_MT7621 || COMPILE_TEST + +config CLK_MT7621 + bool "Clock driver for MediaTek MT7621" + depends on SOC_MT7621 || COMPILE_TEST + default SOC_MT7621 + help + This driver supports MediaTek MT7621 basic clocks. +endmenu diff --git a/drivers/clk/ralink/Makefile b/drivers/clk/ralink/Makefile new file mode 100644 index ..cf6f9216379d --- /dev/null +++ b/drivers/clk/ralink/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_CLK_MT7621) += clk-mt7621.o diff --git a/drivers/clk/ralink/clk-mt7621.c b/drivers/clk/ralink/clk-mt7621.c new file mode 100644 index ..4e929f13fe7c --- /dev/null +++ b/drivers/clk/ralink/clk-mt7621.c @@ -0,0 +1,435 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Mediatek MT7621 Clock Driver + * Author: Sergio Paracuellos + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +/* Configuration registers */ +#define SYSC_REG_SYSTEM_CONFIG0 0x10 +#define SYSC_REG_SYSTEM_CONFIG1 0x14 +#define SYSC_REG_CLKCFG0 0x2c +#define SYSC_REG_CLKCFG1 0x30 +#define SYSC_REG_CUR_CLK_STS 0x44 + +#define MEMC_REG_CPU_PLL 0x648 +#define XTAL_MODE_SEL_MASK 0x7 +#define XTAL_MODE_SEL_SHIFT6 + +#define CPU_CLK_SEL_MASK 0x3 +#define CPU_CLK_SEL_SHIFT 30 + +#define CUR_CPU_FDIV_MASK 0x1f +#define CUR_CPU_FDIV_SHIFT 8 +#define CUR_CPU_FFRAC_MASK 0x1f +#define CUR_CPU_FFRAC_SHIFT0 + +#define CPU_PLL_PREDIV_MASK0x3 +#define CPU_PLL_PREDIV_SHIFT
