lkml.org 
[lkml]   [2016]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver
From
Date
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@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.

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.

\
 
 \ /
  Last update: 2016-08-08 17:01    [W:0.107 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site