Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1513061 > unrolled thread

[PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-11-01 04:30 +0100
    Re: [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter Andrew Lunn <andrew@lunn.ch> - 2016-11-01 10:50 +0100
      Re: [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-11-02 02:10 +0100
        Re: [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter Andrew Lunn <andrew@lunn.ch> - 2016-11-02 10:20 +0100
          Re: [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-11-02 23:40 +0100
            Re: [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter Andrew Lunn <andrew@lunn.ch> - 2016-11-03 00:10 +0100

#1513061 — [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter

Most of the chips will have a port register control bits to force the
port's link up, down, or let normal link detection occurs.

Implement such operation to use it later when setting duplex, etc.

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

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 181d3b9..cc43e6f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3160,42 +3160,49 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
 	.set_switch_mac = mv88e6xxx_g1_set_switch_mac,
 	.phy_read = mv88e6xxx_phy_ppu_read,
 	.phy_write = mv88e6xxx_phy_ppu_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6095_ops = {
 	.set_switch_mac = mv88e6xxx_g1_set_switch_mac,
 	.phy_read = mv88e6xxx_phy_ppu_read,
 	.phy_write = mv88e6xxx_phy_ppu_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6123_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_read,
 	.phy_write = mv88e6xxx_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6131_ops = {
 	.set_switch_mac = mv88e6xxx_g1_set_switch_mac,
 	.phy_read = mv88e6xxx_phy_ppu_read,
 	.phy_write = mv88e6xxx_phy_ppu_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6161_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_read,
 	.phy_write = mv88e6xxx_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6165_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_read,
 	.phy_write = mv88e6xxx_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6171_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6172_ops = {
@@ -3204,12 +3211,14 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6175_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6176_ops = {
@@ -3218,12 +3227,14 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6185_ops = {
 	.set_switch_mac = mv88e6xxx_g1_set_switch_mac,
 	.phy_read = mv88e6xxx_phy_ppu_read,
 	.phy_write = mv88e6xxx_phy_ppu_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6240_ops = {
@@ -3232,6 +3243,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6320_ops = {
@@ -3240,6 +3252,7 @@ static const struct mv88e6xxx_ops mv88e6320_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6321_ops = {
@@ -3248,18 +3261,21 @@ static const struct mv88e6xxx_ops mv88e6321_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6350_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6351_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_ops mv88e6352_ops = {
@@ -3268,6 +3284,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
 	.phy_read = mv88e6xxx_g2_smi_phy_read,
 	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
 };
 
 static const struct mv88e6xxx_info mv88e6xxx_table[] = {
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 6f48e58..f55b829 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -727,6 +727,14 @@ struct mv88e6xxx_ops {
 			u16 *val);
 	int (*phy_write)(struct mv88e6xxx_chip *chip, int addr, int reg,
 			 u16 val);
+
+#define LINK_UNKNOWN	-1
+
+	/* Port's MAC link state
+	 * LINK_UNKNOWN for normal link detection, 0 to force link down,
+	 * otherwise force link up.
+	 */
+	int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
 };
 
 enum stat_type {
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 53d17e6..3b6c7c0 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -30,6 +30,39 @@ int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,
 	return mv88e6xxx_write(chip, addr, reg, val);
 }
 
+/* Offset 0x01: MAC (or PCS or Physical) Control Register
+ *
+ * Link, Duplex and Flow Control have one force bit, one value bit.
+ */
+
+int mv88e6xxx_port_set_link(struct mv88e6xxx_chip *chip, int port, int link)
+{
+	u16 reg;
+	int err;
+
+	err = mv88e6xxx_port_read(chip, port, PORT_PCS_CTRL, &reg);
+	if (err)
+		return err;
+
+	reg &= ~(PORT_PCS_CTRL_FORCE_LINK | PORT_PCS_CTRL_LINK_UP);
+
+	if (link != LINK_UNKNOWN) {
+		reg |= PORT_PCS_CTRL_FORCE_LINK;
+		if (link)
+			reg |= PORT_PCS_CTRL_LINK_UP;
+	}
+
+	err = mv88e6xxx_port_write(chip, port, PORT_PCS_CTRL, reg);
+	if (err)
+		return err;
+
+	netdev_dbg(chip->ds->ports[port].netdev, "%s link %s\n",
+		   reg & PORT_PCS_CTRL_FORCE_LINK ? "Force" : "Unforce",
+		   reg & PORT_PCS_CTRL_LINK_UP ? "up" : "down");
+
+	return 0;
+}
+
 /* Offset 0x04: Port Control Register */
 
 static const char * const mv88e6xxx_port_state_names[] = {
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 921eecf..87e7249 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -21,6 +21,8 @@ int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg,
 int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,
 			 u16 val);
 
+int mv88e6xxx_port_set_link(struct mv88e6xxx_chip *chip, int port, int link);
+
 int mv88e6xxx_port_set_state(struct mv88e6xxx_chip *chip, int port, u8 state);
 
 int mv88e6xxx_port_set_vlan_map(struct mv88e6xxx_chip *chip, int port, u16 map);
-- 
2.10.2

[toc] | [next] | [standalone]

#1513184

> +#define LINK_UNKNOWN	-1
> +
> +	/* Port's MAC link state
> +	 * LINK_UNKNOWN for normal link detection, 0 to force link down,
> +	 * otherwise force link up.
> +	 */
> +	int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);

Hi Vivien

Maybe LINK_AUTO would be better than UNKNOWN? Or LINK_UNFORCED.

      Andrew

[toc] | [prev] | [next] | [standalone]

#1513600

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> +#define LINK_UNKNOWN	-1
>> +
>> +	/* Port's MAC link state
>> +	 * LINK_UNKNOWN for normal link detection, 0 to force link down,
>> +	 * otherwise force link up.
>> +	 */
>> +	int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
>
> Maybe LINK_AUTO would be better than UNKNOWN? Or LINK_UNFORCED.

I used LINK_UNKNOWN to be consistent with the supported SPEED_UNKNOWN
and DUPLEX_UNKNOWN values of PHY devices.

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1513770

On Wed, Nov 02, 2016 at 02:07:09AM +0100, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> +#define LINK_UNKNOWN	-1
> >> +
> >> +	/* Port's MAC link state
> >> +	 * LINK_UNKNOWN for normal link detection, 0 to force link down,
> >> +	 * otherwise force link up.
> >> +	 */
> >> +	int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
> >
> > Maybe LINK_AUTO would be better than UNKNOWN? Or LINK_UNFORCED.
> 
> I used LINK_UNKNOWN to be consistent with the supported SPEED_UNKNOWN
> and DUPLEX_UNKNOWN values of PHY devices.

Hi Vivien

These are i think for reporting back to user space what duplex or link
is currently being used. But here you are setting, not
reporting. Setting something to an unknown state is rather odd, and in
fact, it is not unknown, it is unforced.

      Andrew

[toc] | [prev] | [next] | [standalone]

#1514192

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Wed, Nov 02, 2016 at 02:07:09AM +0100, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> >> +#define LINK_UNKNOWN	-1
>> >> +
>> >> +	/* Port's MAC link state
>> >> +	 * LINK_UNKNOWN for normal link detection, 0 to force link down,
>> >> +	 * otherwise force link up.
>> >> +	 */
>> >> +	int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
>> >
>> > Maybe LINK_AUTO would be better than UNKNOWN? Or LINK_UNFORCED.
>> 
>> I used LINK_UNKNOWN to be consistent with the supported SPEED_UNKNOWN
>> and DUPLEX_UNKNOWN values of PHY devices.
>
> These are i think for reporting back to user space what duplex or link
> is currently being used. But here you are setting, not
> reporting. Setting something to an unknown state is rather odd, and in
> fact, it is not unknown, it is unforced.

Do you expect to return an error if adjust_link is called with
phydev->duplex == DUPLEX_UNKNOWN, or, do you expect to fallback to
unforced duplex when setting such value?

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1514200

> Do you expect to return an error if adjust_link is called with
> phydev->duplex == DUPLEX_UNKNOWN, or, do you expect to fallback to
> unforced duplex when setting such value?

ethtool(1) itself does not allow you to specify "unknown". It only
allows "full" or "half". So passing DUPLEX_UNKNOWN means using the API
directly. The core ethtool code does not sanity check the request, so
will pass on DUPLEX_UNKNOWN to the drivers.

A quick search of the drivers, 99% seem to ignore DUPLEX_UNKNOWN. The
1% is bnx2x, which has:

        /* If received a request for an unknown duplex, assume full*/
        if (cmd->duplex == DUPLEX_UNKNOWN)
                cmd->duplex = DUPLEX_FULL;

I personally would return -EINVAL, since it is unclear what
DUPLEX_UNKNOWN means. It could be argued that falling back to Half is
correct, since failed autoneg generally results in 10/Half. Every
Ethernet can do that, where as a device needs to be 25 years or
younger to support Full :-)

	  Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web