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

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

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: 2011-01-06 21:02:30

Richard Cochran [off-list ref] writes:

quoted hunk

+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 :-(

quoted hunk

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

quoted hunk

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

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

From: Richard Cochran <richardcochran@gmail.com>
Date: 2011-01-07 17:08:05

Krzysztof,

Thanks for your review. I have some responses, below.

But before that, I have a big favor to ask you. Can you please look at
the TODO in my patch and give me a hint? I don't know how to relate
the port instance (NPE A,B,C) to the two time stamping channels.

On Thu, Jan 06, 2011 at 10:01:59PM +0100, Krzysztof Halasa wrote:

Richard Cochran [off-list ref] writes:

quoted

+	u32 SrcUUIDHi;  /* 0x5C Sequence Identifier/Source UUID0 High */

I don't like these XxxYyyZzz either :-(

Okay, I'll change that.

quoted

+static void do_tx_timestamp(struct port *port, struct sk_buff *skb)
+{
+#ifdef __ARMEB__

...

quoted

+#endif
+}

And what if we're little-endian?

It is a NOOP in that case.

Why does it depend on BE?

The time stamp code clones the skb, but the LE version frees the skb
too early. Perhaps we can move that dev_kfree_skb(skb) in the LE case
to be the last statement in eth_xmit(). What do you think?

quoted

@@ -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?

It won't hurt to init the BPF unconditionally. The important bits are
checked with cpu_is_ixp46x().

BTW which CPU is required? IXP46x (455/460/465)? Does it work on 43x?

IIRC, it does not work on 43x.

I don't know about 455 and 460, but I can find out...

quoted

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

Don't like these either :-(

Do you mean, you don't like the constant on the left hand side?

Is that prohibited by CodingStyle or similar?

I got into the habit of writing it that way to prevent a typo like:

	if (irq = NO_IRQ)

Not showstoppers but...

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

Well, I have had the experience back in 2.4 days of having my drivers
ruined by the changing IO macros in the kernel. The wrappers are
supposed to help if that ever happens again. Seeing *two* leading
underscores in the macro names certainly makes me nervous.

Thanks again,

Richard

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help