On Fri, Nov 13, 2009 at 9:14 AM, Wolfram Sang <w.s...@pengutronix.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.s...@pengutronix.de>
> Cc: Wolfgang Grandegger <w...@grandegger.com>
> Cc: Grant Likely <grant.lik...@secretlab.ca>
> Cc: David Miller <da...@davemloft.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(®s->cantarq, priv->tx_active);
> + /* Suppress TX done interrupts */
> + out_8(®s->cantier, 0);
> + }
> +
> + canctl1 = in_8(®s->canctl1);
> + if ((mode & MSCAN_SLPRQ) && (canctl1 & MSCAN_SLPAK) == 0) {
> + out_8(®s->canctl0,
> + in_8(®s->canctl0) | MSCAN_SLPRQ);
> + for (i = 0; i < MSCAN_SET_MODE_RETRIES; i++) {
> + if (in_8(®s->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(®s->canctl0,
> + in_8(®s->canctl0) | MSCAN_INITRQ);
> + for (i = 0; i < MSCAN_SET_MODE_RETRIES; i++) {
> + if (in_8(®s->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(®s->canctl0,
> + in_8(®s->canctl0) | MSCAN_CSWAI);
> +
> + } else {
> + canctl1 = in_8(®s->canctl1);
> + if (canctl1 & (MSCAN_SLPAK | MSCAN_INITAK)) {
> + out_8(®s->canctl0, in_8(®s->canctl0) &
> + ~(MSCAN_SLPRQ | MSCAN_INITRQ));
> + for (i = 0; i < MSCAN_SET_MODE_RETRIES; i++) {
> + canctl1 = in_8(®s->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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
