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

From: Geert Uytterhoeven
Date: Tue Feb 27 2024 - 11:35:07 EST

Next message: Linux regression tracking (Thorsten Leemhuis): "Re: [PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume"
Previous message: Thomas Gleixner: "Re: [PATCH v8 2/3] clocksource: Add JH7110 timer driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Sato-san,

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

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 -- geert@linux-m68korg

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

Next message: Linux regression tracking (Thorsten Leemhuis): "Re: [PATCH regression fix] misc: lis3lv02d_i2c: Fix regulators getting en-/dis-abled twice on suspend/resume"
Previous message: Thomas Gleixner: "Re: [PATCH v8 2/3] clocksource: Add JH7110 timer driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]