Re: [PATCH V14 3/4] ptp: Added a clock driver for the IXP46x.
On Monday 18 April 2011, Richard Cochran wrote:
+static void ixp_rx_timestamp(struct port *port, struct sk_buff *skb)
+{
+ struct skb_shared_hwtstamps *shhwtstamps;
+ struct ixp46x_ts_regs *regs;
+ u64 ns;
+ u32 ch, hi, lo, val;
+ u16 uid, seq;
+
+ if (!port-hwts_rx_en)
+ return;
+
+ ch = PORT2CHANNEL(port);
+
+ regs = (struct ixp46x_ts_regs __iomem *) IXP4XX_TIMESYNC_BASE_VIRT;
+
+ val = __raw_readl(regs-channel[ch].ch_event);
+
+ if (!(val RX_SNAPSHOT_LOCKED))
+ return;
+
+ lo = __raw_readl(regs-channel[ch].src_uuid_lo);
+ hi = __raw_readl(regs-channel[ch].src_uuid_hi);
+
I guess you should use readl(), not __raw_readl() here. The __raw_* functions
are not meant for device drivers.
Arnd
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V14 3/4] ptp: Added a clock driver for the IXP46x.
On Mon, Apr 18, 2011 at 08:56:03AM +0200, Arnd Bergmann wrote: On Monday 18 April 2011, Richard Cochran wrote: + + lo = __raw_readl(regs-channel[ch].src_uuid_lo); + hi = __raw_readl(regs-channel[ch].src_uuid_hi); + I guess you should use readl(), not __raw_readl() here. The __raw_* functions are not meant for device drivers. Krzysztof had a different opinion about this. https://lkml.org/lkml/2011/1/8/67 Anyway, it is his driver, and I just followed what he does elsewhere in the driver. It make sense to me to keep the driver consistent. Maybe we should make the change throughout? Thanks, Richard ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V14 3/4] ptp: Added a clock driver for the IXP46x.
Also, I forgot to add Krzysztof's ack to the commit message, but he did ack V12. Thanks, Richard ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V14 3/4] ptp: Added a clock driver for the IXP46x.
On Monday 18 April 2011, Richard Cochran wrote: On Mon, Apr 18, 2011 at 08:56:03AM +0200, Arnd Bergmann wrote: On Monday 18 April 2011, Richard Cochran wrote: + + lo = __raw_readl(regs-channel[ch].src_uuid_lo); + hi = __raw_readl(regs-channel[ch].src_uuid_hi); + I guess you should use readl(), not __raw_readl() here. The __raw_* functions are not meant for device drivers. Krzysztof had a different opinion about this. https://lkml.org/lkml/2011/1/8/67 Anyway, it is his driver, and I just followed what he does elsewhere in the driver. It make sense to me to keep the driver consistent. Maybe we should make the change throughout? It would certainly be useful to fix it up. I now realized that this driver supports both big-endian and little-endian configurations, so just using readl() is certainly broken. There should probably be an ixp specific version of the safe I/O accessors to deal with it on all on-chip peripherals in a safe way. Arnd ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V14 3/4] ptp: Added a clock driver for the IXP46x.
On Monday 18 April 2011, Arnd Bergmann wrote: On Monday 18 April 2011, Richard Cochran wrote: On Mon, Apr 18, 2011 at 08:56:03AM +0200, Arnd Bergmann wrote: On Monday 18 April 2011, Richard Cochran wrote: + + lo = __raw_readl(regs-channel[ch].src_uuid_lo); + hi = __raw_readl(regs-channel[ch].src_uuid_hi); + I guess you should use readl(), not __raw_readl() here. The __raw_* functions are not meant for device drivers. Krzysztof had a different opinion about this. https://lkml.org/lkml/2011/1/8/67 Anyway, it is his driver, and I just followed what he does elsewhere in the driver. It make sense to me to keep the driver consistent. Maybe we should make the change throughout? It would certainly be useful to fix it up. I now realized that this driver supports both big-endian and little-endian configurations, so just using readl() is certainly broken. There should probably be an ixp specific version of the safe I/O accessors to deal with it on all on-chip peripherals in a safe way. Anyway, not your problem then, just leave it like it is. Acked-by: Arnd Bergmann a...@arndb.de ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V14 3/4] ptp: Added a clock driver for the IXP46x.
On Mon, 2011-04-18 at 08:29 +0200, Richard Cochran wrote:
This patch adds a driver for the hardware time stamping unit found on the
IXP465. The basic clock operations and an external trigger are implemented.
[...]
--- a/drivers/net/arm/ixp4xx_eth.c
+++ b/drivers/net/arm/ixp4xx_eth.c
[...]
@@ -246,6 +255,169 @@ static int ports_open;
static struct port *npe_port_tab[MAX_NPES];
static struct dma_pool *dma_pool;
+static struct sock_filter ptp_filter[] = {
+ PTP_FILTER
+};
+
+static int ixp_ptp_match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16
seq)
+{
+ unsigned int type;
+ u16 *hi, *id;
+ u8 *lo, *data = skb-data;
+
+ type = sk_run_filter(skb, ptp_filter);
+
+ if (PTP_CLASS_V1_IPV4 == type) {
+
+ id = (u16 *)(data + 42 + 30);
+ hi = (u16 *)(data + 42 + 22);
+ lo = data + 42 + 24;
[...]
PTP_FILTER does not verify that the packet length is sufficient to hold
a complete PTP header, nor does it require that the IPv4 header length
is 5 (i.e. 20 bytes). So you have to check those here rather than using
magic numbers.
I think you also need to use be16_to_cpup() to read 'id' and 'hi', since
the host byte order may vary.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
