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 <[email protected]>
>>>> ---
>>>> 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
