diff mbox series

[net-next,v3,4/6] net: dsa: mv88e6xxx: simplify SERDES code for Topaz and Peridot

Message ID 20190825035915.13112-5-marek.behun@nic.cz
State Superseded
Delegated to: David Miller
Headers show
Series net: dsa: mv88e6xxx: Peridot/Topaz SERDES changes | expand

Commit Message

Marek Behún Aug. 25, 2019, 3:59 a.m. UTC 
By adding an additional serdes_get_lane implementation (for Topaz), we
can merge the implementations of other SERDES functions (powering and
IRQs). We can skip checking port numbers, since the serdes_get_lane()
methods inform if there is no lane on a port or if the lane cannot be
used for given cmode.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/dsa/mv88e6xxx/chip.c   |  18 ++---
 drivers/net/dsa/mv88e6xxx/port.c   |   4 +-
 drivers/net/dsa/mv88e6xxx/serdes.c | 104 ++++++++---------------------
 drivers/net/dsa/mv88e6xxx/serdes.h |   7 +-
 4 files changed, 42 insertions(+), 91 deletions(-)

Comments

Vivien Didelot Aug. 25, 2019, 4:02 p.m. UTC | #1 
Hi Marek,

On Sun, 25 Aug 2019 05:59:13 +0200, Marek Behún <marek.behun@nic.cz> wrote:
> +int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, s8 *lane)
> +{
> +	u8 cmode = chip->ports[port].cmode;
> +
> +	*lane = -1;
> +
> +	if (port != 5)
> +		return 0;

Aren't you relying on -ENODEV as well?

> +
> +	if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASE_X ||
> +	    cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
> +	    cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
> +		*lane = MV88E6341_PORT5_LANE;
> +
> +	return 0;
> +}
Marek Behún Aug. 25, 2019, 4:36 p.m. UTC | #2 
On Sun, 25 Aug 2019 12:02:32 -0400
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> Aren't you relying on -ENODEV as well?

Vivien, I am not relying o -ENODEV. I changed the serdes_get_lane
semantics:
 - previously:
   - if port has a lane for current cmode, return given lane number
   - otherwise return -ENODEV
   - if other error occured during serdes_get_lane, return that error
     (this never happened, because all implementations only need port
     number and cmode, and cmode is cached, so no function was called
     that could err)
 - after this commit:
   - if port has a lane for current cmode, return 0 and put lane number
     into *lane
   - otherwise return 0 and put -1 into *lane
   - if error occured, return that error number

I removed the -ENODEV semantics for "no lane on port" event.
There are two reasons for this:
  1. once you requested lane number to be put into a place pointed to
     by a pointer, rather than the return value, the code seemed better
     to me (you may of course disagree, this is a personal opinion) when
     I did:
       if (err)
           return err;
       if (lane < 0)
           return 0;
     rather than
       if (err == -ENODEV)
           return 0;
       if (err)
           return err;
  2. some future implementation may actually need to call some MDIO
     read/write functions, which may or may not return -ENODEV. That
     could conflict with the -ENODEV returned when there is no lane.

Marek
Vivien Didelot Aug. 26, 2019, 4:08 p.m. UTC | #3 
Hi Marek,

On Sun, 25 Aug 2019 18:36:09 +0200, Marek Behun <marek.behun@nic.cz> wrote:
> > Aren't you relying on -ENODEV as well?
> 
> Vivien, I am not relying o -ENODEV. I changed the serdes_get_lane
> semantics:
>  - previously:
>    - if port has a lane for current cmode, return given lane number
>    - otherwise return -ENODEV
>    - if other error occured during serdes_get_lane, return that error
>      (this never happened, because all implementations only need port
>      number and cmode, and cmode is cached, so no function was called
>      that could err)
>  - after this commit:
>    - if port has a lane for current cmode, return 0 and put lane number
>      into *lane
>    - otherwise return 0 and put -1 into *lane
>    - if error occured, return that error number
> 
> I removed the -ENODEV semantics for "no lane on port" event.
> There are two reasons for this:
>   1. once you requested lane number to be put into a place pointed to
>      by a pointer, rather than the return value, the code seemed better
>      to me (you may of course disagree, this is a personal opinion) when
>      I did:
>        if (err)
>            return err;
>        if (lane < 0)
>            return 0;
>      rather than
>        if (err == -ENODEV)
>            return 0;
>        if (err)
>            return err;

A single return path for invalid queries, eventually checking a specific
error, is always more idiomatic and better than checking two places which
could lead in mistakes as your previous patch did. So this is more readable:

    if (err)
        return err;

or:

    if (err && err != -ENODEV)
        return err;

or:

    if (err) {
        if (err = -ENODEV)
            err = 0;
        return err;
    }

>   2. some future implementation may actually need to call some MDIO
>      read/write functions, which may or may not return -ENODEV. That
>      could conflict with the -ENODEV returned when there is no lane.

The current code is already using -ENODEV to inform about "no lane for port",
even if it can be used by lower level functions, same as -EINVAL. That is fine.

So if you have to respin the series again, I would really prefer to see an
unsigned lane parameter, otherwise, fine...


Thanks,

	Vivien
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5a3fff1971b9..202ccce65b1c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2927,7 +2927,8 @@  static const struct mv88e6xxx_ops mv88e6141_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
-	.serdes_power = mv88e6341_serdes_power,
+	.serdes_power = mv88e6390_serdes_power,
+	.serdes_get_lane = mv88e6341_serdes_get_lane,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.phylink_validate = mv88e6341_phylink_validate,
 };
@@ -3301,10 +3302,10 @@  static const struct mv88e6xxx_ops mv88e6190x_ops = {
 	.rmu_disable = mv88e6390_g1_rmu_disable,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
-	.serdes_power = mv88e6390x_serdes_power,
+	.serdes_power = mv88e6390_serdes_power,
 	.serdes_get_lane = mv88e6390x_serdes_get_lane,
-	.serdes_irq_setup = mv88e6390x_serdes_irq_setup,
-	.serdes_irq_free = mv88e6390x_serdes_irq_free,
+	.serdes_irq_setup = mv88e6390_serdes_irq_setup,
+	.serdes_irq_free = mv88e6390_serdes_irq_free,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.phylink_validate = mv88e6390x_phylink_validate,
 };
@@ -3621,7 +3622,8 @@  static const struct mv88e6xxx_ops mv88e6341_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
-	.serdes_power = mv88e6341_serdes_power,
+	.serdes_power = mv88e6390_serdes_power,
+	.serdes_get_lane = mv88e6341_serdes_get_lane,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6390_avb_ops,
 	.ptp_ops = &mv88e6352_ptp_ops,
@@ -3854,10 +3856,10 @@  static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.rmu_disable = mv88e6390_g1_rmu_disable,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
-	.serdes_power = mv88e6390x_serdes_power,
+	.serdes_power = mv88e6390_serdes_power,
 	.serdes_get_lane = mv88e6390x_serdes_get_lane,
-	.serdes_irq_setup = mv88e6390x_serdes_irq_setup,
-	.serdes_irq_free = mv88e6390x_serdes_irq_free,
+	.serdes_irq_setup = mv88e6390_serdes_irq_setup,
+	.serdes_irq_free = mv88e6390_serdes_irq_free,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6390_avb_ops,
 	.ptp_ops = &mv88e6352_ptp_ops,
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 6a1fa5c72fdb..d20be5327640 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -445,7 +445,7 @@  int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 				return err;
 		}
 
-		err = mv88e6390x_serdes_power(chip, port, false);
+		err = mv88e6390_serdes_power(chip, port, false);
 		if (err)
 			return err;
 	}
@@ -470,7 +470,7 @@  int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		if (err)
 			return err;
 
-		err = mv88e6390x_serdes_power(chip, port, true);
+		err = mv88e6390_serdes_power(chip, port, true);
 		if (err)
 			return err;
 
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index a9209465e06b..1946ccb6694c 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -286,6 +286,23 @@  void mv88e6352_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
 	chip->ports[port].serdes_irq = 0;
 }
 
+int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, s8 *lane)
+{
+	u8 cmode = chip->ports[port].cmode;
+
+	*lane = -1;
+
+	if (port != 5)
+		return 0;
+
+	if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASE_X ||
+	    cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
+	    cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+		*lane = MV88E6341_PORT5_LANE;
+
+	return 0;
+}
+
 int mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, s8 *lane)
 {
 	u8 cmode = chip->ports[port].cmode;
@@ -445,26 +462,9 @@  static int mv88e6390_serdes_power_sgmii(struct mv88e6xxx_chip *chip, s8 lane,
 	return err;
 }
 
-static int mv88e6390_serdes_power_lane(struct mv88e6xxx_chip *chip, int port,
-				       s8 lane, bool on)
-{
-	u8 cmode = chip->ports[port].cmode;
-
-	switch (cmode) {
-	case MV88E6XXX_PORT_STS_CMODE_SGMII:
-	case MV88E6XXX_PORT_STS_CMODE_1000BASE_X:
-	case MV88E6XXX_PORT_STS_CMODE_2500BASEX:
-		return mv88e6390_serdes_power_sgmii(chip, lane, on);
-	case MV88E6XXX_PORT_STS_CMODE_XAUI:
-	case MV88E6XXX_PORT_STS_CMODE_RXAUI:
-		return mv88e6390_serdes_power_10g(chip, lane, on);
-	}
-
-	return 0;
-}
-
 int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
 {
+	u8 cmode = chip->ports[port].cmode;
 	s8 lane;
 	int err;
 
@@ -474,30 +474,14 @@  int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
 	if (lane < 0)
 		return 0;
 
-	switch (port) {
-	case 9 ... 10:
-		return mv88e6390_serdes_power_lane(chip, port, lane, on);
-	}
-
-	return 0;
-}
-
-int mv88e6390x_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
-{
-	s8 lane;
-	int err;
-
-	err = mv88e6xxx_serdes_get_lane(chip, port, &lane);
-	if (err)
-		return err;
-	if (lane < 0)
-		return 0;
-
-	switch (port) {
-	case 2 ... 4:
-	case 5 ... 7:
-	case 9 ... 10:
-		return mv88e6390_serdes_power_lane(chip, port, lane, on);
+	switch (cmode) {
+	case MV88E6XXX_PORT_STS_CMODE_SGMII:
+	case MV88E6XXX_PORT_STS_CMODE_1000BASE_X:
+	case MV88E6XXX_PORT_STS_CMODE_2500BASEX:
+		return mv88e6390_serdes_power_sgmii(chip, lane, on);
+	case MV88E6XXX_PORT_STS_CMODE_XAUI:
+	case MV88E6XXX_PORT_STS_CMODE_RXAUI:
+		return mv88e6390_serdes_power_10g(chip, lane, on);
 	}
 
 	return 0;
@@ -662,7 +646,7 @@  static irqreturn_t mv88e6390_serdes_thread_fn(int irq, void *dev_id)
 	return ret;
 }
 
-int mv88e6390x_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
+int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
 {
 	int err;
 	s8 lane;
@@ -700,15 +684,7 @@  int mv88e6390x_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
 	return mv88e6390_serdes_irq_enable(chip, port, lane);
 }
 
-int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port)
-{
-	if (port < 9)
-		return 0;
-
-	return mv88e6390x_serdes_irq_setup(chip, port);
-}
-
-void mv88e6390x_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
+void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
 {
 	int err;
 	s8 lane;
@@ -732,27 +708,3 @@  void mv88e6390x_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
 
 	chip->ports[port].serdes_irq = 0;
 }
-
-void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port)
-{
-	if (port < 9)
-		return;
-
-	mv88e6390x_serdes_irq_free(chip, port);
-}
-
-int mv88e6341_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
-{
-	u8 cmode = chip->ports[port].cmode;
-
-	if (port != 5)
-		return 0;
-
-	if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASE_X ||
-	    cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
-	    cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
-		return mv88e6390_serdes_power_sgmii(chip, MV88E6341_ADDR_SERDES,
-						    on);
-
-	return 0;
-}
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 1ddb8fb3aab9..7fded708b05d 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -28,7 +28,7 @@ 
 #define MV88E6352_SERDES_INT_STATUS	0x13
 
 
-#define MV88E6341_ADDR_SERDES		0x15
+#define MV88E6341_PORT5_LANE		0x15
 
 #define MV88E6390_PORT9_LANE0		0x09
 #define MV88E6390_PORT9_LANE1		0x12
@@ -87,16 +87,13 @@  static inline int mv88e6xxx_serdes_get_lane(struct mv88e6xxx_chip *chip,
 	return chip->info->ops->serdes_get_lane(chip, port, lane);
 }
 
+int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, s8 *lane);
 int mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, s8 *lane);
 int mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port, s8 *lane);
-int mv88e6341_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
 int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
 int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
-int mv88e6390x_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
 int mv88e6390_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
 void mv88e6390_serdes_irq_free(struct mv88e6xxx_chip *chip, int port);
-int mv88e6390x_serdes_irq_setup(struct mv88e6xxx_chip *chip, int port);
-void mv88e6390x_serdes_irq_free(struct mv88e6xxx_chip *chip, int port);
 int mv88e6352_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port);
 int mv88e6352_serdes_get_strings(struct mv88e6xxx_chip *chip,
 				 int port, uint8_t *data);