Introduce Nuvoton Arbel NPCM8XX BMC SoC
Subsystems (2)
Tomer Maimon
maintainerv15 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/linux-clk/msg78406.html
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 | 561 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 570 insertions(+) create mode 100644 drivers/clk/clk-npcm8xx.c
Signature · 1 line
2.33.0
Tomer Maimon
maintainerPATCH v15 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.
--- drivers/clk/Kconfig | 8 + drivers/clk/Makefile | 1 + drivers/clk/clk-npcm8xx.c | 561 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 570 insertions(+) create mode 100644 drivers/clk/clk-npcm8xx.c
drivers/clk/Kconfig +8 -0
@@ -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_NXP def_bool COMMON_CLK && (ARCH_LPC18XX || ARCH_LPC32XX) select REGMAP_MMIO if ARCH_LPC32XX
drivers/clk/Makefile +1 -0
@@ -49,6 +49,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 +561 -0
561 lines not on this page · raw message
Signature · 1 line
2.33.0
Stephen Boyd
maintainerRe: [PATCH v15 1/1] clk: npcm8xx: add clock controller
Quoting Tomer Maimon (2023-03-09 11:44:02)
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +0
-0@@ -0,0 +1,561 @@
[...]
Quoted 29 lines
drivers/clk/clk-npcm8xx.c new file +29
-0+ +struct npcm8xx_pll_data { +const char *name; +struct clk_parent_data parent; +unsigned int reg; +unsigned long flags; +}; + +struct npcm8xx_clk_div_data { +u32 reg; +u8 shift; +u8 width; +const char *name; +const struct clk_parent_data parent_data; +u8 clk_divider_flags; +unsigned long flags; +int onecell_idx; +}; + +struct npcm8xx_clk_mux_data { +u8 shift; +u32 mask; +const u32 *table; +const char *name; +const struct clk_parent_data *parent_data; +u8 num_parents; +unsigned long flags; +}; +
[...]
drivers/clk/clk-npcm8xx.c
Quoted 2 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ +static struct npcm8xx_pll_data npcm8xx_pll_clks[] = {
Can this be const?
drivers/clk/clk-npcm8xx.c line 153
Quoted 9 lines
drivers/clk/clk-npcm8xx.c new file +9
-0+ { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 }, + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 }, + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 }, + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 }, +}; + +static const u32 cpuck_mux_table[] = {0 ,1 ,2 ,7 }; +static const struct clk_parent_data cpuck_mux_parents[] = { + { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },
You should only have .fw_name or .index when introducing new drivers. The .name field is for existing drivers that want to migrate to clk_parent_data.
drivers/clk/clk-npcm8xx.c
Quoted 2 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 }, + { .name = NPCM8XX_CLK_S_REFCLK },
Note, this line says to use '.index = 0', and .name will be ignored. Maybe just use the index for everything? That makes it simpler and potentially faster because we don't have to do string comparisons anywhere.
Quoted 9 lines
drivers/clk/clk-npcm8xx.c new file +9
-0+ { .fw_name = NPCM8XX_CLK_S_PLL2, .name = NPCM8XX_CLK_S_PLL2 } +}; + +static const u32 pixcksel_mux_table[] = {0 ,2 }; +static const struct clk_parent_data pixcksel_mux_parents[] = { + { .fw_name =NPCM8XX_CLK_S_PLL_GFX , .name =NPCM8XX_CLK_S_PLL_GFX }, + { .name =NPCM8XX_CLK_S_REFCLK } +}; +
[...]
Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+ +static unsigned long npcm8xx_clk_pll_recalc_rate (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk_pll *pll =to_npcm8xx_clk_pll (hw);
drivers/clk/clk-npcm8xx.c line 335
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+unsigned long fbdv, indv, otdv1, otdv2; +unsigned int val; +u64 ret; + +if (parent_rate ==0 ) { +pr_debug ("%s: parent rate is zero\n" , __func__); +return 0 ; + } + + val =readl_relaxed (pll->pllcon + pll->reg);
Is pll->reg ever set?
Quoted 57 lines
drivers/clk/clk-npcm8xx.c new file +57
-0+ + indv =FIELD_GET (PLLCON_INDV , val); + fbdv =FIELD_GET (PLLCON_FBDV , val); + otdv1 =FIELD_GET (PLLCON_OTDV1 , val); + otdv2 =FIELD_GET (PLLCON_OTDV2 , val); + + ret = (u64 )parent_rate * fbdv; +do_div (ret, indv * otdv1 * otdv2); + +return ret; +} + +static const struct clk_ops npcm8xx_clk_pll_ops = { + .recalc_rate = npcm8xx_clk_pll_recalc_rate, +}; + +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 ); +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;
drivers/clk/clk-npcm8xx.c line 402
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+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 ; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + clk_base =devm_ioremap (dev, res->start,resource_size (res));
Can you use devm_platform_ioremap_resource() instead?
Quoted 1 line
+ if (!clk_base) {
Then this is checked for error pointer.
Quoted 1 line
+
dev_err(&pdev->dev, "Failed to remap I/O memory\n");
And no error message.
Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+return -ENOMEM ; + } + + npcm8xx_clk_data->num =NPCM8XX_NUM_CLOCKS ; +
[....]
Quoted 9 lines
drivers/clk/clk-npcm8xx.c new file +9
-0+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)) {
drivers/clk/clk-npcm8xx.c line 496
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+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,
Do we have a devm_ variant of this function? If not, can you add it?
Quoted 2 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ &div_data->parent_data, + div_data->flags,
Tomer Maimon
maintainerRe: [PATCH v15 1/1] clk: npcm8xx: add clock controller
HI Stephen,
Thanks for your comments
Quoted 39 lines · Stephen Boyd
Quoting Tomer Maimon (2023-03-09 11:44:02)
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +0
-0@@ -0,0 +1,561 @@[...]
Quoted 29 lines
drivers/clk/clk-npcm8xx.c new file +29
-0+ +struct npcm8xx_pll_data { +const char *name; +struct clk_parent_data parent; +unsigned int reg; +unsigned long flags; +}; + +struct npcm8xx_clk_div_data { +u32 reg; +u8 shift; +u8 width; +const char *name; +const struct clk_parent_data parent_data; +u8 clk_divider_flags; +unsigned long flags; +int onecell_idx; +}; + +struct npcm8xx_clk_mux_data { +u8 shift; +u32 mask; +const u32 *table; +const char *name; +const struct clk_parent_data *parent_data; +u8 num_parents; +unsigned long flags; +}; +[...]
Quoted 4 lines
drivers/clk/clk-npcm8xx.c
Quoted 2 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ +static struct npcm8xx_pll_data npcm8xx_pll_clks[] = {Can this be const?
Will add
Quoted 14 lines
drivers/clk/clk-npcm8xx.c line 153
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +9
-0+ { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 }, + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 }, + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 }, + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 }, +}; + +static const u32 cpuck_mux_table[] = {0 ,1 ,2 ,7 }; +static const struct clk_parent_data cpuck_mux_parents[] = { + { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },You should only have .fw_name or .index when introducing new drivers. The .name field is for existing drivers that want to migrate to
clk_parent_data.
I thought using .name was done when the clock defines in the DT, like the ref clock.
If the other clocks are not defined both .fw_name and .name the clocks are not registered properly.
Quoted 8 lines
drivers/clk/clk-npcm8xx.c
Quoted 3 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 }, + { .name = NPCM8XX_CLK_S_REFCLK },Note, this line says to use '.index = 0', and .name will be ignored. Maybe just use the index for everything? That makes it simpler and potentially faster because we don't have to do string comparisons anywhere.
Should the clk_parent_data mux use only .index? if yes how should the clock tree have a connection between the parent's clock and the mux for example:
for example, how should the driver connect between NPCM8XX_CLK_S_PLL1_DIV2 and the index number in the clk_parent_data?
Quoted 20 lines
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +9
-0+ { .fw_name = NPCM8XX_CLK_S_PLL2, .name = NPCM8XX_CLK_S_PLL2 } +}; + +static const u32 pixcksel_mux_table[] = {0 ,2 }; +static const struct clk_parent_data pixcksel_mux_parents[] = { + { .fw_name =NPCM8XX_CLK_S_PLL_GFX , .name =NPCM8XX_CLK_S_PLL_GFX }, + { .name =NPCM8XX_CLK_S_REFCLK } +}; +[...]
Quoted 9 lines
drivers/clk/clk-npcm8xx.c new file +9
-0+ +static unsigned long npcm8xx_clk_pll_recalc_rate (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk_pll *pll =to_npcm8xx_clk_pll (hw); +unsigned long fbdv, indv, otdv1, otdv2; +unsigned int val; +u64 ret; +
Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 335
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+if (parent_rate ==0 ) { +pr_debug ("%s: parent rate is zero\n" , __func__); +return 0 ; + } + + val =readl_relaxed (pll->pllcon + pll->reg);Is
pll->regever set?
Will remove.
Quoted 62 lines
Quoted 62 lines
drivers/clk/clk-npcm8xx.c new file +61
-0+ + indv =FIELD_GET (PLLCON_INDV , val); + fbdv =FIELD_GET (PLLCON_FBDV , val); + otdv1 =FIELD_GET (PLLCON_OTDV1 , val); + otdv2 =FIELD_GET (PLLCON_OTDV2 , val); + + ret = (u64 )parent_rate * fbdv; +do_div (ret, indv * otdv1 * otdv2); + +return ret; +} + +static const struct clk_ops npcm8xx_clk_pll_ops = { + .recalc_rate = npcm8xx_clk_pll_recalc_rate, +}; + +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 ); +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),
Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 402
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ 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));Can you use
devm_platform_ioremap_resource()instead?
We should use devm_ioremap since the clock register is used for the reset driver as well.
Quoted 28 lines
Quoted 2 lines
+ if (!clk_base) {
Then this is checked for error pointer.
Quoted 1 line
+
dev_err(&pdev->dev, "Failed to remap I/O memory\n");And no error message.
Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+return -ENOMEM ; + } + + npcm8xx_clk_data->num =NPCM8XX_NUM_CLOCKS ; +[....]
Quoted 13 lines
drivers/clk/clk-npcm8xx.c new file +13
-0+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;
Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 496
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ +/* 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,Do we have a
devm_variant of this function? If not, can you add it?
I will try to do it.
Quoted 3 lines
drivers/clk/clk-npcm8xx.c line 498
Quoted 3 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ &div_data->parent_data, + div_data->flags,
Thanks,
Tomer
Stephen Boyd
maintainerRe: [PATCH v15 1/1] clk: npcm8xx: add clock controller
Quoting Tomer Maimon (2023-03-31 11:07:19)
Quoted 21 lines · Stephen Boyd
Quoted 12 lines
Quoting Tomer Maimon (2023-03-09 11:44:02)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+ { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 }, + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 }, + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 }, + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 }, +};Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 153
Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+ +static const u32 cpuck_mux_table[] = {0 ,1 ,2 ,7 }; +static const struct clk_parent_data cpuck_mux_parents[] = { + { .fw_name =NPCM8XX_CLK_S_PLL0 , .name =NPCM8XX_CLK_S_PLL0 },You should only have .fw_name or .index when introducing new drivers. The .name field is for existing drivers that want to migrate to
clk_parent_data.
Quoted 4 lines
I thought using .name was done when the clock defines in the DT, like the ref clock.
If the other clocks are not defined both .fw_name and .name the clocks are not registered properly.
Are you saying that having .name fixes it?
Quoted 8 lines
Quoted 8 lines
drivers/clk/clk-npcm8xx.c
Quoted 3 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 }, + { .name = NPCM8XX_CLK_S_REFCLK },Note, this line says to use '.index = 0', and .name will be ignored. Maybe just use the index for everything? That makes it simpler and potentially faster because we don't have to do string comparisons anywhere.
Quoted 5 lines
Should the
clk_parent_datamux use only .index? if yes how should the clock tree have a connection between the parent's clock and the mux for example:
for example, how should the driver connect betweenNPCM8XX_CLK_S_PLL1_DIV2and the index number in theclk_parent_data?
It's not required, but it makes things simpler to only use .index or direct clk_hw pointers (.hw). I'm working on a clk documentation overhaul series right now, about 4 years later than I should have done it. It will cover this.
The .index field corresponds to the cell index in your devicetree 'clocks' property of the clk provider (the node with #clock-cells property). If the clk is internal, just use a .hw member and point to it directly. Don't consume your own clks in DT. If NPCM8XX_CLK_S_PLL1_DIV2 is a clk provided/registered by this device then it should be pointed to directly with the clk_hw pointer. If NPCM8XX_CLK_S_PLL1_DIV2 is an external clk that is consumed via the 'clocks' property in DT, then it should be specified as a parent via the .index member.
Quoted 21 lines
Quoted 13 lines
Quoted 13 lines
drivers/clk/clk-npcm8xx.c new file +13
-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 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),Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 402
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ 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));Can you use
devm_platform_ioremap_resource()instead?
Quoted 2 lines
We should use
devm_ioremapsince the clock register is used for the reset driver as well.
Are the clk and reset drivers sharing the register range? If so, please use auxiliary bus to register the reset driver, and map the register region once in the driver that registers the auxiliary device. Pass the iomem pointer to the auxiliary device.
Tomer Maimon
maintainerRe: [PATCH v15 1/1] clk: npcm8xx: add clock controller
Hi Stephen,
Sorry for the late reply and thanks a lot for your clarifications.
I have sent today V16 with some changes according to your comments.
Quoted 49 lines · Stephen Boyd
Quoting Tomer Maimon (2023-03-31 11:07:19)
Quoted 21 lines · Stephen Boyd
Quoted 12 lines
Quoting Tomer Maimon (2023-03-09 11:44:02)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+ { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 }, + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 }, + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 }, + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 }, +};Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 153
Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+ +static const u32 cpuck_mux_table[] = {0 ,1 ,2 ,7 }; +static const struct clk_parent_data cpuck_mux_parents[] = { + { .fw_name =NPCM8XX_CLK_S_PLL0 , .name =NPCM8XX_CLK_S_PLL0 },You should only have .fw_name or .index when introducing new drivers. The .name field is for existing drivers that want to migrate to
clk_parent_data.Quoted 4 lines
I thought using .name was done when the clock defines in the DT, like the ref clock.
If the other clocks are not defined both .fw_name and .name the clocks are not registered properly.Are you saying that having .name fixes it?
Quoted 8 lines
Quoted 8 lines
drivers/clk/clk-npcm8xx.c
Quoted 3 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 }, + { .name = NPCM8XX_CLK_S_REFCLK },Note, this line says to use '.index = 0', and .name will be ignored. Maybe just use the index for everything? That makes it simpler and potentially faster because we don't have to do string comparisons anywhere.
Quoted 5 lines
Should the
clk_parent_datamux use only .index? if yes how should the clock tree have a connection between the parent's clock and the mux for example:
for example, how should the driver connect betweenNPCM8XX_CLK_S_PLL1_DIV2and the index number in theclk_parent_data?It's not required, but it makes things simpler to only use .index or direct
clk_hwpointers (.hw). I'm working on a clk documentation overhaul series right now, about 4 years later than I should have done it. It will cover this.
Quoted 33 lines
The .index field corresponds to the cell index in your devicetree 'clocks' property of the clk provider (the node with #clock-cells property). If the clk is internal, just use a .hw member and point to it directly. Don't consume your own clks in DT. If
NPCM8XX_CLK_S_PLL1_DIV2is a clk provided/registered by this device then it should be pointed to directly with theclk_hwpointer. IfNPCM8XX_CLK_S_PLL1_DIV2is an external clk that is consumed via the 'clocks' property in DT, then it should be specified as a parent via the .index member.Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
drivers/clk/clk-npcm8xx.c new file +13
-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 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),Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 402
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ 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));Can you use
devm_platform_ioremap_resource()instead?We should use
devm_ioremapsince the clock register is used for the reset driver as well.
We using the same ioremap sequence in npcm7xx clock driver since the clock and reset share the same register region.
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); clk_base = devm_ioremap(dev, res->start, resource_size(res));
https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/clk/clk-npcm7xx.c#L413
Why we cannot use it in the NPCM8XX clock driver?
Quoted 4 lines
Are the clk and reset drivers sharing the register range? If so, please use auxiliary bus to register the reset driver, and map the register region once in the driver that registers the auxiliary device. Pass the iomem pointer to the auxiliary device.
Sorry, but I didn't understand what you mean by using the auxiliary bus to register the reset driver do you have an example of how it should be done?
Thanks,
Tomer