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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!