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

[Wadim Egorov <w.egorov@phytec.de>]

Hi,

Am 23.01.24 um 11:21 schrieb Stefan Wahren:
> 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.

I appreciate the feedback :)
Defining line names should be fine. But I would still prefer to have the 
muxing in an overlay bound to a specific use case.

Regards,
Wadim

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