Re: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device tree

[Geert Uytterhoeven]

Hi Tomer,

On Fri, Jun 10, 2022 at 12:30 AM Tomer Maimon <tmaimon77@xxxxxxxxx> wrote:
> On Wed, 8 Jun 2022 at 13:21, Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > On 08/06/2022 11:56, Tomer Maimon wrote:
> > > This adds initial device tree support for the
> > > Nuvoton NPCM845 Board Management controller (BMC) SoC family.
> > >
> > > The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and
> > > have various peripheral IPs.
> > >
> > > Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>

> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > > @@ -0,0 +1,197 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2021 Nuvoton Technology tomer.maimon@xxxxxxxxxxx
> > > +
> > > +#include <dt-bindings/clock/nuvoton,npcm8xx-clock.h>
> > > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +#include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +/ {
> > > +     #address-cells = <2>;
> > > +     #size-cells = <2>;
> > > +     interrupt-parent = <&gic>;
> > > +
> > > +     /* external reference clock */
> > > +     clk_refclk: clk-refclk {
> > > +             compatible = "fixed-clock";
> > > +             #clock-cells = <0>;
> > > +             clock-frequency = <25000000>;
> >
> > Ignored comment.
> Could we use it as a default clock-frequency?

If the oscillator is present on the board, and not an SoC builtin, its
clock frequency should be described in the board DTS.
Some clocks may be optional, and left unpopulated.
Others clocks may be fed with different frequencies than the default.

> >
> > > +             clock-output-names = "refclk";
> > > +     };
> > > +
> > > +     /* external reference clock for cpu. float in normal operation */
> > > +     clk_sysbypck: clk-sysbypck {
> > > +             compatible = "fixed-clock";
> > > +             #clock-cells = <0>;
> > > +             clock-frequency = <1000000000>;
> >
> > Ignored comment.
> same as above
> >
> > > +             clock-output-names = "sysbypck";
> > > +     };
> > > +
> > > +     /* external reference clock for MC. float in normal operation */
> > > +     clk_mcbypck: clk-mcbypck {
> > > +             compatible = "fixed-clock";
> > > +             #clock-cells = <0>;
> > > +             clock-frequency = <1050000000>;
> same as above
> > > +             clock-output-names = "mcbypck";
> > > +     };

>  "+             cpu0: cpu@0 {
>  +                     device_type = "cpu";
>  +                     compatible = "arm,cortex-a35";
>  +                     clocks = <&clk NPCM8XX_CLK_CPU>;
>  +                     reg = <0x0 0x0>;
> Why do you have two address cells? A bit more complicated and not
> necessary, I think."
> the arm,cortex-a35 is 64 Bit this is why we use  #address-cells = <2>;
> and therefore reg = <0x0 0x0>;

These addresses are not addresses on the main memory bus (which
is indeed 64-bit), but on the logical CPU bus.
Now, Documentation/devicetree/bindings/arm/cpus.yaml says you can
have #address-cells = <2> if you have non-zero MPIDR_EL1 high bits.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds