Re: [RFC Patch net-next 0/6] net: dsa: microchip: add gPTP support for LAN937x switch

[Christian Eggers]

Hi Vladimir,

On Wednesday, 26 October 2022, 23:44:55 CEST, Vladimir Oltean wrote:
> Arun didn't share the PPS output patch publicly, so I don't know why
> we're discussing this here. Anyway, in it, Arun (incorrectly)
> implemented support for PTP_CLK_REQ_PPS rather than PTP_CLK_REQ_PEROUT,
> so there will not be any n_periodic_outputs visible in sysfs. For now,
> try via pps_available and pps_enable.
I can continue testing this week.  I can either try with the (incorrect)
PTP_CLK_REQ_PPS or I can try to forward port my earlier patches.

> > BTW: Which is the preferred delay measurement which I shall test (E2E/P2P)?
> 
> As this time around there is somebody from Microchip finally on the
> line, I will not interfere in this part. I tried once, and failed to
> understand the KSZ PTP philosophy. I hope you get some answers from
> Arun. Just one question below.
> 
> > I started with E2E is this was configured in the hardware and needs no 1-step
> > time stamping, but I had to add PTP_MSGTYPE_DELAY_REQ in ksz_port_txtstamp().
> 
> Hm? So if E2E "doesn't need" 1-step TX timestamping and KSZ9563 doesn't
> support 2-step TX timestamping, then what kind of TX timestamping is
> used here for Delay_Req messages?
> Perhaps you mean that E2E doesn't need moving the RX timestamp of the
> Pdelay_Req (t2) into the KSZ TX timestamp trailer of the Pdelay_Resp (t3)?
I think that Delay_Req is not related to 1-step / 2-step time stamping. As far
as I understand, this is only relevant for SYNC and PDelay_Resp.

> > > May be this is due to kconfig of config_ksz_ptp  defined bool instead
> > > of tristate. Do I need to change the config_ksz_ptp to tristate in
> > > order to compile as modules?
> > 
> > I'm not an expert for kbuild and cannot tell whether it's allowed to use
> > bool options which depend on tristate options. At least ksz_ptp.c is compiled
> > by kbuild if tristate is used. But I needed to add additional EXPORT_SYMBOL()
> > statements for all non-static functions (see below) for successful linking.
> 
> If ksz_ptp.o gets linked into ksz_ptp.ko, then yes. But this probably
> doesn't make sense, as you point out. So EXPORT_SYMBOL() should not be
> needed.
> 
> > I'm unsure whether it makes sense to build ksz_ptp as a separate module.
> > Perhaps it should be (conditionally) added to ksz_switch.ko.

So let's conditionally add ksz_ptp.o to ksz_switch.ko.

> >  static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
> >  				   struct hwtstamp_config *config)
> > @@ -106,7 +108,7 @@ static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
> >  	case HWTSTAMP_TX_OFF:
> >  		prt->hwts_tx_en = false;
> >  		break;
> > -	case HWTSTAMP_TX_ON:
> > +	case HWTSTAMP_TX_ONESTEP_P2P:
> 
> One shouldn't replace the other; this implementation is simplistic, of course.
> 
> Also, why did you choose HWTSTAMP_TX_ONESTEP_P2P and not HWTSTAMP_TX_ONESTEP_SYNC?
Because my (old) ptp4l.conf was configured for P2P+p2p1step.  When I started working on
KSZ9563 PTP two years ago, you suggested doing P2P first, because E2E is affected by nasty
packet filters on the switch hardware...  But for the first tests now I switched to E2E
for the reasons you mentioned above.

Probably HWTSTAMP_TX_ON needs to be rejected for KSZ9563 and only HWTSTAMP_TX_ONESTEP_SYNC
(for E2E) and HWTSTAMP_TX_ONESTEP_P2P (for P2P) should be accepted.

> 
> >  		prt->hwts_tx_en = true;
> >  		break;
> >  	default:
> > @@ -162,6 +164,7 @@ int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
> >  	mutex_unlock(&ptp_data->lock);
> >  	return ret;
> >  }
> > +EXPORT_SYMBOL(ksz_hwtstamp_set);
> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index 582add3398d3..e7680718b478 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c
> > @@ -251,17 +251,69 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477);
> >  #define KSZ9893_TAIL_TAG_OVERRIDE	BIT(5)
> >  #define KSZ9893_TAIL_TAG_LOOKUP		BIT(6)
> > 
> > +/* Time stamp tag is only inserted if PTP is enabled in hardware. */
> > +static void ksz9893_xmit_timestamp(struct sk_buff *skb)
> > +{
> > +//	struct sk_buff *clone = KSZ9477_SKB_CB(skb)->clone;
> > +//	struct ptp_header *ptp_hdr;
> > +//	unsigned int ptp_type;
> > +	u32 tstamp_raw = 0;
> > +	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ9477_PTP_TAG_LEN));
> > +}
> 
> This is needed for one-step TX timestamping, ok.
Yes, here is some work left for 1-step PDelay_Resp.

> 
> > +
> > +/* Defer transmit if waiting for egress time stamp is required.  */
> > +static struct sk_buff *ksz9893_defer_xmit(struct dsa_port *dp,
> > +					  struct sk_buff *skb)
> 
> No need to duplicate, can rename lan937x_defer_xmit() and call that.
I wanted only "to make it run", I let the details for Arun.

> 
> Although I'm not exactly clear *which* packets will need deferred
> transmission on ksz9xxx. To my knowledge, such a procedure is only
> necessary for 2-step TX timestamping, when the TX timestamp must be
> propagated back to the socket error queue via skb_complete_tx_timestamp().
> For one-step, AFAIK*, this isn't needed.
As far as I remember, deferred xmit is needed for outgoing time stamps,
which are reported back to the application via the socket's error queue.
Because the KSZ time stamp unit only reports the time stamp itself (but
no sequence number or similar), only a single outgoing packet is allowed
to pass the TS unit at once. Otherwise no mapping between the TS and the
skb would be impossible.

> 
> This is not used, right? Because the function call is shortcircuited by
> the "if (test_bit(KSZ_HWTS_EN, &priv->state))" test earlier.
I am, quite sure that ksz9893_defer_xmit() is actually required.

> 
> *Or is this intended to be used for the "Software Two-Step Simulation
> Mode in hardware 1-Step Mode" that was suggested in the errata sheet,
> where one-step Sync messages still get their TX timestamp reported to
> user space as if they were two-step?
> http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9563R-Errata-80000786B.pdf
No, I didn't try to got that way.

regards,
Christian