Re: [PATCH v28 2/3] reset: npcm: register npcm8xx clock auxiliary bus device

[Guenter Roeck <linux@roeck-us.net>]

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