net: Add 405EX support to new EMAC driver
Subsystems (2)
PATCH 1/1 net: Add 405EX support to new EMAC driver
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.
--- 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(-)
drivers/net/ibm_newemac/core.c +2 -1
@@ -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" ))
drivers/net/ibm_newemac/rgmii.c +11 -5
@@ -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 ();
drivers/net/ibm_newemac/rgmii.h +1 -1
@@ -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 {
Signature · 1 line
1.5.3.4.498.g9c514
Quoted 2 lines · 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.
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
Quoted 6 lines · Olof Johansson
Quoted 2 lines · 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.
Hi,
Quoted 8 lines
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.
Quoted 21 lines · Benjamin Herrenschmidt
Quoted 6 lines · Olof Johansson
Quoted 2 lines · 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.
Hi,
Quoted 8 lines
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.
Quoted 8 lines
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. :-)
Quoted 3 lines
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 :-)
Quoted 2 lines
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
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 6 lines
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>;
};Quoted 6 lines
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.
Quoted 6 lines
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
Quoted 10 lines · Stefan Roese
Quoted 3 lines
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
Quoted 1 line
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?
Quoted 8 lines
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.
Quoted 9 lines
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.
Quoted 6 lines
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.
Quoted 12 lines · Benjamin Herrenschmidt
Quoted 11 lines
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; };
Quoted 5 lines
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.
Quoted 7 lines
Quoted 6 lines
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?
Quoted 8 lines
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