[v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[Holger Brunck]

The mv88e6352, mv88e6240 and mv88e6176  have a serdes interface. This patch
allows to configure the output swing to a desired value in the
phy-handle of the port. The value which is peak to peak has to be
specified in microvolts. As the chips only supports eight dedicated
values we return EINVAL if the value in the DTS does not match one of
these values.

CC: Andrew Lunn <andrew@lunn.ch>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Marek Behún <kabel@kernel.org>
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
---
v4: - adapted for new dt-binding
      https://www.spinics.net/lists/netdev/msg793918.html
 drivers/net/dsa/mv88e6xxx/chip.c   | 22 +++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h   |  4 ++++
 drivers/net/dsa/mv88e6xxx/serdes.c | 48 ++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h |  5 ++++
 4 files changed, 79 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 58ca684..2363529 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2857,7 +2857,10 @@ static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)
 
 static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 {
+	struct device_node *phy_handle = NULL;
 	struct dsa_switch *ds = chip->ds;
+	struct dsa_port *dp;
+	int tx_amp;
 	int err;
 	u16 reg;
 
@@ -3011,6 +3014,22 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 			return err;
 	}
 
+	if (chip->info->ops->serdes_set_tx_p2p_amplitude) {
+		dp = dsa_to_port(ds, port);
+		if (dp)
+			phy_handle =  of_parse_phandle(dp->dn, "phy-handle", 0);
+
+		if (phy_handle &&
+		    !of_property_read_u32(phy_handle,
+					  "tx-p2p-microvolt",
+					  &tx_amp)) {
+			err = mv88e6352_serdes_set_tx_p2p_amplitude(chip, port,
+								    tx_amp);
+			if (err)
+				return err;
+		}
+	}
+
 	/* Port based VLAN map: give each port the same default address
 	 * database, and allow bidirectional communication between the
 	 * CPU and DSA port(s), and the other ports.
@@ -4073,6 +4092,7 @@ static int mv88e6xxx_set_eeprom(struct dsa_switch *ds,
 	.serdes_irq_status = mv88e6352_serdes_irq_status,
 	.serdes_get_regs_len = mv88e6352_serdes_get_regs_len,
 	.serdes_get_regs = mv88e6352_serdes_get_regs,
+	.serdes_set_tx_p2p_amplitude = mv88e6352_serdes_set_tx_p2p_amplitude,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.phylink_validate = mv88e6352_phylink_validate,
 };
@@ -4353,6 +4373,7 @@ static int mv88e6xxx_set_eeprom(struct dsa_switch *ds,
 	.serdes_irq_status = mv88e6352_serdes_irq_status,
 	.serdes_get_regs_len = mv88e6352_serdes_get_regs_len,
 	.serdes_get_regs = mv88e6352_serdes_get_regs,
+	.serdes_set_tx_p2p_amplitude = mv88e6352_serdes_set_tx_p2p_amplitude,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6352_ptp_ops,
@@ -4759,6 +4780,7 @@ static int mv88e6xxx_set_eeprom(struct dsa_switch *ds,
 	.serdes_get_stats = mv88e6352_serdes_get_stats,
 	.serdes_get_regs_len = mv88e6352_serdes_get_regs_len,
 	.serdes_get_regs = mv88e6352_serdes_get_regs,
+	.serdes_set_tx_p2p_amplitude = mv88e6352_serdes_set_tx_p2p_amplitude,
 	.phylink_validate = mv88e6352_phylink_validate,
 };
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 8271b8a..3d56712 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -586,6 +586,10 @@ struct mv88e6xxx_ops {
 	void (*serdes_get_regs)(struct mv88e6xxx_chip *chip, int port,
 				void *_p);
 
+	/* SERDES SGMII/Fiber Output Amplitude */
+	int (*serdes_set_tx_p2p_amplitude)(struct mv88e6xxx_chip *chip,
+					   int port, int val);
+
 	/* Address Translation Unit operations */
 	int (*atu_get_hash)(struct mv88e6xxx_chip *chip, u8 *hash);
 	int (*atu_set_hash)(struct mv88e6xxx_chip *chip, u8 hash);
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 2b05ead..c09c528 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1310,6 +1310,54 @@ void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
 	}
 }
 
+struct mv88e6352_serdes_p2p_to_val {
+	int mv;
+	u16 regval;
+};
+
+static struct mv88e6352_serdes_p2p_to_val mv88e6352_serdes_p2p_to_val[] = {
+	/* Mapping of configurable mikrovolt values to the register value */
+	{ 14000, 0},
+	{ 112000, 1},
+	{ 210000, 2},
+	{ 308000, 3},
+	{ 406000, 4},
+	{ 504000, 5},
+	{ 602000, 6},
+	{ 700000, 7},
+};
+
+int mv88e6352_serdes_set_tx_p2p_amplitude(struct mv88e6xxx_chip *chip, int port,
+					  int val)
+{
+	bool found = false;
+	u16 reg;
+	int err;
+	int i;
+
+	if (!mv88e6352_port_has_serdes(chip, port))
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_p2p_to_val); ++i) {
+		if (mv88e6352_serdes_p2p_to_val[i].mv == val) {
+			reg = mv88e6352_serdes_p2p_to_val[i].regval;
+			found = true;
+			break;
+		}
+	}
+
+	if (!found)
+		return -EINVAL;
+
+	err = mv88e6352_serdes_read(chip, MV88E6352_SERDES_SPEC_CTRL2, &reg);
+	if (err)
+		return err;
+
+	reg = (reg & MV88E6352_SERDES_OUT_AMP_MASK) | val;
+
+	return mv88e6352_serdes_write(chip, MV88E6352_SERDES_SPEC_CTRL2, reg);
+}
+
 static int mv88e6393x_serdes_power_lane(struct mv88e6xxx_chip *chip, int lane,
 					bool on)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 8dd8ed2..5602d94 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -27,6 +27,8 @@
 #define MV88E6352_SERDES_INT_FIBRE_ENERGY	BIT(4)
 #define MV88E6352_SERDES_INT_STATUS	0x13
 
+#define MV88E6352_SERDES_SPEC_CTRL2	0x1a
+#define MV88E6352_SERDES_OUT_AMP_MASK		0xfffc
 
 #define MV88E6341_PORT5_LANE		0x15
 
@@ -176,6 +178,9 @@ int mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
 int mv88e6390_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
 void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p);
 
+int mv88e6352_serdes_set_tx_p2p_amplitude(struct mv88e6xxx_chip *chip, int port,
+					  int val);
+
 /* Return the (first) SERDES lane address a port is using, -errno otherwise. */
 static inline int mv88e6xxx_serdes_get_lane(struct mv88e6xxx_chip *chip,
 					    int port)
-- 
1.8.3.1

Re: [v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[Andrew Lunn]

> +static struct mv88e6352_serdes_p2p_to_val mv88e6352_serdes_p2p_to_val[] = {

Please add a const to this. It will make the memory usage a little
smaller and help protect from overwrite.

> +	/* Mapping of configurable mikrovolt values to the register value */
> +	{ 14000, 0},
> +	{ 112000, 1},
> +	{ 210000, 2},
> +	{ 308000, 3},
> +	{ 406000, 4},
> +	{ 504000, 5},
> +	{ 602000, 6},
> +	{ 700000, 7},
> +};
> +
> +int mv88e6352_serdes_set_tx_p2p_amplitude(struct mv88e6xxx_chip *chip, int port,
> +					  int val)
> +{
> +	bool found = false;
> +	u16 reg;
> +	int err;
> +	int i;
> +
> +	if (!mv88e6352_port_has_serdes(chip, port))
> +		return -EOPNOTSUPP;

Russell just reworked this call. Did you take that into account?

> +
> +	for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_p2p_to_val); ++i) {
> +		if (mv88e6352_serdes_p2p_to_val[i].mv == val) {
> +			reg = mv88e6352_serdes_p2p_to_val[i].regval;

i has the same value as mv88e6352_serdes_p2p_to_val[i].regval, so you
can drop it and just use i.

    Andrew

Re: [v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[Marek Behún]

On Tue,  8 Feb 2022 10:44:55 +0100
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> @@ -3011,6 +3014,22 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  			return err;
>  	}
>  
> +	if (chip->info->ops->serdes_set_tx_p2p_amplitude) {
> +		dp = dsa_to_port(ds, port);
> +		if (dp)
> +			phy_handle =  of_parse_phandle(dp->dn, "phy-handle", 0);

two spaces after '=' operator, only one needed

> +		if (phy_handle &&
> +		    !of_property_read_u32(phy_handle,
> +					  "tx-p2p-microvolt",
> +					  &tx_amp)) {
> +			err = mv88e6352_serdes_set_tx_p2p_amplitude(chip, port,
> +								    tx_amp);
> +			if (err)
> +				return err;
> +		}

you need to decrease reference of the phy_handle you just got with
of_parse_phandle:

  if (phy_handle)
    of_node_put(phy_handle);

> +	}
> +

> +struct mv88e6352_serdes_p2p_to_val {
> +	int mv;
> +	u16 regval;
> +};
> +
> +static struct mv88e6352_serdes_p2p_to_val mv88e6352_serdes_p2p_to_val[] = {
> +	/* Mapping of configurable mikrovolt values to the register value */
> +	{ 14000, 0},
> +	{ 112000, 1},
> +	{ 210000, 2},
> +	{ 308000, 3},
> +	{ 406000, 4},
> +	{ 504000, 5},
> +	{ 602000, 6},
> +	{ 700000, 7},

add spaces before ending '}'


Marek

Re: [v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[Marek Behún]

On Tue,  8 Feb 2022 10:44:55 +0100
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> +static struct mv88e6352_serdes_p2p_to_val mv88e6352_serdes_p2p_to_val[] = {
> +	/* Mapping of configurable mikrovolt values to the register value */
> +	{ 14000, 0},
> +	{ 112000, 1},
> +	{ 210000, 2},
> +	{ 308000, 3},
> +	{ 406000, 4},
> +	{ 504000, 5},
> +	{ 602000, 6},
> +	{ 700000, 7},
> +};

...

> +	reg = (reg & MV88E6352_SERDES_OUT_AMP_MASK) | val;

This is weird: normally in mask we have those bits set that are to be changed.
So amplitude mask should be bits that specify the amplitue, and this
should be
  reg &= ~MV88E6352_SERDES_OUT_AMP_MASK;
  reg |= val & MV88E6352_SERDES_OUT_AMP_MASK;
and mask should be defined inversely.

...

> +#define MV88E6352_SERDES_OUT_AMP_MASK		0xfffc

And this is also weird. 0xfffc is all bits set except last 2, but in
the mapping above the maximum value is 7, so you use 3 bits for
amplitude...

Marek

RE: [v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[Holger Brunck]

> > +static struct mv88e6352_serdes_p2p_to_val
> mv88e6352_serdes_p2p_to_val[] = {
> > +     /* Mapping of configurable mikrovolt values to the register value */
> > +     { 14000, 0},
> > +     { 112000, 1},
> > +     { 210000, 2},
> > +     { 308000, 3},
> > +     { 406000, 4},
> > +     { 504000, 5},
> > +     { 602000, 6},
> > +     { 700000, 7},
> > +};
> 
> ...
> 
> > +     reg = (reg & MV88E6352_SERDES_OUT_AMP_MASK) | val;
> 
> This is weird: normally in mask we have those bits set that are to be changed.
> So amplitude mask should be bits that specify the amplitue, and this should be
>   reg &= ~MV88E6352_SERDES_OUT_AMP_MASK;
>   reg |= val & MV88E6352_SERDES_OUT_AMP_MASK; and mask should be
> defined inversely.
> 

ok I will change that.

> ...
> 
> > +#define MV88E6352_SERDES_OUT_AMP_MASK                0xfffc
> 
> And this is also weird. 0xfffc is all bits set except last 2, but in the mapping above
> the maximum value is 7, so you use 3 bits for amplitude...
>

you are absolutely right. In this case it would be 0xfff8 to handle all three bits.

Thanks
Holger

RE: [v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[Holger Brunck]

> > @@ -3011,6 +3014,22 @@ static int mv88e6xxx_setup_port(struct
> mv88e6xxx_chip *chip, int port)
> >                       return err;
> >       }
> >
> > +     if (chip->info->ops->serdes_set_tx_p2p_amplitude) {
> > +             dp = dsa_to_port(ds, port);
> > +             if (dp)
> > +                     phy_handle =  of_parse_phandle(dp->dn, "phy-handle", 0);
> 
> two spaces after '=' operator, only one needed
> 

ok

> > +             if (phy_handle &&
> > +                 !of_property_read_u32(phy_handle,
> > +                                       "tx-p2p-microvolt",
> > +                                       &tx_amp)) {
> > +                     err = mv88e6352_serdes_set_tx_p2p_amplitude(chip, port,
> > +                                                                 tx_amp);
> > +                     if (err)
> > +                             return err;
> > +             }
> 
> you need to decrease reference of the phy_handle you just got with
> of_parse_phandle:
> 
>   if (phy_handle)
>     of_node_put(phy_handle);
> 

ok

> > +     }
> > +
> 
> > +struct mv88e6352_serdes_p2p_to_val {
> > +     int mv;
> > +     u16 regval;
> > +};
> > +
> > +static struct mv88e6352_serdes_p2p_to_val
> mv88e6352_serdes_p2p_to_val[] = {
> > +     /* Mapping of configurable mikrovolt values to the register value */
> > +     { 14000, 0},
> > +     { 112000, 1},
> > +     { 210000, 2},
> > +     { 308000, 3},
> > +     { 406000, 4},
> > +     { 504000, 5},
> > +     { 602000, 6},
> > +     { 700000, 7},
> 
> add spaces before ending '}'
> 

hm, why didn't checkpatch complained?  Anyhow I need to do changes here anyhow.

Thanks
Holger

RE: [v4] dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude configurable

[Holger Brunck]

> 
> > +static struct mv88e6352_serdes_p2p_to_val
> > +mv88e6352_serdes_p2p_to_val[] = {
> 
> Please add a const to this. It will make the memory usage a little smaller and
> help protect from overwrite.
>

ok
 
> > +     /* Mapping of configurable mikrovolt values to the register value */
> > +     { 14000, 0},
> > +     { 112000, 1},
> > +     { 210000, 2},
> > +     { 308000, 3},
> > +     { 406000, 4},
> > +     { 504000, 5},
> > +     { 602000, 6},
> > +     { 700000, 7},
> > +};
> > +
> > +int mv88e6352_serdes_set_tx_p2p_amplitude(struct mv88e6xxx_chip *chip,
> int port,
> > +                                       int val) {
> > +     bool found = false;
> > +     u16 reg;
> > +     int err;
> > +     int i;
> > +
> > +     if (!mv88e6352_port_has_serdes(chip, port))
> > +             return -EOPNOTSUPP;
> 
> Russell just reworked this call. Did you take that into account?
> 

no I was not aware of this. Thanks for pointing out. I will base my
changes then on his patch series.

> > +
> > +     for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_p2p_to_val); ++i) {
> > +             if (mv88e6352_serdes_p2p_to_val[i].mv == val) {
> > +                     reg = mv88e6352_serdes_p2p_to_val[i].regval;
> 
> i has the same value as mv88e6352_serdes_p2p_to_val[i].regval, so you can
> drop it and just use i.
> 

ok

Best regards
Holger