diff mbox series

[RFC] ARM: dts: imx53-qsb: Add MCIMX-LVDS1 display module support

Message ID 20240726065012.618606-1-victor.liu@nxp.com (mailing list archive)
State New
Headers show
Series [RFC] ARM: dts: imx53-qsb: Add MCIMX-LVDS1 display module support | expand

Commit Message

Liu Ying July 26, 2024, 6:50 a.m. UTC
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

Comments

Dmitry Baryshkov July 27, 2024, 11:15 a.m. UTC | #1
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 = <&reg_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
>
Liu Ying July 29, 2024, 4:47 a.m. UTC | #2
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 = <&reg_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
>>
>
Liu Ying July 30, 2024, 2:48 a.m. UTC | #3
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 = <&reg_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
>>>
>>
>
Liu Ying Aug. 13, 2024, 9:31 a.m. UTC | #4
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 = <&reg_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 mbox series

Patch

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 = <&reg_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>;
+			};
+		};
+	};
+};