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