Hi Jun, On 03/18/2015 04:01 AM, Jun Nie wrote: > 2015-03-14 2:09 GMT+08:00 Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx <mailto:linux@xxxxxxxxxxxxxxxx>>: > > On Fri, Mar 13, 2015 at 02:02:49PM -0400, Peter Hurley wrote: > > On 03/13/2015 01:46 PM, Russell King - ARM Linux wrote: > > > On Fri, Mar 13, 2015 at 01:41:02PM -0400, Peter Hurley wrote: > > >> I think this would be much cleaner keeping the existing register index > > >> symbolic constants and using a remapping LUT with inline i/o accessors. > > >> > > >> Plus it has the advantage of disallowing certain indexes which are not > > >> mapped (rather than an accidental alias). > > >> > > >> I realize that was not the original model set forth but I don't think > > >> the original model contemplated a complete remap. > > > > > > In that case, we will want to ensure that we cache the data register > > > and flag register iomem pointer in loops, so we don't have to > > > constantly reload the base and offsets. > > > > > > This code gets run on some fairly slow ARM systems - including entirely > > > FPGA based systems. > > > > I would the think the compiler would factor ptr calculations outside > > any loop, but maybe not. > > It can't. IO accessors tend to have barriers, possibly calling out > to functions, and the barriers force the compiler to reload almost > everything which is not in a local variable. > > (Note that none of the accessors in amba-pl011.c needs to have these > barriers, and the driver probably should've been converted to the > relaxed accessors when they were merged.) > > Could you help elaborate IO accessors? Any example code for reference? By "i/o accessors", I mean wrapper functions that abstract the register i/o. Generally, UART drivers handle significant model/platform i/o differences with some functional abstraction, although the details vary. Some drivers assign and call through the struct uart_port::serial_in() and ::serial_out() methods, but that's usually because the underlying platform i/o is different (readl() vs. inb()). For this driver, what you outline below seems basically acceptable with the following comments: > I am thinking below style code according to your comments. > > enum pl011_reg { > UART01x_DR/* Data read or written from the interface. */ > UART01x_RSR/* Receive status register (Read). */ > UART01x_ECR= 1;/* Error clear register (Write). */ > UART010_LCRH/* Line control register, high byte. */ > UART010_LCRM/* Line control register, middle byte. */ > ... > }; I don't think it's necessary to re-define the register index symbolic constants; the compiler will constant-fold expressions, even through inline functions. For example, static unsigned int pl011_in(struct uart_port *port, int index) { return readl_relaxed(port->membase + lut[index << 2]; } .... ibrd = pl011_in(uap->port, UART011_IBRD); will be folded as ibrd = readl_relaxed(port->membase + lut[9]; Also, performing i/o with an inline helper function will allow you to range-check the register index. > unsigned int zte_uart[0x23] = { > 0x04,/* UART01x_DR */ > 0x04,/* UART01x_RSR,UART01x_ECR */ > 0x08,/* UART010_LCRH */ > 0x1C/* UART010_LCRM */ > ... > }; > > unsigned int st_uart[0x23] = { > 0x0,/* UART01x_DR */ > 0x04,/* UART01x_RSR,UART01x_ECR */ > 0x08,/* UART010_LCRH */ > 0x1C/* UART010_LCRM*/ > ... > }; > > unsigned int (*reg_lut)[23]; The table could be 'u8' type. This is basically a space/time trade-off. AFAICT, the driver never accesses an offset > 0x48 (ie, index > 0x12), so range-checking the index would allow you to use smaller tables. I would ask Russell which he would prefer. > static int zx_uart_probe(struct platform_device *pdev) > { > ... > reg_lut = zte_uart; > ... > } > > /* Normal usage */ > writew(val, uap->port.membase + reg_lut[UART01x_DR]); > > /* Usage in loop */ > void __iomem *dr = uap->port.membase + reg_lut[UART01x_DR]; > while (..) { > .. > writew(val, dr); > .. > } One issue that arises is how to handle register i/o to unmapped registers. An error message is not possible because of the certainty of deadlock on the port spinlock. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
