Thread (73 messages) 73 messages, 10 authors, 2024-02-27

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

flat view

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2024-02-27 16:34:37
Also in: dri-devel, linux-clk, linux-devicetree, linux-ide, linux-pci, linux-renesas-soc, linux-serial, linux-sh, lkml

Hi Sato-san,

On Tue, Jan 9, 2024 at 9:24 AM Yoshinori Sato
[off-list ref] wrote:
Renesas SH7750 and SH7751 series CPG driver.
This driver supported frequency control and clock gating.

Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
Thanks for your patch!
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
 # 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
quoted hunk ↗ jump to hunk
--- /dev/null
+++ b/drivers/clk/renesas/clk-sh7750.c
quoted hunk ↗ jump to hunk
+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;
+}
quoted hunk ↗ jump to hunk
+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++) ...
quoted hunk ↗ jump to hunk
+               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?
quoted hunk ↗ jump to hunk
+                       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.
quoted hunk ↗ jump to hunk
+       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? ...
quoted hunk ↗ jump to hunk
+       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...
+}
quoted hunk ↗ jump to hunk
+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 -- geert@linux-m68k.org

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help