Fix of Synopsys DesignWare Bug in Serial Driver

Fix of Synopsys DesignWare Bug in Serial Driver

[Rene Nielsen]

The following patch circumvents a bug in the Synopsys DesignWare
16550-compatible UART.
The UART would (a.o.) hang if a character is received while the
baud-rate is being reconfigured.
 
Regards,
Rene Schipp von Branitz Nielsen
Vitesse Semiconductors

Index: ser_16x5x.c
===================================================================
RCS file:
/cvs/ecos/ecos/packages/devs/serial/generic/16x5x/current/src/ser_16x5x.
c,v
retrieving revision 1.17
diff -u -r1.17 ser_16x5x.c
--- ser_16x5x.c 29 Jan 2009 17:48:39 -0000      1.17
+++ ser_16x5x.c 12 Feb 2009 10:05:07 -0000
@@ -77,6 +77,10 @@
 #define REG_msr SER_REG(6)    // Modem status register
 #define REG_scr SER_REG(7)    // Scratch register
 
+#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
+#define REG_usr SER_REG(0x1F) // UART status register
+#endif
+
 // Transmit control Registers
 #define REG_thr SER_REG(0)    // Transmit holding register
 #define REG_ier SER_REG(1)    // Interrupt enable register
@@ -129,6 +133,9 @@
 #define ISR_Tx        0x02
 #define ISR_Rx        0x04
 #define ISR_LS        0x06
+#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
+#define ISR_BUSY      0x07
+#endif
 #define ISR_RxTO      0x0C
 #define ISR_64BFIFO   0x20
 #define ISR_FIFOworks 0x40
@@ -259,10 +266,39 @@
     _lcr = select_word_length[new_config->word_length -
CYGNUM_SERIAL_WORD_LENGTH_5] | 
         select_stop_bits[new_config->stop] |
         select_parity[new_config->parity];
+   
+#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
+    while(1) {
+        // Gotta wait until the LCR_DL bit is set.
+        unsigned char _lcr_rd, _usr;
+        HAL_WRITE_UINT8(base+REG_lcr, _lcr | LCR_DL);
+        HAL_READ_UINT8(base+REG_lcr, _lcr_rd);
+        if(_lcr_rd & LCR_DL)
+            break;
+        // Read the USR to clear any busy interrupts
+        HAL_READ_UINT8(base+REG_usr, _usr);
+    }
+#else
     HAL_WRITE_UINT8(base+REG_lcr, _lcr | LCR_DL);
+#endif
+
     HAL_WRITE_UINT8(base+REG_mdl, baud_divisor >> 8);
     HAL_WRITE_UINT8(base+REG_ldl, baud_divisor & 0xFF);
+
+#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
+    while(1) {
+        unsigned char _lcr_rd, _usr;
+        HAL_WRITE_UINT8(base+REG_lcr, _lcr);
+        HAL_READ_UINT8(base+REG_lcr, _lcr_rd);
+        if(!(_lcr_rd & LCR_DL))
+            break;
+        // Read the USR to clear any busy interrupts
+        HAL_READ_UINT8(base+REG_usr, _usr);
+    }
+#else
     HAL_WRITE_UINT8(base+REG_lcr, _lcr);
+#endif
+
     if (init) {
 #ifdef CYGPKG_IO_SERIAL_GENERIC_16X5X_FIFO
         unsigned char _fcr_thresh;
@@ -581,6 +617,19 @@
     // Check if we have an interrupt pending - note that the interrupt
     // is pending of the low bit of the isr is *0*, not 1.
     HAL_READ_UINT8(base+REG_isr, _isr);
+
+#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
+    if(_isr & ISR_BUSY) {
+        // This must be checked before the loop below, because
+        // the LSBit is set in the ISR_BUSY mask.
+        // Read uart status register to clear this interrupt, which
+        // may occur if writing to the LCR register while the UART is
busy.
+        cyg_uint8 _usr;
+        HAL_READ_UINT8(base+REG_usr, _usr);
+        HAL_READ_UINT8(base+REG_isr, _isr);
+    }
+#endif
+
     while ((_isr & ISR_nIP) == 0) {
         switch (_isr&0xE) {
         case ISR_Rx:


Index: ser_generic_16x5x.cdl
===================================================================
RCS file:
/cvs/ecos/ecos/packages/devs/serial/generic/16x5x/current/cdl/ser_generi
c_16x5x.cdl,v
retrieving revision 1.10
diff -u -r1.10 ser_generic_16x5x.cdl
--- ser_generic_16x5x.cdl       29 Jan 2009 17:48:39 -0000      1.10
+++ ser_generic_16x5x.cdl       12 Feb 2009 10:06:21 -0000
@@ -121,6 +121,19 @@
                Configures the maximum number of bytes written to the
                16x5x UART transmit FIFO when the TX interrupt occurs."
        }
+        
+    cdl_option CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE {
+        display       "16x5x UART is from Synopsys"
+        flavor        bool
+        default_value 0
+        description   "
+            In designs using Synopsys's DesignWare APB UART v.3.04a or
+            earlier, or v.3.05a or later with UART_16550_COMPATIBLE set
+            to 'No', the UART's busy-functionality may cause the UART
+            to hang when programming new divisors while receiving a
+            character. Enabling this option fixes this problem."
+    }
+
     }
             
     cdl_component CYGPKG_IO_SERIAL_GENERIC_16X5X_OPTIONS {

Re: Fix of Synopsys DesignWare Bug in Serial Driver

[Jonathan Larmour]

Rene Nielsen wrote:
> The following patch circumvents a bug in the Synopsys DesignWare
> 16550-compatible UART.
> The UART would (a.o.) hang if a character is received while the
> baud-rate is being reconfigured.


> +#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
> +#define REG_usr SER_REG(0x1F) // UART status register
> +#endif

If this register is specific to that chip, in the 16x5x driver I'd 
recommend it be called REG_synopsys_designware_usr. I know it's a 
mouthful, but initially I went looking for a generic chip definition, 
before I realised it was specific to this model. Maybe an abbreviation 
like REG_SYN_DWARE_usr would be ok.

> +#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
> +#define ISR_BUSY      0x07
> +#endif

Again should probably be marked as being specific to this chip if it stays 
in the 16x5x driver.

> +#if CYGPKG_IO_SERIAL_GENERIC_16X5X_SYNOPSYS_DESIGNWARE
> +    while(1) {
> +        // Gotta wait until the LCR_DL bit is set.
> +        unsigned char _lcr_rd, _usr;
> +        HAL_WRITE_UINT8(base+REG_lcr, _lcr | LCR_DL);
> +        HAL_READ_UINT8(base+REG_lcr, _lcr_rd);
> +        if(_lcr_rd & LCR_DL)
> +            break;
> +        // Read the USR to clear any busy interrupts
> +        HAL_READ_UINT8(base+REG_usr, _usr);
> +    }
> +#else
>      HAL_WRITE_UINT8(base+REG_lcr, _lcr | LCR_DL);
> +#endif
> +
>      HAL_WRITE_UINT8(base+REG_mdl, baud_divisor >> 8);
>      HAL_WRITE_UINT8(base+REG_ldl, baud_divisor & 0xFF);

Although I don't know the specific nature of the bug, it seems to me that 
this only reduces the size of the problem window, rather than eliminates 
it. Add in the possibility of occasional pre-emption and that makes it 
more likely to happen. I guess it depends how often you intend to change 
baud rate, but it doesn't seem foolproof.

In general I'm not entirely happy about putting in special cases in 
generic drivers. I think it's better handled with hooks, and that is meant 
to be the general principle in eCos. So I think you could instead have two 
macros SER_16X5X_WRITE_LCR and SER_16x5x_READ_ISR which would default to 
the existing implementation, but your serial driver can set it to your 
workaround code. That would also mean not needing to rename the register 
and bit names I mentioned further up. Those macros would be set by your 
driver in the CYGDAT_IO_SERIAL_GENERIC_16X5X_INL included file.

If you could submit a patch along those lines, that would be good, thanks.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

Re: Fix of Synopsys DesignWare Bug in Serial Driver

[Jonathan Larmour]

Jonathan Larmour wrote:
> 
> If you could submit a patch along those lines, that would be good, thanks.

And don't forget the changelog entry :-).

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

RE: Fix of Synopsys DesignWare Bug in Serial Driver

[Rene Nielsen]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

> Although I don't know the specific nature of the bug,
> it seems to me that this only reduces the size of
> the problem window, rather than eliminates it.
> Add in the possibility of occasional pre-emption
> and that makes it more likely to happen. I guess
> it depends how often you intend to change baud rate,
> but it doesn't seem foolproof.
We only change the baudrate when the application takes over from
RedBoot. Besides, we've run this code for years and it seems to work and
never deadlocks.

> In general I'm not entirely happy about putting in
> special cases in generic drivers. I think it's better
> handled with hooks, and that is meant to be the
> general principle in eCos. So I think you could instead
> have two macros SER_16X5X_WRITE_LCR and SER_16x5x_READ_ISR
> which would default to the existing implementation,
> but your serial driver can set it to your workaround
> code. That would also mean not needing to rename the
> register and bit names I mentioned further up. Those
> macros would be set by your driver in the
> CYGDAT_IO_SERIAL_GENERIC_16X5X_INL included file.
I certainly agree. I wasn't happy about my first implementation either.
Your idea is indeed much more flexible, and the attached patch
implements exactly that. Thanks a lot for guiding me in the right
direction!!

> And don't forget the changelog entry :-).
Included with the patch. Slowly getting used to the process :-)

Thanks a lot,
Rene Schipp von Branitz Nielsen
Vitesse Semiconductors

[-- Attachment #2: serial_16x5x_2009_02_17.patch --]
[-- Type: application/octet-stream, Size: 2510 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/serial/generic/16x5x/current/ChangeLog,v
retrieving revision 1.18
diff -u -r1.18 ChangeLog
--- ChangeLog	29 Jan 2009 17:48:39 -0000	1.18
+++ ChangeLog	17 Feb 2009 22:37:48 -0000
@@ -1,3 +1,10 @@
+2009-02-17  Rene Schipp von Branitz Nielsen <rbn@vitesse.com>
+
+	* src/ser_16x5x.c:
+	Allow platform code to override the default implementation for
+	writing the LCR register and reading the ISR register by using
+	the SER_16X5X_WRITE_LCR() and SER_16X5X_READ_ISR() macros.
+
 2008-07-08  Uwe Kindler <uwe_kindler@web.de>
 
 	* cdl/ser_generic_16x5x.cdl
Index: src/ser_16x5x.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/serial/generic/16x5x/current/src/ser_16x5x.c,v
retrieving revision 1.17
diff -u -r1.17 ser_16x5x.c
--- src/ser_16x5x.c	29 Jan 2009 17:48:39 -0000	1.17
+++ src/ser_16x5x.c	17 Feb 2009 22:37:49 -0000
@@ -229,6 +229,17 @@
 # define CYG_IO_SERIAL_GENERIC_16X5X_INT_PRIORITY 4
 #endif
 
+// Allow platform code to override the default implementation of
+// a write to the LCR
+#ifndef SER_16X5X_WRITE_LCR
+#define SER_16X5X_WRITE_LCR(_base_, _val_) HAL_WRITE_UINT8((_base_) + REG_lcr, _val_)
+#endif
+
+// Allow platform code to override the default implementation of
+// a read of the ISR
+#ifndef SER_16X5X_READ_ISR
+#define SER_16X5X_READ_ISR(_base_, _val_) HAL_READ_UINT8((_base_) + REG_isr, _val_)
+#endif
 
 // Internal function to actually configure the hardware to desired
 // baud rate, etc.
@@ -259,10 +270,10 @@
     _lcr = select_word_length[new_config->word_length - CYGNUM_SERIAL_WORD_LENGTH_5] | 
         select_stop_bits[new_config->stop] |
         select_parity[new_config->parity];
-    HAL_WRITE_UINT8(base+REG_lcr, _lcr | LCR_DL);
+    SER_16X5X_WRITE_LCR(base, _lcr | LCR_DL);
     HAL_WRITE_UINT8(base+REG_mdl, baud_divisor >> 8);
     HAL_WRITE_UINT8(base+REG_ldl, baud_divisor & 0xFF);
-    HAL_WRITE_UINT8(base+REG_lcr, _lcr);
+    SER_16X5X_WRITE_LCR(base, _lcr);
     if (init) {
 #ifdef CYGPKG_IO_SERIAL_GENERIC_16X5X_FIFO
         unsigned char _fcr_thresh;
@@ -580,7 +591,7 @@
 
     // Check if we have an interrupt pending - note that the interrupt
     // is pending of the low bit of the isr is *0*, not 1.
-    HAL_READ_UINT8(base+REG_isr, _isr);
+    SER_16X5X_READ_ISR(base, _isr);
     while ((_isr & ISR_nIP) == 0) {
         switch (_isr&0xE) {
         case ISR_Rx:

Re: Fix of Synopsys DesignWare Bug in Serial Driver

[Jonathan Larmour]

Rene Nielsen wrote:
>Jifl wrote: 
>>I think it's better handled with hooks,[snip]
> 
> I certainly agree. I wasn't happy about my first implementation either.
> Your idea is indeed much more flexible, and the attached patch
> implements exactly that. Thanks a lot for guiding me in the right
> direction!!

And thankyou for sorting it out. Committed.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine