[PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[Baruch Siach]

mv88e6xxx_port_set_cmode() relies on cmode stored in struct
mv88e6xxx_port to skip cmode update when the requested value matches the
cached value. It turns out that mv88e6xxx_port_hidden_write() might
change the port cmode setting as a side effect, so we can't rely on the
cached value to determine that cmode update in not necessary.

Force cmode update in mv88e6341_port_set_cmode(), to make
serdes configuration work again. Other mv88e6xxx_port_set_cmode()
callers keep the current behaviour.

This fixes serdes configuration of the 6141 switch on SolidRun Clearfog
GT-8K.

Fixes: 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family")
Reported-by: Denis Odintsov <d.odintsov@traviangames.com>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/dsa/mv88e6xxx/port.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 7fe256c5739d..0b43c650e100 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -393,7 +393,7 @@ phy_interface_t mv88e6390x_port_max_speed_mode(int port)
 }
 
 static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
-				    phy_interface_t mode)
+				    phy_interface_t mode, bool force)
 {
 	u8 lane;
 	u16 cmode;
@@ -427,8 +427,8 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		cmode = 0;
 	}
 
-	/* cmode doesn't change, nothing to do for us */
-	if (cmode == chip->ports[port].cmode)
+	/* cmode doesn't change, nothing to do for us unless forced */
+	if (cmode == chip->ports[port].cmode && !force)
 		return 0;
 
 	lane = mv88e6xxx_serdes_get_lane(chip, port);
@@ -484,7 +484,7 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 	if (port != 9 && port != 10)
 		return -EOPNOTSUPP;
 
-	return mv88e6xxx_port_set_cmode(chip, port, mode);
+	return mv88e6xxx_port_set_cmode(chip, port, mode, false);
 }
 
 int mv88e6390_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
@@ -504,7 +504,7 @@ int mv88e6390_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		break;
 	}
 
-	return mv88e6xxx_port_set_cmode(chip, port, mode);
+	return mv88e6xxx_port_set_cmode(chip, port, mode, false);
 }
 
 static int mv88e6341_port_set_cmode_writable(struct mv88e6xxx_chip *chip,
@@ -555,7 +555,7 @@ int mv88e6341_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 	if (err)
 		return err;
 
-	return mv88e6xxx_port_set_cmode(chip, port, mode);
+	return mv88e6xxx_port_set_cmode(chip, port, mode, true);
 }
 
 int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode)
-- 
2.24.0

Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[Vivien Didelot]

Hi Baruch,

On Thu, 19 Dec 2019 11:48:22 +0200, Baruch Siach <baruch@tkos.co.il> wrote:
> mv88e6xxx_port_set_cmode() relies on cmode stored in struct
> mv88e6xxx_port to skip cmode update when the requested value matches the
> cached value. It turns out that mv88e6xxx_port_hidden_write() might
> change the port cmode setting as a side effect, so we can't rely on the
> cached value to determine that cmode update in not necessary.
> 
> Force cmode update in mv88e6341_port_set_cmode(), to make
> serdes configuration work again. Other mv88e6xxx_port_set_cmode()
> callers keep the current behaviour.
> 
> This fixes serdes configuration of the 6141 switch on SolidRun Clearfog
> GT-8K.
> 
> Fixes: 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family")
> Reported-by: Denis Odintsov <d.odintsov@traviangames.com>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Andrew,

We tend to avoid caching values in the mv88e6xxx driver the more we can and
query the hardware instead to avoid errors like this. We can consider calling a
new mv88e6xxx_port_get_cmode() helper when needed (e.g. in higher level callers
like mv88e6xxx_serdes_power() and mv88e6xxx_serdes_irq_thread_fn()) and pass
the value down to the routines previously accessing chip->ports[port].cmode,
as a new argument. I can prepare a patch doing this. What do you think?


Thanks,

	Vivien

Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[Andrew Lunn]

> Andrew,
> 
> We tend to avoid caching values in the mv88e6xxx driver the more we can and
> query the hardware instead to avoid errors like this. We can consider calling a
> new mv88e6xxx_port_get_cmode() helper when needed (e.g. in higher level callers
> like mv88e6xxx_serdes_power() and mv88e6xxx_serdes_irq_thread_fn()) and pass
> the value down to the routines previously accessing chip->ports[port].cmode,
> as a new argument. I can prepare a patch doing this. What do you think?

Hi Vivien

There is one problem area. The lower ports on the 6390X, port
2-7. They can have two different cmode values, 0x1f for internal PHY,
and 0x09 for 1000BaseX when using an SFP. The switch will swap between
external and internal depending on which gets link first. But in order
for the SFP to get link, the SERDES needs to be powered on. And we
currently decide to power it on, and enable interrupts, via the
cmode. In the normal case, this works, e.g. ports 9 and 10 of 6390,
the fibre port of 6352. The cmode of 6390X is directly writable, we
get the phy-mode from DT and write the correct value. For 6352,
strapping is used, cmode is read only, but has the correct value.

But for the lower ports of 6390X, the hardware cmode defaults to 0x1f
until the fibre gets link, and then it changes to 0x09. It is not
writable. The current generic code does write to the register, which
in this case is pointless since it is read only, and it updates the
cached value, which is R/W. Later, when the port is enabled, we look
at the cached value, and based on that, decide is the SERDES should be
powered up, interrupts enabled. At this point, the cached value is
what we have in DT, but the hardware cmode is probably still 0x1f,
internal PHY. If we were to use the hardware value, we would never
enable the SERDES and so never get link.

This is all a bit hidden in the code. So if you want to remove the
cache, you need to add something in its place. Maybe explicitly keep
the configured value in the port structure, and modify the code using
cmode so that it either looks at the actual cmode, or the configured
cmode, depending on what it is trying to achieve.

ZII devel C is a good test bed for this, port 3 has both a copper PHY
wired to an RJ45 socket, and an SFF.

      Andrew

Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[Vivien Didelot]

Hi Andrew,

On Fri, 20 Dec 2019 14:27:25 -0500, Vivien Didelot <vivien.didelot@gmail.com> wrote:
> > mv88e6xxx_port_set_cmode() relies on cmode stored in struct
> > mv88e6xxx_port to skip cmode update when the requested value matches the
> > cached value. It turns out that mv88e6xxx_port_hidden_write() might
> > change the port cmode setting as a side effect, so we can't rely on the
> > cached value to determine that cmode update in not necessary.
> > 
> > Force cmode update in mv88e6341_port_set_cmode(), to make
> > serdes configuration work again. Other mv88e6xxx_port_set_cmode()
> > callers keep the current behaviour.
> > 
> > This fixes serdes configuration of the 6141 switch on SolidRun Clearfog
> > GT-8K.
> > 
> > Fixes: 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family")
> > Reported-by: Denis Odintsov <d.odintsov@traviangames.com>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> 
> We tend to avoid caching values in the mv88e6xxx driver the more we can and
> query the hardware instead to avoid errors like this. We can consider calling a
> new mv88e6xxx_port_get_cmode() helper when needed (e.g. in higher level callers
> like mv88e6xxx_serdes_power() and mv88e6xxx_serdes_irq_thread_fn()) and pass
> the value down to the routines previously accessing chip->ports[port].cmode,
> as a new argument. I can prepare a patch doing this. What do you think?

I did not mention it, but mv88e6390x_serdes_get_lane() would access
mv88e6xxx_port_get_cmode(chip, 9) and mv88e6xxx_port_get_cmode(chip, 10)
internally since its implementation is specific.


Thanks,

	Vivien

Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[Baruch Siach]

Hi Vivien,

On Fri, Dec 20 2019, Vivien Didelot wrote:
> On Thu, 19 Dec 2019 11:48:22 +0200, Baruch Siach <baruch@tkos.co.il> wrote:
>> mv88e6xxx_port_set_cmode() relies on cmode stored in struct
>> mv88e6xxx_port to skip cmode update when the requested value matches the
>> cached value. It turns out that mv88e6xxx_port_hidden_write() might
>> change the port cmode setting as a side effect, so we can't rely on the
>> cached value to determine that cmode update in not necessary.
>> 
>> Force cmode update in mv88e6341_port_set_cmode(), to make
>> serdes configuration work again. Other mv88e6xxx_port_set_cmode()
>> callers keep the current behaviour.
>> 
>> This fixes serdes configuration of the 6141 switch on SolidRun Clearfog
>> GT-8K.
>> 
>> Fixes: 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family")
>> Reported-by: Denis Odintsov <d.odintsov@traviangames.com>
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>
> Andrew,
>
> We tend to avoid caching values in the mv88e6xxx driver the more we can and
> query the hardware instead to avoid errors like this. We can consider calling a
> new mv88e6xxx_port_get_cmode() helper when needed (e.g. in higher level callers
> like mv88e6xxx_serdes_power() and mv88e6xxx_serdes_irq_thread_fn()) and pass
> the value down to the routines previously accessing chip->ports[port].cmode,
> as a new argument. I can prepare a patch doing this. What do you think?

I'm not sure that cmode read would always give a valid value. On 6141 I
see an invalid 0x6 value after mv88e6xxx_port_hidden_write().

As I understand, this cache elimination work would target v5.6+. Would
you ack this patch for v5.5-rc to fix currently broken setup?

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[David Miller]

From: Baruch Siach <baruch@tkos.co.il>
Date: Thu, 19 Dec 2019 11:48:22 +0200

> mv88e6xxx_port_set_cmode() relies on cmode stored in struct
> mv88e6xxx_port to skip cmode update when the requested value matches the
> cached value. It turns out that mv88e6xxx_port_hidden_write() might
> change the port cmode setting as a side effect, so we can't rely on the
> cached value to determine that cmode update in not necessary.
> 
> Force cmode update in mv88e6341_port_set_cmode(), to make
> serdes configuration work again. Other mv88e6xxx_port_set_cmode()
> callers keep the current behaviour.
> 
> This fixes serdes configuration of the 6141 switch on SolidRun Clearfog
> GT-8K.
> 
> Fixes: 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family")
> Reported-by: Denis Odintsov <d.odintsov@traviangames.com>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

This thread has stalled on December 20th with Baruch asking if we can put this
in for now as a temporary fix that solves the given problem whilst a better
long term analysis and change is being worked on.

Vivien/Andrew/etc. please follow up with either a NACK or ACK.

Thanks.

Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[Andrew Lunn]

On Thu, Jan 02, 2020 at 12:45:56PM -0800, David Miller wrote:
> From: Baruch Siach <baruch@tkos.co.il>
> Date: Thu, 19 Dec 2019 11:48:22 +0200
> 
> > mv88e6xxx_port_set_cmode() relies on cmode stored in struct
> > mv88e6xxx_port to skip cmode update when the requested value matches the
> > cached value. It turns out that mv88e6xxx_port_hidden_write() might
> > change the port cmode setting as a side effect, so we can't rely on the
> > cached value to determine that cmode update in not necessary.
> > 
> > Force cmode update in mv88e6341_port_set_cmode(), to make
> > serdes configuration work again. Other mv88e6xxx_port_set_cmode()
> > callers keep the current behaviour.
> > 
> > This fixes serdes configuration of the 6141 switch on SolidRun Clearfog
> > GT-8K.
> > 
> > Fixes: 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family")
> > Reported-by: Denis Odintsov <d.odintsov@traviangames.com>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> 
> This thread has stalled on December 20th with Baruch asking if we can put this
> in for now as a temporary fix that solves the given problem whilst a better
> long term analysis and change is being worked on.

Hi David

It seems like a reasonable fix for the moment.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 2 Jan 2020 23:36:41 +0100

> On Thu, Jan 02, 2020 at 12:45:56PM -0800, David Miller wrote:
>> From: Baruch Siach <baruch@tkos.co.il>
>> Date: Thu, 19 Dec 2019 11:48:22 +0200
>> 
>> > mv88e6xxx_port_set_cmode() relies on cmode stored in struct
>> > mv88e6xxx_port to skip cmode update when the requested value matches the
>> > cached value. It turns out that mv88e6xxx_port_hidden_write() might
>> > change the port cmode setting as a side effect, so we can't rely on the
>> > cached value to determine that cmode update in not necessary.
>> > 
>> > Force cmode update in mv88e6341_port_set_cmode(), to make
>> > serdes configuration work again. Other mv88e6xxx_port_set_cmode()
>> > callers keep the current behaviour.
>> > 
>> > This fixes serdes configuration of the 6141 switch on SolidRun Clearfog
>> > GT-8K.
>> > 
>> > Fixes: 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family")
>> > Reported-by: Denis Odintsov <d.odintsov@traviangames.com>
>> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> 
>> This thread has stalled on December 20th with Baruch asking if we can put this
>> in for now as a temporary fix that solves the given problem whilst a better
>> long term analysis and change is being worked on.
> 
> Hi David
> 
> It seems like a reasonable fix for the moment.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thanks Andrew.

Applied and queued up for -stable.