Hi Wolfram, what is your plan now? Provide patches for mainline inclusion (net-next-2.6) or for the Socket-CAN SVN trunk? Please don't send patches for both. We can do the backport to the SVN trunk later.
Wolfram Sang wrote: > This patch adds MPC512x support to mscan. Tested with a phyCORE-MPC5121e and > 2.6.31. > cansequence ran quite some time successfully. > > Note 1: It depends on changes to the clock-implementation of the MPC512x. > Those patches need to be discussed and approved on the ppc list and can be > found here: > http://patchwork.ozlabs.org/patch/37417/ > http://patchwork.ozlabs.org/patch/37427/ > (If they work for you, I'd be happy about Acked-by-tags.) My impression is that they will not be accepted as already a new clock interface is available (via patches). > Note 2: If this approach is accepted, I will next do the remaining cleanup > work (giving the functions the proper 5xxx names, ...) as follow up patches to > hopefully get this driver into mainline soonish. Would be nice to have that fixed a.s.a.p. > Signed-off-by: Wolfram Sang <[email protected]> > > Kconfig | 8 +++--- > mscan/mpc52xx_can.c | 64 > +++++++++++++++++++++++++++++++++++++++++----------- > mscan/mscan.h | 3 +- > 3 files changed, 57 insertions(+), 18 deletions(-) I assume this patch is for the SVN trunk. Please provide patches against top of tree, and not a subdirectory to ease testing. > Index: Kconfig > =================================================================== > --- Kconfig (revision 1072) > +++ Kconfig (working copy) > @@ -234,12 +234,12 @@ > the Motorola MC68HC12 Microcontroller Family. > > config CAN_MPC52XX > - tristate "Freescale MPC5200 onboard CAN controller" > - depends on CAN_MSCAN && (PPC_MPC52xx || PPC_52xx) > + tristate "Freescale MPC5xxx onboard CAN controller" > + depends on CAN_MSCAN && (PPC_MPC512x || PPC_MPC52xx || PPC_52xx) > default LITE5200 > ---help--- > - If you say yes here you get support for Freescale MPC5200 > - onboard dualCAN controller. > + If you say yes here you get support for Freescale MPC5xxx > + onboard CAN controller. > > This driver can also be built as a module. If so, the module > will be called mpc52xx_can. > Index: mscan/mscan.h > =================================================================== > --- mscan/mscan.h (revision 1072) > +++ mscan/mscan.h (working copy) > @@ -52,7 +52,8 @@ > #define MSCAN_FOR_MPC5200 > #endif > > -#ifdef MSCAN_FOR_MPC5200 > +/* Following block only needed for MPC52xx! */ > +#ifdef CONFIG_PPC_MPC52xx > #define MSCAN_CLKSRC_BUS 0 > #define MSCAN_CLKSRC_XTAL MSCAN_CLKSRC > #else > Index: mscan/mpc52xx_can.c As said above, please do s/mpc52xx/mpc5xxx/ immediately. It does not harm if we have both, the old mpc52xx_can.c and the new mpc5xxx_can.c in the SVN trunk including the related CONFIG_CAN_* parameters. The mpc5xxx_can.c should not include the legacy driver stuff (pre-OF), of course. > =================================================================== > --- mscan/mpc52xx_can.c (revision 1072) > +++ mscan/mpc52xx_can.c (working copy) > @@ -4,6 +4,7 @@ > * Copyright (C) 2004-2005 Andrey Volkov <[email protected]>, > * Varma Electronics Oy > * Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]> > + * Copyright (C) 2009 Wolfram Sang <[email protected]> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the version 2 of the GNU General Public License > @@ -28,6 +29,7 @@ > #include <socketcan/can/dev.h> > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,27) > #include <linux/of_platform.h> > +#include <linux/clk.h> > #include <sysdev/fsl_soc.h> > #endif > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,16) > @@ -260,19 +262,53 @@ > > /* > * Get the frequency of the external oscillator clock connected > - * to the SYS_XTAL_IN pin, or retrun 0 if it cannot be determined. > + * to the SYS_XTAL_IN pin, or return 0 if it cannot be determined. > */ > -static unsigned int __devinit mpc52xx_can_xtal_freq(struct device_node *np) > + > +#define CLK_NAME_SIZE 14 > + > +static unsigned int __devinit mpc512x_can_clock_freq(struct of_device > *ofdev, > + int clock_src) > { > + struct clk *mscan_clk, *port_clk; > + char clk_name[CLK_NAME_SIZE]; > + const u32 *cellp; > + > + cellp = of_get_property(ofdev->node, "cell-index", NULL); > + if (!cellp) > + return 0; Hm, the mscan node does not have a cell-index any more because it's deprecated (removed by the device tree police), IIRC. > + mscan_clk = clk_get(NULL, "mscan_clk"); > + if (!mscan_clk) { > + dev_err(&ofdev->dev, "can't get mscan clock!\n"); > + return 0; > + } > + clk_enable(mscan_clk); > + > + if (clock_src == MSCAN_CLKSRC_BUS) > + return mpc5xxx_get_bus_frequency(ofdev->node); > + > + /* Do we have a port clock (Chip Rev2 and later) */ > + snprintf(clk_name, CLK_NAME_SIZE, "mscan%d_clk", *cellp); > + port_clk = clk_get(&ofdev->dev, clk_name); > + > + if (port_clk) > + return clk_get_rate(port_clk); > + else > + return clk_get_rate(mscan_clk); > +} I have not looked into your clock patches, but how can the user select the clock source and the clock frequency (via clock divider)? This should be selectable via DTS file, I think. > +static unsigned int __devinit mpc52xx_can_xtal_freq(struct of_device *ofdev) > +{ > struct mpc52xx_cdm __iomem *cdm; > struct device_node *np_cdm; > unsigned int freq; > u32 val; > > #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,31) > - freq = mpc52xx_find_ipb_freq(np); > + freq = mpc52xx_find_ipb_freq(ofdev->node); > #else > - freq = mpc5xxx_get_bus_frequency(np); > + freq = mpc5xxx_get_bus_frequency(ofdev->node); > #endif > if (!freq) > return 0; > @@ -284,7 +320,7 @@ > cdm = of_iomap(np_cdm, 0); > of_node_put(np_cdm); > if (!np_cdm) { > - printk(KERN_ERR "%s() failed abnormally\n", __func__); > + dev_err(&ofdev->dev, "couldn't find clock!"); > return 0; > } > > @@ -314,22 +350,21 @@ > * bug, it can not be selected for the old MPC5200 Rev. A chips. > */ > > -static unsigned int __devinit mpc52xx_can_clock_freq(struct device_node *np, > +static unsigned int __devinit mpc52xx_can_clock_freq(struct of_device > *ofdev, > int clock_src) > { > unsigned int pvr; > - > pvr = mfspr(SPRN_PVR); > > #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,31) > if (clock_src == MSCAN_CLKSRC_BUS || pvr == 0x80822011) > - return mpc52xx_find_ipb_freq(np); > + return mpc52xx_find_ipb_freq(ofdev->node); > #else > if (clock_src == MSCAN_CLKSRC_BUS || pvr == 0x80822011) > - return mpc5xxx_get_bus_frequency(np); > + return mpc5xxx_get_bus_frequency(ofdev->node); > #endif > > - return mpc52xx_can_xtal_freq(np); > + return mpc52xx_can_xtal_freq(ofdev); > } > > static int __devinit mpc52xx_can_probe(struct of_device *ofdev, > @@ -340,6 +375,7 @@ > struct can_priv *priv; > struct resource res; > void __iomem *base; > + unsigned int (*can_clock)(struct of_device *, int) = id->data; > int err, irq, res_size, clock_src; > > err = of_address_to_resource(np, 0, &res); > @@ -393,7 +429,8 @@ > clock_src = MSCAN_CLKSRC_BUS; > else > clock_src = MSCAN_CLKSRC_XTAL; > - priv->clock.freq = mpc52xx_can_clock_freq(np, clock_src); > + > + priv->clock.freq = can_clock(ofdev, clock_src); > if (!priv->clock.freq) { > dev_err(&ofdev->dev, "couldn't get MSCAN clock frequency\n"); > err = -ENODEV; > @@ -488,8 +525,9 @@ > #endif > > static struct of_device_id __devinitdata mpc52xx_can_table[] = { > - {.compatible = "fsl,mpc5200-mscan"}, > - {.compatible = "fsl,mpc5200b-mscan"}, > + { .compatible = "fsl,mpc5200-mscan", .data = mpc52xx_can_clock_freq }, > + { .compatible = "fsl,mpc5200b-mscan", .data = mpc52xx_can_clock_freq }, > + { .compatible = "fsl,mpc5121-mscan", .data = mpc512x_can_clock_freq }, > {}, > }; What's also missing is the proper bus-off recovery method for the MCP512x. Unfortunately, it's also not yet OK for the MPC5200. It should be implemented as listed below: - if possible, automatic bus-off recovery should be turned off, which is possible on the MPC512x (but not the default for compatibility with the MPC5200. - if bus-off recovery cannot be switched of, the driver should handle it as follows: - if restart_ms == 0, the device should be stopped on bus-off to suppress automatic recovery. - if restart_ms > 0, the hardware should do the automatic recovery and a RESTARTED error message should be sent when it re-enters the error-active state (or leaves the bus-off state). Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
