Message ID | 20090728120624.GS2714@pengutronix.de |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Sascha Hauer wrote: > Hi, > > Here is the second version of the flexcan driver. Hi Sascha, some more points i forgot to mention, sorry ... > +/* Structure of the message buffer */ > +struct flexcan_mb { > + u32 can_dlc; > + u32 can_id; > + u32 data[2]; > +}; This looks really hackish and does not reflect the structure of a flexcan message buffer! The data is 'u8' and the name of 'dlc' for the description/flag register is bad. > + > +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct can_frame *frame = (struct can_frame *)skb->data; > + struct flexcan_priv *priv = netdev_priv(dev); > + struct flexcan_regs __iomem *regs = priv->base; > + u32 can_id; > + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16); Naming this variable 'dlc' does not hit the point. See below. > + u32 *can_data; Really this needs to be fixed up by defining a proper mailbox struct. > + > + netif_stop_queue(dev); > + > + if (frame->can_id & CAN_EFF_FLAG) { > + can_id = frame->can_id & MB_ID_EXT; Please use CAN_EFF_MASK here. > + dlc |= MB_CNT_IDE | MB_CNT_SRR; > + } else { > + can_id = (frame->can_id & CAN_SFF_MASK) << 18; > + } Just nitpicking for Kernel coding style: remove the last '{' and '}' pair. > + > + if (frame->can_id & CAN_RTR_FLAG) > + dlc |= MB_CNT_RTR; > + > + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); > + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); > + > + can_data = (u32 *)frame->data; > + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]); > + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]); IMHO it is not really transparent, that this is a correct handling to copy the can_frame.data[] on all architectures. I bet creating a for-statement regarding the dlc is not slower and makes really clear, what's going on here. > + > + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); > + > + kfree_skb(skb); > + > + return NETDEV_TX_OK; > +} > + > +static void flexcan_rx_frame(struct net_device *ndev, > + struct flexcan_mb __iomem *mb) > +{ > + struct net_device_stats *stats = &ndev->stats; > + struct sk_buff *skb; > + struct can_frame *frame; > + int ctrl = readl(&mb->can_dlc); 'ctrl' is much better than 'dlc' naming above :-) > + int length = (ctrl >> 16) & 0x0f; is probably not enough ... use %9 or an additional check. > + u32 id; > + > + skb = dev_alloc_skb(sizeof(struct can_frame)); > + if (!skb) { > + stats->rx_dropped++; > + return; > + } > + > + frame = (struct can_frame *)skb_put(skb, > + sizeof(struct can_frame)); > + > + frame->can_dlc = length; > + id = readl(&mb->can_id) & MB_ID_EXT; like above use CAN_EFF_MASK or at least rename MB_ID_EXT to MB_ID_EXT_MASK > + > + if (ctrl & MB_CNT_IDE) { > + frame->can_id = id & CAN_EFF_MASK; > + frame->can_id |= CAN_EFF_FLAG; > + if (ctrl & MB_CNT_RTR) > + frame->can_id |= CAN_RTR_FLAG; > + } else { > + frame->can_id = id >> 18; > + if (ctrl & MB_CNT_RTR) > + frame->can_id |= CAN_RTR_FLAG; > + } > + > + *(u32 *)frame->data = be32_to_cpu(readl(&mb->data[0])); > + *((u32 *)frame->data + 1) = be32_to_cpu(readl(&mb->data[1])); Same as above. Please write readable an maintainable code and let the compiler do his job. Thanks, Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Oliver, On Tue, Jul 28, 2009 at 03:21:40PM +0200, Oliver Hartkopp wrote: > Sascha Hauer wrote: > > Hi, > > > > Here is the second version of the flexcan driver. > > Hi Sascha, > > some more points i forgot to mention, sorry ... > > > > +/* Structure of the message buffer */ > > +struct flexcan_mb { > > + u32 can_dlc; > > + u32 can_id; > > + u32 data[2]; > > +}; > > This looks really hackish and does not reflect the structure of a flexcan > message buffer! The data is 'u8' and the name of 'dlc' for the > description/flag register is bad. > see below.. > > > + > > +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev) > > +{ > > + struct can_frame *frame = (struct can_frame *)skb->data; > > + struct flexcan_priv *priv = netdev_priv(dev); > > + struct flexcan_regs __iomem *regs = priv->base; > > > + u32 can_id; > > + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16); > > Naming this variable 'dlc' does not hit the point. See below. > > > + u32 *can_data; > > Really this needs to be fixed up by defining a proper mailbox struct. > > > > + > > + netif_stop_queue(dev); > > + > > + if (frame->can_id & CAN_EFF_FLAG) { > > + can_id = frame->can_id & MB_ID_EXT; > > Please use CAN_EFF_MASK here. I used MX_ID_EXT intentionally because it it flexcan specific and just happens to be the same as CAN_EFF_MASK. I can change it if you like. > > > > + dlc |= MB_CNT_IDE | MB_CNT_SRR; > > + } else { > > + can_id = (frame->can_id & CAN_SFF_MASK) << 18; > > + } > > Just nitpicking for Kernel coding style: > remove the last '{' and '}' pair. No, Documentation/CondingStyle suggests that if one branch needs braces the other branch should use them, too. > > > + > > + if (frame->can_id & CAN_RTR_FLAG) > > + dlc |= MB_CNT_RTR; > > + > > + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); > > + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); > > + > > + can_data = (u32 *)frame->data; > > + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]); > > + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]); > > IMHO it is not really transparent, that this is a correct handling to copy the > can_frame.data[] on all architectures. I bet creating a for-statement > regarding the dlc is not slower and makes really clear, what's going on here. This is indeed a problem here. The original Coldfire code I used as a template used a loop around unsigned char * which did the wrong thing for me. So yes, this is not generic here, but I have no idea how the generic code looks like. As Coldfire is big endian this doesn't seem that wrong. > > > + > > + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); > > + > > + kfree_skb(skb); > > + > > + return NETDEV_TX_OK; > > +} > > + > > +static void flexcan_rx_frame(struct net_device *ndev, > > + struct flexcan_mb __iomem *mb) > > +{ > > + struct net_device_stats *stats = &ndev->stats; > > + struct sk_buff *skb; > > + struct can_frame *frame; > > + int ctrl = readl(&mb->can_dlc); > > 'ctrl' is much better than 'dlc' naming above :-) ok, will change it above. > > > + int length = (ctrl >> 16) & 0x0f; > > is probably not enough ... use %9 or an additional check. ok, I'll add a sanity check. > > > + u32 id; > > + > > + skb = dev_alloc_skb(sizeof(struct can_frame)); > > + if (!skb) { > > + stats->rx_dropped++; > > + return; > > + } > > + > > + frame = (struct can_frame *)skb_put(skb, > > + sizeof(struct can_frame)); > > + > > + frame->can_dlc = length; > > + id = readl(&mb->can_id) & MB_ID_EXT; > > like above use CAN_EFF_MASK > > or at least rename MB_ID_EXT to MB_ID_EXT_MASK > > > + > > + if (ctrl & MB_CNT_IDE) { > > + frame->can_id = id & CAN_EFF_MASK; > > + frame->can_id |= CAN_EFF_FLAG; > > + if (ctrl & MB_CNT_RTR) > > + frame->can_id |= CAN_RTR_FLAG; > > + } else { > > + frame->can_id = id >> 18; > > + if (ctrl & MB_CNT_RTR) > > + frame->can_id |= CAN_RTR_FLAG; > > + } > > + > > + *(u32 *)frame->data = be32_to_cpu(readl(&mb->data[0])); > > + *((u32 *)frame->data + 1) = be32_to_cpu(readl(&mb->data[1])); > > Same as above. > > Please write readable an maintainable code and let the compiler do his job. > > Thanks, > Oliver >
Sascha Hauer wrote: > Hi Oliver, > > On Tue, Jul 28, 2009 at 03:21:40PM +0200, Oliver Hartkopp wrote: >> Sascha Hauer wrote: >>> Hi, >>> >>> Here is the second version of the flexcan driver. >> Hi Sascha, >> >> some more points i forgot to mention, sorry ... >> >> >>> +/* Structure of the message buffer */ >>> +struct flexcan_mb { >>> + u32 can_dlc; >>> + u32 can_id; >>> + u32 data[2]; >>> +}; >> This looks really hackish and does not reflect the structure of a flexcan >> message buffer! The data is 'u8' and the name of 'dlc' for the >> description/flag register is bad. >> > > see below.. Especially can_dlc, can_id and data[] are known from struct can_frame which really can confuse here ... > >>> + >>> +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev) >>> +{ >>> + struct can_frame *frame = (struct can_frame *)skb->data; >>> + struct flexcan_priv *priv = netdev_priv(dev); >>> + struct flexcan_regs __iomem *regs = priv->base; >>> + u32 can_id; >>> + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16); >> Naming this variable 'dlc' does not hit the point. See below. >> >>> + u32 *can_data; >> Really this needs to be fixed up by defining a proper mailbox struct. >> >> >>> + >>> + netif_stop_queue(dev); >>> + >>> + if (frame->can_id & CAN_EFF_FLAG) { >>> + can_id = frame->can_id & MB_ID_EXT; >> Please use CAN_EFF_MASK here. > > I used MX_ID_EXT intentionally because it it flexcan specific and just > happens to be the same as CAN_EFF_MASK. I can change it if you like. Yes, i've seen that. I would tend to use CAN_EFF_MASK here as you apply it on frame->can_id. When you get it from the controller MB_ID_EXT_MASK would be the better one. > >> >>> + dlc |= MB_CNT_IDE | MB_CNT_SRR; >>> + } else { >>> + can_id = (frame->can_id & CAN_SFF_MASK) << 18; >>> + } >> Just nitpicking for Kernel coding style: >> remove the last '{' and '}' pair. > > No, Documentation/CondingStyle suggests that if one branch needs braces > the other branch should use them, too. Sorry. Didn't know that. > >>> + >>> + if (frame->can_id & CAN_RTR_FLAG) >>> + dlc |= MB_CNT_RTR; >>> + >>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); >>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); >>> + >>> + can_data = (u32 *)frame->data; >>> + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]); >>> + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]); >> IMHO it is not really transparent, that this is a correct handling to copy the >> can_frame.data[] on all architectures. I bet creating a for-statement >> regarding the dlc is not slower and makes really clear, what's going on here. > > This is indeed a problem here. The original Coldfire code I used as a > template used a loop around unsigned char * which did the wrong thing > for me. This could be a good starting point for an investigation ;-) > So yes, this is not generic here, but I have no idea how the > generic code looks like. As Coldfire is big endian this doesn't seem > that wrong. I would try to define a proper flexcan_mb struct like struct flexcan_mb { u8 code; u8 ctrl; u16 timestamp; u32 id; u8 data[8]; } And then see how it looks like ;-) Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp wrote: > Sascha Hauer wrote: >>>> + >>>> + if (frame->can_id & CAN_RTR_FLAG) >>>> + dlc |= MB_CNT_RTR; >>>> + >>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); >>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); Are you sure, that this is correct? Indeed i wonder, if it would make sense to skip the entire struct flexcan_mb approach and fiddle byte-by-byte inside the registers ... Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 28, 2009 at 03:50:48PM +0200, Oliver Hartkopp wrote: > Sascha Hauer wrote: > > Hi Oliver, > > > > On Tue, Jul 28, 2009 at 03:21:40PM +0200, Oliver Hartkopp wrote: > >> Sascha Hauer wrote: > >>> Hi, > >>> > >>> Here is the second version of the flexcan driver. > >> Hi Sascha, > >> > >> some more points i forgot to mention, sorry ... > >> > >> > >>> +/* Structure of the message buffer */ > >>> +struct flexcan_mb { > >>> + u32 can_dlc; > >>> + u32 can_id; > >>> + u32 data[2]; > >>> +}; > >> This looks really hackish and does not reflect the structure of a flexcan > >> message buffer! The data is 'u8' and the name of 'dlc' for the > >> description/flag register is bad. > >> > > > > see below.. > > Especially can_dlc, can_id and data[] are known from struct can_frame which > really can confuse here ... > > > > >>> + > >>> +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev) > >>> +{ > >>> + struct can_frame *frame = (struct can_frame *)skb->data; > >>> + struct flexcan_priv *priv = netdev_priv(dev); > >>> + struct flexcan_regs __iomem *regs = priv->base; > >>> + u32 can_id; > >>> + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16); > >> Naming this variable 'dlc' does not hit the point. See below. > >> > >>> + u32 *can_data; > >> Really this needs to be fixed up by defining a proper mailbox struct. > >> > >> > >>> + > >>> + netif_stop_queue(dev); > >>> + > >>> + if (frame->can_id & CAN_EFF_FLAG) { > >>> + can_id = frame->can_id & MB_ID_EXT; > >> Please use CAN_EFF_MASK here. > > > > I used MX_ID_EXT intentionally because it it flexcan specific and just > > happens to be the same as CAN_EFF_MASK. I can change it if you like. > > Yes, i've seen that. I would tend to use CAN_EFF_MASK here as you apply it on > frame->can_id. > > When you get it from the controller MB_ID_EXT_MASK would be the better one. > > > > >> > >>> + dlc |= MB_CNT_IDE | MB_CNT_SRR; > >>> + } else { > >>> + can_id = (frame->can_id & CAN_SFF_MASK) << 18; > >>> + } > >> Just nitpicking for Kernel coding style: > >> remove the last '{' and '}' pair. > > > > No, Documentation/CondingStyle suggests that if one branch needs braces > > the other branch should use them, too. > > Sorry. Didn't know that. > > > > >>> + > >>> + if (frame->can_id & CAN_RTR_FLAG) > >>> + dlc |= MB_CNT_RTR; > >>> + > >>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); > >>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); > >>> + > >>> + can_data = (u32 *)frame->data; > >>> + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]); > >>> + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]); > >> IMHO it is not really transparent, that this is a correct handling to copy the > >> can_frame.data[] on all architectures. I bet creating a for-statement > >> regarding the dlc is not slower and makes really clear, what's going on here. > > > > This is indeed a problem here. The original Coldfire code I used as a > > template used a loop around unsigned char * which did the wrong thing > > for me. > > This could be a good starting point for an investigation ;-) > > > So yes, this is not generic here, but I have no idea how the > > generic code looks like. As Coldfire is big endian this doesn't seem > > that wrong. > > I would try to define a proper flexcan_mb struct like > > struct flexcan_mb { > u8 code; > u8 ctrl; > u16 timestamp; > u32 id; > u8 data[8]; > } I can't properly define it like this because I have a little endian system and u8 code is on offset 3 on my hardware. I just checked it because I always get confused with endian problems ;) I hope you're not suggesting me to do something like #ifdef __LITTLE_ENDIAN struct flexcan_mb { u16 timestamp; u8 ctrl; u8 code; u32 id; u8 data[8]; } #else struct flexcan_mb { u8 code; u8 ctrl; u16 timestamp; u32 id; u8 data[8]; } #endif (which would still require endian specific handling for the actual CAN data) > > And then see how it looks like ;-) Well, I would have to do a le/be conversion manually whereas cpu_to_be32 *should* do the right thing. I don't have any ColdFire hardware to test though. The more I think about it the more I think that my original code does the right thing(tm) Sascha
On Tue, Jul 28, 2009 at 04:12:10PM +0200, Oliver Hartkopp wrote: > Oliver Hartkopp wrote: > > Sascha Hauer wrote: > > >>>> + > >>>> + if (frame->can_id & CAN_RTR_FLAG) > >>>> + dlc |= MB_CNT_RTR; > >>>> + > >>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); > >>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); > > Are you sure, that this is correct? Yes, I am sure, at least on my hardware. > > Indeed i wonder, if it would make sense to skip the entire struct flexcan_mb > approach and fiddle byte-by-byte inside the registers ... You'll have to to 32 bit accesses to get it right on little and big endian. what we can do is: writel(can_data[0] << 24 | can_data[1] << 16 | can_data[2] << 8 | can_data[3], msg_buf + 0x8); Sascha
Sascha Hauer wrote: > On Tue, Jul 28, 2009 at 04:12:10PM +0200, Oliver Hartkopp wrote: >> Oliver Hartkopp wrote: >>> Sascha Hauer wrote: >>>>>> + >>>>>> + if (frame->can_id & CAN_RTR_FLAG) >>>>>> + dlc |= MB_CNT_RTR; >>>>>> + >>>>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); >>>>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); >> Are you sure, that this is correct? > > Yes, I am sure, at least on my hardware. I looked into the writel() macro which does a cpu_to_le32() to the value - sorry that i did not check this before ... > >> Indeed i wonder, if it would make sense to skip the entire struct flexcan_mb >> approach and fiddle byte-by-byte inside the registers ... > > You'll have to to 32 bit accesses to get it right on little and big > endian. > > what we can do is: > > writel(can_data[0] << 24 | can_data[1] << 16 | can_data[2] << 8 | can_data[3], > msg_buf + 0x8); This looks easier to understand and makes things clear, when you look into the specification. Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 28, 2009 at 04:48:34PM +0200, Oliver Hartkopp wrote: > Sascha Hauer wrote: > > On Tue, Jul 28, 2009 at 04:12:10PM +0200, Oliver Hartkopp wrote: > >> Oliver Hartkopp wrote: > >>> Sascha Hauer wrote: > >>>>>> + > >>>>>> + if (frame->can_id & CAN_RTR_FLAG) > >>>>>> + dlc |= MB_CNT_RTR; > >>>>>> + > >>>>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); > >>>>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); > >> Are you sure, that this is correct? > > > > Yes, I am sure, at least on my hardware. > > I looked into the writel() macro which does a cpu_to_le32() to the value - > sorry that i did not check this before ... > > > > >> Indeed i wonder, if it would make sense to skip the entire struct flexcan_mb > >> approach and fiddle byte-by-byte inside the registers ... > > > > You'll have to to 32 bit accesses to get it right on little and big > > endian. > > > > what we can do is: > > > > writel(can_data[0] << 24 | can_data[1] << 16 | can_data[2] << 8 | can_data[3], > > msg_buf + 0x8); > > This looks easier to understand and makes things clear, when you look into the > specification. Ok, I'll change it like this. Sascha
Sascha Hauer wrote: > On Tue, Jul 28, 2009 at 04:48:34PM +0200, Oliver Hartkopp wrote: >> Sascha Hauer wrote: >>> On Tue, Jul 28, 2009 at 04:12:10PM +0200, Oliver Hartkopp wrote: >>>> Oliver Hartkopp wrote: >>>>> Sascha Hauer wrote: >>>>>>>> + >>>>>>>> + if (frame->can_id & CAN_RTR_FLAG) >>>>>>>> + dlc |= MB_CNT_RTR; >>>>>>>> + >>>>>>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); >>>>>>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); >>>> Are you sure, that this is correct? >>> Yes, I am sure, at least on my hardware. >> I looked into the writel() macro which does a cpu_to_le32() to the value - >> sorry that i did not check this before ... >> >>>> Indeed i wonder, if it would make sense to skip the entire struct flexcan_mb >>>> approach and fiddle byte-by-byte inside the registers ... >>> You'll have to to 32 bit accesses to get it right on little and big >>> endian. >>> >>> what we can do is: >>> >>> writel(can_data[0] << 24 | can_data[1] << 16 | can_data[2] << 8 | can_data[3], >>> msg_buf + 0x8); >> This looks easier to understand and makes things clear, when you look into the >> specification. > > Ok, I'll change it like this. OK, even if I do not really share Oliver's concerns. The second write could be suppressed if dlc <= 4. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index 33821a8..99c6da4 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -74,6 +74,12 @@ config CAN_KVASER_PCI This driver is for the the PCIcanx and PCIcan cards (1, 2 or 4 channel) from Kvaser (http://www.kvaser.com). +config CAN_FLEXCAN + tristate "Support for Freescale FLEXCAN based chips" + depends on CAN_DEV + ---help--- + Say Y here if you want to support for Freescale FlexCAN. + config CAN_DEBUG_DEVICES bool "CAN devices debugging messages" depends on CAN diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile index 523a941..25f2032 100644 --- a/drivers/net/can/Makefile +++ b/drivers/net/can/Makefile @@ -8,5 +8,6 @@ obj-$(CONFIG_CAN_DEV) += can-dev.o can-dev-y := dev.o obj-$(CONFIG_CAN_SJA1000) += sja1000/ +obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c new file mode 100644 index 0000000..8a2e820 --- /dev/null +++ b/drivers/net/can/flexcan.c @@ -0,0 +1,692 @@ +/* + * flexcan.c - FLEXCAN CAN controller driver + * + * Copyright (c) 2005-2006 Varma Electronics Oy + * Copyright (c) 2009 Sascha Hauer, Pengutronix + * + * Based on code originally by Andrey Volkov <avolkov@varma-el.com> + * + * LICENCE: + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <linux/platform_device.h> +#include <linux/can/netlink.h> +#include <linux/can/error.h> +#include <linux/interrupt.h> +#include <linux/netdevice.h> +#include <linux/if_ether.h> +#include <linux/can/dev.h> +#include <linux/if_arp.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/delay.h> +#include <linux/list.h> +#include <linux/can.h> +#include <linux/clk.h> +#include <linux/io.h> + +#define DRIVER_NAME "flexcan" + +/* FLEXCAN module configuration register (CANMCR) bits */ +#define CANMCR_MDIS (1 << 31) +#define CANMCR_FRZ (1 << 30) +#define CANMCR_FEN (1 << 29) +#define CANMCR_HALT (1 << 28) +#define CANMCR_SOFTRST (1 << 25) +#define CANMCR_FRZACK (1 << 24) +#define CANMCR_SUPV (1 << 23) +#define CANMCR_SRX_DIS (1 << 17) +#define CANMCR_MAXMB(x) ((x) & 0x0f) +#define CANMCR_IDAM_A (0 << 8) +#define CANMCR_IDAM_B (1 << 8) +#define CANMCR_IDAM_C (2 << 8) + +/* FLEXCAN control register (CANCTRL) bits */ +#define CANCTRL_PRESDIV(x) (((x) & 0xff) << 24) +#define CANCTRL_RJW(x) (((x) & 0x03) << 22) +#define CANCTRL_PSEG1(x) (((x) & 0x07) << 19) +#define CANCTRL_PSEG2(x) (((x) & 0x07) << 16) +#define CANCTRL_BOFFMSK (1 << 15) +#define CANCTRL_ERRMSK (1 << 14) +#define CANCTRL_CLKSRC (1 << 13) +#define CANCTRL_LPB (1 << 12) +#define CANCTRL_TWRN_MSK (1 << 11) +#define CANCTRL_RWRN_MSK (1 << 10) +#define CANCTRL_SAMP (1 << 7) +#define CANCTRL_BOFFREC (1 << 6) +#define CANCTRL_TSYNC (1 << 5) +#define CANCTRL_LBUF (1 << 4) +#define CANCTRL_LOM (1 << 3) +#define CANCTRL_PROPSEG(x) ((x) & 0x07) + +/* FLEXCAN error counter register (ERRCNT) bits */ +#define ERRCNT_REXECTR(x) (((x) & 0xff) << 8) +#define ERRCNT_TXECTR(x) ((x) & 0xff) + +/* FLEXCAN error and status register (ERRSTAT) bits */ +#define ERRSTAT_TWRNINT (1 << 17) +#define ERRSTAT_RWRNINT (1 << 16) +#define ERRSTAT_BIT1ERR (1 << 15) +#define ERRSTAT_BIT0ERR (1 << 14) +#define ERRSTAT_ACKERR (1 << 13) +#define ERRSTAT_CRCERR (1 << 12) +#define ERRSTAT_FRMERR (1 << 11) +#define ERRSTAT_STFERR (1 << 10) +#define ERRSTAT_TXWRN (1 << 9) +#define ERRSTAT_RXWRN (1 << 8) +#define ERRSTAT_IDLE (1 << 7) +#define ERRSTAT_TXRX (1 << 6) +#define ERRSTAT_FLTCONF_MASK (3 << 4) +#define ERRSTAT_FLTCONF_ERROR_ACTIVE (0 << 4) +#define ERRSTAT_FLTCONF_ERROR_PASSIVE (1 << 4) +#define ERRSTAT_FLTCONF_ERROR_BUS_OFF (2 << 4) +#define ERRSTAT_BOFFINT (1 << 2) +#define ERRSTAT_ERRINT (1 << 1) +#define ERRSTAT_WAKINT (1 << 0) +#define ERRSTAT_INT (ERRSTAT_BOFFINT | ERRSTAT_ERRINT | ERRSTAT_TWRNINT | \ + ERRSTAT_RWRNINT) + +/* FLEXCAN interrupt flag register (IFLAG) bits */ +#define IFLAG_BUF(x) (1 << (x)) +#define IFLAG_RX_FIFO_OVERFLOW (1 << 7) +#define IFLAG_RX_FIFO_WARN (1 << 6) +#define IFLAG_RX_FIFO_AVAILABLE (1 << 5) + +/* FLEXCAN message buffers */ +#define MB_CNT_CODE(x) (((x) & 0xf) << 24) +#define MB_CNT_SRR (1 << 22) +#define MB_CNT_IDE (1 << 21) +#define MB_CNT_RTR (1 << 20) +#define MB_CNT_LENGTH(x) (((x) & 0xf) << 16) +#define MB_CNT_TIMESTAMP(x) ((x) & 0xffff) + +#define MB_ID_STD (0x7ff << 18) +#define MB_ID_EXT 0x1fffffff +#define MB_CODE_MASK 0xf0ffffff + +/* Structure of the message buffer */ +struct flexcan_mb { + u32 can_dlc; + u32 can_id; + u32 data[2]; +}; + +/* Structure of the hardware registers */ +struct flexcan_regs { + u32 canmcr; /* 0x00 */ + u32 canctrl; /* 0x04 */ + u32 timer; /* 0x08 */ + u32 reserved1; /* 0x0c */ + u32 rxgmask; /* 0x10 */ + u32 rx14mask; /* 0x14 */ + u32 rx15mask; /* 0x18 */ + u32 errcnt; /* 0x1c */ + u32 errstat; /* 0x20 */ + u32 imask2; /* 0x24 */ + u32 imask1; /* 0x28 */ + u32 iflag2; /* 0x2c */ + u32 iflag1; /* 0x30 */ + u32 reserved4[19]; + struct flexcan_mb cantxfg[64]; +}; + +struct flexcan_priv { + struct can_priv can; + void __iomem *base; + + struct net_device *dev; + struct clk *clk; +}; + +static struct can_bittiming_const flexcan_bittiming_const = { + .name = DRIVER_NAME, + .tseg1_min = 1, + .tseg1_max = 16, + .tseg2_min = 2, + .tseg2_max = 8, + .sjw_max = 4, + .brp_min = 1, + .brp_max = 256, + .brp_inc = 1, +}; + +#define TX_BUF_ID 8 + +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev) +{ + struct can_frame *frame = (struct can_frame *)skb->data; + struct flexcan_priv *priv = netdev_priv(dev); + struct flexcan_regs __iomem *regs = priv->base; + u32 can_id; + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16); + u32 *can_data; + + netif_stop_queue(dev); + + if (frame->can_id & CAN_EFF_FLAG) { + can_id = frame->can_id & MB_ID_EXT; + dlc |= MB_CNT_IDE | MB_CNT_SRR; + } else { + can_id = (frame->can_id & CAN_SFF_MASK) << 18; + } + + if (frame->can_id & CAN_RTR_FLAG) + dlc |= MB_CNT_RTR; + + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id); + + can_data = (u32 *)frame->data; + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]); + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]); + + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc); + + kfree_skb(skb); + + return NETDEV_TX_OK; +} + +static void flexcan_rx_frame(struct net_device *ndev, + struct flexcan_mb __iomem *mb) +{ + struct net_device_stats *stats = &ndev->stats; + struct sk_buff *skb; + struct can_frame *frame; + int ctrl = readl(&mb->can_dlc); + int length = (ctrl >> 16) & 0x0f; + u32 id; + + skb = dev_alloc_skb(sizeof(struct can_frame)); + if (!skb) { + stats->rx_dropped++; + return; + } + + frame = (struct can_frame *)skb_put(skb, + sizeof(struct can_frame)); + + frame->can_dlc = length; + id = readl(&mb->can_id) & MB_ID_EXT; + + if (ctrl & MB_CNT_IDE) { + frame->can_id = id & CAN_EFF_MASK; + frame->can_id |= CAN_EFF_FLAG; + if (ctrl & MB_CNT_RTR) + frame->can_id |= CAN_RTR_FLAG; + } else { + frame->can_id = id >> 18; + if (ctrl & MB_CNT_RTR) + frame->can_id |= CAN_RTR_FLAG; + } + + *(u32 *)frame->data = be32_to_cpu(readl(&mb->data[0])); + *((u32 *)frame->data + 1) = be32_to_cpu(readl(&mb->data[1])); + + stats->rx_packets++; + stats->rx_bytes += frame->can_dlc; + skb->dev = ndev; + skb->protocol = __constant_htons(ETH_P_CAN); + skb->ip_summed = CHECKSUM_UNNECESSARY; + + netif_rx(skb); +} + +static void flexcan_error(struct net_device *ndev, u32 stat) +{ + struct can_frame *cf; + struct sk_buff *skb; + struct flexcan_priv *priv = netdev_priv(ndev); + struct net_device_stats *stats = &ndev->stats; + enum can_state state = priv->can.state; + int error_warning = 0, rx_errors = 0, tx_errors = 0; + + skb = dev_alloc_skb(sizeof(struct can_frame)); + if (!skb) + return; + + skb->dev = ndev; + skb->protocol = __constant_htons(ETH_P_CAN); + skb->ip_summed = CHECKSUM_UNNECESSARY; + + cf = (struct can_frame *)skb_put(skb, sizeof(*cf)); + memset(cf, 0, sizeof(*cf)); + + cf->can_id = CAN_ERR_FLAG; + cf->can_dlc = CAN_ERR_DLC; + + if (stat & ERRSTAT_RWRNINT) { + error_warning = 1; + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING; + } + + if (stat & ERRSTAT_TWRNINT) { + error_warning = 1; + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING; + } + + switch ((stat >> 4) & 0x3) { + case 0: + state = CAN_STATE_ERROR_ACTIVE; + break; + case 1: + state = CAN_STATE_ERROR_PASSIVE; + break; + default: + state = CAN_STATE_BUS_OFF; + break; + } + + if (stat & ERRSTAT_BOFFINT) { + cf->can_id |= CAN_ERR_BUSOFF; + can_bus_off(ndev); + } + + if (stat & ERRSTAT_BIT1ERR) { + rx_errors = 1; + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + cf->data[2] |= CAN_ERR_PROT_BIT1; + } + + if (stat & ERRSTAT_BIT0ERR) { + rx_errors = 1; + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + cf->data[2] |= CAN_ERR_PROT_BIT0; + } + + if (stat & ERRSTAT_FRMERR) { + rx_errors = 1; + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + cf->data[2] |= CAN_ERR_PROT_FORM; + } + + if (stat & ERRSTAT_STFERR) { + rx_errors = 1; + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + cf->data[2] |= CAN_ERR_PROT_STUFF; + } + + + if (stat & ERRSTAT_ACKERR) { + tx_errors = 1; + cf->can_id |= CAN_ERR_ACK; + } + + if (error_warning) + priv->can.can_stats.error_warning++; + if (rx_errors) + stats->rx_errors++; + if (tx_errors) + stats->tx_errors++; + + priv->can.state = state; + + netif_rx(skb); + + ndev->last_rx = jiffies; + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; +} + +static irqreturn_t flexcan_isr(int irq, void *dev_id) +{ + struct net_device *ndev = dev_id; + struct net_device_stats *stats = &ndev->stats; + struct flexcan_priv *priv = netdev_priv(ndev); + struct flexcan_regs __iomem *regs = priv->base; + u32 iflags, errstat; + + errstat = readl(®s->errstat); + if (errstat & ERRSTAT_INT) { + flexcan_error(ndev, errstat); + writel(errstat & ERRSTAT_INT, ®s->errstat); + } + + iflags = readl(®s->iflag1); + + if (iflags & IFLAG_RX_FIFO_OVERFLOW) { + writel(IFLAG_RX_FIFO_OVERFLOW, ®s->iflag1); + stats->rx_over_errors++; + stats->rx_errors++; + } + + while (iflags & IFLAG_RX_FIFO_AVAILABLE) { + struct flexcan_mb __iomem *mb = ®s->cantxfg[0]; + + flexcan_rx_frame(ndev, mb); + writel(IFLAG_RX_FIFO_AVAILABLE, ®s->iflag1); + readl(®s->timer); + iflags = readl(®s->iflag1); + } + + if (iflags & (1 << TX_BUF_ID)) { + stats->tx_packets++; + writel((1 << TX_BUF_ID), ®s->iflag1); + netif_wake_queue(ndev); + } + + return IRQ_HANDLED; +} + +static int flexcan_set_bittiming(struct net_device *ndev) +{ + struct flexcan_priv *priv = netdev_priv(ndev); + struct can_bittiming *bt = &priv->can.bittiming; + struct flexcan_regs __iomem *regs = priv->base; + u32 reg; + + reg = readl(®s->canctrl); + reg &= ~(CANCTRL_SAMP | CANCTRL_PRESDIV(0xff) | + CANCTRL_PSEG1(7) | CANCTRL_PSEG2(7)); + reg |= CANCTRL_PRESDIV(bt->brp - 1) | + CANCTRL_PSEG1(bt->phase_seg1 - 1) | + CANCTRL_PSEG2(bt->phase_seg2 - 1) | + CANCTRL_RJW(3) | + CANCTRL_PROPSEG(bt->prop_seg - 1); + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) + reg |= CANCTRL_SAMP; + writel(reg, ®s->canctrl); + + dev_dbg(&ndev->dev, "setting canctrl=0x%08x\n", reg); + + return 0; +} + +static int flexcan_open(struct net_device *ndev) +{ + int ret, i; + struct flexcan_priv *priv = netdev_priv(ndev); + struct flexcan_regs __iomem *regs = priv->base; + u32 reg; + + ret = open_candev(ndev); + if (ret) + return ret; + + ret = request_irq(ndev->irq, flexcan_isr, 0, DRIVER_NAME, ndev); + if (ret) + goto err_irq; + + clk_enable(priv->clk); + + reg = CANMCR_SOFTRST | CANMCR_HALT; + writel(CANMCR_SOFTRST, ®s->canmcr); + udelay(10); + + if (readl(®s->canmcr) & CANMCR_SOFTRST) { + dev_err(&ndev->dev, "Failed to softreset can module.\n"); + ret = -ENODEV; + goto err_reset; + } + + /* Enable error and bus off interrupt */ + reg = readl(®s->canctrl); + reg |= CANCTRL_CLKSRC | CANCTRL_ERRMSK | CANCTRL_BOFFMSK | + CANCTRL_BOFFREC | CANCTRL_TWRN_MSK | CANCTRL_TWRN_MSK; + writel(reg, ®s->canctrl); + + /* Set lowest buffer transmitted first */ + reg |= CANCTRL_LBUF; + writel(reg, ®s->canctrl); + + for (i = 0; i < 64; i++) { + writel(0, ®s->cantxfg[i].can_dlc); + writel(0, ®s->cantxfg[i].can_id); + writel(0, ®s->cantxfg[i].data[0]); + writel(0, ®s->cantxfg[i].data[1]); + + /* Put MB into rx queue */ + writel(MB_CNT_CODE(0x04), ®s->cantxfg[i].can_dlc); + } + + /* acceptance mask/acceptance code (accept everything) */ + writel(0x0, ®s->rxgmask); + writel(0x0, ®s->rx14mask); + writel(0x0, ®s->rx15mask); + + /* Enable flexcan module */ + reg = readl(®s->canmcr); + reg &= ~(CANMCR_MDIS | CANMCR_FRZ); + reg |= CANMCR_IDAM_C | CANMCR_FEN; + writel(reg, ®s->canmcr); + + /* Synchronize with the can bus */ + reg &= ~CANMCR_HALT; + writel(reg, ®s->canmcr); + + /* Enable interrupts */ + writel(IFLAG_RX_FIFO_OVERFLOW | IFLAG_RX_FIFO_AVAILABLE | + IFLAG_BUF(TX_BUF_ID), + ®s->imask1); + + netif_start_queue(ndev); + + return 0; + +err_reset: + free_irq(ndev->irq, ndev); +err_irq: + close_candev(ndev); + + return ret; +} + +static int flexcan_close(struct net_device *ndev) +{ + struct flexcan_priv *priv = netdev_priv(ndev); + struct flexcan_regs __iomem *regs = priv->base; + + netif_stop_queue(ndev); + + /* Disable all interrupts */ + writel(0, ®s->imask1); + free_irq(ndev->irq, ndev); + + close_candev(ndev); + + /* Disable module */ + writel(CANMCR_MDIS, ®s->canmcr); + clk_disable(priv->clk); + return 0; +} + +static int flexcan_set_mode(struct net_device *ndev, enum can_mode mode) +{ + struct flexcan_priv *priv = netdev_priv(ndev); + struct flexcan_regs __iomem *regs = priv->base; + u32 reg; + + switch (mode) { + case CAN_MODE_START: + reg = readl(®s->canctrl); + reg &= ~CANCTRL_BOFFREC; + writel(reg, ®s->canctrl); + reg |= CANCTRL_BOFFREC; + writel(reg, ®s->canctrl); + priv->can.state = CAN_STATE_ERROR_ACTIVE; + + if (netif_queue_stopped(ndev)) + netif_wake_queue(ndev); + break; + + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static const struct net_device_ops flexcan_netdev_ops = { + .ndo_open = flexcan_open, + .ndo_stop = flexcan_close, + .ndo_start_xmit = flexcan_start_xmit, +}; + +static int register_flexcandev(struct net_device *ndev) +{ + struct flexcan_priv *priv = netdev_priv(ndev); + struct flexcan_regs __iomem *regs = priv->base; + u32 reg; + + reg = readl(®s->canmcr); + reg &= ~CANMCR_MDIS; + writel(reg, ®s->canmcr); + udelay(100); + reg |= CANMCR_FRZ | CANMCR_HALT | CANMCR_FEN; + writel(reg, ®s->canmcr); + + reg = readl(®s->canmcr); + + /* Currently we only support newer versions of this core featuring + * a RX FIFO. Older cores found on some Coldfire derivates are not + * yet supported. + */ + if (!(reg & CANMCR_FEN)) { + dev_err(&ndev->dev, "Could not enable RX FIFO, unsupported " + "core"); + return -ENODEV; + } + + ndev->flags |= IFF_ECHO; /* we support local echo in hardware */ + ndev->netdev_ops = &flexcan_netdev_ops; + + return register_candev(ndev); +} + +static void unregister_flexcandev(struct net_device *ndev) +{ + struct flexcan_priv *priv = netdev_priv(ndev); + struct flexcan_regs __iomem *regs = priv->base; + u32 reg; + + reg = readl(®s->canmcr); + reg |= CANMCR_FRZ | CANMCR_HALT | CANMCR_MDIS; + writel(reg, ®s->canmcr); + + unregister_candev(ndev); +} + +static int __devinit flexcan_probe(struct platform_device *pdev) +{ + struct resource *mem; + struct net_device *ndev; + struct flexcan_priv *priv; + u32 mem_size; + int ret; + + ndev = alloc_candev(sizeof(struct flexcan_priv)); + if (!ndev) + return -ENOMEM; + + priv = netdev_priv(ndev); + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + ndev->irq = platform_get_irq(pdev, 0); + if (!mem || !ndev->irq) { + ret = -ENODEV; + goto failed_req; + } + + mem_size = resource_size(mem); + + if (!request_mem_region(mem->start, mem_size, DRIVER_NAME)) { + ret = -EBUSY; + goto failed_req; + } + + SET_NETDEV_DEV(ndev, &pdev->dev); + + priv->base = ioremap(mem->start, mem_size); + if (!priv->base) { + ret = -ENOMEM; + goto failed_map; + } + + priv->clk = clk_get(&pdev->dev, NULL); + if (IS_ERR(priv->clk)) { + ret = PTR_ERR(priv->clk); + goto failed_clock; + } + priv->can.clock.freq = clk_get_rate(priv->clk); + + platform_set_drvdata(pdev, ndev); + + priv->can.do_set_bittiming = flexcan_set_bittiming; + priv->can.bittiming_const = &flexcan_bittiming_const; + priv->can.do_set_mode = flexcan_set_mode; + + ret = register_flexcandev(ndev); + if (ret) + goto failed_register; + + return 0; + +failed_register: + clk_put(priv->clk); +failed_clock: + iounmap(priv->base); +failed_map: + release_mem_region(mem->start, mem_size); +failed_req: + free_candev(ndev); + + return ret; +} + +static int __devexit flexcan_remove(struct platform_device *pdev) +{ + struct net_device *ndev = platform_get_drvdata(pdev); + struct flexcan_priv *priv = netdev_priv(ndev); + struct resource *mem; + + unregister_flexcandev(ndev); + platform_set_drvdata(pdev, NULL); + iounmap(priv->base); + clk_put(priv->clk); + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + release_mem_region(mem->start, resource_size(mem)); + free_candev(ndev); + + return 0; +} + +static struct platform_driver flexcan_driver = { + .driver = { + .name = DRIVER_NAME, + }, + .probe = flexcan_probe, + .remove = __devexit_p(flexcan_remove), +}; + +static int __init flexcan_init(void) +{ + return platform_driver_register(&flexcan_driver); +} + +static void __exit flexcan_exit(void) +{ + platform_driver_unregister(&flexcan_driver); +} + +module_init(flexcan_init); +module_exit(flexcan_exit); + +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>"); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("CAN port driver for flexcan based chip");