On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote: > > >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > > >> > index 28da9dd..e58e6aa 100644 > > >> > --- a/drivers/serial/ns16550.c > > >> > +++ b/drivers/serial/ns16550.c > > >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int > > >> > offset, int value) > > >> > unsigned char *addr; > > >> > > > >> > offset *= 1 << plat->reg_shift; > > >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > > >> > + addr = (unsigned char *)plat->base + offset; > > >> > +#else > > >> > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > > >> > +#endif > > >> > > >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs > > >> to be another parameter? Possibly a flag. But with driver-model we > > >> need to be able to support both options in the core code. > > > > > > Hi Simon, > > > > > > Are you sure systems rely on using I/O ports with map_physmem? The only > > > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, > > > in include/configs/x86-common.h, and so far as I can tell they don't use > > > device model which suggests this code has simply been untested before. I > > > don't see why you would use map_physmem on an I/O port address that is > > > then going to be passed to inb/outb & I think the code here is simply > > > wrong to do so. > > > > the current code looks wrong. serial_in_shift() is expanded to inb() > > in case of CONFIG_SYS_NS16550_PORT_MAPPED and to > > in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case > > a map_physmem() is required and should be done in serial_in_shift() > > itself or preferrably only once in > > ns16550_serial_ofdata_to_platdata(). > > > > I think the correct approach would be the following: > > This is better I think. But how about adding a device tree binding to > select I/O access? In principle each device might have its own > settings.
Note that's what I worked towards last time I had a crack at this, but it just expanded into an attempt to tackle the mess that is ns16550.c & rather lost sight of the original goal of making Malta work. https://patchwork.ozlabs.org/patch/575643/ https://patchwork.ozlabs.org/patch/577194/ Thanks, Paul _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
