Re: [PATCH V8 12/13] ptp: Added a clock driver for the IXP46x.

[Krzysztof Halasa <khc@pm.waw.pl>]

Richard Cochran <richardcochran@gmail.com> writes:

> +struct ixp46x_channel_ctl {
> +	u32 Ch_Control; /* 0x40 Time Synchronization Channel Control */
> +	u32 Ch_Event;   /* 0x44 Time Synchronization Channel Event */
> +	u32 TxSnapLo;   /* 0x48 Transmit Snapshot Low Register */
> +	u32 TxSnapHi;   /* 0x4C Transmit Snapshot High Register */
> +	u32 RxSnapLo;   /* 0x50 Receive Snapshot Low Register */
> +	u32 RxSnapHi;   /* 0x54 Receive Snapshot High Register */
> +	u32 SrcUUIDLo;  /* 0x58 Source UUID0 Low Register */
> +	u32 SrcUUIDHi;  /* 0x5C Sequence Identifier/Source UUID0 High */

I don't like these XxxYyyZzz either :-(

> +static void do_tx_timestamp(struct port *port, struct sk_buff *skb)
> +{
> +#ifdef __ARMEB__
> +	struct skb_shared_hwtstamps shhwtstamps;
> +	struct ixp46x_ts_regs *regs;
> +	struct skb_shared_info *shtx;
> +	u64 ns;
> +	u32 ch, cnt, hi, lo, val;
> +
> +	shtx = skb_shinfo(skb);
> +	if (unlikely(shtx->tx_flags & SKBTX_HW_TSTAMP && port->hwts_tx_en))
> +		shtx->tx_flags |= SKBTX_IN_PROGRESS;
> +	else
> +		return;
> +
> +	ch = PORT2CHANNEL(port);
> +
> +	regs = (struct ixp46x_ts_regs __iomem *) IXP4XX_TIMESYNC_BASE_VIRT;
> +
> +	/*
> +	 * This really stinks, but we have to poll for the Tx time stamp.
> +	 * Usually, the time stamp is ready after 4 to 6 microseconds.
> +	 */
> +	for (cnt = 0; cnt < 100; cnt++) {
> +		val = __raw_readl(&regs->channel[ch].Ch_Event);
> +		if (val & TX_SNAPSHOT_LOCKED)
> +			break;
> +		udelay(1);
> +	}
> +	if (!(val & TX_SNAPSHOT_LOCKED)) {
> +		shtx->tx_flags &= ~SKBTX_IN_PROGRESS;
> +		return;
> +	}
> +
> +	lo = __raw_readl(&regs->channel[ch].TxSnapLo);
> +	hi = __raw_readl(&regs->channel[ch].TxSnapHi);
> +	ns = ((u64) hi) << 32;
> +	ns |= lo;
> +	ns <<= TICKS_NS_SHIFT;
> +
> +	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +	shhwtstamps.hwtstamp = ns_to_ktime(ns);
> +	skb_tstamp_tx(skb, &shhwtstamps);
> +
> +	__raw_writel(TX_SNAPSHOT_LOCKED, &regs->channel[ch].Ch_Event);
> +#endif
> +}

And what if we're little-endian? Why does it depend on BE?

> @@ -1171,6 +1357,11 @@ static int __devinit eth_init_one(struct platform_device *pdev)
>  	char phy_id[MII_BUS_ID_SIZE + 3];
>  	int err;
>  
> +	if (ptp_filter_init(ptp_filter, ARRAY_SIZE(ptp_filter))) {
> +		pr_err("ixp4xx_eth: bad ptp filter\n");
> +		return -EINVAL;
> +	}
> +
>  	if (!(dev = alloc_etherdev(sizeof(struct port))))
>  		return -ENOMEM;

Shouldn't it depend on CPU type?
BTW which CPU is required? IXP46x (455/460/465)? Does it work on 43x?

> +	if (NO_IRQ == irq)
> +		return NO_IRQ;

Don't like these either :-(
Not showstoppers but...

Also I don't like the ixp_read/ixp_write() trivial macros. Why not
simply call __raw_readl() and __raw_writel()?
-- 
Krzysztof Halasa