lkml.org 
[lkml]   [2021]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 4/7] net: dsa: b53: Add PHC clock support
From


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

\
 
 \ /
  Last update: 2021-11-06 03:33    [W:0.155 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site