[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[Andre Schwarz]

The Vitesse VSC8601 RGMII PHY has internal delay for both Rx
and Tx clock lines. They are configured using 2 bits in extended
register 0x17.
Therefore CFG_VSC8601_SKEW_TX and CFG_VSC8601_SKEW_RX have
been introduced with valid values 0-3 giving 0.0, 1.4,1.7 and 2.0ns delay.

Signed-off-by: Andre Schwarz <andre.schwarz@matrix-vision.de>
--

 drivers/net/tsec.c |    6 ++++++
 drivers/net/tsec.h |    4 ++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
index 9d22aa3..06250ae 100644
--- a/drivers/net/tsec.c
+++ b/drivers/net/tsec.c
@@ -1277,6 +1277,12 @@ struct phy_info phy_info_VSC8601 = {
                 {MIIM_CONTROL, MIIM_CONTROL_INIT, &mii_cr_init},
 #ifdef CFG_VSC8601_SKEWFIX
                
{MIIM_VSC8601_EPHY_CON,MIIM_VSC8601_EPHY_CON_INIT_SKEW,NULL},
+#if defined(CFG_VSC8601_SKEW_TX) && defined(CFG_VSC8601_SKEW_RX)
+                {MIIM_EXT_PAGE_ACCESS,1,NULL},
+#define VSC8101_SKEW    (CFG_VSC8601_SKEW_TX<<14)|(CFG_VSC8601_SKEW_RX<<12)
+                {MIIM_VSC8601_SKEW_CTRL,VSC8101_SKEW,NULL},
+                {MIIM_EXT_PAGE_ACCESS,0,NULL},
+#endif
 #endif
                 {miim_end,}
                  },
diff --git a/drivers/net/tsec.h b/drivers/net/tsec.h
index cfa7d1a..e8776b0 100644
--- a/drivers/net/tsec.h
+++ b/drivers/net/tsec.h
@@ -112,6 +112,8 @@
 #define MIIM_GBIT_CONTROL    0x9
 #define MIIM_GBIT_CONTROL_INIT    0xe00
 
+#define MIIM_EXT_PAGE_ACCESS    0x1f
+
 /* Broadcom BCM54xx -- taken from linux sungem_phy */
 #define MIIM_BCM54xx_AUXSTATUS            0x19
 #define MIIM_BCM54xx_AUXSTATUS_LINKMODE_MASK    0x0700
@@ -163,6 +165,8 @@
 /* Vitesse VSC8601 Extended PHY Control Register 1 */
 #define MIIM_VSC8601_EPHY_CON            0x17
 #define MIIM_VSC8601_EPHY_CON_INIT_SKEW    0x1120
+#define MIIM_VSC8601_SKEW_CTRL        0x1c
+#define MIIM_VSC8601_EPHY_CON_INIT_SKEW    0x1120
 
 /* 88E1011 PHY Status Register */
 #define MIIM_88E1011_PHY_STATUS         0x11


MATRIX VISION GmbH, Talstra?e 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Gesch?ftsf?hrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[Andy Fleming]

On Thu, Apr 24, 2008 at 9:45 AM, Andre Schwarz
<andre.schwarz@matrix-vision.de> wrote:

>  {MIIM_VSC8601_EPHY_CON,MIIM_VSC8601_EPHY_CON_INIT_SKEW,NULL},
>  +#if defined(CFG_VSC8601_SKEW_TX) && defined(CFG_VSC8601_SKEW_RX)
>  +                {MIIM_EXT_PAGE_ACCESS,1,NULL},
>  +#define VSC8101_SKEW    (CFG_VSC8601_SKEW_TX<<14)|(CFG_VSC8601_SKEW_RX<<12)
>  +                {MIIM_VSC8601_SKEW_CTRL,VSC8101_SKEW,NULL},
>  +                {MIIM_EXT_PAGE_ACCESS,0,NULL},
>  +#endif
>   #endif


I'm not sure this is the best solution to this.  It seems like it
wouldn't scale well.  Either we need to set a bit somewhere that the
phy driver can read (and thereby determine how to configure the skew),
or we need to set the value to write in the board config file.  I'm
partial to the first solution, as it encapsulates the information
inside the code that deals with it.

[...]

>   /* Broadcom BCM54xx -- taken from linux sungem_phy */
>   #define MIIM_BCM54xx_AUXSTATUS            0x19
>   #define MIIM_BCM54xx_AUXSTATUS_LINKMODE_MASK    0x0700
>  @@ -163,6 +165,8 @@
>   /* Vitesse VSC8601 Extended PHY Control Register 1 */
>   #define MIIM_VSC8601_EPHY_CON            0x17
>   #define MIIM_VSC8601_EPHY_CON_INIT_SKEW    0x1120
>  +#define MIIM_VSC8601_SKEW_CTRL        0x1c
>  +#define MIIM_VSC8601_EPHY_CON_INIT_SKEW    0x1120

Am I crazy, or did you just doubly define MIIM_VSC8601_EPHY_CON_INIT_SKEW?

[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[Andre Schwarz]

Andy,

thanks for your comments.


Andy Fleming schrieb:
> On Thu, Apr 24, 2008 at 9:45 AM, Andre Schwarz
> <andre.schwarz@matrix-vision.de> wrote:
>
>   
>>  {MIIM_VSC8601_EPHY_CON,MIIM_VSC8601_EPHY_CON_INIT_SKEW,NULL},
>>  +#if defined(CFG_VSC8601_SKEW_TX) && defined(CFG_VSC8601_SKEW_RX)
>>  +                {MIIM_EXT_PAGE_ACCESS,1,NULL},
>>  +#define VSC8101_SKEW    (CFG_VSC8601_SKEW_TX<<14)|(CFG_VSC8601_SKEW_RX<<12)
>>  +                {MIIM_VSC8601_SKEW_CTRL,VSC8101_SKEW,NULL},
>>  +                {MIIM_EXT_PAGE_ACCESS,0,NULL},
>>  +#endif
>>   #endif
>>     
>
>
> I'm not sure this is the best solution to this.  It seems like it
> wouldn't scale well.  Either we need to set a bit somewhere that the
> phy driver can read (and thereby determine how to configure the skew),
> or we need to set the value to write in the board config file.  I'm
> partial to the first solution, as it encapsulates the information
> inside the code that deals with it.
>
> [...]
>
>   
I don't understand "scale well". What should be scalable ?

Of course using a function would be better.
I silently assumed that the other bits are set to zero which is true
after reset.
There are only two other bits : Packet size and 10M preamble mode.
Both should be left untouched, i.e. "0".

>>   /* Broadcom BCM54xx -- taken from linux sungem_phy */
>>   #define MIIM_BCM54xx_AUXSTATUS            0x19
>>   #define MIIM_BCM54xx_AUXSTATUS_LINKMODE_MASK    0x0700
>>  @@ -163,6 +165,8 @@
>>   /* Vitesse VSC8601 Extended PHY Control Register 1 */
>>   #define MIIM_VSC8601_EPHY_CON            0x17
>>   #define MIIM_VSC8601_EPHY_CON_INIT_SKEW    0x1120
>>  +#define MIIM_VSC8601_SKEW_CTRL        0x1c
>>  +#define MIIM_VSC8601_EPHY_CON_INIT_SKEW    0x1120
>>     
>
> Am I crazy, or did you just doubly define MIIM_VSC8601_EPHY_CON_INIT_SKEW?
>   
This obviously is a mistake - sorry.



regards,
Andre

MATRIX VISION GmbH, Talstra?e 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Gesch?ftsf?hrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[Andy Fleming]

On Tue, Apr 29, 2008 at 1:58 AM, Andre Schwarz
<andre.schwarz@matrix-vision.de> wrote:
> Andy,
>
>  thanks for your comments.
>
>
>  Andy Fleming schrieb:
>
> > On Thu, Apr 24, 2008 at 9:45 AM, Andre Schwarz
>  > <andre.schwarz@matrix-vision.de> wrote:
>  >
>  >
>  >>  {MIIM_VSC8601_EPHY_CON,MIIM_VSC8601_EPHY_CON_INIT_SKEW,NULL},
>  >>  +#if defined(CFG_VSC8601_SKEW_TX) && defined(CFG_VSC8601_SKEW_RX)
>  >>  +                {MIIM_EXT_PAGE_ACCESS,1,NULL},
>  >>  +#define VSC8101_SKEW    (CFG_VSC8601_SKEW_TX<<14)|(CFG_VSC8601_SKEW_RX<<12)
>  >>  +                {MIIM_VSC8601_SKEW_CTRL,VSC8101_SKEW,NULL},
>  >>  +                {MIIM_EXT_PAGE_ACCESS,0,NULL},
>  >>  +#endif
>  >>   #endif
>  >>
>  >
>  >
>  > I'm not sure this is the best solution to this.  It seems like it
>  > wouldn't scale well.  Either we need to set a bit somewhere that the
>  > phy driver can read (and thereby determine how to configure the skew),
>  > or we need to set the value to write in the board config file.  I'm
>  > partial to the first solution, as it encapsulates the information
>  > inside the code that deals with it.
>  >
>  > [...]
>  >
>  >
>  I don't understand "scale well". What should be scalable ?
>
>  Of course using a function would be better.
>  I silently assumed that the other bits are set to zero which is true
>  after reset.
>  There are only two other bits : Packet size and 10M preamble mode.
>  Both should be left untouched, i.e. "0".
>


Sorry, I should have been more explicit.  In this case I'm referring
to how well the solution scales as we deal with more configuration
options.  Your solution isn't a terrible example of this, but #ifdefs
that seem straightforward when there is one of them quickly can become
unmanageable.  I'll admit, though, this is the sort of problem that is
best solved by rearchitecting the PHY code in tsec.c, which is, I
believe, under way by Ben.

In light of the fact that the #ifdefs here are feature-based, and that
the PHY code is probably changing soon, anyway, I'd be ok with letting
this patch through (with the duplicate #define removed, of course).

Andy

[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[André Schwarz]

Andy,

it's no problem to re-send a "more suitable" patch as soon as Ben has 
finished the re-work. I'll wait for him and send an updated version to 
the list.


Cheers,
Andr?

Andy Fleming schrieb:
> On Tue, Apr 29, 2008 at 1:58 AM, Andre Schwarz
> <andre.schwarz@matrix-vision.de> wrote:
>> Andy,
>>
>>  thanks for your comments.
>>
>>
>>  Andy Fleming schrieb:
>>
>>> On Thu, Apr 24, 2008 at 9:45 AM, Andre Schwarz
>>  > <andre.schwarz@matrix-vision.de> wrote:
>>  >
>>  >
>>  >>  {MIIM_VSC8601_EPHY_CON,MIIM_VSC8601_EPHY_CON_INIT_SKEW,NULL},
>>  >>  +#if defined(CFG_VSC8601_SKEW_TX) && defined(CFG_VSC8601_SKEW_RX)
>>  >>  +                {MIIM_EXT_PAGE_ACCESS,1,NULL},
>>  >>  +#define VSC8101_SKEW    (CFG_VSC8601_SKEW_TX<<14)|(CFG_VSC8601_SKEW_RX<<12)
>>  >>  +                {MIIM_VSC8601_SKEW_CTRL,VSC8101_SKEW,NULL},
>>  >>  +                {MIIM_EXT_PAGE_ACCESS,0,NULL},
>>  >>  +#endif
>>  >>   #endif
>>  >>
>>  >
>>  >
>>  > I'm not sure this is the best solution to this.  It seems like it
>>  > wouldn't scale well.  Either we need to set a bit somewhere that the
>>  > phy driver can read (and thereby determine how to configure the skew),
>>  > or we need to set the value to write in the board config file.  I'm
>>  > partial to the first solution, as it encapsulates the information
>>  > inside the code that deals with it.
>>  >
>>  > [...]
>>  >
>>  >
>>  I don't understand "scale well". What should be scalable ?
>>
>>  Of course using a function would be better.
>>  I silently assumed that the other bits are set to zero which is true
>>  after reset.
>>  There are only two other bits : Packet size and 10M preamble mode.
>>  Both should be left untouched, i.e. "0".
>>
> 
> 
> Sorry, I should have been more explicit.  In this case I'm referring
> to how well the solution scales as we deal with more configuration
> options.  Your solution isn't a terrible example of this, but #ifdefs
> that seem straightforward when there is one of them quickly can become
> unmanageable.  I'll admit, though, this is the sort of problem that is
> best solved by rearchitecting the PHY code in tsec.c, which is, I
> believe, under way by Ben.
> 
> In light of the fact that the #ifdefs here are feature-based, and that
> the PHY code is probably changing soon, anyway, I'd be ok with letting
> this patch through (with the duplicate #define removed, of course).
> 
> Andy


MATRIX VISION GmbH, Talstra?e 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Gesch?ftsf?hrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[Ben Warren]

Andr? Schwarz wrote:
> Andy,
>
> it's no problem to re-send a "more suitable" patch as soon as Ben has 
> finished the re-work. I'll wait for him and send an updated version to 
> the list.
>
>
>   
It's better to get this in now if you don't mind.

regards,
Ben

[U-Boot-Users] [PATCH] add config options for VSC8601 RGMII PHY

[Andre Schwarz]

Ben Warren schrieb:
> Andr? Schwarz wrote:
>> Andy,
>>
>> it's no problem to re-send a "more suitable" patch as soon as Ben has
>> finished the re-work. I'll wait for him and send an updated version
>> to the list.
>>
>>
>>   
> It's better to get this in now if you don't mind.
>
> regards,
> Ben
I don't mind and will send it a a new patch.


Cheers,
Andre

MATRIX VISION GmbH, Talstra?e 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Gesch?ftsf?hrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner