Re: [U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches

[Kevin Smith]

Hi Joe,

Thanks for the review.  ACK to all comments.  A few clarifications are 
included below.  I will submit a v3, including multichip addressing for 
Albert's platform.

On 01/26/2016 06:11 PM, Joe Hershberger wrote:
>> +   int reg, u16 data)
>> +{
>> +   struct mii_dev *phys_bus;
> Why "phys_bus"? Isn't it an mdio_bus? Maybe that would be a better name.
Agreed.  This is extracting the actual mdio bus from the "fake" indirect 
mii device (see below).  I will change to a better name.
>> +
>> +static int mv88e61xx_probe(struct phy_device *phydev)
>> +{
>> +   struct mii_dev *mii_dev;
>> +
>> +   /* This device requires indirect reads/writes to the PHY registers
>> +* which the generic PHY code can't handle.  Make a fake MII device 
>> to
>> +* handle reads/writes */
>> +   mii_dev = mdio_alloc();
>> +   if (!mii_dev)
>> +   return -1;
>>
>> +
>> +   /* Store the actual bus in the fake mii device */
>> +   mii_dev->priv = phydev->bus;
>> +   strncpy(mii_dev->name, "mv88e61xx_protocol", sizeof(mii_dev->name));
>> +   mii_dev->read = mv88e61xx_phy_read_bus;
>> +   mii_dev->write = mv88e61xx_phy_write_bus;
>> +
>> +   /* Replace the bus with the fake device */
> Fake how? This is a confusing comment to me as written.
The genphy functions assume that they can write to the PHY directly 
using the MII bus, and the address it uses is the address of a 
register.  This is not the case for this chip with multiple PHY 
interfaces, which have to be accessed indirectly.  To handle this, I 
have created a "fake" mii_dev whose read/write functions are the 
indirection functions, and stored the actual mdio_bus in the private 
data for the "fake" device, which is then used by the indirect 
functions.  This allows this driver to make use of common genphy stuff 
where appropriate.  Maybe "wrapper" or "indirect bus" is a better name 
for it.  Let me know if you have a preference or better idea.
>> +
>> +   mac_addr = phydev->addr;
>> +
>> +   for (i = 0; i < PORT_COUNT; i++) {
>> +   if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
>> +   phydev->addr = i;
>> +   mv88e61xx_phy_enable(phydev, i);
>> +   mv88e61xx_phy_setup(phydev, i);
>> +   mv88e61xx_phy_config_port(phydev, i);
> These all return status, but are ignored.
Even if one fails, it seems appropriate to me to continue initializing 
the others and not bail completely.  Should I catch the error, print a 
warning and "continue" in the loop?  Or is completely bailing the right 
thing to do?  If there are some errors, but other successes, what should 
the function return?
>> +
>> +
>> +
>> +   /* After reset, the energy detect signal remains high for a few 
>> seconds
>> +* regardless of whether a cable is connected.  This function will
>> +* return false positives during this time. */
> This is OK?
Yes.  This is used to skip the autonegotiation process if nothing is 
plugged into the port.  If you have 5-6 ports enabled but only one cable 
plugged in, waiting for all the other autonegotiation timeouts takes a 
long time.  This tries to be smart and not wait when nothing is plugged 
in.  A false positive just tries to autonegotiate.  After the first 
autonegotiate timeout, the energy detection hysteresis (or whatever) has 
corrected itself, so you only have to wait for one.

Thanks,
Kevin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches

[Albert ARIBAUD]

Hello Kevin,

On Wed, 27 Jan 2016 16:29:42 +, Kevin Smith
 wrote:
> Hi Joe,

> On 01/26/2016 06:11 PM, Joe Hershberger wrote:

> >> +   /* Replace the bus with the fake device */
> > Fake how? This is a confusing comment to me as written.
> The genphy functions assume that they can write to the PHY directly 
> using the MII bus, and the address it uses is the address of a 
> register.  This is not the case for this chip with multiple PHY 
> interfaces, which have to be accessed indirectly.  To handle this, I 
> have created a "fake" mii_dev whose read/write functions are the 
> indirection functions, and stored the actual mdio_bus in the private 
> data for the "fake" device, which is then used by the indirect 
> functions.  This allows this driver to make use of common genphy stuff 
> where appropriate.  Maybe "wrapper" or "indirect bus" is a better name 
> for it.  Let me know if you have a preference or better idea.

"Indirect bus" is better IMO, as it gives a clue about what actually
happens.

> >> +
> >> +   mac_addr = phydev->addr;
> >> +
> >> +   for (i = 0; i < PORT_COUNT; i++) {
> >> +   if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
> >> +   phydev->addr = i;
> >> +   mv88e61xx_phy_enable(phydev, i);
> >> +   mv88e61xx_phy_setup(phydev, i);
> >> +   mv88e61xx_phy_config_port(phydev, i);
> > These all return status, but are ignored.
> Even if one fails, it seems appropriate to me to continue initializing 
> the others and not bail completely.  Should I catch the error, print a 
> warning and "continue" in the loop?  Or is completely bailing the right 
> thing to do?  If there are some errors, but other successes, what should 
> the function return?

Warn for each port switch initialization failure, bail out if no port
could be initialized?

> Thanks,
> Kevin

Amicalement,
-- 
Albert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches

[Joe Hershberger]

On Wed, Jan 27, 2016 at 11:28 AM, Albert ARIBAUD
 wrote:
> Hello Kevin,
>
> On Wed, 27 Jan 2016 16:29:42 +, Kevin Smith
>  wrote:
>> Hi Joe,
>
>> On 01/26/2016 06:11 PM, Joe Hershberger wrote:
>
>> >> +   /* Replace the bus with the fake device */
>> > Fake how? This is a confusing comment to me as written.
>> The genphy functions assume that they can write to the PHY directly
>> using the MII bus, and the address it uses is the address of a
>> register.  This is not the case for this chip with multiple PHY
>> interfaces, which have to be accessed indirectly.  To handle this, I
>> have created a "fake" mii_dev whose read/write functions are the
>> indirection functions, and stored the actual mdio_bus in the private
>> data for the "fake" device, which is then used by the indirect
>> functions.  This allows this driver to make use of common genphy stuff
>> where appropriate.  Maybe "wrapper" or "indirect bus" is a better name
>> for it.  Let me know if you have a preference or better idea.
>
> "Indirect bus" is better IMO, as it gives a clue about what actually
> happens.

Sounds good to me. May as well use the same nomenclature for the read
and write functions.

>> >> +
>> >> +   mac_addr = phydev->addr;
>> >> +
>> >> +   for (i = 0; i < PORT_COUNT; i++) {
>> >> +   if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
>> >> +   phydev->addr = i;
>> >> +   mv88e61xx_phy_enable(phydev, i);
>> >> +   mv88e61xx_phy_setup(phydev, i);
>> >> +   mv88e61xx_phy_config_port(phydev, i);
>> > These all return status, but are ignored.
>> Even if one fails, it seems appropriate to me to continue initializing
>> the others and not bail completely.  Should I catch the error, print a
>> warning and "continue" in the loop?  Or is completely bailing the right
>> thing to do?  If there are some errors, but other successes, what should
>> the function return?
>
> Warn for each port switch initialization failure, bail out if no port
> could be initialized?

I like it.

Cheers,
-Joe
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches

[Joe Hershberger]

Hi Kevin,

On Mon, Dec 21, 2015 at 3:45 PM, Kevin Smith
 wrote:
> The previous mv88e61xx driver was a driver for configuring the
> switch, but did not integrate with the PHY/networking system, so
> it could not be used as a PHY by U-boot.  This is a complete
> rework to support this device as a PHY.
>
> Signed-off-by: Kevin Smith 
> Acked-by: Prafulla Wadaskar 
> Cc: Joe Hershberger 
> Cc: Stefan Roese 
> Cc: Marek Vasut 
> ---
>  drivers/net/phy/mv88e61xx.c | 664 
> 
>  drivers/net/phy/phy.c   |   3 +
>  include/phy.h   |   1 +
>  3 files changed, 668 insertions(+)
>  create mode 100644 drivers/net/phy/mv88e61xx.c
>
> diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
> new file mode 100644
> index 000..d24272e
> --- /dev/null
> +++ b/drivers/net/phy/mv88e61xx.c
> @@ -0,0 +1,664 @@
> +/*
> + * (C) Copyright 2015
> + * Elecsys Corporation 
> + * Kevin Smith 
> + *
> + * Original driver:
> + * (C) Copyright 2009
> + * Marvell Semiconductor 
> + * Prafulla Wadaskar 
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +/*
> + * PHY driver for mv88e61xx ethernet switches.
> + *
> + * This driver configures the mv88e61xx for basic use as a PHY.  The switch
> + * supports a VLAN configuration that determines how traffic will be routed
> + * between the ports.  This driver uses a simple configuration that routes
> + * traffic from each PHY port only to the CPU port, and from the CPU port to
> + * any PHY port.
> + *
> + * The configuration determines which PHY ports to activate using the
> + * CONFIG_MV88E61XX_PHY_PORTS bitmask.  Setting bit 0 will activate port 0, 
> bit
> + * 1 activates port 1, etc.  Do not set the bit for the port the CPU is
> + * connected to unless it is connected over a PHY interface (not MII).
> + *
> + * This driver was written for and tested on the mv88e6176 with an SGMII
> + * connection.  Other configurations should be supported, but some additions 
> or
> + * changes may be required.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PHY_AUTONEGOTIATE_TIMEOUT  5000
> +
> +#define PORT_COUNT 7
> +#define PORT_MASK  ((1 << PORT_COUNT) - 1)
> +
> +/* Device addresses */
> +#define DEVADDR_PHY(p) (p)
> +#define DEVADDR_PORT(p)(0x10 + (p))
> +#define DEVADDR_SERDES 0x0F
> +#define DEVADDR_GLOBAL_1   0x1B
> +#define DEVADDR_GLOBAL_2   0x1C
> +
> +/* Global registers */
> +#define GLOBAL1_STATUS 0x00
> +#define GLOBAL1_CONTROL0x04
> +#define GLOBAL1_MONITOR_CONTROL0x1A
> +
> +/* Global 2 registers */
> +#define GLOBAL2_REG_PHY_CMD0x18
> +#define GLOBAL2_REG_PHY_DATA   0x19
> +
> +/* Port registers */
> +#define PORT_REG_STATUS0x00
> +#define PORT_REG_PHYS_CONTROL  0x01
> +#define PORT_REG_SWITCH_ID 0x03
> +#define PORT_REG_CONTROL   0x04
> +#define PORT_REG_VLAN_MAP  0x06
> +#define PORT_REG_VLAN_ID   0x07
> +
> +/* Phy registers */
> +#define PHY_REG_CONTROL1   0x10
> +#define PHY_REG_STATUS10x11
> +#define PHY_REG_PAGE   0x16
> +
> +/* Serdes registers */
> +#define SERDES_REG_CONTROL_1   0x10
> +
> +/* Phy page numbers */
> +#define PHY_PAGE_COPPER0
> +#define PHY_PAGE_SERDES1
> +
> +#define PHY_WRITE_CMD  0x9400
> +#define PHY_READ_CMD   0x9800

Please at least include a comment that these are commands written to
the GLOBAL2_REG_PHY_CMD and are made selecting...

Enable Busy (start), Clause 22 frames, and write / read.

Even better would be to also use names that are a closer to the
register they apply to, though that can get a bit long with these
windowed interfaces. Maybe GLOBAL2_REG_PHY_WRITE_CMD and
GLOBAL2_REG_PHY_READ_CMD?

Even better than that would be constants for the bitfield values that
are ORed in these commands.

> +
> +/* PHY Status Register */
> +#define PHY_REG_STATUS1_SPEED  0xc000
> +#define PHY_REG_STATUS1_GBIT   0x8000
> +#define PHY_REG_STATUS1_1000x4000
> +#define PHY_REG_STATUS1_DUPLEX 0x2000
> +#define PHY_REG_STATUS1_SPDDONE0x0800
> +#define PHY_REG_STATUS1_LINK   0x0400
> +#define PHY_REG_STATUS1_ENERGY 0x0010
> +
> +/* Check for required macros */
> +#ifndef CONFIG_MV88E61XX_PHY_PORTS
> +#error Define CONFIG_MV88E61XX_PHY_PORTS to indicate which physical ports \
> +   to activate
> +#endif
> +#ifndef CONFIG_MV88E61XX_CPU_PORT
> +#error Define CONFIG_MV88E61XX_CPU_PORT to the 

[U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches

[Kevin Smith]

The previous mv88e61xx driver was a driver for configuring the
switch, but did not integrate with the PHY/networking system, so
it could not be used as a PHY by U-boot.  This is a complete
rework to support this device as a PHY.

Signed-off-by: Kevin Smith 
Acked-by: Prafulla Wadaskar 
Cc: Joe Hershberger 
Cc: Stefan Roese 
Cc: Marek Vasut 
---
 drivers/net/phy/mv88e61xx.c | 664 
 drivers/net/phy/phy.c   |   3 +
 include/phy.h   |   1 +
 3 files changed, 668 insertions(+)
 create mode 100644 drivers/net/phy/mv88e61xx.c

diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c
new file mode 100644
index 000..d24272e
--- /dev/null
+++ b/drivers/net/phy/mv88e61xx.c
@@ -0,0 +1,664 @@
+/*
+ * (C) Copyright 2015
+ * Elecsys Corporation 
+ * Kevin Smith 
+ *
+ * Original driver:
+ * (C) Copyright 2009
+ * Marvell Semiconductor 
+ * Prafulla Wadaskar 
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+/*
+ * PHY driver for mv88e61xx ethernet switches.
+ *
+ * This driver configures the mv88e61xx for basic use as a PHY.  The switch
+ * supports a VLAN configuration that determines how traffic will be routed
+ * between the ports.  This driver uses a simple configuration that routes
+ * traffic from each PHY port only to the CPU port, and from the CPU port to
+ * any PHY port.
+ *
+ * The configuration determines which PHY ports to activate using the
+ * CONFIG_MV88E61XX_PHY_PORTS bitmask.  Setting bit 0 will activate port 0, bit
+ * 1 activates port 1, etc.  Do not set the bit for the port the CPU is
+ * connected to unless it is connected over a PHY interface (not MII).
+ *
+ * This driver was written for and tested on the mv88e6176 with an SGMII
+ * connection.  Other configurations should be supported, but some additions or
+ * changes may be required.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#define PHY_AUTONEGOTIATE_TIMEOUT  5000
+
+#define PORT_COUNT 7
+#define PORT_MASK  ((1 << PORT_COUNT) - 1)
+
+/* Device addresses */
+#define DEVADDR_PHY(p) (p)
+#define DEVADDR_PORT(p)(0x10 + (p))
+#define DEVADDR_SERDES 0x0F
+#define DEVADDR_GLOBAL_1   0x1B
+#define DEVADDR_GLOBAL_2   0x1C
+
+/* Global registers */
+#define GLOBAL1_STATUS 0x00
+#define GLOBAL1_CONTROL0x04
+#define GLOBAL1_MONITOR_CONTROL0x1A
+
+/* Global 2 registers */
+#define GLOBAL2_REG_PHY_CMD0x18
+#define GLOBAL2_REG_PHY_DATA   0x19
+
+/* Port registers */
+#define PORT_REG_STATUS0x00
+#define PORT_REG_PHYS_CONTROL  0x01
+#define PORT_REG_SWITCH_ID 0x03
+#define PORT_REG_CONTROL   0x04
+#define PORT_REG_VLAN_MAP  0x06
+#define PORT_REG_VLAN_ID   0x07
+
+/* Phy registers */
+#define PHY_REG_CONTROL1   0x10
+#define PHY_REG_STATUS10x11
+#define PHY_REG_PAGE   0x16
+
+/* Serdes registers */
+#define SERDES_REG_CONTROL_1   0x10
+
+/* Phy page numbers */
+#define PHY_PAGE_COPPER0
+#define PHY_PAGE_SERDES1
+
+#define PHY_WRITE_CMD  0x9400
+#define PHY_READ_CMD   0x9800
+
+/* PHY Status Register */
+#define PHY_REG_STATUS1_SPEED  0xc000
+#define PHY_REG_STATUS1_GBIT   0x8000
+#define PHY_REG_STATUS1_1000x4000
+#define PHY_REG_STATUS1_DUPLEX 0x2000
+#define PHY_REG_STATUS1_SPDDONE0x0800
+#define PHY_REG_STATUS1_LINK   0x0400
+#define PHY_REG_STATUS1_ENERGY 0x0010
+
+/* Check for required macros */
+#ifndef CONFIG_MV88E61XX_PHY_PORTS
+#error Define CONFIG_MV88E61XX_PHY_PORTS to indicate which physical ports \
+   to activate
+#endif
+#ifndef CONFIG_MV88E61XX_CPU_PORT
+#error Define CONFIG_MV88E61XX_CPU_PORT to the port the CPU is attached to
+#endif
+
+
+enum {
+   SWITCH_MODEL_6176,
+   /* Leave this last */
+   SWITCH_MODEL_UNKNOWN
+};
+
+static u16 switch_model_ids[] = {
+   [SWITCH_MODEL_6176] = 0x176,
+};
+
+
+/* Wait for the current SMI PHY command to complete */
+static int mv88e61xx_smi_wait(struct mii_dev *bus)
+{
+   int reg;
+   u32 timeout = 100;
+
+   do {
+   reg = bus->read(bus, DEVADDR_GLOBAL_2, MDIO_DEVAD_NONE,
+   GLOBAL2_REG_PHY_CMD);
+   if (reg >= 0 && (reg & (1 << 15)) == 0)
+   return 0;
+
+   mdelay(1);
+   } while (--timeout);
+
+   puts("SMI busy timeout\n");
+   return -1;
+}
+
+
+/* Write a PHY register indirectly */
+static int mv88e61xx_phy_write_bus(struct mii_dev