[PATCH 3/9] rockchip: veyron: Add logging for power init

[Simon Glass]

HI Quentin,

On Wed, 5 Jun 2024 at 02:36, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> On 6/5/24 5:25 AM, Simon Glass wrote:
> > Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can
> > be enabled.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >   board/google/veyron/veyron.c | 27 ++++++++++++---------------
> >   1 file changed, 12 insertions(+), 15 deletions(-)
> >
> > diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c
> > index 32dbcdc4d10..23fe8bf088c 100644
> > --- a/board/google/veyron/veyron.c
> > +++ b/board/google/veyron/veyron.c
> > @@ -29,44 +29,41 @@ static int veyron_init(void)
> >       int ret;
> >
> >       ret = regulator_get_by_platname("vdd_arm", &dev);
> > -     if (ret) {
> > -             debug("Cannot set regulator name\n");
> > -             return ret;
> > -     }
> > +     if (ret)
> > +             return log_msg_ret("vdd", ret);
> >
>
> Those log messages aren't for code in SPL as far as I could tell, is
> there any reason to make them that small/cryptic?

Oh yes, it happens early in U-Boot proper before any messages which is
probably what confused me.

Re size, they just need to support searching the code base. Long
strings mean that many boards fail to boot / hit their limits when
CONFIG_LOG_ERROR_RETURN is enabled. So I try to keep them to 3
characters.

>
> >       /* Slowly raise to max CPU voltage to prevent overshoot */
> >       ret = regulator_set_value(dev, 1200000);
> >       if (ret)
> > -             return ret;
> > +             return log_msg_ret("s12", ret);
> >       udelay(175); /* Must wait for voltage to stabilize, 2mV/us */
> >       ret = regulator_set_value(dev, 1400000);
> >       if (ret)
> > -             return ret;
> > +             return log_msg_ret("s14", ret);
> >       udelay(100); /* Must wait for voltage to stabilize, 2mV/us */
> >
> >       ret = rockchip_get_clk(&clk.dev);
> >       if (ret)
> > -             return ret;
> > +             return log_msg_ret("clk", ret);
> >       clk.id = PLL_APLL;
> >       ret = clk_set_rate(&clk, 1800000000);
> >       if (IS_ERR_VALUE(ret))
> > -             return ret;
> > +             return log_msg_ret("s18", ret);
> >
> >       ret = regulator_get_by_platname("vcc33_sd", &dev);
> >       if (ret) {
> >               debug("Cannot get regulator name\n");
> > -             return ret;
> > +             if (ret)
> > +                     return log_msg_ret("vcc", ret);
>
> I think you can just merge the debug and log_msg_ret here?

They are actually different. The debug message is how I actually found
this problem.

>
> Otherwise looking good to me,
>
> Cheers,
> Quentin

Regards,
Simon