Messages in this thread | | | Date | Wed, 24 Jan 2024 12:39:26 +0100 | Subject | Re: [PATCH v4 3/3] arm64: dts: imx93: Add phyBOARD-Segin-i.MX93 support | From | Stefan Wahren <> |
| |
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 >>
| |