Introduce Nuvoton Arbel NPCM8XX BMC SoC
Subsystems (3)
Tomer Maimon
maintainerv23 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.
for your note:
1. dt-bindings clock modification started from v22 since the upstream npcm8xx clock driver haven't merged yet and requires dt binding update according to the new npcm8xx clock driver.
2. 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:
- Rob Herring: https://www.spinics.net/lists/devicetree/msg663403.html
- Krzysztof Kozlowski: https://www.spinics.net/lists/devicetree/msg665206.html
Changes since version 22:
- Modify commit message to explain broken ABI in dt-binding
- Using regmap parenet regmap memory therefore remove use of npcm8xx rst-clock patch.
- Leave npcm7xx rst node as is
Changes since version 21:
- Since using regmap instead of ioremap replace reg to syscon property in dt-bindings and dts.
- Add reference clock property to the dt-bindings and dts.
- Using .index instead of .name in clk_parent_data structures.
- Using string where any macros are used once.
Changes since version 20:
- Using regmap instead of ioremap.
the clock and reset modules are sharing the same memory region and cause failure when using devm_platform_ioremap_resource function, this version uses regmap to handle shared reset and clock memory region, in case it is approved I will modify the reset driver to use the regmap as well.
- Using clk_hw instead of clk_parent_data structre.
- Divider clock definition to one line
Changes since version 19:
- Remove unnecessary free command.
- Defining pr_fmt().
- Using dev_err_probe.
- Return zero in the end of the probe function.
Changes since version 18:
- NPCM8XX clock driver did not changed from version 18 only build and tested under kernel 6.6-rc1.
Changes since version 17:
- NPCM8XX clock driver did not changed from version 17 only build and tested under kernel 6.5-rc3.
Changes since version 16:
- NPCM8XX clock driver
- Using devm_kzalloc instead kzalloc.
- Remove unnecessary parenthesis.
- Modify incorrect spelling.
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 5:
- NPCM8XX clock driver
- Remove refclk if devm_of_clk_add_hw_provider function failed.
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.
Changes since version 2:
- NPCM8XX clock driver
- Add debug new line.
- Add 25M fixed rate clock.
- Remove unused clocks and clock name from dt-binding.
Changes since version 1:
- NPCM8XX clock driver
- Modify dt-binding.
- Remove unsed definition and include.
- Include alphabetically.
- Use clock devm.
Tomer Maimon (3):
dt-bindings: clock: npcm845: Add reference 25m clock property
arm64: dts: nuvoton: npcm8xx: add reference 25m clock
clk: npcm8xx: add clock controller
.../bindings/clock/nuvoton,npcm845-clk.yaml | 12 + .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 2 + .../boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 6 + drivers/clk/Kconfig | 8 + drivers/clk/Makefile | 1 + drivers/clk/clk-npcm8xx.c | 509 ++++++++++++++++++ 6 files changed, 538 insertions(+) create mode 100644 drivers/clk/clk-npcm8xx.c
Signature · 1 line
2.34.1
Tomer Maimon
maintainerPATCH v23 1/3 dt-bindings: clock: npcm845: Add reference 25m clock property
The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding clock property.
The new required clock property does not break the NPCM8XX clock ABI since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla.
--- .../bindings/clock/nuvoton,npcm845-clk.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml +12 -0
@@ -21,6 +21,14 @@ properties: reg: maxItems: 1 + clocks: + items: + - description: 25Mhz reference clock + + clock-names: + items: + - const: refclk + '#clock-cells': const: 1 description:
@@ -30,6 +38,8 @@ properties: required: - compatible - reg + - clocks + - clock-names - '#clock-cells' additionalProperties: false
@@ -44,6 +54,8 @@ examples: compatible = "nuvoton,npcm845-clk"; reg = <0x0 0xf0801000 0x0 0x1000>; #clock-cells = <1>; + clocks = <&refclk>; + clock-names = "refclk"; }; }; ...
Signature · 1 line
2.34.1
Re: [PATCH v23 1/3] dt-bindings: clock: npcm845: Add reference 25m clock property
Quoted 8 lines · Tomer Maimon
The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding clock property.
The new required clock property does not break the NPCM8XX clock ABI since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla.
Signed-off-by Tomer Maimon <[email protected]>---
Best regards,
Krzysztof
Stephen Boyd
maintainerRe: [PATCH v23 1/3] dt-bindings: clock: npcm845: Add reference 25m clock property
In reply to Tomer MaimonQuoting Tomer Maimon (2024-01-31 10:26:51)
Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml examples: · line 58
Quoted 10 lines
Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml +2
-0@@ -44,6 +54,8 @@ examples: compatible = "nuvoton,npcm845-clk"; reg = <0x0 0xf0801000 0x0 0x1000>; #clock-cells = <1>; + clocks = <&refclk>; + clock-names = "refclk";
The driver seems to want this to be a child of the mfd syscon. Is that right?
Tomer Maimon
maintainerRe: [PATCH v23 1/3] dt-bindings: clock: npcm845: Add reference 25m clock property
Hi Stephen,
The plan is for both the clock and reset will be under only the memory region handle like
sysctrl: system-controller@f0801000 {
compatible = "syscon", "simple-mfd";
reg = <0x0 0xf0801000 0x0 0x1000>;
rstc: reset-controller {
compatible = "nuvoton,npcm845-reset";
reg = <0x0 0xf0801000 0x0 0xC4>;
#reset-cells = <2>;
nuvoton,sysgcr = <&gcr>;
};
clk: clock-controller {
compatible = "nuvoton,npcm845-clk";
#clock-cells = <1>;
clocks = <&refclk>;
clock-names = "refclk";
};
};is it problematic?
But this commit is not related to it.
Thanks,
Tomer
Quoted 15 lines · Stephen Boyd
Quoting Tomer Maimon (2024-01-31 10:26:51)
Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml examples: · line 58
Quoted 10 lines
Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml +2
-0@@ -44,6 +54,8 @@ examples: compatible = "nuvoton,npcm845-clk"; reg = <0x0 0xf0801000 0x0 0x1000>; #clock-cells = <1>; + clocks = <&refclk>; + clock-names = "refclk";The driver seems to want this to be a child of the mfd syscon. Is that right?
Tomer Maimon
maintainerPATCH v23 2/3 arm64: dts: nuvoton: npcm8xx: add reference 25m clock
In reply to Tomer MaimonAdd 25Mhz reference clock that to the clock node since the reference clock in not a part of Nuvoton BMC NPCM8XX SoC.
--- arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 2 ++ arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 6 ++++++ 2 files changed, 8 insertions(+)
arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi +2 -0
@@ -58,6 +58,8 @@ clk: clock-controller@f0801000 { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; reg = <0x0 0xf0801000 0x0 0x1000>; + clocks = <&refclk>; + clock-names = "refclk"; }; apb {
arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts +6 -0
@@ -19,6 +19,12 @@ chosen { memory { reg = <0x0 0x0 0x0 0x40000000>; }; + + refclk: refclk-25mhz { + compatible = "fixed-clock"; + clock-frequency = <25000000>; + #clock-cells = <0>; + }; }; &serial0 {
Signature · 1 line
2.34.1
Tomer Maimon
maintainerPATCH v23 3/3 clk: npcm8xx: add clock controller
In reply to Tomer MaimonNuvoton 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 | 509 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 518 insertions(+) create mode 100644 drivers/clk/clk-npcm8xx.c
drivers/clk/Kconfig +8 -0
@@ -334,6 +334,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
@@ -52,6 +52,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_PALMAS) += clk-palmas.o obj-$(CONFIG_CLK_LS1028A_PLLDIG) += clk-plldig.o
drivers/clk/clk-npcm8xx.c new file +509 -0
509 lines not on this page · raw message
Signature · 1 line
2.34.1
Stephen Boyd
maintainerRe: [PATCH v23 3/3] clk: npcm8xx: add clock controller
Quoting Tomer Maimon (2024-01-31 10:26:53)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +4
-0@@ -0,0 +1,509 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Nuvoton NPCM8xx Clock Generator +* All the clocks are initialized by the bootloader, so this driver allows only
[...]
Quoted 11 lines
drivers/clk/clk-npcm8xx.c new file +11
-0+ +/* external clock definition */ +#define NPCM8XX_CLK_S_REFCLK "refclk" + +/* pll definition */ +#define NPCM8XX_CLK_S_PLL0 "pll0" +#define NPCM8XX_CLK_S_PLL1 "pll1" +#define NPCM8XX_CLK_S_PLL2 "pll2" +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" + +/* early divider definition */
drivers/clk/clk-npcm8xx.c line 110
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" +#define NPCM8XX_CLK_S_PLL_GFX_DIV2 "pll_gfx_div2" +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" + +/* mux definition */ +#define NPCM8XX_CLK_S_CPU_MUX "cpu_mux" + +/* div definition */ +#define NPCM8XX_CLK_S_TH "th" +#define NPCM8XX_CLK_S_AXI "axi"
Please inline all these string #defines to the place they're used.
Quoted 2 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ +static struct clk_hw hw_pll1_div2, hw_pll2_div2, hw_gfx_div2, hw_pre_clk;
[..]
Quoted 11 lines
drivers/clk/clk-npcm8xx.c new file +11
-0+static struct clk_hw * +npcm8xx_clk_register (struct device *dev,const char *name, +struct regmap *clk_regmap,unsigned int offset, +unsigned long flags,const struct clk_ops *npcm8xx_clk_ops, +const struct clk_parent_data *parent_data, +const struct clk_hw *parent_hw,u8 num_parents, +u8 shift,u32 mask,unsigned long width, +const u32 *table,unsigned long clk_flags) +{ +struct npcm8xx_clk *clk; +struct clk_init_data init = {};
drivers/clk/clk-npcm8xx.c line 266
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+int ret; + + clk =devm_kzalloc (dev,sizeof (*clk),GFP_KERNEL ); +if (!clk) +return ERR_PTR (-ENOMEM ); + + init.name = name; + init.ops = npcm8xx_clk_ops; + init.parent_data = parent_data; + init.parent_hws = parent_hw ? &parent_hw :NULL ;
Is it necessary to check? Can't it be set unconditionally?
Quoted 17 lines
drivers/clk/clk-npcm8xx.c new file +17
-0+ init.num_parents = num_parents; + init.flags = flags; + + clk->clk_regmap = clk_regmap; + clk->hw.init = &init; + clk->offset = offset; + clk->shift = shift; + clk->mask = mask; + clk->width = width; + clk->table = table; + clk->flags = clk_flags; + + ret =devm_clk_hw_register (dev, &clk->hw); +if (ret) +return ERR_PTR (ret); + +return &clk->hw;
[...]
Quoted 12 lines
drivers/clk/clk-npcm8xx.c new file +12
-0+ +static unsigned long npcm8xx_clk_div_get_parent (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk *div =to_npcm8xx_clk (hw); +unsigned int val; + +regmap_read (div->clk_regmap, div->offset, &val); + val = val >> div->shift; + val &=clk_div_mask (div->width); + + return divider_recalc_rate(hw, parent_rate, val, NULL, div->flags,
drivers/clk/clk-npcm8xx.c line 352
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+ div->width); +} + +static const struct clk_ops npcm8xx_clk_div_ops = { + .recalc_rate = npcm8xx_clk_div_get_parent, +}; + +static int npcm8xx_clk_probe (struct platform_device *pdev) +{ +struct device_node *parent_np =of_get_parent (pdev->dev.of_node);
The parent of this device is not a syscon.
Quoted 4 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+struct clk_hw_onecell_data *npcm8xx_clk_data; +struct device *dev = &pdev->dev; +struct regmap *clk_regmap; +struct clk_hw *hw;
drivers/clk/clk-npcm8xx.c line 366
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+unsigned 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_regmap =syscon_node_to_regmap (parent_np); +of_node_put (parent_np);
Is there another binding update that is going to move this node to be a child of the syscon?
gcr: system-controller@f0800000 {
compatible = "nuvoton,npcm845-gcr", "syscon";
reg = <0x0 0xf0800000 0x0 0x1000>;
};Tomer Maimon
maintainerRe: [PATCH v23 3/3] clk: npcm8xx: add clock controller
Hi Stephen,
Quoted 28 lines · Stephen Boyd
Quoting Tomer Maimon (2024-01-31 10:26:53)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +4
-0@@ -0,0 +1,509 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Nuvoton NPCM8xx Clock Generator +* All the clocks are initialized by the bootloader, so this driver allows only [...]
Quoted 15 lines
drivers/clk/clk-npcm8xx.c new file +15
-0+ +/* external clock definition */ +#define NPCM8XX_CLK_S_REFCLK "refclk" + +/* pll definition */ +#define NPCM8XX_CLK_S_PLL0 "pll0" +#define NPCM8XX_CLK_S_PLL1 "pll1" +#define NPCM8XX_CLK_S_PLL2 "pll2" +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" + +/* early divider definition */ +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" +#define NPCM8XX_CLK_S_PLL_GFX_DIV2 "pll_gfx_div2" +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" +
Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 110
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+/* mux definition */ +#define NPCM8XX_CLK_S_CPU_MUX "cpu_mux" + +/* div definition */ +#define NPCM8XX_CLK_S_TH "th" +#define NPCM8XX_CLK_S_AXI "axi"Please inline all these string #defines to the place they're used.
The version V21 you mention using define only when the definition is used more than once
https://www.spinics.net/lists/kernel/msg5045826.html
Should I remove all the string definitions and add the string to the array?
Quoted 19 lines
Quoted 3 lines
drivers/clk/clk-npcm8xx.c new file +2
-0+ +static struct clk_hw hw_pll1_div2, hw_pll2_div2, hw_gfx_div2, hw_pre_clk;[..]
Quoted 15 lines
drivers/clk/clk-npcm8xx.c new file +15
-0+static struct clk_hw * +npcm8xx_clk_register (struct device *dev,const char *name, +struct regmap *clk_regmap,unsigned int offset, +unsigned long flags,const struct clk_ops *npcm8xx_clk_ops, +const struct clk_parent_data *parent_data, +const struct clk_hw *parent_hw,u8 num_parents, +u8 shift,u32 mask,unsigned long width, +const u32 *table,unsigned long clk_flags) +{ +struct npcm8xx_clk *clk; +struct clk_init_data init = {}; +int ret; + + clk =devm_kzalloc (dev,sizeof (*clk),GFP_KERNEL ); +if (!clk)
Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 266
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+return ERR_PTR (-ENOMEM ); + + init.name = name; + init.ops = npcm8xx_clk_ops; + init.parent_data = parent_data; + init.parent_hws = parent_hw ? &parent_hw :NULL ;Is it necessary to check? Can't it be set unconditionally?
Will remove
Quoted 35 lines
Quoted 18 lines
drivers/clk/clk-npcm8xx.c new file +17
-0+ init.num_parents = num_parents; + init.flags = flags; + + clk->clk_regmap = clk_regmap; + clk->hw.init = &init; + clk->offset = offset; + clk->shift = shift; + clk->mask = mask; + clk->width = width; + clk->table = table; + clk->flags = clk_flags; + + ret =devm_clk_hw_register (dev, &clk->hw); +if (ret) +return ERR_PTR (ret); + +return &clk->hw;[...]
Quoted 16 lines
drivers/clk/clk-npcm8xx.c new file +16
-0+ +static unsigned long npcm8xx_clk_div_get_parent (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk *div =to_npcm8xx_clk (hw); +unsigned int val; + +regmap_read (div->clk_regmap, div->offset, &val); + val = val >> div->shift; + val &=clk_div_mask (div->width); + +return divider_recalc_rate (hw, parent_rate, val,NULL , div->flags, + div->width); +} + +static const struct clk_ops npcm8xx_clk_div_ops = {
Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 352
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ .recalc_rate = npcm8xx_clk_div_get_parent, +}; + +static int npcm8xx_clk_probe (struct platform_device *pdev) +{ +struct device_node *parent_np =of_get_parent (pdev->dev.of_node);The parent of this device is not a syscon.
Once I have registered the map that handles both reset and the clock in general is syscon, this is why we will modify the DTS so the clock and the reset will be under syscon father node
sysctrl: system-controller@f0801000 {
compatible = "syscon", "simple-mfd";
reg = <0x0 0xf0801000 0x0 0x1000>;
rstc: reset-controller {
compatible = "nuvoton,npcm845-reset";
reg = <0x0 0xf0801000 0x0 0xC4>;
#reset-cells = <2>;
nuvoton,sysgcr = <&gcr>;
};
clk: clock-controller {
compatible = "nuvoton,npcm845-clk";
#clock-cells = <1>;
clocks = <&refclk>;
clock-names = "refclk";
};
};You can see other drivers that using the same method like
https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
Quoted 19 lines
Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+struct clk_hw_onecell_data *npcm8xx_clk_data; +struct device *dev = &pdev->dev; +struct regmap *clk_regmap; +struct clk_hw *hw;drivers/clk/clk-npcm8xx.c line 366
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+unsigned 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_regmap =syscon_node_to_regmap (parent_np); +of_node_put (parent_np);Is there another binding update that is going to move this node to be a child of the syscon?
Quoted 4 lines
gcr: system-controller@f0800000 {
compatible = "nuvoton,npcm845-gcr", "syscon";
reg = <0x0 0xf0800000 0x0 0x1000>;
};No, sorry but I'm not going to use the GCR node the handle the clock and reset modules, the GCR has different memory space. the clock driver will have the following device tree
sysctrl: system-controller@f0801000 {
compatible = "syscon", "simple-mfd";
reg = <0x0 0xf0801000 0x0 0x1000>;
rstc: reset-controller {
compatible = "nuvoton,npcm845-reset";
reg = <0x0 0xf0801000 0x0 0xC4>;
#reset-cells = <2>;
nuvoton,sysgcr = <&gcr>;
};
clk: clock-controller {
compatible = "nuvoton,npcm845-clk";
#clock-cells = <1>;
clocks = <&refclk>;
clock-names = "refclk";
};
};Stephen appreciate it if you could speed this upstream and apply it in this kernel version.
hope the clarification is understood and if it is fine I can send V24 this week.
Thanks,
Tomer
Stephen Boyd
maintainerRe: [PATCH v23 3/3] clk: npcm8xx: add clock controller
Quoting Tomer Maimon (2024-02-25 10:00:35)
Quoted 39 lines
Hi Stephen,
Quoted 28 lines · Stephen Boyd
Quoting Tomer Maimon (2024-01-31 10:26:53)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +4
-0@@ -0,0 +1,509 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Nuvoton NPCM8xx Clock Generator +* All the clocks are initialized by the bootloader, so this driver allows only [...]
Quoted 15 lines
drivers/clk/clk-npcm8xx.c new file +15
-0+ +/* external clock definition */ +#define NPCM8XX_CLK_S_REFCLK "refclk" + +/* pll definition */ +#define NPCM8XX_CLK_S_PLL0 "pll0" +#define NPCM8XX_CLK_S_PLL1 "pll1" +#define NPCM8XX_CLK_S_PLL2 "pll2" +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" + +/* early divider definition */ +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" +#define NPCM8XX_CLK_S_PLL_GFX_DIV2 "pll_gfx_div2" +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" +Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 110
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+/* mux definition */ +#define NPCM8XX_CLK_S_CPU_MUX "cpu_mux" + +/* div definition */ +#define NPCM8XX_CLK_S_TH "th" +#define NPCM8XX_CLK_S_AXI "axi"Please inline all these string #defines to the place they're used.
Quoted 4 lines
The version V21 you mention using define only when the definition is used more than once
https://www.spinics.net/lists/kernel/msg5045826.html
Should I remove all the string definitions and add the string to the array?
If it's a clk name for a clk registered in this file it should be inlined. Is that the case for everything besides refclk? And even refclk could be inlined so that we don't have to jump to the definition of a string.
Quoted 45 lines
Quoted 16 lines
Quoted 16 lines
drivers/clk/clk-npcm8xx.c new file +16
-0+ +static unsigned long npcm8xx_clk_div_get_parent (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk *div =to_npcm8xx_clk (hw); +unsigned int val; + +regmap_read (div->clk_regmap, div->offset, &val); + val = val >> div->shift; + val &=clk_div_mask (div->width); + +return divider_recalc_rate (hw, parent_rate, val,NULL , div->flags, + div->width); +} + +static const struct clk_ops npcm8xx_clk_div_ops = {Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 352
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ .recalc_rate = npcm8xx_clk_div_get_parent, +}; + +static int npcm8xx_clk_probe (struct platform_device *pdev) +{ +struct device_node *parent_np =of_get_parent (pdev->dev.of_node);The parent of this device is not a syscon.
Once I have registered the map that handles both reset and the clock in general is syscon, this is why we will modify the DTS so the clock and the reset will be under syscon father node
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>; #reset-cells = <2>; nuvoton,sysgcr = <&gcr>; }; clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };
Quoted 2 lines
You can see other drivers that using the same method like
https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
You will need a similar file like Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yaml then to describe the child nodes.
Socionext may not be the best example to follow. I generally try to avoid syscon and simply put #reset-cells and #clock-cells in the node for the device. You can use the auxiliary bus to register drivers for clk and reset and put them into the resepective driver directories. Avoid syscon means random drivers can't reach into the device with a regmap handle and read/write registers that they're not supposed to.
Quoted 19 lines
Quoted 19 lines
Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+struct clk_hw_onecell_data *npcm8xx_clk_data; +struct device *dev = &pdev->dev; +struct regmap *clk_regmap; +struct clk_hw *hw;drivers/clk/clk-npcm8xx.c line 366
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+unsigned 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_regmap =syscon_node_to_regmap (parent_np); +of_node_put (parent_np);Is there another binding update that is going to move this node to be a child of the syscon?
Quoted 7 lines
Quoted 4 lines
gcr: system-controller@f0800000 { compatible = "nuvoton,npcm845-gcr", "syscon"; reg = <0x0 0xf0800000 0x0 0x1000>; };No, sorry but I'm not going to use the GCR node the handle the clock and reset modules, the GCR has different memory space. the clock driver will have the following device tree
What does the reset driver use the CGR node for? The driver looks like it's using it to control USB phy resets.
Quoted 7 lines
sysctrl: system-controller@f0801000 {
compatible = "syscon", "simple-mfd";
reg = <0x0 0xf0801000 0x0 0x1000>;
rstc: reset-controller {
compatible = "nuvoton,npcm845-reset";
reg = <0x0 0xf0801000 0x0 0xC4>;This isn't a valid reg property for a child node like this.
Quoted 11 lines
#reset-cells = <2>;
nuvoton,sysgcr = <&gcr>;
};
clk: clock-controller {
compatible = "nuvoton,npcm845-clk";
#clock-cells = <1>;
clocks = <&refclk>;
clock-names = "refclk";
};
};Tomer Maimon
maintainerRe: [PATCH v23 3/3] clk: npcm8xx: add clock controller
Hi Stephen,
Thanks for your reply.
Quoted 46 lines · Stephen Boyd
Quoting Tomer Maimon (2024-02-25 10:00:35)
Quoted 39 lines
Hi Stephen,
Quoted 28 lines · Stephen Boyd
Quoting Tomer Maimon (2024-01-31 10:26:53)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +4
-0@@ -0,0 +1,509 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Nuvoton NPCM8xx Clock Generator +* All the clocks are initialized by the bootloader, so this driver allows only [...]
Quoted 15 lines
drivers/clk/clk-npcm8xx.c new file +15
-0+ +/* external clock definition */ +#define NPCM8XX_CLK_S_REFCLK "refclk" + +/* pll definition */ +#define NPCM8XX_CLK_S_PLL0 "pll0" +#define NPCM8XX_CLK_S_PLL1 "pll1" +#define NPCM8XX_CLK_S_PLL2 "pll2" +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" + +/* early divider definition */ +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" +#define NPCM8XX_CLK_S_PLL_GFX_DIV2 "pll_gfx_div2" +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" +Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 110
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+/* mux definition */ +#define NPCM8XX_CLK_S_CPU_MUX "cpu_mux" + +/* div definition */ +#define NPCM8XX_CLK_S_TH "th" +#define NPCM8XX_CLK_S_AXI "axi"Please inline all these string #defines to the place they're used.
Quoted 4 lines
The version V21 you mention using define only when the definition is used more than once
https://www.spinics.net/lists/kernel/msg5045826.html
Should I remove all the string definitions and add the string to the array?
Quoted 4 lines
If it's a clk name for a clk registered in this file it should be inlined. Is that the case for everything besides refclk? And even refclk could be inlined so that we don't have to jump to the definition of a string.
I will add the string in the clock arrays and remove all the string definitions.
Quoted 49 lines
Quoted 46 lines
Quoted 17 lines
Quoted 17 lines
drivers/clk/clk-npcm8xx.c new file +16
-0+ +static unsigned long npcm8xx_clk_div_get_parent (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk *div =to_npcm8xx_clk (hw); +unsigned int val; + +regmap_read (div->clk_regmap, div->offset, &val); + val = val >> div->shift; + val &=clk_div_mask (div->width); + +return divider_recalc_rate (hw, parent_rate, val,NULL , div->flags, + div->width); +} + +static const struct clk_ops npcm8xx_clk_div_ops = {Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 352
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ .recalc_rate = npcm8xx_clk_div_get_parent, +}; + +static int npcm8xx_clk_probe (struct platform_device *pdev) +{ +struct device_node *parent_np =of_get_parent (pdev->dev.of_node);The parent of this device is not a syscon.
Once I have registered the map that handles both reset and the clock in general is syscon, this is why we will modify the DTS so the clock and the reset will be under syscon father node
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>; #reset-cells = <2>; nuvoton,sysgcr = <&gcr>; }; clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };Quoted 2 lines
You can see other drivers that using the same method like
https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
Quoted 3 lines
You will need a similar file like
Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yamlthen to describe the child nodes.
I can do it.
Quoted 3 lines
Socionext may not be the best example to follow. I generally try to avoid syscon and simply put #reset-cells and #clock-cells in the node
If I remove syscon I can't use syscon_node_to_regmap function, What should I use If I remove syscon? auxiliary bus? something else?
Quoted 2 lines
for the device. You can use the auxiliary bus to register drivers for clk and reset and put them into the resepective driver directories.
I little bit confused, what is an auxiliary bus to register drivers, can you provide me an example?
Quoted 2 lines
Avoid syscon means random drivers can't reach into the device with a regmap handle and read/write registers that they're not supposed to.
Indeed, but the drivers could use the reset and clock memory map only if the module is also a child node.
Please let me know what is your preferred way to handle it:
1. stick with syscon and upstream-defined documentation for the rst clk syscon.
2. avoid syscon and use an auxiliary bus, appreciate if you could give me an example of how it should be done.
3. Avoid sycon and handle it differently.
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+struct clk_hw_onecell_data *npcm8xx_clk_data; +struct device *dev = &pdev->dev; +struct regmap *clk_regmap; +struct clk_hw *hw;drivers/clk/clk-npcm8xx.c line 366
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+unsigned 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_regmap =syscon_node_to_regmap (parent_np); +of_node_put (parent_np);Is there another binding update that is going to move this node to be a child of the syscon?
gcr: system-controller@f0800000 { compatible = "nuvoton,npcm845-gcr", "syscon"; reg = <0x0 0xf0800000 0x0 0x1000>; };
Quoted 6 lines
Quoted 3 lines
No, sorry but I'm not going to use the GCR node the handle the clock and reset modules, the GCR has different memory space. the clock driver will have the following device tree
What does the reset driver use the CGR node for? The driver looks like it's using it to control USB phy resets.
Yes, the USB PHY reset is handled through the GCR registers.
Quoted 10 lines
Quoted 8 lines
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>;This isn't a valid reg property for a child node like this.
O.K.
Quoted 5 lines
Quoted 5 lines
#reset-cells = <2>; nuvoton,sysgcr = <&gcr>; };
Quoted 7 lines
Quoted 7 lines
clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };
Appreciate your guidance!
Thanks,
Tomer
Tomer Maimon
maintainerRe: [PATCH v23 3/3] clk: npcm8xx: add clock controller
Hi Stephen,
Appreciate it if you could reply to my email afew days ago, It is really important to us to move this driver to upstream.
Thanks,
Tomer
Quoted 189 lines · Tomer Maimon
Hi Stephen,
Thanks for your reply.
Quoted 46 lines · Stephen Boyd
Quoting Tomer Maimon (2024-02-25 10:00:35)
Quoted 39 lines
Hi Stephen,
Quoted 28 lines · Stephen Boyd
Quoting Tomer Maimon (2024-01-31 10:26:53)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +4
-0@@ -0,0 +1,509 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Nuvoton NPCM8xx Clock Generator +* All the clocks are initialized by the bootloader, so this driver allows only [...]
Quoted 15 lines
drivers/clk/clk-npcm8xx.c new file +15
-0+ +/* external clock definition */ +#define NPCM8XX_CLK_S_REFCLK "refclk" + +/* pll definition */ +#define NPCM8XX_CLK_S_PLL0 "pll0" +#define NPCM8XX_CLK_S_PLL1 "pll1" +#define NPCM8XX_CLK_S_PLL2 "pll2" +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" + +/* early divider definition */ +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" +#define NPCM8XX_CLK_S_PLL_GFX_DIV2 "pll_gfx_div2" +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" +Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 110
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+/* mux definition */ +#define NPCM8XX_CLK_S_CPU_MUX "cpu_mux" + +/* div definition */ +#define NPCM8XX_CLK_S_TH "th" +#define NPCM8XX_CLK_S_AXI "axi"Please inline all these string #defines to the place they're used.
Quoted 4 lines
The version V21 you mention using define only when the definition is used more than once
https://www.spinics.net/lists/kernel/msg5045826.html
Should I remove all the string definitions and add the string to the array?Quoted 4 lines
If it's a clk name for a clk registered in this file it should be inlined. Is that the case for everything besides refclk? And even refclk could be inlined so that we don't have to jump to the definition of a string.
I will add the string in the clock arrays and remove all the string definitions.
Quoted 49 lines
Quoted 46 lines
Quoted 17 lines
Quoted 17 lines
drivers/clk/clk-npcm8xx.c new file +16
-0+ +static unsigned long npcm8xx_clk_div_get_parent (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk *div =to_npcm8xx_clk (hw); +unsigned int val; + +regmap_read (div->clk_regmap, div->offset, &val); + val = val >> div->shift; + val &=clk_div_mask (div->width); + +return divider_recalc_rate (hw, parent_rate, val,NULL , div->flags, + div->width); +} + +static const struct clk_ops npcm8xx_clk_div_ops = {Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 352
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ .recalc_rate = npcm8xx_clk_div_get_parent, +}; + +static int npcm8xx_clk_probe (struct platform_device *pdev) +{ +struct device_node *parent_np =of_get_parent (pdev->dev.of_node);The parent of this device is not a syscon.
Once I have registered the map that handles both reset and the clock in general is syscon, this is why we will modify the DTS so the clock and the reset will be under syscon father node
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>; #reset-cells = <2>; nuvoton,sysgcr = <&gcr>; }; clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };Quoted 2 lines
You can see other drivers that using the same method like
https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yamlQuoted 3 lines
You will need a similar file like
Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yamlthen to describe the child nodes.I can do it.
Quoted 3 lines
Socionext may not be the best example to follow. I generally try to avoid syscon and simply put #reset-cells and #clock-cells in the node
If I remove syscon I can't use
syscon_node_to_regmapfunction, What should I use If I remove syscon? auxiliary bus? something else?Quoted 2 lines
for the device. You can use the auxiliary bus to register drivers for clk and reset and put them into the resepective driver directories.
I little bit confused, what is an auxiliary bus to register drivers, can you provide me an example?
Quoted 2 lines
Avoid syscon means random drivers can't reach into the device with a regmap handle and read/write registers that they're not supposed to.
Indeed, but the drivers could use the reset and clock memory map only if the module is also a child node.
Please let me know what is your preferred way to handle it:
1. stick with syscon and upstream-defined documentation for the rst clk syscon.
2. avoid syscon and use an auxiliary bus, appreciate if you could give me an example of how it should be done.
3. Avoid sycon and handle it differently.Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+struct clk_hw_onecell_data *npcm8xx_clk_data; +struct device *dev = &pdev->dev; +struct regmap *clk_regmap; +struct clk_hw *hw;drivers/clk/clk-npcm8xx.c line 366
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+unsigned 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_regmap =syscon_node_to_regmap (parent_np); +of_node_put (parent_np);Is there another binding update that is going to move this node to be a child of the syscon?
gcr: system-controller@f0800000 { compatible = "nuvoton,npcm845-gcr", "syscon"; reg = <0x0 0xf0800000 0x0 0x1000>; };Quoted 6 lines
Quoted 3 lines
No, sorry but I'm not going to use the GCR node the handle the clock and reset modules, the GCR has different memory space. the clock driver will have the following device tree
What does the reset driver use the CGR node for? The driver looks like it's using it to control USB phy resets.
Yes, the USB PHY reset is handled through the GCR registers.
Quoted 10 lines
Quoted 8 lines
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>;This isn't a valid reg property for a child node like this.
O.K.
Quoted 5 lines
Quoted 5 lines
#reset-cells = <2>; nuvoton,sysgcr = <&gcr>; };Quoted 7 lines
Quoted 7 lines
clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };Appreciate your guidance!
Thanks,
Tomer
Tomer Maimon
maintainerRe: [PATCH v23 3/3] clk: npcm8xx: add clock controller
Hi Stephen,
Kind remainder, appreciate if you can reply about the comment that been sent few weeks ago.
Thanks,
Tomer
Quoted 200 lines · Tomer Maimon
Hi Stephen,
Appreciate it if you could reply to my email afew days ago, It is really important to us to move this driver to upstream.
Thanks,
Tomer
Quoted 189 lines · Tomer Maimon
Hi Stephen,
Thanks for your reply.
Quoted 46 lines · Stephen Boyd
Quoting Tomer Maimon (2024-02-25 10:00:35)
Quoted 39 lines
Hi Stephen,
Quoted 28 lines · Stephen Boyd
Quoting Tomer Maimon (2024-01-31 10:26:53)
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +4
-0@@ -0,0 +1,509 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Nuvoton NPCM8xx Clock Generator +* All the clocks are initialized by the bootloader, so this driver allows only [...]
Quoted 15 lines
drivers/clk/clk-npcm8xx.c new file +15
-0+ +/* external clock definition */ +#define NPCM8XX_CLK_S_REFCLK "refclk" + +/* pll definition */ +#define NPCM8XX_CLK_S_PLL0 "pll0" +#define NPCM8XX_CLK_S_PLL1 "pll1" +#define NPCM8XX_CLK_S_PLL2 "pll2" +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" + +/* early divider definition */ +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" +#define NPCM8XX_CLK_S_PLL_GFX_DIV2 "pll_gfx_div2" +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" +Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 110
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+/* mux definition */ +#define NPCM8XX_CLK_S_CPU_MUX "cpu_mux" + +/* div definition */ +#define NPCM8XX_CLK_S_TH "th" +#define NPCM8XX_CLK_S_AXI "axi"Please inline all these string #defines to the place they're used.
Quoted 4 lines
The version V21 you mention using define only when the definition is used more than once
https://www.spinics.net/lists/kernel/msg5045826.html
Should I remove all the string definitions and add the string to the array?Quoted 4 lines
If it's a clk name for a clk registered in this file it should be inlined. Is that the case for everything besides refclk? And even refclk could be inlined so that we don't have to jump to the definition of a string.
I will add the string in the clock arrays and remove all the string definitions.
Quoted 49 lines
Quoted 46 lines
Quoted 17 lines
Quoted 17 lines
drivers/clk/clk-npcm8xx.c new file +16
-0+ +static unsigned long npcm8xx_clk_div_get_parent (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk *div =to_npcm8xx_clk (hw); +unsigned int val; + +regmap_read (div->clk_regmap, div->offset, &val); + val = val >> div->shift; + val &=clk_div_mask (div->width); + +return divider_recalc_rate (hw, parent_rate, val,NULL , div->flags, + div->width); +} + +static const struct clk_ops npcm8xx_clk_div_ops = {Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 352
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ .recalc_rate = npcm8xx_clk_div_get_parent, +}; + +static int npcm8xx_clk_probe (struct platform_device *pdev) +{ +struct device_node *parent_np =of_get_parent (pdev->dev.of_node);The parent of this device is not a syscon.
Once I have registered the map that handles both reset and the clock in general is syscon, this is why we will modify the DTS so the clock and the reset will be under syscon father node
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>; #reset-cells = <2>; nuvoton,sysgcr = <&gcr>; }; clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };Quoted 2 lines
You can see other drivers that using the same method like
https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yamlQuoted 3 lines
You will need a similar file like
Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yamlthen to describe the child nodes.I can do it.
Quoted 3 lines
Socionext may not be the best example to follow. I generally try to avoid syscon and simply put #reset-cells and #clock-cells in the node
If I remove syscon I can't use
syscon_node_to_regmapfunction, What should I use If I remove syscon? auxiliary bus? something else?Quoted 2 lines
for the device. You can use the auxiliary bus to register drivers for clk and reset and put them into the resepective driver directories.
I little bit confused, what is an auxiliary bus to register drivers, can you provide me an example?
Quoted 2 lines
Avoid syscon means random drivers can't reach into the device with a regmap handle and read/write registers that they're not supposed to.
Indeed, but the drivers could use the reset and clock memory map only if the module is also a child node.
Please let me know what is your preferred way to handle it:
1. stick with syscon and upstream-defined documentation for the rst clk syscon.
2. avoid syscon and use an auxiliary bus, appreciate if you could give me an example of how it should be done.
3. Avoid sycon and handle it differently.Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +4
-0+struct clk_hw_onecell_data *npcm8xx_clk_data; +struct device *dev = &pdev->dev; +struct regmap *clk_regmap; +struct clk_hw *hw;drivers/clk/clk-npcm8xx.c line 366
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+unsigned 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_regmap =syscon_node_to_regmap (parent_np); +of_node_put (parent_np);Is there another binding update that is going to move this node to be a child of the syscon?
gcr: system-controller@f0800000 { compatible = "nuvoton,npcm845-gcr", "syscon"; reg = <0x0 0xf0800000 0x0 0x1000>; };Quoted 6 lines
Quoted 3 lines
No, sorry but I'm not going to use the GCR node the handle the clock and reset modules, the GCR has different memory space. the clock driver will have the following device tree
What does the reset driver use the CGR node for? The driver looks like it's using it to control USB phy resets.
Yes, the USB PHY reset is handled through the GCR registers.
Quoted 10 lines
Quoted 8 lines
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>;This isn't a valid reg property for a child node like this.
O.K.
Quoted 5 lines
Quoted 5 lines
#reset-cells = <2>; nuvoton,sysgcr = <&gcr>; };Quoted 7 lines
Quoted 7 lines
clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };Appreciate your guidance!
Thanks,
Tomer
Stephen Boyd
maintainerRe: [PATCH v23 3/3] clk: npcm8xx: add clock controller
In reply to Tomer MaimonQuoting Tomer Maimon (2024-02-29 13:29:46)
Quoted 63 lines
Hi Stephen,
Thanks for your reply.
Quoted 55 lines · Stephen Boyd
Quoting Tomer Maimon (2024-02-25 10:00:35)
Quoted 50 lines
Hi Stephen,
Quoted 18 lines · Stephen Boyd
Quoting Tomer Maimon (2024-01-31 10:26:53)
Quoted 16 lines
drivers/clk/clk-npcm8xx.c new file +16
-0+ +static unsigned long npcm8xx_clk_div_get_parent (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct npcm8xx_clk *div =to_npcm8xx_clk (hw); +unsigned int val; + +regmap_read (div->clk_regmap, div->offset, &val); + val = val >> div->shift; + val &=clk_div_mask (div->width); + +return divider_recalc_rate (hw, parent_rate, val,NULL , div->flags, + div->width); +} + +static const struct clk_ops npcm8xx_clk_div_ops = {Quoted 8 lines
drivers/clk/clk-npcm8xx.c line 352
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ .recalc_rate = npcm8xx_clk_div_get_parent, +}; + +static int npcm8xx_clk_probe (struct platform_device *pdev) +{ +struct device_node *parent_np =of_get_parent (pdev->dev.of_node);The parent of this device is not a syscon.
Once I have registered the map that handles both reset and the clock in general is syscon, this is why we will modify the DTS so the clock and the reset will be under syscon father node
sysctrl: system-controller@f0801000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0xf0801000 0x0 0x1000>; rstc: reset-controller { compatible = "nuvoton,npcm845-reset"; reg = <0x0 0xf0801000 0x0 0xC4>; #reset-cells = <2>; nuvoton,sysgcr = <&gcr>; }; clk: clock-controller { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; }; };Quoted 2 lines
You can see other drivers that using the same method like
https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yamlQuoted 3 lines
You will need a similar file like
Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yamlthen to describe the child nodes.
Quoted 6 lines
I can do it.
Quoted 3 lines
Socionext may not be the best example to follow. I generally try to avoid syscon and simply put #reset-cells and #clock-cells in the node
If I remove syscon I can't use
syscon_node_to_regmapfunction, What should I use If I remove syscon? auxiliary bus? something else?
You should use auxiliary bus. You can make a regmap in the parent driver and pass that to the child auxiliary devices still.
Quoted 4 lines
Quoted 2 lines
for the device. You can use the auxiliary bus to register drivers for clk and reset and put them into the resepective driver directories.
I little bit confused, what is an auxiliary bus to register drivers, can you provide me an example?
$ git grep -l auxiliary_ -- drivers/clk/ drivers/clk/microchip/clk-mpfs.c drivers/clk/starfive/clk-starfive-jh7110-sys.c
You can decide to make either the clk or the reset driver the "main" driver that registers the other auxiliary devices. Either way the DT binding has a single node instead of one per logical driver in the kernel.
Quoted 10 lines
Quoted 2 lines
Avoid syscon means random drivers can't reach into the device with a regmap handle and read/write registers that they're not supposed to.
Indeed, but the drivers could use the reset and clock memory map only if the module is also a child node.
Please let me know what is your preferred way to handle it:
1. stick with syscon and upstream-defined documentation for the rst clk syscon.
2. avoid syscon and use an auxiliary bus, appreciate if you could give me an example of how it should be done.
3. Avoid sycon and handle it differently.
I prefer 2
Quoted 11 lines · Tomer Maimon
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.
for your note:
1. dt-bindings clock modification started from v22 since the upstream npcm8xx clock driver haven't merged yet and requires dt binding update according to the new npcm8xx clock driver.
Quoted 7 lines
2. 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:
- Rob Herring: https://www.spinics.net/lists/devicetree/msg663403.html
- Krzysztof Kozlowski: https://www.spinics.net/lists/devicetree/msg665206.html
Use lore links which are way much more helpful.
Best regards,
Krzysztof