Date:   Sun, 24 Apr 2022 16:27:06 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>,
        bcm-kernel-feedback-list@...adcom.com, andrew@...n.ch,
        hkallweit1@...il.com, linux@...linux.org.uk,
        richardcochran@...il.com
Cc:     netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH net-next v1 1/4] net: phy: broadcom: Add PTP support for
 some Broadcom PHYs.



On 4/23/2022 7:23 PM, Jonathan Lemon wrote:
> This adds PTP support for BCM54210E Broadcom PHYs, in particular,
> the BCM54213PE, as used in the Rasperry PI CM4.  It has only been
> tested on that hardware.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
> ---

[snip]

Mostly checking register names/offsets/usage because I am not familiar 
enough with how PTP is supposed to work.

> +#define MODE_SEL_SHIFT_PORT		0
> +#define MODE_SEL_SHIFT_CPU		8
> +
> +#define rx_mode(sel, evt, act) \
> +	(((MODE_RX_##act) << (MODE_EVT_SHIFT_##evt)) << (MODE_SEL_SHIFT_##sel))
> +
> +#define tx_mode(sel, evt, act) \
> +	(((MODE_TX_##act) << (MODE_EVT_SHIFT_##evt)) << (MODE_SEL_SHIFT_##sel))

I would capitalize these two macros to make it clear that they are 
exactly that, macros.

> +
> +/* needs global TS capture first */
> +#define TX_TS_CAPTURE		0x0821
> +#define  TX_TS_CAP_EN			BIT(0)
> +#define RX_TS_CAPTURE		0x0822
> +#define  RX_TS_CAP_EN			BIT(0)
> +
> +#define TIME_CODE_0		0x0854

Maybe add a macro here as well:

#define TIME_CODE(x)		(TIME_CODE0 + (x))

> +#define TIME_CODE_1		0x0855
> +#define TIME_CODE_2		0x0856
> +#define TIME_CODE_3		0x0857
> +#define TIME_CODE_4		0x0858
> +
> +#define DPLL_SELECT		0x085b
> +#define  DPLL_HB_MODE2			BIT(6)

Seems like you have better documentation than I do :)

> +#define SHADOW_CTRL		0x085c

Unused

> +#define SHADOW_LOAD		0x085d
> +#define  TIME_CODE_LOAD			BIT(10)
> +#define  SYNC_OUT_LOAD			BIT(9)
> +#define  NCO_TIME_LOAD			BIT(7)

NCO Divider load is bit 8 and Local time Load bit is 7, can you check 
which one you need?

> +#define  FREQ_LOAD			BIT(6)
> +#define INTR_MASK		0x085e
> +#define INTR_STATUS		0x085f
> +#define  INTC_FSYNC			BIT(0)
> +#define  INTC_SOP			BIT(1)
> +
> +#define FREQ_REG_LSB		0x0873
> +#define FREQ_REG_MSB		0x0874

Those two hold the NCO frequency, can you rename accordingly?

> +
> +#define NCO_TIME_0		0x0875
> +#define NCO_TIME_1		0x0876
> +#define NCO_TIME_2_CTRL		0x0877
> +#define  FREQ_MDIO_SEL			BIT(14)
> +
> +#define SYNC_OUT_0		0x0878
> +#define SYNC_OUT_1		0x0879
> +#define SYNC_OUT_2		0x087a

Likewise a macro could be useful here.

> +
> +#define TS_READ_CTRL		0x0885
> +#define  TS_READ_START			BIT(0)
> +#define  TS_READ_END			BIT(1)
> +
> +#define TIMECODE_CTRL		0x08c3
> +#define  TX_TIMECODE_SEL		GENMASK(7, 0)
> +#define  RX_TIMECODE_SEL		GENMASK(15, 8)

This one looks out of order compared to the other registers

> +
> +#define TS_REG_0		0x0889 > +#define TS_REG_1		0x088a
> +#define TS_REG_2		0x088b
> +#define TS_REG_3		0x08c4
> +#define TS_INFO_0		0x088c
> +#define TS_INFO_1		0x088d
> +
> +#define HB_REG_0		0x0886
> +#define HB_REG_1		0x0887
> +#define HB_REG_2		0x0888
> +#define HB_REG_3		0x08ec > +#define HB_REG_4		0x08ed


> +#define HB_STAT_CTRL		0x088e
> +#define  HB_READ_START			BIT(10)
> +#define  HB_READ_END			BIT(11)
> +#define  HB_READ_MASK			GENMASK(11, 10)
> +
> +#define NSE_CTRL		0x087f

This one is also out of order.

[snip]
> +struct bcm_ptp_private *bcm_ptp_probe(struct phy_device *phydev)
> +{
> +	struct bcm_ptp_private *priv;
> +	struct ptp_clock *clock;
> +
> +	switch (BRCM_PHY_MODEL(phydev)) {
> +	case PHY_ID_BCM54210E:
> +#ifdef PHY_ID_BCM54213PE
> +	case PHY_ID_BCM54213PE:
> +#endif

This does not exist upstream.
-- 
Florian

Powered by blists - more mailing lists