lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <15ff5315-57a3-46ae-9f1d-f707165294e2@nxp.com>
Date: Tue, 30 Jul 2024 10:48:38 +0800
From: Liu Ying <victor.liu@....com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: devicetree@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, shawnguo@...nel.org, s.hauer@...gutronix.de,
 kernel@...gutronix.de, festevam@...il.com, saravanak@...gle.com
Subject: Re: [RFC PATCH] ARM: dts: imx53-qsb: Add MCIMX-LVDS1 display module
 support

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@....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@...53qsb:~# 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@...00000/ldb@...a8008/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@...00000/ldb@...a8008/lvds-channel@...ort@...ndpoint";
> 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@...00000 {
>>>  		      <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@...53qsb:~# ls /proc/device-tree/panel-lvds/
> backlight     compatible    name          port          power-supply
> 
> Using 'panel', more are listed.
> root@...53qsb:~# 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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ