Re: [PATCH 4/7] net: dsa: b53: Add PHC clock support

[Martin Kaistra]

Am 06.11.21 um 03:32 schrieb 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@xxxxxxxxxxxxx>
> > ---
>
> [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?

Appearently not, I just tested it without this line and it seems to work 
fine.

It just seemed strange to me, that while the datasheet mentions 40 as 
the default value, when reading the register without writing this 
initial value, I just get back 0.

I'll remove the line for v2.

Thanks,
Martin