| Message ID | 20240605032521.1142768-4-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Bug-fixes for a few boards | expand |
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? > /* 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? Otherwise looking good to me, Cheers, Quentin
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
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@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. > 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
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); /* 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); } ret = regulator_set_value(dev, 3300000); if (ret) - return ret; + return log_msg_ret("s33", ret); ret = regulators_enable_boot_on(false); - if (ret) { - debug("%s: Cannot enable boot on regulators\n", __func__); - return ret; - } + if (ret) + return log_msg_ret("boo", ret); return 0; } @@ -81,7 +78,7 @@ int board_early_init_r(void) if (!fdt_node_check_compatible(gd->fdt_blob, 0, "google,veyron")) { ret = veyron_init(); if (ret) - return ret; + return log_msg_ret("vey", ret); } #endif /*
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(-)