Message-ID: <CAJ3bTp5vKmEgjWsJ7YhxKZ+T8nK-VwJN=BiO5T1NaEN4g-kUpg@mail.gmail.com>
Date:	Fri, 8 Mar 2013 13:21:20 +0530
From:	Rayagond K <rayagond@...avyalabs.com>
To:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
Cc:	Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
	bh74.an@...sung.com
Subject: Re: [net-next.git 2/9] stmmac: add IEEE 1588-2002 PTP support

On Fri, Mar 8, 2013 at 12:32 PM, Giuseppe CAVALLARO
<peppe.cavallaro@...com> wrote:
>
> On 3/8/2013 7:34 AM, Richard Cochran wrote:
>>
>> I have a few comments, below.
>
>
> thanks for them.
>
>
>>> HW Timestamp support can be enabled while configuring the Kernel and
>>> the Koption is: STMMAC_USE_HWSTAMP
>>> At runtime, the support is verified by looking at the HW capability
>>> register.
>>
>>
>> As davem said, since you have the ability to detect this at run time,
>> then there is no need for a kconfig option.
>
>
> I'll try to give you more details about this replying to D. Miller
>
> [snip]
>
>
>>> +       /* get tx/rx timestamp low value */
>>> +       u32 (*get_timestamp_low) (struct dma_desc *p);
>>> +       /* get tx/rx timestamp high value */
>>> +       u32 (*get_timestamp_high) (struct dma_desc *p);
>>
>>
>> These two separate functions should be combined into one.
>
>
> ok
>
>
>>> +       /* get rx timestamp status */
>>> +       int (*get_rx_timestamp_status) (struct dma_desc *p);
>
>
>>> +
>>> +static int enh_desc_get_rx_timestamp_status(struct dma_desc *p)
>>> +{
>>> +       /* FIXME if Enhance descriptor with 8 DWORDS is enabled */
>>
>>
>> Why not fix these FIXMEs for the next respin?
>
>
> This is fixed in the patch #5 where we use the extended descriptors
> for PTP2.
>
>
>>> +       if ((p->des2 == 0xffffffff) && (p->des3 == 0xffffffff))
>>> +               /* timestamp is currupted, hence don't store it */
>>
>>
>> corrupted
>
>
> thanks
>
>
>
>>> +       /* Convert the ptp_clock to nano second
>>> +        * formula = (1/ptp_clock) * 1000000000
>>> +        * where, ptp_clock = 50MHz for FINE correction method &
>>> +        * ptp_clock = STMMAC_SYSCLOCK for COARSE correction method
>>> +        */
>>
>>
>> What are these coarse/fine method? Can't we always use the fine one?
>
>
> This is explained in the "4.1.2 System Time Register Module" of Synopsys
> Databook. Summarizing, the MAC can have an optional module and use this
> coarse correction method. In the fine correction method, a slave clock’s
> frequency drift with respect to the master clock is corrected over a period
> of time instead of in one clock, as in coarse correction.
>
> Pls, Rayagond feels free to provide more details...

Yes Synopsys core support two method for correcting the system time ie
COARSE method  and FINE method.

In the fine correction method, a slave clock’s frequency drift with
respect to the master clock (as defined in IEEE 1588) is corrected
over a period of time instead of in one clock, as in coarse
correction. This helps maintain linear time and does not introduce
drastic changes (or a large jitter) in the reference time between PTP
Sync message intervals.

And updating the SSNR (Sub Second Increment Register) depends on which
method is selected during the initialization.

>
>>> +       if (value & PTP_TCR_TSCFUPDT)
>>> +               data = (1000000000ULL / 50000000);
>>> +       else
>>> +               data = (1000000000ULL / STMMAC_SYSCLOCK);
>>> +
>>> +       writel(data, ioaddr + PTP_SSIR);
>>> +}
>>> +
>>> +static int stmmac_init_systime(void __iomem *ioaddr, u32 sec, u32 nsec)
>>> +{
>>> +       int limit;
>>> +       u32 value;
>>> +
>>> +       /* wait for previous(if any) system time initialize to complete
>>> */
>>> +       limit = 100;
>>> +       while (limit--) {
>>> +               if (!(readl(ioaddr + PTP_TCR) & PTP_TCR_TSINIT))
>>> +                       break;
>>> +               mdelay(10);
>>> +       }
>>> +
>>> +       if (limit < 0)
>>> +               return -EBUSY;
>>> +
>>
>>
>> Ugh, this is terrible...
>>
>>> +       writel(sec, ioaddr + PTP_STSUR);
>>> +       writel(nsec, ioaddr + PTP_STNSUR);
>>> +       /* issue command to initialize the system time value */
>>> +       value = readl(ioaddr + PTP_TCR);
>>> +       value |= PTP_TCR_TSINIT;
>>> +       writel(value, ioaddr + PTP_TCR);
>>> +
>>> +       /* wait for present system time initialize to complete */
>>> +       limit = 100;
>>> +       while (limit--) {
>>> +               if (!(readl(ioaddr + PTP_TCR) & PTP_TCR_TSINIT))
>>> +                       break;
>>> +               mdelay(10);
>>> +       }
>>> +       if (limit < 0)
>>> +               return -EBUSY;
>>
>>
>> ... for a number of reasons.
>>
>> 1. You are polling for 100*10ms = one second, and the caller has
>>     disabled interrupts! This is way too long. Is the hardware really
>>     that slow? If so, then you need to use delayed work or find some
>>     other way not to block.

Sorry for this, during testing I just used maximum delay as there was
bug in the HW initially. But after fixing the HW issue I noticed that
HW is not that much slow. We will reduce the delay in all functions.

>>
>> 2. You are waiting both before and after for the status bit. Pick one
>>     or the other. You don't need both.
>>
>> This same pattern is repeated a few times here, and it needs fixing in
>> each case.

Sure will change it. I added just to make sure double check here.
Practically first wait is not required, will remove this.

>
>
> This is for the Author. I've no HW where test. Rayagond tested all on his
> side. Pls Rayagond fixes it and gives me the new code.
>
>
>>> +       struct stmmac_priv *priv = netdev_priv(dev);
>>> +       struct hwtstamp_config config;
>>> +       struct timespec now;
>>> +       u64 temp = 0;
>>
>>
>> You add this new code here, but you change it all around again a few
>> patches later. Please just submit the final, combined version.
>
>
> we kept these separately because the patch #5 (for example) depends on
> another one that adds the extended descriptor support. Also If I add
> all the code in a single patch this will be very big. I had some
> problems to review all separately. So I suspect that if we merge all
> in a single patch this will not help (especially myself). At any rate,
> tell me if you prefer to have a single patch. I can do that.
>
> peppe
>>
>>
>> Thanks,
>> Richard
>>
>

Thanks
Rayagond.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists