Re: [External] Re: [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 device tree

>> >> Add initial device tree support for the Lenovo ThinkEdge SE70, an
>> >> industrial edge gateway based on the NVIDIA Jetson Xavier NX module
>> >> (P3668-0001) with a custom carrier board.
>> >>
>> >> This initial submission includes:
>> >> - 40-pin expansion header pinmux configuration
>> >> - External Micro SD card slot with dedicated 3.3V regulator
>> >> - I2C bus for 40-pin header
>> >>
>> >> Static verification passed: dt_binding_check and dtbs compilation
>> >> complete without errors.
>> >>
>> >> Signed-off-by: Jiqi Li <[email protected]>
>> >> ---
>> >> Changes in v2:
>> >> - Remove unused input-related headers from baseboard DTSI
>> >>
>> >> Changes in v3:
>> >> - Merge baseboard content into main DTS, drop separate baseboard DTSI
>> >> - Use stdout-path instead of bootargs in chosen node
>> >> ---
>> >>  arch/arm64/boot/dts/nvidia/Makefile           |   1 +
>> >>  .../nvidia/tegra194-lenovo-thinkedge-se70.dts | 113 ++++++++++++++++++
>> >>  2 files changed, 114 insertions(+)
>> >>  create mode 100644 arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts
>> >>
>> >> diff --git a/arch/arm64/boot/dts/nvidia/Makefile b/arch/arm64/boot/dts/nvidia/Makefile
>> >> index 72c0cb5efa47..736a3f8a923f 100644
>> >> --- a/arch/arm64/boot/dts/nvidia/Makefile
>> >> +++ b/arch/arm64/boot/dts/nvidia/Makefile
>> >> @@ -29,6 +29,7 @@ dtb-$(CONFIG_ARCH_TEGRA_186_SOC) += tegra186-p3509-0000+p3636-0001.dtb
>> >>  dtb-$(CONFIG_ARCH_TEGRA_194_SOC) += tegra194-p2972-0000.dtb
>> >>  dtb-$(CONFIG_ARCH_TEGRA_194_SOC) += tegra194-p3509-0000+p3668-0000.dtb
>> >>  dtb-$(CONFIG_ARCH_TEGRA_194_SOC) += tegra194-p3509-0000+p3668-0001.dtb
>> >> +dtb-$(CONFIG_ARCH_TEGRA_194_SOC) += tegra194-lenovo-thinkedge-se70.dtb
>> >>  dtb-$(CONFIG_ARCH_TEGRA_234_SOC) += tegra234-sim-vdk.dtb
>> >>  dtb-$(CONFIG_ARCH_TEGRA_234_SOC) += tegra234-p3737-0000+p3701-0000.dtb
>> >>  dtb-$(CONFIG_ARCH_TEGRA_234_SOC) += tegra234-p3737-0000+p3701-0008.dtb
>> >> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts b/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts
>> >> new file mode 100644
>> >> index 000000000000..530d5bcaeba7
>> >> --- /dev/null
>> >> +++ b/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts
>> >> @@ -0,0 +1,113 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +/dts-v1/;
>> >> +
>> >> +#include "tegra194-p3668-0001.dtsi"
>> >> +
>> >> +#include <dt-bindings/gpio/tegra194-gpio.h>
>> >> +
>> >> +/ {
>> >> +     model = "Lenovo ThinkEdge SE70";
>> >> +     compatible = "lenovo,thinkedge-se70", "nvidia,p3668-0001", "nvidia,tegra194";
>> >> +
>> >> +     chosen {
>> >> +             stdout-path = "serial0:115200n8";
>> >> +     };
>> >> +
>> >> +     vdd_5v0_sys: regulator-vdd-5v0-sys {
>> >> +             compatible = "regulator-fixed";
>> >> +             regulator-name = "VDD_5V_SYS";
>> >> +             regulator-min-microvolt = <5000000>;
>> >> +             regulator-max-microvolt = <5000000>;
>> >> +             regulator-always-on;
>> >> +             regulator-boot-on;
>> >> +     };
>> >
>> >Nothing currently references this regulator. Should e.g. the SD
>> >regulator below supply from it, or should it be removed?
>> >
>> >> +
>> >> +     /* Fixed 3.3V power supply regulator for external Micro SD slot */
>> >> +     vdd_3v3_sd: regulator-sd-3v3 {
>> >
>> >Should be called regulator-vdd-3v3-sd.
>> >
>> >Also, please move the regulator nodes to the bottom of the file
>> >matching other device trees (alphabetical order).
>> >
>> >Thank you!
>> >Mikko
>> >
>>
>> Hi Mikko,
>>
>> Thank you for the detailed review of the regulator nodes.
>>
>> I have addressed your comments for the upcoming v4 as follows:
>>
>> 1. Regulator naming and placement:
>>    - I will rename `vdd_3v3_sd: regulator-sd-3v3` to
>>      `regulator-vdd-3v3-sd` to follow the naming convention.
>>    - I will move both regulator nodes to the bottom of the file and
>>      order them alphabetically (regulator-vdd-3v3-sd then
>>      regulator-vdd-5v0-sys).
>>
>> 2. Regarding `vdd_5v0_sys`:
>>    - After removing it, I encountered compilation errors because it is
>>      referenced by nodes in `tegra194.dtsi` (USB padctl) and
>>      `tegra194-p3668.dtsi` (PMIC regulators). Therefore, it must be
>>      kept in the file. I will add a comment to clarify it is a system
>>      supply required by the SoM and SoC layers.
>
>Ah, indeed. That's fine then. I agree that a brief comment would be
>helpful.

Hi Mikko,

Just a gentle ping on this series. Based on your previous feedback, the
v4 patches were sent on July 8 and addressed all your comments:
- Renamed regulator-sd-3v3 to regulator-vdd-3v3-sd
- Moved regulator nodes to the bottom and sorted them alphabetically
- Added a comment to vdd_5v0_sys clarifying it is required by the SoM
  and SoC layers (as it is referenced by USB and PMIC nodes)

Could you please take another look when you have a moment?

Patchwork link for reference:
https://patchwork.ozlabs.org/project/linux-tegra/list/?series=512658

Best regards,
Jiqi Li

>
>>
>> I will submit v4 shortly with these changes incorporated.
>>
>> Best regards,
>> Jiqi Li
>>
>> >> +             compatible = "regulator-fixed";
>> >> +             regulator-name = "VDD_3V3_SD";
>> >> +             regulator-min-microvolt = <3300000>;
>> >> +             regulator-max-microvolt = <3300000>;
>> >> +             gpio = <&gpio TEGRA194_MAIN_GPIO(Q, 5) GPIO_ACTIVE_HIGH>;
>> >> +             regulator-boot-on;
>> >> +             enable-active-high;
>> >> +     };
>> >> +
>> >> +     bus@0 {
>> >> +             /* 40-pin expansion header custom pinmux configuration */
>> >> +             pinmux@2430000 {
>> >> +                     status = "okay";
>> >> +                     pinctrl-names = "default";
>> >> +                     pinctrl-0 = <&pinmux_hdr40>;
>> >> +
>> >> +                     pinmux_hdr40: pinmux-hdr40 {
>> >> +                             pin7 {
>> >> +                                     nvidia,pins = "aud_mclk_ps4";
>> >> +                                     nvidia,function = "aud";
>> >> +                                     nvidia,pull = <TEGRA_PIN_PULL_NONE>;
>> >> +                                     nvidia,tristate = <TEGRA_PIN_DISABLE>;
>> >> +                                     nvidia,enable-input = <TEGRA_PIN_DISABLE>;
>> >> +                             };
>> >> +                             pin11 {
>> >> +                                     nvidia,pins = "uart1_rts_pr4";
>> >> +                                     nvidia,function = "uarta";
>> >> +                                     nvidia,pull = <TEGRA_PIN_PULL_NONE>;
>> >> +                                     nvidia,tristate = <TEGRA_PIN_DISABLE>;
>> >> +                                     nvidia,enable-input = <TEGRA_PIN_DISABLE>;
>> >> +                             };
>> >> +                             pin12 {
>> >> +                                     nvidia,pins = "dap5_sclk_pt5";
>> >> +                                     nvidia,function = "i2s5";
>> >> +                                     nvidia,pull = <TEGRA_PIN_PULL_DOWN>;
>> >> +                                     nvidia,tristate = <TEGRA_PIN_DISABLE>;
>> >> +                                     nvidia,enable-input = <TEGRA_PIN_ENABLE>;
>> >> +                             };
>> >> +                             pin35 {
>> >> +                                     nvidia,pins = "dap5_fs_pu0";
>> >> +                                     nvidia,function = "i2s5";
>> >> +                                     nvidia,pull = <TEGRA_PIN_PULL_DOWN>;
>> >> +                                     nvidia,tristate = <TEGRA_PIN_DISABLE>;
>> >> +                                     nvidia,enable-input = <TEGRA_PIN_ENABLE>;
>> >> +                             };
>> >> +                             pin36 {
>> >> +                                     nvidia,pins = "uart1_cts_pr5";
>> >> +                                     nvidia,function = "uarta";
>> >> +                                     nvidia,pull = <TEGRA_PIN_PULL_UP>;
>> >> +                                     nvidia,tristate = <TEGRA_PIN_ENABLE>;
>> >> +                                     nvidia,enable-input = <TEGRA_PIN_ENABLE>;
>> >> +                             };
>> >> +                             pin38 {
>> >> +                                     nvidia,pins = "dap5_din_pt7";
>> >> +                                     nvidia,function = "i2s5";
>> >> +                                     nvidia,pull = <TEGRA_PIN_PULL_DOWN>;
>> >> +                                     nvidia,tristate = <TEGRA_PIN_ENABLE>;
>> >> +                                     nvidia,enable-input = <TEGRA_PIN_ENABLE>;
>> >> +                             };
>> >> +                             pin40 {
>> >> +                                     nvidia,pins = "dap5_dout_pt6";
>> >> +                                     nvidia,function = "i2s5";
>> >> +                                     nvidia,pull = <TEGRA_PIN_PULL_DOWN>;
>> >> +                                     nvidia,tristate = <TEGRA_PIN_DISABLE>;
>> >> +                                     nvidia,enable-input = <TEGRA_PIN_DISABLE>;
>> >> +                             };
>> >> +                     };
>> >> +             };
>> >> +
>> >> +             /* Configure i2c bus clock to 400kHz for carrier board peripherals */
>> >> +             i2c@3160000 {
>> >> +                     clock-frequency = <400000>;
>> >> +                     status = "okay";
>> >> +             };
>> >> +
>> >> +             /* SDMMC3: External Micro SD card slot */
>> >> +             mmc@3440000 {
>> >> +                     bus-width = <4>;
>> >> +                     cd-gpios = <&gpio_aon TEGRA194_AON_GPIO(CC, 4) GPIO_ACTIVE_LOW>;
>> >> +                     disable-wp;
>> >> +                     no-1-8-v;
>> >> +                     vmmc-supply = <&vdd_3v3_sd>;
>> >> +                     status = "okay";
>> >> +             };
>> >> +
>> >> +     };
>> >> +};
>> >> --
>> >> 2.43.0
>> >>
>> >>
>> >
>>
>>
>>