[PATCH net-next 0/5] net: dsa: mv88e6xxx: further PTP-related cleanups

[PATCH net-next 0/5] net: dsa: mv88e6xxx: further PTP-related cleanups

[Russell King (Oracle)]

Hi,

Further mv88e6xxx PTP-related cleanups, mostly centred around the
register definitions, but also moving one function prototype to a
more logical header.

These do not conflict with "net: dsa: mv88e6xxx: clean up PTP clock
during setup failure" sent earlier today.

 drivers/net/dsa/mv88e6xxx/hwtstamp.c |   2 +-
 drivers/net/dsa/mv88e6xxx/hwtstamp.h |   1 +
 drivers/net/dsa/mv88e6xxx/ptp.c      |  24 +++----
 drivers/net/dsa/mv88e6xxx/ptp.h      | 124 +++++++----------------------------
 4 files changed, 39 insertions(+), 112 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 1/5] net: dsa: mv88e6xxx: rename TAI definitions according to core

[Russell King (Oracle)]

The TAI_EVENT_STATUS and TAI_CFG definitions are only used for the
88E6352-family of TAI implementations. Rename them as such, and
remove the TAI_EVENT_TIME_* definitions that are unused (although
we read them as a block.)

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/ptp.c | 24 ++++++++---------
 drivers/net/dsa/mv88e6xxx/ptp.h | 46 ++++++++++++++++-----------------
 2 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
index fa17ad6e378f..f7603573d3a9 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.c
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -144,7 +144,7 @@ static u64 mv88e6352_ptp_clock_read(struct cyclecounter *cc)
 	u16 phc_time[2];
 	int err;
 
-	err = mv88e6xxx_tai_read(chip, MV88E6XXX_TAI_TIME_LO, phc_time,
+	err = mv88e6xxx_tai_read(chip, MV88E6352_TAI_TIME_LO, phc_time,
 				 ARRAY_SIZE(phc_time));
 	if (err)
 		return 0;
@@ -158,7 +158,7 @@ static u64 mv88e6165_ptp_clock_read(struct cyclecounter *cc)
 	u16 phc_time[2];
 	int err;
 
-	err = mv88e6xxx_tai_read(chip, MV88E6XXX_PTP_GC_TIME_LO, phc_time,
+	err = mv88e6xxx_tai_read(chip, MV88E6165_PTP_GC_TIME_LO, phc_time,
 				 ARRAY_SIZE(phc_time));
 	if (err)
 		return 0;
@@ -176,17 +176,17 @@ static int mv88e6352_config_eventcap(struct mv88e6xxx_chip *chip, int rising)
 	u16 evcap_config;
 	int err;
 
-	evcap_config = MV88E6XXX_TAI_CFG_CAP_OVERWRITE |
-		       MV88E6XXX_TAI_CFG_CAP_CTR_START;
+	evcap_config = MV88E6352_TAI_CFG_CAP_OVERWRITE |
+		       MV88E6352_TAI_CFG_CAP_CTR_START;
 	if (!rising)
-		evcap_config |= MV88E6XXX_TAI_CFG_EVREQ_FALLING;
+		evcap_config |= MV88E6352_TAI_CFG_EVREQ_FALLING;
 
-	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_CFG, evcap_config);
+	err = mv88e6xxx_tai_write(chip, MV88E6352_TAI_CFG, evcap_config);
 	if (err)
 		return err;
 
 	/* Write the capture config; this also clears the capture counter */
-	return mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_EVENT_STATUS, 0);
+	return mv88e6xxx_tai_write(chip, MV88E6352_TAI_EVENT_STATUS, 0);
 }
 
 static void mv88e6352_tai_event_work(struct work_struct *ugly)
@@ -199,7 +199,7 @@ static void mv88e6352_tai_event_work(struct work_struct *ugly)
 	int err;
 
 	mv88e6xxx_reg_lock(chip);
-	err = mv88e6xxx_tai_read(chip, MV88E6XXX_TAI_EVENT_STATUS,
+	err = mv88e6xxx_tai_read(chip, MV88E6352_TAI_EVENT_STATUS,
 				 status, ARRAY_SIZE(status));
 	mv88e6xxx_reg_unlock(chip);
 
@@ -207,19 +207,19 @@ static void mv88e6352_tai_event_work(struct work_struct *ugly)
 		dev_err(chip->dev, "failed to read TAI status register\n");
 		return;
 	}
-	if (status[0] & MV88E6XXX_TAI_EVENT_STATUS_ERROR) {
+	if (status[0] & MV88E6352_TAI_EVENT_STATUS_ERROR) {
 		dev_warn(chip->dev, "missed event capture\n");
 		return;
 	}
-	if (!(status[0] & MV88E6XXX_TAI_EVENT_STATUS_VALID))
+	if (!(status[0] & MV88E6352_TAI_EVENT_STATUS_VALID))
 		goto out;
 
 	raw_ts = ((u32)status[2] << 16) | status[1];
 
 	/* Clear the valid bit so the next timestamp can come in */
-	status[0] &= ~MV88E6XXX_TAI_EVENT_STATUS_VALID;
+	status[0] &= ~MV88E6352_TAI_EVENT_STATUS_VALID;
 	mv88e6xxx_reg_lock(chip);
-	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_EVENT_STATUS, status[0]);
+	err = mv88e6xxx_tai_write(chip, MV88E6352_TAI_EVENT_STATUS, status[0]);
 	mv88e6xxx_reg_unlock(chip);
 	if (err) {
 		dev_err(chip->dev, "failed to write TAI status register\n");
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index b3fd177d67e3..67deb2f0fddb 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -16,19 +16,19 @@
 #include "chip.h"
 
 /* Offset 0x00: TAI Global Config */
-#define MV88E6XXX_TAI_CFG			0x00
-#define MV88E6XXX_TAI_CFG_CAP_OVERWRITE		0x8000
-#define MV88E6XXX_TAI_CFG_CAP_CTR_START		0x4000
-#define MV88E6XXX_TAI_CFG_EVREQ_FALLING		0x2000
-#define MV88E6XXX_TAI_CFG_TRIG_ACTIVE_LO	0x1000
-#define MV88E6XXX_TAI_CFG_IRL_ENABLE		0x0400
-#define MV88E6XXX_TAI_CFG_TRIG_IRQ_EN		0x0200
-#define MV88E6XXX_TAI_CFG_EVREQ_IRQ_EN		0x0100
-#define MV88E6XXX_TAI_CFG_TRIG_LOCK		0x0080
-#define MV88E6XXX_TAI_CFG_BLOCK_UPDATE		0x0008
-#define MV88E6XXX_TAI_CFG_MULTI_PTP		0x0004
-#define MV88E6XXX_TAI_CFG_TRIG_MODE_ONESHOT	0x0002
-#define MV88E6XXX_TAI_CFG_TRIG_ENABLE		0x0001
+#define MV88E6352_TAI_CFG			0x00
+#define MV88E6352_TAI_CFG_CAP_OVERWRITE		0x8000
+#define MV88E6352_TAI_CFG_CAP_CTR_START		0x4000
+#define MV88E6352_TAI_CFG_EVREQ_FALLING		0x2000
+#define MV88E6352_TAI_CFG_TRIG_ACTIVE_LO	0x1000
+#define MV88E6352_TAI_CFG_IRL_ENABLE		0x0400
+#define MV88E6352_TAI_CFG_TRIG_IRQ_EN		0x0200
+#define MV88E6352_TAI_CFG_EVREQ_IRQ_EN		0x0100
+#define MV88E6352_TAI_CFG_TRIG_LOCK		0x0080
+#define MV88E6352_TAI_CFG_BLOCK_UPDATE		0x0008
+#define MV88E6352_TAI_CFG_MULTI_PTP		0x0004
+#define MV88E6352_TAI_CFG_TRIG_MODE_ONESHOT	0x0002
+#define MV88E6352_TAI_CFG_TRIG_ENABLE		0x0001
 
 /* Offset 0x01: Timestamp Clock Period (ps) */
 #define MV88E6XXX_TAI_CLOCK_PERIOD		0x01
@@ -53,18 +53,16 @@
 #define MV88E6XXX_TAI_IRL_COMP_PS		0x08
 
 /* Offset 0x09: Event Status */
-#define MV88E6XXX_TAI_EVENT_STATUS		0x09
-#define MV88E6XXX_TAI_EVENT_STATUS_ERROR	0x0200
-#define MV88E6XXX_TAI_EVENT_STATUS_VALID	0x0100
-#define MV88E6XXX_TAI_EVENT_STATUS_CTR_MASK	0x00ff
-
 /* Offset 0x0A/0x0B: Event Time */
-#define MV88E6XXX_TAI_EVENT_TIME_LO		0x0a
-#define MV88E6XXX_TAI_EVENT_TYPE_HI		0x0b
+#define MV88E6352_TAI_EVENT_STATUS		0x09
+#define MV88E6352_TAI_EVENT_STATUS_CAP_TRIG	0x4000
+#define MV88E6352_TAI_EVENT_STATUS_ERROR	0x0200
+#define MV88E6352_TAI_EVENT_STATUS_VALID	0x0100
+#define MV88E6352_TAI_EVENT_STATUS_CTR_MASK	0x00ff
 
 /* Offset 0x0E/0x0F: PTP Global Time */
-#define MV88E6XXX_TAI_TIME_LO			0x0e
-#define MV88E6XXX_TAI_TIME_HI			0x0f
+#define MV88E6352_TAI_TIME_LO			0x0e
+#define MV88E6352_TAI_TIME_HI			0x0f
 
 /* Offset 0x10/0x11: Trig Generation Time */
 #define MV88E6XXX_TAI_TRIG_TIME_LO		0x10
@@ -101,8 +99,8 @@
 #define MV88E6XXX_PTP_GC_INT_STATUS		0x08
 
 /* Offset 0x9/0xa: Global Time */
-#define MV88E6XXX_PTP_GC_TIME_LO		0x09
-#define MV88E6XXX_PTP_GC_TIME_HI		0x0A
+#define MV88E6165_PTP_GC_TIME_LO		0x09
+#define MV88E6165_PTP_GC_TIME_HI		0x0A
 
 /* 6165 Per Port Registers */
 /* Offset 0: Arrival Time 0 Status */
-- 
2.47.3

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: rename TAI definitions according to core

[Vladimir Oltean]

On Mon, Sep 15, 2025 at 02:06:15PM +0100, Russell King (Oracle) wrote:
> The TAI_EVENT_STATUS and TAI_CFG definitions are only used for the
> 88E6352-family of TAI implementations. Rename them as such, and
> remove the TAI_EVENT_TIME_* definitions that are unused (although
> we read them as a block.)
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
> index b3fd177d67e3..67deb2f0fddb 100644
> --- a/drivers/net/dsa/mv88e6xxx/ptp.h
> +++ b/drivers/net/dsa/mv88e6xxx/ptp.h
> @@ -16,19 +16,19 @@
>  #include "chip.h"
>  
>  /* Offset 0x00: TAI Global Config */
> -#define MV88E6XXX_TAI_CFG			0x00
> -#define MV88E6XXX_TAI_CFG_CAP_OVERWRITE		0x8000
> -#define MV88E6XXX_TAI_CFG_CAP_CTR_START		0x4000
> -#define MV88E6XXX_TAI_CFG_EVREQ_FALLING		0x2000
> -#define MV88E6XXX_TAI_CFG_TRIG_ACTIVE_LO	0x1000
> -#define MV88E6XXX_TAI_CFG_IRL_ENABLE		0x0400
> -#define MV88E6XXX_TAI_CFG_TRIG_IRQ_EN		0x0200
> -#define MV88E6XXX_TAI_CFG_EVREQ_IRQ_EN		0x0100
> -#define MV88E6XXX_TAI_CFG_TRIG_LOCK		0x0080
> -#define MV88E6XXX_TAI_CFG_BLOCK_UPDATE		0x0008
> -#define MV88E6XXX_TAI_CFG_MULTI_PTP		0x0004
> -#define MV88E6XXX_TAI_CFG_TRIG_MODE_ONESHOT	0x0002
> -#define MV88E6XXX_TAI_CFG_TRIG_ENABLE		0x0001
> +#define MV88E6352_TAI_CFG			0x00
> +#define MV88E6352_TAI_CFG_CAP_OVERWRITE		0x8000
> +#define MV88E6352_TAI_CFG_CAP_CTR_START		0x4000
> +#define MV88E6352_TAI_CFG_EVREQ_FALLING		0x2000
> +#define MV88E6352_TAI_CFG_TRIG_ACTIVE_LO	0x1000
> +#define MV88E6352_TAI_CFG_IRL_ENABLE		0x0400
> +#define MV88E6352_TAI_CFG_TRIG_IRQ_EN		0x0200
> +#define MV88E6352_TAI_CFG_EVREQ_IRQ_EN		0x0100
> +#define MV88E6352_TAI_CFG_TRIG_LOCK		0x0080
> +#define MV88E6352_TAI_CFG_BLOCK_UPDATE		0x0008
> +#define MV88E6352_TAI_CFG_MULTI_PTP		0x0004
> +#define MV88E6352_TAI_CFG_TRIG_MODE_ONESHOT	0x0002
> +#define MV88E6352_TAI_CFG_TRIG_ENABLE		0x0001
>  
>  /* Offset 0x01: Timestamp Clock Period (ps) */
>  #define MV88E6XXX_TAI_CLOCK_PERIOD		0x01
> @@ -53,18 +53,16 @@
>  #define MV88E6XXX_TAI_IRL_COMP_PS		0x08
>  
>  /* Offset 0x09: Event Status */
> -#define MV88E6XXX_TAI_EVENT_STATUS		0x09
> -#define MV88E6XXX_TAI_EVENT_STATUS_ERROR	0x0200
> -#define MV88E6XXX_TAI_EVENT_STATUS_VALID	0x0100
> -#define MV88E6XXX_TAI_EVENT_STATUS_CTR_MASK	0x00ff
> -
>  /* Offset 0x0A/0x0B: Event Time */

Was it intentional to keep the comment for a register with removed
definitions, and this placement for it? It looks like this (confusing
to me):

/* Offset 0x09: Event Status */
/* Offset 0x0A/0x0B: Event Time */
#define MV88E6352_TAI_EVENT_STATUS		0x09

> -#define MV88E6XXX_TAI_EVENT_TIME_LO		0x0a
> -#define MV88E6XXX_TAI_EVENT_TYPE_HI		0x0b
> +#define MV88E6352_TAI_EVENT_STATUS		0x09
> +#define MV88E6352_TAI_EVENT_STATUS_CAP_TRIG	0x4000
> +#define MV88E6352_TAI_EVENT_STATUS_ERROR	0x0200
> +#define MV88E6352_TAI_EVENT_STATUS_VALID	0x0100
> +#define MV88E6352_TAI_EVENT_STATUS_CTR_MASK	0x00ff
>  
>  /* Offset 0x0E/0x0F: PTP Global Time */
> -#define MV88E6XXX_TAI_TIME_LO			0x0e
> -#define MV88E6XXX_TAI_TIME_HI			0x0f
> +#define MV88E6352_TAI_TIME_LO			0x0e
> +#define MV88E6352_TAI_TIME_HI			0x0f
>  
>  /* Offset 0x10/0x11: Trig Generation Time */
>  #define MV88E6XXX_TAI_TRIG_TIME_LO		0x10
> @@ -101,8 +99,8 @@
>  #define MV88E6XXX_PTP_GC_INT_STATUS		0x08
>  
>  /* Offset 0x9/0xa: Global Time */
> -#define MV88E6XXX_PTP_GC_TIME_LO		0x09
> -#define MV88E6XXX_PTP_GC_TIME_HI		0x0A
> +#define MV88E6165_PTP_GC_TIME_LO		0x09
> +#define MV88E6165_PTP_GC_TIME_HI		0x0A
>  
>  /* 6165 Per Port Registers */
>  /* Offset 0: Arrival Time 0 Status */
> -- 
> 2.47.3
> 

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: rename TAI definitions according to core

[Russell King (Oracle)]

On Tue, Sep 16, 2025 at 11:46:45AM +0300, Vladimir Oltean wrote:
> On Mon, Sep 15, 2025 at 02:06:15PM +0100, Russell King (Oracle) wrote:
> >  /* Offset 0x09: Event Status */
> > -#define MV88E6XXX_TAI_EVENT_STATUS		0x09
> > -#define MV88E6XXX_TAI_EVENT_STATUS_ERROR	0x0200
> > -#define MV88E6XXX_TAI_EVENT_STATUS_VALID	0x0100
> > -#define MV88E6XXX_TAI_EVENT_STATUS_CTR_MASK	0x00ff
> > -
> >  /* Offset 0x0A/0x0B: Event Time */
> 
> Was it intentional to keep the comment for a register with removed
> definitions, and this placement for it? It looks like this (confusing
> to me):
> 
> /* Offset 0x09: Event Status */
> /* Offset 0x0A/0x0B: Event Time */
> #define MV88E6352_TAI_EVENT_STATUS		0x09

Yes, totally intentional.

All three registers are read by the code - as a single block, rather
than individually. While the definitions for the event time are not
referenced, I wanted to keep their comment, and that seemed to be
the most logical way.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: rename TAI definitions according to core

[Vladimir Oltean]

On Tue, Sep 16, 2025 at 02:36:00PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 16, 2025 at 11:46:45AM +0300, Vladimir Oltean wrote:
> > On Mon, Sep 15, 2025 at 02:06:15PM +0100, Russell King (Oracle) wrote:
> > >  /* Offset 0x09: Event Status */
> > > -#define MV88E6XXX_TAI_EVENT_STATUS		0x09
> > > -#define MV88E6XXX_TAI_EVENT_STATUS_ERROR	0x0200
> > > -#define MV88E6XXX_TAI_EVENT_STATUS_VALID	0x0100
> > > -#define MV88E6XXX_TAI_EVENT_STATUS_CTR_MASK	0x00ff
> > > -
> > >  /* Offset 0x0A/0x0B: Event Time */
> > 
> > Was it intentional to keep the comment for a register with removed
> > definitions, and this placement for it? It looks like this (confusing
> > to me):
> > 
> > /* Offset 0x09: Event Status */
> > /* Offset 0x0A/0x0B: Event Time */
> > #define MV88E6352_TAI_EVENT_STATUS		0x09
> 
> Yes, totally intentional.
> 
> All three registers are read by the code - as a single block, rather
> than individually. While the definitions for the event time are not
> referenced, I wanted to keep their comment, and that seemed to be
> the most logical way.

What I don't find so logical is that the bit fields of MV88E6352_TAI_EVENT_STATUS
follow a comment which refers to "Event Time".

Do we read the registers in a single mv88e6xxx_tai_read() call because
the hardware requires us, or because of convenience? For writes, we
write only a single u16 corresponding to the Event Status, so I suspect
they are not completely indivisible, but I don't have documentation to
confirm.

This is more of what I was expecting.

/* Offset 0x09: Event Status */
#define MV88E6352_TAI_EVENT_STATUS		0x09
#define MV88E6352_TAI_EVENT_STATUS_CAP_TRIG	0x4000
#define MV88E6352_TAI_EVENT_STATUS_ERROR	0x0200
#define MV88E6352_TAI_EVENT_STATUS_VALID	0x0100
#define MV88E6352_TAI_EVENT_STATUS_CTR_MASK	0x00ff
/* Offset 0x0A/0x0B: Event Time Lo/Hi. Always read together with Event Status */

Anyway, this is not so important.

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: rename TAI definitions according to core

[Russell King (Oracle)]

On Tue, Sep 16, 2025 at 05:35:33PM +0300, Vladimir Oltean wrote:
> On Tue, Sep 16, 2025 at 02:36:00PM +0100, Russell King (Oracle) wrote:
> > On Tue, Sep 16, 2025 at 11:46:45AM +0300, Vladimir Oltean wrote:
> > > On Mon, Sep 15, 2025 at 02:06:15PM +0100, Russell King (Oracle) wrote:
> > > >  /* Offset 0x09: Event Status */
> > > > -#define MV88E6XXX_TAI_EVENT_STATUS		0x09
> > > > -#define MV88E6XXX_TAI_EVENT_STATUS_ERROR	0x0200
> > > > -#define MV88E6XXX_TAI_EVENT_STATUS_VALID	0x0100
> > > > -#define MV88E6XXX_TAI_EVENT_STATUS_CTR_MASK	0x00ff
> > > > -
> > > >  /* Offset 0x0A/0x0B: Event Time */
> > > 
> > > Was it intentional to keep the comment for a register with removed
> > > definitions, and this placement for it? It looks like this (confusing
> > > to me):
> > > 
> > > /* Offset 0x09: Event Status */
> > > /* Offset 0x0A/0x0B: Event Time */
> > > #define MV88E6352_TAI_EVENT_STATUS		0x09
> > 
> > Yes, totally intentional.
> > 
> > All three registers are read by the code - as a single block, rather
> > than individually. While the definitions for the event time are not
> > referenced, I wanted to keep their comment, and that seemed to be
> > the most logical way.
> 
> What I don't find so logical is that the bit fields of MV88E6352_TAI_EVENT_STATUS
> follow a comment which refers to "Event Time".
> 
> Do we read the registers in a single mv88e6xxx_tai_read() call because
> the hardware requires us, or because of convenience?

For the packet timestamp registers that follow basically the same
format and layout, they're defined as a block that can be accessed
atomically. Nothing is stated with respect to these registers.

As the status register contains bits to say whether the timestamp was
overwritten, if reading them were not atomic, there would be no way to
be certain that the timestamp is remotely correct, especially when the
hardware is allowed to overwrite events.

Consider this scenario, where overwriting is permitted, if not atomic:

- event happens
- read status register
- read time lo register (first event time lo value)
- event happens
- read time high register (second event's time high value)

If it isn't atomic, there's no way to be certain that the time high
value corresponds with the time lo value.

If overwriting is not permitted then:
- event happens
- read status register
- read time lo register (first event time lo value)
- event happens
- read time high register (documented in this scenario to be invalid)

which is worse - and we wouldn't have read the status register to
know that the second event happened (which will flag an "Error" bit
in the status register in this case.)

So, the only sensible thing is to assume that, just like the other
timestamp capture registers, these behave the same. IOW, they are
atomic when read consecutively.

(The format of the timestamp registers have the same status + time lo
+ time high format, but with an additional PTP sequence number
register.)

> For writes, we
> write only a single u16 corresponding to the Event Status, so I suspect
> they are not completely indivisible, but I don't have documentation to
> confirm.

The write is required to clear the status bits, (a) so that we know
when a new event occurs, (b) clears any interrupt(s) that were raised
for it, and (c) if overwriting is not permitted, allows the next event
to be logged.

There's two modes for this register. DSA uses the "allow overwrite"
mode, so reading this better be atomic like the similar PTP
timestamping registers.

I suspect, however, that the answer is "we just don't know". Is there
any Marvell hardware out there where the PTP pins are used? Not that
I'm aware of, none of the ZII boards use it. Maybe Andrew has more
information on that.

> This is more of what I was expecting.
> 
> /* Offset 0x09: Event Status */
> #define MV88E6352_TAI_EVENT_STATUS		0x09
> #define MV88E6352_TAI_EVENT_STATUS_CAP_TRIG	0x4000
> #define MV88E6352_TAI_EVENT_STATUS_ERROR	0x0200
> #define MV88E6352_TAI_EVENT_STATUS_VALID	0x0100
> #define MV88E6352_TAI_EVENT_STATUS_CTR_MASK	0x00ff
> /* Offset 0x0A/0x0B: Event Time Lo/Hi. Always read together with Event Status */

Okay, I'll change it to that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 2/5] net: dsa: mv88e6xxx: remove unused TAI definitions

[Russell King (Oracle)]

Remove the TAI definitions that the code never uses.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/ptp.h | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index 67deb2f0fddb..3e0296303d61 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -33,25 +33,6 @@
 /* Offset 0x01: Timestamp Clock Period (ps) */
 #define MV88E6XXX_TAI_CLOCK_PERIOD		0x01
 
-/* Offset 0x02/0x03: Trigger Generation Amount */
-#define MV88E6XXX_TAI_TRIG_GEN_AMOUNT_LO	0x02
-#define MV88E6XXX_TAI_TRIG_GEN_AMOUNT_HI	0x03
-
-/* Offset 0x04: Clock Compensation */
-#define MV88E6XXX_TAI_TRIG_CLOCK_COMP		0x04
-
-/* Offset 0x05: Trigger Configuration */
-#define MV88E6XXX_TAI_TRIG_CFG			0x05
-
-/* Offset 0x06: Ingress Rate Limiter Clock Generation Amount */
-#define MV88E6XXX_TAI_IRL_AMOUNT		0x06
-
-/* Offset 0x07: Ingress Rate Limiter Compensation */
-#define MV88E6XXX_TAI_IRL_COMP			0x07
-
-/* Offset 0x08: Ingress Rate Limiter Compensation */
-#define MV88E6XXX_TAI_IRL_COMP_PS		0x08
-
 /* Offset 0x09: Event Status */
 /* Offset 0x0A/0x0B: Event Time */
 #define MV88E6352_TAI_EVENT_STATUS		0x09
@@ -64,13 +45,6 @@
 #define MV88E6352_TAI_TIME_LO			0x0e
 #define MV88E6352_TAI_TIME_HI			0x0f
 
-/* Offset 0x10/0x11: Trig Generation Time */
-#define MV88E6XXX_TAI_TRIG_TIME_LO		0x10
-#define MV88E6XXX_TAI_TRIG_TIME_HI		0x11
-
-/* Offset 0x12: Lock Status */
-#define MV88E6XXX_TAI_LOCK_STATUS		0x12
-
 /* Offset 0x00: Ether Type */
 #define MV88E6XXX_PTP_GC_ETYPE			0x00
 
-- 
2.47.3

Re: [PATCH net-next 2/5] net: dsa: mv88e6xxx: remove unused TAI definitions

[Vladimir Oltean]

On Mon, Sep 15, 2025 at 02:06:20PM +0100, Russell King (Oracle) wrote:
> Remove the TAI definitions that the code never uses.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next 3/5] net: dsa: mv88e6xxx: remove duplicated register definition

[Russell King (Oracle)]

There are two identical MV88E6XXX_PTP_GC_ETYPE definitions in ptp.h,
and MV88E6XXX_PTP_ETHERTYPE in hwtstamp.h which all refer to the
exact same register. As the code that accesses this register is in
hwtstamp.c, use the hwtstamp.h definition, and remove the
unnecessary duplicated definition in ptp.h

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 2 +-
 drivers/net/dsa/mv88e6xxx/ptp.h      | 6 ------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index f663799b0b3b..6e6472a3b75a 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -570,7 +570,7 @@ int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
 	}
 
 	/* Set the ethertype of L2 PTP messages */
-	err = mv88e6xxx_ptp_write(chip, MV88E6XXX_PTP_GC_ETYPE, ETH_P_1588);
+	err = mv88e6xxx_ptp_write(chip, MV88E6XXX_PTP_ETHERTYPE, ETH_P_1588);
 	if (err)
 		return err;
 
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index 3e0296303d61..60c00a15d466 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -45,13 +45,7 @@
 #define MV88E6352_TAI_TIME_LO			0x0e
 #define MV88E6352_TAI_TIME_HI			0x0f
 
-/* Offset 0x00: Ether Type */
-#define MV88E6XXX_PTP_GC_ETYPE			0x00
-
 /* 6165 Global Control Registers */
-/* Offset 0x00: Ether Type */
-#define MV88E6XXX_PTP_GC_ETYPE			0x00
-
 /* Offset 0x01: Message ID */
 #define MV88E6XXX_PTP_GC_MESSAGE_ID		0x01
 
-- 
2.47.3

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: remove duplicated register definition

[Vladimir Oltean]

On Mon, Sep 15, 2025 at 02:06:25PM +0100, Russell King (Oracle) wrote:
> There are two identical MV88E6XXX_PTP_GC_ETYPE definitions in ptp.h,
> and MV88E6XXX_PTP_ETHERTYPE in hwtstamp.h which all refer to the
> exact same register. As the code that accesses this register is in
> hwtstamp.c, use the hwtstamp.h definition, and remove the
> unnecessary duplicated definition in ptp.h
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next 4/5] net: dsa: mv88e6xxx: remove unused 88E6165 register definitions

[Russell King (Oracle)]

Remove the unused 88E6165 register definitions. For the port
registers, add a comment describing that each arrival and departure
offset is for a set of four registers that correspond with status,
two timestamp registers and the PTP sequence ID captured from the
packet.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/ptp.h | 45 +++------------------------------
 1 file changed, 3 insertions(+), 42 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index 60c00a15d466..3cf3bed82872 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -46,61 +46,22 @@
 #define MV88E6352_TAI_TIME_HI			0x0f
 
 /* 6165 Global Control Registers */
-/* Offset 0x01: Message ID */
-#define MV88E6XXX_PTP_GC_MESSAGE_ID		0x01
-
-/* Offset 0x02: Time Stamp Arrive Time */
-#define MV88E6XXX_PTP_GC_TS_ARR_PTR		0x02
-
-/* Offset 0x03: Port Arrival Interrupt Enable */
-#define MV88E6XXX_PTP_GC_PORT_ARR_INT_EN	0x03
-
-/* Offset 0x04: Port Departure Interrupt Enable */
-#define MV88E6XXX_PTP_GC_PORT_DEP_INT_EN	0x04
-
-/* Offset 0x05: Configuration */
-#define MV88E6XXX_PTP_GC_CONFIG			0x05
-#define MV88E6XXX_PTP_GC_CONFIG_DIS_OVERWRITE	BIT(1)
-#define MV88E6XXX_PTP_GC_CONFIG_DIS_TS		BIT(0)
-
-/* Offset 0x8: Interrupt Status */
-#define MV88E6XXX_PTP_GC_INT_STATUS		0x08
-
 /* Offset 0x9/0xa: Global Time */
 #define MV88E6165_PTP_GC_TIME_LO		0x09
 #define MV88E6165_PTP_GC_TIME_HI		0x0A
 
-/* 6165 Per Port Registers */
+/* 6165 Per Port Registers. The arrival and departure registers are a
+ * common block consisting of status, two time registers and the sequence ID
+ */
 /* Offset 0: Arrival Time 0 Status */
 #define MV88E6165_PORT_PTP_ARR0_STS	0x00
 
-/* Offset 0x01/0x02: PTP Arrival 0 Time */
-#define MV88E6165_PORT_PTP_ARR0_TIME_LO	0x01
-#define MV88E6165_PORT_PTP_ARR0_TIME_HI	0x02
-
-/* Offset 0x03: PTP Arrival 0 Sequence ID */
-#define MV88E6165_PORT_PTP_ARR0_SEQID	0x03
-
 /* Offset 0x04: PTP Arrival 1 Status */
 #define MV88E6165_PORT_PTP_ARR1_STS	0x04
 
-/* Offset 0x05/0x6E: PTP Arrival 1 Time */
-#define MV88E6165_PORT_PTP_ARR1_TIME_LO	0x05
-#define MV88E6165_PORT_PTP_ARR1_TIME_HI	0x06
-
-/* Offset 0x07: PTP Arrival 1 Sequence ID */
-#define MV88E6165_PORT_PTP_ARR1_SEQID	0x07
-
 /* Offset 0x08: PTP Departure Status */
 #define MV88E6165_PORT_PTP_DEP_STS	0x08
 
-/* Offset 0x09/0x0a: PTP Deperture Time */
-#define MV88E6165_PORT_PTP_DEP_TIME_LO	0x09
-#define MV88E6165_PORT_PTP_DEP_TIME_HI	0x0a
-
-/* Offset 0x0b: PTP Departure Sequence ID */
-#define MV88E6165_PORT_PTP_DEP_SEQID	0x0b
-
 /* Offset 0x0d: Port Status */
 #define MV88E6164_PORT_STATUS		0x0d
 
-- 
2.47.3

[PATCH net-next 5/5] net: dsa: mv88e6xxx: move mv88e6xxx_hwtstamp_work() prototype

[Russell King (Oracle)]

Since mv88e6xxx_hwtstamp_work() is defined in hwtstamp.c, its
prototype should be in hwtstamp.h, so move it there.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.h | 1 +
 drivers/net/dsa/mv88e6xxx/ptp.h      | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index 22e4acc957f0..c359821d5a6e 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -124,6 +124,7 @@ void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
 int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
 			  struct kernel_ethtool_ts_info *info);
 
+long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp);
 int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip);
 void mv88e6xxx_hwtstamp_free(struct mv88e6xxx_chip *chip);
 int mv88e6352_hwtstamp_port_enable(struct mv88e6xxx_chip *chip, int port);
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index 3cf3bed82872..63ae2831065c 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -67,7 +67,6 @@
 
 #ifdef CONFIG_NET_DSA_MV88E6XXX_PTP
 
-long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp);
 int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip);
 void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip);
 
-- 
2.47.3

Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: move mv88e6xxx_hwtstamp_work() prototype

[Vladimir Oltean]

On Mon, Sep 15, 2025 at 02:06:35PM +0100, Russell King (Oracle) wrote:
> Since mv88e6xxx_hwtstamp_work() is defined in hwtstamp.c, its
> prototype should be in hwtstamp.h, so move it there.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

This leaves the shim definition (for when CONFIG_NET_DSA_MV88E6XXX_PTP
is not defined) in ptp.h. It creates an inconsistency and potential
problem - the same header should provide all definitions of the same
function.

Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: move mv88e6xxx_hwtstamp_work() prototype

[Russell King (Oracle)]

On Tue, Sep 16, 2025 at 11:09:03AM +0300, Vladimir Oltean wrote:
> On Mon, Sep 15, 2025 at 02:06:35PM +0100, Russell King (Oracle) wrote:
> > Since mv88e6xxx_hwtstamp_work() is defined in hwtstamp.c, its
> > prototype should be in hwtstamp.h, so move it there.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> 
> This leaves the shim definition (for when CONFIG_NET_DSA_MV88E6XXX_PTP
> is not defined) in ptp.h. It creates an inconsistency and potential
> problem - the same header should provide all definitions of the same
> function.

The only caller of mv88e6xxx_hwtstamp_work() is from ptp.c, and both
hwtstamp.c which provides this function and ptp.c are only built if
CONFIG_NET_DSA_MV88E6XXX_PTP is set. So, this shim serves no useful
purpose.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: move mv88e6xxx_hwtstamp_work() prototype

[Vladimir Oltean]

On Tue, Sep 16, 2025 at 09:36:10AM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 16, 2025 at 11:09:03AM +0300, Vladimir Oltean wrote:
> > On Mon, Sep 15, 2025 at 02:06:35PM +0100, Russell King (Oracle) wrote:
> > > Since mv88e6xxx_hwtstamp_work() is defined in hwtstamp.c, its
> > > prototype should be in hwtstamp.h, so move it there.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > 
> > This leaves the shim definition (for when CONFIG_NET_DSA_MV88E6XXX_PTP
> > is not defined) in ptp.h. It creates an inconsistency and potential
> > problem - the same header should provide all definitions of the same
> > function.
> 
> The only caller of mv88e6xxx_hwtstamp_work() is from ptp.c, and both
> hwtstamp.c which provides this function and ptp.c are only built if
> CONFIG_NET_DSA_MV88E6XXX_PTP is set. So, this shim serves no useful
> purpose.

Ok, in the same situation we have:
- mv88e6352_hwtstamp_port_enable()
- mv88e6352_hwtstamp_port_disable()
- mv88e6165_global_enable()
- mv88e6165_global_disable()

which have no shim definition in hwtstamp.h. So I agree that the
mv88e6xxx_hwtstamp_work() shim can be removed. The remaining functions
are called by chip.c, so they need the shim.

Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: move mv88e6xxx_hwtstamp_work() prototype

[Andrew Lunn]

> This leaves the shim definition (for when CONFIG_NET_DSA_MV88E6XXX_PTP
> is not defined) in ptp.h. It creates an inconsistency and potential
> problem - the same header should provide all definitions of the same
> function.

How big is the PTP code? We have added a lot of code to this driver
since PTP was added. I suspect the PTP code is now small compared to
the rest of the driver, so does it still make sense to have it
optional? Also once the PTP code gets moved into a library and shared
by the Marvell PHY driver and other Marvell MAC drivers, won't we have
an overall code shrink even when it is enabled in DSA?

Maybe it is time for CONFIG_NET_DSA_MV88E6XXX_PTP to go away?

	Andrew

Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: move mv88e6xxx_hwtstamp_work() prototype

[Russell King (Oracle)]

On Tue, Sep 16, 2025 at 02:46:05PM +0200, Andrew Lunn wrote:
> > This leaves the shim definition (for when CONFIG_NET_DSA_MV88E6XXX_PTP
> > is not defined) in ptp.h. It creates an inconsistency and potential
> > problem - the same header should provide all definitions of the same
> > function.
> 
> How big is the PTP code? We have added a lot of code to this driver
> since PTP was added. I suspect the PTP code is now small compared to
> the rest of the driver, so does it still make sense to have it
> optional? Also once the PTP code gets moved into a library and shared
> by the Marvell PHY driver and other Marvell MAC drivers, won't we have
> an overall code shrink even when it is enabled in DSA?
> 
> Maybe it is time for CONFIG_NET_DSA_MV88E6XXX_PTP to go away?

Unmodified:
wc -l:
   611 drivers/net/dsa/mv88e6xxx/hwtstamp.c
   180 drivers/net/dsa/mv88e6xxx/hwtstamp.h
   564 drivers/net/dsa/mv88e6xxx/ptp.c
   176 drivers/net/dsa/mv88e6xxx/ptp.h

size:
   text    data     bss     dec     hex filename
   4004       0      16    4020     fb4 drivers/net/dsa/mv88e6xxx/hwtstamp.o
 130360    3505     224  134089   20bc9 drivers/net/dsa/mv88e6xxx/mv88e6xxx.ko
   3917       0      64    3981     f8d drivers/net/dsa/mv88e6xxx/ptp.o

Current post-conversion:
wc -l:
   459 drivers/net/dsa/mv88e6xxx/hwtstamp.c
   169 drivers/net/dsa/mv88e6xxx/hwtstamp.h
   418 drivers/net/dsa/mv88e6xxx/ptp.c
    83 drivers/net/dsa/mv88e6xxx/ptp.h

size:
   text    data     bss     dec     hex filename
   3988       0       0    3988     f94 drivers/net/dsa/mv88e6xxx/hwtstamp.o
 129312    3505     144  132961   20761 drivers/net/dsa/mv88e6xxx/mv88e6xxx.ko
   2709       0       0    2709     a95 drivers/net/dsa/mv88e6xxx/ptp.o

With a bit more effort to avoid the repeated chip->info->ops->foo
dereferecing chains in ptp.c, I can get this down to:

   text    data     bss     dec     hex filename
   2677       0       0    2677     a75 drivers/net/dsa/mv88e6xxx/ptp.o

So that reduces the PTP .text size by between 31 and 32%.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!