Re: Fix of Synopsys DesignWare Bug in Serial Driver

[Jonathan Larmour <jifl at eCosCentric dot com>]

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