Re: [EXT] Re: [PATCH 2/2] arm64: dts: freescale: add i.MX952 FRDM Board support device tree
Frank Li <[email protected]>
| Newsgroups | dev.linux.lists.imx,dev.linux.lists.sashiko-reviews,org.kernel.vger.linux-devicetree |
|---|---|
| Message-ID | <amE-dICUsWdjZ_Nb@SMW015318> |
On Wed, Jul 22, 2026 at 07:57:58AM +0000, Bough Chen wrote: > > -----Original Message----- > > From: Frank Li <[email protected]> > > Sent: 2026年7月20日 23:07 > > To: Joseph Guo <[email protected]> > > Cc: [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected] > > Subject: Re: [EXT] Re: [PATCH 2/2] arm64: dts: freescale: add i.MX952 FRDM > > Board support device tree > > > > On Mon, Jul 20, 2026 at 03:03:20PM +0800, Joseph Guo wrote: > > > On 7/20/2026 2:55 PM, [email protected] wrote: > > > > [You don't often get email from [email protected]. Learn why > > > > this is important at https://aka.ms/LearnAboutSenderIdentification ] > > > > > > > > Caution: This is an external email. Please take care when clicking > > > > links or opening attachments. When in doubt, report the message > > > > using the 'Report this email' button > > > > > > > > > > > > Thank you for your contribution! Sashiko AI review found 2 potential > > issue(s) to consider: > > > > - [High] Shared GPIO requested exclusively by two CAN transceivers > > causes probe failure for the second transceiver. > > > > - [Low] Use of undocumented, downstream-only device tree property > > `fsl,cd-gpio-wakeup-disable`. > > > > -- > > > > > > > > --- Patch [2]: [PATCH 2/2] arm64: dts: freescale: add i.MX952 FRDM > > > > Board support device tree --- commit > > > > 8bbcb28d08a05e0f4c57023333492b4e023f7caf > > > > Author: Joseph Guo <[email protected]> > > > > > > > > arm64: dts: freescale: add i.MX952 FRDM Board support device tree > > > > > > > > Add basic device tree support for i.MX952 FRDM Board. The > > > > board-common hardware description is placed in imx952-frdm.dtsi so > > > > that it can be shared with future i.MX952-based FRDM derivative boards. > > > > > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx952-frdm.dtsi > > > >> b/arch/arm64/boot/dts/freescale/imx952-frdm.dtsi > > > >> new file mode 100644 > > > >> index 0000000000000..b9ef6bec2bd5a > > > >> --- /dev/null > > > >> +++ b/arch/arm64/boot/dts/freescale/imx952-frdm.dtsi > > > >> @@ -0,0 +1,727 @@ > > > > [ ... ] > > > >> + flexcan2_phy: can-phy0 { > > > >> + compatible = "nxp,tja1051"; > > > >> + #phy-cells = <0>; > > > >> + max-bitrate = <8000000>; > > > >> + silent-gpios = <&pcal6416 6 GPIO_ACTIVE_HIGH>; > > > >> + }; > > > >> + > > > >> + flexcan3_phy: can-phy1 { > > > >> + compatible = "nxp,tja1051"; > > > >> + #phy-cells = <0>; > > > >> + max-bitrate = <8000000>; > > > >> + silent-gpios = <&pcal6416 6 GPIO_ACTIVE_HIGH>; > > > > > > > > [Severity: High] > > > > Does this cause a probe failure for the second transceiver? > > > > > > > > The first PHY node successfully requests this GPIO line, taking > > > > exclusive ownership. Because the phy-can-transceiver driver uses > > > > standard exclusive requests (without GPIOD_FLAGS_BIT_NONEXCLUSIVE), > > > > the second request for the exact same GPIO line fails with -EBUSY. > > > > > > > > This prevents the flexcan3_phy and the flexcan3 controller from > > > > probing and functioning. > > > > > > > > > > This two CAN PHY did share same pin as their silent-gpio on hardware. > > > > Hardware share gpio, but driver may not handle this case. Are you sure > > > > flexcan3_phy probed if can-phy0 already probed? > > > Hi Frank > > I dug into this. Both of you and sashiko are effectively right, but the outcome depends entirely on kernel config, so let me lay out the facts. > > The driver side confirms Frank's/Sashiko's concern. In drivers/phy/phy-can-transceiver.c the silent line is requested exclusively: > > silent_gpio = devm_gpiod_get_index_optional(dev, "silent", i, > GPIOD_OUT_LOW); > No GPIOD_FLAGS_BIT_NONEXCLUSIVE, and tja1051_drvdata carries CAN_TRANSCEIVER_SILENT_PRESENT, so the line really is requested. On a plain build, the second phy referencing the same pin would hit test_and_set_bit(GPIOD_FLAG_REQUESTED) in gpiod_request_commit() and get -EBUSY, failing the second transceiver and its flexcan controller. And NONEXCLUSIVE cannot be set from DT - the DT gpio flags cell only encodes ACTIVE_LOW / OPEN_DRAIN / PULL etc. So DTS tweaks alone cannot fix it. > > Yet on the i.MX95 15x15 FRDM (same design: flexcan2_phy and flexcan5_phy both use silent-gpios = <&pcal6524 7 ...>) I tested it and both can0/can1 probe and pass traffic. The reason is the gpio-shared-proxy framework (drivers/gpio/gpiolib-shared.c, gpio-shared-proxy.c). At postcore_initcall gpiolib scans the DT; when the same controller+offset is referenced by > > 1 consumer via a "*-gpios" property, it claims the real line once (marked "shared") and hands each consumer its own proxy descriptor with vote-based arbitration - so the second probe no longer returns -EBUSY, with no DTS change. > > The important subtlety is how that framework got enabled in my build. It is gated by HAVE_SHARED_GPIOS, and in both your BSP tree(frank.li/linux.git) and current linux-next, the only selector is ARCH_QCOM (commit e511d484cbe4, "arm64: select HAVE_SHARED_GPIOS for ARCH_QCOM"). ARCH_MXC does NOT select it. My kernel has CONFIG_GPIO_SHARED=y because CONFIG_ARCH_QCOM=y is also set (as in the arm64 defconfig), which selects HAVE_SHARED_GPIOS -> GPIO_SHARED=y globally. Since the framework scans the whole DT regardless of SoC, our i.MX CAN nodes benefit from a config that Qualcomm pulled in. It works by coincidence of the multi-platform build, not because i.MX enables it. > > Consequences for upstreaming this DTS: > > Pure i.MX config (ARCH_MXC=y, no ARCH_QCOM): nothing selects HAVE_SHARED_GPIOS, GPIO_SHARED=n, the framework is compiled out, and the second CAN transceiver fails with -EBUSY. This is exactly Sashiko's scenario, and it is real. > > arm64 defconfig (QCOM + i.MX): framework is pulled in by QCOM and it works - what I tested. > > So the DTS is fine as written, but to make the shared silent-gpio robust for any i.MX-only configuration we should send a small prerequisite patch mirroring the Qualcomm one: > > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ config ARCH_MXC > select IMX_GPCV2 > select IMX_GPCV2_PM_DOMAINS > + select HAVE_SHARED_GPIOS > select PM > select PM_GENERIC_DOMAINS > (and CONFIG_GPIO_SHARED_PROXY should be built-in/module in the i.MX defconfig; it is default m). With that in place, the shared silent-gpio in the FRDM DTSI probes correctly regardless of whether ARCH_QCOM is also enabled. If you think this method is okay, then I think Joseph can post this Kconfig patch alongside the board DTS. > Thanks you for detail information. It is quite helpful. Frank > Separately, on the [Low] finding: Joseph, please drop the downstream-only "fsl,cd-gpio-wakeup-disable" from &usdhc2 in the upstream DTSI - there is no upstream binding and no driver consumes it, so it is dead code and would trip make dtbs_check > > Regards > Haibo Chen > > > > > Frank > > > > > > > > The same design on i.MX95 15x15 FRDM. This should be fine. > > > > > > Regards, > > > Joseph > > > > > > >> + }; > > > > [ ... ] > > > >> +&usdhc2 { > > > >> + bus-width = <4>; > > > >> + cd-gpios = <&gpio3 0 GPIO_ACTIVE_LOW>; > > > >> + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > > > >> + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; > > > >> + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; > > > >> + pinctrl-3 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > > > >> + pinctrl-names = "default", "state_100mhz", "state_200mhz", > > "sleep"; > > > >> + vmmc-supply = <®_usdhc2_vmmc>; > > > >> + fsl,cd-gpio-wakeup-disable; > > > > > > > > [Severity: Low] > > > > Is this out-of-tree vendor property invalid in the upstream kernel? > > > > > > > > The property fsl,cd-gpio-wakeup-disable does not exist in any > > > > upstream device tree bindings (such as fsl-imx-esdhc.yaml) and is > > > > not handled by any driver code in the upstream Linux tree. This acts > > > > as dead code and will trigger `make dtbs_check` validation warnings. > > > > > > > >> + status = "okay"; > > > >> +}; > > > > > > > > -- > > > > Sashiko AI review ・ > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsa > > > > > > shiko.dev%2F%23%2Fpatchset%2F20260720-imx952_frdm-v1-0-6d145eaaa5 > > c4% > > > > > > 40nxp.com%3Fpart%3D2&data=05%7C02%7Chaibo.chen%40nxp.com%7C29 > > e9bc792 > > > > > > 4f2455f2cdd08dee67565f4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > > %7C0%7 > > > > > > C639201589065857811%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGk > > iOnRydWUs > > > > > > IlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D > > > > %7C0%7C%7C%7C&sdata=hQfdYq0rrk8kvruJk%2FGc32LxLfMMmRAJn7kG > > bT5BMjA%3D > > > > &reserved=0 > > > > > > -- > > > Best Regards, > > > Joseph >