From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Zarre Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527 Date: Tue, 10 Jan 2012 20:02:35 +0100 Message-ID: <4F0C8B4B.8070104@essax.com> References: <4F0C31F1.20908@essax.com> <4F0C4EA8.7060002@grandegger.com> <4F0C63C6.5080305@essax.com> <4F0C661F.6030508@grandegger.com> Reply-To: info@essax.com Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.essax.com.mt ([80.71.48.244]:41142 "EHLO mail.essax.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932079Ab2AJTCn (ORCPT ); Tue, 10 Jan 2012 14:02:43 -0500 In-Reply-To: <4F0C661F.6030508@grandegger.com> Sender: linux-can-owner@vger.kernel.org List-ID: To: Wolfgang Grandegger , David Laight Cc: Oliver Hartkopp , henrik@proconx.com, netdev@vger.kernel.org, linux-can@vger.kernel.org, socketcan-users@lists.berlios.de, IreneV , Stanislav Yelenskiy , oe@port.de, henrik@focus-sw.com Hello Wolfgang, -------- Original Message -------- Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527 From: Wolfgang Grandegger To: info@essax.com Cc: David Laight , Oliver Hartkopp , henrik@proconx.com, netdev@vger.kernel.org, linux-can@vger.kernel.org, socketcan-users@lists.berlios.de, IreneV , Stanislav Yelenskiy , oe@port.de, henrik@focus-sw.com Date: Tue Jan 10 2012 17:23:59 GMT+0100 (CET) > Hi Wolfgang, > > On 01/10/2012 05:13 PM, Wolfgang Zarre wrote: >> Hello Wolfgang, >> >>> On 01/10/2012 01:41 PM, Wolfgang Zarre wrote: >>>> Hello David, >>>>> >>>>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, >>>>>> int reg, u8 val) >>>>>> { >>>>>> unsigned long base = (unsigned long)priv->reg_base; >>>>>> + unsigned long flags; >>>>>> >>>>>> + spin_lock_irqsave(&outb_lock, flags); >>>>>> outb(reg, base); >>>>>> outb(val, base + 1); >>>>>> + spin_unlock_irqrestore(&outb_lock, flags); >>>>> >>>>> Is there a 'read_reg_indirect' function?? >>>> >>>> Yes, there is. >>>> >>>>> If so it also needs to use the same mutex. >>>> >>>> Actually, I don't think that we have a problem with mutex >>>> beside that it's using just one inb() statement but having >>>> for sure with an interrupt between both outb() statements which >>>> seems to be critical for the cc770. >>> >>> But the indirect read function also sets the address register before >>> reading the data using inb(). This sequence should also not be >>> interrupted and therefore we need to synchronize. For the indirect >>> access of the SJA1000 we also need to add spinlocks. Wonder why nobody >>> complained so far. >> >> So, if I understand correct that means that inb() can be interrupted >> between >> setting the address and reading. If this is the case then yes, we need >> spinlock if this is not the case then IMHO we wouldn't need or am I wrong? > > I think we speak about different things. inb() cannot be interrupted but > outb() followed by inb(). For indirect accesses we need something like: > > /* Spinlock for cc770_isa_port_write_reg_indirect */ > static DEFINE_SPINLOCK(cc770_isa_port_lock); > > static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv, > int reg) > { > unsigned long base = (unsigned long)priv->reg_base; > unsigned long flags; > u8 val; > > spin_lock_irqsave(&cc770_isa_port_lock, flags); > outb(reg, base); > val = inb(base + 1); > spin_unlock_irqrestore(&cc770_isa_port_lock, flags); > > return val; > } > > static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv, > int reg, u8 val) > { > unsigned long base = (unsigned long)priv->reg_base; > unsigned long flags; > > spin_lock_irqsave(&cc770_isa_port_lock, flags); > outb(reg, base); > outb(val, base + 1); > spin_unlock_irqrestore(&cc770_isa_port_lock, flags); > } > > Hope we are in synch now. > Thanks a lot. Yes, now phase locked and in synch and sorry, by mistake I looked at the wrong function (cc770_isa_port_read_reg) in the heat of the moment. Absolutely clear, there we need a spinlock definitely. I'll start another test run just to confirm. > Wolfgang. > Wolfgang