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

From: Arun.Ramadoss
Date: Thu Oct 27 2022 - 11:52:08 EST


Hi Christian,
On Thu, 2022-10-27 at 00:44 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Oct 26, 2022 at 06:47:53PM +0200, Christian Eggers wrote:
> > Hi Arun, hi Vladimir,
> >
> > On Tuesday, 18 October 2022, 15:42:41 CEST,
> > Arun.Ramadoss@xxxxxxxxxxxxx wrote:
> > > ...
> > > Thanks Vladimir. I will wait for Christian feedback.
> > >
> > > Hi Christian,
> > > To test this patch on KSZ9563, we need to increase the number of
> > > interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id
> > > of
> > > KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same
> > > for
> > > both chips. But this chip differ with number of port interrupts.
> > > So we
> > > need to update it. We are generating a new patch for adding the
> > > new
> > > element in the ksz_chip_data for KSZ9563.
> > > For now, you can update the code as below for testing the patch
> >
> > today I hard first success with your patch series on KSZ9563! ptp4l
> > reported
> > delay measurements between switch port 1 and the connected Meinberg
> > clock:

I tried to bring up the KSZ9563 setup following are my observation
- With this patch series, I am getting the Null pointer exception.
- After applying the patch provided by you, switch probe is successful.

Usually I test the gPTP using the following command
# ptp4l -f ~/ptp4l/gPTP.cfg -i lan1

How did you test this PTP in your setup, so that I can also get the
same result as below.

> >
> > > ptp4l[75.590]: port 2: new foreign master ec4670.fffe.0a9dcc-1
> > > ptp4l[79.590]: selected best master clock ec4670.fffe.0a9dcc
> > > ptp4l[79.590]: updating UTC offset to 37
> > > ptp4l[79.591]: port 2: LISTENING to UNCALIBRATED on RS_SLAVE
> > > ptp4l[81.114]: port 2: delay timeout
> > > ptp4l[81.117]: delay filtered 338 raw 338
> > > ptp4l[81.118]: port 2: minimum delay request interval 2^1
> > > ptp4l[81.434]: port 1: announce timeout
> > > ptp4l[81.434]: config item lan0.udp_ttl is 1
> > > ptp4l[81.436]: config item (null).dscp_event is 0
> > > ptp4l[81.437]: config item (null).dscp_general is 0
> > > ptp4l[81.437]: selected best master clock ec4670.fffe.0a9dcc
> > > ptp4l[81.438]: updating UTC offset to 37
> > > ptp4l[81.843]: master offset 33 s0 freq +6937 path
> > > delay 338
> > > ptp4l[82.844]: master offset 26 s2 freq +6930 path
> > > delay 338
> > > ptp4l[82.844]: port 2: UNCALIBRATED to SLAVE on
> > > MASTER_CLOCK_SELECTED
> > > ptp4l[83.844]: master offset 32 s2 freq +6962 path
> > > delay 338
> > > ptp4l[84.844]: master offset 3 s2 freq +6943 path
> > > delay 338
> > > ptp4l[85.844]: master offset -14 s2 freq +6927 path
> > > delay 338
> > > ptp4l[86.042]: port 2: delay timeout
> > > ptp4l[86.045]: delay filtered 336 raw 335
> > > ptp4l[86.211]: port 2: delay timeout
> > > ptp4l[86.213]: delay filtered 335 raw 331
> > > ptp4l[86.844]: master offset 3 s2 freq +6939 path
> > > delay 335
> > > ptp4l[87.847]: master offset -7 s2 freq +6930 path
> > > delay 335
> >
> > As a next step I'll try to configure the external output for 1PPS.
> > Is this
> > already implemented in your patches? The file
> > /sys/class/ptp/ptp2/n_periodic_outputs
> > shows '0' on my system.
>
> 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.
>
> >
> > 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)?
>
> > > 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.
> >
> > On Tuesday, 18 October 2022, 08:44:04 CEST,
> > Arun.Ramadoss@xxxxxxxxxxxxx wrote:
> > > I had developed this patch set to add gPTP support for LAN937x
> > > based on
> > > the Christian eggers patch for KSZ9563.
> >
> > Maybe this could be mentioned somewhere (e.g. extra line in file
> > header of
> > ksz_ptp.c).

Sure, I will add it in the File Header in the next version.

> > It took a lot of effort (for me) to get this initially running
> > (e.g. due to limited documentation / support by Microchip). But
> > I'm quite happy
> > that this is continued now as it is likely that I'll need PTP
> > support for the
> > KSZ9563 soon.
> >
> > For KSZ9563, we will need support for 1-step time stamping as two-
> > step
> > is not possible.
> >
> > I've stashed all my local changes into an additional patch (see
> > below).
> > Please feel free to integrate this into your series. As soon I get
> > 1PPS
> > running, I'll continue testing.

I thought 1PPS and periodic output are same, So I sent the 1PPS patch.
I need to look into periodic output.

> > Note that I'll be unavailable between Friday
> > and next Tuesday.

If you can elaborate the test need to be done in KSZ9563, I can try to
do during your Vacation.

> >
> > regards,
> > Christian
> > 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?
>
> > 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.
>
> > +
> > +/* 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.
>
> 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.
>
> This is not used, right? Because the function call is shortcircuited
> by
> the "if (test_bit(KSZ_HWTS_EN, &priv->state))" test earlier.
>
> *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