[U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT

[Paul Burton <paul.burton@imgtec.com>]

On Fri, Jan 29, 2016 at 07:56:19AM -0700, Simon Glass wrote:
> Hi Paul,
> 
> On 29 January 2016 at 06:54, Paul Burton <paul.burton@imgtec.com> 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 <paul.burton@imgtec.com>
> > ---
> >
> >  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 <ns16550.h>
> >  #include <serial.h>
> >  #include <watchdog.h>
> > +#include <linux/ioport.h>
> >  #include <linux/types.h>
> >  #include <asm/io.h>
> >
> > @@ -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