Re: [RESEND RFC PATCH 04/12] clk: SH7750 / 7751 clk driver.

Hi Sato-san,

On Thu, Aug 31, 2023 at 11:32 AM Yoshinori Sato
<[email protected]> wrote:
> Use COMMON_CLK framework clock driver.
>
> Signed-off-by: Yoshinori Sato <[email protected]>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/clk/sh/Kconfig
> @@ -0,0 +1,7 @@
> +config COMMON_CLK_SH7750
> +       bool "Clcok driver for SH7750/SH7751"

Clock

> +       depends on CPU_SUBTYPE_SH7750 || CPU_SUBTYPE_SH7750S || \
> +                  CPU_SUBTYPE_SH7750R || \
> +                  CPU_SUBTYPE_SH7751 || CPU_SUBTYPE_SH7751R

|| COMPILE_TEST?

Anyway, I would do

    select COMMON_CLK_SH7750 if CPU_SUBTYPE_SH7750 || ...

at the top, and

    bool "Clock driver for SH7750/SH7751" if COMPILE_TEST

cfr. drivers/clk/renesas/Kconfig.

BTW, do you plan to put all SuperH clock drivers under SH?
For a simple CPG like in SH7750/7751 that could make sense.
For the more complex ones like SH7724, you probably want to plug
into the existing drivers/clk/renesas/renesas-cpg-mssr.c instead,
as it is very similar to the CPG on later Renesas ARM SoCs.

But even SH7751 has Standby Control registers with Module Stop
bits...

> +       help
> +         This driver supports the Renesas SH7750 and SH7751 CPG.
> diff --git a/drivers/clk/sh/Makefile b/drivers/clk/sh/Makefile
> new file mode 100644
> index 000000000000..7122c37655aa
> --- /dev/null
> +++ b/drivers/clk/sh/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_COMMON_CLK_SH7750) += clk-sh7750.o
> +obj-$(CONFIG_COMMON_CLK_SH7750) += clk-shdiv.o

These can be one line.

> diff --git a/drivers/clk/sh/clk-sh7750.c b/drivers/clk/sh/clk-sh7750.c
> new file mode 100644
> index 000000000000..f41712a9cf44
> --- /dev/null
> +++ b/drivers/clk/sh/clk-sh7750.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Renesas SH7750/51 clock driver
> + *
> + * Copyright 2023 Yoshinori Sato <[email protected]>
> + */
> +
> +#include <linux/clk.h>

Do you need the consumer API?

> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +
> +struct clk *sh_div_clk_register(struct device *dev, const char *name,
> +                               const char *parent_name,
> +                               void __iomem *reg, u8 shift, u8 width,
> +                               const struct clk_div_table *table,
> +                               spinlock_t *lock);

Please move this to a (private) header file.  Else builds with W=1
will complain about "warning: no previous prototype".

> +
> +static DEFINE_SPINLOCK(clklock);
> +
> +static struct clk_div_table pdiv_table[] = {

const

> +       { .val = 0, .div = 2, },
> +       { .val = 1, .div = 3, },
> +       { .val = 2, .div = 4, },
> +       { .val = 3, .div = 6, },
> +       { .val = 4, .div = 8, },
> +       { .val = 0, .div = 0, },
> +};
> +
> +static struct clk_div_table div_table[] = {

const

> +       { .val = 0, .div = 1, },
> +       { .val = 1, .div = 2, },
> +       { .val = 2, .div = 3, },
> +       { .val = 3, .div = 4, },
> +       { .val = 4, .div = 6, },
> +       { .val = 5, .div = 8, },
> +       { .val = 0, .div = 0, },
> +};
> +
> +struct pll_clock {
> +       struct clk_hw hw;
> +       void __iomem *frqcr;
> +       void __iomem *wdt;
> +       int md;

u32

> +       bool div1;
> +};
> +
> +#define to_pll_clock(_hw) container_of(_hw, struct pll_clock, hw)
> +
> +static unsigned long pll_recalc_rate(struct clk_hw *hw,
> +                                     unsigned long parent_rate)
> +{
> +       struct pll_clock *pll_clock = to_pll_clock(hw);
> +       unsigned long rate = parent_rate;
> +       uint16_t frqcr;
> +       static const int pll1[] = { 12, 12, 6, 12, 6, 12, 1};
> +
> +       frqcr = ioread16(pll_clock->frqcr);
> +       if (frqcr & (1 << 10)) {

Please add a define for "1 << 10" (or "BIT(10)").

#define FRQCR_PLL1EN    BIT(10)

> +               rate *= pll1[pll_clock->md];
> +               if (pll_clock->md < 6 && pll_clock->div1)
> +                       rate /= 2;
> +       }
> +       return rate;
> +}
> +
> +static const struct clk_ops pll_ops = {
> +       .recalc_rate = pll_recalc_rate,
> +};
> +
> +static void __init sh7750_pll_clk_setup(struct device_node *node)
> +{
> +       unsigned int num_parents;
> +       struct clk *clk;
> +       const char *clk_name = node->name;
> +       const char *parent_name;
> +       struct pll_clock *pll_clock;
> +       struct clk_init_data init;
> +
> +       num_parents = of_clk_get_parent_count(node);
> +       if (num_parents < 1) {
> +               pr_err("%s: no parent found", clk_name);
> +               return;
> +       }
> +
> +       pll_clock = kzalloc(sizeof(struct pll_clock), GFP_KERNEL);
> +       if (!pll_clock)
> +               return;
> +
> +       pll_clock->frqcr = of_iomap(node, 0);
> +       if (pll_clock->frqcr == NULL) {
> +               pr_err("%s: failed to map frequenct control register",

frequency

> +                      clk_name);
> +               goto free_clock;
> +       }
> +
> +       pll_clock->wdt = of_iomap(node, 1);
> +       if (pll_clock->wdt == NULL) {
> +               pr_err("%s: failed to map watchdog register", clk_name);
> +               goto unmap_frqcr;
> +       }
> +
> +       of_property_read_u32_index(node, "sh7750,md", 0, &pll_clock->md);

R-Mobile A1 uses "renesas,mode" for this, cfr.
Documentation/devicetree/bindings/clock/renesas,cpg-clocks.yaml

> +       if (pll_clock->md >= 7) {
> +               pr_err("%s: failed to clock mode setting (%d)\n",

invalid clock mode setting?
%u

> +                      clk_name, pll_clock->md);
> +               goto unmap_wdt;
> +       }
> +       pll_clock->div1 = !of_property_read_bool(node, "sh7750,rtype");

Shouldn't this be derived from the compatible value instead?

> +static void __init sh7750_div_clk_setup(struct device_node *node)
> +{
> +       unsigned int num_parents;
> +       struct clk *clk;
> +       const char *clk_name = node->name;
> +       const char *parent_name;
> +       void __iomem *freqcr = NULL;
> +       int i;

unsigned int

> +       int num_clks;
> +       int offset;
> +
> +       num_parents = of_clk_get_parent_count(node);
> +       if (num_parents < 1) {
> +               pr_err("%s: no parent found", clk_name);
> +               return;
> +       }
> +
> +       num_clks = of_property_count_strings(node, "clock-output-names");

Please no more clock-output-names.

> +CLK_OF_DECLARE(sh7750_div_clk, "renesas,sh7750-div-clock",
> +              sh7750_div_clk_setup);
> +CLK_OF_DECLARE(sh7750_pll_clk, "renesas,sh7750-pll-clock",
> +              sh7750_pll_clk_setup);

I think this should be a unified clock driver generating all clocks.
As there are no bindings yet, I will comment on the DTS files instead.

> diff --git a/drivers/clk/sh/clk-shdiv.c b/drivers/clk/sh/clk-shdiv.c
> new file mode 100644
> index 000000000000..2c016c413dd6
> --- /dev/null
> +++ b/drivers/clk/sh/clk-shdiv.c

> +static const struct clk_ops sh_clk_divider_ops = {
> +       .recalc_rate = sh_clk_divider_recalc_rate,
> +       .round_rate = sh_clk_divider_round_rate,

Please implement .determine_rate() instead of the deprecated
.round_rate().

> +       .set_rate = sh_clk_divider_set_rate,
> +};

> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -2,7 +2,9 @@
>  #
>  # Makefile for the SuperH specific drivers.
>  #
> +ifneq ($(CONFIG_RENESAS_SH_INTC),y)
>  obj-$(CONFIG_SH_INTC)                  += intc/
> +endif

This change does not belong in this patch.

>  ifneq ($(CONFIG_COMMON_CLK),y)
>  obj-$(CONFIG_HAVE_CLK)                 += clk/
>  endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds