Re: [PATCH v2 2/2] arm64: dts: ti: Add support for phyBOARD-Electra-AM642

[Wadim Egorov]

Am 08.11.22 um 19:01 schrieb Krzysztof Kozlowski:
> On 08/11/2022 18:50, Wadim Egorov wrote:
>> Add basic support for phyCORE-AM64x SoM & phyBOARD-Electra-AM642 CB.
>>
>> The phyCORE-AM64x [1] is a SoM (System on Module) featuring TI's AM64x SoC.
>> It can be used in combination with different carrier boards.
>> This module can come with different sizes and models for
>> DDR, eMMC, SPI NOR Flash and various SoCs from the AM64x family.
>>
>> A development Kit, called phyBOARD-Electra [2] is used as a carrier board
>> reference design around the AM64x SoM.
>>
>> Supported features:
>>    * Debug UART
>>    * Heartbeat LED
>>    * GPIO buttons & LEDs
>>    * SPI NOR flash
>>    * SPI TPM Chip
>>    * eMMC
>>    * CAN
>>    * Ethernet
>>    * Micro SD card
>>    * I2C EEPROM
>>    * I2C RTC
>>    * I2C LED Dimmer
>>    * USB
>>
>> For more details, see:
>>
>> [1] Product page SoM: https://www.phytec.com/product/phycore-am64x
>> [2] Product page CB: https://www.phytec.com/product/phyboard-am64x
>>
>> Signed-off-by: Wadim Egorov <w.egorov@xxxxxxxxx>
>> ---
>> v2:
>>    - Update commit message
>>    - Add links to product pages for SoM & CB
>>    - Drop "fixed" from regulator node names
>>    - Use generic node names for pmic, tpm, leds & keys
>>    - Update compatible of som.dtsi
>>    - Remove not needed node labels for pmic, tpm
>>    - Remove not needed R5 core mailbox & reserved memory definitions
>>      This is use case specific and should not be defined in the som.dtsi
>>    - Provide aliases for used devices by SoM & CB
>>    - Fixed led definition: Provide proper default-trigger, function & color
>>    - Renamed rtc label to i2c_som_rtc
>> ---
>>   arch/arm64/boot/dts/ti/Makefile               |   1 +
>>   .../boot/dts/ti/k3-am64-phycore-som.dtsi      | 286 ++++++++++++++++
>>   .../dts/ti/k3-am642-phyboard-electra-rdk.dts  | 321 ++++++++++++++++++
>>   3 files changed, 608 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/ti/k3-am64-phycore-som.dtsi
>>   create mode 100644 arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
>>
>> diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
>> index 4555a5be2257..79331f16cd4b 100644
>> --- a/arch/arm64/boot/dts/ti/Makefile
>> +++ b/arch/arm64/boot/dts/ti/Makefile
>> @@ -20,6 +20,7 @@ dtb-$(CONFIG_ARCH_K3) += k3-j7200-common-proc-board.dtb
>>   dtb-$(CONFIG_ARCH_K3) += k3-j721s2-common-proc-board.dtb
>>   
>>   dtb-$(CONFIG_ARCH_K3) += k3-am642-evm.dtb
>> +dtb-$(CONFIG_ARCH_K3) += k3-am642-phyboard-electra-rdk.dtb
>>   dtb-$(CONFIG_ARCH_K3) += k3-am642-sk.dtb
>>   
>>   dtb-$(CONFIG_ARCH_K3) += k3-am625-sk.dtb
>> diff --git a/arch/arm64/boot/dts/ti/k3-am64-phycore-som.dtsi b/arch/arm64/boot/dts/ti/k3-am64-phycore-som.dtsi
>> new file mode 100644
>> index 000000000000..86e6b87b826a
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/ti/k3-am64-phycore-som.dtsi
>> @@ -0,0 +1,286 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2021 PHYTEC America, LLC - https://www.phytec.com
>> + * Author: Matt McKee <mmckee@xxxxxxxxxx>
>> + *
>> + * Copyright (C) 2022 PHYTEC Messtechnik GmbH
>> + * Author: Wadim Egorov <w.egorov@xxxxxxxxx>
>> + *
>> + * Product homepage:
>> + * https://www.phytec.com/product/phycore-am64x
>> + */
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/leds/common.h>
>> +#include <dt-bindings/net/ti-dp83867.h>
>> +
>> +/ {
>> +	model = "PHYTEC phyCORE-AM64x";
>> +	compatible = "phytec,am64-phycore-som", "ti,am642";
>> +
>> +	aliases {
>> +		ethernet0 = &cpsw_port1;
>> +		mmc0 = &sdhci0;
>> +		rtc0 = &i2c_som_rtc;
>> +	};
>> +
>> +	memory@80000000 {
>> +		device_type = "memory";
>> +		reg = <0x00000000 0x80000000 0x00000000 0x80000000>;
>> +	};
>> +
>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		secure_ddr: optee@9e800000 {
>> +			reg = <0x00 0x9e800000 0x00 0x01800000>; /* for OP-TEE */
>> +			alignment = <0x1000>;
>> +			no-map;
>> +		};
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
>> +
>> +		led-0 {
>> +			pinctrl-names = "default";
>> +			pinctrl-0 = <&leds_pins_default>;
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			gpios = <&main_gpio0 12 GPIO_ACTIVE_HIGH>;
>> +			linux,default-trigger = "heartbeat";
>> +			function = LED_FUNCTION_HEARTBEAT;
>> +		};
>> +	};
>> +
>> +	vcc_5v0_som: vcc-5v0-som {
> I asked to drop "fixed", not other generic parts.
>
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Hey Krzysztof,

oh, I see I removed to much here. Will fix it.

...

>> +	leds {
>> +		compatible = "gpio-leds";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&user_leds_pins_default>;
>> +
>> +		led-1 {
>> +			color = <LED_COLOR_ID_RED>;
>> +			gpios = <&main_gpio0 15 GPIO_ACTIVE_HIGH>;
>> +			linux,default-trigger = "mmc0";
>> +			function = LED_FUNCTION_DISK;
>> +		};
>> +
>> +		led-2 {
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			gpios = <&main_gpio0 16 GPIO_ACTIVE_HIGH>;
>> +			linux,default-trigger = "mmc1";
>> +			function = LED_FUNCTION_DISK;
>> +		};
>> +	};
>> +
>> +	vcc_3v3_mmc: regulator-sd {
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.

But what is wrong with the "regulator-sd" node name?

Regards,
Wadim

>
> Thank you.
>
> Best regards,
> Krzysztof
>