Re: [DO NOT MERGE v6 15/37] clk: renesas: Add SH7750/7751 CPG Driver

Hi Sato-san,

On Tue, Jan 9, 2024 at 9:24 AM Yoshinori Sato
<[email protected]> wrote:
> Renesas SH7750 and SH7751 series CPG driver.
> This driver supported frequency control and clock gating.
>
> Signed-off-by: Yoshinori Sato <[email protected]>

Thanks for your patch!

> --- a/drivers/clk/renesas/Kconfig
> +++ b/drivers/clk/renesas/Kconfig
> @@ -193,6 +196,10 @@ config CLK_SH73A0
>         select CLK_RENESAS_CPG_MSTP
>         select CLK_RENESAS_DIV6
>
> +config CLK_SH7750
> +       bool "SH7750/7751 family clock support" if COMPILE_TEST
> +       help
> +         This is a driver for SH7750 / SH7751 CPG.

This is a duplicate of the below. Please drop it.

>
>  # Family
>  config CLK_RCAR_CPG_LIB
> @@ -223,6 +230,11 @@ config CLK_RZG2L
>         bool "Renesas RZ/{G2L,G2UL,G3S,V2L} family clock support" if COMPILE_TEST
>         select RESET_CONTROLLER
>
> +config CLK_SH7750
> +       bool "Renesas SH7750/7751 family clock support" if COMPILE_TEST
> +       help
> +         This is a driver for SH7750 / SH7751 CPG.
> +
>  # Generic
>  config CLK_RENESAS_CPG_MSSR
>         bool "CPG/MSSR clock support" if COMPILE_TEST

> --- /dev/null
> +++ b/drivers/clk/renesas/clk-sh7750.c

> +static int register_pll(struct device_node *node, struct cpg_priv *cpg)
> +{
> +       const char *clk_name = node->name;
> +       const char *parent_name;
> +       struct clk_init_data init = {
> +               .name = PLLOUT,
> +               .ops = &pll_ops,
> +               .flags = 0,
> +               .num_parents = 1,
> +       };
> +       int ret;
> +
> +       parent_name = of_clk_get_parent_name(node, 0);
> +       init.parent_names = &parent_name;
> +       cpg->hw.init = &init;
> +
> +       ret = of_clk_hw_register(node, &cpg->hw);
> +       if (ret < 0) {
> +               pr_err("%s: failed to register %s pll clock (%d)\n",
> +                      __func__, clk_name, ret);
> +               return ret;
> +       }
> +       if (ret < 0)
> +               pr_err("%s: failed to add provider %s (%d)\n",
> +                      __func__, clk_name, ret);

Bogus check and error message.

> +       return ret;
> +}

> +static int register_div(struct device_node *node, struct cpg_priv *cpg)
> +{
> +       static const char * const divout[] = {
> +               "fck", "bck", "ick",
> +       };
> +       static const char * const stbcrout[] = {
> +               "sci_clk", "rtc_clk", "tmu012_clk",     /* STBCR */
> +               "scif_clk", "dmac_clk",                 /* STBCR */
> +               "ubc_clk", "sq_clk",                    /* STBCR2 */
> +       };
> +       static const char * const clkstpout[] = {
> +               "intc_clk", "tmu34_clk", "pcic_clk",    /* CLKSTP00 */
> +       };
> +
> +       unsigned int i;
> +       int ret;
> +       struct clk_hw_onecell_data *data;
> +       struct clk_hw *reg_hw;
> +       int num_clk = ARRAY_SIZE(divout) + ARRAY_SIZE(stbcrout) + ARRAY_SIZE(clkstpout);
> +
> +       data = kzalloc(struct_size(data, hws, num_clk + 1), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       num_clk = 0;
> +       for (i = 0; i < ARRAY_SIZE(divout); i++) {
> +               reg_hw = __clk_hw_register_divider(NULL, node, divout[i],
> +                                                  PLLOUT, NULL, NULL,
> +                                                  0, cpg->frqcr, i * 3, 3,
> +                                                  CLK_DIVIDER_REG_16BIT,
> +                                                  (i == 0) ? pdiv_table : div_table,
> +                                                  &cpg->clklock);
> +               if (IS_ERR(reg_hw)) {
> +                       ret = PTR_ERR(reg_hw);
> +                       goto error;
> +               }
> +               data->hws[num_clk++] = reg_hw;
> +       }
> +       for (i = 0; i < ARRAY_SIZE(stbcrout); i++) {
> +               u32 off =  (i < 5) ? STBCR : STBCR2;
> +
> +               if (i >= 5 && !(cpg->feat & MSTP_CR2))
> +                       break;

Alternatively, you could set the maximum loop counter upfront

    n = cpg->feat & MSTP_CR2 ? ARRAY_SIZE(stbcrout) : 5;
    for (i = 0; i < n; i++) ...

> +               reg_hw = __clk_hw_register_gate(NULL, node, stbcrout[i],
> +                                               divout[0], NULL, NULL,
> +                                               0, cpg->frqcr + off, i % 5,
> +                                               CLK_GATE_REG_8BIT | CLK_GATE_SET_TO_DISABLE,
> +                                               &cpg->clklock);
> +               if (IS_ERR(reg_hw)) {
> +                       ret = PTR_ERR(reg_hw);
> +                       goto error;
> +               }
> +               data->hws[num_clk++] = reg_hw;
> +       }
> +       if (cpg->feat & MSTP_CLKSTP) {
> +               for (i = 0; i < ARRAY_SIZE(clkstpout); i++) {
> +                       if (i == 2 && !(cpg->feat & MSTP_CSTP2))
> +                               continue;

Set maximum loop counter upfront?

> +                       reg_hw = clk_hw_register_clkstp(node, clkstpout[i],
> +                                                       divout[0], cpg->clkstp00,
> +                                                       i, &cpg->clklock);
> +                       if (IS_ERR(reg_hw)) {
> +                               ret = PTR_ERR(reg_hw);
> +                               goto error;
> +                       }
> +                       data->hws[num_clk++] = reg_hw;
> +               }
> +       }
> +       data->num = num_clk;
> +       ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, data);
> +       if (ret < 0)
> +               goto error;
> +       return 0;
> +
> +error:
> +       pr_err("%pOF: failed to register clock (%d)\n",
> +                      node, ret);
> +       for (num_clk--; num_clk >= 0; num_clk--)
> +               kfree(data->hws[num_clk]);
> +       kfree(data);
> +       return ret;
> +}
> +
> +static struct cpg_priv *sh7750_cpg_setup(struct device_node *node, u32 feat)
> +{
> +       unsigned int num_parents;
> +       u32 mode;
> +       struct cpg_priv *cpg;
> +       int ret = 0;
> +
> +       num_parents = of_clk_get_parent_count(node);
> +       if (num_parents < 1) {
> +               pr_err("%s: no parent found", node->name);
> +               return ERR_PTR(-ENODEV);
> +       }

Do you need num_parents?

> +
> +       of_property_read_u32_index(node, "renesas,mode", 0, &mode);

mode may be used uninitialized, if "renesas,mode" is missing.

> +       if (mode >= 7) {
> +               pr_err("%s: Invalid clock mode setting (%u)\n",
> +                      node->name, mode);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       cpg = kzalloc(sizeof(struct cpg_priv), GFP_KERNEL);
> +       if (!cpg)
> +               return ERR_PTR(-ENOMEM);
> +
> +       cpg->frqcr = of_iomap(node, 0);
> +       if (cpg->frqcr == NULL) {
> +               pr_err("%pOF: failed to map divide register", node);
> +               ret = -ENODEV;
> +               goto cpg_free;
> +       }
> +
> +       if (feat & MSTP_CLKSTP) {
> +               cpg->clkstp00 = of_iomap(node, 1);
> +               if (cpg->clkstp00 == NULL) {
> +                       pr_err("%pOF: failed to map clkstp00 register", node);
> +                       ret = -ENODEV;
> +                       goto unmap_frqcr;
> +               }
> +       }
> +       cpg->feat = feat;
> +       cpg->mode = mode;
> +
> +       ret = register_pll(node, cpg);
> +       if (ret < 0)
> +               goto unmap_clkstp00;
> +
> +       ret = register_div(node, cpg);
> +       if (ret < 0)
> +               goto unmap_clkstp00;
> +

Perhaps "cpg_data = cpg;" here, and return an error code instead? ...

> +       return cpg;
> +
> +unmap_clkstp00:
> +       iounmap(cpg->clkstp00);
> +unmap_frqcr:
> +       iounmap(cpg->frqcr);
> +cpg_free:
> +       kfree(cpg);
> +       return ERR_PTR(ret);
> +}
> +
> +static void __init sh7750_cpg_init(struct device_node *node)
> +{
> +       cpg_data = sh7750_cpg_setup(node, cpg_feature[CPG_SH7750]);
> +       if (IS_ERR(cpg_data))
> +               cpg_data = NULL;

... then all cpg_data handling can be removed here...

> +}

> +static int sh7750_cpg_probe(struct platform_device *pdev)
> +{
> +       u32 feature;
> +
> +       if (cpg_data)
> +               return 0;
> +       feature = *(u32 *)of_device_get_match_data(&pdev->dev);
> +       cpg_data = sh7750_cpg_setup(pdev->dev.of_node, feature);
> +       if (IS_ERR(cpg_data))
> +               return PTR_ERR(cpg_data);
> +       return 0;

... and this can be simplified to

    return sh7750_cpg_setup(...);

> +}

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