Patchwork [U-Boot,v2,1/2] mx6sabresd: Avoid hang when HDMI cable is not connected

login
register
mail settings
Submitter Fabio Estevam
Date Sept. 11, 2013, 9:14 p.m.
Message ID <1378934070-11721-1-git-send-email-festevam@gmail.com>
Download mbox | patch
Permalink /patch/274368/
State Awaiting Upstream
Delegated to: Stefano Babic
Headers show

Comments

Fabio Estevam - Sept. 11, 2013, 9:14 p.m.
From: Fabio Estevam <fabio.estevam@freescale.com>

Since commit d9b894603 (mx6sabresd: Add LVDS splash screen support) the
following hang happens if the HDMI cable is not connected or the 'panel'
variable is not set:

U-Boot 2013.10-rc2-12978-g47ac53d-dirty (Sep 11 2013 - 15:07:38)               
                                                                               
CPU:   Freescale i.MX6Q rev1.2 at 792 MHz                                      
Reset cause: POR                                                               
Board: MX6-SabreSD                                                             
DRAM:  1 GiB                                                                   
MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
...

Provide a check to 'dev->detect' in order to prevent the hang.

Reported-by: Pardeep Kumar Singla <b45784@freescale.com>
Suggested-by: Eric Bénard <eric@eukrea.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Check dev->detect

 board/freescale/mx6sabresd/mx6sabresd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Stefano Babic - Sept. 12, 2013, 9:47 a.m.
Hi Fabio,

CC to Eric and Troy.

On 11/09/2013 23:14, Fabio Estevam wrote:

> diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
> index c832bd9..0f91fe2 100644
> --- a/board/freescale/mx6sabresd/mx6sabresd.c
> +++ b/board/freescale/mx6sabresd/mx6sabresd.c
> @@ -313,7 +313,7 @@ int board_video_skip(void)
>  	if (!panel) {
>  		for (i = 0; i < ARRAY_SIZE(displays); i++) {
>  			struct display_info_t const *dev = displays+i;
> -			if (dev->detect(dev)) {
> +			if (dev->detect && dev->detect(dev)) {

The same should happen on the Nitrogen board. Should this fix extended
to the other boards using hdmi ?

Best regards,
Stefano
Eric Nelson - Sept. 12, 2013, 3:02 p.m.
Thanks Stefano,

On 09/12/2013 02:47 AM, Stefano Babic wrote:
> Hi Fabio,
>
> CC to Eric and Troy.
>
> On 11/09/2013 23:14, Fabio Estevam wrote:
>
>> diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
>> index c832bd9..0f91fe2 100644
>> --- a/board/freescale/mx6sabresd/mx6sabresd.c
>> +++ b/board/freescale/mx6sabresd/mx6sabresd.c
>> @@ -313,7 +313,7 @@ int board_video_skip(void)
>>   	if (!panel) {
>>   		for (i = 0; i < ARRAY_SIZE(displays); i++) {
>>   			struct display_info_t const *dev = displays+i;
>> -			if (dev->detect(dev)) {
>> +			if (dev->detect && dev->detect(dev)) {
>
> The same should happen on the Nitrogen board. Should this fix extended
> to the other boards using hdmi ?
>

This isn't needed yet in the stock (main-line) code base, since
we haven't added any panels without detection.

We do have a form this patch and a lot of panels in our local tree
on Github, but wanted to avoid unnecessary noise on the list
because we've integrated a dozen or so panels and the existing
structure really doesn't scale.

We had a separate discussion regarding treating the displays
as data (environment), but have stalled somewhat on that front.

The current device-tree code for i.MX6 uses mode strings instead
of the detailed timing data that's really needed for a proper
solution.

Regards,


Eric
Stefano Babic - Sept. 13, 2013, 8:16 a.m.
Hi Eric,

On 12/09/2013 17:02, Eric Nelson wrote:

> This isn't needed yet in the stock (main-line) code base, since
> we haven't added any panels without detection.

ok, thanks - I will then apply Fabio's patches for the Freescale's boards.

> 
> We do have a form this patch and a lot of panels in our local tree
> on Github, but wanted to avoid unnecessary noise on the list
> because we've integrated a dozen or so panels and the existing
> structure really doesn't scale.
> 
> We had a separate discussion regarding treating the displays
> as data (environment), but have stalled somewhat on that front.

I think also that using the environment seems a clumsy solution. We used
a lot of times in the past, and we have not unified their usage.

> 
> The current device-tree code for i.MX6 uses mode strings instead
> of the detailed timing data that's really needed for a proper
> solution.
> 

I admit that using DT also for u-boot seems a better solution. You're
right about i.MX6 in kernel, but on the other hand I like how it is
described for i.MX28 boards. The display-timings node contain all
information we need. It would be nice to have the same for i.MX6.

Best regards,
Stefano
Fabio Estevam - Sept. 13, 2013, 1:58 p.m.
Hi Stefano,

On Fri, Sep 13, 2013 at 5:16 AM, Stefano Babic <sbabic@denx.de> wrote:
> Hi Eric,
>
> On 12/09/2013 17:02, Eric Nelson wrote:
>
>> This isn't needed yet in the stock (main-line) code base, since
>> we haven't added any panels without detection.
>
> ok, thanks - I will then apply Fabio's patches for the Freescale's boards.

Cool, thanks.

> I admit that using DT also for u-boot seems a better solution. You're
> right about i.MX6 in kernel, but on the other hand I like how it is
> described for i.MX28 boards. The display-timings node contain all
> information we need. It would be nice to have the same for i.MX6.

We currently have the same in DT for mx6 as well. Check this commit,
for example:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/boot/dts/imx6q-sabrelite.dts?id=722cfacd2e719b6447fb4bd5cd3372725876336d

Regards,

Fabio Estevam
Stefano Babic - Sept. 13, 2013, 2:19 p.m.
Hi Fabio,

On 13/09/2013 15:58, Fabio Estevam wrote:
> We currently have the same in DT for mx6 as well. Check this commit,
> for example:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/boot/dts/imx6q-sabrelite.dts?id=722cfacd2e719b6447fb4bd5cd3372725876336d

Cool, thanks for the hint !

Regards,
Stefano Babic
Eric Nelson - Sept. 13, 2013, 2:51 p.m.
Hi Fabio,

On 09/13/2013 06:58 AM, Fabio Estevam wrote:
> Hi Stefano,
>
> On Fri, Sep 13, 2013 at 5:16 AM, Stefano Babic <sbabic@denx.de> wrote:
>> I admit that using DT also for u-boot seems a better solution. You're
>> right about i.MX6 in kernel, but on the other hand I like how it is
>> described for i.MX28 boards. The display-timings node contain all
>> information we need. It would be nice to have the same for i.MX6.
>
> We currently have the same in DT for mx6 as well. Check this commit,
> for example:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/boot/dts/imx6q-sabrelite.dts?id=722cfacd2e719b6447fb4bd5cd3372725876336d
>

Nice!

So, how should we get this done?

We have support for parsing individual lines of DT, and the primary
thing(s) needed by U-Boot's display are in the timing block of the
device tree, but I wonder whether it makes sense to implement a
full parser for that, or a simpler parser for fb_videomode.

Since both cfb_console in U-Boot and the of_get_fb_videomode()
routine in the kernel use fb_videomode, it should be straightforward
to hand the information off.

I don't think the outer information (the lvds-channel@0 and
ldb blocks in imx6q-sabrelite.dts) should be parsed by U-Boot
as device-tree code since that path leads to having a full
device-tree compiler that seems inappropriate.

Let me know your thoughts.


Eric
Fabio Estevam - Sept. 13, 2013, 6:11 p.m.
Hi Eric,

On Fri, Sep 13, 2013 at 11:51 AM, Eric Nelson
<eric.nelson@boundarydevices.com> wrote:

> Nice!
>
> So, how should we get this done?
>
> We have support for parsing individual lines of DT, and the primary
> thing(s) needed by U-Boot's display are in the timing block of the
> device tree, but I wonder whether it makes sense to implement a
> full parser for that, or a simpler parser for fb_videomode.

Looks like the parser for fb_videomode is enough.

I am not familiar with the usage of device tree in U-boot yet.

How do other SoC families handle the video display-timings in dt
currently in U-boot?

Regards,

Fabio Estevam
Eric Nelson - Sept. 13, 2013, 6:34 p.m.
Hi Fabio,

On 09/13/2013 11:11 AM, Fabio Estevam wrote:
> Hi Eric,
>
> On Fri, Sep 13, 2013 at 11:51 AM, Eric Nelson
> <eric.nelson@boundarydevices.com> wrote:
>
>> Nice!
>>
>> So, how should we get this done?
>>
>> We have support for parsing individual lines of DT, and the primary
>> thing(s) needed by U-Boot's display are in the timing block of the
>> device tree, but I wonder whether it makes sense to implement a
>> full parser for that, or a simpler parser for fb_videomode.
>
> Looks like the parser for fb_videomode is enough.
>
> I am not familiar with the usage of device tree in U-boot yet.
>
> How do other SoC families handle the video display-timings in dt
> currently in U-boot?
>

AFAIK, with a full device-tree blob, but without any means of
updating it, which ends up in the same place as the compiled-in
settings.

In order to configure for a new kernel, you have to "compile"
the DTB and save it to the boot media.

This is a perfectly reasonable thing to do late in the development
process, but makes connecting new panels cumbersome.

Regards,


Eric
Stefano Babic - Sept. 20, 2013, 4:03 p.m.
On 11/09/2013 23:14, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Since commit d9b894603 (mx6sabresd: Add LVDS splash screen support) the
> following hang happens if the HDMI cable is not connected or the 'panel'
> variable is not set:
> 
> U-Boot 2013.10-rc2-12978-g47ac53d-dirty (Sep 11 2013 - 15:07:38)               
>                                                                                
> CPU:   Freescale i.MX6Q rev1.2 at 792 MHz                                      
> Reset cause: POR                                                               
> Board: MX6-SabreSD                                                             
> DRAM:  1 GiB                                                                   
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
> ...
> 
> Provide a check to 'dev->detect' in order to prevent the hang.
> 
> Reported-by: Pardeep Kumar Singla <b45784@freescale.com>
> Suggested-by: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---

Applied to u-boot-imx, thanks!

Best regards,
Stefano Babic

Patch

diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
index c832bd9..0f91fe2 100644
--- a/board/freescale/mx6sabresd/mx6sabresd.c
+++ b/board/freescale/mx6sabresd/mx6sabresd.c
@@ -313,7 +313,7 @@  int board_video_skip(void)
 	if (!panel) {
 		for (i = 0; i < ARRAY_SIZE(displays); i++) {
 			struct display_info_t const *dev = displays+i;
-			if (dev->detect(dev)) {
+			if (dev->detect && dev->detect(dev)) {
 				panel = dev->mode.name;
 				printf("auto-detected panel %s\n", panel);
 				break;