Hi Fabio, Thank you for your comments. I have some points to discuss inline: Em qui., 14 de nov. de 2019 às 18:13, Fabio Estevam <festevam@xxxxxxxxx> escreveu: > > Hi Marco, > > On Thu, Nov 14, 2019 at 4:56 PM Marco Antonio Franchi > <marco.franchi@xxxxxxx> wrote: > > > > This patch adds the device tree to support Google Coral Edge TPU, > > historicaly named as fsl-imx8mq-phanbell, a computer on module > > which can be used for AI/ML propose. > > > > It introduces a minimal enablement support for this module and > > What are the features that have been tested? I can include one list at the v2. > > Also, is the schematics available? Yes: https://storage.googleapis.com/site_and_emails_static_assets/Files/Coral-Dev-Board-baseboard-schematic.pdf > > > was totally based on the NXP i.MX 8MQ EVK board and i.MX 8MQ Phanbell > > Google Source Code for Coral Edge TPU Mendel release: > > https://coral.googlesource.com/linux-imx/ > > > > This patch was tested using the U-Boot 2017-03-1-release-chef, > > which is supported by the Coral Edge TPU Mendel release: > > https://coral.googlesource.com/uboot-imx/ > > I would suggest removing this paragraph from the commit log as it is > not relevant to the dts itself. This U-Boot is the unique available for the Coral Edge TPU, and it does not provides the fdt_file settup, so I cannot change the Device Tree name and I thought it was important to put this information somehow here. > > > diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile > > index 38e344a2f0ff..cc7e02a30ed1 100644 > > --- a/arch/arm64/boot/dts/freescale/Makefile > > +++ b/arch/arm64/boot/dts/freescale/Makefile > > @@ -21,6 +21,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls2088a-rdb.dtb > > dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-qds.dtb > > dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb > > > > +dtb-$(CONFIG_ARCH_MXC) += fsl-imx8mq-phanbell.dtb > > Please remove the fsl prefix and call it mx8mq-phanbell.dtb instead to > align with the other imx8mq dtbs. If I applied this change, I won't be able to boot the board, due to the U-Boot dependence. Should I try to apply the U-Boot mainline support first? > > > +&i2c1 { > > + clock-frequency = <400000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_i2c1>; > > + status = "okay"; > > + > > + pmic: pmic@4b { > > + reg = <0x4b>; > > + compatible = "rohm,bd71837"; > > + pinctrl-0 = <&pinctrl_pmic>; > > + gpio_intr = <&gpio1 3 GPIO_ACTIVE_LOW>; > > This property does not exist upstream. > > You should describe the interrupt like this instead: > > interrupt-parent = <&gpio1>; > interrupts = <3 GPIO_ACTIVE_LOW>; > Sure, I will! > > + > > + gpo { > > + rohm,drv = <0x0C>; > > This property does not exist upstream. > > > +&sai2 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_sai2>; > > + assigned-clocks = > > + <&clk IMX8MQ_AUDIO_PLL1_BYPASS>, <&clk IMX8MQ_CLK_SAI2>; > > Please don't split the lines as it gets harder to read. > > > + assigned-clock-parents = > > + <&clk IMX8MQ_AUDIO_PLL1>, <&clk IMX8MQ_AUDIO_PLL1_OUT>; > > Same here. > > > + assigned-clock-rates = <0>, <24576000>; > > + status = "okay"; > > +}; > > + > > +&wdog1 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_wdog>; > > + fsl,ext-reset-output; > > + status = "okay"; > > +}; > > + > > +&iomuxc { > > + pinctrl-names = "default"; > > + > > + imx8mq-evk { > > No need for this imx8mq-evk container. > > > + pinctrl_pmic: pmicirq { > > + fsl,pins = < > > + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x41 /*0x17059*/ > > This comment looks confusing. I would suggest removing it. > > Regards, > > Fabio Estevam Ok for all the other comments. Thanks for your suggestion Fabio. Please, just check the comments regarding the Device Tree name and I will send the v2 with the required changes. BR, Marco
