From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Burton Date: Fri, 29 Jan 2016 16:09:37 +0000 Subject: [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT In-Reply-To: References: <1454075695-31981-1-git-send-email-paul.burton@imgtec.com> <1454075695-31981-5-git-send-email-paul.burton@imgtec.com> Message-ID: <20160129160937.GC3017@NP-P-BURTON> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Fri, Jan 29, 2016 at 07:56:19AM -0700, Simon Glass wrote: > Hi Paul, > > On 29 January 2016 at 06:54, Paul Burton wrote: > > Support making use of I/O accessors for ports when a device tree > > specified an I/O resource. This allows for systems where we have a mix > > of ns16550 ports, some of which should be accessed via I/O ports & some > > of which should be accessed via readb/writeb. > > > > Signed-off-by: Paul Burton > > --- > > > > drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++-------- > > include/ns16550.h | 31 ++++++++++++++++++------------- > > 2 files changed, 56 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > > index 93dad33..b1d5319 100644 > > --- a/drivers/serial/ns16550.c > > +++ b/drivers/serial/ns16550.c > > @@ -11,6 +11,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -102,7 +103,7 @@ static void ns16550_writeb(NS16550_t port, int offset, int value) > > offset *= 1 << plat->reg_shift; > > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > > /* > > - * As far as we know it doesn't make sense to support selection of > > + * For many platforms it doesn't make sense to support selection of > > * these options at run-time, so use the existing CONFIG options. > > */ > > serial_out_shift(addr, plat->reg_shift, value); > > @@ -119,13 +120,33 @@ static int ns16550_readb(NS16550_t port, int offset) > > return serial_in_shift(addr, plat->reg_shift); > > } > > > > +static void ns16550_outb(NS16550_t port, int offset, int value) > > +{ > > + struct ns16550_platdata *plat = port->plat; > > + > > + offset *= 1 << plat->reg_shift; > > + outb(value, plat->base + offset); > > +} > > + > > +static int ns16550_inb(NS16550_t port, int offset) > > +{ > > + struct ns16550_platdata *plat = port->plat; > > + > > + offset *= 1 << plat->reg_shift; > > + return inb(plat->base + offset); > > +} > > + > > /* We can clean these up once everything is moved to driver model */ > > -#define serial_out(value, addr) \ > > - ns16550_writeb(com_port, \ > > - (unsigned char *)addr - (unsigned char *)com_port, value) > > -#define serial_in(addr) \ > > - ns16550_readb(com_port, \ > > - (unsigned char *)addr - (unsigned char *)com_port) > > +#define serial_out(value, addr) do { \ > > + int offset = (unsigned char *)addr - (unsigned char *)com_port; \ > > + com_port->plat->out(com_port, offset, value); \ > > I really don't want to introduce function pointers here. This driver > is a mess but my hope was that when we remove the non-driver-model > code we can clean it up. > > I suggest storing the access method in com_port->plat and then using > if() or switch() to select the right one. We can always add #ifdefs to > keep the code size down if it becomes a problem. Hi Simon, I originally had a field in the platform data & an if statement, but switched to this because it seemed neater & avoids checks on every read or write. I can put it back if you insist, but whilst I agree that the driver needs some cleanup I'm not sure this would qualify. > > +} while (0) > > + > > +#define serial_in(addr) ({ \ > > + int offset = (unsigned char *)addr - (unsigned char *)com_port; \ > > + int value = com_port->plat->in(com_port, offset); \ > > + value; \ > > +}) > > #endif > > > > static inline int calc_divisor(NS16550_t port, int clock, int baudrate) > > @@ -365,9 +386,10 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) > > { > > struct ns16550_platdata *plat = dev->platdata; > > fdt_addr_t addr; > > + unsigned int flags; > > > > /* try Processor Local Bus device first */ > > - addr = dev_get_addr(dev); > > + addr = dev_get_addr_flags(dev, &flags); > > #if defined(CONFIG_PCI) && defined(CONFIG_DM_PCI) > > if (addr == FDT_ADDR_T_NONE) { > > /* then try pci device */ > > @@ -400,6 +422,14 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) > > if (addr == FDT_ADDR_T_NONE) > > return -EINVAL; > > > > + if (flags & IORESOURCE_IO) { > > + plat->in = ns16550_inb; > > + plat->out = ns16550_outb; > > + } else { > > + plat->in = ns16550_readb; > > + plat->out = ns16550_writeb; > > + } > > + > > plat->base = addr; > > plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset, > > "reg-shift", 0); > > diff --git a/include/ns16550.h b/include/ns16550.h > > index 4e62067..097f64b 100644 > > --- a/include/ns16550.h > > +++ b/include/ns16550.h > > @@ -45,19 +45,6 @@ > > unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1]; > > #endif > > > > -/** > > - * struct ns16550_platdata - information about a NS16550 port > > - * > > - * @base: Base register address > > - * @reg_shift: Shift size of registers (0=byte, 1=16bit, 2=32bit...) > > - * @clock: UART base clock speed in Hz > > - */ > > -struct ns16550_platdata { > > - unsigned long base; > > - int reg_shift; > > - int clock; > > -}; > > - > > struct udevice; > > > > struct NS16550 { > > @@ -100,6 +87,24 @@ struct NS16550 { > > > > typedef struct NS16550 *NS16550_t; > > > > +/** > > + * struct ns16550_platdata - information about a NS16550 port > > + * > > + * @base: Base register address > > + * @reg_shift: Shift size of registers (0=byte, 1=16bit, 2=32bit...) > > + * @clock: UART base clock speed in Hz > > + * @is_io: Use I/O, not memory, accessors > > + */ > > +struct ns16550_platdata { > > + unsigned long base; > > + int reg_shift; > > + int clock; > > +#ifdef CONFIG_DM_SERIAL > > + int (*in)(NS16550_t port, int offset); > > + void (*out)(NS16550_t port, int offset, int value); > > +#endif > > +}; > > Does the above need to move? It would reduce the diff if you left it > in the same place. It needs the declaration of NS16550_t. A forward declaration would do I suppose, but it seemed neater to not need it. Thanks, Paul > > + > > /* > > * These are the definitions for the FIFO Control Register > > */ > > -- > > 2.7.0 > > > > Regards, > Simon