Re: [PATCH v4 39/39] ARM: dts: at91: sam9x75_curiosity: add sam9x75 curiosity board

[<Varshini.Rajendran@microchip.com>]

Hi Claudiu,

On 03/03/24 5:49 pm, claudiu beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> s/ARM: dts: at91/ARM: dts: microchip
> 
> in title.
> 
> On 23.02.2024 19:31, Varshini Rajendran wrote:
>> Add device tree file for sam9x75 curiosity board.
>>
>> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com>
>> ---
>> Changes in v4:
>> - Removed full node paths
>> - Renamed Leds with color names
>> - Corrected regulator node names
>> - Added support for classd and i2s nodes and their corresponding
>>    pinctrl nodes
>> - Dropped USB nodes owing to the discussion here
>> https://lore.kernel.org/linux-devicetree/CAL_JsqJ9PrX6fj-EbffeJce09MXs=B7t+KS_kOinxaRx38=WxA@mail.gmail.com/
>> (Explained elaborately in the cover letter)
>> - Updated the linux,code property with the necessary value
>> ---
>>   arch/arm/boot/dts/microchip/Makefile          |   3 +
>>   .../dts/microchip/at91-sam9x75_curiosity.dts  | 309 ++++++++++++++++++
>>   2 files changed, 312 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts
>>
>> diff --git a/arch/arm/boot/dts/microchip/Makefile b/arch/arm/boot/dts/microchip/Makefile
>> index efde9546c8f4..5b3d518da319 100644
>> --- a/arch/arm/boot/dts/microchip/Makefile
>> +++ b/arch/arm/boot/dts/microchip/Makefile
>> @@ -12,6 +12,7 @@ DTC_FLAGS_at91-sama5d3_eds := -@
>>   DTC_FLAGS_at91-sama5d3_xplained := -@
>>   DTC_FLAGS_at91-sama5d4_xplained := -@
>>   DTC_FLAGS_at91-sama7g5ek := -@
>> +DTC_FLAGS_at91-sam9x75_curiosity := -@
> 
> Keep it alphanumerically sorted, thus after sam9x60 entry.
> 
>>   dtb-$(CONFIG_SOC_AT91RM9200) += \
>>        at91rm9200ek.dtb \
>>        mpa1600.dtb
>> @@ -59,6 +60,8 @@ dtb-$(CONFIG_SOC_AT91SAM9) += \
>>   dtb-$(CONFIG_SOC_SAM9X60) += \
>>        at91-sam9x60_curiosity.dtb \
>>        at91-sam9x60ek.dtb
>> +dtb-$(CONFIG_SOC_SAM9X7) += \
>> +     at91-sam9x75_curiosity.dtb
>>   dtb-$(CONFIG_SOC_SAM_V7) += \
>>        at91-kizbox2-2.dtb \
>>        at91-kizbox3-hs.dtb \
>> diff --git a/arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts b/arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts
>> new file mode 100644
>> index 000000000000..be37022d3d05
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/microchip/at91-sam9x75_curiosity.dts
>> @@ -0,0 +1,309 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * at91-sam9x75_curiosity.dts - Device Tree file for Microchip SAM9X75 Curiosity board
>> + *
>> + * Copyright (C) 2023 Microchip Technology Inc. and its subsidiaries
>> + *
>> + * Author: Varshini Rajendran <varshini.rajendran@microchip.com>
>> + */
>> +/dts-v1/;
>> +#include "sam9x7.dtsi"
>> +#include <dt-bindings/input/input.h>
>> +
>> +/ {
>> +     model = "Microchip SAM9X75 Curiosity";
>> +     compatible = "microchip,sam9x75-curiosity", "microchip,sam9x7", "atmel,at91sam9";
>> +
>> +     aliases {
>> +             i2c0 = &i2c6;
>> +     };
>> +
>> +     chosen {
>> +             stdout-path = "serial0:115200n8";
>> +     };
>> +
>> +     gpio-keys {
>> +             compatible = "gpio-keys";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&pinctrl_key_gpio_default>;
>> +
>> +             button-user {
>> +                     label = "USER";
>> +                     gpios = <&pioC 9 GPIO_ACTIVE_LOW>;
>> +                     linux,code = <KEY_0>;
>> +                     wakeup-source;
>> +             };
>> +     };
>> +
>> +     leds {
>> +             compatible = "gpio-leds";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&pinctrl_led_gpio_default>;
>> +
>> +             led-red {
>> +                     label = "red";
>> +                     gpios = <&pioC 19 GPIO_ACTIVE_HIGH>;
>> +             };
>> +
>> +             led-green {
>> +                     label = "green";
>> +                     gpios = <&pioC 21 GPIO_ACTIVE_HIGH>;
>> +             };
>> +
>> +             led-blue {
>> +                     label = "blue";
>> +                     gpios = <&pioC 20 GPIO_ACTIVE_HIGH>;
>> +                     linux,default-trigger = "heartbeat";
>> +             };
>> +     };
>> +
>> +     memory@20000000 {
>> +             device_type = "memory";
>> +             reg = <0x20000000 0x10000000>;
>> +     };
>> +};
>> +
>> +&classd {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&pinctrl_classd>;
>> +     atmel,pwm-type = "diff";
>> +     atmel,non-overlap-time = <10>;
>> +     status = "okay";
>> +};
>> +
>> +&dbgu {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&pinctrl_dbgu>;
>> +     status = "okay";
>> +};
>> +
>> +&dma0 {
>> +     status = "okay";
>> +};
>> +
>> +&flx6 {
>> +     atmel,flexcom-mode = <ATMEL_FLEXCOM_MODE_TWI>;
>> +     status = "okay";
>> +};
>> +
>> +&i2c6 {
> 
> I don't know if you got any review comments w/ regards to this in the
> previous email but having flexcoms and inner node grouped together is
> easier to follow (at least to me). e.g.:
> 
Yes, I received a comment not to duplicate nodes. 
https://patchwork.kernel.org/project/linux-mmc/patch/20230623203056.689705-46-varshini.rajendran@microchip.com/

>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&pinctrl_flx6_default>;
>> +     i2c-analog-filter;
>> +     i2c-digital-filter;
>> +     i2c-digital-filter-width-ns = <35>;
>> +     status = "okay";
>> +
>> +     pmic@5b {
>> +             compatible = "microchip,mcp16502";
>> +             reg = <0x5b>;
>> +
>> +             regulators {
>> +                     vdd_3v3: VDD_IO {
>> +                             regulator-name = "VDD_IO";
>> +                             regulator-min-microvolt = <3000000>;
>> +                             regulator-max-microvolt = <3600000>;
> 
> I can't find the schematics for this but these values here should reflect
> the voltage that the board support not the ones that the PMIC itself
> supports. Valid for all the other regulators.
> 
Okay. I will double check this with hardware experts as well.
> 
>> +                             regulator-initial-mode = <2>;
>> +                             regulator-allowed-modes = <2>, <4>;
>> +                             regulator-always-on;
>> +
>> +                             regulator-state-standby {
>> +                                     regulator-on-in-suspend;
>> +                                     regulator-mode = <4>;
>> +                             };
>> +
>> +                             regulator-state-mem {
>> +                                     regulator-mode = <4>;
>> +                             };
>> +                     };
>> +
>> +                     vddioddr: VDD_DDR {
>> +                             regulator-name = "VDD_DDR";
>> +                             regulator-min-microvolt = <1283000>;
>> +                             regulator-max-microvolt = <1450000>;
>> +                             regulator-initial-mode = <2>;
>> +                             regulator-allowed-modes = <2>, <4>;
>> +                             regulator-always-on;
>> +
>> +                             regulator-state-standby {
>> +                                     regulator-on-in-suspend;
>> +                                     regulator-mode = <4>;
>> +                             };
>> +
>> +                             regulator-state-mem {
>> +                                     regulator-on-in-suspend;
>> +                                     regulator-mode = <4>;
>> +                             };
>> +                     };
>> +
>> +                     vddcore: VDD_CORE {
>> +                             regulator-name = "VDD_CORE";
>> +                             regulator-min-microvolt = <500000>;
>> +                             regulator-max-microvolt = <1210000>;
>> +                             regulator-initial-mode = <2>;
>> +                             regulator-allowed-modes = <2>, <4>;
>> +                             regulator-always-on;
>> +
>> +                             regulator-state-standby {
>> +                                     regulator-on-in-suspend;
>> +                                     regulator-mode = <4>;
>> +                             };
>> +
>> +                             regulator-state-mem {
>> +                                     regulator-mode = <4>;
>> +                             };
>> +                     };
>> +
>> +                     vddcpu: VDD_OTHER {
>> +                             regulator-name = "VDD_OTHER";
>> +                             regulator-min-microvolt = <1700000>;
>> +                             regulator-max-microvolt = <3600000>;
>> +                             regulator-initial-mode = <2>;
>> +                             regulator-allowed-modes = <2>, <4>;
>> +                             regulator-ramp-delay = <3125>;
>> +                             regulator-always-on;
>> +
>> +                             regulator-state-standby {
>> +                                     regulator-on-in-suspend;
>> +                                     regulator-mode = <4>;
>> +                             };
>> +
>> +                             regulator-state-mem {
>> +                                     regulator-mode = <4>;
>> +                             };
>> +                     };
>> +
>> +                     vldo1: LDO1 {
>> +                             regulator-name = "LDO1";
>> +                             regulator-min-microvolt = <1200000>;
>> +                             regulator-max-microvolt = <3700000>;
>> +                             regulator-always-on;
>> +
>> +                             regulator-state-standby {
>> +                                     regulator-on-in-suspend;
>> +                             };
>> +                     };
>> +
>> +                     vldo2: LDO2 {
>> +                             regulator-name = "LDO2";
>> +                             regulator-min-microvolt = <1200000>;
>> +                             regulator-max-microvolt = <3700000>;
>> +
>> +                             regulator-state-standby {
>> +                                     regulator-on-in-suspend;
>> +                             };
>> +                     };
>> +             };
>> +     };
>> +};
>> +
>> +&i2s {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&pinctrl_i2s_default>;
>> +     #sound-dai-cells = <0>;
>> +     status = "disabled";
> 
> Any reason this is disabled?
There is a conflict. My bad, I missed mentioning it since the other node 
support is not added yet. I will add it in the next version.
> 
>> +};
>> +
>> +&main_xtal {
>> +     clock-frequency = <24000000>;
>> +};
>> +
>> +&pinctrl {
>> +
> 
> This line could be removed.
> 
>> +     classd {
>> +             pinctrl_classd: classd {
>> +                     atmel,pins =
>> +                             <AT91_PIOA 18 AT91_PERIPH_C AT91_PINCTRL_PULL_UP
> 
> Try to be compliant with coding style from here (valid everywhere):
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/dts-coding-style.rst#n167
> 
>> +                              AT91_PIOA 19 AT91_PERIPH_C AT91_PINCTRL_PULL_DOWN>;
>> +             };
>> +     };
>> +
>> +     dbgu {
>> +             pinctrl_dbgu: dbgu-0 {
> 
> usually pinctrl label is something like the following in Microchip AT91 DTSes:
> pinctrl_<ip-name>_default and node name is <ip-name>-default.
> 
> Please use the same rule everywhere.
> 
>> +                     atmel,pins = <AT91_PIOA 26 AT91_PERIPH_A AT91_PINCTRL_PULL_UP
>> +                                   AT91_PIOA 27 AT91_PERIPH_A AT91_PINCTRL_NONE>;
>> +             };
>> +     };
>> +
>> +     flexcom {
>> +             pinctrl_flx6_default: flx6-twi {
>> +                     atmel,pins =
>> +                             <AT91_PIOA 24 AT91_PERIPH_A AT91_PINCTRL_PULL_UP
>> +                              AT91_PIOA 25 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
>> +             };
>> +     };
>> +
>> +     gpio-keys {
>> +             pinctrl_key_gpio_default: key-gpio-default {
>> +                     atmel,pins = <AT91_PIOC 9 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>;
>> +             };
>> +     };
>> +
>> +     i2s {
>> +             pinctrl_i2s_default: i2s {
>> +                     atmel,pins =
>> +                             <AT91_PIOB 26 AT91_PERIPH_D AT91_PINCTRL_NONE           /* I2SCK */
>> +                              AT91_PIOB 15 AT91_PERIPH_D AT91_PINCTRL_NONE           /* I2SWS */
>> +                              AT91_PIOB 16 AT91_PERIPH_D AT91_PINCTRL_NONE           /* I2SDIN */
>> +                              AT91_PIOB 17 AT91_PERIPH_D AT91_PINCTRL_NONE           /* I2SDOUT */
>> +                              AT91_PIOB 25 AT91_PERIPH_D AT91_PINCTRL_NONE>;         /* I2SMCK */
>> +             };
>> +     };
>> +
>> +     leds {
>> +             pinctrl_led_gpio_default: led-gpio-default {
>> +                     atmel,pins = <AT91_PIOC 19 AT91_PERIPH_GPIO AT91_PINCTRL_NONE
>> +                                   AT91_PIOC 21 AT91_PERIPH_GPIO AT91_PINCTRL_NONE
>> +                                   AT91_PIOC 20 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>;
>> +             };
>> +     };
>> +
>> +     sdmmc0 {
>> +             pinctrl_sdmmc0_default: sdmmc0 {
>> +                     atmel,pins =
>> +                             <AT91_PIOA 2 AT91_PERIPH_A (AT91_PINCTRL_DRIVE_STRENGTH_HI | AT91_PINCTRL_SLEWRATE_DIS)                                 /* PA2 CK  periph A with pullup */
>> +                              AT91_PIOA 1 AT91_PERIPH_A (AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DRIVE_STRENGTH_HI | AT91_PINCTRL_SLEWRATE_DIS)          /* PA1 CMD periph A with pullup */
>> +                              AT91_PIOA 0 AT91_PERIPH_A (AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DRIVE_STRENGTH_HI | AT91_PINCTRL_SLEWRATE_DIS)          /* PA0 DAT0 periph A */
>> +                              AT91_PIOA 3 AT91_PERIPH_A (AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DRIVE_STRENGTH_HI | AT91_PINCTRL_SLEWRATE_DIS)          /* PA3 DAT1 periph A with pullup */
>> +                              AT91_PIOA 4 AT91_PERIPH_A (AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DRIVE_STRENGTH_HI | AT91_PINCTRL_SLEWRATE_DIS)          /* PA4 DAT2 periph A with pullup */
>> +                              AT91_PIOA 5 AT91_PERIPH_A (AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DRIVE_STRENGTH_HI | AT91_PINCTRL_SLEWRATE_DIS)>;        /* PA5 DAT3 periph A with pullup */
>> +             };
>> +     };
>> +
> 
> You can remove this line
> 
>> +}; /* pinctrl */
>> +
>> +&rtt {
>> +     atmel,rtt-rtc-time-reg = <&gpbr 0x0>;
>> +};
>> +
>> +&sdmmc0 {
>> +     bus-width = <4>;
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&pinctrl_sdmmc0_default>;
>> +     cd-gpios = <&pioA 23 GPIO_ACTIVE_LOW>;
>> +     disable-wp;
>> +     status = "okay";
>> +};
>> +
>> +&slow_xtal {
>> +     clock-frequency = <32768>;
>> +};
>> +
>> +&power_management {
>> +     debounce-delay-us = <976>;
>> +     status = "okay";
>> +
>> +     input@0 {
>> +             reg = <0>;
>> +     };
>> +};
>> +
>> +&trng {
>> +     status = "okay";
>> +};
>> +
>> +&watchdog {
>> +     status = "okay";
>> +};

-- 
Thanks and Regards,
Varshini Rajendran.