Message-ID: <fa686aa40911130939x54ac53f9x173a875a5a4435d3@mail.gmail.com>
Date:	Fri, 13 Nov 2009 10:39:56 -0700
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Wolfram Sang <w.sang@...gutronix.de>
Cc:	netdev@...r.kernel.org, socketcan-core@...ts.berlios.de,
	linuxppc-dev@...abs.org, Wolfgang Grandegger <wg@...ndegger.com>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH] net/can: add driver for mscan family & mpc52xx_mscan

On Fri, Nov 13, 2009 at 9:14 AM, Wolfram Sang <w.sang@...gutronix.de> wrote:
> Taken from socketcan-svn, fixed remaining todos, cleaned up, tested with a
> phyCORE-MPC5200B-IO and a custom board.
>
> Signed-off-by: Wolfram Sang <w.sang@...gutronix.de>
> Cc: Wolfgang Grandegger <wg@...ndegger.com>
> Cc: Grant Likely <grant.likely@...retlab.ca>
> Cc: David Miller <davem@...emloft.net>

I don't see any locking in this driver.  What keeps the mscan_isr or
other routines from conflicting with each other?  What is the
concurrency model for CAN devices?

More comments below.  I don't have the background to delve into the
CAN details, but I can make some comments on the general structure of
the driver.

g.

> ---
>
> +static struct of_device_id mpc52xx_cdm_ids[] __devinitdata = {
> +       { .compatible = "fsl,mpc5200-cdm", },
> +       { .compatible = "fsl,mpc5200b-cdm", },
> +       {}
> +};

You can drop the 'b' line. The 'b' version is compatible with the
original, and all in-tree 5200b files claim compatibility with the
non-'b' version.

> +
> +/*
> + * Get the frequency of the external oscillator clock connected
> + * to the SYS_XTAL_IN pin, or return 0 if it cannot be determined.
> + */
> +static unsigned int  __devinit mpc52xx_can_xtal_freq(struct of_device *of)
> +{
> +       struct mpc52xx_cdm  __iomem *cdm;
> +       struct device_node *np_cdm;
> +       unsigned int freq;
> +       u32 val;
> +
> +       freq = mpc5xxx_get_bus_frequency(of->node);
> +       if (!freq)
> +               return 0;
> +
> +       /*
> +        * Determine SYS_XTAL_IN frequency from the clock domain settings
> +        */
> +       np_cdm = of_find_matching_node(NULL, mpc52xx_cdm_ids);
> +       if (!np_cdm) {
> +               dev_err(&of->dev, "can't get clock node!\n");
> +               return 0;
> +       }
> +       cdm = of_iomap(np_cdm, 0);
> +       of_node_put(np_cdm);
> +
> +       if (in_8(&cdm->ipb_clk_sel) & 0x1)
> +               freq *= 2;
> +       val  = in_be32(&cdm->rstcfg);
> +       if (val & (1 << 5))
> +               freq *= 8;
> +       else
> +               freq *= 4;

freq *= (val & (1 << 5)) ? 8 : 4;

> +       if (val & (1 << 6))
> +               freq /= 12;
> +       else
> +               freq /= 16;

Ditto.

> +
> +       iounmap(cdm);
> +
> +       return freq;
> +}
> +
> +/*
> + * Get frequency of the MSCAN clock source
> + *
> + * Either the oscillator clock (SYS_XTAL_IN) or the IP bus clock (IP_CLK)
> + * can be selected. According to the MPC5200 user's manual, the oscillator
> + * clock is the better choice as it has less jitter but due to a hardware
> + * bug, it can not be selected for the old MPC5200 Rev. A chips.
> + */
> +
> +static unsigned int  __devinit mpc52xx_can_clock_freq(struct of_device *of,
> +                                                     int clock_src)
> +{
> +       unsigned int pvr;
> +
> +       pvr = mfspr(SPRN_PVR);
> +
> +       if (clock_src == MSCAN_CLKSRC_BUS || pvr == 0x80822011)
> +               return mpc5xxx_get_bus_frequency(of->node);
> +
> +       return mpc52xx_can_xtal_freq(of);
> +}

mpc52xx_can_xtal_freq() is only used by this function.  Do they need
to be separate?

> +static int __devinit mpc5xxx_can_probe(struct of_device *ofdev,
> +                                      const struct of_device_id *id)
> +{
> +       struct device_node *np = ofdev->node;
> +       struct net_device *dev;
> +       struct mscan_priv *priv;
> +       void __iomem *base;
> +       const char *clk_src;
> +       int err, irq, clock_src;
> +
> +       base = of_iomap(ofdev->node, 0);
> +       if (!base) {
> +               dev_err(&ofdev->dev, "couldn't ioremap\n");
> +               err = -ENOMEM;
> +               goto exit_release_mem;
> +       }
> +
> +       irq = irq_of_parse_and_map(np, 0);
> +       if (!irq) {
> +               dev_err(&ofdev->dev, "no irq found\n");
> +               err = -ENODEV;
> +               goto exit_unmap_mem;
> +       }
> +
> +       dev = alloc_mscandev();
> +       if (!dev) {
> +               err = -ENOMEM;
> +               goto exit_dispose_irq;
> +       }
> +
> +       priv = netdev_priv(dev);
> +       priv->reg_base = base;
> +       dev->irq = irq;
> +
> +       /*
> +        * Either the oscillator clock (SYS_XTAL_IN) or the IP bus clock
> +        * (IP_CLK) can be selected as MSCAN clock source. According to
> +        * the MPC5200 user's manual, the oscillator clock is the better
> +        * choice as it has less jitter. For this reason, it is selected
> +        * by default.
> +        */
> +       clk_src = of_get_property(np, "fsl,mscan-clk-src", NULL);
> +       if (clk_src && strcmp(clk_src, "ip") == 0)

Should protect against non-null.  strncmp() maybe?


> +static struct of_device_id __devinitdata mpc5xxx_can_table[] = {
> +       {.compatible = "fsl,mpc5200-mscan"},
> +       {.compatible = "fsl,mpc5200b-mscan"},
> +       {},
> +};

Ditto here; the 'b' version can be dropped.

> +static int mscan_set_mode(struct net_device *dev, u8 mode)
> +{
> +       struct mscan_priv *priv = netdev_priv(dev);
> +       struct mscan_regs *regs = (struct mscan_regs *)priv->reg_base;
> +       int ret = 0;
> +       int i;
> +       u8 canctl1;
> +
> +       if (mode != MSCAN_NORMAL_MODE) {
> +
> +               if (priv->tx_active) {
> +                       /* Abort transfers before going to sleep */#
> +                       out_8(&regs->cantarq, priv->tx_active);
> +                       /* Suppress TX done interrupts */
> +                       out_8(&regs->cantier, 0);
> +               }
> +
> +               canctl1 = in_8(&regs->canctl1);
> +               if ((mode & MSCAN_SLPRQ) && (canctl1 & MSCAN_SLPAK) == 0) {
> +                       out_8(&regs->canctl0,
> +                             in_8(&regs->canctl0) | MSCAN_SLPRQ);
> +                       for (i = 0; i < MSCAN_SET_MODE_RETRIES; i++) {
> +                               if (in_8(&regs->canctl1) & MSCAN_SLPAK)
> +                                       break;
> +                               udelay(100);

Ugh.  Can you sleep instead?  This burns a lot of CPU cycles to no purpose.

> +                       }
> +                       /*
> +                        * The mscan controller will fail to enter sleep mode,
> +                        * while there are irregular activities on bus, like
> +                        * somebody keeps retransmitting. This behavior is
> +                        * undocumented and seems to differ between mscan built
> +                        * in mpc5200b and mpc5200. We proceed in that case,
> +                        * since otherwise the slprq will be kept set and the
> +                        * controller will get stuck. NOTE: INITRQ or CSWAI
> +                        * will abort all active transmit actions, if still
> +                        * any, at once.
> +                        */
> +                       if (i >= MSCAN_SET_MODE_RETRIES)
> +                               dev_dbg(dev->dev.parent,
> +                                       "device failed to enter sleep mode. "
> +                                       "We proceed anyhow.\n");
> +                       else
> +                               priv->can.state = CAN_STATE_SLEEPING;
> +               }
> +
> +               if ((mode & MSCAN_INITRQ) && (canctl1 & MSCAN_INITAK) == 0) {
> +                       out_8(&regs->canctl0,
> +                             in_8(&regs->canctl0) | MSCAN_INITRQ);
> +                       for (i = 0; i < MSCAN_SET_MODE_RETRIES; i++) {
> +                               if (in_8(&regs->canctl1) & MSCAN_INITAK)
> +                                       break;
> +                       }
> +                       if (i >= MSCAN_SET_MODE_RETRIES)
> +                               ret = -ENODEV;
> +               }
> +               if (!ret)
> +                       priv->can.state = CAN_STATE_STOPPED;
> +
> +               if (mode & MSCAN_CSWAI)
> +                       out_8(&regs->canctl0,
> +                             in_8(&regs->canctl0) | MSCAN_CSWAI);
> +
> +       } else {
> +               canctl1 = in_8(&regs->canctl1);
> +               if (canctl1 & (MSCAN_SLPAK | MSCAN_INITAK)) {
> +                       out_8(&regs->canctl0, in_8(&regs->canctl0) &
> +                             ~(MSCAN_SLPRQ | MSCAN_INITRQ));
> +                       for (i = 0; i < MSCAN_SET_MODE_RETRIES; i++) {
> +                               canctl1 = in_8(&regs->canctl1);
> +                               if (!(canctl1 & (MSCAN_INITAK | MSCAN_SLPAK)))
> +                                       break;
> +                       }
> +                       if (i >= MSCAN_SET_MODE_RETRIES)
> +                               ret = -ENODEV;
> +                       else
> +                               priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +               }
> +       }
> +       return ret;
> +}

[snip]

> +static int mscan_do_set_mode(struct net_device *dev, enum can_mode mode)
> +{
> +
> +       struct mscan_priv *priv = netdev_priv(dev);
> +       int ret = 0;
> +
> +       if (!priv->open_time)
> +               return -EINVAL;
> +
> +       switch (mode) {
> +       case CAN_MODE_SLEEP:
> +       case CAN_MODE_STOP:
> +               netif_stop_queue(dev);
> +               mscan_set_mode(dev,
> +                              (mode ==
> +                               CAN_MODE_STOP) ? MSCAN_INIT_MODE :
> +                              MSCAN_SLEEP_MODE);

A little hard on the eyes.  Can you rework to not spill over 4 lines?
(ie. calc mode flag on the line above?)

> +               break;
> +       case CAN_MODE_START:
> +               if (priv->can.state <= CAN_STATE_BUS_OFF)
> +                       mscan_set_mode(dev, MSCAN_INIT_MODE);
> +               ret = mscan_start(dev);
> +               if (ret)
> +                       break;
> +               if (netif_queue_stopped(dev))
> +                       netif_wake_queue(dev);
> +               break;
> +
> +       default:
> +               ret = -EOPNOTSUPP;
> +               break;
> +       }
> +       return ret;
> +}

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists