[07/13] clk: exynos: Add Samsung clock framework

Heavily based on Linux kernel Samsung clock framework, with some changes
to accommodate the differences in U-Boot CCF implementation. It's also
quite minimal as compared to the Linux version.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/exynos/Makefile  |   9 +-
 drivers/clk/exynos/clk-pll.c | 167 +++++++++++++++++++++++++
 drivers/clk/exynos/clk-pll.h |  23 ++++
 drivers/clk/exynos/clk.c     | 121 +++++++++++++++++++
 drivers/clk/exynos/clk.h     | 228 +++++++++++++++++++++++++++++++++++
 5 files changed, 546 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/exynos/clk-pll.c
 create mode 100644 drivers/clk/exynos/clk-pll.h
 create mode 100644 drivers/clk/exynos/clk.c
 create mode 100644 drivers/clk/exynos/clk.h

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Sam Protsenko
> Sent: Wednesday, December 13, 2023 12:17 PM
> To: Minkyu Kang <mk7.kang@samsung.com>; Tom Rini <trini@konsulko.com>;
> Lukasz Majewski <lukma@denx.de>; Sean Anderson <seanga2@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>; Heinrich Schuchardt
> <xypron.glpk@gmx.de>; u-boot@lists.denx.de
> Subject: [PATCH 07/13] clk: exynos: Add Samsung clock framework
> 
> Heavily based on Linux kernel Samsung clock framework, with some changes
> to accommodate the differences in U-Boot CCF implementation. It's also
> quite minimal as compared to the Linux version.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/exynos/Makefile  |   9 +-
>  drivers/clk/exynos/clk-pll.c | 167 +++++++++++++++++++++++++
>  drivers/clk/exynos/clk-pll.h |  23 ++++
>  drivers/clk/exynos/clk.c     | 121 +++++++++++++++++++
>  drivers/clk/exynos/clk.h     | 228 +++++++++++++++++++++++++++++++++++
>  5 files changed, 546 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/clk/exynos/clk-pll.c
>  create mode 100644 drivers/clk/exynos/clk-pll.h
>  create mode 100644 drivers/clk/exynos/clk.c
>  create mode 100644 drivers/clk/exynos/clk.h
> 
> diff --git a/drivers/clk/exynos/Makefile b/drivers/clk/exynos/Makefile
> index 7faf238571ef..04c5b9a39e16 100644
> --- a/drivers/clk/exynos/Makefile
> +++ b/drivers/clk/exynos/Makefile
> @@ -1,6 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  #
>  # Copyright (C) 2016 Samsung Electronics
> -# Thomas Abraham <thomas.ab@samsung.com>
> +# Copyright (C) 2023 Linaro Ltd.
> +#
> +# Authors:
> +#   Thomas Abraham <thomas.ab@samsung.com>
> +#   Sam Protsenko <semen.protsenko@linaro.org>
> 
> -obj-$(CONFIG_CLK_EXYNOS7420)	+= clk-exynos7420.o
> +obj-$(CONFIG_$(SPL_TPL_)CLK_CCF)	+= clk.o clk-pll.o
> +obj-$(CONFIG_CLK_EXYNOS7420)		+= clk-exynos7420.o
> diff --git a/drivers/clk/exynos/clk-pll.c b/drivers/clk/exynos/clk-pll.c
> new file mode 100644
> index 000000000000..9e496ff83aaf
> --- /dev/null
> +++ b/drivers/clk/exynos/clk-pll.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2016 Samsung Electronics
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Authors:
> + *   Thomas Abraham <thomas.ab@exynos.com>

Need to correct Thomas's email to samsung.com if you want to keep his
original credit even though his e-mail was already stale since he left the
company.

> + *   Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * This file contains the utility functions to register the pll clocks.
> + */
> +
> +#include <asm/io.h>
> +#include <div64.h>
> +#include <malloc.h>
> +#include <clk-uclass.h>
> +#include <dm/device.h>
> +#include <clk.h>
> +#include "clk.h"
> +
> +#define UBOOT_DM_CLK_SAMSUNG_PLL0822X	"samsung_clk_pll0822x"
> +#define UBOOT_DM_CLK_SAMSUNG_PLL0831X	"samsung_clk_pll0831x"
> +
> +struct samsung_clk_pll {
> +	struct clk		clk;
> +	void __iomem		*con_reg;
> +	enum samsung_pll_type	type;
> +};
> +
> +#define to_clk_pll(_clk) container_of(_clk, struct samsung_clk_pll, clk)
> +
> +/*
> + * PLL0822x Clock Type
> + */
> +
> +#define PLL0822X_MDIV_MASK		0x3ff
> +#define PLL0822X_PDIV_MASK		0x3f
> +#define PLL0822X_SDIV_MASK		0x7
> +#define PLL0822X_MDIV_SHIFT		16
> +#define PLL0822X_PDIV_SHIFT		8
> +#define PLL0822X_SDIV_SHIFT		0
> +
> +static unsigned long samsung_pll0822x_recalc_rate(struct clk *clk)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(clk);
> +	u32 mdiv, pdiv, sdiv, pll_con3;
> +	u64 fvco = clk_get_parent_rate(clk);
> +
> +	pll_con3 = readl_relaxed(pll->con_reg);
> +	mdiv = (pll_con3 >> PLL0822X_MDIV_SHIFT) & PLL0822X_MDIV_MASK;
> +	pdiv = (pll_con3 >> PLL0822X_PDIV_SHIFT) & PLL0822X_PDIV_MASK;
> +	sdiv = (pll_con3 >> PLL0822X_SDIV_SHIFT) & PLL0822X_SDIV_MASK;
> +
> +	fvco *= mdiv;
> +	do_div(fvco, (pdiv << sdiv));
> +	return (unsigned long)fvco;
> +}
> +
> +static const struct clk_ops samsung_pll0822x_clk_min_ops = {
> +	.get_rate = samsung_pll0822x_recalc_rate,
> +};
> +
> +/*
> + * PLL0831x Clock Type
> + */
> +
> +#define PLL0831X_KDIV_MASK		0xffff
> +#define PLL0831X_MDIV_MASK		0x1ff
> +#define PLL0831X_PDIV_MASK		0x3f
> +#define PLL0831X_SDIV_MASK		0x7
> +#define PLL0831X_MDIV_SHIFT		16
> +#define PLL0831X_PDIV_SHIFT		8
> +#define PLL0831X_SDIV_SHIFT		0
> +#define PLL0831X_KDIV_SHIFT		0
> +
> +static unsigned long samsung_pll0831x_recalc_rate(struct clk *clk)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(clk);
> +	u32 mdiv, pdiv, sdiv, pll_con3, pll_con5;
> +	s16 kdiv;
> +	u64 fvco = clk_get_parent_rate(clk);
> +
> +	pll_con3 = readl_relaxed(pll->con_reg);
> +	pll_con5 = readl_relaxed(pll->con_reg + 8);
> +	mdiv = (pll_con3 >> PLL0831X_MDIV_SHIFT) & PLL0831X_MDIV_MASK;
> +	pdiv = (pll_con3 >> PLL0831X_PDIV_SHIFT) & PLL0831X_PDIV_MASK;
> +	sdiv = (pll_con3 >> PLL0831X_SDIV_SHIFT) & PLL0831X_SDIV_MASK;
> +	kdiv = (s16)((pll_con5 >> PLL0831X_KDIV_SHIFT) &
> PLL0831X_KDIV_MASK);
> +
> +	fvco *= (mdiv << 16) + kdiv;
> +	do_div(fvco, (pdiv << sdiv));
> +	fvco >>= 16;
> +
> +	return (unsigned long)fvco;
> +}
> +
> +static const struct clk_ops samsung_pll0831x_clk_min_ops = {
> +	.get_rate = samsung_pll0831x_recalc_rate,
> +};
> +
> +static struct clk *_samsung_clk_register_pll(void __iomem *base,
> +					const struct samsung_pll_clock
*pll_clk)
> +{
> +	struct samsung_clk_pll *pll;
> +	struct clk *clk;
> +	const char *drv_name;
> +	int ret;
> +
> +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> +	if (!pll)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pll->con_reg = base + pll_clk->con_offset;
> +	pll->type = pll_clk->type;
> +	clk = &pll->clk;
> +	clk->flags = pll_clk->flags;
> +
> +	switch (pll_clk->type) {
> +	case pll_0822x:
> +		drv_name = UBOOT_DM_CLK_SAMSUNG_PLL0822X;
> +		break;
> +	case pll_0831x:
> +		drv_name = UBOOT_DM_CLK_SAMSUNG_PLL0831X;
> +		break;
> +	default:
> +		kfree(pll);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	ret = clk_register(clk, drv_name, pll_clk->name, pll_clk-
> >parent_name);
> +	if (ret) {
> +		kfree(pll);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return clk;
> +}
> +
> +void samsung_clk_register_pll(void __iomem *base,
> +			      const struct samsung_pll_clock *clk_list,
> +			      unsigned int nr_clk)
> +{
> +	unsigned int cnt;
> +
> +	for (cnt = 0; cnt < nr_clk; cnt++) {
> +		struct clk *clk;
> +		const struct samsung_pll_clock *pll_clk;
> +
> +		pll_clk = &clk_list[cnt];
> +		clk = _samsung_clk_register_pll(base, pll_clk);
> +		clk_dm(pll_clk->id, clk);
> +	}
> +}
> +
> +U_BOOT_DRIVER(samsung_pll0822x_clk) = {
> +	.name	= UBOOT_DM_CLK_SAMSUNG_PLL0822X,
> +	.id	= UCLASS_CLK,
> +	.ops	= &samsung_pll0822x_clk_min_ops,
> +	.flags	= DM_FLAG_PRE_RELOC,
> +};
> +
> +U_BOOT_DRIVER(samsung_pll0831x_clk) = {
> +	.name	= UBOOT_DM_CLK_SAMSUNG_PLL0831X,
> +	.id	= UCLASS_CLK,
> +	.ops	= &samsung_pll0831x_clk_min_ops,
> +	.flags	= DM_FLAG_PRE_RELOC,
> +};
> diff --git a/drivers/clk/exynos/clk-pll.h b/drivers/clk/exynos/clk-pll.h
> new file mode 100644
> index 000000000000..3b477369aeb8
> --- /dev/null
> +++ b/drivers/clk/exynos/clk-pll.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2016 Samsung Electronics
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Authors:
> + *   Thomas Abraham <thomas.ab@exynos.com>

Ditto.

Othewise,
Reviewed-by: Chanho Park <chanho61.park@samsung.com>

Hi,


2023년 12월 13일 (수) 12:27, Sam Protsenko <semen.protsenko@linaro.org>님이 작성:

> Heavily based on Linux kernel Samsung clock framework, with some changes
> to accommodate the differences in U-Boot CCF implementation. It's also
> quite minimal as compared to the Linux version.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/exynos/Makefile  |   9 +-
>  drivers/clk/exynos/clk-pll.c | 167 +++++++++++++++++++++++++
>  drivers/clk/exynos/clk-pll.h |  23 ++++
>  drivers/clk/exynos/clk.c     | 121 +++++++++++++++++++
>  drivers/clk/exynos/clk.h     | 228 +++++++++++++++++++++++++++++++++++
>  5 files changed, 546 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/clk/exynos/clk-pll.c
>  create mode 100644 drivers/clk/exynos/clk-pll.h
>  create mode 100644 drivers/clk/exynos/clk.c
>  create mode 100644 drivers/clk/exynos/clk.h
>
> diff --git a/drivers/clk/exynos/Makefile b/drivers/clk/exynos/Makefile
> index 7faf238571ef..04c5b9a39e16 100644
> --- a/drivers/clk/exynos/Makefile
> +++ b/drivers/clk/exynos/Makefile
> @@ -1,6 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  #
>  # Copyright (C) 2016 Samsung Electronics
> -# Thomas Abraham <thomas.ab@samsung.com>
> +# Copyright (C) 2023 Linaro Ltd.
> +#
> +# Authors:
> +#   Thomas Abraham <thomas.ab@samsung.com>
> +#   Sam Protsenko <semen.protsenko@linaro.org>
>
> -obj-$(CONFIG_CLK_EXYNOS7420)   += clk-exynos7420.o
> +obj-$(CONFIG_$(SPL_TPL_)CLK_CCF)       += clk.o clk-pll.o
> +obj-$(CONFIG_CLK_EXYNOS7420)           += clk-exynos7420.o
> diff --git a/drivers/clk/exynos/clk-pll.c b/drivers/clk/exynos/clk-pll.c
> new file mode 100644
> index 000000000000..9e496ff83aaf
> --- /dev/null
> +++ b/drivers/clk/exynos/clk-pll.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2016 Samsung Electronics
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Authors:
> + *   Thomas Abraham <thomas.ab@exynos.com>
> + *   Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * This file contains the utility functions to register the pll clocks.
> + */
> +
> +#include <asm/io.h>
> +#include <div64.h>
> +#include <malloc.h>
> +#include <clk-uclass.h>
> +#include <dm/device.h>
> +#include <clk.h>
> +#include "clk.h"
> +
> +#define UBOOT_DM_CLK_SAMSUNG_PLL0822X  "samsung_clk_pll0822x"
> +#define UBOOT_DM_CLK_SAMSUNG_PLL0831X  "samsung_clk_pll0831x"
> +
> +struct samsung_clk_pll {
> +       struct clk              clk;
> +       void __iomem            *con_reg;
> +       enum samsung_pll_type   type;
> +};
> +
> +#define to_clk_pll(_clk) container_of(_clk, struct samsung_clk_pll, clk)
> +
> +/*
> + * PLL0822x Clock Type
> + */
> +
> +#define PLL0822X_MDIV_MASK             0x3ff
> +#define PLL0822X_PDIV_MASK             0x3f
> +#define PLL0822X_SDIV_MASK             0x7
> +#define PLL0822X_MDIV_SHIFT            16
> +#define PLL0822X_PDIV_SHIFT            8
> +#define PLL0822X_SDIV_SHIFT            0
> +
> +static unsigned long samsung_pll0822x_recalc_rate(struct clk *clk)
> +{
> +       struct samsung_clk_pll *pll = to_clk_pll(clk);
> +       u32 mdiv, pdiv, sdiv, pll_con3;
> +       u64 fvco = clk_get_parent_rate(clk);
> +
> +       pll_con3 = readl_relaxed(pll->con_reg);
> +       mdiv = (pll_con3 >> PLL0822X_MDIV_SHIFT) & PLL0822X_MDIV_MASK;
> +       pdiv = (pll_con3 >> PLL0822X_PDIV_SHIFT) & PLL0822X_PDIV_MASK;
> +       sdiv = (pll_con3 >> PLL0822X_SDIV_SHIFT) & PLL0822X_SDIV_MASK;
> +
> +       fvco *= mdiv;
> +       do_div(fvco, (pdiv << sdiv));
> +       return (unsigned long)fvco;
> +}
> +
> +static const struct clk_ops samsung_pll0822x_clk_min_ops = {
> +       .get_rate = samsung_pll0822x_recalc_rate,
> +};
> +
> +/*
> + * PLL0831x Clock Type
> + */
> +
> +#define PLL0831X_KDIV_MASK             0xffff
> +#define PLL0831X_MDIV_MASK             0x1ff
> +#define PLL0831X_PDIV_MASK             0x3f
> +#define PLL0831X_SDIV_MASK             0x7
> +#define PLL0831X_MDIV_SHIFT            16
> +#define PLL0831X_PDIV_SHIFT            8
> +#define PLL0831X_SDIV_SHIFT            0
> +#define PLL0831X_KDIV_SHIFT            0
> +
> +static unsigned long samsung_pll0831x_recalc_rate(struct clk *clk)
> +{
> +       struct samsung_clk_pll *pll = to_clk_pll(clk);
> +       u32 mdiv, pdiv, sdiv, pll_con3, pll_con5;
> +       s16 kdiv;
> +       u64 fvco = clk_get_parent_rate(clk);
> +
> +       pll_con3 = readl_relaxed(pll->con_reg);
> +       pll_con5 = readl_relaxed(pll->con_reg + 8);
> +       mdiv = (pll_con3 >> PLL0831X_MDIV_SHIFT) & PLL0831X_MDIV_MASK;
> +       pdiv = (pll_con3 >> PLL0831X_PDIV_SHIFT) & PLL0831X_PDIV_MASK;
> +       sdiv = (pll_con3 >> PLL0831X_SDIV_SHIFT) & PLL0831X_SDIV_MASK;
> +       kdiv = (s16)((pll_con5 >> PLL0831X_KDIV_SHIFT) &
> PLL0831X_KDIV_MASK);
> +
> +       fvco *= (mdiv << 16) + kdiv;
> +       do_div(fvco, (pdiv << sdiv));
> +       fvco >>= 16;
> +
> +       return (unsigned long)fvco;
> +}
> +
> +static const struct clk_ops samsung_pll0831x_clk_min_ops = {
> +       .get_rate = samsung_pll0831x_recalc_rate,
> +};
> +
> +static struct clk *_samsung_clk_register_pll(void __iomem *base,
> +                                       const struct samsung_pll_clock
> *pll_clk)
> +{
> +       struct samsung_clk_pll *pll;
> +       struct clk *clk;
> +       const char *drv_name;
> +       int ret;
> +
> +       pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> +       if (!pll)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pll->con_reg = base + pll_clk->con_offset;
> +       pll->type = pll_clk->type;
> +       clk = &pll->clk;
> +       clk->flags = pll_clk->flags;
> +
> +       switch (pll_clk->type) {
> +       case pll_0822x:
> +               drv_name = UBOOT_DM_CLK_SAMSUNG_PLL0822X;
> +               break;
> +       case pll_0831x:
> +               drv_name = UBOOT_DM_CLK_SAMSUNG_PLL0831X;
> +               break;
> +       default:
> +               kfree(pll);
> +               return ERR_PTR(-ENODEV);
> +       }
> +
> +       ret = clk_register(clk, drv_name, pll_clk->name,
> pll_clk->parent_name);
> +       if (ret) {
> +               kfree(pll);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return clk;
> +}
> +
> +void samsung_clk_register_pll(void __iomem *base,
> +                             const struct samsung_pll_clock *clk_list,
> +                             unsigned int nr_clk)
> +{
> +       unsigned int cnt;
> +
> +       for (cnt = 0; cnt < nr_clk; cnt++) {
> +               struct clk *clk;
> +               const struct samsung_pll_clock *pll_clk;
> +
> +               pll_clk = &clk_list[cnt];
> +               clk = _samsung_clk_register_pll(base, pll_clk);
> +               clk_dm(pll_clk->id, clk);
> +       }
> +}
> +
> +U_BOOT_DRIVER(samsung_pll0822x_clk) = {
> +       .name   = UBOOT_DM_CLK_SAMSUNG_PLL0822X,
> +       .id     = UCLASS_CLK,
> +       .ops    = &samsung_pll0822x_clk_min_ops,
> +       .flags  = DM_FLAG_PRE_RELOC,
> +};
> +
> +U_BOOT_DRIVER(samsung_pll0831x_clk) = {
> +       .name   = UBOOT_DM_CLK_SAMSUNG_PLL0831X,
> +       .id     = UCLASS_CLK,
> +       .ops    = &samsung_pll0831x_clk_min_ops,
> +       .flags  = DM_FLAG_PRE_RELOC,
> +};
> diff --git a/drivers/clk/exynos/clk-pll.h b/drivers/clk/exynos/clk-pll.h
> new file mode 100644
> index 000000000000..3b477369aeb8
> --- /dev/null
> +++ b/drivers/clk/exynos/clk-pll.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2016 Samsung Electronics
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Authors:
> + *   Thomas Abraham <thomas.ab@exynos.com>
> + *   Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * Common Clock Framework support for all PLL's in Samsung platforms.
> + */
> +
> +#ifndef __EXYNOS_CLK_PLL_H
> +#define __EXYNOS_CLK_PLL_H
> +
> +#include <linux/clk-provider.h>
> +
> +enum samsung_pll_type {
> +       pll_0822x,
> +       pll_0831x,


why don't you modify to uppercase?


> +};
> +
> +#endif /* __EXYNOS_CLK_PLL_H */
> diff --git a/drivers/clk/exynos/clk.c b/drivers/clk/exynos/clk.c
> new file mode 100644
> index 000000000000..430767f072d8
> --- /dev/null
> +++ b/drivers/clk/exynos/clk.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Linaro Ltd.
> + * Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * This file includes utility functions to register clocks to common
> + * clock framework for Samsung platforms.
> + */
> +
> +#include <dm.h>
> +#include "clk.h"
> +
> +void samsung_clk_register_mux(void __iomem *base,
> +                             const struct samsung_mux_clock *clk_list,
> +                             unsigned int nr_clk)
> +{
> +       unsigned int cnt;
> +
> +       for (cnt = 0; cnt < nr_clk; cnt++) {
> +               struct clk *clk;
> +               const struct samsung_mux_clock *m;


wouldn't it be better if use a more meaningful name like mux?


> +
> +               m = &clk_list[cnt];


Is there any possibility that the value is null or wrong (e.g. overflow)


> +               clk = clk_register_mux(NULL, m->name, m->parent_names,
> +                       m->num_parents, m->flags, base + m->offset,
> m->shift,
> +                       m->width, m->mux_flags);
> +               clk_dm(m->id, clk);
> +       }
> +}
> +
> +void samsung_clk_register_div(void __iomem *base,
> +                             const struct samsung_div_clock *clk_list,
> +                             unsigned int nr_clk)
> +{
> +       unsigned int cnt;
> +
> +       for (cnt = 0; cnt < nr_clk; cnt++) {
> +               struct clk *clk;
> +               const struct samsung_div_clock *d;
> +
> +               d = &clk_list[cnt];
> +               clk = clk_register_divider(NULL, d->name, d->parent_name,
> +                       d->flags, base + d->offset, d->shift,
> +                       d->width, d->div_flags);
> +               clk_dm(d->id, clk);
> +       }
> +}
> +
> +void samsung_clk_register_gate(void __iomem *base,
> +                              const struct samsung_gate_clock *clk_list,
> +                              unsigned int nr_clk)
> +{
> +       unsigned int cnt;
> +
> +       for (cnt = 0; cnt < nr_clk; cnt++) {
> +               struct clk *clk;
> +               const struct samsung_gate_clock *g;
> +
> +               g = &clk_list[cnt];
> +               clk = clk_register_gate(NULL, g->name, g->parent_name,
> +                       g->flags, base + g->offset, g->bit_idx,
> +                       g->gate_flags, NULL);
> +               clk_dm(g->id, clk);
> +       }
> +}
> +
> +typedef void (*samsung_clk_register_fn)(void __iomem *base,
> +                                       const void *clk_list,
> +                                       unsigned int nr_clk);
> +
> +static const samsung_clk_register_fn samsung_clk_register_fns[] = {
> +       [S_CLK_MUX]     =
> (samsung_clk_register_fn)samsung_clk_register_mux,
> +       [S_CLK_DIV]     =
> (samsung_clk_register_fn)samsung_clk_register_div,
> +       [S_CLK_GATE]    =
> (samsung_clk_register_fn)samsung_clk_register_gate,
> +       [S_CLK_PLL]     =
> (samsung_clk_register_fn)samsung_clk_register_pll,
> +};
> +
> +/**
> + * samsung_cmu_register_clocks() - Register provided clock groups
> + * @base: Base address of CMU registers
> + * @clk_groups: list of clock groups
> + * @nr_groups: count of clock groups in @clk_groups
> + *
> + * Having the array of clock groups @clk_groups makes it possible to keep
> a
> + * correct clocks registration order.
> + */
> +void samsung_cmu_register_clocks(void __iomem *base,
> +                                const struct samsung_clk_group
> *clk_groups,
> +                                unsigned int nr_groups)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < nr_groups; i++) {
> +               const struct samsung_clk_group *g = &clk_groups[i];
> +
> +               samsung_clk_register_fns[g->type](base, g->clk_list,
> g->nr_clk);
> +       }
> +}
> +
> +/**
> + * samsung_cmu_register_one - Register all CMU clocks
> + * @dev: CMU device
> + * @clk_groups: list of CMU clock groups
> + * @nr_groups: count of CMU clock groups in @clk_groups
> + *
> + * Return: 0 on success or negative value on error.
> + */
> +int samsung_cmu_register_one(struct udevice *dev,
> +                            const struct samsung_clk_group *clk_groups,
> +                            unsigned int nr_groups)
> +{
> +       void __iomem *base;
> +
> +       base = dev_read_addr_ptr(dev);
> +       if (!base)
> +               return -EINVAL;
> +
> +       samsung_cmu_register_clocks(base, clk_groups, nr_groups);
> +
> +       return 0;
> +}
> diff --git a/drivers/clk/exynos/clk.h b/drivers/clk/exynos/clk.h
> new file mode 100644
> index 000000000000..91a51b877a63
> --- /dev/null
> +++ b/drivers/clk/exynos/clk.h
> @@ -0,0 +1,228 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Linaro Ltd.
> + * Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * Common Clock Framework support for all Samsung platforms.
> + */
> +
> +#ifndef __EXYNOS_CLK_H
> +#define __EXYNOS_CLK_H
> +
> +#include <errno.h>
> +#include <linux/clk-provider.h>
> +#include "clk-pll.h"
> +
> +/**
> + * struct samsung_mux_clock - information about mux clock
> + * @id: platform specific id of the clock
> + * @name: name of this mux clock
> + * @parent_names: array of pointer to parent clock names
> + * @num_parents: number of parents listed in @parent_names
> + * @flags: optional flags for basic clock
> + * @offset: offset of the register for configuring the mux
> + * @shift: starting bit location of the mux control bit-field in @reg
> + * @width: width of the mux control bit-field in @reg
> + * @mux_flags: flags for mux-type clock
> + */
> +struct samsung_mux_clock {
> +       unsigned int            id;
> +       const char              *name;
> +       const char              * const *parent_names;
> +       u8                      num_parents;
> +       unsigned long           flags;
> +       unsigned long           offset;
> +       u8                      shift;
> +       u8                      width;
> +       u8                      mux_flags;
> +};
> +
> +#define PNAME(x) static const char * const x[]
> +
> +#define __MUX(_id, cname, pnames, o, s, w, f, mf)                      \
> +       {                                                               \
> +               .id             = _id,                                  \
> +               .name           = cname,                                \
> +               .parent_names   = pnames,                               \
> +               .num_parents    = ARRAY_SIZE(pnames),                   \
> +               .flags          = (f) | CLK_SET_RATE_NO_REPARENT,       \
> +               .offset         = o,                                    \
> +               .shift          = s,                                    \
> +               .width          = w,                                    \
> +               .mux_flags      = mf,                                   \
> +       }
> +
> +#define MUX(_id, cname, pnames, o, s, w)                               \
> +       __MUX(_id, cname, pnames, o, s, w, 0, 0)
> +
> +#define MUX_F(_id, cname, pnames, o, s, w, f, mf)                      \
> +       __MUX(_id, cname, pnames, o, s, w, f, mf)
> +
> +/**
> + * struct samsung_div_clock - information about div clock
> + * @id: platform specific id of the clock
> + * @name: name of this div clock
> + * @parent_name: name of the parent clock
> + * @flags: optional flags for basic clock
> + * @offset: offset of the register for configuring the div
> + * @shift: starting bit location of the div control bit-field in @reg
> + * @width: width of the bitfield
> + * @div_flags: flags for div-type clock
> + */
> +struct samsung_div_clock {
> +       unsigned int            id;
> +       const char              *name;
> +       const char              *parent_name;
> +       unsigned long           flags;
> +       unsigned long           offset;
> +       u8                      shift;
> +       u8                      width;
> +       u8                      div_flags;
> +};
> +
> +#define __DIV(_id, cname, pname, o, s, w, f, df)       \
> +       {                                               \
> +               .id             = _id,                  \
> +               .name           = cname,                \
> +               .parent_name    = pname,                \
> +               .flags          = f,                    \
> +               .offset         = o,                    \
> +               .shift          = s,                    \
> +               .width          = w,                    \
> +               .div_flags      = df,                   \
> +       }
> +
> +#define DIV(_id, cname, pname, o, s, w)                        \
> +       __DIV(_id, cname, pname, o, s, w, 0, 0)
> +
> +#define DIV_F(_id, cname, pname, o, s, w, f, df)       \
> +       __DIV(_id, cname, pname, o, s, w, f, df)
> +
> +/**
> + * struct samsung_gate_clock - information about gate clock
> + * @id: platform specific id of the clock
> + * @name: name of this gate clock
> + * @parent_name: name of the parent clock
> + * @flags: optional flags for basic clock
> + * @offset: offset of the register for configuring the gate
> + * @bit_idx: bit index of the gate control bit-field in @reg
> + * @gate_flags: flags for gate-type clock
> + */
> +struct samsung_gate_clock {
> +       unsigned int            id;
> +       const char              *name;
> +       const char              *parent_name;
> +       unsigned long           flags;
> +       unsigned long           offset;
> +       u8                      bit_idx;
> +       u8                      gate_flags;
> +};
> +
> +#define __GATE(_id, cname, pname, o, b, f, gf)                 \
> +       {                                                       \
> +               .id             = _id,                          \
> +               .name           = cname,                        \
> +               .parent_name    = pname,                        \
> +               .flags          = f,                            \
> +               .offset         = o,                            \
> +               .bit_idx        = b,                            \
> +               .gate_flags     = gf,                           \
> +       }
> +
> +#define GATE(_id, cname, pname, o, b, f, gf)                   \
> +       __GATE(_id, cname, pname, o, b, f, gf)
> +
> +/**
> + * struct samsung_pll_clock - information about pll clock
> + * @id: platform specific id of the clock
> + * @name: name of this pll clock
> + * @parent_name: name of the parent clock
> + * @flags: optional flags for basic clock
> + * @con_offset: offset of the register for configuring the PLL
> + * @type: type of PLL to be registered
> + */
> +struct samsung_pll_clock {
> +       unsigned int            id;
> +       const char              *name;
> +       const char              *parent_name;
> +       unsigned long           flags;
> +       int                     con_offset;
> +       enum samsung_pll_type   type;
> +};
> +
> +#define PLL(_typ, _id, _name, _pname, _con)            \
> +       {                                               \
> +               .id             = _id,                  \
> +               .name           = _name,                \
> +               .parent_name    = _pname,               \
> +               .flags          = CLK_GET_RATE_NOCACHE, \
> +               .con_offset     = _con,                 \
> +               .type           = _typ,                 \
> +       }
> +
> +enum samsung_clock_type {
> +       S_CLK_MUX,
> +       S_CLK_DIV,
> +       S_CLK_GATE,
> +       S_CLK_PLL,
> +};
> +
> +/**
> + * struct samsung_clock_group - contains a list of clocks of one type
> + * @type: type of clocks this structure contains
> + * @clk_list: list of clocks
> + * @nr_clk: count of clocks in @clk_list
> + */
> +struct samsung_clk_group {
> +       enum samsung_clock_type type;
> +       const void *clk_list;
> +       unsigned int nr_clk;
> +};
> +
> +void samsung_clk_register_mux(void __iomem *base,
> +                             const struct samsung_mux_clock *clk_list,
> +                             unsigned int nr_clk);
> +void samsung_clk_register_div(void __iomem *base,
> +                             const struct samsung_div_clock *clk_list,
> +                             unsigned int nr_clk);
> +void samsung_clk_register_gate(void __iomem *base,
> +                              const struct samsung_gate_clock *clk_list,
> +                              unsigned int nr_clk);
> +void samsung_clk_register_pll(void __iomem *base,
> +                             const struct samsung_pll_clock *clk_list,
> +                             unsigned int nr_clk);
> +
> +void samsung_cmu_register_clocks(void __iomem *base,
> +                                const struct samsung_clk_group
> *clk_groups,
> +                                unsigned int nr_groups);
> +int samsung_cmu_register_one(struct udevice *dev,
> +                            const struct samsung_clk_group *clk_groups,
> +                            unsigned int nr_groups);
> +
> +/**
> + * samsung_register_cmu - Register CMU clocks ensuring parent CMU is
> present
> + * @dev: CMU device
> + * @clk_groups: list of CMU clock groups
> + * @parent_drv: name of parent CMU driver
> + *
> + * Register provided CMU clocks, but make sure CMU_TOP driver is
> instantiated
> + * first.
> + *
> + * Return: 0 on success or negative value on error.
> + */
> +#define samsung_register_cmu(dev, clk_groups, parent_drv)              \
> +({                                                                     \
> +       struct udevice *__parent;                                       \
> +       int __ret;                                                      \
> +                                                                       \
> +       __ret = uclass_get_device_by_driver(UCLASS_CLK,                 \
> +               DM_DRIVER_GET(parent_drv), &__parent);                  \
> +       if (__ret || !__parent)                                         \
> +               __ret = -ENOENT;                                        \
> +       else                                                            \
> +               __ret = samsung_cmu_register_one(dev, clk_groups,       \
> +                       ARRAY_SIZE(clk_groups));                        \
> +       __ret;                                                          \
> +})
> +
> +#endif /* __EXYNOS_CLK_H */
> --
> 2.39.2
>

Thanks.
Minkyu Kang.

On Wed, Dec 27, 2023 at 3:12 AM Minkyu Kang <promsoft@gmail.com> wrote:
>
> Hi,
>
>
> 2023년 12월 13일 (수) 12:27, Sam Protsenko <semen.protsenko@linaro.org>님이 작성:
>>
>> Heavily based on Linux kernel Samsung clock framework, with some changes
>> to accommodate the differences in U-Boot CCF implementation. It's also
>> quite minimal as compared to the Linux version.
>>
>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>> ---

[snip]

>> diff --git a/drivers/clk/exynos/clk-pll.h b/drivers/clk/exynos/clk-pll.h
>> new file mode 100644
>> index 000000000000..3b477369aeb8
>> --- /dev/null
>> +++ b/drivers/clk/exynos/clk-pll.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (C) 2016 Samsung Electronics
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Authors:
>> + *   Thomas Abraham <thomas.ab@exynos.com>
>> + *   Sam Protsenko <semen.protsenko@linaro.org>
>> + *
>> + * Common Clock Framework support for all PLL's in Samsung platforms.
>> + */
>> +
>> +#ifndef __EXYNOS_CLK_PLL_H
>> +#define __EXYNOS_CLK_PLL_H
>> +
>> +#include <linux/clk-provider.h>
>> +
>> +enum samsung_pll_type {
>> +       pll_0822x,
>> +       pll_0831x,
>
>
> why don't you modify to uppercase?
>

That code was basically copied over from Linux kernel (from
drivers/clk/samsung/clk-pll.h file). I'm trying to keep it as close to
the original as possible, to ease any possible backporting in future.
Although kernel coding style indeed tends to stick to uppercase in
enums, in my opinion the backporting/compatibility concern outweighs
the style one. Hope it's ok with you if I keep it as is in v2?

>>
>> +};
>> +
>> +#endif /* __EXYNOS_CLK_PLL_H */
>> diff --git a/drivers/clk/exynos/clk.c b/drivers/clk/exynos/clk.c
>> new file mode 100644
>> index 000000000000..430767f072d8
>> --- /dev/null
>> +++ b/drivers/clk/exynos/clk.c
>> @@ -0,0 +1,121 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Linaro Ltd.
>> + * Sam Protsenko <semen.protsenko@linaro.org>
>> + *
>> + * This file includes utility functions to register clocks to common
>> + * clock framework for Samsung platforms.
>> + */
>> +
>> +#include <dm.h>
>> +#include "clk.h"
>> +
>> +void samsung_clk_register_mux(void __iomem *base,
>> +                             const struct samsung_mux_clock *clk_list,
>> +                             unsigned int nr_clk)
>> +{
>> +       unsigned int cnt;
>> +
>> +       for (cnt = 0; cnt < nr_clk; cnt++) {
>> +               struct clk *clk;
>> +               const struct samsung_mux_clock *m;
>
>
> wouldn't it be better if use a more meaningful name like mux?
>

My reasoning for choosing the name that short in this case was because
of super-short scope (3 lines of code), and OTOH this variable is
massively used during that scope, like this:

        clk = clk_register_mux(NULL, m->name, m->parent_names,
                m->num_parents, m->flags, base + m->offset, m->shift,
                m->width, m->mux_flags);

Hope it makes sense. If you still prefer 'mux', please let me know and
I'll use it in v2.

>>
>> +
>> +               m = &clk_list[cnt];
>
>
> Is there any possibility that the value is null or wrong (e.g. overflow)
>

I decided to keep it with no error handling because I didn't feel like
it would bring much value. Because this code is supposed to be used
via samsung_cmu_register_one(), and the CMU structure passed to that
function is usually going to be defined in this idiomatic way (as can
be seen in clk-exynos850.c driver):

        static const struct samsung_clk_group top_cmu_clks[] = {
                { S_CLK_PLL, top_pure_pll_clks, ARRAY_SIZE(top_pure_pll_clks) },
                { S_CLK_MUX, top_pure_mux_clks, ARRAY_SIZE(top_pure_mux_clks) },
                 ...

and the corresponding clocks structures are also defined like this:

        static const struct samsung_mux_clock top_pure_mux_clks[] = {
                MUX(CLK_MOUT_SHARED0_PLL, "mout_shared0_pll",
mout_shared0_pll_p,
                    PLL_CON0_PLL_SHARED0, 4, 1),
                MUX(CLK_MOUT_SHARED1_PLL, "mout_shared1_pll",
mout_shared1_pll_p,
                    PLL_CON0_PLL_SHARED1, 4, 1),
                ...

I'd say the odds for messing this up are next to none, because of
using ARRAY_SIZE() and clock macros like MUX(). Especially because the
example is already set in clk-exynos850 driver and I assume everybody
would just use it as a template, which usually happens. So after
exploring the alternative approach (with added error handling) I felt
it was unjustifiable cluttered comparing to the more concise version
present in this series, at least in this particular code. Also, that
code resembles its kernel counterpart, where the clock pointer isn't
checked as well.

I'm not even sure how to handle possible errors here, as it's the
critical platform code. So maybe letting it just crash is not a bad
decision too. But if you have a strong opinion on this one, please let
me know how you would like me to handle that.

>> +               clk = clk_register_mux(NULL, m->name, m->parent_names,
>> +                       m->num_parents, m->flags, base + m->offset, m->shift,
>> +                       m->width, m->mux_flags);
>> +               clk_dm(m->id, clk);
>> +       }
>>> +}

[snip]

On Tue, Dec 19, 2023 at 5:38 AM Chanho Park <chanho61.park@samsung.com> wrote:
>

[snip]

> > diff --git a/drivers/clk/exynos/clk-pll.c b/drivers/clk/exynos/clk-pll.c
> > new file mode 100644
> > index 000000000000..9e496ff83aaf
> > --- /dev/null
> > +++ b/drivers/clk/exynos/clk-pll.c
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2016 Samsung Electronics
> > + * Copyright (C) 2023 Linaro Ltd.
> > + *
> > + * Authors:
> > + *   Thomas Abraham <thomas.ab@exynos.com>
>
> Need to correct Thomas's email to samsung.com if you want to keep his
> original credit even though his e-mail was already stale since he left the
> company.
>

Thanks for the review, will do in v2!

[snip]

> > + *
> > + * Authors:
> > + *   Thomas Abraham <thomas.ab@exynos.com>
>
> Ditto.
>
> Othewise,
> Reviewed-by: Chanho Park <chanho61.park@samsung.com>
>