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

Vladimir Oltean
2025-09-16T08:46:50Z

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
>