Re: [PATCH v4 03/11] clk: actions: Add common clock driver

Hi,

On 07/03/2020 06:42, Amit Singh Tomar wrote:

This patch converts S900 clock driver to something common that can be used for other SoCs, for instance S700(few of clk registers are same).

Signed-off-by: Amit Singh Tomar amittomer25@gmail.com

Changes since v3:

• Fixed register spelling.
• Removed the ethernet related clocks.
• Returned -EINVAL instead 0.

Changes since v2: * Fixed the commit message. * Checked for the clk->id. * Added a .data member with SoC type. * Removed #ifdefs from few places. 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_s900.h | 57 ------------ arch/arm/include/asm/arch-owl/regs_s700.h | 56 ++++++++++++ configs/bubblegum_96_defconfig | 3 - drivers/clk/owl/Kconfig | 8 +- drivers/clk/owl/Makefile | 2 +- drivers/clk/owl/clk_owl.c | 147 ++++++++++++++++++++++++++++++ drivers/clk/owl/clk_owl.h | 65 +++++++++++++ drivers/clk/owl/clk_s900.c | 137 ---------------------------- 9 files changed, 273 insertions(+), 204 deletions(-) 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 create mode 100644 drivers/clk/owl/clk_owl.h delete mode 100644 drivers/clk/owl/clk_s900.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 4889630..7a25928 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -886,6 +886,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_s900.h b/arch/arm/include/asm/arch-owl/clk_s900.h deleted file mode 100644 index 88e88f7..0000000 --- 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 manivannan.sadhasivam@linaro.org
• */

-#ifndef _OWL_CLK_S900_H_ -#define _OWL_CLK_S900_H_

-#include <clk-uclass.h>

-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_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_EN BIT(8) -#define CMU_DEVPLL_OUT (660 / 6)

-/* UARTCLK register definitions */ -#define CMU_UARTCLK_SRC_DEVPLL BIT(16)

-/* DEVCLKEN1 register definitions */ -#define CMU_DEVCLKEN1_UART5 BIT(21)

-#define PLL_STABILITY_WAIT_US 50

-#endif diff --git a/arch/arm/include/asm/arch-owl/regs_s700.h b/arch/arm/include/asm/arch-owl/regs_s700.h new file mode 100644 index 0000000..a0bd737 --- /dev/null +++ b/arch/arm/include/asm/arch-owl/regs_s700.h @@ -0,0 +1,56 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Actions Semi S700 Register Definitions
• */

+#ifndef _OWL_REGS_S700_H_ +#define _OWL_REGS_S700_H_

+#define CMU_COREPLL (0x0000) +#define CMU_DEVPLL (0x0004) +#define CMU_DDRPLL (0x0008) +#define CMU_NANDPLL (0x000C) +#define CMU_DISPLAYPLL (0x0010) +#define CMU_AUDIOPLL (0x0014) +#define CMU_TVOUTPLL (0x0018) +#define CMU_BUSCLK (0x001C) +#define CMU_SENSORCLK (0x0020) +#define CMU_LCDCLK (0x0024) +#define CMU_DSIPLLCLK (0x0028) +#define CMU_CSICLK (0x002C) +#define CMU_DECLK (0x0030) +#define CMU_SICLK (0x0034) +#define CMU_BUSCLK1 (0x0038) +#define CMU_HDECLK (0x003C) +#define CMU_VDECLK (0x0040) +#define CMU_VCECLK (0x0044) +#define CMU_NANDCCLK (0x004C) +#define CMU_SD0CLK (0x0050) +#define CMU_SD1CLK (0x0054) +#define CMU_SD2CLK (0x0058) +#define CMU_UART0CLK (0x005C) +#define CMU_UART1CLK (0x0060) +#define CMU_UART2CLK (0x0064) +#define CMU_UART3CLK (0x0068) +#define CMU_UART4CLK (0x006C) +#define CMU_UART5CLK (0x0070) +#define CMU_UART6CLK (0x0074) +#define CMU_PWM0CLK (0x0078) +#define CMU_PWM1CLK (0x007C) +#define CMU_PWM2CLK (0x0080) +#define CMU_PWM3CLK (0x0084) +#define CMU_PWM4CLK (0x0088) +#define CMU_PWM5CLK (0x008C) +#define CMU_GPU3DCLK (0x0090) +#define CMU_CORECTL (0x009C) +#define CMU_DEVCLKEN0 (0x00A0) +#define CMU_DEVCLKEN1 (0x00A4) +#define CMU_DEVRST0 (0x00A8) +#define CMU_DEVRST1 (0x00AC) +#define CMU_USBPLL (0x00B0) +#define CMU_ETHERNETPLL (0x00B4) +#define CMU_CVBSPLL (0x00B8) +#define CMU_SSTSCLK (0x00C0)

Any chance you can remove the parentheses?

+#endif diff --git a/configs/bubblegum_96_defconfig b/configs/bubblegum_96_defconfig index 8c94def..e76e9a2 100644 --- a/configs/bubblegum_96_defconfig +++ b/configs/bubblegum_96_defconfig @@ -17,6 +17,3 @@ CONFIG_CMD_CACHE=y CONFIG_CMD_TIMER=y CONFIG_DEFAULT_DEVICE_TREE="bubblegum_96" CONFIG_SYS_RELOC_GD_ENV_ADDR=y -CONFIG_CLK=y -CONFIG_CLK_OWL=y -CONFIG_CLK_S900=y diff --git a/drivers/clk/owl/Kconfig b/drivers/clk/owl/Kconfig index 661f198..c6afef9 100644 --- a/drivers/clk/owl/Kconfig +++ b/drivers/clk/owl/Kconfig @@ -3,10 +3,6 @@ config CLK_OWL depends on CLK && ARCH_OWL help Enable support for clock managemet unit present in Actions Semi

•  OWL SoCs.
  

•  Owl series S900/S700 SoCs.
  

-config CLK_S900

•    bool "Actions Semi S900 clock driver"
  

•    depends on CLK_OWL && ARM64
  

•    help
  

•      Enable support for the clocks in Actions Semi S900 SoC.
  

diff --git a/drivers/clk/owl/Makefile b/drivers/clk/owl/Makefile index 63ab573..5218b6b 100644 --- a/drivers/clk/owl/Makefile +++ b/drivers/clk/owl/Makefile @@ -1,3 +1,3 @@ # SPDX-License-Identifier: GPL-2.0+

-obj-$(CONFIG_CLK_S900) += clk_s900.o +obj-$(CONFIG_CLK_OWL) += clk_owl.o diff --git a/drivers/clk/owl/clk_owl.c b/drivers/clk/owl/clk_owl.c new file mode 100644 index 0000000..2e4d550 --- /dev/null +++ b/drivers/clk/owl/clk_owl.c @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Common clock driver for Actions Semi SoCs.
• • Copyright (C) 2015 Actions Semi Co., Ltd.
• • Copyright (C) 2018 Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
• */

+#include <common.h> +#include <dm.h> +#include "clk_owl.h" +#include <asm/io.h> +#if defined(CONFIG_MACH_S900) +#include <asm/arch-owl/regs_s900.h> +#include <dt-bindings/clock/actions,s900-cmu.h> +#elif defined(CONFIG_MACH_S700) +#include <asm/arch-owl/regs_s700.h> +#include <dt-bindings/clock/actions,s700-cmu.h>

Both those include files do not exist yet. This breaks bisectability for bubblegum_96. So I would suggest you remove the s700 lines for now, and change the s900 filename to match the existing one. Then change CLK_UART<x> to CLOCK_UART<x> below to make it compile.

Please check that bubblegum_96_defconfig still builds after *every* commit.

Once you sync up the binding include file for S900 later on, you should revert those changes back in the same commit.

+#endif

+void owl_clk_init(struct owl_clk_priv *priv) +{

• u32 bus_clk = 0, core_pll, dev_pll;

+#if defined(CONFIG_MACH_S900)

• /* Enable ASSIST_PLL */
• setbits_le32(priv->base + CMU_ASSISTPLL, BIT(0));
• udelay(PLL_STABILITY_WAIT_US);

+#endif

• /* Source HOSC to DEV_CLK */
• clrbits_le32(priv->base + CMU_DEVPLL, CMU_DEVPLL_CLK);
• /* Configure BUS_CLK */
• bus_clk |= (CMU_PDBGDIV_DIV | CMU_PERDIV_DIV | CMU_NOCDIV_DIV |

• 	CMU_DMMCLK_SRC | CMU_APBCLK_DIV | CMU_AHBCLK_DIV |
  

• 	CMU_NOCCLK_SRC | CMU_CORECLK_HOSC);
  

• writel(bus_clk, priv->base + CMU_BUSCLK);
• udelay(PLL_STABILITY_WAIT_US);
• /* Configure CORE_PLL */
• core_pll = readl(priv->base + CMU_COREPLL);
• core_pll |= (CMU_COREPLL_EN | CMU_COREPLL_HOSC_EN | CMU_COREPLL_OUT);
• writel(core_pll, priv->base + CMU_COREPLL);
• udelay(PLL_STABILITY_WAIT_US);
• /* Configure DEV_PLL */
• dev_pll = readl(priv->base + CMU_DEVPLL);
• dev_pll |= (CMU_DEVPLL_EN | CMU_DEVPLL_OUT);
• writel(dev_pll, priv->base + CMU_DEVPLL);
• udelay(PLL_STABILITY_WAIT_US);
• /* Source CORE_PLL for CORE_CLK */
• clrsetbits_le32(priv->base + CMU_BUSCLK, CMU_CORECLK_MASK,

• 	CMU_CORECLK_CPLL);
  

• /* Source DEV_PLL for DEV_CLK */
• setbits_le32(priv->base + CMU_DEVPLL, CMU_DEVPLL_CLK);
• udelay(PLL_STABILITY_WAIT_US);

+}

+void owl_uart_clk_enable(struct owl_clk_priv *priv) +{

• if (priv->soc == S900) {

• /* Source HOSC for UART5 interface */
  

• clrbits_le32(priv->base + CMU_UART5CLK, CMU_UARTCLK_SRC_DEVPLL);
  

• /* Enable UART5 interface clock */
  

• setbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_UART5);
  

• } else if (priv->soc == S700) {

• /* Source HOSC for UART3 interface */
  

• clrbits_le32(priv->base + CMU_UART3CLK, CMU_UARTCLK_SRC_DEVPLL);
  

• /* Enable UART3 interface clock */
  

• setbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_UART3);
  

• }

+}

+int owl_clk_enable(struct clk *clk) +{

• struct owl_clk_priv *priv = dev_get_priv(clk->dev);
• switch (clk->id) {
• case CLK_UART5:
• case CLK_UART3:

• owl_uart_clk_enable(priv);
  

So that's bit weird: Here you join the clocks from both SoCs, just to differentiate them again in the function above. Why not just merge owl_uart_clk_enable() into this function, something like: case CLK_UART5: if (priv->soc == S700) return -EINVAL; clrbits(....); setbits(....); break; case CLK_UART3: if (priv->soc == S900) ...

That is much more future-proof and easier to extend later.

• break;
  

• default:

• return -EINVAL;
  

• }
• return 0;

+}

+int owl_clk_disable(struct clk *clk) +{

• struct owl_clk_priv *priv = dev_get_priv(clk->dev);
• if (priv->soc == S900) {

Please use the provided clock ID in a switch/case instead of relying on the UART clock being the only clock ever disabled. This also saves you the comments.

• /* Disable UART5 interface clock */
  

• clrbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_UART5);
  

• } else if (priv->soc == S700) {

• /* Disable UART3 interface clock */
  

• clrbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_UART3);
  

• }
• return 0;

+}

+static int owl_clk_probe(struct udevice *dev) +{

• struct owl_clk_priv *priv = dev_get_priv(dev);
• priv->base = dev_read_addr(dev);
• if (priv->base == FDT_ADDR_T_NONE)

• return -EINVAL;
  

• /* setup necessary clocks */
• owl_clk_init(priv);
• return 0;

+}

+static const struct clk_ops owl_clk_ops = {

• .enable = owl_clk_enable,
• .disable = owl_clk_disable,

+};

+static const struct udevice_id owl_clk_ids[] = { +#if defined(CONFIG_MACH_S900)

• { .compatible = "actions,s900-cmu", .data = S900 },

+#elif defined(CONFIG_MACH_S700)

• { .compatible = "actions,s700-cmu", .data = S700 },
• { }

+#endif

The #endif needs to be one line up, otherwise you break S900.

The rest looks OK now in general.

Cheers, Andre.

+};

+U_BOOT_DRIVER(clk_owl) = {

• .name = "clk_owl",
• .id = UCLASS_CLK,
• .of_match = owl_clk_ids,
• .ops = &owl_clk_ops,
• .priv_auto_alloc_size = sizeof(struct owl_clk_priv),
• .probe = owl_clk_probe,

+}; diff --git a/drivers/clk/owl/clk_owl.h b/drivers/clk/owl/clk_owl.h new file mode 100644 index 0000000..9a65091 --- /dev/null +++ b/drivers/clk/owl/clk_owl.h @@ -0,0 +1,65 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Actions Semi SoCs Clock Definitions
• • Copyright (C) 2015 Actions Semi Co., Ltd.
• • Copyright (C) 2018 Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
• */

+#ifndef _OWL_CLK_H_ +#define _OWL_CLK_H_

+#include <clk-uclass.h>

+enum owl_soc {

• S700,
• S900,

+};

+struct owl_clk_priv {

• phys_addr_t base;
• enum owl_soc soc;

+};

+/* 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_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_EN BIT(8) +#define CMU_DEVPLL_OUT (660 / 6)

+/* UARTCLK register definitions */ +#define CMU_UARTCLK_SRC_DEVPLL BIT(16)

+#define PLL_STABILITY_WAIT_US 50

+#define CMU_DEVCLKEN1_UART5 BIT(21) +#define CMU_DEVCLKEN1_UART3 BIT(11)

+#define CMU_DEVCLKEN1_ETH_S700 BIT(23)

+#endif diff --git a/drivers/clk/owl/clk_s900.c b/drivers/clk/owl/clk_s900.c deleted file mode 100644 index a7c15d2..0000000 --- a/drivers/clk/owl/clk_s900.c +++ /dev/null @@ -1,137 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/*

• • Actions Semi S900 clock driver
• • Copyright (C) 2015 Actions Semi Co., Ltd.
• • Copyright (C) 2018 Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
• */

-#include <common.h> -#include <dm.h> -#include <asm/arch-owl/clk_s900.h> -#include <asm/arch-owl/regs_s900.h> -#include <asm/io.h>

-#include <dt-bindings/clock/s900_cmu.h>

-void owl_clk_init(struct owl_clk_priv *priv) -{

• u32 bus_clk = 0, core_pll, dev_pll;
• /* Enable ASSIST_PLL */
• setbits_le32(priv->base + CMU_ASSISTPLL, BIT(0));
• udelay(PLL_STABILITY_WAIT_US);
• /* Source HOSC to DEV_CLK */
• clrbits_le32(priv->base + CMU_DEVPLL, CMU_DEVPLL_CLK);
• /* Configure BUS_CLK */
• bus_clk |= (CMU_PDBGDIV_DIV | CMU_PERDIV_DIV | CMU_NOCDIV_DIV |

• 	CMU_DMMCLK_SRC | CMU_APBCLK_DIV | CMU_AHBCLK_DIV |
  

• 	CMU_NOCCLK_SRC | CMU_CORECLK_HOSC);
  

• writel(bus_clk, priv->base + CMU_BUSCLK);
• udelay(PLL_STABILITY_WAIT_US);
• /* Configure CORE_PLL */
• core_pll = readl(priv->base + CMU_COREPLL);
• core_pll |= (CMU_COREPLL_EN | CMU_COREPLL_HOSC_EN | CMU_COREPLL_OUT);
• writel(core_pll, priv->base + CMU_COREPLL);
• udelay(PLL_STABILITY_WAIT_US);
• /* Configure DEV_PLL */
• dev_pll = readl(priv->base + CMU_DEVPLL);
• dev_pll |= (CMU_DEVPLL_EN | CMU_DEVPLL_OUT);
• writel(dev_pll, priv->base + CMU_DEVPLL);
• udelay(PLL_STABILITY_WAIT_US);
• /* Source CORE_PLL for CORE_CLK */
• clrsetbits_le32(priv->base + CMU_BUSCLK, CMU_CORECLK_MASK,

• 	CMU_CORECLK_CPLL);
  

• /* Source DEV_PLL for DEV_CLK */
• setbits_le32(priv->base + CMU_DEVPLL, CMU_DEVPLL_CLK);
• udelay(PLL_STABILITY_WAIT_US);

-}

-void owl_uart_clk_enable(struct owl_clk_priv *priv) -{

• /* Source HOSC for UART5 interface */
• clrbits_le32(priv->base + CMU_UART5CLK, CMU_UARTCLK_SRC_DEVPLL);
• /* Enable UART5 interface clock */
• setbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_UART5);

-}

-void owl_uart_clk_disable(struct owl_clk_priv *priv) -{

• /* Disable UART5 interface clock */
• clrbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_UART5);

-}

-int owl_clk_enable(struct clk *clk) -{

• struct owl_clk_priv *priv = dev_get_priv(clk->dev);
• switch (clk->id) {
• case CLOCK_UART5:

• owl_uart_clk_enable(priv);
  

• break;
  

• default:

• return 0;
  

• }
• return 0;

-}

-int owl_clk_disable(struct clk *clk) -{

• struct owl_clk_priv *priv = dev_get_priv(clk->dev);
• switch (clk->id) {
• case CLOCK_UART5:

• owl_uart_clk_disable(priv);
  

• break;
  

• default:

• return 0;
  

• }
• return 0;

-}

-static int owl_clk_probe(struct udevice *dev) -{

• struct owl_clk_priv *priv = dev_get_priv(dev);
• priv->base = dev_read_addr(dev);
• if (priv->base == FDT_ADDR_T_NONE)

• return -EINVAL;
  

• /* setup necessary clocks */
• owl_clk_init(priv);
• return 0;

-}

-static struct clk_ops owl_clk_ops = {

• .enable = owl_clk_enable,
• .disable = owl_clk_disable,

-};

-static const struct udevice_id owl_clk_ids[] = {

• { .compatible = "actions,s900-cmu" },
• { }

-};

-U_BOOT_DRIVER(clk_owl) = {

• .name = "clk_s900",
• .id = UCLASS_CLK,
• .of_match = owl_clk_ids,
• .ops = &owl_clk_ops,
• .priv_auto_alloc_size = sizeof(struct owl_clk_priv),
• .probe = owl_clk_probe,

-};