[v2,7/8] clk: samsung: Add Exynos850 clock driver stub

Commit Message

Sam Protsenko Aug. 6, 2021, 3:21 p.m. UTC

For now it's just a stub driver to make the serial driver work. Later it
will be implemented properly. This driver doesn't really change clocks,
only registers the UART clock as a fixed-rate clock. Without this clock
driver the UART driver won't work, as it's trying to obtain "uart" clock
and fails if it's not able to.

In order to get a functional serial console we have to implement that
minimal clock driver with "uart" clock. It's not necessary to actually
configure clocks, as those are already configured in bootloader, so
kernel can rely on that for now.

Signed-off-by: Sam Protsenko <[e-mail]>
---
Changes in v2:
  - Used hard coded clock indexes, as clock bindings were removed; will
    add clock bindings back (reimplemented) once proper clock driver is
    ready
  - Removed .data = 0 for exynos850-oscclk, as it's in BSS section
  - Removed comma for terminator {}
  - Made exynos850_clk_init() static
  - Removed checking np for NULL, as it's already done in of_iomap()

 drivers/clk/samsung/Makefile        |  1 +
 drivers/clk/samsung/clk-exynos850.c | 64 +++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 drivers/clk/samsung/clk-exynos850.c

Comments

Krzysztof Kozlowski Aug. 9, 2021, 10:55 a.m. UTC | #1

On 06/08/2021 17:21, Sam Protsenko wrote:
> For now it's just a stub driver to make the serial driver work. Later it
> will be implemented properly. This driver doesn't really change clocks,
> only registers the UART clock as a fixed-rate clock. Without this clock
> driver the UART driver won't work, as it's trying to obtain "uart" clock
> and fails if it's not able to.

You know that as temporary solution you can add necessary clocks
directly in your DTS as fixed-rate-clocks? Effect would be quite similar
to the one here for UART driver but instead adding some temporary code
you would add temporary DTS nodes and references.

I am fine with this approach although the binding (if ever defined...)
would need to be marked as experimental.

> 
> In order to get a functional serial console we have to implement that
> minimal clock driver with "uart" clock. It's not necessary to actually
> configure clocks, as those are already configured in bootloader, so
> kernel can rely on that for now.
> 
> Signed-off-by: Sam Protsenko <[e-mail]>
> ---
> Changes in v2:
>   - Used hard coded clock indexes, as clock bindings were removed; will
>     add clock bindings back (reimplemented) once proper clock driver is
>     ready
>   - Removed .data = 0 for exynos850-oscclk, as it's in BSS section
>   - Removed comma for terminator {}
>   - Made exynos850_clk_init() static
>   - Removed checking np for NULL, as it's already done in of_iomap()
> 
>  drivers/clk/samsung/Makefile        |  1 +
>  drivers/clk/samsung/clk-exynos850.c | 64 +++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>  create mode 100644 drivers/clk/samsung/clk-exynos850.c
> 
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 028b2e27a37e..c46cf11e4d0b 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos5433.o
>  obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
>  obj-$(CONFIG_EXYNOS_CLKOUT)	+= clk-exynos-clkout.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
> +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
>  obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
>  obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
>  obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> new file mode 100644
> index 000000000000..36c7c7fe7cf0
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos850.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> + * Copyright (C) 2021 Linaro Ltd.
> + *
> + * Common Clock Framework support for Exynos850 SoC.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#include "clk.h"
> +
> +/* Will be extracted to bindings header once proper clk driver is implemented */
> +#define OSCCLK		1
> +#define DOUT_UART	2
> +#define CLK_NR_CLKS	3
> +
> +/* Fixed rate clocks generated outside the SoC */
> +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = {
> +	FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000),
> +};
> +
> +/*
> + * Model the UART clock as a fixed-rate clock for now, to make serial driver
> + * work. This clock is already configured in the bootloader.
> + */
> +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = {
> +	FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000),
> +};
> +
> +static const struct of_device_id ext_clk_match[] __initconst = {
> +	{ .compatible = "samsung,exynos850-oscclk" },
> +	{}
> +};
> +
> +static void __init exynos850_clk_init(struct device_node *np)
> +{
> +	void __iomem *reg_base;
> +	struct samsung_clk_provider *ctx;
> +
> +	reg_base = of_iomap(np, 0);
> +	if (!reg_base)
> +		panic("%s: failed to map registers\n", __func__);
> +
> +	ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
> +	if (!ctx)
> +		panic("%s: unable to allocate ctx\n", __func__);

Not needed, the samsung_clk_init() panics or returns valid memory.

> +
> +	samsung_clk_of_register_fixed_ext(ctx,
> +			exynos850_fixed_rate_ext_clks,
> +			ARRAY_SIZE(exynos850_fixed_rate_ext_clks),
> +			ext_clk_match);
> +
> +	samsung_clk_register_fixed_rate(ctx, exynos850_peri_clks,
> +			ARRAY_SIZE(exynos850_peri_clks));
> +
> +	samsung_clk_of_add_provider(np, ctx);
> +}
> +
> +CLK_OF_DECLARE(exynos850_clk, "samsung,exynos850-clock", exynos850_clk_init);
> 

Best regards,
Krzysztof

Krzysztof Kozlowski Aug. 9, 2021, 11:22 a.m. UTC | #2

On 06/08/2021 17:21, Sam Protsenko wrote:
> For now it's just a stub driver to make the serial driver work. Later it
> will be implemented properly. This driver doesn't really change clocks,
> only registers the UART clock as a fixed-rate clock. Without this clock
> driver the UART driver won't work, as it's trying to obtain "uart" clock
> and fails if it's not able to.
> 
> In order to get a functional serial console we have to implement that
> minimal clock driver with "uart" clock. It's not necessary to actually
> configure clocks, as those are already configured in bootloader, so
> kernel can rely on that for now.
> 
> Signed-off-by: Sam Protsenko <[e-mail]>
> ---
> Changes in v2:
>   - Used hard coded clock indexes, as clock bindings were removed; will
>     add clock bindings back (reimplemented) once proper clock driver is
>     ready
>   - Removed .data = 0 for exynos850-oscclk, as it's in BSS section
>   - Removed comma for terminator {}
>   - Made exynos850_clk_init() static
>   - Removed checking np for NULL, as it's already done in of_iomap()
> 
>  drivers/clk/samsung/Makefile        |  1 +
>  drivers/clk/samsung/clk-exynos850.c | 64 +++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>  create mode 100644 drivers/clk/samsung/clk-exynos850.c
> 
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 028b2e27a37e..c46cf11e4d0b 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos5433.o
>  obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
>  obj-$(CONFIG_EXYNOS_CLKOUT)	+= clk-exynos-clkout.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
> +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
>  obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
>  obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
>  obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> new file mode 100644
> index 000000000000..36c7c7fe7cf0
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos850.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> + * Copyright (C) 2021 Linaro Ltd.
> + *
> + * Common Clock Framework support for Exynos850 SoC.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#include "clk.h"
> +
> +/* Will be extracted to bindings header once proper clk driver is implemented */
> +#define OSCCLK		1
> +#define DOUT_UART	2
> +#define CLK_NR_CLKS	3
> +
> +/* Fixed rate clocks generated outside the SoC */
> +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = {
> +	FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000),
> +};
> +
> +/*
> + * Model the UART clock as a fixed-rate clock for now, to make serial driver
> + * work. This clock is already configured in the bootloader.
> + */
> +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = {
> +	FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000),
> +};
> +
> +static const struct of_device_id ext_clk_match[] __initconst = {
> +	{ .compatible = "samsung,exynos850-oscclk" },

One more thing - I am not sure anymore if this is correct. AFAIR, we
wanted to drop compatibles for external clocks.

Chanwoo, Sylwester, Tomasz,
Do you remember the recommended approach? Shall it be like Exynos542x
(samsung,exynos5420-oscclk) or Exynos5433?


BTW, I am now converting some of existing clock controller bindings to
dtschema.

Best regards,
Krzysztof

Sam Protsenko Aug. 9, 2021, 6:51 p.m. UTC | #3

On Mon, 9 Aug 2021 at 13:55, Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 06/08/2021 17:21, Sam Protsenko wrote:
> > For now it's just a stub driver to make the serial driver work. Later it
> > will be implemented properly. This driver doesn't really change clocks,
> > only registers the UART clock as a fixed-rate clock. Without this clock
> > driver the UART driver won't work, as it's trying to obtain "uart" clock
> > and fails if it's not able to.
>
> You know that as temporary solution you can add necessary clocks
> directly in your DTS as fixed-rate-clocks? Effect would be quite similar
> to the one here for UART driver but instead adding some temporary code
> you would add temporary DTS nodes and references.
>
> I am fine with this approach although the binding (if ever defined...)
> would need to be marked as experimental.
>

Let's keep this driver then. My next step would be implementing the
proper clk driver, so this review would be a good starting point for
me. I will, of course, address your other comments.

> >
> > In order to get a functional serial console we have to implement that
> > minimal clock driver with "uart" clock. It's not necessary to actually
> > configure clocks, as those are already configured in bootloader, so
> > kernel can rely on that for now.
> >
> > Signed-off-by: Sam Protsenko <[e-mail]>
> > ---
> > Changes in v2:
> >   - Used hard coded clock indexes, as clock bindings were removed; will
> >     add clock bindings back (reimplemented) once proper clock driver is
> >     ready
> >   - Removed .data = 0 for exynos850-oscclk, as it's in BSS section
> >   - Removed comma for terminator {}
> >   - Made exynos850_clk_init() static
> >   - Removed checking np for NULL, as it's already done in of_iomap()
> >
> >  drivers/clk/samsung/Makefile        |  1 +
> >  drivers/clk/samsung/clk-exynos850.c | 64 +++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> >  create mode 100644 drivers/clk/samsung/clk-exynos850.c
> >
> > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> > index 028b2e27a37e..c46cf11e4d0b 100644
> > --- a/drivers/clk/samsung/Makefile
> > +++ b/drivers/clk/samsung/Makefile
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)       += clk-exynos5433.o
> >  obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
> >  obj-$(CONFIG_EXYNOS_CLKOUT)  += clk-exynos-clkout.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos7.o
> > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos850.o
> >  obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
> >  obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
> >  obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> > diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> > new file mode 100644
> > index 000000000000..36c7c7fe7cf0
> > --- /dev/null
> > +++ b/drivers/clk/samsung/clk-exynos850.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> > + * Copyright (C) 2021 Linaro Ltd.
> > + *
> > + * Common Clock Framework support for Exynos850 SoC.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +
> > +#include "clk.h"
> > +
> > +/* Will be extracted to bindings header once proper clk driver is implemented */
> > +#define OSCCLK               1
> > +#define DOUT_UART    2
> > +#define CLK_NR_CLKS  3
> > +
> > +/* Fixed rate clocks generated outside the SoC */
> > +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = {
> > +     FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000),
> > +};
> > +
> > +/*
> > + * Model the UART clock as a fixed-rate clock for now, to make serial driver
> > + * work. This clock is already configured in the bootloader.
> > + */
> > +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = {
> > +     FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000),
> > +};
> > +
> > +static const struct of_device_id ext_clk_match[] __initconst = {
> > +     { .compatible = "samsung,exynos850-oscclk" },
> > +     {}
> > +};
> > +
> > +static void __init exynos850_clk_init(struct device_node *np)
> > +{
> > +     void __iomem *reg_base;
> > +     struct samsung_clk_provider *ctx;
> > +
> > +     reg_base = of_iomap(np, 0);
> > +     if (!reg_base)
> > +             panic("%s: failed to map registers\n", __func__);
> > +
> > +     ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
> > +     if (!ctx)
> > +             panic("%s: unable to allocate ctx\n", __func__);
>
> Not needed, the samsung_clk_init() panics or returns valid memory.
>

Done. Btw, I noticed that similar check is present in clk-exynos5433.c.

> > +
> > +     samsung_clk_of_register_fixed_ext(ctx,
> > +                     exynos850_fixed_rate_ext_clks,
> > +                     ARRAY_SIZE(exynos850_fixed_rate_ext_clks),
> > +                     ext_clk_match);
> > +
> > +     samsung_clk_register_fixed_rate(ctx, exynos850_peri_clks,
> > +                     ARRAY_SIZE(exynos850_peri_clks));
> > +
> > +     samsung_clk_of_add_provider(np, ctx);
> > +}
> > +
> > +CLK_OF_DECLARE(exynos850_clk, "samsung,exynos850-clock", exynos850_clk_init);
> >
>
> Best regards,
> Krzysztof

Sam Protsenko Aug. 9, 2021, 7:48 p.m. UTC | #4

On Mon, 9 Aug 2021 at 14:23, Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 06/08/2021 17:21, Sam Protsenko wrote:
> > For now it's just a stub driver to make the serial driver work. Later it
> > will be implemented properly. This driver doesn't really change clocks,
> > only registers the UART clock as a fixed-rate clock. Without this clock
> > driver the UART driver won't work, as it's trying to obtain "uart" clock
> > and fails if it's not able to.
> >
> > In order to get a functional serial console we have to implement that
> > minimal clock driver with "uart" clock. It's not necessary to actually
> > configure clocks, as those are already configured in bootloader, so
> > kernel can rely on that for now.
> >
> > Signed-off-by: Sam Protsenko <[e-mail]>
> > ---
> > Changes in v2:
> >   - Used hard coded clock indexes, as clock bindings were removed; will
> >     add clock bindings back (reimplemented) once proper clock driver is
> >     ready
> >   - Removed .data = 0 for exynos850-oscclk, as it's in BSS section
> >   - Removed comma for terminator {}
> >   - Made exynos850_clk_init() static
> >   - Removed checking np for NULL, as it's already done in of_iomap()
> >
> >  drivers/clk/samsung/Makefile        |  1 +
> >  drivers/clk/samsung/clk-exynos850.c | 64 +++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> >  create mode 100644 drivers/clk/samsung/clk-exynos850.c
> >
> > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> > index 028b2e27a37e..c46cf11e4d0b 100644
> > --- a/drivers/clk/samsung/Makefile
> > +++ b/drivers/clk/samsung/Makefile
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)       += clk-exynos5433.o
> >  obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
> >  obj-$(CONFIG_EXYNOS_CLKOUT)  += clk-exynos-clkout.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos7.o
> > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos850.o
> >  obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
> >  obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
> >  obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> > diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> > new file mode 100644
> > index 000000000000..36c7c7fe7cf0
> > --- /dev/null
> > +++ b/drivers/clk/samsung/clk-exynos850.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2019 Samsung Electronics Co., Ltd.
> > + * Copyright (C) 2021 Linaro Ltd.
> > + *
> > + * Common Clock Framework support for Exynos850 SoC.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +
> > +#include "clk.h"
> > +
> > +/* Will be extracted to bindings header once proper clk driver is implemented */
> > +#define OSCCLK               1
> > +#define DOUT_UART    2
> > +#define CLK_NR_CLKS  3
> > +
> > +/* Fixed rate clocks generated outside the SoC */
> > +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = {
> > +     FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000),
> > +};
> > +
> > +/*
> > + * Model the UART clock as a fixed-rate clock for now, to make serial driver
> > + * work. This clock is already configured in the bootloader.
> > + */
> > +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = {
> > +     FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000),
> > +};
> > +
> > +static const struct of_device_id ext_clk_match[] __initconst = {
> > +     { .compatible = "samsung,exynos850-oscclk" },
>
> One more thing - I am not sure anymore if this is correct. AFAIR, we
> wanted to drop compatibles for external clocks.
>

I'll remove oscclk from the clock driver and device tree. It's not
needed right now anyway, as that driver is just a stub.

But I'd still like to know the proper way to define external clocks. I
can see that in exynos7.dtsi and exynos5433.dtsi there is just regular
fixed clock defined for "oscclk" (or "fin_pll"), and then that clock
is referenced in corresponding clock driver by its
'clock-output-names' property. I guess that approach is the
recommended one?

> Chanwoo, Sylwester, Tomasz,
> Do you remember the recommended approach? Shall it be like Exynos542x
> (samsung,exynos5420-oscclk) or Exynos5433?
>
>
> BTW, I am now converting some of existing clock controller bindings to
> dtschema.
>

I'll try to review those in a day, that might also be helpful when I
get to clk implementation. Btw, thank you for reviewing my patches in
such a quick pace!

> Best regards,
> Krzysztof

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Aug. 10, 2021, 7:55 a.m. UTC | #5

On 09.08.2021 21:48, Sam Protsenko wrote:
>>> +/* Will be extracted to bindings header once proper clk driver is implemented */
>>> +#define OSCCLK               1
>>> +#define DOUT_UART    2
>>> +#define CLK_NR_CLKS  3
>>> +
>>> +/* Fixed rate clocks generated outside the SoC */
>>> +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = {
>>> +     FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000),
>>> +};
>>> +
>>> +/*
>>> + * Model the UART clock as a fixed-rate clock for now, to make serial driver
>>> + * work. This clock is already configured in the bootloader.
>>> + */
>>> +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = {
>>> +     FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000),
>>> +};
>>> +
>>> +static const struct of_device_id ext_clk_match[] __initconst = {
>>> +     { .compatible = "samsung,exynos850-oscclk" },
>>
>> One more thing - I am not sure anymore if this is correct. AFAIR, we
>> wanted to drop compatibles for external clocks.
>>
> I'll remove oscclk from the clock driver and device tree. It's not
> needed right now anyway, as that driver is just a stub.
> 
> But I'd still like to know the proper way to define external clocks. I
> can see that in exynos7.dtsi and exynos5433.dtsi there is just regular
> fixed clock defined for "oscclk" (or "fin_pll"), and then that clock
> is referenced in corresponding clock driver by its
> 'clock-output-names' property. I guess that approach is the
> recommended one?

Yes, we should use generic "fixed-clock" in DT to model the external
root clock. Registering the external clock from within the CMU driver
is a legacy method that predates generic "fixed-clock" and should be
avoided.  

That said I think this temporary stub driver is not needed at all, 
you could well define a fixed clock in DT and reference it in the UART 
node, as Krzysztof suggested.

--
Regards, 
Sylwester

Sam Protsenko Aug. 11, 2021, 11:20 a.m. UTC | #6

On Tue, 10 Aug 2021 at 10:55, Sylwester Nawrocki <[e-mail]> wrote:
>
> On 09.08.2021 21:48, Sam Protsenko wrote:
> >>> +/* Will be extracted to bindings header once proper clk driver is implemented */
> >>> +#define OSCCLK               1
> >>> +#define DOUT_UART    2
> >>> +#define CLK_NR_CLKS  3
> >>> +
> >>> +/* Fixed rate clocks generated outside the SoC */
> >>> +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = {
> >>> +     FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000),
> >>> +};
> >>> +
> >>> +/*
> >>> + * Model the UART clock as a fixed-rate clock for now, to make serial driver
> >>> + * work. This clock is already configured in the bootloader.
> >>> + */
> >>> +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = {
> >>> +     FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000),
> >>> +};
> >>> +
> >>> +static const struct of_device_id ext_clk_match[] __initconst = {
> >>> +     { .compatible = "samsung,exynos850-oscclk" },
> >>
> >> One more thing - I am not sure anymore if this is correct. AFAIR, we
> >> wanted to drop compatibles for external clocks.
> >>
> > I'll remove oscclk from the clock driver and device tree. It's not
> > needed right now anyway, as that driver is just a stub.
> >
> > But I'd still like to know the proper way to define external clocks. I
> > can see that in exynos7.dtsi and exynos5433.dtsi there is just regular
> > fixed clock defined for "oscclk" (or "fin_pll"), and then that clock
> > is referenced in corresponding clock driver by its
> > 'clock-output-names' property. I guess that approach is the
> > recommended one?
>
> Yes, we should use generic "fixed-clock" in DT to model the external
> root clock. Registering the external clock from within the CMU driver
> is a legacy method that predates generic "fixed-clock" and should be
> avoided.
>

Thanks for confirming this. I'll go with generic fixed clock in my
clock patch series then.

> That said I think this temporary stub driver is not needed at all,
> you could well define a fixed clock in DT and reference it in the UART
> node, as Krzysztof suggested.
>

Ok, I'll remove the stub clock driver in v3. Using fixed clock in
device tree for serial seems to work fine.

> --
> Regards,
> Sylwester

Patch

diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
index 028b2e27a37e..c46cf11e4d0b 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -17,6 +17,7 @@  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos5433.o
 obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
 obj-$(CONFIG_EXYNOS_CLKOUT)	+= clk-exynos-clkout.o
 obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
+obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
 obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
 obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
 obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
new file mode 100644
index 000000000000..36c7c7fe7cf0
--- /dev/null
+++ b/drivers/clk/samsung/clk-exynos850.c
@@ -0,0 +1,64 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2021 Linaro Ltd.
+ *
+ * Common Clock Framework support for Exynos850 SoC.
+ */
+
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include "clk.h"
+
+/* Will be extracted to bindings header once proper clk driver is implemented */
+#define OSCCLK		1
+#define DOUT_UART	2
+#define CLK_NR_CLKS	3
+
+/* Fixed rate clocks generated outside the SoC */
+static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = {
+	FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000),
+};
+
+/*
+ * Model the UART clock as a fixed-rate clock for now, to make serial driver
+ * work. This clock is already configured in the bootloader.
+ */
+static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = {
+	FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000),
+};
+
+static const struct of_device_id ext_clk_match[] __initconst = {
+	{ .compatible = "samsung,exynos850-oscclk" },
+	{}
+};
+
+static void __init exynos850_clk_init(struct device_node *np)
+{
+	void __iomem *reg_base;
+	struct samsung_clk_provider *ctx;
+
+	reg_base = of_iomap(np, 0);
+	if (!reg_base)
+		panic("%s: failed to map registers\n", __func__);
+
+	ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
+	if (!ctx)
+		panic("%s: unable to allocate ctx\n", __func__);
+
+	samsung_clk_of_register_fixed_ext(ctx,
+			exynos850_fixed_rate_ext_clks,
+			ARRAY_SIZE(exynos850_fixed_rate_ext_clks),
+			ext_clk_match);
+
+	samsung_clk_register_fixed_rate(ctx, exynos850_peri_clks,
+			ARRAY_SIZE(exynos850_peri_clks));
+
+	samsung_clk_of_add_provider(np, ctx);
+}
+
+CLK_OF_DECLARE(exynos850_clk, "samsung,exynos850-clock", exynos850_clk_init);