Re: [net-next.git 2/9] stmmac: add IEEE 1588-2002 PTP support

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

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...

>> +	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.
>
> 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.

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
>