RE: [PATCH 3/3] arm64: dts: freescale: Add support for i.MX8QXP AI_ML board

[Aisheng Dong]

> > > +_lpuart1 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <_lpuart1>;
> > > + status = "okay";
> > > +};
> > > +
> > > +/* Debug */
> > > +_lpuart2 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <_lpuart2>;
> > > + status = "okay";
> > > +};
> > > +
> > > +/* PCI-E */
> >
> > A bit confusing for the comments...
> >
> 
> Hmm. How about, "PCI-E UART"?
> 

It seems to be related to your board. So up to you.
I'm fine with it.

Regards
Aisheng

Re: [PATCH 3/3] arm64: dts: freescale: Add support for i.MX8QXP AI_ML board

[Manivannan Sadhasivam]

Hi Dong,

On Wed, Jul 17, 2019 at 10:40:10AM +, Aisheng Dong wrote:
> > From: Manivannan Sadhasivam 
> > Sent: Wednesday, July 17, 2019 2:11 PM
> > 
> > Add support for i.MX8QXP AI_ML board from Einfochips. This board is one of
> > the Consumer Edition boards of the 96Boards family based on i.MX8QXP SoC
> > from NXP/Freescale.
> > 
> > The initial support includes following peripherals which are tested and 
> > known
> > to be working:
> > 
> > 1. Debug serial via UART2
> > 2. uSD
> > 3. WiFi
> > 4. Ethernet
> > 
> > Signed-off-by: Manivannan Sadhasivam 
> 
> The patch looks good to me. Only a few nitpicks below.
> Otherwise,
> Reviewed-by: Dong Aisheng 
> 
> Regards
> Dong Aisheng
> 

Thanks for the review!

> > ---
> >  arch/arm64/boot/dts/freescale/Makefile|   1 +
> >  .../boot/dts/freescale/imx8qxp-ai_ml.dts  | 249 ++
> >  2 files changed, 250 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/Makefile
> > b/arch/arm64/boot/dts/freescale/Makefile
> > index 0bd122f60549..bd8460549d1a 100644
> > --- a/arch/arm64/boot/dts/freescale/Makefile
> > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > @@ -24,4 +24,5 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
> > +dtb-$(CONFIG_ARCH_MXC) += imx8qxp-ai_ml.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb diff --git
> > a/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> > b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> > new file mode 100644
> > index ..dcd36e57d916
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018 Einfochips
> > + * Copyright 2019 Linaro Ltd.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "imx8qxp.dtsi"
> > +
> > +/ {
> > +   model = "Einfochips i.MX8QXP AI_ML";
> > +   compatible = "einfochips,imx8qxp-ai_ml", "fsl,imx8qxp";
> > +
> > +   aliases {
> > +   serial1 = _lpuart1;
> > +   serial2 = _lpuart2;
> > +   serial3 = _lpuart3;
> > +   };
> > +
> > +   chosen {
> > +   stdout-path = _lpuart2;
> > +   };
> > +
> > +   memory@8000 {
> > +   device_type = "memory";
> > +   reg = <0x 0x8000 0 0x8000>;
> > +   };
> > +
> > +   leds {
> > +   compatible = "gpio-leds";
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_leds>;
> > +
> > +   user_led1 {
> > +   label = "green:user1";
> > +   gpios = <_gpio4 16 GPIO_ACTIVE_HIGH>;
> > +   linux,default-trigger = "heartbeat";
> > +   };
> > +
> > +   user_led2 {
> > +   label = "green:user2";
> > +   gpios = <_gpio0 6 GPIO_ACTIVE_HIGH>;
> > +   linux,default-trigger = "none";
> > +   };
> > +
> > +   user_led3 {
> > +   label = "green:user3";
> > +   gpios = <_gpio0 7 GPIO_ACTIVE_HIGH>;
> > +   linux,default-trigger = "mmc1";
> > +   default-state = "off";
> > +   };
> > +
> > +   user_led4 {
> > +   label = "green:user4";
> > +   gpios = <_gpio4 21 GPIO_ACTIVE_HIGH>;
> > +   panic-indicator;
> > +   linux,default-trigger = "none";
> > +   };
> > +
> > +   wlan_active_led {
> > +   label = "yellow:wlan";
> > +   gpios = <_gpio4 17 GPIO_ACTIVE_HIGH>;
> > +   linux,default-trigger = "phy0tx";
> > +   default-state = "off";
> > +   };
> > +
> > +   bt_active_led {
> > +   label = "blue:bt";
> > +   gpios = <_gpio4 18 GPIO_ACTIVE_HIGH>;
> > +   linux,default-trigger = "hci0-power";
> > +   default-state = "off";
> > +   };
> > +   };
> > +
> > +   sdio_pwrseq: sdio-pwrseq {
> > +   compatible = "mmc-pwrseq-simple";
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_wifi_reg_on>;
> > +   reset-gpios = <_gpio3 24 GPIO_ACTIVE_LOW>;
> > +   };
> > +};
> > +
> > +/* BT */
> > +_lpuart0 {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_lpuart0>;
> > +   uart-has-rtscts;
> > +   status = "okay";
> > +};
> > +
> > +/* LS-I2C0 */
> 
> Typo?
> 

Ah, yes. It should be LS-UART0, will fix it.

> > +_lpuart1 {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_lpuart1>;
> > +   status = "okay";
> > +};
> > +
> > +/* Debug */
> > +_lpuart2 {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_lpuart2>;
> > +   status = "okay";
> > +};
> > +
> > +/* PCI-E */
> 
> A bit confusing for the comments...
> 

Hmm. How about, 

RE: [PATCH 3/3] arm64: dts: freescale: Add support for i.MX8QXP AI_ML board

[Aisheng Dong]

> From: Manivannan Sadhasivam 
> Sent: Wednesday, July 17, 2019 2:11 PM
> 
> Add support for i.MX8QXP AI_ML board from Einfochips. This board is one of
> the Consumer Edition boards of the 96Boards family based on i.MX8QXP SoC
> from NXP/Freescale.
> 
> The initial support includes following peripherals which are tested and known
> to be working:
> 
> 1. Debug serial via UART2
> 2. uSD
> 3. WiFi
> 4. Ethernet
> 
> Signed-off-by: Manivannan Sadhasivam 

The patch looks good to me. Only a few nitpicks below.
Otherwise,
Reviewed-by: Dong Aisheng 

Regards
Dong Aisheng

> ---
>  arch/arm64/boot/dts/freescale/Makefile|   1 +
>  .../boot/dts/freescale/imx8qxp-ai_ml.dts  | 249 ++
>  2 files changed, 250 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> 
> diff --git a/arch/arm64/boot/dts/freescale/Makefile
> b/arch/arm64/boot/dts/freescale/Makefile
> index 0bd122f60549..bd8460549d1a 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -24,4 +24,5 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
> +dtb-$(CONFIG_ARCH_MXC) += imx8qxp-ai_ml.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb diff --git
> a/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> new file mode 100644
> index ..dcd36e57d916
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 Einfochips
> + * Copyright 2019 Linaro Ltd.
> + */
> +
> +/dts-v1/;
> +
> +#include "imx8qxp.dtsi"
> +
> +/ {
> + model = "Einfochips i.MX8QXP AI_ML";
> + compatible = "einfochips,imx8qxp-ai_ml", "fsl,imx8qxp";
> +
> + aliases {
> + serial1 = _lpuart1;
> + serial2 = _lpuart2;
> + serial3 = _lpuart3;
> + };
> +
> + chosen {
> + stdout-path = _lpuart2;
> + };
> +
> + memory@8000 {
> + device_type = "memory";
> + reg = <0x 0x8000 0 0x8000>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 = <_leds>;
> +
> + user_led1 {
> + label = "green:user1";
> + gpios = <_gpio4 16 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "heartbeat";
> + };
> +
> + user_led2 {
> + label = "green:user2";
> + gpios = <_gpio0 6 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "none";
> + };
> +
> + user_led3 {
> + label = "green:user3";
> + gpios = <_gpio0 7 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "mmc1";
> + default-state = "off";
> + };
> +
> + user_led4 {
> + label = "green:user4";
> + gpios = <_gpio4 21 GPIO_ACTIVE_HIGH>;
> + panic-indicator;
> + linux,default-trigger = "none";
> + };
> +
> + wlan_active_led {
> + label = "yellow:wlan";
> + gpios = <_gpio4 17 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "phy0tx";
> + default-state = "off";
> + };
> +
> + bt_active_led {
> + label = "blue:bt";
> + gpios = <_gpio4 18 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "hci0-power";
> + default-state = "off";
> + };
> + };
> +
> + sdio_pwrseq: sdio-pwrseq {
> + compatible = "mmc-pwrseq-simple";
> + pinctrl-names = "default";
> + pinctrl-0 = <_wifi_reg_on>;
> + reset-gpios = <_gpio3 24 GPIO_ACTIVE_LOW>;
> + };
> +};
> +
> +/* BT */
> +_lpuart0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_lpuart0>;
> + uart-has-rtscts;
> + status = "okay";
> +};
> +
> +/* LS-I2C0 */

Typo?

> +_lpuart1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_lpuart1>;
> + status = "okay";
> +};
> +
> +/* Debug */
> +_lpuart2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_lpuart2>;
> + status = "okay";
> +};
> +
> +/* PCI-E */

A bit confusing for the comments...

> +_lpuart3 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_lpuart3>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_fec1>;
> + phy-mode = "rgmii-id";
> + phy-handle = <>;
> + fsl,magic-packet;
> + status = "okay";
> +
> + mdio {
> + #address-cells = <1>;
>