diff mbox series

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

Message ID 20240605083321.1211198-3-florian.vaussard@gmail.com (mailing list archive)
State New
Headers show
Series ARM: dts: socfpga: Add support for Terasic DE1-SOC board | expand

Commit Message

Florian Vaussard June 5, 2024, 8:33 a.m. UTC 
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@gmail.com>
---
 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

Comments

Krzysztof Kozlowski June 5, 2024, 2:33 p.m. UTC | #1 
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@gmail.com>
> ---
>  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@gmail.com>
> + */
> +
> +#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.

> +		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...

> +			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?

Best regards,
Krzysztof
Florian Vaussard June 6, 2024, 7:05 a.m. UTC | #2 
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@gmail.com>
>> ---
>>   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@gmail.com>
>> + */
>> +
>> +#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
Krzysztof Kozlowski June 6, 2024, 7:30 a.m. UTC | #3 
On 06/06/2024 09:05, Florian Vaussard wrote:
>>> +};
>>> +
>>> +&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.
> 

It's fine then.

Best regards,
Krzysztof
diff mbox series

Patch

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@gmail.com>
+ */
+
+#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";
+		stdout-path = "serial0:115200n8";
+	};
+
+	memory@0 {
+		name = "memory";
+		device_type = "memory";
+		reg = <0x0 0x40000000>; /* 1GB */
+	};
+
+	aliases {
+		/* this allow the ethaddr uboot environmnet variable contents
+		 * to be added to the gmac1 device tree blob.
+		 */
+		ethernet0 = &gmac1;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		hps_led {
+			label = "hps:green:led";
+			gpios = <&portb 24 0>;	/* HPS_GPIO53 */
+			linux,default-trigger = "heartbeat";
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+
+		hps_key {
+			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>;
+	status = "okay";
+};
+
+&usb1 {
+	status = "okay";
+};