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

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: "palmer@dabbelt.com" <palmer@dabbelt.com>
Cc: "linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"sboyd@kernel.org" <sboyd@kernel.org>,
	"seanga2@gmail.com" <seanga2@gmail.com>,
	"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Subject: Re: [PATCH v8 02/22] riscv: Fix sifive serial driver
Date: Fri, 11 Dec 2020 04:34:51 +0000	[thread overview]
Message-ID: <3c3238a00beaab166a67e3baf0ad4714d3c90ef1.camel@wdc.com> (raw)
In-Reply-To: <mhng-b369aaef-f262-4ca3-bcda-5bd4ee893332@palmerdabbelt-glaptop1>

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