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

Joe Hershberger

10 Aug 2011 10 Aug '11

9:12 a.m.

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

Show replies by date

Detlev Zundel

10 Aug 10 Aug

2:29 p.m.

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@denx.de

Joe Hershberger

9:15 p.m.

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

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

Detlev Zundel

9:24 p.m.

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@denx.de

Andy Fleming

9:49 p.m.

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

10:59 p.m.

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@denx.de

Andy Fleming

4:10 p.m.

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

9:20 p.m.

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

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

Wolfgang Denk

25 Aug 25 Aug

12:46 a.m.

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@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

Joe Hershberger

12:48 a.m.

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

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

Andy Fleming

10 Aug 10 Aug

11:14 p.m.

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

11 Aug 11 Aug

2:26 p.m.

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

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 Hershbergerjoe.hershberger@ni.com Cc: Joe Hershbergerjoe.hershberger@gmail.com Cc: Mingkai HuMingkai.hu@freescale.com Cc: Andy Flemingafleming@freescale.com Cc: Kumar Galagalak@kernel.crashing.org Cc: Detlev Zundeldzu@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

5005

Age (days ago)

5019

Last active (days ago)

List overview

Download

11 comments

6 participants

tags (0)

participants (6)

Andy Fleming
Detlev Zundel
Joe Hershberger
Joe Hershberger
Sergei Shtylyov
Wolfgang Denk