diff mbox series

[v8,02/22] riscv: Fix sifive serial driver

Message ID 20201210140313.258739-3-damien.lemoal@wdc.com
State Superseded
Headers show
Series None | expand

Commit Message

Damien Le Moal Dec. 10, 2020, 2:02 p.m. UTC
Setup the port uartclk in sifive_serial_probe() so that the base baud
rate is correctly printed during device probe instead of always showing
"0".  I.e. the probe message is changed from

38000000.serial: ttySIF0 at MMIO 0x38000000 (irq = 1,
base_baud = 0) is a SiFive UART v0

to the correct:

38000000.serial: ttySIF0 at MMIO 0x38000000 (irq = 1,
base_baud = 115200) is a SiFive UART v0

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/tty/serial/sifive.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Palmer Dabbelt Dec. 11, 2020, 2:09 a.m. UTC | #1
On Thu, 10 Dec 2020 06:02:53 PST (-0800), Damien Le Moal wrote:
> Setup the port uartclk in sifive_serial_probe() so that the base baud
> rate is correctly printed during device probe instead of always showing
> "0".  I.e. the probe message is changed from
>
> 38000000.serial: ttySIF0 at MMIO 0x38000000 (irq = 1,
> base_baud = 0) is a SiFive UART v0
>
> to the correct:
>
> 38000000.serial: ttySIF0 at MMIO 0x38000000 (irq = 1,
> base_baud = 115200) is a SiFive UART v0
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/tty/serial/sifive.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
> index 13eadcb8aec4..214bf3086c68 100644
> --- a/drivers/tty/serial/sifive.c
> +++ b/drivers/tty/serial/sifive.c
> @@ -999,6 +999,7 @@ static int sifive_serial_probe(struct platform_device *pdev)
>  	/* Set up clock divider */
>  	ssp->clkin_rate = clk_get_rate(ssp->clk);
>  	ssp->baud_rate = SIFIVE_DEFAULT_BAUD_RATE;
> +	ssp->port.uartclk = ssp->baud_rate * 16;
>  	__ssp_update_div(ssp);
>
>  	platform_set_drvdata(pdev, ssp);

Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
Acked-by: Palmer Dabbelt <palmerdabbelt@google.com>

As an aside: is this an intrinsic property of being a serial port, or specific
to the SiFive driver?  We seem to multiply/divide by 16 quite a bit to convert
between baud rates and clocks, so if it's specific to SiFive then it seems
reasonable to name that constant in this driver.  If it's a serial thing then I
guess we should just do whatever everyone else is doing.

Don't think that blocks this patch, though, as it's all over the place.
Damien Le Moal Dec. 11, 2020, 4:34 a.m. UTC | #2
On Thu, 2020-12-10 at 18:09 -0800, Palmer Dabbelt wrote:
> On Thu, 10 Dec 2020 06:02:53 PST (-0800), Damien Le Moal wrote:

> > Setup the port uartclk in sifive_serial_probe() so that the base baud

> > rate is correctly printed during device probe instead of always showing

> > "0".  I.e. the probe message is changed from

> > 

> > 38000000.serial: ttySIF0 at MMIO 0x38000000 (irq = 1,

> > base_baud = 0) is a SiFive UART v0

> > 

> > to the correct:

> > 

> > 38000000.serial: ttySIF0 at MMIO 0x38000000 (irq = 1,

> > base_baud = 115200) is a SiFive UART v0

> > 

> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>

> > ---

> >  drivers/tty/serial/sifive.c | 1 +

> >  1 file changed, 1 insertion(+)

> > 

> > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c

> > index 13eadcb8aec4..214bf3086c68 100644

> > --- a/drivers/tty/serial/sifive.c

> > +++ b/drivers/tty/serial/sifive.c

> > @@ -999,6 +999,7 @@ static int sifive_serial_probe(struct platform_device *pdev)

> >  	/* Set up clock divider */

> >  	ssp->clkin_rate = clk_get_rate(ssp->clk);

> >  	ssp->baud_rate = SIFIVE_DEFAULT_BAUD_RATE;

> > +	ssp->port.uartclk = ssp->baud_rate * 16;

> >  	__ssp_update_div(ssp);

> > 

> >  	platform_set_drvdata(pdev, ssp);

> 

> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>

> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com>

> 

> As an aside: is this an intrinsic property of being a serial port, or specific

> to the SiFive driver?  We seem to multiply/divide by 16 quite a bit to convert

> between baud rates and clocks, so if it's specific to SiFive then it seems

> reasonable to name that constant in this driver.  If it's a serial thing then I

> guess we should just do whatever everyone else is doing.


That port.uartclk field is not specific to the sifive driver. And setting its
value correctly lead to the correct message being printed. That message is also
generic and not special to the SiFive serial driver.

> Don't think that blocks this patch, though, as it's all over the place.


I did not dig too far in that driver, I only wanted to fix the message so that
meaningful information is printed. But yes, I also thought that a little
cleanup would be good. The DT binding doc is also incomplete as it is missing
the "sifive,fu540-c000-uart0" compatible string.


-- 
Damien Le Moal
Western Digital
diff mbox series

Patch

diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
index 13eadcb8aec4..214bf3086c68 100644
--- a/drivers/tty/serial/sifive.c
+++ b/drivers/tty/serial/sifive.c
@@ -999,6 +999,7 @@  static int sifive_serial_probe(struct platform_device *pdev)
 	/* Set up clock divider */
 	ssp->clkin_rate = clk_get_rate(ssp->clk);
 	ssp->baud_rate = SIFIVE_DEFAULT_BAUD_RATE;
+	ssp->port.uartclk = ssp->baud_rate * 16;
 	__ssp_update_div(ssp);
 
 	platform_set_drvdata(pdev, ssp);