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

[Sean Anderson <seanga2@gmail.com>]

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
>