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

[Tomer Maimon]

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@xxxxxxxxxxxx> 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
>