On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote: > Hello, > > Am 08.08.2016 um 13:39 schrieb Andreas Werner: > >On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote: > >>Hello Andreas, > >> > >>a first quick review.... > >> > >>Am 26.07.2016 um 11:16 schrieb Andreas Werner: > >>>This CAN Controller is found on MEN Chameleon FPGAs. > >>> > >>>The driver/device supports the CAN2.0 specification. > >>>There are 255 RX and 255 Tx buffer within the IP. The > >>>pointer for the buffer are handled by HW to make the > >>>access from within the driver as simple as possible. > >>> > >>>The driver also supports parameters to configure the > >>>buffer level interrupt for RX/TX as well as a RX timeout > >>>interrupt. > >>> > >>>With this configuration options, the driver/device > >>>provides flexibility for different types of usecases. > >>> > >>>Signed-off-by: Andreas Werner <andreas.wer...@men.de> > >>>--- > >>>drivers/net/can/Kconfig | 10 + > >>>drivers/net/can/Makefile | 1 + > >>>drivers/net/can/men_z192_can.c | 989 > >>>+++++++++++++++++++++++++++++++++++++++++ > >>>3 files changed, 1000 insertions(+) > >>>create mode 100644 drivers/net/can/men_z192_can.c > > ---snip--- > > >>>+/* Buffer level control values */ > >>>+#define MEN_Z192_MIN_BUF_LVL 0 > >>>+#define MEN_Z192_MAX_BUF_LVL 254 > >>>+#define MEN_Z192_RX_BUF_LVL_DEF 5 > >>>+#define MEN_Z192_TX_BUF_LVL_DEF 5 > >>>+#define MEN_Z192_RX_TOUT_MIN 0 > >>>+#define MEN_Z192_RX_TOUT_MAX 65535 > >>>+#define MEN_Z192_RX_TOUT_DEF 1000 > >>>+ > >>>+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; > >>>+module_param(txlvl, int, S_IRUGO); > >>>+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" > >>>+ __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); > >>>+ > >>>+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; > >>>+module_param(rxlvl, int, S_IRUGO); > >>>+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" > >>>+ __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); > >>>+ > >> > >>What impact does the level have on the latency? Could you please add some > >>comments. > > > >It has a impact on the latency. > >rxlvl = 0 -> if one frame got received, a IRQ will be generated > >rxlvl = 254 -> if 255 frames got received, a IRQ will be generated > > Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can > be good for. The application usually wants the message as soon as possible. > Anyway, the default should be *0*. For RX and TX. >
The HW provides such feature and the driver should be able to control it. It was developed to control the IRQ load (like NAPI) by defining a level of the buffer when the IRQ got asserted. I aggree with you to set the default to "0" which is the main usecase. > >>>+static int rx_timeout = MEN_Z192_RX_TOUT_DEF; > >>>+module_param(rx_timeout, int, S_IRUGO); > >>>+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" > >>>+ __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); > >> > >>Ditto. What is "rx_timeout" good for. > >> > > > >The rx timeout is used im combination with the rxlvl to assert the > >if the buffer level is not reached within this timeout. > > What event will the application receive in case of a timeout. > Its just to control the time when the RX IRQ will be asserted if the buffer level is not reached. Imagine if the rx_timeout is not existing and the rxlvl is set to 50 and only 30 packets are received. The RX IRQ will be never asserted. By defining the rx_timeout, we can control the time when the RX IRQ is asserted if the buffer level is not reached. The application does not receive any special signal, its just the RX IRQ. > >Both, the timeout and the level are used to give the user as much > >control over the latency and the IRQ handling as possible. > >With this two options, the driver can be configured for different > >use cases. > > > >I will add this as the comment to make it more clear. > > Even a bit more would be appreciated. > Sure... > > ---snip--- > > >>>+static int men_z192_read_frame(struct net_device *ndev, unsigned int > >>>frame_nr) > >>>+{ > >>>+ struct net_device_stats *stats = &ndev->stats; > >>>+ struct men_z192 *priv = netdev_priv(ndev); > >>>+ struct men_z192_cf_buf __iomem *cf_buf; > >>>+ struct can_frame *cf; > >>>+ struct sk_buff *skb; > >>>+ u32 cf_offset; > >>>+ u32 length; > >>>+ u32 data; > >>>+ u32 id; > >>>+ > >>>+ skb = alloc_can_skb(ndev, &cf); > >>>+ if (unlikely(!skb)) { > >>>+ stats->rx_dropped++; > >>>+ return 0; > >>>+ } > >>>+ > >>>+ cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; > >>>+ > >>>+ cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; > >>>+ length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; > >>>+ id = readl(&cf_buf->can_id); > >>>+ > >>>+ if (id & MEN_Z192_CFBUF_IDE) { > >>>+ /* Extended frame */ > >>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; > >>>+ cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> > >>>+ MEN_Z192_CFBUF_ID2_SHIFT; > >>>+ > >>>+ cf->can_id |= CAN_EFF_FLAG; > >>>+ > >>>+ if (id & MEN_Z192_CFBUF_E_RTR) > >>>+ cf->can_id |= CAN_RTR_FLAG; > >>>+ } else { > >>>+ /* Standard frame */ > >>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> > >>>+ MEN_Z192_CFBUF_ID1_SHIFT; > >>>+ > >>>+ if (id & MEN_Z192_CFBUF_S_RTR) > >>>+ cf->can_id |= CAN_RTR_FLAG; > >>>+ } > >>>+ > >>>+ cf->can_dlc = get_can_dlc(length); > >>>+ > >>>+ /* remote transmission request frame > >>>+ * contains no data field even if the > >>>+ * data length is set to a value > 0 > >>>+ */ > >>>+ if (!(cf->can_id & CAN_RTR_FLAG)) { > >>>+ if (cf->can_dlc > 0) { > >>>+ data = readl(&cf_buf->data[0]); > >>>+ *(__be32 *)cf->data = cpu_to_be32(data); > >> > >>Do you really need the extra copy? > >> > >>>+ } > >>>+ if (cf->can_dlc > 4) { > >>>+ data = readl(&cf_buf->data[1]); > >>>+ *(__be32 *)(cf->data + 4) = cpu_to_be32(data); > >> > >>Ditto. > > > >No its not really needed. I thought its more clean and more readable than > >putting this in one line withouth the copy. > > It should be fast in the first place. > Ok, will change that. [...] > > >>>+static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) > >>>+{ > >>>+ int ret; > >>>+ > >>>+ switch (mode) { > >>>+ case CAN_MODE_START: > >>>+ ret = men_z192_start(ndev); > >>>+ if (ret) > >>>+ return ret; > >> > >>"if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and > >>in many other places. > >> > > > >Yes and no. I think its a general question about the naming of those > >variables. > >I will check all the variables in the driver if it really makes sense > >to rename it. > > > >For my opinion, "ret" is more generic. But you are right, "err" would be more > >readable in some places. > > if (err) > > makes immediately clear that it's an error case. ret is more general, e.g. > for the return value of read/write: > > if (ret < 0) > error-case > else if (ret == 0) > end-of-file > else > btyes-read > > Just my personal preference to make the code more readable. Ok, I will think about it. > > >>>+ > >>>+ netif_wake_queue(ndev); > >>>+ break; > >>>+ default: > >>>+ return -EOPNOTSUPP; > >>>+ } > >>>+ > >>>+ return 0; > >>>+static int men_z192_probe(struct mcb_device *mdev, > >>>+ const struct mcb_device_id *id) > >>>+{ > >>>+ struct device *dev = &mdev->dev; > >>>+ struct men_z192 *priv; > >>>+ struct net_device *ndev; > >>>+ void __iomem *dev_base; > >>>+ struct resource *mem; > >>>+ u32 timebase; > >>>+ int ret = 0; > >>>+ int irq; > >>>+ > >>>+ mem = mcb_request_mem(mdev, dev_name(dev)); > >>>+ if (IS_ERR(mem)) { > >>>+ dev_err(dev, "failed to request device memory"); > >>>+ return PTR_ERR(mem); > >>>+ } > >>>+ > >>>+ dev_base = ioremap(mem->start, resource_size(mem)); > >>>+ if (!dev_base) { > >>>+ dev_err(dev, "failed to ioremap device memory"); > >>>+ ret = -ENXIO; > >>>+ goto out_release; > >>>+ } > >>>+ > >>>+ irq = mcb_get_irq(mdev); > >>>+ if (irq <= 0) { > >>>+ ret = -ENODEV; > >>>+ goto out_unmap; > >>>+ } > >>>+ > >>>+ ndev = alloc_candev(sizeof(struct men_z192), 1); > >> > >>You specify here one echo_skb but it's not used anywhere. Local loopback > >>seems not to be implemented. > >> > > > >Agree with you, will set it to "0". > > No, the local loopback is mandetory! > Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c it is not mandatory. In the Documentation/networking/can.txt there is also a "should" and a fallback mechnism if the driver does not support the local loopback. I'm currently ok with this fallback mechanism. Anyway I am not sure that the driver can handle the echo skb correctly. If i understand it correctly, the can_get_echo_skb() is normally called on a "TX done IRQ" to get the skb and loop it back. I do not have such a "TX done IRQ" and have not implemented implemented and added the local looback. May be I can put and get the echo skb within the xmit function? Does this make sense? Regards Andy > Wolfgang.