Messages in this thread | | | Date | Fri, 5 Nov 2021 19:32:53 -0700 | Subject | Re: [PATCH 4/7] net: dsa: b53: Add PHC clock support | From | Florian Fainelli <> |
| |
On 11/4/2021 6:31 AM, Martin Kaistra wrote: > The BCM53128 switch has an internal clock, which can be used for > timestamping. Add support for it. > > The 32-bit free running clock counts nanoseconds. In order to account > for the wrap-around at 999999999 (0x3B9AC9FF) while using the cycle > counter infrastructure, we need to set a 30bit mask and use the > overflow_point property. > > Enable the Broadsync HD timestamping feature in b53_ptp_init() for PTPv2 > Ethertype (0x88f7). > > Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de> > ---
[snip]
> +int b53_ptp_init(struct b53_device *dev) > +{ > + mutex_init(&dev->ptp_mutex); > + > + INIT_DELAYED_WORK(&dev->overflow_work, b53_ptp_overflow_check); > + > + /* Enable BroadSync HD for all ports */ > + b53_write16(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_EN_CTRL1, 0x00ff);
Can you do this for all enabled user ports instead of each port, that way it is clera that this register is supposed to be a bitmask of ports for which you desire PTP timestamping to be enabled?
> + > + /* Enable BroadSync HD Time Stamping Reporting (Egress) */ > + b53_write8(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TS_REPORT_CTRL, 0x01);
Can you add a define for this bit in b53_regs.h and name it:
#define TSRPT_PKT_EN BIT(0)
which will enable timestamp reporting towards the IMP port.
> + > + /* Enable BroadSync HD Time Stamping for PTPv2 ingress */ > + > + /* MPORT_CTRL0 | MPORT0_TS_EN */ > + b53_write16(dev, B53_ARLCTRL_PAGE, 0x0e, (1 << 15) | 0x01);
Please add a definition for 0x0e which is the multi-port control register and is 16-bit wide.
Bit 15 is MPORT0_TS_EN and it will ensure that packets matching multiport 0 (address or ethertype) will be timestamped.
and then add a macro or generic definitions that are applicable to all multiport control registers, something like:
#define MPORT_CTRL_DIS_FORWARD 0 #define MPORT_CTRL_CMP_ADDR 1 #define MPORT_CTRL_CMP_ETYPE 2 #define MPORT_CTRL_CMP_ADDR_ETYPE 3
#define MPORT_CTRL_SHIFT(x) ((x) << 2) #define MPORT_CTRL_MASK 0x3
> + /* Forward to IMP port 8 */ > + b53_write64(dev, B53_ARLCTRL_PAGE, 0x18, (1 << 8));
0x18 is the multiport vector N register so we would want a macro to define the multiprot vector being used (up to 6 of them), and this is a 32-bit register, not a 64-bit register. The 8 here should be checked against the actual CPU port index number, it is 8 for you, it could be 5 for someone else, or 7, even.
> + /* PTPv2 Ether Type */ > + b53_write64(dev, B53_ARLCTRL_PAGE, 0x10, (u64)0x88f7 << 48);
Use ETH_P_1588 here and 0x10 deserves a define which is the multiport address N register. Likewise, we need a base offset of 0x10 and then a macro to address the 6 multiports that exists.
> + > + /* Setup PTP clock */ > + dev->ptp_clock_info.owner = THIS_MODULE; > + snprintf(dev->ptp_clock_info.name, sizeof(dev->ptp_clock_info.name), > + dev_name(dev->dev)); > + > + dev->ptp_clock_info.max_adj = 1000000000ULL; > + dev->ptp_clock_info.n_alarm = 0; > + dev->ptp_clock_info.n_pins = 0; > + dev->ptp_clock_info.n_ext_ts = 0; > + dev->ptp_clock_info.n_per_out = 0; > + dev->ptp_clock_info.pps = 0;
memset the structure ahead of time so you only need explicit initialization where needed?
> + dev->ptp_clock_info.adjfine = b53_ptp_adjfine; > + dev->ptp_clock_info.adjtime = b53_ptp_adjtime; > + dev->ptp_clock_info.gettime64 = b53_ptp_gettime; > + dev->ptp_clock_info.settime64 = b53_ptp_settime; > + dev->ptp_clock_info.enable = b53_ptp_enable; > + > + dev->ptp_clock = ptp_clock_register(&dev->ptp_clock_info, dev->dev); > + if (IS_ERR(dev->ptp_clock)) > + return PTR_ERR(dev->ptp_clock); > + > + /* The switch provides a 32 bit free running counter. Use the Linux > + * cycle counter infrastructure which is suited for such scenarios. > + */ > + dev->cc.read = b53_ptp_read; > + dev->cc.mask = CYCLECOUNTER_MASK(30); > + dev->cc.overflow_point = 999999999; > + dev->cc.mult = (1 << 28); > + dev->cc.shift = 28; > + > + b53_write32(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TIMEBASE_ADJ1, 40);
You are writing the default value of the register, is that of any use? -- Florian
| |