On Fri, Jan 11, 2019 at 10:03 AM Alexey Brodkin <alexey.brod...@synopsys.com> wrote: > > Hi Simon, > > > -----Original Message----- > > From: Simon Goldschmidt [mailto:simon.k.r.goldschm...@gmail.com] > > Sent: Friday, January 11, 2019 11:41 AM > > To: Alexey Brodkin <alexey.brod...@synopsys.com> > > Cc: Patrice Chotard <patrice.chot...@st.com>; Simon Glass > > <s...@chromium.org>; Anup Patel > > <a...@brainfault.org>; Lokesh Vutla <lokeshvu...@ti.com>; Patrick Delaunay > > <patrick.delau...@st.com>; > > Marek Vasut <marek.va...@gmail.com>; u-boot@lists.denx.de; Álvaro Fernández > > Rojas <nolt...@gmail.com>; > > Ryder Lee <ryder....@mediatek.com>; Vikas Manocha <vikas.mano...@st.com>; > > Alexander Graf > > <ag...@suse.de>; Weijie Gao <weijie....@mediatek.com>; Marek Vasut > > <ma...@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.brod...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot