[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Stefan Roese]

With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
ethernet support does not work any more with d-cache enabled on the
AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
to 128 but this change was not refected in the rx_buffer flush and
invalidate functions, as these also use this macro.

This patch now fixes this by calculating the rx buffer size correctly
again in those functions. With this change, ethernet works again
reliably on my AT91SAM board.

Signed-off-by: Stefan Roese <sr@denx.de>
Fixes: c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Eugen Hristev <eugen.hristev@microchip.com>
Cc: Anup Patel <anup.patel@wdc.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Joe Hershberger <joe.hershberger@ni.com>
---
 drivers/net/macb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index c99cf663a4..a6d7b8ae8d 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -296,13 +296,15 @@ static inline void macb_flush_ring_desc(struct macb_device *macb, bool rx)
 static inline void macb_flush_rx_buffer(struct macb_device *macb)
 {
 	flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
-			   ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
+			   ALIGN(macb->rx_buffer_size * MACB_RX_RING_SIZE,
+				 PKTALIGN));
 }
 
 static inline void macb_invalidate_rx_buffer(struct macb_device *macb)
 {
 	invalidate_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
-				ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
+				ALIGN(macb->rx_buffer_size * MACB_RX_RING_SIZE,
+				      PKTALIGN));
 }
 
 #if defined(CONFIG_CMD_NET)
-- 
2.23.0

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Bin Meng]

On Mon, Aug 26, 2019 at 3:18 PM Stefan Roese <sr@denx.de> wrote:
>
> With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> ethernet support does not work any more with d-cache enabled on the
> AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
> to 128 but this change was not refected in the rx_buffer flush and
> invalidate functions, as these also use this macro.
>
> This patch now fixes this by calculating the rx buffer size correctly
> again in those functions. With this change, ethernet works again
> reliably on my AT91SAM board.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Fixes: c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> Cc: Anup Patel <anup.patel@wdc.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> ---
>  drivers/net/macb.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Joe Hershberger]

On Mon, Aug 26, 2019 at 2:18 AM Stefan Roese <sr@denx.de> wrote:
>
> With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> ethernet support does not work any more with d-cache enabled on the
> AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
> to 128 but this change was not refected in the rx_buffer flush and
> invalidate functions, as these also use this macro.
>
> This patch now fixes this by calculating the rx buffer size correctly
> again in those functions. With this change, ethernet works again
> reliably on my AT91SAM board.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Fixes: c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> Cc: Anup Patel <anup.patel@wdc.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] net: macb: Fix rx buffer cache handling

[Joe Hershberger]

Hi Stefan,

https://patchwork.ozlabs.org/patch/1152998/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Ramon Fried]

> > > Should it be:
> > >
> > >            flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> > >                             ALIGN(macb->rx_buffer_size *
> > > MACB_RX_RING_SIZE, PKTALIGN));
> >
> > Yes, this looks correct. v2 will follow soon.
> >
> > Thanks,
> > Stefan
>
The entire cache handling is broken in this driver, I will fix it in
future patches.
Thanks, Ramon.

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Ramon Fried]

On Fri, Aug 23, 2019 at 1:37 PM Stefan Roese <sr@denx.de> wrote:
>
> Hi Bin,
>
> On 23.08.19 11:22, Bin Meng wrote:
> > On Fri, Aug 23, 2019 at 5:17 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> Hi Stefan,
> >>
> >> On Fri, Aug 23, 2019 at 5:02 PM Stefan Roese <sr@denx.de> wrote:
> >>>
> >>> With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> >>> ethernet support does not work any more with d-cache enabled on the
> >>> AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
> >>> to 128 but this change was not refected in the rx_buffer flush and
> >>> invalidate functions, as these also use this macro.
> >>>
> >>> This patch now fixes this by calculating the rx buffer size correctly
> >>> again in those functions. With this change, ethernet works again
> >>> reliably on my AT91SAM board.
> >>>
> >>> Signed-off-by: Stefan Roese <sr@denx.de>
> >>> Cc: Ramon Fried <rfried.dev@gmail.com>
> >>> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> >>> Cc: Anup Patel <anup.patel@wdc.com>
> >>> Cc: Bin Meng <bmeng.cn@gmail.com>
> >>> Cc: Joe Hershberger <joe.hershberger@ni.com>
> >>> ---
> >>>   drivers/net/macb.c | 6 ++++--
> >>>   1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> >>> index c99cf663a4..fca380d012 100644
> >>> --- a/drivers/net/macb.c
> >>> +++ b/drivers/net/macb.c
> >>> @@ -296,13 +296,15 @@ static inline void macb_flush_ring_desc(struct macb_device *macb, bool rx)
> >>>   static inline void macb_flush_rx_buffer(struct macb_device *macb)
> >>>   {
> >>>          flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> >>> -                          ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
> >>> +                          ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
> >>> +                                PKTALIGN));
> >>
> >> This looks wrong to me.
> >>
> >> Shouldn't it be:
> >>
> >>           flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> >>                            ALIGN(macb->rx_buffer_size, PKTALIGN));
> >
> > Ah, for MACB, looks it should be "MACB_RX_BUFFER_SIZE *
> > MACB_RX_RING_SIZE", but for GEM, I am not sure.
> >
> > Should it be:
> >
> >            flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> >                             ALIGN(macb->rx_buffer_size *
> > MACB_RX_RING_SIZE, PKTALIGN));
>
> Yes, this looks correct. v2 will follow soon.
>
> Thanks,
> Stefan

Thanks Stefan,
Can you fix your commit message to show: Fixes:...

You can use the following bash alias to generate the line: (pass
c6d07bf440bc as argument)
alias gfixes='git --no-pager show -s --abbrev-commit --abbrev=12
--pretty=format:"Fixes: %h (\"%s\")%n"'

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Ramon Fried]

On Fri, Aug 23, 2019 at 12:22 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Aug 23, 2019 at 5:17 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Stefan,
> >
> > On Fri, Aug 23, 2019 at 5:02 PM Stefan Roese <sr@denx.de> wrote:
> > >
> > > With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> > > ethernet support does not work any more with d-cache enabled on the
> > > AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
> > > to 128 but this change was not refected in the rx_buffer flush and
> > > invalidate functions, as these also use this macro.
> > >
> > > This patch now fixes this by calculating the rx buffer size correctly
> > > again in those functions. With this change, ethernet works again
> > > reliably on my AT91SAM board.
> > >
> > > Signed-off-by: Stefan Roese <sr@denx.de>
> > > Cc: Ramon Fried <rfried.dev@gmail.com>
> > > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > > Cc: Anup Patel <anup.patel@wdc.com>
> > > Cc: Bin Meng <bmeng.cn@gmail.com>
> > > Cc: Joe Hershberger <joe.hershberger@ni.com>
> > > ---
> > >  drivers/net/macb.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > > index c99cf663a4..fca380d012 100644
> > > --- a/drivers/net/macb.c
> > > +++ b/drivers/net/macb.c
> > > @@ -296,13 +296,15 @@ static inline void macb_flush_ring_desc(struct macb_device *macb, bool rx)
> > >  static inline void macb_flush_rx_buffer(struct macb_device *macb)
> > >  {
> > >         flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> > > -                          ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
> > > +                          ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
> > > +                                PKTALIGN));
> >
> > This looks wrong to me.
> >
> > Shouldn't it be:
> >
> >          flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> >                           ALIGN(macb->rx_buffer_size, PKTALIGN));
>
> Ah, for MACB, looks it should be "MACB_RX_BUFFER_SIZE *
> MACB_RX_RING_SIZE", but for GEM, I am not sure.
>
> Should it be:
>
>           flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
>                            ALIGN(macb->rx_buffer_size *
> MACB_RX_RING_SIZE, PKTALIGN));
>
Good catch !, I'll issue a fix soon.
Thanks,
Ramon
> >
> > >  }
> > >
> > >  static inline void macb_invalidate_rx_buffer(struct macb_device *macb)
> > >  {
> > >         invalidate_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> > > -                               ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
> > > +                               ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
> > > +                                     PKTALIGN));
> > >  }
> > >
> > >  #if defined(CONFIG_CMD_NET)
> > > --
>
> Regards,
> Bin

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Stefan Roese]

Hi Bin,

On 23.08.19 11:22, Bin Meng wrote:
> On Fri, Aug 23, 2019 at 5:17 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Hi Stefan,
>>
>> On Fri, Aug 23, 2019 at 5:02 PM Stefan Roese <sr@denx.de> wrote:
>>>
>>> With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
>>> ethernet support does not work any more with d-cache enabled on the
>>> AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
>>> to 128 but this change was not refected in the rx_buffer flush and
>>> invalidate functions, as these also use this macro.
>>>
>>> This patch now fixes this by calculating the rx buffer size correctly
>>> again in those functions. With this change, ethernet works again
>>> reliably on my AT91SAM board.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Ramon Fried <rfried.dev@gmail.com>
>>> Cc: Eugen Hristev <eugen.hristev@microchip.com>
>>> Cc: Anup Patel <anup.patel@wdc.com>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>> ---
>>>   drivers/net/macb.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>>> index c99cf663a4..fca380d012 100644
>>> --- a/drivers/net/macb.c
>>> +++ b/drivers/net/macb.c
>>> @@ -296,13 +296,15 @@ static inline void macb_flush_ring_desc(struct macb_device *macb, bool rx)
>>>   static inline void macb_flush_rx_buffer(struct macb_device *macb)
>>>   {
>>>          flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
>>> -                          ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
>>> +                          ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
>>> +                                PKTALIGN));
>>
>> This looks wrong to me.
>>
>> Shouldn't it be:
>>
>>           flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
>>                            ALIGN(macb->rx_buffer_size, PKTALIGN));
> 
> Ah, for MACB, looks it should be "MACB_RX_BUFFER_SIZE *
> MACB_RX_RING_SIZE", but for GEM, I am not sure.
> 
> Should it be:
> 
>            flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
>                             ALIGN(macb->rx_buffer_size *
> MACB_RX_RING_SIZE, PKTALIGN));

Yes, this looks correct. v2 will follow soon.

Thanks,
Stefan

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Bin Meng]

On Fri, Aug 23, 2019 at 5:17 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Stefan,
>
> On Fri, Aug 23, 2019 at 5:02 PM Stefan Roese <sr@denx.de> wrote:
> >
> > With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> > ethernet support does not work any more with d-cache enabled on the
> > AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
> > to 128 but this change was not refected in the rx_buffer flush and
> > invalidate functions, as these also use this macro.
> >
> > This patch now fixes this by calculating the rx buffer size correctly
> > again in those functions. With this change, ethernet works again
> > reliably on my AT91SAM board.
> >
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > Cc: Ramon Fried <rfried.dev@gmail.com>
> > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > Cc: Anup Patel <anup.patel@wdc.com>
> > Cc: Bin Meng <bmeng.cn@gmail.com>
> > Cc: Joe Hershberger <joe.hershberger@ni.com>
> > ---
> >  drivers/net/macb.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index c99cf663a4..fca380d012 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -296,13 +296,15 @@ static inline void macb_flush_ring_desc(struct macb_device *macb, bool rx)
> >  static inline void macb_flush_rx_buffer(struct macb_device *macb)
> >  {
> >         flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> > -                          ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
> > +                          ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
> > +                                PKTALIGN));
>
> This looks wrong to me.
>
> Shouldn't it be:
>
>          flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
>                           ALIGN(macb->rx_buffer_size, PKTALIGN));

Ah, for MACB, looks it should be "MACB_RX_BUFFER_SIZE *
MACB_RX_RING_SIZE", but for GEM, I am not sure.

Should it be:

          flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
                           ALIGN(macb->rx_buffer_size *
MACB_RX_RING_SIZE, PKTALIGN));

>
> >  }
> >
> >  static inline void macb_invalidate_rx_buffer(struct macb_device *macb)
> >  {
> >         invalidate_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> > -                               ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
> > +                               ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
> > +                                     PKTALIGN));
> >  }
> >
> >  #if defined(CONFIG_CMD_NET)
> > --

Regards,
Bin

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Bin Meng]

Hi Stefan,

On Fri, Aug 23, 2019 at 5:02 PM Stefan Roese <sr@denx.de> wrote:
>
> With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
> ethernet support does not work any more with d-cache enabled on the
> AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
> to 128 but this change was not refected in the rx_buffer flush and
> invalidate functions, as these also use this macro.
>
> This patch now fixes this by calculating the rx buffer size correctly
> again in those functions. With this change, ethernet works again
> reliably on my AT91SAM board.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> Cc: Anup Patel <anup.patel@wdc.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> ---
>  drivers/net/macb.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index c99cf663a4..fca380d012 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -296,13 +296,15 @@ static inline void macb_flush_ring_desc(struct macb_device *macb, bool rx)
>  static inline void macb_flush_rx_buffer(struct macb_device *macb)
>  {
>         flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> -                          ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
> +                          ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
> +                                PKTALIGN));

This looks wrong to me.

Shouldn't it be:

         flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
                          ALIGN(macb->rx_buffer_size, PKTALIGN));

>  }
>
>  static inline void macb_invalidate_rx_buffer(struct macb_device *macb)
>  {
>         invalidate_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
> -                               ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
> +                               ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
> +                                     PKTALIGN));
>  }
>
>  #if defined(CONFIG_CMD_NET)
> --

Regards,
Bin

[U-Boot] [PATCH] net: macb: Fix rx buffer cache handling

[Stefan Roese]

With commit c6d07bf440bc ("net/macb: increase RX buffer size for GEM")
ethernet support does not work any more with d-cache enabled on the
AT91SAM. The reason is, that MACB_RX_BUFFER_SIZE was changed from 4096
to 128 but this change was not refected in the rx_buffer flush and
invalidate functions, as these also use this macro.

This patch now fixes this by calculating the rx buffer size correctly
again in those functions. With this change, ethernet works again
reliably on my AT91SAM board.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Eugen Hristev <eugen.hristev@microchip.com>
Cc: Anup Patel <anup.patel@wdc.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Joe Hershberger <joe.hershberger@ni.com>
---
 drivers/net/macb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index c99cf663a4..fca380d012 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -296,13 +296,15 @@ static inline void macb_flush_ring_desc(struct macb_device *macb, bool rx)
 static inline void macb_flush_rx_buffer(struct macb_device *macb)
 {
 	flush_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
-			   ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
+			   ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
+				 PKTALIGN));
 }
 
 static inline void macb_invalidate_rx_buffer(struct macb_device *macb)
 {
 	invalidate_dcache_range(macb->rx_buffer_dma, macb->rx_buffer_dma +
-				ALIGN(MACB_RX_BUFFER_SIZE, PKTALIGN));
+				ALIGN(MACB_RX_BUFFER_SIZE * MACB_RX_RING_SIZE,
+				      PKTALIGN));
 }
 
 #if defined(CONFIG_CMD_NET)
-- 
2.23.0