Re: Fix silly thinko in sungem network driver.

Re: Fix silly thinko in sungem network driver.

[Benjamin Herrenschmidt]

On Sat, 2004-02-21 at 09:29, Linux Kernel Mailing List wrote:
> ChangeSet 1.1584, 2004/02/20 14:29:11-08:00, torvalds@ppc970.osdl.org
> 
> 	Fix silly thinko in sungem network driver.
> 	

Argh. Actually, the patch is wrong as we are losing the 2 other
magic bits (the RonPaul bit, dunno what it is, that's how it's
called by Apple, end the other bugfix bit, some more apple magic).

Fixed patch on its way ... (as soon as I finish pulling, don't
cancel that cset, I'll apply a patch on top of it)

Ben.

Re: Fix silly thinko in sungem network driver.

[Benjamin Herrenschmidt]

On Sat, 2004-02-21 at 11:04, Benjamin Herrenschmidt wrote:
> On Sat, 2004-02-21 at 09:29, Linux Kernel Mailing List wrote:
> > ChangeSet 1.1584, 2004/02/20 14:29:11-08:00, torvalds@ppc970.osdl.org
> > 
> > 	Fix silly thinko in sungem network driver.
> > 	
> 
> Argh. Actually, the patch is wrong as we are losing the 2 other
> magic bits (the RonPaul bit, dunno what it is, that's how it's
> called by Apple, end the other bugfix bit, some more apple magic).
> 
> Fixed patch on its way ... (as soon as I finish pulling, don't
> cancel that cset, I'll apply a patch on top of it)

Here it is:

--- linux-2.5/drivers/net/sungem.c	2004-02-21 11:10:33.390479384 +1100
+++ linuxppc-2.5-benh/drivers/net/sungem.c	2004-02-21 11:11:03.448909808 +1100
@@ -1837,7 +1837,8 @@
 	 * thresholds (and Apple bug fixes don't exist)
 	 */
 	if (!(readl(gp->regs + GREG_CFG) & GREG_CFG_IBURST)) {
-		cfg = ((2 << 1) & GREG_CFG_TXDMALIM);
+		cfg &= ~(GREG_CFG_TXDMALIM | GREG_CFG_TXDMALIM);
+		cfg |= ((2 << 1) & GREG_CFG_TXDMALIM);
 		cfg |= ((8 << 6) & GREG_CFG_RXDMALIM);
 		writel(cfg, gp->regs + GREG_CFG);
 	}	

Re: Fix silly thinko in sungem network driver.

[Benjamin Herrenschmidt]

> Here it is:
>
> .../...

And here's the version without the typo 

(I shall not code before having breakfast...)

--- linux-2.5/drivers/net/sungem.c	2004-02-21 11:10:33.390479384 +1100
+++ linuxppc-2.5-benh/drivers/net/sungem.c	2004-02-21 11:18:23.396027680 +1100
@@ -1837,7 +1837,8 @@
 	 * thresholds (and Apple bug fixes don't exist)
 	 */
 	if (!(readl(gp->regs + GREG_CFG) & GREG_CFG_IBURST)) {
-		cfg = ((2 << 1) & GREG_CFG_TXDMALIM);
+		cfg &= ~(GREG_CFG_TXDMALIM | GREG_CFG_RXDMALIM);
+		cfg |= ((2 << 1) & GREG_CFG_TXDMALIM);
 		cfg |= ((8 << 6) & GREG_CFG_RXDMALIM);
 		writel(cfg, gp->regs + GREG_CFG);
 	}	

Re: Fix silly thinko in sungem network driver.

[David S. Miller]

On Sat, 21 Feb 2004 11:12:03 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Sat, 2004-02-21 at 11:04, Benjamin Herrenschmidt wrote:
> > On Sat, 2004-02-21 at 09:29, Linux Kernel Mailing List wrote:
> > > ChangeSet 1.1584, 2004/02/20 14:29:11-08:00, torvalds@ppc970.osdl.org
> > > 
> > > 	Fix silly thinko in sungem network driver.
> > > 	
> > 
> > Argh. Actually, the patch is wrong as we are losing the 2 other
> > magic bits (the RonPaul bit, dunno what it is, that's how it's
> > called by Apple, end the other bugfix bit, some more apple magic).
> > 
> > Fixed patch on its way ... (as soon as I finish pulling, don't
> > cancel that cset, I'll apply a patch on top of it)
> 
> Here it is:

I thought the idea was that if IBURST doesn't stick, then the Apple specific
bits aren't implemented?

That's what the comment says.

Re: Fix silly thinko in sungem network driver.

[Benjamin Herrenschmidt]

On Sat, 2004-02-21 at 11:23, David S. Miller wrote:

> I thought the idea was that if IBURST doesn't stick, then the Apple specific
> bits aren't implemented?
> 
> That's what the comment says.

I though the IBURST that doesn't stick was specific to latest Apple versions ?
Those latest Apple versions are _also_ the ones implementing the magic bug
fix bits (those appeared with the G5).

Or did I get it backward ? Hrm, maybe I did... Here's Apple code:

	fConfiguration	= kConfiguration_TX_DMA_Limit		// default Configuration value
					| kConfiguration_RX_DMA_Limit
					| kConfiguration_Infinite_Burst
					| kConfiguration_RonPaulBit
					| kConfiguration_EnableBug2Fix;
	WRITE_REGISTER( Configuration, fConfiguration );	// try the default

	ui32 = READ_REGISTER( Configuration );				// read it back
    if ( (ui32 & kConfiguration_Infinite_Burst) == 0 )	
    {													// not infinite-burst capable:
        ELG( 0, 0, 'Lims', "UniNEnet::initChip: set TX_DMA_Limit and RX_DMA_Limit." );
		fConfiguration	= (0x02 << 1) | (0x08 << 6);	// change TX_DMA_Limit, RX_DMA_Limit
		WRITE_REGISTER( Configuration, fConfiguration );
    }

What does your doco says ?

Ben.

Re: Fix silly thinko in sungem network driver.

[Benjamin Herrenschmidt]

And some more data:

Apple .h says:

#define kConfiguration_Infinite_Burst	0x00000001	// for Tx only
#define kConfiguration_RonPaulBit		0x00000800	// for pci reads at end of infinite burst, next command is memory read multiple
#define kConfiguration_EnableBug2Fix	0x00001000	// fix Rx hang after overflow

And the OF firmware (forth) driver does:

config rl@ 801 or config rl!

Which is interesting, that is it sets the RonPaulBit and the Infinite
Burst bit, and doesn't test the result...

Linus, does your G5 infinite burst bit sticks at all ? I think the
idea is that _older_ apple ones didn't have infinite burst, while newer
(G5) ones do, but also need the bug fix bits.

Which means the code Linus has is ok (without my latest patch).

Ben.

Re: Fix silly thinko in sungem network driver.

[David S. Miller]

On Sat, 21 Feb 2004 11:24:50 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Or did I get it backward ? Hrm, maybe I did... Here's Apple code:

This matches the current code, with Linus's fix applied.

> What does your doco says ?

It just says that all Sun versions of GEM support the IBURST bit.

It seems clear to me that the current code in Linus's tree is correct
and matches what Apple is doing.

Re: Fix silly thinko in sungem network driver.

[Benjamin Herrenschmidt]

> It just says that all Sun versions of GEM support the IBURST bit.
> 
> It seems clear to me that the current code in Linus's tree is correct
> and matches what Apple is doing.

Yes it does. it's a bad week for me apparently. I think most of
the patches I sent to linus lately had at least a typo, now that ...
pffff....

Ben.