Oliver Hartkopp wrote:
> On 13.05.2010 20:51, Klaus Hitschler wrote:
>
>> Currently I think it is a matter of SMP systems. If the context switches
>> become faster it may be an issue of UP processors, too.
>>
>> I have not seen the behaviour if the SJA1000 is accessed via ioports. ioport
>> accesses seem to be slow enough.
>>
>> BTW this behaviour is often seen when chips designed for slow embedded
>> hardware are used as peripheral for fast processors. It is a characteristic
>> symptom that something is not simple usable on SMP systems if the driver's
>> code shows small delays between register accesses and the chip's registers
>> are
>> accessed concurrently inside other code paths.
>>
>> I think my kind of workaround may work with both UP and SMP systems.
>
>
> That also my impression.
>
> It took a while to create a minimum invasive patch that doesn't touch all the
> drivers, but here it is:
>
> Index: sja1000/sja1000.c
> ===================================================================
> --- sja1000/sja1000.c (Revision 1177)
> +++ sja1000/sja1000.c (Arbeitskopie)
> @@ -88,6 +88,29 @@
> .brp_inc = 1,
> };
>
> +
> +static void sja1000_write_cmd_reg(struct sja1000_priv *priv, u8 val)
> +{
> + /* the command register needs some locking in SMP systems */
nitpick:
from my school english point of view: on SMP systems sounds better
> +
> +#ifdef CONFIG_SMP
> +
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->wr_reg_lock, flags);
> + priv->write_reg(priv, REG_CMR, val);
> + priv->read_reg(priv, REG_SR);
> + spin_unlock_irqrestore(&priv->wr_reg_lock, flags);
> +
> +#else
> +
> + /* write to the command register without locking */
> + priv->write_reg(priv, REG_CMR, val);
> +
> +#endif
> +}
> +
> +
> static int sja1000_probe_chip(struct net_device *dev)
> {
> struct sja1000_priv *priv = netdev_priv(dev);
> @@ -305,7 +328,7 @@
>
> can_put_echo_skb(skb, dev, 0);
>
> - priv->write_reg(priv, REG_CMR, CMD_TR);
> + sja1000_write_cmd_reg(priv, CMD_TR);
>
> return NETDEV_TX_OK;
> }
> @@ -358,7 +381,7 @@
> cf->can_id = id;
>
> /* release receive buffer */
> - priv->write_reg(priv, REG_CMR, CMD_RRB);
> + sja1000_write_cmd_reg(priv, CMD_RRB);
>
> netif_rx(skb);
>
> @@ -393,7 +416,7 @@
> cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> stats->rx_over_errors++;
> stats->rx_errors++;
> - priv->write_reg(priv, REG_CMR, CMD_CDO); /* clear bit */
> + sja1000_write_cmd_reg(priv, CMD_CDO); /* clear bit */
> }
>
> if (isrc & IRQ_EI) {
> Index: sja1000/sja1000.h
> ===================================================================
> --- sja1000/sja1000.h (Revision 1177)
> +++ sja1000/sja1000.h (Arbeitskopie)
> @@ -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 */
hmm...do we want to wrap this is ifdef CONFIG_SMP?
> u16 flags; /* custom mode flags */
> u8 ocr; /* output control register */
> u8 cdr; /* clock divider register */
>
>
> Any Acked-by or Signed-off-by out there?
>
> Regards,
> Oliver
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 |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
