Date:	Mon, 8 Aug 2016 16:35:34 +0200
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Andreas Werner <andreas.werner@....de>
Cc:	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

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.

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

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

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

Wolfgang.

Powered by blists - more mailing lists