Re: [PATCH 2/2] riscv: dts: starfive: add DT for Orange Pi RV

[Icenowy Zheng]

在 2025-04-10星期四的 16:54 +0000，Yao Zi写道：
> On Wed, Apr 09, 2025 at 05:18:01PM +0800, Icenowy Zheng wrote:
> > Orange Pi RV is a newly released SBC with JH7110 SoC, single GbE
> > port
> > (connected to JH7110 GMAC0 via a YT8531 PHY), 4 USB ports (via a
> > VL805
> > PCIe USB controller connected to JH7110 PCIE0), a M.2 M-key slot
> > (connected to JH7110 PCIE1), a HDMI video output, a 3.5mm audio
> > output
> > and a microSD slot.
> > 
> > Onboard peripherals contain a SPI NOR (which contains the U-Boot
> > firmware) and an Ampak AP6256 SDIO Wi-Fi module.
> > 
> > As the schematics isn't available yet, the SDIO Wi-Fi is left
> > disabled
> > yet.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@xxxxxxxxxx>
> > ---
> >  arch/riscv/boot/dts/starfive/Makefile         |  1 +
> >  .../boot/dts/starfive/jh7110-orangepi-rv.dts  | 73
> > +++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> >  create mode 100644 arch/riscv/boot/dts/starfive/jh7110-orangepi-
> > rv.dts
> > 
> > diff --git a/arch/riscv/boot/dts/starfive/Makefile
> > b/arch/riscv/boot/dts/starfive/Makefile
> > index b3bb12f78e7d5..24f1a44828350 100644
> > --- a/arch/riscv/boot/dts/starfive/Makefile
> > +++ b/arch/riscv/boot/dts/starfive/Makefile
> > @@ -10,6 +10,7 @@ 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-orangepi-rv.dtb
> >  dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-pine64-star64.dtb
> >  dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-
> > v1.2a.dtb
> >  dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-
> > v1.3b.dtb
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-orangepi-rv.dts
> > b/arch/riscv/boot/dts/starfive/jh7110-orangepi-rv.dts
> > new file mode 100644
> > index 0000000000000..bde01f117e0b2
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/starfive/jh7110-orangepi-rv.dts
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * Copyright (C) 2025 Icenowy Zheng <uwu@xxxxxxxxxx>
> > + */
> > +
> > +/dts-v1/;
> > +#include "jh7110-common.dtsi"
> > +#include <dt-bindings/leds/common.h>
> > +
> > +/ {
> > +       model = "Xunlong Orange Pi RV";
> > +       compatible = "xunlong,orangepi-rv", "starfive,jh7110";
> > +
> > +       leds {
> > +               compatible = "gpio-leds";
> > +
> > +               led-ack {
> > +                       gpios = <&aongpio 3 GPIO_ACTIVE_HIGH>;
> > +                       color = <LED_COLOR_ID_GREEN>;
> > +                       function = LED_FUNCTION_HEARTBEAT;
> > +                       linux,default-trigger = "heartbeat";
> > +                       label = "ack";
> 
> Should we sort the properties in alphabet order? i.e. color,
> function,
> gpios, label then linux,default-trigger. See dts-coding-style.rst,

Well in case of GPIO LED, I think gpios is something like reg? Although
this is only my personal feel, and label really needs to be reordered
then.

> 
> > The following order of properties in device nodes is preferred:
> > 
> > 1. "compatible"
> > 2. "reg"
> > 3. "ranges"
> > 4. Standard/common properties (defined by common bindings, e.g.
> > without
> > vendor-prefixes)
> > 5. Vendor-specific properties
> > 6. "status" (if applicable)
> > 7. Child nodes, where each node is preceded with a blank line
> 
> > +               };
> > +       };
> > +};
> > +
> > +&gmac0 {
> > +       starfive,tx-use-rgmii-clk;
> > +       assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
> > +       assigned-clock-parents = <&aoncrg
> > JH7110_AONCLK_GMAC0_RMII_RTX>;
> > +       status = "okay";
> 
> Vendor property starfive,tx-use-rgmii-clk should go after the common
> ones.

Okay, I will fix this (and the one below) in next revision.

Thanks,
Icenowy

> 
> > +};
> > +
> > +&i2c0 {
> > +       status = "okay";
> > +};
> > +
> > +&mmc0 {
> > +       /* TODO: Ampak AP6256 Wi-Fi module attached here */
> > +       status = "disabled";
> > +};
> > +
> > +&mmc1 {
> > +       /delete-property/ cd-gpios;
> > +       broken-cd;
> > +};
> > +
> > +&pcie0 {
> > +       status = "okay";
> > +};
> > +
> > +&pcie1 {
> > +       status = "okay";
> > +};
> > +
> > +&phy0 {
> > +       motorcomm,tx-clk-adj-enabled;
> > +       motorcomm,tx-clk-10-inverted;
> > +       motorcomm,tx-clk-100-inverted;
> > +       motorcomm,tx-clk-1000-inverted;
> > +       motorcomm,rx-clk-drv-microamp = <3970>;
> > +       motorcomm,rx-data-drv-microamp = <2910>;
> > +       rx-internal-delay-ps = <1500>;
> > +       tx-internal-delay-ps = <1500>;
> > +};
> 
> Ditto, move the vendor properties below the common ones.
> 
> > +&pwmdac {
> > +       status = "okay";
> > +};
> > +
> > +&spi0 {
> > +       status = "okay";
> > +};
> > -- 
> > 2.49.0
> > 
> 
> Best regards,
> Yao Zi