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

[Marek Vasut]

On 4/10/22 10:46, Francesco Dolcini wrote:
> On Fri, Apr 08, 2022 at 05:02:15PM +0200, Marek Vasut wrote:
>> On 4/8/22 08:46, Francesco Dolcini wrote:
>>>> +	/delete-node/ gpio-keys;
>>> would it be better if we had a label in the imx8mm-verdin.dtsi and we
>>> could just set status=disabled here?
>>
>> It would be better if there was Verdin SoM dtsi and Verdin carrier board dts
>> which includes the SoM dtsi. Right now, it seems these two things are
>> conflated a bit.
>>
>> There are no GPIO buttons on the Verdin SoM, there are some on the Dahlia
>> carrier board I think.
> 
> The GPIO keys, for example the suspend button, are part of Verdin family
> SODIMM connector pinout/definition (see related datasheets). In the SoM
> dtsi we implement our standard family definition.
> 
> Of course, you are free to redefine this in any way you prefer. In
> general the way we envision this is to just enable/disable in the
> carrier board or overlay dts, this is the reason I proposed to add
> a label there. I do not see any real value on deleting the node at all,
> just some potential for additional maintenance burden.

There are two reasons for not adding DT nodes for hardware which is not 
populated:
- You are polluting the DT with unused nodes representing hardware which
   can never be present on the system, that makes the DT bigger and more
   complex, for no reason.
- Once DTOs enter the picture, these so far only useless nodes and
   properties actively become a problem. You cannot delete either node or
   property from a base DT using a DTO, because neither /delete-node/ nor
   /delete-property/ can be encoded into the DTO blob .

So, speaking about this GPIO button which is not physically on the SoM, 
I would propose the following:
- Keep some sort of gpio-line-names property on the SoM level, to have
   mapping between GPIOs and SoM edge connector pins
- Move the "gpio-button" node into the carrier board DT

If someone designs a carrier board with this button populated, then they 
should describe it in the DT. If someone designs a carrier board without 
any physical button, then they won't describe it in the DT.

And as a bit of an encore, if you have optional components on a SoM 
(e.g. you have different SoM variants, some of which may contain extra 
components and some of them may not, e.g. to save cost or whatever), 
then you should describe the components in SoM DT, but set it to 
status="disabled" by default. And then when a carrier board is populated 
with specific SoM model, it can enable those extra populated SoM 
components on carrier board DT level (or in some proxy SoM-variant DTSI 
if you really want to be very precise about the separation, but that 
leads to combinatorial explosion of SoM-variant DTSIs over time).

>>>> +	/* CAN controller on the baseboard */
>>>> +	canfd: can at 0 {
>>>> +		compatible = "microchip,mcp2518fd";
>>>> +		clocks = <&clk20m>;
>>> You are using the node defined in the verdin.dtsi here, while I guess
>>> this is a separate oscillator part of the carrier board.
>>>
>>> Should you define a new clock? My concern is that we had discussion to
>>> change the SoM to move from 20 to 40 MHz, and we would remove this entry
>>> from the dtsi if we would do such a change.
>>
>> Is the 20 MHz oscillator on your SoM or on the carrier board ?
> In the SoM, not available on the SODIMM connector. Your clock if for
> sure not this one.

Right, lemme switch this one to dedicated clock node.

>>>> +&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?
>>
>> They are not present on the hardware, why should they be described in the DT
>> ? Hence the /delete-node/ .
> Not 100% correct (see the SoM datasheet)

I can't find e.g. the hdmi at 48 LT8912 DSI-to-HDMI bridge, nor the 
bridge at 2c SN65DSI83 DSI-to-LVDS bridge in the datasheet, and visual 
inspection of the SoM also leads to nothing. I also cannot find them on 
the Dahlia carrier board. But I can find them on the optional expansion 
modules for the Dahlia carrier board. So, why are they described in the 
SoM DT (they shouldn't be) ?

Maybe the other components are actually on the SoM, but then, why are 
those not enabled by default ? Maybe because not all SoM variants have 
them populated ?

> , anyway is this causing you any
> real trouble having those nodes disabled instead of removed from the
> dts? I assume nor the file size nor the parsing time are an issue there.

See above, it is the same argument as regarding the gpio-button .
The SoM DTSI shouldn't become a dumping ground for nodes describing any 
random hardware which might be attached to arbitrary carrier board.

> Of course, not a big deal for me you removing this nodes in the menlo
> dts, but in general I'm happy to make your life easier when using our
> som dtsi file, this is the reason for asking.

See above, I hope that clarifies it.