Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran <richardcochran@gmail.com>]

I have some issues with this patch...

On Fri, Sep 02, 2016 at 02:53:36PM +0200, Andrei Pistirica wrote:

> - Frequency adjustment is not directly supported by this IP.
>   addend is the initial value ns increment and similarly addendesub.
>   The ppb (parts per billion) provided is used as
>   ns_incr = addend +/- (ppb/rate).
>   Similarly the remainder of the above is used to populate subns increment.
>   In case the ppb requested is negative AND subns adjustment greater than
>   the addendsub, ns_incr is reduced by 1 and subns_incr is adjusted in
>   positive accordingly.

This makes no sense.  If you cannot adjust the frequency, then you
must implement a timecounter/cyclecounter and do in software.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3f385ab..8c3779d 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,12 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +#include <linux/net_tstamp.h>
> +#include <linux/ptp_clock.h>
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/skbuff.h>
> +#include <linux/timecounter.h>

Here in the header file, you only need ptp_clock_kernel.h.  You don't
need all the others here.  Move them to the .c file, but only if
really needed.  (timecounter.h isn't used, now is it?)

> @@ -129,6 +135,20 @@
>  #define GEM_RXIPCCNT		0x01a8 /* IP header Checksum Error Counter */
>  #define GEM_RXTCPCCNT		0x01ac /* TCP Checksum Error Counter */
>  #define GEM_RXUDPCCNT		0x01b0 /* UDP Checksum Error Counter */

> +#define GEM_TISUBN		0x01bc /* 1588 Timer Increment Sub-ns */

This regsiter does not exist.  Looking at
 
   Zynq-7000 AP SoC Technical Reference Manual
   UG585 (v1.10) February 23, 2015

starting on page 1273 we see:

udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment

There is no register at 0x1BC.

> +#define GEM_TSH			0x01c0 /* 1588 Timer Seconds High */

This one doesn't exist either.  What is going on here?

> +#define GEM_TSL			0x01d0 /* 1588 Timer Seconds Low */
> +#define GEM_TN			0x01d4 /* 1588 Timer Nanoseconds */
> +#define GEM_TA			0x01d8 /* 1588 Timer Adjust */
> +#define GEM_TI			0x01dc /* 1588 Timer Increment */
> +#define GEM_EFTSL		0x01e0 /* PTP Event Frame Tx Seconds Low */
> +#define GEM_EFTN		0x01e4 /* PTP Event Frame Tx Nanoseconds */
> +#define GEM_EFRSL		0x01e8 /* PTP Event Frame Rx Seconds Low */
> +#define GEM_EFRN		0x01ec /* PTP Event Frame Rx Nanoseconds */
> +#define GEM_PEFTSL		0x01f0 /* PTP Peer Event Frame Tx Secs Low */
> +#define GEM_PEFTN		0x01f4 /* PTP Peer Event Frame Tx Ns */
> +#define GEM_PEFRSL		0x01f8 /* PTP Peer Event Frame Rx Sec Low */
> +#define GEM_PEFRN		0x01fc /* PTP Peer Event Frame Rx Ns */

BTW, it is really annoying that you invent new register names.  Why
can't you use the names from the TRM?

> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +void macb_ptp_init(struct net_device *ndev);
> +#else
> +void macb_ptp_init(struct net_device *ndev) { }

This should be static inline.

> +#endif

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> new file mode 100644
> index 0000000..6d6a6ec
> --- /dev/null
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -0,0 +1,224 @@
> +/*
> + * PTP 1588 clock for SAMA5D2 platform.
> + *
> + * Copyright (C) 2015 Xilinx Inc.
> + * Copyright (C) 2016 Microchip Technology
> + *
> + * Authors: Harini Katakam <harinik@xilinx.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/etherdevice.h>
> +#include <linux/platform_device.h>
> +#include <linux/time64.h>
> +#include <linux/ptp_classify.h>
> +#include <linux/if_ether.h>
> +#include <linux/if_vlan.h>
> +
> +#include "macb.h"
> +
> +#define  GMAC_TIMER_NAME "gmac-ptp"
> +
> +static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
> +{
> +	u64 sech, secl;
> +
> +	/* get GEM internal time */
> +	sech = gem_readl(bp, TSH);
> +	secl = gem_readl(bp, TSL);

Does reading TSH latch the time?  The TRM is silent about that, and
most other designs latch on reading the LSB.

> +	ts->tv_sec = (sech << 32) | secl;
> +	ts->tv_nsec = gem_readl(bp, TN);
> +}
> +
> +static inline void macb_tsu_set_time(struct macb *bp,
> +				     const struct timespec64 *ts)
> +{
> +	u32 ns, sech, secl;
> +	s64 word_mask = 0xffffffff;
> +
> +	sech = (u32)ts->tv_sec;
> +	secl = (u32)ts->tv_sec;
> +	ns = ts->tv_nsec;
> +	if (ts->tv_sec > word_mask)
> +		sech = (ts->tv_sec >> 32);
> +
> +	/* set GEM internal time */

Writing three registers is supposed to be one operation, and yet you
have no mutex or spinlock to protect against concurrent settime calls.

> +	gem_writel(bp, TSH, sech);
> +	gem_writel(bp, TSL, secl);
> +	gem_writel(bp, TN, ns);

Also, how does the HW behave here?  Latch on TN write?

> +}
> +
> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> +	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
> +	unsigned long rate = bp->tsu_rate;
> +	u64 adjsub;
> +	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
> +	bool neg_adj = false;
> +
> +	if (ppb < 0) {
> +		neg_adj = true;
> +		ppb = -ppb;
> +	}
> +
> +	addend = bp->ns_incr;
> +	addendsub = bp->subns_incr;
> +
> +	diff = div_u64_rem(ppb, rate, &rem);

One ...

> +	addend = neg_adj ? addend - diff : addend + diff;
> +
> +	if (rem) {
> +		adjsub = rem;
> +		/* Multiple by 2^24 as subns field is 24 bits */
> +		adjsub = adjsub << 24;
> +
> +		diffsub = div_u64(adjsub, rate);

... Two ...

> +	} else {
> +		diffsub = 0;
> +	}
> +
> +	if (neg_adj && (diffsub > addendsub)) {
> +		addend -= 1;
> +		rem = (NSEC_PER_SEC - rem);
> +		neg_adj = false;
> +
> +		adjsub = rem;
> +		adjsub = adjsub << 24;
> +		diffsub = div_u64(adjsub, rate);

Three.  You need three 64 bit divisions?  There must be a better way.

If I guess correctly, the nsincr/subnsincr value is added to the
device's time on every clock.  So scaling these effectively adjusts
the frequency.  Here is an idea for you.

You have a 24 bit fractional field.  Combine that with 8 bits of whole
nanoseconds to form one 32 tuning word.  Restrict the nanoseconds to 8
bits in the setup code. (8 bits allows input clocks down to 4 MHz.)

Multiply the tuning word by the 32 ppb variable, keeping the 64 bit
result.  Divide the result by 10^9 and that gives you the delta to add
to (or subtract from) the nominal tuning word.

See gianfar.c for an example.

> +	}
> +
> +	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
> +	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
> +	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
> +		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
> +
> +	gem_writel(bp, TISUBN, subnsreg);
> +	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
> +
> +	return 0;
> +}

Thanks,
Richard