[PATCH] PinePhone: enable LED on boot for improved visual feedback

[PATCH] PinePhone: enable LED on boot for improved visual feedback

[Arnaud Ferraris]

Hi,

As the PinePhone doesn't provide any visual feedback when booting, these
patches take advantage of the built-in RGB LED to indicate the device
is indeed powered on.

I've been carrying those downstream in Mobian for some time now,
allowing users to see their phone is booting (otherwise it can be
several seconds before the kernel is loaded and able to show a sign of
life).

Best regards,
Arnaud

[PATCH 1/2] board: sunxi: enable status LED early

[Arnaud Ferraris]

For some systems, such as the PinePhone, there is no way for the end
user to make sure the system is indeed booting before the boot script is
executed, which takes several seconds. Therefore, it can be useful to
provide early visual feedback as soon as possible.

In order achieve this goal, this patch initializes the status LED (if
configured) in the SPL.

Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
---
 board/sunxi/board.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 1a46100e40..6e0bf5fbf9 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -46,6 +46,9 @@
 #include <spl.h>
 #include <sy8106a.h>
 #include <asm/setup.h>
+#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)
+#include <status_led.h>
+#endif
 
 #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD)
 /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */
@@ -672,6 +675,10 @@ void sunxi_board_init(void)
 {
 	int power_failed = 0;
 
+#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)
+	status_led_init();
+#endif
+
 #ifdef CONFIG_SY8106A_POWER
 	power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT);
 #endif
-- 
2.33.0

[PATCH 2/2] pinephone_defconfig: add support for early-boot status LED

[Arnaud Ferraris]

From: Arnaud Ferraris <arnaud.ferraris@gmail.com>

This commit enables the green status LED (PD18/GPIO 114) on boot in the
SPL, in order to provide visual feedback that the PinePhone is booting.

Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
---
 configs/pinephone_defconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig
index 64e13d3132..9d39204a43 100644
--- a/configs/pinephone_defconfig
+++ b/configs/pinephone_defconfig
@@ -1,6 +1,7 @@
 CONFIG_ARM=y
 CONFIG_ARCH_SUNXI=y
 CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pinephone-1.2"
+CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL=y
 CONFIG_MACH_SUN50I=y
 CONFIG_SUNXI_DRAM_LPDDR3_STOCK=y
@@ -10,3 +11,8 @@ CONFIG_MMC_SUNXI_SLOT_EXTRA=2
 CONFIG_PINEPHONE_DT_SELECTION=y
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
 CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2"
+CONFIG_LED_STATUS=y
+CONFIG_LED_STATUS_GPIO=y
+CONFIG_LED_STATUS0=y
+CONFIG_LED_STATUS_BIT=114
+CONFIG_LED_STATUS_STATE=2
-- 
2.33.0

Re: [PATCH 2/2] pinephone_defconfig: add support for early-boot status LED

[Andre Przywara]

On Mon,  6 Sep 2021 22:57:53 +0200
Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote:

> From: Arnaud Ferraris <arnaud.ferraris@gmail.com>
> 
> This commit enables the green status LED (PD18/GPIO 114) on boot in the
> SPL, in order to provide visual feedback that the PinePhone is booting.

Looks alright, and while I don't have a Pinephone to test this, I tried
the similar setting on a Pine64-LTS. It should be noted that this
increases the SPL by 364 bytes (in my setup), which kills the Pine H64,
for instance. But it looks fine for A64 boards.
 
> Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  configs/pinephone_defconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig
> index 64e13d3132..9d39204a43 100644
> --- a/configs/pinephone_defconfig
> +++ b/configs/pinephone_defconfig
> @@ -1,6 +1,7 @@
>  CONFIG_ARM=y
>  CONFIG_ARCH_SUNXI=y
>  CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pinephone-1.2"
> +CONFIG_SPL_DRIVERS_MISC=y
>  CONFIG_SPL=y
>  CONFIG_MACH_SUN50I=y
>  CONFIG_SUNXI_DRAM_LPDDR3_STOCK=y
> @@ -10,3 +11,8 @@ CONFIG_MMC_SUNXI_SLOT_EXTRA=2
>  CONFIG_PINEPHONE_DT_SELECTION=y
>  # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>  CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2"
> +CONFIG_LED_STATUS=y
> +CONFIG_LED_STATUS_GPIO=y
> +CONFIG_LED_STATUS0=y
> +CONFIG_LED_STATUS_BIT=114
> +CONFIG_LED_STATUS_STATE=2

Re: [PATCH 1/2] board: sunxi: enable status LED early

[Andre Przywara]

On Mon,  6 Sep 2021 22:57:52 +0200
Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote:

Hi Arnaud,

> For some systems, such as the PinePhone, there is no way for the end
> user to make sure the system is indeed booting before the boot script is
> executed, which takes several seconds. Therefore, it can be useful to
> provide early visual feedback as soon as possible.
> 
> In order achieve this goal, this patch initializes the status LED (if
> configured) in the SPL.

Thanks for sending this, it looks much better than the previous
attempt on the list from earlier this year.


> Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
> ---
>  board/sunxi/board.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 1a46100e40..6e0bf5fbf9 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -46,6 +46,9 @@
>  #include <spl.h>
>  #include <sy8106a.h>
>  #include <asm/setup.h>
> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)
> +#include <status_led.h>

status_led.h already guards for CONFIG_LED_STATUS, so you can just
unconditionally include this here, no #ifdefs needed.

> +#endif
>  
>  #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD)
>  /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */
> @@ -672,6 +675,10 @@ void sunxi_board_init(void)
>  {
>  	int power_failed = 0;
>  
> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)

Unfortunately status_led.h doesn't define a dummy prototype for this,
so we need the #ifdef CONFIG_LED_STATUS. But I don't think you need
CONFIG_SPL_DRIVERS_MISC here. If that should only trigger in the SPL, use:

	if (IS_ENABLED(CONFIG_SPL_BUILD))
		status_led_init();

Cheers,
Andre

> +	status_led_init();
> +#endif
> +
>  #ifdef CONFIG_SY8106A_POWER
>  	power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT);
>  #endif

Re: [PATCH 1/2] board: sunxi: enable status LED early

[Arnaud Ferraris]

Hi André,

Thanks for your feedback!

Le 07/09/2021 à 01:46, Andre Przywara a écrit :
> On Mon,  6 Sep 2021 22:57:52 +0200
> Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote:
> 
> Hi Arnaud,
> 
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 1a46100e40..6e0bf5fbf9 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -46,6 +46,9 @@
>>  #include <spl.h>
>>  #include <sy8106a.h>
>>  #include <asm/setup.h>
>> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)
>> +#include <status_led.h>
> 
> status_led.h already guards for CONFIG_LED_STATUS, so you can just
> unconditionally include this here, no #ifdefs needed.

Understood, will do.

> 
>> +#endif
>>  
>>  #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD)
>>  /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */
>> @@ -672,6 +675,10 @@ void sunxi_board_init(void)
>>  {
>>  	int power_failed = 0;
>>  
>> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)
> 
> Unfortunately status_led.h doesn't define a dummy prototype for this,
> so we need the #ifdef CONFIG_LED_STATUS. But I don't think you need
> CONFIG_SPL_DRIVERS_MISC here. If that should only trigger in the SPL, use:
> 
> 	if (IS_ENABLED(CONFIG_SPL_BUILD))
> 		status_led_init();
> 

Actually the status_led driver isn't compiled into SPL unless
CONFIG_SPL_DRIVERS_MISC is set, resulting in a link error if that's not
the case. We should therefore check for both CONFIG_LED_STATUS and
CONFIG_SPL_DRIVERS_MISC to prevent build errors both at compile time and
link time.

The alternative would be:

#ifdef CONFIG_LED_STATUS
	if (IS_ENABLED(CONFIG_SPL_DRIVERS_MISC))
		status_led_init();
#endif

Which is more or less the same, except that it relies on the compiler
optimizing out this function call if CONFIG_SPL_DRIVERS_MISC isn't
defined. That assumption feels less safe to me, but overall I'm fine
with both implementations.

Please also note the whole sunxi_board_init() function itself is already
inside a #ifdef CONFIG_SPL_BUILD block.

Cheers,
Arnaud

> Cheers,
> Andre
> 
>> +	status_led_init();
>> +#endif
>> +
>>  #ifdef CONFIG_SY8106A_POWER
>>  	power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT);
>>  #endif

Re: [PATCH 1/2] board: sunxi: enable status LED early

[Andre Przywara]

On Tue, 7 Sep 2021 13:41:11 +0200
Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote:

Hi Arnaud,

> Thanks for your feedback!
> 
> Le 07/09/2021 à 01:46, Andre Przywara a écrit :
> > On Mon,  6 Sep 2021 22:57:52 +0200
> > Arnaud Ferraris <arnaud.ferraris@collabora.com> wrote:
> > 
> > Hi Arnaud,
> >   
> >> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> >> index 1a46100e40..6e0bf5fbf9 100644
> >> --- a/board/sunxi/board.c
> >> +++ b/board/sunxi/board.c
> >> @@ -46,6 +46,9 @@
> >>  #include <spl.h>
> >>  #include <sy8106a.h>
> >>  #include <asm/setup.h>
> >> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)
> >> +#include <status_led.h>  
> > 
> > status_led.h already guards for CONFIG_LED_STATUS, so you can just
> > unconditionally include this here, no #ifdefs needed.  
> 
> Understood, will do.
> 
> >   
> >> +#endif
> >>  
> >>  #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD)
> >>  /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */
> >> @@ -672,6 +675,10 @@ void sunxi_board_init(void)
> >>  {
> >>  	int power_failed = 0;
> >>  
> >> +#if defined(CONFIG_LED_STATUS) && defined(CONFIG_SPL_DRIVERS_MISC)  
> > 
> > Unfortunately status_led.h doesn't define a dummy prototype for this,
> > so we need the #ifdef CONFIG_LED_STATUS. But I don't think you need
> > CONFIG_SPL_DRIVERS_MISC here. If that should only trigger in the SPL, use:
> > 
> > 	if (IS_ENABLED(CONFIG_SPL_BUILD))
> > 		status_led_init();
> >   
> 
> Actually the status_led driver isn't compiled into SPL unless
> CONFIG_SPL_DRIVERS_MISC is set, resulting in a link error if that's not
> the case. We should therefore check for both CONFIG_LED_STATUS and
> CONFIG_SPL_DRIVERS_MISC to prevent build errors both at compile time and
> link time.
> 
> The alternative would be:
> 
> #ifdef CONFIG_LED_STATUS
> 	if (IS_ENABLED(CONFIG_SPL_DRIVERS_MISC))
> 		status_led_init();
> #endif

Right, the nasty bit here is that you could just have LED_STATUS for
U-Boot proper, but there is no separate CONFIG_SPL_LED_STATUS, so we
need both symbols, although it looks weird.

> Which is more or less the same, except that it relies on the compiler
> optimizing out this function call if CONFIG_SPL_DRIVERS_MISC isn't
> defined. That assumption feels less safe to me, but overall I'm fine
> with both implementations.

Yes, but I still prefer to have as little #ifdef as possible,
because ...

> Please also note the whole sunxi_board_init() function itself is already
> inside a #ifdef CONFIG_SPL_BUILD block.

You are right, I missed that. That whole file is an #ifdef
nightmare, and especially nested #ifdefs are the worst. So I would like
to not *add* more of them, hence asking for IS_ENABLED() wherever
possible.

Thanks,
Andre

> Cheers,
> Arnaud
> 
> > Cheers,
> > Andre
> >   
> >> +	status_led_init();
> >> +#endif
> >> +
> >>  #ifdef CONFIG_SY8106A_POWER
> >>  	power_failed = sy8106a_set_vout1(CONFIG_SY8106A_VOUT1_VOLT);
> >>  #endif