Introduce Nuvoton Arbel NPCM8XX BMC SoC

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 49 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 49 +++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +23 -0

new file mode 100644
index 000000000000..3d4fddc090ca
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM8XX Clock Controller Binding
+
+maintainers:
+  - Tomer Maimon <[email protected]>
+
+description: |
+  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
+  generates and supplies clocks to all modules within the BMC.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm845-clk
+
+  reg:
+    maxItems: 1
+

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +16 -0

+
+additionalProperties: false
+
+examples:
+  - |
+    ahb {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@f0801000 {
+            compatible = "nuvoton,npcm845-clk";
+            reg = <0x0 0xf0801000 0x0 0x1000>;
+            #clock-cells = <1>;
+        };
+    };
+...

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h new file +0 -0

new file mode 100644
index 000000000000..e5cce08b00e1

Quoted 39 lines · Tomer Maimon

Add binding for the Arbel BMC NPCM8XX Clock controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../bindings/clock/nuvoton,npcm845-clk.yaml   | 49 +++++++++++++++++++
 .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 49 +++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
 create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +23 -0

new file mode 100644
index 000000000000..3d4fddc090ca
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NPCM8XX Clock Controller Binding
+
+maintainers:
+  - Tomer Maimon <[email protected]>
+
+description: |
+  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
+  generates and supplies clocks to all modules within the BMC.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm845-clk
+
+  reg:
+    maxItems: 1
+

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml line 33

Quoted 10 lines

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +10 -0

+  '#clock-cells':
+    const: 1
+    description:
+      See include/dt-bindings/clock/nuvoton,npcm8xx-clock.h for the full
+      list of NPCM8XX clock IDs.
+
+required:
+  - compatible
+  - reg
+  - "#clock-cells"

Use same quotes through your patchset - either " or '. Do not mix.

Quoted 18 lines

Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml new file +16 -0

+
+additionalProperties: false
+
+examples:
+  - |
+    ahb {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@f0801000 {
+            compatible = "nuvoton,npcm845-clk";
+            reg = <0x0 0xf0801000 0x0 0x1000>;
+            #clock-cells = <1>;
+        };
+    };
+...

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h new file +0 -0

new file mode 100644

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h line 1

Quoted 5 lines

index 000000000000..e5cce08b00e1

include/dt-bindings/clock/nuvoton,npcm8xx-clock.h new file +1 -0

@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */

My comment about filename is still waiting to be resolved. I asked for same filename as bindings in v1.

https://lore.kernel.org/all/CAP6Zq1gXEqqquKzxTHoxYnvh2LCGt-ES+k=aX61FKyHW5WB62w@mail.gmail.com/

I am still going to NAK it if it is not implemented.

Best regards,
Krzysztof

Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller which generates and supplies clocks to all modules within the BMC.

Signed-off-by Tomer Maimon <[email protected]>

---
  drivers/clk/Kconfig       |   6 +
  drivers/clk/Makefile      |   1 +
  drivers/clk/clk-npcm8xx.c | 760 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 767 insertions(+)

create mode 100644 drivers/clk/clk-npcm8xx.c

drivers/clk/clk-npcm8xx.c new file +20 -0

+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+			 const char *name, const char *parent_name,
+			 unsigned long flags)
+{
+	struct npcm8xx_clk_pll *pll;
+	struct clk_init_data init;
+	struct clk_hw *hw;
+	int ret;
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+
+	init.name = name;
+	init.ops = &npcm8xx_clk_pll_ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;

drivers/clk/clk-npcm8xx.c new file +6 -0

+		return ERR_PTR(ret);
+	}
+
+	return hw;
+}
+

drivers/clk/clk-npcm8xx.c new file +15 -0

+static int npcm8xx_clk_probe(struct platform_device *pdev)
+{
+	struct clk_hw_onecell_data *npcm8xx_clk_data;
+	struct device *dev = &pdev->dev;
+	void __iomem *clk_base;
+	struct clk_hw *hw;
+	int i;
+
+	npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
+							 NPCM8XX_NUM_CLOCKS),
+					GFP_KERNEL);
+	if (!npcm8xx_clk_data)
+		return -ENOMEM;
+
+	clk_base = devm_platform_ioremap_resource(pdev, 0);

drivers/clk/clk-npcm8xx.c new file +17 -0

+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+	npcm8xx_clk_data->hws[NPCM8XX_CLK_REFCLK] = hw;
+
+	/* Register plls */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_plls); i++) {
+		const struct npcm8xx_clk_pll_data *pll_data = &npcm8xx_plls[i];
+
+		hw = npcm8xx_clk_register_pll(dev, clk_base + pll_data->reg,
+					      pll_data->name,
+					      pll_data->parent_name,
+					      pll_data->flags);
+		if (IS_ERR(hw)) {
+			dev_err(dev, "npcm8xx_clk: Can't register pll\n");
+			return PTR_ERR(hw);
+		}
+

drivers/clk/clk-npcm8xx.c new file +27 -0

+	}
+
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PRE_CLK,
+					       NPCM8XX_CLK_S_CPU_MUX, 0, 1, 2);
+	if (IS_ERR(hw)) {
+		dev_err(dev, "npcm8xx_clk: Can't register ckclk div2\n");
+		return PTR_ERR(hw);
+	}
+
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_AXI,
+					       NPCM8XX_CLK_S_TH, 0, 1, 2);
+	if (IS_ERR(hw)) {
+		dev_err(dev, "npcm8xx_clk: Can't register axi div2\n");
+		return PTR_ERR(hw);
+	}
+
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_ATB,
+					       NPCM8XX_CLK_S_AXI, 0, 1, 2);
+	if (IS_ERR(hw)) {
+		dev_err(dev, "npcm8xx_clk: Can't register atb div2\n");
+		return PTR_ERR(hw);
+	}
+
+	/* Register muxes */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) {
+		const struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i];
+

drivers/clk/clk-npcm8xx.c new file +10 -0

+		if (IS_ERR(hw)) {
+			dev_err(dev, "npcm8xx_clk: Can't register mux\n");
+			return PTR_ERR(hw);
+		}
+
+		if (mux_data->onecell_idx >= 0)
+			npcm8xx_clk_data->hws[mux_data->onecell_idx] = hw;
+	}
+
+	/* Register clock dividers specified in npcm8xx_divs */

drivers/clk/clk-npcm8xx.c new file +33 -0

+		if (IS_ERR(hw)) {
+			dev_err(dev, "npcm8xx_clk: Can't register div table\n");
+			return PTR_ERR(hw);
+		}
+
+		if (div_data->onecell_idx >= 0)
+			npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   npcm8xx_clk_data);
+}
+
+static const struct of_device_id npcm8xx_clk_dt_ids[] = {
+	{ .compatible = "nuvoton,npcm845-clk", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, npcm8xx_clk_dt_ids);
+
+static struct platform_driver npcm8xx_clk_driver = {
+	.probe  = npcm8xx_clk_probe,
+	.driver = {
+		.name = "npcm8xx_clk",
+		.of_match_table = npcm8xx_clk_dt_ids,
+	},
+};
+
+static int __init npcm8xx_clk_driver_init(void)
+{
+	return platform_driver_register(&npcm8xx_clk_driver);
+}
+arch_initcall(npcm8xx_clk_driver_init);
+

Quoted 11 lines · Tomer Maimon

Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller which generates and supplies clocks to all modules within the BMC.

Signed-off-by Tomer Maimon <[email protected]>

---
  drivers/clk/Kconfig       |   6 +
  drivers/clk/Makefile      |   1 +
  drivers/clk/clk-npcm8xx.c | 760 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 767 insertions(+)

create mode 100644 drivers/clk/clk-npcm8xx.c

[...]

Hi, below a few comments related to error handling and possible dev_err_probe() usage to savec a few LoC.

CJ

Quoted 20 lines

drivers/clk/clk-npcm8xx.c new file +20 -0

+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+                      const char *name, const char *parent_name,
+                      unsigned long flags)
+{
+     struct npcm8xx_clk_pll *pll;
+     struct clk_init_data init;
+     struct clk_hw *hw;
+     int ret;
+
+     pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+     if (!pll)
+             return ERR_PTR(-ENOMEM);
+
+     pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+
+     init.name = name;
+     init.ops = &npcm8xx_clk_pll_ops;
+     init.parent_names = &parent_name;
+     init.num_parents = 1;

drivers/clk/clk-npcm8xx.c line 100

Quoted 10 lines

drivers/clk/clk-npcm8xx.c new file +10 -0

+     init.flags = flags;
+
+     pll->pllcon = pllcon;
+     pll->hw.init = &init;
+
+     hw = &pll->hw;
+
+     ret = devm_clk_hw_register(dev, hw);
+     if (ret) {
+             kfree(pll);

Hi,

there is no other kfree(() in this patch. It is handled by the framework once the clk is registered or should there be another kfree() somewhere or should pll be devm_alloc()'ed?

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+             return ERR_PTR(ret);
+     }
+
+     return hw;
+}
+

[...]

Quoted 20 lines

drivers/clk/clk-npcm8xx.c new file +20 -0

+static int npcm8xx_clk_probe(struct platform_device *pdev)
+{
+     struct clk_hw_onecell_data *npcm8xx_clk_data;
+     struct device *dev = &pdev->dev;
+     void __iomem *clk_base;
+     struct clk_hw *hw;
+     int i;
+
+     npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
+                                                      NPCM8XX_NUM_CLOCKS),
+                                     GFP_KERNEL);
+     if (!npcm8xx_clk_data)
+             return -ENOMEM;
+
+     clk_base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(clk_base))
+             return PTR_ERR(clk_base);
+
+     npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
+

drivers/clk/clk-npcm8xx.c line 636

Quoted 5 lines

drivers/clk/clk-npcm8xx.c new file +5 -0

+     for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++)
+             npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+     /* Reference 25MHz clock */
+     hw = clk_hw_register_fixed_rate(dev, "refclk", NULL, 0, NPCM8XX_REF_CLK);

Other resoruces are managed, but not this one.
Is it on purpose?

Is an error handling path needed or a devm_add_action_or_reset()?

Quoted 21 lines

drivers/clk/clk-npcm8xx.c new file +21 -0

+     if (IS_ERR(hw))
+             return PTR_ERR(hw);
+     npcm8xx_clk_data->hws[NPCM8XX_CLK_REFCLK] = hw;
+
+     /* Register plls */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_plls); i++) {
+             const struct npcm8xx_clk_pll_data *pll_data = &npcm8xx_plls[i];
+
+             hw = npcm8xx_clk_register_pll(dev, clk_base + pll_data->reg,
+                                           pll_data->name,
+                                           pll_data->parent_name,
+                                           pll_data->flags);
+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register pll\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (pll_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[pll_data->onecell_idx] = hw;
+     }
+

drivers/clk/clk-npcm8xx.c line 663

Quoted 6 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+     /* Register fixed dividers */
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL1_DIV2,
+                                            NPCM8XX_CLK_S_PLL1, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register fixed div\n");
+             return PTR_ERR(hw);

return dev_err_probe()?

drivers/clk/clk-npcm8xx.c line 670

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +7 -0

+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL2_DIV2,
+                                            NPCM8XX_CLK_S_PLL2, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register pll div2\n");
+             return PTR_ERR(hw);

Same here and in other calls below.

Quoted 30 lines

drivers/clk/clk-npcm8xx.c new file +30 -0

+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PRE_CLK,
+                                            NPCM8XX_CLK_S_CPU_MUX, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register ckclk div2\n");
+             return PTR_ERR(hw);
+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_AXI,
+                                            NPCM8XX_CLK_S_TH, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register axi div2\n");
+             return PTR_ERR(hw);
+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_ATB,
+                                            NPCM8XX_CLK_S_AXI, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register atb div2\n");
+             return PTR_ERR(hw);
+     }
+
+     /* Register muxes */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) {
+             const struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i];
+
+             hw = clk_hw_register_mux_table(dev, mux_data->name,
+                                            mux_data->parent_names,
+                                            mux_data->num_parents,

drivers/clk/clk-npcm8xx.c line 707

Quoted 7 lines

drivers/clk/clk-npcm8xx.c new file +7 -0

+                                            mux_data->flags,
+                                            clk_base + NPCM8XX_CLKSEL,
+                                            mux_data->shift,
+                                            mux_data->mask, 0,
+                                            mux_data->table,
+                                            &npcm8xx_clk_lock);
+

Same here. Error handling?

Quoted 15 lines

drivers/clk/clk-npcm8xx.c new file +14 -0

+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register mux\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (mux_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[mux_data->onecell_idx] = hw;
+     }
+
+     /* Register clock dividers specified in npcm8xx_divs */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) {
+             const struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i];
+
+             hw = clk_hw_register_divider(dev, div_data->name,

drivers/clk/clk-npcm8xx.c line 727

Quoted 6 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+                                          div_data->parent_name,
+                                          div_data->flags,
+                                          clk_base + div_data->reg,
+                                          div_data->shift, div_data->width,
+                                          div_data->clk_divider_flags,
+                                          &npcm8xx_clk_lock);

devm_clk_hw_register_divider()?

Quoted 26 lines

drivers/clk/clk-npcm8xx.c new file +25 -0

+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register div table\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (div_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+     }
+
+     return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+                                        npcm8xx_clk_data);
+}
+
+static const struct of_device_id npcm8xx_clk_dt_ids[] = {
+     { .compatible = "nuvoton,npcm845-clk", },
+     { }
+};
+MODULE_DEVICE_TABLE(of, npcm8xx_clk_dt_ids);
+
+static struct platform_driver npcm8xx_clk_driver = {
+     .probe  = npcm8xx_clk_probe,
+     .driver = {
+             .name = "npcm8xx_clk",
+             .of_match_table = npcm8xx_clk_dt_ids,
+     },

drivers/clk/clk-npcm8xx.c line 760

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +8 -0

+};
+
+static int __init npcm8xx_clk_driver_init(void)
+{
+     return platform_driver_register(&npcm8xx_clk_driver);
+}
+arch_initcall(npcm8xx_clk_driver_init);
+

Hi Christophe,

Thanks for your comments.

Quoted 54 lines · Christophe JAILLET

Quoted 11 lines · Tomer Maimon

Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller which generates and supplies clocks to all modules within the BMC.

Signed-off-by Tomer Maimon <tmaimon77-Re5JQEeQqe8AvxtiuMwx3w-XMD5yJDbdMQAvxtiuMwx3w@public.gmane.organe.org>

---
   drivers/clk/Kconfig       |   6 +
   drivers/clk/Makefile      |   1 +
   drivers/clk/clk-npcm8xx.c | 760 ++++++++++++++++++++++++++++++++++++++
   3 files changed, 767 insertions(+)

create mode 100644 drivers/clk/clk-npcm8xx.c

[...]

Hi, below a few comments related to error handling and possible dev_err_probe() usage to savec a few LoC.

CJ

Quoted 20 lines

drivers/clk/clk-npcm8xx.c new file +20 -0

+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+                      const char *name, const char *parent_name,
+                      unsigned long flags)
+{
+     struct npcm8xx_clk_pll *pll;
+     struct clk_init_data init;
+     struct clk_hw *hw;
+     int ret;
+
+     pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+     if (!pll)
+             return ERR_PTR(-ENOMEM);
+
+     pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+
+     init.name = name;
+     init.ops = &npcm8xx_clk_pll_ops;
+     init.parent_names = &parent_name;
+     init.num_parents = 1;

drivers/clk/clk-npcm8xx.c line 100

Quoted 10 lines

drivers/clk/clk-npcm8xx.c new file +10 -0

+     init.flags = flags;
+
+     pll->pllcon = pllcon;
+     pll->hw.init = &init;
+
+     hw = &pll->hw;
+
+     ret = devm_clk_hw_register(dev, hw);
+     if (ret) {
+             kfree(pll);

Hi,

Quoted 3 lines

there is no other kfree(() in this patch. It is handled by the framework once the clk is registered or should there be another kfree() somewhere or should pll be devm_alloc()'ed?

Could use the devm_kzalloc but the hw is sent to the devm function and not the pll struct.

Quoted 31 lines

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+             return ERR_PTR(ret);
+     }
+
+     return hw;
+}
+

[...]

Quoted 20 lines

drivers/clk/clk-npcm8xx.c new file +20 -0

+static int npcm8xx_clk_probe(struct platform_device *pdev)
+{
+     struct clk_hw_onecell_data *npcm8xx_clk_data;
+     struct device *dev = &pdev->dev;
+     void __iomem *clk_base;
+     struct clk_hw *hw;
+     int i;
+
+     npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
+                                                      NPCM8XX_NUM_CLOCKS),
+                                     GFP_KERNEL);
+     if (!npcm8xx_clk_data)
+             return -ENOMEM;
+
+     clk_base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(clk_base))
+             return PTR_ERR(clk_base);
+
+     npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
+

Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 636

Quoted 5 lines

drivers/clk/clk-npcm8xx.c new file +5 -0

+     for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++)
+             npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+     /* Reference 25MHz clock */
+     hw = clk_hw_register_fixed_rate(dev, "refclk", NULL, 0, NPCM8XX_REF_CLK);

Other resoruces are managed, but not this one.
Is it on purpose?

This is the fixed clock - the SoC Reference clock, what do you mean by not managed?

Quoted 23 lines

Is an error handling path needed or a devm_add_action_or_reset()?

Quoted 21 lines

drivers/clk/clk-npcm8xx.c new file +21 -0

+     if (IS_ERR(hw))
+             return PTR_ERR(hw);
+     npcm8xx_clk_data->hws[NPCM8XX_CLK_REFCLK] = hw;
+
+     /* Register plls */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_plls); i++) {
+             const struct npcm8xx_clk_pll_data *pll_data = &npcm8xx_plls[i];
+
+             hw = npcm8xx_clk_register_pll(dev, clk_base + pll_data->reg,
+                                           pll_data->name,
+                                           pll_data->parent_name,
+                                           pll_data->flags);
+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register pll\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (pll_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[pll_data->onecell_idx] = hw;
+     }
+

Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 663

Quoted 6 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+     /* Register fixed dividers */
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL1_DIV2,
+                                            NPCM8XX_CLK_S_PLL1, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register fixed div\n");
+             return PTR_ERR(hw);

return dev_err_probe()?

it will save online but still enter to a bigger function, I am not sure that the error will be EPROBE_DEFER, and we have returned the error in the code.

Quoted 41 lines

drivers/clk/clk-npcm8xx.c line 670

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +7 -0

+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL2_DIV2,
+                                            NPCM8XX_CLK_S_PLL2, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register pll div2\n");
+             return PTR_ERR(hw);

Same here and in other calls below.

Quoted 30 lines

drivers/clk/clk-npcm8xx.c new file +30 -0

+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PRE_CLK,
+                                            NPCM8XX_CLK_S_CPU_MUX, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register ckclk div2\n");
+             return PTR_ERR(hw);
+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_AXI,
+                                            NPCM8XX_CLK_S_TH, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register axi div2\n");
+             return PTR_ERR(hw);
+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_ATB,
+                                            NPCM8XX_CLK_S_AXI, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register atb div2\n");
+             return PTR_ERR(hw);
+     }
+
+     /* Register muxes */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) {
+             const struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i];
+
+             hw = clk_hw_register_mux_table(dev, mux_data->name,
+                                            mux_data->parent_names,
+                                            mux_data->num_parents,

Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 707

Quoted 7 lines

drivers/clk/clk-npcm8xx.c new file +7 -0

+                                            mux_data->flags,
+                                            clk_base + NPCM8XX_CLKSEL,
+                                            mux_data->shift,
+                                            mux_data->mask, 0,
+                                            mux_data->table,
+                                            &npcm8xx_clk_lock);
+

Same here. Error handling?

what do you mean?

Quoted 15 lines

Quoted 15 lines

drivers/clk/clk-npcm8xx.c new file +14 -0

+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register mux\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (mux_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[mux_data->onecell_idx] = hw;
+     }
+
+     /* Register clock dividers specified in npcm8xx_divs */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) {
+             const struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i];
+
+             hw = clk_hw_register_divider(dev, div_data->name,

Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 727

Quoted 6 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+                                          div_data->parent_name,
+                                          div_data->flags,
+                                          clk_base + div_data->reg,
+                                          div_data->shift, div_data->width,
+                                          div_data->clk_divider_flags,
+                                          &npcm8xx_clk_lock);

devm_clk_hw_register_divider()?

Will do.

Quoted 26 lines

Quoted 26 lines

drivers/clk/clk-npcm8xx.c new file +25 -0

+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register div table\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (div_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+     }
+
+     return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+                                        npcm8xx_clk_data);
+}
+
+static const struct of_device_id npcm8xx_clk_dt_ids[] = {
+     { .compatible = "nuvoton,npcm845-clk", },
+     { }
+};
+MODULE_DEVICE_TABLE(of, npcm8xx_clk_dt_ids);
+
+static struct platform_driver npcm8xx_clk_driver = {
+     .probe  = npcm8xx_clk_probe,
+     .driver = {
+             .name = "npcm8xx_clk",
+             .of_match_table = npcm8xx_clk_dt_ids,
+     },

Quoted 9 lines

drivers/clk/clk-npcm8xx.c line 760

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +8 -0

+};
+
+static int __init npcm8xx_clk_driver_init(void)
+{
+     return platform_driver_register(&npcm8xx_clk_driver);
+}
+arch_initcall(npcm8xx_clk_driver_init);
+

Best regards,

Tomer

Quoted 60 lines · Tomer Maimon

Hi Christophe,

Thanks for your comments.

Quoted 54 lines · Christophe JAILLET

Quoted 11 lines · Tomer Maimon

Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller which generates and supplies clocks to all modules within the BMC.

Signed-off-by Tomer Maimon <tmaimon77-Re5JQEeQqe8AvxtiuMwx3w-XMD5yJDbdMQAvxtiuMwx3w@public.gmane.organe.org>

---
   drivers/clk/Kconfig       |   6 +
   drivers/clk/Makefile      |   1 +
   drivers/clk/clk-npcm8xx.c | 760 ++++++++++++++++++++++++++++++++++++++
   3 files changed, 767 insertions(+)

create mode 100644 drivers/clk/clk-npcm8xx.c

[...]

Hi, below a few comments related to error handling and possible dev_err_probe() usage to savec a few LoC.

CJ

Quoted 20 lines

drivers/clk/clk-npcm8xx.c new file +20 -0

+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+                      const char *name, const char *parent_name,
+                      unsigned long flags)
+{
+     struct npcm8xx_clk_pll *pll;
+     struct clk_init_data init;
+     struct clk_hw *hw;
+     int ret;
+
+     pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+     if (!pll)
+             return ERR_PTR(-ENOMEM);
+
+     pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+
+     init.name = name;
+     init.ops = &npcm8xx_clk_pll_ops;
+     init.parent_names = &parent_name;
+     init.num_parents = 1;

drivers/clk/clk-npcm8xx.c line 100

Quoted 10 lines

drivers/clk/clk-npcm8xx.c new file +10 -0

+     init.flags = flags;
+
+     pll->pllcon = pllcon;
+     pll->hw.init = &init;
+
+     hw = &pll->hw;
+
+     ret = devm_clk_hw_register(dev, hw);
+     if (ret) {
+             kfree(pll);

Hi,

Quoted 5 lines

Quoted 3 lines

there is no other kfree(() in this patch. It is handled by the framework once the clk is registered or should there be another kfree() somewhere or should pll be devm_alloc()'ed?

Could use the devm_kzalloc but the hw is sent to the devm function and not the pll struct.

My point, as explained below is that some resources are managed (i.e. allocated by a devm_ function and released by the framework if the probe fails or if the driver is removed), but not all.

I think that it is not consistent and that maybe the release of the corresponding resources should be handled in a way or another.

Quoted 40 lines

Quoted 32 lines

Quoted 9 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+             return ERR_PTR(ret);
+     }
+
+     return hw;
+}
+

[...]

Quoted 20 lines

drivers/clk/clk-npcm8xx.c new file +20 -0

+static int npcm8xx_clk_probe(struct platform_device *pdev)
+{
+     struct clk_hw_onecell_data *npcm8xx_clk_data;
+     struct device *dev = &pdev->dev;
+     void __iomem *clk_base;
+     struct clk_hw *hw;
+     int i;
+
+     npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
+                                                      NPCM8XX_NUM_CLOCKS),
+                                     GFP_KERNEL);
+     if (!npcm8xx_clk_data)
+             return -ENOMEM;
+
+     clk_base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(clk_base))
+             return PTR_ERR(clk_base);
+
+     npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
+

Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 636

Quoted 5 lines

drivers/clk/clk-npcm8xx.c new file +5 -0

+     for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++)
+             npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+     /* Reference 25MHz clock */
+     hw = clk_hw_register_fixed_rate(dev, "refclk", NULL, 0, NPCM8XX_REF_CLK);

Other resoruces are managed, but not this one.
Is it on purpose?

Quoted 2 lines

This is the fixed clock - the SoC Reference clock, what do you mean by not managed?

clk_hw_register_fixed_rate() allocates some memory. If an error occures later on or if the driver is .remove()'ed, this memory will leak. (unless I missed something).

Most of your other function calls use some devm_ variant, so the corresponding resources are released automatically in case of error or if the driver is unloaed.

Quoted 32 lines

Quoted 24 lines

Is an error handling path needed or a devm_add_action_or_reset()?

Quoted 21 lines

drivers/clk/clk-npcm8xx.c new file +21 -0

+     if (IS_ERR(hw))
+             return PTR_ERR(hw);
+     npcm8xx_clk_data->hws[NPCM8XX_CLK_REFCLK] = hw;
+
+     /* Register plls */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_plls); i++) {
+             const struct npcm8xx_clk_pll_data *pll_data = &npcm8xx_plls[i];
+
+             hw = npcm8xx_clk_register_pll(dev, clk_base + pll_data->reg,
+                                           pll_data->name,
+                                           pll_data->parent_name,
+                                           pll_data->flags);
+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register pll\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (pll_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[pll_data->onecell_idx] = hw;
+     }
+

Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 663

Quoted 6 lines

drivers/clk/clk-npcm8xx.c new file +6 -0

+     /* Register fixed dividers */
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL1_DIV2,
+                                            NPCM8XX_CLK_S_PLL1, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register fixed div\n");
+             return PTR_ERR(hw);

return dev_err_probe()?

Quoted 3 lines

it will save online but still enter to a bigger function, I am not sure that the error will be EPROBE_DEFER, and we have returned the error in the code.

It is mostly a matter of taste, so if you don't think my proposal improves anything, just forget about it.

Quoted 49 lines

Quoted 41 lines

drivers/clk/clk-npcm8xx.c line 670

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +7 -0

+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL2_DIV2,
+                                            NPCM8XX_CLK_S_PLL2, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register pll div2\n");
+             return PTR_ERR(hw);

Same here and in other calls below.

Quoted 30 lines

drivers/clk/clk-npcm8xx.c new file +30 -0

+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PRE_CLK,
+                                            NPCM8XX_CLK_S_CPU_MUX, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register ckclk div2\n");
+             return PTR_ERR(hw);
+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_AXI,
+                                            NPCM8XX_CLK_S_TH, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register axi div2\n");
+             return PTR_ERR(hw);
+     }
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_ATB,
+                                            NPCM8XX_CLK_S_AXI, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register atb div2\n");
+             return PTR_ERR(hw);
+     }
+
+     /* Register muxes */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) {
+             const struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i];
+
+             hw = clk_hw_register_mux_table(dev, mux_data->name,
+                                            mux_data->parent_names,
+                                            mux_data->num_parents,

Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 707

Quoted 7 lines

drivers/clk/clk-npcm8xx.c new file +7 -0

+                                            mux_data->flags,
+                                            clk_base + NPCM8XX_CLKSEL,
+                                            mux_data->shift,
+                                            mux_data->mask, 0,
+                                            mux_data->table,
+                                            &npcm8xx_clk_lock);
+

Same here. Error handling?

Quoted 1 line

what do you mean?

I mean the same as above. clk_hw_register_mux_table() allocates some memory. If an error occures later on or if the driver is .remove()'ed, this memory will leak. (unless I missed something).

Error handling was not a really good wording. You should read "should the resources allocated by this function be released if an error occures below"

Quoted 24 lines

Quoted 24 lines

Quoted 12 lines

drivers/clk/clk-npcm8xx.c new file +10 -0

+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register mux\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (mux_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[mux_data->onecell_idx] = hw;
+     }
+
+     /* Register clock dividers specified in npcm8xx_divs */

drivers/clk/clk-npcm8xx.c line 727

Quoted 10 lines

drivers/clk/clk-npcm8xx.c new file +10 -0

+     for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) {
+             const struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i];
+
+             hw = clk_hw_register_divider(dev, div_data->name,
+                                          div_data->parent_name,
+                                          div_data->flags,
+                                          clk_base + div_data->reg,
+                                          div_data->shift, div_data->width,
+                                          div_data->clk_divider_flags,
+                                          &npcm8xx_clk_lock);

devm_clk_hw_register_divider()?

Quoted 1 line

Will do.

Here, the devm_ alternative exist. But my point is the same as above. (release of resources in cae of error or if the driver is removed)

Quoted 40 lines

Quoted 26 lines

Quoted 26 lines

drivers/clk/clk-npcm8xx.c new file +25 -0

+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register div table\n");
+                     return PTR_ERR(hw);
+             }
+
+             if (div_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+     }
+
+     return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+                                        npcm8xx_clk_data);
+}
+
+static const struct of_device_id npcm8xx_clk_dt_ids[] = {
+     { .compatible = "nuvoton,npcm845-clk", },
+     { }
+};
+MODULE_DEVICE_TABLE(of, npcm8xx_clk_dt_ids);
+
+static struct platform_driver npcm8xx_clk_driver = {
+     .probe  = npcm8xx_clk_probe,
+     .driver = {
+             .name = "npcm8xx_clk",
+             .of_match_table = npcm8xx_clk_dt_ids,
+     },

Quoted 9 lines

drivers/clk/clk-npcm8xx.c line 760

Quoted 8 lines

drivers/clk/clk-npcm8xx.c new file +8 -0

+};
+
+static int __init npcm8xx_clk_driver_init(void)
+{
+     return platform_driver_register(&npcm8xx_clk_driver);
+}
+arch_initcall(npcm8xx_clk_driver_init);
+

Best regards,

Tomer

Describe syscon property that handles general control registers (GCR) in Nuvoton BMC NPCM reset driver.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../devicetree/bindings/reset/nuvoton,npcm750-reset.yaml     | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/reset/nuvoton,npcm750-reset.yaml +0 -0

index fa5e4ea6400e..7ba3a6ff82ed 100644

Quoted 16 lines · Tomer Maimon

Describe syscon property that handles general control registers (GCR) in Nuvoton BMC NPCM reset driver.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../devicetree/bindings/reset/nuvoton,npcm750-reset.yaml     | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/reset/nuvoton,npcm750-reset.yaml +0 -0

index fa5e4ea6400e..7ba3a6ff82ed 100644
@@ -19,6 +19,9 @@ properties:
   '#reset-cells':
     const: 2
 

Documentation/devicetree/bindings/reset/nuvoton,npcm750-reset.yaml properties: · line 23

Quoted 2 lines

Documentation/devicetree/bindings/reset/nuvoton,npcm750-reset.yaml +2 -0

+  nuvoton,sysgcr:
+    description: a phandle to access GCR registers.

That's not valid. You miss here ref. My comment was to skip quotes and you removed entire ref.

Best regards,
Krzysztof

Add binding document and device tree binding constants for Nuvoton BMC NPCM8XX reset controller.

Signed-off-by Tomer Maimon <[email protected]>

This adds initial device tree support for the Nuvoton NPCM845 Board Management controller (BMC) SoC family.

The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and have various peripheral IPs.

Signed-off-by Tomer Maimon <[email protected]>

Add initial Nuvoton NPCM845 evaluation board device tree.

Signed-off-by Tomer Maimon <[email protected]>

---
 arch/arm64/boot/dts/nuvoton/Makefile          |  2 ++
 .../boot/dts/nuvoton/nuvoton-npcm845-evb.dts  | 30 +++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 arch/arm64/boot/dts/nuvoton/Makefile
 create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts

Enable basic drivers for NPCM8XX booting up support: Architecture, Clock, and WD.

Signed-off-by Tomer Maimon <[email protected]>