Message-ID: <20160809061038.GC1733@awelinux>
Date:	Tue, 9 Aug 2016 08:10:38 +0200
From:	Andreas Werner <andreas.werner@....de>
To:	Wolfgang Grandegger <wg@...ndegger.com>
CC:	Andreas Werner <andreas.werner@....de>, <mkl@...gutronix.de>,
	<linux-can@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <davem@...emloft.net>,
	<jthumshirn@...e.de>, <andy@...nerandy.de>,
	<michael.miehling@....de>
Subject: Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller
 driver

On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote:
> Am 08.08.2016 um 16:05 schrieb Andreas Werner:
> >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.werner@....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.
> 
> Now I got it. After timeout an interrupt will be trigger regardless of the
> thresholds. The default settings should result in minimum latencies.
> 

yes :-)
I will set the rx_timeout to 0 to get the minimum latency.

> >>>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.
> 
> Well, s/driver/hardware/ ! Local loopback is the preferred mechanism.
> 

Sure...

> >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.
> 
> What does "MEN_Z192_TFLG_TXIF" signal?
> 

It is not a "TX Done" IRQ, it is the tx buffer level IRQ.
The IRQ is triggered when the number of available tx buffer entries is as
configured with txlvl. (after the buffer was full)

Example:
txlvl = 0
tx buffer has 255 entries. 

-> The IRQ is triggered as soon as 1 frame got transmitted (254 entries).

---

txlvl = 254
tx buffer has 255 entries.

-> The IRQ is triggered as soon as the buffer has one entry and it got transmitted


> >May be I can put and get the echo skb within the xmit function?
> >Does this make sense?
> 
> It only makes sense if the driver knows when one or more transfers are done.
> 

Then i do not think that I can use the txlvl IRQ in this case and need to use
the fallback mechanism.

> Wolfgang.

Powered by blists - more mailing lists