[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Alexey Brodkin]

Hi Simon,

> -----Original Message-----
> From: Simon Goldschmidt [mailto:simon.k.r.goldschmidt at gmail.com]
> Sent: Friday, January 11, 2019 11:41 AM
> To: Alexey Brodkin <alexey.brodkin at synopsys.com>
> Cc: Patrice Chotard <patrice.chotard at st.com>; Simon Glass <sjg at chromium.org>; Anup Patel
> <anup at brainfault.org>; Lokesh Vutla <lokeshvutla at ti.com>; Patrick Delaunay <patrick.delaunay at st.com>;
> Marek Vasut <marek.vasut at gmail.com>; u-boot at lists.denx.de; Álvaro Fernández Rojas <noltari at gmail.com>;
> Ryder Lee <ryder.lee at mediatek.com>; Vikas Manocha <vikas.manocha at st.com>; Alexander Graf
> <agraf at suse.de>; Weijie Gao <weijie.gao at mediatek.com>; Marek Vasut <marex at denx.de>
> Subject: Re: [PATCH v1 3/4] serial: add an of-platdata driver for "snps,dw-apb-uart"
> 
> On Fri, Jan 11, 2019 at 9:33 AM Alexey Brodkin
> <alexey.brodkin at synopsys.com> wrote:
> >
> > Hi Simon,
> >
> > [snip]
> >
> > > >> +config DESIGNWARE_SERIAL
> > > >> +  bool "DesignWare UART support"
> > > >> +  depends on DM_SERIAL && SPL_OF_PLATDATA
> > > >
> > > > Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> > > > What about CONFIG_OF_EMBED?
> >
> > Ok I completely forgot that standard ns16550 driver already covers DW APB UART,
> > see https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_u-
> 2Dboot_latest_source_drivers_serial_ns16550.c-
> 23L456&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=wyhoU5_3rGv5y
> 373_cpetmUHK9_ALMCC29QnKS2As5k&s=uMDuaOZ__YoyqakveKqByAtb1lfRQBYktcVgGH3oawE&e=
> >
> > > > I'd happily switch my ARC boards on this driver and get rid of all
> > > > CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h
> > >
> > > I checked include/configs/socfpga_common.h again and by its using
> > > Kconfig and DM_SERIAL, there are no CONFIG_SYS_NS16550_xxx defines left
> > > (other than CONFIG_SYS_NS16550_SERIAL, which I will remove).
> >
> > So it's not that easy apparently :)
> > At least CONFIG_SYS_NS16550_MEM32 is still used really required
> > for getting correct accessor used, see implementation of
> > serial_{in|out}_shift() in drivers/serial/ns16550.c.
> >
> > If CONFIG_SYS_NS16550_MEM32 is not set then simple readb() is used so
> > that 8-bit data offset in 32-bit word is lost and we're dead in the water.
> 
> Isn't that covered by 'plat->reg_shift' which is read from dts property
> "reg-shift = <2>"?

Well actually CONFIG_SYS_NS16550_MEM32 is an alias to "reg-io-width = <4>"
which selects exactly accessor (right as CONFIG_SYS_NS16550_MEM32 in
serial_{in|out}_shift()). But even though we do read "reg-io-width"
from .dtb it is not used in the driver. It was added by Andy Shevchenko
recently, see http://git.denx.de/?p=u-boot.git;a=commit;h=4e7207791c05b3ecff916c1b1b1b21401ec29b0b

BTW as I may see some SoCFPGA configs do mention this define:
---------------------------------->8-----------------------------
# git grep CONFIG_SYS_NS16550_MEM32 | grep socfpga
include/configs/socfpga_arria10_socdk.h:33:#define CONFIG_SYS_NS16550_MEM32
include/configs/socfpga_stratix10_socdk.h:146:#define CONFIG_SYS_NS16550_MEM32
---------------------------------->8-----------------------------

as well as this DT property:
---------------------------------->8-----------------------------
# git grep reg-io-width | grep socfpga
arch/arm/dts/socfpga.dtsi:877:                  reg-io-width = <4>;
arch/arm/dts/socfpga.dtsi:889:                  reg-io-width = <4>;
arch/arm/dts/socfpga_arria10.dtsi:829:                  reg-io-width = <4>;
arch/arm/dts/socfpga_arria10.dtsi:840:                  reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:252:                        reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:265:                        reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:314:                        reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:326:                        reg-io-width = <4>;
---------------------------------->8-----------------------------

So I guess you may experiment with it a bit.

-Alexey