Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <666b195b-e7d7-6f1f-e09d-bfe113c2f4fe@gmail.com>
Date:   Fri, 5 Nov 2021 19:32:53 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH 4/7] net: dsa: b53: Add PHC clock support
Content-Language: en-US
To:     Martin Kaistra <martin.kaistra@linutronix.de>,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>
Cc:     Richard Cochran <richardcochran@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        John Stultz <john.stultz@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Stephen Boyd <sboyd@kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
References: <20211104133204.19757-1-martin.kaistra@linutronix.de>
 <20211104133204.19757-5-martin.kaistra@linutronix.de>
From:   Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20211104133204.19757-5-martin.kaistra@linutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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