| Message ID | 1426676247-14023-1-git-send-email-ben.dooks@codethink.co.uk |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Around Wed 18 Mar 2015 10:57:27 +0000 or thereabout, Ben Dooks wrote: > [note this has yet to be compile tested on avr32] > Compiles fine without warnings. > The changes to run the macb driver in 29af05aeb98e ("net: macb: > Add big endian CPU support") to support big endian operation on > ARM may not work on AVR32 which already is naturally big endian > architecture (and the driver already works here). The 29af05aeb98e will brick the macb driver for AVR32, as readZ_relaxed translates to readZ calls, which turns into le_to_cpu reads. > In this case the readl/writel relaxed will do the opposite of __raw > accesors which arleady work. Add an indirection of cdneth_ prefixed > accesors which are changed as necessary. Also do not issue the DMA > descritpor endian fetch configuration for AVR32. > > From discussions with Arnd Bergman, the following fix changes the use > of readl_relaxed and writel_relaxed with a version that can be put > back to __raw_readl/__raw_writel for the CONFIG_AVR32 case (and also > remove the change to the DMA descriptor endian). Thank you for spotting this. > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Reported-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Hans-Christian Egtvedt <egtvedt@samfundet.no> > -- > CC: Linux Networking List <netdev@vger.kernel.org> > CC: Arun Chandran <achandran@mvista.com> > CC: Haavard Skinnemoen <hskinnemoen@gmail.com> > CC: Hans-Christian Egtvedt <egtvedt@samfundet.no> > CC: Linux Kernel List <linux-kernel@vger.kernel.org> > CC: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > drivers/net/ethernet/cadence/macb.c | 14 +++++++------- > drivers/net/ethernet/cadence/macb.h | 20 ++++++++++++++------ > 2 files changed, 21 insertions(+), 13 deletions(-) <snipp diff>
Le 18/03/2015 13:25, Hans-Christian Egtvedt a écrit : > Around Wed 18 Mar 2015 10:57:27 +0000 or thereabout, Ben Dooks wrote: >> [note this has yet to be compile tested on avr32] >> > > Compiles fine without warnings. > >> The changes to run the macb driver in 29af05aeb98e ("net: macb: >> Add big endian CPU support") to support big endian operation on >> ARM may not work on AVR32 which already is naturally big endian >> architecture (and the driver already works here). > > The 29af05aeb98e will brick the macb driver for AVR32, as readZ_relaxed > translates to readZ calls, which turns into le_to_cpu reads. > >> In this case the readl/writel relaxed will do the opposite of __raw >> accesors which arleady work. Add an indirection of cdneth_ prefixed >> accesors which are changed as necessary. Also do not issue the DMA >> descritpor endian fetch configuration for AVR32. >> >> From discussions with Arnd Bergman, the following fix changes the use >> of readl_relaxed and writel_relaxed with a version that can be put >> back to __raw_readl/__raw_writel for the CONFIG_AVR32 case (and also >> remove the change to the DMA descriptor endian). > > Thank you for spotting this. > >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> Reported-by: Arnd Bergmann <arnd@arndb.de> > > Acked-by: Hans-Christian Egtvedt <egtvedt@samfundet.no> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> -- >> CC: Linux Networking List <netdev@vger.kernel.org> >> CC: Arun Chandran <achandran@mvista.com> >> CC: Haavard Skinnemoen <hskinnemoen@gmail.com> >> CC: Hans-Christian Egtvedt <egtvedt@samfundet.no> >> CC: Linux Kernel List <linux-kernel@vger.kernel.org> >> CC: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> drivers/net/ethernet/cadence/macb.c | 14 +++++++------- >> drivers/net/ethernet/cadence/macb.h | 20 ++++++++++++++------ >> 2 files changed, 21 insertions(+), 13 deletions(-) > > <snipp diff> >
On Wed, Mar 18, 2015 at 4:27 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: > > [note this has yet to be compile tested on avr32] > > The changes to run the macb driver in 29af05aeb98e ("net: macb: Yes I mistakenly thought zynq board was the first to run it in BIG endian mode. > Add big endian CPU support") to support big endian operation on > ARM may not work on AVR32 which already is naturally big endian > architecture (and the driver already works here). > > In this case the readl/writel relaxed will do the opposite of __raw > accesors which arleady work. Add an indirection of cdneth_ prefixed > accesors which are changed as necessary. Also do not issue the DMA > descritpor endian fetch configuration for AVR32. > > From discussions with Arnd Bergman, the following fix changes the use > of readl_relaxed and writel_relaxed with a version that can be put > back to __raw_readl/__raw_writel for the CONFIG_AVR32 case (and also > remove the change to the DMA descriptor endian). > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Reported-by: Arnd Bergmann <arnd@arndb.de> > -- > CC: Linux Networking List <netdev@vger.kernel.org> > CC: Arun Chandran <achandran@mvista.com> > CC: Haavard Skinnemoen <hskinnemoen@gmail.com> > CC: Hans-Christian Egtvedt <egtvedt@samfundet.no> > CC: Linux Kernel List <linux-kernel@vger.kernel.org> > CC: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > drivers/net/ethernet/cadence/macb.c | 14 +++++++------- > drivers/net/ethernet/cadence/macb.h | 20 ++++++++++++++------ > 2 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c > index b71e316..1c3f6e7 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -449,7 +449,7 @@ static void macb_update_stats(struct macb *bp) > WARN_ON((unsigned long)(end - p - 1) != (MACB_TPF - MACB_PFR) / 4); > > for(; p < end; p++, reg++) > - *p += readl_relaxed(reg); > + *p += cdneth_readl(reg); > } > > static int macb_halt_tx(struct macb *bp) > @@ -1587,7 +1587,7 @@ static void macb_configure_dma(struct macb *bp) > dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L); > dmacfg &= ~GEM_BIT(ENDIA_PKT); > /* Tell the chip to byteswap descriptors on big-endian hosts */ > -#ifdef __BIG_ENDIAN With https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-next/+/62f6924cb1543a10d05150d77f9a5e01e12c9ce1 in net-next I think CONFIG_AVR32 check is not required. --Arun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 18, 2015 at 6:46 PM, Arun Chandran <achandran@mvista.com> wrote: > On Wed, Mar 18, 2015 at 4:27 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: >> >> [note this has yet to be compile tested on avr32] >> >> The changes to run the macb driver in 29af05aeb98e ("net: macb: > > Yes I mistakenly thought zynq board was the first to run it in > BIG endian mode. > >> Add big endian CPU support") to support big endian operation on >> ARM may not work on AVR32 which already is naturally big endian >> architecture (and the driver already works here). >> >> In this case the readl/writel relaxed will do the opposite of __raw >> accesors which arleady work. Add an indirection of cdneth_ prefixed >> accesors which are changed as necessary. Also do not issue the DMA >> descritpor endian fetch configuration for AVR32. >> >> From discussions with Arnd Bergman, the following fix changes the use >> of readl_relaxed and writel_relaxed with a version that can be put >> back to __raw_readl/__raw_writel for the CONFIG_AVR32 case (and also >> remove the change to the DMA descriptor endian). >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> Reported-by: Arnd Bergmann <arnd@arndb.de> >> -- >> CC: Linux Networking List <netdev@vger.kernel.org> >> CC: Arun Chandran <achandran@mvista.com> >> CC: Haavard Skinnemoen <hskinnemoen@gmail.com> >> CC: Hans-Christian Egtvedt <egtvedt@samfundet.no> >> CC: Linux Kernel List <linux-kernel@vger.kernel.org> >> CC: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> drivers/net/ethernet/cadence/macb.c | 14 +++++++------- >> drivers/net/ethernet/cadence/macb.h | 20 ++++++++++++++------ >> 2 files changed, 21 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c >> index b71e316..1c3f6e7 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -449,7 +449,7 @@ static void macb_update_stats(struct macb *bp) >> WARN_ON((unsigned long)(end - p - 1) != (MACB_TPF - MACB_PFR) / 4); >> >> for(; p < end; p++, reg++) >> - *p += readl_relaxed(reg); >> + *p += cdneth_readl(reg); >> } >> >> static int macb_halt_tx(struct macb *bp) >> @@ -1587,7 +1587,7 @@ static void macb_configure_dma(struct macb *bp) >> dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L); >> dmacfg &= ~GEM_BIT(ENDIA_PKT); >> /* Tell the chip to byteswap descriptors on big-endian hosts */ >> -#ifdef __BIG_ENDIAN > > > With https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-next/+/62f6924cb1543a10d05150d77f9a5e01e12c9ce1 > in net-next I think CONFIG_AVR32 check is not required. > > --Arun I mean only the below check for dmacfg is not required /* Tell the chip to byteswap descriptors on big-endian hosts */ -#ifdef __BIG_ENDIAN +#if defined(__BIG_ENDIAN) && !defined(CONFIG_AVR32) dmacfg |= GEM_BIT(ENDIA_DESC); #endif --Arun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 03/18/2015 11:57 AM, Ben Dooks wrote: > [note this has yet to be compile tested on avr32] > > The changes to run the macb driver in 29af05aeb98e ("net: macb: > Add big endian CPU support") to support big endian operation on > ARM may not work on AVR32 which already is naturally big endian > architecture (and the driver already works here). > > In this case the readl/writel relaxed will do the opposite of __raw > accesors which arleady work. Add an indirection of cdneth_ prefixed here is type. > accesors which are changed as necessary. Also do not issue the DMA > descritpor endian fetch configuration for AVR32. ditto. > > From discussions with Arnd Bergman, the following fix changes the use > of readl_relaxed and writel_relaxed with a version that can be put > back to __raw_readl/__raw_writel for the CONFIG_AVR32 case (and also > remove the change to the DMA descriptor endian). > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Reported-by: Arnd Bergmann <arnd@arndb.de> > -- > CC: Linux Networking List <netdev@vger.kernel.org> > CC: Arun Chandran <achandran@mvista.com> > CC: Haavard Skinnemoen <hskinnemoen@gmail.com> > CC: Hans-Christian Egtvedt <egtvedt@samfundet.no> > CC: Linux Kernel List <linux-kernel@vger.kernel.org> > CC: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > drivers/net/ethernet/cadence/macb.c | 14 +++++++------- > drivers/net/ethernet/cadence/macb.h | 20 ++++++++++++++------ > 2 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c > index b71e316..1c3f6e7 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -449,7 +449,7 @@ static void macb_update_stats(struct macb *bp) > WARN_ON((unsigned long)(end - p - 1) != (MACB_TPF - MACB_PFR) / 4); > > for(; p < end; p++, reg++) > - *p += readl_relaxed(reg); > + *p += cdneth_readl(reg); > } > > static int macb_halt_tx(struct macb *bp) > @@ -1587,7 +1587,7 @@ static void macb_configure_dma(struct macb *bp) > dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L); > dmacfg &= ~GEM_BIT(ENDIA_PKT); > /* Tell the chip to byteswap descriptors on big-endian hosts */ > -#ifdef __BIG_ENDIAN > +#if defined(__BIG_ENDIAN) && !defined(CONFIG_AVR32) > dmacfg |= GEM_BIT(ENDIA_DESC); > #endif the code has changed here - please rebase it on the top of net-next tree. > if (bp->dev->features & NETIF_F_HW_CSUM) > @@ -1836,14 +1836,14 @@ static void gem_update_stats(struct macb *bp) > > for (i = 0; i < GEM_STATS_LEN; ++i, ++p) { > u32 offset = gem_statistics[i].offset; > - u64 val = readl_relaxed(bp->regs + offset); > + u64 val = cdneth_readl(bp->regs + offset); > > bp->ethtool_stats[i] += val; > *p += val; > > if (offset == GEM_OCTTXL || offset == GEM_OCTRXL) { > /* Add GEM_OCTTXH, GEM_OCTRXH */ > - val = readl_relaxed(bp->regs + offset + 4); > + val = cdneth_readl(bp->regs + offset + 4); > bp->ethtool_stats[i] += ((u64)val) << 32; > *(++p) += val; > } > @@ -2191,17 +2191,17 @@ static void macb_probe_queues(void __iomem *mem, > unsigned int hw_q; > u32 mid; > > - *queue_mask = 0x1; > + *queue_mask = 0x1;\ > *num_queues = 1; > > /* is it macb or gem ? */ > - mid = readl_relaxed(mem + MACB_MID); > + mid = cdneth_readl(mem + MACB_MID); > > if (MACB_BFEXT(IDNUM, mid) != 0x2) > return; > > /* bit 0 is never set but queue 0 always exists */ > - *queue_mask = readl_relaxed(mem + GEM_DCFG6) & 0xff; > + *queue_mask = cdneth_readl(mem + GEM_DCFG6) & 0xff; > > *queue_mask |= 0x1; > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index 6cfff0b..c003e98 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -422,19 +422,27 @@ > << GEM_##name##_OFFSET)) \ > | GEM_BF(name, value)) > > +#ifdef CONFIG_AVR32 > +#define cdneth_readl __raw_readl > +#define cdneth_writel __raw_writel > +#else > +#define cdneth_readl readl_relaxed > +#define cdneth_writel writel_relaxed > +#endif but this seems to me the way back. If cadence IP is big-endian then you can you what I have suggested to you before. Just do endian detection on IP itself. It means one checking will be for arm platforms if cpu runs big or little endian and if IP is big on little endian. It will end up in dynamic IO real/write functions which seems to the best way to go. Of course doing this in this style means to add one more load in every io operations but I don't think this is impact performance a lot. Thanks, Michal
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index b71e316..1c3f6e7 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -449,7 +449,7 @@ static void macb_update_stats(struct macb *bp) WARN_ON((unsigned long)(end - p - 1) != (MACB_TPF - MACB_PFR) / 4); for(; p < end; p++, reg++) - *p += readl_relaxed(reg); + *p += cdneth_readl(reg); } static int macb_halt_tx(struct macb *bp) @@ -1587,7 +1587,7 @@ static void macb_configure_dma(struct macb *bp) dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L); dmacfg &= ~GEM_BIT(ENDIA_PKT); /* Tell the chip to byteswap descriptors on big-endian hosts */ -#ifdef __BIG_ENDIAN +#if defined(__BIG_ENDIAN) && !defined(CONFIG_AVR32) dmacfg |= GEM_BIT(ENDIA_DESC); #endif if (bp->dev->features & NETIF_F_HW_CSUM) @@ -1836,14 +1836,14 @@ static void gem_update_stats(struct macb *bp) for (i = 0; i < GEM_STATS_LEN; ++i, ++p) { u32 offset = gem_statistics[i].offset; - u64 val = readl_relaxed(bp->regs + offset); + u64 val = cdneth_readl(bp->regs + offset); bp->ethtool_stats[i] += val; *p += val; if (offset == GEM_OCTTXL || offset == GEM_OCTRXL) { /* Add GEM_OCTTXH, GEM_OCTRXH */ - val = readl_relaxed(bp->regs + offset + 4); + val = cdneth_readl(bp->regs + offset + 4); bp->ethtool_stats[i] += ((u64)val) << 32; *(++p) += val; } @@ -2191,17 +2191,17 @@ static void macb_probe_queues(void __iomem *mem, unsigned int hw_q; u32 mid; - *queue_mask = 0x1; + *queue_mask = 0x1;\ *num_queues = 1; /* is it macb or gem ? */ - mid = readl_relaxed(mem + MACB_MID); + mid = cdneth_readl(mem + MACB_MID); if (MACB_BFEXT(IDNUM, mid) != 0x2) return; /* bit 0 is never set but queue 0 always exists */ - *queue_mask = readl_relaxed(mem + GEM_DCFG6) & 0xff; + *queue_mask = cdneth_readl(mem + GEM_DCFG6) & 0xff; *queue_mask |= 0x1; diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 6cfff0b..c003e98 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -422,19 +422,27 @@ << GEM_##name##_OFFSET)) \ | GEM_BF(name, value)) +#ifdef CONFIG_AVR32 +#define cdneth_readl __raw_readl +#define cdneth_writel __raw_writel +#else +#define cdneth_readl readl_relaxed +#define cdneth_writel writel_relaxed +#endif + /* Register access macros */ #define macb_readl(port,reg) \ - readl_relaxed((port)->regs + MACB_##reg) + cdneth_readl((port)->regs + MACB_##reg) #define macb_writel(port,reg,value) \ - writel_relaxed((value), (port)->regs + MACB_##reg) + cdneth_writel((value), (port)->regs + MACB_##reg) #define gem_readl(port, reg) \ - readl_relaxed((port)->regs + GEM_##reg) + cdneth_readl((port)->regs + GEM_##reg) #define gem_writel(port, reg, value) \ - writel_relaxed((value), (port)->regs + GEM_##reg) + cdneth_writel((value), (port)->regs + GEM_##reg) #define queue_readl(queue, reg) \ - readl_relaxed((queue)->bp->regs + (queue)->reg) + cdneth_readl((queue)->bp->regs + (queue)->reg) #define queue_writel(queue, reg, value) \ - writel_relaxed((value), (queue)->bp->regs + (queue)->reg) + cdneth_writel((value), (queue)->bp->regs + (queue)->reg) /* Conditional GEM/MACB macros. These perform the operation to the correct * register dependent on whether the device is a GEM or a MACB. For registers
[note this has yet to be compile tested on avr32] The changes to run the macb driver in 29af05aeb98e ("net: macb: Add big endian CPU support") to support big endian operation on ARM may not work on AVR32 which already is naturally big endian architecture (and the driver already works here). In this case the readl/writel relaxed will do the opposite of __raw accesors which arleady work. Add an indirection of cdneth_ prefixed accesors which are changed as necessary. Also do not issue the DMA descritpor endian fetch configuration for AVR32. From discussions with Arnd Bergman, the following fix changes the use of readl_relaxed and writel_relaxed with a version that can be put back to __raw_readl/__raw_writel for the CONFIG_AVR32 case (and also remove the change to the DMA descriptor endian). Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Reported-by: Arnd Bergmann <arnd@arndb.de> -- CC: Linux Networking List <netdev@vger.kernel.org> CC: Arun Chandran <achandran@mvista.com> CC: Haavard Skinnemoen <hskinnemoen@gmail.com> CC: Hans-Christian Egtvedt <egtvedt@samfundet.no> CC: Linux Kernel List <linux-kernel@vger.kernel.org> CC: Nicolas Ferre <nicolas.ferre@atmel.com> --- drivers/net/ethernet/cadence/macb.c | 14 +++++++------- drivers/net/ethernet/cadence/macb.h | 20 ++++++++++++++------ 2 files changed, 21 insertions(+), 13 deletions(-)