RE: [PATCH 2/4] ARM: dts: imx6ull-dhcom: Add DH electronics DHCOM i.MX6ULL SoM and PDK2 board

[Christoph Niedermaier <cniedermaier@dh-electronics.com>]

From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
Sent: Thursday, November 17, 2022 2:10 PM
> On 17/11/2022 11:31, Christoph Niedermaier wrote:
>> Add support for DH electronics DHCOM SoM and PDK2 rev. 400 carrier
>> board. This is an SoM with i.MX6ULL and an evaluation kit. The
>> baseboard provides Ethernet, UART, USB, CAN and optional display.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +#include "imx6ull-dhcom-som.dtsi"
>> +
>> +/ {
>> +     model = "DH electronics i.MX6ULL DHCOM on Premium Developer Kit (2)";
>> +     compatible = "dh,imx6ull-dhcom-pdk2", "dh,imx6ull-dhcom-som",
>> +                  "dh,imx6ull-dhcor-som", "fsl,imx6ull";
>> +
>> +     clk_ext_audio_codec: clock-codec {
>> +             #clock-cells = <0>;
>> +             clock-frequency = <24000000>;
>> +             compatible = "fixed-clock";
>> +     };
>> +
>> +     display_bl: display-bl {
>> +             brightness-levels = <0 16 22 30 40 55 75 102 138 188 255>;
>> +             compatible = "pwm-backlight";
>> +             default-brightness-level = <8>;
>> +             enable-gpios = <&gpio5 8 GPIO_ACTIVE_HIGH>; /* GPIO G */
>> +             power-supply = <&reg_panel_3v3>;
>> +             pwms = <&pwm1 0 50000 PWM_POLARITY_INVERTED>;
>> +             status = "okay";
> 
> okay is by default, unless you override a node. Are you overriding?

No, I am not overriding.
Will be removed in next version.

>> +     };
>> +
>> +     gpio-keys {
>> +             compatible = "gpio-keys";
>> +
>> +             button-0 {
>> +                     gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; /* GPIO A */
>> +                     label = "TA1-GPIO-A";
>> +                     linux,code = <KEY_A>;
>> +                     wakeup-source;
>> +             };
>> +
> 
> (....)
> 
>> +
>> +&fec2 { /* DHCOM ETH2 */
>> +     phy-handle = <&mdio2_phy1>;
>> +     phy-mode = "rmii";
>> +     pinctrl-0 = <&pinctrl_fec2>;
>> +     pinctrl-names = "default";
>> +     status = "okay";
>> +
>> +     mdio {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +
>> +             mdio2_phy0: ethernet-phy@0 { /* SMSC LAN8710Ai */
>> +                     clock-names = "rmii-ref";
>> +                     clocks = <&clks IMX6UL_CLK_ENET_REF>;
>> +                     compatible = "ethernet-phy-id0007.c0f0",
>> +                                  "ethernet-phy-ieee802.3-c22";
>> +                     interrupt-parent = <&gpio5>;
>> +                     interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
>> +                     pinctrl-0 = <&pinctrl_fec1_phy &pinctrl_snvs_fec1_phy>;
>> +                     pinctrl-names = "default";
>> +                     reg = <0>;
> 
> compatible first, reg second, then the rest of properties. Same in other
> places.

Sorry, I have sorted it alphabetically.
Should the status property be sorted at the end or also alphabetically?

>> +                     reset-assert-us = <500>;
>> +                     reset-deassert-us = <500>;
>> +                     reset-gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
>> +                     smsc,disable-energy-detect; /* Make plugin detection reliable */
>> +             };
>> +
>> +             mdio2_phy1: ethernet-phy@1 { /* SMSC LAN8710Ai */
>> +                     clock-names = "rmii-ref";
>> +                     clocks = <&clks IMX6UL_CLK_ENET2_REF>;
>> +                     compatible = "ethernet-phy-id0007.c0f0",
>> +                                  "ethernet-phy-ieee802.3-c22";
>> +                     interrupt-parent = <&gpio5>;
>> +                     interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
>> +                     pinctrl-0 = <&pinctrl_fec2_phy &pinctrl_snvs_fec2_phy>;
>> +                     pinctrl-names = "default";
>> +                     reg = <1>;
>> +                     reset-assert-us = <500>;
>> +                     reset-deassert-us = <500>;
>> +                     reset-gpios = <&gpio3 24 GPIO_ACTIVE_LOW>;
>> +                     smsc,disable-energy-detect; /* Make plugin detection reliable */
>> +             };
>> +     };
>> +};
>> +
> 
> (...)
> 
>> diff --git a/arch/arm/boot/dts/imx6ull-dhcor-som.dtsi b/arch/arm/boot/dts/imx6ull-dhcor-som.dtsi
>> new file mode 100644
>> index 000000000000..4155458897e2
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/imx6ull-dhcor-som.dtsi
>> @@ -0,0 +1,247 @@
>> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
>> +/*
>> + * Copyright (C) 2022 DH electronics GmbH
>> + */
>> +
>> +#include <dt-bindings/clock/imx6ul-clock.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/leds/common.h>
>> +#include <dt-bindings/pwm/pwm.h>
>> +#include "imx6ull.dtsi"
>> +
>> +/ {
>> +     memory@80000000 { /* Appropriate memory size will be filled by U-Boot */
>> +             device_type = "memory";
>> +             reg = <0x80000000 0>;
>> +     };
>> +};
>> +
>> +&cpu0 {
>> +     fsl,soc-operating-points = <
>> +             /* KHz  uV */
>> +             900000  1250000
>> +             792000  1250000
>> +             528000  1175000
>> +             396000  1175000
>> +             198000  1175000
>> +     >;
>> +     operating-points = <
>> +             /* kHz  uV */
>> +             900000  1275000
>> +             792000  1250000
>> +             528000  1175000
>> +             396000  1025000
>> +             198000  950000
>> +     >;
> 
> Why two properties? No support for operating-points-v2? Or is it
> override of SoC DTSI?

Yes, overriding SoC DTSI.

> 
>> +};
>> +
>> +&gpio1 {
>> +     pinctrl-0 = <&pinctrl_spi1_switch>;
>> +     pinctrl-names = "default";
>> +     /*
>> +      * Pin SPI_BOOT_FLASH_EN (GPIO 1.9) is a switch for either using the
>> +      * DHCOM SPI1 interface or accessing the SPI bootflash. Both using
>> +      * ecspi1, but muxed to different pins. The DHCOM SPI1 interface uses
>> +      * the pins PAD_LCD_DATA21..23 and the SPI bootflash uses the pins
>> +      * PAD_CSI_DATA04..07. If the SPI bootflash is enabled the pins for
>> +      * DHCOM GPIOs N/O/P/Q/R/S/T/U aren't usable anymore, because they
>> +      * are used for the bus interface to the SPI bootflash. The GPIOs are
>> +      * disconnected by a buffer which is also controlled via the pin
>> +      * SPI_BOOT_FLASH_EN. Therefore the access to the bootflash is a
>> +      * special case and is disabled by setting GPIO 1.9 to high.
>> +      */
>> +     spi1-switch-hog {
>> +             gpio-hog;
>> +             gpios = <9 0>;
>> +             output-high;
>> +             line-name = "spi1-switch";
>> +     };
>> +};
>> +
>> +&i2c1 {
>> +     clock-frequency = <100000>;
>> +     pinctrl-0 = <&pinctrl_i2c1>;
>> +     pinctrl-1 = <&pinctrl_i2c1_gpio>;
>> +     pinctrl-names = "default", "gpio";
>> +     scl-gpios = <&gpio1 28 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> +     sda-gpios = <&gpio1 29 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> +     status = "okay";
>> +
>> +     pmic@58 {
>> +             compatible = "dlg,da9061";
>> +             reg = <0x58>;
>> +
>> +             onkey {
>> +                     compatible = "dlg,da9061-onkey", "dlg,da9062-onkey";
>> +                     status = "disabled";
>> +             };
>> +
>> +             regulators {
>> +                     vdd_soc_in_1v4: buck1 {
>> +                             regulator-always-on;
>> +                             regulator-boot-on;
>> +                             regulator-max-microvolt = <1400000>;
>> +                             regulator-min-microvolt = <1400000>;
>> +                             regulator-name = "vdd_soc_in_1v4";
>> +                     };
>> +
>> +                     vcc_3v3: buck2 {
>> +                             regulator-always-on;
>> +                             regulator-boot-on;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-min-microvolt = <3300000>;
>> +                             regulator-name = "vcc_3v3";
>> +                     };
>> +
>> +                     /*
>> +                      * The current DRR3 memory can be supplied with a
>> +                      * voltage of either 1.35V or 1.5V. For reasons of
>> +                      * backward compatibility to only 1.5V DDR3 memory,
>> +                      * the voltage is set to 1.5V.
>> +                      */
>> +                     vcc_ddr_1v35: buck3 {
>> +                             regulator-always-on;
>> +                             regulator-boot-on;
>> +                             regulator-max-microvolt = <1500000>;
>> +                             regulator-min-microvolt = <1500000>;
>> +                             regulator-name = "vcc_ddr_1v35";
>> +                     };
>> +
>> +                     vcc_2v5: ldo1 {
>> +                             regulator-always-on;
>> +                             regulator-boot-on;
>> +                             regulator-max-microvolt = <2500000>;
>> +                             regulator-min-microvolt = <2500000>;
>> +                             regulator-name = "vcc_2v5";
>> +                     };
>> +
>> +                     vdd_snvs_in_3v3: ldo2 {
>> +                             regulator-always-on;
>> +                             regulator-boot-on;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-min-microvolt = <3300000>;
>> +                             regulator-name = "vdd_snvs_in_3v3";
>> +                     };
>> +
>> +                     vcc_1v8: ldo3 {
>> +                             regulator-always-on;
>> +                             regulator-boot-on;
>> +                             regulator-max-microvolt = <1800000>;
>> +                             regulator-min-microvolt = <1800000>;
>> +                             regulator-name = "vcc_1v8";
>> +                     };
>> +
>> +                     vcc_1v2: ldo4 {
>> +                             regulator-always-on;
>> +                             regulator-boot-on;
>> +                             regulator-max-microvolt = <1200000>;
>> +                             regulator-min-microvolt = <1200000>;
>> +                             regulator-name = "vcc_1v2";
>> +                     };
>> +             };
>> +
>> +             thermal {
>> +                     compatible = "dlg,da9061-thermal", "dlg,da9062-thermal";
>> +                     status = "disabled";
>> +             };
>> +
>> +             watchdog {
>> +                     compatible = "dlg,da9061-watchdog", "dlg,da9062-watchdog";
>> +                     status = "disabled";
>> +             };
>> +     };
>> +};
>> +
>> +&ocotp {
>> +     read-only; /* Don't get write access by default */
>> +};
>> +
>> +&reg_arm {
>> +     vin-supply = <&vdd_soc_in_1v4>;
>> +};
>> +
>> +&reg_soc {
>> +     vin-supply = <&vdd_soc_in_1v4>;
>> +};
>> +
>> +&uart2 { /* BT on LGA (BT_REG_ON is connected to LGA pin E1) */
>> +     pinctrl-0 = <&pinctrl_uart2>;
>> +     pinctrl-names = "default";
>> +     uart-has-rtscts;
>> +     status = "okay";
>> +
>> +     /*
>> +      * Actually, the maximum speed of the chip is 4MBdps, but there are
>> +      * limitations that prevent this speed. It hasn't yet been figured out
>> +      * what the reason for this is. Currently, the maximum speed of 3MBdps
>> +      * can be used without any problems. If the limitation can be overcome,
>> +      * the speed can be increased accordingly.
>> +      */
>> +     bluetooth: bluetooth { /* muRata 1DX */
>> +             compatible = "brcm,bcm43430a1-bt";
>> +             max-speed = <3000000>;
>> +             vbat-supply = <&vcc_3v3>;
>> +             vddio-supply = <&vcc_3v3>;
>> +     };
>> +};
>> +
>> +&usdhc1 { /* WiFi on LGA (WL_REG_ON is connected to LGA pin E3) */
> 
> This is weird commenting style. Don't do this. Comments go usually
> before the block. Fix it here and in other places.

Will be fixed in next version.

>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +     bus-width = <4>;
>> +     no-1-8-v;
>> +     non-removable;
>> +     keep-power-in-suspend;
>> +     pinctrl-0 = <&pinctrl_usdhc1_wifi>;
>> +     pinctrl-names = "default";
>> +     wakeup-source;
>> +     status = "okay";
>> +
>> +     brcmf: wifi@1 { /* muRata 1DX */
>> +             compatible = "brcm,bcm4329-fmac";
>> +             reg = <1>;
>> +     };
>> +};
>> +
>> +&iomuxc {
>> +     pinctrl_i2c1: i2c1-grp {
>> +             fsl,pins = <
>> +                     MX6UL_PAD_UART4_TX_DATA__I2C1_SCL       0x4001b8b0
>> +                     MX6UL_PAD_UART4_RX_DATA__I2C1_SDA       0x4001b8b0
>> +             >;
>> +     };
>> +
>> +     pinctrl_i2c1_gpio: i2c1-gpio-grp {
>> +             fsl,pins = <
>> +                     MX6UL_PAD_UART4_TX_DATA__GPIO1_IO28     0x4001b8b0
>> +                     MX6UL_PAD_UART4_RX_DATA__GPIO1_IO29     0x4001b8b0
>> +             >;
>> +     };
>> +
>> +     pinctrl_spi1_switch: spi1-switch-grp {
>> +             fsl,pins = <
>> +                     MX6UL_PAD_GPIO1_IO09__GPIO1_IO09        0x120b0 /* SPI_BOOT_FLASH_EN */
>> +             >;
>> +     };
>> +
>> +     pinctrl_uart2: uart2-grp {
>> +             fsl,pins = <
>> +                     MX6UL_PAD_UART2_TX_DATA__UART2_DCE_TX   0x1b0b1
>> +                     MX6UL_PAD_UART2_RX_DATA__UART2_DCE_RX   0x1b0b1
>> +                     MX6UL_PAD_UART3_RX_DATA__UART2_DCE_RTS  0x1b0b1
>> +                     MX6UL_PAD_UART3_TX_DATA__UART2_DCE_CTS  0x1b0b1
>> +             >;
>> +     };
>> +
>> +     pinctrl_usdhc1_wifi: usdhc1-wifi-grp {
>> +             fsl,pins = <
>> +                     MX6UL_PAD_SD1_CMD__USDHC1_CMD           0x1b0b0
>> +                     MX6UL_PAD_SD1_CLK__USDHC1_CLK           0x10010
>> +                     MX6UL_PAD_SD1_DATA0__USDHC1_DATA0       0x1b0b0
>> +                     MX6UL_PAD_SD1_DATA1__USDHC1_DATA1       0x1b0b0
>> +                     MX6UL_PAD_SD1_DATA2__USDHC1_DATA2       0x1b0b0
>> +                     MX6UL_PAD_SD1_DATA3__USDHC1_DATA3       0x1b0b0
>> +             >;
>> +     };
>> +};
> 
> Best regards,
> Krzysztof

Thanks and regards
Christoph
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel