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

[Harini Katakam]

Hi Andrei,

On Mon, Dec 12, 2016 at 3:52 PM,  <Andrei.Pistirica@xxxxxxxxxxxxx> wrote:
>
>
>> -----Original Message-----
>> From: Rafal Ozieblo [mailto:rafalo@xxxxxxxxxxx]
>> Sent: Friday, December 09, 2016 11:20 AM
>> To: Andrei Pistirica - M16132; richardcochran@xxxxxxxxx
>> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
>> kernel@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
>> nicolas.ferre@xxxxxxxxx; harinikatakamlinux@xxxxxxxxx;
>> harini.katakam@xxxxxxxxxx; punnaia@xxxxxxxxxx; michals@xxxxxxxxxx;
>> anirudh@xxxxxxxxxx; boris.brezillon@xxxxxxxxxxxxxxxxxx;
>> alexandre.belloni@xxxxxxxxxxxxxxxxxx; tbultel@xxxxxxxxxxxxxxx
>> Subject: RE: [RFC PATCH net-next v3 1/2] macb: Add 1588 support in Cadence
>> GEM.
>>
>> -----Original Message-----
>> > From: Andrei.Pistirica@xxxxxxxxxxxxx
>> > [mailto:Andrei.Pistirica@xxxxxxxxxxxxx]
>> > Sent: 8 grudnia 2016 15:42
>> > To: richardcochran@xxxxxxxxx
>> > Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
>> > nicolas.ferre@xxxxxxxxx; harinikatakamlinux@xxxxxxxxx;
>> > harini.katakam@xxxxxxxxxx; punnaia@xxxxxxxxxx; michals@xxxxxxxxxx;
>> > anirudh@xxxxxxxxxx; boris.brezillon@xxxxxxxxxxxxxxxxxx;
>> > alexandre.belloni@xxxxxxxxxxxxxxxxxx; tbultel@xxxxxxxxxxxxxxx; 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@xxxxxxxxx]
>> > > Sent: Wednesday, December 07, 2016 11:04 PM
>> > > To: Andrei Pistirica - M16132
>> > > Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
>> > > kernel@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
>> > > nicolas.ferre@xxxxxxxxx; harinikatakamlinux@xxxxxxxxx;
>> > > harini.katakam@xxxxxxxxxx; punnaia@xxxxxxxxxx; michals@xxxxxxxxxx;
>> > > anirudh@xxxxxxxxxx; boris.brezillon@xxxxxxxxxxxxxxxxxx;
>> > > alexandre.belloni@xxxxxxxxxxxxxxxxxx; tbultel@xxxxxxxxxxxxxxx;
>> > > rafalo@xxxxxxxxxxx
>> > > 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