On 07/30/2024, Liu Ying wrote: > 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. Hi Saravana, It looks like this issue is caused by/related to fw_devlink. Any thoughts please? > > 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
