diff mbox

[U-Boot] tsec: Configure the buffer descriptor bases to always include all of the descriptors

Message ID 1312960344-1499-1-git-send-email-joe.hershberger@ni.com
State Rejected
Headers show

Commit Message

Joe Hershberger Aug. 10, 2011, 7:12 a.m. UTC 
Previously only the last N were included based on the current one in use.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Cc: Joe Hershberger <joe.hershberger@gmail.com>
Cc: Mingkai Hu <Mingkai.hu@freescale.com>
Cc: Andy Fleming <afleming@freescale.com>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Detlev Zundel <dzu@denx.de>
---
 drivers/net/tsec.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Detlev Zundel Aug. 10, 2011, 12:29 p.m. UTC | #1 
Hi Joe,

> Previously only the last N were included based on the current one in use.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>
> Cc: Mingkai Hu <Mingkai.hu@freescale.com>
> Cc: Andy Fleming <afleming@freescale.com>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Detlev Zundel <dzu@denx.de>
> ---
>  drivers/net/tsec.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index 78ffc95..1805ca0 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -250,8 +250,8 @@ static void startup_tsec(struct eth_device *dev)
>  	txIdx = 0;
>  
>  	/* Point to the buffer descriptors */
> -	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
> -	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
> +	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
> +	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));
>  
>  	/* Initialize the Rx Buffer descriptors */
>  	for (i = 0; i < PKTBUFSRX; i++) {

I see these two lines just before the code you change (one is even in
the context of your patch):

        /* reset the indices to zero */
        rxIdx = 0;
        txIdx = 0;

So can you tell me, what your change actually does?  I cannot remember
that we have concurrency issues here, or do we?

Cheers
  Detlev
Andy Fleming Aug. 10, 2011, 2:10 p.m. UTC | #2 
On Aug 10, 2011, at 2:12 AM, Joe Hershberger wrote:

> Previously only the last N were included based on the current one in use.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>
> Cc: Mingkai Hu <Mingkai.hu@freescale.com>
> Cc: Andy Fleming <afleming@freescale.com>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Detlev Zundel <dzu@denx.de>


I'm curious if you were seeing a problem that this fixes?


> ---
> drivers/net/tsec.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index 78ffc95..1805ca0 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -250,8 +250,8 @@ static void startup_tsec(struct eth_device *dev)
> 	txIdx = 0;
> 
> 	/* Point to the buffer descriptors */
> -	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
> -	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
> +	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
> +	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));


However, while I don't believe this fixes a technical problem, I believe this makes the code more straightforward.

So if this is a fix to a problem, we need more information to understand what you're really fixing. If this is just fixing something that looked wrong...:

Acked-by: Andy Fleming <afleming@freescale.com>
Joe Hershberger Aug. 10, 2011, 7:15 p.m. UTC | #3 
On Wed, Aug 10, 2011 at 7:29 AM, Detlev Zundel <dzu@denx.de> wrote:
>> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
>> index 78ffc95..1805ca0 100644
>> --- a/drivers/net/tsec.c
>> +++ b/drivers/net/tsec.c
>> @@ -250,8 +250,8 @@ static void startup_tsec(struct eth_device *dev)
>>       txIdx = 0;
>>
>>       /* Point to the buffer descriptors */
>> -     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
>> -     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
>> +     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
>> +     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));
>>
>>       /* Initialize the Rx Buffer descriptors */
>>       for (i = 0; i < PKTBUFSRX; i++) {
>
> I see these two lines just before the code you change (one is even in
> the context of your patch):
>
>        /* reset the indices to zero */
>        rxIdx = 0;
>        txIdx = 0;
>
> So can you tell me, what your change actually does?  I cannot remember
> that we have concurrency issues here, or do we?

My apologies... I ported this patch from my work in u-boot 2009.11 and
did not notice that change above.  I think explicitly using 0 when
assigning the base address pointers is clearer, though.

It seems the resetting of the indexes to 0 was added by Andy Fleming
in 063c12633d5ad74d52152d9c358e715475e17629, though the log doesn't
discuss it..

Best regards,
-Joe
Joe Hershberger Aug. 10, 2011, 7:20 p.m. UTC | #4 
On Wed, Aug 10, 2011 at 9:10 AM, Andy Fleming <afleming@freescale.com> wrote:
>
> On Aug 10, 2011, at 2:12 AM, Joe Hershberger wrote:
>
>> Previously only the last N were included based on the current one in use.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Joe Hershberger <joe.hershberger@gmail.com>
>> Cc: Mingkai Hu <Mingkai.hu@freescale.com>
>> Cc: Andy Fleming <afleming@freescale.com>
>> Cc: Kumar Gala <galak@kernel.crashing.org>
>> Cc: Detlev Zundel <dzu@denx.de>
>
>
> I'm curious if you were seeing a problem that this fixes?

I was searching for a performance problem on the MPC8313, and
discovered this, which seemed wrong.  It was not, however, the source
of my problem.

>> ---
>> drivers/net/tsec.c |    4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
>> index 78ffc95..1805ca0 100644
>> --- a/drivers/net/tsec.c
>> +++ b/drivers/net/tsec.c
>> @@ -250,8 +250,8 @@ static void startup_tsec(struct eth_device *dev)
>>       txIdx = 0;
>>
>>       /* Point to the buffer descriptors */
>> -     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
>> -     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
>> +     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
>> +     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));
>
> However, while I don't believe this fixes a technical problem, I believe this makes the code more straightforward.

I agree.  It is more straightforward to use 0 explicitly.

> So if this is a fix to a problem, we need more information to understand what you're really fixing. If this is just fixing something that looked wrong...:
>
> Acked-by: Andy Fleming <afleming@freescale.com>

It fixes something that was wrong before you committed
063c12633d5ad74d52152d9c358e715475e17629, but at this point, it's just
cosmetic.

Best regards,
-Joe
Detlev Zundel Aug. 10, 2011, 7:24 p.m. UTC | #5 
Hi Joe,

> On Wed, Aug 10, 2011 at 7:29 AM, Detlev Zundel <dzu@denx.de> wrote:
>>> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
>>> index 78ffc95..1805ca0 100644
>>> --- a/drivers/net/tsec.c
>>> +++ b/drivers/net/tsec.c
>>> @@ -250,8 +250,8 @@ static void startup_tsec(struct eth_device *dev)
>>>       txIdx = 0;
>>>
>>>       /* Point to the buffer descriptors */
>>> -     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
>>> -     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
>>> +     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
>>> +     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));
>>>
>>>       /* Initialize the Rx Buffer descriptors */
>>>       for (i = 0; i < PKTBUFSRX; i++) {
>>
>> I see these two lines just before the code you change (one is even in
>> the context of your patch):
>>
>>        /* reset the indices to zero */
>>        rxIdx = 0;
>>        txIdx = 0;
>>
>> So can you tell me, what your change actually does?  I cannot remember
>> that we have concurrency issues here, or do we?
>
> My apologies... I ported this patch from my work in u-boot 2009.11 and
> did not notice that change above.  I think explicitly using 0 when
> assigning the base address pointers is clearer, though.
>
> It seems the resetting of the indexes to 0 was added by Andy Fleming
> in 063c12633d5ad74d52152d9c358e715475e17629, though the log doesn't
> discuss it..

Yes, I see - it even slipped my review :(  For the patch as such I don't
have a preference - looking at the code both ways really read the same
for me.

Cheers
  Detlev
Andy Fleming Aug. 10, 2011, 7:49 p.m. UTC | #6 
On Aug 10, 2011, at 2:24 PM, Detlev Zundel wrote:

> Hi Joe,
> 
>> On Wed, Aug 10, 2011 at 7:29 AM, Detlev Zundel <dzu@denx.de> wrote:
>>>> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
>>>> index 78ffc95..1805ca0 100644
>>>> --- a/drivers/net/tsec.c
>>>> +++ b/drivers/net/tsec.c
>>>> @@ -250,8 +250,8 @@ static void startup_tsec(struct eth_device *dev)
>>>>       txIdx = 0;
>>>> 
>>>>       /* Point to the buffer descriptors */
>>>> -     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
>>>> -     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
>>>> +     out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
>>>> +     out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));
>>>> 
>>>>       /* Initialize the Rx Buffer descriptors */
>>>>       for (i = 0; i < PKTBUFSRX; i++) {
>>> 
>>> I see these two lines just before the code you change (one is even in
>>> the context of your patch):
>>> 
>>>        /* reset the indices to zero */
>>>        rxIdx = 0;
>>>        txIdx = 0;
>>> 
>>> So can you tell me, what your change actually does?  I cannot remember
>>> that we have concurrency issues here, or do we?
>> 
>> My apologies... I ported this patch from my work in u-boot 2009.11 and
>> did not notice that change above.  I think explicitly using 0 when
>> assigning the base address pointers is clearer, though.
>> 
>> It seems the resetting of the indexes to 0 was added by Andy Fleming
>> in 063c12633d5ad74d52152d9c358e715475e17629, though the log doesn't
>> discuss it..
> 
> Yes, I see - it even slipped my review :(  For the patch as such I don't
> have a preference - looking at the code both ways really read the same
> for me.


Well, it wasn't added in that patch, exactly.  What really happened is I accidentally applied two patches, and then had to break them up again. That part accidentally got put in the second patch. A careful review of the patch history indicates that the indices have always been zeroed out beforehand (though sometimes in separate functions).

All the same, it looks like this patch is a good idea, to me.

Andy
Detlev Zundel Aug. 10, 2011, 8:59 p.m. UTC | #7 
Hi Andy,

[...]

>>> It seems the resetting of the indexes to 0 was added by Andy Fleming
>>> in 063c12633d5ad74d52152d9c358e715475e17629, though the log doesn't
>>> discuss it..
>> 
>> Yes, I see - it even slipped my review :(  For the patch as such I don't
>> have a preference - looking at the code both ways really read the same
>> for me.
>
>
> Well, it wasn't added in that patch, exactly.  What really happened is
> I accidentally applied two patches, and then had to break them up
> again. That part accidentally got put in the second patch. A careful
> review of the patch history indicates that the indices have always
> been zeroed out beforehand (though sometimes in separate functions).

It slipped my review nevertheless.

> All the same, it looks like this patch is a good idea, to me.

Then submit an acked-by which should help the patch along.

Cheers
  Detlev
Andy Fleming Aug. 10, 2011, 9:14 p.m. UTC | #8 
On Aug 10, 2011, at 2:12 AM, Joe Hershberger wrote:

> Previously only the last N were included based on the current one in use.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>
> Cc: Mingkai Hu <Mingkai.hu@freescale.com>
> Cc: Andy Fleming <afleming@freescale.com>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Detlev Zundel <dzu@denx.de>

Acked-by: Andy Fleming <afleming@freescale.com>
Sergei Shtylyov Aug. 11, 2011, 12:26 p.m. UTC | #9 
Hello.

On 10-08-2011 11:12, Joe Hershberger wrote:

> Previously only the last N were included based on the current one in use.

> Signed-off-by: Joe Hershberger<joe.hershberger@ni.com>
> Cc: Joe Hershberger<joe.hershberger@gmail.com>
> Cc: Mingkai Hu<Mingkai.hu@freescale.com>
> Cc: Andy Fleming<afleming@freescale.com>
> Cc: Kumar Gala<galak@kernel.crashing.org>
> Cc: Detlev Zundel<dzu@denx.de>
> ---
>   drivers/net/tsec.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)

> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index 78ffc95..1805ca0 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -250,8 +250,8 @@ static void startup_tsec(struct eth_device *dev)
>   	txIdx = 0;
>
>   	/* Point to the buffer descriptors */
> -	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
> -	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
> +	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
> +	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));

    Note that & and [0] are not really needed.

WBR, Sergei
Wolfgang Denk Aug. 24, 2011, 10:46 p.m. UTC | #10 
Dear Joe Hershberger,

In message <CANr=Z=aNQtW_YV-XcqsE8f2RPDpYYMhKL7WWSrcPa_pqZKj8uw@mail.gmail.com> you wrote:
>
> It fixes something that was wrong before you committed
> 063c12633d5ad74d52152d9c358e715475e17629, but at this point, it's just
> cosmetic.

I don't think this is worth to change the code.  Can you accept that
we drop this patch?

Best regards,

Wolfgang Denk
Joe Hershberger Aug. 24, 2011, 10:48 p.m. UTC | #11 
On Wed, Aug 24, 2011 at 5:46 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Joe Hershberger,
>
> In message <CANr=Z=aNQtW_YV-XcqsE8f2RPDpYYMhKL7WWSrcPa_pqZKj8uw@mail.gmail.com> you wrote:
>>
>> It fixes something that was wrong before you committed
>> 063c12633d5ad74d52152d9c358e715475e17629, but at this point, it's just
>> cosmetic.
>
> I don't think this is worth to change the code.  Can you accept that
> we drop this patch?

Yes, it is fine to drop this patch.

-Joe
diff mbox

Patch

diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
index 78ffc95..1805ca0 100644
--- a/drivers/net/tsec.c
+++ b/drivers/net/tsec.c
@@ -250,8 +250,8 @@  static void startup_tsec(struct eth_device *dev)
 	txIdx = 0;
 
 	/* Point to the buffer descriptors */
-	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
-	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
+	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[0]));
+	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[0]));
 
 	/* Initialize the Rx Buffer descriptors */
 	for (i = 0; i < PKTBUFSRX; i++) {