[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[Peter Tyser]

Fix TBI PHY accesses to use the proper offset in CPU register space.  The
previous code would incorrectly access the TBI PHY by reading/writing to CPU
register space at the same location as would be used to access external PHYs.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---

Dohh... v2 of the patch introduced a compile warning.  Please apply
this version instead.

I replaced the hardcoding of CFG_TBIPA_VALUE in the tsec_configure_serdes()
to support possible changes of the TBI addressing down the road, but
kept the default TBI PHY address at CFG_TBIPA_VALUE as Andy Flemming
suggested.

 drivers/net/tsec.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
index f81211a..cc7d528 100644
--- a/drivers/net/tsec.c
+++ b/drivers/net/tsec.c
@@ -283,11 +283,13 @@ uint tsec_local_mdio_read(volatile tsec_t *phyregs, uint phyid, uint regnum)
 /* Configure the TBI for SGMII operation */
 static void tsec_configure_serdes(struct tsec_private *priv)
 {
-	tsec_local_mdio_write(priv->phyregs, CFG_TBIPA_VALUE, TBI_ANA,
+	/* Access TBI PHY registers at given TSEC register offset as opposed to the
+	 * register offset used for external PHY accesses */
+	tsec_local_mdio_write(priv->regs, priv->regs->tbipa, TBI_ANA,
 			TBIANA_SETTINGS);
-	tsec_local_mdio_write(priv->phyregs, CFG_TBIPA_VALUE, TBI_TBICON,
+	tsec_local_mdio_write(priv->regs, priv->regs->tbipa, TBI_TBICON,
 			TBICON_CLK_SELECT);
-	tsec_local_mdio_write(priv->phyregs, CFG_TBIPA_VALUE, TBI_CR,
+	tsec_local_mdio_write(priv->regs, priv->regs->tbipa, TBI_CR,
 			TBICR_SETTINGS);
 }
 
@@ -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");
 
 	/* Reset MII (due to new addresses) */
-- 
1.5.4.3

[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[Ben Warren]

Andy,

You're much more qualified than me to accept or reject this, so if you 
don't mind, please pick it up.

regards,
Ben

Peter Tyser wrote:
> Fix TBI PHY accesses to use the proper offset in CPU register space.  The
> previous code would incorrectly access the TBI PHY by reading/writing to CPU
> register space at the same location as would be used to access external PHYs.
>
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> ---
>
> Dohh... v2 of the patch introduced a compile warning.  Please apply
> this version instead.
>
> I replaced the hardcoding of CFG_TBIPA_VALUE in the tsec_configure_serdes()
> to support possible changes of the TBI addressing down the road, but
> kept the default TBI PHY address at CFG_TBIPA_VALUE as Andy Flemming
> suggested.
>
>  drivers/net/tsec.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index f81211a..cc7d528 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -283,11 +283,13 @@ uint tsec_local_mdio_read(volatile tsec_t *phyregs, uint phyid, uint regnum)
>  /* Configure the TBI for SGMII operation */
>  static void tsec_configure_serdes(struct tsec_private *priv)
>  {
> -	tsec_local_mdio_write(priv->phyregs, CFG_TBIPA_VALUE, TBI_ANA,
> +	/* Access TBI PHY registers at given TSEC register offset as opposed to the
> +	 * register offset used for external PHY accesses */
> +	tsec_local_mdio_write(priv->regs, priv->regs->tbipa, TBI_ANA,
>  			TBIANA_SETTINGS);
> -	tsec_local_mdio_write(priv->phyregs, CFG_TBIPA_VALUE, TBI_TBICON,
> +	tsec_local_mdio_write(priv->regs, priv->regs->tbipa, TBI_TBICON,
>  			TBICON_CLK_SELECT);
> -	tsec_local_mdio_write(priv->phyregs, CFG_TBIPA_VALUE, TBI_CR,
> +	tsec_local_mdio_write(priv->regs, priv->regs->tbipa, TBI_CR,
>  			TBICR_SETTINGS);
>  }
>  
> @@ -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");
>  
>  	/* Reset MII (due to new addresses) */
>   

[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[Andy Fleming]

> @@ -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.

Andy

[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[Peter Tyser]

On Mon, 2008-09-15 at 16:13 -0500, Andy Fleming wrote:
> > @@ -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.

For example, if a board had 4 sgmii interfaces with 4 external PHYs on
an mpc8572 - all 4 PHYs could be physically connected to the MDIO bus of
TSEC1.  The cpu address offsets of the 4 TSECs would be:
TSEC1 regs    = 0x24000
TSEC1 phyregs = 0x24000
TSEC2 regs    = 0x25000
TSEC2 phyregs = 0x24000
TSEC3 regs    = 0x26000
TSEC3 phyregs = 0x24000
TSEC4 regs    = 0x27000
TSEC4 phyregs = 0x24000

With the old code, on bootup the ethernet initialization would go like:

configure TSEC1 - write TSEC1's TBIPA address, write TSEC1's TBIPA
address (2nd write to TSEC1 TBIPA)

configure TSEC2 - write TSEC2's TBIPA address, write TSEC1's TBIPA
address (3rd time)

configure TSEC3 - write TSEC3's TBIPA address, write TSEC1's TBIPA
address (4th time)

configure TSEC4 - write TSEC4's TBIPA address, write TSEC1's TBIPA
address (5th time)

So the old code would write TSEC1's TBIPA address 5 times.

The patch I submitted does the following:
configure TSEC1 - write TSEC1's TBIPA address

configure TSEC2 - write TSEC2's TBIPA address

configure TSEC3 - write TSEC3's TBIPA address

configure TSEC4 - write TSEC4's TBIPA address

This 2nd method seems correct to me - each TSECs TBIPA address should
only be set for itself, not itself as well as the TSEC its using to
access its external PHY.

Best,
Peter

[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[Andy Fleming]

On Mon, Sep 15, 2008 at 5:17 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
> On Mon, 2008-09-15 at 16:13 -0500, Andy Fleming wrote:
>> > @@ -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:

1) TSEC0 is not enabled
2) 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.

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.  :)

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.

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.

Andy

[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[Peter Tyser]

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:
> 
> 1) TSEC0 is not enabled
> 2) 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

[U-Boot] [PATCH v3] Support for multiple SGMII/TBI interfaces for TSEC ethernet

[Andy Fleming]

> 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.


Oh, there is much confusion, here.

1) 0 is reserved, according to the spec.  However, *most* Freescale
development systems use address zero for the PHY connected to TSEC0.
2) The manual only recently added this information, perhaps to
encourage our own board designers to pay attention to it.  :)
3) If the TBI PHY doesn't show up on a scan, it is because we have
done something to make them ignored.  I used to see the TBI PHY all
the time, and I think we changed things so it didn't show up anymore
(to avoid confusion).
4) The current default value for CFG_TBIPA_VALUE is 0x1f, not 0.  So
keep that in mind.

Anyway, I think we should leave in that write for now.  In the future,
we will change the code to sensibly move the TBIPA out of the way,
like Linux does.


> 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.

Yeah, I'm not sure what it's reserved for, but many board designers
ignore that, and put the first PHY at 0.

> 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.

No extra MII writes should be done.  Just a few memory-mapped io
register writes.

>
>>
>> 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.


Sorry, I should have been clearer -- we support accessing the TBI PHY
connected to TSEC0.  The others require knowing that you have to use
different registers.  One solution might be to create 4 MDIO buses,
but that seems excessive.  We might want to just create a
tsec-specific command to deal with it.  Anyway, thank you for bearing
with me.


>
> 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.

Please do.

Thanks,
Andy