Re: [RFC PATCH v2 10/30] drivers/clk: SH7750 / SH7751 CPG Driver

Hi Sato-san,

On Wed, Sep 13, 2023 at 11:25 AM Yoshinori Sato
<[email protected]> wrote:
> Signed-off-by: Yoshinori Sato <[email protected]>

Thanks for your patch!

>  drivers/clk/renesas/clk-sh7750.c | 236 +++++++++++++++++++++++++++++++

Patch prefix should be "clk: renesas: ".

> --- /dev/null
> +++ b/drivers/clk/renesas/clk-sh7750.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas SH7750/51 CPG driver
> + *
> + * Copyright 2023 Yoshinori Sato <[email protected]>
> + */
> +
> +#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>

Please add a blank line.

> +#include "clk-shdiv.h"

This doesn't exist until "[RFC PATCH v2 11/30] drivers/clk: SuperH
generai clock divider helper", so please move that patch up in the
series.

> +#include <dt-bindings/clock/sh7750.h>

This doesn't exist until "[RFC PATCH v2 24/30] include/dt-bindings:
Add SH7750 CPG header.", so please move that patch up in the series.

> +
> +static DEFINE_SPINLOCK(clklock);

I think it would make sense to move this into struct cpg_priv.


> +struct cpg_priv {
> +       struct clk_hw hw;
> +       void __iomem *frqcr;
> +       void __iomem *wdt;
> +       u32 mode;
> +       bool have_div1;
> +};
> +
> +#define to_priv(_hw) container_of(_hw, struct cpg_priv, hw)
> +
> +#define FRQCR_PLL1EN BIT(10)
> +static const int pll1mult[] = { 12, 12, 6, 12, 6, 12, 1};

unsigned int

> +
> +static unsigned long pll_recalc_rate(struct clk_hw *hw,
> +                                     unsigned long parent_rate)
> +{
> +       struct cpg_priv *cpg = to_priv(hw);
> +       unsigned long rate = parent_rate;
> +       uint16_t frqcr;

u16

Please don't mix uint<n>_t and u<n> in the same driver.

> +static int pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> +       struct cpg_priv *cpg = to_priv(hw);
> +
> +       get_round_rate(cpg, &req->rate, NULL, req->rate, req->best_parent_rate);
> +       return 0;
> +}
> +
> +static int pll_set_rate(struct clk_hw *hw,
> +                       unsigned long rate, unsigned long prate)
> +{
> +       struct cpg_priv *cpg = to_priv(hw);
> +       bool oldpll, newpll;
> +       uint16_t frqcr;

u16


> +static void __init register_div(struct device_node *node, struct cpg_priv *cpg)
> +{
> +       static const char * const divout[] = {
> +               [SH7750_CPG_FCK] = "fck",
> +               [SH7750_CPG_BCK] = "bck",
> +               [SH7750_CPG_ICK] = "ick",
> +       };
> +       struct clk *clk;
> +       int i;

unsigned int

> +
> +       for (i = 0; i < ARRAY_SIZE(divout); i++) {
> +               if (sh_div_clk_register(node, divout[i], PLLOUT,
> +                                       cpg->frqcr, i * 3, 3,
> +                                       (i == 0) ? pdiv_table : div_table,
> +                                       &clklock) < 0)
> +                       pr_err("%s: failed to register %s div clock (%ld)\n",
> +                              __func__, divout[i], PTR_ERR(clk));
> +       }
> +}
> +
> +
> +static void __init sh7750_cpg_setup(struct device_node *node)
> +{
> +       unsigned int num_parents;
> +       struct cpg_priv *cpg;
> +
> +       num_parents = of_clk_get_parent_count(node);
> +       if (num_parents < 1) {
> +               pr_err("%s: no parent found", node->name);
> +               return;
> +       }
> +
> +       cpg = kzalloc(sizeof(struct cpg_priv), GFP_KERNEL);
> +       if (!cpg) {
> +               pr_err("%s: failed to alloc memory", node->name);
> +               return;
> +       }
> +
> +       of_property_read_u32_index(node, "renesas,mode", 0, &cpg->mode);
> +       if (cpg->mode >= 7) {
> +               pr_err("%s: Invalid clock mode setting (%u)\n",
> +                      node->name, cpg->mode);
> +               goto cpg_free;
> +       }
> +       cpg->have_div1 = (of_device_is_compatible(node, "renesas,sh7750r-pll-clk") == 0);

What is this for? There are no users of "renesas,sh7750r-pll-clk".

> +
> +       cpg->frqcr = of_iomap(node, 0);
> +       if (cpg->frqcr == NULL) {
> +               pr_err("%s: failed to map divide register", node->name);
> +               goto cpg_free;
> +       }
> +
> +       cpg->wdt = of_iomap(node, 1);
> +       if (cpg->wdt == NULL) {
> +               pr_err("%s: failed to map watchdog register", node->name);
> +               goto unmap_frqcr;
> +       }
> +
> +       if (!register_pll(node, cpg))
> +               goto unmap_wdt;
> +
> +       register_div(node, cpg);
> +
> +unmap_wdt:
> +       iounmap(cpg->wdt);
> +unmap_frqcr:
> +       iounmap(cpg->frqcr);
> +cpg_free:
> +       kfree(cpg);
> +}
> +
> +CLK_OF_DECLARE(sh7750_cpg, "renesas,sh7750-cpg",
> +              sh7750_cpg_setup);

Probably you want a second entry for "renesas,sh7751-cpg"?

Is there a good reason to use CLK_OF_DECLARE()?
Perhaps because you need it early for the timer?

Most modern clock drivers use a platform_driver, registered from
e.g. subsys_initcall().  When you need an early initialization phase,
you can use CLK_OF_DECLARE_DRIVER() for the early part.

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