| Message ID | 1408569876-28539-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Fabio,
I feel the name "ARCH_DMA_MINALIGN_MX6SX" is not good here. Because the 64 bytes alignment is only required by the DMA engine in FEC controller, not a ARCH DMA value.
Best regards,
Ye Li
On 8/21/2014 5:24 AM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> mx6solox has a requirement for 64 bytes alignment for RX DMA transfer.
>
> Adjust it accordingly.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested
> by Otavio
>
> drivers/net/fec_mxc.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 4cefda4..1a5105e 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR;
> */
> #define FEC_XFER_TIMEOUT 5000
>
> +#define ARCH_DMA_MINALIGN_MX6SX 64
> +
> #ifndef CONFIG_MII
> #error "CONFIG_MII has to be defined!"
> #endif
> @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec)
> return 0;
> }
>
> +static int dma_rx_align(void)
> +{
> +#ifdef CONFIG_MX6SX
> + return ARCH_DMA_MINALIGN_MX6SX;
> +#else
> + return ARCH_DMA_MINALIGN;
> +#endif
> +}
> +
> /**
> * Initialize receive task's buffer descriptors
> * @param[in] fec all we know about the device yet
> @@ -286,7 +297,7 @@ static void fec_rbd_init(struct fec_priv *fec, int count, int dsize)
> * Reload the RX descriptors with default values and wipe
> * the RX buffers.
> */
> - size = roundup(dsize, ARCH_DMA_MINALIGN);
> + size = roundup(dsize, dma_rx_align());
> for (i = 0; i < count; i++) {
> data = (uint8_t *)fec->rbd_base[i].data_pointer;
> memset(data, 0, dsize);
> @@ -881,9 +892,9 @@ static int fec_alloc_descs(struct fec_priv *fec)
> /* Allocate RX buffers. */
>
> /* Maximum RX buffer size. */
> - size = roundup(FEC_MAX_PKT_SIZE, ARCH_DMA_MINALIGN);
> + size = roundup(FEC_MAX_PKT_SIZE, dma_rx_align());
> for (i = 0; i < FEC_RBD_NUM; i++) {
> - data = memalign(ARCH_DMA_MINALIGN, size);
> + data = memalign(dma_rx_align(), size);
> if (!data) {
> printf("%s: error allocating rxbuf %d\n", __func__, i);
> goto err_ring;
On Wednesday, August 20, 2014 at 11:24:35 PM, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > mx6solox has a requirement for 64 bytes alignment for RX DMA transfer. > > Adjust it accordingly. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > Changes since v1: > - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested > by Otavio > > drivers/net/fec_mxc.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 4cefda4..1a5105e 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR; > */ > #define FEC_XFER_TIMEOUT 5000 > > +#define ARCH_DMA_MINALIGN_MX6SX 64 > + > #ifndef CONFIG_MII > #error "CONFIG_MII has to be defined!" > #endif > @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec) > return 0; > } > > +static int dma_rx_align(void) > +{ > +#ifdef CONFIG_MX6SX > + return ARCH_DMA_MINALIGN_MX6SX; > +#else > + return ARCH_DMA_MINALIGN; > +#endif > +} This is not really an alignment, is it ? What is this really, cacheline length problem or what ? See RXDESC_PER_CACHELINE in the driver and make sure you don't break that mechanism as it would bite you ... btw. you're using tab/space inconsistently past the 'return' keyword. Also, use the fec_ prefix in the functions in this driver consistently please. [..] Best regards, Marek Vasut
Hi Fabio, On 20.08.2014 23:24, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > mx6solox has a requirement for 64 bytes alignment for RX DMA transfer. > > Adjust it accordingly. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > Changes since v1: > - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested > by Otavio > > drivers/net/fec_mxc.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 4cefda4..1a5105e 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR; > */ > #define FEC_XFER_TIMEOUT 5000 > > +#define ARCH_DMA_MINALIGN_MX6SX 64 > + > #ifndef CONFIG_MII > #error "CONFIG_MII has to be defined!" > #endif > @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec) > return 0; > } > > +static int dma_rx_align(void) > +{ > +#ifdef CONFIG_MX6SX > + return ARCH_DMA_MINALIGN_MX6SX; > +#else > + return ARCH_DMA_MINALIGN; > +#endif Why don't you just use the bigger value (64) for all SoC versions? Shouldn't hurt, right. And would keep the source clean. Thanks, Stefan
Hi Fabio, On 20/08/2014 23:24, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > mx6solox has a requirement for 64 bytes alignment for RX DMA transfer. > > Adjust it accordingly. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > Changes since v1: > - Avoid too many ifdef's by providing a dma_rx_align() funtion as suggested > by Otavio > > drivers/net/fec_mxc.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 4cefda4..1a5105e 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR; > */ > #define FEC_XFER_TIMEOUT 5000 > > +#define ARCH_DMA_MINALIGN_MX6SX 64 > + > #ifndef CONFIG_MII > #error "CONFIG_MII has to be defined!" > #endif > @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec) > return 0; > } > > +static int dma_rx_align(void) > +{ > +#ifdef CONFIG_MX6SX > + return ARCH_DMA_MINALIGN_MX6SX; > +#else > + return ARCH_DMA_MINALIGN; > +#endif > +} > + I go just a bit further according to Otavio's comment. In this way, we do not remove #ifdef, we have only moved. What about using is_cpu_type() instead of this ? If you do not want to add the macro for not mx6 socs (preferable way, I think), the get_cpu_rev() should be available for all i.MXes. > /** > * Initialize receive task's buffer descriptors > * @param[in] fec all we know about the device yet > @@ -286,7 +297,7 @@ static void fec_rbd_init(struct fec_priv *fec, int count, int dsize) > * Reload the RX descriptors with default values and wipe > * the RX buffers. > */ > - size = roundup(dsize, ARCH_DMA_MINALIGN); > + size = roundup(dsize, dma_rx_align()); > for (i = 0; i < count; i++) { > data = (uint8_t *)fec->rbd_base[i].data_pointer; > memset(data, 0, dsize); > @@ -881,9 +892,9 @@ static int fec_alloc_descs(struct fec_priv *fec) > /* Allocate RX buffers. */ > > /* Maximum RX buffer size. */ > - size = roundup(FEC_MAX_PKT_SIZE, ARCH_DMA_MINALIGN); > + size = roundup(FEC_MAX_PKT_SIZE, dma_rx_align()); > for (i = 0; i < FEC_RBD_NUM; i++) { > - data = memalign(ARCH_DMA_MINALIGN, size); > + data = memalign(dma_rx_align(), size); > if (!data) { > printf("%s: error allocating rxbuf %d\n", __func__, i); > goto err_ring; > Best regards, Stefano Babic
On Thu, Aug 21, 2014 at 5:01 AM, Stefano Babic <sbabic@denx.de> wrote: > I go just a bit further according to Otavio's comment. In this way, we > do not remove #ifdef, we have only moved. What about using is_cpu_type() > instead of this ? If you do not want to add the macro for not mx6 socs > (preferable way, I think), the get_cpu_rev() should be available for all > i.MXes. Yes, the issue with this approach is that is_cpu_type() is currently only available for mx6. mx5, mx27, mx25, mx28 also need to be able to 'see' is_cpu_type(), but this requires some work to do though.
On Thu, Aug 21, 2014 at 3:03 AM, Stefan Roese <sr@denx.de> wrote: > Why don't you just use the bigger value (64) for all SoC versions? Shouldn't > hurt, right. And would keep the source clean. I think this should work fine. Thanks for the suggestion, Stefan.
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 4cefda4..1a5105e 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -28,6 +28,8 @@ DECLARE_GLOBAL_DATA_PTR; */ #define FEC_XFER_TIMEOUT 5000 +#define ARCH_DMA_MINALIGN_MX6SX 64 + #ifndef CONFIG_MII #error "CONFIG_MII has to be defined!" #endif @@ -267,6 +269,15 @@ static int fec_tx_task_disable(struct fec_priv *fec) return 0; } +static int dma_rx_align(void) +{ +#ifdef CONFIG_MX6SX + return ARCH_DMA_MINALIGN_MX6SX; +#else + return ARCH_DMA_MINALIGN; +#endif +} + /** * Initialize receive task's buffer descriptors * @param[in] fec all we know about the device yet @@ -286,7 +297,7 @@ static void fec_rbd_init(struct fec_priv *fec, int count, int dsize) * Reload the RX descriptors with default values and wipe * the RX buffers. */ - size = roundup(dsize, ARCH_DMA_MINALIGN); + size = roundup(dsize, dma_rx_align()); for (i = 0; i < count; i++) { data = (uint8_t *)fec->rbd_base[i].data_pointer; memset(data, 0, dsize); @@ -881,9 +892,9 @@ static int fec_alloc_descs(struct fec_priv *fec) /* Allocate RX buffers. */ /* Maximum RX buffer size. */ - size = roundup(FEC_MAX_PKT_SIZE, ARCH_DMA_MINALIGN); + size = roundup(FEC_MAX_PKT_SIZE, dma_rx_align()); for (i = 0; i < FEC_RBD_NUM; i++) { - data = memalign(ARCH_DMA_MINALIGN, size); + data = memalign(dma_rx_align(), size); if (!data) { printf("%s: error allocating rxbuf %d\n", __func__, i); goto err_ring;