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(®s->tbase, (unsigned int)(&rtx.txbd[txIdx]));out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
out_be32(®s->tbase, (unsigned int)(&rtx.txbd[0]));out_be32(®s->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