diff mbox

[net-next,v2,2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527

Message ID 4F0D4FBA.1080108@essax.com
State Awaiting Upstream, archived
Delegated to: David Miller
Headers show

Commit Message

Wolfgang Zarre Jan. 11, 2012, 9 a.m. UTC 
Hello Wolfgang,

> Hello Wolfgang,
>
>> On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote:
>>> On 01/09/2012 10:47 PM, Wolfgang Zarre wrote:
>>> [...]
>>>
>>>>>> OK. My concern: Can we be sure that 16bit accesses are always
>>>>> supported
>>>>>> by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
>>>>> the
>>>>>> 8bit accesses already help?
>>>>>
>>>>> Hmmm... are there any register reads that need the
>>>>> same 'double cycle' sequence ??
>>>>> If so you need to stop reads being interleaved (with
>>>>> themselves and writes) so requesting a 16bit access
>>>>> doesn't help.
>>>>>
>>>>> Which means you need a spinlock...
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>
>>>> @David: Thank You very much for that hint. You are right and to
>>>> implement correct we need a spinlock.
>>>>
>>>> @Wolfgang: I was thinking about Your question regarding 8/16 bit and
>>>> in fact it wouldn't work at all on a clean 8 bit cards.
>>>>
>>>> Further it wouldn't work on 16 bit cards where the MSB is not equal
>>>> to base port +1 and anyway, it's depending always on how the chip is
>>>> interfaced to the ISA bus and in which mode the chip is configured.
>>>>
>>>>
>>>> And therefore I was giving David's hint a try in using a spinlock in
>>>> function cc770_isa_port_write_reg_indirect() and patched as follows:
>>>>
>>>> ---------------------------------------------------------------------
>>>> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
>>>> index 2d12f89..dad6707 100644
>>>> --- a/drivers/net/can/cc770/cc770.c
>>>> +++ b/drivers/net/can/cc770/cc770.c
>>>> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff
>>>> *skb, struct net_device *dev)
>>>>
>>>> stats->tx_bytes += dlc;
>>>>
>>>> -
>>>> - /*
>>>> - * HM: We had some cases of repeated IRQs so make sure the
>>>> - * INT is acknowledged I know it's already further up, but
>>>> - * doing again fixed the issue
>>>> - */
>>>> - cc770_write_reg(priv, msgobj[mo].ctrl0,
>>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
>>>> -
>>>> return NETDEV_TX_OK;
>>>> }
>>>>
>>>> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device
>>>> *dev, unsigned int o)
>>>> /* Nothing more to send, switch off interrupts */
>>>> cc770_write_reg(priv, msgobj[mo].ctrl0,
>>>> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
>>>> - /*
>>>> - * We had some cases of repeated IRQ so make sure the
>>>> - * INT is acknowledged
>>>> - */
>>>> - cc770_write_reg(priv, msgobj[mo].ctrl0,
>>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
>>
>> Please provide an extra patch for these unrelated changes. If we really
>> want to remove it.
>>
>
> Sure, this I can do.
>

Ok, here the patch to remove:
--------------------------------------------------------
----------------------------------------------------------


>>>> stats->tx_packets++;
>>>> can_get_echo_skb(dev, 0);
>>>> diff --git a/drivers/net/can/cc770/cc770_isa.c
>>>> b/drivers/net/can/cc770/cc770_isa.c
>>>> index 4be5fe2..fe39eed 100644
>>>> --- a/drivers/net/can/cc770/cc770_isa.c
>>>> +++ b/drivers/net/can/cc770/cc770_isa.c
>>>> @@ -110,6 +110,9 @@ MODULE_PARM_DESC(bcr, "Bus configuration register
>>>> (default=0x40 [CBY])");
>>>> #define CC770_IOSIZE 0x20
>>>> #define CC770_IOSIZE_INDIRECT 0x02
>>>>
>>>> +/* Spinlock for cc770_isa_port_write_reg_indirect */
>>>> +static DEFINE_SPINLOCK( outb_lock);
>>>> +
>>>
>>> Do we need a global or a per device spin lock? If this should be a per
>>> device one, please introduce a cc770_isa_priv and put the spinlock
>>> there. Don't forget to initialize the spinlock.
>>
>> Yes, that's what I was thinking as well but in the ocan driver I find:
>>
>> /*
>> * we need a spinlock here, as the address register looks shared between
>> * two PC-ECAN devices. Moreover, we need to protect WRT interrupts
>> */
>>
>> Looks like wired hardware. Anyway, a global spinlock might be safer.
>>
>
> Hmmm, actually I thought to place the spinlock local because of having
> the problem just with the interrupt and not with mutex.
>
> But if global wouldn't it then better to make an array[MAX_DEV] for the
> lock with initialisation in _init or _start?
>
> But if PC-ECAN works with that configuration?
>
>> Wolfgang.
>
> Wolfgang

Wolfgang
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Laight Jan. 11, 2012, 9:37 a.m. UTC | #1 
> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct 
> net_device *dev, unsigned int o)
>   	/* Nothing more to send, switch off interrupts */
>   	cc770_write_reg(priv, msgobj[mo].ctrl0,
>   			MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
> -	/*
> -	 * We had some cases of repeated IRQ so make sure the
> -	 * INT is acknowledged
> -	 */
> -	cc770_write_reg(priv, msgobj[mo].ctrl0,
> -			MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);

A wild guess is that this is needed to clear an interrupt
that was asserted during the ISR processing.
So you need to ack the IRQ as well as mask it.

Not sure if the difference between the xxx_UNC and xxx_RES
bits though.

	David


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde Jan. 11, 2012, 9:38 a.m. UTC | #2 
Hello,

On 01/11/2012 10:00 AM, Wolfgang Zarre wrote:
>>> Please provide an extra patch for these unrelated changes. If we really
>>> want to remove it.
>>>
>>
>> Sure, this I can do.
>>
> 
> Ok, here the patch to remove:

Looks good, please add a patch description and put in a patch series
together with the spinlock patch (see other mail).

Marc

> --------------------------------------------------------
> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
> index 2d12f89..dad6707 100644
> --- a/drivers/net/can/cc770/cc770.c
> +++ b/drivers/net/can/cc770/cc770.c
> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff
> *skb, struct net_device *dev)
> 
>      stats->tx_bytes += dlc;
> 
> -
> -    /*
> -     * HM: We had some cases of repeated IRQs so make sure the
> -     * INT is acknowledged I know it's already further up, but
> -     * doing again fixed the issue
> -     */
> -    cc770_write_reg(priv, msgobj[mo].ctrl0,
> -            MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
> -
>      return NETDEV_TX_OK;
>  }
> 
> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device
> *dev, unsigned int o)
>      /* Nothing more to send, switch off interrupts */
>      cc770_write_reg(priv, msgobj[mo].ctrl0,
>              MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
> -    /*
> -     * We had some cases of repeated IRQ so make sure the
> -     * INT is acknowledged
> -     */
> -    cc770_write_reg(priv, msgobj[mo].ctrl0,
> -            MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
> 
>      stats->tx_packets++;
>      can_get_echo_skb(dev, 0);
> ----------------------------------------------------------

Marc
Wolfgang Zarre Jan. 11, 2012, 2:37 p.m. UTC | #3 
Hello David,

>
>> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct
>> net_device *dev, unsigned int o)
>>    	/* Nothing more to send, switch off interrupts */
>>    	cc770_write_reg(priv, msgobj[mo].ctrl0,
>>    			MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
>> -	/*
>> -	 * We had some cases of repeated IRQ so make sure the
>> -	 * INT is acknowledged
>> -	 */
>> -	cc770_write_reg(priv, msgobj[mo].ctrl0,
>> -			MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
>
> A wild guess is that this is needed to clear an interrupt
> that was asserted during the ISR processing.
> So you need to ack the IRQ as well as mask it.

I was digging a bit and as far as I understood by studying both
manuals of i82527 and cc770 the procedure is in any case most
probably not completely clean.

 From my point of view there would be no need to set and reset
TXIE/RXIE after every request because according to the description
as below (e.g for TXIE):


       MSB LSB    Meaning
Write  0   0  Not allowed
               (indeterminate)
        0   1  reset
        1   0  set
        1   1  unchanged



TXIE Transmit Interrupt Enable
  one  An interrupt will be generated after a successful
       transmission of a frame
  zero No interrupt will be generated after a successful
       transmission of a frame
       The Transmit Interrupt Enable bit enables the
       82527 to initiate an interrupt after the successful
       transmission by the corresponding message
       object This bit is written by the CPU

In fact with that we are telling the chip if we want to have
interrupts or not for a transmission and therefore it would
be enough, so far I understood, to set it at _init and switch
it of at _deinit and between just using 'unchanged'.

Actually to ack the IRQ we would have to set TXOK of the
status register to zero after reading a one for a successful
transmission but is in fact the chip doesn't care if we
reset TXOK or not.

However, if time permits I'll try to dig deeper and doing some
tests because it would need some changes more.


Anyway, I might be wrong as well because sometimes I'm checking
under heavy workload the wrong bit's as You know already ;-)


More or less the reason for the request of removal was just because
assuming that the repeated interrupts were caused as well not having
spinlock's and on the other hand if obsolete to save I/O resources
which affects mostly smaller systems with heavy load.

However, maybe Wolfgang can give an advice.


>
> Not sure if the difference between the xxx_UNC and xxx_RES
> bits though.
>
> 	David
>
>
Wolfgang
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfgang Zarre Jan. 11, 2012, 2:42 p.m. UTC | #4 
Hello Marc,

> Hello,
>
> On 01/11/2012 10:00 AM, Wolfgang Zarre wrote:
>>>> Please provide an extra patch for these unrelated changes. If we really
>>>> want to remove it.
>>>>
>>>
>>> Sure, this I can do.
>>>
>>
>> Ok, here the patch to remove:
>
> Looks good, please add a patch description and put in a patch series
> together with the spinlock patch (see other mail).

Thanks, also for the additional hints.

It might be that I let this patch for now until clarified if we
should remove or not.

>
> Marc
>
>> --------------------------------------------------------
>> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
>> index 2d12f89..dad6707 100644
>> --- a/drivers/net/can/cc770/cc770.c
>> +++ b/drivers/net/can/cc770/cc770.c
>> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff
>> *skb, struct net_device *dev)
>>
>>       stats->tx_bytes += dlc;
>>
>> -
>> -    /*
>> -     * HM: We had some cases of repeated IRQs so make sure the
>> -     * INT is acknowledged I know it's already further up, but
>> -     * doing again fixed the issue
>> -     */
>> -    cc770_write_reg(priv, msgobj[mo].ctrl0,
>> -            MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
>> -
>>       return NETDEV_TX_OK;
>>   }
>>
>> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device
>> *dev, unsigned int o)
>>       /* Nothing more to send, switch off interrupts */
>>       cc770_write_reg(priv, msgobj[mo].ctrl0,
>>               MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
>> -    /*
>> -     * We had some cases of repeated IRQ so make sure the
>> -     * INT is acknowledged
>> -     */
>> -    cc770_write_reg(priv, msgobj[mo].ctrl0,
>> -            MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
>>
>>       stats->tx_packets++;
>>       can_get_echo_skb(dev, 0);
>> ----------------------------------------------------------
>
> Marc
>

Wolfgang
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde Jan. 11, 2012, 3:02 p.m. UTC | #5 
On 01/11/2012 03:42 PM, Wolfgang Zarre wrote:
> Thanks, also for the additional hints.

You're welcome.

> It might be that I let this patch for now until clarified if we
> should remove or not.

Yes, the just prepare the spinlock patch.

Marc
diff mbox

Patch

diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
index 2d12f89..dad6707 100644
--- a/drivers/net/can/cc770/cc770.c
+++ b/drivers/net/can/cc770/cc770.c
@@ -460,15 +460,6 @@  static netdev_tx_t cc770_start_xmit(struct sk_buff *skb, struct net_device *dev)

  	stats->tx_bytes += dlc;

-
-	/*
-	 * HM: We had some cases of repeated IRQs so make sure the
-	 * INT is acknowledged I know it's already further up, but
-	 * doing again fixed the issue
-	 */
-	cc770_write_reg(priv, msgobj[mo].ctrl0,
-			MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
  	return NETDEV_TX_OK;
  }

@@ -689,12 +680,6 @@  static void cc770_tx_interrupt(struct net_device *dev, unsigned int o)
  	/* Nothing more to send, switch off interrupts */
  	cc770_write_reg(priv, msgobj[mo].ctrl0,
  			MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
-	/*
-	 * We had some cases of repeated IRQ so make sure the
-	 * INT is acknowledged
-	 */
-	cc770_write_reg(priv, msgobj[mo].ctrl0,
-			MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);

  	stats->tx_packets++;
  	can_get_echo_skb(dev, 0);