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

[Stefan Wahren <wahrenst@gmx.net>]

Am 24.01.24 um 06:13 schrieb Wadim Egorov:
> 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 :)
Thanks

> Defining line names should be fine. But I would still prefer to have
> the muxing in an overlay bound to a specific use case.
I'm fine with this. Unfortunately Mathieu dropped the line names in V5
today :-(

AFAIR reviewers should have 2 weeks time maximum. This was just 2 days.
>
> Regards,
> Wadim
>
>>
>> But at the end it's your product.
>>>
>>> Regards,
>>> Wadim
>>>
>>>
>>>>
>>>> Best regards
>>