[net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

This patch adds a generic driver for SJA1000 chips on the OpenFirmware
platform bus found on embedded PowerPC systems. You need a SJA1000 node
definition in your flattened device tree source (DTS) file similar to:

   can@3,100 {
           compatible = "nxp,sja1000";
           reg = <3 0x100 0x80>;
           clock-frequency = <8000000>;
           cdr-reg = <0x48>;
           ocr-reg = <0x0a>;
           interrupts = <2 0>;
           interrupt-parent = <&mpic>;
   };

See also Documentation/powerpc/dts-bindings/can/sja1000.txt.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 Documentation/powerpc/dts-bindings/can/sja1000.txt |   37 +++
 drivers/net/can/Kconfig                            |    9 
 drivers/net/can/sja1000/Makefile                   |    1 
 drivers/net/can/sja1000/sja1000_of_platform.c      |  215 +++++++++++++++++++++
 4 files changed, 262 insertions(+)

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================
--- net-next-2.6.orig/drivers/net/can/Kconfig
+++ net-next-2.6/drivers/net/can/Kconfig
@@ -51,6 +51,15 @@ config CAN_SJA1000_PLATFORM
 	  boards from Phytec (http://www.phytec.de) like the PCM027,
 	  PCM038.
 
+config CAN_SJA1000_OF_PLATFORM
+	depends on CAN_SJA1000 && PPC_OF
+	tristate "Generic OF Platform Bus based SJA1000 driver"
+	---help---
+	  This driver adds support for the SJA1000 chips connected to
+	  the OpenFirmware "platform bus" found on embedded systems with
+	  OpenFirmware bindings, e.g. if you have a PowerPC based system
+	  you may want to enable this option.
+
 config CAN_EMS_PCI
 	tristate "EMS CPC-PCI and CPC-PCIe Card"
 	depends on PCI && CAN_SJA1000
Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================
--- net-next-2.6.orig/drivers/net/can/sja1000/Makefile
+++ net-next-2.6/drivers/net/can/sja1000/Makefile
@@ -4,6 +4,7 @@
 
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
 obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
+obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += sja1000_of_platform.o
 obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o
 obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o
 
Index: net-next-2.6/drivers/net/can/sja1000/sja1000_of_platform.c
===================================================================
--- /dev/null
+++ net-next-2.6/drivers/net/can/sja1000/sja1000_of_platform.c
@@ -0,0 +1,215 @@
+/*
+ * Driver for SJA1000 CAN controllers on the OpenFirmware platform bus
+ *
+ * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+/* This is a generic driver for SJA1000 chips on the OpenFirmware platform
+ * bus found on embedded PowerPC systems. You need a SJA1000 CAN node
+ * definition in your flattened device tree source (DTS) file similar to:
+ *
+ *   can@3,100 {
+ *           compatible = "philips,sja1000";
+ *           reg = <3 0x100 0x80>;
+ *           clock-frequency = <8000000>;
+ *           cdr-reg = <0x48>;
+ *           ocr-reg = <0x0a>;
+ *           interrupts = <2 0>;
+ *           interrupt-parent = <&mpic>;
+ *   };
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+
+#include <linux/of_platform.h>
+#include <asm/prom.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME "sja1000_of_platform"
+
+MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>");
+MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the OF platform bus");
+MODULE_LICENSE("GPL v2");
+
+#define SJA1000_OFP_CAN_CLOCK  (16000000 / 2)
+
+#define SJA1000_OFP_OCR        OCR_TX0_PULLDOWN
+#define SJA1000_OFP_CDR        (CDR_CBP | CDR_CLK_OFF)
+
+static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg)
+{
+	return in_8((void __iomem *)(dev->base_addr + reg));
+}
+
+static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val)
+{
+	out_8((void __iomem *)(dev->base_addr + reg), val);
+}
+
+static int __devexit sja1000_ofp_remove(struct of_device *ofdev)
+{
+	struct net_device *dev = dev_get_drvdata(&ofdev->dev);
+	struct device_node *np = ofdev->node;
+	struct resource res;
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+
+	unregister_sja1000dev(dev);
+	free_sja1000dev(dev);
+	iounmap((void __iomem *)dev->base_addr);
+	irq_dispose_mapping(dev->irq);
+
+	of_address_to_resource(np, 0, &res);
+	release_mem_region(res.start, resource_size(&res));
+
+	return 0;
+}
+
+static int __devinit sja1000_ofp_probe(struct of_device *ofdev,
+				       const struct of_device_id *id)
+{
+	struct device_node *np = ofdev->node;
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+	struct resource res;
+	const u32 *prop;
+	int err, irq, res_size, prop_size;
+	void __iomem *base;
+
+	err = of_address_to_resource(np, 0, &res);
+	if (err) {
+		dev_err(&ofdev->dev, "invalid address\n");
+		return err;
+	}
+
+	res_size = resource_size(&res);
+
+	if (!request_mem_region(res.start, res_size, DRV_NAME)) {
+		dev_err(&ofdev->dev, "couldn't request %#x..%#x\n",
+			res.start, res.end);
+		return -EBUSY;
+	}
+
+	base = ioremap_nocache(res.start, res_size);
+	if (!base) {
+		dev_err(&ofdev->dev, "couldn't ioremap %#x..%#x\n",
+			res.start, res.end);
+		err = -ENOMEM;
+		goto exit_release_mem;
+	}
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq == NO_IRQ) {
+		dev_err(&ofdev->dev, "no irq found\n");
+		err = -ENODEV;
+		goto exit_unmap_mem;
+	}
+
+	dev = alloc_sja1000dev(0);
+	if (!dev) {
+		err = -ENOMEM;
+		goto exit_dispose_irq;
+	}
+
+	priv = netdev_priv(dev);
+
+	priv->read_reg = sja1000_ofp_read_reg;
+	priv->write_reg = sja1000_ofp_write_reg;
+
+	prop = of_get_property(np, "clock-frequency", &prop_size);
+	if (prop && (prop_size ==  sizeof(u32)))
+		priv->can.clock.freq = *prop;
+	else
+		priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK;
+
+	prop = of_get_property(np, "ocr-reg", &prop_size);
+	if (prop && (prop_size == sizeof(u32)))
+		priv->ocr = (u8)*prop;
+	else
+		priv->ocr = SJA1000_OFP_OCR;
+
+	prop = of_get_property(np, "cdr-reg", &prop_size);
+	if (prop && (prop_size == sizeof(u32)))
+		priv->cdr = (u8)*prop;
+	else
+		priv->cdr = SJA1000_OFP_CDR;
+
+	priv->irq_flags = IRQF_SHARED;
+
+	dev->irq = irq;
+	dev->base_addr = (unsigned long)base;
+
+	dev_info(&ofdev->dev,
+		 "base=0x%lx irq=%d clock=%d ocr=0x%02x cdr=0x%02x\n",
+		 dev->base_addr, dev->irq, priv->can.clock.freq,
+		 priv->ocr, priv->cdr);
+
+	dev_set_drvdata(&ofdev->dev, dev);
+	SET_NETDEV_DEV(dev, &ofdev->dev);
+
+	err = register_sja1000dev(dev);
+	if (err) {
+		dev_err(&ofdev->dev, "registering %s failed (err=%d)\n",
+			DRV_NAME, err);
+		goto exit_free_sja1000;
+	}
+
+	return 0;
+
+exit_free_sja1000:
+	free_sja1000dev(dev);
+exit_dispose_irq:
+	irq_dispose_mapping(irq);
+exit_unmap_mem:
+	iounmap(base);
+exit_release_mem:
+	release_mem_region(res.start, res_size);
+
+	return err;
+}
+
+static struct of_device_id __devinitdata sja1000_ofp_table[] = {
+	{.compatible = "philips,sja1000"},
+	{.compatible = "nxp,sja1000"},
+	{},
+};
+
+static struct of_platform_driver sja1000_ofp_driver = {
+	.owner = THIS_MODULE,
+	.name = DRV_NAME,
+	.probe = sja1000_ofp_probe,
+	.remove = __devexit_p(sja1000_ofp_remove),
+	.match_table = sja1000_ofp_table,
+};
+
+static int __init sja1000_ofp_init(void)
+{
+	return of_register_platform_driver(&sja1000_ofp_driver);
+}
+module_init(sja1000_ofp_init);
+
+static void __exit sja1000_ofp_exit(void)
+{
+	return of_unregister_platform_driver(&sja1000_ofp_driver);
+};
+module_exit(sja1000_ofp_exit);
Index: net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt
===================================================================
--- /dev/null
+++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt
@@ -0,0 +1,37 @@
+Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
+
+Required properties:
+
+- compatible : should be "nxp,sja1000".
+- reg : should specify the chip select, address offset and size used
+	for the chip depending on the bus it is connected to.
+- interrupts: property with a value describing the interrupt source
+	(number and sensitivity) for that device. The encoding depends
+	on the type of interrupt controller used.
+
+Optional properties:
+
+- interrupt-parent : the phandle for the interrupt controller that
+	services interrupts for that device.
+- clock-frequency : CAN system clock frequency in Hz, which is normally
+	half of the oscillator clock frequency. If not specified, a
+	default value of 8000000 (8 MHz) is used.
+- cdr-reg : value of the SJA1000 clock divider register according to
+	the SJA1000 data sheet. If not specified, a default value of
+	0x48 is used.
+- ocr-reg : value of the SJA1000 output control register according to
+	the SJA1000 data sheet. If not specified, a default value of
+	0x0a is used.
+
+Examples:
+
+can@3,100 {
+	compatible = "nxp,sja1000";
+	reg = <3 0x100 0x80>;
+	clock-frequency = <8000000>;
+	cdr-reg = <0x48>;
+	ocr-reg = <0x0a>;
+	interrupts = <2 0>;
+	interrupt-parent = <&mpic>;
+};
+

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Grant Likely]

Hi Wolfgang, thanks for the quick response.  Comments below...

On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt
> @@ -0,0 +1,37 @@
> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
> +
> +Required properties:
> +
> +- compatible : should be "nxp,sja1000".
> +- reg : should specify the chip select, address offset and size used
> +       for the chip depending on the bus it is connected to.
> +- interrupts: property with a value describing the interrupt source
> +       (number and sensitivity) for that device. The encoding depends
> +       on the type of interrupt controller used.

Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood
properties.  I don't think we need to keep boilerplate defining the
meaning every time a new binding is added.  (general musing; not an
ack or nack of this patch)

However, what should be defined is *what* the register range is (ie.
one tuple; location of device registers), and what the interrupts are
(ie. single tuple for device's irq line).  Granted this is a trivial
case, but in the case of devices with more than one address range or
irq line, the meaning of each tuple is critical information.  I think
it would be a good pattern to establish.

> +Optional properties:
> +
> +- interrupt-parent : the phandle for the interrupt controller that
> +       services interrupts for that device.

Thinking further; I wouldn't even mention "interrupt-parent" here.
Anyone working with this stuff must already understand irq routing.

> +- clock-frequency : CAN system clock frequency in Hz, which is normally
> +       half of the oscillator clock frequency. If not specified, a
> +       default value of 8000000 (8 MHz) is used.

A clock-frequency property typically refers to the bus clock
frequency.  Something like can-frequency would be better.

> +- cdr-reg : value of the SJA1000 clock divider register according to
> +       the SJA1000 data sheet. If not specified, a default value of
> +       0x48 is used.

Ewh.  The driver should be clueful enough to derive the clock divider
value given both the bus and can frequencies.  I don't like this
property.

> +- ocr-reg : value of the SJA1000 output control register according to
> +       the SJA1000 data sheet. If not specified, a default value of
> +       0x0a is used.

Ditto here; the binding should describe the usage mode; not the
register settings to get the usage mode.  What sort of settings will
the .dts author be writing here?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Grant Likely]

Hi Wolfgang, thanks for the quick response.  Comments below...

On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt
> @@ -0,0 +1,37 @@
> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
> +
> +Required properties:
> +
> +- compatible : should be "nxp,sja1000".
> +- reg : should specify the chip select, address offset and size used
> + =A0 =A0 =A0 for the chip depending on the bus it is connected to.
> +- interrupts: property with a value describing the interrupt source
> + =A0 =A0 =A0 (number and sensitivity) for that device. The encoding depe=
nds
> + =A0 =A0 =A0 on the type of interrupt controller used.

Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood
properties.  I don't think we need to keep boilerplate defining the
meaning every time a new binding is added.  (general musing; not an
ack or nack of this patch)

However, what should be defined is *what* the register range is (ie.
one tuple; location of device registers), and what the interrupts are
(ie. single tuple for device's irq line).  Granted this is a trivial
case, but in the case of devices with more than one address range or
irq line, the meaning of each tuple is critical information.  I think
it would be a good pattern to establish.

> +Optional properties:
> +
> +- interrupt-parent : the phandle for the interrupt controller that
> + =A0 =A0 =A0 services interrupts for that device.

Thinking further; I wouldn't even mention "interrupt-parent" here.
Anyone working with this stuff must already understand irq routing.

> +- clock-frequency : CAN system clock frequency in Hz, which is normally
> + =A0 =A0 =A0 half of the oscillator clock frequency. If not specified, a
> + =A0 =A0 =A0 default value of 8000000 (8 MHz) is used.

A clock-frequency property typically refers to the bus clock
frequency.  Something like can-frequency would be better.

> +- cdr-reg : value of the SJA1000 clock divider register according to
> + =A0 =A0 =A0 the SJA1000 data sheet. If not specified, a default value o=
f
> + =A0 =A0 =A0 0x48 is used.

Ewh.  The driver should be clueful enough to derive the clock divider
value given both the bus and can frequencies.  I don't like this
property.

> +- ocr-reg : value of the SJA1000 output control register according to
> + =A0 =A0 =A0 the SJA1000 data sheet. If not specified, a default value o=
f
> + =A0 =A0 =A0 0x0a is used.

Ditto here; the binding should describe the usage mode; not the
register settings to get the usage mode.  What sort of settings will
the .dts author be writing here?

Cheers,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Hi Grant,

Grant Likely wrote:
> Hi Wolfgang, thanks for the quick response.  Comments below...
> 
> On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt
>> @@ -0,0 +1,37 @@
>> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
>> +
>> +Required properties:
>> +
>> +- compatible : should be "nxp,sja1000".
>> +- reg : should specify the chip select, address offset and size used
>> +       for the chip depending on the bus it is connected to.
>> +- interrupts: property with a value describing the interrupt source
>> +       (number and sensitivity) for that device. The encoding depends
>> +       on the type of interrupt controller used.
> 
> Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood
> properties.  I don't think we need to keep boilerplate defining the
> meaning every time a new binding is added.  (general musing; not an
> ack or nack of this patch)

OK.

> However, what should be defined is *what* the register range is (ie.
> one tuple; location of device registers), and what the interrupts are
> (ie. single tuple for device's irq line).  Granted this is a trivial
> case, but in the case of devices with more than one address range or
> irq line, the meaning of each tuple is critical information.  I think
> it would be a good pattern to establish.

Sounds reasonable.

>> +Optional properties:
>> +
>> +- interrupt-parent : the phandle for the interrupt controller that
>> +       services interrupts for that device.
> 
> Thinking further; I wouldn't even mention "interrupt-parent" here.
> Anyone working with this stuff must already understand irq routing.

OK, I will remove it.

>> +- clock-frequency : CAN system clock frequency in Hz, which is normally
>> +       half of the oscillator clock frequency. If not specified, a
>> +       default value of 8000000 (8 MHz) is used.
> 
> A clock-frequency property typically refers to the bus clock
> frequency.  Something like can-frequency would be better.

Ah, right, but I'm also not happy with "can-frequency". The manual
speaks about the "internal clock", which is half of the external
oscillator frequency. Maybe "internal-clock-frequency" might be better.

>> +- cdr-reg : value of the SJA1000 clock divider register according to
>> +       the SJA1000 data sheet. If not specified, a default value of
>> +       0x48 is used.
> 
> Ewh.  The driver should be clueful enough to derive the clock divider
> value given both the bus and can frequencies.  I don't like this
> property.

The clock divider register controls the CLKOUT frequency for the
microcontroller another CAN controller and allows to deactivate the
CLKOUT pin. It's not used to configure the CAN bus frequency.

> 
>> +- ocr-reg : value of the SJA1000 output control register according to
>> +       the SJA1000 data sheet. If not specified, a default value of
>> +       0x0a is used.
> 
> Ditto here; the binding should describe the usage mode; not the
> register settings to get the usage mode.  What sort of settings will
> the .dts author be writing here?

Unfortunately, there are many:

clkout-frequency
bypass-comperator
tx1-output-on-rx-irq

#define OCR_MODE_BIPHASE  0x00
#define OCR_MODE_TEST     0x01
#define OCR_MODE_NORMAL   0x02
#define OCR_MODE_CLOCK    0x03

#define OCR_TX0_INVERT    0x04
#define OCR_TX0_PULLDOWN  0x08
#define OCR_TX0_PULLUP    0x10
#define OCR_TX0_PUSHPULL  0x18
#define OCR_TX1_INVERT    0x20
#define OCR_TX1_PULLDOWN  0x40
#define OCR_TX1_PULLUP    0x80
#define OCR_TX1_PUSHPULL  0xc0

I think implementing properties for each option is overkill.

Wolfgang.

> 

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Hi Grant,

Grant Likely wrote:
> Hi Wolfgang, thanks for the quick response.  Comments below...
> 
> On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt
>> @@ -0,0 +1,37 @@
>> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
>> +
>> +Required properties:
>> +
>> +- compatible : should be "nxp,sja1000".
>> +- reg : should specify the chip select, address offset and size used
>> +       for the chip depending on the bus it is connected to.
>> +- interrupts: property with a value describing the interrupt source
>> +       (number and sensitivity) for that device. The encoding depends
>> +       on the type of interrupt controller used.
> 
> Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood
> properties.  I don't think we need to keep boilerplate defining the
> meaning every time a new binding is added.  (general musing; not an
> ack or nack of this patch)

OK.

> However, what should be defined is *what* the register range is (ie.
> one tuple; location of device registers), and what the interrupts are
> (ie. single tuple for device's irq line).  Granted this is a trivial
> case, but in the case of devices with more than one address range or
> irq line, the meaning of each tuple is critical information.  I think
> it would be a good pattern to establish.

Sounds reasonable.

>> +Optional properties:
>> +
>> +- interrupt-parent : the phandle for the interrupt controller that
>> +       services interrupts for that device.
> 
> Thinking further; I wouldn't even mention "interrupt-parent" here.
> Anyone working with this stuff must already understand irq routing.

OK, I will remove it.

>> +- clock-frequency : CAN system clock frequency in Hz, which is normally
>> +       half of the oscillator clock frequency. If not specified, a
>> +       default value of 8000000 (8 MHz) is used.
> 
> A clock-frequency property typically refers to the bus clock
> frequency.  Something like can-frequency would be better.

Ah, right, but I'm also not happy with "can-frequency". The manual
speaks about the "internal clock", which is half of the external
oscillator frequency. Maybe "internal-clock-frequency" might be better.

>> +- cdr-reg : value of the SJA1000 clock divider register according to
>> +       the SJA1000 data sheet. If not specified, a default value of
>> +       0x48 is used.
> 
> Ewh.  The driver should be clueful enough to derive the clock divider
> value given both the bus and can frequencies.  I don't like this
> property.

The clock divider register controls the CLKOUT frequency for the
microcontroller another CAN controller and allows to deactivate the
CLKOUT pin. It's not used to configure the CAN bus frequency.

> 
>> +- ocr-reg : value of the SJA1000 output control register according to
>> +       the SJA1000 data sheet. If not specified, a default value of
>> +       0x0a is used.
> 
> Ditto here; the binding should describe the usage mode; not the
> register settings to get the usage mode.  What sort of settings will
> the .dts author be writing here?

Unfortunately, there are many:

clkout-frequency
bypass-comperator
tx1-output-on-rx-irq

#define OCR_MODE_BIPHASE  0x00
#define OCR_MODE_TEST     0x01
#define OCR_MODE_NORMAL   0x02
#define OCR_MODE_CLOCK    0x03

#define OCR_TX0_INVERT    0x04
#define OCR_TX0_PULLDOWN  0x08
#define OCR_TX0_PULLUP    0x10
#define OCR_TX0_PUSHPULL  0x18
#define OCR_TX1_INVERT    0x20
#define OCR_TX1_PULLDOWN  0x40
#define OCR_TX1_PULLUP    0x80
#define OCR_TX1_PUSHPULL  0xc0

I think implementing properties for each option is overkill.

Wolfgang.

> 

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Arnd Bergmann]

On Friday 22 May 2009, Wolfgang Grandegger wrote:
> This patch adds a generic driver for SJA1000 chips on the OpenFirmware
> platform bus found on embedded PowerPC systems. 

Nice driver!

> +static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg)
> +{
> +	return in_8((void __iomem *)(dev->base_addr + reg));
> +}
> +
> +static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val)
> +{
> +	out_8((void __iomem *)(dev->base_addr + reg), val);
> +}

Minor nitpicking: dev->base_addr should be defined as an __iomem pointer
so you can avoid the cast here and in the ioremap/iounmap path.

	Arnd <><

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Arnd Bergmann]

On Friday 22 May 2009, Wolfgang Grandegger wrote:
> This patch adds a generic driver for SJA1000 chips on the OpenFirmware
> platform bus found on embedded PowerPC systems. 

Nice driver!

> +static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg)
> +{
> +	return in_8((void __iomem *)(dev->base_addr + reg));
> +}
> +
> +static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val)
> +{
> +	out_8((void __iomem *)(dev->base_addr + reg), val);
> +}

Minor nitpicking: dev->base_addr should be defined as an __iomem pointer
so you can avoid the cast here and in the ioremap/iounmap path.

	Arnd <><

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Wolfgang Grandegger wrote:
> Hi Grant,
> 
> Grant Likely wrote:
>> Hi Wolfgang, thanks for the quick response.  Comments below...
>>
>> On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt
>>> @@ -0,0 +1,37 @@
>>> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
>>> +
>>> +Required properties:
>>> +
>>> +- compatible : should be "nxp,sja1000".
>>> +- reg : should specify the chip select, address offset and size used
>>> +       for the chip depending on the bus it is connected to.
>>> +- interrupts: property with a value describing the interrupt source
>>> +       (number and sensitivity) for that device. The encoding depends
>>> +       on the type of interrupt controller used.
>> Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood
>> properties.  I don't think we need to keep boilerplate defining the
>> meaning every time a new binding is added.  (general musing; not an
>> ack or nack of this patch)
> 
> OK.
> 
>> However, what should be defined is *what* the register range is (ie.
>> one tuple; location of device registers), and what the interrupts are
>> (ie. single tuple for device's irq line).  Granted this is a trivial
>> case, but in the case of devices with more than one address range or
>> irq line, the meaning of each tuple is critical information.  I think
>> it would be a good pattern to establish.
> 
> Sounds reasonable.
> 
>>> +Optional properties:
>>> +
>>> +- interrupt-parent : the phandle for the interrupt controller that
>>> +       services interrupts for that device.
>> Thinking further; I wouldn't even mention "interrupt-parent" here.
>> Anyone working with this stuff must already understand irq routing.
> 
> OK, I will remove it.
> 
>>> +- clock-frequency : CAN system clock frequency in Hz, which is normally
>>> +       half of the oscillator clock frequency. If not specified, a
>>> +       default value of 8000000 (8 MHz) is used.
>> A clock-frequency property typically refers to the bus clock
>> frequency.  Something like can-frequency would be better.
> 
> Ah, right, but I'm also not happy with "can-frequency". The manual
> speaks about the "internal clock", which is half of the external
> oscillator frequency. Maybe "internal-clock-frequency" might be better.
> 
>>> +- cdr-reg : value of the SJA1000 clock divider register according to
>>> +       the SJA1000 data sheet. If not specified, a default value of
>>> +       0x48 is used.
>> Ewh.  The driver should be clueful enough to derive the clock divider
>> value given both the bus and can frequencies.  I don't like this
>> property.
> 
> The clock divider register controls the CLKOUT frequency for the
> microcontroller another CAN controller and allows to deactivate the
> CLKOUT pin. It's not used to configure the CAN bus frequency.
> 
>>> +- ocr-reg : value of the SJA1000 output control register according to
>>> +       the SJA1000 data sheet. If not specified, a default value of
>>> +       0x0a is used.
>> Ditto here; the binding should describe the usage mode; not the
>> register settings to get the usage mode.  What sort of settings will
>> the .dts author be writing here?
> 
> Unfortunately, there are many:
> 
> clkout-frequency
> bypass-comperator
> tx1-output-on-rx-irq
> 
> #define OCR_MODE_BIPHASE  0x00
> #define OCR_MODE_TEST     0x01
> #define OCR_MODE_NORMAL   0x02
> #define OCR_MODE_CLOCK    0x03
> 
> #define OCR_TX0_INVERT    0x04
> #define OCR_TX0_PULLDOWN  0x08
> #define OCR_TX0_PULLUP    0x10
> #define OCR_TX0_PUSHPULL  0x18
> #define OCR_TX1_INVERT    0x20
> #define OCR_TX1_PULLDOWN  0x40
> #define OCR_TX1_PULLUP    0x80
> #define OCR_TX1_PUSHPULL  0xc0
> 
> I think implementing properties for each option is overkill.

Would the following more descriptive properties be OK?

  clock-out-frequency = <0>, // CLKOUT pin clock off
                      = <4000000>; // frequency on CLKOUT pin

  bypass-input-comparator; // allows to bypass the CAN input comparator.

  tx1-output-on-rx-irq;    // allows the TX1 output to be used as a
                           // dedicated RX interrupt output.

  output-control-mode = <0x0> // bi-phase output mode
                        <0x1> // test output mode
                        <0x2> // normal output mode (default)
			<0x3> // clock output mode

  output-pin-config = <0x01> // TX0 invert
                      <0x02> // TX0 pull-down
                      <0x04> // TX0 pull-up
                      <0x06> // TX0 push-pull
                      <0x08> // TX1 invert
                      <0x10> // TX1 pull-down
                      <0x20> // TX1 pull-up
                      <0x30> // TX1 push-pull

Wolfgang.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Arnd Bergmann wrote:
> On Friday 22 May 2009, Wolfgang Grandegger wrote:
>> This patch adds a generic driver for SJA1000 chips on the OpenFirmware
>> platform bus found on embedded PowerPC systems. 
> 
> Nice driver!
> 
>> +static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg)
>> +{
>> +	return in_8((void __iomem *)(dev->base_addr + reg));
>> +}
>> +
>> +static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val)
>> +{
>> +	out_8((void __iomem *)(dev->base_addr + reg), val);
>> +}
> 
> Minor nitpicking: dev->base_addr should be defined as an __iomem pointer
> so you can avoid the cast here and in the ioremap/iounmap path.

Here the member "base_addr" of "struct net_device" is used and it's not
up to me to change the type.

Wolfgang.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Arnd Bergmann wrote:
> On Friday 22 May 2009, Wolfgang Grandegger wrote:
>> This patch adds a generic driver for SJA1000 chips on the OpenFirmware
>> platform bus found on embedded PowerPC systems. 
> 
> Nice driver!
> 
>> +static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg)
>> +{
>> +	return in_8((void __iomem *)(dev->base_addr + reg));
>> +}
>> +
>> +static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val)
>> +{
>> +	out_8((void __iomem *)(dev->base_addr + reg), val);
>> +}
> 
> Minor nitpicking: dev->base_addr should be defined as an __iomem pointer
> so you can avoid the cast here and in the ioremap/iounmap path.

Here the member "base_addr" of "struct net_device" is used and it's not
up to me to change the type.

Wolfgang.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Arnd Bergmann]

On Saturday 23 May 2009, Wolfgang Grandegger wrote:
> Arnd Bergmann wrote:
> > 
> > Minor nitpicking: dev->base_addr should be defined as an __iomem pointer
> > so you can avoid the cast here and in the ioremap/iounmap path.
> 
> Here the member "base_addr" of "struct net_device" is used and it's not
> up to me to change the type.

Right, that makes sense. However, most drivers use the field to store the
physical address, not the iomap token. Maybe there should be a new field
in struct sja1000_priv for the virtual address, but that would be a change
to the base driver, not just to the OF portion.

Thanks,

	Arnd <><

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Grant Likely]

On Sat, May 23, 2009 at 10:44 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Wolfgang Grandegger wrote:
>> Grant Likely wrote:
>>>> +- clock-frequency : CAN system clock frequency in Hz, which is normally
>>>> +       half of the oscillator clock frequency. If not specified, a
>>>> +       default value of 8000000 (8 MHz) is used.
>>> A clock-frequency property typically refers to the bus clock
>>> frequency.  Something like can-frequency would be better.
>>
>> Ah, right, but I'm also not happy with "can-frequency". The manual
>> speaks about the "internal clock", which is half of the external
>> oscillator frequency. Maybe "internal-clock-frequency" might be better.

Would it be better to specify the external clock frequency, and the
driver know that internal freq is half that?  I ask because external
clock freq is a value the HW designer actually has control over.

>>>> +- cdr-reg : value of the SJA1000 clock divider register according to
>>>> +       the SJA1000 data sheet. If not specified, a default value of
>>>> +       0x48 is used.
>>> Ewh.  The driver should be clueful enough to derive the clock divider
>>> value given both the bus and can frequencies.  I don't like this
>>> property.
>>
>> The clock divider register controls the CLKOUT frequency for the
>> microcontroller another CAN controller and allows to deactivate the
>> CLKOUT pin. It's not used to configure the CAN bus frequency.
>>
>>>> +- ocr-reg : value of the SJA1000 output control register according to
>>>> +       the SJA1000 data sheet. If not specified, a default value of
>>>> +       0x0a is used.
>>> Ditto here; the binding should describe the usage mode; not the
>>> register settings to get the usage mode.  What sort of settings will
>>> the .dts author be writing here?
>>
>> Unfortunately, there are many:
>>
>> clkout-frequency
>> bypass-comperator
>> tx1-output-on-rx-irq
>>
>> #define OCR_MODE_BIPHASE  0x00
>> #define OCR_MODE_TEST     0x01
>> #define OCR_MODE_NORMAL   0x02
>> #define OCR_MODE_CLOCK    0x03
>>
>> #define OCR_TX0_INVERT    0x04
>> #define OCR_TX0_PULLDOWN  0x08
>> #define OCR_TX0_PULLUP    0x10
>> #define OCR_TX0_PUSHPULL  0x18
>> #define OCR_TX1_INVERT    0x20
>> #define OCR_TX1_PULLDOWN  0x40
>> #define OCR_TX1_PULLUP    0x80
>> #define OCR_TX1_PUSHPULL  0xc0
>>
>> I think implementing properties for each option is overkill.

Ugh, I see what you mean.

> Would the following more descriptive properties be OK?
>
>  clock-out-frequency = <0>, // CLKOUT pin clock off
>                      = <4000000>; // frequency on CLKOUT pin

Or how about CLKOUT pin off if the property isn't present?  Otherwise,
this looks okay.  BTW, I'd consider prefixing this with 'nxp,' or
'sja1000,' to protect the namespace.  clock-out-frequency sounds like
one of those names which could be commonly used in the future.  I'd so
the same for the other chip-specific properties too.

Segher, what's your opinion on this?

>  bypass-input-comparator; // allows to bypass the CAN input comparator.
>
>  tx1-output-on-rx-irq;    // allows the TX1 output to be used as a
>                           // dedicated RX interrupt output.
>
>  output-control-mode = <0x0> // bi-phase output mode
>                        <0x1> // test output mode
>                        <0x2> // normal output mode (default)
>                        <0x3> // clock output mode
>
>  output-pin-config = <0x01> // TX0 invert
>                      <0x02> // TX0 pull-down
>                      <0x04> // TX0 pull-up
>                      <0x06> // TX0 push-pull
>                      <0x08> // TX1 invert
>                      <0x10> // TX1 pull-down
>                      <0x20> // TX1 pull-up
>                      <0x30> // TX1 push-pull

hmmm; It is very chip specific and it is a lot of mucking around.  If
you prefix the property with the manufacturer name, then perhaps the
explicit register setting is okay.

Are TX0 & TX1 protocol pins or GPIOs?  If gpio, then maybe it is worth
the mucking about to then use the gpios binding to specify the pin
mode.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Grant Likely]

On Sat, May 23, 2009 at 10:44 AM, Wolfgang Grandegger <wg@grandegger.com> w=
rote:
> Wolfgang Grandegger wrote:
>> Grant Likely wrote:
>>>> +- clock-frequency : CAN system clock frequency in Hz, which is normal=
ly
>>>> + =A0 =A0 =A0 half of the oscillator clock frequency. If not specified=
, a
>>>> + =A0 =A0 =A0 default value of 8000000 (8 MHz) is used.
>>> A clock-frequency property typically refers to the bus clock
>>> frequency. =A0Something like can-frequency would be better.
>>
>> Ah, right, but I'm also not happy with "can-frequency". The manual
>> speaks about the "internal clock", which is half of the external
>> oscillator frequency. Maybe "internal-clock-frequency" might be better.

Would it be better to specify the external clock frequency, and the
driver know that internal freq is half that?  I ask because external
clock freq is a value the HW designer actually has control over.

>>>> +- cdr-reg : value of the SJA1000 clock divider register according to
>>>> + =A0 =A0 =A0 the SJA1000 data sheet. If not specified, a default valu=
e of
>>>> + =A0 =A0 =A0 0x48 is used.
>>> Ewh. =A0The driver should be clueful enough to derive the clock divider
>>> value given both the bus and can frequencies. =A0I don't like this
>>> property.
>>
>> The clock divider register controls the CLKOUT frequency for the
>> microcontroller another CAN controller and allows to deactivate the
>> CLKOUT pin. It's not used to configure the CAN bus frequency.
>>
>>>> +- ocr-reg : value of the SJA1000 output control register according to
>>>> + =A0 =A0 =A0 the SJA1000 data sheet. If not specified, a default valu=
e of
>>>> + =A0 =A0 =A0 0x0a is used.
>>> Ditto here; the binding should describe the usage mode; not the
>>> register settings to get the usage mode. =A0What sort of settings will
>>> the .dts author be writing here?
>>
>> Unfortunately, there are many:
>>
>> clkout-frequency
>> bypass-comperator
>> tx1-output-on-rx-irq
>>
>> #define OCR_MODE_BIPHASE =A00x00
>> #define OCR_MODE_TEST =A0 =A0 0x01
>> #define OCR_MODE_NORMAL =A0 0x02
>> #define OCR_MODE_CLOCK =A0 =A00x03
>>
>> #define OCR_TX0_INVERT =A0 =A00x04
>> #define OCR_TX0_PULLDOWN =A00x08
>> #define OCR_TX0_PULLUP =A0 =A00x10
>> #define OCR_TX0_PUSHPULL =A00x18
>> #define OCR_TX1_INVERT =A0 =A00x20
>> #define OCR_TX1_PULLDOWN =A00x40
>> #define OCR_TX1_PULLUP =A0 =A00x80
>> #define OCR_TX1_PUSHPULL =A00xc0
>>
>> I think implementing properties for each option is overkill.

Ugh, I see what you mean.

> Would the following more descriptive properties be OK?
>
> =A0clock-out-frequency =3D <0>, // CLKOUT pin clock off
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D <4000000>; // frequency on=
 CLKOUT pin

Or how about CLKOUT pin off if the property isn't present?  Otherwise,
this looks okay.  BTW, I'd consider prefixing this with 'nxp,' or
'sja1000,' to protect the namespace.  clock-out-frequency sounds like
one of those names which could be commonly used in the future.  I'd so
the same for the other chip-specific properties too.

Segher, what's your opinion on this?

> =A0bypass-input-comparator; // allows to bypass the CAN input comparator.
>
> =A0tx1-output-on-rx-irq; =A0 =A0// allows the TX1 output to be used as a
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 // dedicated RX inter=
rupt output.
>
> =A0output-control-mode =3D <0x0> // bi-phase output mode
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x1> // test output mode
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x2> // normal output mod=
e (default)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x3> // clock output mode
>
> =A0output-pin-config =3D <0x01> // TX0 invert
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x02> // TX0 pull-down
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x04> // TX0 pull-up
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x06> // TX0 push-pull
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x08> // TX1 invert
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x10> // TX1 pull-down
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x20> // TX1 pull-up
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0<0x30> // TX1 push-pull

hmmm; It is very chip specific and it is a lot of mucking around.  If
you prefix the property with the manufacturer name, then perhaps the
explicit register setting is okay.

Are TX0 & TX1 protocol pins or GPIOs?  If gpio, then maybe it is worth
the mucking about to then use the gpios binding to specify the pin
mode.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Arnd Bergmann wrote:
> On Saturday 23 May 2009, Wolfgang Grandegger wrote:
>> Arnd Bergmann wrote:
>>> Minor nitpicking: dev->base_addr should be defined as an __iomem pointer
>>> so you can avoid the cast here and in the ioremap/iounmap path.
>> Here the member "base_addr" of "struct net_device" is used and it's not
>> up to me to change the type.
> 
> Right, that makes sense. However, most drivers use the field to store the
> physical address, not the iomap token. Maybe there should be a new field
> in struct sja1000_priv for the virtual address, but that would be a change
> to the base driver, not just to the OF portion.

Is that common practice? If yes, I will add a member to store the
virtual address to struct sja1000_priv.

Wolfgang.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Arnd Bergmann wrote:
> On Saturday 23 May 2009, Wolfgang Grandegger wrote:
>> Arnd Bergmann wrote:
>>> Minor nitpicking: dev->base_addr should be defined as an __iomem pointer
>>> so you can avoid the cast here and in the ioremap/iounmap path.
>> Here the member "base_addr" of "struct net_device" is used and it's not
>> up to me to change the type.
> 
> Right, that makes sense. However, most drivers use the field to store the
> physical address, not the iomap token. Maybe there should be a new field
> in struct sja1000_priv for the virtual address, but that would be a change
> to the base driver, not just to the OF portion.

Is that common practice? If yes, I will add a member to store the
virtual address to struct sja1000_priv.

Wolfgang.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Grant Likely wrote:
> On Sat, May 23, 2009 at 10:44 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Wolfgang Grandegger wrote:
>>> Grant Likely wrote:
>>>>> +- clock-frequency : CAN system clock frequency in Hz, which is normally
>>>>> +       half of the oscillator clock frequency. If not specified, a
>>>>> +       default value of 8000000 (8 MHz) is used.
>>>> A clock-frequency property typically refers to the bus clock
>>>> frequency.  Something like can-frequency would be better.
>>> Ah, right, but I'm also not happy with "can-frequency". The manual
>>> speaks about the "internal clock", which is half of the external
>>> oscillator frequency. Maybe "internal-clock-frequency" might be better.
> 
> Would it be better to specify the external clock frequency, and the
> driver know that internal freq is half that?  I ask because external
> clock freq is a value the HW designer actually has control over.

I'm sharing your arguments: "external-clock-frequency". There is always
some confusion about external and internal clock frequency but the name
should make it clear.

>>>>> +- cdr-reg : value of the SJA1000 clock divider register according to
>>>>> +       the SJA1000 data sheet. If not specified, a default value of
>>>>> +       0x48 is used.
>>>> Ewh.  The driver should be clueful enough to derive the clock divider
>>>> value given both the bus and can frequencies.  I don't like this
>>>> property.
>>> The clock divider register controls the CLKOUT frequency for the
>>> microcontroller another CAN controller and allows to deactivate the
>>> CLKOUT pin. It's not used to configure the CAN bus frequency.
>>>
>>>>> +- ocr-reg : value of the SJA1000 output control register according to
>>>>> +       the SJA1000 data sheet. If not specified, a default value of
>>>>> +       0x0a is used.
>>>> Ditto here; the binding should describe the usage mode; not the
>>>> register settings to get the usage mode.  What sort of settings will
>>>> the .dts author be writing here?
>>> Unfortunately, there are many:
>>>
>>> clkout-frequency
>>> bypass-comperator
>>> tx1-output-on-rx-irq
>>>
>>> #define OCR_MODE_BIPHASE  0x00
>>> #define OCR_MODE_TEST     0x01
>>> #define OCR_MODE_NORMAL   0x02
>>> #define OCR_MODE_CLOCK    0x03
>>>
>>> #define OCR_TX0_INVERT    0x04
>>> #define OCR_TX0_PULLDOWN  0x08
>>> #define OCR_TX0_PULLUP    0x10
>>> #define OCR_TX0_PUSHPULL  0x18
>>> #define OCR_TX1_INVERT    0x20
>>> #define OCR_TX1_PULLDOWN  0x40
>>> #define OCR_TX1_PULLUP    0x80
>>> #define OCR_TX1_PUSHPULL  0xc0
>>>
>>> I think implementing properties for each option is overkill.
> 
> Ugh, I see what you mean.
> 
>> Would the following more descriptive properties be OK?
>>
>>  clock-out-frequency = <0>, // CLKOUT pin clock off
>>                      = <4000000>; // frequency on CLKOUT pin
> 
> Or how about CLKOUT pin off if the property isn't present?  Otherwise,

Yep, that will be the default anyhow.

> this looks okay.  BTW, I'd consider prefixing this with 'nxp,' or
> 'sja1000,' to protect the namespace.  clock-out-frequency sounds like
> one of those names which could be commonly used in the future.  I'd so
> the same for the other chip-specific properties too.

> Segher, what's your opinion on this?

I personally don't have a real preference.

>>  bypass-input-comparator; // allows to bypass the CAN input comparator.
>>
>>  tx1-output-on-rx-irq;    // allows the TX1 output to be used as a
>>                           // dedicated RX interrupt output.
>>
>>  output-control-mode = <0x0> // bi-phase output mode
>>                        <0x1> // test output mode
>>                        <0x2> // normal output mode (default)
>>                        <0x3> // clock output mode
>>
>>  output-pin-config = <0x01> // TX0 invert
>>                      <0x02> // TX0 pull-down
>>                      <0x04> // TX0 pull-up
>>                      <0x06> // TX0 push-pull
>>                      <0x08> // TX1 invert
>>                      <0x10> // TX1 pull-down
>>                      <0x20> // TX1 pull-up
>>                      <0x30> // TX1 push-pull
> 
> hmmm; It is very chip specific and it is a lot of mucking around.  If
> you prefix the property with the manufacturer name, then perhaps the
> explicit register setting is okay.

OK.

> Are TX0 & TX1 protocol pins or GPIOs?  If gpio, then maybe it is worth
> the mucking about to then use the gpios binding to specify the pin
> mode.

These are the output from the CAN output driver 0 or 1 to the physical
bus line. E.g., in normal output mode the CAN bit sequence is sent via
TX0 or TX1. From a non-hardware expert point of view, they must be
programmed properly to get the hardware to work.

Wolfgang.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Arnd Bergmann]

On Monday 25 May 2009, Wolfgang Grandegger wrote:
> > Right, that makes sense. However, most drivers use the field to store the
> > physical address, not the iomap token. Maybe there should be a new field
> > in struct sja1000_priv for the virtual address, but that would be a change
> > to the base driver, not just to the OF portion.
> 
> Is that common practice? If yes, I will add a member to store the
> virtual address to struct sja1000_priv.

I grepped through the network driver for usage of ->base_addr, and
it's somewhat inconsistent. The majority of the users use it for
a physical address, but there are also a few that use it for the
__iomem token.

Casts between unsigned long and qualified (__iomem, __user, const, ...)
pointers do not cause a warning, but can easily lead to bugs when
another user casts to an unqualified pointer.

	Arnd <><

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Arnd Bergmann]

On Monday 25 May 2009, Wolfgang Grandegger wrote:
> > Right, that makes sense. However, most drivers use the field to store the
> > physical address, not the iomap token. Maybe there should be a new field
> > in struct sja1000_priv for the virtual address, but that would be a change
> > to the base driver, not just to the OF portion.
> 
> Is that common practice? If yes, I will add a member to store the
> virtual address to struct sja1000_priv.

I grepped through the network driver for usage of ->base_addr, and
it's somewhat inconsistent. The majority of the users use it for
a physical address, but there are also a few that use it for the
__iomem token.

Casts between unsigned long and qualified (__iomem, __user, const, ...)
pointers do not cause a warning, but can easily lead to bugs when
another user casts to an unqualified pointer.

	Arnd <><

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[David Miller]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Date: Tue, 26 May 2009 10:10:30 +0100

> On Monday 25 May 2009, Wolfgang Grandegger wrote:
>> > Right, that makes sense. However, most drivers use the field to store the
>> > physical address, not the iomap token. Maybe there should be a new field
>> > in struct sja1000_priv for the virtual address, but that would be a change
>> > to the base driver, not just to the OF portion.
>> 
>> Is that common practice? If yes, I will add a member to store the
>> virtual address to struct sja1000_priv.
> 
> I grepped through the network driver for usage of ->base_addr, and
> it's somewhat inconsistent. The majority of the users use it for
> a physical address, but there are also a few that use it for the
> __iomem token.
> 
> Casts between unsigned long and qualified (__iomem, __user, const, ...)
> pointers do not cause a warning, but can easily lead to bugs when
> another user casts to an unqualified pointer.

It's such a baroque thing, there is no reason to set it at all if you
ask me.  It's only use is to allow ISA and similar primitive bus
devices to have their I/O ports changed via ifconfig.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 26 May 2009 10:10:30 +0100

> On Monday 25 May 2009, Wolfgang Grandegger wrote:
>> > Right, that makes sense. However, most drivers use the field to store the
>> > physical address, not the iomap token. Maybe there should be a new field
>> > in struct sja1000_priv for the virtual address, but that would be a change
>> > to the base driver, not just to the OF portion.
>> 
>> Is that common practice? If yes, I will add a member to store the
>> virtual address to struct sja1000_priv.
> 
> I grepped through the network driver for usage of ->base_addr, and
> it's somewhat inconsistent. The majority of the users use it for
> a physical address, but there are also a few that use it for the
> __iomem token.
> 
> Casts between unsigned long and qualified (__iomem, __user, const, ...)
> pointers do not cause a warning, but can easily lead to bugs when
> another user casts to an unqualified pointer.

It's such a baroque thing, there is no reason to set it at all if you
ask me.  It's only use is to allow ISA and similar primitive bus
devices to have their I/O ports changed via ifconfig.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Benjamin Herrenschmidt]

On Tue, 2009-05-26 at 10:10 +0100, Arnd Bergmann wrote:
> On Monday 25 May 2009, Wolfgang Grandegger wrote:
> > > Right, that makes sense. However, most drivers use the field to store the
> > > physical address, not the iomap token. Maybe there should be a new field
> > > in struct sja1000_priv for the virtual address, but that would be a change
> > > to the base driver, not just to the OF portion.
> > 
> > Is that common practice? If yes, I will add a member to store the
> > virtual address to struct sja1000_priv.
> 
> I grepped through the network driver for usage of ->base_addr, and
> it's somewhat inconsistent. The majority of the users use it for
> a physical address, but there are also a few that use it for the
> __iomem token.

In addition, iirc, it's not big enough to hold >32 bit physical
addresses on 32-bit platforms no ?

Ben.

> Casts between unsigned long and qualified (__iomem, __user, const, ...)
> pointers do not cause a warning, but can easily lead to bugs when
> another user casts to an unqualified pointer.
> 
> 	Arnd <><
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Benjamin Herrenschmidt]

On Tue, 2009-05-26 at 10:10 +0100, Arnd Bergmann wrote:
> On Monday 25 May 2009, Wolfgang Grandegger wrote:
> > > Right, that makes sense. However, most drivers use the field to store the
> > > physical address, not the iomap token. Maybe there should be a new field
> > > in struct sja1000_priv for the virtual address, but that would be a change
> > > to the base driver, not just to the OF portion.
> > 
> > Is that common practice? If yes, I will add a member to store the
> > virtual address to struct sja1000_priv.
> 
> I grepped through the network driver for usage of ->base_addr, and
> it's somewhat inconsistent. The majority of the users use it for
> a physical address, but there are also a few that use it for the
> __iomem token.

In addition, iirc, it's not big enough to hold >32 bit physical
addresses on 32-bit platforms no ?

Ben.

> Casts between unsigned long and qualified (__iomem, __user, const, ...)
> pointers do not cause a warning, but can easily lead to bugs when
> another user casts to an unqualified pointer.
> 
> 	Arnd <><
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Arnd Bergmann]

On Tuesday 26 May 2009, David Miller wrote:
> It's such a baroque thing, there is no reason to set it at all if you
> ask me.  It's only use is to allow ISA and similar primitive bus
> devices to have their I/O ports changed via ifconfig.

My original comment was about the fact that sja1000 was doing
dev->base_addr = (unsigned long)ioremap(phys_addr, size), I didn't
even think about SIOCGIFMAP and command line overrides, but that
surely makes it worse and the driver should be changed to
store the virtual register address in its private data structure.

drivers/net/fec.c seems to have the same problem, which manifests
in a number of ugly casts and direct pointer dereferences in places
where it should do writel() or out_be32().

	Arnd <><

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Sascha Hauer]

On Tue, May 26, 2009 at 10:42:05AM +0100, Arnd Bergmann wrote:
> On Tuesday 26 May 2009, David Miller wrote:
> > It's such a baroque thing, there is no reason to set it at all if you
> > ask me.  It's only use is to allow ISA and similar primitive bus
> > devices to have their I/O ports changed via ifconfig.
> 
> My original comment was about the fact that sja1000 was doing
> dev->base_addr = (unsigned long)ioremap(phys_addr, size), I didn't
> even think about SIOCGIFMAP and command line overrides, but that
> surely makes it worse and the driver should be changed to
> store the virtual register address in its private data structure.
> 
> drivers/net/fec.c seems to have the same problem, which manifests
> in a number of ugly casts and direct pointer dereferences in places
> where it should do writel() or out_be32().

Ack. I'll prepare a patch for fec.c. Internally the driver already uses
a void __iomem * and writel/readl in -next. There is only one usage
left.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Sascha Hauer]

On Tue, May 26, 2009 at 10:42:05AM +0100, Arnd Bergmann wrote:
> On Tuesday 26 May 2009, David Miller wrote:
> > It's such a baroque thing, there is no reason to set it at all if you
> > ask me.  It's only use is to allow ISA and similar primitive bus
> > devices to have their I/O ports changed via ifconfig.
> 
> My original comment was about the fact that sja1000 was doing
> dev->base_addr = (unsigned long)ioremap(phys_addr, size), I didn't
> even think about SIOCGIFMAP and command line overrides, but that
> surely makes it worse and the driver should be changed to
> store the virtual register address in its private data structure.
> 
> drivers/net/fec.c seems to have the same problem, which manifests
> in a number of ugly casts and direct pointer dereferences in places
> where it should do writel() or out_be32().

Ack. I'll prepare a patch for fec.c. Internally the driver already uses
a void __iomem * and writel/readl in -next. There is only one usage
left.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

David Miller wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Tue, 26 May 2009 10:10:30 +0100
> 
>> On Monday 25 May 2009, Wolfgang Grandegger wrote:
>>>> Right, that makes sense. However, most drivers use the field to store the
>>>> physical address, not the iomap token. Maybe there should be a new field
>>>> in struct sja1000_priv for the virtual address, but that would be a change
>>>> to the base driver, not just to the OF portion.
>>> Is that common practice? If yes, I will add a member to store the
>>> virtual address to struct sja1000_priv.
>> I grepped through the network driver for usage of ->base_addr, and
>> it's somewhat inconsistent. The majority of the users use it for
>> a physical address, but there are also a few that use it for the
>> __iomem token.
>>
>> Casts between unsigned long and qualified (__iomem, __user, const, ...)
>> pointers do not cause a warning, but can easily lead to bugs when
>> another user casts to an unqualified pointer.
> 
> It's such a baroque thing, there is no reason to set it at all if you
> ask me.  It's only use is to allow ISA and similar primitive bus
> devices to have their I/O ports changed via ifconfig.

OK, I see, there are good reasons not to (mis-)use dev->base_addr. I
will prepare a patch for the SJA1000 CAN drivers.

Wolfgang.

Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Wolfgang Grandegger]

Wolfgang Grandegger wrote:
> David Miller wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> Date: Tue, 26 May 2009 10:10:30 +0100
>>
>>> On Monday 25 May 2009, Wolfgang Grandegger wrote:
>>>>> Right, that makes sense. However, most drivers use the field to store the
>>>>> physical address, not the iomap token. Maybe there should be a new field
>>>>> in struct sja1000_priv for the virtual address, but that would be a change
>>>>> to the base driver, not just to the OF portion.
>>>> Is that common practice? If yes, I will add a member to store the
>>>> virtual address to struct sja1000_priv.
>>> I grepped through the network driver for usage of ->base_addr, and
>>> it's somewhat inconsistent. The majority of the users use it for
>>> a physical address, but there are also a few that use it for the
>>> __iomem token.
>>>
>>> Casts between unsigned long and qualified (__iomem, __user, const, ...)
>>> pointers do not cause a warning, but can easily lead to bugs when
>>> another user casts to an unqualified pointer.
>> It's such a baroque thing, there is no reason to set it at all if you
>> ask me.  It's only use is to allow ISA and similar primitive bus
>> devices to have their I/O ports changed via ifconfig.
> 
> OK, I see, there are good reasons not to (mis-)use dev->base_addr. I
> will prepare a patch for the SJA1000 CAN drivers.

I have just sent out a patch series fixing this issue and providing
a revised patch for the SJA1000 OF platform driver:

[net-next-2.6 PATCH 0/3] can: sja1000: misused netdev->base_addr and OF platform driver

Wolfgang.