Re: [PATCH 2/2] smc91x: remove ARM hack for unaligned 16-bit writes

Nicolas Pitre
2016-08-25T17:59:53Z

On Thu, 25 Aug 2016, Arnd Bergmann wrote:

> The ARM specific I/O operations are almost the same as the generic
> ones, with the exception of the SMC_outw macro that works around
> a problem of some platforms that cannot write to 16-bit registers
> at an address that is not 32-bit aligned.
>
> By inspection, I found that this is handled already in the
> register abstractions for almost all cases, the exceptions being
> SMC_SET_MAC_ADDR() and SMC_SET_MCAST(). I assume that all
> platforms that require the hack for the other registers also
> need it here, so the ones listed explictly here are the only
> ones that work correctly, while the other ones either don't
> need the hack at all, or they will set an incorrect MAC
> address (which can often go unnoticed).

Probably that all PXA based platforms that use the SMC91x with 32-bit
accesses need this. The others simply wired only 16 data lines, or
only 8 like the Neponset.

However I no longer have the concerned hardware and can't test it.

> This changes the two macros that set the unaligned registers
> to use 32-bit writes if possible, which should do the right
> thing in all combinations. The ARM specific SMC_outw gets removed
> as a consequence.
>
> The only difference between the ARM behavior and the default is
> the selection of the LED settings. The fact that we have different
> defaults based on the CPU architectures here is a bit suspicious,
> but probably harmless, and I have no plan of touching that.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/net/ethernet/smsc/smc91x.h | 50 +++++++++++++++++++++++---------------
> 1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/smsc/smc91x.h b/drivers/net/ethernet/smsc/smc91x.h
> index 22333477d0b5..908473d9ede0 100644
> --- a/drivers/net/ethernet/smsc/smc91x.h
> +++ b/drivers/net/ethernet/smsc/smc91x.h
> @@ -58,6 +58,7 @@
> #define SMC_inw(a, r) readw((a) + (r))
> #define SMC_inl(a, r) readl((a) + (r))
> #define SMC_outb(v, a, r) writeb(v, (a) + (r))
> +#define SMC_outw(v, a, r) writew(v, (a) + (r))
> #define SMC_outl(v, a, r) writel(v, (a) + (r))
> #define SMC_insw(a, r, p, l) readsw((a) + (r), p, l)
> #define SMC_outsw(a, r, p, l) writesw((a) + (r), p, l)
> @@ -65,19 +66,6 @@
> #define SMC_outsl(a, r, p, l) writesl((a) + (r), p, l)
> #define SMC_IRQ_FLAGS (-1) /* from resource */
>
> -/* We actually can't write halfwords properly if not word aligned */
> -static inline void SMC_outw(u16 val, void __iomem *ioaddr, int reg)
> -{
> - if ((machine_is_mainstone() || machine_is_stargate2() ||
> - machine_is_pxa_idp()) && reg & 2) {
> - unsigned int v = val << 16;
> - v |= readl(ioaddr + (reg & ~2)) & 0xffff;
> - writel(v, ioaddr + (reg & ~2));
> - } else {
> - writew(val, ioaddr + reg);
> - }
> -}
> -
> #elif defined(CONFIG_SH_SH4202_MICRODEV)
>
> #define SMC_CAN_USE_8BIT 0
> @@ -1029,18 +1017,40 @@ static const char * chip_ids[ 16 ] = {
>
> #define SMC_SET_MAC_ADDR(lp, addr) \
> do { \
> - SMC_out16(addr[0]|(addr[1] << 8), ioaddr, ADDR0_REG(lp)); \
> - SMC_out16(addr[2]|(addr[3] << 8), ioaddr, ADDR1_REG(lp)); \
> - SMC_out16(addr[4]|(addr[5] << 8), ioaddr, ADDR2_REG(lp)); \
> + if (SMC_32BIT(lp)) { \
> + SMC_outl((addr[0] )|(addr[1] << 8) | \
> + (addr[2] << 16)|(addr[3] << 24), \
> + ioaddr, ADDR0_REG(lp)); \
> + } else { \
> + SMC_out16(addr[0]|(addr[1] << 8), ioaddr, \
> + ADDR0_REG(lp)); \
> + SMC_out16(addr[2]|(addr[3] << 8), ioaddr, \
> + ADDR1_REG(lp)); \
> + } \
> + SMC_out16(addr[4]|(addr[5] << 8), ioaddr, \
> + ADDR2_REG(lp)); \
> } while (0)
>
> #define SMC_SET_MCAST(lp, x) \
> do { \
> const unsigned char *mt = (x); \
> - SMC_out16(mt[0] | (mt[1] << 8), ioaddr, MCAST_REG1(lp)); \
> - SMC_out16(mt[2] | (mt[3] << 8), ioaddr, MCAST_REG2(lp)); \
> - SMC_out16(mt[4] | (mt[5] << 8), ioaddr, MCAST_REG3(lp)); \
> - SMC_out16(mt[6] | (mt[7] << 8), ioaddr, MCAST_REG4(lp)); \
> + if (SMC_32BIT(lp)) { \
> + SMC_outl((mt[0] ) | (mt[1] << 8) | \
> + (mt[2] << 16) | (mt[3] << 24), \
> + ioaddr, MCAST_REG1(lp)); \
> + SMC_outl((mt[4] ) | (mt[5] << 8) | \
> + (mt[6] << 16) | (mt[7] << 24), \
> + ioaddr, MCAST_REG3(lp)); \
> + } else { \
> + SMC_out16(mt[0] | (mt[1] << 8), \
> + ioaddr, MCAST_REG1(lp)); \
> + SMC_out16(mt[2] | (mt[3] << 8), \
> + ioaddr, MCAST_REG2(lp)); \
> + SMC_out16(mt[4] | (mt[5] << 8), \
> + ioaddr, MCAST_REG3(lp)); \
> + SMC_out16(mt[6] | (mt[7] << 8), \
> + ioaddr, MCAST_REG4(lp)); \
> + } \
> } while (0)
>
> #define SMC_PUT_PKT_HDR(lp, status, length) \
> --
> 2.9.0
>
>