[PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Holger Brunck]

This can be configured from the device tree. Add this property to the
documentation accordingly.
The eight different values added in the dt-bindings file correspond to
the values we can configure on 88E6352, 88E6240 and 88E6176 switches
according to the datasheet.

CC: Andrew Lunn <andrew@lunn.ch>
CC: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
---
 .../devicetree/bindings/net/dsa/marvell.txt    |  3 +++
 include/dt-bindings/net/mv-88e6xxx.h           | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)
 create mode 100644 include/dt-bindings/net/mv-88e6xxx.h

diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
index 2363b412410c..bff397a2dc49 100644
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
@@ -46,6 +46,9 @@ Optional properties:
 - mdio?		: Container of PHYs and devices on the external MDIO
 			  bus. The node must contains a compatible string of
 			  "marvell,mv88e6xxx-mdio-external"
+- serdes-output-amplitude: Configure the output amplitude of the serdes
+			   interface.
+    serdes-output-amplitude = <MV88E6352_SERDES_OUT_AMP_210MV>;
 
 Example:
 
diff --git a/include/dt-bindings/net/mv-88e6xxx.h b/include/dt-bindings/net/mv-88e6xxx.h
new file mode 100644
index 000000000000..9bc6decaee83
--- /dev/null
+++ b/include/dt-bindings/net/mv-88e6xxx.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Device Tree constants for the Marvell 88E6XXX switch devices
+ */
+
+#ifndef _DT_BINDINGS_MV_88E6XXX_H
+#define _DT_BINDINGS_MV_88E6XXX_H
+
+#define MV88E6352_SERDES_OUT_AMP_14MV		0x0
+#define MV88E6352_SERDES_OUT_AMP_112MV		0x1
+#define MV88E6352_SERDES_OUT_AMP_210MV		0x2
+#define MV88E6352_SERDES_OUT_AMP_308MV		0x3
+#define MV88E6352_SERDES_OUT_AMP_406MV		0x4
+#define MV88E6352_SERDES_OUT_AMP_504MV		0x5
+#define MV88E6352_SERDES_OUT_AMP_602MV		0x6
+#define MV88E6352_SERDES_OUT_AMP_700MV		0x7
+
+#endif
-- 
2.34.0

[PATCH 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 switch.

CC: Andrew Lunn <andrew@lunn.ch>
CC: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c   | 14 ++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h   |  3 +++
 drivers/net/dsa/mv88e6xxx/serdes.c | 14 ++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 ++++
 4 files changed, 35 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index f00cbf5753b9..5182128959a0 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3173,9 +3173,11 @@ static void mv88e6xxx_teardown(struct dsa_switch *ds)
 static int mv88e6xxx_setup(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	struct device_node *np = chip->dev->of_node;
 	u8 cmode;
 	int err;
 	int i;
+	int out_amp;
 
 	chip->ds = ds;
 	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
@@ -3292,6 +3294,15 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	if (err)
 		goto unlock;
 
+	if (chip->info->ops->serdes_set_out_amplitude && np) {
+		if (!of_property_read_u32(np, "serdes-output-amplitude",
+					  &out_amp)) {
+			err = mv88e6352_serdes_set_out_amplitude(chip, out_amp);
+			if (err)
+				goto unlock;
+		}
+	}
+
 unlock:
 	mv88e6xxx_reg_unlock(chip);
 
@@ -4076,6 +4087,7 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.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_out_amplitude = mv88e6352_serdes_set_out_amplitude,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.phylink_validate = mv88e6352_phylink_validate,
 };
@@ -4356,6 +4368,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.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_out_amplitude = mv88e6352_serdes_set_out_amplitude,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6352_ptp_ops,
@@ -4762,6 +4775,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.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_out_amplitude = mv88e6352_serdes_set_out_amplitude,
 	.phylink_validate = mv88e6352_phylink_validate,
 };
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 8271b8aa7b71..d931039ee995 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -586,6 +586,9 @@ struct mv88e6xxx_ops {
 	void (*serdes_get_regs)(struct mv88e6xxx_chip *chip, int port,
 				void *_p);
 
+	/* SERDES SGMII/Fiber Output Amplitude */
+	int (*serdes_set_out_amplitude)(struct mv88e6xxx_chip *chip, 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 6ea003678798..835137595de5 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1271,6 +1271,20 @@ void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
 	}
 }
 
+int mv88e6352_serdes_set_out_amplitude(struct mv88e6xxx_chip *chip, int val)
+{
+	u16 reg;
+	int err;
+
+	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_port_errata(struct mv88e6xxx_chip *chip, int lane)
 {
 	u16 reg, pcs;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index cbb3ba30caea..71dddc65b642 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
 
@@ -172,6 +174,8 @@ void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p);
 int mv88e6390_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
 void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p);
 
+int mv88e6352_serdes_set_out_amplitude(struct mv88e6xxx_chip *chip, 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)
-- 
2.34.0

Re: [PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Andrew Lunn]

On Fri, Nov 26, 2021 at 04:42:48PM +0100, Holger Brunck wrote:
> This can be configured from the device tree. Add this property to the
> documentation accordingly.
> The eight different values added in the dt-bindings file correspond to
> the values we can configure on 88E6352, 88E6240 and 88E6176 switches
> according to the datasheet.
> 
> CC: Andrew Lunn <andrew@lunn.ch>
> CC: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
> ---
>  .../devicetree/bindings/net/dsa/marvell.txt    |  3 +++
>  include/dt-bindings/net/mv-88e6xxx.h           | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+)
>  create mode 100644 include/dt-bindings/net/mv-88e6xxx.h
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
> index 2363b412410c..bff397a2dc49 100644
> --- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
> @@ -46,6 +46,9 @@ Optional properties:
>  - mdio?		: Container of PHYs and devices on the external MDIO
>  			  bus. The node must contains a compatible string of
>  			  "marvell,mv88e6xxx-mdio-external"
> +- serdes-output-amplitude: Configure the output amplitude of the serdes
> +			   interface.
> +    serdes-output-amplitude = <MV88E6352_SERDES_OUT_AMP_210MV>;

Please make this actually millivolts, not an enum. Have the property
called serdes-output-amplitude-mv. The driver should convert from a mv
value to whatever value you need to write to the register, or return
-EINVAL.

	Andrew

Re: [PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Andrew Lunn]

On Fri, Nov 26, 2021 at 04:42:48PM +0100, Holger Brunck wrote:
> This can be configured from the device tree. Add this property to the
> documentation accordingly.
> The eight different values added in the dt-bindings file correspond to
> the values we can configure on 88E6352, 88E6240 and 88E6176 switches
> according to the datasheet.

This should probably be a port property, not a switch property. It
applies to the SERDES, and the SERDES belongs to a port. What you have
now only works because there is a single SERDES for this switch
family, but other switch families have multiple SERDESes.

	Andrew

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

[Andrew Lunn]

> +	if (chip->info->ops->serdes_set_out_amplitude && np) {
> +		if (!of_property_read_u32(np, "serdes-output-amplitude",
> +					  &out_amp)) {
> +			err = mv88e6352_serdes_set_out_amplitude(chip, out_amp);
> +			if (err)
> +				goto unlock;
> +		}
> +	}

If the property is in DT, but the device does not have a
ops->serdes_set_out_amplitude(), please return -EOPNOTSUP.  We want to
avoid the case somebody wrongly cut/pastes a DT fragment to a
different switch.

	  Andrew

Re: [PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Jakub Kicinski]

On Fri, 26 Nov 2021 16:42:48 +0100 Holger Brunck wrote:
> This can be configured from the device tree. Add this property to the
> documentation accordingly.
> The eight different values added in the dt-bindings file correspond to
> the values we can configure on 88E6352, 88E6240 and 88E6176 switches
> according to the datasheet.
> 
> CC: Andrew Lunn <andrew@lunn.ch>
> CC: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>

Not sure why but FWIW your patches did not show up in patchwork on in
lore. We can see Andrew's review but not the patches themselves:

https://lore.kernel.org/all/YaEIVQ6gKOSD1Vf%2F@lunn.ch/

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

[Marek Behún]

On Fri, 26 Nov 2021 16:42:49 +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 switch.
> 
> CC: Andrew Lunn <andrew@lunn.ch>
> CC: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c   | 14 ++++++++++++++
>  drivers/net/dsa/mv88e6xxx/chip.h   |  3 +++
>  drivers/net/dsa/mv88e6xxx/serdes.c | 14 ++++++++++++++
>  drivers/net/dsa/mv88e6xxx/serdes.h |  4 ++++
>  4 files changed, 35 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index f00cbf5753b9..5182128959a0 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3173,9 +3173,11 @@ static void mv88e6xxx_teardown(struct dsa_switch *ds)
>  static int mv88e6xxx_setup(struct dsa_switch *ds)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct device_node *np = chip->dev->of_node;
>  	u8 cmode;
>  	int err;
>  	int i;
> +	int out_amp;

Reverse christmas tree please, where possible.

  	struct mv88e6xxx_chip *chip = ds->priv;
+	struct device_node *np = chip->dev->of_node;
+	int out_amp;
  	u8 cmode;
  	int err;
  	int

>  
>  	chip->ds = ds;
>  	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
> @@ -3292,6 +3294,15 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>  	if (err)
>  		goto unlock;
>  
> +	if (chip->info->ops->serdes_set_out_amplitude && np) {
> +		if (!of_property_read_u32(np, "serdes-output-amplitude",

Hmm. Andrew, why don't we use <linux/property.h> instead of
<linux/of*.h> stuff in this dirver? Is there a reason or is this just
because it wasn't converted yet?

A simple device_property_read_u32() would be better here and we
wouldn't need the np pointer.

...
  
> +int mv88e6352_serdes_set_out_amplitude(struct mv88e6xxx_chip *chip, int val)
> +{
> +	u16 reg;
> +	int err;
> +
> +	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);
> +}

Is there a reason why we call this from driver probe instead of 6352's
serdes_power() ?

Marek

Re: [PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Jakub Kicinski]

On Fri, 26 Nov 2021 10:37:26 -0800 Jakub Kicinski wrote:
> On Fri, 26 Nov 2021 16:42:48 +0100 Holger Brunck wrote:
> > This can be configured from the device tree. Add this property to the
> > documentation accordingly.
> > The eight different values added in the dt-bindings file correspond to
> > the values we can configure on 88E6352, 88E6240 and 88E6176 switches
> > according to the datasheet.
> > 
> > CC: Andrew Lunn <andrew@lunn.ch>
> > CC: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>  
> 
> Not sure why but FWIW your patches did not show up in patchwork on in
> lore. We can see Andrew's review but not the patches themselves:
> 
> https://lore.kernel.org/all/YaEIVQ6gKOSD1Vf%2F@lunn.ch/

Ah, they made it thru now, disregard.

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

[Andrew Lunn]

> > +	if (chip->info->ops->serdes_set_out_amplitude && np) {
> > +		if (!of_property_read_u32(np, "serdes-output-amplitude",
> 
> Hmm. Andrew, why don't we use <linux/property.h> instead of
> <linux/of*.h> stuff in this dirver? Is there a reason or is this just
> because it wasn't converted yet?

The problem with device_property_read is that it takes a device. But
this is not actually a device scoped property, it should be considered
a port scoped property. And the port is not a device. DSA is not
likely to convert to the device API because the device API is too
limiting.

	Andrew

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

[Marek Behún]

On Fri, 26 Nov 2021 21:43:45 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > +	if (chip->info->ops->serdes_set_out_amplitude && np) {
> > > +		if (!of_property_read_u32(np, "serdes-output-amplitude",  
> > 
> > Hmm. Andrew, why don't we use <linux/property.h> instead of
> > <linux/of*.h> stuff in this dirver? Is there a reason or is this just
> > because it wasn't converted yet?  
> 
> The problem with device_property_read is that it takes a device. But
> this is not actually a device scoped property, it should be considered
> a port scoped property. And the port is not a device. DSA is not
> likely to convert to the device API because the device API is too
> limiting.

You're right, device_property_read() needs a device, and this seems
like a port-specific property. But from the patch it seems Holger is
using the switch device node:

  struct device_node *np = chip->dev->of_node;

so either this is wrong or he could use device_property API. Of course
that would need a complete conversion, with device_* or fwnode_*.
functions. I was wondering if device_* + fwnode_* functions are
preferred instead of of_* functions (since they can be used also with
ACPI, for example).

Marek

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

[Andrew Lunn]

> You're right, device_property_read() needs a device, and this seems
> like a port-specific property. But from the patch it seems Holger is
> using the switch device node:
> 
>   struct device_node *np = chip->dev->of_node;
> 
> so either this is wrong or he could use device_property API.

I already commented it should be a port property, probably to patch
1/2.

> Of course
> that would need a complete conversion, with device_* or fwnode_*.
> functions. I was wondering if device_* + fwnode_* functions are
> preferred instead of of_* functions (since they can be used also with
> ACPI, for example).

I doubt ACPI is ever going to happen for DSA. Despite the A in ACPI,
ACPI is for simple hardware, server and desktop like hardware.

   Andrew

Re: [PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Andrew Lunn]

> @@ -46,6 +46,9 @@ Optional properties:
>  - mdio?		: Container of PHYs and devices on the external MDIO
>  			  bus. The node must contains a compatible string of
>  			  "marvell,mv88e6xxx-mdio-external"
> +- serdes-output-amplitude: Configure the output amplitude of the serdes
> +			   interface.
> +    serdes-output-amplitude = <MV88E6352_SERDES_OUT_AMP_210MV>;

I checked a couple of Marvell PHY datasheets. The 1510, 1512, and 1518
have the exact same register/bits. So ideally we want DT property name
which somebody can later reuse in the Marvell PHY
driver. serdes-output-amplitude-mv seems O.K. for that.

	Andrew

RE: [PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Holger Brunck]

> On Fri, Nov 26, 2021 at 04:42:48PM +0100, Holger Brunck wrote:
> > This can be configured from the device tree. Add this property to the
> > documentation accordingly.
> > The eight different values added in the dt-bindings file correspond to
> > the values we can configure on 88E6352, 88E6240 and 88E6176 switches
> > according to the datasheet.
> >
> > CC: Andrew Lunn <andrew@lunn.ch>
> > CC: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
> > ---
> >  .../devicetree/bindings/net/dsa/marvell.txt    |  3 +++
> >  include/dt-bindings/net/mv-88e6xxx.h           | 18 ++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> >  create mode 100644 include/dt-bindings/net/mv-88e6xxx.h
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt
> > b/Documentation/devicetree/bindings/net/dsa/marvell.txt
> > index 2363b412410c..bff397a2dc49 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
> > +++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
> > @@ -46,6 +46,9 @@ Optional properties:
> >  - mdio?              : Container of PHYs and devices on the external MDIO
> >                         bus. The node must contains a compatible string of
> >                         "marvell,mv88e6xxx-mdio-external"
> > +- serdes-output-amplitude: Configure the output amplitude of the serdes
> > +                        interface.
> > +    serdes-output-amplitude = <MV88E6352_SERDES_OUT_AMP_210MV>;
> 
> Please make this actually millivolts, not an enum. Have the property called
> serdes-output-amplitude-mv. The driver should convert from a mv value to
> whatever value you need to write to the register, or return -EINVAL.
> 

ok I will change that.

Best regards
Holger

RE: [PATCH 1/2] Docs/devicetree: add serdes-output-amplitude to marvell.txt

[Holger Brunck]

> 
> On Fri, Nov 26, 2021 at 04:42:48PM +0100, Holger Brunck wrote:
> > This can be configured from the device tree. Add this property to the
> > documentation accordingly.
> > The eight different values added in the dt-bindings file correspond to
> > the values we can configure on 88E6352, 88E6240 and 88E6176 switches
> > according to the datasheet.
> 
> This should probably be a port property, not a switch property. It applies to the
> SERDES, and the SERDES belongs to a port. What you have now only works
> because there is a single SERDES for this switch family, but other switch families
> have multiple SERDESes.
> 

yes you are right it is more a port property. So I will try to parse the DT node for
the port and add the property there. But in this case I need to double check that
the specific port is the one supporting this feature. Not sure yet how to do that, 
I need to check. Also I will move the parsing out of mv88e6xxx_setup to
mv88e6xxx_setup_port then.

Best regards
Holger

RE: [PATCH 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 switch.
> >
> > CC: Andrew Lunn <andrew@lunn.ch>
> > CC: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c   | 14 ++++++++++++++
> >  drivers/net/dsa/mv88e6xxx/chip.h   |  3 +++
> >  drivers/net/dsa/mv88e6xxx/serdes.c | 14 ++++++++++++++
> > drivers/net/dsa/mv88e6xxx/serdes.h |  4 ++++
> >  4 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > b/drivers/net/dsa/mv88e6xxx/chip.c
> > index f00cbf5753b9..5182128959a0 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -3173,9 +3173,11 @@ static void mv88e6xxx_teardown(struct
> > dsa_switch *ds)  static int mv88e6xxx_setup(struct dsa_switch *ds)  {
> >       struct mv88e6xxx_chip *chip = ds->priv;
> > +     struct device_node *np = chip->dev->of_node;
> >       u8 cmode;
> >       int err;
> >       int i;
> > +     int out_amp;
> 
> Reverse christmas tree please, where possible.
> 
>         struct mv88e6xxx_chip *chip = ds->priv;
> +       struct device_node *np = chip->dev->of_node;
> +       int out_amp;
>         u8 cmode;
>         int err;
>         int
> 

ok.

> >
> >       chip->ds = ds;
> >       ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip); @@ -3292,6
> > +3294,15 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> >       if (err)
> >               goto unlock;
> >
> > +     if (chip->info->ops->serdes_set_out_amplitude && np) {
> > +             if (!of_property_read_u32(np, "serdes-output-amplitude",
> 
> Hmm. Andrew, why don't we use <linux/property.h> instead of <linux/of*.h>
> stuff in this dirver? Is there a reason or is this just because it wasn't converted
> yet?
> 
> A simple device_property_read_u32() would be better here and we wouldn't
> need the np pointer.
> 
> ...
> 
> > +int mv88e6352_serdes_set_out_amplitude(struct mv88e6xxx_chip *chip,
> > +int val) {
> > +     u16 reg;
> > +     int err;
> > +
> > +     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); }
> 
> Is there a reason why we call this from driver probe instead of 6352's
> serdes_power() ?
> 

serdes_power is always called for enable and disable. It should be better to configure
it only once. But I agree that it should be moved from chip_probe to port_setup.

Best regards
Holger