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

[Javad Rahimi]

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

>
> 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 ....
>
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 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)?
>
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    */
>