[PATCH net-next v2 0/2] mv88e6xxx: SERDES on mv88e6321 letter

[PATCH net-next v2 0/2] mv88e6xxx: SERDES on mv88e6321 letter

[Fidan Aliyeva]

This patch series add code support to be able to use SERDES feature of
mv88e6321 version of Marvel mv88e6xxx series. mv88e6321 has 2 ports to
support high speed SERDES but the support is lacking in the driver.

mv88e6321 version has a similar architecture to mv88e6352 version making it
possible to reuse its pcs functions. That's why the patch series consist of
2 parts:
1. Refactor the serdes functions and pcs_init of mv88e6352 to be more
generic
2. Add the SERDES support for mv88e6321 reusing 6352's pcs functions

The final code has been built on top of net-next tree and tested on
mv88e6321 ethernet device directly by ip ping tests, performance tests and
also verifying the switch's expected register values.

Referred document: 88E6321/88E6320 Functional Specification

Code has been built with allmodconfig and allyesconfig. checkpatch.pl was
also run

---
Changes in v2:
  - Removed 6321-specific pcs_init and made 6352's pcs_init more generic
  as suggested by Andrew Lunn
  - Added the correct mailing list

---
Fidan Aliyeva (2):
  mv88e6xxx: Refactor 6352's serdes functions
  mv88e6xxx: Add SERDES Support for mv88e6321

 drivers/net/dsa/mv88e6xxx/chip.c     |  8 +++
 drivers/net/dsa/mv88e6xxx/pcs-6352.c | 12 ++--
 drivers/net/dsa/mv88e6xxx/serdes.c   | 87 ++++++++++++++++++++++------
 drivers/net/dsa/mv88e6xxx/serdes.h   |  5 ++
 4 files changed, 86 insertions(+), 26 deletions(-)

--
2.36.0

[PATCH net-next v2 1/2] mv88e6xxx: Refactor 6352's serdes functions

[Fidan Aliyeva]

This is a preparation patch for adding SERDES support for mv88e6321
version of the ethernet switch. The patch aims to make
mv88e6352_pcs_init, as well as some of serdes functions, more generic
so they can be reused for mv88e6321.

Changes:
1. Add mv88e6352_serdes_get_lane function which checks if the port
supports SERDES by reading G2 global scratch register. Then returns
the address of the SERDES lane.
2. Add this function as .serdes_get_lane member to all the chip
versions which use mv88e6352_pcs_init.
3. Replace serdes check by global register in mv88e6352_pcs_init
function by mv88e6xxx_serdes_get_lane function making it more generic.
4. Replace serdes check in mv88e6352_serdes_get_regs_len by
mv88e6xxx_serdes_get_lane. This function is the only other function which
checks the scratch register with lock; thus, can call
mv88e6xxx_serdes_get_lane instead.
5. Add mv88e6352_serdes_get_regs_from_lane helper function and refactor
mv88e6352_serdes_get_regs to call this function with MV88E6352_ADDR_SERDES.
The helper function will later be reused for 6321.
mv88e6352_serdes_get_regs itself is not reused because
mv886exxx_serdes_get_lane cannot be called inside 6352's serdes_get_regs.
The reason is 6352's serdes_get_lane requires register lock which has
already been locked by the serdes_get_regs's caller function.
6. Add lane argument to mv88e6352_serdes_read so it can be reused later for
6321.

Co-developed-by: Thomas Eckerman <thomas.eckerman.ext@ericsson.com>
Signed-off-by: Thomas Eckerman <thomas.eckerman.ext@ericsson.com>
Signed-off-by: Fidan Aliyeva <fidan.aliyeva.ext@ericsson.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c     |  4 ++
 drivers/net/dsa/mv88e6xxx/pcs-6352.c | 12 +++---
 drivers/net/dsa/mv88e6xxx/serdes.c   | 58 +++++++++++++++++++---------
 drivers/net/dsa/mv88e6xxx/serdes.h   |  1 +
 4 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8ca5fd40df92..15a8028a02a8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4662,6 +4662,7 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.stu_getnext = mv88e6352_g1_stu_getnext,
 	.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
+	.serdes_get_lane = mv88e6352_serdes_get_lane,
 	.serdes_get_regs_len = mv88e6352_serdes_get_regs_len,
 	.serdes_get_regs = mv88e6352_serdes_get_regs,
 	.gpio_ops = &mv88e6352_gpio_ops,
@@ -4765,6 +4766,7 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.stu_getnext = mv88e6352_g1_stu_getnext,
 	.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
+	.serdes_get_lane = mv88e6352_serdes_get_lane,
 	.serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
 	.serdes_get_regs_len = mv88e6352_serdes_get_regs_len,
 	.serdes_get_regs = mv88e6352_serdes_get_regs,
@@ -5040,6 +5042,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.stu_getnext = mv88e6352_g1_stu_getnext,
 	.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
+	.serdes_get_lane = mv88e6352_serdes_get_lane,
 	.serdes_irq_mapping = mv88e6352_serdes_irq_mapping,
 	.serdes_get_regs_len = mv88e6352_serdes_get_regs_len,
 	.serdes_get_regs = mv88e6352_serdes_get_regs,
@@ -5475,6 +5478,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6352_ptp_ops,
+	.serdes_get_lane = mv88e6352_serdes_get_lane,
 	.serdes_get_sset_count = mv88e6352_serdes_get_sset_count,
 	.serdes_get_strings = mv88e6352_serdes_get_strings,
 	.serdes_get_stats = mv88e6352_serdes_get_stats,
diff --git a/drivers/net/dsa/mv88e6xxx/pcs-6352.c b/drivers/net/dsa/mv88e6xxx/pcs-6352.c
index 9ebf0f89f817..4228ae5bb9db 100644
--- a/drivers/net/dsa/mv88e6xxx/pcs-6352.c
+++ b/drivers/net/dsa/mv88e6xxx/pcs-6352.c
@@ -324,19 +324,17 @@ static int mv88e6352_pcs_init(struct mv88e6xxx_chip *chip, int port)
 	struct mii_bus *bus;
 	struct device *dev;
 	unsigned int irq;
-	int err;
+	int lane, err;

-	mv88e6xxx_reg_lock(chip);
-	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
-	mv88e6xxx_reg_unlock(chip);
-	if (err <= 0)
-		return err;
+	lane = mv88e6xxx_serdes_get_lane(chip, port);
+	if (lane < 0)
+		return 0;

 	irq = mv88e6xxx_serdes_irq_mapping(chip, port);
 	bus = mv88e6xxx_default_mdio_bus(chip);
 	dev = chip->dev;

-	mpcs = marvell_c22_pcs_alloc(dev, bus, MV88E6352_ADDR_SERDES);
+	mpcs = marvell_c22_pcs_alloc(dev, bus, lane);
 	if (!mpcs)
 		return -ENOMEM;

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index a936ee80ce00..273c3b169f65 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -17,10 +17,10 @@
 #include "port.h"
 #include "serdes.h"

-static int mv88e6352_serdes_read(struct mv88e6xxx_chip *chip, int reg,
-				 u16 *val)
+static int mv88e6352_serdes_read(struct mv88e6xxx_chip *chip, int lane,
+				 int reg, u16 *val)
 {
-	return mv88e6xxx_phy_page_read(chip, MV88E6352_ADDR_SERDES,
+	return mv88e6xxx_phy_page_read(chip, lane,
 				       MV88E6352_SERDES_PAGE_FIBER,
 				       reg, val);
 }
@@ -102,6 +102,21 @@ int mv88e6xxx_pcs_decode_state(struct device *dev, u16 bmsr, u16 lpa,
 	return 0;
 }

+int mv88e6352_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
+{
+	int err;
+
+	mv88e6xxx_reg_lock(chip);
+	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
+	mv88e6xxx_reg_unlock(chip);
+	if (err < 0)
+		return err;
+	else if (err == 0)
+		return -ENODEV;
+
+	return MV88E6352_ADDR_SERDES;
+}
+
 struct mv88e6352_serdes_hw_stat {
 	char string[ETH_GSTRING_LEN];
 	int sizeof_stat;
@@ -141,14 +156,14 @@ int mv88e6352_serdes_get_strings(struct mv88e6xxx_chip *chip, int port,
 	return ARRAY_SIZE(mv88e6352_serdes_hw_stats);
 }

-static uint64_t mv88e6352_serdes_get_stat(struct mv88e6xxx_chip *chip,
+static uint64_t mv88e6352_serdes_get_stat(struct mv88e6xxx_chip *chip, int lane,
 					  struct mv88e6352_serdes_hw_stat *stat)
 {
 	u64 val = 0;
 	u16 reg;
 	int err;

-	err = mv88e6352_serdes_read(chip, stat->reg, &reg);
+	err = mv88e6352_serdes_read(chip, lane, stat->reg, &reg);
 	if (err) {
 		dev_err(chip->dev, "failed to read statistic\n");
 		return 0;
@@ -157,7 +172,7 @@ static uint64_t mv88e6352_serdes_get_stat(struct mv88e6xxx_chip *chip,
 	val = reg;

 	if (stat->sizeof_stat == 32) {
-		err = mv88e6352_serdes_read(chip, stat->reg + 1, &reg);
+		err = mv88e6352_serdes_read(chip, lane, stat->reg + 1, &reg);
 		if (err) {
 			dev_err(chip->dev, "failed to read statistic\n");
 			return 0;
@@ -185,7 +200,7 @@ size_t mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,

 	for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_hw_stats); i++) {
 		stat = &mv88e6352_serdes_hw_stats[i];
-		value = mv88e6352_serdes_get_stat(chip, stat);
+		value = mv88e6352_serdes_get_stat(chip, MV88E6352_ADDR_SERDES, stat);
 		mv88e6xxx_port->serdes_stats[i] += value;
 		data[i] = mv88e6xxx_port->serdes_stats[i];
 	}
@@ -200,35 +215,40 @@ unsigned int mv88e6352_serdes_irq_mapping(struct mv88e6xxx_chip *chip, int port)

 int mv88e6352_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port)
 {
-	int err;
+	int lane = -ENODEV;

-	mv88e6xxx_reg_lock(chip);
-	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
-	mv88e6xxx_reg_unlock(chip);
-	if (err <= 0)
-		return err;
+	lane = mv88e6xxx_serdes_get_lane(chip, port);
+	if (lane < 0)
+		return 0;

 	return 32 * sizeof(u16);
 }

-void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
+static void mv88e6352_serdes_get_regs_from_lane(struct mv88e6xxx_chip *chip, int lane, void *_p)
 {
 	u16 *p = _p;
 	u16 reg;
 	int err;
 	int i;

-	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
-	if (err <= 0)
-		return;
-
 	for (i = 0 ; i < 32; i++) {
-		err = mv88e6352_serdes_read(chip, i, &reg);
+		err = mv88e6352_serdes_read(chip, lane, i, &reg);
 		if (!err)
 			p[i] = reg;
 	}
 }

+void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
+{
+	int err;
+
+	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
+	if (err <= 0)
+		return;
+
+	mv88e6352_serdes_get_regs_from_lane(chip, MV88E6352_ADDR_SERDES, _p);
+}
+
 int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
 {
 	u8 cmode = chip->ports[port].cmode;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 17a3e85fabaa..21e050b328cc 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -115,6 +115,7 @@ int mv88e6xxx_pcs_decode_state(struct device *dev, u16 bmsr, u16 lpa,
 			       u16 status, struct phylink_link_state *state);

 int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
+int mv88e6352_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
 int mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
 int mv88e6390x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
 int mv88e6393x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
--
2.36.0

[PATCH net-next v2 2/2] mv88e6xxx: Add SERDES Support for mv88e6321

[Fidan Aliyeva]

Add serdes and pcs_ops functions for mv88e6321. In mv88e6321
2 ports support serdes functionality; port 0 and port 1. These ports are
serdes-only ports.

Changes:

1. Add a function support to return the lane address for the port based on
cmode. Once that in place, reuse mv88e6352's serdes_get_regs_len and
pcs_init functions for mv88e6321.
2. Add mv88e6321_serdes_get_regs function by reusing
mv88e6352_serdes_get_regs_from_lane

Tested on mv88e6321 switch port 0.
Builds were done with allyesconfig and allmodconfig W=1.

Co-developed-by: Thomas Eckerman <thomas.eckerman.ext@ericsson.com>
Signed-off-by: Thomas Eckerman <thomas.eckerman.ext@ericsson.com>
Signed-off-by: Fidan Aliyeva <fidan.aliyeva.ext@ericsson.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c   |  4 ++++
 drivers/net/dsa/mv88e6xxx/serdes.c | 29 +++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 ++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 15a8028a02a8..c1f7fa21d964 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5259,10 +5259,14 @@ static const struct mv88e6xxx_ops mv88e6321_ops = {
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.stu_getnext = mv88e6352_g1_stu_getnext,
 	.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
+	.serdes_get_lane = mv88e6321_serdes_get_lane,
+	.serdes_get_regs_len = mv88e6352_serdes_get_regs_len,
+	.serdes_get_regs = mv88e6321_serdes_get_regs,
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6352_ptp_ops,
 	.phylink_get_caps = mv88e632x_phylink_get_caps,
+	.pcs_ops = &mv88e6352_pcs_ops,
 };

 static const struct mv88e6xxx_ops mv88e6341_ops = {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 273c3b169f65..3f8c178cf843 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -249,6 +249,35 @@ void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
 	mv88e6352_serdes_get_regs_from_lane(chip, MV88E6352_ADDR_SERDES, _p);
 }

+int mv88e6321_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
+{
+	int lane = -ENODEV;
+	u8 cmode;
+
+	if (port != 0 && port != 1)
+		return lane;
+
+	cmode = chip->ports[port].cmode;
+
+	if (cmode == MV88E6XXX_PORT_STS_CMODE_100BASEX ||
+	    cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
+	    cmode == MV88E6XXX_PORT_STS_CMODE_SGMII)
+		lane = port + MV88E6321_PORT0_LANE;
+
+	return lane;
+}
+
+void mv88e6321_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
+{
+	int lane;
+
+	lane = mv88e6xxx_serdes_get_lane(chip, port);
+	if (lane < 0)
+		return;
+
+	mv88e6352_serdes_get_regs_from_lane(chip, lane, _p);
+}
+
 int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
 {
 	u8 cmode = chip->ports[port].cmode;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 21e050b328cc..c11ea2118efc 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -14,6 +14,8 @@

 struct phylink_link_state;

+#define MV88E6321_PORT0_LANE		0x0c
+
 #define MV88E6352_ADDR_SERDES		0x0f
 #define MV88E6352_SERDES_PAGE_FIBER	0x01
 #define MV88E6352_SERDES_IRQ		0x0b
@@ -114,6 +116,7 @@ struct phylink_link_state;
 int mv88e6xxx_pcs_decode_state(struct device *dev, u16 bmsr, u16 lpa,
 			       u16 status, struct phylink_link_state *state);

+int mv88e6321_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
 int mv88e6341_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
 int mv88e6352_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
 int mv88e6390_serdes_get_lane(struct mv88e6xxx_chip *chip, int port);
@@ -134,6 +137,7 @@ int mv88e6390_serdes_get_strings(struct mv88e6xxx_chip *chip, int port,
 size_t mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
 				  uint64_t *data);

+void mv88e6321_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p);
 int mv88e6352_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
 void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p);
 int mv88e6390_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
--
2.36.0

Re: [PATCH net-next v2 1/2] mv88e6xxx: Refactor 6352's serdes functions

[Andrew Lunn]

> @@ -185,7 +200,7 @@ size_t mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
> 
>  	for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_hw_stats); i++) {
>  		stat = &mv88e6352_serdes_hw_stats[i];
> -		value = mv88e6352_serdes_get_stat(chip, stat);
> +		value = mv88e6352_serdes_get_stat(chip, MV88E6352_ADDR_SERDES, stat);

If you generalise this, you can use the same code for the mv88e6321.

> +void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
> +{
> +	int err;
> +
> +	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
> +	if (err <= 0)
> +		return;
> +
> +	mv88e6352_serdes_get_regs_from_lane(chip, MV88E6352_ADDR_SERDES, _p);

Here as well.

This is however looking a lot better. Thanks for the generalisation
patch.

    Andrew

---
pw-bot: cr

Re: Re: [PATCH net-next v2 1/2] mv88e6xxx: Refactor 6352's serdes functions

[Fidan Aliyeva]

> > @@ -185,7 +200,7 @@ size_t mv88e6352_serdes_get_stats(struct 
> > mv88e6xxx_chip *chip, int port,
> >
> >       for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_hw_stats); i++) {
> >               stat = &mv88e6352_serdes_hw_stats[i];
> > -             value = mv88e6352_serdes_get_stat(chip, stat);
> > +             value = mv88e6352_serdes_get_stat(chip, 
> > + MV88E6352_ADDR_SERDES, stat);
> 
> If you generalise this, you can use the same code for the mv88e6321.
> 
> > +void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, 
> > +void *_p) {
> > +     int err;
> > +
> > +     err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
> > +     if (err <= 0)
> > +             return;
> > +
> > +     mv88e6352_serdes_get_regs_from_lane(chip, MV88E6352_ADDR_SERDES, 
> > + _p);
> 
> Here as well.

Hi, Andrew. Thank you for your review and feedback on both this and
previous version.

I wanted to make those functions generic and not introduce new functions
other than 6321_serdes_get_lane. However, those functions cannot be
generalised the obvious way because they run with reg_lock already taken
which would cause deadlock in mv88e6352_serdes_get_lane function.

Do you have proposal on how that problem can be avoided to make these
functions generic, too?

Best,
Fidan

Re: Re: [PATCH net-next v2 1/2] mv88e6xxx: Refactor 6352's serdes functions

[Andrew Lunn]

> I wanted to make those functions generic and not introduce new functions
> other than 6321_serdes_get_lane. However, those functions cannot be
> generalised the obvious way because they run with reg_lock already taken
> which would cause deadlock in mv88e6352_serdes_get_lane function.

Ah, the scratch register. None of the other serdes_get_lane()
functions need to read a register. O.K.

So we don't expect the scratch register to change at runtime do we?

Nope, the value in it is read during reset. After that, it does not
matter what happens to the pin, the value in the scratch register is
fixed. So maybe read it during mv88e6xxx_setup_port() and store the
value in struct mv88e6xxx_port?

      Andrew

Re: Re: [PATCH net-next v2 1/2] mv88e6xxx: Refactor 6352's serdes functions

[Fidan Aliyeva]

> > I wanted to make those functions generic and not introduce new 
> > functions other than 6321_serdes_get_lane. However, those functions 
> > cannot be generalised the obvious way because they run with reg_lock 
> > already taken which would cause deadlock in mv88e6352_serdes_get_lane function.
> 
> Ah, the scratch register. None of the other serdes_get_lane() functions need to read a register. O.K.
> 
> So we don't expect the scratch register to change at runtime do we?
> 
> Nope, the value in it is read during reset. After that, it does not matter what happens to the pin, the value in the scratch register is fixed. So maybe read it during mv88e6xxx_setup_port() and store the value in struct mv88e6xxx_port?

Hi again. Thank you for your feedback. I can make the proposed change. But
I do not have the functional specification for 6352 to refer the change
to, neither do I have access to a 6352 to be able to test the change on.
Do you have the functional specification document of the switch, maybe?

Thanks,
Fidan.

Re: Re: [PATCH net-next v2 1/2] mv88e6xxx: Refactor 6352's serdes functions

[Andrew Lunn]

On Sat, May 02, 2026 at 11:25:52AM +0200, Fidan Aliyeva wrote:
> > > I wanted to make those functions generic and not introduce new 
> > > functions other than 6321_serdes_get_lane. However, those functions 
> > > cannot be generalised the obvious way because they run with reg_lock 
> > > already taken which would cause deadlock in mv88e6352_serdes_get_lane function.
> > 
> > Ah, the scratch register. None of the other serdes_get_lane() functions need to read a register. O.K.
> > 
> > So we don't expect the scratch register to change at runtime do we?
> > 
> > Nope, the value in it is read during reset. After that, it does not matter what happens to the pin, the value in the scratch register is fixed. So maybe read it during mv88e6xxx_setup_port() and store the value in struct mv88e6xxx_port?
> 
> Hi again. Thank you for your feedback. I can make the proposed change. But
> I do not have the functional specification for 6352 to refer the change
> to, neither do I have access to a 6352 to be able to test the change on.
> Do you have the functional specification document of the switch, maybe?

I have a 6352 board i can test with. It does not have any fibre ports,
but i can test it does not deadlock, or give an error message about
the lock not being held.

  Andrew

Re: Re: [PATCH net-next v2 1/2] mv88e6xxx: Refactor 6352's serdes functions

[Fidan Aliyeva]

> > > > I wanted to make those functions generic and not introduce new 
> > > > functions other than 6321_serdes_get_lane. However, those 
> > > > functions cannot be generalised the obvious way because they run 
> > > > with reg_lock already taken which would cause deadlock in mv88e6352_serdes_get_lane function.
> > >
> > > Ah, the scratch register. None of the other serdes_get_lane() functions need to read a register. O.K.
> > >
> > > So we don't expect the scratch register to change at runtime do we?
> > >
> > > Nope, the value in it is read during reset. After that, it does not matter what happens to the pin, the value in the scratch register is fixed. So maybe read it during mv88e6xxx_setup_port() and store the value in struct mv88e6xxx_port?
> >
> > Hi again. Thank you for your feedback. I can make the proposed change. 
> > But I do not have the functional specification for 6352 to refer the 
> > change to, neither do I have access to a 6352 to be able to test the change on.
> > Do you have the functional specification document of the switch, maybe?
>
> I have a 6352 board i can test with. It does not have any fibre ports, but i can test it does not deadlock, or give an error message about the lock not being held.

That is kind of you. We appreciate your help, thank you!

We will prepare the patch and send it for RFC.

Best,
Fidan

Re: [PATCH net-next v2 0/2] mv88e6xxx: SERDES on mv88e6321 letter

[Marek Behún]

Hello Fidan, Andrew,

last year I worked on 6320+6321 serdes support, but never send it
due to different workload.

Quickly looking at your patches, there are some things I think are
missing in comparison to what I did - I have 5 patches:

1/5: drops serdes methods for 88E6172, which does not have serdes

2/5: is similar to your patch 1/2, adding serdes_get_lane to 6352 family.
     The difference is the implementation of mv88e6352_serdes_get_lane(),
     which does not read scratch register. Yours is probably better in
     this regard, but mv88e6352_g2_scratch_port_has_serdes() is also
     called in mv88e6352_pcs_init(). So maybe this is redundant?

     Also, I rename MV88E6352_ADDR_SERDES macro to MV88E6352_SERDES_LANE
     to be in line with other such macros in serdes.h

3/5: extends 6352 serdes pcs for 6320 family.
     In addition to your code I also implement .serdes_irq_mapping().

     These depend on other mv88e6xxx fixes I forgot to send, which I
     will do now.

4/5: Add hidden register access methods for 6320 and 6352 family.
     These can be used to change SerDes modes.

5/5: Add support for changing between sgmii and 1000base-x on 6320 family
     via hidden register acces.

     This was done by reverse engineering hidden registers (trying
     different values), since it is not documented correctly.

Would you prefer for your patches to be applied first and then I can
rebase, or would you rather like to look at my code first?

Marek

On Thu, Apr 30, 2026 at 02:49:05PM +0200, Fidan Aliyeva wrote:
> This patch series add code support to be able to use SERDES feature of
> mv88e6321 version of Marvel mv88e6xxx series. mv88e6321 has 2 ports to
> support high speed SERDES but the support is lacking in the driver.
> 
> mv88e6321 version has a similar architecture to mv88e6352 version making it
> possible to reuse its pcs functions. That's why the patch series consist of
> 2 parts:
> 1. Refactor the serdes functions and pcs_init of mv88e6352 to be more
> generic
> 2. Add the SERDES support for mv88e6321 reusing 6352's pcs functions
> 
> The final code has been built on top of net-next tree and tested on
> mv88e6321 ethernet device directly by ip ping tests, performance tests and
> also verifying the switch's expected register values.
> 
> Referred document: 88E6321/88E6320 Functional Specification
> 
> Code has been built with allmodconfig and allyesconfig. checkpatch.pl was
> also run
> 
> ---
> Changes in v2:
>   - Removed 6321-specific pcs_init and made 6352's pcs_init more generic
>   as suggested by Andrew Lunn
>   - Added the correct mailing list
> 
> ---
> Fidan Aliyeva (2):
>   mv88e6xxx: Refactor 6352's serdes functions
>   mv88e6xxx: Add SERDES Support for mv88e6321
> 
>  drivers/net/dsa/mv88e6xxx/chip.c     |  8 +++
>  drivers/net/dsa/mv88e6xxx/pcs-6352.c | 12 ++--
>  drivers/net/dsa/mv88e6xxx/serdes.c   | 87 ++++++++++++++++++++++------
>  drivers/net/dsa/mv88e6xxx/serdes.h   |  5 ++
>  4 files changed, 86 insertions(+), 26 deletions(-)
> 
> --
> 2.36.0
> 
>