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

[PATCH 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 | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index fa086e0..13db5d8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
 	return 0;
 }
 
-int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
+static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
+				    int reg)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int ret;
 
-	mutex_lock(&ps->smi_mutex);
 	ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
 	if (ret < 0)
-		goto error;
+		goto clear;
 	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
-error:
+clear:
 	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
-	mutex_unlock(&ps->smi_mutex);
+
 	return ret;
 }
 
-int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
-			     int reg, int val)
+int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
+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 error;
+		goto clear;
 
 	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
-error:
+clear:
 	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+
+	return ret;
+}
+
+int mv88e6xxx_phy_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;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
 	mutex_unlock(&ps->smi_mutex);
+
 	return ret;
 }
 
-- 
2.7.4

[PATCH 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 | 99 +++++++++++++++++++++++++++++++--------------
 drivers/net/dsa/mv88e6xxx.h |  8 ++++
 2 files changed, 76 insertions(+), 31 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 13db5d8..05b2efb 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2264,6 +2264,57 @@ 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 clear;
+
+	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
+clear:
+	_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 clear;
+
+	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
+clear:
+	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+
+	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);
@@ -2367,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
@@ -2708,21 +2776,6 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
 	return 0;
 }
 
-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 clear;
-	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
-clear:
-	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
-
-	return ret;
-}
-
 int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -2735,22 +2788,6 @@ int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
 	return ret;
 }
 
-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 clear;
-
-	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
-clear:
-	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
-
-	return ret;
-}
-
 int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val)
 {
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 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup

[Vivien Didelot]

Patrick Uiterwijk <patrick@puiterwijk.org> writes:

> 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 | 99 +++++++++++++++++++++++++++++++--------------
>  drivers/net/dsa/mv88e6xxx.h |  8 ++++
>  2 files changed, 76 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 13db5d8..05b2efb 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -2264,6 +2264,57 @@ 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 clear;
> +
> +	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
> +clear:
> +	_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 clear;
> +
> +	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
> +clear:
> +	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
> +
> +	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);
> @@ -2367,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
> @@ -2708,21 +2776,6 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
>  	return 0;
>  }
>  
> -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 clear;
> -	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
> -clear:
> -	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
> -
> -	return ret;
> -}
> -
>  int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
>  {
>  	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> @@ -2735,22 +2788,6 @@ int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
>  	return ret;
>  }
>  
> -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 clear;
> -
> -	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
> -clear:
> -	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
> -
> -	return ret;
> -}
> -
>  int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
>  			     int reg, int val)
>  {

Please introduce directly _mv88e6xxx_phy_page_{read,write} above
mv88e6xxx_setup_port in patch 1/2, where they will actually be
needed. That will avoid to move them in patch 2/2.

Thanks,
Vivien

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

[Guenter Roeck]

On 03/25/2016 05:10 PM, Patrick Uiterwijk wrote:
> 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 | 42 ++++++++++++++++++++++++++++++++----------
>   1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index fa086e0..13db5d8 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
>   	return 0;
>   }
>
> -int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
> +static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
> +				    int reg)
>   {
> -	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>   	int ret;
>
> -	mutex_lock(&ps->smi_mutex);
>   	ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
>   	if (ret < 0)
> -		goto error;
> +		goto clear;
>   	ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
> -error:
> +clear:

Is there some good reason for changing the name of those labels ?

Guenter

>   	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
> -	mutex_unlock(&ps->smi_mutex);
> +
>   	return ret;
>   }
>
> -int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
> -			     int reg, int val)
> +int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
>   {
>   	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>   	int ret;
>
>   	mutex_lock(&ps->smi_mutex);
> +	ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
> +	mutex_unlock(&ps->smi_mutex);
> +
> +	return ret;
> +}
> +
> +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 error;
> +		goto clear;
>
>   	ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
> -error:
> +clear:
>   	_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
> +
> +	return ret;
> +}
> +
> +int mv88e6xxx_phy_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;
> +
> +	mutex_lock(&ps->smi_mutex);
> +	ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
>   	mutex_unlock(&ps->smi_mutex);
> +
>   	return ret;
>   }
>
>

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

[Patrick Uiterwijk]

On Sat, Mar 26, 2016 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/25/2016 05:10 PM, Patrick Uiterwijk wrote:
>>
>> 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 | 42
>> ++++++++++++++++++++++++++++++++----------
>>   1 file changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>> index fa086e0..13db5d8 100644
>> --- a/drivers/net/dsa/mv88e6xxx.c
>> +++ b/drivers/net/dsa/mv88e6xxx.c
>> @@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds,
>> bool ppu_active)
>>         return 0;
>>   }
>>
>> -int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>> int reg)
>> +static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int
>> page,
>> +                                   int reg)
>>   {
>> -       struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>         int ret;
>>
>> -       mutex_lock(&ps->smi_mutex);
>>         ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
>>         if (ret < 0)
>> -               goto error;
>> +               goto clear;
>>         ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
>> -error:
>> +clear:
>
>
> Is there some good reason for changing the name of those labels ?

Vivien suggested to rename this since it makes more clear that this write is
meant to return to page 0 to make sure that phylib doesn't get confused
about the currently active page.

Patrick

>
> Guenter
>
>
>>         _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>> -       mutex_unlock(&ps->smi_mutex);
>> +
>>         return ret;
>>   }
>>
>> -int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
>> -                            int reg, int val)
>> +int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>> int reg)
>>   {
>>         struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>         int ret;
>>
>>         mutex_lock(&ps->smi_mutex);
>> +       ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
>> +       mutex_unlock(&ps->smi_mutex);
>> +
>> +       return ret;
>> +}
>> +
>> +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 error;
>> +               goto clear;
>>
>>         ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
>> -error:
>> +clear:
>>         _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>> +
>> +       return ret;
>> +}
>> +
>> +int mv88e6xxx_phy_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;
>> +
>> +       mutex_lock(&ps->smi_mutex);
>> +       ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
>>         mutex_unlock(&ps->smi_mutex);
>> +
>>         return ret;
>>   }
>>
>>
>

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

[Guenter Roeck]

On 03/25/2016 06:58 PM, Patrick Uiterwijk wrote:
> On Sat, Mar 26, 2016 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 03/25/2016 05:10 PM, Patrick Uiterwijk wrote:
>>>
>>> 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 | 42
>>> ++++++++++++++++++++++++++++++++----------
>>>    1 file changed, 32 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>>> index fa086e0..13db5d8 100644
>>> --- a/drivers/net/dsa/mv88e6xxx.c
>>> +++ b/drivers/net/dsa/mv88e6xxx.c
>>> @@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds,
>>> bool ppu_active)
>>>          return 0;
>>>    }
>>>
>>> -int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>>> int reg)
>>> +static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int
>>> page,
>>> +                                   int reg)
>>>    {
>>> -       struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>>          int ret;
>>>
>>> -       mutex_lock(&ps->smi_mutex);
>>>          ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
>>>          if (ret < 0)
>>> -               goto error;
>>> +               goto clear;
>>>          ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
>>> -error:
>>> +clear:
>>
>>
>> Is there some good reason for changing the name of those labels ?
>
> Vivien suggested to rename this since it makes more clear that this write is
> meant to return to page 0 to make sure that phylib doesn't get confused
> about the currently active page.
>

And "clear:" accomplishes that ? I would not have guessed.
Wonder if anyone else does. I would have used a comment.
	/* Try to return to page 0 even after an error */
or something like that.

Guenter

> Patrick
>
>>
>> Guenter
>>
>>
>>>          _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>>> -       mutex_unlock(&ps->smi_mutex);
>>> +
>>>          return ret;
>>>    }
>>>
>>> -int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
>>> -                            int reg, int val)
>>> +int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>>> int reg)
>>>    {
>>>          struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>>          int ret;
>>>
>>>          mutex_lock(&ps->smi_mutex);
>>> +       ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
>>> +       mutex_unlock(&ps->smi_mutex);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +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 error;
>>> +               goto clear;
>>>
>>>          ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
>>> -error:
>>> +clear:
>>>          _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +int mv88e6xxx_phy_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;
>>> +
>>> +       mutex_lock(&ps->smi_mutex);
>>> +       ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
>>>          mutex_unlock(&ps->smi_mutex);
>>> +
>>>          return ret;
>>>    }
>>>
>>>
>>
>

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

[Vivien Didelot]

Hi Guenter,

Guenter Roeck <linux@roeck-us.net> writes:

>>> Is there some good reason for changing the name of those labels ?
>>
>> Vivien suggested to rename this since it makes more clear that this write is
>> meant to return to page 0 to make sure that phylib doesn't get confused
>> about the currently active page.
>>
>
> And "clear:" accomplishes that ? I would not have guessed.
> Wonder if anyone else does. I would have used a comment.
> 	/* Try to return to page 0 even after an error */
> or something like that.

"error" definitely doesn't make sense, especially in case of success. If
one has a better suggestion that "clear" for the label, I don't really
mind.

Thanks,
Vivien

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

[Andrew Lunn]

> "error" definitely doesn't make sense, especially in case of success. If
> one has a better suggestion that "clear" for the label, I don't really
> mind.

Hi Vivien, Guenter

page_0 ?

      Andrew

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

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> "error" definitely doesn't make sense, especially in case of success. If
>> one has a better suggestion that "clear" for the label, I don't really
>> mind.
>
> Hi Vivien, Guenter
>
> page_0 ?

For some unjustified reasons I prefered single word labels, but multiple
words will be cleared indeed.

page_0 is fine. What about an even more explicit "clear_page_0" label?

Thanks,
Vivien

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

[Andrew Lunn]

> page_0 is fine. What about an even more explicit "clear_page_0" label?

We are not clearing anything. We are changing to page 0. So

restore_page_0?

	Andrew

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

[Vivien Didelot]

Andrew Lunn <andrew@lunn.ch> writes:

>> page_0 is fine. What about an even more explicit "clear_page_0" label?
>
> We are not clearing anything. We are changing to page 0. So
>
> restore_page_0?

+1

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

[Guenter Roeck]

On 03/26/2016 11:32 AM, Vivien Didelot wrote:
> Hi Guenter,
>
> Guenter Roeck <linux@roeck-us.net> writes:
>
>>>> Is there some good reason for changing the name of those labels ?
>>>
>>> Vivien suggested to rename this since it makes more clear that this write is
>>> meant to return to page 0 to make sure that phylib doesn't get confused
>>> about the currently active page.
>>>
>>
>> And "clear:" accomplishes that ? I would not have guessed.
>> Wonder if anyone else does. I would have used a comment.
>> 	/* Try to return to page 0 even after an error */
>> or something like that.
>
> "error" definitely doesn't make sense, especially in case of success. If
> one has a better suggestion that "clear" for the label, I don't really
> mind.

Sounds like POV to me. I don't like changing label names, because someone
else may come the next day and change it again. At the end, one ends up
in a label name war. It also makes patches look more complicated than
necessary, and it _is_ an unrelated change. I don't understand the
problem with adding a comment, and using a label name in place of a
comment seems odd to me.

Anyway, this has all become philosophical, meaning I'll stay out of it.
Pick whatever you want ...

Cheers,
Guenter

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

[Vivien Didelot]

Hi Guenter,

Guenter Roeck <linux@roeck-us.net> writes:

>>>>> Is there some good reason for changing the name of those labels ?
>>>>
>>>> Vivien suggested to rename this since it makes more clear that this write is
>>>> meant to return to page 0 to make sure that phylib doesn't get confused
>>>> about the currently active page.
>>>>
>>>
>>> And "clear:" accomplishes that ? I would not have guessed.
>>> Wonder if anyone else does. I would have used a comment.
>>> 	/* Try to return to page 0 even after an error */
>>> or something like that.
>>
>> "error" definitely doesn't make sense, especially in case of success. If
>> one has a better suggestion that "clear" for the label, I don't really
>> mind.
>
> Sounds like POV to me. I don't like changing label names, because someone
> else may come the next day and change it again. At the end, one ends up
> in a label name war. It also makes patches look more complicated than
> necessary, and it _is_ an unrelated change. I don't understand the
> problem with adding a comment, and using a label name in place of a
> comment seems odd to me.
>
> Anyway, this has all become philosophical, meaning I'll stay out of it.
> Pick whatever you want ...

Sorry I don't fully agree. There is no war or philosophical
concerns. "error" is just not correct here, this is not only an error
path. Why should we end up with a wrongly named label plus a comment,
when we can have a self documented function?

But I do agree that this change is not related. As the patch is moving
the core of these functions, it was just a good opportunity to rename
the label. I would understand and won't mind a very first 1/3 patch only
renaming the label. That might be a bit overkill however...

Thanks,
Vivien