Hi Andy,
@@ -299,12 +301,10 @@ static int init_phy(struct eth_device *dev) { struct tsec_private *priv = (struct tsec_private *)dev->priv; struct phy_info *curphy;
volatile tsec_t *phyregs = priv->phyregs; volatile tsec_t *regs = priv->regs; /* Assign a Physical address to the TBI */ regs->tbipa = CFG_TBIPA_VALUE;phyregs->tbipa = CFG_TBIPA_VALUE; asm("sync");What was the purpose of doing this? The problem I have with it is in the odd situation where the TSEC whose MII regs are connected to the bus is not enabled. It would mean that the TBIPA would never be set to CFG_TBIPA_VALUE.
I don't quite understand what you mean. My understanding was that if a TSEC is not enabled, the TBIPA for that TSEC should not be enabled either.
The original code was writing the TBIPA value 2 times for every TSEC - once at the TSEC register address in cpu space, and a second time at the TSEC's MII register address in cpu space.
Yes, unfortunately, it might be necessary. Imagine this scenario:
- TSEC0 is not enabled
- The current value of TSEC0's TBIPA register contains the address of
a PHY we want to use 3) One of the other TSECs comes up, and tries to access its PHY at that address.
You see, the TBIPA register setting has *two* purposes. Most controllers aren't connected to any external PHYs (via their MII regs), so the purpose of TBIPA is to tell the controller what address you want to use to access the TBI PHY. However, for controllers attached to an external PHY, setting the TBIPA register serves not only to define where the TBI PHY is, but where *other* PHYs are not. If TSEC0's TBIPA register is not set, it might conflict with a PHY that doesn't belong to TSEC0.
According to the 8548 and 8572 manuals (not sure about others...), the TBIPA register value for all PHYs is 0x0 at reset, which is reserved according to the manuals. The description of the MIIMADD register supports this by stating: "Up to 31 PHYs can be addressed (0 is reserved)". So I believe that if the TBIPA value is 0, the corresponding TBI PHY can't be accessed and thus doesn't cause any problems as far as overlapping with other PHY addresses. The fact that the TBI phy never shows up when probing the MII bus on a variety of 8548 boards (which don't use any TBI PHYs) I've used supports this.
Also, in the above example, if you were to write the TBIPA register of TSEC0 which did not have an external PHY with a non-zero value, I believe this unused TBI PHY would show up when probing the MII bus (see comments below) which would be a bit confusing to a user. It would already be confusing to see 8 PHYs listed for 4 interfaces - 9 would make it even less sane:)
I was under the impression that PHY address 0 was reserved for broadcast usage, or something similar. I don't have the PHY spec handy to verify that however.
It's admittedly an unlikely corner case, but I trust in the ability of board designers to break my code in such ways. My trust has been validated several times. :)
Agreed wholeheartedly:)
So while I agree with you that it's almost always wasteful, it's necessary without more significant changes. One other option would be to only write priv->phyregs->tbipa (n times, even though it would almost always be written to the same place all n times). Then, priv->regs->tbipa would contain whatever was in the register to start with, which is fine, since the address only matters for the regs which access external PHYs.
If you feel more comfortable leaving the write to priv->phyregs->tbipa in place, its OK with me. I don't think its necessary (at least for the 8548 and 8572 processors) but the only damage it does is some extra MII writes and adding a non-existent TBI PHY which a user might find a bit confusing.
One other note: currently we don't support accessing the TBI PHYs through the mii utilities. This is mostly due to difficulties in finding addresses for the TBI PHYs that don't conflict with existing PHY addresses. If you have some clever ideas that don't involve hard-coding the addresses (I have to anticipate the advent of chips with 10+ ethernet controllers), I'm open to hearing them. :) It's an issue I intend to address, but it's not getting any timeslices this quarter.
FWIW, I was able to see the TBI PHY on an 8572 board using U-Boot's mii command. I only had one TBI PHY configured, but I could read its registers at address 0x1f as desired. I was surprised this worked and didn't dig into why it did however... I'm not sure how the mii command would work with multiple TBI PHYs at 0x1f on different busses however, but if it doesn't work that seems like an issue with the mii subsystem more than the tsec driver in my opinion. We have a board in the pipeline with multiple TBI PHYs - perhaps I can dig into it then.
Thanks for the feedback. I'd vote for keeping the patch the same as it is now, but if you'd like me to resubmit with the "phyregs->tbipa = CFG_TBIPA_VALUE" put back in let me know and I'll send it again.
Thanks again, Peter