On Thu, Dec 9, 2021 at 7:18 PM Andre Przywara andre.przywara@arm.com wrote: Hi Andre,

On Thu, 9 Dec 2021 18:50:06 +0330 Javad Rahimi javad321javad@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 ....

Sure, I will change the patch title and will present what exactly this patch is doing and why it should be done.

...

Signed-off-by: Javad Rahimi javad321javad@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)?

Exactly, I will try to fold it into the "state" variable. Because only one bit of this variable is used. Other bit fields are free to be used.

...

} 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?

Right, I missed calling "status_led_init" before for loop. Because I thought "status_led_init" is called before any LED I/O manipulation. If these functions are called without calling "status_led_init" the initialization should be done, However, I think it may be a bad practice to call any I/O handling function without doing some initialization.

...

•         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?

Same as above :)

Cheers, Andre

...

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