On Tuesday 04 December 2007, Timur Tabi wrote:
> Add support for UART serial ports using a Freescale QUICC Engine
> (found on some MPC83xx and MPC85xx SOCs).
>
> Because of a silicon bug in some QE-enabled SOCs (e.g. 8323 and 8360), a new
> microcode is required. This microcode implements UART via a work-around,
> hence it's called "Soft-UART". This driver can use QE firmware upload feature
> to upload the correct microcode to the QE.
Can you use the driver on CPUs without this particular bug when it's built
in Soft-UART mode?
> +
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART_UPLOAD
> +#include <linux/firmware.h>
> +#include <asm/reg.h>
> +#endif
> +
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
> +/*
> + * The GUMR flag for Soft UART. This would normally be defined in qe.h,
> + * but Soft-UART is a hack and we want to keep everything related to it in
> + * this file.
> + */
> +#define UCC_SLOW_GUMR_H_SUART 0x00004000 /* Soft UART */
> +
> +/*
> + * firmware_loaded is 1 if the firmware has been loaded, 0 otherwise.
> + */
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART_UPLOAD
> +static int firmware_loaded;
> +#endif
Try to reduce the number of #ifdefs in your code. In particular, you should
not do conditional #includes and #defines, as they often lead to subtle bugs.
> +struct ucc_uart_pram {
> + struct ucc_slow_pram common;
> + u8 res1[8]; /* reserved */
> + __be16 maxidl; /* Maximum idle chars */
> + __be16 idlc; /* temp idle counter */
> + __be16 brkcr; /* Break count register */
> + __be16 parec; /* receive parity error counter */
> + __be16 frmec; /* receive framing error counter */
> + __be16 nosec; /* receive noise counter */
> + __be16 brkec; /* receive break condition counter */
> + __be16 brkln; /* last received break length */
> + __be16 uaddr[2]; /* UART address character 1 & 2 */
> + __be16 rtemp; /* Temp storage */
> + __be16 toseq; /* Transmit out of sequence char */
> + __be16 cchars[8]; /* control characters 1-8 */
> + __be16 rccm; /* receive control character mask */
> + __be16 rccr; /* receive control character register */
> + __be16 rlbc; /* receive last break character */
> + __be16 res2; /* reserved */
> + __be32 res3; /* reserved, should be cleared */
> + u8 res4; /* reserved, should be cleared */
> + u8 res5[3]; /* reserved, should be cleared */
> + __be32 res6; /* reserved, should be cleared */
> + __be32 res7; /* reserved, should be cleared */
> + __be32 res8; /* reserved, should be cleared */
> + __be32 res9; /* reserved, should be cleared */
> + __be32 res10; /* reserved, should be cleared */
> + __be32 res11; /* reserved, should be cleared */
> + __be32 res12; /* reserved, should be cleared */
> + __be32 res13; /* reserved, should be cleared */
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
> + __be16 supsmr; /* 0x90, Shadow UPSMR */
> + __be16 res92; /* 0x92, reserved, initialize to 0 */
> + __be32 rx_state; /* 0x94, RX state, initialize to 0 */
> + __be32 rx_cnt; /* 0x98, RX count, initialize to 0 */
> + u8 rx_length; /* 0x9C, Char length, set to 1+CL+PEN+1+SL */
> + u8 rx_bitmark; /* 0x9D, reserved, initialize to 0 */
> + u8 rx_temp_dlst_qe; /* 0x9E, reserved, initialize to 0 */
> + u8 res14[0xBC - 0x9F]; /* reserved */
> + __be32 dump_ptr; /* 0xBC, Dump pointer */
> + __be32 rx_frame_rem; /* 0xC0, reserved, initialize to 0 */
> + u8 rx_frame_rem_size; /* 0xC4, reserved, initialize to 0 */
> + u8 tx_mode; /* 0xC5, mode, 0=AHDLC, 1=UART */
> + u16 tx_state; /* 0xC6, TX state */
> + u8 res15[0xD0 - 0xC8]; /* reserved */
> + __be32 resD0; /* 0xD0, reserved, initialize to 0 */
> + u8 resD4; /* 0xD4, reserved, initialize to 0 */
> + __be16 resD5; /* 0xD5, reserved, initialize to 0 */
> +#endif
> +} __attribute__ ((packed));
The structure is perfectly packed even without your __attribute__ ((packed)),
so you should leave out the attribute in order to get more efficient code
accessing it.
> +
> +#ifdef DEBUG
> +static void dump_ucc_uart_pram(struct ucc_uart_pram __iomem *uccup)
> +{
> + unsigned int i;
Do you really need the debugging function like this in the code?
Usually they are rather pointless once the code works, and will
suffer from bitrot because nobody enables the code.
> +
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
> +#define UCC_UART_SUPSMR_SL 0x8000
> +#define UCC_UART_SUPSMR_RPM_MASK 0x6000
> +#define UCC_UART_SUPSMR_RPM_ODD 0x0000
> +#define UCC_UART_SUPSMR_RPM_LOW 0x2000
again, the #ifdef should be left out if it can.
> + * Given the virtual address for a character buffer, this function returns
> + * the physical (DMA) equivalent.
> + */
> +static inline dma_addr_t cpu2qe_addr(void *addr, struct uart_qe_port
> *qe_port)
> +{
> + if (likely((addr >= qe_port->bd_virt)) &&
> + (addr < (qe_port->bd_virt + qe_port->bd_size)))
> + return qe_port->bd_phys + (addr - qe_port->bd_virt);
> +
> + /* something nasty happened */
> + printk(KERN_ERR "%s: addr=%p\n", __FUNCTION__, addr);
> + BUG();
> + return 0;
> +}
I'm guessing that you don't really mean dma_addr_t here, but rather
phys_addr_t, which is something different.
> +
> +/*
> + * Physical to virtual address translation.
> + *
> + * Given the physical (DMA) address for a character buffer, this function
> + * returns the virtual equivalent.
> + */
> +static inline void *qe2cpu_addr(dma_addr_t addr, struct uart_qe_port
> *qe_port)
same here.
Arnd <><
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
