This is the mail archive of the mailing list for the eCos project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix of Synopsys DesignWare Bug in Serial Driver

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.

+#define REG_usr SER_REG(0x1F) // UART status register

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.

+#define ISR_BUSY      0x07

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

+    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);
+    }
     HAL_WRITE_UINT8(base+REG_lcr, _lcr | LCR_DL);
     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.

*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited     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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]