| Message ID | 20190118111820.71349-10-anup.patel@wdc.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | SiFive FU540 Support | expand |
Hi Anup, On Fri, 2019-01-18 at 11:19 +0000, Anup Patel wrote: > From: Atish Patra <atish.patra@wdc.com> > > It is possible that input clock is not available because clk > device was not available and 'clock-frequency' DT property is > also not available. Why would the clock device not be available? I suspect the problem is that the clock driver is not probed pre- relocation. Adding DM_FLAG_PRE_RELOC to the driver flags would fix this. > > In this case, instead of failing we should just skip baudrate > config by returning zero. Won't this cause issues if an incorrect baudrate is set? Thanks, Lukas > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > Signed-off-by: Anup Patel <anup.patel@wdc.com> > Reviewed-by: Alexander Graf <agraf@suse.de> > --- > drivers/serial/serial_sifive.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/serial/serial_sifive.c > b/drivers/serial/serial_sifive.c > index ea4d35d48c..537bc7a975 100644 > --- a/drivers/serial/serial_sifive.c > +++ b/drivers/serial/serial_sifive.c > @@ -99,27 +99,27 @@ static int _sifive_serial_getc(struct uart_sifive > *regs) > > static int sifive_serial_setbrg(struct udevice *dev, int baudrate) > { > - int err; > + int ret; > struct clk clk; > struct sifive_uart_platdata *platdata = dev_get_platdata(dev); > + u32 clock = 0; > > - err = clk_get_by_index(dev, 0, &clk); > - if (!err) { > - err = clk_get_rate(&clk); > - if (!IS_ERR_VALUE(err)) > - platdata->clock = err; > - } else if (err != -ENOENT && err != -ENODEV && err != -ENOSYS) > { > + ret = clk_get_by_index(dev, 0, &clk); > + if (IS_ERR_VALUE(ret)) { > debug("SiFive UART failed to get clock\n"); > - return err; > - } > - > - if (!platdata->clock) > - platdata->clock = dev_read_u32_default(dev, "clock- > frequency", 0); > - if (!platdata->clock) { > - debug("SiFive UART clock not defined\n"); > - return -EINVAL; > + ret = dev_read_u32(dev, "clock-frequency", &clock); > + if (IS_ERR_VALUE(ret)) { > + debug("SiFive UART clock not defined\n"); > + return 0; > + } > + } else { > + clock = clk_get_rate(&clk); > + if (IS_ERR_VALUE(clock)) { > + debug("SiFive UART clock get rate failed\n"); > + return 0; > + } > } > - > + platdata->clock = clock; > _sifive_serial_setbrg(platdata->regs, platdata->clock, > baudrate); > > return 0;
On 1/20/19 12:22 PM, Auer, Lukas wrote: > Hi Anup, > > On Fri, 2019-01-18 at 11:19 +0000, Anup Patel wrote: >> From: Atish Patra <atish.patra@wdc.com> >> >> It is possible that input clock is not available because clk >> device was not available and 'clock-frequency' DT property is >> also not available. > > Why would the clock device not be available? > I suspect the problem is that the clock driver is not probed pre- > relocation. Adding DM_FLAG_PRE_RELOC to the driver flags would fix > this. > The problem is serial driver gets probed first before clock driver even if we add DM_FLAG_PRE_RELOC. >> >> In this case, instead of failing we should just skip baudrate >> config by returning zero. > > Won't this cause issues if an incorrect baudrate is set? > It might be possible that baudrate is configured by earlier boot stage. Thus, any early information can be displayed in console by the serial driver even if it is not configured correctly by u-boot. Regards, Atish > Thanks, > Lukas > >> >> Signed-off-by: Atish Patra <atish.patra@wdc.com> >> Signed-off-by: Anup Patel <anup.patel@wdc.com> >> Reviewed-by: Alexander Graf <agraf@suse.de> >> --- >> drivers/serial/serial_sifive.c | 32 ++++++++++++++++---------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/serial/serial_sifive.c >> b/drivers/serial/serial_sifive.c >> index ea4d35d48c..537bc7a975 100644 >> --- a/drivers/serial/serial_sifive.c >> +++ b/drivers/serial/serial_sifive.c >> @@ -99,27 +99,27 @@ static int _sifive_serial_getc(struct uart_sifive >> *regs) >> >> static int sifive_serial_setbrg(struct udevice *dev, int baudrate) >> { >> - int err; >> + int ret; >> struct clk clk; >> struct sifive_uart_platdata *platdata = dev_get_platdata(dev); >> + u32 clock = 0; >> >> - err = clk_get_by_index(dev, 0, &clk); >> - if (!err) { >> - err = clk_get_rate(&clk); >> - if (!IS_ERR_VALUE(err)) >> - platdata->clock = err; >> - } else if (err != -ENOENT && err != -ENODEV && err != -ENOSYS) >> { >> + ret = clk_get_by_index(dev, 0, &clk); >> + if (IS_ERR_VALUE(ret)) { >> debug("SiFive UART failed to get clock\n"); >> - return err; >> - } >> - >> - if (!platdata->clock) >> - platdata->clock = dev_read_u32_default(dev, "clock- >> frequency", 0); >> - if (!platdata->clock) { >> - debug("SiFive UART clock not defined\n"); >> - return -EINVAL; >> + ret = dev_read_u32(dev, "clock-frequency", &clock); >> + if (IS_ERR_VALUE(ret)) { >> + debug("SiFive UART clock not defined\n"); >> + return 0; >> + } >> + } else { >> + clock = clk_get_rate(&clk); >> + if (IS_ERR_VALUE(clock)) { >> + debug("SiFive UART clock get rate failed\n"); >> + return 0; >> + } >> } >> - >> + platdata->clock = clock; >> _sifive_serial_setbrg(platdata->regs, platdata->clock, >> baudrate); >> >> return 0; >
On Sun, 2019-01-20 at 17:07 -0800, Atish Patra wrote: > On 1/20/19 12:22 PM, Auer, Lukas wrote: > > Hi Anup, > > > > On Fri, 2019-01-18 at 11:19 +0000, Anup Patel wrote: > > > From: Atish Patra <atish.patra@wdc.com> > > > > > > It is possible that input clock is not available because clk > > > device was not available and 'clock-frequency' DT property is > > > also not available. > > > > Why would the clock device not be available? > > I suspect the problem is that the clock driver is not probed pre- > > relocation. Adding DM_FLAG_PRE_RELOC to the driver flags would fix > > this. > > > > The problem is serial driver gets probed first before clock driver > even > if we add DM_FLAG_PRE_RELOC. That makes sense. I just browsed through the code to see what other boards do here. The serial driver for the MPC83xx SoC series directly probes the clock driver (see get_serial_clock in driver/clk/mpc83xx_clk.c called from the serial driver). Maybe we should do something similar here? > > > > In this case, instead of failing we should just skip baudrate > > > config by returning zero. > > > > Won't this cause issues if an incorrect baudrate is set? > > > It might be possible that baudrate is configured by earlier boot > stage. > Thus, any early information can be displayed in console by the > serial > driver even if it is not configured correctly by u-boot. > Yes, it is very likely not going to be a problem, but if we can fix it it would be great :) Thanks, Lukas > > Regards, > Atish > > Thanks, > > Lukas > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > > Reviewed-by: Alexander Graf <agraf@suse.de> > > > --- > > > drivers/serial/serial_sifive.c | 32 ++++++++++++++++----------- > > > ----- > > > 1 file changed, 16 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/serial/serial_sifive.c > > > b/drivers/serial/serial_sifive.c > > > index ea4d35d48c..537bc7a975 100644 > > > --- a/drivers/serial/serial_sifive.c > > > +++ b/drivers/serial/serial_sifive.c > > > @@ -99,27 +99,27 @@ static int _sifive_serial_getc(struct > > > uart_sifive > > > *regs) > > > > > > static int sifive_serial_setbrg(struct udevice *dev, int > > > baudrate) > > > { > > > - int err; > > > + int ret; > > > struct clk clk; > > > struct sifive_uart_platdata *platdata = > > > dev_get_platdata(dev); > > > + u32 clock = 0; > > > > > > - err = clk_get_by_index(dev, 0, &clk); > > > - if (!err) { > > > - err = clk_get_rate(&clk); > > > - if (!IS_ERR_VALUE(err)) > > > - platdata->clock = err; > > > - } else if (err != -ENOENT && err != -ENODEV && err != -ENOSYS) > > > { > > > + ret = clk_get_by_index(dev, 0, &clk); > > > + if (IS_ERR_VALUE(ret)) { > > > debug("SiFive UART failed to get clock\n"); > > > - return err; > > > - } > > > - > > > - if (!platdata->clock) > > > - platdata->clock = dev_read_u32_default(dev, "clock- > > > frequency", 0); > > > - if (!platdata->clock) { > > > - debug("SiFive UART clock not defined\n"); > > > - return -EINVAL; > > > + ret = dev_read_u32(dev, "clock-frequency", &clock); > > > + if (IS_ERR_VALUE(ret)) { > > > + debug("SiFive UART clock not defined\n"); > > > + return 0; > > > + } > > > + } else { > > > + clock = clk_get_rate(&clk); > > > + if (IS_ERR_VALUE(clock)) { > > > + debug("SiFive UART clock get rate failed\n"); > > > + return 0; > > > + } > > > } > > > - > > > + platdata->clock = clock; > > > _sifive_serial_setbrg(platdata->regs, platdata->clock, > > > baudrate); > > > > > > return 0;
On Mon, 2019-01-21 at 12:39 +0000, Auer, Lukas wrote: > On Sun, 2019-01-20 at 17:07 -0800, Atish Patra wrote: > > On 1/20/19 12:22 PM, Auer, Lukas wrote: > > > Hi Anup, > > > > > > On Fri, 2019-01-18 at 11:19 +0000, Anup Patel wrote: > > > > From: Atish Patra <atish.patra@wdc.com> > > > > > > > > It is possible that input clock is not available because clk > > > > device was not available and 'clock-frequency' DT property is > > > > also not available. > > > > > > Why would the clock device not be available? > > > I suspect the problem is that the clock driver is not probed pre- > > > relocation. Adding DM_FLAG_PRE_RELOC to the driver flags would > > > fix > > > this. > > > > > > > The problem is serial driver gets probed first before clock driver > > even > > if we add DM_FLAG_PRE_RELOC. > > That makes sense. I just browsed through the code to see what other > boards do here. The serial driver for the MPC83xx SoC series directly > probes the clock driver (see get_serial_clock in > driver/clk/mpc83xx_clk.c called from the serial driver). Maybe we > should do something similar here? > What are your thoughts on this, can you add something like it? Thanks, Lukas > > > > In this case, instead of failing we should just skip baudrate > > > > config by returning zero. > > > > > > Won't this cause issues if an incorrect baudrate is set? > > > > > It might be possible that baudrate is configured by earlier boot > > stage. > > Thus, any early information can be displayed in console by the > > serial > > driver even if it is not configured correctly by u-boot. > > > > Yes, it is very likely not going to be a problem, but if we can fix > it > it would be great :) > > Thanks, > Lukas > > > Regards, > > Atish > > > Thanks, > > > Lukas > > > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > > > Reviewed-by: Alexander Graf <agraf@suse.de> > > > > --- > > > > drivers/serial/serial_sifive.c | 32 ++++++++++++++++--------- > > > > -- > > > > ----- > > > > 1 file changed, 16 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/serial/serial_sifive.c > > > > b/drivers/serial/serial_sifive.c > > > > index ea4d35d48c..537bc7a975 100644 > > > > --- a/drivers/serial/serial_sifive.c > > > > +++ b/drivers/serial/serial_sifive.c > > > > @@ -99,27 +99,27 @@ static int _sifive_serial_getc(struct > > > > uart_sifive > > > > *regs) > > > > > > > > static int sifive_serial_setbrg(struct udevice *dev, int > > > > baudrate) > > > > { > > > > - int err; > > > > + int ret; > > > > struct clk clk; > > > > struct sifive_uart_platdata *platdata = > > > > dev_get_platdata(dev); > > > > + u32 clock = 0; > > > > > > > > - err = clk_get_by_index(dev, 0, &clk); > > > > - if (!err) { > > > > - err = clk_get_rate(&clk); > > > > - if (!IS_ERR_VALUE(err)) > > > > - platdata->clock = err; > > > > - } else if (err != -ENOENT && err != -ENODEV && err != > > > > -ENOSYS) > > > > { > > > > + ret = clk_get_by_index(dev, 0, &clk); > > > > + if (IS_ERR_VALUE(ret)) { > > > > debug("SiFive UART failed to get clock\n"); > > > > - return err; > > > > - } > > > > - > > > > - if (!platdata->clock) > > > > - platdata->clock = dev_read_u32_default(dev, > > > > "clock- > > > > frequency", 0); > > > > - if (!platdata->clock) { > > > > - debug("SiFive UART clock not defined\n"); > > > > - return -EINVAL; > > > > + ret = dev_read_u32(dev, "clock-frequency", > > > > &clock); > > > > + if (IS_ERR_VALUE(ret)) { > > > > + debug("SiFive UART clock not > > > > defined\n"); > > > > + return 0; > > > > + } > > > > + } else { > > > > + clock = clk_get_rate(&clk); > > > > + if (IS_ERR_VALUE(clock)) { > > > > + debug("SiFive UART clock get rate > > > > failed\n"); > > > > + return 0; > > > > + } > > > > } > > > > - > > > > + platdata->clock = clock; > > > > _sifive_serial_setbrg(platdata->regs, platdata->clock, > > > > baudrate); > > > > > > > > return 0; > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
diff --git a/drivers/serial/serial_sifive.c b/drivers/serial/serial_sifive.c index ea4d35d48c..537bc7a975 100644 --- a/drivers/serial/serial_sifive.c +++ b/drivers/serial/serial_sifive.c @@ -99,27 +99,27 @@ static int _sifive_serial_getc(struct uart_sifive *regs) static int sifive_serial_setbrg(struct udevice *dev, int baudrate) { - int err; + int ret; struct clk clk; struct sifive_uart_platdata *platdata = dev_get_platdata(dev); + u32 clock = 0; - err = clk_get_by_index(dev, 0, &clk); - if (!err) { - err = clk_get_rate(&clk); - if (!IS_ERR_VALUE(err)) - platdata->clock = err; - } else if (err != -ENOENT && err != -ENODEV && err != -ENOSYS) { + ret = clk_get_by_index(dev, 0, &clk); + if (IS_ERR_VALUE(ret)) { debug("SiFive UART failed to get clock\n"); - return err; - } - - if (!platdata->clock) - platdata->clock = dev_read_u32_default(dev, "clock-frequency", 0); - if (!platdata->clock) { - debug("SiFive UART clock not defined\n"); - return -EINVAL; + ret = dev_read_u32(dev, "clock-frequency", &clock); + if (IS_ERR_VALUE(ret)) { + debug("SiFive UART clock not defined\n"); + return 0; + } + } else { + clock = clk_get_rate(&clk); + if (IS_ERR_VALUE(clock)) { + debug("SiFive UART clock get rate failed\n"); + return 0; + } } - + platdata->clock = clock; _sifive_serial_setbrg(platdata->regs, platdata->clock, baudrate); return 0;