[PATCH net-next v2 1/2] net: dsa: mv88e6xxx: Introduce _mv88e6xxx_phy_page_{read,write}

[PATCH net-next v2 1/2] net: dsa: mv88e6xxx: Introduce _mv88e6xxx_phy_page_{read,write}

[Patrick Uiterwijk]

Add versions of the phy_page_read and _write functions to
be used in a context where the SMI mutex is held.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 drivers/net/dsa/mv88e6xxx.c | 49 +++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index fa086e0..86a2029 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2264,6 +2264,38 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
 	mutex_unlock(&ps->smi_mutex);
 }
 
+static int _mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
+				     int reg, int val)
+{
+	int ret;
+
+	ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
+	if (ret < 0)
+		goto restore_page_0;
+
+	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
+restore_page_0:
+	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+
+	return ret;
+}
+
+static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
+				    int reg)
+{
+	int ret;
+
+	ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
+	if (ret < 0)
+		goto restore_page_0;
+
+	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
+restore_page_0:
+	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+
+	return ret;
+}
+
 static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -2714,13 +2746,9 @@ int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
-	ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
-	if (ret < 0)
-		goto error;
-	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
-error:
-	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+	ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
 	mutex_unlock(&ps->smi_mutex);
+
 	return ret;
 }
 
@@ -2731,14 +2759,9 @@ int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
-	ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
-	if (ret < 0)
-		goto error;
-
-	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
-error:
-	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+	ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
 	mutex_unlock(&ps->smi_mutex);
+
 	return ret;
 }
 
-- 
2.7.4

[PATCH net-next v2 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup

[Patrick Uiterwijk]

Some of the vendor-specific bootloaders set up this part
of the initialization for us, so this was never added.
However, since upstream bootloaders don't initialize the
chip specifically, they leave the fiber MII's PDOWN flag
set, which means that the CPU port doesn't connect.

This patch checks whether this flag has been clear prior
by something else, and if not make us clear it.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 drivers/net/dsa/mv88e6xxx.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  8 ++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 86a2029..9807913 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2296,6 +2296,25 @@ restore_page_0:
 	return ret;
 }
 
+static int mv88e6xxx_power_on_serdes(struct dsa_switch *ds)
+{
+	int ret;
+
+	ret = _mv88e6xxx_phy_page_read(ds, REG_FIBER_SERDES, PAGE_FIBER_SERDES,
+				       MII_BMCR);
+	if (ret < 0)
+		return ret;
+
+	if (ret & BMCR_PDOWN) {
+		ret = ret & ~BMCR_PDOWN;
+		ret = _mv88e6xxx_phy_page_write(ds, REG_FIBER_SERDES,
+						PAGE_FIBER_SERDES, MII_BMCR,
+						ret);
+	}
+
+	return ret;
+}
+
 static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -2399,6 +2418,23 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 			goto abort;
 	}
 
+	/* If this port is connected to a SerDes, make sure the SerDes is not
+	 * powered down.
+	 */
+	if (mv88e6xxx_6352_family(ds)) {
+		ret = _mv88e6xxx_reg_read(ds, REG_PORT(port), PORT_STATUS);
+		if (ret < 0)
+			goto abort;
+		ret &= PORT_STATUS_CMODE_MASK;
+		if ((ret == PORT_STATUS_CMODE_100BASE_X) ||
+		    (ret == PORT_STATUS_CMODE_1000BASE_X) ||
+		    (ret == PORT_STATUS_CMODE_SGMII)) {
+			ret = mv88e6xxx_power_on_serdes(ds);
+			if (ret < 0)
+				goto abort;
+		}
+	}
+
 	/* Port Control 2: don't force a good FCS, set the maximum frame size to
 	 * 10240 bytes, disable 802.1q tags checking, don't discard tagged or
 	 * untagged frames on this port, do a destination address lookup on all
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 9a038ab..26a424a 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -28,6 +28,10 @@
 #define SMI_CMD_OP_45_READ_DATA_INC	((3 << 10) | SMI_CMD_BUSY)
 #define SMI_DATA		0x01
 
+/* Fiber/SERDES Registers are located at SMI address F, page 1 */
+#define REG_FIBER_SERDES	0x0f
+#define PAGE_FIBER_SERDES	0x01
+
 #define REG_PORT(p)		(0x10 + (p))
 #define PORT_STATUS		0x00
 #define PORT_STATUS_PAUSE_EN	BIT(15)
@@ -45,6 +49,10 @@
 #define PORT_STATUS_MGMII	BIT(6) /* 6185 */
 #define PORT_STATUS_TX_PAUSED	BIT(5)
 #define PORT_STATUS_FLOW_CTRL	BIT(4)
+#define PORT_STATUS_CMODE_MASK	0x0f
+#define PORT_STATUS_CMODE_100BASE_X	0x8
+#define PORT_STATUS_CMODE_1000BASE_X	0x9
+#define PORT_STATUS_CMODE_SGMII		0xa
 #define PORT_PCS_CTRL		0x01
 #define PORT_PCS_CTRL_RGMII_DELAY_RXCLK	BIT(15)
 #define PORT_PCS_CTRL_RGMII_DELAY_TXCLK	BIT(14)
-- 
2.7.4

Re: [PATCH net-next v2 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup

[Vivien Didelot]

Hi Patrick,

Two comments below.

Patrick Uiterwijk <patrick@puiterwijk.org> writes:

> +static int mv88e6xxx_power_on_serdes(struct dsa_switch *ds)

Since this function assumes the SMI lock is already held, its name
should be prefixed with _ by convention (_mv88e6xxx_power_on_serdes).

> +{
> +	int ret;
> +
> +	ret = _mv88e6xxx_phy_page_read(ds, REG_FIBER_SERDES, PAGE_FIBER_SERDES,
> +				       MII_BMCR);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & BMCR_PDOWN) {
> +		ret = ret & ~BMCR_PDOWN;

                ret &= ~BMCR_PDOWN;

> +		ret = _mv88e6xxx_phy_page_write(ds, REG_FIBER_SERDES,
> +						PAGE_FIBER_SERDES, MII_BMCR,
> +						ret);
> +	}
> +
> +	return ret;
> +}

Thanks,
Vivien

Re: [PATCH net-next v2 1/2] net: dsa: mv88e6xxx: Introduce _mv88e6xxx_phy_page_{read,write}

[Vivien Didelot]

Patrick Uiterwijk <patrick@puiterwijk.org> writes:

> Add versions of the phy_page_read and _write functions to
> be used in a context where the SMI mutex is held.
>
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>

Tested-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,
Vivien

Re: [PATCH net-next v2 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup

[Andrew Lunn]

On Tue, Mar 29, 2016 at 12:23:06PM -0400, Vivien Didelot wrote:
> Hi Patrick,
> 
> Two comments below.
> 
> Patrick Uiterwijk <patrick@puiterwijk.org> writes:
> 
> > +static int mv88e6xxx_power_on_serdes(struct dsa_switch *ds)
> 
> Since this function assumes the SMI lock is already held, its name
> should be prefixed with _ by convention (_mv88e6xxx_power_on_serdes).

We decided to drop at, since nearly everything would end up with a _
prefix. The assert_smi_lock() should find any missing locks, and
lockdep/deadlocks will make it clear when the lock is taken twice.

	  Andrew

Re: [PATCH net-next v2 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup

[Vivien Didelot]

Hi Andrew, Patrick,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, Mar 29, 2016 at 12:23:06PM -0400, Vivien Didelot wrote:
>> Hi Patrick,
>> 
>> Two comments below.
>> 
>> Patrick Uiterwijk <patrick@puiterwijk.org> writes:
>> 
>> > +static int mv88e6xxx_power_on_serdes(struct dsa_switch *ds)
>> 
>> Since this function assumes the SMI lock is already held, its name
>> should be prefixed with _ by convention (_mv88e6xxx_power_on_serdes).
>
> We decided to drop at, since nearly everything would end up with a _
> prefix. The assert_smi_lock() should find any missing locks, and
> lockdep/deadlocks will make it clear when the lock is taken twice.

OK, I didn't know that. This makes sense. There is no need to respin a
v3 only for my previous &= comment then.

Thanks,
-v

Re: [PATCH net-next v2 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup

[Patrick Uiterwijk]

Hi Vivien,

On Tue, Mar 29, 2016 at 6:49 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> Hi Andrew, Patrick,
>
> Andrew Lunn <andrew@lunn.ch> writes:
>
>> On Tue, Mar 29, 2016 at 12:23:06PM -0400, Vivien Didelot wrote:
>>> Hi Patrick,
>>>
>>> Two comments below.
>>>
>>> Patrick Uiterwijk <patrick@puiterwijk.org> writes:
>>>
>>> > +static int mv88e6xxx_power_on_serdes(struct dsa_switch *ds)
>>>
>>> Since this function assumes the SMI lock is already held, its name
>>> should be prefixed with _ by convention (_mv88e6xxx_power_on_serdes).
>>
>> We decided to drop at, since nearly everything would end up with a _
>> prefix. The assert_smi_lock() should find any missing locks, and
>> lockdep/deadlocks will make it clear when the lock is taken twice.
>
> OK, I didn't know that. This makes sense. There is no need to respin a
> v3 only for my previous &= comment then.

Does that mean the merger will fix this up?
Or that I'll roll a v3 when I get a reviewed-by for the second patch?

Thanks,
Patrick

Re: [PATCH net-next v2 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup

[Andrew Lunn]

On Wed, Mar 30, 2016 at 12:58:04AM +0000, Patrick Uiterwijk wrote:
> Hi Vivien,
> 
> On Tue, Mar 29, 2016 at 6:49 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
> > Hi Andrew, Patrick,
> >
> > Andrew Lunn <andrew@lunn.ch> writes:
> >
> >> On Tue, Mar 29, 2016 at 12:23:06PM -0400, Vivien Didelot wrote:
> >>> Hi Patrick,
> >>>
> >>> Two comments below.
> >>>
> >>> Patrick Uiterwijk <patrick@puiterwijk.org> writes:
> >>>
> >>> > +static int mv88e6xxx_power_on_serdes(struct dsa_switch *ds)
> >>>
> >>> Since this function assumes the SMI lock is already held, its name
> >>> should be prefixed with _ by convention (_mv88e6xxx_power_on_serdes).
> >>
> >> We decided to drop at, since nearly everything would end up with a _
> >> prefix. The assert_smi_lock() should find any missing locks, and
> >> lockdep/deadlocks will make it clear when the lock is taken twice.
> >
> > OK, I didn't know that. This makes sense. There is no need to respin a
> > v3 only for my previous &= comment then.
> 
> Does that mean the merger will fix this up?
> Or that I'll roll a v3 when I get a reviewed-by for the second patch?

Hi Patrick

Role a v3, and you can add

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

as well as Viviens for patch #1.

     Andrew