| Message ID | 1301088437-31915-1-git-send-email-mathieu.poirier@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 2011-03-25 at 15:27 -0600, mathieu.poirier@linaro.org wrote: > From: Alessandro Rubini <rubini@gnudd.com> > > At least one device I'm using needs address shifting to access > registers in the LAN9221 device (smsc911x driver). This patch > adds a shift parameter in platform_data. The feature must be > enabled at configuration time, because shifting by a pdata > parameter makes access slower in those devices where no shift is > needed (I tested one, it was 20% slower). [...] I guess you are using 16-bit registers, so that FIFO access involves calling smsc911x_reg_{read,write}() in a loop. And the compiler probably can't hoist the address calculation out of the loop due to potential aliasing between the register lock and the platform data (yes, really). Try calculating the FIFO address in a local variable at the top of smsc911x_tx_writefifo() and smsc911x_rx_readfifo() and then using that variable in the calls to smsc911x_reg_{read,write}(). Ben.
David, Please see my comment below. Thanks, Mathieu. On 25 March 2011 15:31, David Miller <davem@davemloft.net> wrote: > From: mathieu.poirier@linaro.org > Date: Fri, 25 Mar 2011 15:27:17 -0600 > > > From: Alessandro Rubini <rubini@gnudd.com> > > > > At least one device I'm using needs address shifting to access > > registers in the LAN9221 device (smsc911x driver). This patch > > adds a shift parameter in platform_data. The feature must be > > enabled at configuration time, because shifting by a pdata > > parameter makes access slower in those devices where no shift is > > needed (I tested one, it was 20% slower). > > > > If the platform data requests shifted access but the feature is > > unavailable, the probe method complains to the console. Board > > config files should set the configuration option when needed. > > > > Signed-off-by: Alessandro Rubini <rubini@gnudd.com> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Implement this at run time, keying off of a boolean or similar > in the platform data. > > Implement a set of ops, one to do a PIO read one to do a PIO write, > and choose the set of ops appropriate for the device depending upon > the setting found in the platform device. > I'm find with this approach but confused on the way to implement it. I could add the shifted read/write functions in smsc911x.c and choose, based on a boolean found in the 'smsc911x_platform_config' struct, which will be used (the original or the shifted methods). The drawback with this approach is the new shifted read/write methods are polluting smsc911x.c, something I'm not so fond of. It might be acceptable for one set of OPS but what if there is many more ? A better approach would be to pass the read/write methods to the smsc911x core using the 'smsc911x_platform_config' structure. That way smsc911x.c is kept clean but we'd have to move 'struct smsc911x_data' to smsc911x.h to implement the methods from the board files, something I'm not about do to without some sort of consensus. 'smsc911x_tx_writefifo' and 'smsc911x_rx_readfifo' are also of concern to me. I understand the usage of writesl/readsl but it introduces more challenges. If we don't want to replace their usage by a while loop as it is the case when SMSC911X_SWAP_FIFO and SMSC911X_USE_16BIT are set, we will need to introduce rx_readfifo and rx_writefifo in the OPS. Enlightenment is required on the above. Best regards, Mathieu.
My first email bounced on the list, hence sending again. Please see my comments below. Thanks, Mathieu. On 25 March 2011 15:31, David Miller <davem@davemloft.net> wrote: > From: mathieu.poirier@linaro.org > Date: Fri, 25 Mar 2011 15:27:17 -0600 > >> From: Alessandro Rubini <rubini@gnudd.com> >> >> At least one device I'm using needs address shifting to access >> registers in the LAN9221 device (smsc911x driver). This patch >> adds a shift parameter in platform_data. The feature must be >> enabled at configuration time, because shifting by a pdata >> parameter makes access slower in those devices where no shift is >> needed (I tested one, it was 20% slower). >> >> If the platform data requests shifted access but the feature is >> unavailable, the probe method complains to the console. Board >> config files should set the configuration option when needed. >> >> Signed-off-by: Alessandro Rubini <rubini@gnudd.com> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Implement this at run time, keying off of a boolean or similar > in the platform data. > > Implement a set of ops, one to do a PIO read one to do a PIO write, > and choose the set of ops appropriate for the device depending upon > the setting found in the platform device. > I'm find with this approach but confused on the way to implement it. I could add the shifted read/write functions in smsc911x.c and choose, based on a boolean found in the 'smsc911x_platform_config' struct, which will be used (the original or the shifted methods). The drawback with this approach is the new shifted read/write methods are polluting smsc911x.c, something I'm not so fond of. It might be acceptable for one set of OPS but what if there is many more ? A better approach would be to pass the read/write methods to the smsc911x core using the 'smsc911x_platform_config' structure. That way smsc911x.c is kept clean but we'd have to move 'struct smsc911x_data' to smsc911x.h to implement the methods from the board files, something I'm not about do to without some sort of consensus. 'smsc911x_tx_writefifo' and 'smsc911x_rx_readfifo' are also of concern to me. I understand the usage of writesl/readsl but it introduces more challenges. If we don't want to replace their usage by a while loop as it is the case when SMSC911X_SWAP_FIFO and SMSC911X_USE_16BIT are set, we will need to introduce rx_readfifo and rx_writefifo in the OPS. Enlightenment is required on the above. Best regards, Mathieu.
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 0382332..70f2a17 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -1075,6 +1075,16 @@ config SMSC911X_ARCH_HOOKS hooks for more comprehensive interrupt control and also to override the source of the MAC address. +config SMSC911X_ENABLE_SHIFTED_ADDRESS + def_bool n + depends on SMSC911X + help + The option allows device registers to be accessed in a sparse + way. This is needed to allow boards where the address bus is + not wired in the usual way. You'll most like don't need this + option, unless your vendor-provided board configuration file + enables it. + config NET_VENDOR_RACAL bool "Racal-Interlan (Micom) NI cards" depends on ISA diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c index d70bde9..67fb5d6 100644 --- a/drivers/net/smsc911x.c +++ b/drivers/net/smsc911x.c @@ -69,6 +69,13 @@ static int debug = 3; module_param(debug, int, 0); MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); +/* Shift value is uncommon and makes things slower: if not selected, force 0 */ +#ifdef CONFIG_SMSC911X_ENABLE_SHIFTED_ADDRESS +#define __smsc_shift(pdata, reg) ((reg) << ((pdata)->config.shift)) +#else +#define __smsc_shift(pdata, reg) (reg) +#endif + struct smsc911x_data { void __iomem *ioaddr; @@ -121,11 +128,13 @@ struct smsc911x_data { static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg) { if (pdata->config.flags & SMSC911X_USE_32BIT) - return readl(pdata->ioaddr + reg); + return readl(pdata->ioaddr + __smsc_shift(pdata, reg)); if (pdata->config.flags & SMSC911X_USE_16BIT) - return ((readw(pdata->ioaddr + reg) & 0xFFFF) | - ((readw(pdata->ioaddr + reg + 2) & 0xFFFF) << 16)); + return ((readw(pdata->ioaddr + + __smsc_shift(pdata, reg)) & 0xFFFF) | + ((readw(pdata->ioaddr + + __smsc_shift(pdata, reg + 2)) & 0xFFFF) << 16)); BUG(); return 0; @@ -147,13 +156,15 @@ static inline void __smsc911x_reg_write(struct smsc911x_data *pdata, u32 reg, u32 val) { if (pdata->config.flags & SMSC911X_USE_32BIT) { - writel(val, pdata->ioaddr + reg); + writel(val, pdata->ioaddr + __smsc_shift(pdata, reg)); return; } if (pdata->config.flags & SMSC911X_USE_16BIT) { - writew(val & 0xFFFF, pdata->ioaddr + reg); - writew((val >> 16) & 0xFFFF, pdata->ioaddr + reg + 2); + writew(val & 0xFFFF, + pdata->ioaddr + __smsc_shift(pdata, reg)); + writew((val >> 16) & 0xFFFF, + pdata->ioaddr + __smsc_shift(pdata, reg + 2)); return; } @@ -187,7 +198,8 @@ smsc911x_tx_writefifo(struct smsc911x_data *pdata, unsigned int *buf, } if (pdata->config.flags & SMSC911X_USE_32BIT) { - writesl(pdata->ioaddr + TX_DATA_FIFO, buf, wordcount); + writesl(pdata->ioaddr + __smsc_shift(pdata, TX_DATA_FIFO), + buf, wordcount); goto out; } @@ -219,7 +231,8 @@ smsc911x_rx_readfifo(struct smsc911x_data *pdata, unsigned int *buf, } if (pdata->config.flags & SMSC911X_USE_32BIT) { - readsl(pdata->ioaddr + RX_DATA_FIFO, buf, wordcount); + readsl(pdata->ioaddr + __smsc_shift(pdata, RX_DATA_FIFO), + buf, wordcount); goto out; } @@ -2023,6 +2036,14 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev) goto out_free_netdev_2; } + /* If platform data wants a shifted address bus, we must support it */ + if (pdata->config.shift && __smsc_shift(pdata, 1) == 1) { + pr_err("smsc911x: CONFIG_SMSC911X_ENABLE_SHIFTED_ADDRESS" + " is required but not available\n"); + retval = -EINVAL; + goto out_unmap_io_3; + } + retval = smsc911x_init(dev); if (retval < 0) goto out_unmap_io_3; diff --git a/include/linux/smsc911x.h b/include/linux/smsc911x.h index 7144e8a..ea9a8c5 100644 --- a/include/linux/smsc911x.h +++ b/include/linux/smsc911x.h @@ -29,6 +29,7 @@ struct smsc911x_platform_config { unsigned int irq_polarity; unsigned int irq_type; unsigned int flags; + unsigned int shift; /* used if CONFIG_SMSC911X_ENABLE_SHIFTED_ADDRESS */ phy_interface_t phy_interface; unsigned char mac[6]; };