Re: [PATCH] arm64: dts: freescale: add initial support for Google i.MX 8MQ Phanbell

[Marco Franchi]

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