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

[Quentin Schulz]

Hi Simon,

On 6/6/24 5:04 PM, Simon Glass wrote:
> 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.
> 

git grep vcc/vdd is going to return a lot of matches :)

This is for one board, so there isn't much reason to argue about this, 
so fine :)

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

I meant:

"""
if (ret) {
     debug("Cannot get regulator name\n");
     if (ret)
         return log_msg_ret("vcc", ret);
}
"""

Nothing changes ret before the third line, so at the very least we could 
have:

"""
if (ret) {
     debug("Cannot get regulator name\n");
     return log_msg_ret("vcc", ret);
}
"""

But i was also suggesting we merge this into:

"""
if (ret)
     return log_msg_ret("Cannot get vcc regulator name", ret);
"""

But since you seem to want to keep only a few characters, then just 
removing the debug entirely? Or what's the plan with it? (I see 
log_msg_ret as a debug message, but I'm probably wrong here?).

Cheers,
Quentin