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

[<Arun.Ramadoss@microchip.com>]

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@microchip.com 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@microchip.com 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