* [v3 1/2] Docs/devicetree: add serdes.yaml @ 2021-12-07 19:07 Holger Brunck 2021-12-07 19:07 ` [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable Holger Brunck 0 siblings, 1 reply; 26+ messages in thread From: Holger Brunck @ 2021-12-07 19:07 UTC (permalink / raw) To: netdev; +Cc: 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-07 19:07 [v3 1/2] Docs/devicetree: add serdes.yaml Holger Brunck @ 2021-12-07 19:07 ` Holger Brunck 2021-12-07 19:27 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Holger Brunck @ 2021-12-07 19:07 UTC (permalink / raw) To: netdev; +Cc: Holger Brunck, Andrew Lunn, Jakub Kicinski, Marek Behún 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, ®); + 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-07 19:07 ` [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable Holger Brunck @ 2021-12-07 19:27 ` Marek Behún 2021-12-08 12:29 ` Holger Brunck 0 siblings, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-07 19:27 UTC (permalink / raw) To: Holger Brunck, Andrew Lunn; +Cc: netdev, Jakub Kicinski 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-07 19:27 ` Marek Behún @ 2021-12-08 12:29 ` Holger Brunck 2021-12-08 15:28 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Holger Brunck @ 2021-12-08 12:29 UTC (permalink / raw) To: Marek Behún; +Cc: netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 12:29 ` Holger Brunck @ 2021-12-08 15:28 ` Marek Behún 2021-12-08 15:49 ` Holger Brunck 0 siblings, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-08 15:28 UTC (permalink / raw) To: Holger Brunck; +Cc: netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 15:28 ` Marek Behún @ 2021-12-08 15:49 ` Holger Brunck 2021-12-08 16:17 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Holger Brunck @ 2021-12-08 15:49 UTC (permalink / raw) To: Marek Behún; +Cc: netdev, Jakub Kicinski, Andrew Lunn > > > > > > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 15:49 ` Holger Brunck @ 2021-12-08 16:17 ` Marek Behún 2021-12-08 16:21 ` Marek Behún 2021-12-08 17:00 ` Andrew Lunn 0 siblings, 2 replies; 26+ messages in thread From: Marek Behún @ 2021-12-08 16:17 UTC (permalink / raw) To: Holger Brunck; +Cc: netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:17 ` Marek Behún @ 2021-12-08 16:21 ` Marek Behún 2021-12-08 16:41 ` Vladimir Oltean 2021-12-08 17:00 ` Andrew Lunn 1 sibling, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-08 16:21 UTC (permalink / raw) To: Vladimir Oltean; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:21 ` Marek Behún @ 2021-12-08 16:41 ` Vladimir Oltean 2021-12-08 16:49 ` Vladimir Oltean 2021-12-08 16:51 ` Marek Behún 0 siblings, 2 replies; 26+ messages in thread From: Vladimir Oltean @ 2021-12-08 16:41 UTC (permalink / raw) To: Marek Behún; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:41 ` Vladimir Oltean @ 2021-12-08 16:49 ` Vladimir Oltean 2021-12-08 17:00 ` Marek Behún 2021-12-08 16:51 ` Marek Behún 1 sibling, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2021-12-08 16:49 UTC (permalink / raw) To: Marek Behún; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:49 ` Vladimir Oltean @ 2021-12-08 17:00 ` Marek Behún 2021-12-08 17:19 ` Vladimir Oltean 0 siblings, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-08 17:00 UTC (permalink / raw) To: Holger Brunck; +Cc: Vladimir Oltean, netdev, Jakub Kicinski, Andrew Lunn 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). ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:00 ` Marek Behún @ 2021-12-08 17:19 ` Vladimir Oltean 2021-12-08 17:36 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2021-12-08 17:19 UTC (permalink / raw) To: Marek Behún; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:19 ` Vladimir Oltean @ 2021-12-08 17:36 ` Marek Behún 2021-12-08 17:55 ` Andrew Lunn 0 siblings, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-08 17:36 UTC (permalink / raw) To: Vladimir Oltean; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:36 ` Marek Behún @ 2021-12-08 17:55 ` Andrew Lunn 2021-12-08 18:15 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Andrew Lunn @ 2021-12-08 17:55 UTC (permalink / raw) To: Marek Behún; +Cc: Vladimir Oltean, Holger Brunck, netdev, Jakub Kicinski 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:55 ` Andrew Lunn @ 2021-12-08 18:15 ` Marek Behún 0 siblings, 0 replies; 26+ messages in thread From: Marek Behún @ 2021-12-08 18:15 UTC (permalink / raw) To: Andrew Lunn; +Cc: Vladimir Oltean, Holger Brunck, netdev, Jakub Kicinski 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:41 ` Vladimir Oltean 2021-12-08 16:49 ` Vladimir Oltean @ 2021-12-08 16:51 ` Marek Behún 2021-12-08 16:59 ` Vladimir Oltean 1 sibling, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-08 16:51 UTC (permalink / raw) To: Vladimir Oltean; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:51 ` Marek Behún @ 2021-12-08 16:59 ` Vladimir Oltean 2021-12-08 17:10 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2021-12-08 16:59 UTC (permalink / raw) To: Marek Behún; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:59 ` Vladimir Oltean @ 2021-12-08 17:10 ` Marek Behún 2021-12-08 17:12 ` Marek Behún 2021-12-08 17:20 ` Vladimir Oltean 0 siblings, 2 replies; 26+ messages in thread From: Marek Behún @ 2021-12-08 17:10 UTC (permalink / raw) To: Vladimir Oltean; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:10 ` Marek Behún @ 2021-12-08 17:12 ` Marek Behún 2021-12-08 17:20 ` Vladimir Oltean 1 sibling, 0 replies; 26+ messages in thread From: Marek Behún @ 2021-12-08 17:12 UTC (permalink / raw) To: Vladimir Oltean; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:10 ` Marek Behún 2021-12-08 17:12 ` Marek Behún @ 2021-12-08 17:20 ` Vladimir Oltean 1 sibling, 0 replies; 26+ messages in thread From: Vladimir Oltean @ 2021-12-08 17:20 UTC (permalink / raw) To: Marek Behún; +Cc: Holger Brunck, netdev, Jakub Kicinski, Andrew Lunn 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 16:17 ` Marek Behún 2021-12-08 16:21 ` Marek Behún @ 2021-12-08 17:00 ` Andrew Lunn 2021-12-08 17:16 ` Marek Behún 1 sibling, 1 reply; 26+ messages in thread From: Andrew Lunn @ 2021-12-08 17:00 UTC (permalink / raw) To: Marek Behún; +Cc: Holger Brunck, netdev, Jakub Kicinski > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:00 ` Andrew Lunn @ 2021-12-08 17:16 ` Marek Behún 2021-12-15 10:27 ` Holger Brunck 0 siblings, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-08 17:16 UTC (permalink / raw) To: Andrew Lunn; +Cc: Holger Brunck, netdev, Jakub Kicinski 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-08 17:16 ` Marek Behún @ 2021-12-15 10:27 ` Holger Brunck 2021-12-15 20:53 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Holger Brunck @ 2021-12-15 10:27 UTC (permalink / raw) To: Marek Behún; +Cc: netdev, Jakub Kicinski, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-15 10:27 ` Holger Brunck @ 2021-12-15 20:53 ` Marek Behún 2022-01-20 7:52 ` Holger Brunck 0 siblings, 1 reply; 26+ messages in thread From: Marek Behún @ 2021-12-15 20:53 UTC (permalink / raw) To: Holger Brunck; +Cc: netdev, Jakub Kicinski, Andrew Lunn 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>; }; ... }; }; ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2021-12-15 20:53 ` Marek Behún @ 2022-01-20 7:52 ` Holger Brunck 2022-01-20 17:57 ` Marek Behún 0 siblings, 1 reply; 26+ messages in thread From: Holger Brunck @ 2022-01-20 7:52 UTC (permalink / raw) To: Marek Behún; +Cc: netdev, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable 2022-01-20 7:52 ` Holger Brunck @ 2022-01-20 17:57 ` Marek Behún 0 siblings, 0 replies; 26+ messages in thread From: Marek Behún @ 2022-01-20 17:57 UTC (permalink / raw) To: Holger Brunck; +Cc: netdev, Andrew Lunn 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") ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-01-20 17:57 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-07 19:07 [v3 1/2] Docs/devicetree: add serdes.yaml Holger Brunck 2021-12-07 19:07 ` [v3 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable Holger Brunck 2021-12-07 19:27 ` Marek Behún 2021-12-08 12:29 ` Holger Brunck 2021-12-08 15:28 ` Marek Behún 2021-12-08 15:49 ` Holger Brunck 2021-12-08 16:17 ` Marek Behún 2021-12-08 16:21 ` Marek Behún 2021-12-08 16:41 ` Vladimir Oltean 2021-12-08 16:49 ` Vladimir Oltean 2021-12-08 17:00 ` Marek Behún 2021-12-08 17:19 ` Vladimir Oltean 2021-12-08 17:36 ` Marek Behún 2021-12-08 17:55 ` Andrew Lunn 2021-12-08 18:15 ` Marek Behún 2021-12-08 16:51 ` Marek Behún 2021-12-08 16:59 ` Vladimir Oltean 2021-12-08 17:10 ` Marek Behún 2021-12-08 17:12 ` Marek Behún 2021-12-08 17:20 ` Vladimir Oltean 2021-12-08 17:00 ` Andrew Lunn 2021-12-08 17:16 ` Marek Behún 2021-12-15 10:27 ` Holger Brunck 2021-12-15 20:53 ` Marek Behún 2022-01-20 7:52 ` Holger Brunck 2022-01-20 17:57 ` Marek Behún
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).