[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4963de6e-275b-e1d2-859c-7c7364b4599b@phytec.de>
Date: Tue, 8 Nov 2022 18:25:47 +0000
From: Wadim Egorov <W.Egorov@...tec.de>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "upstream@...tec.de" <upstream@...tec.de>, "nm@...com" <nm@...com>,
"vigneshr@...com" <vigneshr@...com>,
"kristo@...nel.org" <kristo@...nel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH v2 2/2] arm64: dts: ti: Add support for
phyBOARD-Electra-AM642
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@...tec.de>
>> ---
>> 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@...tec.com>
>> + *
>> + * Copyright (C) 2022 PHYTEC Messtechnik GmbH
>> + * Author: Wadim Egorov <w.egorov@...tec.de>
>> + *
>> + * 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@...00000 {
>> + device_type = "memory";
>> + reg = <0x00000000 0x80000000 0x00000000 0x80000000>;
>> + };
>> +
>> + reserved-memory {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + ranges;
>> +
>> + secure_ddr: optee@...00000 {
>> + 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
>
Powered by blists - more mailing lists