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

[Anup Patel]

> -----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>

Regards,
Anup