| Message ID | 20241020134959.519462-4-guodong@riscstar.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DeepComputing FML13V01 board dts | expand |
On Sun, Oct 20, 2024 at 09:49:59PM +0800, Guodong Xu wrote: > From: Sandie Cao <sandie.cao@deepcomputing.io> > > The FML13V01 board from DeepComputing incorporates a StarFive JH7110 SoC. > It is a mainboard designed for the Framework Laptop 13 Chassis, which has > (Framework) SKU FRANHQ0001. > > The FML13V01 board features: > - StarFive JH7110 SoC > - LPDDR4 8GB > - eMMC 32GB or 128GB > - QSPI Flash > - MicroSD Slot > - PCIe-based Wi-Fi > - 4 USB-C Ports > - Port 1: PD 3.0 (60W Max), USB 3.2 Gen 1, DP 1.4 (4K@30Hz/2.5K@60Hz) > - Port 2: PD 3.0 (60W Max), USB 3.2 Gen 1 > - Port 3 & 4: USB 3.2 Gen 1 > > Create the DTS file for the DeepComputing FML13V01 board. Seven device > nodes have been verified functional and remain enabled: i2c2, i2c5, i2c6 > qspi, mmc0, mmc1 and usb0. All others remain disabled, or are disabled > by nodes in "jh7110-deepcomputing-fml13v01.dts". > > Signed-off-by: Sandie Cao <sandie.cao@deepcomputing.io> > [elder@riscstar.com: revised the description, updated some nodes] > Signed-off-by: Alex Elder <elder@riscstar.com> > Signed-off-by: Guodong Xu <guodong@riscstar.com> > --- > v5: No change > v4: Changed model string to "DeepComputing FML13V01" > Changed dts filename and Makefile accordingly to reflect the change > Updated device nodes status, and verified functional > Revised the commit message > v3: Updated the commit message > v2: Changed the model and copmatible strings > Updated the commit message with board features > > arch/riscv/boot/dts/starfive/Makefile | 1 + > .../jh7110-deepcomputing-fml13v01.dts | 44 +++++++++++++++++++ > 2 files changed, 45 insertions(+) > create mode 100644 arch/riscv/boot/dts/starfive/jh7110-deepcomputing-fml13v01.dts > > diff --git a/arch/riscv/boot/dts/starfive/Makefile b/arch/riscv/boot/dts/starfive/Makefile > index 7a163a7d6ba3..b3bb12f78e7d 100644 > --- a/arch/riscv/boot/dts/starfive/Makefile > +++ b/arch/riscv/boot/dts/starfive/Makefile > @@ -8,6 +8,7 @@ DTC_FLAGS_jh7110-starfive-visionfive-2-v1.3b := -@ > dtb-$(CONFIG_ARCH_STARFIVE) += jh7100-beaglev-starlight.dtb > dtb-$(CONFIG_ARCH_STARFIVE) += jh7100-starfive-visionfive-v1.dtb > > +dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-deepcomputing-fml13v01.dtb > dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-milkv-mars.dtb > dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-pine64-star64.dtb > dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-v1.2a.dtb > diff --git a/arch/riscv/boot/dts/starfive/jh7110-deepcomputing-fml13v01.dts b/arch/riscv/boot/dts/starfive/jh7110-deepcomputing-fml13v01.dts > new file mode 100644 > index 000000000000..b515b7d04c37 > --- /dev/null > +++ b/arch/riscv/boot/dts/starfive/jh7110-deepcomputing-fml13v01.dts > @@ -0,0 +1,44 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > +/* > + * Copyright (C) 2024 DeepComputing (HK) Limited > + */ > + > +/dts-v1/; > +#include "jh7110-common.dtsi" > + > +/ { > + model = "DeepComputing FML13V01"; > + compatible = "deepcomputing,fml13v01", "starfive,jh7110"; > +}; > + > +&camss { > + status = "disabled"; > +}; > + > +&csi2rx { > + status = "disabled"; > +}; > + > +&gmac0 { > + status = "disabled"; > +}; > + > +&i2c0 { > + status = "disabled"; > +}; > + > +&pwm { > + status = "disabled"; > +}; > + > +&pwmdac { > + status = "disabled"; > +}; > + > +&spi0 { > + status = "disabled"; If your board has to disable all these, then they should not have been enabled in DTSI in the first place. Only blocks present and working in the SoC (without amny external needs) should be enabled. I suggest to fix that aspect first. Best regards, Krzysztof
On Mon, Oct 21, 2024 at 09:17:59AM +0200, Krzysztof Kozlowski wrote: > On Sun, Oct 20, 2024 at 09:49:59PM +0800, Guodong Xu wrote: > > From: Sandie Cao <sandie.cao@deepcomputing.io> > > +&camss { > > + status = "disabled"; > > +}; > > + > > +&csi2rx { > > + status = "disabled"; > > +}; You can drop these two, I marked them disabled in the common file earlier this week. 1 > > + > > +&gmac0 { > > + status = "disabled"; > > +}; > > + > > +&i2c0 { > > + status = "disabled"; > > +}; > > + > > +&pwm { > > + status = "disabled"; > > +}; > > + > > +&pwmdac { > > + status = "disabled"; > > +}; > > + > > +&spi0 { > > + status = "disabled"; > > If your board has to disable all these, then they should not have been > enabled in DTSI in the first place. Only blocks present and working in > the SoC (without amny external needs) should be enabled. > > I suggest to fix that aspect first. Eh, I don't think I agree. Having 5 disables here is a lesser evil than reproducing 90% of jh7110-common.dtsi or shunting a bunch of stuff around. Emil?
On 21/10/2024 13:16, Conor Dooley wrote: > On Mon, Oct 21, 2024 at 09:17:59AM +0200, Krzysztof Kozlowski wrote: >> On Sun, Oct 20, 2024 at 09:49:59PM +0800, Guodong Xu wrote: >>> From: Sandie Cao <sandie.cao@deepcomputing.io> >>> +&camss { >>> + status = "disabled"; >>> +}; >>> + >>> +&csi2rx { >>> + status = "disabled"; >>> +}; > > You can drop these two, I marked them disabled in the common file > earlier this week. > 1 >>> + >>> +&gmac0 { >>> + status = "disabled"; >>> +}; >>> + >>> +&i2c0 { >>> + status = "disabled"; >>> +}; >>> + >>> +&pwm { >>> + status = "disabled"; >>> +}; >>> + >>> +&pwmdac { >>> + status = "disabled"; >>> +}; >>> + >>> +&spi0 { >>> + status = "disabled"; >> >> If your board has to disable all these, then they should not have been >> enabled in DTSI in the first place. Only blocks present and working in >> the SoC (without amny external needs) should be enabled. >> >> I suggest to fix that aspect first. > > Eh, I don't think I agree. Having 5 disables here is a lesser evil than > reproducing 90% of jh7110-common.dtsi or shunting a bunch of stuff > around. Emil? Why reproducing 90%? Only enable would be here, no? Or you want to say the common DTSI has things which do not exist? Best regards, Krzysztof
On 10/21/24 7:47 AM, Krzysztof Kozlowski wrote: > On 21/10/2024 13:16, Conor Dooley wrote: >> On Mon, Oct 21, 2024 at 09:17:59AM +0200, Krzysztof Kozlowski wrote: >>> On Sun, Oct 20, 2024 at 09:49:59PM +0800, Guodong Xu wrote: >>>> From: Sandie Cao <sandie.cao@deepcomputing.io> >>>> +&camss { >>>> + status = "disabled"; >>>> +}; >>>> + >>>> +&csi2rx { >>>> + status = "disabled"; >>>> +}; >> >> You can drop these two, I marked them disabled in the common file >> earlier this week. >> 1 >>>> + >>>> +&gmac0 { >>>> + status = "disabled"; >>>> +}; >>>> + >>>> +&i2c0 { >>>> + status = "disabled"; >>>> +}; >>>> + >>>> +&pwm { >>>> + status = "disabled"; >>>> +}; >>>> + >>>> +&pwmdac { >>>> + status = "disabled"; >>>> +}; >>>> + >>>> +&spi0 { >>>> + status = "disabled"; >>> >>> If your board has to disable all these, then they should not have been >>> enabled in DTSI in the first place. Only blocks present and working in >>> the SoC (without amny external needs) should be enabled. >>> >>> I suggest to fix that aspect first. >> >> Eh, I don't think I agree. Having 5 disables here is a lesser evil than >> reproducing 90% of jh7110-common.dtsi or shunting a bunch of stuff >> around. Emil? > > Why reproducing 90%? Only enable would be here, no? Or you want to say > the common DTSI has things which do not exist? For what it's worth, I agree with Krzysztof. In the (long) cover page we pointed this out, and offered to do it in a followup patch. But if requested we can do it now. So in v6, a new patch would be inserted before the other three, and it would: - Remove the status = "okay" lines for those nodes that are not enabled in this new platform, in "jh7110-common.dtsi" - Add nodes where appropriate in: jh7110-milkv-mars.dts jh7110-pine64-star64.dts jh7110-starfive-visionfive-2.dtsi They'll look like this, to enable the ones disabled above, e.g.: &gmac0 { status = "okay"; }; &i2c0 { status = "okay"; }; You guys should come to agreement, but I do think what Krzysztof says is the right approach. And unless convinced otherwise, this will be what shows up in the next version of this series. -Alex > Best regards, > Krzysztof >
On Mon, Oct 21, 2024 at 08:44:16AM -0500, Alex Elder wrote: > On 10/21/24 7:47 AM, Krzysztof Kozlowski wrote: > > On 21/10/2024 13:16, Conor Dooley wrote: > > > On Mon, Oct 21, 2024 at 09:17:59AM +0200, Krzysztof Kozlowski wrote: > > > > On Sun, Oct 20, 2024 at 09:49:59PM +0800, Guodong Xu wrote: > > > > > From: Sandie Cao <sandie.cao@deepcomputing.io> > > > > > +&camss { > > > > > + status = "disabled"; > > > > > +}; > > > > > + > > > > > +&csi2rx { > > > > > + status = "disabled"; > > > > > +}; > > > > > > You can drop these two, I marked them disabled in the common file > > > earlier this week. > > > 1 > > > > > + > > > > > +&gmac0 { > > > > > + status = "disabled"; > > > > > +}; > > > > > + > > > > > +&i2c0 { > > > > > + status = "disabled"; > > > > > +}; > > > > > + > > > > > +&pwm { > > > > > + status = "disabled"; > > > > > +}; > > > > > + > > > > > +&pwmdac { > > > > > + status = "disabled"; > > > > > +}; > > > > > + > > > > > +&spi0 { > > > > > + status = "disabled"; > > > > > > > > If your board has to disable all these, then they should not have been > > > > enabled in DTSI in the first place. Only blocks present and working in > > > > the SoC (without amny external needs) should be enabled. > > > > > > > > I suggest to fix that aspect first. > > > > > > Eh, I don't think I agree. Having 5 disables here is a lesser evil than > > > reproducing 90% of jh7110-common.dtsi or shunting a bunch of stuff > > > around. Emil? > > > > Why reproducing 90%? Only enable would be here, no? Or you want to say > > the common DTSI has things which do not exist? > > For what it's worth, I agree with Krzysztof. In the (long) cover > page we pointed this out, and offered to do it in a followup patch. > But if requested we can do it now. > > So in v6, a new patch would be inserted before the other three, > and it would: > - Remove the status = "okay" lines for those nodes that are not enabled > in this new platform, in "jh7110-common.dtsi" > - Add nodes where appropriate in: > jh7110-milkv-mars.dts > jh7110-pine64-star64.dts > jh7110-starfive-visionfive-2.dtsi > They'll look like this, to enable the ones disabled above, e.g.: > &gmac0 { > status = "okay"; > }; > > &i2c0 { > status = "okay"; > }; > > You guys should come to agreement, but I do think what Krzysztof says > is the right approach. And unless convinced otherwise, this will be > what shows up in the next version of this series. Ultimately, it is up to Emil how he wants these laid out.
Conor Dooley wrote: > On Mon, Oct 21, 2024 at 08:44:16AM -0500, Alex Elder wrote: > > On 10/21/24 7:47 AM, Krzysztof Kozlowski wrote: > > > On 21/10/2024 13:16, Conor Dooley wrote: > > > > On Mon, Oct 21, 2024 at 09:17:59AM +0200, Krzysztof Kozlowski wrote: > > > > > On Sun, Oct 20, 2024 at 09:49:59PM +0800, Guodong Xu wrote: > > > > > > From: Sandie Cao <sandie.cao@deepcomputing.io> > > > > > > +&camss { > > > > > > + status = "disabled"; > > > > > > +}; > > > > > > + > > > > > > +&csi2rx { > > > > > > + status = "disabled"; > > > > > > +}; > > > > > > > > You can drop these two, I marked them disabled in the common file > > > > earlier this week. > > > > 1 > > > > > > + > > > > > > +&gmac0 { > > > > > > + status = "disabled"; > > > > > > +}; > > > > > > + > > > > > > +&i2c0 { > > > > > > + status = "disabled"; > > > > > > +}; > > > > > > + > > > > > > +&pwm { > > > > > > + status = "disabled"; > > > > > > +}; > > > > > > + > > > > > > +&pwmdac { > > > > > > + status = "disabled"; > > > > > > +}; > > > > > > + > > > > > > +&spi0 { > > > > > > + status = "disabled"; > > > > > > > > > > If your board has to disable all these, then they should not have been > > > > > enabled in DTSI in the first place. Only blocks present and working in > > > > > the SoC (without amny external needs) should be enabled. > > > > > > > > > > I suggest to fix that aspect first. > > > > > > > > Eh, I don't think I agree. Having 5 disables here is a lesser evil than > > > > reproducing 90% of jh7110-common.dtsi or shunting a bunch of stuff > > > > around. Emil? > > > > > > Why reproducing 90%? Only enable would be here, no? Or you want to say > > > the common DTSI has things which do not exist? > > > > For what it's worth, I agree with Krzysztof. In the (long) cover > > page we pointed this out, and offered to do it in a followup patch. > > But if requested we can do it now. > > > > So in v6, a new patch would be inserted before the other three, > > and it would: > > - Remove the status = "okay" lines for those nodes that are not enabled > > in this new platform, in "jh7110-common.dtsi" > > - Add nodes where appropriate in: > > jh7110-milkv-mars.dts > > jh7110-pine64-star64.dts > > jh7110-starfive-visionfive-2.dtsi > > They'll look like this, to enable the ones disabled above, e.g.: > > &gmac0 { > > status = "okay"; > > }; > > > > &i2c0 { > > status = "okay"; > > }; > > > > You guys should come to agreement, but I do think what Krzysztof says > > is the right approach. And unless convinced otherwise, this will be > > what shows up in the next version of this series. > > Ultimately, it is up to Emil how he wants these laid out. Thanks, but I agree. Please begin with a patch moving the nodes that are no longer common out of jh7110-common.dtsi and into the vf2, mars and pine64 consumers. You should probably do the same with the &usb0 node instead of overriding the dr_mode property. /Emil
On 10/23/24 11:49 AM, Emil Renner Berthing wrote: >> Ultimately, it is up to Emil how he wants these laid out. > Thanks, but I agree. Please begin with a patch moving the nodes that are no > longer common out of jh7110-common.dtsi and into the vf2, mars and pine64 > consumers. You should probably do the same with the &usb0 node instead > of overriding the dr_mode property. > > /Emil Thenk you Emil, we will prepare the next version of the series to do these things. -Alex
diff --git a/arch/riscv/boot/dts/starfive/Makefile b/arch/riscv/boot/dts/starfive/Makefile index 7a163a7d6ba3..b3bb12f78e7d 100644 --- a/arch/riscv/boot/dts/starfive/Makefile +++ b/arch/riscv/boot/dts/starfive/Makefile @@ -8,6 +8,7 @@ DTC_FLAGS_jh7110-starfive-visionfive-2-v1.3b := -@ dtb-$(CONFIG_ARCH_STARFIVE) += jh7100-beaglev-starlight.dtb dtb-$(CONFIG_ARCH_STARFIVE) += jh7100-starfive-visionfive-v1.dtb +dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-deepcomputing-fml13v01.dtb dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-milkv-mars.dtb dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-pine64-star64.dtb dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-v1.2a.dtb diff --git a/arch/riscv/boot/dts/starfive/jh7110-deepcomputing-fml13v01.dts b/arch/riscv/boot/dts/starfive/jh7110-deepcomputing-fml13v01.dts new file mode 100644 index 000000000000..b515b7d04c37 --- /dev/null +++ b/arch/riscv/boot/dts/starfive/jh7110-deepcomputing-fml13v01.dts @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/* + * Copyright (C) 2024 DeepComputing (HK) Limited + */ + +/dts-v1/; +#include "jh7110-common.dtsi" + +/ { + model = "DeepComputing FML13V01"; + compatible = "deepcomputing,fml13v01", "starfive,jh7110"; +}; + +&camss { + status = "disabled"; +}; + +&csi2rx { + status = "disabled"; +}; + +&gmac0 { + status = "disabled"; +}; + +&i2c0 { + status = "disabled"; +}; + +&pwm { + status = "disabled"; +}; + +&pwmdac { + status = "disabled"; +}; + +&spi0 { + status = "disabled"; +}; + +&usb0 { + dr_mode = "host"; +};