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

HI Quentin,

On Wed, 5 Jun 2024 at 02:36, Quentin Schulz quentin.schulz@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@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