From: David Gibson <hidden> Date: 2007-05-29 06:06:12
This patch fixes some problems with the way the Ethernet PHYs are
represented in the device tree for the Holly board. This means
changes to the dts itself, and to the code with instantiates the
tsi108 ethernet platform devices based on the device tree.
- First, and most importantly, the PHYs are given with an
identical 'reg' property. This reg currently encodes the accessible
register used to initiate mdio interaction with the PHYs, rather than
a meaningful address on the parent bus (mdio in this case), which is
incorrect. Instead we give the address of these registers as 'reg' in
the mdio node itself, and encode the ID of each phy in their 'reg'
propertyies. The PHY's unit name addresses are updated to match.
- Second, the PHYs give only "bcm54xx" as a compatible
property. This is broken, since there are many bcm54xx PHY models,
and they have differences which can matter. We replace the compatible
property with the precise PHY model, bcm5461a in this case.
- Third, the node representing the mdio link had a compatible
property of "tsi-ethernet", identical to the ethernet MAC nodes. This
is clearly incorrect, since it's a different sort of device, and
replaced with "tsi-mdio".
- Finally, currently the platform device constructor enables a
workaround in the tsi108 ethernet driver based on the compatible
property of the PHY. This is incorrect, because the workaround in
question is necessary due to the board's wiring of the PHY, not the
model of PHY itself. This patch alters the constructor to instead
enable the workaround based on a new special property in the PHY node.
Signed-off-by: David Gibson <redacted>
---
Fixed various problems pointed out in feedback.
Index: working-2.6/arch/powerpc/boot/dts/holly.dts
===================================================================
@@ -75,9 +75,8 @@ static int __init tsi108_eth_of_init(voi(np=of_find_compatible_node(np,"network","tsi-ethernet"))!=NULL;i++){structresourcer[2];-structdevice_node*phy;+structdevice_node*phy,*mdio;hw_infotsi_eth_data;-constunsignedint*id;constunsignedint*phy_id;constvoid*mac_addr;constphandle*ph;
@@ -111,6 +110,13 @@ static int __init tsi108_eth_of_init(voiif(mac_addr)memcpy(tsi_eth_data.mac_addr,mac_addr,6);+ph=of_get_property(np,"mdio-handle",NULL);+mdio=of_find_node_by_phandle(*ph);+ret=of_address_to_resource(mdio,0,&res);+of_node_put(mdio);+if(ret)+gotounreg;+ph=of_get_property(np,"phy-handle",NULL);phy=of_find_node_by_phandle(*ph);
@@ -119,20 +125,25 @@ static int __init tsi108_eth_of_init(voigotounreg;}-id=of_get_property(phy,"reg",NULL);-phy_id=of_get_property(phy,"phy-id",NULL);-ret=of_address_to_resource(phy,0,&res);-if(ret){-of_node_put(phy);-gotounreg;-}+phy_id=of_get_property(phy,"reg",NULL);+tsi_eth_data.regs=r[0].start;tsi_eth_data.phyregs=res.start;tsi_eth_data.phy=*phy_id;tsi_eth_data.irq_num=irq_of_parse_and_map(np,0);-if(of_device_is_compatible(phy,"bcm54xx"))++/* Some boards with the TSI108 bridge (e.g. Holly)+*haveamiswiringoftheethernetPHYswhich+*requiresaworkaround.Thespecial+*"txc-rxc-delay-disable"propertyenablesthis+*workaround.FIXME:Needtoportthetsi108_eth+*driveritselftophylibanduseanon-misleading+*namefortheworkaroundflag-it'snotactuallyto+*dowiththemodelofPHYinuse*/+if(of_get_property(phy,"txc-rxc-delay-disable",NULL))tsi_eth_data.phy_type=TSI108_PHY_BCM54XX;of_node_put(phy);+ret=platform_device_add_data(tsi_eth_dev,&tsi_eth_data,sizeof(hw_info));
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Hrm, did I miss this before? A more exact "compatible"
property would be better ("tsi109-mdio" "tsi108-mdio" or
something like that).
Rest looks good,
Segher
Hrm, did I miss this before? A more exact "compatible"
property would be better ("tsi109-mdio" "tsi108-mdio" or
something like that).
If we must, then tsi108-mdio is what I would recommend. They are the
same between 108, 109, and 110 as far as I know, so it's the lowest
common denominator.
josh
Hrm, did I miss this before? A more exact "compatible"
property would be better ("tsi109-mdio" "tsi108-mdio" or
something like that).
If we must, then tsi108-mdio is what I would recommend. They are the
same between 108, 109, and 110 as far as I know, so it's the lowest
common denominator.
[Assuming what is really on the board is a tsi109...]
I recommend putting both 109 and 108 in the property, in
that order; that way, if you need to do something special
on the tsi109 implementation (something you might not yet
know about perhaps, maybe a bug; or some extra feature on
the tsi109 device that the driver doesn't handle yet), the
driver has a chance to do that. If the driver doesn't care
and only uses tsi108 features, it obviously can probe for
tsi108 only and all will be fine as well.
Segher
On Tue, 2007-05-29 at 16:49 +0200, Segher Boessenkool wrote:
quoted
quoted
quoted
+ compatible = "tsi-mdio";
Hrm, did I miss this before? A more exact "compatible"
property would be better ("tsi109-mdio" "tsi108-mdio" or
something like that).
If we must, then tsi108-mdio is what I would recommend. They are the
same between 108, 109, and 110 as far as I know, so it's the lowest
common denominator.
[Assuming what is really on the board is a tsi109...]
For Holly, yes. For Taiga, it's 108. For Hackberry, it's 110.
I recommend putting both 109 and 108 in the property, in
that order; that way, if you need to do something special
on the tsi109 implementation (something you might not yet
know about perhaps, maybe a bug; or some extra feature on
the tsi109 device that the driver doesn't handle yet), the
driver has a chance to do that. If the driver doesn't care
and only uses tsi108 features, it obviously can probe for
tsi108 only and all will be fine as well.
*shrug* Either way works for me.
We're adding these compatible properties to DTS files and the drivers at
the same time. Unless (until?) there are firmwares for these boards
that start specifying something else in a real device tree, it really
doesn't matter much. Not that there's anything wrong with your
reasoning. Just seems like we're trying really hard to define something
"correctly" when we control what's on both sides :).
josh
Hrm, did I miss this before? A more exact "compatible"
property would be better ("tsi109-mdio" "tsi108-mdio" or
something like that).
Hrm, yeah. I thought about that, but left it as tsi-mdio to match the
other rather non-specific compatible properties ("tsi-ethernet",
"tsi-brige", "tsi-i2c" and so forth).
But I guess I'd better fix that too. Next version of the patch fixes
a whole bunch of places to use tsi108/tsi109 compatible properties.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-05-30 11:18:08
On Tue, 2007-05-29 at 14:29 -0500, Josh Boyer wrote:
*shrug* Either way works for me.
We're adding these compatible properties to DTS files and the drivers
at
the same time. Unless (until?) there are firmwares for these boards
that start specifying something else in a real device tree, it really
doesn't matter much. Not that there's anything wrong with your
reasoning. Just seems like we're trying really hard to define
something
"correctly" when we control what's on both sides :).
I do think Segher is right there... compatible 109 and then 108
Ben.
We're adding these compatible properties to DTS files and the drivers
at
the same time. Unless (until?) there are firmwares for these boards
that start specifying something else in a real device tree, it really
doesn't matter much.
Sure. So you have time to work out things now, I
suggest you take advantage of that :-)
Not that there's anything wrong with your
reasoning. Just seems like we're trying really hard to define
something
"correctly" when we control what's on both sides :).
Right now, and in your case, you do. OTOH, the
goal is to have the DTS be a well-established
stable interface between the firmware/bootloader/
bootwrapper and the kernel; there is no room for
either side of that interface playing dirty tricks,
not on any board ;-)
Also, the DTS files in the kernel source tree should
server as a best-of-breed example for people doing
custom device trees for their own boards. We better
whip them into good shape or we'll all look foolish...
Segher
On Wed, 2007-05-30 at 13:36 +0200, Segher Boessenkool wrote:
quoted
We're adding these compatible properties to DTS files and the drivers
at
the same time. Unless (until?) there are firmwares for these boards
that start specifying something else in a real device tree, it really
doesn't matter much.
Sure. So you have time to work out things now, I
suggest you take advantage of that :-)
quoted
Not that there's anything wrong with your
reasoning. Just seems like we're trying really hard to define
something
"correctly" when we control what's on both sides :).
Right now, and in your case, you do. OTOH, the
goal is to have the DTS be a well-established
stable interface between the firmware/bootloader/
bootwrapper and the kernel; there is no room for
either side of that interface playing dirty tricks,
not on any board ;-)
Also, the DTS files in the kernel source tree should
server as a best-of-breed example for people doing
custom device trees for their own boards. We better
whip them into good shape or we'll all look foolish...
Yeah, I know. Ignore my earlier email. I blame it on lack of sleep.
The only issue we might have in the future is if DT capable firmware for
these boards shows up and does something completely different.
Hopefully that won't happen.
josh
Not that there's anything wrong with your
reasoning. Just seems like we're trying really hard to define
something
"correctly" when we control what's on both sides :).
Right now, and in your case, you do. OTOH, the
goal is to have the DTS be a well-established
stable interface between the firmware/bootloader/
bootwrapper and the kernel; there is no room for
either side of that interface playing dirty tricks,
not on any board ;-)
Also, the DTS files in the kernel source tree should
server as a best-of-breed example for people doing
custom device trees for their own boards. We better
whip them into good shape or we'll all look foolish...
Yeah, I know. Ignore my earlier email. I blame it on lack of sleep.
The only issue we might have in the future is if DT capable firmware
for
these boards shows up and does something completely different.
Yeah, that's exactly the same problem as we would have
if we would code our device trees without trying to at
least create an informal binding for the nodes in question:
total chaos.
Hopefully that won't happen.
Hopefully, indeed.
If a third party constructs a board with some weird
device tree, then they probably have a big set of Linux
patches to go with that. Now either they work with the
kernel community to get that integrated into mainline
(which means they need to do a lot of changes to the DTS
as well if it indeed is weird / wrong, so they better
start doing that *before* selling the boards); or they
can happily maintain their own kernel fork, like so many
companies already do.
I don't see a problem here :-)
Segher
From: David Gibson <hidden> Date: 2007-05-31 01:37:36
On Wed, May 30, 2007 at 09:17:15PM +1000, Benjamin Herrenschmidt wrote:
On Tue, 2007-05-29 at 14:29 -0500, Josh Boyer wrote:
quoted
*shrug* Either way works for me.
We're adding these compatible properties to DTS files and the drivers
at
the same time. Unless (until?) there are firmwares for these boards
that start specifying something else in a real device tree, it really
doesn't matter much. Not that there's anything wrong with your
reasoning. Just seems like we're trying really hard to define
something
"correctly" when we control what's on both sides :).
I do think Segher is right there... compatible 109 and then 108
That's what I've done in the new patch.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson