[v3 1/2] Docs/devicetree: add serdes.yaml

[v3 1/2] Docs/devicetree: add serdes.yaml

[Holger Brunck]

Add a yaml file to document configurable properties for serdes
interfaces. Fow now only the property serdes-tx-amplitude-millivolt is
present.

Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
---
 Documentation/devicetree/bindings/net/serdes.yaml | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/serdes.yaml

diff --git a/Documentation/devicetree/bindings/net/serdes.yaml b/Documentation/devicetree/bindings/net/serdes.yaml
new file mode 100644
index 00000000..8a71403
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/serdes.yaml
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/serdes.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Serdes
+
+maintainers:
+  - Holger Brunck <holger.brunck@hitachienergy.com>
+
+description:
+  Bindings for Serdes interfaces
+
+properties:
+  serdes-tx-amplitude-millivolt:
+    type: object
+    description:
+      Tx output amplitude of serdes interface in millivolts.
+
+additionalProperties: false
+
-- 
1.8.3.1

[v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output 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
devicetree node of the port. As the chips only supports eight dedicated
values we return EINVAL if the value in the DTS does not match.

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>
---
 drivers/net/dsa/mv88e6xxx/chip.c   | 18 ++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h   |  4 ++++
 drivers/net/dsa/mv88e6xxx/serdes.c | 48 ++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h |  5 ++++
 4 files changed, 75 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index f00cbf5..1d337dd8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2861,6 +2861,8 @@ static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)
 static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 {
 	struct dsa_switch *ds = chip->ds;
+	struct dsa_port *dp;
+	int tx_amp;
 	int err;
 	u16 reg;
 
@@ -3014,6 +3016,19 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 			return err;
 	}
 
+	if (chip->info->ops->serdes_set_tx_amplitude) {
+		dp = dsa_to_port(ds, port);
+		if (dp &&
+		    !of_property_read_u32(dp->dn,
+					  "serdes-tx-amplitude-millivolt",
+					  &tx_amp)) {
+			err = mv88e6352_serdes_set_tx_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.
@@ -4076,6 +4091,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_amplitude = mv88e6352_serdes_set_tx_amplitude,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.phylink_validate = mv88e6352_phylink_validate,
 };
@@ -4356,6 +4372,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_amplitude = mv88e6352_serdes_set_tx_amplitude,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6352_ptp_ops,
@@ -4762,6 +4779,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_amplitude = mv88e6352_serdes_set_tx_amplitude,
 	.phylink_validate = mv88e6352_phylink_validate,
 };
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 8271b8a..ef5fc26 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_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 5527301..448535c 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1307,6 +1307,54 @@ void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
 	}
 }
 
+struct mv88e6352_serdes_amp_to_val {
+	int mv;
+	u16 regval;
+};
+
+static struct mv88e6352_serdes_amp_to_val mv88e6352_serdes_amp_to_val[] = {
+	/* Mapping of configurable millivolt values to the register value */
+	{ 14, 0},
+	{ 112, 1},
+	{ 210, 2},
+	{ 308, 3},
+	{ 406, 4},
+	{ 504, 5},
+	{ 602, 6},
+	{ 700, 7},
+};
+
+int mv88e6352_serdes_set_tx_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_amp_to_val); ++i) {
+		if (mv88e6352_serdes_amp_to_val[i].mv == val) {
+			reg = mv88e6352_serdes_amp_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..526139c 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_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: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Tue,  7 Dec 2021 20:07:30 +0100
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> The mv88e6352, mv88e6240 and mv88e6176  have a serdes interface. This patch
> allows to configure the output swing to a desired value in the
> devicetree node of the port. As the chips only supports eight dedicated
> values we return EINVAL if the value in the DTS does not match.
> 
> 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>

Holger, Andrew,

there is another issue with this, which I only realized yesterday. What
if the different amplitude needs to be set only for certain SerDes
modes?

I am bringing this up because I discovered that on Turris Mox we need
to increase SerDes output amplitude when A3720 SOC is connected
directly to 88E6141 switch, but only for 2500base-x mode. For
1000base-x, the default amplitude is okay. (Also when the SOC is
connected to 88E6190, the amplitude does not need to be changed at all.)

I plan to solve this in the comphy driver, not in device-tree.

But if the solution is to be done in DTS, shouldn't there be a
possibility to define the amplitude for a specific serdes mode only?

For example
  serdes-2500base-x-tx-amplitude-millivolt
or
  serdes-tx-amplitude-millivolt-2500base-x
or
  serdes-tx-amplitude-millivolt,2500base-x
?

What do you think?

Marek

RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Holger Brunck]

Hi Marek,

> > The mv88e6352, mv88e6240 and mv88e6176  have a serdes interface. This
> > patch allows to configure the output swing to a desired value in the
> > devicetree node of the port. As the chips only supports eight
> > dedicated values we return EINVAL if the value in the DTS does not match.
> >
> > 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>
> 
> Holger, Andrew,
> 
> there is another issue with this, which I only realized yesterday. What if the
> different amplitude needs to be set only for certain SerDes modes?
> 
> I am bringing this up because I discovered that on Turris Mox we need to
> increase SerDes output amplitude when A3720 SOC is connected directly to
> 88E6141 switch, but only for 2500base-x mode. For 1000base-x, the default
> amplitude is okay. (Also when the SOC is connected to 88E6190, the amplitude
> does not need to be changed at all.)
> 

on my board I have a fixed link connected with SGMII and there is no dedicated
value given in the manual.

> I plan to solve this in the comphy driver, not in device-tree.
> 
> But if the solution is to be done in DTS, shouldn't there be a possibility to define
> the amplitude for a specific serdes mode only?
> 
> For example
>   serdes-2500base-x-tx-amplitude-millivolt
> or
>   serdes-tx-amplitude-millivolt-2500base-x
> or
>   serdes-tx-amplitude-millivolt,2500base-x
> ?
> 
> What do you think?
> 

in the data sheet for the MV6352 I am using there are no dedicated values stated for
different modes at all, they can be chosen arbitrary. So in my case I think it makes
sense to keep it as it is for now. Other driver may have other needs and may enhance
this later on.
 
Best regards
Holger   

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 12:29:13 +0000
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> Hi Marek,
> 
> > > The mv88e6352, mv88e6240 and mv88e6176  have a serdes interface. This
> > > patch allows to configure the output swing to a desired value in the
> > > devicetree node of the port. As the chips only supports eight
> > > dedicated values we return EINVAL if the value in the DTS does not match.
> > >
> > > 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>  
> > 
> > Holger, Andrew,
> > 
> > there is another issue with this, which I only realized yesterday. What if the
> > different amplitude needs to be set only for certain SerDes modes?
> > 
> > I am bringing this up because I discovered that on Turris Mox we need to
> > increase SerDes output amplitude when A3720 SOC is connected directly to
> > 88E6141 switch, but only for 2500base-x mode. For 1000base-x, the default
> > amplitude is okay. (Also when the SOC is connected to 88E6190, the amplitude
> > does not need to be changed at all.)
> >   
> 
> on my board I have a fixed link connected with SGMII and there is no dedicated
> value given in the manual.
> 
> > I plan to solve this in the comphy driver, not in device-tree.
> > 
> > But if the solution is to be done in DTS, shouldn't there be a possibility to define
> > the amplitude for a specific serdes mode only?
> > 
> > For example
> >   serdes-2500base-x-tx-amplitude-millivolt
> > or
> >   serdes-tx-amplitude-millivolt-2500base-x
> > or
> >   serdes-tx-amplitude-millivolt,2500base-x
> > ?
> > 
> > What do you think?
> >   
> 
> in the data sheet for the MV6352 I am using there are no dedicated values stated for
> different modes at all, they can be chosen arbitrary. So in my case I think it makes
> sense to keep it as it is for now. Other driver may have other needs and may enhance
> this later on.

Hi Holger,

but the mv88e6xxx driver also drives switches that allow changing
serdes modes. There does not need be dedicated TX amplitude register for
each serdes mode, the point is that we may want to declare different
amplitudes for different modes.

So the question is: if we go with your binding proposal for the whole
mv88e6xxx driver, and in the future someone will want to declare
different amplitudes for different modes on another model, would he
need to deprecate your binding or would it be easy to extend?

Marek

RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output 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 devicetree node of the port. As the chips only supports
> > > > eight dedicated values we return EINVAL if the value in the DTS does not
> match.
> > > >
> > > > 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>
> > >
> > > Holger, Andrew,
> > >
> > > there is another issue with this, which I only realized yesterday.
> > > What if the different amplitude needs to be set only for certain SerDes
> modes?
> > >
> > > I am bringing this up because I discovered that on Turris Mox we
> > > need to increase SerDes output amplitude when A3720 SOC is connected
> > > directly to
> > > 88E6141 switch, but only for 2500base-x mode. For 1000base-x, the
> > > default amplitude is okay. (Also when the SOC is connected to
> > > 88E6190, the amplitude does not need to be changed at all.)
> > >
> >
> > on my board I have a fixed link connected with SGMII and there is no
> > dedicated value given in the manual.
> >
> > > I plan to solve this in the comphy driver, not in device-tree.
> > >
> > > But if the solution is to be done in DTS, shouldn't there be a
> > > possibility to define the amplitude for a specific serdes mode only?
> > >
> > > For example
> > >   serdes-2500base-x-tx-amplitude-millivolt
> > > or
> > >   serdes-tx-amplitude-millivolt-2500base-x
> > > or
> > >   serdes-tx-amplitude-millivolt,2500base-x
> > > ?
> > >
> > > What do you think?
> > >
> >
> > in the data sheet for the MV6352 I am using there are no dedicated
> > values stated for different modes at all, they can be chosen
> > arbitrary. So in my case I think it makes sense to keep it as it is
> > for now. Other driver may have other needs and may enhance this later on.
> 
> Hi Holger,
> 
> but the mv88e6xxx driver also drives switches that allow changing serdes
> modes. There does not need be dedicated TX amplitude register for each serdes
> mode, the point is that we may want to declare different amplitudes for
> different modes.
> 
> So the question is: if we go with your binding proposal for the whole mv88e6xxx
> driver, and in the future someone will want to declare different amplitudes for
> different modes on another model, would he need to deprecate your binding or
> would it be easy to extend?
> 

ok I see. So if I follow your proposal in my case it would be something like:
serdes-sgmii-tx-amplitude-millivolt to start with ? 

I can do that. Andrew what do you think?

Best regards
Holger

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 15:49:19 +0000
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> > >  
> > > > > The mv88e6352, mv88e6240 and mv88e6176  have a serdes interface.
> > > > > This patch allows to configure the output swing to a desired value
> > > > > in the devicetree node of the port. As the chips only supports
> > > > > eight dedicated values we return EINVAL if the value in the DTS does not  
> > match.  
> > > > >
> > > > > 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>  
> > > >
> > > > Holger, Andrew,
> > > >
> > > > there is another issue with this, which I only realized yesterday.
> > > > What if the different amplitude needs to be set only for certain SerDes  
> > modes?  
> > > >
> > > > I am bringing this up because I discovered that on Turris Mox we
> > > > need to increase SerDes output amplitude when A3720 SOC is connected
> > > > directly to
> > > > 88E6141 switch, but only for 2500base-x mode. For 1000base-x, the
> > > > default amplitude is okay. (Also when the SOC is connected to
> > > > 88E6190, the amplitude does not need to be changed at all.)
> > > >  
> > >
> > > on my board I have a fixed link connected with SGMII and there is no
> > > dedicated value given in the manual.
> > >  
> > > > I plan to solve this in the comphy driver, not in device-tree.
> > > >
> > > > But if the solution is to be done in DTS, shouldn't there be a
> > > > possibility to define the amplitude for a specific serdes mode only?
> > > >
> > > > For example
> > > >   serdes-2500base-x-tx-amplitude-millivolt
> > > > or
> > > >   serdes-tx-amplitude-millivolt-2500base-x
> > > > or
> > > >   serdes-tx-amplitude-millivolt,2500base-x
> > > > ?
> > > >
> > > > What do you think?
> > > >  
> > >
> > > in the data sheet for the MV6352 I am using there are no dedicated
> > > values stated for different modes at all, they can be chosen
> > > arbitrary. So in my case I think it makes sense to keep it as it is
> > > for now. Other driver may have other needs and may enhance this later on.  
> > 
> > Hi Holger,
> > 
> > but the mv88e6xxx driver also drives switches that allow changing serdes
> > modes. There does not need be dedicated TX amplitude register for each serdes
> > mode, the point is that we may want to declare different amplitudes for
> > different modes.
> > 
> > So the question is: if we go with your binding proposal for the whole mv88e6xxx
> > driver, and in the future someone will want to declare different amplitudes for
> > different modes on another model, would he need to deprecate your binding or
> > would it be easy to extend?
> >   
> 
> ok I see. So if I follow your proposal in my case it would be something like:
> serdes-sgmii-tx-amplitude-millivolt to start with ? 
> 
> I can do that. Andrew what do you think?

Or maybe two properties:
  serdes-tx-amplitude-millivolt = <700 1000 1100>;
  serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";
?

If
  serdes-tx-amplitude-modes
is omitted, then
  serdes-tx-amplitude-millivolt
should only contain one value, and this is used for all serdes modes.

This would be compatible with your change. You only need to define the
bidning for now, your code can stay the same - you don't need to add
support for multiple values or for the second property now, it can be
done later when needed. But the binding should be defined to support
those different modes.

Marek

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

Hello Vladimir,

On Wed, 8 Dec 2021 17:17:20 +0100
Marek Behún <kabel@kernel.org> wrote:

> On Wed, 8 Dec 2021 15:49:19 +0000
> Holger Brunck <holger.brunck@hitachienergy.com> wrote:
> 
> > > >    
> > > > > > The mv88e6352, mv88e6240 and mv88e6176  have a serdes interface.
> > > > > > This patch allows to configure the output swing to a desired value
> > > > > > in the devicetree node of the port. As the chips only supports
> > > > > > eight dedicated values we return EINVAL if the value in the DTS does not    
> > > match.    
> > > > > >
> > > > > > 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>    
> > > > >
> > > > > Holger, Andrew,
> > > > >
> > > > > there is another issue with this, which I only realized yesterday.
> > > > > What if the different amplitude needs to be set only for certain SerDes    
> > > modes?    
> > > > >
> > > > > I am bringing this up because I discovered that on Turris Mox we
> > > > > need to increase SerDes output amplitude when A3720 SOC is connected
> > > > > directly to
> > > > > 88E6141 switch, but only for 2500base-x mode. For 1000base-x, the
> > > > > default amplitude is okay. (Also when the SOC is connected to
> > > > > 88E6190, the amplitude does not need to be changed at all.)
> > > > >    
> > > >
> > > > on my board I have a fixed link connected with SGMII and there is no
> > > > dedicated value given in the manual.
> > > >    
> > > > > I plan to solve this in the comphy driver, not in device-tree.
> > > > >
> > > > > But if the solution is to be done in DTS, shouldn't there be a
> > > > > possibility to define the amplitude for a specific serdes mode only?
> > > > >
> > > > > For example
> > > > >   serdes-2500base-x-tx-amplitude-millivolt
> > > > > or
> > > > >   serdes-tx-amplitude-millivolt-2500base-x
> > > > > or
> > > > >   serdes-tx-amplitude-millivolt,2500base-x
> > > > > ?
> > > > >
> > > > > What do you think?
> > > > >    
> > > >
> > > > in the data sheet for the MV6352 I am using there are no dedicated
> > > > values stated for different modes at all, they can be chosen
> > > > arbitrary. So in my case I think it makes sense to keep it as it is
> > > > for now. Other driver may have other needs and may enhance this later on.    
> > > 
> > > Hi Holger,
> > > 
> > > but the mv88e6xxx driver also drives switches that allow changing serdes
> > > modes. There does not need be dedicated TX amplitude register for each serdes
> > > mode, the point is that we may want to declare different amplitudes for
> > > different modes.
> > > 
> > > So the question is: if we go with your binding proposal for the whole mv88e6xxx
> > > driver, and in the future someone will want to declare different amplitudes for
> > > different modes on another model, would he need to deprecate your binding or
> > > would it be easy to extend?
> > >     
> > 
> > ok I see. So if I follow your proposal in my case it would be something like:
> > serdes-sgmii-tx-amplitude-millivolt to start with ? 
> > 
> > I can do that. Andrew what do you think?  
> 
> Or maybe two properties:
>   serdes-tx-amplitude-millivolt = <700 1000 1100>;
>   serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";
> ?
> 
> If
>   serdes-tx-amplitude-modes
> is omitted, then
>   serdes-tx-amplitude-millivolt
> should only contain one value, and this is used for all serdes modes.
> 
> This would be compatible with your change. You only need to define the
> bidning for now, your code can stay the same - you don't need to add
> support for multiple values or for the second property now, it can be
> done later when needed. But the binding should be defined to support
> those different modes.

Vladimir, can you send your thoughts about this proposal? We are trying
to propose binding for defining serdes TX amplitude.

Marek

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 05:21:04PM +0100, Marek Behún wrote:
> Hello Vladimir,
> > > > but the mv88e6xxx driver also drives switches that allow changing serdes
> > > > modes. There does not need be dedicated TX amplitude register for each serdes
> > > > mode, the point is that we may want to declare different amplitudes for
> > > > different modes.
> > > >
> > > > So the question is: if we go with your binding proposal for the whole mv88e6xxx
> > > > driver, and in the future someone will want to declare different amplitudes for
> > > > different modes on another model, would he need to deprecate your binding or
> > > > would it be easy to extend?
> > > >
> > >
> > > ok I see. So if I follow your proposal in my case it would be something like:
> > > serdes-sgmii-tx-amplitude-millivolt to start with ?
> > >
> > > I can do that. Andrew what do you think?
> >
> > Or maybe two properties:
> >   serdes-tx-amplitude-millivolt = <700 1000 1100>;
> >   serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";
> > ?
> >
> > If
> >   serdes-tx-amplitude-modes
> > is omitted, then
> >   serdes-tx-amplitude-millivolt
> > should only contain one value, and this is used for all serdes modes.
> >
> > This would be compatible with your change. You only need to define the
> > bidning for now, your code can stay the same - you don't need to add
> > support for multiple values or for the second property now, it can be
> > done later when needed. But the binding should be defined to support
> > those different modes.
>
> Vladimir, can you send your thoughts about this proposal? We are trying
> to propose binding for defining serdes TX amplitude.

I don't have any specific concern here. It sounds reasonable for
different data rates to require different transmitter configurations.
Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
properties sounds okay, although I think a prefix with "-names" at the
end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
so maybe "serdes-tx-amplitude-millivolt-names"?
Maybe we could name the first element "default", and just the others
would be named after a phy-mode. This way, if a specific TX amplitude is
found in the device tree for the currently operating PHY mode, it can be
used, otherwise the default (first) amplitude can be used.

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 06:41:31PM +0200, Vladimir Oltean wrote:
> On Wed, Dec 08, 2021 at 05:21:04PM +0100, Marek Behún wrote:
> > Hello Vladimir,
> > > > > but the mv88e6xxx driver also drives switches that allow changing serdes
> > > > > modes. There does not need be dedicated TX amplitude register for each serdes
> > > > > mode, the point is that we may want to declare different amplitudes for
> > > > > different modes.
> > > > >
> > > > > So the question is: if we go with your binding proposal for the whole mv88e6xxx
> > > > > driver, and in the future someone will want to declare different amplitudes for
> > > > > different modes on another model, would he need to deprecate your binding or
> > > > > would it be easy to extend?
> > > > >
> > > >
> > > > ok I see. So if I follow your proposal in my case it would be something like:
> > > > serdes-sgmii-tx-amplitude-millivolt to start with ?
> > > >
> > > > I can do that. Andrew what do you think?
> > >
> > > Or maybe two properties:
> > >   serdes-tx-amplitude-millivolt = <700 1000 1100>;
> > >   serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";
> > > ?
> > >
> > > If
> > >   serdes-tx-amplitude-modes
> > > is omitted, then
> > >   serdes-tx-amplitude-millivolt
> > > should only contain one value, and this is used for all serdes modes.
> > >
> > > This would be compatible with your change. You only need to define the
> > > bidning for now, your code can stay the same - you don't need to add
> > > support for multiple values or for the second property now, it can be
> > > done later when needed. But the binding should be defined to support
> > > those different modes.
> >
> > Vladimir, can you send your thoughts about this proposal? We are trying
> > to propose binding for defining serdes TX amplitude.
> 
> I don't have any specific concern here. It sounds reasonable for
> different data rates to require different transmitter configurations.
> Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
> properties sounds okay, although I think a prefix with "-names" at the
> end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
> so maybe "serdes-tx-amplitude-millivolt-names"?
> Maybe we could name the first element "default", and just the others
> would be named after a phy-mode. This way, if a specific TX amplitude is
> found in the device tree for the currently operating PHY mode, it can be
> used, otherwise the default (first) amplitude can be used.

Also, maybe drop the "serdes-" prefix? The property will sit under a
SERDES lane node, so it would be a bit redundant?

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 18:41:31 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Wed, Dec 08, 2021 at 05:21:04PM +0100, Marek Behún wrote:
> > Hello Vladimir,  
> > > > > but the mv88e6xxx driver also drives switches that allow changing serdes
> > > > > modes. There does not need be dedicated TX amplitude register for each serdes
> > > > > mode, the point is that we may want to declare different amplitudes for
> > > > > different modes.
> > > > >
> > > > > So the question is: if we go with your binding proposal for the whole mv88e6xxx
> > > > > driver, and in the future someone will want to declare different amplitudes for
> > > > > different modes on another model, would he need to deprecate your binding or
> > > > > would it be easy to extend?
> > > > >  
> > > >
> > > > ok I see. So if I follow your proposal in my case it would be something like:
> > > > serdes-sgmii-tx-amplitude-millivolt to start with ?
> > > >
> > > > I can do that. Andrew what do you think?  
> > >
> > > Or maybe two properties:
> > >   serdes-tx-amplitude-millivolt = <700 1000 1100>;
> > >   serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";
> > > ?
> > >
> > > If
> > >   serdes-tx-amplitude-modes
> > > is omitted, then
> > >   serdes-tx-amplitude-millivolt
> > > should only contain one value, and this is used for all serdes modes.
> > >
> > > This would be compatible with your change. You only need to define the
> > > bidning for now, your code can stay the same - you don't need to add
> > > support for multiple values or for the second property now, it can be
> > > done later when needed. But the binding should be defined to support
> > > those different modes.  
> >
> > Vladimir, can you send your thoughts about this proposal? We are trying
> > to propose binding for defining serdes TX amplitude.  
> 
> I don't have any specific concern here. It sounds reasonable for
> different data rates to require different transmitter configurations.
> Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
> properties sounds okay, although I think a prefix with "-names" at the
> end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
> so maybe "serdes-tx-amplitude-millivolt-names"?
> Maybe we could name the first element "default", and just the others
> would be named after a phy-mode. This way, if a specific TX amplitude is
> found in the device tree for the currently operating PHY mode, it can be
> used, otherwise the default (first) amplitude can be used.

Yes, the pair
  serdes-tx-amplitude-millivolt
  serdes-tx-amplitude-millivolt-names
is the best.

If the second is not defined, the first should contain only one value,
and that is used as default.

If multiple values are defined, but "default" is not, the driver should
set default value as the default value of the corresponding register.

The only remaining question is this: I need to implement this also for
comphy driver. In this case, the properties should be defined in the
comphy node, not in the MAC node. But the comphy also supports PCIe,
USB3 and SATA modes. We don't have strings for them. So this will need
to be extended in the future.

But for now this proposal seems most legit. I think the properties
should be defined in common PHY bindings, and other bindings should
refer to them via $ref.

Marek

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 05:51:29PM +0100, Marek Behún wrote:
> > > Vladimir, can you send your thoughts about this proposal? We are trying
> > > to propose binding for defining serdes TX amplitude.
> >
> > I don't have any specific concern here. It sounds reasonable for
> > different data rates to require different transmitter configurations.
> > Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
> > properties sounds okay, although I think a prefix with "-names" at the
> > end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
> > so maybe "serdes-tx-amplitude-millivolt-names"?
> > Maybe we could name the first element "default", and just the others
> > would be named after a phy-mode. This way, if a specific TX amplitude is
> > found in the device tree for the currently operating PHY mode, it can be
> > used, otherwise the default (first) amplitude can be used.
>
> Yes, the pair
>   serdes-tx-amplitude-millivolt
>   serdes-tx-amplitude-millivolt-names
> is the best.
>
> If the second is not defined, the first should contain only one value,
> and that is used as default.
>
> If multiple values are defined, but "default" is not, the driver should
> set default value as the default value of the corresponding register.
>
> The only remaining question is this: I need to implement this also for
> comphy driver. In this case, the properties should be defined in the
> comphy node, not in the MAC node. But the comphy also supports PCIe,
> USB3 and SATA modes. We don't have strings for them. So this will need
> to be extended in the future.
>
> But for now this proposal seems most legit. I think the properties
> should be defined in common PHY bindings, and other bindings should
> refer to them via $ref.

I wouldn't $ref the tx-amplitude-millivolt-names from the phy-mode,
because (a) not all phy-mode values are valid (think of parallel interfaces
like rgmii) and (b) because sata, pcie, usb are also valid SERDES
protocols as you point out. With the risk of a bit of duplication, I
think I'd keep the SERDES protocol names a separate thing for the YAML
validator.

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Andrew Lunn]

> Or maybe two properties:
>   serdes-tx-amplitude-millivolt = <700 1000 1100>;
>   serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";

I think this is good. But you are missing the DT email list from Cc:
You need Robs input.

We probably should list the accepted mode values somehow. I don't know
if we can reference ethernet-controller.yaml : phy-connection-type, or
if we need a new list. This gets interesting when PCIe and USB needs
to use this property, what names are used, and if it is possible to
combine two different lists?

	Andrew

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 18:49:32 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Wed, Dec 08, 2021 at 06:41:31PM +0200, Vladimir Oltean wrote:
> > On Wed, Dec 08, 2021 at 05:21:04PM +0100, Marek Behún wrote:  
> > > Hello Vladimir,  
> > > > > > but the mv88e6xxx driver also drives switches that allow changing serdes
> > > > > > modes. There does not need be dedicated TX amplitude register for each serdes
> > > > > > mode, the point is that we may want to declare different amplitudes for
> > > > > > different modes.
> > > > > >
> > > > > > So the question is: if we go with your binding proposal for the whole mv88e6xxx
> > > > > > driver, and in the future someone will want to declare different amplitudes for
> > > > > > different modes on another model, would he need to deprecate your binding or
> > > > > > would it be easy to extend?
> > > > > >  
> > > > >
> > > > > ok I see. So if I follow your proposal in my case it would be something like:
> > > > > serdes-sgmii-tx-amplitude-millivolt to start with ?
> > > > >
> > > > > I can do that. Andrew what do you think?  
> > > >
> > > > Or maybe two properties:
> > > >   serdes-tx-amplitude-millivolt = <700 1000 1100>;
> > > >   serdes-tx-amplitude-modes = "sgmii", "2500base-x", "10gbase-r";
> > > > ?
> > > >
> > > > If
> > > >   serdes-tx-amplitude-modes
> > > > is omitted, then
> > > >   serdes-tx-amplitude-millivolt
> > > > should only contain one value, and this is used for all serdes modes.
> > > >
> > > > This would be compatible with your change. You only need to define the
> > > > bidning for now, your code can stay the same - you don't need to add
> > > > support for multiple values or for the second property now, it can be
> > > > done later when needed. But the binding should be defined to support
> > > > those different modes.  
> > >
> > > Vladimir, can you send your thoughts about this proposal? We are trying
> > > to propose binding for defining serdes TX amplitude.  
> > 
> > I don't have any specific concern here. It sounds reasonable for
> > different data rates to require different transmitter configurations.
> > Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
> > properties sounds okay, although I think a prefix with "-names" at the
> > end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
> > so maybe "serdes-tx-amplitude-millivolt-names"?
> > Maybe we could name the first element "default", and just the others
> > would be named after a phy-mode. This way, if a specific TX amplitude is
> > found in the device tree for the currently operating PHY mode, it can be
> > used, otherwise the default (first) amplitude can be used.  
> 
> Also, maybe drop the "serdes-" prefix? The property will sit under a
> SERDES lane node, so it would be a bit redundant?

Hmm. Holger's proposal adds the property into the port node, not SerDes
lane node. mv88e6xxx does not define bindings for SerDes lane nodes
(yet).

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 18:59:38 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Wed, Dec 08, 2021 at 05:51:29PM +0100, Marek Behún wrote:
> > > > Vladimir, can you send your thoughts about this proposal? We are trying
> > > > to propose binding for defining serdes TX amplitude.  
> > >
> > > I don't have any specific concern here. It sounds reasonable for
> > > different data rates to require different transmitter configurations.
> > > Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
> > > properties sounds okay, although I think a prefix with "-names" at the
> > > end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
> > > so maybe "serdes-tx-amplitude-millivolt-names"?
> > > Maybe we could name the first element "default", and just the others
> > > would be named after a phy-mode. This way, if a specific TX amplitude is
> > > found in the device tree for the currently operating PHY mode, it can be
> > > used, otherwise the default (first) amplitude can be used.  
> >
> > Yes, the pair
> >   serdes-tx-amplitude-millivolt
> >   serdes-tx-amplitude-millivolt-names
> > is the best.
> >
> > If the second is not defined, the first should contain only one value,
> > and that is used as default.
> >
> > If multiple values are defined, but "default" is not, the driver should
> > set default value as the default value of the corresponding register.
> >
> > The only remaining question is this: I need to implement this also for
> > comphy driver. In this case, the properties should be defined in the
> > comphy node, not in the MAC node. But the comphy also supports PCIe,
> > USB3 and SATA modes. We don't have strings for them. So this will need
> > to be extended in the future.
> >
> > But for now this proposal seems most legit. I think the properties
> > should be defined in common PHY bindings, and other bindings should
> > refer to them via $ref.  
> 
> I wouldn't $ref the tx-amplitude-millivolt-names from the phy-mode,
> because (a) not all phy-mode values are valid (think of parallel interfaces
> like rgmii) and (b) because sata, pcie, usb are also valid SERDES
> protocols as you point out. With the risk of a bit of duplication, I
> think I'd keep the SERDES protocol names a separate thing for the YAML
> validator.

Not what I meant. What I meant was that the tx-amplitude-millivolt*
properties should be defined in binding for common PHY (not network PHY)
  Documentation/devicetree/bindings/phy/phy-bindings.txt,
and then the mv88e6xxx binding should refer it's
tx-amplitude-millivolt* properties from there.

And the definition in common PHY binding should list all modes in an
enum, containing all network SerDes modes, plus the other modes like
PCIe, USB3, DisplayPort, LVDS, SATA, ...

Marek

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 18:10:09 +0100
Marek Behún <kabel@kernel.org> wrote:

> On Wed, 8 Dec 2021 18:59:38 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > On Wed, Dec 08, 2021 at 05:51:29PM +0100, Marek Behún wrote:  
> > > > > Vladimir, can you send your thoughts about this proposal? We are trying
> > > > > to propose binding for defining serdes TX amplitude.    
> > > >
> > > > I don't have any specific concern here. It sounds reasonable for
> > > > different data rates to require different transmitter configurations.
> > > > Having separate "serdes-tx-amplitude-millivolt" and "serdes-tx-amplitude-modes"
> > > > properties sounds okay, although I think a prefix with "-names" at the
> > > > end is more canonical ("pinctrl-names", "clock-names", "reg-names" etc),
> > > > so maybe "serdes-tx-amplitude-millivolt-names"?
> > > > Maybe we could name the first element "default", and just the others
> > > > would be named after a phy-mode. This way, if a specific TX amplitude is
> > > > found in the device tree for the currently operating PHY mode, it can be
> > > > used, otherwise the default (first) amplitude can be used.    
> > >
> > > Yes, the pair
> > >   serdes-tx-amplitude-millivolt
> > >   serdes-tx-amplitude-millivolt-names
> > > is the best.
> > >
> > > If the second is not defined, the first should contain only one value,
> > > and that is used as default.
> > >
> > > If multiple values are defined, but "default" is not, the driver should
> > > set default value as the default value of the corresponding register.
> > >
> > > The only remaining question is this: I need to implement this also for
> > > comphy driver. In this case, the properties should be defined in the
> > > comphy node, not in the MAC node. But the comphy also supports PCIe,
> > > USB3 and SATA modes. We don't have strings for them. So this will need
> > > to be extended in the future.
> > >
> > > But for now this proposal seems most legit. I think the properties
> > > should be defined in common PHY bindings, and other bindings should
> > > refer to them via $ref.    
> > 
> > I wouldn't $ref the tx-amplitude-millivolt-names from the phy-mode,
> > because (a) not all phy-mode values are valid (think of parallel interfaces
> > like rgmii) and (b) because sata, pcie, usb are also valid SERDES
> > protocols as you point out. With the risk of a bit of duplication, I
> > think I'd keep the SERDES protocol names a separate thing for the YAML
> > validator.  
> 
> Not what I meant. What I meant was that the tx-amplitude-millivolt*
> properties should be defined in binding for common PHY (not network PHY)
>   Documentation/devicetree/bindings/phy/phy-bindings.txt,
> and then the mv88e6xxx binding should refer it's
> tx-amplitude-millivolt* properties from there.
> 
> And the definition in common PHY binding should list all modes in an
> enum, containing all network SerDes modes, plus the other modes like
> PCIe, USB3, DisplayPort, LVDS, SATA, ...
> 
> Marek

Note that one PHY binding already defines such property:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/phy/brcm,sata-phy.yaml?h=v5.16-rc4#n86

So I think we need to make it generic.

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 18:00:05 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> This gets interesting when PCIe and USB needs
> to use this property, what names are used, and if it is possible to
> combine two different lists?

I don't think it is possible, I tried that once and couldn't get it to
work.

I am going to try write the proposal. But unfortunately PHY binding is
not converted to YAML yet :(

Marek

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 06:00:57PM +0100, Marek Behún wrote:
> > Also, maybe drop the "serdes-" prefix? The property will sit under a
> > SERDES lane node, so it would be a bit redundant?
> 
> Hmm. Holger's proposal adds the property into the port node, not SerDes
> lane node. mv88e6xxx does not define bindings for SerDes lane nodes
> (yet).

We need to be careful about that. You're saying that there chances of
there being a separate SERDES driver for mv88e6xxx in the future?

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 06:10:09PM +0100, Marek Behún wrote:
> > > Yes, the pair
> > >   serdes-tx-amplitude-millivolt
> > >   serdes-tx-amplitude-millivolt-names
> > > is the best.
> > >
> > > If the second is not defined, the first should contain only one value,
> > > and that is used as default.
> > >
> > > If multiple values are defined, but "default" is not, the driver should
> > > set default value as the default value of the corresponding register.
> > >
> > > The only remaining question is this: I need to implement this also for
> > > comphy driver. In this case, the properties should be defined in the
> > > comphy node, not in the MAC node. But the comphy also supports PCIe,
> > > USB3 and SATA modes. We don't have strings for them. So this will need
> > > to be extended in the future.
> > >
> > > But for now this proposal seems most legit. I think the properties
> > > should be defined in common PHY bindings, and other bindings should
> > > refer to them via $ref.
> >
> > I wouldn't $ref the tx-amplitude-millivolt-names from the phy-mode,
> > because (a) not all phy-mode values are valid (think of parallel interfaces
> > like rgmii) and (b) because sata, pcie, usb are also valid SERDES
> > protocols as you point out. With the risk of a bit of duplication, I
> > think I'd keep the SERDES protocol names a separate thing for the YAML
> > validator.
>
> Not what I meant. What I meant was that the tx-amplitude-millivolt*
> properties should be defined in binding for common PHY (not network PHY)
>   Documentation/devicetree/bindings/phy/phy-bindings.txt,
> and then the mv88e6xxx binding should refer it's
> tx-amplitude-millivolt* properties from there.
>
> And the definition in common PHY binding should list all modes in an
> enum, containing all network SerDes modes, plus the other modes like
> PCIe, USB3, DisplayPort, LVDS, SATA, ...

Sorry, I misunderstood what you meant by "common PHY bindings", thanks
for clarifying. Yes, Documentation/devicetree/bindings/phy/phy-bindings.txt
seems like the place, after being converted to YAML.

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 19:19:09 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Wed, Dec 08, 2021 at 06:00:57PM +0100, Marek Behún wrote:
> > > Also, maybe drop the "serdes-" prefix? The property will sit under a
> > > SERDES lane node, so it would be a bit redundant?  
> > 
> > Hmm. Holger's proposal adds the property into the port node, not SerDes
> > lane node. mv88e6xxx does not define bindings for SerDes lane nodes
> > (yet).  
> 
> We need to be careful about that. You're saying that there chances of
> there being a separate SERDES driver for mv88e6xxx in the future?

I don't think so. Although Russell is working on rewriting the SerDes
code to new Phylink API, the SerDes code will always be a part of
mv88e6xxx driver, I think.

But we already have bindings for internal PHYs on mv88e6xxx, so it
wouldn't be tough to add bindings for SerDeses. The question is whether
this is necessary, since the ports are coupled with their SerDeses in
HW, and the coupling cannot be changed.

But there are models, like 88E6390X, where a SerDes lane is shared
between multiple ports, for example lane 0x12 is shared between port 2
and port 9:
- either the lane can belong to port 2, if port 2 uses serdes
- or the lane can belong to port 9 and port 9 can be in multi-lane mode
  (xaui or rxaui). In this case port 2 uses the internal copper PHY,
  AFAIK

So the question is whether we want to be able to defined this
tx-amplitude different for different lanes. But if so, I think we will
need to add bindings for SerDes PHYs into mv88e6xxx.

So anyway, regarding whether the properites should have a "serdes-"
prefix:
- if they are in SerDes node, it isn't necessary
- if we put them in port nodes, it depends, but probably also is not
  necessary, since I don't think there will ever be a conflict with
  copper TX amplitude or something.

Marek

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Andrew Lunn]

On Wed, Dec 08, 2021 at 06:36:26PM +0100, Marek Behún wrote:
> On Wed, 8 Dec 2021 19:19:09 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > On Wed, Dec 08, 2021 at 06:00:57PM +0100, Marek Behún wrote:
> > > > Also, maybe drop the "serdes-" prefix? The property will sit under a
> > > > SERDES lane node, so it would be a bit redundant?  
> > > 
> > > Hmm. Holger's proposal adds the property into the port node, not SerDes
> > > lane node. mv88e6xxx does not define bindings for SerDes lane nodes
> > > (yet).  
> > 
> > We need to be careful about that. You're saying that there chances of
> > there being a separate SERDES driver for mv88e6xxx in the future?
> 
> I don't think so. Although Russell is working on rewriting the SerDes
> code to new Phylink API, the SerDes code will always be a part of
> mv88e6xxx driver, I think.

In theory, the 6352 family uses standard c22 layout for its SERDES. It
might be possible to use generic code for that. But given the
architecture, i expect such a change would have the mv88e6xxx
instantiate such generic code, not use an external device.

For the 6390 family the SERDES and the MAC are pretty intertwined, and
it is not a 1:1 mapping. It might be possible to make use of shared
code, but i've much doubt it will be a separate device.

I would put the properties in the port nodes, next to phy-mode,
phy-handle, etc.

Where it might get interesting is the 10G modes, where there are 4
lanes. Is it possible to configure the voltage for each lane? Or is it
one setting for all 4 lanes? I've not looked at the data sheet, so i
cannot answer this.
y
    Andrew

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 8 Dec 2021 18:55:48 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Dec 08, 2021 at 06:36:26PM +0100, Marek Behún wrote:
> > On Wed, 8 Dec 2021 19:19:09 +0200
> > Vladimir Oltean <olteanv@gmail.com> wrote:
> >   
> > > On Wed, Dec 08, 2021 at 06:00:57PM +0100, Marek Behún wrote:  
> > > > > Also, maybe drop the "serdes-" prefix? The property will sit under a
> > > > > SERDES lane node, so it would be a bit redundant?    
> > > > 
> > > > Hmm. Holger's proposal adds the property into the port node, not SerDes
> > > > lane node. mv88e6xxx does not define bindings for SerDes lane nodes
> > > > (yet).    
> > > 
> > > We need to be careful about that. You're saying that there chances of
> > > there being a separate SERDES driver for mv88e6xxx in the future?  
> > 
> > I don't think so. Although Russell is working on rewriting the SerDes
> > code to new Phylink API, the SerDes code will always be a part of
> > mv88e6xxx driver, I think.  
> 
> In theory, the 6352 family uses standard c22 layout for its SERDES. It
> might be possible to use generic code for that. But given the
> architecture, i expect such a change would have the mv88e6xxx
> instantiate such generic code, not use an external device.
> 
> For the 6390 family the SERDES and the MAC are pretty intertwined, and
> it is not a 1:1 mapping. It might be possible to make use of shared
> code, but i've much doubt it will be a separate device.
> 
> I would put the properties in the port nodes, next to phy-mode,
> phy-handle, etc.
> 
> Where it might get interesting is the 10G modes, where there are 4
> lanes. Is it possible to configure the voltage for each lane? Or is it
> one setting for all 4 lanes? I've not looked at the data sheet, so i
> cannot answer this.
> y
>     Andrew

The FS for PHY and Serdes for 6390X does not document TX amplitude
registers. Release notes document some additional registers, or mention
how to change frequency, but do not document the registers explicitly.

So we don't know currently how to change TX amplitude on those
switches. But I guess I could find out the same way I found out about
88E6393X frequency change from undocumented register. Or if some vendor
needs it, they can ask Marvell which registers they should use to
change TX amplitude.

I personally don't have any device with these switches though.

Marek

RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Holger Brunck]

Hi Marek,

> > This gets interesting when PCIe and USB needs to use this property,
> > what names are used, and if it is possible to combine two different
> > lists?
>
> I don't think it is possible, I tried that once and couldn't get it to work.
>
> I am going to try write the proposal. But unfortunately PHY binding is not
> converted to YAML yet :(
>

I saw you recent patches to convert this. Thanks!

This make my serdes.yaml obsolete then, correct? Should I then only re-post
my driver code, once your patches are accepted?

Best regards
Holger

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

On Wed, 15 Dec 2021 10:27:02 +0000
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> Hi Marek,
> 
> > > This gets interesting when PCIe and USB needs to use this property,
> > > what names are used, and if it is possible to combine two different
> > > lists?  
> >
> > I don't think it is possible, I tried that once and couldn't get it to work.
> >
> > I am going to try write the proposal. But unfortunately PHY binding is not
> > converted to YAML yet :(
> >  
> 
> I saw you recent patches to convert this. Thanks!
> 
> This make my serdes.yaml obsolete then, correct? Should I then only re-post
> my driver code, once your patches are accepted?

Yes, please let's do it this way. It may take some time for Rob to
review this, though, and he may require some changes.

Also I was thinking whether it wouldn't be better to put the property
into a separate SerDes PHY node, i.e.

  switch {
    compatible = "marvell,mv88e6085";
    ...

    ports {
      port@6 {
        reg = <0x6>;
        phy-handle = <&switch_serdes_phy>;
      };

      ...
    };

    mdio {
      switch_serdes_phy: ethernet-phy@f {
        reg = <0xf>;
        tx-amplitude-microvolt = <1234567>;
      };

      ...
    };
  };

RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Holger Brunck]

Hi Marek,

> >
> > > > This gets interesting when PCIe and USB needs to use this
> > > > property, what names are used, and if it is possible to combine
> > > > two different lists?
> > >
> > > I don't think it is possible, I tried that once and couldn't get it to work.
> > >
> > > I am going to try write the proposal. But unfortunately PHY binding
> > > is not converted to YAML yet :(
> > >
> >
> > I saw you recent patches to convert this. Thanks!
> >
> > This make my serdes.yaml obsolete then, correct? Should I then only
> > re-post my driver code, once your patches are accepted?
> 
> Yes, please let's do it this way. It may take some time for Rob to review this,
> though, and he may require some changes.
> 

I saw your v3 patch for the bindings and I would adapt then my patch
accordingly to tx-p2p-microvolt. 

> Also I was thinking whether it wouldn't be better to put the property into a
> separate SerDes PHY node, i.e.
> 
>   switch {
>     compatible = "marvell,mv88e6085";
>     ...
> 
>     ports {
>       port@6 {
>         reg = <0x6>;
>         phy-handle = <&switch_serdes_phy>;
>       };
> 
>       ...
>     };
> 
>     mdio {
>       switch_serdes_phy: ethernet-phy@f {
>         reg = <0xf>;
>         tx-amplitude-microvolt = <1234567>;
>       };
> 
>       ...
>     };
>   };

this would mean in regard to my patch instead of checking directly for the
property in mv88e6xxx_setup_port  I would parse for the phy-handle first
and then for the property? 

Should I wait until your patch is accepted and merged?

Best regards
Holger

Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

[Marek Behún]

Hi Holger,

On Thu, 20 Jan 2022 07:52:01 +0000
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> Hi Marek,
> 
> > >  
> > > > > This gets interesting when PCIe and USB needs to use this
> > > > > property, what names are used, and if it is possible to combine
> > > > > two different lists?  
> > > >
> > > > I don't think it is possible, I tried that once and couldn't get it to work.
> > > >
> > > > I am going to try write the proposal. But unfortunately PHY binding
> > > > is not converted to YAML yet :(
> > > >  
> > >
> > > I saw you recent patches to convert this. Thanks!
> > >
> > > This make my serdes.yaml obsolete then, correct? Should I then only
> > > re-post my driver code, once your patches are accepted?  
> > 
> > Yes, please let's do it this way. It may take some time for Rob to review this,
> > though, and he may require some changes.
> >   
> 
> I saw your v3 patch for the bindings and I would adapt then my patch
> accordingly to tx-p2p-microvolt. 
> 
> > Also I was thinking whether it wouldn't be better to put the property into a
> > separate SerDes PHY node, i.e.
> > 
> >   switch {
> >     compatible = "marvell,mv88e6085";
> >     ...
> > 
> >     ports {
> >       port@6 {
> >         reg = <0x6>;
> >         phy-handle = <&switch_serdes_phy>;
> >       };
> > 
> >       ...
> >     };
> > 
> >     mdio {
> >       switch_serdes_phy: ethernet-phy@f {
> >         reg = <0xf>;
> >         tx-amplitude-microvolt = <1234567>;
> >       };
> > 
> >       ...
> >     };
> >   };  
> 
> this would mean in regard to my patch instead of checking directly for the
> property in mv88e6xxx_setup_port  I would parse for the phy-handle first
> and then for the property? 
> 
> Should I wait until your patch is accepted and merged?

I don't know if Rob will be merging it, but we need to wait at least
for his review/acknowledgement. In the meantime you can prepare your
patch as RFC, though.

Also I want to add a function that will give you the voltage amplitude
given a node and a mode, i.e.

  fwnode_phy_get_tx_p2p_amplitude(fwnode, "1000base-x")