All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yao Zi <me@ziyao.cc>
To: "E Shattow" <e@freeshell.de>,
	"João Paulo Gonçalves" <jpaulo.silvagoncalves@gmail.com>,
	u-boot@lists.u-boot-project.org, "Yao Zi" <me@ziyao.cc>
Cc: Tom Rini <trini@konsulko.com>, Sumit Garg <sumit.garg@kernel.org>,
	Gui-Dong Han <hanguidong02@gmail.com>,
	Joshua Milas <josh.milas@gmail.com>,
	Chen Wang <chen.wang@linux.dev>,
	Inochi Amaoto <inochiama@gmail.com>,
	Lukasz Majewski <lukma@nabladev.com>, Peng Fan <peng.fan@nxp.com>,
	Tim Ouyang <tim609@andestech.com>,
	Leo Liang <leo.liang@sifive.com>,
	Hiago De Franco <hfranco@baylibre.com>,
	Raymond Mao <raymond.mao@riscstar.com>,
	Michal Simek <michal.simek@amd.com>, Yixun Lan <dlan@kernel.org>,
	Andrei Lalaev <andrey.lalaev@gmail.com>
Subject: Re: [PATCH 4/4] board: sophgo: Add support for Milk-V Duo S
Date: Sat, 17 Oct 2026 04:07:02 +0000	[thread overview]
Message-ID: <atL0ZiK90D-tzC67@pie> (raw)
In-Reply-To: <d61a5f4a-f7eb-496f-86b9-70aedcfe2e66@freeshell.de>

On Fri, Oct 16, 2026 at 04:51:34PM -0700, E Shattow wrote:
> Hi João Paulo,
> 
> On 10/16/26 12:52, João Paulo Gonçalves wrote:
> > Add support for the Milk-V Duo S, a development board based on the
> > Sophgo SG2000 SoC with 512 MiB of RAM.
> > 
> > The SG2000 differs from the CV1800B used on the Milk-V Duo 64M and the
> > SG2002 used on the Milk-V Duo 256M, but these SoCs share most of their
> > IP blocks, including the CPU core, so the existing CV1800B support can
> > be reused.
> > 
> > Add a separate board directory, target and defconfig for the Duo S,
> > based on the Milk-V Duo 256M ones. Like the Duo 256M, it reuses the
> > common Milk-V Duo board code. The defconfig uses the upstream devicetree
> > via OF_UPSTREAM.
> > 
> > Link: https://milkv.io/docs/duo/getting-started/duos
> > Signed-off-by: João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
> > Acked-by: Andrei Lalaev <andrey.lalaev@gmail.com>
> > Acked-by: Hiago De Franco <hfranco@baylibre.com>
> > ---
> >  arch/riscv/Kconfig                       |  4 ++
> >  board/sophgo/milkv_duo_s/Kconfig         | 30 +++++++++++
> >  board/sophgo/milkv_duo_s/MAINTAINERS     |  8 +++
> >  board/sophgo/milkv_duo_s/Makefile        |  6 +++
> >  board/sophgo/milkv_duo_s/milkv_duo_s.env |  8 +++
> >  configs/milkv_duo_s_defconfig            | 43 ++++++++++++++++
> >  doc/board/sophgo/index.rst               |  1 +
> >  doc/board/sophgo/milkv_duo_s.rst         | 86 ++++++++++++++++++++++++++++++++
> >  include/configs/milkv_duo_s.h            | 12 +++++
> >  9 files changed, 198 insertions(+)

...

> > diff --git a/board/sophgo/milkv_duo_s/milkv_duo_s.env b/board/sophgo/milkv_duo_s/milkv_duo_s.env
> > new file mode 100644
> > index 000000000000..4c9f53d08118
> > --- /dev/null
> > +++ b/board/sophgo/milkv_duo_s/milkv_duo_s.env
> > @@ -0,0 +1,8 @@
> > +fdt_addr_r=0x84000000
> > +fdtfile=DEFAULT_DEVICE_TREE.dtb
> 
> Sorry I do not understand, what is "DEFAULT_DEVICE_TREE.dtb" where is
> this file actually?

It would be expanded to the value of CONFIG_DEFAULT_DEVICE_TREE, with
quotes stripped. See usage/environment.rst, and Makefile:2166.

...

> > diff --git a/doc/board/sophgo/milkv_duo_s.rst b/doc/board/sophgo/milkv_duo_s.rst
> > new file mode 100644
> > index 000000000000..2ae749a08215
> > --- /dev/null
> > +++ b/doc/board/sophgo/milkv_duo_s.rst
> > @@ -0,0 +1,86 @@
> > +.. SPDX-License-Identifier: GPL-2.0+
> > +
> > +Milk-V Duo S
> > +============
> > +
> > +SG2000 RISC-V SoC
> > +-----------------
> > +The SG2000 is a high-performance, low-power 64-bit RISC-V/ARM SoC from Sophgo.
> > +
> > +Mainline support
> > +----------------
> > +Support for the following drivers is already enabled:
> > +
> > +1. ns16550 UART Driver.
> > +2. Synopsys Designware MSHC Driver.
> > +3. Synopsys Designware Ethernet Controller.
> > +
> > +Building
> > +~~~~~~~~
> > +1. Add the RISC-V toolchain to your PATH.
> > +2. Setup ARCH & cross compilation environment variable:
> > +
> > +.. code-block:: console
> > +
> > +   export CROSS_COMPILE=<riscv64 toolchain prefix>
> > +   cd <U-Boot-dir>> +   make milkv_duo_s_defconfig
> > +   make
> 
> Consider this variation:
> 
> export CROSS_COMPILE=<riscv64 toolchain prefix>
> make -C u-boot.git O=../u-boot milkv_duo_s_defconfig
> make -C u-boot.git O=../u-boot

I don't think this improves things much, let's stop being nitpicking
here.

> > +
> > +This will generate u-boot.bin
> > +
> > +Booting
> > +~~~~~~~
> > +Currently, we rely on vendor FSBL (First Stage Boot Loader) to initialize the
> > +clock and load OpenSBI and the u-boot image, then bootup from it.
> > +
> > +To run u-boot.bin on top of FSBL, follow these steps:
> > +
> > +1. Generate a compatible u-boot.bin using U-Boot with the Milk-V Duo S
> > +   default configuration.
> > +
> > +2. Use mainline OpenSBI with a newer version than 1.5 to generate fw_dynamic.
> 
> Suspicious statement, I think it is wrong here.
> 
> Is this in regard to mainline OpenSBI commit f7a92f6b ("lib: utils/fdt:
> Add support for parsing riscv,isa-extensions") ?   If so, a short
> statement e.g. "Use OpenSBI for generic platform with support for
> parsing riscv,isa-extensions, introduced in mainline OpenSBI v1.6
> release" or simply "Use mainline OpenSBI v1.6+" because you don't tell
> us anyways why it is important that the needed change happens between
> v1.5 and v1.6 OpenSBI version.

I think it's likely related to 2bed4c1c57a7 ("platform: generic: thead:
add Sophgo CV18XX/SG200X series"), which applies a PMU-related quirk to
SG2002. But anyway, yes, an explanation of the version requirement would
be helpful, though I doubt it's SoC-specific instead of board-specific,
thus better put in a separate file.

> > +   Use FW_FDT_PATH to point to sg2000-milkv-duo-s.dtb device tree file while
> > +   compiling fw_dynamic. Example:
> > +
> > +.. code-block:: console
> > +
> > +   cd open-sbi
> > +   PLATFORM=generic FW_FDT_PATH=../u-boot/dts/upstream/src/riscv/sophgo/sg2000-milkv-duo-s.dtb make
> 
> See above comment about setting the O= make variable. Consider this again:
> 
> make -C opensbi.git O=../opensbi clean
> make -C opensbi.git O=../opensbi PLATFORM=generic
> FW_FDT_PATH=../u-boot/dts/upstream/src/riscv/sophgo/sg2000-milkv-duo-s.dtb
> 
> An explicit invocation of clean target before generic platform build
> should be listed because of a well-known OpenSBI build problem:
> 
> https://lore.kernel.org/opensbi/0d6bf7c7-7605-4ad6-a35c-54efc5230fb8@sifive.com/

This sounds like an OpenSBI bug, and I'm not sure whether we should pay
for it to maintain it in the documentation.

Furthermore, anyone building OpenSBI would be affected by the issue,
which makes it sound more unnatural to emphasize this in a board-level
documentation (e.g., should we modify all other RISC-V ports to mention
this?).

To me, the board/SoC-level documentation is only responsible for
describing the board/SoC-specific stuff. It's necessary to mention how
to build related firmware, but I don't think we should get too deep
here.

> > +
> > +This will generate build/platform/generic/firmware/fw_dynamic.bin.
> > +
> > +3. Use the vendor-provided tool [1] to create a unified fip.bin file containing
> 
> I think this description is correct but it just sounds strange to me
> with the word "unified" for a mix of libre open source software into
> third-party proprietary tools and proprietary executable code blob.

Sophgo FSBL had been source-available for a long time, and Sophgo has
recently explicitly licensed it. Thus it doesn't sound correct to call
them "proprietary" and "blob".

> Consider "Use the vendor-provided fiptool [1] to prepare FSBL containing..."

This is incorrect. In the context of CV18xx/SG200x bootflow, FSBL refers
to the bootloader preceeding U-Boot (whether it's the vendor one or not),
and fip.bin is combined from FSBL, OpenSBI, and U-Boot.

> > +   FSBL, OpenSBI, and U-Boot.
> 
> "SPL, OpenSBI with initial device tree and U-Boot payload"
> 
> > +   Note that you will have to use the file cv181x.bin as the FSBL.
> 
> Naming what we would want from U-Boot SPL to be known as "SPL" and then
> wrapped with mkimage action to become "FSBL" is more easily
> understandable. We do not have this yet, but think about a future where
> there is U-Boot SPL and mkimage support here, how would you describe
> this? Use that and apply it to the third party vendor workflow now.

Sorry for not getting you points, but I couldn't understand what
cannot be described...

> If cv181x.bin is ready-to-go rename as "fip.bin" and SoC BootROM loader
> functionality accepts this (no fiptool) then yes it is also true to
> describe as "FSBL".
> 
> In fact the terminology FSBL and SPL may be equivalent but we can make
> this arbitrary distinction in documentation to make it clearer what is
> the compiled executable code SPL and what is the SPL image formed with
> additional preparation.

This sounds reasonable. However, I don't think it should belong to this
series adding support for another board.

In case that you don't know, CV1800B, SG2000, and SG2002 have the same
bootflow, and we already have CV1800B (milkv_duo) and SG2002
(milkv_duo_256m) support available in mainline U-Boot, without
explaining these terms. So I don't think we should put such burden on
one adding a new board.

It'll be nice if Joao could add some related documentation, but I don't
think it's a blocker for merging this series.

> > +
> > +4. Place the generated fip.bin file into the FAT partition of the SD card.
> 
> Consider "Place the prepared fip.bin file" or better "Copy the prepared
> FSBL as fip.bin file into the FAT partition of the SD card."
> 
> I like "prepared" word to describe wrapping up one or many data sources
> in a header expected by the SoC BootROM loader functionality.

I think this is a little too nitpicking. This is probably copied from
documentation of other CV18xx/SG200x boards, and I don't think there's a
strong difference among the three versions...

...

> > diff --git a/include/configs/milkv_duo_s.h b/include/configs/milkv_duo_s.h
> > new file mode 100644
> > index 000000000000..f60aa23fc181
> > --- /dev/null
> > +++ b/include/configs/milkv_duo_s.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2026, João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
> > + *
> > + */
> > +
> > +#ifndef __CONFIG_H
> > +#define __CONFIG_H
> > +
> > +#define CFG_SYS_SDRAM_BASE	0x80000000
> > +
> > +#endif /* __CONFIG_H */
> > 
> 
> If able, please reverse-engineer / port support for the Milk-V Duo S
> FSBL image format to U-Boot mkimage tool. Also look more closely at
> fully-relocatable OpenSBI fw_dynamic.bin support.

Nice point, but this won't be a blocker for the series, either :)

> -E Shattow

Best regards,
Yao Zi

  reply	other threads:[~2026-10-17  4:07 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-16 19:52 [PATCH 0/4] board: sophgo: Add support for Milk-V Duo S João Paulo Gonçalves
2026-10-16 19:52 ` [PATCH 1/4] riscv64: dts: sophgo: add SG2000 dtsi João Paulo Gonçalves
2026-10-16 19:52 ` [PATCH 2/4] riscv64: dts: sophgo: add initial Milk-V Duo S board support João Paulo Gonçalves
2026-10-16 19:52 ` [PATCH 3/4] clk: sophgo: cv1800b: Add SG2000 compatible string João Paulo Gonçalves
2026-10-16 23:52   ` E Shattow
2026-10-16 19:52 ` [PATCH 4/4] board: sophgo: Add support for Milk-V Duo S João Paulo Gonçalves
2026-10-16 23:51   ` E Shattow
2026-10-17  4:07     ` Yao Zi [this message]
2026-10-16 21:54 ` [PATCH 0/4] " Hiago De Franco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=atL0ZiK90D-tzC67@pie \
    --to=me@ziyao.cc \
    --cc=andrey.lalaev@gmail.com \
    --cc=chen.wang@linux.dev \
    --cc=dlan@kernel.org \
    --cc=e@freeshell.de \
    --cc=hanguidong02@gmail.com \
    --cc=hfranco@baylibre.com \
    --cc=inochiama@gmail.com \
    --cc=josh.milas@gmail.com \
    --cc=jpaulo.silvagoncalves@gmail.com \
    --cc=leo.liang@sifive.com \
    --cc=lukma@nabladev.com \
    --cc=michal.simek@amd.com \
    --cc=peng.fan@nxp.com \
    --cc=raymond.mao@riscstar.com \
    --cc=sumit.garg@kernel.org \
    --cc=tim609@andestech.com \
    --cc=trini@konsulko.com \
    --cc=u-boot@lists.u-boot-project.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.