linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements
@ 2016-06-14 18:31 Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 01/12] net: dsa: mv88e6xxx: fix style issues Vivien Didelot
                   ` (11 more replies)
  0 siblings, 12 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

Some switch models have different way to access their internal registers
through SMI, and different places where to find the switch ID register.

This patchset abstracts these differences with a new SMI ops structure
and new data in the info structure: the port_base_addr member indicates
where start the ports registers and the MV88E6XXX_FLAG_MULTI_CHIP flag
indicates the need for an indirect SMI access.

The new MDIO probe code uses the compatible info data to detect the
device, and the legacy probe code iterate on compatible info table
(which currently only contains the 88E6085 info) to detect devices.

With these changes, the driver can easily support other switch models
with different register access. For instance, the 88E6060 uses a direct
SMI access even if the chip SMI address is non-zero and port registers
(where the switch ID registers is located) start at 0x8. The port
registers of the 88E6390X start at 0x10. Adding support to probe these
two models would basically look like this:

     static const struct mv88e6xxx_info mv88e6xxx_table[] = {
    +    [MV88E6060] = {
    +        .prod_num = PORT_SWITCH_ID_PROD_NUM_6060,
    +        .family = MV88E6XXX_FAMILY_6060,
    +        .name = "Marvell 88E6060",
    +        .num_databases = 16,
    +        .num_ports = 6,
    +        .port_base_addr = 0x8,
    +        .flags = ...,
    +    },
    +
         [MV88E6085] = {
             .prod_num = PORT_SWITCH_ID_PROD_NUM_6085,
             .family = MV88E6XXX_FAMILY_6097,
    ...
             .port_base_addr = 0x10,
             .flags = MV88E6XXX_FLAGS_FAMILY_6352,
         },
    +
    +    [MV88E6390] = {
    +        .prod_num = PORT_SWITCH_ID_PROD_NUM_6390,
    +        .family = MV88E6XXX_FAMILY_6390,
    +        .name = "Marvell 88E6390X",
    +        .num_databases = 4096,
    +        .num_ports = 11,
    +        .port_base_addr = 0x0,
    +        .flags = ... | MV88E6XXX_FLAG_MULTI_CHIP,
    +    },
     };
     
     static const struct of_device_id mv88e6xxx_of_id_table[] = {
         {
    +        .compatible = "marvell,mv88e6060",
    +        .data = &mv88e6xxx_table[MV88E6060],
    +    }, {
             .compatible = "marvell,mv88e6085",
             .data = &mv88e6xxx_table[MV88E6085],
    +    }, {
    +        .compatible = "marvell,mv88e6390",
    +        .data = &mv88e6xxx_table[MV88E6390],
         },
         { /* sentinel */ },
     };

This patchset also adds helpers to abstract common code of probe
functions and make them more readable before adding the changes
described above.

Changes since v1 [1]:

  - merge style fix from Ben Dooks
  - add Acked-by/Reviewed-by tags
  - drop one compatible string per model
  - detect the SMI device based on the compatible info
  - add an SMI ops structure

[1] https://lkml.org/lkml/2016/6/8/1201

Vivien Didelot (12):
  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 port base address to info
  net: dsa: mv88e6xxx: put chip info in ID table
  net: dsa: mv88e6xxx: read switch ID from info
  net: dsa: mv88e6xxx: add SMI detection helper
  net: dsa: mv88e6xxx: iterate on compatible info
  net: dsa: mv88e6xxx: add an SMI ops structure
  net: dsa: mv88e6xxx: add addressing mode to info

 drivers/net/dsa/mv88e6xxx.c | 342 ++++++++++++++++++++++++++++----------------
 drivers/net/dsa/mv88e6xxx.h |  26 +++-
 2 files changed, 244 insertions(+), 124 deletions(-)

-- 
2.8.3

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 01/12] net: dsa: mv88e6xxx: fix style issues
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 21:14   ` Andrew Lunn
  2016-06-14 18:31 ` [PATCH v2 net-next v2 02/12] net: dsa: mv88e6xxx: remove redundant assignments Vivien Didelot
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, 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

It also rebases and integrates changes sent by Ben Dooks [1]:

    The driver has a number of functions that are not exported or
    declared elsewhere, so make them static to avoid the following
    warnings from sparse:

    drivers/net/dsa/mv88e6xxx.c:113:5: warning: symbol 'mv88e6xxx_reg_read' was not declared. Should it be static?
    drivers/net/dsa/mv88e6xxx.c:167:5: warning: symbol 'mv88e6xxx_reg_write' was not declared. Should it be static?
    drivers/net/dsa/mv88e6xxx.c:231:5: warning: symbol 'mv88e6xxx_set_addr' was not declared. Should it be static?
    drivers/net/dsa/mv88e6xxx.c:367:6: warning: symbol 'mv88e6xxx_ppu_state_init' was not declared. Should it be static?
    drivers/net/dsa/mv88e6xxx.c:3157:5: warning: symbol 'mv88e6xxx_phy_page_read' was not declared. Should it be static?
    drivers/net/dsa/mv88e6xxx.c:3169:5: warning: symbol 'mv88e6xxx_phy_page_write' was not declared. Should it be static?
    drivers/net/dsa/mv88e6xxx.c:3583:26: warning: symbol 'mv88e6xxx_switch_driver' was not declared. Should it be static?
    drivers/net/dsa/mv88e6xxx.c:3621:5: warning: symbol 'mv88e6xxx_probe' was not declared. Should it be static?

[1] http://patchwork.ozlabs.org/patch/632708/

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

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index ee06055..1972ec5 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -111,7 +111,8 @@ static int _mv88e6xxx_reg_read(struct mv88e6xxx_priv_state *ps,
 	return ret;
 }
 
-int mv88e6xxx_reg_read(struct mv88e6xxx_priv_state *ps, int addr, int reg)
+static int mv88e6xxx_reg_read(struct mv88e6xxx_priv_state *ps, int addr,
+			      int reg)
 {
 	int ret;
 
@@ -165,8 +166,8 @@ static int _mv88e6xxx_reg_write(struct mv88e6xxx_priv_state *ps, int addr,
 	return __mv88e6xxx_reg_write(ps->bus, ps->sw_addr, addr, reg, val);
 }
 
-int mv88e6xxx_reg_write(struct mv88e6xxx_priv_state *ps, int addr,
-			int reg, u16 val)
+static int mv88e6xxx_reg_write(struct mv88e6xxx_priv_state *ps, int addr,
+			       int reg, u16 val)
 {
 	int ret;
 
@@ -229,7 +230,7 @@ static int mv88e6xxx_set_addr_indirect(struct dsa_switch *ds, u8 *addr)
 	return 0;
 }
 
-int mv88e6xxx_set_addr(struct dsa_switch *ds, u8 *addr)
+static int mv88e6xxx_set_addr(struct dsa_switch *ds, u8 *addr)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 
@@ -370,7 +371,7 @@ static void mv88e6xxx_ppu_access_put(struct mv88e6xxx_priv_state *ps)
 	mutex_unlock(&ps->ppu_mutex);
 }
 
-void mv88e6xxx_ppu_state_init(struct mv88e6xxx_priv_state *ps)
+static void mv88e6xxx_ppu_state_init(struct mv88e6xxx_priv_state *ps)
 {
 	mutex_init(&ps->ppu_mutex);
 	INIT_WORK(&ps->ppu_work, mv88e6xxx_ppu_reenable_work);
@@ -490,7 +491,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 +1315,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 +1660,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 +2095,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 +2738,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)) {
@@ -3158,7 +3161,8 @@ unlock:
 	return err;
 }
 
-int mv88e6xxx_mdio_page_read(struct dsa_switch *ds, int port, int page, int reg)
+static int mv88e6xxx_mdio_page_read(struct dsa_switch *ds, int port, int page,
+				    int reg)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int ret;
@@ -3170,8 +3174,8 @@ int mv88e6xxx_mdio_page_read(struct dsa_switch *ds, int port, int page, int reg)
 	return ret;
 }
 
-int mv88e6xxx_mdio_page_write(struct dsa_switch *ds, int port, int page,
-			      int reg, int val)
+static int mv88e6xxx_mdio_page_write(struct dsa_switch *ds, int port, int page,
+				     int reg, int val)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int ret;
@@ -3650,7 +3654,7 @@ static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 	return name;
 }
 
-struct dsa_switch_driver mv88e6xxx_switch_driver = {
+static struct dsa_switch_driver mv88e6xxx_switch_driver = {
 	.tag_protocol		= DSA_TAG_PROTO_EDSA,
 	.probe			= mv88e6xxx_drv_probe,
 	.setup			= mv88e6xxx_setup,
@@ -3686,7 +3690,7 @@ struct dsa_switch_driver mv88e6xxx_switch_driver = {
 	.port_fdb_dump          = mv88e6xxx_port_fdb_dump,
 };
 
-int mv88e6xxx_probe(struct mdio_device *mdiodev)
+static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct device *dev = &mdiodev->dev;
 	struct device_node *np = dev->of_node;
-- 
2.8.3

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 02/12] net: dsa: mv88e6xxx: remove redundant assignments
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 01/12] net: dsa: mv88e6xxx: fix style issues Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 03/12] net: dsa: mv88e6xxx: use already declared variables Vivien Didelot
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 1972ec5..673283a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3708,7 +3708,6 @@ static 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);
@@ -3748,8 +3747,6 @@ static 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 03/12] net: dsa: mv88e6xxx: use already declared variables
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 01/12] net: dsa: mv88e6xxx: fix style issues Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 02/12] net: dsa: mv88e6xxx: remove redundant assignments Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 04/12] net: dsa: mv88e6xxx: do not increment bus refcount Vivien Didelot
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 673283a..b3170ea 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3728,7 +3728,7 @@ static 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) {
@@ -3743,13 +3743,13 @@ static 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 04/12] net: dsa: mv88e6xxx: do not increment bus refcount
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (2 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 03/12] net: dsa: mv88e6xxx: use already declared variables Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 05/12] net: dsa: mv88e6xxx: add switch register helpers Vivien Didelot
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, 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>
Acked-by: Andrew Lunn <andrew@lunn.ch>
---
 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 b3170ea..4b4bffc 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3712,8 +3712,6 @@ static 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);
@@ -3767,7 +3765,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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 05/12] net: dsa: mv88e6xxx: add switch register helpers
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (3 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 04/12] net: dsa: mv88e6xxx: do not increment bus refcount Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 21:17   ` Andrew Lunn
  2016-06-14 18:31 ` [PATCH v2 net-next v2 06/12] net: dsa: mv88e6xxx: add port base address to info Vivien Didelot
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

The mixed assignments, allocations and registrations in the probe code
make it hard to follow the logic and figure out what is DSA or chip
specific.

Extract the struct dsa_switch related code in a simple
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 4b4bffc..ad7735d 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3690,30 +3690,48 @@ static 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);
+}
+
 static 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;
@@ -3745,9 +3763,7 @@ static 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;
@@ -3764,8 +3780,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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 06/12] net: dsa: mv88e6xxx: add port base address to info
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (4 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 05/12] net: dsa: mv88e6xxx: add switch register helpers Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 07/12] net: dsa: mv88e6xxx: put chip info in ID table Vivien Didelot
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

Not all Marvell switches have their Port Registers SMI Addresses
starting at 0x10. Add this data in the info structure.

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

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index ad7735d..6f43280 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3442,6 +3442,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6085",
 		.num_databases = 4096,
 		.num_ports = 10,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6097,
 	},
 
@@ -3451,6 +3452,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6095/88E6095F",
 		.num_databases = 256,
 		.num_ports = 11,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6095,
 	},
 
@@ -3460,6 +3462,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6123",
 		.num_databases = 4096,
 		.num_ports = 3,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6165,
 	},
 
@@ -3469,6 +3472,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6131",
 		.num_databases = 256,
 		.num_ports = 8,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6185,
 	},
 
@@ -3478,6 +3482,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6161",
 		.num_databases = 4096,
 		.num_ports = 6,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6165,
 	},
 
@@ -3487,6 +3492,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6165",
 		.num_databases = 4096,
 		.num_ports = 6,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6165,
 	},
 
@@ -3496,6 +3502,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6171",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6351,
 	},
 
@@ -3505,6 +3512,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6172",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6352,
 	},
 
@@ -3514,6 +3522,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6175",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6351,
 	},
 
@@ -3523,6 +3532,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6176",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6352,
 	},
 
@@ -3532,6 +3542,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6185",
 		.num_databases = 256,
 		.num_ports = 10,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6185,
 	},
 
@@ -3541,6 +3552,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6240",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6352,
 	},
 
@@ -3550,6 +3562,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6320",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6320,
 	},
 
@@ -3559,6 +3572,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6321",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6320,
 	},
 
@@ -3568,6 +3582,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6350",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6351,
 	},
 
@@ -3577,6 +3592,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6351",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6351,
 	},
 
@@ -3586,6 +3602,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6352",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.port_base_addr = 0x10,
 		.flags = MV88E6XXX_FLAGS_FAMILY_6352,
 	},
 };
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 8221c3c..613cfa1 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -519,6 +519,7 @@ struct mv88e6xxx_info {
 	const char *name;
 	unsigned int num_databases;
 	unsigned int num_ports;
+	unsigned int port_base_addr;
 	unsigned long flags;
 };
 
-- 
2.8.3

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 07/12] net: dsa: mv88e6xxx: put chip info in ID table
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (5 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 06/12] net: dsa: mv88e6xxx: add port base address to info Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 08/12] net: dsa: mv88e6xxx: read switch ID from info Vivien Didelot
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

Add the chip info structure as the data of the compatible of device,
which will be used later by probe code.

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

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 6f43280..8c39dd0 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3607,6 +3607,16 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 	},
 };
 
+static const struct of_device_id mv88e6xxx_of_id_table[] = {
+	{
+		.compatible = "marvell,mv88e6085",
+		.data = &mv88e6xxx_table[MV88E6085],
+	},
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, mv88e6xxx_of_id_table);
+
 static const struct mv88e6xxx_info *
 mv88e6xxx_lookup_info(unsigned int prod_num, const struct mv88e6xxx_info *table,
 		      unsigned int num)
@@ -3801,19 +3811,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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 08/12] net: dsa: mv88e6xxx: read switch ID from info
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (6 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 07/12] net: dsa: mv88e6xxx: put chip info in ID table Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 18:50   ` Sergei Shtylyov
  2016-06-14 18:31 ` [PATCH v2 net-next v2 09/12] net: dsa: mv88e6xxx: add SMI detection helper Vivien Didelot
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

Retrieve the info structure of the compatible of device in the new probe
function, in order to know how to access the switch ID register.

That way, a compatible info can be used to describe how to access the
switch registers on models with different registers layout or addressing
modes.

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

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 8c39dd0..8ac9f9a 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>
@@ -3745,6 +3746,8 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 {
 	struct device *dev = &mdiodev->dev;
 	struct device_node *np = dev->of_node;
+	const struct of_device_id *of_id;
+	const struct mv88e6xxx_info *info;
 	struct mv88e6xxx_priv_state *ps;
 	int id, prod_num, rev;
 	u32 eeprom_len;
@@ -3759,7 +3762,13 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	ps->sw_addr = mdiodev->addr;
 	mutex_init(&ps->smi_mutex);
 
-	id = mv88e6xxx_reg_read(ps, REG_PORT(0), PORT_SWITCH_ID);
+	of_id = of_match_node(mv88e6xxx_of_id_table, np);
+	if (!of_id)
+		return -EINVAL;
+
+	info = (const struct mv88e6xxx_info *)of_id->data;
+
+	id = mv88e6xxx_reg_read(ps, info->port_base_addr, PORT_SWITCH_ID);
 	if (id < 0)
 		return id;
 
-- 
2.8.3

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 09/12] net: dsa: mv88e6xxx: add SMI detection helper
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (7 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 08/12] net: dsa: mv88e6xxx: read switch ID from info Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 21:50   ` Andrew Lunn
  2016-06-14 18:31 ` [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info Vivien Didelot
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

Extract the allocation and switch ID reading code used by both legacy
and new probing into an helper function which uses a info structure to
describe how to access the switch ID register.

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

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 8ac9f9a..2f36d01 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3631,22 +3631,15 @@ 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_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
+		     const struct mv88e6xxx_info *info)
 {
-	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);
+	id = __mv88e6xxx_reg_read(bus, sw_addr, info->port_base_addr,
+				  PORT_SWITCH_ID);
 	if (id < 0)
 		return NULL;
 
@@ -3658,28 +3651,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_smi_detect(dsa_dev, bus, sw_addr, &mv88e6xxx_table[0]);
+	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;
 }
 
 static struct dsa_switch_driver mv88e6xxx_switch_driver = {
@@ -3749,35 +3760,17 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	const struct of_device_id *of_id;
 	const struct mv88e6xxx_info *info;
 	struct mv88e6xxx_priv_state *ps;
-	int id, prod_num, rev;
 	u32 eeprom_len;
 	int err;
 
-	ps = devm_kzalloc(dev, sizeof(*ps), GFP_KERNEL);
-	if (!ps)
-		return -ENOMEM;
-
-	ps->dev = dev;
-	ps->bus = mdiodev->bus;
-	ps->sw_addr = mdiodev->addr;
-	mutex_init(&ps->smi_mutex);
-
 	of_id = of_match_node(mv88e6xxx_of_id_table, np);
 	if (!of_id)
 		return -EINVAL;
 
 	info = (const struct mv88e6xxx_info *)of_id->data;
 
-	id = mv88e6xxx_reg_read(ps, info->port_base_addr, 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)
+	ps = mv88e6xxx_smi_detect(dev, mdiodev->bus, mdiodev->addr, info);
+	if (!ps)
 		return -ENODEV;
 
 	ps->reset = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
@@ -3805,9 +3798,6 @@ static 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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (8 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 09/12] net: dsa: mv88e6xxx: add SMI detection helper Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 19:38   ` Sergei Shtylyov
  2016-06-14 21:46   ` Andrew Lunn
  2016-06-14 18:31 ` [PATCH v2 net-next v2 11/12] net: dsa: mv88e6xxx: add an SMI ops structure Vivien Didelot
  2016-06-14 18:31 ` [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot
  11 siblings, 2 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

With legacy probing, we cannot have a compatible info structure. We have
to guess it. Instead of using only the first info structure of the info
table, iterate over the compatible data.

That way, the legacy code will support new compatible chips with
different register access without requiring any code change.

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

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 2f36d01..88c09d5 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3668,6 +3668,25 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
 	return ps;
 }
 
+static struct mv88e6xxx_priv_state *
+mv88e6xxx_drv_detect(struct device *dev, struct mii_bus *bus, int sw_addr)
+{
+	struct mv88e6xxx_priv_state *ps = NULL;
+	const struct mv88e6xxx_info *info;
+	const struct of_device_id *id;
+
+	/* Iterate over compatible info to detect the chip */
+	for (id = &mv88e6xxx_of_id_table[0]; id && id->data; ++id) {
+		info = (const struct mv88e6xxx_info *)id->data;
+
+		ps = mv88e6xxx_smi_detect(dev, bus, sw_addr, info);
+		if (ps)
+			break;
+	}
+
+	return ps;
+}
+
 static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 				       struct device *host_dev, int sw_addr,
 				       void **priv)
@@ -3680,7 +3699,7 @@ static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 	if (!bus)
 		return NULL;
 
-	ps = mv88e6xxx_smi_detect(dsa_dev, bus, sw_addr, &mv88e6xxx_table[0]);
+	ps = mv88e6xxx_drv_detect(dsa_dev, bus, sw_addr);
 	if (!ps)
 		return NULL;
 
-- 
2.8.3

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 11/12] net: dsa: mv88e6xxx: add an SMI ops structure
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (9 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 22:04   ` Andrew Lunn
  2016-06-14 18:31 ` [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot
  11 siblings, 1 reply; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

The Marvell switch models have different mode the access the internal
SMI registers. When the chip address on the SMI master bus is 0, the
chips respond to all SMI devices addresses known to them. When the chip
address is not zero, most chips use an indirect access to registers
using two SMI Command and Data registers.

Add a pointer to a new SMI ops structure in the chip to explicit the
addressing mode and simplify the probing code.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 133 +++++++++++++++++++++++++++++---------------
 drivers/net/dsa/mv88e6xxx.h |   9 +++
 2 files changed, 98 insertions(+), 44 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 88c09d5..fc28a6c 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -46,6 +46,38 @@ static void assert_smi_lock(struct mv88e6xxx_priv_state *ps)
  * an indirect addressing mechanism needs to be used to access its
  * registers.
  */
+
+static int mv88e6xxx_smi_direct_read(struct mii_bus *bus, int sw_addr,
+				     int addr, int reg, u16 *val)
+{
+	int ret;
+
+	ret = mdiobus_read_nested(bus, addr, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret & 0xffff;
+
+	return 0;
+}
+
+static int mv88e6xxx_smi_direct_write(struct mii_bus *bus, int sw_addr,
+				      int addr, int reg, u16 val)
+{
+	int ret;
+
+	ret = mdiobus_write_nested(bus, addr, reg, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static const struct mv88e6xxx_smi_ops mv88e6xxx_smi_direct_ops = {
+	.read = mv88e6xxx_smi_direct_read,
+	.write = mv88e6xxx_smi_direct_write,
+};
+
 static int mv88e6xxx_reg_wait_ready(struct mii_bus *bus, int sw_addr)
 {
 	int ret;
@@ -63,14 +95,11 @@ static int mv88e6xxx_reg_wait_ready(struct mii_bus *bus, int sw_addr)
 	return -ETIMEDOUT;
 }
 
-static int __mv88e6xxx_reg_read(struct mii_bus *bus, int sw_addr, int addr,
-				int reg)
+static int mv88e6xxx_smi_indirect_read(struct mii_bus *bus, int sw_addr,
+				       int addr, int reg, u16 *val)
 {
 	int ret;
 
-	if (sw_addr == 0)
-		return mdiobus_read_nested(bus, addr, reg);
-
 	/* Wait for the bus to become free. */
 	ret = mv88e6xxx_reg_wait_ready(bus, sw_addr);
 	if (ret < 0)
@@ -92,46 +121,16 @@ static int __mv88e6xxx_reg_read(struct mii_bus *bus, int sw_addr, int addr,
 	if (ret < 0)
 		return ret;
 
-	return ret & 0xffff;
-}
-
-static int _mv88e6xxx_reg_read(struct mv88e6xxx_priv_state *ps,
-			       int addr, int reg)
-{
-	int ret;
-
-	assert_smi_lock(ps);
-
-	ret = __mv88e6xxx_reg_read(ps->bus, ps->sw_addr, addr, reg);
-	if (ret < 0)
-		return ret;
-
-	dev_dbg(ps->dev, "<- addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
-		addr, reg, ret);
-
-	return ret;
-}
-
-static int mv88e6xxx_reg_read(struct mv88e6xxx_priv_state *ps, int addr,
-			      int reg)
-{
-	int ret;
+	*val = ret & 0xffff;
 
-	mutex_lock(&ps->smi_mutex);
-	ret = _mv88e6xxx_reg_read(ps, addr, reg);
-	mutex_unlock(&ps->smi_mutex);
-
-	return ret;
+	return 0;
 }
 
-static int __mv88e6xxx_reg_write(struct mii_bus *bus, int sw_addr, int addr,
-				 int reg, u16 val)
+static int mv88e6xxx_smi_indirect_write(struct mii_bus *bus, int sw_addr,
+					int addr, int reg, u16 val)
 {
 	int ret;
 
-	if (sw_addr == 0)
-		return mdiobus_write_nested(bus, addr, reg, val);
-
 	/* Wait for the bus to become free. */
 	ret = mv88e6xxx_reg_wait_ready(bus, sw_addr);
 	if (ret < 0)
@@ -156,15 +155,56 @@ static int __mv88e6xxx_reg_write(struct mii_bus *bus, int sw_addr, int addr,
 	return 0;
 }
 
+static const struct mv88e6xxx_smi_ops mv88e6xxx_smi_indirect_ops = {
+	.read = mv88e6xxx_smi_indirect_read,
+	.write = mv88e6xxx_smi_indirect_write,
+};
+
+static int _mv88e6xxx_reg_read(struct mv88e6xxx_priv_state *ps,
+			       int addr, int reg)
+{
+	u16 val;
+	int err;
+
+	assert_smi_lock(ps);
+
+	err = ps->smi_ops->read(ps->bus, ps->sw_addr, addr, reg, &val);
+	if (err)
+		return err;
+
+	dev_dbg(ps->dev, "<- addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
+		addr, reg, val);
+
+	return val;
+}
+
+static int mv88e6xxx_reg_read(struct mv88e6xxx_priv_state *ps, int addr,
+			      int reg)
+{
+	int ret;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_reg_read(ps, addr, reg);
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
 static int _mv88e6xxx_reg_write(struct mv88e6xxx_priv_state *ps, int addr,
 				int reg, u16 val)
 {
+	int err;
+
 	assert_smi_lock(ps);
 
+	err = ps->smi_ops->write(ps->bus, ps->sw_addr, addr, reg, val);
+	if (err)
+		return err;
+
 	dev_dbg(ps->dev, "-> addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
 		addr, reg, val);
 
-	return __mv88e6xxx_reg_write(ps->bus, ps->sw_addr, addr, reg, val);
+	return 0;
 }
 
 static int mv88e6xxx_reg_write(struct mv88e6xxx_priv_state *ps, int addr,
@@ -3635,12 +3675,16 @@ static struct mv88e6xxx_priv_state *
 mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
 		     const struct mv88e6xxx_info *info)
 {
+	const struct mv88e6xxx_smi_ops *ops;
 	struct mv88e6xxx_priv_state *ps;
-	int id, prod_num, rev;
+	int prod_num, rev;
+	u16 id;
+
+	ops = &mv88e6xxx_smi_direct_ops;
+	if (sw_addr > 0)
+		ops = &mv88e6xxx_smi_indirect_ops;
 
-	id = __mv88e6xxx_reg_read(bus, sw_addr, info->port_base_addr,
-				  PORT_SWITCH_ID);
-	if (id < 0)
+	if (ops->read(bus, sw_addr, info->port_base_addr, PORT_SWITCH_ID, &id))
 		return NULL;
 
 	prod_num = (id & 0xfff0) >> 4;
@@ -3661,6 +3705,7 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
 	ps->dev = dev;
 	ps->bus = bus;
 	ps->sw_addr = sw_addr;
+	ps->smi_ops = ops;
 	ps->info = info;
 
 	mutex_init(&ps->smi_mutex);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 613cfa1..100e4e6 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -542,6 +542,13 @@ struct mv88e6xxx_vtu_stu_entry {
 	u8	data[DSA_MAX_PORTS];
 };
 
+struct mv88e6xxx_smi_ops {
+	int (*read)(struct mii_bus *bus, int sw_addr,
+		    int addr, int reg, u16 *val);
+	int (*write)(struct mii_bus *bus, int sw_addr,
+		     int addr, int reg, u16 val);
+};
+
 struct mv88e6xxx_priv_port {
 	struct net_device *bridge_dev;
 };
@@ -555,6 +562,8 @@ struct mv88e6xxx_priv_state {
 	/* The device this structure is associated to */
 	struct device *dev;
 
+	const struct mv88e6xxx_smi_ops *smi_ops;
+
 	/* When using multi-chip addressing, this mutex protects
 	 * access to the indirect access registers.  (In single-chip
 	 * mode, this mutex is effectively useless.)
-- 
2.8.3

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
  2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
                   ` (10 preceding siblings ...)
  2016-06-14 18:31 ` [PATCH v2 net-next v2 11/12] net: dsa: mv88e6xxx: add an SMI ops structure Vivien Didelot
@ 2016-06-14 18:31 ` Vivien Didelot
  2016-06-14 21:34   ` Andrew Lunn
  2016-06-14 22:09   ` Andrew Lunn
  11 siblings, 2 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 18:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn,
	Florian Fainelli, Vivien Didelot

When the SMI address of the switch chip on the SMI master bus is not
zero, some chips (e.g. 88E6352) use an indirect access through two SMI
Command and Data registers, while others (e.g. 88E6060) still use a
direct access.

Add a capability flag to describe chips supporting the Multi-chip
Addressing Mode.

Use the SMI indirect access ops only for switches with this flag and
change the direct SMI direct access ops to support non-zero chip
address.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c |  6 +++---
 drivers/net/dsa/mv88e6xxx.h | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index fc28a6c..8e12246 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -52,7 +52,7 @@ static int mv88e6xxx_smi_direct_read(struct mii_bus *bus, int sw_addr,
 {
 	int ret;
 
-	ret = mdiobus_read_nested(bus, addr, reg);
+	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
 	if (ret < 0)
 		return ret;
 
@@ -66,7 +66,7 @@ static int mv88e6xxx_smi_direct_write(struct mii_bus *bus, int sw_addr,
 {
 	int ret;
 
-	ret = mdiobus_write_nested(bus, addr, reg, val);
+	ret = mdiobus_write_nested(bus, sw_addr + addr, reg, val);
 	if (ret < 0)
 		return ret;
 
@@ -3681,7 +3681,7 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
 	u16 id;
 
 	ops = &mv88e6xxx_smi_direct_ops;
-	if (sw_addr > 0)
+	if (sw_addr > 0 && info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
 		ops = &mv88e6xxx_smi_indirect_ops;
 
 	if (ops->read(bus, sw_addr, info->port_base_addr, PORT_SWITCH_ID, &id))
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 100e4e6..3a5ef6a 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -387,6 +387,12 @@ enum mv88e6xxx_cap {
 	 */
 	MV88E6XXX_CAP_EEPROM,
 
+	/* Multi-chip Addressing Mode.
+	 * Some chips require an indirect SMI access when their SMI device
+	 * address is not zero. to See SMI Command and Data Registers.
+	 */
+	MV88E6XXX_CAP_MULTI_CHIP,
+
 	/* Port State Filtering for 802.1D Spanning Tree.
 	 * See PORT_CONTROL_STATE_* values in the PORT_CONTROL register.
 	 */
@@ -439,6 +445,7 @@ enum mv88e6xxx_cap {
 #define MV88E6XXX_FLAG_ATU		BIT(MV88E6XXX_CAP_ATU)
 #define MV88E6XXX_FLAG_EEE		BIT(MV88E6XXX_CAP_EEE)
 #define MV88E6XXX_FLAG_EEPROM		BIT(MV88E6XXX_CAP_EEPROM)
+#define MV88E6XXX_FLAG_MULTI_CHIP	BIT(MV88E6XXX_CAP_MULTI_CHIP)
 #define MV88E6XXX_FLAG_PORTSTATE	BIT(MV88E6XXX_CAP_PORTSTATE)
 #define MV88E6XXX_FLAG_PPU		BIT(MV88E6XXX_CAP_PPU)
 #define MV88E6XXX_FLAG_PPU_ACTIVE	BIT(MV88E6XXX_CAP_PPU_ACTIVE)
@@ -452,25 +459,29 @@ enum mv88e6xxx_cap {
 
 #define MV88E6XXX_FLAGS_FAMILY_6095	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PPU |		\
 	 MV88E6XXX_FLAG_VLANTABLE |	\
 	 MV88E6XXX_FLAG_VTU)
 
 #define MV88E6XXX_FLAGS_FAMILY_6097	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PPU |		\
 	 MV88E6XXX_FLAG_STU |		\
 	 MV88E6XXX_FLAG_VLANTABLE |	\
 	 MV88E6XXX_FLAG_VTU)
 
 #define MV88E6XXX_FLAGS_FAMILY_6165	\
-	(MV88E6XXX_FLAG_STU |		\
+	(MV88E6XXX_FLAG_MULTI_CHIP |	\
+	 MV88E6XXX_FLAG_STU |		\
 	 MV88E6XXX_FLAG_SWITCH_MAC |	\
 	 MV88E6XXX_FLAG_TEMP |		\
 	 MV88E6XXX_FLAG_VTU)
 
 #define MV88E6XXX_FLAGS_FAMILY_6185	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PPU |		\
 	 MV88E6XXX_FLAG_VLANTABLE |	\
 	 MV88E6XXX_FLAG_VTU)
@@ -479,6 +490,7 @@ enum mv88e6xxx_cap {
 	(MV88E6XXX_FLAG_ATU |		\
 	 MV88E6XXX_FLAG_EEE |		\
 	 MV88E6XXX_FLAG_EEPROM |	\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PORTSTATE |	\
 	 MV88E6XXX_FLAG_PPU_ACTIVE |	\
 	 MV88E6XXX_FLAG_SMI_PHY |	\
@@ -490,6 +502,7 @@ enum mv88e6xxx_cap {
 
 #define MV88E6XXX_FLAGS_FAMILY_6351	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PORTSTATE |	\
 	 MV88E6XXX_FLAG_PPU_ACTIVE |	\
 	 MV88E6XXX_FLAG_SMI_PHY |	\
@@ -503,6 +516,7 @@ enum mv88e6xxx_cap {
 	(MV88E6XXX_FLAG_ATU |		\
 	 MV88E6XXX_FLAG_EEE |		\
 	 MV88E6XXX_FLAG_EEPROM |	\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PORTSTATE |	\
 	 MV88E6XXX_FLAG_PPU_ACTIVE |	\
 	 MV88E6XXX_FLAG_SMI_PHY |	\
-- 
2.8.3

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 08/12] net: dsa: mv88e6xxx: read switch ID from info
  2016-06-14 18:31 ` [PATCH v2 net-next v2 08/12] net: dsa: mv88e6xxx: read switch ID from info Vivien Didelot
@ 2016-06-14 18:50   ` Sergei Shtylyov
  2016-06-14 21:01     ` Vivien Didelot
  0 siblings, 1 reply; 30+ messages in thread
From: Sergei Shtylyov @ 2016-06-14 18:50 UTC (permalink / raw)
  To: Vivien Didelot, netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn, Florian Fainelli

Hello.

On 06/14/2016 09:31 PM, Vivien Didelot wrote:

> Retrieve the info structure of the compatible of device in the new probe
> function, in order to know how to access the switch ID register.
>
> That way, a compatible info can be used to describe how to access the
> switch registers on models with different registers layout or addressing
> modes.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 8c39dd0..8ac9f9a 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
[...]
> @@ -3745,6 +3746,8 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
>  {
>  	struct device *dev = &mdiodev->dev;
>  	struct device_node *np = dev->of_node;
> +	const struct of_device_id *of_id;
> +	const struct mv88e6xxx_info *info;
>  	struct mv88e6xxx_priv_state *ps;
>  	int id, prod_num, rev;
>  	u32 eeprom_len;
> @@ -3759,7 +3762,13 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
>  	ps->sw_addr = mdiodev->addr;
>  	mutex_init(&ps->smi_mutex);
>
> -	id = mv88e6xxx_reg_read(ps, REG_PORT(0), PORT_SWITCH_ID);
> +	of_id = of_match_node(mv88e6xxx_of_id_table, np);

    You could use of_device_get_match_data() here.

> +	if (!of_id)
> +		return -EINVAL;
> +
> +	info = (const struct mv88e6xxx_info *)of_id->data;

    Pointer casts from 'void *' are automatic.

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info
  2016-06-14 18:31 ` [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info Vivien Didelot
@ 2016-06-14 19:38   ` Sergei Shtylyov
  2016-06-14 21:46   ` Andrew Lunn
  1 sibling, 0 replies; 30+ messages in thread
From: Sergei Shtylyov @ 2016-06-14 19:38 UTC (permalink / raw)
  To: Vivien Didelot, netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn, Florian Fainelli

On 06/14/2016 09:31 PM, Vivien Didelot wrote:

> With legacy probing, we cannot have a compatible info structure. We have
> to guess it. Instead of using only the first info structure of the info
> table, iterate over the compatible data.
>
> That way, the legacy code will support new compatible chips with
> different register access without requiring any code change.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 2f36d01..88c09d5 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -3668,6 +3668,25 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
>  	return ps;
>  }
>
> +static struct mv88e6xxx_priv_state *
> +mv88e6xxx_drv_detect(struct device *dev, struct mii_bus *bus, int sw_addr)
> +{
> +	struct mv88e6xxx_priv_state *ps = NULL;
> +	const struct mv88e6xxx_info *info;
> +	const struct of_device_id *id;
> +
> +	/* Iterate over compatible info to detect the chip */
> +	for (id = &mv88e6xxx_of_id_table[0]; id && id->data; ++id) {
> +		info = (const struct mv88e6xxx_info *)id->data;

    The explicit cast shouldn't be needed...

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 08/12] net: dsa: mv88e6xxx: read switch ID from info
  2016-06-14 18:50   ` Sergei Shtylyov
@ 2016-06-14 21:01     ` Vivien Didelot
  0 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 21:01 UTC (permalink / raw)
  To: Sergei Shtylyov, netdev
  Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn, Florian Fainelli

Hi,

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:

>> -	id = mv88e6xxx_reg_read(ps, REG_PORT(0), PORT_SWITCH_ID);
>> +	of_id = of_match_node(mv88e6xxx_of_id_table, np);
>
>     You could use of_device_get_match_data() here.
>
>> +	if (!of_id)
>> +		return -EINVAL;
>> +
>> +	info = (const struct mv88e6xxx_info *)of_id->data;
>
>     Pointer casts from 'void *' are automatic.

I applied your comments and also squashed patches 7 and 8 together.
I'll respin a v3 soon unless there are other comments.

Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 01/12] net: dsa: mv88e6xxx: fix style issues
  2016-06-14 18:31 ` [PATCH v2 net-next v2 01/12] net: dsa: mv88e6xxx: fix style issues Vivien Didelot
@ 2016-06-14 21:14   ` Andrew Lunn
  0 siblings, 0 replies; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 21:14 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

On Tue, Jun 14, 2016 at 02:31:42PM -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
> 
> It also rebases and integrates changes sent by Ben Dooks [1]:
> 
>     The driver has a number of functions that are not exported or
>     declared elsewhere, so make them static to avoid the following
>     warnings from sparse:
> 
>     drivers/net/dsa/mv88e6xxx.c:113:5: warning: symbol 'mv88e6xxx_reg_read' was not declared. Should it be static?
>     drivers/net/dsa/mv88e6xxx.c:167:5: warning: symbol 'mv88e6xxx_reg_write' was not declared. Should it be static?
>     drivers/net/dsa/mv88e6xxx.c:231:5: warning: symbol 'mv88e6xxx_set_addr' was not declared. Should it be static?
>     drivers/net/dsa/mv88e6xxx.c:367:6: warning: symbol 'mv88e6xxx_ppu_state_init' was not declared. Should it be static?
>     drivers/net/dsa/mv88e6xxx.c:3157:5: warning: symbol 'mv88e6xxx_phy_page_read' was not declared. Should it be static?
>     drivers/net/dsa/mv88e6xxx.c:3169:5: warning: symbol 'mv88e6xxx_phy_page_write' was not declared. Should it be static?
>     drivers/net/dsa/mv88e6xxx.c:3583:26: warning: symbol 'mv88e6xxx_switch_driver' was not declared. Should it be static?
>     drivers/net/dsa/mv88e6xxx.c:3621:5: warning: symbol 'mv88e6xxx_probe' was not declared. Should it be static?
> 
> [1] http://patchwork.ozlabs.org/patch/632708/
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

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

    Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 05/12] net: dsa: mv88e6xxx: add switch register helpers
  2016-06-14 18:31 ` [PATCH v2 net-next v2 05/12] net: dsa: mv88e6xxx: add switch register helpers Vivien Didelot
@ 2016-06-14 21:17   ` Andrew Lunn
  0 siblings, 0 replies; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 21:17 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

On Tue, Jun 14, 2016 at 02:31:46PM -0400, Vivien Didelot wrote:
> The mixed assignments, allocations and registrations in the probe code
> make it hard to follow the logic and figure out what is DSA or chip
> specific.
> 
> Extract the struct dsa_switch related code in a simple
> 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>

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

    Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
  2016-06-14 18:31 ` [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot
@ 2016-06-14 21:34   ` Andrew Lunn
  2016-06-14 21:52     ` Vivien Didelot
  2016-06-14 22:09   ` Andrew Lunn
  1 sibling, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 21:34 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

> @@ -3681,7 +3681,7 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
>  	u16 id;
>  
>  	ops = &mv88e6xxx_smi_direct_ops;
> -	if (sw_addr > 0)
> +	if (sw_addr > 0 && info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
>  		ops = &mv88e6xxx_smi_indirect_ops;

Hi Vivien

Is sw_addr is > 0 and MV88E6XXX_FLAG_MULTI_CHIP is not set, you should
return -EINVAL. The device tree is invalid.

       Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info
  2016-06-14 18:31 ` [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info Vivien Didelot
  2016-06-14 19:38   ` Sergei Shtylyov
@ 2016-06-14 21:46   ` Andrew Lunn
  2016-06-14 22:13     ` Vivien Didelot
  1 sibling, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 21:46 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

On Tue, Jun 14, 2016 at 02:31:51PM -0400, Vivien Didelot wrote:
> With legacy probing, we cannot have a compatible info structure. We have
> to guess it. Instead of using only the first info structure of the info
> table, iterate over the compatible data.
> 
> That way, the legacy code will support new compatible chips with
> different register access without requiring any code change.

I don't think this is safe when used in combination with multi-chip
addresses. This code will perform writes on various addresses,
addresses which could be real registers on a device.

I don't see a need to support guessing. The new binding will work,
without any guessing. So use that.

	Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 09/12] net: dsa: mv88e6xxx: add SMI detection helper
  2016-06-14 18:31 ` [PATCH v2 net-next v2 09/12] net: dsa: mv88e6xxx: add SMI detection helper Vivien Didelot
@ 2016-06-14 21:50   ` Andrew Lunn
  2016-06-14 22:10     ` Vivien Didelot
  0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 21:50 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

On Tue, Jun 14, 2016 at 02:31:50PM -0400, Vivien Didelot wrote:
> Extract the allocation and switch ID reading code used by both legacy
> and new probing into an helper function which uses a info structure to
> describe how to access the switch ID register.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c | 74 ++++++++++++++++++++-------------------------
>  1 file changed, 32 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 8ac9f9a..2f36d01 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -3631,22 +3631,15 @@ 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_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
> +		     const struct mv88e6xxx_info *info)
>  {
> -	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);
> +	id = __mv88e6xxx_reg_read(bus, sw_addr, info->port_base_addr,
> +				  PORT_SWITCH_ID);
>  	if (id < 0)
>  		return NULL;
>  
> @@ -3658,28 +3651,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;

I don't like the way this detect function goes a lot further than
detection. I would say detection finished when you have the info
structure. Return at that point, and let the probe do the rest.

       Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
  2016-06-14 21:34   ` Andrew Lunn
@ 2016-06-14 21:52     ` Vivien Didelot
  0 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 21:52 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> @@ -3681,7 +3681,7 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
>>  	u16 id;
>>  
>>  	ops = &mv88e6xxx_smi_direct_ops;
>> -	if (sw_addr > 0)
>> +	if (sw_addr > 0 && info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
>>  		ops = &mv88e6xxx_smi_indirect_ops;
>
> Is sw_addr is > 0 and MV88E6XXX_FLAG_MULTI_CHIP is not set, you should
> return -EINVAL. The device tree is invalid.

OK, I'll change this snippet for the following until we explicitly add
support for such device with non-zero address and direct SMI access:

    if (sw_addr == 0)
        ops = &mv88e6xxx_smi_direct_ops;
    else if (info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
        ops = &mv88e6xxx_smi_indirect_ops;
    else
        return NULL;

Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 11/12] net: dsa: mv88e6xxx: add an SMI ops structure
  2016-06-14 18:31 ` [PATCH v2 net-next v2 11/12] net: dsa: mv88e6xxx: add an SMI ops structure Vivien Didelot
@ 2016-06-14 22:04   ` Andrew Lunn
  2016-06-14 22:26     ` Vivien Didelot
  0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 22:04 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

> +struct mv88e6xxx_smi_ops {
> +	int (*read)(struct mii_bus *bus, int sw_addr,
> +		    int addr, int reg, u16 *val);
> +	int (*write)(struct mii_bus *bus, int sw_addr,
> +		     int addr, int reg, u16 val);
> +};
> +

I think this API would be better if it used ps, not bus and sw_addr.

The only problem is the very first read to get the switch ID. I would
add one more layer in between, so that you can call the lowest level
functions without having a ps structure.

	  Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
  2016-06-14 18:31 ` [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot
  2016-06-14 21:34   ` Andrew Lunn
@ 2016-06-14 22:09   ` Andrew Lunn
  2016-06-14 22:24     ` Vivien Didelot
  1 sibling, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 22:09 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

On Tue, Jun 14, 2016 at 02:31:53PM -0400, Vivien Didelot wrote:
> When the SMI address of the switch chip on the SMI master bus is not
> zero, some chips (e.g. 88E6352) use an indirect access through two SMI
> Command and Data registers, while others (e.g. 88E6060) still use a
> direct access.
> 
> Add a capability flag to describe chips supporting the Multi-chip
> Addressing Mode.
> 
> Use the SMI indirect access ops only for switches with this flag and
> change the direct SMI direct access ops to support non-zero chip
> address.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c |  6 +++---
>  drivers/net/dsa/mv88e6xxx.h | 16 +++++++++++++++-
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index fc28a6c..8e12246 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -52,7 +52,7 @@ static int mv88e6xxx_smi_direct_read(struct mii_bus *bus, int sw_addr,
>  {
>  	int ret;
>  
> -	ret = mdiobus_read_nested(bus, addr, reg);
> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
>  	if (ret < 0)
>  		return ret;

If we are doing direct access, doesn't it means sw_addr is 0?

So isn't this pointless?

   Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 09/12] net: dsa: mv88e6xxx: add SMI detection helper
  2016-06-14 21:50   ` Andrew Lunn
@ 2016-06-14 22:10     ` Vivien Didelot
  0 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 22:10 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> -	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;
>
> I don't like the way this detect function goes a lot further than
> detection. I would say detection finished when you have the info
> structure. Return at that point, and let the probe do the rest.

OK, I split detection and allocation.

Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info
  2016-06-14 21:46   ` Andrew Lunn
@ 2016-06-14 22:13     ` Vivien Didelot
  0 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 22:13 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, Jun 14, 2016 at 02:31:51PM -0400, Vivien Didelot wrote:
>> With legacy probing, we cannot have a compatible info structure. We have
>> to guess it. Instead of using only the first info structure of the info
>> table, iterate over the compatible data.
>> 
>> That way, the legacy code will support new compatible chips with
>> different register access without requiring any code change.
>
> I don't think this is safe when used in combination with multi-chip
> addresses. This code will perform writes on various addresses,
> addresses which could be real registers on a device.
>
> I don't see a need to support guessing. The new binding will work,
> without any guessing. So use that.

OK, I drop this patch and limit the detection in the legacy probing
against the 6085 chip info.

Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
  2016-06-14 22:09   ` Andrew Lunn
@ 2016-06-14 22:24     ` Vivien Didelot
  2016-06-14 22:44       ` Andrew Lunn
  0 siblings, 1 reply; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 22:24 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> -	ret = mdiobus_read_nested(bus, addr, reg);
>> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
>>  	if (ret < 0)
>>  		return ret;
>
> If we are doing direct access, doesn't it means sw_addr is 0?
>
> So isn't this pointless?

6060 has no indirect access and directly responds to 16 SMI addresses,
regardless its chip address which can be strapped to either 0 or 16.

If we want to add support for it in mv88e6xxx someday (which is likely),
the code is ready for that.

Question 1) given this, should I still consider your first comment on
this patch about the mv88e6xxx_smi_ops assignment?

Question 2) is MV88E6XXX_FLAG_MULTI_CHIP confusing? I took a short name
for style but maybe a longer MV88E6XXX_FLAG_MULTI_CHIP_ADDRESSING or
MV88E6XXX_FLAG_MULTI_CHIP_MODE would be clearer to make to distinction
between "Single-chip Addressing Mode" and "Multi-chip Addressing Mode".

Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 11/12] net: dsa: mv88e6xxx: add an SMI ops structure
  2016-06-14 22:04   ` Andrew Lunn
@ 2016-06-14 22:26     ` Vivien Didelot
  0 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 22:26 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> +struct mv88e6xxx_smi_ops {
>> +	int (*read)(struct mii_bus *bus, int sw_addr,
>> +		    int addr, int reg, u16 *val);
>> +	int (*write)(struct mii_bus *bus, int sw_addr,
>> +		     int addr, int reg, u16 val);
>> +};
>> +
>
> I think this API would be better if it used ps, not bus and sw_addr.
>
> The only problem is the very first read to get the switch ID. I would
> add one more layer in between, so that you can call the lowest level
> functions without having a ps structure.

That's why I keep it simple for the moment.

The low-level API using ps is now _mv88e6xxx_reg_{read,write}. I can
rename them to mv88e6xxx_smi_{read,write} in v3 or later.

Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
  2016-06-14 22:24     ` Vivien Didelot
@ 2016-06-14 22:44       ` Andrew Lunn
  2016-06-14 23:11         ` Vivien Didelot
  0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2016-06-14 22:44 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

On Tue, Jun 14, 2016 at 06:24:17PM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> -	ret = mdiobus_read_nested(bus, addr, reg);
> >> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
> >>  	if (ret < 0)
> >>  		return ret;
> >
> > If we are doing direct access, doesn't it means sw_addr is 0?
> >
> > So isn't this pointless?
> 
> 6060 has no indirect access and directly responds to 16 SMI addresses,
> regardless its chip address which can be strapped to either 0 or 16.

Ah! O.K.

wnr854t-setup.c uses 0.
rd88f6183ap-ge-setup.c uses 0.
wrt350n-v2-setup.c uses 0.
rd88f5181l-fxo-setup.c uses 0.
rd88f5181l-ge-setup.c uses 0.
mach-bf518/boards/ezbrd.c uses 0.

The 6060 is a very old device. I doubt we will get any new boards
contributed using it. We are also going to have trouble actually
finding a device with one in order to test a merged mv88e6xxx and
mv88e6060 driver.

So i say we ignore the possibility of an 6060 on 16, until one really
comes along.

> Question 2) is MV88E6XXX_FLAG_MULTI_CHIP confusing?

No, i think it is fine.

    Andrew

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
  2016-06-14 22:44       ` Andrew Lunn
@ 2016-06-14 23:11         ` Vivien Didelot
  0 siblings, 0 replies; 30+ messages in thread
From: Vivien Didelot @ 2016-06-14 23:11 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli

Hi,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, Jun 14, 2016 at 06:24:17PM -0400, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> >> -	ret = mdiobus_read_nested(bus, addr, reg);
>> >> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
>> >>  	if (ret < 0)
>> >>  		return ret;
>> >
>> > If we are doing direct access, doesn't it means sw_addr is 0?
>> >
>> > So isn't this pointless?
>> 
>> 6060 has no indirect access and directly responds to 16 SMI addresses,
>> regardless its chip address which can be strapped to either 0 or 16.
>
> Ah! O.K.
>
> wnr854t-setup.c uses 0.
> rd88f6183ap-ge-setup.c uses 0.
> wrt350n-v2-setup.c uses 0.
> rd88f5181l-fxo-setup.c uses 0.
> rd88f5181l-ge-setup.c uses 0.
> mach-bf518/boards/ezbrd.c uses 0.
>
> The 6060 is a very old device. I doubt we will get any new boards
> contributed using it. We are also going to have trouble actually
> finding a device with one in order to test a merged mv88e6xxx and
> mv88e6060 driver.
>
> So i say we ignore the possibility of an 6060 on 16, until one really
> comes along.

Sounds good to me!

Thanks,

        Vivien

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2016-06-14 23:11 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-14 18:31 [PATCH v2 net-next v2 00/12] net: dsa: mv88e6xxx: misc probe improvements Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 01/12] net: dsa: mv88e6xxx: fix style issues Vivien Didelot
2016-06-14 21:14   ` Andrew Lunn
2016-06-14 18:31 ` [PATCH v2 net-next v2 02/12] net: dsa: mv88e6xxx: remove redundant assignments Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 03/12] net: dsa: mv88e6xxx: use already declared variables Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 04/12] net: dsa: mv88e6xxx: do not increment bus refcount Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 05/12] net: dsa: mv88e6xxx: add switch register helpers Vivien Didelot
2016-06-14 21:17   ` Andrew Lunn
2016-06-14 18:31 ` [PATCH v2 net-next v2 06/12] net: dsa: mv88e6xxx: add port base address to info Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 07/12] net: dsa: mv88e6xxx: put chip info in ID table Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 08/12] net: dsa: mv88e6xxx: read switch ID from info Vivien Didelot
2016-06-14 18:50   ` Sergei Shtylyov
2016-06-14 21:01     ` Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 09/12] net: dsa: mv88e6xxx: add SMI detection helper Vivien Didelot
2016-06-14 21:50   ` Andrew Lunn
2016-06-14 22:10     ` Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 10/12] net: dsa: mv88e6xxx: iterate on compatible info Vivien Didelot
2016-06-14 19:38   ` Sergei Shtylyov
2016-06-14 21:46   ` Andrew Lunn
2016-06-14 22:13     ` Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 11/12] net: dsa: mv88e6xxx: add an SMI ops structure Vivien Didelot
2016-06-14 22:04   ` Andrew Lunn
2016-06-14 22:26     ` Vivien Didelot
2016-06-14 18:31 ` [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot
2016-06-14 21:34   ` Andrew Lunn
2016-06-14 21:52     ` Vivien Didelot
2016-06-14 22:09   ` Andrew Lunn
2016-06-14 22:24     ` Vivien Didelot
2016-06-14 22:44       ` Andrew Lunn
2016-06-14 23:11         ` Vivien Didelot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).