[PATCH 2/2] arm64: dts: imx8mm: Add i.MX8M Mini Toradex Verdin based Menlo board

[Marek Vasut]

On 4/8/22 09:56, Marcel Ziswiler wrote:
> Hi Marek

Hi,

I'm skipping the replies to what I commended on to Francesco already.

[...]

>>> +&i2c1 {
>>> +       /* IMX8MM ERRATA e7805 -- I2C is limited to 384 kHz due to SoC bug */
>>> +       clock-frequency = <100000>;
>> should this and the related changes in the other i2c nodes done in the
>> verdin.dtsi? Marcel? (errata is here:
>> https://www.nxp.com/docs/en/errata/IMX8MDQLQ_1N14W.pdf)
> 
> Hehe, good catch. Yeah, we probably should move this one into imx8mm-verdin.dtsi instead. On the other hand, it
> won't hurt doing it twice resp. this one will always override it like that.
> 
> @Francesco: You referenced the wrong errata document. Remember, we are talking about the i.MX 8M Mini here and
> not the i.MX8M. The proper one would be the following:
> 
> https://www.nxp.com/webapp/Download?colCode=IMX8MM_0N87W
> 
> Anyway, looks like NXP has not fixed this and is re-using the exact same I2C IP with the same errata (;-p).

It seems they are all broken, MX8M{M,N,P,Q}, same problem.

Maybe adding such limit in the controller driver might be the better 
approach ?

[...]

>>> +&i2c4 {
>>> +       /* IMX8MM ERRATA e7805 -- I2C is limited to 384 kHz due to SoC bug */
>>> +       clock-frequency = <100000>;
>>> +       /delete-node/ bridge at 2c;
>>> +       /delete-node/ hwmon at 40;
>>> +       /delete-node/ hdmi at 48;
>>> +       /delete-node/ touch at 4a;
>>> +       /delete-node/ hwmontemp at 4f;
>>> +       /delete-node/ eeprom at 50;
>>> +       /delete-node/ eeprom at 57;
>> All of those are disabled in the dtsi, is it really worth deleting the
>> nodes?
> 
> I am also not too big of a fan of this delete-node directive. At least it already hurt us multiple times in our
> downstream device tree overlay endeavour.

Indeed, the /delete-node/ is a DTC directive, it can not be represented 
in DTB, so anything which is optional shouldn't be in the base DT, else 
the DTO cannot remove it afterward and that is a real nasty catch.

That's why the /delete-node/ is a good idea in base DT, then DTOs can 
only fill in the extras which is no problem. And since this board does 
use DTOs for display pipeline, anything which shouldn't be in the base 
DT should not be present in the base DTB at all.

[...]

>>> +       pinctrl_led: ledgrp {
>>> +               fsl,pins = <
>>> +                       MX8MM_IOMUXC_SAI1_TXD6_GPIO4_IO18               0x1c4
>>> +                       MX8MM_IOMUXC_SAI1_TXFS_GPIO4_IO10               0x1c4
>>> +               >;
>>> +       };
>>> +
>>> +       pinctrl_uart4_rts: uart4rtsgrp {
>>> +               fsl,pins = <
>>> +                       /* SODIMM 222 */
>>> +                       MX8MM_IOMUXC_GPIO1_IO09_GPIO1_IO9               0x184
>>> +               >;
>>> +       };
>>> +};
> 
> At least in imx8mm-verdin.dtsi we do have this iomuxc node at the very end. Not sure whether this is a hard
> convention though.

I cannot tell, I saw it done either way in different DTs.

[...]

>>> +&reg_usb_otg2_vbus {
>>> +       /delete-property/ enable-active-high;
>>> +       gpio = <&gpio1 14 GPIO_ACTIVE_LOW>;
>>> +};
> 
> Okay, I was on the verge of getting rid of that regulator and relying on the IP built-in USB power enable
> functionality due to us having seen issues with such regulator during suspend/resume resp. power-off. However,
> I guess, there might be other solutions using such regular regulators. Not sure...

That should be doable too, on this board the regulator polarity is just 
inverted compared to Dahlia carrier board.

So, all I would have to do is:
/delete-property/ power-active-high;

Or add that property to Dahlia DT and keep it out of the Verdin SoM 
DTSI, then I won't add it.

Will you send a patch and CC me ?

>>> +&sai2 {
>>> +       status = "disabled";
>>> +};
>>> +
>>> +&uart1 {
>>> +       uart-has-rtscts;
>>> +       status = "okay";
>>> +};
>>> +
>>> +&uart2 {
>>> +       uart-has-rtscts;
> 
> That one we already have in the module's device tree. But, yeah, I guess it won't hurt.

Fixed

[...]