Re: [PATCH 2/2] arm64: dts: imx8mp: Add support for Data Modul i.MX8M Plus eDM SBC

[Frieder Schrempf <frieder.schrempf@kontron.de>]

On 16.01.23 12:25, Marek Vasut wrote:
> On 1/16/23 11:51, Frieder Schrempf wrote:
>> On 16.01.23 11:39, Marek Vasut wrote:
>>> On 1/11/23 08:38, Ahmad Fatoum wrote:
>>>> Hello Marek,
>>>
>>> Hi,
>>>
>>> [...]
>>>
>>>> Could you share your get_maintainers.pl invocation? I'd like to
>>>> adjust the reviewer entry in MAINTAINERS, so such patches get
>>>> into our kernel@pengutronix.de inbox as well.
>>>
>>> Plain get_maintainer -f path/to/dts
>>>
>>>> Some more comments below.
>>>>
>>>>> +    pmic: pmic@25 {
>>>>> +        compatible = "nxp,pca9450c";
>>>>> +        reg = <0x25>;
>>>>> +        pinctrl-names = "default";
>>>>> +        pinctrl-0 = <&pinctrl_pmic>;
>>>>> +        interrupt-parent = <&gpio1>;
>>>>> +        interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>>>>> +        sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
>>>>
>>>> Here you assume GPIO1_IO04 is muxed as GPIO.
>>>>
>>>>> +    pinctrl_usdhc2: usdhc2-grp {
>>>>> +        fsl,pins = <
>>>>> +            MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT        0xc1
>>>>
>>>> Here you mux it for USDHC2_VSELECT though. Is this intended?
>>>
>>> That's a good question, other DTs do the same thing, I suspect the
>>> sd-vsel-gpios could be dropped,
>>
>> The sd-vsel-gpios is only needed when the hardware has an arbitrary GPIO
>> connected to the SD_VSEL signal of the PCA9450. IMHO, if SD_VSEL is
>> connected to the VSELECT signal it would be better to drop sd-vsel-gpios.
> 
> Current practice does not match this statement however, notice how they
> are all gpio 1 4 :
> 
> $ git grep sd-vsel-gpios
> Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml:
> sd-vsel-gpios:
> arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi: sd-vsel-gpios =
> <&gpio1 4 GPIO_ACTIVE_HIGH>;
> arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc.dts:
> sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> arch/arm64/boot/dts/freescale/imx8mp-dhcom-som.dtsi: sd-vsel-gpios =
> <&gpio1 4 GPIO_ACTIVE_HIGH>;
> arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi: sd-vsel-gpios =
> <&gpio1 4 GPIO_ACTIVE_HIGH>;

Yes, but these are probably all copy-pasted from somewhere. Maybe from
my original Kontron DT (before [1]) where I was actually using
sd-vsel-gpios.
But these DTs all mux the VSELECT signal to the GPIO1_IO04 pad, so
sd-vsel-gpios is useless.

> 
>>> but as long as it is not outright
>>> harmful, it can be used for backward compatibility to support less
>>> complete OSes which may not handle the eSDHC VSELECT bit , so I figured
>>> it is good to keep both options.
>>
>> Would be interesting what OSes you have in mind? The eSDHC
>> driver/hardware should always handle the VSELECT signal, no?
> 
> Not necessarily, but the ones I am aware of (U-Boot, Linux) do handle
> VSELECT.

Ok, but even if VSELECT is not handled. Setting sd-vsel-gpios doesn't
fix this as long as the GPIO1_IO04 pad is muxed to VSELECT.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=eef2c0217e02b6c7ed5b10b82ea944127145e113