LinuxLists.cc - [PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

[permalink] [raw]

Subject: Re: [PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

On 05/17/2012 10:59 PM, Federico Vaga wrote:
> Signed-off-by: Federico Vaga <[email protected]>
> Acked-by: Giancarlo Asnaghi <[email protected]>
> Cc: Alan Cox <[email protected]>
> ---
> drivers/net/can/Kconfig | 11 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/sta2x11_can.c | 1085 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1097 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/can/sta2x11_can.c

Thanks for your contribution. At a first glance, this driver looks
similar to the pch_can and the c_can driver. It seems that a C_CAN based
controller is used on that board as well. If that's true, it should be
handled by the C_CAN driver. To get ride of the obsolete pch_can driver,
I sent some time ago the patch "[RFC/PATCH] c_can: add driver for the
PCH CAN controller":

http://marc.info/?t=132991563600003&r=1&w=4

I could serve as base of a generic c_can_pci driver.

Wolfgang.

2012-05-26 08:36:33

[permalink] [raw]

Subject: Re: [PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

> Thanks for your contribution. At a first glance, this driver looks
> similar to the pch_can and the c_can driver. It seems that a C_CAN based
> controller is used on that board as well. If that's true, it should be
> handled by the C_CAN driver. To get ride of the obsolete pch_can driver,
> I sent some time ago the patch "[RFC/PATCH] c_can: add driver for the
> PCH CAN controller":
>
> http://marc.info/?t=132991563600003&r=1&w=4
>
> I could serve as base of a generic c_can_pci driver.

You are right, this is a C-CAN but it isn't specified on the board manual.
I compared the board manual with the Bosch C-CAN manual, and it is the same.
I'm working on a PCI module for this board, when it is ready we can think for
a generic c_can_pci

Thank you

--
Federico Vaga

2012-05-26 19:58:11

[permalink] [raw]

Subject: Re: [PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

On 05/26/2012 10:36 AM, Federico Vaga wrote:
>> Thanks for your contribution. At a first glance, this driver looks
>> similar to the pch_can and the c_can driver. It seems that a C_CAN based
>> controller is used on that board as well. If that's true, it should be
>> handled by the C_CAN driver. To get ride of the obsolete pch_can driver,
>> I sent some time ago the patch "[RFC/PATCH] c_can: add driver for the
>> PCH CAN controller":
>>
>> http://marc.info/?t=132991563600003&r=1&w=4
>>
>> I could serve as base of a generic c_can_pci driver.
>
> You are right, this is a C-CAN but it isn't specified on the board manual.
> I compared the board manual with the Bosch C-CAN manual, and it is the same.

Like with the Intel EG20T PCH CAN controller.

> I'm working on a PCI module for this board, when it is ready we can think for
> a generic c_can_pci

That would be nice, thanks. You might have realized that AnilKumar has
provided support for the D_CAN controllers to the C_CAN driver which
will show up mainline soon.

Wolfgang.

2012-06-04 13:31:19

[permalink] [raw]

Subject: generic module for c-can on pci

As suggested I developed a generic module for C-CAN
on PCI. Probably I will do some changes about our
specific board, but I think that the module is generic
enough.

2012-06-04 13:31:31

[permalink] [raw]

Subject: [PATCH RFC] c_can_pci: generic module for c_can on PCI

Signed-off-by: Federico Vaga <[email protected]>
Acked-by: Giancarlo Asnaghi <[email protected]>
Cc: Alan Cox <[email protected]>
---
drivers/net/can/c_can/Kconfig | 11 +-
drivers/net/can/c_can/Makefile | 1 +
drivers/net/can/c_can/c_can_pci.c | 221 +++++++++++++++++++++++++++++++++++++
3 files changed, 230 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/can/c_can/c_can_pci.c

diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
index ffb9773..74ef97d 100644
--- a/drivers/net/can/c_can/Kconfig
+++ b/drivers/net/can/c_can/Kconfig
@@ -2,14 +2,19 @@ menuconfig CAN_C_CAN
tristate "Bosch C_CAN devices"
depends on CAN_DEV && HAS_IOMEM

-if CAN_C_CAN
-
config CAN_C_CAN_PLATFORM
tristate "Generic Platform Bus based C_CAN driver"
+ depends on CAN_C_CAN
---help---
This driver adds support for the C_CAN chips connected to
the "platform bus" (Linux abstraction for directly to the
processor attached devices) which can be found on various
boards from ST Microelectronics (http://www.st.com)
like the SPEAr1310 and SPEAr320 evaluation boards.
-endif
+
+config CAN_C_CAN_PCI
+ tristate "Generic PCI Bus based C_CAN driver"
+ depends on CAN_C_CAN
+ ---help---
+ This driver adds support for the C_CAN chips connected to
+ the PCI bus.
diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile
index 9273f6d..ad1cc84 100644
--- a/drivers/net/can/c_can/Makefile
+++ b/drivers/net/can/c_can/Makefile
@@ -4,5 +4,6 @@

obj-$(CONFIG_CAN_C_CAN) += c_can.o
obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o
+obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o

ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
new file mode 100644
index 0000000..b635375
--- /dev/null
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -0,0 +1,221 @@
+/*
+ * Platform CAN bus driver for Bosch C_CAN controller
+ *
+ * Copyright (C) 2012 Federico Vaga <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/clk.h>
+#include <linux/pci.h>
+#include <linux/can/dev.h>
+
+#include "c_can.h"
+
+enum c_can_pci_reg_align {
+ C_CAN_REG_ALIGN_16,
+ C_CAN_REG_ALIGN_32,
+};
+
+struct c_can_pci_data {
+ unsigned int reg_align; /* Set the register alignment in the memory */
+ unsigned int freq; /* Set the frequency if clk is not usable */
+};
+
+/*
+ * 16-bit c_can registers can be arranged differently in the memory
+ * architecture of different implementations. For example: 16-bit
+ * registers can be aligned to a 16-bit boundary or 32-bit boundary etc.
+ * Handle the same by providing a common read/write interface.
+ */
+static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
+ void *reg)
+{
+ return readw(reg);
+}
+
+static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv,
+ void *reg, u16 val)
+{
+ writew(val, reg);
+}
+
+static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv,
+ void *reg)
+{
+ return readw(reg + (long)reg - (long)priv->regs);
+}
+
+static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
+ void *reg, u16 val)
+{
+ writew(val, reg + (long)reg - (long)priv->regs);
+}
+
+static int __devinit c_can_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data;
+ struct c_can_priv *priv;
+ struct net_device *dev;
+ void __iomem *addr;
+ struct clk *clk;
+ int ret;
+
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ dev_err(&pdev->dev, "pci_enable_device FAILED\n");
+ goto out;
+ }
+
+ ret = pci_request_regions(pdev, KBUILD_MODNAME);
+ if (ret) {
+ dev_err(&pdev->dev, "pci_request_regions FAILED\n");
+ goto out_disable_device;
+ }
+
+ pci_set_master(pdev);
+ pci_enable_msi(pdev);
+
+ addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
+ if (!addr) {
+ dev_err(&pdev->dev,
+ "device has no PCI memory resources, "
+ "failing adapter\n");
+ ret = -ENOMEM;
+ goto out_release_regions;
+ }
+
+ /* allocate the c_can device */
+ dev = alloc_c_can_dev();
+ if (!dev) {
+ ret = -ENOMEM;
+ goto out_iounmap;
+ }
+
+ priv = netdev_priv(dev);
+ pci_set_drvdata(pdev, dev);
+ SET_NETDEV_DEV(dev, &pdev->dev);
+
+ dev->irq = pdev->irq;
+ priv->regs = addr;
+
+ if (!c_can_pci_data->freq) {
+ /* get the appropriate clk */
+ clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(clk)) {
+ dev_err(&pdev->dev, "no clock defined\n");
+ ret = -ENODEV;
+ goto out_free_c_can;
+ }
+ priv->can.clock.freq = clk_get_rate(clk);
+ priv->priv = clk;
+ } else {
+ priv->can.clock.freq = c_can_pci_data->freq;
+ priv->priv = NULL;
+ }
+
+ switch (c_can_pci_data->reg_align) {
+ case C_CAN_REG_ALIGN_32:
+ priv->read_reg = c_can_pci_read_reg_aligned_to_32bit;
+ priv->write_reg = c_can_pci_write_reg_aligned_to_32bit;
+ break;
+ case C_CAN_REG_ALIGN_16:
+ default:
+ priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
+ priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
+ break;
+ }
+
+ ret = register_c_can_dev(dev);
+ if (ret) {
+ dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
+ KBUILD_MODNAME, ret);
+ goto out_free_clock;
+ }
+
+ dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
+ KBUILD_MODNAME, priv->regs, dev->irq);
+
+ return 0;
+
+out_free_clock:
+ if (!priv->priv)
+ clk_put(priv->priv);
+out_free_c_can:
+ pci_set_drvdata(pdev, NULL);
+ free_c_can_dev(dev);
+out_iounmap:
+ pci_iounmap(pdev, addr);
+out_release_regions:
+ pci_disable_msi(pdev);
+ pci_clear_master(pdev);
+ pci_release_regions(pdev);
+out_disable_device:
+ /*
+ * do not call pci_disable_device on sta2x11 because it
+ * break all other Bus masters on this EP
+ */
+ if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
+ pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
+ goto out;
+ pci_disable_device(pdev);
+out:
+ return ret;
+}
+
+static void __devexit c_can_pci_remove(struct pci_dev *pdev)
+{
+ struct net_device *dev = pci_get_drvdata(pdev);
+ struct c_can_priv *priv = netdev_priv(dev);
+
+ pci_set_drvdata(pdev, NULL);
+ free_c_can_dev(dev);
+ if (!priv->priv)
+ clk_put(priv->priv);
+ pci_iounmap(pdev, priv->regs);
+ pci_disable_msi(pdev);
+ pci_clear_master(pdev);
+ pci_release_regions(pdev);
+ /*
+ * do not call pci_disable_device on sta2x11 because it
+ * break all other Bus masters on this EP
+ */
+ if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
+ pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
+ return;
+ pci_disable_device(pdev);
+}
+
+static struct c_can_pci_data c_can_sta2x11= {
+ .reg_align = C_CAN_REG_ALIGN_32,
+ .freq = 52000000, /* 52 Mhz */
+};
+
+#define C_CAN_ID(_vend, _dev, _driverdata) { \
+ PCI_DEVICE(_vend, _dev), \
+ .driver_data = (unsigned long)&_driverdata, \
+}
+DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
+ C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
+ c_can_sta2x11),
+ {},
+};
+static struct pci_driver sta2x11_pci_driver = {
+ .name = KBUILD_MODNAME,
+ .id_table = c_can_pci_tbl,
+ .probe = c_can_pci_probe,
+ .remove = __devexit_p(c_can_pci_remove),
+};
+
+module_pci_driver(sta2x11_pci_driver);
+
+MODULE_AUTHOR("Federico Vaga <[email protected]>");
+MODULE_LICENSE("GPL V2");
+MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller");
+MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);
--
1.7.10.2

2012-06-04 14:05:04

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

On 06/04/2012 03:32 PM, Federico Vaga wrote:
> Signed-off-by: Federico Vaga <[email protected]>
> Acked-by: Giancarlo Asnaghi <[email protected]>
> Cc: Alan Cox <[email protected]>

Please port you driver to the recent c_can changes. Use the c_can branch
of the linux-can-next repo[1] as base for your work. You have to rework
the register access function. Please have a look if there are devm_
variants for the registration/mapping of the pci and clock.

[1] https://gitorious.org/linux-can/linux-can-next

More comments inline. Marc

> ---
> drivers/net/can/c_can/Kconfig | 11 +-
> drivers/net/can/c_can/Makefile | 1 +
> drivers/net/can/c_can/c_can_pci.c | 221 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+), 3 deletions(-)
> create mode 100644 drivers/net/can/c_can/c_can_pci.c
>
> diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
> index ffb9773..74ef97d 100644
> --- a/drivers/net/can/c_can/Kconfig
> +++ b/drivers/net/can/c_can/Kconfig
> @@ -2,14 +2,19 @@ menuconfig CAN_C_CAN
> tristate "Bosch C_CAN devices"
> depends on CAN_DEV && HAS_IOMEM
>
> -if CAN_C_CAN

please keep the if CAN_C_CAN...

> -
> config CAN_C_CAN_PLATFORM
> tristate "Generic Platform Bus based C_CAN driver"
> + depends on CAN_C_CAN

...then you don't have to add the depends on here.

> ---help---
> This driver adds support for the C_CAN chips connected to
> the "platform bus" (Linux abstraction for directly to the
> processor attached devices) which can be found on various
> boards from ST Microelectronics (http://www.st.com)
> like the SPEAr1310 and SPEAr320 evaluation boards.
> -endif

... Just move you pci driver inside the if...endif block...
> +
> +config CAN_C_CAN_PCI
> + tristate "Generic PCI Bus based C_CAN driver"
> + depends on CAN_C_CAN

...and remove the depends on CAN_C_CAN. You probably have to add a
depends on PCI.

> + ---help---
> + This driver adds support for the C_CAN chips connected to
> + the PCI bus.
> diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile
> index 9273f6d..ad1cc84 100644
> --- a/drivers/net/can/c_can/Makefile
> +++ b/drivers/net/can/c_can/Makefile
> @@ -4,5 +4,6 @@
>
> obj-$(CONFIG_CAN_C_CAN) += c_can.o
> obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o
> +obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o
>
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> new file mode 100644
> index 0000000..b635375
> --- /dev/null
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -0,0 +1,221 @@
> +/*
> + * Platform CAN bus driver for Bosch C_CAN controller
> + *
> + * Copyright (C) 2012 Federico Vaga <[email protected]>
> + *
^^^ double space :)

> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/clk.h>
> +#include <linux/pci.h>
> +#include <linux/can/dev.h>
> +
> +#include "c_can.h"
> +
> +enum c_can_pci_reg_align {
> + C_CAN_REG_ALIGN_16,
> + C_CAN_REG_ALIGN_32,
> +};
> +
> +struct c_can_pci_data {
> + unsigned int reg_align; /* Set the register alignment in the memory */
^^^^^^^^^^^^
use the enum you defined above.

> + unsigned int freq; /* Set the frequency if clk is not usable */
> +};
> +
> +/*
> + * 16-bit c_can registers can be arranged differently in the memory
> + * architecture of different implementations. For example: 16-bit
> + * registers can be aligned to a 16-bit boundary or 32-bit boundary etc.
> + * Handle the same by providing a common read/write interface.
> + */
> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
> + void *reg)
> +{
> + return readw(reg);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv,
> + void *reg, u16 val)
> +{
> + writew(val, reg);
> +}
> +
> +static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv,
> + void *reg)
> +{
> + return readw(reg + (long)reg - (long)priv->regs);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
> + void *reg, u16 val)
> +{
> + writew(val, reg + (long)reg - (long)priv->regs);
> +}
> +
> +static int __devinit c_can_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data;
> + struct c_can_priv *priv;
> + struct net_device *dev;
> + void __iomem *addr;
> + struct clk *clk;
> + int ret;
> +
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "pci_enable_device FAILED\n");
> + goto out;
> + }
> +
> + ret = pci_request_regions(pdev, KBUILD_MODNAME);
> + if (ret) {
> + dev_err(&pdev->dev, "pci_request_regions FAILED\n");
> + goto out_disable_device;
> + }
> +
> + pci_set_master(pdev);
> + pci_enable_msi(pdev);
> +
> + addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> + if (!addr) {
> + dev_err(&pdev->dev,
> + "device has no PCI memory resources, "
> + "failing adapter\n");
> + ret = -ENOMEM;
> + goto out_release_regions;
> + }
> +
> + /* allocate the c_can device */
> + dev = alloc_c_can_dev();
> + if (!dev) {
> + ret = -ENOMEM;
> + goto out_iounmap;
> + }
> +
> + priv = netdev_priv(dev);
> + pci_set_drvdata(pdev, dev);
> + SET_NETDEV_DEV(dev, &pdev->dev);
> +
> + dev->irq = pdev->irq;
> + priv->regs = addr;
> +
> + if (!c_can_pci_data->freq) {
> + /* get the appropriate clk */
> + clk = clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "no clock defined\n");
> + ret = -ENODEV;
> + goto out_free_c_can;
> + }
> + priv->can.clock.freq = clk_get_rate(clk);
> + priv->priv = clk;
> + } else {
> + priv->can.clock.freq = c_can_pci_data->freq;
> + priv->priv = NULL;
> + }
> +
> + switch (c_can_pci_data->reg_align) {
> + case C_CAN_REG_ALIGN_32:
> + priv->read_reg = c_can_pci_read_reg_aligned_to_32bit;
> + priv->write_reg = c_can_pci_write_reg_aligned_to_32bit;
> + break;
> + case C_CAN_REG_ALIGN_16:
> + default:
> + priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
> + priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
> + break;
> + }
> +
> + ret = register_c_can_dev(dev);
> + if (ret) {
> + dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> + KBUILD_MODNAME, ret);
> + goto out_free_clock;
> + }
> +
> + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> + KBUILD_MODNAME, priv->regs, dev->irq);
> +
> + return 0;
> +
> +out_free_clock:
> + if (!priv->priv)
^^^

looks fishy

> + clk_put(priv->priv);
> +out_free_c_can:
> + pci_set_drvdata(pdev, NULL);
> + free_c_can_dev(dev);
> +out_iounmap:
> + pci_iounmap(pdev, addr);
> +out_release_regions:
> + pci_disable_msi(pdev);
> + pci_clear_master(pdev);
> + pci_release_regions(pdev);
> +out_disable_device:
> + /*
> + * do not call pci_disable_device on sta2x11 because it
> + * break all other Bus masters on this EP
> + */
> + if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> + pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> + goto out;
> + pci_disable_device(pdev);
> +out:
> + return ret;
> +}
> +
> +static void __devexit c_can_pci_remove(struct pci_dev *pdev)
> +{
> + struct net_device *dev = pci_get_drvdata(pdev);
> + struct c_can_priv *priv = netdev_priv(dev);
> +
> + pci_set_drvdata(pdev, NULL);
> + free_c_can_dev(dev);
> + if (!priv->priv)
dito
> + clk_put(priv->priv);
> + pci_iounmap(pdev, priv->regs);
> + pci_disable_msi(pdev);
> + pci_clear_master(pdev);
> + pci_release_regions(pdev);
> + /*
> + * do not call pci_disable_device on sta2x11 because it
> + * break all other Bus masters on this EP
> + */
> + if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> + pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> + return;
> + pci_disable_device(pdev);
> +}
> +
> +static struct c_can_pci_data c_can_sta2x11= {
> + .reg_align = C_CAN_REG_ALIGN_32,
> + .freq = 52000000, /* 52 Mhz */
> +};
> +
> +#define C_CAN_ID(_vend, _dev, _driverdata) { \
> + PCI_DEVICE(_vend, _dev), \
> + .driver_data = (unsigned long)&_driverdata, \
> +}
> +DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
^^^^

static?

> + C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
> + c_can_sta2x11),
> + {},
> +};
> +static struct pci_driver sta2x11_pci_driver = {
> + .name = KBUILD_MODNAME,
> + .id_table = c_can_pci_tbl,
> + .probe = c_can_pci_probe,
> + .remove = __devexit_p(c_can_pci_remove),
> +};
> +
> +module_pci_driver(sta2x11_pci_driver);
> +
> +MODULE_AUTHOR("Federico Vaga <[email protected]>");
> +MODULE_LICENSE("GPL V2");

IIRC, the correct case is "GPL v2"

> +MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller");
> +MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);

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

Attachments:

signature.asc (262.00 B)
OpenPGP digital signature

2012-06-04 15:53:05

by Alan

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> +enum c_can_pci_reg_align {
> + C_CAN_REG_ALIGN_16,
> + C_CAN_REG_ALIGN_32,
> +};

Anythign wrong with

bool aligned32;

> +
> +struct c_can_pci_data {
> + unsigned int reg_align; /* Set the register alignment in the memory */

Not the enum .. indeed

> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
> + void *reg)

I'm a bit worried this function name might be too short ;)

> + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> + KBUILD_MODNAME, priv->regs, dev->irq);

dev_dbg

> + * do not call pci_disable_device on sta2x11 because it
> + * break all other Bus masters on this EP
> + */
> + if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> + pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> + goto out;

Is that the disabling or the dropping it into D3. We have a PCI quirk
flag for the latter. See "quirk_no_ata_d3". That will also avoid any
accidents elsewhere. Right now the quirk has "ata" in the name but the
ata is just historically because we had to quirk various disk controllers.

2012-06-04 16:42:23

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv
> > *priv, + void *reg)
>
> I'm a bit worried this function name might be too short ;)

I know :) I was inspired by the same function in c_can_platform.c

About these function I suggest to move them into c_can.c because they
are the same for c_can_platform.c and c_can_pci.c Then add a new field
c_can_priv->offset which can be used to shift the register offset
coherently with the memory alignment. Finally, remove c_can_priv-
>read_reg and c_can_priv->write_reg and use internal c_can.c function to
read and write registers.

static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
{
return readw(priv->base + (priv->regs[index] << priv->offset));
}
static void c_can_write_reg(struct c_can_priv *priv, enum reg index,
u16 val)
{
writew(val, priv->base + (priv->regs[index] << priv->offset));
}

If it's ok, I can made a patch for this in the next days.

> > + * do not call pci_disable_device on sta2x11 because it
> > + * break all other Bus masters on this EP
> > + */
> > + if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> > + pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> > + goto out;
>
> Is that the disabling or the dropping it into D3. We have a PCI quirk
> flag for the latter. See "quirk_no_ata_d3". That will also avoid any
> accidents elsewhere. Right now the quirk has "ata" in the name but the
> ata is just historically because we had to quirk various disk
> controllers.

We are investigating if this is still necessary on the current version
of the board.

--
Federico Vaga

2012-06-04 16:45:58

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> Anythign wrong with
>
> bool aligned32;

I personally think booleans are evil. But both this and the other
thing:

>> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
>> + void *reg)
>
> I'm a bit worried this function name might be too short ;)

come from the platform driver this is based on (I already blamed
federico offlist for not preserving authorship of the original file).

So, this file is mostly copied from the platform driver, which is a
duplication of code. A mandated duplication, given how the thing
is currently laid out: the c_can core driver exports functions that
the other two files are using (the platform and the new pci driver).

In my opinion, it would be much better to have one less layer and no
exports at all. The core driver should be a platform driver, and the
pci driver would just build platform data and register the platform
device.

Sure this isn't up to federico, who has the pci device but cannot
access any boards where the previous driver is used. What do the
maintainers think? I (or federico :) may propose a reshaping, if
the idea makes sense.

/alessandro, another user of the sta2x11 where c_can_pci lives

2012-06-05 03:43:24

[permalink] [raw]

Subject: RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> -----Original Message-----
> From: [email protected] [mailto:linux-can-
> [email protected]] On Behalf Of Federico Vaga
> Sent: Monday, June 04, 2012 10:16 PM
> To: Alan Cox
> Cc: Wolfgang Grandegger; Marc Kleine-Budde; Giancarlo ASNAGHI; Alan
> Cox; Alessandro Rubini; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
>
> > > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv
> > > *priv, + void *reg)
> >
> > I'm a bit worried this function name might be too short ;)
>
> I know :) I was inspired by the same function in c_can_platform.c

There was a purpose to keeping these long function names when I wrote
the c_can_platform driver initially. These were kept to support the
SoCs (even the flaky ones) which I could trace at that time and used C_CAN controllers
(e.g. Hynix, ST's SPEAr eMPUs, etc..) and had different register bank layouts.

In some of these SoC's the C_CAN registers which are essentially 16-bit or 32-bit
registers are aligned always to a 32-bit boundary (i.e. even a 16-bit register
is aligned to 32-bit boundary).

So, I had to implement two variants of the read/write reg routines. I am not sure your SoC implementation needs them.
If it does, I will categorize it as flaky as well :)

> About these function I suggest to move them into c_can.c because they
> are the same for c_can_platform.c and c_can_pci.c Then add a new field
> c_can_priv->offset which can be used to shift the register offset
> coherently with the memory alignment. Finally, remove c_can_priv-
> >read_reg and c_can_priv->write_reg and use internal c_can.c function
> to
> read and write registers.

See above. There was a reason for keeping these routines in c_can_platform.c
Simply put, every platform having a Bosch C_CAN module can have it's own implementation
of the bus (for example you use PCI) and register bank layout (16-bit or 32-bit aligned).

I would suggest to keep the same arrangement.

> static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
> {
> return readw(priv->base + (priv->regs[index] << priv->offset));
> }
> static void c_can_write_reg(struct c_can_priv *priv, enum reg index,
> u16 val)
> {
> writew(val, priv->base + (priv->regs[index] << priv->offset));
> }
>
>
> If it's ok, I can made a patch for this in the next days.
>

[snip..]

Regards,
Bhupesh

2012-06-05 11:16:07

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> In some of these SoC's the C_CAN registers which are essentially
> 16-bit or 32-bit registers are aligned always to a 32-bit boundary
> (i.e. even a 16-bit register is aligned to 32-bit boundary).
>
> So, I had to implement two variants of the read/write reg routines. I
> am not sure your SoC implementation needs them. If it does, I will
> categorize it as flaky as well :)

My implementation is align to 32, but I'm trying to make a generic PCI
wrapper (some other could be aligned to 16)

> See above. There was a reason for keeping these routines in
> c_can_platform.c Simply put, every platform having a Bosch C_CAN
> module can have it's own implementation of the bus (for example you
> use PCI) and register bank layout (16-bit or 32-bit aligned).

I don't understand the reason to keep these functions in
c_can_platform.c . Two generic read/write functions could be written
into c_can.c by using a shift value (0 if aligned to 16, 1 if aligned to
32) as I showed in the previous mail:

> > static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
> > {
> >
> > return readw(priv->base + (priv->regs[index] << priv->offset));
> >
> > }
> > static void c_can_write_reg(struct c_can_priv *priv, enum reg index,
> >
> > u16 val)
> >
> > {
> >
> > writew(val, priv->base + (priv->regs[index] << priv->offset));
> >
> > }

Every platform having a Bosch C_CAN/D_CAN can specify its shift value (0
or 1) and it's done.

--
Federico Vaga

2012-06-05 13:04:45

[permalink] [raw]

Subject: RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> -----Original Message-----
> From: Federico Vaga [mailto:[email protected]]
> Sent: Tuesday, June 05, 2012 4:49 PM
> To: Bhupesh SHARMA
> Cc: Alan Cox; Wolfgang Grandegger; Marc Kleine-Budde; Giancarlo
> ASNAGHI; Alan Cox; Alessandro Rubini; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
>
> > In some of these SoC's the C_CAN registers which are essentially
> > 16-bit or 32-bit registers are aligned always to a 32-bit boundary
> > (i.e. even a 16-bit register is aligned to 32-bit boundary).
> >
> > So, I had to implement two variants of the read/write reg routines. I
> > am not sure your SoC implementation needs them. If it does, I will
> > categorize it as flaky as well :)
>
> My implementation is align to 32, but I'm trying to make a generic PCI
> wrapper (some other could be aligned to 16)

So it means your implementation is also flaky and you are probably wasting HW memory
space while integrating the Bosch C_CAN module in your SoC :)

> > See above. There was a reason for keeping these routines in
> > c_can_platform.c Simply put, every platform having a Bosch C_CAN
> > module can have it's own implementation of the bus (for example you
> > use PCI) and register bank layout (16-bit or 32-bit aligned).
>
> I don't understand the reason to keep these functions in
> c_can_platform.c . Two generic read/write functions could be written
> into c_can.c by using a shift value (0 if aligned to 16, 1 if aligned
> to
> 32) as I showed in the previous mail:
>
> > > static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
> > > {
> > >
> > > return readw(priv->base + (priv->regs[index] << priv->offset));
> > >
> > > }
> > > static void c_can_write_reg(struct c_can_priv *priv, enum reg
> index,
> > >
> > > u16 val)
> > >
> > > {
> > >
> > > writew(val, priv->base + (priv->regs[index] << priv->offset));
> > >
> > > }
>
> Every platform having a Bosch C_CAN/D_CAN can specify its shift value
> (0
> or 1) and it's done.
>

I am not a big fan of adding platform specific flakes in any core file, that why we keep the platform
file separate from the core ones. But I will left Marc and Wolfgang to further comment on the same.

Regards,
Bhupesh

2012-06-05 13:14:18

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

>> My implementation is align to 32, but I'm trying to make a generic PCI
>> wrapper (some other could be aligned to 16)

> So it means your implementation is also flaky and you are probably
> wasting HW memory space while integrating the Bosch C_CAN module in
> your SoC :)

Then I may say _your_ implementation is flaky because it wastes one
bit in the address decoder and a lot of logic gates in the data
bus. It's normal to align registers at 32 bits, as it's simpler and
faster. Most SoCs have only 32-bit aligned registers, for a reason.

> I am not a big fan of adding platform specific flakes in any core
> file, that why we keep the platform file separate from the core
> ones.

A number of other drivers have a shift parameter, because it's very
common for the hardware integrator to feel free to choose the easiest
wiring for the device. The choice to keep the platform driver
separate from the core driver only adds complication in my opinion:
you need to export 4 symbols and yhen every user must duplicate code
(like federico is replicating theplatform driver in the pci driver).

I'd really prefer to have the core driver be a platform driver, and
the others just add platform data to describe how it is wired. That's
actually the reason why the platform bus exists.

> But I will left Marc and Wolfgang to further comment on the same.

I agree: let them decide.

/alessandro

2012-06-05 13:22:15

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

On 06/05/2012 03:13 PM, Alessandro Rubini wrote:
>>> My implementation is align to 32, but I'm trying to make a generic PCI
>>> wrapper (some other could be aligned to 16)
>
>> So it means your implementation is also flaky and you are probably
>> wasting HW memory space while integrating the Bosch C_CAN module in
>> your SoC :)
>
> Then I may say _your_ implementation is flaky because it wastes one
> bit in the address decoder and a lot of logic gates in the data
> bus. It's normal to align registers at 32 bits, as it's simpler and
> faster. Most SoCs have only 32-bit aligned registers, for a reason.
>
>> I am not a big fan of adding platform specific flakes in any core
>> file, that why we keep the platform file separate from the core
>> ones.
>
> A number of other drivers have a shift parameter, because it's very
> common for the hardware integrator to feel free to choose the easiest
> wiring for the device. The choice to keep the platform driver
> separate from the core driver only adds complication in my opinion:
> you need to export 4 symbols and yhen every user must duplicate code
> (like federico is replicating theplatform driver in the pci driver).
>
> I'd really prefer to have the core driver be a platform driver, and
> the others just add platform data to describe how it is wired. That's
> actually the reason why the platform bus exists.
>
>> But I will left Marc and Wolfgang to further comment on the same.
>
> I agree: let them decide.

I personally like the "pci device sets up a platform device" idea.

My question is, is this considered being a good practise?

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 |

Attachments:

signature.asc (262.00 B)
OpenPGP digital signature

2012-06-05 13:23:12

[permalink] [raw]

Subject: RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Tuesday, June 05, 2012 6:44 PM
> To: Bhupesh SHARMA
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Giancarlo ASNAGHI;
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
>
> >> My implementation is align to 32, but I'm trying to make a generic
> PCI
> >> wrapper (some other could be aligned to 16)
>
> > So it means your implementation is also flaky and you are probably
> > wasting HW memory space while integrating the Bosch C_CAN module in
> > your SoC :)
>
> Then I may say _your_ implementation is flaky because it wastes one
> bit in the address decoder and a lot of logic gates in the data
> bus. It's normal to align registers at 32 bits, as it's simpler and
> faster. Most SoCs have only 32-bit aligned registers, for a reason.

You missed my original point. I mentioned in my first mail itself, that I studied a
few SoCs integrating the C_CAN module from Bosch before writing the driver.
Not all have aligned their register space to a 32-bit boundary.
My platform driver still supports them. This _does_ not imply that our
SoC has/may have the same problem :)

Each SoC designer can have his/her own different view on this sort of implementation.
The platform driver was written to support both the implementations (SW is supposed
to support all sort of HW design constraints :) ).

> > I am not a big fan of adding platform specific flakes in any core
> > file, that why we keep the platform file separate from the core
> > ones.
>
> A number of other drivers have a shift parameter, because it's very
> common for the hardware integrator to feel free to choose the easiest
> wiring for the device. The choice to keep the platform driver
> separate from the core driver only adds complication in my opinion:
> you need to export 4 symbols and yhen every user must duplicate code
> (like federico is replicating theplatform driver in the pci driver).
>
> I'd really prefer to have the core driver be a platform driver, and
> the others just add platform data to describe how it is wired. That's
> actually the reason why the platform bus exists.
>
> > But I will left Marc and Wolfgang to further comment on the same.
>
> I agree: let them decide.

Sure..

Regards,
Bhupesh

2012-06-05 13:30:37

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> I personally like the "pci device sets up a platform device" idea.

Good. Than me or federico will submit a proposal.

> My question is, is this considered being a good practise?

I don't think there are many pci bridges around, but platform drivers
exists just for that reason: to be instantiated when you know how the
wiring ("platform") details. I.e., somebody registers the platform
device associated to the driver.

Sometimes the platform device is compiled in, sometimes it comes from
the device tree. I think it can come from PCI as well.

thanks
/alessandro, apologizing with Bhupesh Sharma for his tone in the previous mail

2012-06-05 15:13:15

by AnilKumar, Chimata

[permalink] [raw]

Subject: RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

Hi Alessandro/Federico,

On Tue, Jun 05, 2012 at 19:00:13, Alessandro Rubini wrote:
> > I personally like the "pci device sets up a platform device" idea.
>
> Good. Than me or federico will submit a proposal.
>

I am late to the discussion, is there any specific reason to maintain a
separate platform file (c_can_pci.c). I think 90% of the code is copied
from c_can_paltform.c, code changes will be less if you merge to existing
c_can platform driver. You can look at D_CAN integration to C_CAN driver.

[1] https://gitorious.org/linux-can/linux-can-next

Regards
AnilKumar-

2012-06-05 16:50:40

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> I am late to the discussion, is there any specific reason to maintain a
> separate platform file (c_can_pci.c).

Because it depends on pci and ifdef is bad.

> I think 90% of the code is copied from c_can_paltform.c, code
> changes will be less if you merge to existing c_can platform driver.

Yes, but then we need to ifdef around, which merges two bad files
into a single but worse file.

But since the only current user of c_can is the platform device, why
not merging the platform with the core and having pci just register a
platform device? The only problem I see is that we need cooperation,
because neither me nor federico have a c_can equipped board besides
the pci one.

thanks
/alessandro

2012-06-06 03:51:51

[permalink] [raw]

Subject: RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

Hi,

> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Tuesday, June 05, 2012 10:20 PM
> To: [email protected]
> Cc: [email protected]; Bhupesh SHARMA; [email protected];
> [email protected]; [email protected]; Giancarlo ASNAGHI;
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
>
> > I am late to the discussion, is there any specific reason to maintain
> a
> > separate platform file (c_can_pci.c).
>
> Because it depends on pci and ifdef is bad.
>
> > I think 90% of the code is copied from c_can_paltform.c, code
> > changes will be less if you merge to existing c_can platform driver.
>
> Yes, but then we need to ifdef around, which merges two bad files
> into a single but worse file.
>
> But since the only current user of c_can is the platform device, why
> not merging the platform with the core and having pci just register a
> platform device? The only problem I see is that we need cooperation,
> because neither me nor federico have a c_can equipped board besides
> the pci one.
>

I can see examples of where different platform files are present for SJA CAN controller
as well depending on the underlying bus being used: OpenFirmware, ISA, PCI, etc..,
whilst there is a single core file there as well 'sja1000.c'

[1] Kvaser PCI platform driver, using services exposed by sja1000 core:
http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/kvaser_pci.c

[2] EMS PCI platform driver, using services exposed by sja1000 core:
http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/ems_pci.c

[3] SJA1000 core:
http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/sja1000.c

Here each platform driver has its own version of register read/write routine implementation.
The C_CAN approach is similar to that used by SJA1000. Instead of merging the "platform with the core",
I would instead suggest to have two separate platform drivers (for each bus type) and invoke common
routines kept in say another file 'c_can_platform_common.c', thus insuring that there is no code
duplicity and we have a clean hierarchical structure as well. So we can have:
- Core file, c_can.c
- Common platform file, c_can_platform_common.c
- Platform file, c_can_platform.c, c_can_pci.c, etc..

This ensures that nothing breaks at the end of the existing C_CAN users and we have a clean
file structure as well.

Ofcourse, Wolfgang has a better idea of this structure, as he defined the same for SJA1000 and I
consulted with him on this, while he was reviewing my initial C_CAN patch set. I will let him and Marc
comment further on my proposal. Your comments are also most welcome :)

Regards,
Bhupesh

2012-06-11 13:18:26

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

How we proceed?
I submit my c_can_pci.c as a separated module, we create a
c_can_platform_common.c,
or we are thinking about a generic c_can.c as plaftorm driver?

--
Federico Vaga

2012-06-11 13:52:14

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

On 06/04/2012 06:45 PM, Alessandro Rubini wrote:
>> Anythign wrong with
>>
>> bool aligned32;
>
> I personally think booleans are evil. But both this and the other
> thing:
>
>>> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
>>> + void *reg)
>>
>> I'm a bit worried this function name might be too short ;)
>
> come from the platform driver this is based on (I already blamed
> federico offlist for not preserving authorship of the original file).
>
> So, this file is mostly copied from the platform driver, which is a
> duplication of code. A mandated duplication, given how the thing
> is currently laid out: the c_can core driver exports functions that
> the other two files are using (the platform and the new pci driver).
>
> In my opinion, it would be much better to have one less layer and no
> exports at all. The core driver should be a platform driver, and the
> pci driver would just build platform data and register the platform
> device.

Do you have examples for that approach? Not sure yet if it really saves
code and makes it more readable.

> Sure this isn't up to federico, who has the pci device but cannot
> access any boards where the previous driver is used. What do the
> maintainers think? I (or federico :) may propose a reshaping, if
> the idea makes sense.

I would suggest to provide the c_can_pci driver using the *current* API,
even if it's not optimal. Federicos patch then already looks quite good.
It should use the new register access methods introduced by the D_CAN
support patch, though.

Any further improvements to the device abstraction and a more consistent
handling of the platform data are welcome.

Wolfgang.

2012-06-11 14:09:48

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

Hi Federico,

here comes my late review. Mark and others have already commented and I
will focus on further improvements...

On 06/04/2012 03:32 PM, Federico Vaga wrote:

A few more words would be nice here.

> Signed-off-by: Federico Vaga <[email protected]>
> Acked-by: Giancarlo Asnaghi <[email protected]>
> Cc: Alan Cox <[email protected]>
> ---
> drivers/net/can/c_can/Kconfig | 11 +-
> drivers/net/can/c_can/Makefile | 1 +
> drivers/net/can/c_can/c_can_pci.c | 221 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+), 3 deletions(-)
> create mode 100644 drivers/net/can/c_can/c_can_pci.c
>
> diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
> index ffb9773..74ef97d 100644
> --- a/drivers/net/can/c_can/Kconfig
> +++ b/drivers/net/can/c_can/Kconfig
> @@ -2,14 +2,19 @@ menuconfig CAN_C_CAN
> tristate "Bosch C_CAN devices"
> depends on CAN_DEV && HAS_IOMEM
>
> -if CAN_C_CAN
> -

Please don't change unrelated things. It's done that way also in other
CAN subdirectories.

> config CAN_C_CAN_PLATFORM
> tristate "Generic Platform Bus based C_CAN driver"
> + depends on CAN_C_CAN
> ---help---
> This driver adds support for the C_CAN chips connected to
> the "platform bus" (Linux abstraction for directly to the
> processor attached devices) which can be found on various
> boards from ST Microelectronics (http://www.st.com)
> like the SPEAr1310 and SPEAr320 evaluation boards.
> -endif
> +
> +config CAN_C_CAN_PCI
> + tristate "Generic PCI Bus based C_CAN driver"
> + depends on CAN_C_CAN
> + ---help---
> + This driver adds support for the C_CAN chips connected to
> + the PCI bus.
> diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile
> index 9273f6d..ad1cc84 100644
> --- a/drivers/net/can/c_can/Makefile
> +++ b/drivers/net/can/c_can/Makefile
> @@ -4,5 +4,6 @@
>
> obj-$(CONFIG_CAN_C_CAN) += c_can.o
> obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o
> +obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o
>
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> new file mode 100644
> index 0000000..b635375
> --- /dev/null
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -0,0 +1,221 @@
> +/*
> + * Platform CAN bus driver for Bosch C_CAN controller

s /Platform/PCI/ ?

> + *
> + * Copyright (C) 2012 Federico Vaga <[email protected]>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/clk.h>
> +#include <linux/pci.h>
> +#include <linux/can/dev.h>
> +
> +#include "c_can.h"
> +
> +enum c_can_pci_reg_align {
> + C_CAN_REG_ALIGN_16,
> + C_CAN_REG_ALIGN_32,
> +};
> +
> +struct c_can_pci_data {
> + unsigned int reg_align; /* Set the register alignment in the memory */

Should be "enum c_can_pci_reg_align" here.

> + unsigned int freq; /* Set the frequency if clk is not usable */
> +};
> +
> +/*
> + * 16-bit c_can registers can be arranged differently in the memory
> + * architecture of different implementations. For example: 16-bit
> + * registers can be aligned to a 16-bit boundary or 32-bit boundary etc.
> + * Handle the same by providing a common read/write interface.
> + */
> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
> + void *reg)
> +{
> + return readw(reg);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv,
> + void *reg, u16 val)
> +{
> + writew(val, reg);
> +}
> +
> +static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv,
> + void *reg)
> +{
> + return readw(reg + (long)reg - (long)priv->regs);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
> + void *reg, u16 val)
> +{
> + writew(val, reg + (long)reg - (long)priv->regs);
> +}

This will look better with the new register access methods.

> +static int __devinit c_can_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data;
> + struct c_can_priv *priv;
> + struct net_device *dev;
> + void __iomem *addr;
> + struct clk *clk;
> + int ret;
> +
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "pci_enable_device FAILED\n");
> + goto out;
> + }
> +
> + ret = pci_request_regions(pdev, KBUILD_MODNAME);
> + if (ret) {
> + dev_err(&pdev->dev, "pci_request_regions FAILED\n");
> + goto out_disable_device;
> + }
> +
> + pci_set_master(pdev);
> + pci_enable_msi(pdev);
> +
> + addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> + if (!addr) {
> + dev_err(&pdev->dev,
> + "device has no PCI memory resources, "
> + "failing adapter\n");
> + ret = -ENOMEM;
> + goto out_release_regions;
> + }
> +
> + /* allocate the c_can device */
> + dev = alloc_c_can_dev();
> + if (!dev) {
> + ret = -ENOMEM;
> + goto out_iounmap;
> + }
> +
> + priv = netdev_priv(dev);
> + pci_set_drvdata(pdev, dev);
> + SET_NETDEV_DEV(dev, &pdev->dev);
> +
> + dev->irq = pdev->irq;
> + priv->regs = addr;
> +
> + if (!c_can_pci_data->freq) {
> + /* get the appropriate clk */
> + clk = clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "no clock defined\n");
> + ret = -ENODEV;
> + goto out_free_c_can;
> + }
> + priv->can.clock.freq = clk_get_rate(clk);
> + priv->priv = clk;
> + } else {
> + priv->can.clock.freq = c_can_pci_data->freq;
> + priv->priv = NULL;
> + }
> +
> + switch (c_can_pci_data->reg_align) {
> + case C_CAN_REG_ALIGN_32:
> + priv->read_reg = c_can_pci_read_reg_aligned_to_32bit;
> + priv->write_reg = c_can_pci_write_reg_aligned_to_32bit;
> + break;
> + case C_CAN_REG_ALIGN_16:
> + default:
> + priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
> + priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
> + break;
> + }
> +
> + ret = register_c_can_dev(dev);
> + if (ret) {
> + dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> + KBUILD_MODNAME, ret);
> + goto out_free_clock;
> + }
> +
> + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> + KBUILD_MODNAME, priv->regs, dev->irq);
> +
> + return 0;
> +
> +out_free_clock:
> + if (!priv->priv)
> + clk_put(priv->priv);
> +out_free_c_can:
> + pci_set_drvdata(pdev, NULL);
> + free_c_can_dev(dev);
> +out_iounmap:
> + pci_iounmap(pdev, addr);
> +out_release_regions:
> + pci_disable_msi(pdev);
> + pci_clear_master(pdev);
> + pci_release_regions(pdev);
> +out_disable_device:
> + /*
> + * do not call pci_disable_device on sta2x11 because it
> + * break all other Bus masters on this EP
> + */

Puh!

> + if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> + pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> + goto out;
> + pci_disable_device(pdev);
> +out:
> + return ret;
> +}
> +
> +static void __devexit c_can_pci_remove(struct pci_dev *pdev)
> +{
> + struct net_device *dev = pci_get_drvdata(pdev);
> + struct c_can_priv *priv = netdev_priv(dev);
> +
> + pci_set_drvdata(pdev, NULL);
> + free_c_can_dev(dev);

Should be moved a few line down.

> + if (!priv->priv)
> + clk_put(priv->priv);
> + pci_iounmap(pdev, priv->regs);
> + pci_disable_msi(pdev);
> + pci_clear_master(pdev);
> + pci_release_regions(pdev);
> + /*
> + * do not call pci_disable_device on sta2x11 because it
> + * break all other Bus masters on this EP
> + */
> + if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> + pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> + return;
> + pci_disable_device(pdev);
> +}
> +
> +static struct c_can_pci_data c_can_sta2x11= {
> + .reg_align = C_CAN_REG_ALIGN_32,
> + .freq = 52000000, /* 52 Mhz */
> +};
> +
> +#define C_CAN_ID(_vend, _dev, _driverdata) { \
> + PCI_DEVICE(_vend, _dev), \
> + .driver_data = (unsigned long)&_driverdata, \
> +}
> +DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
> + C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
> + c_can_sta2x11),
> + {},
> +};
> +static struct pci_driver sta2x11_pci_driver = {

Why do you not use a generic name here?

> + .name = KBUILD_MODNAME,
> + .id_table = c_can_pci_tbl,
> + .probe = c_can_pci_probe,
> + .remove = __devexit_p(c_can_pci_remove),
> +};
> +
> +module_pci_driver(sta2x11_pci_driver);
> +
> +MODULE_AUTHOR("Federico Vaga <[email protected]>");
> +MODULE_LICENSE("GPL V2");
> +MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller");
> +MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);

Thanks for your contribution.

Wolfgang.

2012-06-11 14:21:47

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

On 06/11/2012 03:18 PM, Federico Vaga wrote:
> How we proceed?
> I submit my c_can_pci.c as a separated module, we create a
> c_can_platform_common.c,
> or we are thinking about a generic c_can.c as plaftorm driver?

I would accept your patch with the remaining fixes especially the new
register access methods introduced by the D_CAN support patch recently.

Any further improvements to the device abstraction and a more consistent
handling of the platform data or register access should be addressed by
sub-sequent patches.

Wolfgang.

2012-06-11 14:23:35

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

>> In my opinion, it would be much better to have one less layer and no
>> exports at all. The core driver should be a platform driver, and the
>> pci driver would just build platform data and register the platform
>> device.
>
> Do you have examples for that approach? Not sure yet if it really saves
> code and makes it more readable.

Maybe the physmap mtd driver is a good example. Everybody's using it
(but not from PCI). I found drivers/pcmcia/bcm63xx_pcmcia.c that
registers a platform driver from a pci probe function, but I'm sure
there are other ones.

OTOH, I have another example of how not to do stuff, but I won't point
fingers now (it's not a CAN thing).

I just think the platform bus is there just for this reason: to provide
data to a generic driver, without module dependencies and such stuff.

> I would suggest to provide the c_can_pci driver using the *current* API,
> even if it's not optimal. Federicos patch then already looks quite good.
> It should use the new register access methods introduced by the D_CAN
> support patch, though.

Great. When it's in I'll show my proposal as an RFC patch, as time permits,
so we'll see if it's better or not.

> Any further improvements to the device abstraction and a more consistent
> handling of the platform data are welcome.

Good to know, thanks a lot

/alessandro

2012-06-12 14:22:39

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

> > +out_free_clock:
> > + if (!priv->priv)
>
> ^^^
>
> looks fishy

Also c_can_platform.c use priv->priv when it needs to get clk. I can add
a comment to specify what the statement do.

--
Federico Vaga

2012-06-12 14:47:06

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

On 06/12/2012 04:25 PM, Federico Vaga wrote:
>>> +out_free_clock:
>>> + if (!priv->priv)
>>
>> ^^^
>>
>> looks fishy
>
> Also c_can_platform.c use priv->priv when it needs to get clk. I can add
> a comment to specify what the statement do.

> +out_free_clock:
> + if (!priv->priv)
> + clk_put(priv->priv);

Why do you call clk_put on priv->priv, if priv->priv is NULL?

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 |

Attachments:

signature.asc (262.00 B)
OpenPGP digital signature

2012-06-12 14:49:47