| Message ID | 20201020034558.19438-4-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: mv88e6xxx: serdes link without phy | expand |
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] ?
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 0xffff which is probably better than repeating the last value although it's still ambiguous because 0xffff 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.
On Tue, Oct 20, 2020 at 09:24:04PM +0000, 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 0xffff > which is probably better than repeating the last value although it's > still ambiguous because 0xffff 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 0xffff.) 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.
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_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p); int mv88e6390_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
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 <chris.packham@alliedtelesis.co.nz> --- 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(+)