Re: [PATCH v3 3/3] net: dsa: mv88e6xxx: Support serdes ports on MV88E6123/6131

[Russell King - ARM Linux admin]

On Tue, Oct 20, 2020 at 09:24:04PM +, Chris Packham wrote:
> 
> On 20/10/20 11:18 pm, Russell King - ARM Linux admin wrote:
> > On Tue, Oct 20, 2020 at 04:45:58PM +1300, Chris Packham wrote:
> >> +void mv88e6123_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, 
> >> void *_p)
> >> +{
> >> +  u16 *p = _p;
> >> +  u16 reg;
> >> +  int i;
> >> +
> >> +  if (mv88e6xxx_serdes_get_lane(chip, port) == 0)
> >> +  return;
> >> +
> >> +  for (i = 0; i < 26; i++) {
> >> +  mv88e6xxx_phy_read(chip, port, i, ®);
> > Shouldn't this deal with a failed read in some way, rather than just
> > assigning the last or possibly uninitialised value to p[i] ?
> 
> mv88e6390_serdes_get_regs() and mv88e6352_serdes_get_regs() also ignore 
> the error. The generic mv88e6xxx_get_regs() memsets p[] to 0xff so if 
> the serdes_get_regs functions just left it alone we'd return 0x 
> which is probably better than repeating the last value although it's 
> still ambiguous because 0x is a valid value for plenty of these 
> registers.
> 
> Since it looks like I need to come up with an alternative to patch #1 
> I'll concentrate on that but making the serdes_get_regs() a little more 
> error tolerant is a cleanup I can easily tack on onto this series.

Yep, it looks like they all suffer the same problem. Interestingly,
mv88e6xxx_get_regs() does handle the error by avoiding writing the
register entry (so it gets left as 0x.)

Incidentally, that's also the value you'll get when reading from a
PHY that doesn't respond, since the MDIO data line is pulled high
when undriven.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 3/3] net: dsa: mv88e6xxx: Support serdes ports on MV88E6123/6131

[Chris Packham]

On 20/10/20 11:18 pm, Russell King - ARM Linux admin wrote:
> On Tue, Oct 20, 2020 at 04:45:58PM +1300, Chris Packham wrote:
>> +void mv88e6123_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void 
>> *_p)
>> +{
>> +u16 *p = _p;
>> +u16 reg;
>> +int i;
>> +
>> +if (mv88e6xxx_serdes_get_lane(chip, port) == 0)
>> +return;
>> +
>> +for (i = 0; i < 26; i++) {
>> +mv88e6xxx_phy_read(chip, port, i, ®);
> Shouldn't this deal with a failed read in some way, rather than just
> assigning the last or possibly uninitialised value to p[i] ?

mv88e6390_serdes_get_regs() and mv88e6352_serdes_get_regs() also ignore 
the error. The generic mv88e6xxx_get_regs() memsets p[] to 0xff so if 
the serdes_get_regs functions just left it alone we'd return 0x 
which is probably better than repeating the last value although it's 
still ambiguous because 0x is a valid value for plenty of these 
registers.

Since it looks like I need to come up with an alternative to patch #1 
I'll concentrate on that but making the serdes_get_regs() a little more 
error tolerant is a cleanup I can easily tack on onto this series.

Re: [PATCH v3 3/3] net: dsa: mv88e6xxx: Support serdes ports on MV88E6123/6131

[Russell King - ARM Linux admin]

On Tue, Oct 20, 2020 at 04:45:58PM +1300, Chris Packham wrote:
> +void mv88e6123_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void 
> *_p)
> +{
> + u16 *p = _p;
> + u16 reg;
> + int i;
> +
> + if (mv88e6xxx_serdes_get_lane(chip, port) == 0)
> + return;
> +
> + for (i = 0; i < 26; i++) {
> + mv88e6xxx_phy_read(chip, port, i, ®);

Shouldn't this deal with a failed read in some way, rather than just
assigning the last or possibly uninitialised value to p[i] ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v3 3/3] net: dsa: mv88e6xxx: Support serdes ports on MV88E6123/6131

[Chris Packham]

Implement serdes_power, serdes_get_lane and serdes_pcs_get_state ops for
the MV88E6123 so that the ports without a built-in PHY supported as
serdes ports and directly connected to other network interfaces or to
SFPs. Also implement serdes_get_regs_len and serdes_get_regs to aid
future debugging.

Signed-off-by: Chris Packham 
---

This is untested (apart from compilation) it assumes the SERDES "phy"
address corresponds to the port number but I'm not confident that is a
valid assumption.

Changes in v3:
- None
Changes in v2:
- new

 drivers/net/dsa/mv88e6xxx/chip.c   | 10 +++
 drivers/net/dsa/mv88e6xxx/serdes.c | 44 ++
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 +++
 3 files changed, 58 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 62d4d7b5d9ac..5344fc84b03e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3574,6 +3574,11 @@ static const struct mv88e6xxx_ops mv88e6123_ops = {
.set_egress_port = mv88e6095_g1_set_egress_port,
.watchdog_ops = &mv88e6097_watchdog_ops,
.mgmt_rsvd2cpu = mv88e6352_g2_mgmt_rsvd2cpu,
+   .serdes_power = mv88e6123_serdes_power,
+   .serdes_get_lane = mv88e6185_serdes_get_lane,
+   .serdes_pcs_get_state = mv88e6185_serdes_pcs_get_state,
+   .serdes_get_regs_len = mv88e6123_serdes_get_regs_len,
+   .serdes_get_regs = mv88e6123_serdes_get_regs,
.pot_clear = mv88e6xxx_g2_pot_clear,
.reset = mv88e6352_g1_reset,
.atu_get_hash = mv88e6165_g1_atu_get_hash,
@@ -3613,6 +3618,11 @@ static const struct mv88e6xxx_ops mv88e6131_ops = {
.set_egress_port = mv88e6095_g1_set_egress_port,
.watchdog_ops = &mv88e6097_watchdog_ops,
.mgmt_rsvd2cpu = mv88e6185_g2_mgmt_rsvd2cpu,
+   .serdes_power = mv88e6123_serdes_power,
+   .serdes_get_lane = mv88e6185_serdes_get_lane,
+   .serdes_pcs_get_state = mv88e6185_serdes_pcs_get_state,
+   .serdes_get_regs_len = mv88e6123_serdes_get_regs_len,
+   .serdes_get_regs = mv88e6123_serdes_get_regs,
.ppu_enable = mv88e6185_g1_ppu_enable,
.set_cascade_port = mv88e6185_g1_set_cascade_port,
.ppu_disable = mv88e6185_g1_ppu_disable,
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c 
b/drivers/net/dsa/mv88e6xxx/serdes.c
index d4f40a739b17..eb89debbf576 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -428,6 +428,50 @@ u8 mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, 
int port)
return lane;
 }
 
+int mv88e6123_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
+  bool up)
+{
+   u16 val, new_val;
+   int err;
+
+   err = mv88e6xxx_phy_read(chip, port, MII_BMCR, &val);
+   if (err)
+   return err;
+
+   if (up)
+   new_val = val & ~BMCR_PDOWN;
+   else
+   new_val = val | BMCR_PDOWN;
+
+   if (val != new_val)
+   err = mv88e6xxx_phy_write(chip, port, MII_BMCR, val);
+
+   return err;
+}
+
+int mv88e6123_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port)
+{
+   if (mv88e6xxx_serdes_get_lane(chip, port) == 0)
+   return 0;
+
+   return 26 * sizeof(u16);
+}
+
+void mv88e6123_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
+{
+   u16 *p = _p;
+   u16 reg;
+   int i;
+
+   if (mv88e6xxx_serdes_get_lane(chip, port) == 0)
+   return;
+
+   for (i = 0; i < 26; i++) {
+   mv88e6xxx_phy_read(chip, port, i, ®);
+   p[i] = reg;
+   }
+}
+
 int mv88e6185_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
   bool up)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h 
b/drivers/net/dsa/mv88e6xxx/serdes.h
index c24ec4122c9e..b573139928c4 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -104,6 +104,8 @@ unsigned int mv88e6352_serdes_irq_mapping(struct 
mv88e6xxx_chip *chip,
  int port);
 unsigned int mv88e6390_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
  int port);
+int mv88e6123_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
+  bool up);
 int mv88e6185_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
   bool up);
 int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, u8 lane,
@@ -129,6 +131,8 @@ int mv88e6390_serdes_get_strings(struct mv88e6xxx_chip 
*chip,
 int mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
   uint64_t *data);
 
+int mv88e6123_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
+void mv88e6123_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void 
*_p);
 int mv88e6352_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
 void mv88e6352_ser