[PATCH v1] Cubieboard2:Fix Cubieboard2 freeze, when LED boot enabled

[Andre Przywara]

On Thu,  9 Dec 2021 18:50:06 +0330
Javad Rahimi <javad321javad at gmail.com> wrote:

Hi Javad,

thanks for the patch!

> When enabling the LED support for Cubieboard2 (SUN7I)
> the uboot freezes in status_led_init() function.
> It uses a static global variable, while the .BSS is defined in DRAM area
> while that function is called before DRAM

So I'd say that the commit message and the subject is misleading, it's not
a real fix, because nothing was really broken before. That fact that sunxi
puts the BSS into DRAM, even when this is not available at the very
beginning, is more a nasty workaround than a feature.

So can you please reword the commit message, to focus on what this patch
actually does, and mention that this allow to get rid of a global
variable. And then just briefly mention that it helps sunxi to enable LEDs
early, as a motivation.

Some things below ....

> Signed-off-by: Javad Rahimi <javad321javad at gmail.com>
> ---
> 
>  drivers/misc/status_led.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
> index a6e9c03a02..7d30355423 100644
> --- a/drivers/misc/status_led.c
> +++ b/drivers/misc/status_led.c
> @@ -23,6 +23,7 @@ typedef struct {
>  	int state;
>  	int period;
>  	int cnt;
> +	int initialized:1;

This bitfield doesn't really help here, since the compiler will pad it
with 31 more bits. So just make it a "bool", and let the compiler figure
out what's best here.
Or can you check whether you can fold this bit into the "state" variable?
By introducing something like an "uninitialised" state? Or is there is
another way to detect initialisation (period != 0)?

>  } led_dev_t;
>  
>  led_dev_t led_dev[] = {
> @@ -30,12 +31,14 @@ led_dev_t led_dev[] = {
>  		CONFIG_LED_STATUS_STATE,
>  		LED_STATUS_PERIOD,
>  		0,
> +		0,
>  	},
>  #if defined(CONFIG_LED_STATUS1)
>  	{	CONFIG_LED_STATUS_BIT1,
>  		CONFIG_LED_STATUS_STATE1,
>  		LED_STATUS_PERIOD1,
>  		0,
> +		0,
>  	},
>  #endif
>  #if defined(CONFIG_LED_STATUS2)
> @@ -43,6 +46,7 @@ led_dev_t led_dev[] = {
>  		CONFIG_LED_STATUS_STATE2,
>  		LED_STATUS_PERIOD2,
>  		0,
> +		0,
>  	},
>  #endif
>  #if defined(CONFIG_LED_STATUS3)
> @@ -50,6 +54,7 @@ led_dev_t led_dev[] = {
>  		CONFIG_LED_STATUS_STATE3,
>  		LED_STATUS_PERIOD3,
>  		0,
> +		0,
>  	},
>  #endif
>  #if defined(CONFIG_LED_STATUS4)
> @@ -57,6 +62,7 @@ led_dev_t led_dev[] = {
>  		CONFIG_LED_STATUS_STATE4,
>  		LED_STATUS_PERIOD4,
>  		0,
> +		0,
>  	},
>  #endif
>  #if defined(CONFIG_LED_STATUS5)
> @@ -64,22 +70,26 @@ led_dev_t led_dev[] = {
>  		CONFIG_LED_STATUS_STATE5,
>  		LED_STATUS_PERIOD5,
>  		0,
> +		0,
>  	},
>  #endif
>  };
>  
>  #define MAX_LED_DEV	(sizeof(led_dev)/sizeof(led_dev_t))
>  
> -static int status_led_init_done = 0;
>  
>  void status_led_init(void)
>  {
>  	led_dev_t *ld;
>  	int i;
>  
> -	for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++)
> +	for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++) {
>  		__led_init (ld->mask, ld->state);
> -	status_led_init_done = 1;
> +		ld->initialized = 1;
> +	}
> +
> +	return 0;
> +
>  }
>  
>  void status_led_tick(ulong timestamp)
> @@ -87,11 +97,11 @@ void status_led_tick(ulong timestamp)
>  	led_dev_t *ld;
>  	int i;
>  
> -	if (!status_led_init_done)
> -		status_led_init();
> -
>  	for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++) {
>  
> +		if (!ld->initialized)
> +			continue;

But this is not what the current code does, is it? At the moment a call to
status_led_tick() would initialise all LEDs, now you just skip over (all
of) them?

> +
>  		if (ld->state != CONFIG_LED_STATUS_BLINKING)
>  			continue;
>  
> @@ -110,11 +120,11 @@ void status_led_set(int led, int state)
>  	if (led < 0 || led >= MAX_LED_DEV)
>  		return;
>  
> -	if (!status_led_init_done)
> -		status_led_init();
> -
>  	ld = &led_dev[led];
>  
> +	if (!ld->initialized)
> +		return;

Same here, I think?

Cheers,
Andre

>  	ld->state = state;
>  	if (state == CONFIG_LED_STATUS_BLINKING) {
>  		ld->cnt = 0;		/* always start with full period    */