On 07/29/2024, Liu Ying wrote: > Hi Dmitry, > > On 07/27/2024, Dmitry Baryshkov wrote: >> On Fri, Jul 26, 2024 at 02:50:12PM GMT, Liu Ying wrote: >>> MCIMX-LVDS1[1] display module integrates a HannStar HSD100PXN1 LVDS >>> display panel and a touch IC. Add an overlay to support the LVDS >>> panel on i.MX53 QSB / QSRB platforms. >>> >>> [1] https://www.nxp.com/part/MCIMX-LVDS1 >>> >>> Signed-off-by: Liu Ying <victor.liu@xxxxxxx> >>> --- >>> I mark RFC in patch subject prefix because if the DT overlay is used, both ldb >>> and panel devices end up as devices deferred. However, if the DT overlay is >>> not used and the devices are defined in imx53-qsb-common.dtsi, then they can be >>> probed ok. >>> >>> With a dev_err_probe() added to imx_ldb_probe() in imx-ldb.c, devices_deferred >>> indicates 53fa8008.ldb and panel-lvds kind of depend on each other. >>> >>> root@imx53qsb:~# cat /sys/kernel/debug/devices_deferred >>> 53fa8008.ldb imx-ldb: failed to find panel or bridge for channel0 >>> panel-lvds platform: wait for supplier /soc/bus@50000000/ldb@53fa8008/lvds-channel@0 >>> >>> It looks like the issue is related to fw_devlink, because if "fw_devlink=off" >>> is added to kernel bootup command line, then the issue doesn't happen. >> >> Could you please fdtdump /sys/firmware/fdt (or just generated DTB files) >> in both cases and compare the dumps for sensible differences? > > I fdtdump imx53-qsrb-mcimx-lvds1.dtb and imx53-qsrb.dtb. > > I see three sensible differences. > 1) panel-lvds node position. > For imx53-qsrb-mcimx-lvds1.dtb, it comes very early and is next to > 'compatible = "fsl,imx53-qsrb", "fsl,imx53";'. > For imx53-qsrb.dtb, it comes later and is next to panel node in '/' node. It turns out only 1) panel-lvds node position matters. I can reproduce the issue with imx53-qsrb.dtb(no DT overlay) if I put the panel-lvds node before the soc node. If the panel-lvds node is after the soc node, then the issue doesn't happen with imx53-qsrb.dtb. The ldb node(LVDS display bridge) and IPU(display controller) node are in the soc node. Maybe, the order of the ldb node and the panel-lvds node in DT blob matters(be my guess). > > 2) properties order in panel-lvds node. > For imx53-qsrb-mcimx-lvds1.dtb, it shows > panel-lvds { > power-supply = <0x0000001c>; > backlight = <0x00000030>; > compatible = "hannstar,hsd100pxn1"; > port { > endpoint { > phandle = <0x0000007d>; > remote-endpoint = <0x0000007c>; > }; > }; > }; > For imx53-qsrb.dtb, it shows > panel-lvds { > compatible = "hannstar,hsd100pxn1"; > backlight = <0x00000031>; > power-supply = <0x0000001d>; > port { > endpoint { > remote-endpoint = <0x00000033>; > phandle = <0x00000017>; > }; > }; > }; > > 3) No 'lvds0_out' and 'panel_lvds_in' in __symbols__ node for > imx53-qsrb-mcimx-lvds1.dtb, but for imx53-qsrb.dtb they are in it. > lvds0_out = "/soc/bus@50000000/ldb@53fa8008/lvds-channel@0/port@2/endpoint"; > panel_lvds_in = "/panel-lvds/port/endpoint"; > > BTW, reverting Saravana's commits > 7cb50f6c9fba ("of: property: fw_devlink: Fix stupid bug in remote-endpoint parsing") > and/or > 7fddac12c382 ("driver core: Fix device_link_flag_is_sync_state_only()") > avoids the issue from happening. > >> >>> >>> Saravana, DT folks, any ideas? >>> >>> Thanks. >>> >>> arch/arm/boot/dts/nxp/imx/Makefile | 4 ++ >>> .../boot/dts/nxp/imx/imx53-qsb-common.dtsi | 4 +- >>> .../dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso | 43 +++++++++++++++++++ >>> 3 files changed, 49 insertions(+), 2 deletions(-) >>> create mode 100644 arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso >>> >>> diff --git a/arch/arm/boot/dts/nxp/imx/Makefile b/arch/arm/boot/dts/nxp/imx/Makefile >>> index 92e291603ea1..7116889e1515 100644 >>> --- a/arch/arm/boot/dts/nxp/imx/Makefile >>> +++ b/arch/arm/boot/dts/nxp/imx/Makefile >>> @@ -46,8 +46,10 @@ dtb-$(CONFIG_SOC_IMX53) += \ >>> imx53-ppd.dtb \ >>> imx53-qsb.dtb \ >>> imx53-qsb-hdmi.dtb \ >>> + imx53-qsb-mcimx-lvds1.dtb \ >>> imx53-qsrb.dtb \ >>> imx53-qsrb-hdmi.dtb \ >>> + imx53-qsrb-mcimx-lvds1.dtb \ >>> imx53-sk-imx53.dtb \ >>> imx53-sk-imx53-atm0700d4-lvds.dtb \ >>> imx53-sk-imx53-atm0700d4-rgb.dtb \ >>> @@ -57,7 +59,9 @@ dtb-$(CONFIG_SOC_IMX53) += \ >>> imx53-usbarmory.dtb \ >>> imx53-voipac-bsb.dtb >>> imx53-qsb-hdmi-dtbs := imx53-qsb.dtb imx53-qsb-hdmi.dtbo >>> +imx53-qsb-mcimx-lvds1-dtbs := imx53-qsb.dtb imx53-qsb-mcimx-lvds1.dtbo >>> imx53-qsrb-hdmi-dtbs := imx53-qsrb.dtb imx53-qsb-hdmi.dtbo >>> +imx53-qsrb-mcimx-lvds1-dtbs := imx53-qsrb.dtb imx53-qsb-mcimx-lvds1.dtbo >>> dtb-$(CONFIG_SOC_IMX6Q) += \ >>> imx6dl-alti6p.dtb \ >>> imx6dl-apf6dev.dtb \ >>> diff --git a/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi b/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi >>> index 05d7a462ea25..430792a91ccf 100644 >>> --- a/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi >>> +++ b/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi >>> @@ -16,7 +16,7 @@ memory@70000000 { >>> <0xb0000000 0x20000000>; >>> }; >>> >>> - backlight_parallel: backlight-parallel { >>> + backlight: backlight { >> >> Nit: this seems unrelated to the LVDS support > > Do you suggest to do this in a separate patch? > If yes, is it worth adding a Fixes tag? > >> >>> compatible = "pwm-backlight"; >>> pwms = <&pwm2 0 5000000 0>; >>> brightness-levels = <0 4 8 16 32 64 128 255>; >>> @@ -89,7 +89,7 @@ panel_dpi: panel { >>> compatible = "sii,43wvf1g"; >>> pinctrl-names = "default"; >>> pinctrl-0 = <&pinctrl_display_power>; >>> - backlight = <&backlight_parallel>; >>> + backlight = <&backlight>; >>> enable-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; >>> >>> port { >>> diff --git a/arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso b/arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso >>> new file mode 100644 >>> index 000000000000..27f6bedf3d39 >>> --- /dev/null >>> +++ b/arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso >>> @@ -0,0 +1,43 @@ >>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >>> +/* >>> + * Copyright 2024 NXP >>> + */ >>> + >>> +/dts-v1/; >>> +/plugin/; >>> + >>> +&{/} { >>> + panel-lvds { >> >> Nit: Just 'panel' should be enough. > > Nope. > > 'panel-lvds' is needed to differentiate it from 'panel' in > imx53-qsb-common.dtsi which is a DPI panel. > > Using 'panel-lvds', procfs lists exactly the properties needed. > root@imx53qsb:~# ls /proc/device-tree/panel-lvds/ > backlight compatible name port power-supply > > Using 'panel', more are listed. > root@imx53qsb:~# ls /proc/device-tree/panel/ > backlight compatible enable-gpios name phandle pinctrl-0 pinctrl-names port power-supply > >> >>> + compatible = "hannstar,hsd100pxn1"; >>> + backlight = <&backlight>; >>> + power-supply = <®_3p2v>; >>> + >>> + port { >>> + panel_lvds_in: endpoint { >>> + remote-endpoint = <&lvds0_out>; >>> + }; >>> + }; >>> + }; >>> +}; >>> + >>> +&ldb { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + status = "okay"; >>> + >>> + lvds-channel@0 { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + fsl,data-mapping = "spwg"; >>> + fsl,data-width = <18>; >>> + status = "okay"; >>> + >>> + port@2 { >>> + reg = <2>; >>> + >>> + lvds0_out: endpoint { >>> + remote-endpoint = <&panel_lvds_in>; >>> + }; >>> + }; >>> + }; >>> +}; >>> -- >>> 2.34.1 >>> >> > -- Regards, Liu Ying
