| Message ID | 20240912191038.981105-3-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
Quoting Tomer Maimon (2024-09-12 12:10:37) > Add NPCM8xx clock controller auxiliary bus device registration. > > The NPCM8xx clock controller is registered as an aux device because the > reset and the clock controller share the same register region. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > Tested-by: Benjamin Fair <benjaminfair@google.com> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > --- Applied to clk-next
Hi, On Thu, Sep 12, 2024 at 10:10:37PM +0300, Tomer Maimon wrote: > Add NPCM8xx clock controller auxiliary bus device registration. > > The NPCM8xx clock controller is registered as an aux device because the > reset and the clock controller share the same register region. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > Tested-by: Benjamin Fair <benjaminfair@google.com> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> Does this work with real hardware ? I tried with the new qemu emulation, but that gets stuck in the serial driver initialization. I found that the clock device instantiates but does not register as clock provider because it does not have a device node. I needed something like the patch below to get beyond that point. Thanks, Guenter --- From: Guenter Roeck <linux@roeck-us.net> Subject: [PATCH] reset: npcm: Provide device node to clock driver Without device node, the clock driver can not register itself as clock provider. With debugging enabled, this manifests itself with of_serial f0000000.serial: error -EPROBE_DEFER: failed to get clock of_serial f0000000.serial: Driver of_serial requests probe deferral platform f0000000.serial: Added to deferred list ... Warning: unable to open an initial console. Look up the device node and attach it to the clock device to solve the problem. Fixes: 22823157d90c ("reset: npcm: register npcm8xx clock auxiliary bus device") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/reset/reset-npcm.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c index e5b6127783a7..43bc46755e82 100644 --- a/drivers/reset/reset-npcm.c +++ b/drivers/reset/reset-npcm.c @@ -409,6 +409,8 @@ static struct auxiliary_device *npcm_clock_adev_alloc(struct npcm_rc_data *rst_d adev->name = clk_name; adev->dev.parent = rst_data->dev; adev->dev.release = npcm_clock_adev_release; + adev->dev.of_node = of_find_compatible_node(rst_data->dev->parent->of_node, + NULL, "nuvoton,npcm845-clk"); adev->id = 555u; ret = auxiliary_device_init(adev);
Hi Guenter,
Yes, of course, it works in real hardware.
The modification was made since the reset and clock share the same
register memory region.
To enable the clock change needs to be done in the device tree as
follows (we are planning to send these change patches soon):
diff -Naur a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
--- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
2025-02-26 16:20:39.000000000 +0200
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
2025-03-17 12:29:17.876551537 +0200
@@ -47,19 +47,16 @@
interrupt-parent = <&gic>;
ranges;
- rstc: reset-controller@f0801000 {
+ clk: rstc: reset-controller@f0801000 {
compatible = "nuvoton,npcm845-reset";
- reg = <0x0 0xf0801000 0x0 0x78>;
- #reset-cells = <2>;
+ reg = <0x0 0xf0801000 0x0 0xC4>;
nuvoton,sysgcr = <&gcr>;
- };
-
- clk: clock-controller@f0801000 {
- compatible = "nuvoton,npcm845-clk";
+ #reset-cells = <2>;
+ clocks = <&refclk>;
#clock-cells = <1>;
- reg = <0x0 0xf0801000 0x0 0x1000>;
};
+
apb {
#address-cells = <1>;
#size-cells = <1>;
diff -Naur a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
--- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
2025-02-26 16:20:39.000000000 +0200
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts
2025-03-17 12:24:52.293171764 +0200
@@ -19,6 +19,13 @@
memory@0 {
reg = <0x0 0x0 0x0 0x40000000>;
};
+
+ refclk: refclk-25mhz {
+ compatible = "fixed-clock";
+ clock-output-names = "ref";
+ clock-frequency = <25000000>;
+ #clock-cells = <0>;
+ };
};
&serial0 {
Is it better to modify the reset driver with your suggestion or change
the device tree?
Thanks,
Tomer
On Sun, 16 Mar 2025 at 17:22, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Hi,
>
> On Thu, Sep 12, 2024 at 10:10:37PM +0300, Tomer Maimon wrote:
> > Add NPCM8xx clock controller auxiliary bus device registration.
> >
> > The NPCM8xx clock controller is registered as an aux device because the
> > reset and the clock controller share the same register region.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > Tested-by: Benjamin Fair <benjaminfair@google.com>
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
>
> Does this work with real hardware ? I tried with the new qemu emulation,
> but that gets stuck in the serial driver initialization. I found that the clock
> device instantiates but does not register as clock provider because it does
> not have a device node. I needed something like the patch below to get beyond
> that point.
>
> Thanks,
> Guenter
>
> ---
> From: Guenter Roeck <linux@roeck-us.net>
> Subject: [PATCH] reset: npcm: Provide device node to clock driver
>
> Without device node, the clock driver can not register itself as clock
> provider. With debugging enabled, this manifests itself with
>
> of_serial f0000000.serial: error -EPROBE_DEFER: failed to get clock
> of_serial f0000000.serial: Driver of_serial requests probe deferral
> platform f0000000.serial: Added to deferred list
> ...
> Warning: unable to open an initial console.
>
> Look up the device node and attach it to the clock device to solve the
> problem.
>
> Fixes: 22823157d90c ("reset: npcm: register npcm8xx clock auxiliary bus device")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/reset/reset-npcm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c
> index e5b6127783a7..43bc46755e82 100644
> --- a/drivers/reset/reset-npcm.c
> +++ b/drivers/reset/reset-npcm.c
> @@ -409,6 +409,8 @@ static struct auxiliary_device *npcm_clock_adev_alloc(struct npcm_rc_data *rst_d
> adev->name = clk_name;
> adev->dev.parent = rst_data->dev;
> adev->dev.release = npcm_clock_adev_release;
> + adev->dev.of_node = of_find_compatible_node(rst_data->dev->parent->of_node,
> + NULL, "nuvoton,npcm845-clk");
> adev->id = 555u;
>
> ret = auxiliary_device_init(adev);
> --
> 2.45.2
>
Hi Tomer, On 3/17/25 03:39, Tomer Maimon wrote: > Hi Guenter, > > Yes, of course, it works in real hardware. > The modification was made since the reset and clock share the same > register memory region. > > To enable the clock change needs to be done in the device tree as > follows (we are planning to send these change patches soon): > > diff -Naur a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > 2025-02-26 16:20:39.000000000 +0200 > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > 2025-03-17 12:29:17.876551537 +0200 > @@ -47,19 +47,16 @@ > interrupt-parent = <&gic>; > ranges; > > - rstc: reset-controller@f0801000 { > + clk: rstc: reset-controller@f0801000 { > compatible = "nuvoton,npcm845-reset"; > - reg = <0x0 0xf0801000 0x0 0x78>; > - #reset-cells = <2>; > + reg = <0x0 0xf0801000 0x0 0xC4>; > nuvoton,sysgcr = <&gcr>; > - }; > - > - clk: clock-controller@f0801000 { > - compatible = "nuvoton,npcm845-clk"; > + #reset-cells = <2>; > + clocks = <&refclk>; > #clock-cells = <1>; > - reg = <0x0 0xf0801000 0x0 0x1000>; > }; > > + > apb { > #address-cells = <1>; > #size-cells = <1>; > diff -Naur a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > 2025-02-26 16:20:39.000000000 +0200 > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > 2025-03-17 12:24:52.293171764 +0200 > @@ -19,6 +19,13 @@ > memory@0 { > reg = <0x0 0x0 0x0 0x40000000>; > }; > + > + refclk: refclk-25mhz { > + compatible = "fixed-clock"; > + clock-output-names = "ref"; > + clock-frequency = <25000000>; > + #clock-cells = <0>; > + }; > }; > > &serial0 { > > Is it better to modify the reset driver with your suggestion or change > the device tree? > My assumption was that the devicetree file is correct, and that it would match the devicetree file in the actual devices. I since noticed that the file is widely incomplete when comparing it with the various downstream versions, so that was obviously wrong. Also, my change seemed odd, but then I did not know how such situations are supposed to be handled. Also, it looks like the devicetree file needs to be changed anyway unless something else is wrong, because booting Linux still stalls later. Presumably that is because the reference clock is missing in the upstream devicetree file (the serial port clock frequency is reported as 0). Given this, fixing the devicetree files seems to be the way to go. Thanks, Guenter
Hi Guenter, Thanks a lot for your recommendations and sorry for the inconvenience. We will fix the device tree and send the patch soon. Best regards, Tomer On Mon, 17 Mar 2025 at 16:09, Guenter Roeck <linux@roeck-us.net> wrote: > > Hi Tomer, > > On 3/17/25 03:39, Tomer Maimon wrote: > > Hi Guenter, > > > > Yes, of course, it works in real hardware. > > The modification was made since the reset and clock share the same > > register memory region. > > > > To enable the clock change needs to be done in the device tree as > > follows (we are planning to send these change patches soon): > > > > diff -Naur a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > 2025-02-26 16:20:39.000000000 +0200 > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > 2025-03-17 12:29:17.876551537 +0200 > > @@ -47,19 +47,16 @@ > > interrupt-parent = <&gic>; > > ranges; > > > > - rstc: reset-controller@f0801000 { > > + clk: rstc: reset-controller@f0801000 { > > compatible = "nuvoton,npcm845-reset"; > > - reg = <0x0 0xf0801000 0x0 0x78>; > > - #reset-cells = <2>; > > + reg = <0x0 0xf0801000 0x0 0xC4>; > > nuvoton,sysgcr = <&gcr>; > > - }; > > - > > - clk: clock-controller@f0801000 { > > - compatible = "nuvoton,npcm845-clk"; > > + #reset-cells = <2>; > > + clocks = <&refclk>; > > #clock-cells = <1>; > > - reg = <0x0 0xf0801000 0x0 0x1000>; > > }; > > > > + > > apb { > > #address-cells = <1>; > > #size-cells = <1>; > > diff -Naur a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > 2025-02-26 16:20:39.000000000 +0200 > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > 2025-03-17 12:24:52.293171764 +0200 > > @@ -19,6 +19,13 @@ > > memory@0 { > > reg = <0x0 0x0 0x0 0x40000000>; > > }; > > + > > + refclk: refclk-25mhz { > > + compatible = "fixed-clock"; > > + clock-output-names = "ref"; > > + clock-frequency = <25000000>; > > + #clock-cells = <0>; > > + }; > > }; > > > > &serial0 { > > > > Is it better to modify the reset driver with your suggestion or change > > the device tree? > > > > My assumption was that the devicetree file is correct, and that it would match > the devicetree file in the actual devices. I since noticed that the file is > widely incomplete when comparing it with the various downstream versions, > so that was obviously wrong. Also, my change seemed odd, but then I did > not know how such situations are supposed to be handled. > > Also, it looks like the devicetree file needs to be changed anyway unless something > else is wrong, because booting Linux still stalls later. Presumably that is because > the reference clock is missing in the upstream devicetree file (the serial port clock > frequency is reported as 0). Given this, fixing the devicetree files seems to be the > way to go. > > Thanks, > Guenter >
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 67bce340a87e..c6bf5275cca2 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -157,6 +157,7 @@ config RESET_MESON_AUDIO_ARB config RESET_NPCM bool "NPCM BMC Reset Driver" if COMPILE_TEST default ARCH_NPCM + select AUXILIARY_BUS help This enables the reset controller driver for Nuvoton NPCM BMC SoCs. diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c index 8935ef95a2d1..4f3b9fc58de0 100644 --- a/drivers/reset/reset-npcm.c +++ b/drivers/reset/reset-npcm.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2019 Nuvoton Technology corporation. +#include <linux/auxiliary_bus.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/io.h> @@ -10,11 +11,14 @@ #include <linux/property.h> #include <linux/reboot.h> #include <linux/reset-controller.h> +#include <linux/slab.h> #include <linux/spinlock.h> #include <linux/mfd/syscon.h> #include <linux/regmap.h> #include <linux/of_address.h> +#include <soc/nuvoton/clock-npcm8xx.h> + /* NPCM7xx GCR registers */ #define NPCM_MDLR_OFFSET 0x7C #define NPCM7XX_MDLR_USBD0 BIT(9) @@ -89,6 +93,7 @@ struct npcm_rc_data { const struct npcm_reset_info *info; struct regmap *gcr_regmap; u32 sw_reset_number; + struct device *dev; void __iomem *base; spinlock_t lock; }; @@ -372,6 +377,67 @@ static const struct reset_control_ops npcm_rc_ops = { .status = npcm_rc_status, }; +static void npcm_clock_unregister_adev(void *_adev) +{ + struct auxiliary_device *adev = _adev; + + auxiliary_device_delete(adev); + auxiliary_device_uninit(adev); +} + +static void npcm_clock_adev_release(struct device *dev) +{ + struct auxiliary_device *adev = to_auxiliary_dev(dev); + struct npcm_clock_adev *rdev = to_npcm_clock_adev(adev); + + kfree(rdev); +} + +static struct auxiliary_device *npcm_clock_adev_alloc(struct npcm_rc_data *rst_data, char *clk_name) +{ + struct npcm_clock_adev *rdev; + struct auxiliary_device *adev; + int ret; + + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL); + if (!rdev) + return ERR_PTR(-ENOMEM); + + rdev->base = rst_data->base; + + adev = &rdev->adev; + adev->name = clk_name; + adev->dev.parent = rst_data->dev; + adev->dev.release = npcm_clock_adev_release; + adev->id = 555u; + + ret = auxiliary_device_init(adev); + if (ret) { + kfree(rdev); + return ERR_PTR(ret); + } + + return adev; +} + +static int npcm8xx_clock_controller_register(struct npcm_rc_data *rst_data, char *clk_name) +{ + struct auxiliary_device *adev; + int ret; + + adev = npcm_clock_adev_alloc(rst_data, clk_name); + if (IS_ERR(adev)) + return PTR_ERR(adev); + + ret = auxiliary_device_add(adev); + if (ret) { + auxiliary_device_uninit(adev); + return ret; + } + + return devm_add_action_or_reset(rst_data->dev, npcm_clock_unregister_adev, adev); +} + static int npcm_rc_probe(struct platform_device *pdev) { struct npcm_rc_data *rc; @@ -392,6 +458,7 @@ static int npcm_rc_probe(struct platform_device *pdev) rc->rcdev.of_node = pdev->dev.of_node; rc->rcdev.of_reset_n_cells = 2; rc->rcdev.of_xlate = npcm_reset_xlate; + rc->dev = &pdev->dev; ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev); if (ret) { @@ -408,12 +475,19 @@ static int npcm_rc_probe(struct platform_device *pdev) rc->restart_nb.priority = 192, rc->restart_nb.notifier_call = npcm_rc_restart, ret = register_restart_handler(&rc->restart_nb); - if (ret) + if (ret) { dev_warn(&pdev->dev, "failed to register restart handler\n"); + return ret; + } } } - return ret; + switch (rc->info->bmc_id) { + case BMC_NPCM8XX: + return npcm8xx_clock_controller_register(rc, "clk-npcm8xx"); + default: + return 0; + } } static struct platform_driver npcm_rc_driver = { diff --git a/include/soc/nuvoton/clock-npcm8xx.h b/include/soc/nuvoton/clock-npcm8xx.h new file mode 100644 index 000000000000..1d974e89d8a8 --- /dev/null +++ b/include/soc/nuvoton/clock-npcm8xx.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __SOC_NPCM8XX_CLOCK_H +#define __SOC_NPCM8XX_CLOCK_H + +#include <linux/auxiliary_bus.h> +#include <linux/container_of.h> + +struct npcm_clock_adev { + void __iomem *base; + struct auxiliary_device adev; +}; + +static inline struct npcm_clock_adev *to_npcm_clock_adev(struct auxiliary_device *_adev) +{ + return container_of(_adev, struct npcm_clock_adev, adev); +} + +#endif