[PATCH] lib: utils/serial: Support Synopsys DesignWare APB UART

[Anup Patel]

> -----Original Message-----
> From: Anup Patel
> Sent: 19 May 2021 12:05
> To: 'Bin Meng' <bmeng.cn at gmail.com>; Jessica Clarke <jrtc27 at jrtc27.com>
> Cc: OpenSBI <opensbi at lists.infradead.org>
> Subject: RE: [PATCH] lib: utils/serial: Support Synopsys DesignWare APB
> UART
> 
> 
> 
> > -----Original Message-----
> > From: Bin Meng <bmeng.cn at gmail.com>
> > Sent: 14 May 2021 07:48
> > To: Jessica Clarke <jrtc27 at jrtc27.com>
> > Cc: Anup Patel <Anup.Patel at wdc.com>; OpenSBI
> > <opensbi at lists.infradead.org>
> > Subject: Re: [PATCH] lib: utils/serial: Support Synopsys DesignWare
> > APB UART
> >
> > On Fri, May 14, 2021 at 9:59 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> > >
> > > On 14 May 2021, at 02:27, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > On Fri, May 14, 2021 at 9:19 AM Jessica Clarke <jrtc27 at jrtc27.com>
> > wrote:
> > > >>
> > > >> On 14 May 2021, at 02:16, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >>>
> > > >>> Synopsys DesignWare APB UART is seen on the StarFive JH7100 SoC.
> > > >>> Its programming interface is compatible with the existing 8250
> > > >>> UART driver. Simply add its compatible string to the driver
> > > >>> makes it work with the StarFive JH7100 SoC on a BeagleV board.
> > > >>>
> > > >>> With this patch, the generic platform firmware can be used out
> > > >>> of the box on the BeagleV board.
> > > >>>
> > > >>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > >>> ---
> > > >>>
> > > >>> lib/utils/serial/fdt_serial_uart8250.c | 1 +
> > > >>> 1 file changed, 1 insertion(+)
> > > >>>
> > > >>> diff --git a/lib/utils/serial/fdt_serial_uart8250.c
> > > >>> b/lib/utils/serial/fdt_serial_uart8250.c
> > > >>> index 918193a..36f364c 100644
> > > >>> --- a/lib/utils/serial/fdt_serial_uart8250.c
> > > >>> +++ b/lib/utils/serial/fdt_serial_uart8250.c
> > > >>> @@ -28,6 +28,7 @@ static int serial_uart8250_init(void *fdt, int
> > > >>> nodeoff, static const struct fdt_match serial_uart8250_match[] = {
> > > >>>      { .compatible = "ns16550" },
> > > >>>      { .compatible = "ns16550a" },
> > > >>> +     { .compatible = "snps,dw-apb-uart" },
> > > >>
> > > >> If it’s compatible, why does it not use
> > > >>
> > > >>  compatible = “snps,dw-apb-uart”, “ns16550”;
> > > >>
> > > >> or similar in its FDT?
> > > >
> > > > Good question. By looking at the Linux kernel driver, it is
> > > > supported as a variant of 8250 (drivers/tty/serial/8250/8250_dw.c).
> > > >
> > > > I have not looked into details, but I suspect there are some minor
> > > > oddities which matter for the kernel. The existing driver is
> > > > enough to make OpenSBI happy.
> > >
> > > It seems there are various additional clocks and resets you may need
> > > to handle (baudclk is required, apb_pclk is optional and there’s an
> > > optional
> > reset too).
> > > For this particular board the FDT has a them clocks as all
> > > fixed-clock, but in general this is not a correct implementation of
> > > the driver, as the clocks must be enabled when attaching.
> >
> > I don't think OpenSBI needs to handle the clock and reset of each
> > peripheral in its driver. Such should to be done in the bootloader,
> > i.e.: U-Boot SPL.
> 
> Most likely, we will not require to handle clock and reset of each peripheral.
> At least, the previous booting stage should set the "clock-frequency" DT
> property in UART DT node to be used as console.
> 
> Regarding this patch, I am fine with it because the 8250 DT bindings doesn’t
> mandate "ns16550" string to be always there. We lot of
> ARM64 DTS files having just "snps,dw-apb-uart" as compatible string.
> 
> Reviewed-by: Anup Patel <anup.patel at wdc.com>

Applied this patch to the riscv/opensbi repo.

Regards,
Anup

> 
> Regards,
> Anup