Re: [PATCH net-next 1/8] net: dsa: mv88e6xxx: Implement external MDIO bus on mv88e6390

[Andrew Lunn]

On Fri, Jan 20, 2017 at 07:04:35PM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn  writes:
> 
> > The mv88e6390 has two MDIO busses. The internal MDIO bus is used for
> > the internal PHYs. The external MDIO can be used for external PHYs.
> > The external MDIO bus will be instantiated if there is an
> > "mdio-external" node in the device tree.
> 
> Thanks for pushing the 88E6390 support. Some comments below.
> 
> > +static int mv88e6xxx_read_phy(struct mv88e6xxx_chip *chip, int addr, int 
> > reg,
> > +static int mv88e6xxx_write_phy(struct mv88e6xxx_chip *chip, int addr, int 
> > reg,
> >  static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
> >  static int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy,
> 
> Adding mv88e6xxx_read/write_phy() in addition to existing
> mv88e6xxx_phy_read/write() feels really confusing and hard to
> maintain. Can that be done the other way around maybe?

Yes, i agree. I didn't particularly like it either. What might be
simpler is to pass the struct mii_bus all the way down, where as we
only currently have chip.

 Andrew

Re: [PATCH net-next 1/8] net: dsa: mv88e6xxx: Implement external MDIO bus on mv88e6390

[Andrew Lunn]

> This looks fine, although I am not clear why we cannot utilize a
> standard representation of a MDIO bus (with PHY devices as child nodes)
> which has a specific compatible string, e.g:
> marvell,mv88e6390-external-mdio, and that is a child node of the 6390
> Ethernet switch itself, something like:

Humm, interesting idea. I did not think of this, because the current
code just looks for an mdio property, and does not care about any
compatible string.

I will think about this.

Thanks

  Andrew

Re: [PATCH net-next 1/8] net: dsa: mv88e6xxx: Implement external MDIO bus on mv88e6390

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

> The mv88e6390 has two MDIO busses. The internal MDIO bus is used for
> the internal PHYs. The external MDIO can be used for external PHYs.
> The external MDIO bus will be instantiated if there is an
> "mdio-external" node in the device tree.

Thanks for pushing the 88E6390 support. Some comments below.

> +static int mv88e6xxx_read_phy(struct mv88e6xxx_chip *chip, int addr, int reg,
> +static int mv88e6xxx_write_phy(struct mv88e6xxx_chip *chip, int addr, int 
> reg,
>  static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
>  static int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy,

Adding mv88e6xxx_read/write_phy() in addition to existing
mv88e6xxx_phy_read/write() feels really confusing and hard to
maintain. Can that be done the other way around maybe?

> +static int mv88e6xxx_external_mdio_register(struct mv88e6xxx_chip *chip,
> + struct device_node *np)
> +static void mv88e6xxx_external_mdio_unregister(struct mv88e6xxx_chip *chip)
>  static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
>  struct device_node *np)

We already have mv88e6xxx_mdio_register/unregister(). Isn't it possible
to tweak them to take a struct mv88e6xxx_mdio_bus instance and use them
twice for both internal and external MDIO busses?

> + if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_EXTERNAL_MDIO)) {
> + err = mv88e6xxx_external_mdio_register(chip, np);
> + if (err)
> + goto out_mdio;
> + }

We are trying to get rid of the flags and family checks... Please don't
add new ones. If the external MDIO bus is a new feature of switches like
88E6390, isn't it better to add new external_phy_read/write ops and
register the bus if they are provided?

if (chip->info->ops->external_phy_read) {
struct mv88e6xxx_mdio_bus *external_mdio_bus;
...
err = mv88e6xxx_mdio_register(external_mdio_bus);
if (err)
...
}

Thanks,

Vivien

Re: [PATCH net-next 1/8] net: dsa: mv88e6xxx: Implement external MDIO bus on mv88e6390

[Florian Fainelli]

On 01/20/2017 03:30 PM, Andrew Lunn wrote:
> The mv88e6390 has two MDIO busses. The internal MDIO bus is used for
> the internal PHYs. The external MDIO can be used for external PHYs.
> The external MDIO bus will be instantiated if there is an
> "mdio-external" node in the device tree.

This looks fine, although I am not clear why we cannot utilize a
standard representation of a MDIO bus (with PHY devices as child nodes)
which has a specific compatible string, e.g:
marvell,mv88e6390-external-mdio, and that is a child node of the 6390
Ethernet switch itself, something like:

/* assuming this is, e.g: an independent or CPU EThernet MAC MDIO bus */
 {
switch@0 {
compatible = "marvell,mv88e6390";
reg = <0>;

ports {
#address-cells = <1>;
#size-cells = <0>;

port@0 {
phy-handle = ;
reg = <0>;
};
};

mdio {
compatible = "marvell,mv88e6390-external-mdio";
#address-cells = <1>;
#size-cells = <0>;

phy0: phy@0 {
reg = <0>;
};
};
};
};

In both cases (your proposal) and this one, we still have a dependency
on the Ethernet switch driver being probed to create the internal and
external MDIO buses.

Thanks!
-- 
Florian

[PATCH net-next 1/8] net: dsa: mv88e6xxx: Implement external MDIO bus on mv88e6390

[Andrew Lunn]

The mv88e6390 has two MDIO busses. The internal MDIO bus is used for
the internal PHYs. The external MDIO can be used for external PHYs.
The external MDIO bus will be instantiated if there is an
"mdio-external" node in the device tree.

Signed-off-by: Andrew Lunn 
---
 .../devicetree/bindings/net/dsa/marvell.txt|   4 +
 drivers/net/dsa/mv88e6xxx/chip.c   | 136 +
 drivers/net/dsa/mv88e6xxx/global2.c|  10 +-
 drivers/net/dsa/mv88e6xxx/global2.h|   4 +-
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h  |  35 --
 5 files changed, 148 insertions(+), 41 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt 
b/Documentation/devicetree/bindings/net/dsa/marvell.txt
index b3dd6b40e0de..51b881f1645e 100644
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
@@ -28,6 +28,10 @@ Optional properties:
 #interrupt-cells = <2> : Controller uses two cells, number and flag
 - mdio : container of PHY and devices on the switches MDIO
  bus
+
+Optional for the "marvell,mv88e6390"
+- mdio-external: A collection of PHY nodes on external bus
+
 Example:
 
mdio {
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c7e08e13bb54..77e960826402 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -206,6 +206,12 @@ int mv88e6xxx_read(struct mv88e6xxx_chip *chip, int addr, 
int reg, u16 *val)
return 0;
 }
 
+static int mv88e6xxx_read_phy(struct mv88e6xxx_chip *chip, int addr, int reg,
+ u16 *val, bool external)
+{
+   return mv88e6xxx_read(chip, addr, reg, val);
+}
+
 int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val)
 {
int err;
@@ -222,26 +228,32 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int 
addr, int reg, u16 val)
return 0;
 }
 
+static int mv88e6xxx_write_phy(struct mv88e6xxx_chip *chip, int addr, int reg,
+  u16 val, bool external)
+{
+   return mv88e6xxx_write(chip, addr, reg, val);
+}
+
 static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
- int reg, u16 *val)
+ int reg, u16 *val, bool external)
 {
int addr = phy; /* PHY devices addresses start at 0x0 */
 
if (!chip->info->ops->phy_read)
return -EOPNOTSUPP;
 
-   return chip->info->ops->phy_read(chip, addr, reg, val);
+   return chip->info->ops->phy_read(chip, addr, reg, val, external);
 }
 
 static int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy,
-  int reg, u16 val)
+  int reg, u16 val, bool external)
 {
int addr = phy; /* PHY devices addresses start at 0x0 */
 
if (!chip->info->ops->phy_write)
return -EOPNOTSUPP;
 
-   return chip->info->ops->phy_write(chip, addr, reg, val);
+   return chip->info->ops->phy_write(chip, addr, reg, val, external);
 }
 
 static int mv88e6xxx_phy_page_get(struct mv88e6xxx_chip *chip, int phy, u8 
page)
@@ -249,7 +261,7 @@ static int mv88e6xxx_phy_page_get(struct mv88e6xxx_chip 
*chip, int phy, u8 page)
if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_PHY_PAGE))
return -EOPNOTSUPP;
 
-   return mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
+   return mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page, false);
 }
 
 static void mv88e6xxx_phy_page_put(struct mv88e6xxx_chip *chip, int phy)
@@ -257,7 +269,7 @@ static void mv88e6xxx_phy_page_put(struct mv88e6xxx_chip 
*chip, int phy)
int err;
 
/* Restore PHY page Copper 0x0 for access via the registered MDIO bus */
-   err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, PHY_PAGE_COPPER);
+   err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, PHY_PAGE_COPPER, false);
if (unlikely(err)) {
dev_err(chip->dev, "failed to restore PHY %d page Copper 
(%d)\n",
phy, err);
@@ -275,7 +287,7 @@ static int mv88e6xxx_phy_page_read(struct mv88e6xxx_chip 
*chip, int phy,
 
err = mv88e6xxx_phy_page_get(chip, phy, page);
if (!err) {
-   err = mv88e6xxx_phy_read(chip, phy, reg, val);
+   err = mv88e6xxx_phy_read(chip, phy, reg, val, false);
mv88e6xxx_phy_page_put(chip, phy);
}
 
@@ -293,7 +305,7 @@ static int mv88e6xxx_phy_page_write(struct mv88e6xxx_chip 
*chip, int phy,
 
err = mv88e6xxx_phy_page_get(chip, phy, page);
if (!err) {
-   err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
+   err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page, false);
mv88e6xxx_phy_page_put(chip, phy);
}
 
@@ -612,7 +624,7 @@ static void mv88e6xxx_ppu_state_destroy(struct