From: Gustav Ekelund <gustaek@axis.com>

The watchdog event bits are not cleared during SW reset in the mv88e6393x
switch. This causes one event to be handled over and over again.

Explicitly clear the watchdog event register to 0 after the SW reset.

Signed-off-by: Gustav Ekelund <gustaek@axis.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c    |  2 +-
 drivers/net/dsa/mv88e6xxx/global2.c | 17 +++++++++++++++++
 drivers/net/dsa/mv88e6xxx/global2.h |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 30383c4f8fd0..ee22d4785e9e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5596,7 +5596,7 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 	 * .port_set_upstream_port method.
 	 */
 	.set_egress_port = mv88e6393x_set_egress_port,
-	.watchdog_ops = &mv88e6390_watchdog_ops,
+	.watchdog_ops = &mv88e6393x_watchdog_ops,
 	.mgmt_rsvd2cpu = mv88e6393x_port_mgmt_rsvd2cpu,
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index ed3b2f88e783..bef8297d4f78 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -943,6 +943,23 @@ const struct mv88e6xxx_irq_ops mv88e6390_watchdog_ops = {
 	.irq_free = mv88e6390_watchdog_free,
 };
 
+static int mv88e6393x_watchdog_action(struct mv88e6xxx_chip *chip, int irq)
+{
+	mv88e6390_watchdog_action(chip, irq);
+
+	mv88e6xxx_g2_write(chip, MV88E6390_G2_WDOG_CTL,
+			   MV88E6390_G2_WDOG_CTL_UPDATE |
+			   MV88E6390_G2_WDOG_CTL_PTR_EVENT);
+
+	return IRQ_HANDLED;
+}
+
+const struct mv88e6xxx_irq_ops mv88e6393x_watchdog_ops = {
+	.irq_action = mv88e6393x_watchdog_action,
+	.irq_setup = mv88e6390_watchdog_setup,
+	.irq_free = mv88e6390_watchdog_free,
+};
+
 static irqreturn_t mv88e6xxx_g2_watchdog_thread_fn(int irq, void *dev_id)
 {
 	struct mv88e6xxx_chip *chip = dev_id;
diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h
index e973114d6890..7e091965582b 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.h
+++ b/drivers/net/dsa/mv88e6xxx/global2.h
@@ -369,6 +369,7 @@ int mv88e6xxx_g2_device_mapping_write(struct mv88e6xxx_chip *chip, int target,
 extern const struct mv88e6xxx_irq_ops mv88e6097_watchdog_ops;
 extern const struct mv88e6xxx_irq_ops mv88e6250_watchdog_ops;
 extern const struct mv88e6xxx_irq_ops mv88e6390_watchdog_ops;
+extern const struct mv88e6xxx_irq_ops mv88e6393x_watchdog_ops;
 
 extern const struct mv88e6xxx_avb_ops mv88e6165_avb_ops;
 extern const struct mv88e6xxx_avb_ops mv88e6352_avb_ops;
-- 
2.30.2

On Tue, Mar 28, 2023 at 01:55:11PM +0200, Gustav Ekelund wrote:
> From: Gustav Ekelund <gustaek@axis.com>
> 
> The watchdog event bits are not cleared during SW reset in the mv88e6393x
> switch. This causes one event to be handled over and over again.
> 
> Explicitly clear the watchdog event register to 0 after the SW reset.
> 
> Signed-off-by: Gustav Ekelund <gustaek@axis.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c    |  2 +-
>  drivers/net/dsa/mv88e6xxx/global2.c | 17 +++++++++++++++++
>  drivers/net/dsa/mv88e6xxx/global2.h |  1 +
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 30383c4f8fd0..ee22d4785e9e 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -5596,7 +5596,7 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
>  	 * .port_set_upstream_port method.
>  	 */
>  	.set_egress_port = mv88e6393x_set_egress_port,
> -	.watchdog_ops = &mv88e6390_watchdog_ops,
> +	.watchdog_ops = &mv88e6393x_watchdog_ops,
>  	.mgmt_rsvd2cpu = mv88e6393x_port_mgmt_rsvd2cpu,
>  	.pot_clear = mv88e6xxx_g2_pot_clear,
>  	.reset = mv88e6352_g1_reset,
> diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
> index ed3b2f88e783..bef8297d4f78 100644
> --- a/drivers/net/dsa/mv88e6xxx/global2.c
> +++ b/drivers/net/dsa/mv88e6xxx/global2.c
> @@ -943,6 +943,23 @@ const struct mv88e6xxx_irq_ops mv88e6390_watchdog_ops = {
>  	.irq_free = mv88e6390_watchdog_free,
>  };
>  
> +static int mv88e6393x_watchdog_action(struct mv88e6xxx_chip *chip, int irq)
> +{
> +	mv88e6390_watchdog_action(chip, irq);
> +
> +	mv88e6xxx_g2_write(chip, MV88E6390_G2_WDOG_CTL,
> +			   MV88E6390_G2_WDOG_CTL_UPDATE |
> +			   MV88E6390_G2_WDOG_CTL_PTR_EVENT);
> +
> +	return IRQ_HANDLED;
> +}

Shouldn't this update be in .irq_setup() method? In the commit message
you're saying that the problem is that bits aren't cleared with SW
reset. So I would guess that the change should be in the setup of
watchdog IRQ, not in IRQ action?

(I am not disagreeing, I am just asking because I don't have access to
documentation right now.)

Marek

> +const struct mv88e6xxx_irq_ops mv88e6393x_watchdog_ops = {
> +	.irq_action = mv88e6393x_watchdog_action,
> +	.irq_setup = mv88e6390_watchdog_setup,
> +	.irq_free = mv88e6390_watchdog_free,
> +};
> +
>  static irqreturn_t mv88e6xxx_g2_watchdog_thread_fn(int irq, void *dev_id)
>  {
>  	struct mv88e6xxx_chip *chip = dev_id;
> diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h
> index e973114d6890..7e091965582b 100644
> --- a/drivers/net/dsa/mv88e6xxx/global2.h
> +++ b/drivers/net/dsa/mv88e6xxx/global2.h
> @@ -369,6 +369,7 @@ int mv88e6xxx_g2_device_mapping_write(struct mv88e6xxx_chip *chip, int target,
>  extern const struct mv88e6xxx_irq_ops mv88e6097_watchdog_ops;
>  extern const struct mv88e6xxx_irq_ops mv88e6250_watchdog_ops;
>  extern const struct mv88e6xxx_irq_ops mv88e6390_watchdog_ops;
> +extern const struct mv88e6xxx_irq_ops mv88e6393x_watchdog_ops;
>  
>  extern const struct mv88e6xxx_avb_ops mv88e6165_avb_ops;
>  extern const struct mv88e6xxx_avb_ops mv88e6352_avb_ops;
> -- 
> 2.30.2
>

> > +static int mv88e6393x_watchdog_action(struct mv88e6xxx_chip *chip, int irq)
> > +{
> > +	mv88e6390_watchdog_action(chip, irq);
> > +
> > +	mv88e6xxx_g2_write(chip, MV88E6390_G2_WDOG_CTL,
> > +			   MV88E6390_G2_WDOG_CTL_UPDATE |
> > +			   MV88E6390_G2_WDOG_CTL_PTR_EVENT);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> Shouldn't this update be in .irq_setup() method? In the commit message
> you're saying that the problem is that bits aren't cleared with SW
> reset. So I would guess that the change should be in the setup of
> watchdog IRQ, not in IRQ action?

I think it is a bit more complex than that. At least for the 6352,
which i just looked at the data sheet, the interrupt bits are listed
as "ROC". Which is missing from the list of definitions, but seems to
mean Read Only, Clear on read. So even if it was not cleared on
software reset, it would only fire once, and then be cleared.

The problem description here is that it does not clear on read, it
needs an explicit write. Which suggests Marvell changed it for the
6393.

So i have a couple of questions:

1) Is this new behaviour limited to the 6393, or does the 6390 also
need this write?

2) What about other interrupts? Is this the only one which has changed
behaviour?

	Andrew

On Tue, Mar 28, 2023 at 02:30:37PM +0200, Andrew Lunn wrote:
> > > +static int mv88e6393x_watchdog_action(struct mv88e6xxx_chip *chip, int irq)
> > > +{
> > > +	mv88e6390_watchdog_action(chip, irq);
> > > +
> > > +	mv88e6xxx_g2_write(chip, MV88E6390_G2_WDOG_CTL,
> > > +			   MV88E6390_G2_WDOG_CTL_UPDATE |
> > > +			   MV88E6390_G2_WDOG_CTL_PTR_EVENT);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > 
> > Shouldn't this update be in .irq_setup() method? In the commit message
> > you're saying that the problem is that bits aren't cleared with SW
> > reset. So I would guess that the change should be in the setup of
> > watchdog IRQ, not in IRQ action?
> 
> I think it is a bit more complex than that. At least for the 6352,
> which i just looked at the data sheet, the interrupt bits are listed
> as "ROC". Which is missing from the list of definitions, but seems to
> mean Read Only, Clear on read. So even if it was not cleared on
> software reset, it would only fire once, and then be cleared.
> 
> The problem description here is that it does not clear on read, it
> needs an explicit write. Which suggests Marvell changed it for the
> 6393.
> 
> So i have a couple of questions:
> 
> 1) Is this new behaviour limited to the 6393, or does the 6390 also
> need this write?

OK I am looking at the func specs of 6390 and 6393x, at the table
descrinbing the Data Path Watch Dog Event register (index 0x12 of global
2, which is the one being written), and the tables are exactly the same.

For every non-reserved bit there is the following:
  This bit is cleared by a SWReset (Global 1 offset 0x04). It will
  automatically be cleared to zero if the SWReset on WD bit (index 0x13)
  is set to a one and this event's Func bit is cleared to zero (index
  0x11).

Moreover only bit 0 of this register (ForceWD Event) is RWR. Bits 1 to 3
(EgressWD Event, QC WD Event and CT WD Event) are all RO. Bits 4-7 are
reserved. (Once again, exactly as in func spec of 6390.)

So I am not exactly sure what is going on. The errata document I have
does not mention watch dog at all.

Marek

> 2) What about other interrupts? Is this the only one which has changed
> behaviour?
> 
> 	Andrew

On 3/28/23 14:47, Marek Behún wrote:
> On Tue, Mar 28, 2023 at 02:30:37PM +0200, Andrew Lunn wrote:
>>>> +static int mv88e6393x_watchdog_action(struct mv88e6xxx_chip *chip, int irq)
>>>> +{
>>>> +	mv88e6390_watchdog_action(chip, irq);
>>>> +
>>>> +	mv88e6xxx_g2_write(chip, MV88E6390_G2_WDOG_CTL,
>>>> +			   MV88E6390_G2_WDOG_CTL_UPDATE |
>>>> +			   MV88E6390_G2_WDOG_CTL_PTR_EVENT);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>
>>> Shouldn't this update be in .irq_setup() method? In the commit message
>>> you're saying that the problem is that bits aren't cleared with SW
>>> reset. So I would guess that the change should be in the setup of
>>> watchdog IRQ, not in IRQ action?
>>
>> I think it is a bit more complex than that. At least for the 6352,
>> which i just looked at the data sheet, the interrupt bits are listed
>> as "ROC". Which is missing from the list of definitions, but seems to
>> mean Read Only, Clear on read. So even if it was not cleared on
>> software reset, it would only fire once, and then be cleared.
>>
>> The problem description here is that it does not clear on read, it
>> needs an explicit write. Which suggests Marvell changed it for the
>> 6393.
>>
>> So i have a couple of questions:
>>
>> 1) Is this new behaviour limited to the 6393, or does the 6390 also
>> need this write?
> 
> OK I am looking at the func specs of 6390 and 6393x, at the table
> descrinbing the Data Path Watch Dog Event register (index 0x12 of global
> 2, which is the one being written), and the tables are exactly the same.
> 
> For every non-reserved bit there is the following:
>    This bit is cleared by a SWReset (Global 1 offset 0x04). It will
>    automatically be cleared to zero if the SWReset on WD bit (index 0x13)
>    is set to a one and this event's Func bit is cleared to zero (index
>    0x11).
> 
> Moreover only bit 0 of this register (ForceWD Event) is RWR. Bits 1 to 3
> (EgressWD Event, QC WD Event and CT WD Event) are all RO. Bits 4-7 are
> reserved. (Once again, exactly as in func spec of 6390.)
> 
> So I am not exactly sure what is going on. The errata document I have
> does not mention watch dog at all.
> 
> Marek
> 
>> 2) What about other interrupts? Is this the only one which has changed
>> behaviour?
>>
>> 	Andrew
1) Marvell has confirmed that 6393x (Amethyst) differs from 6390 
(Peridot) with quote: “I tried this on my board and see G2 offset 0x1B 
index 12 bit 0 does not clear, I also tried doing a SWReset and the bit 
is still 1. I did try the same on a Peridot board and it clears as 
advertised.”

2) Marvell are not aware of any other stuck bits, but has confirmed that 
the WD event bits are not cleared on SW reset which is indeed 
contradictory to what the data sheet suggests.

Bug can be reproduced on 6393x with the Force WD event by writing 0x9201 
to G2 offset 0x1b.

Best regards
Gustav

On Tue, Mar 28, 2023 at 03:34:03PM +0200, Gustav Ekelund wrote:

> 1) Marvell has confirmed that 6393x (Amethyst) differs from 6390 (Peridot)
> with quote: “I tried this on my board and see G2 offset 0x1B index 12 bit 0
> does not clear, I also tried doing a SWReset and the bit is still 1. I did
> try the same on a Peridot board and it clears as advertised.”
> 
> 2) Marvell are not aware of any other stuck bits, but has confirmed that the
> WD event bits are not cleared on SW reset which is indeed contradictory to
> what the data sheet suggests.

Hi Gustav

Please expand the commit message with a summary of this
information. It answers the questions both Marek and i have been
asking, so deserves to be in the commit message.

	Andrew

On Tue, Mar 28, 2023 at 03:45:51PM +0200, Andrew Lunn wrote:
> On Tue, Mar 28, 2023 at 03:34:03PM +0200, Gustav Ekelund wrote:
> 
> > 1) Marvell has confirmed that 6393x (Amethyst) differs from 6390 (Peridot)
> > with quote: “I tried this on my board and see G2 offset 0x1B index 12 bit 0
> > does not clear, I also tried doing a SWReset and the bit is still 1. I did
> > try the same on a Peridot board and it clears as advertised.”
> > 
> > 2) Marvell are not aware of any other stuck bits, but has confirmed that the
> > WD event bits are not cleared on SW reset which is indeed contradictory to
> > what the data sheet suggests.
> 
> Hi Gustav
> 
> Please expand the commit message with a summary of this
> information. It answers the questions both Marek and i have been
> asking, so deserves to be in the commit message.
> 
> 	Andrew

Maybe also add a comment next to the code writing to the register, that
this is due to an yet unreleased erratum on 6393x.

Marek

On 3/28/23 16:47, Marek Behún wrote:
> On Tue, Mar 28, 2023 at 03:45:51PM +0200, Andrew Lunn wrote:
>> On Tue, Mar 28, 2023 at 03:34:03PM +0200, Gustav Ekelund wrote:
>>
>>> 1) Marvell has confirmed that 6393x (Amethyst) differs from 6390 (Peridot)
>>> with quote: “I tried this on my board and see G2 offset 0x1B index 12 bit 0
>>> does not clear, I also tried doing a SWReset and the bit is still 1. I did
>>> try the same on a Peridot board and it clears as advertised.”
>>>
>>> 2) Marvell are not aware of any other stuck bits, but has confirmed that the
>>> WD event bits are not cleared on SW reset which is indeed contradictory to
>>> what the data sheet suggests.
>>
>> Hi Gustav
>>
>> Please expand the commit message with a summary of this
>> information. It answers the questions both Marek and i have been
>> asking, so deserves to be in the commit message.
>>
>> 	Andrew
> 
> Maybe also add a comment next to the code writing to the register, that
> this is due to an yet unreleased erratum on 6393x.
> 
> Marek
Hi Marek and Andrew

As you pointed out, it is only the force WD bit that is writeable and
the others are read-only. Just needed to clarify that the patch is only
meant to solve clearing the force WD event bit (bit 0).
I will clarify this in the commit as well.

The errata is allegedly planned to be documented in their next version
of release notes.

Thank you for reviewing!

Best regards
Gustav