[PATCH net-next 0/8] net: dsa: mv88e6xxx: misc probe improvements

[PATCH net-next 0/8] net: dsa: mv88e6xxx: misc probe improvements

[Vivien Didelot]

This patchset cleans a bit both legacy and the new MDIO probing functions by
extracting common code and adding helpers.

Then, thanks to the new probing model allowing switch chips to be true Linux
devices, a device node can explicit the switch model in the tree and thus
allows a more robust probing through the MDIO bus.

Vivien Didelot (8):
  net: dsa: mv88e6xxx: fix style issues
  net: dsa: mv88e6xxx: remove redundant assignments
  net: dsa: mv88e6xxx: use already declared variables
  net: dsa: mv88e6xxx: do not increment bus refcount
  net: dsa: mv88e6xxx: add switch register helpers
  net: dsa: mv88e6xxx: add chip detection helper
  net: dsa: mv88e6xxx: explicit compatible devices
  net: dsa: mv88e6xxx: fail on mismatching probe

 Documentation/devicetree/bindings/net/dsa/dsa.txt  |   6 +-
 .../devicetree/bindings/net/dsa/marvell.txt        |  19 ++-
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts          |   6 +-
 drivers/net/dsa/mv88e6xxx.c                        | 189 +++++++++++++--------
 4 files changed, 140 insertions(+), 80 deletions(-)

-- 
2.8.3

[PATCH net-next 1/8] net: dsa: mv88e6xxx: fix style issues

[Vivien Didelot]

This patch fixes 5 style problems reported by checkpatch:

    WARNING: suspect code indent for conditional statements (8, 24)
    #492: FILE: drivers/net/dsa/mv88e6xxx.c:492:
    +	if (phydev->link)
    +			reg |= PORT_PCS_CTRL_LINK_UP;

    CHECK: Logical continuations should be on the previous line
    #1318: FILE: drivers/net/dsa/mv88e6xxx.c:1318:
    +		     oldstate == PORT_CONTROL_STATE_FORWARDING)
    +		    && (state == PORT_CONTROL_STATE_DISABLED ||

    CHECK: multiple assignments should be avoided
    #1662: FILE: drivers/net/dsa/mv88e6xxx.c:1662:
    +		vlan->vid_begin = vlan->vid_end = next.vid;

    WARNING: line over 80 characters
    #2097: FILE: drivers/net/dsa/mv88e6xxx.c:2097:
    +				       const struct switchdev_obj_port_vlan *vlan,

    WARNING: suspect code indent for conditional statements (16, 32)
    #2734: FILE: drivers/net/dsa/mv88e6xxx.c:2734:
    +		if (mv88e6xxx_6352_family(ps) || mv88e6xxx_6351_family(ps) ||
    [...]
    +				reg |= PORT_CONTROL_EGRESS_ADD_TAG;

    total: 0 errors, 3 warnings, 2 checks, 3805 lines checked

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index ee06055..6a2528f 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -490,7 +490,7 @@ static void mv88e6xxx_adjust_link(struct dsa_switch *ds, int port,
 
 	reg |= PORT_PCS_CTRL_FORCE_LINK;
 	if (phydev->link)
-			reg |= PORT_PCS_CTRL_LINK_UP;
+		reg |= PORT_PCS_CTRL_LINK_UP;
 
 	if (mv88e6xxx_6065_family(ps) && phydev->speed > SPEED_100)
 		goto out;
@@ -1314,9 +1314,9 @@ static int _mv88e6xxx_port_state(struct mv88e6xxx_priv_state *ps, int port,
 		 * Blocking or Listening state.
 		 */
 		if ((oldstate == PORT_CONTROL_STATE_LEARNING ||
-		     oldstate == PORT_CONTROL_STATE_FORWARDING)
-		    && (state == PORT_CONTROL_STATE_DISABLED ||
-			state == PORT_CONTROL_STATE_BLOCKING)) {
+		     oldstate == PORT_CONTROL_STATE_FORWARDING) &&
+		    (state == PORT_CONTROL_STATE_DISABLED ||
+		     state == PORT_CONTROL_STATE_BLOCKING)) {
 			ret = _mv88e6xxx_atu_remove(ps, 0, port, false);
 			if (ret)
 				return ret;
@@ -1659,7 +1659,8 @@ static int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds, int port,
 			continue;
 
 		/* reinit and dump this VLAN obj */
-		vlan->vid_begin = vlan->vid_end = next.vid;
+		vlan->vid_begin = next.vid;
+		vlan->vid_end = next.vid;
 		vlan->flags = 0;
 
 		if (next.data[port] == GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED)
@@ -2093,9 +2094,10 @@ unlock:
 	return ret;
 }
 
-static int mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
-				       const struct switchdev_obj_port_vlan *vlan,
-				       struct switchdev_trans *trans)
+static int
+mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
+			    const struct switchdev_obj_port_vlan *vlan,
+			    struct switchdev_trans *trans)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int err;
@@ -2735,7 +2737,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_priv_state *ps, int port)
 		    mv88e6xxx_6165_family(ps) || mv88e6xxx_6097_family(ps) ||
 		    mv88e6xxx_6095_family(ps) || mv88e6xxx_6065_family(ps) ||
 		    mv88e6xxx_6185_family(ps) || mv88e6xxx_6320_family(ps)) {
-				reg |= PORT_CONTROL_EGRESS_ADD_TAG;
+			reg |= PORT_CONTROL_EGRESS_ADD_TAG;
 		}
 	}
 	if (dsa_is_dsa_port(ds, port)) {
-- 
2.8.3

Re: [PATCH net-next 1/8] net: dsa: mv88e6xxx: fix style issues

[Andrew Lunn]

On Wed, Jun 08, 2016 at 08:44:49PM -0400, Vivien Didelot wrote:
> This patch fixes 5 style problems reported by checkpatch:
> 
>     WARNING: suspect code indent for conditional statements (8, 24)
>     #492: FILE: drivers/net/dsa/mv88e6xxx.c:492:
>     +	if (phydev->link)
>     +			reg |= PORT_PCS_CTRL_LINK_UP;
> 
>     CHECK: Logical continuations should be on the previous line
>     #1318: FILE: drivers/net/dsa/mv88e6xxx.c:1318:
>     +		     oldstate == PORT_CONTROL_STATE_FORWARDING)
>     +		    && (state == PORT_CONTROL_STATE_DISABLED ||
> 
>     CHECK: multiple assignments should be avoided
>     #1662: FILE: drivers/net/dsa/mv88e6xxx.c:1662:
>     +		vlan->vid_begin = vlan->vid_end = next.vid;
> 
>     WARNING: line over 80 characters
>     #2097: FILE: drivers/net/dsa/mv88e6xxx.c:2097:
>     +				       const struct switchdev_obj_port_vlan *vlan,
> 
>     WARNING: suspect code indent for conditional statements (16, 32)
>     #2734: FILE: drivers/net/dsa/mv88e6xxx.c:2734:
>     +		if (mv88e6xxx_6352_family(ps) || mv88e6xxx_6351_family(ps) ||
>     [...]
>     +				reg |= PORT_CONTROL_EGRESS_ADD_TAG;
> 
>     total: 0 errors, 3 warnings, 2 checks, 3805 lines checked
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 2/8] net: dsa: mv88e6xxx: remove redundant assignments

[Vivien Didelot]

The chip->ds and ds->slave_mii_bus assignments are common to both legacy
and new MDIO probing and are already done in the later setup code.

Remove the duplicated assignments from the MDIO probing code.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 6a2528f..cb8b1b7 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3706,7 +3706,6 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	ds->priv = ps;
 	ds->dev = dev;
 	ps->dev = dev;
-	ps->ds = ds;
 	ps->bus = mdiodev->bus;
 	ps->sw_addr = mdiodev->addr;
 	mutex_init(&ps->smi_mutex);
@@ -3746,8 +3745,6 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (err)
 		return err;
 
-	ds->slave_mii_bus = ps->mdio_bus;
-
 	dev_set_drvdata(dev, ds);
 
 	err = dsa_register_switch(ds, mdiodev->dev.of_node);
-- 
2.8.3

Re: [PATCH net-next 2/8] net: dsa: mv88e6xxx: remove redundant assignments

[Andrew Lunn]

On Wed, Jun 08, 2016 at 08:44:50PM -0400, Vivien Didelot wrote:
> The chip->ds and ds->slave_mii_bus assignments are common to both legacy
> and new MDIO probing and are already done in the later setup code.
> 
> Remove the duplicated assignments from the MDIO probing code.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 3/8] net: dsa: mv88e6xxx: use already declared variables

[Vivien Didelot]

In the MDIO probing function, dev is already assigned to &mdiodev->dev
and np is already assigned to mdiodev->dev.of_node, so use them.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index cb8b1b7..cfa30ae 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3726,7 +3726,7 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (!ps->info)
 		return -ENODEV;
 
-	ps->reset = devm_gpiod_get(&mdiodev->dev, "reset", GPIOD_ASIS);
+	ps->reset = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
 	if (IS_ERR(ps->reset)) {
 		err = PTR_ERR(ps->reset);
 		if (err == -ENOENT) {
@@ -3741,13 +3741,13 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	    !of_property_read_u32(np, "eeprom-length", &eeprom_len))
 		ps->eeprom_len = eeprom_len;
 
-	err = mv88e6xxx_mdio_register(ps, mdiodev->dev.of_node);
+	err = mv88e6xxx_mdio_register(ps, np);
 	if (err)
 		return err;
 
 	dev_set_drvdata(dev, ds);
 
-	err = dsa_register_switch(ds, mdiodev->dev.of_node);
+	err = dsa_register_switch(ds, np);
 	if (err) {
 		mv88e6xxx_mdio_unregister(ps);
 		return err;
-- 
2.8.3

Re: [PATCH net-next 3/8] net: dsa: mv88e6xxx: use already declared variables

[Andrew Lunn]

On Wed, Jun 08, 2016 at 08:44:51PM -0400, Vivien Didelot wrote:
> In the MDIO probing function, dev is already assigned to &mdiodev->dev
> and np is already assigned to mdiodev->dev.of_node, so use them.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 4/8] net: dsa: mv88e6xxx: do not increment bus refcount

[Vivien Didelot]

The MDIO device probe and remove functions are respectively incrementing
and decrementing the bus refcount themselves. Since these bus level
actions are out of the device scope, remove them.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index cfa30ae..02b0af7 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3710,8 +3710,6 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	ps->sw_addr = mdiodev->addr;
 	mutex_init(&ps->smi_mutex);
 
-	get_device(&ps->bus->dev);
-
 	ds->drv = &mv88e6xxx_switch_driver;
 
 	id = mv88e6xxx_reg_read(ps, REG_PORT(0), PORT_SWITCH_ID);
@@ -3765,7 +3763,6 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 
 	dsa_unregister_switch(ds);
-	put_device(&ps->bus->dev);
 
 	mv88e6xxx_mdio_unregister(ps);
 }
-- 
2.8.3

Re: [PATCH net-next 4/8] net: dsa: mv88e6xxx: do not increment bus refcount

[Andrew Lunn]

On Wed, Jun 08, 2016 at 08:44:52PM -0400, Vivien Didelot wrote:
> The MDIO device probe and remove functions are respectively incrementing
> and decrementing the bus refcount themselves. Since these bus level
> actions are out of the device scope, remove them.

I agree with the patch. But have you checked the mdio layer is doing
the right thing? If not, we should fix that first.

    Andrew

Re: [PATCH net-next 4/8] net: dsa: mv88e6xxx: do not increment bus refcount

[Vivien Didelot]

Hi,

Andrew Lunn <andrew@lunn.ch> writes:

> On Wed, Jun 08, 2016 at 08:44:52PM -0400, Vivien Didelot wrote:
>> The MDIO device probe and remove functions are respectively incrementing
>> and decrementing the bus refcount themselves. Since these bus level
>> actions are out of the device scope, remove them.
>
> I agree with the patch. But have you checked the mdio layer is doing
> the right thing? If not, we should fix that first.

So I added some printing after incrementing/decrementing the refcount in
get_device/put_device to track &ps->bus->dev, which name is "0.1".

Regardless having this patch or not, the refcount of the 0.1 mii_bus
device is 5 before loading the mv88e6xxx module on vf610-zii-dev-rev-b.

Below is a portion of dmesg:

    [    8.921647] get_device: 400d1000.etherne refcount: 4
    [    8.926225] get_device: 0.1 refcount: 2
    [    8.929561] get_device: mdio-mux refcount: 5
    [    8.934076] get_device: 0.1 refcount: 3
    [    8.937446] get_device: 0.1 refcount: 4
    [    8.940792] put_device: 0.1 refcount: 3
    [    8.944181] libphy: mdio_mux: probed
    [    8.947885] mdio_bus 0.1:00: mdio_device_register
    [    8.952649] get_device: 0.1:00 refcount: 2
    [    8.956283] get_device: 0.1 refcount: 4
    [    8.959838] get_device: 0.1:00 refcount: 3
    [    8.963991] get_device: 0.1:00 refcount: 4
    [    8.967598] put_device: 0.1:00 refcount: 3
    [    8.971298] get_device: 0.2 refcount: 2
    [    8.974687] get_device: mdio-mux refcount: 7

So it seems like of_ is managing the bus refcount on events such as a
new child node (0.1:00).

Thanks,

        Vivien

Re: [PATCH net-next 4/8] net: dsa: mv88e6xxx: do not increment bus refcount

[Andrew Lunn]

On Fri, Jun 10, 2016 at 03:59:22PM -0400, Vivien Didelot wrote:
> Hi,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Wed, Jun 08, 2016 at 08:44:52PM -0400, Vivien Didelot wrote:
> >> The MDIO device probe and remove functions are respectively incrementing
> >> and decrementing the bus refcount themselves. Since these bus level
> >> actions are out of the device scope, remove them.
> >
> > I agree with the patch. But have you checked the mdio layer is doing
> > the right thing? If not, we should fix that first.
> 
> So I added some printing after incrementing/decrementing the refcount in
> get_device/put_device to track &ps->bus->dev, which name is "0.1".
> 
> Regardless having this patch or not, the refcount of the 0.1 mii_bus
> device is 5 before loading the mv88e6xxx module on vf610-zii-dev-rev-b.
> 
> Below is a portion of dmesg:
> 
>     [    8.921647] get_device: 400d1000.etherne refcount: 4
>     [    8.926225] get_device: 0.1 refcount: 2
>     [    8.929561] get_device: mdio-mux refcount: 5
>     [    8.934076] get_device: 0.1 refcount: 3
>     [    8.937446] get_device: 0.1 refcount: 4
>     [    8.940792] put_device: 0.1 refcount: 3
>     [    8.944181] libphy: mdio_mux: probed
>     [    8.947885] mdio_bus 0.1:00: mdio_device_register
>     [    8.952649] get_device: 0.1:00 refcount: 2
>     [    8.956283] get_device: 0.1 refcount: 4
>     [    8.959838] get_device: 0.1:00 refcount: 3
>     [    8.963991] get_device: 0.1:00 refcount: 4
>     [    8.967598] put_device: 0.1:00 refcount: 3
>     [    8.971298] get_device: 0.2 refcount: 2
>     [    8.974687] get_device: mdio-mux refcount: 7
> 
> So it seems like of_ is managing the bus refcount on events such as a
> new child node (0.1:00).

Great, thanks for looking at this.

Acked-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 5/8] net: dsa: mv88e6xxx: add switch register helpers

[Vivien Didelot]

Extract the allocation and registration code related to the dsa_switch
structure in a mv88e6xxx_register_switch helper function.

For symmetry in the code, add a mv88e6xxx_unregister_switch function.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 02b0af7..584a6b6 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3688,30 +3688,48 @@ struct dsa_switch_driver mv88e6xxx_switch_driver = {
 	.port_fdb_dump          = mv88e6xxx_port_fdb_dump,
 };
 
+static int mv88e6xxx_register_switch(struct mv88e6xxx_priv_state *ps,
+				     struct device_node *np)
+{
+	struct device *dev = ps->dev;
+	struct dsa_switch *ds;
+
+	ds = devm_kzalloc(dev, sizeof(*ds), GFP_KERNEL);
+	if (!ds)
+		return -ENOMEM;
+
+	ds->dev = dev;
+	ds->priv = ps;
+	ds->drv = &mv88e6xxx_switch_driver;
+
+	dev_set_drvdata(dev, ds);
+
+	return dsa_register_switch(ds, np);
+}
+
+static void mv88e6xxx_unregister_switch(struct mv88e6xxx_priv_state *ps)
+{
+	dsa_unregister_switch(ps->ds);
+}
+
 int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct device *dev = &mdiodev->dev;
 	struct device_node *np = dev->of_node;
 	struct mv88e6xxx_priv_state *ps;
 	int id, prod_num, rev;
-	struct dsa_switch *ds;
 	u32 eeprom_len;
 	int err;
 
-	ds = devm_kzalloc(dev, sizeof(*ds) + sizeof(*ps), GFP_KERNEL);
-	if (!ds)
+	ps = devm_kzalloc(dev, sizeof(*ps), GFP_KERNEL);
+	if (!ps)
 		return -ENOMEM;
 
-	ps = (struct mv88e6xxx_priv_state *)(ds + 1);
-	ds->priv = ps;
-	ds->dev = dev;
 	ps->dev = dev;
 	ps->bus = mdiodev->bus;
 	ps->sw_addr = mdiodev->addr;
 	mutex_init(&ps->smi_mutex);
 
-	ds->drv = &mv88e6xxx_switch_driver;
-
 	id = mv88e6xxx_reg_read(ps, REG_PORT(0), PORT_SWITCH_ID);
 	if (id < 0)
 		return id;
@@ -3743,9 +3761,7 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (err)
 		return err;
 
-	dev_set_drvdata(dev, ds);
-
-	err = dsa_register_switch(ds, np);
+	err = mv88e6xxx_register_switch(ps, np);
 	if (err) {
 		mv88e6xxx_mdio_unregister(ps);
 		return err;
@@ -3762,8 +3778,7 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 
-	dsa_unregister_switch(ds);
-
+	mv88e6xxx_unregister_switch(ps);
 	mv88e6xxx_mdio_unregister(ps);
 }
 
-- 
2.8.3

Re: [PATCH net-next 5/8] net: dsa: mv88e6xxx: add switch register helpers

[Andrew Lunn]

On Wed, Jun 08, 2016 at 08:44:53PM -0400, Vivien Didelot wrote:
> Extract the allocation and registration code related to the dsa_switch
> structure in a mv88e6xxx_register_switch helper function.
> 
> For symmetry in the code, add a mv88e6xxx_unregister_switch function.

You say what you are doing, but not why?

    Andrew

Re: [PATCH net-next 5/8] net: dsa: mv88e6xxx: add switch register helpers

[Vivien Didelot]

Andrew Lunn <andrew@lunn.ch> writes:

> On Wed, Jun 08, 2016 at 08:44:53PM -0400, Vivien Didelot wrote:
>> Extract the allocation and registration code related to the dsa_switch
>> structure in a mv88e6xxx_register_switch helper function.
>> 
>> For symmetry in the code, add a mv88e6xxx_unregister_switch function.
>
> You say what you are doing, but not why?

The mixed assignements and registrations in the probe code make it hard
to read and figure out what is dsa or chip specific, so I made that
change to provide a simple function doing one thing, make it easier to
follow the probe logic.

Thanks, I'll update the commit message.

        Vivien

[PATCH net-next 6/8] net: dsa: mv88e6xxx: add chip detection helper

[Vivien Didelot]

Extract the common code to read the switch ID and allocate the private
chip data, found in both legacy and new MDIO probe functions, into its
own mv88e6xxx_detect helper to reduce boiler plate.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 68 +++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 39 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 584a6b6..28070e5 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3601,20 +3601,13 @@ mv88e6xxx_lookup_info(unsigned int prod_num, const struct mv88e6xxx_info *table,
 	return NULL;
 }
 
-static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
-				       struct device *host_dev, int sw_addr,
-				       void **priv)
+static struct mv88e6xxx_priv_state *mv88e6xxx_detect(struct device *dev,
+						     struct mii_bus *bus,
+						     int sw_addr)
 {
 	const struct mv88e6xxx_info *info;
 	struct mv88e6xxx_priv_state *ps;
-	struct mii_bus *bus;
-	const char *name;
 	int id, prod_num, rev;
-	int err;
-
-	bus = dsa_host_dev_to_mii_bus(host_dev);
-	if (!bus)
-		return NULL;
 
 	id = __mv88e6xxx_reg_read(bus, sw_addr, REG_PORT(0), PORT_SWITCH_ID);
 	if (id < 0)
@@ -3628,28 +3621,46 @@ static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 	if (!info)
 		return NULL;
 
-	name = info->name;
+	dev_info(dev, "switch 0x%x detected: %s, revision %u\n", prod_num,
+		 info->name, rev);
 
-	ps = devm_kzalloc(dsa_dev, sizeof(*ps), GFP_KERNEL);
+	ps = devm_kzalloc(dev, sizeof(*ps), GFP_KERNEL);
 	if (!ps)
 		return NULL;
 
+	ps->dev = dev;
 	ps->bus = bus;
 	ps->sw_addr = sw_addr;
 	ps->info = info;
-	ps->dev = dsa_dev;
+
 	mutex_init(&ps->smi_mutex);
 
+	return ps;
+}
+
+static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
+				       struct device *host_dev, int sw_addr,
+				       void **priv)
+{
+	struct mv88e6xxx_priv_state *ps;
+	struct mii_bus *bus;
+	int err;
+
+	bus = dsa_host_dev_to_mii_bus(host_dev);
+	if (!bus)
+		return NULL;
+
+	ps = mv88e6xxx_detect(dsa_dev, bus, sw_addr);
+	if (!ps)
+		return NULL;
+
 	err = mv88e6xxx_mdio_register(ps, NULL);
 	if (err)
 		return NULL;
 
 	*priv = ps;
 
-	dev_info(&ps->bus->dev, "switch 0x%x probed: %s, revision %u\n",
-		 prod_num, name, rev);
-
-	return name;
+	return ps->info->name;
 }
 
 struct dsa_switch_driver mv88e6xxx_switch_driver = {
@@ -3717,29 +3728,11 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	struct device *dev = &mdiodev->dev;
 	struct device_node *np = dev->of_node;
 	struct mv88e6xxx_priv_state *ps;
-	int id, prod_num, rev;
 	u32 eeprom_len;
 	int err;
 
-	ps = devm_kzalloc(dev, sizeof(*ps), GFP_KERNEL);
+	ps = mv88e6xxx_detect(dev, mdiodev->bus, mdiodev->addr);
 	if (!ps)
-		return -ENOMEM;
-
-	ps->dev = dev;
-	ps->bus = mdiodev->bus;
-	ps->sw_addr = mdiodev->addr;
-	mutex_init(&ps->smi_mutex);
-
-	id = mv88e6xxx_reg_read(ps, REG_PORT(0), PORT_SWITCH_ID);
-	if (id < 0)
-		return id;
-
-	prod_num = (id & 0xfff0) >> 4;
-	rev = id & 0x000f;
-
-	ps->info = mv88e6xxx_lookup_info(prod_num, mv88e6xxx_table,
-					 ARRAY_SIZE(mv88e6xxx_table));
-	if (!ps->info)
 		return -ENODEV;
 
 	ps->reset = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
@@ -3767,9 +3760,6 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 		return err;
 	}
 
-	dev_info(dev, "switch 0x%x probed: %s, revision %u\n",
-		 prod_num, ps->info->name, rev);
-
 	return 0;
 }
 
-- 
2.8.3

[PATCH net-next 7/8] net: dsa: mv88e6xxx: explicit compatible devices

[Vivien Didelot]

Thanks to the new device probing, we can explicit the exact switch model
in the device tree.

Name the driver "mv88e6xxx" and list all its compatible supported chips.

In the meantime, rename the of_device_id table to avoid confusion with a
later introduce matching function.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa.txt  |  6 ++--
 .../devicetree/bindings/net/dsa/marvell.txt        | 19 +++++++++++-
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts          |  6 ++--
 drivers/net/dsa/mv88e6xxx.c                        | 34 ++++++++++++++++------
 4 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt
index 9bbbe7f..cbd88a3 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.txt
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt
@@ -80,7 +80,7 @@ linked into one DSA cluster.
 	#size-cells = <0>;
 
 	switch0: switch0@0 {
-		compatible = "marvell,mv88e6085";
+		compatible = "marvell,mv88e6352";
 		#address-cells = <1>;
 		#size-cells = <0>;
 		reg = <0>;
@@ -135,7 +135,7 @@ linked into one DSA cluster.
 	#size-cells = <0>;
 
 	switch1: switch1@0 {
-		compatible = "marvell,mv88e6085";
+		compatible = "marvell,mv88e6352";
 		#address-cells = <1>;
 		#size-cells = <0>;
 		reg = <0>;
@@ -206,7 +206,7 @@ linked into one DSA cluster.
 	#size-cells = <0>;
 
 	switch2: switch2@0 {
-		compatible = "marvell,mv88e6085";
+		compatible = "marvell,mv88e6352";
 		#address-cells = <1>;
 		#size-cells = <0>;
 		reg = <0>;
diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
index 7629189..1a3a03d 100644
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
@@ -14,7 +14,24 @@ The properties described here are those specific to Marvell devices.
 Additional required and optional properties can be found in dsa.txt.
 
 Required properties:
-- compatible           : Should be one of "marvell,mv88e6085",
+- compatible           : Should be one of:
+                         * "marvell,mv88e6085",
+                         * "marvell,mv88e6095",
+                         * "marvell,mv88e6123",
+                         * "marvell,mv88e6131",
+                         * "marvell,mv88e6161",
+                         * "marvell,mv88e6165",
+                         * "marvell,mv88e6171",
+                         * "marvell,mv88e6172",
+                         * "marvell,mv88e6175",
+                         * "marvell,mv88e6176",
+                         * "marvell,mv88e6185",
+                         * "marvell,mv88e6240",
+                         * "marvell,mv88e6320",
+                         * "marvell,mv88e6321",
+                         * "marvell,mv88e6350",
+                         * "marvell,mv88e6351",
+                         * "marvell,mv88e6352",
 - reg                  : Address on the MII bus for the switch.
 
 Optional properties:
diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index 5c1fcab..8c67181 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -87,7 +87,7 @@
 			#size-cells = <0>;
 
 			switch0: switch0@0 {
-				compatible = "marvell,mv88e6085";
+				compatible = "marvell,mv88e6352";
 				#address-cells = <1>;
 				#size-cells = <0>;
 				reg = <0>;
@@ -142,7 +142,7 @@
 			#size-cells = <0>;
 
 			switch1: switch1@0 {
-				compatible = "marvell,mv88e6085";
+				compatible = "marvell,mv88e6352";
 				#address-cells = <1>;
 				#size-cells = <0>;
 				reg = <0>;
@@ -213,7 +213,7 @@
 			reg = <4>;
 
 			switch2: switch2@0 {
-				compatible = "marvell,mv88e6085";
+				compatible = "marvell,mv88e6185";
 				#address-cells = <1>;
 				#size-cells = <0>;
 				reg = <0>;
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 28070e5..4f07110 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3723,6 +3723,29 @@ static void mv88e6xxx_unregister_switch(struct mv88e6xxx_priv_state *ps)
 	dsa_unregister_switch(ps->ds);
 }
 
+static const struct of_device_id mv88e6xxx_of_id_table[] = {
+	{ .compatible = "marvell,mv88e6085", .data = (void *)MV88E6085 },
+	{ .compatible = "marvell,mv88e6095", .data = (void *)MV88E6095 },
+	{ .compatible = "marvell,mv88e6123", .data = (void *)MV88E6123 },
+	{ .compatible = "marvell,mv88e6131", .data = (void *)MV88E6131 },
+	{ .compatible = "marvell,mv88e6161", .data = (void *)MV88E6161 },
+	{ .compatible = "marvell,mv88e6165", .data = (void *)MV88E6165 },
+	{ .compatible = "marvell,mv88e6171", .data = (void *)MV88E6171 },
+	{ .compatible = "marvell,mv88e6172", .data = (void *)MV88E6172 },
+	{ .compatible = "marvell,mv88e6175", .data = (void *)MV88E6175 },
+	{ .compatible = "marvell,mv88e6176", .data = (void *)MV88E6176 },
+	{ .compatible = "marvell,mv88e6185", .data = (void *)MV88E6185 },
+	{ .compatible = "marvell,mv88e6240", .data = (void *)MV88E6240 },
+	{ .compatible = "marvell,mv88e6320", .data = (void *)MV88E6320 },
+	{ .compatible = "marvell,mv88e6321", .data = (void *)MV88E6321 },
+	{ .compatible = "marvell,mv88e6350", .data = (void *)MV88E6350 },
+	{ .compatible = "marvell,mv88e6351", .data = (void *)MV88E6351 },
+	{ .compatible = "marvell,mv88e6352", .data = (void *)MV88E6352 },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, mv88e6xxx_of_id_table);
+
 int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct device *dev = &mdiodev->dev;
@@ -3772,19 +3795,12 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	mv88e6xxx_mdio_unregister(ps);
 }
 
-static const struct of_device_id mv88e6xxx_of_match[] = {
-	{ .compatible = "marvell,mv88e6085" },
-	{ /* sentinel */ },
-};
-
-MODULE_DEVICE_TABLE(of, mv88e6xxx_of_match);
-
 static struct mdio_driver mv88e6xxx_driver = {
 	.probe	= mv88e6xxx_probe,
 	.remove = mv88e6xxx_remove,
 	.mdiodrv.driver = {
-		.name = "mv88e6085",
-		.of_match_table = mv88e6xxx_of_match,
+		.name = "mv88e6xxx",
+		.of_match_table = mv88e6xxx_of_id_table,
 	},
 };
 
-- 
2.8.3

Re: [PATCH net-next 7/8] net: dsa: mv88e6xxx: explicit compatible devices

[Andrew Lunn]

On Wed, Jun 08, 2016 at 08:44:55PM -0400, Vivien Didelot wrote:
> Thanks to the new device probing, we can explicit the exact switch model
> in the device tree.
> 
> Name the driver "mv88e6xxx" and list all its compatible supported chips.

No. This goes against the usual way of doing device tree compatible
strings. As far as probing goes, all the currently supported switches
are compatible with 6095. We can at run time determine the specific
switch model. This list is just a pain to managed, and has no value.

We only need to add a new compatible string when we cannot determine
at probe time what a switch model is, or we need to read the ID
register in a different way.

	 Andrew

Re: [PATCH net-next 7/8] net: dsa: mv88e6xxx: explicit compatible devices

[Vivien Didelot]

Hi,

Andrew Lunn <andrew@lunn.ch> writes:

> On Wed, Jun 08, 2016 at 08:44:55PM -0400, Vivien Didelot wrote:
>> Thanks to the new device probing, we can explicit the exact switch model
>> in the device tree.
>> 
>> Name the driver "mv88e6xxx" and list all its compatible supported chips.
>
> No. This goes against the usual way of doing device tree compatible
> strings. As far as probing goes, all the currently supported switches
> are compatible with 6095. We can at run time determine the specific
> switch model. This list is just a pain to managed, and has no value.
>
> We only need to add a new compatible string when we cannot determine
> at probe time what a switch model is, or we need to read the ID
> register in a different way.

So thinking about this, I might agree that a "compatible" string per
model is not necessary (even though some drivers are doing this, such as
b53), but at least we might want one compatible string per Marvell
switch family. They have different number of ports, different way to
access them via SMI, different way to access the switch ID register.
this information is useful at probe time.

If one string per model is not recommended, I'd suggest one per
family. What do you guys think?

Thanks,

        Vivien

[PATCH net-next 8/8] net: dsa: mv88e6xxx: fail on mismatching probe

[Vivien Didelot]

Now that we have access at probe time to the chip info described in the
device tree, check if the probed device matches the device node,
otherwise warn the user and fail.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 4f07110..8244757 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -21,6 +21,7 @@
 #include <linux/list.h>
 #include <linux/mdio.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/of_mdio.h>
 #include <linux/netdevice.h>
 #include <linux/gpio/consumer.h>
@@ -3746,6 +3747,28 @@ static const struct of_device_id mv88e6xxx_of_id_table[] = {
 
 MODULE_DEVICE_TABLE(of, mv88e6xxx_of_id_table);
 
+static bool mv88e6xxx_of_matches(struct mv88e6xxx_priv_state *ps)
+{
+	const struct mv88e6xxx_info *info;
+	const struct of_device_id *id;
+	enum mv88e6xxx_model model;
+
+	id = of_match_device(mv88e6xxx_of_id_table, ps->dev);
+	if (!id)
+		return false;
+
+	model = (enum mv88e6xxx_model)id->data;
+	info = &mv88e6xxx_table[model];
+
+	if (ps->info->prod_num == info->prod_num)
+		return true;
+
+	dev_err(ps->dev, "described node %s mismatches probed model %s\n",
+		id->compatible, ps->info->name);
+
+	return false;
+}
+
 int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct device *dev = &mdiodev->dev;
@@ -3758,6 +3781,9 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (!ps)
 		return -ENODEV;
 
+	if (!mv88e6xxx_of_matches(ps))
+		return -EINVAL;
+
 	ps->reset = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
 	if (IS_ERR(ps->reset)) {
 		err = PTR_ERR(ps->reset);
-- 
2.8.3

Re: [PATCH net-next 8/8] net: dsa: mv88e6xxx: fail on mismatching probe

[Andrew Lunn]

On Wed, Jun 08, 2016 at 08:44:56PM -0400, Vivien Didelot wrote:
> Now that we have access at probe time to the chip info described in the
> device tree, check if the probed device matches the device node,
> otherwise warn the user and fail.

What good is this? So what if the device tree says a different
model. We don't care, we don't use that information at all, we read it
from the device itself.

The only thing that might make sense to check is the number of ports
in device tree against what we know the switch has. I don't think we
currently do this. But that actually requires a new method in the
driver structure, so the core can ask the driver after probe how many
ports it has.

       Andrew

Re: [PATCH net-next 8/8] net: dsa: mv88e6xxx: fail on mismatching probe

[Vivien Didelot]

Hi,

Andrew Lunn <andrew@lunn.ch> writes:

> On Wed, Jun 08, 2016 at 08:44:56PM -0400, Vivien Didelot wrote:
>> Now that we have access at probe time to the chip info described in the
>> device tree, check if the probed device matches the device node,
>> otherwise warn the user and fail.
>
> What good is this? So what if the device tree says a different
> model. We don't care, we don't use that information at all, we read it
> from the device itself.

So we can end up with a badly described device tree. It seems to be a
question of rigor vs. flexibility. I don't know much about the DT
philosophy and I don't really mind as long as we warn the user.

I'd like to have other opinions on this though before pushing v2.

Thanks,

        Vivien