Re: [PATCH 2/2] ARM: dts: socfpga: Add support for Terasic DE1-SOC board

[Florian Vaussard <florian.vaussard@xxxxxxxxx>]

Hello,

Le 05.06.24 à 16:33, Krzysztof Kozlowski a écrit :
> On 05/06/2024 10:33, Florian Vaussard wrote:
> > Compared to Terasic SoCKit, here are some of the notable differences
> > on the HPS side:
> > - Only 1 user LED and 1 user KEY
> > - The QSPI Flash is not populated
> > - The ADXL345 accelerometer is on I2C0 instead of I2C1
> >
> > Tested to be working:
> > - LED / KEY
> > - Ethernet
> > - Both USB Host ports
> > - SD card
> > - ADXL345 accelerometer
> >
> > Signed-off-by: Florian Vaussard <florian.vaussard@xxxxxxxxx>
> > ---
> >   arch/arm/boot/dts/intel/socfpga/Makefile      |   1 +
> >   .../socfpga/socfpga_cyclone5_de1_soc.dts      | 106 ++++++++++++++++++
> >   2 files changed, 107 insertions(+)
> >   create mode 100644 arch/arm/boot/dts/intel/socfpga/socfpga_cyclone5_de1_soc.dts
> >
> > diff --git a/arch/arm/boot/dts/intel/socfpga/Makefile b/arch/arm/boot/dts/intel/socfpga/Makefile
> > index c467828aeb4b..1d5140b238da 100644
> > --- a/arch/arm/boot/dts/intel/socfpga/Makefile
> > +++ b/arch/arm/boot/dts/intel/socfpga/Makefile
> > @@ -10,6 +10,7 @@ dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
> >   	socfpga_cyclone5_mcvevk.dtb \
> >   	socfpga_cyclone5_socdk.dtb \
> >   	socfpga_cyclone5_de0_nano_soc.dtb \
> > +	socfpga_cyclone5_de1_soc.dtb \
> >   	socfpga_cyclone5_sockit.dtb \
> >   	socfpga_cyclone5_socrates.dtb \
> >   	socfpga_cyclone5_sodia.dtb \
> > diff --git a/arch/arm/boot/dts/intel/socfpga/socfpga_cyclone5_de1_soc.dts b/arch/arm/boot/dts/intel/socfpga/socfpga_cyclone5_de1_soc.dts
> > new file mode 100644
> > index 000000000000..7d811be5f5a7
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/intel/socfpga/socfpga_cyclone5_de1_soc.dts
> > @@ -0,0 +1,106 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2024 Florian Vaussard <florian.vaussard@xxxxxxxxx>
> > + */
> > +
> > +#include "socfpga_cyclone5.dtsi"
> > +
> > +#include <dt-bindings/input/input.h>
> > +
> > +/ {
> > +	model = "Terasic DE1-SOC";
> > +	compatible = "terasic,de1-soc", "altr,socfpga-cyclone5", "altr,socfpga";
> > +
> > +	chosen {
> > +		bootargs = "earlyprintk";
>
> That's debugging, not mainline. Drop bootargs.

OK

This was copy/pasted from socfpga_cyclone5_sockit.dts, like most of this file.
I found 9 other occurrences in socfpga DTS. It looks like a follow-up clean-up
would be needed.

> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	memory@0 {
> > +		name = "memory";
>
> Which binding defines this property?
>
> > +		device_type = "memory";
> > +		reg = <0x0 0x40000000>; /* 1GB */
> > +	};
> > +
> > +	aliases {
> > +		/* this allow the ethaddr uboot environmnet variable contents
>
> Please use Linux coding style comments /* in separate line. Also, typos.
>
> > +		 * to be added to the gmac1 device tree blob.
> > +		 */
> > +		ethernet0 = &gmac1;
> > +	};
> > +
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> > +		hps_led {
>
> No underscores in node names.
>
> > +			label = "hps:green:led";
>
> Drop. Use function and color instead.
>
> > +			gpios = <&portb 24 0>;	/* HPS_GPIO53 */
> > +			linux,default-trigger = "heartbeat";
> > +		};
> > +	};
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +
> > +		hps_key {
>
> No underscores...

Sorry, copy/pasted from socfpga_cyclone5_sockit.dts without double-checking.

> > +			label = "hps_key";
> > +			gpios = <&portb 25 0>;	/* HPS_GPIO54 */
> > +			linux,code = <BTN_0>;
> > +		};
> > +	};
> > +
> > +	regulator_3_3v: regulator {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "VCC3P3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +	};
> > +};
> > +
> > +&gmac1 {
> > +	status = "okay";
> > +	phy-mode = "rgmii";
> > +
> > +	rxd0-skew-ps = <0>;
> > +	rxd1-skew-ps = <0>;
> > +	rxd2-skew-ps = <0>;
> > +	rxd3-skew-ps = <0>;
> > +	txen-skew-ps = <0>;
> > +	txc-skew-ps = <2600>;
> > +	rxdv-skew-ps = <0>;
> > +	rxc-skew-ps = <2000>;
> > +};
> > +
> > +&gpio0 {	/* GPIO 0..29 */
> > +	status = "okay";
> > +};
> > +
> > +&gpio1 {	/* GPIO 30..57 */
> > +	status = "okay";
> > +};
> > +
> > +&gpio2 {	/* GPIO 58..66 (HLGPI 0..13 at offset 13) */
> > +	status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +	status = "okay";
> > +
> > +	accel1: accelerometer@53 {
> > +		compatible = "adi,adxl345";
> > +		reg = <0x53>;
> > +
> > +		interrupt-parent = <&portc>;
> > +		interrupts = <3 2>;
> > +	};
> > +};
> > +
> > +&mmc0 {
> > +	vmmc-supply = <&regulator_3_3v>;
> > +	vqmmc-supply = <&regulator_3_3v>;
>
> That's a noop... Isn't this coming from a PMIC?

No PMIC. VCC3P3_SD is derived from VCC3P3 using a passive circuit, so the
voltage is fixed and always on. I am fine to drop this regulator if you prefer.

Thanks for your review.

Best regards,
Florian