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
 ...
[ message continues ]

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


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

Thinking further; I wouldn't even mention "interrupt-parent" here.

A clock-frequency property typically refers to the bus clock

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

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

[ message continues ]

Hi Grant,





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

The clock divider register controls the CLKOUT frequency for the
microcontroller another CAN controller and allows to deactivate the

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.


--

[ message continues ]

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

[ message continues ]

Would it be better to specify the external clock frequency, and the
driver know that internal freq is half that?  I ask because external


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.


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

[ message continues ]

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




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.

--

[ message continues ]

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

[ message continues ]

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

Wolfgang.
--

[ message continues ]

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

[ message continues ]

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

Wolfgang.
--

[ message continues ]

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

--

[ message continues ]

From: Arnd Bergmann <arnd@arndb.de>

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

[ message continues ]

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

[ message continues ]

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

[ message continues ]

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

[ message continues ]

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

[ message continues ]

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


--

[ message continues ]