Hi Paul,
On 29 January 2016 at 09:09, Paul Burton paul.burton@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@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.
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