[RFC,net-next,v3,1/2] macb: Add 1588 support in Cadence GEM.

Cadence GEM provides a 102 bit time counter with 48 bits for seconds,
30 bits for nsecs and 24 bits for sub-nsecs to control 1588 timestamping.

This patch does the following:
- Registers to ptp clock framework
- Timer initialization is done by writing time of day to the timer counter.
- ns increment register is programmed as NSEC_PER_SEC/tsu-clock-rate.
  For a 16 bit subns precision, the subns increment equals
  remainder of (NS_PER_SEC/TSU_CLK) * (2^16).
- Timestamps are obtained from the TX/RX PTP event/PEER registers.
  The timestamp obtained thus is updated in skb for upper layers to access.
- The drivers register functions with ptp to perform time and frequency
  adjustment.
- Time adjustment is done by writing to the 1558_ADJUST register.
  The controller will read the delta in this register and update the timer
  counter register. Alternatively, for large time offset adjustments,
  the driver reads the secs and nsecs counter values, adds/subtracts the
  delta and updates the timer counter.
- Frequency is adjusted by adjusting addend (8bit nanosecond increment) and
  addendsub (16bit increment nanosecond fractions).
  The 102bit counter is incremented at nominal frequency with addend and
  addendsub values. Each period addend and addendsub values are adjusted
  based on ppm drift.

Signed-off-by: Andrei Pistirica <andrei.pistirica@microchip.com>
Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
Patch history:

Version 1:
This patch is based on original Harini's patch, implemented in a
separate file to ease the review/maintanance and integration with
other platforms (e.g. Zynq Ultrascale+ MPSoC).
Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp
project and also with ptpd2 version 2.3.1. PTP was tested over
IPv4,IPv6 and 802.3 protocols.

In case that macb is compiled as a module, it has been renamed to
cadence-macb.ko to avoid naming confusion in Makefile.

Version 2 modifications:
- bitfields for TSU are named according to SAMA5D2 data sheet
- identify GEM-PTP support based on platform capability
- add spinlock for TSU access
- change macb_ptp_adjfreq and use fewer 64bit divisions

Version 3 modifications:
- new adjfine api with one 64 division for frequency adjustment 
  (based on Richard's input)
- add maximum adjustment frequency (ppb) based on nominal frequency
- per platform PTP configuration
- cosmetic changes
Note 1: Kbuild uses "select" instead of "imply", and the macb maintainer agreed
        to make the change when it will be available in net-next.
Note 2: Guys, this driver does not support GEM-GXL!
Note 3: Patch on net-next, on December 7th.

 drivers/net/ethernet/cadence/Kconfig    |  10 +-
 drivers/net/ethernet/cadence/Makefile   |   8 +-
 drivers/net/ethernet/cadence/macb.h     | 102 +++++++++
 drivers/net/ethernet/cadence/macb_ptp.c | 390 ++++++++++++++++++++++++++++++++
 4 files changed, 508 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/cadence/macb_ptp.c

On Wed, Dec 07, 2016 at 08:21:51PM +0200, Andrei Pistirica wrote:
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +void gem_ptp_init(struct net_device *ndev);
> +void gem_ptp_remove(struct net_device *ndev);
> +
> +void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb);
> +void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb);

These are in the hot path, and so you should do the test before
calling the global function, something like this:

void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb);

static void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb)
{
	if (!bp->hwts_tx_en)
		return;
	gem_ptp_txstamp(bp, skb);
}

Ditto for Rx.

> +#else
> +static inline void gem_ptp_init(struct net_device *ndev) { }
> +static inline void gem_ptp_remove(struct net_device *ndev) { }
> +
> +static inline void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb) { }
> +static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb) { }
> +#endif
> +

> +static int gem_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{
> +	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
> +	u32 word, diff;
> +	u64 adj, rate;
> +	int neg_adj = 0;
> +
> +	if (scaled_ppm < 0) {
> +		neg_adj = 1;
> +		scaled_ppm = -scaled_ppm;
> +	}
> +	rate = scaled_ppm;
> +
> +	/* word: unused(8bit) | ns(8bit) | fractions(16bit) */
> +	word = (bp->ns_incr << 16) + bp->subns_incr;
> +
> +	adj = word;
> +	adj *= rate;
> +	adj >>= 16; /* remove fractions */
> +	adj += 500000UL;
> +	diff = div_u64(adj, 1000000UL);

In order to round correctly, shouldn't this be?

	adj *= rate;
	adj += 500000UL << 16;
	adj >>= 16;
	diff = div_u64(adj, 1000000UL);

> +	word = neg_adj ? word - diff : word + diff;
> +
> +	spin_lock(&bp->tsu_clk_lock);
> +
> +	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, (word & 0xffff)));
> +	gem_writel(bp, TI, GEM_BF(NSINCR, (word >> 16)));
> +
> +	spin_unlock(&bp->tsu_clk_lock);
> +	return 0;
> +}

> +static s32 gem_ptp_max_adj(unsigned int f_nom)
> +{
> +	u64 adj;
> +
> +	/* The 48 bits of seconds for the GEM overflows every:
> +	 * 2^48/(365.25 * 24 * 60 *60) =~ 8 925 512 years (~= 9 mil years),
> +	 * thus the maximum adjust frequency must not overflow CNS register:
> +	 *
> +	 * addend  = 10^9/nominal_freq
> +	 * adj_max = +/- addend*ppb_max/10^9
> +	 * max_ppb = (2^8-1)*nominal_freq-10^9
> +	 */
> +	adj = f_nom;
> +	adj *= 0xffff;
> +	adj -= 1000000000ULL;

What is this computation, and how does it relate to the comment?

> +	return adj;
> +}

> +/* While GEM can timestamp PTP packets, it does not mark the RX descriptor

Does it timestamp PTP event packets only, or all packets?

(See my comment in patch 2/2)

> + * to identify them. UDP packets must be parsed to identify PTP packets.
> + *
> + * Note: Inspired from drivers/net/ethernet/ti/cpts.c
> + */

Thanks,
Richard

On Wed, Dec 07, 2016 at 08:39:09PM +0100, Richard Cochran wrote:
> > +static s32 gem_ptp_max_adj(unsigned int f_nom)
> > +{
> > +	u64 adj;
> > +
> > +	/* The 48 bits of seconds for the GEM overflows every:
> > +	 * 2^48/(365.25 * 24 * 60 *60) =~ 8 925 512 years (~= 9 mil years),
> > +	 * thus the maximum adjust frequency must not overflow CNS register:
> > +	 *
> > +	 * addend  = 10^9/nominal_freq
> > +	 * adj_max = +/- addend*ppb_max/10^9
> > +	 * max_ppb = (2^8-1)*nominal_freq-10^9
> > +	 */
> > +	adj = f_nom;
> > +	adj *= 0xffff;
> > +	adj -= 1000000000ULL;
> 
> What is this computation, and how does it relate to the comment?

I am not sure what you meant, but it sounds like you are on the wrong
track.  Let me explain...

The max_adj has nothing at all to do with the width of the time
register.  Rather, it should reflect the maximum possible change in
the tuning word.

For example, with a nominal 8 ns period, the tuning word is 0x80000.
Looking at running the clock more slowly, the slowest possible word is
0x00001, meaning a difference of 0x7FFFF.  This implies an adjustment
of 0x7FFFF/0x80000 or 999998092 ppb.  Running more quickly, we can
already have 0x100000, twice as fast, or just under 2 billion ppb.

You should consider the extreme cases to determine the most limited
(smallest) max_adj value:

Case 1 - high frequency
~~~~~~~~~~~~~~~~~~~~~~~

With a nominal 1 ns period, we have the nominal tuning word 0x10000.
The smallest is 0x1 for a difference of 0xFFFF.  This corresponds to
an adjustment of 0xFFFF/0x10000 = .9999847412109375 or 999984741 ppb.

Case 2 - low frequency
~~~~~~~~~~~~~~~~~~~~~~

With a nominal 255 ns period, the nominal word is 0xFF0000, the
largest 0xFFFFFF, and the difference is 0xFFFF.  This corresponds to
and adjustment of 0xFFFF/0xFF0000 = .0039215087890625 or 3921508 ppb.

Since 3921508 ppb is a huge adjustment, you can simply use that as a
safe maximum, ignoring the actual input clock.

Thanks,
Richard

Le 07/12/2016 à 20:39, Richard Cochran a écrit :
> On Wed, Dec 07, 2016 at 08:21:51PM +0200, Andrei Pistirica wrote:
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> +void gem_ptp_init(struct net_device *ndev);
>> +void gem_ptp_remove(struct net_device *ndev);
>> +
>> +void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb);
>> +void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb);
> 
> These are in the hot path, and so you should do the test before
> calling the global function, something like this:
> 
> void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb);
> 
> static void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb)
> {
> 	if (!bp->hwts_tx_en)
> 		return;
> 	gem_ptp_txstamp(bp, skb);
> }
> 
> Ditto for Rx.

Hi Richard,

So you mean that as the "global" function won't be "inlined" by the
compiler as the function is not "static" neither in the same file and
that the jump will be implemented anyway. And this even if the function
is only called at a single location...

This way, if we add a kind or accessors function like the one that you
propose, with the test in it, the branch prediction can play his role
without breaking the processor pipeline as the accessors function will
be inlined by the compiler: Am I right?

So, yes, makes sense. Thanks for the hint.

Regards,

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, December 07, 2016 11:04 PM
> To: Andrei Pistirica - M16132
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; davem@davemloft.net;
> nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
> harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
> anirudh@xilinx.com; boris.brezillon@free-electrons.com;
> alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com;
> rafalo@cadence.com
> Subject: Re: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in
> Cadence GEM.
> 
> On Wed, Dec 07, 2016 at 08:39:09PM +0100, Richard Cochran wrote:
> > > +static s32 gem_ptp_max_adj(unsigned int f_nom) {
> > > +	u64 adj;
> > > +
> > > +	/* The 48 bits of seconds for the GEM overflows every:
> > > +	 * 2^48/(365.25 * 24 * 60 *60) =~ 8 925 512 years (~= 9 mil years),
> > > +	 * thus the maximum adjust frequency must not overflow CNS
> register:
> > > +	 *
> > > +	 * addend  = 10^9/nominal_freq
> > > +	 * adj_max = +/- addend*ppb_max/10^9
> > > +	 * max_ppb = (2^8-1)*nominal_freq-10^9
> > > +	 */
> > > +	adj = f_nom;
> > > +	adj *= 0xffff;
> > > +	adj -= 1000000000ULL;
> >
> > What is this computation, and how does it relate to the comment?

I considered the following simple equation: increment value at nominal frequency (which is 10^9/nominal frequency nsecs) + the maximum drift value (nsecs) <= maximum increment value at nominal frequency (which is 8bit:0xffff).
If maximum drift is written as function of nominal frequency and maximum ppb, then the equation above yields that the maximum ppb is: (2^8 - 1) *nominal_frequency - 10^9. The equation is also simplified by the fact that the drift is written as ppm + 16bit_fractions and the increment value is written as nsec + 16bit_fractions.

Rafal said that this value is hardcoded: 0x64E6, while Harini said: 250000000.

I need to dig into this...

> 
> I am not sure what you meant, but it sounds like you are on the wrong track.
> Let me explain...

Thanks.

> 
> The max_adj has nothing at all to do with the width of the time register.
> Rather, it should reflect the maximum possible change in the tuning word.
> 
> For example, with a nominal 8 ns period, the tuning word is 0x80000.
> Looking at running the clock more slowly, the slowest possible word is
> 0x00001, meaning a difference of 0x7FFFF.  This implies an adjustment of
> 0x7FFFF/0x80000 or 999998092 ppb.  Running more quickly, we can already
> have 0x100000, twice as fast, or just under 2 billion ppb.
> 
> You should consider the extreme cases to determine the most limited
> (smallest) max_adj value:
> 
> Case 1 - high frequency
> ~~~~~~~~~~~~~~~~~~~~~~~
> 
> With a nominal 1 ns period, we have the nominal tuning word 0x10000.
> The smallest is 0x1 for a difference of 0xFFFF.  This corresponds to an
> adjustment of 0xFFFF/0x10000 = .9999847412109375 or 999984741 ppb.
> 
> Case 2 - low frequency
> ~~~~~~~~~~~~~~~~~~~~~~
> 
> With a nominal 255 ns period, the nominal word is 0xFF0000, the largest
> 0xFFFFFF, and the difference is 0xFFFF.  This corresponds to and adjustment
> of 0xFFFF/0xFF0000 = .0039215087890625 or 3921508 ppb.
> 
> Since 3921508 ppb is a huge adjustment, you can simply use that as a safe
> maximum, ignoring the actual input clock.
> 
> Thanks,
> Richard
> 
> 

Regards,
Andrei

Hi,

On Thu, Dec 8, 2016 at 8:11 PM,  <Andrei.Pistirica@microchip.com> wrote:
>
>
>> -----Original Message-----
>> From: Richard Cochran [mailto:richardcochran@gmail.com]
>> Sent: Wednesday, December 07, 2016 11:04 PM
>> To: Andrei Pistirica - M16132
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; davem@davemloft.net;
>> nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
>> harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
>> anirudh@xilinx.com; boris.brezillon@free-electrons.com;
>> alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com;
>> rafalo@cadence.com
>> Subject: Re: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in
>> Cadence GEM.
>>
>> On Wed, Dec 07, 2016 at 08:39:09PM +0100, Richard Cochran wrote:
>> > > +static s32 gem_ptp_max_adj(unsigned int f_nom) {
>> > > + u64 adj;
>> > > +
>> > > + /* The 48 bits of seconds for the GEM overflows every:
>> > > +  * 2^48/(365.25 * 24 * 60 *60) =~ 8 925 512 years (~= 9 mil years),
>> > > +  * thus the maximum adjust frequency must not overflow CNS
>> register:
>> > > +  *
>> > > +  * addend  = 10^9/nominal_freq
>> > > +  * adj_max = +/- addend*ppb_max/10^9
>> > > +  * max_ppb = (2^8-1)*nominal_freq-10^9
>> > > +  */
>> > > + adj = f_nom;
>> > > + adj *= 0xffff;
>> > > + adj -= 1000000000ULL;
>> >
>> > What is this computation, and how does it relate to the comment?
>
> I considered the following simple equation: increment value at nominal frequency (which is 10^9/nominal frequency nsecs) + the maximum drift value (nsecs) <= maximum increment value at nominal frequency (which is 8bit:0xffff).
> If maximum drift is written as function of nominal frequency and maximum ppb, then the equation above yields that the maximum ppb is: (2^8 - 1) *nominal_frequency - 10^9. The equation is also simplified by the fact that the drift is written as ppm + 16bit_fractions and the increment value is written as nsec + 16bit_fractions.
>
> Rafal said that this value is hardcoded: 0x64E6, while Harini said: 250000000.

@ Andrei, I may have equated max ppb to max tsu frequency allowed on
the system and set that.
That will be wrong.

>
> I need to dig into this...
>
>>
>> I am not sure what you meant, but it sounds like you are on the wrong track.
>> Let me explain...
>
> Thanks.
>
>>
>> The max_adj has nothing at all to do with the width of the time register.
>> Rather, it should reflect the maximum possible change in the tuning word.
>>
>> For example, with a nominal 8 ns period, the tuning word is 0x80000.
>> Looking at running the clock more slowly, the slowest possible word is
>> 0x00001, meaning a difference of 0x7FFFF.  This implies an adjustment of
>> 0x7FFFF/0x80000 or 999998092 ppb.  Running more quickly, we can already
>> have 0x100000, twice as fast, or just under 2 billion ppb.
>>
>> You should consider the extreme cases to determine the most limited
>> (smallest) max_adj value:
>>
>> Case 1 - high frequency
>> ~~~~~~~~~~~~~~~~~~~~~~~
>>
>> With a nominal 1 ns period, we have the nominal tuning word 0x10000.
>> The smallest is 0x1 for a difference of 0xFFFF.  This corresponds to an
>> adjustment of 0xFFFF/0x10000 = .9999847412109375 or 999984741 ppb.
>>
>> Case 2 - low frequency
>> ~~~~~~~~~~~~~~~~~~~~~~
>>
>> With a nominal 255 ns period, the nominal word is 0xFF0000, the largest
>> 0xFFFFFF, and the difference is 0xFFFF.  This corresponds to and adjustment
>> of 0xFFFF/0xFF0000 = .0039215087890625 or 3921508 ppb.
>>
>> Since 3921508 ppb is a huge adjustment, you can simply use that as a safe
>> maximum, ignoring the actual input clock.
>>

Thanks Richard.
So, if I understand right, this is theoretically limited by the
maximum input clock:
So if the highest frequency allowed (also commonly sourced in my case)
is 200MHz,
then with a 5ns time period, considering the adjustment to slowest
possible word,
0x4FFFF/0x50000 will be 999996948 ppb.
Shouldn't this be the max_adj?
I'm afraid I don't get why we are choosing the most limited max adj..
Sorry if I'm missing something - could you please help me understand?

Regards,
Harini

On Fri, Dec 09, 2016 at 11:07:23AM +0530, Harini Katakam wrote:
> I'm afraid I don't get why we are choosing the most limited max adj..
> Sorry if I'm missing something - could you please help me understand?

This max_adj is only important when the local clock offset is large
and user space chooses not to step the time value.  In that case, user
space will want to run the clock as fast (or as slow) as possible in
order to catch up with the remote clock.

The driver must provide a max_adj that is always safe for user space
to apply via the clock_adjtime() system call.

HTH,
Richard

-----Original Message-----
> From: Andrei.Pistirica@microchip.com [mailto:Andrei.Pistirica@microchip.com]
> Sent: 8 grudnia 2016 15:42
> To: richardcochran@gmail.com
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; davem@davemloft.net; nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com; harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com; anirudh@xilinx.com; boris.brezillon@free-electrons.com; alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com; Rafal Ozieblo
> Subject: RE: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in Cadence GEM.
>
>
>
> > -----Original Message-----
> > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > Sent: Wednesday, December 07, 2016 11:04 PM
> > To: Andrei Pistirica - M16132
> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; davem@davemloft.net;
> > nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
> > harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
> > anirudh@xilinx.com; boris.brezillon@free-electrons.com;
> > alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com;
> > rafalo@cadence.com
> > Subject: Re: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in
> > Cadence GEM.
> >
> > On Wed, Dec 07, 2016 at 08:39:09PM +0100, Richard Cochran wrote:
> > > > +static s32 gem_ptp_max_adj(unsigned int f_nom) {
> > > > +       u64 adj;
> > > > +
> > > > +       /* The 48 bits of seconds for the GEM overflows every:
> > > > +        * 2^48/(365.25 * 24 * 60 *60) =~ 8 925 512 years (~= 9 mil years),
> > > > +        * thus the maximum adjust frequency must not overflow CNS
> > register:
> > > > +        *
> > > > +        * addend  = 10^9/nominal_freq
> > > > +        * adj_max = +/- addend*ppb_max/10^9
> > > > +        * max_ppb = (2^8-1)*nominal_freq-10^9
> > > > +        */
> > > > +       adj = f_nom;
> > > > +       adj *= 0xffff;
> > > > +       adj -= 1000000000ULL;
> > >
> > > What is this computation, and how does it relate to the comment?
>
> I considered the following simple equation: increment value at nominal frequency (which is 10^9/nominal frequency nsecs) + the maximum drift value (nsecs) <= maximum increment value at nominal frequency (which is 8bit:0xffff).
> If maximum drift is written as function of nominal frequency and maximum ppb, then the equation above yields that the maximum ppb is: (2^8 - 1) *nominal_frequency - 10^9. The equation is also simplified by the fact that the drift is written as ppm + 16bit_fractions and the increment value is written as nsec + 16bit_fractions.
>
> Rafal said that this value is hardcoded: 0x64E6, while Harini said: 250000000.

To clarify a little bit. In my reference code this value (0x64E6) was taken from our legacy code. It was used for testing only. I know it should be change to something more accurate. This is the reason why I asked how did you count it (250000000). According to our calculations this value depends on actual set period (incr_ns and incr_sub_ns) and min and max value we can set. The calculation were a little bit intricate, so we decided to leave it as it was.

>
> I need to dig into this...
>
> >
> > I am not sure what you meant, but it sounds like you are on the wrong track.
> > Let me explain...
>
> Thanks.
>
> >
> > The max_adj has nothing at all to do with the width of the time register.
> > Rather, it should reflect the maximum possible change in the tuning word.
> >
> > For example, with a nominal 8 ns period, the tuning word is 0x80000.
> > Looking at running the clock more slowly, the slowest possible word is
> > 0x00001, meaning a difference of 0x7FFFF.  This implies an adjustment
> > of
> > 0x7FFFF/0x80000 or 999998092 ppb.  Running more quickly, we can
> > already have 0x100000, twice as fast, or just under 2 billion ppb.
> >
> > You should consider the extreme cases to determine the most limited
> > (smallest) max_adj value:
> >
> > Case 1 - high frequency
> > ~~~~~~~~~~~~~~~~~~~~~~~
> >
> > With a nominal 1 ns period, we have the nominal tuning word 0x10000.
> > The smallest is 0x1 for a difference of 0xFFFF.  This corresponds to
> > an adjustment of 0xFFFF/0x10000 = .9999847412109375 or 999984741 ppb.
> >
> > Case 2 - low frequency
> > ~~~~~~~~~~~~~~~~~~~~~~
> >
> > With a nominal 255 ns period, the nominal word is 0xFF0000, the
> > largest 0xFFFFFF, and the difference is 0xFFFF.  This corresponds to
> > and adjustment of 0xFFFF/0xFF0000 = .0039215087890625 or 3921508 ppb.
> >
> > Since 3921508 ppb is a huge adjustment, you can simply use that as a
> > safe maximum, ignoring the actual input clock.
> >
> > Thanks,
> > Richard
> >
> >
>
> Regards,
> Andrei
>

Best regards,
Rafal Ozieblo   |   Firmware System Engineer, 
phone nbr.: +48 32 5085469
www.cadence.com

> -----Original Message-----
> From: Rafal Ozieblo [mailto:rafalo@cadence.com]
> Sent: Friday, December 09, 2016 11:20 AM
> To: Andrei Pistirica - M16132; richardcochran@gmail.com
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; davem@davemloft.net;
> nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
> harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
> anirudh@xilinx.com; boris.brezillon@free-electrons.com;
> alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com
> Subject: RE: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in Cadence
> GEM.
> 
> -----Original Message-----
> > From: Andrei.Pistirica@microchip.com
> > [mailto:Andrei.Pistirica@microchip.com]
> > Sent: 8 grudnia 2016 15:42
> > To: richardcochran@gmail.com
> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; davem@davemloft.net;
> > nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
> > harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
> > anirudh@xilinx.com; boris.brezillon@free-electrons.com;
> > alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com; Rafal
> > Ozieblo
> > Subject: RE: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in
> Cadence GEM.
> >
> >
> >
> > > -----Original Message-----
> > > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > > Sent: Wednesday, December 07, 2016 11:04 PM
> > > To: Andrei Pistirica - M16132
> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; davem@davemloft.net;
> > > nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
> > > harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
> > > anirudh@xilinx.com; boris.brezillon@free-electrons.com;
> > > alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com;
> > > rafalo@cadence.com
> > > Subject: Re: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in
> > > Cadence GEM.
> > >
> > > On Wed, Dec 07, 2016 at 08:39:09PM +0100, Richard Cochran wrote:
> > > > > +static s32 gem_ptp_max_adj(unsigned int f_nom) {
> > > > > +       u64 adj;
> > > > > +
> > > > > +       /* The 48 bits of seconds for the GEM overflows every:
> > > > > +        * 2^48/(365.25 * 24 * 60 *60) =~ 8 925 512 years (~= 9 mil years),
> > > > > +        * thus the maximum adjust frequency must not overflow
> > > > > + CNS
> > > register:
> > > > > +        *
> > > > > +        * addend  = 10^9/nominal_freq
> > > > > +        * adj_max = +/- addend*ppb_max/10^9
> > > > > +        * max_ppb = (2^8-1)*nominal_freq-10^9
> > > > > +        */
> > > > > +       adj = f_nom;
> > > > > +       adj *= 0xffff;
> > > > > +       adj -= 1000000000ULL;
> > > >
> > > > What is this computation, and how does it relate to the comment?
> >
> > I considered the following simple equation: increment value at nominal
> frequency (which is 10^9/nominal frequency nsecs) + the maximum drift
> value (nsecs) <= maximum increment value at nominal frequency (which is
> 8bit:0xffff).
> > If maximum drift is written as function of nominal frequency and
> maximum ppb, then the equation above yields that the maximum ppb is:
> (2^8 - 1) *nominal_frequency - 10^9. The equation is also simplified by the
> fact that the drift is written as ppm + 16bit_fractions and the increment
> value is written as nsec + 16bit_fractions.
> >
> > Rafal said that this value is hardcoded: 0x64E6, while Harini said:
> 250000000.
> 
> To clarify a little bit. In my reference code this value (0x64E6) was taken
> from our legacy code. It was used for testing only. I know it should be
> change to something more accurate. This is the reason why I asked how did
> you count it (250000000). According to our calculations this value depends
> on actual set period (incr_ns and incr_sub_ns) and min and max value we
> can set. The calculation were a little bit intricate, so we decided to leave it
> as it was.
> 
> >
> > I need to dig into this...
> >
> > >
> > > I am not sure what you meant, but it sounds like you are on the wrong
> track.
> > > Let me explain...
> >
> > Thanks.
> >
> > >
> > > The max_adj has nothing at all to do with the width of the time register.
> > > Rather, it should reflect the maximum possible change in the tuning
> word.
> > >
> > > For example, with a nominal 8 ns period, the tuning word is 0x80000.
> > > Looking at running the clock more slowly, the slowest possible word
> > > is 0x00001, meaning a difference of 0x7FFFF.  This implies an
> > > adjustment of
> > > 0x7FFFF/0x80000 or 999998092 ppb.  Running more quickly, we can
> > > already have 0x100000, twice as fast, or just under 2 billion ppb.
> > >
> > > You should consider the extreme cases to determine the most limited
> > > (smallest) max_adj value:
> > >
> > > Case 1 - high frequency
> > > ~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > With a nominal 1 ns period, we have the nominal tuning word 0x10000.
> > > The smallest is 0x1 for a difference of 0xFFFF.  This corresponds to
> > > an adjustment of 0xFFFF/0x10000 = .9999847412109375 or 999984741 ppb.
> > >
> > > Case 2 - low frequency
> > > ~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > With a nominal 255 ns period, the nominal word is 0xFF0000, the
> > > largest 0xFFFFFF, and the difference is 0xFFFF.  This corresponds to
> > > and adjustment of 0xFFFF/0xFF0000 = .0039215087890625 or 3921508 ppb.
> > >
> > > Since 3921508 ppb is a huge adjustment, you can simply use that as a
> > > safe maximum, ignoring the actual input clock.
> > >
> > > Thanks,
> > > Richard
> > >
> > >
> >
> > Regards,
> > Andrei
> >
> 
> Best regards,
> Rafal Ozieblo   |   Firmware System Engineer,
> phone nbr.: +48 32 5085469
> www.cadence.com

Hi Guys,

Based on Richard's input, this is what I want to do for our platforms:

struct macb_ptp_info {
        void (*ptp_init)(struct net_device *ndev);
        void (*ptp_remove)(struct net_device *ndev);
+       s32 (*get_ptp_max_adj)(void);
        unsigned int (*get_tsu_rate)(struct macb *bp);
        int (*get_ts_info)(struct net_device *dev,
                           struct ethtool_ts_info *info);
       int (*get_hwtst)(struct net_device *netdev,
                         struct ifreq *ifr);
       int (*set_hwtst)(struct net_device *netdev,
                         struct ifreq *ifr, int cmd); 
};

+static s32 gem_get_ptp_max_adj(void)
+{
+       return 3921508;
+}

 static struct macb_ptp_info gem_ptp_info = {
       .ptp_init        = gem_ptp_init,
       .ptp_remove      = gem_ptp_remove,
+       .get_ptp_max_adj = gem_get_ptp_max_adj,
       .get_tsu_rate    = gem_get_tsu_rate,
       .get_ts_info     = gem_get_ts_info,
       .get_hwtst       = gem_get_hwtst,
       .set_hwtst       = gem_set_hwtst,
 };

[...]
void gem_ptp_init(struct net_device *ndev)
 {
[...]
        /* nominal frequency and maximum adjustment in ppb */
        bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
+       bp->ptp_caps.max_adj = bp->ptp_info->get_ptp_max_adj();
[...]
}

Richard, are you agree with this?

Harini, you can fill the callback with the value for your platform. Tell me if you are ok with function's signature.

Regards,
Andrei

Hi Andrei,

On Mon, Dec 12, 2016 at 3:52 PM,  <Andrei.Pistirica@microchip.com> wrote:
>
>
>> -----Original Message-----
>> From: Rafal Ozieblo [mailto:rafalo@cadence.com]
>> Sent: Friday, December 09, 2016 11:20 AM
>> To: Andrei Pistirica - M16132; richardcochran@gmail.com
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; davem@davemloft.net;
>> nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
>> harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
>> anirudh@xilinx.com; boris.brezillon@free-electrons.com;
>> alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com
>> Subject: RE: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in Cadence
>> GEM.
>>
>> -----Original Message-----
>> > From: Andrei.Pistirica@microchip.com
>> > [mailto:Andrei.Pistirica@microchip.com]
>> > Sent: 8 grudnia 2016 15:42
>> > To: richardcochran@gmail.com
>> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> > linux-arm-kernel@lists.infradead.org; davem@davemloft.net;
>> > nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
>> > harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
>> > anirudh@xilinx.com; boris.brezillon@free-electrons.com;
>> > alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com; Rafal
>> > Ozieblo
>> > Subject: RE: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in
>> Cadence GEM.
>> >
>> >
>> >
>> > > -----Original Message-----
>> > > From: Richard Cochran [mailto:richardcochran@gmail.com]
>> > > Sent: Wednesday, December 07, 2016 11:04 PM
>> > > To: Andrei Pistirica - M16132
>> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> > > kernel@lists.infradead.org; davem@davemloft.net;
>> > > nicolas.ferre@atmel.com; harinikatakamlinux@gmail.com;
>> > > harini.katakam@xilinx.com; punnaia@xilinx.com; michals@xilinx.com;
>> > > anirudh@xilinx.com; boris.brezillon@free-electrons.com;
>> > > alexandre.belloni@free-electrons.com; tbultel@pixelsurmer.com;
>> > > rafalo@cadence.com
>> > > Subject: Re: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in
>> > > Cadence GEM.
>> > >
>> > > On Wed, Dec 07, 2016 at 08:39:09PM +0100, Richard Cochran wrote:
>> > > > > +static s32 gem_ptp_max_adj(unsigned int f_nom) {
>> > > > > +       u64 adj;
>> > > > > +
>> > > > > +       /* The 48 bits of seconds for the GEM overflows every:
>> > > > > +        * 2^48/(365.25 * 24 * 60 *60) =~ 8 925 512 years (~= 9 mil years),
>> > > > > +        * thus the maximum adjust frequency must not overflow
>> > > > > + CNS
>> > > register:
>> > > > > +        *
>> > > > > +        * addend  = 10^9/nominal_freq
>> > > > > +        * adj_max = +/- addend*ppb_max/10^9
>> > > > > +        * max_ppb = (2^8-1)*nominal_freq-10^9
>> > > > > +        */
>> > > > > +       adj = f_nom;
>> > > > > +       adj *= 0xffff;
>> > > > > +       adj -= 1000000000ULL;
>> > > >
>> > > > What is this computation, and how does it relate to the comment?
>> >
>> > I considered the following simple equation: increment value at nominal
>> frequency (which is 10^9/nominal frequency nsecs) + the maximum drift
>> value (nsecs) <= maximum increment value at nominal frequency (which is
>> 8bit:0xffff).
>> > If maximum drift is written as function of nominal frequency and
>> maximum ppb, then the equation above yields that the maximum ppb is:
>> (2^8 - 1) *nominal_frequency - 10^9. The equation is also simplified by the
>> fact that the drift is written as ppm + 16bit_fractions and the increment
>> value is written as nsec + 16bit_fractions.
>> >
>> > Rafal said that this value is hardcoded: 0x64E6, while Harini said:
>> 250000000.
>>
>> To clarify a little bit. In my reference code this value (0x64E6) was taken
>> from our legacy code. It was used for testing only. I know it should be
>> change to something more accurate. This is the reason why I asked how did
>> you count it (250000000). According to our calculations this value depends
>> on actual set period (incr_ns and incr_sub_ns) and min and max value we
>> can set. The calculation were a little bit intricate, so we decided to leave it
>> as it was.
>>
>> >
>> > I need to dig into this...
>> >
>> > >
>> > > I am not sure what you meant, but it sounds like you are on the wrong
>> track.
>> > > Let me explain...
>> >
>> > Thanks.
>> >
>> > >
>> > > The max_adj has nothing at all to do with the width of the time register.
>> > > Rather, it should reflect the maximum possible change in the tuning
>> word.
>> > >
>> > > For example, with a nominal 8 ns period, the tuning word is 0x80000.
>> > > Looking at running the clock more slowly, the slowest possible word
>> > > is 0x00001, meaning a difference of 0x7FFFF.  This implies an
>> > > adjustment of
>> > > 0x7FFFF/0x80000 or 999998092 ppb.  Running more quickly, we can
>> > > already have 0x100000, twice as fast, or just under 2 billion ppb.
>> > >
>> > > You should consider the extreme cases to determine the most limited
>> > > (smallest) max_adj value:
>> > >
>> > > Case 1 - high frequency
>> > > ~~~~~~~~~~~~~~~~~~~~~~~
>> > >
>> > > With a nominal 1 ns period, we have the nominal tuning word 0x10000.
>> > > The smallest is 0x1 for a difference of 0xFFFF.  This corresponds to
>> > > an adjustment of 0xFFFF/0x10000 = .9999847412109375 or 999984741 ppb.
>> > >
>> > > Case 2 - low frequency
>> > > ~~~~~~~~~~~~~~~~~~~~~~
>> > >
>> > > With a nominal 255 ns period, the nominal word is 0xFF0000, the
>> > > largest 0xFFFFFF, and the difference is 0xFFFF.  This corresponds to
>> > > and adjustment of 0xFFFF/0xFF0000 = .0039215087890625 or 3921508 ppb.
>> > >
>> > > Since 3921508 ppb is a huge adjustment, you can simply use that as a
>> > > safe maximum, ignoring the actual input clock.
>> > >
>> > > Thanks,
>> > > Richard
>> > >
>> > >
>> >
>> > Regards,
>> > Andrei
>> >
>>
>> Best regards,
>> Rafal Ozieblo   |   Firmware System Engineer,
>> phone nbr.: +48 32 5085469
>> www.cadence.com
>
> Hi Guys,
>
> Based on Richard's input, this is what I want to do for our platforms:
>
> struct macb_ptp_info {
>         void (*ptp_init)(struct net_device *ndev);
>         void (*ptp_remove)(struct net_device *ndev);
> +       s32 (*get_ptp_max_adj)(void);
>         unsigned int (*get_tsu_rate)(struct macb *bp);
>         int (*get_ts_info)(struct net_device *dev,
>                            struct ethtool_ts_info *info);
>        int (*get_hwtst)(struct net_device *netdev,
>                          struct ifreq *ifr);
>        int (*set_hwtst)(struct net_device *netdev,
>                          struct ifreq *ifr, int cmd);
> };
>
> +static s32 gem_get_ptp_max_adj(void)
> +{
> +       return 3921508;
> +}
>
>  static struct macb_ptp_info gem_ptp_info = {
>        .ptp_init        = gem_ptp_init,
>        .ptp_remove      = gem_ptp_remove,
> +       .get_ptp_max_adj = gem_get_ptp_max_adj,
>        .get_tsu_rate    = gem_get_tsu_rate,
>        .get_ts_info     = gem_get_ts_info,
>        .get_hwtst       = gem_get_hwtst,
>        .set_hwtst       = gem_set_hwtst,
>  };
>
> [...]
> void gem_ptp_init(struct net_device *ndev)
>  {
> [...]
>         /* nominal frequency and maximum adjustment in ppb */
>         bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
> +       bp->ptp_caps.max_adj = bp->ptp_info->get_ptp_max_adj();
> [...]
> }
>
> Richard, are you agree with this?
>
> Harini, you can fill the callback with the value for your platform. Tell me if you are ok with function's signature.
>

Thanks, this works for me.

Regards,
Harini

On Mon, Dec 12, 2016 at 10:22:43AM +0000, Andrei.Pistirica@microchip.com wrote:
> Richard, are you agree with this?

Yes, but please trim your replies next time.  Scrolling through pages
of quoted headers and stale content in order to read one line is very
annoying.

Thanks,
Richard