Re: [RFC PATCH net-next 7/9] net: dsa: microchip: ksz9477: add hardware time stamping support

From: Christian Eggers <ceggers@arri.de>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Richard Cochran <richardcochran@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Helmut Grohne <helmut.grohne@intenta.de>,
	Paul Barker <pbarker@konsulko.com>,
	Codrin Ciubotariu <codrin.ciubotariu@microchip.com>,
	George McCollister <george.mccollister@gmail.com>,
	Marek Vasut <marex@denx.de>,
	Tristram Ha <Tristram.Ha@microchip.com>,
	"David S . Miller" <davem@davemloft.net>,
	Woojung Huh <woojung.huh@microchip.com>,
	"Microchip Linux Driver Support" <UNGLinuxDriver@microchip.com>,
	<netdev@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH net-next 7/9] net: dsa: microchip: ksz9477: add hardware time stamping support
Date: Mon, 2 Nov 2020 11:35:00 +0100	[thread overview]
Message-ID: <1779456.uGjeJ53Q7B@n95hx1g2> (raw)
In-Reply-To: <20201101234149.rrhrjiyt7l4orkm7@skbuf>

On Monday, 2 November 2020, 00:41:49 CET, Vladimir Oltean wrote:
> On Sun, Nov 01, 2020 at 11:14:24PM +0100, Christian Eggers wrote:
> > My assumption is that the KSZ9563 simply doesn't forward specific PTP
> > packages from the slave ports to the CPU port. In my imagination this
> > happens in hardware and is not visible in software.
> 
> You talked about tracking the BMCA by snooping Announce messages. I
> don't think that is going to be the path forward either. Think about the
> general case, where there might not even be a BMCA (like in the
> automotive profile).
Maybe my mail from October, 22 was ambiguous. I meant that despite of the 
presence of filtering, a BCMA algorithm should be about to work (as no 
Announce messages are filtered out).

Additionally I said, that switching between "master" and "slave" mode could 
not be done automatically by the driver, as the driver could at most detect 
the presence of Sync messages (indication for master mode), but would do hard 
to detect a transition to slave mode.

I see a chance that user space (ptp4l) could configure the appropriate 
"hardware filter setup" for master/slave mode. 

> It almost seems to me as if the hardware is trying to be helpful by
> dropping the PTP messages that the application layer would drop anyway.
> Too bad that nobody seems to have told them to make this helpful
> mechanism optional, because as it is, it's standing in the way more than
> helping.
I think the same. Maybe there is some undocumented "filter disable" bit, but 
this information must come from Microchip.

> You know what the real problem is, with DSA you don't have the concept
> of the host port being an Ordinary Clock. DSA does not treat the host
> port as a switched endpoint (aka a plain net device attached to a dumb
> switch, and unaware of that switch), but instead is the conduit interface
> for each front-panel switch interface, which is an individually
> addressable network interface in its own right. You are not supposed to
> use a DSA master interface for networking directly, not for regular
> networking and not for PTP. In fact, DSA-enabled ports, only the PTP
> clock of the switch is usable. If you attempt to run ptp4l on the master
> interface an error will be thrown back at you.
> 
> Why am I mentioning this? Because the setting that's causing trouble for
> us is 'port state of the host port OC', which in the context of what I
> said above is nonsense. There _is_ no host port OC. There are 2 switch
> ports which can act as individual OCs, or as a BC, or as a TC.
But the switch has only one clock at all. I assume that the switch cannot be a 
boundary clock, only TC seems possible.

> But
> consider the case when the switch is a BC, with one of the front-panel
> ports being a master and the other a slave. What mode are you supposed
> to put the host port in, so that it receives both the Sync packets from
> the slave port, and the Delay_Req packets from the master port?! It just
> makes no sense to me. In principle I don't see any reason why this
> switch would not be able to operate as a one-step peer delay BC.
> 
> Unless somebody from Microchip could shed some light on the design
> decisions of this switch (and there are enough Microchip people copied
> already), here's what I would do. I would cut my losses and just support
> peer delay, no E2E delay request-response mechanism (this means you'll
> need to deploy peer delay to all devices within your network, but the
> gains might be worth it). Because peer delay is symmetrical (both link
> partners are both requestors as well as responders), there's no help in
> the world that this switch could volunteer to give you in dropping
> packets on your behalf. So I expect that if you hardcode:
> - the port state for the host port OC as slave, then you'd get the Sync
>   messages from all ports, and the Delay_Req messages would be dropped
>   but you wouldn't care about those anyway, and
> - the selection of TC mode to P2P TC.
When using only P2P, setting the OCMODE bit to "slave" should work.

> Then I would negotiate with Richard whether it's ok to add these extra
> values to enum hwtstamp_rx_filters:
> 	HWTSTAMP_FILTER_PTP_V2_PDELAY
> 	HWTSTAMP_FILTER_PTP_V2_L4_PDELAY
>
As said above, having "filter setups" for E2E/P2P and for MASTER/SLAVE would 
probably fit well for this kind of hardware.

> Given the fact that you're only limiting the timestamping to Pdelay
> because we don't fully understand the hardware, I don't really know
> whether introducing UAPI for this one situation is justifiable. If not,
> then your driver will not have a chance to reject ptp4l -E, and it will
> Simply Not Work.