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

[Oliver Hartkopp <socketcan@hartkopp.net>]

Answering myself ...

i found out that the "HM" comment was introduced in the transition
from v3.2 to v3.3.3 . I took the compared versions from Heinz-Jürgen:

http://www.port.de/pages/misc/can4linux.php

The diff which contains the relevant changes is here:

diff -U 10 -w can4linux-3.2/src/can_i82527funcs.c can4linux-3.3.3/can_i82527funcs.c | dos2unix

--- can4linux-3.2/src/can_i82527funcs.c	2004-05-14 12:02:54.000000000 +0200
+++ can4linux-3.3.3/can_i82527funcs.c	2004-12-14 10:35:20.000000000 +0100
@@ -1,32 +1,40 @@
 /*
  * can_i82527funcs.c - i82527 related code
  *
  * Original version Written by Arnaud Westenberg email:arnaud@wanadoo.nl
  * This software is released under the GPL-License.
  *
+ * Modified and extended to support the esd elctronic system
+ * design gmbh PC/104-CAN Card (www.esd-electronics.com)
+ * by Jean-Jacques Tchouto (tchouto@fokus.fraunhofer.de), 2003
+ *
  * Major Refactoring and Integration into can4linux version 3.1 by
  * Henrik W Maier of FOCUS Software Engineering Pty Ltd <www.focus-sw.com>
  *
  * Modified and extended to support the SBS PC7compact DINrail mounted
  * industry PC by FOCUS Software Engineering Pty Ltd <www.focus-sw.com>
  *
- * Modified and extended to support the esd elctronic system
- * design gmbh PC/104-CAN Card (www.esd-electronics.com)
- * by Jean-Jacques Tchouto (tchouto@fokus.fraunhofer.de), 2003
+ * Updated for 2.6 kernel by Henrik W Maier of FOCUS Software
+ * Engineering Pty Ltd <www.focus-sw.com>.
+ * Bugfix in CAN_SendMessage, added wake_up_interruptible for irq_write_handler
+ * Disabled redundant status change interrupt for RX/TX to decrease int
+ * load on CPU.
+ * Fixed issue of a second iteration in the ISR when sending because the
+ * send interrupt didn't sometimes get acknowledged.
  *
- * $Id: can_i82527funcs.c,v 1.3 2004/05/14 10:02:54 oe Exp $
+ * $Id: can_i82527funcs.c,v 1.4 2004/12/14 09:35:20 oe Exp $
  *
  */
 
 
-#include <can_defs.h>
+#include "can_defs.h"
 #include "can_i82527.h"
 
 
 #if defined (PC104_200)
 inline void CANactivateIRQline(int bd)
 {
     unsigned long canIoPort;
     canIoPort = (unsigned long)Base[bd];
     outb_p(0x86, canIoPort+3);
 }
@@ -216,22 +224,22 @@
            MVAL_SET | TXIE_RES | RXIE_SET | INTPD_RES);
 
     CAN_SetMask(board, AccCode[board], AccMask[board]);
 
     // Clear message object for send
     CANout(board, message1Reg.msgCtrl1Reg,
            RMPD_RES | TXRQ_RES | CPUU_RES | NEWD_RES);
     CANout(board, message1Reg.msgCtrl0Reg,
            MVAL_RES | TXIE_RES | RXIE_RES | INTPD_RES);
 
-    // Clear bus-off, Interrupts on Rx, TX, any Status change and data overrun
-    CANout(board, controlReg, iCTL_IE | iCTL_SIE | iCTL_EIE);
+    // Clear bus-off, Interrupts only for errors, not for status change
+    CANout(board, controlReg, iCTL_IE | iCTL_EIE);
 
     DBGprint(DBG_DATA, ("[%d] CAN_CON 0x%x\n", board, CANin(board, controlReg)));
 
     DBGout();
     return 0;
 }
 
 
 /*
  * Puts chip in bus-off mode
@@ -274,23 +282,28 @@
 int CAN_SendMessage(int board, canmsg_t *tx)
 {
     int i = 0;
     int ext;
     uint8_t id0, id1, id2, id3;
 
     DBGin("i82527_CAN_SendMessage");
 
     DBGin("Wait if there is a transmission in progress");
     // Wait if there is a transmission in progress
-    while ((CANin(board, message1Reg.msgCtrl1Reg) && TXRQ_UNC) == TXRQ_SET) {
+    while ((CANin(board, message1Reg.msgCtrl1Reg) & TXRQ_UNC) == TXRQ_SET) {
  	    #if LINUX_VERSION_CODE >= 131587
-	    if( current->need_resched ) schedule();
+	    # if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
+	    cond_resched();
+	    # else
+	    if( current->need_resched )
+        schedule();
+	    # endif
 	    #else
 	    if( need_resched ) schedule();
 	    #endif
     }
     DBGout();
 
     CANout(board, message1Reg.msgCtrl1Reg,
            RMPD_RES | TXRQ_RES | CPUU_SET | NEWD_SET);
     CANout(board,message1Reg.msgCtrl0Reg,
            MVAL_SET | TXIE_SET | RXIE_RES | INTPD_RES);
@@ -507,32 +520,37 @@
 
     DBGin("i82527_irq_write_handler");
 
     // Enter critical section
     save_flags(flags);
     cli();
 
     if( TxFifo->free[TxFifo->tail] == BUF_EMPTY ) {
         // Nothing more to send, switch off interrupts
         CANout(board, message1Reg.msgCtrl0Reg,
-               (MVAL_RES | TXIE_RES | RXIE_UNC | INTPD_RES));
+               (MVAL_RES | TXIE_RES | RXIE_RES | INTPD_RES));
         TxFifo->status = BUF_EMPTY;
         TxFifo->active = 0;
+        // We had some cases of repeated IRQ, so make sure the INT is acknowledged
+        CANout(board, message1Reg.msgCtrl0Reg,
+               (MVAL_UNC | TXIE_UNC | RXIE_UNC | INTPD_RES));
         /* leave critical section */
         restore_flags(flags);
-
+        // Notify user app
+        wake_up_interruptible(  &CanOutWait[board] );
+        DBGout();
         return;
     }
     CANout(board, message1Reg.msgCtrl1Reg,
                  RMPD_RES | TXRQ_RES | CPUU_SET | NEWD_RES);
     CANout(board, message1Reg.msgCtrl0Reg,
-                 MVAL_SET | TXIE_SET | RXIE_RES | INTPD_UNC);
+                 MVAL_SET | TXIE_SET | RXIE_RES | INTPD_RES);
 
     tx2reg = (TxFifo->data[TxFifo->tail]).length;
 
     ext = (TxFifo->data[TxFifo->tail]).flags & MSG_EXT;
     id = (TxFifo->data[TxFifo->tail]).id;
 
     if ( ext ) {
         CANout(board, message1Reg.messageConfigReg,
                      (tx2reg << 4 ) + ( MCFG_DIR | MCFG_XTD ));
         id0 = (uint8_t)( id << 3 );
@@ -565,20 +583,24 @@
     }
     else {
         CANout(board, message1Reg.msgCtrl1Reg,
                (RMPD_RES | TXRQ_SET | CPUU_RES | NEWD_UNC));
 
     }
 
     TxFifo->free[TxFifo->tail] = BUF_EMPTY; /* now this entry is EMPTY */
     TxFifo->tail = ++(TxFifo->tail) % MAX_BUFSIZE;
 
+    // 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
+    CANout(board, message1Reg.msgCtrl0Reg,
+           (MVAL_UNC | TXIE_UNC | RXIE_UNC | INTPD_RES));
     /* leave critical section */
     restore_flags(flags);
 
     DBGout();
 }
 
 
 /*
  * The plain i82527 interrupt
  *
@@ -604,32 +626,41 @@
  *
  *
  * RX Int with to Receive channels:
  * 1)                _____   ___
  *             _____|     |_|   |__
  *                   30    5  20  µs
  *   first ISR normal length,
  *   time between the two ISR -- short
  *   sec. ISR shorter than first, why? it's the same message
  */
+#if LINUX_VERSION_CODE >= 0x020500
+irqreturn_t CAN_Interrupt ( int irq, void *dev_id, struct pt_regs *ptregs )
+#else
 void CAN_Interrupt(int irq, void *dev_id, struct pt_regs *ptregs)
+#endif
 {
     int board = *(int *) dev_id;
     msg_fifo_t *RxFifo = &Rx_Buf[board];
     msg_fifo_t *TxFifo = &Tx_Buf[board];
     uint8_t irqreg;
+    uint8_t lastIrqreg;
 
     // Read the highest pending interrupt request
     irqreg = CANin(board, interruptReg);
+    lastIrqreg = irqreg;
+    
     while ( irqreg ) {
-        // Handle change in status register
-        if ( irqreg == 0x01 ) {
+        switch (irqreg)
+        {
+            case 1: // Status register
+            {
             uint8_t status;
 
             // Read the STATUS reg
             status = CANin(board, statusReg);
             CANout (board, statusReg, 0);
 
             if ( status & iSTAT_RXOK ) {
                 // Intel datasheet: Software must clear this bit in ISR
                 CANout (board, statusReg, status & ~iSTAT_RXOK);
             }
@@ -644,42 +675,66 @@
                 DBGprint(DBG_DATA,("i82527_CAN_Interrupt: Bus warning\n" ));
             }
             if ( status & iSTAT_BOFF ) {
                 long flags;
 
                 // Note: status bit is read-only, don't clear
                 // TODO must be implemented here for chip statistic
                 (RxFifo->data[RxFifo->head]).flags += MSG_BUSOFF;
 
                 // Clear init flag and reenable interrupts
-                flags = CANin(board, controlReg) | ( iCTL_IE | iCTL_SIE | iCTL_EIE );
+                    flags = CANin(board, controlReg) | ( iCTL_IE | iCTL_EIE );
                 flags &= ~iCTL_INI; // Reset init flag
                 CANout(board, controlReg, flags);
 
                 (RxFifo->data[RxFifo->head]).id = 0xFFFFFFFF;
                 RxFifo->status = BUF_OK;
                 RxFifo->head = ++(RxFifo->head) % MAX_BUFSIZE;
                 if(RxFifo->head == RxFifo->tail) {
                     RxFifo->status = BUF_OVERRUN;
                 }
+                    // Notify user app
+                    wake_up_interruptible(  &CanWait[board] );
 	            DBGprint(DBG_DATA,("i82527_CAN_Interrupt: Bus off\n" ));
             }
-        } // end if irqreg == 0x01
-        else {
-            if (irqreg == 0x02)
+             }
+             break;
+             case 2: // Receiption, message object 15
                 i82527_irq_read_msg15_handler(board, RxFifo);
-            else
+             break;
+             case 3: // Write, message object 1
                 i82527_irq_write_handler(board, TxFifo);
-        } //end else if irqreg == 0x01
+             break;
+             case 4: // message object 2
+                 //printk("*********** Unexpected i82527_CAN_Interrupt: irqreq2=0x%X\n", irqreg);
+                 DBGprint(DBG_DATA,("Unexpected i82527_CAN_Interrupt: irqreq=0x%X\n", irqreg));
+                 CANout(board, message2Reg.msgCtrl0Reg, 
+                        (MVAL_RES | TXIE_RES | RXIE_RES | INTPD_RES));
+             break;
+             default: // Unexpected
+                 //printk("*********** Unexpected i82527_CAN_Interrupt: irqreq2=0x%X\n", irqreg);
+                 DBGprint(DBG_DATA,("Unexpected i82527_CAN_Interrupt: irqreq=0x%X\n", irqreg));
+             break;
+        }
         // Get irq status again for next loop iteration
         irqreg = CANin(board, interruptReg);
-    } // while
+        if (irqreg == lastIrqreg)
+        {
+           //printk("i82527_CAN_Interrupt: irqreq repeated!!!! 0x%X\n", irqreg);
+           DBGprint(DBG_DATA,("i82527_CAN_Interrupt: irqreq repeated!!!! 0x%X\n", irqreg));
+        }
+        lastIrqreg =     irqreg;
+    } /* end while (irqreq) */
+    DBGout();
+#if LINUX_VERSION_CODE >= 0x020500
+    return IRQ_RETVAL(IRQ_HANDLED);
+#endif
 }
 
 
 int CAN_ShowStat(int board)
 {
   // TODO: Implement
   return -1;
 }
 
 
@@ -852,21 +907,20 @@
 	/* PCI scan has already remapped the address */
 	can_base[board] = (unsigned char *)Base[board];
 #else
 	can_base[board] = ioremap(Base[board], can_range[board]);
 #endif
 #else 	  /*  LINUX_VERSION_CODE >= KERNEL_VERSION(2,3,11) */
 
 /* both drivers use high memory area */
 #if !defined(CONFIG_PPC) && !defined(CAN4LINUX_PCI)
     if( check_region(Base[board], CAN_RANGE ) ) {
-		     /* MOD_DEC_USE_COUNT; */
 		     DBGout();
 		     return -EBUSY;
     } else {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,0,0)
           request_region(Base[board], can_range[board], "CAN-IO" );
 #else
           request_region(Base[board], can_range[board] );
 #endif  /* LINUX_VERSION_CODE >= KERNEL_VERSION(2,0,0) */
     }
 #endif /* !defined  ... */


That's it :-)

I hope it helps.

Cheers,
Oliver




On 05.01.2012 20:45, Oliver Hartkopp wrote:

> Hello Wolfgang(s),
> 
> On 05.01.2012 12:51, Wolfgang Grandegger wrote:
> 
>>>>> diff --git a/drivers/net/can/cc770/cc770_isa.c
>>>>> b/drivers/net/can/cc770/cc770_isa.c
>>>>> index 4be5fe2..48fc128 100644
>>>>> --- a/drivers/net/can/cc770/cc770_isa.c
>>>>> +++ b/drivers/net/can/cc770/cc770_isa.c
>>>>> @@ -148,8 +148,7 @@ static void cc770_isa_port_write_reg_indirect(const
>>>>> struct cc770_priv *priv,
>>>>>   {
>>>>>       unsigned long base = (unsigned long)priv->reg_base;
>>>>>
>>>>> -    outb(reg, base);
>>>>> -    outb(val, base + 1);
>>>>> +    outw( reg + ( val<<  8), base);
>>>>
> 
>>>> That modification does fix your problem, right? The others above don't
>>>> help nor harm but we don't know if it's really realted to the same
>>>> problem. I wll dig a bit deeper.
>>>
>>> Exactly. The others above I removed because facing the opposite, even
>>> missing interrupts but then just to avoid other possible side effects
>>> and then assuming that they might be related.
>>
>> 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?
>>
>> About the "HM:" fixes, I did not find any info in the svn log. Maybe
>> Oliver knows why they have been added.
> 
> 
> 
> No, in fact i don't know 8-)
> 
> As you can see from the initial commit msg here:
> 
> http://svn.berlios.de/wsvn/socketcan/branches/ha/candrv/kernel/2.6/drivers/net/can/i82527/?rev=197&peg=197
> 
> "First implementation of i82527 socketcan driver based on parts from
> socketcan sja1000 netdev / ocan hal / can4linux register access.
> Know issues: Currently only receives SFF CAN frames due different
> handling of SFF/EFF and RTR frames in opposite to the SJA1000 controller."
> 
> I took my sja1000 SocketCAN driver and tried to pick the register access
> sniplets for the i82527 from (mostly) can4linux and kicked it until it
> compiled ...
> 
> This register access part is from can4linux which has a long(!) development
> history too, where a special message regarding the ISR can be found, see:
> 
> http://can4linux.svn.sourceforge.net/viewvc/can4linux/trunk/can4linux/i82527funcs.c?revision=36&view=markup
> 
> /*
>  * can_i82527funcs.c - i82527 related code
>  *
>  * Original version Written by Arnaud Westenberg email:arnaud@wanadoo.nl
>  * This software is released under the GPL-License.
>  *
>  * Modified and extended to support the esd elctronic system
>  * design gmbh PC/104-CAN Card (www.esd-electronics.com)
>  * by Jean-Jacques Tchouto (tchouto@fokus.fraunhofer.de), 2003
>  *
>  * Major Refactoring and Integration into can4linux version 3.1 by
>  * Henrik W Maier of FOCUS Software Engineering Pty Ltd <www.focus-sw.com>
>  *
>  * Modified and extended to support the SBS PC7compact DINrail mounted
>  * industry PC by FOCUS Software Engineering Pty Ltd <www.focus-sw.com>
>  *
>  * Updated for 2.6 kernel by Henrik W Maier of FOCUS Software
>  * Engineering Pty Ltd <www.focus-sw.com>.
>  * Bugfix in CAN_SendMessage, added wake_up_interruptible for irq_write_handler
>  * Disabled redundant status change interrupt for RX/TX to decrease int
>  * load on CPU.
>  * Fixed issue of a second iteration in the ISR when sending because the
>  * send interrupt didn't sometimes get acknowledged.
>  *
>  * $Id: i82527funcs.c,v 1.1 2006/04/21 16:26:51 oe Exp $
>  *
>  */
> 
> My original hardware was the Eurotech COM 1274 board for the US DOT VII OBU:
> http://www.eurotech.com/en/download/com-1274
> 
> Obviously the ISR quirk code was added by Henrik W Maier from FOCUS Software:
> 
> http://www.focus-sw.com/can/can4linux.html
> 
> Maybe Henrik or Heinz-Jürgen remember why this ISR handling has been added
> more than seven years ago ... i added both of them in CC.
> 
> I hope the historical src code research helped a bit.
> 
> Indeed i was not aware of that long i82527 history inside the can4linux
> driver. If there's any interest of the register access authors, we should
> add some credits into the cc770 driver.
> 
> Best regards,
> Oliver
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html