Skip to contentSkip to conversation

Introduce Nuvoton Arbel NPCM8XX BMC SoC

PATCH Superseded Tomer Maimon 2 participants 7 messages May 21, 2023 – Jun 1, 2023

Subsystems (2)
Common clk Arm/nuvoton npcm architecture

v16 of 28 Superseded Latest is v28 →

lore
Share & tools
lore

Tomer Maimon

author maintainer
#0 lore raw

v16 cover Introduce Nuvoton Arbel NPCM8XX BMC SoC

This patchset adds clock support for the Nuvoton Arbel NPCM8XX Board Management controller (BMC) SoC family.

This patchset cover letter is based from the initial support for NPCM8xx BMC to keep tracking the version history.

all the other initial support patches had been applied to Linux kernel 6.0.

This patchset was tested on the Arbel NPCM8XX evaluation board.

Addressed comments from:

- Stephen Boyd: https://www.spinics.net/lists/kernel/msg4729384.html

Changes since version 15:
- NPCM8XX clock driver
- Remove unused regs parameter from npcm8xx_pll_data structure.
- Using index and clk_hw parameters to set the clock parent in the clock structures.

Changes since version 14:
- NPCM8XX clock driver
- Remove unnecessary register definitions.
- Remove the internal reference clock, instead use the external DT reference clock.
- rearrange the driver.
- using .names parameter in DT to define clock (refclk).

Changes since version 13:
- NPCM8XX clock driver
- Remove unnecessary definitions and add module.h define
- Use in clk_parent_data struct.fw_name and .name.
- Add module_exit function.
- Add const to divider clock names.
- Add MODULE_DESCRIPTION and MODULE_LICENSE

Changes since version 12:
- NPCM8XX clock driver
- Use clk_parent_data in mux and div clock structure.
- Add const to mux tables.
- Using devm_clk_hw_register_fixed_rate function.
- use only .name clk_parent_data instead .name and .fw_name.
- Modify mask values in mux clocks.

Changes since version 11:
- NPCM8XX clock driver
- Modify Kconfig help.
- Modify loop variable to unsigned int.

Changes since version 11:
- NPCM8XX clock driver
- Modify Kconfig help.
- Modify loop variable to unsigned int.

Changes since version 10:
- NPCM8XX clock driver
- Fix const warning.

Changes since version 9:
- NPCM8XX clock driver
- Move configuration place.
- Using clk_parent_data instead of parent_name
- using devm_ioremap instead of ioremap. deeply sorry, I know we had a long discussion on what should the driver use, from other examples (also in other clock drivers) I see the combination of platform_get_resource and devm_ioremap are commonly used and it answer the reset and clock needs.

Changes since version 8:
- NPCM8XX clock driver
- Move configuration place.
- Add space before and aftre '{' '}'.
- Handle devm_of_clk_add_hw_provider function error.

Changes since version 7:
- NPCM8XX clock driver
- The clock and reset registers using the same memory region, due to it the clock driver should claim the ioremap directly without checking the memory region.

Changes since version 6:
- NPCM reset driver
- Modify warning message.
- dt-bindings: serial: 8250: Add npcm845 compatible string patch accepted, due to it the patch removed from the patchset.

Changes since version 5:
- NPCM8XX clock driver
- Remove refclk if devm_of_clk_add_hw_provider function failed.
- NPCM8XX clock source driver
- Remove NPCM8XX TIMER_OF_DECLARE support, using the same as NPCM7XX.

Changes since version 4:
- NPCM8XX clock driver
- Use the same quote in the dt-binding file.

Changes since version 3:
- NPCM8XX clock driver
- Rename NPCM8xx clock dt-binding header file.
- Remove unused structures.
- Improve Handling the clocks registration.
- NPCM reset driver
- Add ref phandle to dt-binding.

Changes since version 2:
- Remove NPCM8xx WDT compatible patch.
- Remove NPCM8xx UART compatible patch.
- NPCM8XX clock driver
- Add debug new line.
- Add 25M fixed rate clock.
- Remove unused clocks and clock name from dt-binding.
- NPCM reset driver
- Revert to npcm7xx dt-binding.
- Skip dt binding quotes.
- Adding DTS backward compatibility.
- Remove NPCM8xx binding include file.
- Warp commit message.
- NPCM8XX device tree:
- Remove unused clock nodes (used in the clock driver)
- Modify gcr and rst node names.

Changes since version 1:
- NPCM8XX clock driver
- Modify dt-binding.
- Remove unsed definition and include.
- Include alphabetically.
- Use clock devm.
- NPCM reset driver
- Modify dt-binding.
- Modify syscon name.
- Add syscon support to NPCM7XX dts reset node.
- use data structure.
- NPCM8XX device tree:
- Modify evb compatible name.
- Add NPCM7xx compatible.
- Remove disable nodes from the EVB DTS.

Tomer Maimon (1):
clk: npcm8xx: add clock controller

 drivers/clk/Kconfig       |   8 +
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-npcm8xx.c | 566 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 575 insertions(+)
 create mode 100644 drivers/clk/clk-npcm8xx.c
Signature · 1 line
2.33.0
author maintainer
#1 lore raw

PATCH v16 1/1 clk: npcm8xx: add clock controller

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       |   8 +
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-npcm8xx.c | 566 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 575 insertions(+)
 create mode 100644 drivers/clk/clk-npcm8xx.c
drivers/clk/Kconfig +8 -0
index 016814e15536..2249de28a46a 100644
@@ -325,6 +325,14 @@ config COMMON_CLK_LOCHNAGAR
 	  This driver supports the clocking features of the Cirrus Logic
 	  Lochnagar audio development board.
 
+config COMMON_CLK_NPCM8XX
+	tristate "Clock driver for the NPCM8XX SoC Family"
+	depends on ARCH_NPCM || COMPILE_TEST
+	help
+	  This driver supports the clocks on the Nuvoton BMC NPCM8XX SoC Family,
+	  all the clocks are initialized by the bootloader, so this driver
+	  allows only reading of current settings directly from the hardware.
+
 config COMMON_CLK_LOONGSON2
 	bool "Clock driver for Loongson-2 SoC"
 	depends on LOONGARCH || COMPILE_TEST
drivers/clk/Makefile +1 -0
index 0aebef17edc6..88713cbfbab5 100644
@@ -51,6 +51,7 @@ obj-$(CONFIG_ARCH_MILBEAUT_M10V)	+= clk-milbeaut.o
 obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
 obj-$(CONFIG_ARCH_NOMADIK)		+= clk-nomadik.o
 obj-$(CONFIG_ARCH_NPCM7XX)	    	+= clk-npcm7xx.o
+obj-$(CONFIG_COMMON_CLK_NPCM8XX)	+= clk-npcm8xx.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= clk-nspire.o
 obj-$(CONFIG_COMMON_CLK_OXNAS)		+= clk-oxnas.o
 obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
drivers/clk/clk-npcm8xx.c new file +566 -0

566 lines not on this page · raw message

Signature · 1 line
2.33.0
#2 lore raw

Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller

Le 21/05/2023 à 16:32, Tomer Maimon a écrit :

Quoted 4 lines

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]>

Hi,

should there be a v17, below a few nits and questions.

Jusrt my 2c,

Quoted 44 lines
---
  drivers/clk/Kconfig       |   8 +
  drivers/clk/Makefile      |   1 +
  drivers/clk/clk-npcm8xx.c | 566 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 575 insertions(+)

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

drivers/clk/Kconfig +8 -0
index 016814e15536..2249de28a46a 100644
@@ -325,6 +325,14 @@ config COMMON_CLK_LOCHNAGAR
  	  This driver supports the clocking features of the Cirrus Logic
  	  Lochnagar audio development board.
 
+config COMMON_CLK_NPCM8XX
+	tristate "Clock driver for the NPCM8XX SoC Family"
+	depends on ARCH_NPCM || COMPILE_TEST
+	help
+	  This driver supports the clocks on the Nuvoton BMC NPCM8XX SoC Family,
+	  all the clocks are initialized by the bootloader, so this driver
+	  allows only reading of current settings directly from the hardware.
+
  config COMMON_CLK_LOONGSON2
  	bool "Clock driver for Loongson-2 SoC"
  	depends on LOONGARCH || COMPILE_TEST
drivers/clk/Makefile +1 -0
index 0aebef17edc6..88713cbfbab5 100644
@@ -51,6 +51,7 @@ obj-$(CONFIG_ARCH_MILBEAUT_M10V)	+= clk-milbeaut.o
  obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
  obj-$(CONFIG_ARCH_NOMADIK)		+= clk-nomadik.o
  obj-$(CONFIG_ARCH_NPCM7XX)	    	+= clk-npcm7xx.o
+obj-$(CONFIG_COMMON_CLK_NPCM8XX)	+= clk-npcm8xx.o
  obj-$(CONFIG_ARCH_NSPIRE)		+= clk-nspire.o
  obj-$(CONFIG_COMMON_CLK_OXNAS)		+= clk-oxnas.o
  obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
drivers/clk/clk-npcm8xx.c new file +0 -0
new file mode 100644
index 000000000000..9e6ed073d124

drivers/clk/clk-npcm8xx.c line 4

Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4 -0
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton NPCM8xx Clock Generator
+ * All the clocks are initialized by the bootloader, so this driver allow only

nit: allows (as in Kconfig)

[...]

drivers/clk/clk-npcm8xx.c line 32

Quoted 11 lines
drivers/clk/clk-npcm8xx.c new file +11 -0
+/* npcm8xx clock registers*/
+#define NPCM8XX_CLKSEL          (0x04)
+#define NPCM8XX_CLKDIV1         (0x08)
+#define NPCM8XX_CLKDIV2         (0x2C)
+#define NPCM8XX_CLKDIV3         (0x58)
+#define NPCM8XX_CLKDIV4         (0x7C)
+#define NPCM8XX_PLLCON0         (0x0C)
+#define NPCM8XX_PLLCON1         (0x10)
+#define NPCM8XX_PLLCON2         (0x54)
+#define NPCM8XX_PLLCONG         (0x60)
+#define NPCM8XX_THRTL_CNT       (0xC0)

nit: () around this values looks superfluous.

Quoted 3 lines
drivers/clk/clk-npcm8xx.c new file +3 -0
+
+#define PLLCON_LOKI	BIT(31)
+#define PLLCON_LOKS	BIT(30)

[...]

drivers/clk/clk-npcm8xx.c line 360

Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10 -0
+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+			 const char *name, const struct clk_parent_data *parent,
+			 unsigned long flags)
+{
+	struct npcm8xx_clk_pll *pll;
+	struct clk_init_data init = {};
+	int ret;
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);

Everything looks devm_()'ed in this driver, except this kzalloc.
Except the one below, there is no kfree to free this memory, and no .remove() function.

Is it on purpose?

Quoted 33 lines
drivers/clk/clk-npcm8xx.c new file +33 -0
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &npcm8xx_clk_pll_ops;
+	init.parent_data = parent;
+	init.num_parents = 1;
+	init.flags = flags;
+
+	pll->pllcon = pllcon;
+	pll->hw.init = &init;
+
+	ret = devm_clk_hw_register(dev, &pll->hw);
+	if (ret) {
+		kfree(pll);
+		return ERR_PTR(ret);
+	}
+
+	return &pll->hw;
+}
+
+static DEFINE_SPINLOCK(npcm8xx_clk_lock);
+
+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 resource *res;
+	struct clk_hw *hw;
+	unsigned int i;
+	int err;
+

drivers/clk/clk-npcm8xx.c line 403

Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10 -0
+	npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
+							 NPCM8XX_NUM_CLOCKS),
+					GFP_KERNEL);
+	if (!npcm8xx_clk_data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	clk_base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!clk_base) {
+		dev_err(&pdev->dev, "Failed to remap I/O memory\n");

Here and below: mostly a matter of taste, but usually return dev_err_probe() saves a few LoC and keep the error code in the error message.

Quoted 63 lines
drivers/clk/clk-npcm8xx.c new file +63 -0
+		return -ENOMEM;
+	}
+
+	npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
+
+	for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++)
+		npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	/* Register plls */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_pll_clks); i++) {
+		struct npcm8xx_pll_data *pll_clk = &npcm8xx_pll_clks[i];
+
+		hw = npcm8xx_clk_register_pll(dev, clk_base + pll_clk->reg,
+					      pll_clk->name, &pll_clk->parent,
+					      pll_clk->flags);
+		if (IS_ERR(hw)) {
+			dev_err(dev, "npcm8xx_clk: Can't register pll\n");
+			return PTR_ERR(hw);
+		}
+		pll_clk->hw = *hw;
+	}
+
+	/* 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);
+	}
+	hw_pll1_div2 = *hw;
+
+	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 pll2 div2\n");
+		return PTR_ERR(hw);
+	}
+	hw_pll2_div2 = *hw;
+
+	hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL_GFX_DIV2,
+					       NPCM8XX_CLK_S_PLL_GFX, 0, 1, 2);
+	if (IS_ERR(hw)) {
+		dev_err(dev, "npcm8xx_clk: Can't register gfx div2\n");
+		return PTR_ERR(hw);
+	}
+	hw_gfx_div2 = *hw;
+
+	/* Register muxes */
+	for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) {
+		struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i];
+
+		hw = devm_clk_hw_register_mux_parent_data_table(dev,
+								mux_data->name,
+								mux_data->parent_data,
+								mux_data->num_parents,
+								mux_data->flags,
+								clk_base + NPCM8XX_CLKSEL,
+								mux_data->shift,
+								mux_data->mask,
+								0,
+								mux_data->table,
+								&npcm8xx_clk_lock);
+		if (IS_ERR(hw)) {

drivers/clk/clk-npcm8xx.c line 476

Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10 -0
+			dev_err(dev, "npcm8xx_clk: Can't register mux\n");
+			return PTR_ERR(hw);
+		}
+		mux_data->hw = *hw;
+	}
+
+	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");

ckclk or just clk?

Based on naming only, this div2 (here and below) looks strange. Is it on purpose, on just a cut'n'paste from the previous message?

Quoted 46 lines
drivers/clk/clk-npcm8xx.c new file +46 -0
+		return PTR_ERR(hw);
+	}
+	hw_pre_clk = *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);
+	}
+	npcm8xx_clk_data->hws[NPCM8XX_CLK_AXI] = 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);
+	}
+	npcm8xx_clk_data->hws[NPCM8XX_CLK_ATB] = 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_parent_data(dev, div_data->name,
+							 &div_data->parent_data,
+							 div_data->flags,
+							 clk_base + div_data->reg,
+							 div_data->shift,
+							 div_data->width,
+							 div_data->clk_divider_flags,
+							 &npcm8xx_clk_lock);
+		if (IS_ERR(hw)) {
+			dev_err(dev, "npcm8xx_clk: Can't register div table\n");
+			goto err_div_clk;
+		}
+
+		if (div_data->onecell_idx >= 0)
+			npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+	}
+
+	err = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					  npcm8xx_clk_data);
+	if (err) {
+		dev_err(dev, "unable to add clk provider\n");
+		hw = ERR_PTR(err);

drivers/clk/clk-npcm8xx.c line 532

Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10 -0
+		goto err_div_clk;
+	}
+
+	return err;
+
+err_div_clk:
+	while (i--) {
+		if (npcm8xx_divs[i].onecell_idx >= 0)
+			clk_hw_unregister_divider(npcm8xx_clk_data->hws[npcm8xx_divs[i].onecell_idx]);
+	}

There is an error handling path in the probe, but no .remove() function. Should there be one?

CJ

[...]

author maintainer
#3 lore raw

Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller

Hi Christophe,

Thanks for your comments

Quoted 10 lines · Christophe JAILLET
Quoted 4 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]>

Hi,

should there be a v17, below a few nits and questions.

Let's wait for Stephen and see if he has other comments.

Quoted 45 lines

Jusrt my 2c,

Quoted 42 lines
---
  drivers/clk/Kconfig       |   8 +
  drivers/clk/Makefile      |   1 +
  drivers/clk/clk-npcm8xx.c | 566 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 575 insertions(+)

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

drivers/clk/Kconfig +8 -0
index 016814e15536..2249de28a46a 100644
@@ -325,6 +325,14 @@ config COMMON_CLK_LOCHNAGAR
        This driver supports the clocking features of the Cirrus Logic
        Lochnagar audio development board.
 
+config COMMON_CLK_NPCM8XX
+     tristate "Clock driver for the NPCM8XX SoC Family"
+     depends on ARCH_NPCM || COMPILE_TEST
+     help
+       This driver supports the clocks on the Nuvoton BMC NPCM8XX SoC Family,
+       all the clocks are initialized by the bootloader, so this driver
+       allows only reading of current settings directly from the hardware.
+
  config COMMON_CLK_LOONGSON2
      bool "Clock driver for Loongson-2 SoC"
      depends on LOONGARCH || COMPILE_TEST
drivers/clk/Makefile +1 -0
index 0aebef17edc6..88713cbfbab5 100644
@@ -51,6 +51,7 @@ obj-$(CONFIG_ARCH_MILBEAUT_M10V)    += clk-milbeaut.o
  obj-$(CONFIG_ARCH_MOXART)           += clk-moxart.o
  obj-$(CONFIG_ARCH_NOMADIK)          += clk-nomadik.o
  obj-$(CONFIG_ARCH_NPCM7XX)          += clk-npcm7xx.o
+obj-$(CONFIG_COMMON_CLK_NPCM8XX)     += clk-npcm8xx.o
  obj-$(CONFIG_ARCH_NSPIRE)           += clk-nspire.o
  obj-$(CONFIG_COMMON_CLK_OXNAS)              += clk-oxnas.o
  obj-$(CONFIG_COMMON_CLK_PALMAS)             += clk-palmas.o
drivers/clk/clk-npcm8xx.c new file +0 -1
new file mode 100644
index 000000000000..9e6ed073d124
--- /dev/null
Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 4

Quoted 6 lines

+++ b/drivers/clk/clk-npcm8xx.c

drivers/clk/clk-npcm8xx.c new file +4 -0
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton NPCM8xx Clock Generator
+ * All the clocks are initialized by the bootloader, so this driver allow only

nit: allows (as in Kconfig)

will modify in V17

Quoted 16 lines

[...]

drivers/clk/clk-npcm8xx.c line 32

Quoted 11 lines
drivers/clk/clk-npcm8xx.c new file +11 -0
+/* npcm8xx clock registers*/
+#define NPCM8XX_CLKSEL          (0x04)
+#define NPCM8XX_CLKDIV1         (0x08)
+#define NPCM8XX_CLKDIV2         (0x2C)
+#define NPCM8XX_CLKDIV3         (0x58)
+#define NPCM8XX_CLKDIV4         (0x7C)
+#define NPCM8XX_PLLCON0         (0x0C)
+#define NPCM8XX_PLLCON1         (0x10)
+#define NPCM8XX_PLLCON2         (0x54)
+#define NPCM8XX_PLLCONG         (0x60)
+#define NPCM8XX_THRTL_CNT       (0xC0)

nit: () around this values looks superfluous.

I am not sure, it can work both ways.

Quoted 7 lines
Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +3 -0
+
+#define PLLCON_LOKI  BIT(31)
+#define PLLCON_LOKS  BIT(30)

[...]

Quoted 14 lines

drivers/clk/clk-npcm8xx.c line 360

Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10 -0
+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+                      const char *name, const struct clk_parent_data *parent,
+                      unsigned long flags)
+{
+     struct npcm8xx_clk_pll *pll;
+     struct clk_init_data init = {};
+     int ret;
+
+     pll = kzalloc(sizeof(*pll), GFP_KERNEL);

Everything looks devm_()'ed in this driver, except this kzalloc. Except the one below, there is no kfree to free this memory, and no .remove() function.

Also clk_hw_register_divider_parent_data doesn't use devm_ about free the pll, we use it, return at the end of the function. about adding remove, we had a dissection about it in V4, since the clock is a service driver it shouldn't be removed.
https://patchwork.kernel.org/project/linux-watchdog/patch/[email protected]/

Quoted 42 lines

Is it on purpose?

Quoted 39 lines
drivers/clk/clk-npcm8xx.c new file +39 -0
+     if (!pll)
+             return ERR_PTR(-ENOMEM);
+
+     init.name = name;
+     init.ops = &npcm8xx_clk_pll_ops;
+     init.parent_data = parent;
+     init.num_parents = 1;
+     init.flags = flags;
+
+     pll->pllcon = pllcon;
+     pll->hw.init = &init;
+
+     ret = devm_clk_hw_register(dev, &pll->hw);
+     if (ret) {
+             kfree(pll);
+             return ERR_PTR(ret);
+     }
+
+     return &pll->hw;
+}
+
+static DEFINE_SPINLOCK(npcm8xx_clk_lock);
+
+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 resource *res;
+     struct clk_hw *hw;
+     unsigned int i;
+     int err;
+
+     npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
+                                                      NPCM8XX_NUM_CLOCKS),
+                                     GFP_KERNEL);
+     if (!npcm8xx_clk_data)
+             return -ENOMEM;
+
Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 403

Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4 -0
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     clk_base = devm_ioremap(dev, res->start, resource_size(res));
+     if (!clk_base) {
+             dev_err(&pdev->dev, "Failed to remap I/O memory\n");

Here and below: mostly a matter of taste, but usually return dev_err_probe() saves a few LoC and keep the error code in the error message.

Thanks, but I prefer the shorter way :-)

Quoted 68 lines
Quoted 68 lines
drivers/clk/clk-npcm8xx.c new file +67 -0
+             return -ENOMEM;
+     }
+
+     npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
+
+     for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++)
+             npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+     /* Register plls */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_pll_clks); i++) {
+             struct npcm8xx_pll_data *pll_clk = &npcm8xx_pll_clks[i];
+
+             hw = npcm8xx_clk_register_pll(dev, clk_base + pll_clk->reg,
+                                           pll_clk->name, &pll_clk->parent,
+                                           pll_clk->flags);
+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register pll\n");
+                     return PTR_ERR(hw);
+             }
+             pll_clk->hw = *hw;
+     }
+
+     /* 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);
+     }
+     hw_pll1_div2 = *hw;
+
+     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 pll2 div2\n");
+             return PTR_ERR(hw);
+     }
+     hw_pll2_div2 = *hw;
+
+     hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_PLL_GFX_DIV2,
+                                            NPCM8XX_CLK_S_PLL_GFX, 0, 1, 2);
+     if (IS_ERR(hw)) {
+             dev_err(dev, "npcm8xx_clk: Can't register gfx div2\n");
+             return PTR_ERR(hw);
+     }
+     hw_gfx_div2 = *hw;
+
+     /* Register muxes */
+     for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) {
+             struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i];
+
+             hw = devm_clk_hw_register_mux_parent_data_table(dev,
+                                                             mux_data->name,
+                                                             mux_data->parent_data,
+                                                             mux_data->num_parents,
+                                                             mux_data->flags,
+                                                             clk_base + NPCM8XX_CLKSEL,
+                                                             mux_data->shift,
+                                                             mux_data->mask,
+                                                             0,
+                                                             mux_data->table,
+                                                             &npcm8xx_clk_lock);
+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register mux\n");
+                     return PTR_ERR(hw);
+             }
+             mux_data->hw = *hw;
Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 476

Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6 -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");

ckclk or just clk?

will modify in V17

Quoted 55 lines

Based on naming only, this div2 (here and below) looks strange. Is it on purpose, on just a cut'n'paste from the previous message?

Quoted 51 lines
drivers/clk/clk-npcm8xx.c new file +51 -0
+             return PTR_ERR(hw);
+     }
+     hw_pre_clk = *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);
+     }
+     npcm8xx_clk_data->hws[NPCM8XX_CLK_AXI] = 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);
+     }
+     npcm8xx_clk_data->hws[NPCM8XX_CLK_ATB] = 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_parent_data(dev, div_data->name,
+                                                      &div_data->parent_data,
+                                                      div_data->flags,
+                                                      clk_base + div_data->reg,
+                                                      div_data->shift,
+                                                      div_data->width,
+                                                      div_data->clk_divider_flags,
+                                                      &npcm8xx_clk_lock);
+             if (IS_ERR(hw)) {
+                     dev_err(dev, "npcm8xx_clk: Can't register div table\n");
+                     goto err_div_clk;
+             }
+
+             if (div_data->onecell_idx >= 0)
+                     npcm8xx_clk_data->hws[div_data->onecell_idx] = hw;
+     }
+
+     err = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+                                       npcm8xx_clk_data);
+     if (err) {
+             dev_err(dev, "unable to add clk provider\n");
+             hw = ERR_PTR(err);
+             goto err_div_clk;
+     }
+
+     return err;
+
Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 532

Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +5 -0
+err_div_clk:
+     while (i--) {
+             if (npcm8xx_divs[i].onecell_idx >= 0)
+                     clk_hw_unregister_divider(npcm8xx_clk_data->hws[npcm8xx_divs[i].onecell_idx]);
+     }

There is an error handling path in the probe, but no .remove() function. Should there be one?

The same answer as above (V4)

Quoted 4 lines

CJ

[...]

Best regards,

Tomer

#4 lore raw

Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller

Le 22/05/2023 à 14:56, Tomer Maimon a écrit :

Quoted 4 lines

Hi Christophe,

Thanks for your comments

[...]

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6 -0
+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+                      const char *name, const struct clk_parent_data *parent,
+                      unsigned long flags)
+{
+     struct npcm8xx_clk_pll *pll;
Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 360

Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4 -0
+     struct clk_init_data init = {};
+     int ret;
+
+     pll = kzalloc(sizeof(*pll), GFP_KERNEL);

Everything looks devm_()'ed in this driver, except this kzalloc. Except the one below, there is no kfree to free this memory, and no .remove() function.

Quoted 5 lines

Also clk_hw_register_divider_parent_data doesn't use devm_ about free the pll, we use it, return at the end of the function. about adding remove, we had a dissection about it in V4, since the clock is a service driver it shouldn't be removed.
https://patchwork.kernel.org/project/linux-watchdog/patch/[email protected]/

LoL.
At least, I'm consistent :).

Just to make it clear, what I mean about kfree() is not to add one here, but either:
- to use devm_kzalloc() here, to avoid a leak, should loading the
driver fails OR
- have some kfree() where needed (at least in the error handling
path of the probe, if the remove function makes no point)

CJ

author maintainer
#5 lore raw

Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller

Quoted 32 lines · Christophe JAILLET
Quoted 4 lines · Tomer Maimon

Hi Christophe,

Thanks for your comments

[...]

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6 -0
+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+                      const char *name, const struct clk_parent_data *parent,
+                      unsigned long flags)
+{
+     struct npcm8xx_clk_pll *pll;
Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 360

Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4 -0
+     struct clk_init_data init = {};
+     int ret;
+
+     pll = kzalloc(sizeof(*pll), GFP_KERNEL);

Everything looks devm_()'ed in this driver, except this kzalloc. Except the one below, there is no kfree to free this memory, and no .remove() function.

Quoted 5 lines

Also clk_hw_register_divider_parent_data doesn't use devm_ about free the pll, we use it, return at the end of the function. about adding remove, we had a dissection about it in V4, since the clock is a service driver it shouldn't be removed.
https://patchwork.kernel.org/project/linux-watchdog/patch/[email protected]/

LoL.
At least, I'm consistent :).

Quoted 6 lines

Just to make it clear, what I mean about kfree() is not to add one here, but either:
- to use devm_kzalloc() here, to avoid a leak, should loading the
driver fails OR
- have some kfree() where needed (at least in the error handling
path of the probe, if the remove function makes no point)

O.K. Thanks for your clarification.

Quoted 2 lines

CJ

Best regards,

Tomer

author maintainer
#6 lore raw

Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller

Hi Stephen,

Kind reminder regarding the patch, appreciate your comments.

Thanks,

Tomer

Quoted 48 lines · Tomer Maimon
Quoted 32 lines · Christophe JAILLET
Quoted 4 lines · Tomer Maimon

Hi Christophe,

Thanks for your comments

[...]

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6 -0
+static struct clk_hw *
+npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
+                      const char *name, const struct clk_parent_data *parent,
+                      unsigned long flags)
+{
+     struct npcm8xx_clk_pll *pll;
Quoted 8 lines

drivers/clk/clk-npcm8xx.c line 360

Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4 -0
+     struct clk_init_data init = {};
+     int ret;
+
+     pll = kzalloc(sizeof(*pll), GFP_KERNEL);

Everything looks devm_()'ed in this driver, except this kzalloc. Except the one below, there is no kfree to free this memory, and no .remove() function.

Quoted 5 lines

Also clk_hw_register_divider_parent_data doesn't use devm_ about free the pll, we use it, return at the end of the function. about adding remove, we had a dissection about it in V4, since the clock is a service driver it shouldn't be removed.
https://patchwork.kernel.org/project/linux-watchdog/patch/[email protected]/

LoL.
At least, I'm consistent :).

Quoted 6 lines

Just to make it clear, what I mean about kfree() is not to add one here, but either:
- to use devm_kzalloc() here, to avoid a leak, should loading the
driver fails OR
- have some kfree() where needed (at least in the error handling
path of the probe, if the remove function makes no point)

O.K. Thanks for your clarification.

Quoted 2 lines

CJ

Best regards,

Tomer