Netdev List
 help / color / mirror / Atom feed
* [PATCH 4/4] ARM:FlexCAN Controller for platform_ type
From: Bhaskar Upadhaya @ 2011-08-08 15:01 UTC (permalink / raw)
  To: netdev, mkl, holt, wg, davem, linuxppc-release, b22300,
	<socketcan-
  Cc: Bhaskar Upadhaya

Rearrange the existing ARM based FlexCAN implementation, so as to
support powerpc based FlexCAN on P1010.
Signed-off-by: Bhaskar Upadhaya <bhaskar.upadhaya@freescale.com>
---
 Based on http://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
 Branch master

 drivers/net/can/flexcan.c       |  105 ++++++++++++++-------------------------
 drivers/net/can/flexcan_iface.c |   99 ++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+), 68 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 1c1af24..b4d9afb 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -993,37 +993,29 @@ void __devexit unregister_flexcandev(struct net_device *dev)
 	unregister_candev(dev);
 }
 
-static int __devinit flexcan_probe(struct platform_device *pdev)
+int flexcan_dev_init(struct device *pdev, struct flexcan_resource flexcan_res,
+		 struct flexcan_interface *flexcan_ops)
 {
 	struct net_device *dev;
 	struct flexcan_priv *priv;
-	struct resource *mem;
 	struct clk *clk;
 	void __iomem *base;
-	resource_size_t mem_size;
-	int err, irq;
+	int err;
 
-	clk = clk_get(&pdev->dev, NULL);
+	clk = flexcan_ops->clk_get(pdev, NULL);
 	if (IS_ERR(clk)) {
-		dev_err(&pdev->dev, "no clock defined\n");
+		dev_err(pdev, "no clock defined\n");
 		err = PTR_ERR(clk);
 		goto failed_clock;
 	}
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	irq = platform_get_irq(pdev, 0);
-	if (!mem || irq <= 0) {
-		err = -ENODEV;
-		goto failed_get;
-	}
-
-	mem_size = resource_size(mem);
-	if (!request_mem_region(mem->start, mem_size, pdev->name)) {
+	if (!request_mem_region
+	    (flexcan_res.addr, flexcan_res.size, flexcan_res.drv_name)) {
 		err = -EBUSY;
-		goto failed_get;
+		goto failed_req;
 	}
 
-	base = ioremap(mem->start, mem_size);
+	base = ioremap(flexcan_res.addr, flexcan_res.size);
 	if (!base) {
 		err = -ENOMEM;
 		goto failed_map;
@@ -1036,11 +1028,11 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	}
 
 	dev->netdev_ops = &flexcan_netdev_ops;
-	dev->irq = irq;
+	dev->irq = flexcan_res.irq;
 	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
 
 	priv = netdev_priv(dev);
-	priv->can.clock.freq = clk_get_rate(clk);
+	priv->can.clock.freq = flexcan_ops->clk_get_rate(clk);
 	priv->can.bittiming_const = &flexcan_bittiming_const;
 	priv->can.do_set_mode = flexcan_set_mode;
 	priv->can.do_get_berr_counter = flexcan_get_berr_counter;
@@ -1050,20 +1042,21 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	priv->base = base;
 	priv->dev = dev;
 	priv->clk = clk;
-	priv->pdata = pdev->dev.platform_data;
+	priv->pdata = pdev->platform_data;
+	priv->flexcan_ops = flexcan_ops;
 
 	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);
 
-	dev_set_drvdata(&pdev->dev, dev);
-	SET_NETDEV_DEV(dev, &pdev->dev);
+	dev_set_drvdata(pdev, dev);
+	SET_NETDEV_DEV(dev, pdev);
 
 	err = register_flexcandev(dev);
 	if (err) {
-		dev_err(&pdev->dev, "registering netdev failed\n");
+		dev_err(pdev, "registering netdev failed\n");
 		goto failed_register;
 	}
 
-	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
+	dev_info(pdev, "device registered (reg_base=%p, irq=%d)\n",
 		 priv->base, dev->irq);
 
 	return 0;
@@ -1073,55 +1066,31 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
  failed_alloc:
 	iounmap(base);
  failed_map:
-	release_mem_region(mem->start, mem_size);
- failed_get:
+	release_mem_region(flexcan_res.addr, flexcan_res.size);
+ failed_req:
 	clk_put(clk);
  failed_clock:
 	return err;
 }
 
-static int __devexit flexcan_remove(struct platform_device *pdev)
+void flexcan_reg_dump(struct net_device *dev)
 {
-	struct net_device *dev = platform_get_drvdata(pdev);
-	struct flexcan_priv *priv = netdev_priv(dev);
-	struct resource *mem;
-
-	unregister_flexcandev(dev);
-	platform_set_drvdata(pdev, NULL);
-	iounmap(priv->base);
-
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(mem->start, resource_size(mem));
-
-	clk_put(priv->clk);
-
-	free_candev(dev);
-
-	return 0;
-}
-
-static struct platform_driver flexcan_driver = {
-	.driver.name = DRV_NAME,
-	.probe = flexcan_probe,
-	.remove = __devexit_p(flexcan_remove),
-};
-
-static int __init flexcan_init(void)
-{
-	pr_info("%s netdevice driver\n", DRV_NAME);
-	return platform_driver_register(&flexcan_driver);
-}
+	const struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_regs __iomem *regs = priv->base;
 
-static void __exit flexcan_exit(void)
-{
-	platform_driver_unregister(&flexcan_driver);
-	pr_info("%s: driver removed\n", DRV_NAME);
+	dev_dbg(dev->dev.parent, "can-mcr 0x%x \r\n can-ctrl 0x%x \r\n"
+			"can-ecr 0x%x \r\n can-esr 0x%x \r\n"
+			"can-rxgmask 0x%x \r\n can-rx14mask 0x%x \r\n"
+			"can-rx15mask 0x%x \r\n can-imask1 0x%x \r\n"
+			"can-iflag1 0x%x \r\n"
+			"in  func <%s> line <%d> \r\n",
+			flexcan_read(&regs->mcr),
+			flexcan_read(&regs->ctrl),
+			flexcan_read(&regs->ecr),
+			flexcan_read(&regs->esr),
+			flexcan_read(&regs->rxgmask),
+			flexcan_read(&regs->rx14mask),
+			flexcan_read(&regs->rx15mask),
+			flexcan_read(&regs->imask1),
+			flexcan_read(&regs->iflag1), __func__, __LINE__);
 }
-
-module_init(flexcan_init);
-module_exit(flexcan_exit);
-
-MODULE_AUTHOR("Sascha Hauer <kernel@pengutronix.de>, "
-	      "Marc Kleine-Budde <kernel@pengutronix.de>");
-MODULE_LICENSE("GPL v2");
-MODULE_DESCRIPTION("CAN port driver for flexcan based chip");
diff --git a/drivers/net/can/flexcan_iface.c b/drivers/net/can/flexcan_iface.c
index 0c5f6dd..faa6c07 100644
--- a/drivers/net/can/flexcan_iface.c
+++ b/drivers/net/can/flexcan_iface.c
@@ -180,7 +180,55 @@ failed_req:
 	return err;
 }
 
+/**
+ * flexcan_plt_resource_init - initialize the resources for
+ *				"platform" type architecture like "ARM"
+ * @flexcan_res: input buffer filled with address for accessing h/w registers
+ *		of CAN
+ * @pdev: the CAN device to be used
+ * @flexcan_ops: input buffer containing different utility functions
+ *
+ * fills the flexcan_res with the address detail
+ * for accessing the h/w registers of FlexCAN block.
+ * flexcan_ops is filled with different clock functions and normal read/write
+ *
+ * Return value
+ *    - On Success
+ *	       0
+ *    - On Failure
+ *	   error value
+ */
+static int flexcan_plt_resource_init(struct flexcan_resource *flexcan_res,
+				 struct platform_device *pdev,
+				 struct flexcan_interface *flexcan_ops)
+{
+	int err, irq;
+	resource_size_t mem_size;
+	struct resource *mem;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+	if (!mem || irq <= 0) {
+		dev_err(&pdev->dev, "Cannot map to irq\n");
+		err = -ENODEV;
+		goto failed_get;
+	}
+
+	mem_size = resource_size(mem);
+	flexcan_res->addr = mem->start;
+	flexcan_res->size = mem_size;
+	flexcan_res->drv_name = pdev->name;
+
+	flexcan_ops->clk_enable = clk_enable;
+	flexcan_ops->clk_disable = clk_disable;
+	flexcan_ops->clk_get_rate = clk_get_rate;
+	flexcan_ops->clk_get = clk_get;
+	flexcan_ops->clk_put = clk_put;
+	return 0;
 
+failed_get:
+	return err;
+}
 
 /**
  * flexcan_probe - performs the resource initialization
@@ -212,6 +260,15 @@ static int flexcan_probe(struct platform_device *pdev)
 			err = -EINVAL;
 			goto failed_req;
 		}
+	} else {
+		err = flexcan_plt_resource_init(&flexcan_res, pdev,
+						&flexcan_ops);
+		if (err) {
+			dev_err(&pdev->dev, "Flexcan Initialization"
+				 "failed with err (%d)\n", err);
+			err = -EINVAL;
+			goto failed_req;
+		}
 	}
 
 	err = flexcan_dev_init(&pdev->dev, flexcan_res, &flexcan_ops);
@@ -251,6 +308,9 @@ static int flexcan_remove(struct platform_device *pdev)
 		addr = of_translate_address(pdev->dev.of_node,
 		    of_get_address(pdev->dev.of_node, 0, &size, NULL));
 		release_mem_region(addr, size);
+	} else {
+		mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		release_mem_region(addr, size);
 	}	clk_put(priv->clk);
 
 	platform_set_drvdata(pdev, NULL);
@@ -259,3 +319,42 @@ static int flexcan_remove(struct platform_device *pdev)
 	return 0;
 }
 
+
+static struct of_device_id flexcan_match[] = {
+	{
+	 .compatible = "fsl,flexcan-v1.0",
+	 },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, flexcan_match);
+
+static struct platform_driver flexcan_driver = {
+	.driver = {
+		.name = "DRV_NAME",
+		.owner = THIS_MODULE,
+#ifdef CONFIG_OF
+		.of_match_table = flexcan_match,
+#endif
+	},
+	.probe = flexcan_probe,
+	.remove = flexcan_remove,
+};
+
+static int __init flexcan_init(void)
+{
+	return platform_driver_register(&flexcan_driver);
+}
+
+static void __exit flexcan_exit(void)
+{
+	platform_driver_unregister(&flexcan_driver);
+}
+
+module_init(flexcan_init);
+module_exit(flexcan_exit);
+
+MODULE_AUTHOR("Sascha Hauer <kernel@pengutronix.de>, "
+		"Marc Kleine-Budde <kernel@pengutronix.de>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CAN port driver for flexcan based chip");
-- 
1.5.6.5



^ permalink raw reply related

* Re: [PATCH 1/4][upstream] powerpc/p1010: Rearrange header file for FlexCAN
From: Marc Kleine-Budde @ 2011-08-08 15:08 UTC (permalink / raw)
  To: Bhaskar Upadhaya
  Cc: netdev, holt, wg, davem, linuxppc-release, b22300, socketcan-core
In-Reply-To: <1312815586-25756-1-git-send-email-bhaskar.upadhaya@freescale.com>

[-- Attachment #1: Type: text/plain, Size: 3381 bytes --]

On 08/08/2011 04:59 PM, Bhaskar Upadhaya wrote:
> - Rearrange header file so that it can be used by both of_ type
>   and platform_ type architecture.
> - Provide a common read and write interface for of_ type and platform_
>   type architecture for accessing h/w registers.
> Signed-off-by: Bhaskar Upadhaya <bhaskar.upadhaya@freescale.com>

NACK - please have a look at Robin's patches. They are in a much better
shape.

cheers, Marc
> ---
>  Based on http://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
>  Branch master
> 
>  include/linux/can/platform/flexcan.h |   65 ++++++++++++++++++++++++++++++++++
>  1 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/can/platform/flexcan.h b/include/linux/can/platform/flexcan.h
> index 72b713a..8458a87 100644
> --- a/include/linux/can/platform/flexcan.h
> +++ b/include/linux/can/platform/flexcan.h
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (C) 2010 Marc Kleine-Budde <kernel@pengutronix.de>
> + * Copyright 2011 Freescale Semiconductor, Inc.
>   *
>   * This file is released under the GPLv2
>   *
> @@ -8,6 +9,27 @@
>  #ifndef __CAN_PLATFORM_FLEXCAN_H
>  #define __CAN_PLATFORM_FLEXCAN_H
>  
> +#include <linux/clk.h>
> +#include <linux/can/dev.h>
> +#include <linux/io.h>
> +
> +#ifdef CONFIG_OF
> +#include<linux/of_platform.h>
> +#else
> +#include <linux/platform_device.h>
> +#include <mach/clock.h>
> +#endif
> +
> +#define DRV_NAME "flexcan"
> +
> +#ifdef __BIG_ENDIAN
> +#define flexcan_read(x)	in_be32(x)
> +#define flexcan_write(x, y) out_be32(y, x)
> +#else
> +#define flexcan_read(x)	readl(x)
> +#define flexcan_write(x, y) writel(x, y)
> +#endif
> +
>  /**
>   * struct flexcan_platform_data - flex CAN controller platform data
>   * @transceiver_enable:         - called to power on/off the transceiver
> @@ -17,4 +39,47 @@ struct flexcan_platform_data {
>  	void (*transceiver_switch)(int enable);
>  };
>  
> +struct flexcan_interface {
> +	int (*clk_enable) (struct clk *clk);
> +	void (*clk_disable) (struct clk *clk);
> +	void (*clk_put) (struct clk *clk);
> +	unsigned long (*clk_get_rate) (struct clk *clk);
> +	struct clk *(*clk_get) (struct device *dev, const char *id);
> +};
> +
> +struct flexcan_resource {
> +	u32 irq;
> +	u64 addr;
> +	u64 size;
> +	const char *drv_name;
> +};
> +
> +#ifdef CONFIG_OF
> +struct clk {
> +	unsigned long		rate;
> +	void			*data;
> +};
> +#endif
> +
> +struct flexcan_priv {
> +	struct can_priv can;
> +	struct net_device *dev;
> +	struct napi_struct napi;
> +
> +	void __iomem *base;
> +	u32 reg_esr;
> +	u32 reg_ctrl_default;
> +
> +	struct clk *clk;
> +	struct flexcan_interface *flexcan_ops;
> +	struct flexcan_platform_data *pdata;
> +};
> +
> +int flexcan_dev_init(struct device *pdev, struct flexcan_resource
> +			flexcan_res, struct flexcan_interface *flexcan_ops);
> +
> +void __devexit unregister_flexcandev(struct net_device *dev);
> +
> +void flexcan_reg_dump(struct net_device *dev);
> +
>  #endif /* __CAN_PLATFORM_FLEXCAN_H */


-- 
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Robin Holt @ 2011-08-08 15:09 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, U Bhaskar-B22300,
	Marc Kleine-Budde, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4E3FF9EA.6030601-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 04:44 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>
> >>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>> ...
> >>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>
> >>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>
> >>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>> "clock_freq" while
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>
> >>>> documents "clock-frequencies"... :-(.
> >>>
> >>> You answered a different question that I was asking.  I was asking if
> >>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>> as well.
> >>
> >> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>
> >> For the P1010 we can sinmply derive the clock frequency from
> >> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >> properties, etc. The clk implemetation might go into
> >>
> >>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>
> >> or
> >>
> >>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>
> >> And may depend on HAVE_CAN_FLEXCAN
> >>
> >> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >> you using?
> > 
> > I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> > we receive under NDA which introduces the P1010RDB board into the QorIQ
> > platform, and then work from there for the flexcan stuff.  That patch
> > introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> > that Kconfig bit, so I have tweaked it to be selected automatically
> > when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> > selection to determine is we are going to build the flexcan.c file.
> 
> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> we should do it differently for PowerPC. 
> 
> For mainline inclusion, you should provide your patches against the
> David Millers "net-next-2.6" tree, which already seems to have support
> for the P1010RDB:
> 
>   config P1010_RDB
>         bool "Freescale P1010RDB"
>         select DEFAULT_UIMAGE
>         help
>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> 
>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>           (DDR3/3L, SATA II, and PCI  Express).
> 
> 
> > Our contact with Freescale would prefer that I not post that patch until
> > we get the OK from freescale to do so since we received it under NDA.
> 
> I don't think we currently need it. I prefer dropping and cleaning up
> the device tree stuff as it is not needed for the P1010 anyway. If a
> new processor shows up with enhanced capabilities requiring
> configuration via device tree, we or somebody else can provide a patch.
> Marc, what do you think?

I will rebase shortly and provide a newer set of patches.

I do think powerpc does need the device tree support.  That is how the flexcan_probe
is getting called.  How would you suggest I do it otherwise?

Robin

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Marc Kleine-Budde @ 2011-08-08 15:14 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300
In-Reply-To: <4E3FF9EA.6030601-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>


[-- Attachment #1.1: Type: text/plain, Size: 4310 bytes --]

On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> On 08/08/2011 04:44 PM, Robin Holt wrote:
>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>
>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>> ...
>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>
>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>
>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>> "clock_freq" while
>>>>>
>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>
>>>>> documents "clock-frequencies"... :-(.
>>>>
>>>> You answered a different question that I was asking.  I was asking if
>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>> as well.
>>>
>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>
>>> For the P1010 we can sinmply derive the clock frequency from
>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>> properties, etc. The clk implemetation might go into
>>>
>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>
>>> or
>>>
>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>
>>> And may depend on HAVE_CAN_FLEXCAN
>>>
>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>> you using?
>>
>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>> platform, and then work from there for the flexcan stuff.  That patch
>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>> that Kconfig bit, so I have tweaked it to be selected automatically
>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>> selection to determine is we are going to build the flexcan.c file.
> 
> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> we should do it differently for PowerPC. 
> 
> For mainline inclusion, you should provide your patches against the
> David Millers "net-next-2.6" tree, which already seems to have support
> for the P1010RDB:
> 
>   config P1010_RDB
>         bool "Freescale P1010RDB"
>         select DEFAULT_UIMAGE
>         help
>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> 
>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>           (DDR3/3L, SATA II, and PCI  Express).
> 
> 
>> Our contact with Freescale would prefer that I not post that patch until
>> we get the OK from freescale to do so since we received it under NDA.
> 
> I don't think we currently need it. I prefer dropping and cleaning up
> the device tree stuff as it is not needed for the P1010 anyway. If a
> new processor shows up with enhanced capabilities requiring
> configuration via device tree, we or somebody else can provide a patch.
> Marc, what do you think?

ACK - The device tree bindings as in mainline's Documentation is a mess.
If the powerpc guys are happy with a clock interfaces based approach
somewhere in arch/ppc, I'm more than happy to remove:
- fsl,flexcan-clock-source (not implemented, even in the fsl driver)

- fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
- clock-frequency           /   a single clock-frequency attribute

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   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

^ permalink raw reply

* Re: [PATCH 4/4] ARM:FlexCAN Controller for platform_ type
From: Marc Kleine-Budde @ 2011-08-08 15:15 UTC (permalink / raw)
  To: Bhaskar Upadhaya
  Cc: netdev, holt, wg, davem, linuxppc-release, b22300, socketcan-core
In-Reply-To: <1312815660-25828-1-git-send-email-bhaskar.upadhaya@freescale.com>

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On 08/08/2011 05:01 PM, Bhaskar Upadhaya wrote:
> Rearrange the existing ARM based FlexCAN implementation, so as to
> support powerpc based FlexCAN on P1010.
> Signed-off-by: Bhaskar Upadhaya <bhaskar.upadhaya@freescale.com>

NACK - see Robin's patches

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply

* Re: [PATCH 10/12] headers, can: Add missing #include to <linux/can/bcm.h>
From: Oliver Hartkopp @ 2011-08-08 15:15 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Urs Thuermann
In-Reply-To: <1312809655.2591.1149.camel@deadeye>

On 08.08.2011 15:20, Ben Hutchings wrote:
> <linux/can/bcm.h> uses type canid_t, defined in <linux/can.h>.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>


Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks Ben!

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Wolfgang Grandegger @ 2011-08-08 15:16 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, U Bhaskar-B22300,
	Marc Kleine-Budde, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110808144808.GZ4926-sJ/iWh9BUns@public.gmane.org>

On 08/08/2011 04:48 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 04:21 PM, Robin Holt wrote:
...
>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> 
> My question remains "What should we be naming the device tree node in
> general.  Line 5 of the fsl-flexcan.txt file specifically calls the node
> "fsl,flexcan-v1.0"  In the .dts file the freescale patches introduces into
> the arch/powerpc portion of the kernel, they call it that same thing.

We should provide a patch removing that doc. The version suffix does not
follow the device tree convention. A proper compatibility string would be:

  "fsl,p1010-flexcan", "fsl,flexcan"

But as the Flexcan on the P1010 is not treated differently,
"fsl,flexcan" is just fine. Also, the v1.0 is only for the PowerPC SOCs
(ignoring ARM).

> Likewise, in the code already checked into uboot it is the same name.
> Whether it is needed or not for the clock frequency, it does need to
> be consistent between the .dts file and the driver for device discovery
> to work.

Yes, depending on what we decide we need to clean that up as well.

Wolfgang.

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Wolfgang Grandegger @ 2011-08-08 15:18 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Marc Kleine-Budde,
	U Bhaskar-B22300, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110808150925.GA4926-sJ/iWh9BUns@public.gmane.org>

On 08/08/2011 05:09 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>
>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>> ...
>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>
>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>
>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>> "clock_freq" while
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>
>>>>>> documents "clock-frequencies"... :-(.
>>>>>
>>>>> You answered a different question that I was asking.  I was asking if
>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>> as well.
>>>>
>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>
>>>> For the P1010 we can sinmply derive the clock frequency from
>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>> properties, etc. The clk implemetation might go into
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>
>>>> or
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>
>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>
>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>> you using?
>>>
>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>> platform, and then work from there for the flexcan stuff.  That patch
>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>> selection to determine is we are going to build the flexcan.c file.
>>
>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>> we should do it differently for PowerPC. 
>>
>> For mainline inclusion, you should provide your patches against the
>> David Millers "net-next-2.6" tree, which already seems to have support
>> for the P1010RDB:
>>
>>   config P1010_RDB
>>         bool "Freescale P1010RDB"
>>         select DEFAULT_UIMAGE
>>         help
>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>
>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>           (DDR3/3L, SATA II, and PCI  Express).
>>
>>
>>> Our contact with Freescale would prefer that I not post that patch until
>>> we get the OK from freescale to do so since we received it under NDA.
>>
>> I don't think we currently need it. I prefer dropping and cleaning up
>> the device tree stuff as it is not needed for the P1010 anyway. If a
>> new processor shows up with enhanced capabilities requiring
>> configuration via device tree, we or somebody else can provide a patch.
>> Marc, what do you think?
> 
> I will rebase shortly and provide a newer set of patches.
> 
> I do think powerpc does need the device tree support.  That is how the flexcan_probe
> is getting called.  How would you suggest I do it otherwise?

Why do you think that?

Wolfgang.

^ permalink raw reply

* Re: [PATCH 3/4] powerpc/p1010: FlexCAN Controller for of_ type
From: Marc Kleine-Budde @ 2011-08-08 15:21 UTC (permalink / raw)
  To: Bhaskar Upadhaya
  Cc: netdev, holt, wg, davem, linuxppc-release, b22300, socketcan-core
In-Reply-To: <1312815640-25804-1-git-send-email-bhaskar.upadhaya@freescale.com>

[-- Attachment #1: Type: text/plain, Size: 26219 bytes --]

On 08/08/2011 05:00 PM, Bhaskar Upadhaya wrote:
> Provide FlexCAN support for Freescale P1010 SoC.
> Modify the existing FlexCAN, so as to support the of_type approach on
> P1010(power architecture based)SoC.
> 
> FlexCAN is a communication controller implementing the CAN protocol according
> to the CAN 2.0B protocol specification.
> This controller is available on Freescale P1010 platform.
> Signed-off-by: Bhaskar Upadhaya <bhaskar.upadhaya@freescale.com>

NACK - your patch does more than the description states (debug code).
Further you still add bugs to the driver. I've send you patches to fix them.

Marc

> ---
> Based on git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
>  Branch master
> 
>  drivers/net/can/Kconfig         |    8 +-
>  drivers/net/can/Makefile        |    4 +-
>  drivers/net/can/flexcan.c       |  162 ++++++++++++------------
>  drivers/net/can/flexcan_iface.c |  261 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 349 insertions(+), 86 deletions(-)
>  create mode 100644 drivers/net/can/flexcan_iface.c
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index f6c98fb..882da54 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -98,9 +98,12 @@ config HAVE_CAN_FLEXCAN
>  
>  config CAN_FLEXCAN
>  	tristate "Support for Freescale FLEXCAN based chips"
> -	depends on CAN_DEV && HAVE_CAN_FLEXCAN
> +	depends on CAN_DEV && (!ARM || HAVE_CAN_FLEXCAN)
> +	select PPC_CLOCK
>  	---help---
> -	  Say Y here if you want to support for Freescale FlexCAN.
> +	  Say Y here if you want support for Freescale FlexCAN.
> +	  Flexcan Module is implementing the CAN Protocol
> +	  version 2.0
>  
>  config PCH_CAN
>  	tristate "PCH CAN"
> @@ -123,6 +126,7 @@ source "drivers/net/can/softing/Kconfig"
>  config CAN_DEBUG_DEVICES
>  	bool "CAN devices debugging messages"
>  	depends on CAN
> +	default N
>  	---help---
>  	  Say Y here if you want the CAN device drivers to produce a bunch of
>  	  debug messages to the system log.  Select this if you are having
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 24ebfe8..4965e6f 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -19,7 +19,9 @@ obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
>  obj-$(CONFIG_CAN_MCP251X)	+= mcp251x.o
>  obj-$(CONFIG_CAN_BFIN)		+= bfin_can.o
>  obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
> -obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
> +obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan_driver.o
> +flexcan_driver-objs := flexcan.o \
> +		flexcan_iface.o
>  obj-$(CONFIG_PCH_CAN)		+= pch_can.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index a24aa12..1c1af24 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -22,10 +22,8 @@
>  
>  #include <linux/netdevice.h>
>  #include <linux/can.h>
> -#include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/platform/flexcan.h>
> -#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/if_arp.h>
>  #include <linux/if_ether.h>
> @@ -34,11 +32,6 @@
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> -#include <linux/platform_device.h>
> -
> -#include <mach/clock.h>
> -
> -#define DRV_NAME			"flexcan"
>  
>  /* 8 for RX fifo and 2 error handling */
>  #define FLEXCAN_NAPI_WEIGHT		(8 + 2)
> @@ -167,19 +160,6 @@ struct flexcan_regs {
>  	struct flexcan_mb cantxfg[64];
>  };
>  
> -struct flexcan_priv {
> -	struct can_priv can;
> -	struct net_device *dev;
> -	struct napi_struct napi;
> -
> -	void __iomem *base;
> -	u32 reg_esr;
> -	u32 reg_ctrl_default;
> -
> -	struct clk *clk;
> -	struct flexcan_platform_data *pdata;
> -};
> -
>  static struct can_bittiming_const flexcan_bittiming_const = {
>  	.name = DRV_NAME,
>  	.tseg1_min = 4,
> @@ -229,9 +209,10 @@ static inline void flexcan_chip_enable(struct flexcan_priv *priv)
>  	struct flexcan_regs __iomem *regs = priv->base;
>  	u32 reg;
>  
> -	reg = readl(&regs->mcr);
> +	reg = flexcan_read(&regs->mcr);
> +
>  	reg &= ~FLEXCAN_MCR_MDIS;
> -	writel(reg, &regs->mcr);
> +	flexcan_write(reg, &regs->mcr);
>  
>  	udelay(10);
>  }
> @@ -248,9 +229,10 @@ static inline void flexcan_chip_disable(struct flexcan_priv *priv)
>  	struct flexcan_regs __iomem *regs = priv->base;
>  	u32 reg;
>  
> -	reg = readl(&regs->mcr);
> +	reg = flexcan_read(&regs->mcr);
> +
>  	reg |= FLEXCAN_MCR_MDIS;
> -	writel(reg, &regs->mcr);
> +	flexcan_write(reg, &regs->mcr);
>  }
>  
>  /**
> @@ -266,9 +248,9 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
>  {
>  	const struct flexcan_priv *priv = netdev_priv(dev);
>  	struct flexcan_regs __iomem *regs = priv->base;
> -	u32 reg = readl(&regs->ecr);
>  
> -	bec->txerr = (reg >> 0) & 0xff;
> +	u32 reg = flexcan_read(&regs->ecr);
> +	bec->txerr = reg & 0xff;
>  	bec->rxerr = (reg >> 8) & 0xff;
>  
>  	return 0;
> @@ -294,6 +276,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	u32 can_id;
>  	u32 ctrl = FLEXCAN_MB_CNT_CODE(0xc) | (cf->can_dlc << 16);
>  
> +	flexcan_reg_dump(dev);
>  	if (can_dropped_invalid_skb(dev, skb))
>  		return NETDEV_TX_OK;
>  
> @@ -311,21 +294,24 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	if (cf->can_dlc > 0) {
>  		u32 data = be32_to_cpup((__be32 *)&cf->data[0]);
> -		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
> +		flexcan_write(data,
> +			&regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
>  	}
>  	if (cf->can_dlc > 3) {
>  		u32 data = be32_to_cpup((__be32 *)&cf->data[4]);
> -		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
> +		flexcan_write(data,
> +			&regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
>  	}
>  
> -	writel(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
> -	writel(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
> +	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
> +	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
>  
>  	kfree_skb(skb);
>  
>  	/* tx_packets is incremented in flexcan_irq */
>  	stats->tx_bytes += cf->can_dlc;
>  
> +	flexcan_reg_dump(dev);
>  	return NETDEV_TX_OK;
>  }
>  
> @@ -440,7 +426,8 @@ static void do_state(struct net_device *dev,
>  				CAN_ERR_CRTL_TX_WARNING :
>  				CAN_ERR_CRTL_RX_WARNING;
>  		}
> -	case CAN_STATE_ERROR_WARNING:	/* fallthrough */
> +		/* fallthrough */
> +	case CAN_STATE_ERROR_WARNING:
>  		/*
>  		 * from: ERROR_ACTIVE, ERROR_WARNING
>  		 * to  : ERROR_PASSIVE, BUS_OFF
> @@ -536,8 +523,8 @@ static void flexcan_read_fifo(const struct net_device *dev,
>  	struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
>  	u32 reg_ctrl, reg_id;
>  
> -	reg_ctrl = readl(&mb->can_ctrl);
> -	reg_id = readl(&mb->can_id);
> +	reg_ctrl = flexcan_read(&mb->can_ctrl);
> +	reg_id = flexcan_read(&mb->can_id);
>  	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
>  		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
>  	else
> @@ -547,12 +534,13 @@ static void flexcan_read_fifo(const struct net_device *dev,
>  		cf->can_id |= CAN_RTR_FLAG;
>  	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
>  
> -	*(__be32 *)(cf->data + 0) = cpu_to_be32(readl(&mb->data[0]));
> -	*(__be32 *)(cf->data + 4) = cpu_to_be32(readl(&mb->data[1]));
> +	*(__be32 *) (&cf->data[0]) =
> +	    cpu_to_be32(flexcan_read(&mb->data[0]));
> +	*(__be32 *) (&cf->data[4]) =
> +	    cpu_to_be32(flexcan_read(&mb->data[1]));
>  
>  	/* mark as read */
> -	writel(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
> -	readl(&regs->timer);
> +	flexcan_write(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
>  }
>  
>  static int flexcan_read_frame(struct net_device *dev)
> @@ -596,17 +584,17 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
>  	 * The error bits are cleared on read,
>  	 * use saved value from irq handler.
>  	 */
> -	reg_esr = readl(&regs->esr) | priv->reg_esr;
> +	reg_esr = flexcan_read(&regs->esr) | priv->reg_esr;
>  
>  	/* handle state changes */
>  	work_done += flexcan_poll_state(dev, reg_esr);
>  
>  	/* handle RX-FIFO */
> -	reg_iflag1 = readl(&regs->iflag1);
> -	while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
> -	       work_done < quota) {
> +	reg_iflag1 = flexcan_read(&regs->iflag1);
> +	while ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) &&
> +	       (work_done < quota)) {
>  		work_done += flexcan_read_frame(dev);
> -		reg_iflag1 = readl(&regs->iflag1);
> +		reg_iflag1 = flexcan_read(&regs->iflag1);
>  	}
>  
>  	/* report bus errors */
> @@ -616,8 +604,8 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
>  	if (work_done < quota) {
>  		napi_complete(napi);
>  		/* enable IRQs */
> -		writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
> -		writel(priv->reg_ctrl_default, &regs->ctrl);
> +		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
> +		flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
>  	}
>  
>  	return work_done;
> @@ -641,9 +629,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	struct flexcan_regs __iomem *regs = priv->base;
>  	u32 reg_iflag1, reg_esr;
>  
> -	reg_iflag1 = readl(&regs->iflag1);
> -	reg_esr = readl(&regs->esr);
> -	writel(FLEXCAN_ESR_ERR_INT, &regs->esr);	/* ACK err IRQ */
> +	flexcan_reg_dump(dev);
> +	reg_iflag1 = flexcan_read(&regs->iflag1);
> +	reg_esr = flexcan_read(&regs->esr);
> +	flexcan_write(FLEXCAN_ESR_ERR_INT, &regs->esr);/* ACK err IRQ */
>  
>  	/*
>  	 * schedule NAPI in case of:
> @@ -659,16 +648,17 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  		 * save them for later use.
>  		 */
>  		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
> -		writel(FLEXCAN_IFLAG_DEFAULT & ~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE,
> -		       &regs->imask1);
> -		writel(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
> -		       &regs->ctrl);
> +		flexcan_write(FLEXCAN_IFLAG_DEFAULT &
> +			~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->imask1);
> +		flexcan_write(priv->reg_ctrl_default &
> +			~FLEXCAN_CTRL_ERR_ALL, &regs->ctrl);
>  		napi_schedule(&priv->napi);
>  	}
>  
>  	/* FIFO overflow */
>  	if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
> -		writel(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
> +		flexcan_write(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW,
> +			&regs->iflag1);
>  		dev->stats.rx_over_errors++;
>  		dev->stats.rx_errors++;
>  	}
> @@ -677,10 +667,11 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
>  		/* tx_bytes is incremented in flexcan_start_xmit */
>  		stats->tx_packets++;
> -		writel((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
> +		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
>  		netif_wake_queue(dev);
>  	}
>  
> +	flexcan_reg_dump(dev);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -698,7 +689,7 @@ static void flexcan_set_bittiming(struct net_device *dev)
>  	struct flexcan_regs __iomem *regs = priv->base;
>  	u32 reg;
>  
> -	reg = readl(&regs->ctrl);
> +	reg = flexcan_read(&regs->ctrl);
>  	reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
>  		 FLEXCAN_CTRL_RJW(0x3) |
>  		 FLEXCAN_CTRL_PSEG1(0x7) |
> @@ -722,11 +713,11 @@ static void flexcan_set_bittiming(struct net_device *dev)
>  		reg |= FLEXCAN_CTRL_SMP;
>  
>  	dev_info(dev->dev.parent, "writing ctrl=0x%08x\n", reg);
> -	writel(reg, &regs->ctrl);
> +	flexcan_write(reg, &regs->ctrl);
>  
>  	/* print chip status */
>  	dev_dbg(dev->dev.parent, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
> -		readl(&regs->mcr), readl(&regs->ctrl));
> +		flexcan_read(&regs->mcr), flexcan_read(&regs->ctrl));
>  }
>  
>  /**
> @@ -751,10 +742,10 @@ static int flexcan_chip_start(struct net_device *dev)
>  	flexcan_chip_enable(priv);
>  
>  	/* soft reset */
> -	writel(FLEXCAN_MCR_SOFTRST, &regs->mcr);
> +	flexcan_write(FLEXCAN_MCR_SOFTRST, &regs->mcr);
>  	udelay(10);
>  
> -	reg_mcr = readl(&regs->mcr);
> +	reg_mcr = flexcan_read(&regs->mcr);
>  	if (reg_mcr & FLEXCAN_MCR_SOFTRST) {
>  		dev_err(dev->dev.parent,
>  			"Failed to softreset can module (mcr=0x%08x)\n",
> @@ -776,12 +767,12 @@ static int flexcan_chip_start(struct net_device *dev)
>  	 * choose format C
>  	 *
>  	 */
> -	reg_mcr = readl(&regs->mcr);
> +	reg_mcr = flexcan_read(&regs->mcr);
>  	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
>  		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
>  		FLEXCAN_MCR_IDAM_C;
>  	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
> -	writel(reg_mcr, &regs->mcr);
> +	flexcan_write(reg_mcr, &regs->mcr);
>  
>  	/*
>  	 * CTRL
> @@ -799,7 +790,7 @@ static int flexcan_chip_start(struct net_device *dev)
>  	 * (FLEXCAN_CTRL_ERR_MSK), too. Otherwise we don't get any
>  	 * warning or bus passive interrupts.
>  	 */
> -	reg_ctrl = readl(&regs->ctrl);
> +	reg_ctrl = flexcan_read(&regs->ctrl);
>  	reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
>  	reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
>  		FLEXCAN_CTRL_ERR_STATE | FLEXCAN_CTRL_ERR_MSK;
> @@ -807,38 +798,40 @@ static int flexcan_chip_start(struct net_device *dev)
>  	/* save for later use */
>  	priv->reg_ctrl_default = reg_ctrl;
>  	dev_dbg(dev->dev.parent, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
> -	writel(reg_ctrl, &regs->ctrl);
> +	flexcan_write(reg_ctrl, &regs->ctrl);
>  
>  	for (i = 0; i < ARRAY_SIZE(regs->cantxfg); i++) {
> -		writel(0, &regs->cantxfg[i].can_ctrl);
> -		writel(0, &regs->cantxfg[i].can_id);
> -		writel(0, &regs->cantxfg[i].data[0]);
> -		writel(0, &regs->cantxfg[i].data[1]);
> +		flexcan_write(0, &regs->cantxfg[i].can_ctrl);
> +		flexcan_write(0, &regs->cantxfg[i].can_id);
> +		flexcan_write(0, &regs->cantxfg[i].data[0]);
> +		flexcan_write(0, &regs->cantxfg[i].data[1]);
>  
>  		/* put MB into rx queue */
> -		writel(FLEXCAN_MB_CNT_CODE(0x4), &regs->cantxfg[i].can_ctrl);
> +		flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
> +			&regs->cantxfg[i].can_ctrl);
>  	}
>  
>  	/* acceptance mask/acceptance code (accept everything) */
> -	writel(0x0, &regs->rxgmask);
> -	writel(0x0, &regs->rx14mask);
> -	writel(0x0, &regs->rx15mask);
> +	flexcan_write(0x0, &regs->rxgmask);
> +	flexcan_write(0x0, &regs->rx14mask);
> +	flexcan_write(0x0, &regs->rx15mask);
>  
>  	flexcan_transceiver_switch(priv, 1);
>  
>  	/* synchronize with the can bus */
> -	reg_mcr = readl(&regs->mcr);
> +	reg_mcr = flexcan_read(&regs->mcr);
>  	reg_mcr &= ~FLEXCAN_MCR_HALT;
> -	writel(reg_mcr, &regs->mcr);
> +	flexcan_write(reg_mcr, &regs->mcr);
>  
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
>  	/* enable FIFO interrupts */
> -	writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
> +	flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
>  
>  	/* print chip status */
>  	dev_dbg(dev->dev.parent, "%s: reading mcr=0x%08x ctrl=0x%08x\n",
> -		__func__, readl(&regs->mcr), readl(&regs->ctrl));
> +		__func__, flexcan_read(&regs->mcr),
> +			flexcan_read(&regs->ctrl));
>  
>  	return 0;
>  
> @@ -860,12 +853,12 @@ static void flexcan_chip_stop(struct net_device *dev)
>  	u32 reg;
>  
>  	/* Disable all interrupts */
> -	writel(0, &regs->imask1);
> +	flexcan_write(0, &regs->imask1);
>  
>  	/* Disable + halt module */
> -	reg = readl(&regs->mcr);
> +	reg = flexcan_read(&regs->mcr);
>  	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
> -	writel(reg, &regs->mcr);
> +	flexcan_write(reg, &regs->mcr);
>  
>  	flexcan_transceiver_switch(priv, 0);
>  	priv->can.state = CAN_STATE_STOPPED;
> @@ -935,6 +928,8 @@ static int flexcan_set_mode(struct net_device *dev, enum can_mode mode)
>  		break;
>  
>  	default:
> +		dev_dbg(dev->dev.parent, "Setting flexcan mode(%d) in func %s in line"
> +					"%d \r\n", mode, __func__, __LINE__);
>  		return -EOPNOTSUPP;
>  	}
>  
> @@ -957,24 +952,24 @@ static int __devinit register_flexcandev(struct net_device *dev)
>  
>  	/* select "bus clock", chip must be disabled */
>  	flexcan_chip_disable(priv);
> -	reg = readl(&regs->ctrl);
> +	reg = flexcan_read(&regs->ctrl);
>  	reg |= FLEXCAN_CTRL_CLK_SRC;
> -	writel(reg, &regs->ctrl);
> +	flexcan_write(reg, &regs->ctrl);
>  
>  	flexcan_chip_enable(priv);
>  
>  	/* set freeze, halt and activate FIFO, restrict register access */
> -	reg = readl(&regs->mcr);
> +	reg = flexcan_read(&regs->mcr);
>  	reg |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
>  		FLEXCAN_MCR_FEN | FLEXCAN_MCR_SUPV;
> -	writel(reg, &regs->mcr);
> +	flexcan_write(reg, &regs->mcr);
>  
>  	/*
>  	 * Currently we only support newer versions of this core
>  	 * featuring a RX FIFO. Older cores found on some Coldfire
>  	 * derivates are not yet supported.
>  	 */
> -	reg = readl(&regs->mcr);
> +	reg = flexcan_read(&regs->mcr);
>  	if (!(reg & FLEXCAN_MCR_FEN)) {
>  		dev_err(dev->dev.parent,
>  			"Could not enable RX FIFO, unsupported core\n");
> @@ -984,6 +979,7 @@ static int __devinit register_flexcandev(struct net_device *dev)
>  
>  	err = register_candev(dev);
>  
> +	return err;

If you return here, the clock stays enabled....not good

>   out:
>  	/* disable core and turn off clocks */
>  	flexcan_chip_disable(priv);
> @@ -992,7 +988,7 @@ static int __devinit register_flexcandev(struct net_device *dev)
>  	return err;
>  }
>  
> -static void __devexit unregister_flexcandev(struct net_device *dev)
> +void __devexit unregister_flexcandev(struct net_device *dev)
>  {
>  	unregister_candev(dev);
>  }
> diff --git a/drivers/net/can/flexcan_iface.c b/drivers/net/can/flexcan_iface.c
> new file mode 100644
> index 0000000..0c5f6dd
> --- /dev/null
> +++ b/drivers/net/can/flexcan_iface.c
> @@ -0,0 +1,261 @@
> +/*
> + * flexcan_iface.c
> + *
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + *
> + * Based on code originally by Andrey Volkov <avolkov@varma-el.com>
> + *
> + * LICENCE:
> + * 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 version 2.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/can/platform/flexcan.h>
> +
> +struct flexcan_interface flexcan_ops;
> +
> +/**
> + * flexcan_of_get_clk_rate - returns the rate, used for bit-timing
> + *				calculations of FlexCAN
> + */
> +static unsigned long flexcan_of_get_clk_rate(struct clk *clock)
> +{
> +	return clock->rate;
> +}
> +
> +static void flexcan_of_clk_put(struct clk *clk)
> +{
> +	kfree(clk);
> +}
> +
> +/**
> + * flexcan_of_clk_get - calculates the rate, used for bit-timing
> + *			calculations of FlexCAN
> + * @dev: the FlexCAN device to be used
> + * @id: id used to differentiate among different clock nodes
> + *
> + * calculate the rate based on the clock-frequency
> + * and clock-divider values from device tree.
> + * It calculate the rate being "platform" as the clock source
> + * Framework for "oscillator" as clock source is also provided.
> + *
> + * Return value
> + *    - On Success
> + *         the rate as part of clk struct, used to calculate the bit-timing
> + *	   for FlexCAN
> + *    - On Failure
> + *	   error value
> + */
> +static struct clk *flexcan_of_clk_get(struct device *dev, const char *id)
> +{
> +	struct clk *clock;
> +	u32 *clock_freq = NULL;
> +	u32 *clock_divider = NULL;
> +	const char *clk_source;
> +	int err;
> +	unsigned long rate;
> +
> +	clk_source = (char *)of_get_property(dev->of_node,
> +			"fsl,flexcan-clock-source", NULL);
> +	if (clk_source == NULL) {
> +		dev_err(dev, "Cannot find fsl,flexcan-clock-source"
> +				"property\n");
> +		err = -EINVAL;
> +		goto failed_clock;
> +	}
> +	if (!memcmp(clk_source, "platform", strlen(clk_source))) {
> +		clock_divider = (u32 *)of_get_property(dev->of_node,
> +				"fsl,flexcan-clock-divider", NULL);
> +		if (*clock_divider) {
> +			clock_freq = (u32 *) of_get_property(dev->of_node,
> +					"clock-frequency", NULL);
> +			if (clock_freq == NULL) {
> +				dev_err(dev, "Cannot find clock-frequency"
> +							"property\n");
> +				err = -EINVAL;
> +				goto failed_clock;
> +			}
> +			rate = DIV_ROUND_CLOSEST(*clock_freq / *clock_divider,
> +						1000) * 1000;
> +		} else {
> +			dev_err(dev, "Cannot find valid fsl,"
> +					"flexcan-clock-divider\n");
> +			err = -EINVAL;
> +			goto failed_clock;
> +		}
> +	} else if (!memcmp(clk_source, "oscillator", strlen(clk_source))) {
> +		clock_divider = (u32 *)of_get_property(dev->of_node,
> +				"fsl,flexcan-clock-divider", NULL);
> +		clock_freq = (u32 *)of_get_property(dev->of_node,
> +						"clock-frequency", NULL);
> +		if (!(*clock_divider && *clock_freq)) {
> +			dev_err(dev, "Cannot find valid"
> +					"fsl,flexcan-clock-divider or"
> +					"clock-frequency\n");
> +			err = -EINVAL;
> +			goto failed_clock;
> +		} else { /*FIXME for keeping oscillator as clock-source*/
> +				dev_err(dev, "oscillator as clock support is"
> +						"not available\n");
> +				err = -EINVAL;
> +				goto failed_clock;
> +		}
> +	} else {
> +		dev_err(dev, "Invalid flexcan-clock-source\n");
> +		err = -EINVAL;
> +		goto failed_clock;
> +	}
> +
> +	clock = kmalloc(sizeof(struct clk), GFP_KERNEL);
> +	if (!clock) {
> +		dev_err(dev, "Cannot allocate memory\n");
> +			err = -ENOMEM;
> +		goto failed_clock;
> +	}
> +
> +	clock->rate = rate;
> +	dev_info(dev, "clock-frequency is  %lu in line %d in function %s\r\n",
> +			clock->rate, __LINE__, __func__);
> +	return clock;
> +
> + failed_clock:
> +	return ERR_PTR(err);
> +}
> +
> +/**
> + * flexcan_of_resource_init - initialize the resources for
> + *				"of" type platform like powerpc
> + * @flexcan_res: input buffer filled with address for accessing h/w registers
> + *		of FlexCAN
> + * @pdev: the FlexCAN device to be used
> + * @flexcan_ops: input buffer containing different utility functions
> + *
> + * fills the flexcan_res with the address detail
> + * for accessing the h/w registers of FlexCAN block.
> + * flexcan_ops is filled with different clock functions and normal read/write
> + *
> + * Return value
> + *    - On Success
> + *	       0
> + *    - On Failure
> + *	   error value
> + */
> +static int flexcan_of_resource_init(struct flexcan_resource *flexcan_res,
> +					struct device *pdev,
> +					struct flexcan_interface *flexcan_ops)
> +{
> +	u64 addr, size;
> +	int err, irq;
> +
> +	addr = of_translate_address(pdev->of_node,
> +			of_get_address(pdev->of_node, 0, &size, NULL));
> +	flexcan_res->addr = addr;
> +	flexcan_res->size = size;
> +	flexcan_res->drv_name = pdev->driver->name;
> +	irq = irq_of_parse_and_map(pdev->of_node, 0);
> +	if (irq == NO_IRQ) {
> +		dev_err(pdev, "cannot map to irq\n");
> +		err = -EINVAL;
> +		goto failed_req;
> +	}
> +
> +	flexcan_res->irq = irq;
> +
> +	flexcan_ops->clk_enable = NULL;
> +	flexcan_ops->clk_disable = NULL;
> +	flexcan_ops->clk_get_rate = flexcan_of_get_clk_rate;
> +	flexcan_ops->clk_get = flexcan_of_clk_get;
> +	flexcan_ops->clk_put = flexcan_of_clk_put;
> +	return 0;
> +
> +failed_req:
> +	return err;
> +}
> +
> +
> +
> +/**
> + * flexcan_probe - performs the resource initialization
> + *		   after detecting the architecture type like "of" or
> + *		   "platform" type
> + * @pdev: pointer to platform device
> + *
> + * initialises the resources based on "platform" or "of"
> + * type architecture.It also registers the FlexCAN with netdev layer.
> + *
> + * Return value
> + *    - On Success
> + *	       0
> + *    - On Failure
> + *	   error value
> + */
> +static int flexcan_probe(struct platform_device *pdev)
> +{
> +	int err;
> +	struct flexcan_resource flexcan_res;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (np) {
> +		err = flexcan_of_resource_init(&flexcan_res,
> +					&pdev->dev, &flexcan_ops);
> +		if (err) {
> +			dev_err(&pdev->dev, "Flexcan Initialization"
> +				 "failed with err (%d)\n", err);
> +			err = -EINVAL;
> +			goto failed_req;
> +		}
> +	}
> +
> +	err = flexcan_dev_init(&pdev->dev, flexcan_res, &flexcan_ops);
> +	if (err) {
> +		dev_err(&pdev->dev, "Flexcan Initialization failed with err"
> +				"(%d)\n", err);
> +		err = -EINVAL;
> +		goto failed_req;
> +	}
> +
> +	return 0;
> + failed_req:
> +	return err;
> +}
> +
> +/**
> + * flexcan_remove - performs the resource de-initialization
> + *		    after detecting the architecture type like "of" or
> + *		    "platform" type
> + * @pdev: pointer to platform device
> + *
> + * de-initializez the resources based on "platform" or "of"
> + * type architecture.It also unregister the FlexCAN with netdev layer.
> + */
> +static int flexcan_remove(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *mem;
> +	u64 addr = 0, size;
> +
> +	unregister_flexcandev(dev);
> +	iounmap(priv->base);
> +
> +	if (np) {
> +		addr = of_translate_address(pdev->dev.of_node,
> +		    of_get_address(pdev->dev.of_node, 0, &size, NULL));
> +		release_mem_region(addr, size);
> +	}	clk_put(priv->clk);
> +
> +	platform_set_drvdata(pdev, NULL);
> +	free_candev(dev);
> +
> +	return 0;
> +}
> +


-- 
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Wolfgang Grandegger @ 2011-08-08 15:22 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, U Bhaskar-B22300,
	Marc Kleine-Budde, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4E3FFE61.4090109-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
> On 08/08/2011 05:09 PM, Robin Holt wrote:
>> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>
>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>> ...
>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>
>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>
>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>> "clock_freq" while
>>>>>>>
>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>
>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>
>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>> as well.
>>>>>
>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>
>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>> properties, etc. The clk implemetation might go into
>>>>>
>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>
>>>>> or
>>>>>
>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>
>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>
>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>> you using?
>>>>
>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>> selection to determine is we are going to build the flexcan.c file.
>>>
>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>> we should do it differently for PowerPC. 
>>>
>>> For mainline inclusion, you should provide your patches against the
>>> David Millers "net-next-2.6" tree, which already seems to have support
>>> for the P1010RDB:
>>>
>>>   config P1010_RDB
>>>         bool "Freescale P1010RDB"
>>>         select DEFAULT_UIMAGE
>>>         help
>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>
>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>
>>>
>>>> Our contact with Freescale would prefer that I not post that patch until
>>>> we get the OK from freescale to do so since we received it under NDA.
>>>
>>> I don't think we currently need it. I prefer dropping and cleaning up
>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>> new processor shows up with enhanced capabilities requiring
>>> configuration via device tree, we or somebody else can provide a patch.
>>> Marc, what do you think?
>>
>> I will rebase shortly and provide a newer set of patches.
>>
>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>> is getting called.  How would you suggest I do it otherwise?
> 
> Why do you think that?

To be clear. I mean we do not need the extra "fsl," properties for the
clock source and divider and frequency.

Wolfgang.

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Marc Kleine-Budde @ 2011-08-08 15:23 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300
In-Reply-To: <4E3FFE61.4090109-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>


[-- Attachment #1.1: Type: text/plain, Size: 1169 bytes --]

On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
>>>> Our contact with Freescale would prefer that I not post that patch until
>>>> we get the OK from freescale to do so since we received it under NDA.
>>>
>>> I don't think we currently need it. I prefer dropping and cleaning up
>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>> new processor shows up with enhanced capabilities requiring
>>> configuration via device tree, we or somebody else can provide a patch.
>>> Marc, what do you think?
>>
>> I will rebase shortly and provide a newer set of patches.
>>
>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>> is getting called.  How would you suggest I do it otherwise?

I think Wolfgang was talking about removing the clock* attributes from
the device tree, not the device tree bindings at all.

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   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Robin Holt @ 2011-08-08 15:25 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Marc Kleine-Budde,
	U Bhaskar-B22300, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4E3FFE61.4090109-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

On Mon, Aug 08, 2011 at 05:18:57PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:09 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>
> >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>> ...
> >>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>
> >>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>
> >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>> "clock_freq" while
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>
> >>>>>> documents "clock-frequencies"... :-(.
> >>>>>
> >>>>> You answered a different question that I was asking.  I was asking if
> >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>> as well.
> >>>>
> >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>
> >>>> For the P1010 we can sinmply derive the clock frequency from
> >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>> properties, etc. The clk implemetation might go into
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>
> >>>> or
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>
> >>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>
> >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>> you using?
> >>>
> >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>> platform, and then work from there for the flexcan stuff.  That patch
> >>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>> selection to determine is we are going to build the flexcan.c file.
> >>
> >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >> we should do it differently for PowerPC. 
> >>
> >> For mainline inclusion, you should provide your patches against the
> >> David Millers "net-next-2.6" tree, which already seems to have support
> >> for the P1010RDB:
> >>
> >>   config P1010_RDB
> >>         bool "Freescale P1010RDB"
> >>         select DEFAULT_UIMAGE
> >>         help
> >>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>
> >>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>           (DDR3/3L, SATA II, and PCI  Express).
> >>
> >>
> >>> Our contact with Freescale would prefer that I not post that patch until
> >>> we get the OK from freescale to do so since we received it under NDA.
> >>
> >> I don't think we currently need it. I prefer dropping and cleaning up
> >> the device tree stuff as it is not needed for the P1010 anyway. If a
> >> new processor shows up with enhanced capabilities requiring
> >> configuration via device tree, we or somebody else can provide a patch.
> >> Marc, what do you think?
> > 
> > I will rebase shortly and provide a newer set of patches.
> > 
> > I do think powerpc does need the device tree support.  That is how the flexcan_probe
> > is getting called.  How would you suggest I do it otherwise?
> 
> Why do you think that?

In patch 3/5 in this series (attached below), I made a change in how
device discovery works.  Without that of_match stuff, the flexcan
driver was never getting its flexcan_probe function called.  As soon
as I added that, it worked.  Looking at the driver_register path, this
appeared to be the "correct" way to implement the device discovery.
Did I miss something?

Thanks,
Robin


The OpenFirmware devices are not matched without specifying
an of_match array.  Introduce that array as that is used for
matching on the Freescale P1010 processor.

Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

---
I kept the of_match for "fsl,flexcan-v1.0" for the time being.  I will
happily drop it for final submission once I have a boot loader worked
up that matches on either string.
---
 drivers/net/can/flexcan.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index ecdd4e6..d4ac81b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1028,8 +1028,22 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id flexcan_of_match[] = {
+	{
+		.compatible = "fsl,flexcan-v1.0",
+	},
+	{
+		.compatible = "fsl,flexcan",
+	},
+	{},
+};
+
 static struct platform_driver flexcan_driver = {
-	.driver.name = DRV_NAME,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = flexcan_of_match,
+	},
 	.probe = flexcan_probe,
 	.remove = __devexit_p(flexcan_remove),
 };
-- 
1.7.2.1

^ permalink raw reply related

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Wolfgang Grandegger @ 2011-08-08 15:27 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, U Bhaskar-B22300,
	Marc Kleine-Budde, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110808152549.GB4926-sJ/iWh9BUns@public.gmane.org>

On 08/08/2011 05:25 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:18:57PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 05:09 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>>
>>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>>> ...
>>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>>
>>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>>
>>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>>> "clock_freq" while
>>>>>>>>
>>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>>
>>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>>
>>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>>> as well.
>>>>>>
>>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>>
>>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>>> properties, etc. The clk implemetation might go into
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>>
>>>>>> or
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>>
>>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>>
>>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>>> you using?
>>>>>
>>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>>> selection to determine is we are going to build the flexcan.c file.
>>>>
>>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>>> we should do it differently for PowerPC. 
>>>>
>>>> For mainline inclusion, you should provide your patches against the
>>>> David Millers "net-next-2.6" tree, which already seems to have support
>>>> for the P1010RDB:
>>>>
>>>>   config P1010_RDB
>>>>         bool "Freescale P1010RDB"
>>>>         select DEFAULT_UIMAGE
>>>>         help
>>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>>
>>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>>
>>>>
>>>>> Our contact with Freescale would prefer that I not post that patch until
>>>>> we get the OK from freescale to do so since we received it under NDA.
>>>>
>>>> I don't think we currently need it. I prefer dropping and cleaning up
>>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>>> new processor shows up with enhanced capabilities requiring
>>>> configuration via device tree, we or somebody else can provide a patch.
>>>> Marc, what do you think?
>>>
>>> I will rebase shortly and provide a newer set of patches.
>>>
>>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>>> is getting called.  How would you suggest I do it otherwise?
>>
>> Why do you think that?
> 
> In patch 3/5 in this series (attached below), I made a change in how
> device discovery works.  Without that of_match stuff, the flexcan
> driver was never getting its flexcan_probe function called.  As soon
> as I added that, it worked.  Looking at the driver_register path, this
> appeared to be the "correct" way to implement the device discovery.
> Did I miss something?

I already clarified my statement. Hope you agree now.

Wolfgang.

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Wolfgang Grandegger @ 2011-08-08 15:33 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300
In-Reply-To: <4E3FFD5B.7080000-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>
>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>> ...
>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>
>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>
>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>> "clock_freq" while
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>
>>>>>> documents "clock-frequencies"... :-(.
>>>>>
>>>>> You answered a different question that I was asking.  I was asking if
>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>> as well.
>>>>
>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>
>>>> For the P1010 we can sinmply derive the clock frequency from
>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>> properties, etc. The clk implemetation might go into
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>
>>>> or
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>
>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>
>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>> you using?
>>>
>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>> platform, and then work from there for the flexcan stuff.  That patch
>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>> selection to determine is we are going to build the flexcan.c file.
>>
>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>> we should do it differently for PowerPC. 
>>
>> For mainline inclusion, you should provide your patches against the
>> David Millers "net-next-2.6" tree, which already seems to have support
>> for the P1010RDB:
>>
>>   config P1010_RDB
>>         bool "Freescale P1010RDB"
>>         select DEFAULT_UIMAGE
>>         help
>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>
>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>           (DDR3/3L, SATA II, and PCI  Express).
>>
>>
>>> Our contact with Freescale would prefer that I not post that patch until
>>> we get the OK from freescale to do so since we received it under NDA.
>>
>> I don't think we currently need it. I prefer dropping and cleaning up
>> the device tree stuff as it is not needed for the P1010 anyway. If a
>> new processor shows up with enhanced capabilities requiring
>> configuration via device tree, we or somebody else can provide a patch.
>> Marc, what do you think?
> 
> ACK - The device tree bindings as in mainline's Documentation is a mess.
> If the powerpc guys are happy with a clock interfaces based approach
> somewhere in arch/ppc, I'm more than happy to remove:
> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> 
> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> - clock-frequency           /   a single clock-frequency attribute

In the "net-next-2.6" tree there is also:

 $ grep flexcan arch/powerpc/boots/dts/*.dts
  p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
  p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
  p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
  p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
  p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
  p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;

Especially the fsl,flexcan-clock-divider = <2>; might make people think,
that they could set something else.

Wolfgang.



> Marc
> 
> 
> 
> 
> _______________________________________________
> Socketcan-core mailing list
> Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> https://lists.berlios.de/mailman/listinfo/socketcan-core

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Robin Holt @ 2011-08-08 15:38 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, U Bhaskar-B22300,
	Marc Kleine-Budde, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4E3FFF41.7030401-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

On Mon, Aug 08, 2011 at 05:22:41PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
> > On 08/08/2011 05:09 PM, Robin Holt wrote:
> >> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
> >>> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>>
> >>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>>> ...
> >>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>>
> >>>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>>
> >>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>>> "clock_freq" while
> >>>>>>>
> >>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>>
> >>>>>>> documents "clock-frequencies"... :-(.
> >>>>>>
> >>>>>> You answered a different question that I was asking.  I was asking if
> >>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>>> as well.
> >>>>>
> >>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>>
> >>>>> For the P1010 we can sinmply derive the clock frequency from
> >>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>>> properties, etc. The clk implemetation might go into
> >>>>>
> >>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>>
> >>>>> or
> >>>>>
> >>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>>
> >>>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>>
> >>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>>> you using?
> >>>>
> >>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>>> platform, and then work from there for the flexcan stuff.  That patch
> >>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>>> selection to determine is we are going to build the flexcan.c file.
> >>>
> >>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >>> we should do it differently for PowerPC. 
> >>>
> >>> For mainline inclusion, you should provide your patches against the
> >>> David Millers "net-next-2.6" tree, which already seems to have support
> >>> for the P1010RDB:
> >>>
> >>>   config P1010_RDB
> >>>         bool "Freescale P1010RDB"
> >>>         select DEFAULT_UIMAGE
> >>>         help
> >>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>>
> >>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>>           (DDR3/3L, SATA II, and PCI  Express).
> >>>
> >>>
> >>>> Our contact with Freescale would prefer that I not post that patch until
> >>>> we get the OK from freescale to do so since we received it under NDA.
> >>>
> >>> I don't think we currently need it. I prefer dropping and cleaning up
> >>> the device tree stuff as it is not needed for the P1010 anyway. If a
> >>> new processor shows up with enhanced capabilities requiring
> >>> configuration via device tree, we or somebody else can provide a patch.
> >>> Marc, what do you think?
> >>
> >> I will rebase shortly and provide a newer set of patches.
> >>
> >> I do think powerpc does need the device tree support.  That is how the flexcan_probe
> >> is getting called.  How would you suggest I do it otherwise?
> > 
> > Why do you think that?
> 
> To be clear. I mean we do not need the extra "fsl," properties for the
> clock source and divider and frequency.

I agree with that.  The can definition in the .dts file, however,
should be can0@... "fsl,flexcan" in an ideal world, correct?  If that
is correct, then I will make the of_match string match fsl,flexcan and
update the .dts file accordingly.

Thanks,
Robin

^ permalink raw reply

* Re: [RFC PATCH v2 0/9] bql: Byte Queue Limits
From: Stephen Hemminger @ 2011-08-08 15:40 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, netdev
In-Reply-To: <alpine.DEB.2.00.1108072129010.13386@pokey.mtv.corp.google.com>

On Sun, 7 Aug 2011 21:43:13 -0700 (PDT)
Tom Herbert <therbert@google.com> wrote:

>     netdev_tx_completed_queue: Called at end of transmit completion
>       to inform stack of number of bytes and packets processed.
>     netdev_tx_sent_queue: Called to inform stack when packets are
>       queued.

Couldn't these be done for the device in the existing qdisc infra
structure (or dev_start_xmit). Alternatively, rename ndo_start_xmit
to something else and make all the callers use the wrapper.

Changing all the drivers for something that the driver has no real
need to care about seems like incorrect object design.

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Wolfgang Grandegger @ 2011-08-08 15:50 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Marc Kleine-Budde,
	U Bhaskar-B22300, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110808153835.GC4926-sJ/iWh9BUns@public.gmane.org>

On 08/08/2011 05:38 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:22:41PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
>>> On 08/08/2011 05:09 PM, Robin Holt wrote:
>>>> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>>>
>>>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>>>> ...
>>>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>>>
>>>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>>>
>>>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>>>> "clock_freq" while
>>>>>>>>>
>>>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>>>
>>>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>>>
>>>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>>>> as well.
>>>>>>>
>>>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>>>
>>>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>>>> properties, etc. The clk implemetation might go into
>>>>>>>
>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>>>
>>>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>>>
>>>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>>>> you using?
>>>>>>
>>>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>>>> selection to determine is we are going to build the flexcan.c file.
>>>>>
>>>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>>>> we should do it differently for PowerPC. 
>>>>>
>>>>> For mainline inclusion, you should provide your patches against the
>>>>> David Millers "net-next-2.6" tree, which already seems to have support
>>>>> for the P1010RDB:
>>>>>
>>>>>   config P1010_RDB
>>>>>         bool "Freescale P1010RDB"
>>>>>         select DEFAULT_UIMAGE
>>>>>         help
>>>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>>>
>>>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>>>
>>>>>
>>>>>> Our contact with Freescale would prefer that I not post that patch until
>>>>>> we get the OK from freescale to do so since we received it under NDA.
>>>>>
>>>>> I don't think we currently need it. I prefer dropping and cleaning up
>>>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>>>> new processor shows up with enhanced capabilities requiring
>>>>> configuration via device tree, we or somebody else can provide a patch.
>>>>> Marc, what do you think?
>>>>
>>>> I will rebase shortly and provide a newer set of patches.
>>>>
>>>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>>>> is getting called.  How would you suggest I do it otherwise?
>>>
>>> Why do you think that?
>>
>> To be clear. I mean we do not need the extra "fsl," properties for the
>> clock source and divider and frequency.
> 
> I agree with that.  The can definition in the .dts file, however,
> should be can0@... "fsl,flexcan" in an ideal world, correct?  If that

No, it's normally <device-type>@<address>.

> is correct, then I will make the of_match string match fsl,flexcan and
> update the .dts file accordingly.

As I said. For the P1010 the clock get function just needs to return
"fsl_get_sys_freq()". No need to inspect the device tree. And I would
provide the clk implementation in

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c

or even:

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c

Wolfgang.

^ permalink raw reply

* [PATCH net-next 0/2] bnx2x: Logging message cleanups
From: Joe Perches @ 2011-08-08 15:53 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel

Joe Perches (2):
  bnx2x: Remove local defines for %pM and mac address
  bnx2x: Coalesce pr_cont uses and fix DP typos

 drivers/net/bnx2x/bnx2x.h      |    4 -
 drivers/net/bnx2x/bnx2x_cmn.c  |   42 ++++++-----
 drivers/net/bnx2x/bnx2x_cmn.h  |    4 +-
 drivers/net/bnx2x/bnx2x_dcb.c  |    2 +-
 drivers/net/bnx2x/bnx2x_link.c |  155 +++++++++++++++++++++-------------------
 drivers/net/bnx2x/bnx2x_main.c |   47 ++++++-------
 drivers/net/bnx2x/bnx2x_sp.c   |   64 ++++++++---------
 7 files changed, 158 insertions(+), 160 deletions(-)

-- 
1.7.6.405.gc1be0

^ permalink raw reply

* [PATCH net-next 1/2] bnx2x: Remove local defines for %pM and mac address
From: Joe Perches @ 2011-08-08 15:53 UTC (permalink / raw)
  To: Eilon Greenstein; +Cc: netdev, linux-kernel
In-Reply-To: <cover.1312818707.git.joe@perches.com>

Use %pM and mac address directly instead.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/net/bnx2x/bnx2x.h      |    4 ---
 drivers/net/bnx2x/bnx2x_main.c |   14 +++++-------
 drivers/net/bnx2x/bnx2x_sp.c   |   41 ++++++++++++++++-----------------------
 3 files changed, 23 insertions(+), 36 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
index c423504..5aac959 100644
--- a/drivers/net/bnx2x/bnx2x.h
+++ b/drivers/net/bnx2x/bnx2x.h
@@ -115,10 +115,6 @@ do {								 \
 		dev_info(&bp->pdev->dev, __fmt, ##__args);	 \
 } while (0)
 
-#define BNX2X_MAC_FMT		"%pM"
-#define BNX2X_MAC_PRN_LIST(mac)	(mac)
-
-
 #ifdef BNX2X_STOP_ON_ERROR
 void bnx2x_int_disable(struct bnx2x *bp);
 #define bnx2x_panic() do { \
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index 1507091..173b258 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -9315,9 +9315,8 @@ static void __devinit bnx2x_get_mac_hwinfo(struct bnx2x *bp)
 				val = MF_CFG_RD(bp, func_ext_config[func].
 						    iscsi_mac_addr_lower);
 				bnx2x_set_mac_buf(iscsi_mac, val, val2);
-				BNX2X_DEV_INFO("Read iSCSI MAC: "
-					       BNX2X_MAC_FMT"\n",
-					       BNX2X_MAC_PRN_LIST(iscsi_mac));
+				BNX2X_DEV_INFO("Read iSCSI MAC: %pM\n",
+					       iscsi_mac);
 			} else
 				bp->flags |= NO_ISCSI_OOO_FLAG | NO_ISCSI_FLAG;
 
@@ -9327,9 +9326,8 @@ static void __devinit bnx2x_get_mac_hwinfo(struct bnx2x *bp)
 				val = MF_CFG_RD(bp, func_ext_config[func].
 						    fcoe_mac_addr_lower);
 				bnx2x_set_mac_buf(fip_mac, val, val2);
-				BNX2X_DEV_INFO("Read FCoE L2 MAC to "
-					       BNX2X_MAC_FMT"\n",
-					       BNX2X_MAC_PRN_LIST(fip_mac));
+				BNX2X_DEV_INFO("Read FCoE L2 MAC to %pM\n",
+					       fip_mac);
 
 			} else
 				bp->flags |= NO_FCOE_FLAG;
@@ -9384,9 +9382,9 @@ static void __devinit bnx2x_get_mac_hwinfo(struct bnx2x *bp)
 	if (!is_valid_ether_addr(bp->dev->dev_addr))
 		dev_err(&bp->pdev->dev,
 			"bad Ethernet MAC address configuration: "
-			BNX2X_MAC_FMT", change it manually before bringing up "
+			"%pM, change it manually before bringing up "
 			"the appropriate network interface\n",
-			BNX2X_MAC_PRN_LIST(bp->dev->dev_addr));
+			bp->dev->dev_addr);
 }
 
 static int __devinit bnx2x_get_hwinfo(struct bnx2x *bp)
diff --git a/drivers/net/bnx2x/bnx2x_sp.c b/drivers/net/bnx2x/bnx2x_sp.c
index df52f11..b4d9c16 100644
--- a/drivers/net/bnx2x/bnx2x_sp.c
+++ b/drivers/net/bnx2x/bnx2x_sp.c
@@ -707,9 +707,8 @@ static void bnx2x_set_one_mac_e2(struct bnx2x *bp,
 	bnx2x_vlan_mac_set_cmd_hdr_e2(bp, o, add, CLASSIFY_RULE_OPCODE_MAC,
 				      &rule_entry->mac.header);
 
-	DP(BNX2X_MSG_SP, "About to %s MAC "BNX2X_MAC_FMT" for "
-			 "Queue %d\n", (add ? "add" : "delete"),
-			 BNX2X_MAC_PRN_LIST(mac), raw->cl_id);
+	DP(BNX2X_MSG_SP, "About to %s MAC %pM for Queue %d\n",
+			 add ? "add" : "delete", mac, raw->cl_id);
 
 	/* Set a MAC itself */
 	bnx2x_set_fw_mac_addr(&rule_entry->mac.mac_msb,
@@ -801,9 +800,9 @@ static inline void bnx2x_vlan_mac_set_rdata_e1x(struct bnx2x *bp,
 	bnx2x_vlan_mac_set_cfg_entry_e1x(bp, o, add, opcode, mac, vlan_id,
 					 cfg_entry);
 
-	DP(BNX2X_MSG_SP, "%s MAC "BNX2X_MAC_FMT" CLID %d CAM offset %d\n",
-			 (add ? "setting" : "clearing"),
-			 BNX2X_MAC_PRN_LIST(mac), raw->cl_id, cam_offset);
+	DP(BNX2X_MSG_SP, "%s MAC %pM CLID %d CAM offset %d\n",
+			 add ? "setting" : "clearing",
+			 mac, raw->cl_id, cam_offset);
 }
 
 /**
@@ -2579,9 +2578,8 @@ static inline void bnx2x_mcast_hdl_pending_add_e2(struct bnx2x *bp,
 
 		cnt++;
 
-		DP(BNX2X_MSG_SP, "About to configure "BNX2X_MAC_FMT
-				 " mcast MAC\n",
-				 BNX2X_MAC_PRN_LIST(pmac_pos->mac));
+		DP(BNX2X_MSG_SP, "About to configure %pM mcast MAC\n",
+				 pmac_pos->mac);
 
 		list_del(&pmac_pos->link);
 
@@ -2702,9 +2700,8 @@ static inline void bnx2x_mcast_hdl_add(struct bnx2x *bp,
 
 		cnt++;
 
-		DP(BNX2X_MSG_SP, "About to configure "BNX2X_MAC_FMT
-				 " mcast MAC\n",
-				 BNX2X_MAC_PRN_LIST(mlist_pos->mac));
+		DP(BNX2X_MSG_SP, "About to configure %pM mcast MAC\n",
+				 mlist_pos->mac);
 	}
 
 	*line_idx = cnt;
@@ -2998,9 +2995,8 @@ static inline void bnx2x_mcast_hdl_add_e1h(struct bnx2x *bp,
 		bit = bnx2x_mcast_bin_from_mac(mlist_pos->mac);
 		BNX2X_57711_SET_MC_FILTER(mc_filter, bit);
 
-		DP(BNX2X_MSG_SP, "About to configure "
-				 BNX2X_MAC_FMT" mcast MAC, bin %d\n",
-				 BNX2X_MAC_PRN_LIST(mlist_pos->mac), bit);
+		DP(BNX2X_MSG_SP, "About to configure %pM mcast MAC, bin %d\n",
+				 mlist_pos->mac, bit);
 
 		/* bookkeeping... */
 		BIT_VEC64_SET_BIT(o->registry.aprox_match.vec,
@@ -3233,9 +3229,8 @@ static inline int bnx2x_mcast_handle_restore_cmd_e1(
 
 		i++;
 
-		  DP(BNX2X_MSG_SP, "About to configure "BNX2X_MAC_FMT
-				   " mcast MAC\n",
-				   BNX2X_MAC_PRN_LIST(cfg_data.mac));
+		  DP(BNX2X_MSG_SP, "About to configure %pM mcast MAC\n",
+				   cfg_data.mac);
 	}
 
 	*rdata_idx = i;
@@ -3270,9 +3265,8 @@ static inline int bnx2x_mcast_handle_pending_cmds_e1(
 
 			cnt++;
 
-			DP(BNX2X_MSG_SP, "About to configure "BNX2X_MAC_FMT
-					 " mcast MAC\n",
-					 BNX2X_MAC_PRN_LIST(pmac_pos->mac));
+			DP(BNX2X_MSG_SP, "About to configure %pM mcast MAC\n",
+					 pmac_pos->mac);
 		}
 		break;
 
@@ -3357,9 +3351,8 @@ static inline int bnx2x_mcast_refresh_registry_e1(struct bnx2x *bp,
 				&data->config_table[i].middle_mac_addr,
 				&data->config_table[i].lsb_mac_addr,
 				elem->mac);
-			DP(BNX2X_MSG_SP, "Adding registry entry for ["
-					 BNX2X_MAC_FMT"]\n",
-				   BNX2X_MAC_PRN_LIST(elem->mac));
+			DP(BNX2X_MSG_SP, "Adding registry entry for [%pM]\n",
+					 elem->mac);
 			list_add_tail(&elem->link,
 				      &o->registry.exact_match.macs);
 		}
-- 
1.7.6.405.gc1be0

^ permalink raw reply related

* [PATCH net-next 2/2] bnx2x: Coalesce pr_cont uses and fix DP typos
From: Joe Perches @ 2011-08-08 15:53 UTC (permalink / raw)
  To: Eilon Greenstein; +Cc: netdev, linux-kernel
In-Reply-To: <cover.1312818707.git.joe@perches.com>

Uses of pr_cont should be avoided where reasonably possible
because they can be interleaved by other threads and processes.

Coalesce pr_cont uses.

Fix typos, duplicated words and spacing in DP uses caused
by split multi-line formats.  Coalesce some of these
split formats.  Add missing terminating newlines to DP uses.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/net/bnx2x/bnx2x_cmn.c  |   42 ++++++-----
 drivers/net/bnx2x/bnx2x_cmn.h  |    4 +-
 drivers/net/bnx2x/bnx2x_dcb.c  |    2 +-
 drivers/net/bnx2x/bnx2x_link.c |  155 +++++++++++++++++++++-------------------
 drivers/net/bnx2x/bnx2x_main.c |   33 ++++-----
 drivers/net/bnx2x/bnx2x_sp.c   |   23 +++---
 6 files changed, 135 insertions(+), 124 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index 5b0dba6..456baf6 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -953,15 +953,16 @@ void __bnx2x_link_report(struct bnx2x *bp)
 		netdev_err(bp->dev, "NIC Link is Down\n");
 		return;
 	} else {
+		const char *duplex;
+		const char *flow;
+
 		netif_carrier_on(bp->dev);
-		netdev_info(bp->dev, "NIC Link is Up, ");
-		pr_cont("%d Mbps ", cur_data.line_speed);
 
 		if (test_and_clear_bit(BNX2X_LINK_REPORT_FD,
 				       &cur_data.link_report_flags))
-			pr_cont("full duplex");
+			duplex = "full";
 		else
-			pr_cont("half duplex");
+			duplex = "half";
 
 		/* Handle the FC at the end so that only these flags would be
 		 * possibly set. This way we may easily check if there is no FC
@@ -970,16 +971,19 @@ void __bnx2x_link_report(struct bnx2x *bp)
 		if (cur_data.link_report_flags) {
 			if (test_bit(BNX2X_LINK_REPORT_RX_FC_ON,
 				     &cur_data.link_report_flags)) {
-				pr_cont(", receive ");
 				if (test_bit(BNX2X_LINK_REPORT_TX_FC_ON,
 				     &cur_data.link_report_flags))
-					pr_cont("& transmit ");
+					flow = "ON - receive & transmit";
+				else
+					flow = "ON - receive";
 			} else {
-				pr_cont(", transmit ");
+				flow = "ON - transmit";
 			}
-			pr_cont("flow control ON");
+		} else {
+			flow = "none";
 		}
-		pr_cont("\n");
+		netdev_info(bp->dev, "NIC Link is Up, %d Mbps %s duplex, Flow control: %s\n",
+			    cur_data.line_speed, duplex, flow);
 	}
 }
 
@@ -2578,7 +2582,7 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
 #endif
 
 	/* enable this debug print to view the transmission queue being used
-	DP(BNX2X_MSG_FP, "indices: txq %d, fp %d, txdata %d",
+	DP(BNX2X_MSG_FP, "indices: txq %d, fp %d, txdata %d\n",
 	   txq_index, fp_index, txdata_index); */
 
 	/* locate the fastpath and the txdata */
@@ -2587,7 +2591,7 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* enable this debug print to view the tranmission details
 	DP(BNX2X_MSG_FP,"transmitting packet cid %d fp index %d txdata_index %d"
-			" tx_data ptr %p fp pointer %p",
+			" tx_data ptr %p fp pointer %p\n",
 	   txdata->cid, fp_index, txdata_index, txdata, fp); */
 
 	if (unlikely(bnx2x_tx_avail(bp, txdata) <
@@ -2904,14 +2908,14 @@ int bnx2x_setup_tc(struct net_device *dev, u8 num_tc)
 	/* requested to support too many traffic classes */
 	if (num_tc > bp->max_cos) {
 		DP(NETIF_MSG_TX_ERR, "support for too many traffic classes"
-				     " requested: %d. max supported is %d",
+				     " requested: %d. max supported is %d\n",
 				     num_tc, bp->max_cos);
 		return -EINVAL;
 	}
 
 	/* declare amount of supported traffic classes */
 	if (netdev_set_num_tc(dev, num_tc)) {
-		DP(NETIF_MSG_TX_ERR, "failed to declare %d traffic classes",
+		DP(NETIF_MSG_TX_ERR, "failed to declare %d traffic classes\n",
 				     num_tc);
 		return -EINVAL;
 	}
@@ -2919,7 +2923,7 @@ int bnx2x_setup_tc(struct net_device *dev, u8 num_tc)
 	/* configure priority to traffic class mapping */
 	for (prio = 0; prio < BNX2X_MAX_PRIORITY; prio++) {
 		netdev_set_prio_tc_map(dev, prio, bp->prio_to_cos[prio]);
-		DP(BNX2X_MSG_SP, "mapping priority %d to tc %d",
+		DP(BNX2X_MSG_SP, "mapping priority %d to tc %d\n",
 		   prio, bp->prio_to_cos[prio]);
 	}
 
@@ -2928,10 +2932,10 @@ int bnx2x_setup_tc(struct net_device *dev, u8 num_tc)
 	   This can be used for ets or pfc, and save the effort of setting
 	   up a multio class queue disc or negotiating DCBX with a switch
 	netdev_set_prio_tc_map(dev, 0, 0);
-	DP(BNX2X_MSG_SP, "mapping priority %d to tc %d", 0, 0);
+	DP(BNX2X_MSG_SP, "mapping priority %d to tc %d\n", 0, 0);
 	for (prio = 1; prio < 16; prio++) {
 		netdev_set_prio_tc_map(dev, prio, 1);
-		DP(BNX2X_MSG_SP, "mapping priority %d to tc %d", prio, 1);
+		DP(BNX2X_MSG_SP, "mapping priority %d to tc %d\n", prio, 1);
 	} */
 
 	/* configure traffic class to transmission queue mapping */
@@ -2939,7 +2943,7 @@ int bnx2x_setup_tc(struct net_device *dev, u8 num_tc)
 		count = BNX2X_NUM_ETH_QUEUES(bp);
 		offset = cos * MAX_TXQS_PER_COS;
 		netdev_set_tc_queue(dev, cos, count, offset);
-		DP(BNX2X_MSG_SP, "mapping tc %d to offset %d count %d",
+		DP(BNX2X_MSG_SP, "mapping tc %d to offset %d count %d\n",
 		   cos, offset, count);
 	}
 
@@ -3027,7 +3031,7 @@ static void bnx2x_free_fp_mem_at(struct bnx2x *bp, int fp_index)
 			struct bnx2x_fp_txdata *txdata = &fp->txdata[cos];
 
 			DP(BNX2X_MSG_SP,
-			   "freeing tx memory of fp %d cos %d cid %d",
+			   "freeing tx memory of fp %d cos %d cid %d\n",
 			   fp_index, cos, txdata->cid);
 
 			BNX2X_FREE(txdata->tx_buf_ring);
@@ -3109,7 +3113,7 @@ static int bnx2x_alloc_fp_mem_at(struct bnx2x *bp, int index)
 			struct bnx2x_fp_txdata *txdata = &fp->txdata[cos];
 
 			DP(BNX2X_MSG_SP, "allocating tx memory of "
-					 "fp %d cos %d",
+					 "fp %d cos %d\n",
 			   index, cos);
 
 			BNX2X_ALLOC(txdata->tx_buf_ring,
diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
index 223bfee..501a24b 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/bnx2x/bnx2x_cmn.h
@@ -1289,7 +1289,7 @@ static inline void bnx2x_init_txdata(struct bnx2x *bp,
 	txdata->txq_index = txq_index;
 	txdata->tx_cons_sb = tx_cons_sb;
 
-	DP(BNX2X_MSG_SP, "created tx data cid %d, txq %d",
+	DP(BNX2X_MSG_SP, "created tx data cid %d, txq %d\n",
 	   txdata->cid, txdata->txq_index);
 }
 
@@ -1333,7 +1333,7 @@ static inline void bnx2x_init_fcoe_fp(struct bnx2x *bp)
 	bnx2x_init_txdata(bp, &bnx2x_fcoe(bp, txdata[0]),
 			  fp->cid, FCOE_TXQ_IDX(bp), BNX2X_FCOE_L2_TX_INDEX);
 
-	DP(BNX2X_MSG_SP, "created fcoe tx data (fp index %d)", fp->index);
+	DP(BNX2X_MSG_SP, "created fcoe tx data (fp index %d)\n", fp->index);
 
 	/* qZone id equals to FW (per path) client id */
 	bnx2x_fcoe(bp, cl_qzone_id) = bnx2x_fp_qzone_id(fp);
diff --git a/drivers/net/bnx2x/bnx2x_dcb.c b/drivers/net/bnx2x/bnx2x_dcb.c
index a4ea35f..38b5ca5 100644
--- a/drivers/net/bnx2x/bnx2x_dcb.c
+++ b/drivers/net/bnx2x/bnx2x_dcb.c
@@ -350,7 +350,7 @@ static void bnx2x_dcbx_map_nw(struct bnx2x *bp)
 		if (cos_params[i].pri_bitmask & nw_prio) {
 			/* extend the bitmask with unmapped */
 			DP(NETIF_MSG_LINK,
-			   "cos %d extended with 0x%08x", i, unmapped);
+			   "cos %d extended with 0x%08x\n", i, unmapped);
 			cos_params[i].pri_bitmask |= unmapped;
 			break;
 		}
diff --git a/drivers/net/bnx2x/bnx2x_link.c b/drivers/net/bnx2x/bnx2x_link.c
index bcd8f00..5874f68 100644
--- a/drivers/net/bnx2x/bnx2x_link.c
+++ b/drivers/net/bnx2x/bnx2x_link.c
@@ -694,8 +694,8 @@ static int bnx2x_ets_e3b0_disabled(const struct link_params *params,
 	struct bnx2x *bp = params->bp;
 
 	if (!CHIP_IS_E3B0(bp)) {
-		DP(NETIF_MSG_LINK, "bnx2x_ets_e3b0_disabled the chip isn't E3B0"
-				   "\n");
+		DP(NETIF_MSG_LINK,
+		   "bnx2x_ets_e3b0_disabled the chip isn't E3B0\n");
 		return -EINVAL;
 	}
 
@@ -854,8 +854,8 @@ static int bnx2x_ets_e3b0_get_total_bw(
 		if (bnx2x_cos_state_bw == ets_params->cos[cos_idx].state) {
 
 			if (0 == ets_params->cos[cos_idx].params.bw_params.bw) {
-				DP(NETIF_MSG_LINK, "bnx2x_ets_E3B0_config BW"
-						   "was set to 0\n");
+				DP(NETIF_MSG_LINK,
+				   "bnx2x_ets_E3B0_config BW was set to 0\n");
 			return -EINVAL;
 		}
 		*total_bw +=
@@ -866,12 +866,12 @@ static int bnx2x_ets_e3b0_get_total_bw(
 	/*Check taotl BW is valid */
 	if ((100 != *total_bw) || (0 == *total_bw)) {
 		if (0 == *total_bw) {
-			DP(NETIF_MSG_LINK, "bnx2x_ets_E3B0_config toatl BW"
-					   "shouldn't be 0\n");
+			DP(NETIF_MSG_LINK,
+			   "bnx2x_ets_E3B0_config toatl BW shouldn't be 0\n");
 			return -EINVAL;
 		}
-		DP(NETIF_MSG_LINK, "bnx2x_ets_E3B0_config toatl BW should be"
-				   "100\n");
+		DP(NETIF_MSG_LINK,
+		   "bnx2x_ets_E3B0_config toatl BW should be 100\n");
 		/**
 		*   We can handle a case whre the BW isn't 100 this can happen
 		*   if the TC are joined.
@@ -908,13 +908,13 @@ static int bnx2x_ets_e3b0_sp_pri_to_cos_set(const struct link_params *params,
 
 	if (DCBX_INVALID_COS != sp_pri_to_cos[pri]) {
 		DP(NETIF_MSG_LINK, "bnx2x_ets_e3b0_sp_pri_to_cos_set invalid "
-				   "parameter There can't be two COS's with"
+				   "parameter There can't be two COS's with "
 				   "the same strict pri\n");
 		return -EINVAL;
 	}
 
 	if (pri > max_num_of_cos) {
-		DP(NETIF_MSG_LINK, "bnx2x_ets_e3b0_sp_pri_to_cos_set invalid"
+		DP(NETIF_MSG_LINK, "bnx2x_ets_e3b0_sp_pri_to_cos_set invalid "
 			       "parameter Illegal strict priority\n");
 	    return -EINVAL;
 	}
@@ -1090,8 +1090,8 @@ int bnx2x_ets_e3b0_config(const struct link_params *params,
 	u8 cos_entry = 0;
 
 	if (!CHIP_IS_E3B0(bp)) {
-		DP(NETIF_MSG_LINK, "bnx2x_ets_e3b0_disabled the chip isn't E3B0"
-				   "\n");
+		DP(NETIF_MSG_LINK,
+		   "bnx2x_ets_e3b0_disabled the chip isn't E3B0\n");
 		return -EINVAL;
 	}
 
@@ -1108,8 +1108,8 @@ int bnx2x_ets_e3b0_config(const struct link_params *params,
 	bnx2x_status = bnx2x_ets_e3b0_get_total_bw(params, ets_params,
 						   &total_bw);
 	if (0 != bnx2x_status) {
-		DP(NETIF_MSG_LINK, "bnx2x_ets_E3B0_config get_total_bw failed "
-				   "\n");
+		DP(NETIF_MSG_LINK,
+		   "bnx2x_ets_E3B0_config get_total_bw failed\n");
 		return -EINVAL;
 	}
 
@@ -1144,13 +1144,13 @@ int bnx2x_ets_e3b0_config(const struct link_params *params,
 				cos_entry);
 
 		} else {
-			DP(NETIF_MSG_LINK, "bnx2x_ets_e3b0_config cos state not"
-					   " valid\n");
+			DP(NETIF_MSG_LINK,
+			   "bnx2x_ets_e3b0_config cos state not valid\n");
 			return -EINVAL;
 		}
 		if (0 != bnx2x_status) {
-			DP(NETIF_MSG_LINK, "bnx2x_ets_e3b0_config set cos bw "
-					   "failed\n");
+			DP(NETIF_MSG_LINK,
+			   "bnx2x_ets_e3b0_config set cos bw failed\n");
 			return bnx2x_status;
 		}
 	}
@@ -1160,8 +1160,8 @@ int bnx2x_ets_e3b0_config(const struct link_params *params,
 							 sp_pri_to_cos);
 
 	if (0 != bnx2x_status) {
-		DP(NETIF_MSG_LINK, "bnx2x_ets_E3B0_config set_pri_cli_reg "
-				   "failed\n");
+		DP(NETIF_MSG_LINK,
+		   "bnx2x_ets_E3B0_config set_pri_cli_reg failed\n");
 		return bnx2x_status;
 	}
 
@@ -1612,8 +1612,8 @@ static void bnx2x_xmac_init(struct bnx2x *bp, u32 max_speed)
 
 	if (is_port4mode && (REG_RD(bp, MISC_REG_RESET_REG_2) &
 	     MISC_REGISTERS_RESET_REG_2_XMAC)) {
-		DP(NETIF_MSG_LINK, "XMAC already out of reset"
-				   " in 4-port mode\n");
+		DP(NETIF_MSG_LINK,
+		   "XMAC already out of reset in 4-port mode\n");
 		return;
 	}
 
@@ -1636,13 +1636,13 @@ static void bnx2x_xmac_init(struct bnx2x *bp, u32 max_speed)
 		/*  Set the number of ports on the system side to 1 */
 		REG_WR(bp, MISC_REG_XMAC_CORE_PORT_MODE, 0);
 		if (max_speed == SPEED_10000) {
-			DP(NETIF_MSG_LINK, "Init XMAC to 10G x 1"
-					   " port per path\n");
+			DP(NETIF_MSG_LINK,
+			   "Init XMAC to 10G x 1 port per path\n");
 			/* Set the number of ports on the Warp Core to 10G */
 			REG_WR(bp, MISC_REG_XMAC_PHY_PORT_MODE, 3);
 		} else {
-			DP(NETIF_MSG_LINK, "Init XMAC to 20G x 2 ports"
-					   " per path\n");
+			DP(NETIF_MSG_LINK,
+			   "Init XMAC to 20G x 2 ports per path\n");
 			/* Set the number of ports on the Warp Core to 20G */
 			REG_WR(bp, MISC_REG_XMAC_PHY_PORT_MODE, 1);
 		}
@@ -3945,8 +3945,8 @@ static void bnx2x_warpcore_set_sgmii_speed(struct bnx2x_phy *phy,
 			val16 |= 0x0040;
 			break;
 		default:
-			DP(NETIF_MSG_LINK, "Speed not supported: 0x%x"
-					   "\n", phy->req_line_speed);
+			DP(NETIF_MSG_LINK,
+			   "Speed not supported: 0x%x\n", phy->req_line_speed);
 			return;
 		}
 
@@ -4078,9 +4078,9 @@ static int bnx2x_get_mod_abs_int_cfg(struct bnx2x *bp,
 		 */
 		if ((cfg_pin < PIN_CFG_GPIO0_P0) ||
 		    (cfg_pin > PIN_CFG_GPIO3_P1)) {
-			DP(NETIF_MSG_LINK, "ERROR: Invalid cfg pin %x for "
-					   "module detect indication\n",
-				       cfg_pin);
+			DP(NETIF_MSG_LINK,
+			   "ERROR: Invalid cfg pin %x for module detect indication\n",
+			   cfg_pin);
 			return -EINVAL;
 		}
 
@@ -4208,8 +4208,9 @@ static void bnx2x_warpcore_config_init(struct bnx2x_phy *phy,
 			break;
 
 		default:
-			DP(NETIF_MSG_LINK, "Unsupported Serdes Net Interface "
-					   "0x%x\n", serdes_net_if);
+			DP(NETIF_MSG_LINK,
+			   "Unsupported Serdes Net Interface 0x%x\n",
+			   serdes_net_if);
 			return;
 		}
 	}
@@ -6098,8 +6099,8 @@ static int bnx2x_link_initialize(struct link_params *params,
 			if (phy_index == EXT_PHY2 &&
 			    (bnx2x_phy_selection(params) ==
 			     PORT_HW_CFG_PHY_SELECTION_FIRST_PHY)) {
-				DP(NETIF_MSG_LINK, "Not initializing"
-						" second phy\n");
+				DP(NETIF_MSG_LINK,
+				   "Not initializing second phy\n");
 				continue;
 			}
 			params->phy[phy_index].config_init(
@@ -6416,8 +6417,8 @@ int bnx2x_link_update(struct link_params *params, struct link_vars *vars)
 		 */
 		if (active_external_phy == EXT_PHY1) {
 			if (params->phy[EXT_PHY2].phy_specific_func) {
-				DP(NETIF_MSG_LINK, "Disabling TX on"
-						   " EXT_PHY2\n");
+				DP(NETIF_MSG_LINK,
+				   "Disabling TX on EXT_PHY2\n");
 				params->phy[EXT_PHY2].phy_specific_func(
 					&params->phy[EXT_PHY2],
 					params, DISABLE_TX);
@@ -7310,8 +7311,8 @@ static int bnx2x_8726_read_sfp_module_eeprom(struct bnx2x_phy *phy,
 	u16 val = 0;
 	u16 i;
 	if (byte_cnt > 16) {
-		DP(NETIF_MSG_LINK, "Reading from eeprom is"
-			    " is limited to 0xf\n");
+		DP(NETIF_MSG_LINK,
+		   "Reading from eeprom is limited to 0xf\n");
 		return -EINVAL;
 	}
 	/* Set the read command byte count */
@@ -7382,8 +7383,8 @@ static int bnx2x_warpcore_read_sfp_module_eeprom(struct bnx2x_phy *phy,
 					" addr %d, cnt %d\n",
 					addr, byte_cnt);*/
 	if (byte_cnt > 16) {
-		DP(NETIF_MSG_LINK, "Reading from eeprom is"
-			    " is limited to 16 bytes\n");
+		DP(NETIF_MSG_LINK,
+		   "Reading from eeprom is limited to 16 bytes\n");
 		return -EINVAL;
 	}
 
@@ -7412,8 +7413,8 @@ static int bnx2x_8727_read_sfp_module_eeprom(struct bnx2x_phy *phy,
 	u16 val, i;
 
 	if (byte_cnt > 16) {
-		DP(NETIF_MSG_LINK, "Reading from eeprom is"
-			    " is limited to 0xf\n");
+		DP(NETIF_MSG_LINK,
+		   "Reading from eeprom is limited to 0xf\n");
 		return -EINVAL;
 	}
 
@@ -7560,13 +7561,14 @@ static int bnx2x_get_edc_mode(struct bnx2x_phy *phy,
 			check_limiting_mode = 1;
 		} else if (copper_module_type &
 			SFP_EEPROM_FC_TX_TECH_BITMASK_COPPER_PASSIVE) {
-				DP(NETIF_MSG_LINK, "Passive Copper"
-					    " cable detected\n");
+				DP(NETIF_MSG_LINK,
+				   "Passive Copper cable detected\n");
 				*edc_mode =
 				      EDC_MODE_PASSIVE_DAC;
 		} else {
-			DP(NETIF_MSG_LINK, "Unknown copper-cable-"
-				     "type 0x%x !!!\n", copper_module_type);
+			DP(NETIF_MSG_LINK,
+			   "Unknown copper-cable-type 0x%x !!!\n",
+			   copper_module_type);
 			return -EINVAL;
 		}
 		break;
@@ -7604,8 +7606,8 @@ static int bnx2x_get_edc_mode(struct bnx2x_phy *phy,
 						 SFP_EEPROM_OPTIONS_ADDR,
 						 SFP_EEPROM_OPTIONS_SIZE,
 						 options) != 0) {
-			DP(NETIF_MSG_LINK, "Failed to read Option"
-				" field from module EEPROM\n");
+			DP(NETIF_MSG_LINK,
+			   "Failed to read Option field from module EEPROM\n");
 			return -EINVAL;
 		}
 		if ((options[0] & SFP_EEPROM_OPTIONS_LINEAR_RX_OUT_MASK))
@@ -7646,15 +7648,15 @@ static int bnx2x_verify_sfp_module(struct bnx2x_phy *phy,
 		   FEATURE_CONFIG_BC_SUPPORTS_OPT_MDL_VRFY) {
 		/* Use first phy request only in case of non-dual media*/
 		if (DUAL_MEDIA(params)) {
-			DP(NETIF_MSG_LINK, "FW does not support OPT MDL "
-			   "verification\n");
+			DP(NETIF_MSG_LINK,
+			   "FW does not support OPT MDL verification\n");
 			return -EINVAL;
 		}
 		cmd = DRV_MSG_CODE_VRFY_FIRST_PHY_OPT_MDL;
 	} else {
 		/* No support in OPT MDL detection */
-		DP(NETIF_MSG_LINK, "FW does not support OPT MDL "
-			  "verification\n");
+		DP(NETIF_MSG_LINK,
+		   "FW does not support OPT MDL verification\n");
 		return -EINVAL;
 	}
 
@@ -7705,8 +7707,9 @@ static int bnx2x_wait_for_sfp_module_initialized(struct bnx2x_phy *phy,
 	for (timeout = 0; timeout < 60; timeout++) {
 		if (bnx2x_read_sfp_module_eeprom(phy, params, 1, 1, &val)
 		    == 0) {
-			DP(NETIF_MSG_LINK, "SFP+ module initialization "
-				     "took %d ms\n", timeout * 5);
+			DP(NETIF_MSG_LINK,
+			   "SFP+ module initialization took %d ms\n",
+			   timeout * 5);
 			return 0;
 		}
 		msleep(5);
@@ -8478,8 +8481,8 @@ static int bnx2x_8726_config_init(struct bnx2x_phy *phy,
 	/* Set TX PreEmphasis if needed */
 	if ((params->feature_config_flags &
 	     FEATURE_CONFIG_OVERRIDE_PREEMPHASIS_ENABLED)) {
-		DP(NETIF_MSG_LINK, "Setting TX_CTRL1 0x%x,"
-			 "TX_CTRL2 0x%x\n",
+		DP(NETIF_MSG_LINK,
+		   "Setting TX_CTRL1 0x%x, TX_CTRL2 0x%x\n",
 			 phy->tx_preemphasis[0],
 			 phy->tx_preemphasis[1]);
 		bnx2x_cl45_write(bp, phy,
@@ -8763,8 +8766,8 @@ static void bnx2x_8727_handle_mod_abs(struct bnx2x_phy *phy,
 	if (mod_abs & (1<<8)) {
 
 		/* Module is absent */
-		DP(NETIF_MSG_LINK, "MOD_ABS indication "
-			    "show module is absent\n");
+		DP(NETIF_MSG_LINK,
+		   "MOD_ABS indication show module is absent\n");
 		phy->media_type = ETH_PHY_NOT_PRESENT;
 		/*
 		 * 1. Set mod_abs to detect next module
@@ -8791,8 +8794,8 @@ static void bnx2x_8727_handle_mod_abs(struct bnx2x_phy *phy,
 
 	} else {
 		/* Module is present */
-		DP(NETIF_MSG_LINK, "MOD_ABS indication "
-			    "show module is present\n");
+		DP(NETIF_MSG_LINK,
+		   "MOD_ABS indication show module is present\n");
 		/*
 		 * First disable transmitter, and if the module is ok, the
 		 * module_detection will enable it
@@ -8883,8 +8886,9 @@ static u8 bnx2x_8727_read_status(struct bnx2x_phy *phy,
 		if ((val1 & (1<<8)) == 0) {
 			if (!CHIP_IS_E1x(bp))
 				oc_port = BP_PATH(bp) + (params->port << 1);
-			DP(NETIF_MSG_LINK, "8727 Power fault has been detected"
-				       " on port %d\n", oc_port);
+			DP(NETIF_MSG_LINK,
+			   "8727 Power fault has been detected on port %d\n",
+			   oc_port);
 			netdev_err(bp->dev, "Error:  Power fault on Port %d has"
 					    " been detected and the power to "
 					    "that SFP+ module has been removed"
@@ -9660,8 +9664,8 @@ static u8 bnx2x_848xx_read_status(struct bnx2x_phy *phy,
 				MDIO_AN_REG_8481_EXPANSION_REG_RD_RW,
 				&legacy_status);
 
-		DP(NETIF_MSG_LINK, "Legacy speed status"
-			     " = 0x%x\n", legacy_status);
+		DP(NETIF_MSG_LINK, "Legacy speed status = 0x%x\n",
+		   legacy_status);
 		link_up = ((legacy_status & (1<<11)) == (1<<11));
 		if (link_up) {
 			legacy_speed = (legacy_status & (3<<9));
@@ -9679,9 +9683,10 @@ static u8 bnx2x_848xx_read_status(struct bnx2x_phy *phy,
 			else
 				vars->duplex = DUPLEX_HALF;
 
-			DP(NETIF_MSG_LINK, "Link is up in %dMbps,"
-				   " is_duplex_full= %d\n", vars->line_speed,
-				   (vars->duplex == DUPLEX_FULL));
+			DP(NETIF_MSG_LINK,
+			   "Link is up in %dMbps, is_duplex_full= %d\n",
+			   vars->line_speed,
+			   (vars->duplex == DUPLEX_FULL));
 			/* Check legacy speed AN resolution */
 			bnx2x_cl45_read(bp, phy,
 					MDIO_AN_DEVAD,
@@ -10251,9 +10256,10 @@ static u8 bnx2x_54618se_read_status(struct bnx2x_phy *phy,
 		} else /* Should not happen */
 			vars->line_speed = 0;
 
-		DP(NETIF_MSG_LINK, "Link is up in %dMbps,"
-			   " is_duplex_full= %d\n", vars->line_speed,
-			   (vars->duplex == DUPLEX_FULL));
+		DP(NETIF_MSG_LINK,
+		   "Link is up in %dMbps, is_duplex_full= %d\n",
+		   vars->line_speed,
+		   (vars->duplex == DUPLEX_FULL));
 
 		/* Check legacy speed AN resolution */
 		bnx2x_cl22_read(bp, phy,
@@ -11295,8 +11301,9 @@ static void bnx2x_phy_def_cfg(struct link_params *params,
 						      dev_info.
 			port_hw_config[params->port].speed_capability_mask));
 	}
-	DP(NETIF_MSG_LINK, "Default config phy idx %x cfg 0x%x speed_cap_mask"
-		       " 0x%x\n", phy_index, link_config, phy->speed_cap_mask);
+	DP(NETIF_MSG_LINK,
+	   "Default config phy idx %x cfg 0x%x speed_cap_mask 0x%x\n",
+	   phy_index, link_config, phy->speed_cap_mask);
 
 	phy->req_duplex = DUPLEX_FULL;
 	switch (link_config  & PORT_FEATURE_LINK_SPEED_MASK) {
@@ -12254,7 +12261,7 @@ void bnx2x_period_func(struct link_params *params, struct link_vars *vars)
 {
 	struct bnx2x *bp = params->bp;
 	if (!params) {
-		DP(NETIF_MSG_LINK, "Ininitliazed params !\n");
+		DP(NETIF_MSG_LINK, "Uninitialized params !\n");
 		return;
 	}
 	/* DP(NETIF_MSG_LINK, "Periodic called vars->phy_flags 0x%x speed 0x%x
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index 173b258..e899e87 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -4388,7 +4388,7 @@ static inline void bnx2x_handle_rx_mode_eqe(struct bnx2x *bp)
 static inline struct bnx2x_queue_sp_obj *bnx2x_cid_to_q_obj(
 	struct bnx2x *bp, u32 cid)
 {
-	DP(BNX2X_MSG_SP, "retrieving fp from cid %d", cid);
+	DP(BNX2X_MSG_SP, "retrieving fp from cid %d\n", cid);
 #ifdef BCM_CNIC
 	if (cid == BNX2X_FCOE_ETH_CID)
 		return &bnx2x_fcoe(bp, q_obj);
@@ -7176,7 +7176,7 @@ static inline void bnx2x_pf_q_prep_init(struct bnx2x *bp,
 	/* set maximum number of COSs supported by this queue */
 	init_params->max_cos = fp->max_cos;
 
-	DP(BNX2X_MSG_SP, "fp: %d setting queue params max cos to: %d",
+	DP(BNX2X_MSG_SP, "fp: %d setting queue params max cos to: %d\n",
 	    fp->index, init_params->max_cos);
 
 	/* set the context pointers queue object */
@@ -7209,7 +7209,7 @@ int bnx2x_setup_tx_only(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 
 	DP(BNX2X_MSG_SP, "preparing to send tx-only ramrod for connection:"
 			 "cos %d, primary cid %d, cid %d, "
-			 "client id %d, sp-client id %d, flags %lx",
+			 "client id %d, sp-client id %d, flags %lx\n",
 	   tx_index, q_params->q_obj->cids[FIRST_TX_COS_INDEX],
 	   q_params->q_obj->cids[tx_index], q_params->q_obj->cl_id,
 	   tx_only_params->gen_params.spcl_id, tx_only_params->flags);
@@ -7241,7 +7241,7 @@ int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 	int rc;
 	u8 tx_index;
 
-	DP(BNX2X_MSG_SP, "setting up queue %d", fp->index);
+	DP(BNX2X_MSG_SP, "setting up queue %d\n", fp->index);
 
 	/* reset IGU state skip FCoE L2 queue */
 	if (!IS_FCOE_FP(fp))
@@ -7265,7 +7265,7 @@ int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 		return rc;
 	}
 
-	DP(BNX2X_MSG_SP, "init complete");
+	DP(BNX2X_MSG_SP, "init complete\n");
 
 
 	/* Now move the Queue to the SETUP state... */
@@ -7319,7 +7319,7 @@ static int bnx2x_stop_queue(struct bnx2x *bp, int index)
 	struct bnx2x_queue_state_params q_params = {0};
 	int rc, tx_index;
 
-	DP(BNX2X_MSG_SP, "stopping queue %d cid %d", index, fp->cid);
+	DP(BNX2X_MSG_SP, "stopping queue %d cid %d\n", index, fp->cid);
 
 	q_params.q_obj = &fp->q_obj;
 	/* We want to wait for completion in this context */
@@ -7334,7 +7334,7 @@ static int bnx2x_stop_queue(struct bnx2x *bp, int index)
 		/* ascertain this is a normal queue*/
 		txdata = &fp->txdata[tx_index];
 
-		DP(BNX2X_MSG_SP, "stopping tx-only queue %d",
+		DP(BNX2X_MSG_SP, "stopping tx-only queue %d\n",
 							txdata->txq_index);
 
 		/* send halt terminate on tx-only connection */
@@ -10704,7 +10704,7 @@ static int __devinit bnx2x_init_one(struct pci_dev *pdev,
 		return rc;
 	}
 
-	DP(NETIF_MSG_DRV, "max_non_def_sbs %d", max_non_def_sbs);
+	DP(NETIF_MSG_DRV, "max_non_def_sbs %d\n", max_non_def_sbs);
 
 	rc = bnx2x_init_bp(bp);
 	if (rc)
@@ -10759,15 +10759,14 @@ static int __devinit bnx2x_init_one(struct pci_dev *pdev,
 
 	bnx2x_get_pcie_width_speed(bp, &pcie_width, &pcie_speed);
 
-	netdev_info(dev, "%s (%c%d) PCI-E x%d %s found at mem %lx,"
-	       " IRQ %d, ", board_info[ent->driver_data].name,
-	       (CHIP_REV(bp) >> 12) + 'A', (CHIP_METAL(bp) >> 4),
-	       pcie_width,
-	       ((!CHIP_IS_E2(bp) && pcie_speed == 2) ||
-		 (CHIP_IS_E2(bp) && pcie_speed == 1)) ?
-						"5GHz (Gen2)" : "2.5GHz",
-	       dev->base_addr, bp->pdev->irq);
-	pr_cont("node addr %pM\n", dev->dev_addr);
+	netdev_info(dev, "%s (%c%d) PCI-E x%d %s found at mem %lx, IRQ %d, node addr %pM\n",
+		    board_info[ent->driver_data].name,
+		    (CHIP_REV(bp) >> 12) + 'A', (CHIP_METAL(bp) >> 4),
+		    pcie_width,
+		    ((!CHIP_IS_E2(bp) && pcie_speed == 2) ||
+		     (CHIP_IS_E2(bp) && pcie_speed == 1)) ?
+		    "5GHz (Gen2)" : "2.5GHz",
+		    dev->base_addr, bp->pdev->irq, dev->dev_addr);
 
 	return 0;
 
diff --git a/drivers/net/bnx2x/bnx2x_sp.c b/drivers/net/bnx2x/bnx2x_sp.c
index b4d9c16..1f88c19 100644
--- a/drivers/net/bnx2x/bnx2x_sp.c
+++ b/drivers/net/bnx2x/bnx2x_sp.c
@@ -3045,8 +3045,8 @@ static int bnx2x_mcast_setup_e1h(struct bnx2x *bp,
 			break;
 
 		case BNX2X_MCAST_CMD_DEL:
-			DP(BNX2X_MSG_SP, "Invalidating multicast "
-					 "MACs configuration\n");
+			DP(BNX2X_MSG_SP,
+			   "Invalidating multicast MACs configuration\n");
 
 			/* clear the registry */
 			memset(o->registry.aprox_match.vec, 0,
@@ -4239,7 +4239,7 @@ static int bnx2x_queue_comp_cmd(struct bnx2x *bp,
 			 o->cids[BNX2X_PRIMARY_CID_INDEX], o->next_state);
 
 	if (o->next_tx_only)  /* print num tx-only if any exist */
-		DP(BNX2X_MSG_SP, "primary cid %d: num tx-only cons %d",
+		DP(BNX2X_MSG_SP, "primary cid %d: num tx-only cons %d\n",
 			   o->cids[BNX2X_PRIMARY_CID_INDEX], o->next_tx_only);
 
 	o->state = o->next_state;
@@ -4301,7 +4301,7 @@ static void bnx2x_q_fill_init_general_data(struct bnx2x *bp,
 		test_bit(BNX2X_Q_FLG_FCOE, flags) ?
 		LLFC_TRAFFIC_TYPE_FCOE : LLFC_TRAFFIC_TYPE_NW;
 
-	DP(BNX2X_MSG_SP, "flags: active %d, cos %d, stats en %d",
+	DP(BNX2X_MSG_SP, "flags: active %d, cos %d, stats en %d\n",
 	   gen_data->activate_flg, gen_data->cos, gen_data->statistics_en_flg);
 }
 
@@ -4454,7 +4454,7 @@ static void bnx2x_q_fill_setup_tx_only(struct bnx2x *bp,
 				  &data->tx,
 				  &cmd_params->params.tx_only.flags);
 
-	DP(BNX2X_MSG_SP, "cid %d, tx bd page lo %x hi %x",cmd_params->q_obj->cids[0],
+	DP(BNX2X_MSG_SP, "cid %d, tx bd page lo %x hi %x\n",cmd_params->q_obj->cids[0],
 	   data->tx.tx_bd_page_base.lo, data->tx.tx_bd_page_base.hi);
 }
 
@@ -4501,9 +4501,9 @@ static inline int bnx2x_q_init(struct bnx2x *bp,
 
 	/* Set CDU context validation values */
 	for (cos = 0; cos < o->max_cos; cos++) {
-		DP(BNX2X_MSG_SP, "setting context validation. cid %d, cos %d",
+		DP(BNX2X_MSG_SP, "setting context validation. cid %d, cos %d\n",
 				 o->cids[cos], cos);
-		DP(BNX2X_MSG_SP, "context pointer %p", init->cxts[cos]);
+		DP(BNX2X_MSG_SP, "context pointer %p\n", init->cxts[cos]);
 		bnx2x_set_ctx_validation(bp, init->cxts[cos], o->cids[cos]);
 	}
 
@@ -4592,7 +4592,7 @@ static inline int bnx2x_q_send_setup_tx_only(struct bnx2x *bp,
 		return -EINVAL;
 	}
 
-	DP(BNX2X_MSG_SP, "parameters received: cos: %d sp-id: %d",
+	DP(BNX2X_MSG_SP, "parameters received: cos: %d sp-id: %d\n",
 			 tx_only_params->gen_params.cos,
 			 tx_only_params->gen_params.spcl_id);
 
@@ -4603,7 +4603,7 @@ static inline int bnx2x_q_send_setup_tx_only(struct bnx2x *bp,
 	bnx2x_q_fill_setup_tx_only(bp, params, rdata);
 
 	DP(BNX2X_MSG_SP, "sending tx-only ramrod: cid %d, client-id %d,"
-			 "sp-client id %d, cos %d",
+			 "sp-client id %d, cos %d\n",
 			 o->cids[cid_index],
 			 rdata->general.client_id,
 			 rdata->general.sp_client_id, rdata->general.cos);
@@ -5160,8 +5160,9 @@ static inline int bnx2x_func_state_change_comp(struct bnx2x *bp,
 		return -EINVAL;
 	}
 
-	DP(BNX2X_MSG_SP, "Completing command %d for func %d, setting state to "
-			 "%d\n", cmd, BP_FUNC(bp), o->next_state);
+	DP(BNX2X_MSG_SP,
+	   "Completing command %d for func %d, setting state to %d\n",
+	   cmd, BP_FUNC(bp), o->next_state);
 
 	o->state = o->next_state;
 	o->next_state = BNX2X_F_STATE_MAX;
-- 
1.7.6.405.gc1be0

^ permalink raw reply related

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Robin Holt @ 2011-08-08 15:55 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300
In-Reply-To: <4E4001E1.3030508-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
...

> > On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> >> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>
> >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>> ...
> >>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>
> >>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>
> >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>> "clock_freq" while
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>
> >>>>>> documents "clock-frequencies"... :-(.
> >>>>>
> >>>>> You answered a different question that I was asking.  I was asking if
> >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>> as well.
> >>>>
> >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>
> >>>> For the P1010 we can sinmply derive the clock frequency from
> >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>> properties, etc. The clk implemetation might go into
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>
> >>>> or
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>
> >>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>
> >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>> you using?
> >>>
> >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>> platform, and then work from there for the flexcan stuff.  That patch
> >>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>> selection to determine is we are going to build the flexcan.c file.
> >>
> >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >> we should do it differently for PowerPC. 
> >>
> >> For mainline inclusion, you should provide your patches against the
> >> David Millers "net-next-2.6" tree, which already seems to have support
> >> for the P1010RDB:
> >>
> >>   config P1010_RDB
> >>         bool "Freescale P1010RDB"
> >>         select DEFAULT_UIMAGE
> >>         help
> >>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>
> >>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>           (DDR3/3L, SATA II, and PCI  Express).
> >>
> >>
> >>> Our contact with Freescale would prefer that I not post that patch until
> >>> we get the OK from freescale to do so since we received it under NDA.
> >>
> >> I don't think we currently need it. I prefer dropping and cleaning up
> >> the device tree stuff as it is not needed for the P1010 anyway. If a
> >> new processor shows up with enhanced capabilities requiring
> >> configuration via device tree, we or somebody else can provide a patch.
> >> Marc, what do you think?
> > 
> > ACK - The device tree bindings as in mainline's Documentation is a mess.
> > If the powerpc guys are happy with a clock interfaces based approach
> > somewhere in arch/ppc, I'm more than happy to remove:
> > - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> > 
> > - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> > - clock-frequency           /   a single clock-frequency attribute
> 
> In the "net-next-2.6" tree there is also:
> 
>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> 
> Especially the fsl,flexcan-clock-divider = <2>; might make people think,
> that they could set something else.

I am currently lost on the direction.  I think I need something like:

1) Patch 1/5 removing the "#include <mach/clock.h>" stays.
2) Patch 2/5 abstracting readl/writel stays.
3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.
4) Patch 4/5 I have not been given clear direction to not do it but have
   not gotten a favorable response.
5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c
   patch which determines the clock source not using the device tree
   information, but rather from some system register.  I need more detail
   on how this would work for both arm and powerpc.  How would I absract
   that or am I providing a flexcan_clk_* set of functions like I have
   in earlier generations of the patch set?

Thanks,
Robin

^ permalink raw reply

* Re: 802.3ad bonding brain damaged?
From: Eric Dumazet @ 2011-08-08 15:59 UTC (permalink / raw)
  To: David Lamparter; +Cc: Phillip Susi, netdev
In-Reply-To: <1312790234.7020.26.camel@arkology.n2.diac24.net>

Le lundi 08 août 2011 à 09:57 +0200, David Lamparter a écrit :
> Am Sonntag, den 07.08.2011, 15:52 -0400 schrieb Phillip Susi:
> > - From Documentation/networking/bonding.txt:
> > 
> > 	Additionally, the linux bonding 802.3ad implementation
> > 	distributes traffic by peer (using an XOR of MAC addresses),
> > 
> > This is counter to the entire point of 802.3ad. Distributing traffic by
> > hash of the destination address is poor mans load balancing for
> > systems not supporting 802.3ad. 
> 
> No, it isn't. 802.3ad/.1AX explicitly requires that no packet
> re-ordering may ever occur, which can only be guaranteed by enqueueing
> packets for one host on one TX interface. This behaviour is mandated by
> 802.1AX-2008 page 15 which reads:
> 
>   This standard does not mandate any particular distribution
>   algorithm(s); however, any distribution algorithm shall ensure that,
>   when frames are received by a Frame Collector as specified in 5.2.3,
>   the algorithm shall not cause
>   a) Misordering of frames that are part of any given conversation, or
>   b) Duplication of frames.
> | The above requirement to maintain frame ordering is met by ensuring
> | that all frames that compose a given conversation are transmitted on a
> | single link in the order that they are generated by the MAC Client;
>   hence, this requirement does not involve the addition (or
>   modification) of any information to the MAC frame, nor any buffering
>   or processing on the part of the corresponding Frame Collector in
>   order to reorder frames. This approach to the operation of the
>   distribution function permits a wide variety of distribution and load
>   balancing algorithms to be used, while also ensuring interoperability
>   between devices that adopt differing algorithms.
> 

It all depends on the definition of 'conversation'

Phillip assumed two (or more) TCP flows from machine A to machine B
could use two different links, while you assert they MUST use a single
link.




^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Robin Holt @ 2011-08-08 15:59 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300
In-Reply-To: <20110808155540.GD4926-sJ/iWh9BUns@public.gmane.org>

Ignore this.  We cross in the mail.  I will go back to your other thread.

Robin

On Mon, Aug 08, 2011 at 10:55:40AM -0500, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
> > On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> ...
> 
> > > On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> > >> On 08/08/2011 04:44 PM, Robin Holt wrote:
> > >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> > >>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> > >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> > >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> > >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> > >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > >>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> > >>>>>>>
> > >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> > >>>>>>> ...
> > >>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> > >>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> > >>>>>>>
> > >>>>>>> Should I go back to flexcan-v1.0 in my patches?
> > >>>>>>
> > >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> > >>>>>> "clock_freq" while
> > >>>>>>
> > >>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> > >>>>>>
> > >>>>>> documents "clock-frequencies"... :-(.
> > >>>>>
> > >>>>> You answered a different question that I was asking.  I was asking if
> > >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> > >>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> > >>>>> as well.
> > >>>>
> > >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> > >>>>
> > >>>> For the P1010 we can sinmply derive the clock frequency from
> > >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> > >>>> properties, etc. The clk implemetation might go into
> > >>>>
> > >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> > >>>>
> > >>>> or
> > >>>>
> > >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> > >>>>
> > >>>> And may depend on HAVE_CAN_FLEXCAN
> > >>>>
> > >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> > >>>> you using?
> > >>>
> > >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> > >>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> > >>> platform, and then work from there for the flexcan stuff.  That patch
> > >>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> > >>> that Kconfig bit, so I have tweaked it to be selected automatically
> > >>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> > >>> selection to determine is we are going to build the flexcan.c file.
> > >>
> > >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> > >> we should do it differently for PowerPC. 
> > >>
> > >> For mainline inclusion, you should provide your patches against the
> > >> David Millers "net-next-2.6" tree, which already seems to have support
> > >> for the P1010RDB:
> > >>
> > >>   config P1010_RDB
> > >>         bool "Freescale P1010RDB"
> > >>         select DEFAULT_UIMAGE
> > >>         help
> > >>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> > >>
> > >>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> > >>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> > >>           (DDR3/3L, SATA II, and PCI  Express).
> > >>
> > >>
> > >>> Our contact with Freescale would prefer that I not post that patch until
> > >>> we get the OK from freescale to do so since we received it under NDA.
> > >>
> > >> I don't think we currently need it. I prefer dropping and cleaning up
> > >> the device tree stuff as it is not needed for the P1010 anyway. If a
> > >> new processor shows up with enhanced capabilities requiring
> > >> configuration via device tree, we or somebody else can provide a patch.
> > >> Marc, what do you think?
> > > 
> > > ACK - The device tree bindings as in mainline's Documentation is a mess.
> > > If the powerpc guys are happy with a clock interfaces based approach
> > > somewhere in arch/ppc, I'm more than happy to remove:
> > > - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> > > 
> > > - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> > > - clock-frequency           /   a single clock-frequency attribute
> > 
> > In the "net-next-2.6" tree there is also:
> > 
> >  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> > 
> > Especially the fsl,flexcan-clock-divider = <2>; might make people think,
> > that they could set something else.
> 
> I am currently lost on the direction.  I think I need something like:
> 
> 1) Patch 1/5 removing the "#include <mach/clock.h>" stays.
> 2) Patch 2/5 abstracting readl/writel stays.
> 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.
> 4) Patch 4/5 I have not been given clear direction to not do it but have
>    not gotten a favorable response.
> 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c
>    patch which determines the clock source not using the device tree
>    information, but rather from some system register.  I need more detail
>    on how this would work for both arm and powerpc.  How would I absract
>    that or am I providing a flexcan_clk_* set of functions like I have
>    in earlier generations of the patch set?
> 
> Thanks,
> Robin
> _______________________________________________
> Socketcan-core mailing list
> Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> https://lists.berlios.de/mailman/listinfo/socketcan-core

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Wolfgang Grandegger @ 2011-08-08 16:03 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, Marc Kleine-Budde,
	U Bhaskar-B22300
In-Reply-To: <20110808155540.GD4926-sJ/iWh9BUns@public.gmane.org>

On 08/08/2011 05:55 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> ...
> 
>>> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>>
>>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>>> ...
>>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>>
>>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>>
>>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>>> "clock_freq" while
>>>>>>>>
>>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>>
>>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>>
>>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>>> as well.
>>>>>>
>>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>>
>>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>>> properties, etc. The clk implemetation might go into
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>>
>>>>>> or
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>>
>>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>>
>>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>>> you using?
>>>>>
>>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>>> selection to determine is we are going to build the flexcan.c file.
>>>>
>>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>>> we should do it differently for PowerPC. 
>>>>
>>>> For mainline inclusion, you should provide your patches against the
>>>> David Millers "net-next-2.6" tree, which already seems to have support
>>>> for the P1010RDB:
>>>>
>>>>   config P1010_RDB
>>>>         bool "Freescale P1010RDB"
>>>>         select DEFAULT_UIMAGE
>>>>         help
>>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>>
>>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>>
>>>>
>>>>> Our contact with Freescale would prefer that I not post that patch until
>>>>> we get the OK from freescale to do so since we received it under NDA.
>>>>
>>>> I don't think we currently need it. I prefer dropping and cleaning up
>>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>>> new processor shows up with enhanced capabilities requiring
>>>> configuration via device tree, we or somebody else can provide a patch.
>>>> Marc, what do you think?
>>>
>>> ACK - The device tree bindings as in mainline's Documentation is a mess.
>>> If the powerpc guys are happy with a clock interfaces based approach
>>> somewhere in arch/ppc, I'm more than happy to remove:
>>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
>>>
>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>>> - clock-frequency           /   a single clock-frequency attribute
>>
>> In the "net-next-2.6" tree there is also:
>>
>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>
>> Especially the fsl,flexcan-clock-divider = <2>; might make people think,
>> that they could set something else.
> 
> I am currently lost on the direction.  I think I need something like:
> 
> 1) Patch 1/5 removing the "#include <mach/clock.h>" stays.

OK.

> 2) Patch 2/5 abstracting readl/writel stays.

OK.

> 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.

Yep.

> 4) Patch 4/5 I have not been given clear direction to not do it but have
>    not gotten a favorable response.

Please drop this one for mainline.

> 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c

No, I just would prefer a more general place, e.g.:

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c

Furthermore you need patches to cleanup some DTS and platform files and
the Documentation.

Wolfgang.

^ permalink raw reply

* Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
From: Robin Holt @ 2011-08-08 16:08 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300,
	Marc Kleine-Budde
In-Reply-To: <4E4008BA.6030303-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

On Mon, Aug 08, 2011 at 06:03:06PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:55 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> > ...
> > 
> >>> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>>>
> >>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>>>> ...
> >>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>>>
> >>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>>>
> >>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>>>> "clock_freq" while
> >>>>>>>>
> >>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>>>
> >>>>>>>> documents "clock-frequencies"... :-(.
> >>>>>>>
> >>>>>>> You answered a different question that I was asking.  I was asking if
> >>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>>>> as well.
> >>>>>>
> >>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>>>
> >>>>>> For the P1010 we can sinmply derive the clock frequency from
> >>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>>>> properties, etc. The clk implemetation might go into
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>>>
> >>>>>> or
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>>>
> >>>>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>>>
> >>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>>>> you using?
> >>>>>
> >>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>>>> platform, and then work from there for the flexcan stuff.  That patch
> >>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>>>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>>>> selection to determine is we are going to build the flexcan.c file.
> >>>>
> >>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >>>> we should do it differently for PowerPC. 
> >>>>
> >>>> For mainline inclusion, you should provide your patches against the
> >>>> David Millers "net-next-2.6" tree, which already seems to have support
> >>>> for the P1010RDB:
> >>>>
> >>>>   config P1010_RDB
> >>>>         bool "Freescale P1010RDB"
> >>>>         select DEFAULT_UIMAGE
> >>>>         help
> >>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>>>
> >>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>>>           (DDR3/3L, SATA II, and PCI  Express).
> >>>>
> >>>>
> >>>>> Our contact with Freescale would prefer that I not post that patch until
> >>>>> we get the OK from freescale to do so since we received it under NDA.
> >>>>
> >>>> I don't think we currently need it. I prefer dropping and cleaning up
> >>>> the device tree stuff as it is not needed for the P1010 anyway. If a
> >>>> new processor shows up with enhanced capabilities requiring
> >>>> configuration via device tree, we or somebody else can provide a patch.
> >>>> Marc, what do you think?
> >>>
> >>> ACK - The device tree bindings as in mainline's Documentation is a mess.
> >>> If the powerpc guys are happy with a clock interfaces based approach
> >>> somewhere in arch/ppc, I'm more than happy to remove:
> >>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> >>>
> >>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >>> - clock-frequency           /   a single clock-frequency attribute
> >>
> >> In the "net-next-2.6" tree there is also:
> >>
> >>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>
> >> Especially the fsl,flexcan-clock-divider = <2>; might make people think,
> >> that they could set something else.
> > 
> > I am currently lost on the direction.  I think I need something like:
> > 
> > 1) Patch 1/5 removing the "#include <mach/clock.h>" stays.
> 
> OK.

Is that an Acked-by: or not?

> 
> > 2) Patch 2/5 abstracting readl/writel stays.
> 
> OK.

Is that an Acked-by: or not?

> 
> > 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.
> 
> Yep.

Done.

> 
> > 4) Patch 4/5 I have not been given clear direction to not do it but have
> >    not gotten a favorable response.
> 
> Please drop this one for mainline.

Done.

> 
> > 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c
> 
> No, I just would prefer a more general place, e.g.:
> 
>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> 
> Furthermore you need patches to cleanup some DTS and platform files and
> the Documentation.

So we would stay with the clk_* functions.  I assume clk_get() would
return NULL, clk_get_rate() would just return fsl_get_sys_freq() and
the other functions would do nothing.  Doesn't this really polute what
clk_* functions are supposed to do?  Aren't we making flexcan dictate
a different behavior for powerpc than for the arm (and possibly other)
architectures?

Thanks,
Robin

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox