Hi Thomas, On 18 September 2015 at 00:23, Thomas Chou <tho...@wytron.com.tw> wrote: > > Convert altera_jtag_uart to driver model. > > Signed-off-by: Thomas Chou <tho...@wytron.com.tw> > --- > > v2 > add ioremap. > make the change to dts compatible with linux. > > v3 > use fdt address translation patch from Stefan Roese. > fix watchdog and loop as Marek suggested. > > v4 > add clear AC flag to probe(). > remove polling loops and watchdog reset because they are > done in the serial-uclass.c. > > v5 > fix coding style as Marek suggested to altera_uart. > > arch/nios2/dts/3c120_devboard.dts | 5 ++ > configs/nios2-generic_defconfig | 3 + > drivers/serial/Kconfig | 13 ++++ > drivers/serial/altera_jtag_uart.c | 145 > ++++++++++++++++++++++---------------- > include/configs/nios2-generic.h | 3 - > 5 files changed, 106 insertions(+), 63 deletions(-)
This looks right to me but I have some comments below. Reviewed-by: Simon Glass <s...@chromium.org> > > diff --git a/arch/nios2/dts/3c120_devboard.dts > b/arch/nios2/dts/3c120_devboard.dts > index 02524ab..7f76328 100644 > --- a/arch/nios2/dts/3c120_devboard.dts > +++ b/arch/nios2/dts/3c120_devboard.dts > @@ -93,6 +93,7 @@ > reg = <0x00004d50 0x00000008>; > interrupt-parent = <&cpu>; > interrupts = <1>; > + u-boot,dm-pre-reloc; > }; > > tse_mac: ethernet@0x4000 { > @@ -147,6 +148,10 @@ > }; > }; > > + aliases { > + console = &jtag_uart; The normal way is to put this into the 'chosen' node, with the property name stdout-path. > + }; > + > chosen { > bootargs = "debug console=ttyJ0,115200"; > }; > diff --git a/configs/nios2-generic_defconfig b/configs/nios2-generic_defconfig > index 9c1bec5..9dc6a72 100644 > --- a/configs/nios2-generic_defconfig > +++ b/configs/nios2-generic_defconfig > @@ -1,4 +1,5 @@ > CONFIG_NIOS2=y > +CONFIG_DM_SERIAL=y > CONFIG_TARGET_NIOS2_GENERIC=y > CONFIG_DEFAULT_DEVICE_TREE="3c120_devboard" > CONFIG_HUSH_PARSER=y > @@ -14,3 +15,5 @@ CONFIG_CMD_PING=y > CONFIG_OF_CONTROL=y > CONFIG_NET_RANDOM_ETHADDR=y > CONFIG_DM=y > +CONFIG_ALTERA_JTAG_UART=y > +CONFIG_ALTERA_JTAG_UART_BYPASS=y > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig > index ccb80d2..5a8cb3a 100644 > --- a/drivers/serial/Kconfig > +++ b/drivers/serial/Kconfig > @@ -109,6 +109,19 @@ config DEBUG_UART_SHIFT > value. Use this value to specify the shift to use, where 0=byte > registers, 2=32-bit word registers, etc. > > +config ALTERA_JTAG_UART > + bool "Altera JTAG UART support" > + depends on NIOS2 && DM_SERIAL > + help > + Select this to enable an JTAG UART for Altera devices. a JTAG UART Is there anything else to say about it? Perhaps how it works? A link to instructions on how to set it up? > + > +config ALTERA_JTAG_UART_BYPASS > + bool "Bypass output when no connection" > + depends on ALTERA_JTAG_UART > + help > + Bypass console output and keep going even if there is no > + JTAG terminal connection with the host. > + > config ROCKCHIP_SERIAL > bool "Rockchip on-chip UART support" > depends on ARCH_UNIPHIER && DM_SERIAL > diff --git a/drivers/serial/altera_jtag_uart.c > b/drivers/serial/altera_jtag_uart.c > index 9a81402..2c025bc 100644 > --- a/drivers/serial/altera_jtag_uart.c > +++ b/drivers/serial/altera_jtag_uart.c > @@ -6,98 +6,123 @@ > */ > > #include <common.h> > -#include <watchdog.h> > +#include <dm.h> > +#include <errno.h> > #include <asm/io.h> > #include <linux/compiler.h> > #include <serial.h> > > -typedef volatile struct { > - unsigned data; /* Data register */ > - unsigned control; /* Control register */ > -} nios_jtag_t; > +struct altera_jtaguart_regs { > + u32 data; /* Data register */ > + u32 control; /* Control register */ > +}; > + > +struct altera_jtaguart_platdata { > + struct altera_jtaguart_regs *reg; > +}; > > /* data register */ > #define NIOS_JTAG_RVALID (1<<15) /* Read valid */ > -#define NIOS_JTAG_DATA(d) ((d)&0x0ff) /* Read data */ > -#define NIOS_JTAG_RAVAIL(d) ((d)>>16) /* Read space avail */ > > /* control register */ > -#define NIOS_JTAG_RE (1 << 0) /* read intr enable */ > -#define NIOS_JTAG_WE (1 << 1) /* write intr enable */ > -#define NIOS_JTAG_RI (1 << 8) /* read intr pending */ > -#define NIOS_JTAG_WI (1 << 9) /* write intr pending*/ > #define NIOS_JTAG_AC (1 << 10) /* activity indicator */ > #define NIOS_JTAG_RRDY (1 << 12) /* read available */ > #define NIOS_JTAG_WSPACE(d) ((d)>>16) /* Write space avail */ > +/* Write fifo size. FIXME: this should be extracted with sopc2dts */ > +#define NIOS_JTAG_WRITE_DEPTH 64 > > DECLARE_GLOBAL_DATA_PTR; > > -/*------------------------------------------------------------------ > - * JTAG acts as the serial port > - *-----------------------------------------------------------------*/ > -static nios_jtag_t *jtag = (nios_jtag_t *)CONFIG_SYS_NIOS_CONSOLE; > - > -static void altera_jtag_serial_setbrg(void) > -{ > -} > - > -static int altera_jtag_serial_init(void) > +static int altera_jtaguart_setbrg(struct udevice *dev, int baudrate) > { > return 0; > } > > -static void altera_jtag_serial_putc(char c) > +static int altera_jtaguart_putc(struct udevice *dev, const char c) While you are here, how abouve 'ch' instead of 'c' as it is easier to spot. > { > - while (1) { > - unsigned st = readl(&jtag->control); > - if (NIOS_JTAG_WSPACE(st)) > - break; > + struct altera_jtaguart_platdata *plat = dev->platdata; > + struct altera_jtaguart_regs *const regs = plat->reg; > + u32 st = readl(®s->control); > + > #ifdef CONFIG_ALTERA_JTAG_UART_BYPASS It seems odd that you return this error when writing a character. Don't you know whether the connection is valid in the probe() method? Or are you trying to avoid an error at that stage? > - if (!(st & NIOS_JTAG_AC)) /* no connection */ > - return; > + if (!(st & NIOS_JTAG_AC)) /* no connection */ > + return -ENETUNREACH; > #endif > - WATCHDOG_RESET(); > - } > - writel ((unsigned char)c, &jtag->data); > + > + if (NIOS_JTAG_WSPACE(st) == 0) > + return -EAGAIN; > + > + writel(c, ®s->data); > + > + return 0; > } > > -static int altera_jtag_serial_tstc(void) > +static int altera_jtaguart_pending(struct udevice *dev, bool input) > { > - return ( readl (&jtag->control) & NIOS_JTAG_RRDY); > + struct altera_jtaguart_platdata *plat = dev->platdata; > + struct altera_jtaguart_regs *const regs = plat->reg; > + u32 st = readl(®s->control); > + > + if (input) > + return st & NIOS_JTAG_RRDY; I think this should be: return st & NIOS_JTAG_RRDY ? 1 : 0; > + else > + return !(NIOS_JTAG_WSPACE(st) == NIOS_JTAG_WRITE_DEPTH); > } > > -static int altera_jtag_serial_getc(void) > +static int altera_jtaguart_getc(struct udevice *dev) > { > - int c; > - unsigned val; > - > - while (1) { > - WATCHDOG_RESET (); > - val = readl (&jtag->data); > - if (val & NIOS_JTAG_RVALID) > - break; > - } > - c = val & 0x0ff; > - return (c); > -} > + struct altera_jtaguart_platdata *plat = dev->platdata; > + struct altera_jtaguart_regs *const regs = plat->reg; > + u32 val; > > -static struct serial_device altera_jtag_serial_drv = { > - .name = "altera_jtag_uart", > - .start = altera_jtag_serial_init, > - .stop = NULL, > - .setbrg = altera_jtag_serial_setbrg, > - .putc = altera_jtag_serial_putc, > - .puts = default_serial_puts, > - .getc = altera_jtag_serial_getc, > - .tstc = altera_jtag_serial_tstc, > -}; > + val = readl(®s->data); > + > + if (!(val & NIOS_JTAG_RVALID)) > + return -EAGAIN; > > -void altera_jtag_serial_initialize(void) > + return val & 0xff; > +} > + > +static int altera_jtaguart_probe(struct udevice *dev) > { > - serial_register(&altera_jtag_serial_drv); > + struct altera_jtaguart_platdata *plat = dev->platdata; > + struct altera_jtaguart_regs *const regs = plat->reg; > + > +#ifdef CONFIG_ALTERA_JTAG_UART_BYPASS > + writel(NIOS_JTAG_AC, ®s->control); /* clear AC flag */ > +#endif > + return 0; > } > > -__weak struct serial_device *default_serial_console(void) > +static int altera_jtaguart_ofdata_to_platdata(struct udevice *dev) > { > - return &altera_jtag_serial_drv; > + struct altera_jtaguart_platdata *plat = dev_get_platdata(dev); > + > + plat->reg = ioremap(dev_get_addr(dev), > + sizeof(struct altera_jtaguart_regs)); What is the ioremap() for? Is this because we don't support ranges properly yet? If so, perhaps we can assume that will be fixed before this is applied? > + > + return 0; > } > + > +static const struct dm_serial_ops altera_jtaguart_ops = { > + .putc = altera_jtaguart_putc, > + .pending = altera_jtaguart_pending, > + .getc = altera_jtaguart_getc, > + .setbrg = altera_jtaguart_setbrg, > +}; > + > +static const struct udevice_id altera_jtaguart_ids[] = { > + { .compatible = "altr,juart-1.0", }, > + { } > +}; > + > +U_BOOT_DRIVER(altera_jtaguart) = { > + .name = "altera_jtaguart", > + .id = UCLASS_SERIAL, > + .of_match = altera_jtaguart_ids, > + .ofdata_to_platdata = altera_jtaguart_ofdata_to_platdata, > + .platdata_auto_alloc_size = sizeof(struct altera_jtaguart_platdata), > + .probe = altera_jtaguart_probe, > + .ops = &altera_jtaguart_ops, > + .flags = DM_FLAG_PRE_RELOC, > +}; > diff --git a/include/configs/nios2-generic.h b/include/configs/nios2-generic.h > index 66ad2f0..bd6d45c 100644 > --- a/include/configs/nios2-generic.h > +++ b/include/configs/nios2-generic.h > @@ -23,14 +23,11 @@ > /* > * SERIAL > */ > -#define CONFIG_ALTERA_JTAG_UART > #if defined(CONFIG_ALTERA_JTAG_UART) > -# define CONFIG_SYS_NIOS_CONSOLE CONFIG_SYS_JTAG_UART_BASE > #else > # define CONFIG_SYS_NIOS_CONSOLE CONFIG_SYS_UART_BASE > #endif > > -#define CONFIG_ALTERA_JTAG_UART_BYPASS > #define CONFIG_SYS_NIOS_FIXEDBAUD > #define CONFIG_BAUDRATE CONFIG_SYS_UART_BAUD > #define CONFIG_SYS_BAUDRATE_TABLE {CONFIG_BAUDRATE} > -- > 2.1.4 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot