Message-ID: <527CAC38.6040800@pengutronix.de>
Date:	Fri, 08 Nov 2013 10:17:44 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
CC:	netdev@...r.kernel.org, wg@...ndegger.com,
	linux-can@...r.kernel.org, linux-sh@...r.kernel.org,
	vksavl@...il.com
Subject: Re: [PATCH v3] can: add Renesas R-Car CAN driver

On 11/08/2013 12:57 AM, Sergei Shtylyov wrote:
>>> Index: linux-can-next/drivers/net/can/rcar_can.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ linux-can-next/drivers/net/can/rcar_can.c
>>> @@ -0,0 +1,893 @@
>>> +/*
>>> + * Renesas R-Car CAN device driver
>>> + *
>>> + * Copyright (C) 2013 Cogent Embedded, Inc. <source@...entembedded.com>
>>> + * Copyright (C) 2013 Renesas Solutions Corp.
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/types.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/netdevice.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/can/led.h>
>>> +#include <linux/can/dev.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/can/platform/rcar_can.h>
>>> +
>>> +#define DRV_NAME    "rcar_can"
>>> +
>>> +/* Mailbox configuration:
>>> + * mailbox 0 - not used
>>> + * mailbox 1-31 - Rx
>>> + * mailbox 32-63 - Tx
>>> + * no FIFO mailboxes
>>> + */
>>> +#define N_MBX        64
>>> +#define FIRST_TX_MB    32
>>> +#define N_TX_MB        (N_MBX - FIRST_TX_MB)
>>> +#define RX_MBX_MASK    0xFFFFFFFE
> 
>> Please use a common prefix for all defines.
> 
>    OK, done now. Could you however explain why the file-local #define's
> should be prefixed? It's not quite obvious to me...

It's about readability and maintainability. If you don't know the
driver, but all driver local defines and functions have a common prefix,
it's much easier to read if you are not the author of the driver IMHO.

> 
> [...]
>>> +static netdev_tx_t rcar_can_start_xmit(struct sk_buff *skb,
>>> +                       struct net_device *ndev)
>>> +{
>>> +    struct rcar_can_priv *priv = netdev_priv(ndev);
>>> +    struct can_frame *cf = (struct can_frame *)skb->data;
>>> +    u32 data, mier1, mbxno, i;
>>> +    unsigned long flags;
>>> +    u8 mctl = 0;
>>> +
>>> +    if (can_dropped_invalid_skb(ndev, skb))
>>> +        return NETDEV_TX_OK;
>>> +
>>> +    spin_lock_irqsave(&priv->mier_lock, flags);
>>> +    mier1 = readl(&priv->regs->mier1);
>>> +    if (mier1) {
>>> +        i = __builtin_clz(mier1);
>>> +        mbxno = i ? N_MBX - i : FIRST_TX_MB;
>>> +    } else {
>>> +        mbxno = FIRST_TX_MB;
>>> +    }
> 
>> Can you explain how the hardware arbitration works, and you do you
>> guarantee the CAN frames are send by the hardware in the same order you
>> put them into the hardware.
> 
>    Tx mailbox with the smallest mailbox number has the highest priority.
> The other possible Tx mailbox selection rule (not used by the driver
> now) is ID priority transmit mode (as defined in the ISO 11898-1 specs).
> The algorithm used guarantees the mailboxes are filled sequentially.

I see. You are using mier1 to track the used mailboxes....correct?

> +	if (unlikely(mier1 == 0xffffffff))
> +		netif_stop_queue(ndev);

Then you have a race condition in your tx-complete handler
rcar_can_tx_done(), as you call netif_wake_queue() unconditionally. If
mier1 == 0xffffffff you must wait until _all_ mailboxes are transmitted
until you are allowed to reenable the mailboxes. Have a look at the
at91_can driver, it's using a similar scheme. The lowest mailbox is
transmitted first, but there are three additional bits that indicate the
priority.

> I've used 'canfdtest' as suggested by Wolfgang Grandegger to verify, see
> the log below:
> 
> root@...35x-evm:~# ./canfdtest -v -g can0
> interface = can0, family = 29, type = 3, proto = 1
> ...............................................................................C
> 
> Test messages sent and received: 483203
> Exiting...
> root@...35x-evm:~#
> 
> [...]
>>> +static int rcar_can_rx_poll(struct napi_struct *napi, int quota)
>>> +{
>>> +    struct rcar_can_priv *priv = container_of(napi,
>>> +                          struct rcar_can_priv, napi);
>>> +    int num_pkts = 0;
>>> +
>>> +    /* Find mailbox */
>>> +    while (num_pkts < quota) {
>>> +        u8 mctl, mbx;
>>> +
>>> +        mbx = readb(&priv->regs->mssr);
> 
>> How does the RX work? Is it a hardware FIFO?
> 
>    In short, the MSSR register provides the smallest Rx mailbox number
> that is looked up in the Rx search mode. We read MSSR until no search
> results can be obtained, so it is some sort of FIFO.

This looks racy....

>    And there is separate FIFO operation mode: some mailboxes can be
> configured as FIFO and serviced by special registers but this operation
> mode is not supported by the driver.

if you hardware supports a real FIFO then I strongly suggest to make use
of it.

> [...]
>>> +static int rcar_can_probe(struct platform_device *pdev)
>>> +{
>>> +    struct rcar_can_platform_data *pdata;
>>> +    struct rcar_can_priv *priv;
>>> +    struct net_device *ndev;
>>> +    struct resource *mem;
>>> +    void __iomem *addr;
>>> +    int err = -ENODEV;
>>> +    int irq;
>>> +
>>> +    pdata = dev_get_platdata(&pdev->dev);
>>> +    if (!pdata) {
>>> +        dev_err(&pdev->dev, "No platform data provided!\n");
>>> +        goto fail;
>>> +    }
>>> +
>>> +    irq = platform_get_irq(pdev, 0);
>>> +    if (!irq) {
>>> +        dev_err(&pdev->dev, "No IRQ resource\n");
>>> +        goto fail;
>>> +    }
>>> +
>>> +    mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +    addr = devm_ioremap_resource(&pdev->dev, mem);
>>> +    if (IS_ERR(addr)) {
>>> +        err = PTR_ERR(addr);
>>> +        goto fail;
>>> +    }
>>> +
>>> +    ndev = alloc_candev(sizeof(struct rcar_can_priv), N_MBX -
>>> FIRST_TX_MB);
>>> +    if (!ndev) {
>>> +        dev_err(&pdev->dev, "alloc_candev failed\n");
>>> +        err = -ENOMEM;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    priv = netdev_priv(ndev);
>>> +
>>> +    priv->clk = devm_clk_get(&pdev->dev, NULL);
>>> +    if (IS_ERR(priv->clk)) {
>>> +        err = PTR_ERR(priv->clk);
>>> +        dev_err(&pdev->dev, "cannot get clock: %d\n", err);
>>> +        goto fail_clk;
>>> +    }
>>> +
>>> +    ndev->netdev_ops = &rcar_can_netdev_ops;
>>> +    ndev->irq = irq;
>>> +    ndev->flags |= IFF_ECHO;
>>> +    priv->ndev = ndev;
>>> +    priv->regs = addr;
>>> +    priv->clock_select = pdata->clock_select;
>>> +    priv->can.clock.freq = clk_get_rate(priv->clk);
>>> +    priv->can.bittiming_const = &rcar_can_bittiming_const;
>>> +    priv->can.do_set_bittiming = rcar_can_set_bittiming;
> 
>> Please call this function directly during the open() function.
> 
>    OK, done, and the method installation was removed. However, I'm not
> sure why you requested this as many drivers are still using the method.

The callback was there from the beginning, but then we figured out that
we don't need it in the driver, but no one has cleaned up the drivers
yet. So don't use it in new drivers. I know it's not documented anywhere :(

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


Download attachment "signature.asc" of type "application/pgp-signature" (260 bytes)

Powered by blists - more mailing lists