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

[Simon Goldschmidt]

On Fri, Jan 11, 2019 at 10:03 AM Alexey Brodkin
<alexey.brodkin at synopsys.com> wrote:
>
> 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-----------------------------

Right. Sorry for being unclear. I'm working on the "sub-mach" "Gen5" only. Those
two are different types that I can't really test. Seems like Arria10
and Stratix10 might
not be converted to DM yet?

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

Those two are of interest to my boards.

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

Ok, you're right there. I guess it just works for me when using 8-bit access,
but you're right that the driver should use 32-bit access when configured like
that via 'reg-io-width'. I'll have a look at that.

Thanks for insisting ;-)

Regards,
Simon