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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 = <&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
>>>>
>>>
>>
> 

-- 
Regards,
Liu Ying





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux