| Message ID | 20240726065012.618606-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ARM: dts: imx53-qsb: Add MCIMX-LVDS1 display module support | expand |
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@nxp.com> > --- > 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? > > 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 > 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. > + 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 >
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@nxp.com> >> --- >> 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. 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 >> >
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@nxp.com> >>> --- >>> 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 >>> >> >
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@nxp.com> >>>> --- >>>> 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 >>>> >>> >> >
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 { 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 { + 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>; + }; + }; + }; +};
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@nxp.com> --- 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. 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