Hi Oliver, this is not a special Peak problem, the behavior can hit all SJA1000 chips conected to fast hardware busses.
When the command register is written it needs a little bit of time - about 125 nsec - to digest the command. 125 nsecs are a long time for modern processors. (see SJA1000 manual, 6.4.4) If the SJA1000 is used on a single core machine the driver's code is solely interrupted by the same core. In this scenario the switch into the interrupt handler may take the necessary time. But if the same scenario runs on a multicore machine it may be that the driver's non-interrupt-handler code runs in parallel on one core and the driver's interrupt handler code runs on another core. If both code paths access the SJA1000's command register it can happen that a second command register access hits a bus cycle later after the first command register access. With PCI this may be down to 33 nsec. Then it seems that the last register access never had happend. So you need some mechanism to prevent this situation. I've done it with guarded_write() in my driver's code. I appreciate if you adopt the socketcan's SJA1000 code, so I need not to worry about this when I link this code part to future drivers. Kind regards, Klaus Am Freitag, 7. Mai 2010, um 17:26:47 schrieben Sie: > On 07.05.2010 12:29, Matthias Fuchs wrote: > > Hi Oliver, > > > > we have many many SJA1000 based boards and I never heard of a general > > problem with back to back writes to the command register. Actually I > > won't invent when patching the PEAK driver that way but please don't do > > so with the other SJA1000 boards :-) > > > > Also I did not see the situation where back-to-back writes to the command > > register happen. > > Hi Matthias, > > thanks for your opinion on this topic. > > So far i also did not see this kind of problems. But the shown patch fixed > the problem of the write stalls with the BerliOS peak_pci.c driver in the > setup with the PEAK PCI card of my colleague. Maybe it is a timing issue > with the PCI adapter chip on the PEAK PCI card. > > Maybe Klaus can give us a better insight, why the special treatment was > only necessary for the COMMAND register ?!? > > Regards, > Oliver > > > On Wednesday 28 April 2010 20:10, Oliver Hartkopp wrote: > >> Hello Wolfgang, > >> > >> today i got the information from a colleague from IAV regarding the > >> peak_pci driver that the sending traffic stopped reproducible on a SMP > >> system with a 2.6.31 ... > >> > >> As the PEAK driver did not produce this problem, he suggested to check > >> the changes between PEAK driver v6.11 and v6.13 having the Changelog: > >> > >> "By preventing accidental back-to-back writes to the command register of > >> the SJA1000 chip in conjunction with multicore processors write stalls > >> were successfully removed" > >> > >> See: http://www.peak-system.com/fileadmin/media/linux/index.htm > >> > >> The change in the v6.13 PEAK driver only cares about the command > >> register and so i cooked the patch below that really fixed the issue in > >> the setup of the colleague. > >> > >> In a 'real' patch it would probably make more sense to introduce a > >> > >> write_reg_guarded() or write_cmd_reg() > >> > >> instead of checking for the command register value each time ... > >> > >> Do you already know about this SJA1000 problem? Any thoughts about the > >> patch? > >> > >> Regards, > >> Oliver > >> > >> > >> > >> Index: drivers/net/can/sja1000/peak_pci.c > >> =================================================================== > >> --- drivers/net/can/sja1000/peak_pci.c (Revision 1174) > >> +++ drivers/net/can/sja1000/peak_pci.c (Arbeitskopie) > >> @@ -91,10 +91,20 @@ > >> > >> return readb(priv->reg_base + (port<< 2)); > >> > >> } > >> > >> -static void peak_pci_write_reg(const struct sja1000_priv *priv, > >> +static void peak_pci_write_reg(struct sja1000_priv *priv, > >> > >> int port, u8 val) > >> > >> { > >> > >> - writeb(val, priv->reg_base + (port<< 2)); > >> + if (port != REG_CMR) > >> + writeb(val, priv->reg_base + (port<< 2)); > >> + else { > >> + /* The command register needs some locking in SMP */ > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&priv->wr_reg_lock, flags); > >> + writeb(val, priv->reg_base + (REG_CMR<< 2)); > >> + readb(priv->reg_base + (REG_SR<< 2)); > >> + spin_unlock_irqrestore(&priv->wr_reg_lock, flags); > >> + } > >> > >> } > >> > >> static void peak_pci_post_irq(const struct sja1000_priv *priv) > >> > >> Index: drivers/net/can/sja1000/sja1000.h > >> =================================================================== > >> --- drivers/net/can/sja1000/sja1000.h (Revision 1174) > >> +++ drivers/net/can/sja1000/sja1000.h (Arbeitskopie) > >> @@ -158,7 +158,7 @@ > >> > >> /* the lower-layer is responsible for appropriate locking */ > >> u8 (*read_reg) (const struct sja1000_priv *priv, int reg); > >> > >> - void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val); > >> + void (*write_reg) (struct sja1000_priv *priv, int reg, u8 val); > >> > >> void (*pre_irq) (const struct sja1000_priv *priv); > >> void (*post_irq) (const struct sja1000_priv *priv); > >> > >> @@ -168,6 +168,7 @@ > >> > >> void __iomem *reg_base; /* ioremap'ed address to registers */ > >> unsigned long irq_flags; /* for request_irq() */ > >> > >> + spinlock_t wr_reg_lock; /* lock for concurrent register writes */ > >> > >> u16 flags; /* custom mode flags */ > >> u8 ocr; /* output control register */ > >> u8 cdr; /* clock divider register */ > >> > >> _______________________________________________ > >> Socketcan-core mailing list > >> [email protected] > >> https://lists.berlios.de/mailman/listinfo/socketcan-core _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
