clk: npcm8xx: add clock controller
Subsystems (2)
Tomer Maimon
maintainerPATCH v20 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.
---
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.
drivers/clk/Kconfig | 8 + drivers/clk/Makefile | 1 + drivers/clk/clk-npcm8xx.c | 547 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 556 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_LOONGSON2 bool "Clock driver for Loongson-2 SoC" depends on LOONGARCH || COMPILE_TEST
drivers/clk/Makefile +1 -0
@@ -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_PALMAS) += clk-palmas.o obj-$(CONFIG_CLK_LS1028A_PLLDIG) += clk-plldig.o
drivers/clk/clk-npcm8xx.c new file +547 -0
547 lines not on this page · raw message
Signature · 1 line
2.33.0
Stephen Boyd
maintainerQuoting Tomer Maimon (2023-09-28 15:40:51)
Quoted 8 lines
drivers/clk/clk-npcm8xx.c new file +2
-0@@ -0,0 +1,547 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
[...]
drivers/clk/clk-npcm8xx.c line 250
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ +/* configurable dividers: */ +static const struct npcm8xx_clk_div_data npcm8xx_divs[] = { + {NPCM8XX_CLKDIV1 ,28 ,3 ,NPCM8XX_CLK_S_ADC , + { .name =NPCM8XX_CLK_S_PRE_ADC , .index =-1 }, +CLK_DIVIDER_READ_ONLY |CLK_DIVIDER_POWER_OF_TWO ,0 ,NPCM8XX_CLK_ADC },
Please format this some other way. I assume one line means one clk, but here it is actually three lines. Perhaps something like this?
drivers/clk/clk-npcm8xx.c
Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+ { +NPCM8XX_CLKDIV1 ,28 ,3 ,NPCM8XX_CLK_S_ADC , + { .name = NPCM8XX_CLK_S_PRE_ADC, .index = -1 }, +CLK_DIVIDER_READ_ONLY |CLK_DIVIDER_POWER_OF_TWO ,0 ,NPCM8XX_CLK_ADC + },
Please stop using the .name member of struct clk_parent_data. That member is only there to support drivers that are migrating from a binding that didn't specify the parents of clks that are outside of the clk controller with the clocks property in their DT node. I see that the dts exists upstream, but luckily we don't have a driver merged, so we're free to change the binding to specify clks external to the node. The .fw_name member will match a 'clock-names' element for the registering driver's node. The .index member will match the index in the 'clocks' property. Neither of those properties exist in the nuvoton,npcm845-clk DT binding, so neither of those members shall be present. This means that either the binding needs to be updated, or the clk_parent_data structure should be replaced with clk_hw pointers to describe parents. I'm going to guess that there aren't any external clk parents, so to keep things simple this driver should change to use direct clk_hw pointers to describe topology.
Quoted 5 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+ { NPCM8XX_CLKDIV1, 26, 2, NPCM8XX_CLK_S_AHB, { .hw = &hw_pre_clk }, + CLK_DIVIDER_READ_ONLY, CLK_IS_CRITICAL, NPCM8XX_CLK_AHB }, + { NPCM8XX_CLKDIV1, 21, 5, NPCM8XX_CLK_S_PRE_ADC, + { .hw = &npcm8xx_muxes[6].hw }, CLK_DIVIDER_READ_ONLY, 0, -1 }, + { NPCM8XX_CLKDIV1, 16, 5, NPCM8XX_CLK_S_UART,
drivers/clk/clk-npcm8xx.c line 265
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+ { .hw = &npcm8xx_muxes[3].hw }, 0, 0, NPCM8XX_CLK_UART }, + { NPCM8XX_CLKDIV1, 11, 5, NPCM8XX_CLK_S_MMC, + { .hw = &npcm8xx_muxes[2].hw }, CLK_DIVIDER_READ_ONLY, 0, + NPCM8XX_CLK_MMC }, + { NPCM8XX_CLKDIV1, 6, 5, NPCM8XX_CLK_S_SPI3, + { .fw_name = NPCM8XX_CLK_S_AHB, .name = NPCM8XX_CLK_S_AHB }, 0, 0, + NPCM8XX_CLK_SPI3 }, + { NPCM8XX_CLKDIV1, 2, 4, NPCM8XX_CLK_S_PCI, + { .hw = &npcm8xx_muxes[7].hw }, CLK_DIVIDER_READ_ONLY, 0, + NPCM8XX_CLK_PCI },
BTW, I looked at the dts file upstream (nuvoton-common-npcm8xx.dtsi). The reset and clock controller start at the same address, which can only mean that they're actually the same device. The two nodes should be combined into one node and one driver should match that compatible so that one IO mapping is made for the entire clock and reset contoller register space. If you want, that driver can make two auxiliary device drivers for the reset and clk parts of the io space and then those two drivers can reside in drivers/reset and drivers/clk. I don't know where the driver goes that matches the compatible node though, probably in drivers/soc. This allows us to properly model the logical components that make up the device in hardware (clks and resets) while also allowing any device specific things for that entire register space to live in the soc driver. For example, if some power domain needs to be enabled to access that register space it would be attached to the soc driver.
Tomer Maimon
maintainerHi Stephen,
Thanks for your comments and sorry for the late reply.
Quoted 11 lines · Stephen Boyd
Quoting Tomer Maimon (2023-09-28 15:40:51)
Quoted 8 lines
drivers/clk/clk-npcm8xx.c new file +2
-0@@ -0,0 +1,547 @@ +// SPDX-License-Identifier: GPL-2.0 +/* [...]
Quoted 9 lines
drivers/clk/clk-npcm8xx.c line 250
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +6
-0+ +/* configurable dividers: */ +static const struct npcm8xx_clk_div_data npcm8xx_divs[] = { + {NPCM8XX_CLKDIV1 ,28 ,3 ,NPCM8XX_CLK_S_ADC , + { .name =NPCM8XX_CLK_S_PRE_ADC , .index =-1 }, +CLK_DIVIDER_READ_ONLY |CLK_DIVIDER_POWER_OF_TWO ,0 ,NPCM8XX_CLK_ADC },Please format this some other way. I assume one line means one clk, but here it is actually three lines. Perhaps something like this?
Ready in V21
Quoted 7 lines
drivers/clk/clk-npcm8xx.c
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+ { +NPCM8XX_CLKDIV1 ,28 ,3 ,NPCM8XX_CLK_S_ADC , + { .name = NPCM8XX_CLK_S_PRE_ADC, .index = -1 }, +CLK_DIVIDER_READ_ONLY |CLK_DIVIDER_POWER_OF_TWO ,0 ,NPCM8XX_CLK_ADC + },
Quoted 15 lines
Please stop using the .name member of struct
clk_parent_data. That member is only there to support drivers that are migrating from a binding that didn't specify the parents of clks that are outside of the clk controller with the clocks property in their DT node. I see that the dts exists upstream, but luckily we don't have a driver merged, so we're free to change the binding to specify clks external to the node. The .fw_name member will match a 'clock-names' element for the registering driver's node. The .index member will match the index in the 'clocks' property. Neither of those properties exist in the nuvoton,npcm845-clk DT binding, so neither of those members shall be present. This means that either the binding needs to be updated, or theclk_parent_datastructure should be replaced withclk_hwpointers to describe parents. I'm going to guess that there aren't any external clk parents, so to keep things simple this driver should change to use directclk_hwpointers to describe topology.
Ready in V21
Quoted 17 lines
Quoted 6 lines
drivers/clk/clk-npcm8xx.c new file +5
-0+ { NPCM8XX_CLKDIV1, 26, 2, NPCM8XX_CLK_S_AHB, { .hw = &hw_pre_clk }, + CLK_DIVIDER_READ_ONLY, CLK_IS_CRITICAL, NPCM8XX_CLK_AHB }, + { NPCM8XX_CLKDIV1, 21, 5, NPCM8XX_CLK_S_PRE_ADC, + { .hw = &npcm8xx_muxes[6].hw }, CLK_DIVIDER_READ_ONLY, 0, -1 }, + { NPCM8XX_CLKDIV1, 16, 5, NPCM8XX_CLK_S_UART,drivers/clk/clk-npcm8xx.c line 265
Quoted 10 lines
drivers/clk/clk-npcm8xx.c new file +10
-0+ { .hw = &npcm8xx_muxes[3].hw }, 0, 0, NPCM8XX_CLK_UART }, + { NPCM8XX_CLKDIV1, 11, 5, NPCM8XX_CLK_S_MMC, + { .hw = &npcm8xx_muxes[2].hw }, CLK_DIVIDER_READ_ONLY, 0, + NPCM8XX_CLK_MMC }, + { NPCM8XX_CLKDIV1, 6, 5, NPCM8XX_CLK_S_SPI3, + { .fw_name = NPCM8XX_CLK_S_AHB, .name = NPCM8XX_CLK_S_AHB }, 0, 0, + NPCM8XX_CLK_SPI3 }, + { NPCM8XX_CLKDIV1, 2, 4, NPCM8XX_CLK_S_PCI, + { .hw = &npcm8xx_muxes[7].hw }, CLK_DIVIDER_READ_ONLY, 0, + NPCM8XX_CLK_PCI },
Quoted 3 lines
BTW, I looked at the dts file upstream (
nuvoton-common-npcm8xx.dtsi). The reset and clock controller start at the same address, which can only mean that they're actually the same device. The two nodes should be
unfortunately, It is two services (reset and clock) that are handled in the same memory space.
Quoted 4 lines
combined into one node and one driver should match that compatible so that one IO mapping is made for the entire clock and reset contoller register space. If you want, that driver can make two auxiliary device drivers for the reset and clk parts of the io space and then those two
Quoted 8 lines
drivers can reside in
drivers/resetanddrivers/clk. I don't know where the driver goes that matches the compatible node though, probably indrivers/soc. This allows us to properly model the logical components that make up the device in hardware (clks and resets) while also allowing any device specific things for that entire register space to live in the soc driver. For example, if some power domain needs to be enabled to access that register space it would be attached to the soc driver.
Sorry I didn't understand, do you mean to have one driver that handles the clock and the reset modules and will sis under driver/soc or one driver that handles the reset and clock IO space?
What about using regmap to handle the clock and the reset? for this, the NPCM clock driver will use a unique clock setting like it is done in Tegra clk.
https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/clk/tegra/clk-divider.c
instead of using clk_divide and clk_mux default services.
Thanks,
Tomer