Re: [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 <d...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot