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

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

[Joe Hershberger]

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++) {
-- 
1.6.0.2

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

[Detlev Zundel]

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

-- 
Don't trust everything you read, and don't assume every poster in
a thread is actually relevant to the problem.
        -- Stefan Monnier <jwvlj1gk44h.fsf-monnier+emacs@gnu.org>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

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

[Joe Hershberger]

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

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

[Detlev Zundel]

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

-- 
I've never understood the tendency to pick up tastes because they are popular.
In fact,  I think it is  foolish to do that.  I mean, don't  you know what you
like?  People who  are so weak that  they will take  their tastes  from people
around  them  in  the  desperate  desire  to be  accepted,  I think of them as
cowards.               -- Richard M. Stallman
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

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

[Andy Fleming]

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

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

[Detlev Zundel]

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

-- 
F?r jemanden, der in eine Religion geboren wurde, in der das Ringen um eine
einzige Seele ein Stafettenlauf ?ber viele Jahrhunderte sein kann [..], hat
das Tempo des Christentums etwas Schwindelerregendes.   Wenn der Hinduismus
friedlich dahinflie?t wie der Ganges,  dann ist das  Christentum Toronto in
der Rushhour.                        -- Yann Martel
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

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

[Andy Fleming]

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>

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

[Joe Hershberger]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If I have seen further it is by standing on the shoulders of  giants.
              - Isaac Newton, Letter to Robert Hooke, 5 February 1676

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

[Joe Hershberger]

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

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

[Andy Fleming]

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>

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

[Sergei Shtylyov]

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