| Message ID | 8eb6b7ec908337cc5ee6dbe78b47bff40c922be3.1602664950.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | serial: pl01x: Add error value checking | expand |
On Wed, 14 Oct 2020 at 02:42, Michal Simek <michal.simek@xilinx.com> wrote: > > There also a need to check return values to make sure that clocks were > enabled and setup properly. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > drivers/serial/serial_pl01x.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On 14/10/2020 09:42, Michal Simek wrote: Hi, > There also a need to check return values to make sure that clocks were > enabled and setup properly. is that just clean-up or is there a particular problem that's fixed? I am asking because I am not sure how useful debug output in a console driver is. Also in some cases the UART is actually already configured, so failure to get the clock (due to a sloppy/too complicated DTB) is not really fatal at the moment. I am not against this patch, just wanted to make sure we don't break anything. Cheers, Andre > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > drivers/serial/serial_pl01x.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c > index 2772c25f1d2d..8672095ea4de 100644 > --- a/drivers/serial/serial_pl01x.c > +++ b/drivers/serial/serial_pl01x.c > @@ -362,8 +362,18 @@ int pl01x_serial_ofdata_to_platdata(struct udevice *dev) > plat->clock = dev_read_u32_default(dev, "clock", CONFIG_PL011_CLOCK); > ret = clk_get_by_index(dev, 0, &clk); > if (!ret) { > - clk_enable(&clk); > + ret = clk_enable(&clk); > + if (ret && ret != -ENOSYS) { > + dev_err(dev, "failed to enable clock\n"); > + return ret; > + } > + > plat->clock = clk_get_rate(&clk); > + if (IS_ERR_VALUE(plat->clock)) { > + dev_err(dev, "failed to get rate\n"); > + return plat->clock; > + } > + debug("%s: CLK %d\n", __func__, plat->clock); > } > plat->type = dev_get_driver_data(dev); > plat->skip_init = dev_read_bool(dev, "skip-init"); >
Hi, On 15. 10. 20 17:09, André Przywara wrote: > On 14/10/2020 09:42, Michal Simek wrote: > > Hi, > >> There also a need to check return values to make sure that clocks were >> enabled and setup properly. > > is that just clean-up or is there a particular problem that's fixed? > > I am asking because I am not sure how useful debug output in a console > driver is. > Also in some cases the UART is actually already configured, so failure > to get the clock (due to a sloppy/too complicated DTB) is not really > fatal at the moment. > > I am not against this patch, just wanted to make sure we don't break > anything. We have created the patch long time ago and it was also sent to mailing list in October 2018. https://github.com/Xilinx/u-boot-xlnx/commit/7c7266d3d058f4e5d99ad7449eddf6b7e9ff238d#diff-375baede8682cf017faf02039e060958d6574db7900775e7d6f17219bd4d0ecb More or less the most of the part of it where already added by different people. The only missing part was checking error values. There are 2 reasons behind this patch. 1. it is good practice to check all return values - and coverity also reports it. 2. at this time clock driver is up and running and if not you will get errors. In our case we are asking firmware for doing that action/returning value which needs to be catch. And early console can be configured properly when DEBUG console is used that's why debug message will be shown over this interface and it makes sense to show message about it. And if you don't failed you can break regular console by configuring by incorrect values. Thanks, Michal
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index 2772c25f1d2d..8672095ea4de 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -362,8 +362,18 @@ int pl01x_serial_ofdata_to_platdata(struct udevice *dev) plat->clock = dev_read_u32_default(dev, "clock", CONFIG_PL011_CLOCK); ret = clk_get_by_index(dev, 0, &clk); if (!ret) { - clk_enable(&clk); + ret = clk_enable(&clk); + if (ret && ret != -ENOSYS) { + dev_err(dev, "failed to enable clock\n"); + return ret; + } + plat->clock = clk_get_rate(&clk); + if (IS_ERR_VALUE(plat->clock)) { + dev_err(dev, "failed to get rate\n"); + return plat->clock; + } + debug("%s: CLK %d\n", __func__, plat->clock); } plat->type = dev_get_driver_data(dev); plat->skip_init = dev_read_bool(dev, "skip-init");
There also a need to check return values to make sure that clocks were enabled and setup properly. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- drivers/serial/serial_pl01x.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)