Re: [RFC PATCH v2 09/11] riscv: soc: Initial DTS for Allwinner D1 NeZha board

[Maxime Ripard]

On Mon, Jun 07, 2021 at 11:44:03AM +0800, Guo Ren wrote:
> On Mon, Jun 7, 2021 at 12:26 AM Jernej Škrabec <jernej.skrabec@xxxxxxxxx> wrote:
> >
> > Hi!
> >
> > I didn't go through all details. After you fix all comments below, you should
> > run "make dtbs_check" and fix all reported warnings too.
> >
> > Dne nedelja, 06. junij 2021 ob 11:04:07 CEST je guoren@xxxxxxxxxx napisal(a):
> > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > >
> > > Add initial DTS for Allwinner D1 NeZha board having only essential
> > > devices (uart, dummy, clock, reset, clint, plic, etc).
> > >
> > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > Co-Developed-by: Liu Shaohua <liush@xxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Liu Shaohua <liush@xxxxxxxxxxxxxxxxx>
> > > Cc: Anup Patel <anup.patel@xxxxxxx>
> > > Cc: Atish Patra <atish.patra@xxxxxxx>
> > > Cc: Christoph Hellwig <hch@xxxxxx>
> > > Cc: Chen-Yu Tsai <wens@xxxxxxxx>
> > > Cc: Drew Fustini <drew@xxxxxxxxxxxxxxx>
> > > Cc: Maxime Ripard <maxime@xxxxxxxxxx>
> > > Cc: Palmer Dabbelt <palmerdabbelt@xxxxxxxxxx>
> > > Cc: Wei Fu <wefu@xxxxxxxxxx>
> > > Cc: Wei Wu <lazyparser@xxxxxxxxx>
> > > ---
> > >  arch/riscv/boot/dts/Makefile                       |  1 +
> > >  arch/riscv/boot/dts/allwinner/Makefile             |  2 +
> > >  .../boot/dts/allwinner/allwinner-d1-nezha-kit.dts  | 29 ++++++++
> > >  arch/riscv/boot/dts/allwinner/allwinner-d1.dtsi    | 84
> > > ++++++++++++++++++++++ 4 files changed, 116 insertions(+)
> > >  create mode 100644 arch/riscv/boot/dts/allwinner/Makefile
> > >  create mode 100644 arch/riscv/boot/dts/allwinner/allwinner-d1-nezha-kit.dts
> > > create mode 100644 arch/riscv/boot/dts/allwinner/allwinner-d1.dtsi
> > >
> > > diff --git a/arch/riscv/boot/dts/Makefile b/arch/riscv/boot/dts/Makefile
> > > index fe996b8..3e7b264 100644
> > > --- a/arch/riscv/boot/dts/Makefile
> > > +++ b/arch/riscv/boot/dts/Makefile
> > > @@ -2,5 +2,6 @@
> > >  subdir-y += sifive
> > >  subdir-$(CONFIG_SOC_CANAAN_K210_DTB_BUILTIN) += canaan
> > >  subdir-y += microchip
> > > +subdir-y += allwinner
> > >
> > >  obj-$(CONFIG_BUILTIN_DTB) := $(addsuffix /, $(subdir-y))
> > > diff --git a/arch/riscv/boot/dts/allwinner/Makefile
> > > b/arch/riscv/boot/dts/allwinner/Makefile new file mode 100644
> > > index 00000000..4adbf4b
> > > --- /dev/null
> > > +++ b/arch/riscv/boot/dts/allwinner/Makefile
> > > @@ -0,0 +1,2 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +dtb-$(CONFIG_SOC_SUNXI) += allwinner-d1-nezha-kit.dtb
> > > diff --git a/arch/riscv/boot/dts/allwinner/allwinner-d1-nezha-kit.dts
> > > b/arch/riscv/boot/dts/allwinner/allwinner-d1-nezha-kit.dts new file mode
> > > 100644
> > > index 00000000..cd9f7c9
> > > --- /dev/null
> > > +++ b/arch/riscv/boot/dts/allwinner/allwinner-d1-nezha-kit.dts
> >
> > Board DT names are comprised of soc name and board name, in this case it would
> > be "sun20i-d1-nezha-kit.dts"
> >
> > > @@ -0,0 +1,29 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >
> > Usually copyrights are added below spdx id.
> >
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "allwinner-d1.dtsi"
> > > +
> > > +/ {
> > > +     #address-cells = <2>;
> > > +     #size-cells = <2>;
> >
> > This should be part of SoC level DTSI.
> >
> > > +     model = "Allwinner D1 NeZha Kit";
> > > +     compatible = "allwinner,d1-nezha-kit";
> >
> > Board specific compatible string should be followed with SoC compatible, in
> > this case "allwinner,sun20i-d1".  You should document it too.
> >
> > > +
> > > +     chosen {
> > > +             bootargs = "console=ttyS0,115200";
> >
> > Above line doesn't belong here. If anything, it should be added dynamically by
> > bootloader.
>
> After discussion, we still want to keep a default value here.
> Sometimes we could boot with jtag and parse dtb is hard for gdbinit
> script.
>
> >
> > > +             stdout-path = &serial0;
> > > +     };
> > > +
> > > +     memory@40000000 {
> > > +             device_type = "memory";
> > > +             reg = <0x0 0x40000000 0x0 0x20000000>;
> > > +     };
> >
> > Ditto for whole memory node.
>
> Ditto

The thing is that there's never a good value for a default. Let's take
the memory node here: what would be a good default? If we want to make
it work everywhere it's going to be the lowest amount of memory
available on the D1 boards. It's going to be hard to maintain and very
likely to be overlooked, resulting in broken boards anyway.

If someone is savvy enough to use JTAG, it's not really difficult to
modify the DT for their board when they need it.

Maxime

Attachment: signature.asc
Description: PGP signature