Re: [U-Boot] [PATCH v3 3/9] clk: actions: Add common clock driver
On Thu, 17 Jan 2019 20:45:44 +0530 Manivannan Sadhasivam wrote: Hi, > On Tue, Jan 15, 2019 at 12:43:36AM +, André Przywara wrote: > > On 14/01/2019 12:41, Amit Singh Tomar wrote: > > > > Hi, > > > > > CMU block on most of the actions SoC seems to be > > > identical(at-least, S900 and S700). > > > > Actually they are not. Not even for the small subset that we > > implement here. Try "diff -wu > > arch/arm/include/asm/arch-owl/regs_s*.h" for a start, plus the > > differences in the #ifdefs below. > > > This patch converts S900 clock driver to something common that can > > > be used for other SoCs, for instance S700(most of clk registres > > > are same). > > > > I am not sure this is a viable approach, really. > > The driver claims to support both SoC's via their compatible > > strings, but in fact is just implementing the one configured via > > Kconfig. While we should be able to easily replace the #ifdefs with > > something checking the .data member associated with the > > respective .compatible string, it gets hairy with the #defines in > > regs_s[79]00.h. So either it's two different .c files, with the > > register definitions in there, or we change the CMU_* defines to > > CMU_S[79]00_* and include both. > > > > Maybe you could try this and report how it looks like? > > If half of the file is within if-else statements, separating is > > problem more worthwhile. > > Mani, you mentioned the S500, I guess this is even more different, > > right? Which would point into the "separate files" direction. > > > > S500 is different in terms of the clock initializations. Ideally we > should have a minimal set of common clk driver like in Linux which I > authored. Otherwise, we should move forward with individual clk files > for each SoC. Having #ifdef's will definitely make the code look > messy. Yeah, that is probably the right way to go, especially when it comes to clocks like MMC, which are probably similar in their internal workings, but are at different addresses or use different offsets. Problem is we don't know this yet for sure, and Amit has a point that the files are indeed very similar right now. So as a quick measure to ensure correctness, but still keep it simple, I was thinking about keeping most of the patch as it is (still addressing my previous comments) and just protect the compatible strings below with MACH_S[79]00 ifdefs, so that both SoCs share the file, but compile to different drivers supporting only one compatible (due to the different reg_sx00.h and the #ifdef protected parts). This would arguably be the only difference between the s700 and s900 U-Boot binaries, but it's still a good start to get this SoC supported in the first place. We can always rework the clocks once we get more users (both clock-wise and SoC wise) and see what we really need and how much we can share. Cheers, Andre. > > > > > > Signed-off-by: Amit Singh Tomar > > > --- > > > Changes since v1: > > > * Moved CLK and CLK_OWL symbols from defconfig to > > > arch/arm/Kconfig. --- > > > arch/arm/Kconfig | 2 + > > > arch/arm/include/asm/arch-owl/clk_owl.h | 61 + > > > arch/arm/include/asm/arch-owl/clk_s900.h | 57 - > > > arch/arm/include/asm/arch-owl/regs_s700.h | 56 > > > > This file doesn't define an interface, so should live in the > > drivers/clk/owl directory. Same with the regs_s900.h. > > > > > configs/bubblegum_96_defconfig| 3 - > > > drivers/clk/owl/Kconfig | 10 +-- > > > drivers/clk/owl/Makefile | 2 +- > > > drivers/clk/owl/clk_owl.c | 132 > > > > > > drivers/clk/owl/clk_s900.c| 137 > > > -- 9 files changed, 255 > > > insertions(+), 205 deletions(-) create mode 100644 > > > arch/arm/include/asm/arch-owl/clk_owl.h delete mode 100644 > > > arch/arm/include/asm/arch-owl/clk_s900.h create mode 100644 > > > arch/arm/include/asm/arch-owl/regs_s700.h create mode 100644 > > > drivers/clk/owl/clk_owl.c delete mode 100644 > > > drivers/clk/owl/clk_s900.c > > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > > index 1a2e561..1daf3bf 100644 > > > --- a/arch/arm/Kconfig > > > +++ b/arch/arm/Kconfig > > > @@ -764,6 +764,8 @@ config ARCH_OWL > > > select DM > > > select DM_SERIAL > > > select OWL_SERIAL > > > + select CLK > > > + select CLK_OWL > > > select OF_CONTROL > > > imply CMD_DM > > > > > > diff --git a/arch/arm/include/asm/arch-owl/clk_owl.h > > > b/arch/arm/include/asm/arch-owl/clk_owl.h new file mode 100644 > > > index 000..962badd > > > --- /dev/null > > > +++ b/arch/arm/include/asm/arch-owl/clk_owl.h > > > @@ -0,0 +1,61 @@ > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > +/* > > > + * Actions Semi SoCs Clock Definitions > > > + * > > > + * Copyright (C) 2015 Actions Semi Co., Ltd. > > > + * Copyright (C) 2018 Manivannan Sadhasivam > > >
Re: [U-Boot] [PATCH v3 3/9] clk: actions: Add common clock driver
Hi,
On Tue, Jan 15, 2019 at 12:43:36AM +, André Przywara wrote:
> On 14/01/2019 12:41, Amit Singh Tomar wrote:
>
> Hi,
>
> > CMU block on most of the actions SoC seems to be identical(at-least, S900
> > and S700).
>
> Actually they are not. Not even for the small subset that we implement
> here. Try "diff -wu arch/arm/include/asm/arch-owl/regs_s*.h" for a
> start, plus the differences in the #ifdefs below.
>
> > This patch converts S900 clock driver to something common that can
> > be used for other SoCs, for instance S700(most of clk registres are same).
>
> I am not sure this is a viable approach, really.
> The driver claims to support both SoC's via their compatible strings,
> but in fact is just implementing the one configured via Kconfig.
> While we should be able to easily replace the #ifdefs with something
> checking the .data member associated with the respective .compatible
> string, it gets hairy with the #defines in regs_s[79]00.h.
> So either it's two different .c files, with the register definitions in
> there, or we change the CMU_* defines to CMU_S[79]00_* and include both.
>
> Maybe you could try this and report how it looks like?
> If half of the file is within if-else statements, separating is problem
> more worthwhile.
> Mani, you mentioned the S500, I guess this is even more different,
> right? Which would point into the "separate files" direction.
>
S500 is different in terms of the clock initializations. Ideally we should
have a minimal set of common clk driver like in Linux which I authored.
Otherwise, we should move forward with individual clk files for each
SoC. Having #ifdef's will definitely make the code look messy.
Thanks,
Mani
> >
> > Signed-off-by: Amit Singh Tomar
> > ---
> > Changes since v1:
> > * Moved CLK and CLK_OWL symbols from defconfig to arch/arm/Kconfig.
> > ---
> > arch/arm/Kconfig | 2 +
> > arch/arm/include/asm/arch-owl/clk_owl.h | 61 +
> > arch/arm/include/asm/arch-owl/clk_s900.h | 57 -
> > arch/arm/include/asm/arch-owl/regs_s700.h | 56
>
> This file doesn't define an interface, so should live in the
> drivers/clk/owl directory. Same with the regs_s900.h.
>
> > configs/bubblegum_96_defconfig| 3 -
> > drivers/clk/owl/Kconfig | 10 +--
> > drivers/clk/owl/Makefile | 2 +-
> > drivers/clk/owl/clk_owl.c | 132
> >
> > drivers/clk/owl/clk_s900.c| 137
> > --
> > 9 files changed, 255 insertions(+), 205 deletions(-)
> > create mode 100644 arch/arm/include/asm/arch-owl/clk_owl.h
> > delete mode 100644 arch/arm/include/asm/arch-owl/clk_s900.h
> > create mode 100644 arch/arm/include/asm/arch-owl/regs_s700.h
> > create mode 100644 drivers/clk/owl/clk_owl.c
> > delete mode 100644 drivers/clk/owl/clk_s900.c
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 1a2e561..1daf3bf 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -764,6 +764,8 @@ config ARCH_OWL
> > select DM
> > select DM_SERIAL
> > select OWL_SERIAL
> > + select CLK
> > + select CLK_OWL
> > select OF_CONTROL
> > imply CMD_DM
> >
> > diff --git a/arch/arm/include/asm/arch-owl/clk_owl.h
> > b/arch/arm/include/asm/arch-owl/clk_owl.h
> > new file mode 100644
> > index 000..962badd
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-owl/clk_owl.h
> > @@ -0,0 +1,61 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Actions Semi SoCs Clock Definitions
> > + *
> > + * Copyright (C) 2015 Actions Semi Co., Ltd.
> > + * Copyright (C) 2018 Manivannan Sadhasivam
> >
> > + *
> > + */
> > +
> > +#ifndef _OWL_CLK_H_
> > +#define _OWL_CLK_H_
> > +
> > +#include
> > +
> > +struct owl_clk_priv {
> > + phys_addr_t base;
> > +};
> > +
> > +/* BUSCLK register definitions */
> > +#define CMU_PDBGDIV_8 7
> > +#define CMU_PDBGDIV_SHIFT 26
> > +#define CMU_PDBGDIV_DIV(CMU_PDBGDIV_8 << CMU_PDBGDIV_SHIFT)
> > +#define CMU_PERDIV_8 7
> > +#define CMU_PERDIV_SHIFT 20
> > +#define CMU_PERDIV_DIV (CMU_PERDIV_8 << CMU_PERDIV_SHIFT)
> > +#define CMU_NOCDIV_2 1
> > +#define CMU_NOCDIV_SHIFT 19
> > +#define CMU_NOCDIV_DIV (CMU_NOCDIV_2 << CMU_NOCDIV_SHIFT)
> > +#define CMU_DMMCLK_SRC_APLL2
> > +#define CMU_DMMCLK_SRC_SHIFT 10
> > +#define CMU_DMMCLK_SRC (CMU_DMMCLK_SRC_APLL <<
> > CMU_DMMCLK_SRC_SHIFT)
> > +#define CMU_APBCLK_DIV BIT(8)
> > +#define CMU_NOCCLK_SRC BIT(7)
> > +#define CMU_AHBCLK_DIV BIT(4)
> > +#define CMU_CORECLK_MASK 3
> > +#define CMU_CORECLK_CPLL BIT(1)
> > +#define CMU_CORECLK_HOSC BIT(0)
> > +
> > +/* COREPLL register definitions */
> > +#define CMU_COREPLL_EN BIT(9)
> > +#define CMU_COREPLL_HOSC_ENBIT(8)
> > +#define
Re: [U-Boot] [PATCH v3 3/9] clk: actions: Add common clock driver
On 14/01/2019 12:41, Amit Singh Tomar wrote:
Hi,
> CMU block on most of the actions SoC seems to be identical(at-least, S900 and
> S700).
Actually they are not. Not even for the small subset that we implement
here. Try "diff -wu arch/arm/include/asm/arch-owl/regs_s*.h" for a
start, plus the differences in the #ifdefs below.
> This patch converts S900 clock driver to something common that can
> be used for other SoCs, for instance S700(most of clk registres are same).
I am not sure this is a viable approach, really.
The driver claims to support both SoC's via their compatible strings,
but in fact is just implementing the one configured via Kconfig.
While we should be able to easily replace the #ifdefs with something
checking the .data member associated with the respective .compatible
string, it gets hairy with the #defines in regs_s[79]00.h.
So either it's two different .c files, with the register definitions in
there, or we change the CMU_* defines to CMU_S[79]00_* and include both.
Maybe you could try this and report how it looks like?
If half of the file is within if-else statements, separating is problem
more worthwhile.
Mani, you mentioned the S500, I guess this is even more different,
right? Which would point into the "separate files" direction.
>
> Signed-off-by: Amit Singh Tomar
> ---
> Changes since v1:
> * Moved CLK and CLK_OWL symbols from defconfig to arch/arm/Kconfig.
> ---
> arch/arm/Kconfig | 2 +
> arch/arm/include/asm/arch-owl/clk_owl.h | 61 +
> arch/arm/include/asm/arch-owl/clk_s900.h | 57 -
> arch/arm/include/asm/arch-owl/regs_s700.h | 56
This file doesn't define an interface, so should live in the
drivers/clk/owl directory. Same with the regs_s900.h.
> configs/bubblegum_96_defconfig| 3 -
> drivers/clk/owl/Kconfig | 10 +--
> drivers/clk/owl/Makefile | 2 +-
> drivers/clk/owl/clk_owl.c | 132
> drivers/clk/owl/clk_s900.c| 137
> --
> 9 files changed, 255 insertions(+), 205 deletions(-)
> create mode 100644 arch/arm/include/asm/arch-owl/clk_owl.h
> delete mode 100644 arch/arm/include/asm/arch-owl/clk_s900.h
> create mode 100644 arch/arm/include/asm/arch-owl/regs_s700.h
> create mode 100644 drivers/clk/owl/clk_owl.c
> delete mode 100644 drivers/clk/owl/clk_s900.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 1a2e561..1daf3bf 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -764,6 +764,8 @@ config ARCH_OWL
> select DM
> select DM_SERIAL
> select OWL_SERIAL
> + select CLK
> + select CLK_OWL
> select OF_CONTROL
> imply CMD_DM
>
> diff --git a/arch/arm/include/asm/arch-owl/clk_owl.h
> b/arch/arm/include/asm/arch-owl/clk_owl.h
> new file mode 100644
> index 000..962badd
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-owl/clk_owl.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Actions Semi SoCs Clock Definitions
> + *
> + * Copyright (C) 2015 Actions Semi Co., Ltd.
> + * Copyright (C) 2018 Manivannan Sadhasivam
>
> + *
> + */
> +
> +#ifndef _OWL_CLK_H_
> +#define _OWL_CLK_H_
> +
> +#include
> +
> +struct owl_clk_priv {
> + phys_addr_t base;
> +};
> +
> +/* BUSCLK register definitions */
> +#define CMU_PDBGDIV_87
> +#define CMU_PDBGDIV_SHIFT26
> +#define CMU_PDBGDIV_DIV (CMU_PDBGDIV_8 << CMU_PDBGDIV_SHIFT)
> +#define CMU_PERDIV_8 7
> +#define CMU_PERDIV_SHIFT 20
> +#define CMU_PERDIV_DIV (CMU_PERDIV_8 << CMU_PERDIV_SHIFT)
> +#define CMU_NOCDIV_2 1
> +#define CMU_NOCDIV_SHIFT 19
> +#define CMU_NOCDIV_DIV (CMU_NOCDIV_2 << CMU_NOCDIV_SHIFT)
> +#define CMU_DMMCLK_SRC_APLL 2
> +#define CMU_DMMCLK_SRC_SHIFT 10
> +#define CMU_DMMCLK_SRC (CMU_DMMCLK_SRC_APLL <<
> CMU_DMMCLK_SRC_SHIFT)
> +#define CMU_APBCLK_DIV BIT(8)
> +#define CMU_NOCCLK_SRC BIT(7)
> +#define CMU_AHBCLK_DIV BIT(4)
> +#define CMU_CORECLK_MASK 3
> +#define CMU_CORECLK_CPLL BIT(1)
> +#define CMU_CORECLK_HOSC BIT(0)
> +
> +/* COREPLL register definitions */
> +#define CMU_COREPLL_EN BIT(9)
> +#define CMU_COREPLL_HOSC_EN BIT(8)
> +#define CMU_COREPLL_OUT (1104 / 24)
> +
> +/* DEVPLL register definitions */
> +#define CMU_DEVPLL_CLK BIT(12)
> +#define CMU_DEVPLL_ENBIT(8)
> +#define CMU_DEVPLL_OUT (660 / 6)
> +
> +/* UARTCLK register definitions */
> +#define CMU_UARTCLK_SRC_DEVPLL BIT(16)
> +
> +/* DEVCLKEN1 register definitions */
> +#if defined(CONFIG_MACH_S900)
Meh.
> +#define CMU_DEVCLKEN1_UART5 BIT(21)
> +#elif defined(CONFIG_MACH_S700)
> +#define CMU_DEVCLKEN1_UART3 BIT(11)
> +#endif
> +
> +#define PLL_STABILITY_WAIT_US50
> +
> +#endif
> diff --git
[U-Boot] [PATCH v3 3/9] clk: actions: Add common clock driver
CMU block on most of the actions SoC seems to be identical(at-least, S900 and
S700).
This patch converts S900 clock driver to something common that can
be used for other SoCs, for instance S700(most of clk registres are same).
Signed-off-by: Amit Singh Tomar
---
Changes since v1:
* Moved CLK and CLK_OWL symbols from defconfig to arch/arm/Kconfig.
---
arch/arm/Kconfig | 2 +
arch/arm/include/asm/arch-owl/clk_owl.h | 61 +
arch/arm/include/asm/arch-owl/clk_s900.h | 57 -
arch/arm/include/asm/arch-owl/regs_s700.h | 56
configs/bubblegum_96_defconfig| 3 -
drivers/clk/owl/Kconfig | 10 +--
drivers/clk/owl/Makefile | 2 +-
drivers/clk/owl/clk_owl.c | 132
drivers/clk/owl/clk_s900.c| 137 --
9 files changed, 255 insertions(+), 205 deletions(-)
create mode 100644 arch/arm/include/asm/arch-owl/clk_owl.h
delete mode 100644 arch/arm/include/asm/arch-owl/clk_s900.h
create mode 100644 arch/arm/include/asm/arch-owl/regs_s700.h
create mode 100644 drivers/clk/owl/clk_owl.c
delete mode 100644 drivers/clk/owl/clk_s900.c
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 1a2e561..1daf3bf 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -764,6 +764,8 @@ config ARCH_OWL
select DM
select DM_SERIAL
select OWL_SERIAL
+ select CLK
+ select CLK_OWL
select OF_CONTROL
imply CMD_DM
diff --git a/arch/arm/include/asm/arch-owl/clk_owl.h
b/arch/arm/include/asm/arch-owl/clk_owl.h
new file mode 100644
index 000..962badd
--- /dev/null
+++ b/arch/arm/include/asm/arch-owl/clk_owl.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Actions Semi SoCs Clock Definitions
+ *
+ * Copyright (C) 2015 Actions Semi Co., Ltd.
+ * Copyright (C) 2018 Manivannan Sadhasivam
+ *
+ */
+
+#ifndef _OWL_CLK_H_
+#define _OWL_CLK_H_
+
+#include
+
+struct owl_clk_priv {
+ phys_addr_t base;
+};
+
+/* BUSCLK register definitions */
+#define CMU_PDBGDIV_8 7
+#define CMU_PDBGDIV_SHIFT 26
+#define CMU_PDBGDIV_DIV(CMU_PDBGDIV_8 << CMU_PDBGDIV_SHIFT)
+#define CMU_PERDIV_8 7
+#define CMU_PERDIV_SHIFT 20
+#define CMU_PERDIV_DIV (CMU_PERDIV_8 << CMU_PERDIV_SHIFT)
+#define CMU_NOCDIV_2 1
+#define CMU_NOCDIV_SHIFT 19
+#define CMU_NOCDIV_DIV (CMU_NOCDIV_2 << CMU_NOCDIV_SHIFT)
+#define CMU_DMMCLK_SRC_APLL2
+#define CMU_DMMCLK_SRC_SHIFT 10
+#define CMU_DMMCLK_SRC (CMU_DMMCLK_SRC_APLL << CMU_DMMCLK_SRC_SHIFT)
+#define CMU_APBCLK_DIV BIT(8)
+#define CMU_NOCCLK_SRC BIT(7)
+#define CMU_AHBCLK_DIV BIT(4)
+#define CMU_CORECLK_MASK 3
+#define CMU_CORECLK_CPLL BIT(1)
+#define CMU_CORECLK_HOSC BIT(0)
+
+/* COREPLL register definitions */
+#define CMU_COREPLL_EN BIT(9)
+#define CMU_COREPLL_HOSC_ENBIT(8)
+#define CMU_COREPLL_OUT(1104 / 24)
+
+/* DEVPLL register definitions */
+#define CMU_DEVPLL_CLK BIT(12)
+#define CMU_DEVPLL_EN BIT(8)
+#define CMU_DEVPLL_OUT (660 / 6)
+
+/* UARTCLK register definitions */
+#define CMU_UARTCLK_SRC_DEVPLL BIT(16)
+
+/* DEVCLKEN1 register definitions */
+#if defined(CONFIG_MACH_S900)
+#define CMU_DEVCLKEN1_UART5BIT(21)
+#elif defined(CONFIG_MACH_S700)
+#define CMU_DEVCLKEN1_UART3 BIT(11)
+#endif
+
+#define PLL_STABILITY_WAIT_US 50
+
+#endif
diff --git a/arch/arm/include/asm/arch-owl/clk_s900.h
b/arch/arm/include/asm/arch-owl/clk_s900.h
deleted file mode 100644
index 88e88f7..000
--- a/arch/arm/include/asm/arch-owl/clk_s900.h
+++ /dev/null
@@ -1,57 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
-/*
- * Actions Semi S900 Clock Definitions
- *
- * Copyright (C) 2015 Actions Semi Co., Ltd.
- * Copyright (C) 2018 Manivannan Sadhasivam
- *
- */
-
-#ifndef _OWL_CLK_S900_H_
-#define _OWL_CLK_S900_H_
-
-#include
-
-struct owl_clk_priv {
- phys_addr_t base;
-};
-
-/* BUSCLK register definitions */
-#define CMU_PDBGDIV_8 7
-#define CMU_PDBGDIV_SHIFT 26
-#define CMU_PDBGDIV_DIV(CMU_PDBGDIV_8 << CMU_PDBGDIV_SHIFT)
-#define CMU_PERDIV_8 7
-#define CMU_PERDIV_SHIFT 20
-#define CMU_PERDIV_DIV (CMU_PERDIV_8 << CMU_PERDIV_SHIFT)
-#define CMU_NOCDIV_2 1
-#define CMU_NOCDIV_SHIFT 19
-#define CMU_NOCDIV_DIV (CMU_NOCDIV_2 << CMU_NOCDIV_SHIFT)
-#define CMU_DMMCLK_SRC_APLL2
-#define CMU_DMMCLK_SRC_SHIFT 10
-#define CMU_DMMCLK_SRC (CMU_DMMCLK_SRC_APLL << CMU_DMMCLK_SRC_SHIFT)
-#define CMU_APBCLK_DIV BIT(8)
-#define CMU_NOCCLK_SRC BIT(7)
-#define CMU_AHBCLK_DIV BIT(4)
-#define CMU_CORECLK_MASK 3
-#define CMU_CORECLK_CPLL BIT(1)
-#define CMU_CORECLK_HOSC BIT(0)
-
-/* COREPLL register definitions */
-#define CMU_COREPLL_EN
