Re: [PATCH v3 7/7] riscv: dts: starfive: Add StarFive JH7110 VisionFive 2 board device tree

Emil Renner Berthing
2023-02-16T09:28:02Z

On Wed, 15 Feb 2023 at 15:04, Hal Feng <hal.feng@starfivetech.com> wrote:
>
> On 2/14/2023 5:53 PM, Emil Renner Berthing wrote:
> > On Tue, 20 Dec 2022 at 02:12, Hal Feng <hal.feng@starfivetech.com> wrote:
> >>
> >> From: Emil Renner Berthing <kernel@esmil.dk>
> >>
> >> Add a minimal device tree for StarFive JH7110 VisionFive 2 board
> >> which has version A and version B. Support booting and basic
> >> clock/reset/pinctrl/uart drivers.
> >>
> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> >> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> >> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >> ---
> >> arch/riscv/boot/dts/starfive/Makefile | 1 +
> >> .../jh7110-starfive-visionfive-2-va.dts | 13 ++
> >> .../jh7110-starfive-visionfive-2-vb.dts | 13 ++
> >> .../jh7110-starfive-visionfive-2.dtsi | 111 ++++++++++++++++++
> >> 4 files changed, 138 insertions(+)
> >> create mode 100644 arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-va.dts
> >> create mode 100644 arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-vb.dts
> >> create mode 100644 arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> >>
> >> diff --git a/arch/riscv/boot/dts/starfive/Makefile b/arch/riscv/boot/dts/starfive/Makefile
> >> index 0ea1bc15ab30..79e925a4a227 100644
> >> --- a/arch/riscv/boot/dts/starfive/Makefile
> >> +++ b/arch/riscv/boot/dts/starfive/Makefile
> >> @@ -1,2 +1,3 @@
> >> # SPDX-License-Identifier: GPL-2.0
> >> dtb-$(CONFIG_SOC_STARFIVE) += jh7100-beaglev-starlight.dtb
> >> +dtb-$(CONFIG_SOC_STARFIVE) += jh7110-starfive-visionfive-2-va.dtb jh7110-starfive-visionfive-2-vb.dtb
> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-va.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-va.dts
> >> new file mode 100644
> >> index 000000000000..188d3fddbe88
> >> --- /dev/null
> >> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-va.dts
> >> @@ -0,0 +1,13 @@
> >> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> >> +/*
> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> >> + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
> >> + */
> >> +
> >> +/dts-v1/;
> >> +#include "jh7110-starfive-visionfive-2.dtsi"
> >> +
> >> +/ {
> >> + model = "StarFive VisionFive 2 VA";
> >> + compatible = "starfive,visionfive-2-va", "starfive,jh7110";
> >> +};
> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-vb.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-vb.dts
> >> new file mode 100644
> >> index 000000000000..f75c10536f84
> >> --- /dev/null
> >> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-vb.dts
> >> @@ -0,0 +1,13 @@
> >> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> >> +/*
> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> >> + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
> >> + */
> >> +
> >> +/dts-v1/;
> >> +#include "jh7110-starfive-visionfive-2.dtsi"
> >> +
> >> +/ {
> >> + model = "StarFive VisionFive 2 VB";
> >> + compatible = "starfive,visionfive-2-vb", "starfive,jh7110";
> >
> > Hi Hal,
> >
> > I just want to double check, is "VisionFive 2 VA" / "visoinfive-2-va"
> > and "VisionFive 2 VB" / "visionfive-2-vb" really what you want? I
> > still think having these names match what is printed on the silkscreen
> > makes it a lot easier for everybody. Even your own releases calls the
> > boards "v1.2A" and "v1.3B":
> > https://github.com/starfive-tech/VisionFive2/releases/
> >
> > So I'd suggest
> > model = "StarFive VisionFive 2 v1.3B";
> > compatible = "starfive,visionfive-2-v1.3b", "starfive,jh7110";
> >
> > I haven't seen these "VA" and "VB" anywhere else, so if you don't want
> > the version numbers and can promise that there will be no incompatible
> > future revisions of the boards then maybe just drop the "V". Eg.
> > model = "StarFive VisionFive 2 B";
> > compatible = "starfive,visionfive-2-b", "starfive,jh7110";
>
> The version A board has reached the end of life. As far as I know, the
> version B board will not update also unless there are some important
> requirements and StarFive decides to update. Furthermore, it's too late
> to change the compatible as patch 1 was already accepted. Will it be
> easier to read if I modify it as below?
>
> model = "StarFive VisionFive 2 vB";
> compatible = "starfive,visionfive-2-vb", "starfive,jh7110";

Oh, that's sad that the kernel will now end up calling the boards
something that's used nowhere else, even by StarFive :/
But yeah, I guess vA and vB are a little easier to read.

> Best regards,
> Hal