[U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT
Simon Glass
sjg at chromium.org
Fri Jan 29 19:23:27 CET 2016
Hi Paul,
On 29 January 2016 at 09:09, Paul Burton <paul.burton at imgtec.com> wrote:
> 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 at 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 at 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.
I think it's actually worse. We are currently able to use the debug
UART without needing the platform data at all. So even my comment is
not correct - the port type should in fact be a parameter to
serial_out().
>
>> > +} 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.
OK. If you want to move it, please do so in a separate patch.
BTW IMO this is one of the worst drivers in U-Boot. I'm looking
forward to when it improves - and I think adding an IO/memory
parameter will point it in the right direction.
Perhaps we should have a flags word which tells serial_out() whether
to use IO/memory, 8-bit or 32-bit, endian, etc? (Not asking for you to
do this, just asking your opinion).
>
> Thanks,
> Paul
>
>> > +
>> > /*
>> > * These are the definitions for the FIFO Control Register
>> > */
>> > --
>> > 2.7.0
>> >
>>
>> Regards,
>> Simon
Regards,
Simon
More information about the U-Boot
mailing list