[PATCH] net: Add 405EX support to new EMAC driver

[PATCH] net: Add 405EX support to new EMAC driver

[Stefan Roese]

This patch adds support for the 405EX to the new EMAC driver.

Tested on AMCC Kilauea.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 drivers/net/ibm_newemac/core.c  |    3 ++-
 drivers/net/ibm_newemac/rgmii.c |    6 ------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 0de3aa2..fd0a585 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -2466,7 +2466,8 @@ static int __devinit emac_init_config(struct emac_instance *dev)
 	if (of_device_is_compatible(np, "ibm,emac4"))
 		dev->features |= EMAC_FTR_EMAC4;
 	if (of_device_is_compatible(np, "ibm,emac-axon")
-	    || of_device_is_compatible(np, "ibm,emac-440epx"))
+	    || of_device_is_compatible(np, "ibm,emac-440epx")
+	    || of_device_is_compatible(np, "ibm,emac-405ex"))
 		dev->features |= EMAC_FTR_HAS_AXON_STACR
 			| EMAC_FTR_STACR_OC_INVERT;
 	if (of_device_is_compatible(np, "ibm,emac-440spe"))
diff --git a/drivers/net/ibm_newemac/rgmii.c b/drivers/net/ibm_newemac/rgmii.c
index de41695..e393f68 100644
--- a/drivers/net/ibm_newemac/rgmii.c
+++ b/drivers/net/ibm_newemac/rgmii.c
@@ -140,9 +140,6 @@ void rgmii_get_mdio(struct of_device *ofdev, int input)
 
 	RGMII_DBG2(dev, "get_mdio(%d)" NL, input);
 
-	if (dev->type != RGMII_AXON)
-		return;
-
 	mutex_lock(&dev->lock);
 
 	fer = in_be32(&p->fer);
@@ -161,9 +158,6 @@ void rgmii_put_mdio(struct of_device *ofdev, int input)
 
 	RGMII_DBG2(dev, "put_mdio(%d)" NL, input);
 
-	if (dev->type != RGMII_AXON)
-		return;
-
 	fer = in_be32(&p->fer);
 	fer &= ~(0x00080000u >> input);
 	out_be32(&p->fer, fer);
-- 
1.5.3.4.498.g9c514

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Benjamin Herrenschmidt]

On Thu, 2007-11-01 at 15:54 +0100, Stefan Roese wrote:
> This patch adds support for the 405EX to the new EMAC driver.
> 
> Tested on AMCC Kilauea.

 .../...

> diff --git a/drivers/net/ibm_newemac/rgmii.c b/drivers/net/ibm_newemac/rgmii.c
> index de41695..e393f68 100644
> --- a/drivers/net/ibm_newemac/rgmii.c
> +++ b/drivers/net/ibm_newemac/rgmii.c
> @@ -140,9 +140,6 @@ void rgmii_get_mdio(struct of_device *ofdev, int input)
>  
>  	RGMII_DBG2(dev, "get_mdio(%d)" NL, input);
>  
> -	if (dev->type != RGMII_AXON)
> -		return;
> -
>  	mutex_lock(&dev->lock);

That will break 440GX boards that need to use the RGMII for data and the
ZMII for MDIO.

You may want to change the name RGMII_AXON something like RGMII_HAS_MDIO
instead and set that for 405EX as well instead.

Ben.

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Josh Boyer]

On Fri, 02 Nov 2007 07:37:01 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Thu, 2007-11-01 at 15:54 +0100, Stefan Roese wrote:
> > This patch adds support for the 405EX to the new EMAC driver.
> > 
> > Tested on AMCC Kilauea.
> 
>  .../...
> 
> > diff --git a/drivers/net/ibm_newemac/rgmii.c b/drivers/net/ibm_newemac/rgmii.c
> > index de41695..e393f68 100644
> > --- a/drivers/net/ibm_newemac/rgmii.c
> > +++ b/drivers/net/ibm_newemac/rgmii.c
> > @@ -140,9 +140,6 @@ void rgmii_get_mdio(struct of_device *ofdev, int input)
> >  
> >  	RGMII_DBG2(dev, "get_mdio(%d)" NL, input);
> >  
> > -	if (dev->type != RGMII_AXON)
> > -		return;
> > -
> >  	mutex_lock(&dev->lock);
> 
> That will break 440GX boards that need to use the RGMII for data and the
> ZMII for MDIO.
> 
> You may want to change the name RGMII_AXON something like RGMII_HAS_MDIO
> instead and set that for 405EX as well instead.

And perhaps adding a comment about that since the meaning of that code
isn't very obvious. That way people that aren't the original author
of the driver don't get confused again.

josh

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Stefan Roese]

On Thursday 01 November 2007, Josh Boyer wrote:
> > > -	if (dev->type != RGMII_AXON)
> > > -		return;
> > > -
> > >  	mutex_lock(&dev->lock);
> >
> > That will break 440GX boards that need to use the RGMII for data and the
> > ZMII for MDIO.
> >
> > You may want to change the name RGMII_AXON something like RGMII_HAS_MDIO
> > instead and set that for 405EX as well instead.
>
> And perhaps adding a comment about that since the meaning of that code
> isn't very obvious. That way people that aren't the original author
> of the driver don't get confused again.

Will do. I'll send a fixed version tomorrow.

Best regards,
Stefan

[PATCH] net: Add 405EX support to new EMAC driver

[Stefan Roese]

This patch adds support for the 405EX to the new EMAC driver. Some as on
AXON, the 405EX handles the MDIO via the RGMII bridge.

Tested on AMCC Kilauea.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 drivers/net/ibm_newemac/core.c  |    3 ++-
 drivers/net/ibm_newemac/rgmii.c |   16 +++++++++++-----
 drivers/net/ibm_newemac/rgmii.h |    2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 0de3aa2..fd0a585 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -2466,7 +2466,8 @@ static int __devinit emac_init_config(struct emac_instance *dev)
 	if (of_device_is_compatible(np, "ibm,emac4"))
 		dev->features |= EMAC_FTR_EMAC4;
 	if (of_device_is_compatible(np, "ibm,emac-axon")
-	    || of_device_is_compatible(np, "ibm,emac-440epx"))
+	    || of_device_is_compatible(np, "ibm,emac-440epx")
+	    || of_device_is_compatible(np, "ibm,emac-405ex"))
 		dev->features |= EMAC_FTR_HAS_AXON_STACR
 			| EMAC_FTR_STACR_OC_INVERT;
 	if (of_device_is_compatible(np, "ibm,emac-440spe"))
diff --git a/drivers/net/ibm_newemac/rgmii.c b/drivers/net/ibm_newemac/rgmii.c
index de41695..b9a4ce7 100644
--- a/drivers/net/ibm_newemac/rgmii.c
+++ b/drivers/net/ibm_newemac/rgmii.c
@@ -140,7 +140,12 @@ void rgmii_get_mdio(struct of_device *ofdev, int input)
 
 	RGMII_DBG2(dev, "get_mdio(%d)" NL, input);
 
-	if (dev->type != RGMII_AXON)
+	/*
+	 * Some platforms (e.g. 440GX) have RGMII support but don't use it for
+	 * MDIO access. Only continue if platforms is using MDIO over the RGMII
+	 * interface (e.g. AXON, 405EX).
+	 */
+	if (dev->type != RGMII_HAS_MDIO)
 		return;
 
 	mutex_lock(&dev->lock);
@@ -161,7 +166,7 @@ void rgmii_put_mdio(struct of_device *ofdev, int input)
 
 	RGMII_DBG2(dev, "put_mdio(%d)" NL, input);
 
-	if (dev->type != RGMII_AXON)
+	if (dev->type != RGMII_HAS_MDIO)
 		return;
 
 	fer = in_be32(&p->fer);
@@ -251,8 +256,9 @@ static int __devinit rgmii_probe(struct of_device *ofdev,
 	}
 
 	/* Check for RGMII type */
-	if (of_device_is_compatible(ofdev->node, "ibm,rgmii-axon"))
-		dev->type = RGMII_AXON;
+	if (of_device_is_compatible(ofdev->node, "ibm,rgmii-axon") ||
+	    of_device_is_compatible(ofdev->node, "ibm,rgmii-405ex"))
+		dev->type = RGMII_HAS_MDIO;
 	else
 		dev->type = RGMII_STANDARD;
 
@@ -264,7 +270,7 @@ static int __devinit rgmii_probe(struct of_device *ofdev,
 
 	printk(KERN_INFO
 	       "RGMII %s %s initialized\n",
-	       dev->type == RGMII_STANDARD ? "standard" : "axon",
+	       dev->type == RGMII_STANDARD ? "standard" : "has-mdio",
 	       ofdev->node->full_name);
 
 	wmb();
diff --git a/drivers/net/ibm_newemac/rgmii.h b/drivers/net/ibm_newemac/rgmii.h
index 5780683..f1b0ef5 100644
--- a/drivers/net/ibm_newemac/rgmii.h
+++ b/drivers/net/ibm_newemac/rgmii.h
@@ -23,7 +23,7 @@
 
 /* RGMII bridge type */
 #define RGMII_STANDARD		0
-#define RGMII_AXON		1
+#define RGMII_HAS_MDIO		1
 
 /* RGMII bridge */
 struct rgmii_regs {
-- 
1.5.3.4.498.g9c514

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Olof Johansson]

On Fri, Nov 02, 2007 at 08:14:43AM +0100, Stefan Roese wrote:
> This patch adds support for the 405EX to the new EMAC driver. Some as on
> AXON, the 405EX handles the MDIO via the RGMII bridge.

Hi,

This isn't feedback on your patch as much as on "new-emac" in general:

Isn't this the case where there should really be device tree properties
instead? If you had an "ibm,emac-has-axon-stacr" property in the device
node, then you don't have to modify the driver for every new board out
there. Same for the other device properties, of course.

I thought this was what having the device tree was all about. :(


-Olof

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Benjamin Herrenschmidt]

On Fri, 2007-11-02 at 11:03 -0500, Olof Johansson wrote:
> On Fri, Nov 02, 2007 at 08:14:43AM +0100, Stefan Roese wrote:
> > This patch adds support for the 405EX to the new EMAC driver. Some as on
> > AXON, the 405EX handles the MDIO via the RGMII bridge.
> 
> Hi,
> 
> This isn't feedback on your patch as much as on "new-emac" in general:
> 
> Isn't this the case where there should really be device tree properties
> instead? If you had an "ibm,emac-has-axon-stacr" property in the device
> node, then you don't have to modify the driver for every new board out
> there. Same for the other device properties, of course.
> 
> I thought this was what having the device tree was all about. :(

Somewhat yeah. There are subtle variations here or there we haven't
totally indenfified... It might be a better option in our case here to
add "has-mdio" to the rgmii nodes indeed.

Part of the problem with those cells is that the chip folks keep
changing things subtly from one rev to another though, it's not even
totally clear to me yet whether the RGMII registers are totally
compatible betwee axon and 405ex, which is why I've pretty much stuck to
"compatible" properties to identify the variants.

The device-tree can do both. It's still better than no device-tree since
at least you know what cell variant is in there.

As for the STACR, Axon isn't the first one to have that bit flipped, I
think we should name the property differently, something like
"stacr-oc-inverted".

We can still use properties that way for new things in fact. As for EMAC
on cell, well, I can always put some fixup somewhere.

Ben. 

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Olof Johansson]

On Sun, Nov 04, 2007 at 02:37:59PM +1100, Benjamin Herrenschmidt wrote:
> 
> On Fri, 2007-11-02 at 11:03 -0500, Olof Johansson wrote:
> > On Fri, Nov 02, 2007 at 08:14:43AM +0100, Stefan Roese wrote:
> > > This patch adds support for the 405EX to the new EMAC driver. Some as on
> > > AXON, the 405EX handles the MDIO via the RGMII bridge.
> > 
> > Hi,
> > 
> > This isn't feedback on your patch as much as on "new-emac" in general:
> > 
> > Isn't this the case where there should really be device tree properties
> > instead? If you had an "ibm,emac-has-axon-stacr" property in the device
> > node, then you don't have to modify the driver for every new board out
> > there. Same for the other device properties, of course.
> > 
> > I thought this was what having the device tree was all about. :(
> 
> Somewhat yeah. There are subtle variations here or there we haven't
> totally indenfified... It might be a better option in our case here to
> add "has-mdio" to the rgmii nodes indeed.
> 
> Part of the problem with those cells is that the chip folks keep
> changing things subtly from one rev to another though, it's not even
> totally clear to me yet whether the RGMII registers are totally
> compatible betwee axon and 405ex, which is why I've pretty much stuck to
> "compatible" properties to identify the variants.
> 
> The device-tree can do both. It's still better than no device-tree since
> at least you know what cell variant is in there.

Well, it's better than compile-time ifdefs. Providing what version of
the device you have CAN be done without a device tree too. :-)

> As for the STACR, Axon isn't the first one to have that bit flipped, I
> think we should name the property differently, something like
> "stacr-oc-inverted".

Sure, it was the habit of having to modify the driver for platforms that
don't add any new features I was against. I don't really care what the
properties are called :-)

> We can still use properties that way for new things in fact. As for EMAC
> on cell, well, I can always put some fixup somewhere.

Sounds good (with s/can still/should/).


-Olof

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Stefan Roese]

On Sunday 04 November 2007, Benjamin Herrenschmidt wrote:
> > Isn't this the case where there should really be device tree properties
> > instead? If you had an "ibm,emac-has-axon-stacr" property in the device
> > node, then you don't have to modify the driver for every new board out
> > there. Same for the other device properties, of course.
> >
> > I thought this was what having the device tree was all about. :(
>
> Somewhat yeah. There are subtle variations here or there we haven't
> totally indenfified... It might be a better option in our case here to
> add "has-mdio" to the rgmii nodes indeed.

So how exactly do you want me to handle this (I'm still new to this device 
tree stuff, so please bear with me)? Like this?

			RGMII0: emac-rgmii@ef601000 {
				device_type = "rgmii-interface";
				compatible = "ibm,rgmii-405ex", "ibm,rgmii";
				reg = <ef601000 8>;
				has-mdio;
			};

Or add this to the compatible property?

			RGMII0: emac-rgmii@ef601000 {
				device_type = "rgmii-interface";
				compatible = "ibm,rgmii-405ex", "ibm,rgmii", "ibm,has-mdio";
				reg = <ef601000 8>;
			};

> Part of the problem with those cells is that the chip folks keep
> changing things subtly from one rev to another though, it's not even
> totally clear to me yet whether the RGMII registers are totally
> compatible betwee axon and 405ex, which is why I've pretty much stuck to
> "compatible" properties to identify the variants.
>
> The device-tree can do both. It's still better than no device-tree since
> at least you know what cell variant is in there.
>
> As for the STACR, Axon isn't the first one to have that bit flipped, I
> think we should name the property differently, something like
> "stacr-oc-inverted".

It's not only the OC bit-flip on AXON, but also the different STACR register 
layout for read/write op-codes (STAOPC). This seems to be the same on all new 
EMAC core's like on AXON, 440EPx/GRx and 405EX. So "stacr-oc-inverted" is not 
enough here. This is what is needed for 440SPe, which "only" has the bit-flip 
and the "old" STAOPC layout.

So perhaps most flexible would be to add individual properties, 
like "stacr-oc-inverted" and "stacr-staopc-19-20". What do you think? And 
again the additional question: Should the be added as an new property or 
added to the compatible property?

Please advise. Thanks.

Best regards,
Stefan

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Benjamin Herrenschmidt]

On Mon, 2007-11-05 at 10:19 +0100, Stefan Roese wrote:
> 
> > Somewhat yeah. There are subtle variations here or there we haven't
> > totally indenfified... It might be a better option in our case here
> to
> > add "has-mdio" to the rgmii nodes indeed.
> 
> So how exactly do you want me to handle this (I'm still new to this
> device 
> tree stuff, so please bear with me)? Like this?
> 
>                         RGMII0: emac-rgmii@ef601000 {
>                                 device_type = "rgmii-interface";
>                                 compatible = "ibm,rgmii-405ex",
> "ibm,rgmii";
>                                 reg = <ef601000 8>;
>                                 has-mdio;
>                         };
> 

The above.

Properties without values are typically used for such "flags". I'll
fixup the driver to also take that for the inverted STACR and will post
a patch fixing that up asap.

> It's not only the OC bit-flip on AXON, but also the different STACR
> register 
> layout for read/write op-codes (STAOPC). This seems to be the same on
> all new 
> EMAC core's like on AXON, 440EPx/GRx and 405EX. So "stacr-oc-inverted"
> is not 
> enough here. This is what is needed for 440SPe, which "only" has the
> bit-flip 
> and the "old" STAOPC layout.

Ok.

> So perhaps most flexible would be to add individual properties, 
> like "stacr-oc-inverted" and "stacr-staopc-19-20". What do you think?
> And 
> again the additional question: Should the be added as an new property
> or 
> added to the compatible property?

That's always the main question imho ... When it gets nasty like that I
tend to think the compatible property is a good compromise. It's mostly
a matter of taste. Unless you can come up with some more pleasant way to
do it... maybe a stacr-type property with multiple values but it's
really not worth complicating things when a compatible property will do
the job just fine. In that case, it's not really a "feature" of a given
implementation, but more about subtle differences between
implementations.

Ben.

Re: [PATCH] net: Add 405EX support to new EMAC driver

[Stefan Roese]

On Monday 05 November 2007, Benjamin Herrenschmidt wrote:
> > So how exactly do you want me to handle this (I'm still new to this
> > device
> > tree stuff, so please bear with me)? Like this?
> >
> >                         RGMII0: emac-rgmii@ef601000 {
> >                                 device_type = "rgmii-interface";
> >                                 compatible = "ibm,rgmii-405ex",
> > "ibm,rgmii";
> >                                 reg = <ef601000 8>;
> >                                 has-mdio;
> >                         };
>
> The above.
>
> Properties without values are typically used for such "flags". I'll
> fixup the driver to also take that for the inverted STACR and will post
> a patch fixing that up asap.

OK, thanks. I'll wait with further 405ex emac patches until you changed this 
in the driver.

> > So perhaps most flexible would be to add individual properties,
> > like "stacr-oc-inverted" and "stacr-staopc-19-20". What do you think?
> > And
> > again the additional question: Should the be added as an new property
> > or
> > added to the compatible property?
>
> That's always the main question imho ... When it gets nasty like that I
> tend to think the compatible property is a good compromise. It's mostly
> a matter of taste. Unless you can come up with some more pleasant way to
> do it... maybe a stacr-type property with multiple values but it's
> really not worth complicating things when a compatible property will do
> the job just fine. In that case, it's not really a "feature" of a given
> implementation, but more about subtle differences between
> implementations.

OK.

Best regards,
Stefan