Re: [PATCH v4 3/3] arm64: dts: imx93: Add phyBOARD-Segin-i.MX93 support

[Stefan Wahren <wahrenst@gmx.net>]

Hi Wadim,

Am 23.01.24 um 09:25 schrieb Wadim Egorov:
>
> Am 23.01.24 um 08:42 schrieb Stefan Wahren:
>> Hi Wadim,
>>
>> Am 23.01.24 um 07:11 schrieb Wadim Egorov:
>>> Hey Mathieu,
>>>
>>> Am 22.01.24 um 10:53 schrieb Mathieu Othacehe:
>>>> Add basic support for phyBOARD-Segin-i.MX93.
>>>> Main features are:
>>>> * eMMC
>>>> * Ethernet
>>>> * SD-Card
>>>> * UART
>>>>
>>>> Signed-off-by: Mathieu Othacehe <othacehe@gnu.org>
>>>> ---
>>>>   arch/arm64/boot/dts/freescale/Makefile        |   1 +
>>>>   .../dts/freescale/imx93-phyboard-segin.dts    | 141
>>>> ++++++++++++++++++
>>>>   .../boot/dts/freescale/imx93-phycore-som.dtsi | 127 ++++++++++++++++
>>>>   3 files changed, 269 insertions(+)
>>>>   create mode 100644
>>>> arch/arm64/boot/dts/freescale/imx93-phyboard-segin.dts
>>>>   create mode 100644
>>>> arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi
>>>>
>>>> diff --git a/arch/arm64/boot/dts/freescale/Makefile
>>>> b/arch/arm64/boot/dts/freescale/Makefile
>>>> index 2e027675d7bb..65db918c821c 100644
>>>> --- a/arch/arm64/boot/dts/freescale/Makefile
>>>> +++ b/arch/arm64/boot/dts/freescale/Makefile
>>>> @@ -201,6 +201,7 @@ dtb-$(CONFIG_ARCH_MXC) +=
>>>> imx8qxp-colibri-iris-v2.dtb
>>>>   dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb
>>>>   dtb-$(CONFIG_ARCH_MXC) += imx8ulp-evk.dtb
>>>>   dtb-$(CONFIG_ARCH_MXC) += imx93-11x11-evk.dtb
>>>> +dtb-$(CONFIG_ARCH_MXC) += imx93-phyboard-segin.dtb
>>>>   dtb-$(CONFIG_ARCH_MXC) += imx93-tqma9352-mba93xxca.dtb
>>>>   dtb-$(CONFIG_ARCH_MXC) += imx93-tqma9352-mba93xxla.dtb
>>>>   diff --git a/arch/arm64/boot/dts/freescale/imx93-phyboard-segin.dts
>>>> b/arch/arm64/boot/dts/freescale/imx93-phyboard-segin.dts
>>>> new file mode 100644
>>>> index 000000000000..5433c33d1322
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/freescale/imx93-phyboard-segin.dts
>>>> @@ -0,0 +1,141 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>> +/*
>>>> + * Copyright (C) 2023 PHYTEC Messtechnik GmbH
>>>> + * Author: Wadim Egorov <w.egorov@phytec.de>, Christoph Stoidner
>>>> <c.stoidner@phytec.de>
>>>> + * Copyright (C) 2024 Mathieu Othacehe <m.othacehe@gmail.com>
>>>> + *
>>>> + * Product homepage:
>>>> + * phyBOARD-Segin carrier board is reused for the i.MX93 design.
>>>> + *
>>>> https://www.phytec.de/produkte/single-board-computer/phyboard-segin-imx6ul/
>>>>
>>>> + */
>>>> +
>>>> +#include "imx93-phycore-som.dtsi"
>>>> +
>>>> +/{
>>>> +    model = "PHYTEC phyBOARD-Segin-i.MX93";
>>>> +    compatible = "phytec,imx93-phyboard-segin",
>>>> "phytec,imx93-phycore-som",
>>>> +             "fsl,imx93";
>>>> +
>>>> +    chosen {
>>>> +        stdout-path = &lpuart1;
>>>> +    };
>>>> +
>>>> +    reg_usdhc2_vmmc: regulator-usdhc2 {
>>>> +        compatible = "regulator-fixed";
>>>> +        enable-active-high;
>>>> +        gpio = <&gpio3 7 GPIO_ACTIVE_HIGH>;
>>>> +        pinctrl-names = "default";
>>>> +        pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>;
>>>> +        regulator-min-microvolt = <3300000>;
>>>> +        regulator-max-microvolt = <3300000>;
>>>> +        regulator-name = "VCC_SD";
>>>> +    };
>>>> +};
>>>> +
>>>> +/* GPIOs */
>>>> +&gpio1 {
>>>> +    pinctrl-names = "default";
>>>> +    pinctrl-0 = <&pinctrl_gpio1>;
>>>
>>> You are doing more than you describing in your changes log.
>>> Here you are forcing a gpio-only functionality for the X16 header. But
>>> the pins we route down to the X16 expansion connector can be also used
>>> differently.
>>
>> i think the word "forcing" is little bit hard in this case. It doesn't
>> define a gpio-hog.
>
> You are defaulting it to be a GPIO.
Sure, but i still cannot see the problem. Are you concerned about
hardware damage, different behavior in comparison to your downstream BSP
or overwriting the bootloader defaults?
>
>>
>>> Typically we provide device tree overlays for different use cases on
>>> this expansion connectors.
>>
>> Can you please explain why the device tree overlays cannot overwrite the
>> pinmuxing?
>
> It can, and it should. Thats why I mentioned to use different overlays
> for different use cases.
> I think it is nicer to have a board only defining it's static components.
Yes and i would consider the line names as static and board specific.
> At this point we do not know what users will use the expansion
> connector for.
> Adding this kind of functionality with overlays follows the idea of
> defining components where they are actually used/implemented: soc,
> som/board level.
> You can find a few of the adapters we provide as dtsi files in
>   arch/arm/boot/dts/nxp/imx/*peb*
> Nowadays we have overlays and can use them instead.
>
>
>>
>>>
>>> Please drop the muxing.
>>>
>>> Same applies for the gpio names.
>> What's the problem with defining gpio line names for user friendliness?
>> The Raspberry Pi has also an expansion header, all the pins can be muxed
>> to different functions but still have gpio line names.
>
> This may cause confusion if you use overlays defining other
> functionalities as the names you define.
I agree most of the line names on the Raspberry Pi contains a function,
which wasn't the best idea for an expansion header. But this doesn't
mean we must do this here, too.

I just want to give you feedback from my point of view as a user. I
would expect that the gpio line names are defined regardless of the used
overlay.

But at the end it's your product.
>
> Regards,
> Wadim
>
>
>>
>> Best regards