On 03/18/2015 12:08 PM, Peter Hurley wrote: > 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]; ^^^^^^^^^^ Sorry if this confused you ---------------------------^ it should be: 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
