Re: [PATCH v1 2/9] clk: nuvoton: Add support for NPCM845

On 12/14/21 9:57 PM, Stanley Chu wrote:

Add clock controller driver for NPCM845

Signed-off-by: Stanley Chu yschu@nuvoton.com

drivers/clk/Makefile | 1 + drivers/clk/nuvoton/Makefile | 1 + drivers/clk/nuvoton/clk_npcm8xx.c | 213 ++++++++++++++++++++++ include/dt-bindings/clock/npcm845-clock.h | 17 ++ 4 files changed, 232 insertions(+) create mode 100644 drivers/clk/nuvoton/Makefile create mode 100644 drivers/clk/nuvoton/clk_npcm8xx.c create mode 100644 include/dt-bindings/clock/npcm845-clock.h

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 711ae5bc29..a3b64b73c2 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -53,3 +53,4 @@ obj-$(CONFIG_STM32H7) += clk_stm32h7.o obj-$(CONFIG_CLK_VERSAL) += clk_versal.o obj-$(CONFIG_CLK_CDCE9XX) += clk-cdce9xx.o obj-$(CONFIG_CLK_VERSACLOCK) += clk_versaclock.o +obj-$(CONFIG_ARCH_NPCM) += nuvoton/

Please keep this in alphabetical order (I know the file isn't perfect).

diff --git a/drivers/clk/nuvoton/Makefile b/drivers/clk/nuvoton/Makefile new file mode 100644 index 0000000000..998e5329bb --- /dev/null +++ b/drivers/clk/nuvoton/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ARCH_NPCM8XX) += clk_npcm8xx.o diff --git a/drivers/clk/nuvoton/clk_npcm8xx.c b/drivers/clk/nuvoton/clk_npcm8xx.c new file mode 100644 index 0000000000..c547c47e82 --- /dev/null +++ b/drivers/clk/nuvoton/clk_npcm8xx.c @@ -0,0 +1,213 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2021 Nuvoton Technology.
• */

+#include <common.h> +#include <dm.h> +#include <clk-uclass.h> +#include <asm/types.h> +#include <asm/arch/clock.h>

Please add this include file in this patch.

+#include <asm/io.h> +#include <linux/delay.h> +#include <dt-bindings/clock/npcm845-clock.h>

Please order these correctly. See https://www.denx.de/wiki/U-Boot/CodingStyle#Include_files

+struct npcm_clk_priv {

• struct clk_ctl *regs;

+};

+enum regss {

perhaps "pll_id" or similar?

• PLL_0,
• PLL_1,
• PLL_2,
• PLL_CLKREF,

+};

+static u32 clk_get_pll_freq(struct clk_ctl *regs, enum regss pll) +{

• u32 pllval;
• u32 fin = EXT_CLOCK_FREQUENCY_KHZ; /* 25KHz */

Please get this from the device tree.

• u32 fout, nr, nf, no;
• switch (pll) {
• case PLL_0:

• pllval = readl(&regs->pllcon0);
  

• break;
  

• case PLL_1:

• pllval = readl(&regs->pllcon1);
  

• break;
  

• case PLL_2:

• pllval = readl(&regs->pllcon2);
  

• break;
  

• case PLL_CLKREF:

This is not used.

• default:

• return 0;
  

• }
• /* PLL Input Clock Divider */
• nr = (pllval >> PLLCON_INDV) & 0x1f;

With

#define PLLCON_INDV GENMASK(6, 0)

you can do

nr = FIELD_GET(PLLCON_INDV, pllval);

This applies to all your other register accesses.

• /* PLL VCO Output Clock Feedback Divider */
• nf = (pllval >> PLLCON_FBDV) & 0xfff;
• /* PLL Output Clock Divider 1 */
• no = ((pllval >> PLLCON_OTDV1) & 0x7) *

• ((pllval >> PLLCON_OTDV2) & 0x7);
  

• fout = ((10 * fin * nf) / (no * nr));

Can this overflow? Can you add a comment about that?

• return fout * 100;

Where do these 10 and 100 factors come from? Please combine them.

+}

+static u32 npcm_mmc_set_rate(struct clk_ctl *regs, ulong rate) +{

• u32 pll0_freq, div, sdhci_clk;
• /* To acquire PLL0 frequency. */
• pll0_freq = clk_get_pll_freq(regs, PLL_0);
• /* Calculate rounded up div to produce closest to

• * target output clock
  

• */
  

• div = (pll0_freq % rate == 0) ? (pll0_freq / rate) :

• 			(pll0_freq / rate) + 1;
  

div = DIV_ROUND_UP(pll0_freq, rate);

• writel((readl(&regs->clkdiv1) & ~(0x1f << CLKDIV1_MMCCKDIV))

•       | (div - 1) << CLKDIV1_MMCCKDIV, &regs->clkdiv1);
  

example of FIELD_PREP:

clkdiv1 = readl(&regs->clkdiv1); clkdiv1 &= ~CLKDIV1_MMCCKDIV; clkdiv1 |= FIELD_PREP(CLKDIV1_MMCCKDIV, div - 1); writel(clkdiv1, &regs->clkdiv1);

You don't have to break out each line, but please apply this to your register writes.

• /* Wait to the div to stabilize */
• udelay(100);
• /* Select PLL0 as source */
• writel((readl(&regs->clksel) & ~(0x3 << CLKSEL_SDCKSEL))

• | (CLKSEL_SDCKSEL_PLL0 << CLKSEL_SDCKSEL),
  

• &regs->clksel);
  

• sdhci_clk = pll0_freq / div;
• return sdhci_clk;

+}

+static u32 npcm_uart_set_rate(struct clk_ctl *regs, ulong rate) +{

• u32 src_freq, div;
• src_freq = clk_get_pll_freq(regs, PLL_2) / 2;
• div = (src_freq % rate == 0) ? (src_freq / rate) :

• 			(src_freq / rate) + 1;
  

• writel((readl(&regs->clkdiv1) & ~(0x1f << CLKDIV1_UARTDIV))

• | (div - 1) << CLKDIV1_UARTDIV, &regs->clkdiv1);
  

• writel((readl(&regs->clksel) & ~(3 << CLKSEL_UARTCKSEL))

• | (CLKSEL_UARTCKSEL_PLL2 << CLKSEL_UARTCKSEL),
  

• &regs->clksel);
  

• return (src_freq / div);

+}

+static ulong npcm_get_cpu_freq(struct clk_ctl *regs) +{

• ulong fout = 0;
• u32 clksel = readl(&regs->clksel) & (0x3 << CLKSEL_CPUCKSEL);
• if (clksel == CLKSEL_CPUCKSEL_PLL0)

Use a switch here.

• fout = (ulong)clk_get_pll_freq(regs, PLL_0);
  

• else if (clksel == CLKSEL_CPUCKSEL_PLL1)

• fout = (ulong)clk_get_pll_freq(regs, PLL_1);
  

• else if (clksel == CLKSEL_CPUCKSEL_CLKREF)

• fout = EXT_CLOCK_FREQUENCY_MHZ; /* FOUT is specified in MHz */
  

• else

• fout = EXT_CLOCK_FREQUENCY_MHZ; /* FOUT is specified in MHz */
  

• return fout;

+}

+static u32 npcm_get_apb_divisor(struct clk_ctl *regs, u32 apb) +{

• u32 apb_divisor = 2;

Just inline this. E.g.

return 2 << ...;

• /* AHBn div */
• apb_divisor = apb_divisor * (1 << ((readl(&regs->clkdiv1)

• 				>> CLKDIV1_CLK4DIV) & 0x3));
  

• switch (apb) {
• case APB2: /* APB divisor */

• apb_divisor = apb_divisor *
  

• 		(1 << ((readl(&regs->clkdiv2)
  

• 			>> CLKDIV2_APB2CKDIV) & 0x3));
  

• break;
  

• case APB5: /* APB divisor */

• apb_divisor = apb_divisor *
  

• 		(1 << ((readl(&regs->clkdiv2)
  

• 			>> CLKDIV2_APB5CKDIV) & 0x3));
  

• break;
  

• default:

• apb_divisor = 0xFFFFFFFF;
  

Isn't getting here a bug?

• break;
  

• }
• return apb_divisor;

+}

+static ulong npcm_clk_get_rate(struct clk *clk) +{

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

• return npcm_get_cpu_freq(priv->regs) /
  

• 	npcm_get_apb_divisor(priv->regs, APB2);
  

• case CLK_APB5:

• return npcm_get_cpu_freq(priv->regs) /
  

• 	npcm_get_apb_divisor(priv->regs, APB5);
  

I think you can use a more modular approach here:

struct clk parent; switch (clk->id) { case CLK_APB2: parent.id = CLK_AHB; ret = clk_request(clk->dev, &parent); if (ret) return ret; fin = clk_get_rate(&parent); if (IS_ERR_VALUE(fin)) return fin;

return fin / FIELD_GET(CLKDIV2_APB2CKDIV, readl(&regs->clkdiv2)); }

And of course you can go further and create some arrays to hold those parameters if you like.

This switch statement should also return -ENOSYS in the default case.

• }
• return 0;

+}

+static ulong npcm_clk_set_rate(struct clk *clk, ulong rate) +{

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

• return npcm_mmc_set_rate(priv->regs, rate);
  

• case CLK_UART:

• return npcm_uart_set_rate(priv->regs, rate);
  

• default:

• return -ENOENT;
  

-ENOSYS

• }
• return 0;

+}

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

• struct npcm_clk_priv *priv = dev_get_priv(dev);
• void *base;
• base = dev_read_addr_ptr(dev);
• if (!base)

• return -ENOENT;
  

• priv->regs = (struct clk_ctl *)base;

You can directly assign to regs. And there is no need to cast here, since base is a void pointer.

• return 0;

+}

+static struct clk_ops npcm_clk_ops = {

• .get_rate = npcm_clk_get_rate,
• .set_rate = npcm_clk_set_rate,

Please add a .request callback which enforces the max clock id.

--Sean

+};

+static const struct udevice_id npcm_clk_ids[] = {

• { .compatible = "nuvoton,npcm845-clock" },
• { }

+};

+U_BOOT_DRIVER(clk_npcm) = {

• .name = "clk_npcm",
• .id = UCLASS_CLK,
• .of_match = npcm_clk_ids,
• .ops = &npcm_clk_ops,
• .priv_auto = sizeof(struct npcm_clk_priv),
• .probe = npcm_clk_probe,

+}; diff --git a/include/dt-bindings/clock/npcm845-clock.h b/include/dt-bindings/clock/npcm845-clock.h new file mode 100644 index 0000000000..fca10d39c8 --- /dev/null +++ b/include/dt-bindings/clock/npcm845-clock.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0+ */

+#ifndef _DT_BINDINGS_NPCM845_CLOCK_H_ +#define _DT_BINDINGS_NPCM845_CLOCK_H_

+#define CLK_TIMER 0 +#define CLK_UART 1 +#define CLK_SD 2 +#define CLK_EMMC 3 +#define CLK_APB1 4 +#define CLK_APB2 5 +#define CLK_APB3 6 +#define CLK_APB4 7 +#define CLK_APB5 8 +#define CLK_AHB 9

+#endif