EG20T: Update PCH_UART driver to 2.6.36
PATCH 1/1 EG20T: Update PCH_UART driver to 2.6.36
UART driver of Intel EG20T(Topcliff) PCH
Intel EG20T PCH is the platform controller hub that is going to be used in Intel's general embedded platform. All IO peripherals in
Intel EG20T PCH are actually devices sitting on AMBA bus. Intel EG20T PCH has UART I/F. Using this I/F, it is able to access system devices connected to UART.
--- drivers/serial/Kconfig | 8 + drivers/serial/Makefile | 1 + drivers/serial/pch_uart.c | 1549 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1558 insertions(+), 0 deletions(-) create mode 100644 drivers/serial/pch_uart.c
drivers/serial/Kconfig +8 -0
@@ -1632,4 +1632,12 @@ config SERIAL_ALTERA_UART_CONSOLE help Enable a Altera UART port to be the system console. +config SERIAL_PCH_UART + tristate "Intel EG20T PCH UART" + select SERIAL_CORE + depends on PCI + help + This driver is for PCH(Platform controller Hub) UART of Intel EG20T + which is an IOH(Input/Output Hub) for x86 embedded processor. + Enabling PCH_DMA, this PCH UART works as DMA mode. endmenu
drivers/serial/Makefile +1 -0
@@ -89,3 +89,4 @@ obj-$(CONFIG_SERIAL_ALTERA_UART) += altera_uart.o obj-$(CONFIG_SERIAL_MRST_MAX3110) += mrst_max3110.o obj-$(CONFIG_SERIAL_MFD_HSU) += mfd.o obj-$(CONFIG_SERIAL_OMAP) += omap-serial.o +obj-$(CONFIG_SERIAL_PCH_UART) += pch_uart.o
drivers/serial/pch_uart.c new file +1549 -0
1,549 lines not on this page · raw message
Signature · 1 line
1.6.0.6
drivers/serial/pch_uart.c line 28
Quoted 5 lines
drivers/serial/pch_uart.c new file +4
-0+#ifdef CONFIG_PCH_DMA +#include <linux/dmaengine.h> +#include <linux/pch_dma.h> +#endif
The PCH DMA will have been included in the kernel anyway before this merge so you can remove the ifdefs and include it regardless
Review
Quoted 10 lines
+#if !defined(PORT_PCH_256FIFO) || !defined(PORT_PCH_64FIFO)
+ #undefPORT_PCH_256FIFO
+ #undefPORT_PCH_64FIFO
+ #definePORT_PCH_256FIFO(PORT_MAX_8250+1) /* PCH UART with
256 byte(quoted diff) +2
-0+ FIFO */ + #define PORT_PCH_64FIFO (PORT_MAX_8250+2) /* PCH UART with64 byte
(quoted diff) +2
-0+ FIFO */ +#endif
FIFO config really all wants to be done at runtime.
drivers/serial/pch_uart.c line 312
Quoted 6 lines
drivers/serial/pch_uart.c new file +6
-0+static inline void wr (void __iomem *addr,unsigned int value) +{ +#if PCH_UART_DMA_REG_BOUNDARY ==4 +iowrite32 (value, addr); +#else +iowrite8 (value, (void *)addr);
Why the cast ?
Quoted 1 line
+#endif
Again you've got magic ifdefs with no explanation ?
drivers/serial/pch_uart.c line 326
Quoted 13 lines
drivers/serial/pch_uart.c new file +13
-0+} + +static void pch_uart_hal_request (struct pci_dev *pdev,int fifosize, +int base_baud) +{ +struct eg20t_port *priv =pci_get_drvdata (pdev); + + priv->trigger_level =1 ; + priv->fcr =0 ; +} + +static int __get_msr (struct eg20t_port *priv,void __iomem *base) +{
Please avoid __ names for functions
drivers/serial/pch_uart.c line 332
Quoted 6 lines
drivers/serial/pch_uart.c new file +6
-0+unsigned int msr; + + msr =rr (base +PCH_UART_MSR ); + priv->dmsr |= msr &PCH_UART_MSR_DELTA ; + +return (int )msr;
Why - if it is unsigned then why not return unsigned values ?
drivers/serial/pch_uart.c line 356
Quoted 10 lines
drivers/serial/pch_uart.c new file +10
-0+static int pch_uart_hal_enable_interrupt (struct eg20t_port *priv, +unsigned int flag) +{ +void __iomem *base; +int ret; + + base = priv->membase; + ret =__pch_uart_hal_enable_interrupt (base, flag); + +return ret;
Why not just
"return __pch_uart_hal_enable_interrupt(priv->membase, flag);"
in fact why not just remove all these wrappers entirely ?
drivers/serial/pch_uart.c line 592
Quoted 11 lines
drivers/serial/pch_uart.c new file +11
-0+static int push_rx (struct eg20t_port *priv,const unsigned char *buf, +int size) +{ +struct uart_port *port; +struct tty_struct *tty; +int sz, i, j; +int loop; +int pushed; + + port = &priv->port; + tty = port->state->port.tty;
tty ports are refcounted
tty = tty_port_tty_get(...)
and when finished tty_kref_put(tty);
Also tty may be NULL, if the port closed as you were doing this, so check the return from tty_port_tty_get and act accordingly.
drivers/serial/pch_uart.c line 692
Quoted 5 lines
drivers/serial/pch_uart.c new file +5
-0+static int handle_error (struct eg20t_port *priv,int err) +{ +int ret =0 ; +return ret; +}
Why ?????
drivers/serial/pch_uart.c line 694
Quoted 2 lines
drivers/serial/pch_uart.c new file +2
-0+ +#ifndef CONFIG_PCH_DMA
DMA should be runtime configuration - vendors need to build generic kernels so need things like DMA switching to be done on load not on compile
Quoted 3 lines
+static void
pch_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)+{
+}
Seems to be unimplemented ?
Quoted 27 lines
(quoted diff) +2
-0+static void pch_uart_set_termios(struct uart_port *port, + struct ktermios *termios, structktermios *old) +{
drivers/serial/pch_uart.c new file +24
-0+int baud; +unsigned int parity, bits, stb; +struct eg20t_port *priv; +unsigned long flags; + + priv = container_of(port, struct eg20t_port, port); +switch (termios->c_cflag &CSIZE ) { +case CS5 : + bits =PCH_UART_HAL_5BIT ; +break ; +case CS6 : + bits =PCH_UART_HAL_6BIT ; +break ; +case CS7 : + bits =PCH_UART_HAL_7BIT ; +break ; +default :/* CS8 */ + bits =PCH_UART_HAL_8BIT ; +break ; + } +if (termios->c_cflag &CSTOPB ) + stb =PCH_UART_HAL_STB2 ; +else + stb =PCH_UART_HAL_STB1 ;
drivers/serial/pch_uart.c line 1257
Quoted 10 lines
drivers/serial/pch_uart.c new file +10
-0+ +if (termios->c_cflag &PARENB ) { +if (!(termios->c_cflag &PARODD )) + parity =PCH_UART_HAL_PARITY_ODD ; +else + parity =PCH_UART_HAL_PARITY_EVEN ; + + }else { + parity =PCH_UART_HAL_PARITY_NONE ; + }
If you don't support CPARMRK then you should clear that bit in termios->c_flag so the caller knows it couldn't be set.
drivers/serial/pch_uart.c line 1263
Quoted 6 lines
(quoted diff) +1
-0+ baud = uart_get_baud_rate(port, termios, old, 0,
port->uartclk/ 16); +drivers/serial/pch_uart.c new file +4
-0+spin_lock_irqsave (&port->lock, flags); + +uart_update_timeout (port, termios->c_cflag, baud); +pch_uart_hal_set_line (priv, baud, parity, bits, stb);
Baud rate should also be written back here
/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
tty_termios_encode_baud_rate(termios, baud, baud);drivers/serial/pch_uart.c line 1371
Quoted 7 lines
drivers/serial/pch_uart.c new file +7
-0+ txbuf =__get_free_page (GFP_KERNEL |GFP_DMA ); +if (!txbuf) + goto init_port_error_end; + + rxbuf =__get_free_page (GFP_KERNEL |GFP_DMA ); +if (!rxbuf) + goto init_port_free_txbuf;
No - for bus masterable DMA buffers use the dma_alloc_coherent interfaces with the correct device pointer, otherwise it will break on a system with an IOMMU.
I assume the correct device in this case would be the DMA controller ?
The big thing I don't understand here is the locking model - what stops interrupts and other things interfering with each other. For almost all of the calls coming from the serial layer the port lock protects them but I see no protection on the IRQ side at all ?
Quoted 8 lines · Alan Cox
drivers/serial/pch_uart.c line 28
Quoted 5 lines
drivers/serial/pch_uart.c new file +4
-0+#ifdef CONFIG_PCH_DMA +#include <linux/dmaengine.h> +#include <linux/pch_dma.h> +#endif The PCH DMA will have been included in the kernel anyway before this merge so you can remove the ifdefs and include it regardless
No.
PCH DMA have been already included at 2.6.36. You can see "pch_dma.c" in drivers/dma.
Quoted 14 lines
Review
Quoted 12 lines
+#if !defined(PORT_PCH_256FIFO) || !defined(PORT_PCH_64FIFO)
+ #undefPORT_PCH_256FIFO
+ #undefPORT_PCH_64FIFO
+ #definePORT_PCH_256FIFO(PORT_MAX_8250+1) /* PCH UART with 256 byte
+ FIFO */
+ #definePORT_PCH_64FIFO(PORT_MAX_8250+2) /* PCH UART with 64 byte(quoted diff) +2
-0+ FIFO */ +#endifFIFO config really all wants to be done at runtime.
I will modify.
Quoted 11 lines
drivers/serial/pch_uart.c line 312
Quoted 9 lines
drivers/serial/pch_uart.c new file +6
-0+static inline void wr (void __iomem *addr,unsigned int value) +{ +#if PCH_UART_DMA_REG_BOUNDARY ==4 +iowrite32 (value, addr); +#else +iowrite8 (value, (void *)addr);Why the cast ?
I will delete the cast.
Quoted 4 lines
Quoted 2 lines
+#endif
Again you've got magic ifdefs with no explanation ?
I will delete this.
Quoted 7 lines
Quoted 7 lines
drivers/serial/pch_uart.c new file +7
-0+} + +static void pch_uart_hal_request (struct pci_dev *pdev,int fifosize, +int base_baud) +{ +struct eg20t_port *priv =pci_get_drvdata (pdev); +
Quoted 8 lines
drivers/serial/pch_uart.c line 326
Quoted 6 lines
+
priv->trigger_level= 1;
+priv->fcr= 0;drivers/serial/pch_uart.c new file +4
-0+} + +static int __get_msr (struct eg20t_port *priv,void __iomem *base) +{Please avoid __ names for functions
I will modify.
Quoted 9 lines
drivers/serial/pch_uart.c line 332
Quoted 7 lines
+ unsigned int msr;
drivers/serial/pch_uart.c new file +5
-0+ + msr =rr (base +PCH_UART_MSR ); + priv->dmsr |= msr &PCH_UART_MSR_DELTA ; + +return (int )msr;Why - if it is unsigned then why not return unsigned values ?
I will modify.
Quoted 15 lines
drivers/serial/pch_uart.c line 356
Quoted 12 lines
drivers/serial/pch_uart.c new file +10
-0+static int pch_uart_hal_enable_interrupt (struct eg20t_port *priv, +unsigned int flag) +{ +void __iomem *base; +int ret; + + base = priv->membase; + ret =__pch_uart_hal_enable_interrupt (base, flag); + +return ret;Why not just
Quoted 3 lines
"return
__pch_uart_hal_enable_interrupt(priv->membase, flag);"in fact why not just remove all these wrappers entirely ?
I agree.
I will delete thease wrappers.
Quoted 16 lines
drivers/serial/pch_uart.c line 592
Quoted 13 lines
drivers/serial/pch_uart.c new file +11
-0+static int push_rx (struct eg20t_port *priv,const unsigned char *buf, +int size) +{ +struct uart_port *port; +struct tty_struct *tty; +int sz, i, j; +int loop; +int pushed; + + port = &priv->port; + tty = port->state->port.tty;tty ports are refcounted
Quoted 6 lines
tty =
tty_port_tty_get(...)and when finished
tty_kref_put(tty);Also tty may be NULL, if the port closed as you were doing this, so check the return from
tty_port_tty_getand act accordingly.
I have a question.
Though I can't find any accepted UART driver uses these functions(tty_port.../tty_kref...), What's for ?
Quoted 10 lines
drivers/serial/pch_uart.c line 692
Quoted 7 lines
drivers/serial/pch_uart.c new file +5
-0+static int handle_error (struct eg20t_port *priv,int err) +{ +int ret =0 ; +return ret; +}Why ?????
I will modify.
Quoted 7 lines
drivers/serial/pch_uart.c line 694
Quoted 2 lines
drivers/serial/pch_uart.c new file +2
-0+ +#ifndef CONFIG_PCH_DMA DMA should be runtime configuration - vendors need to build generic kernels so need things like DMA switching to be done on load not on compile
Do you mean we souldn't use both below?
#ifndef CONFIG_PCH_DMA
#ifdef CONFIG_PCH_DMA
Many UART drivers accepted by upstream use the above macro, right ?
Quoted 8 lines
Quoted 6 lines
+static void
pch_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)+{
+}Seems to be unimplemented ?
I will implement.
Quoted 34 lines
Quoted 34 lines
+static void pch_uart_set_termios(struct
uart_port*port,
+ struct ktermios *termios, struct
ktermios *old) +{
+ int baud;
+ unsigned int parity, bits, stb;
+ structeg20t_port*priv;
+ unsigned long flags;drivers/serial/pch_uart.c new file +25
-0+ + priv = container_of(port, struct eg20t_port, port); +switch (termios->c_cflag &CSIZE ) { +case CS5 : + bits =PCH_UART_HAL_5BIT ; +break ; +case CS6 : + bits =PCH_UART_HAL_6BIT ; +break ; +case CS7 : + bits =PCH_UART_HAL_7BIT ; +break ; +default :/* CS8 */ + bits =PCH_UART_HAL_8BIT ; +break ; + } +if (termios->c_cflag &CSTOPB ) + stb =PCH_UART_HAL_STB2 ; +else + stb =PCH_UART_HAL_STB1 ; + +if (termios->c_cflag &PARENB ) { +if (!(termios->c_cflag &PARODD )) + parity =PCH_UART_HAL_PARITY_ODD ; +else
Quoted 8 lines
Quoted 5 lines
+ parity =
PCH_UART_HAL_PARITY_EVEN;
+
+ } else {
+ parity =PCH_UART_HAL_PARITY_NONE;
+ }If you don't support CPARMRK then you should clear that bit in
termios->c_flagso the caller knows it couldn't be set.
Sorry, I don't know CPARMRK.
What's CPARMRK ?
Quoted 9 lines
Quoted 8 lines
+ baud =
uart_get_baud_rate(port, termios, old, 0, port->uartclk / 16); +
+spin_lock_irqsave(&port->lock, flags);
+
+uart_update_timeout(port, termios->c_cflag, baud);
+pch_uart_hal_set_line(priv, baud, parity, bits, stb);
Quoted 6 lines
Baud rate should also be written back here
/* Don't rewrite B0 */ if (tty_termios_baud_rate(termios)) tty_termios_encode_baud_rate(termios, baud, baud);
I will add above.
Quoted 11 lines
Quoted 7 lines
+ txbuf =
__get_free_page(GFP_KERNEL|GFP_DMA);
+ if (!txbuf)
+ gotoinit_port_error_end;
+
+ rxbuf =__get_free_page(GFP_KERNEL|GFP_DMA);
+ if (!rxbuf)
+ gotoinit_port_free_txbuf;No - for bus masterable DMA buffers use the
dma_alloc_coherentinterfaces with the correct device pointer, otherwise it will break on a system with an IOMMU.
I will use dma_alloc_coherent.
Quoted 2 lines
I assume the correct device in this case would be the DMA controller ?
Sorry, I can't understand your saying "correct device". What does "correct device" mean ?
Quoted 7 lines
The big thing I don't understand here is the locking model - what stops interrupts and other things interfering with each other. For almost all of the calls coming from the serial layer the port lock protects them but I see no protection on the IRQ side at all ?
Do you mean UART Rx/Tx interrupt Enable/Disable ? If yes, the control hava been already implemented. For example, in handle_tx (called from IRQ handler), you can see pch_uart_hal_disable_interrupt().
---
Thanks,
Tomoya MORINAGA
OKI SEMICONDUCTOR CO., LTD.
Quoted 11 lines
Quoted 7 lines
drivers/serial/pch_uart.c line 28
Quoted 4 lines
drivers/serial/pch_uart.c new file +4
-0+#ifdef CONFIG_PCH_DMA +#include <linux/dmaengine.h> +#include <linux/pch_dma.h> +#endif The PCH DMA will have been included in the kernel anyway before this merge so you can remove the ifdefs and include it regardless
No.
PCH DMA have been already included at 2.6.36. You can see "pch_dma.c" indrivers/dma.
So that means pch_dma is always in the kernel source tree as of 2.6.36
- so the ifdef can go away ?
Quoted 10 lines
Quoted 6 lines
tty =
tty_port_tty_get(...)and when finished
tty_kref_put(tty);Also tty may be NULL, if the port closed as you were doing this, so check the return from
tty_port_tty_getand act accordingly.I have a question.
Though I can't find any accepted UART driver uses thesefunctions(tty_port.../tty_kref...), What's for ?
We are gradually going through converting them all and I'm trying to make sure no new ones creep in that don't use tty_port_tty_get.
Basically it does this
tty_port_tty_get()
will return either a tty pointer with a reference (ie the tty will not be deleted while you hold it), or will return NULL if the physical port is no longer connected to a tty through close/hangup. It does all the locking internally to ensure this is done safely.
tty_kref_put()
drops the reference it obtained and after that point if the tty reference count hits zero it may be deleted.
It ensures this cannot occur
CPU1 CPU2
interrupt:
tty = port->port.tty close port->port.tty = NULL kfree tty write to tty
Quoted 1 line
Many UART drivers accepted by upstream use the above macro, right ?
As far as possible the selection should be done at runtime, especially for PC class devices. Linux distributors want to build a single kernel for everything so having it runtime configured helps a lot.
Quoted 5 lines
Quoted 2 lines
If you don't support CPARMRK then you should clear that bit in
termios->c_flagso the caller knows it couldn't be set.Sorry, I don't know CPARMRK.
What's CPARMRK ?
Sorry my fault. I mean CMSPAR
CMSPAR if set selects mark/space parity. If your hardware doesn't support this then do
termios->c_cflag &= ~CMSPAR;
Quoted 8 lines
I will use
dma_alloc_coherent.Quoted 3 lines
I assume the correct device in this case would be the DMA controller ?
Sorry, I can't understand your saying "correct device". What does "correct device" mean ?
dma_alloc_coherent takes a device pointer which should be the device which is doing the DMA.
Quoted 4 lines
Do you mean UART Rx/Tx interrupt Enable/Disable ? If yes, the control hava been already implemented. For example, in
handle_tx(called from IRQ handler), you can seepch_uart_hal_disable_interrupt().
I will take another look next time the patch is submitted. However if you are relying on disabling an IRQ source to avoid the interrupt running in parallel with another routine remember on a multithreaded CPU and on some hardware that it is possible for this to occur
CPU1 CPU2 Interrupt arrives Disable interrupt Interrupt routine runs Other code runs
Alan
Quoted 10 lines · Alan Cox
Quoted 7 lines
Quoted 3 lines
The PCH DMA will have been included in the kernel anyway before this merge so you can remove the ifdefs and include it regardless
No.
PCH DMA have been already included at 2.6.36. You can see "pch_dma.c" indrivers/dma.So that means
pch_dmais always in the kernel source tree as of 2.6.36
- so the ifdef can go away ?
In case low traffic,
If UART driver is DMA mode only,
1byte data is also transferred using DMA.
This overhead is too big.
As a result, 1byte DMA transfer is slower than Non-DMA mode. Thus, I think Non-DMA mode is also necessary.
Quoted 31 lines
Quoted 11 lines
Quoted 7 lines
tty =
tty_port_tty_get(...)and when finished
tty_kref_put(tty);Also tty may be NULL, if the port closed as you were doing this, so check the return from
tty_port_tty_getand act accordingly.I have a question.
Though I can't find any accepted UART driver uses thesefunctions(tty_port.../tty_kref...), What's for ?We are gradually going through converting them all and I'm trying to make sure no new ones creep in that don't use
tty_port_tty_get.Basically it does this
tty_port_tty_get()will return either a tty pointer with a reference (ie the tty will not be deleted while you hold it), or will return NULL if the physical port is no longer connected to a tty through close/hangup. It does all the locking internally to ensure this is done safely.
tty_kref_put()drops the reference it obtained and after that point if the tty reference count hits zero it may be deleted.
It ensures this cannot occur
Quoted 8 lines
CPU1 CPU2
interrupt:
tty =port->port.tty
closeport->port.tty= NULL
kfree tty
write to tty
Thank you for yor explanation.
I will add above.
Quoted 16 lines
Quoted 4 lines
Many UART drivers accepted by upstream use the above macro, right ?
As far as possible the selection should be done at runtime, especially for PC class devices. Linux distributors want to build a single kernel for everything so having it runtime configured helps a lot.
Quoted 5 lines
Quoted 2 lines
If you don't support CPARMRK then you should clear that bit in
termios->c_flagso the caller knows it couldn't be set.Sorry, I don't know CPARMRK.
What's CPARMRK ?
Quoted 6 lines
Sorry my fault. I mean CMSPAR
CMSPAR if set selects mark/space parity. If your hardware doesn't support this then do
termios->c_cflag&= ~CMSPAR;
eg20t doesn't support mark/space parity.
I will add above.
Quoted 3 lines
Quoted 3 lines
I will use
dma_alloc_coherent.
I have modified and test.
Both __get_free_page and dma_alloc_coherent work well. I have a question.
Could you tell me the defferencies
__get_free_page(GFP_DMA) and dma_alloc_coherent ?
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
I assume the correct device in this case would be the DMA controller ?
Sorry, I can't understand your saying "correct device". What does "correct device" mean ?
dma_alloc_coherenttakes a device pointer which should be the device which is doing the DMA.
Yes, UART cooperates with DMA controller for UART DMA transfer. Can you satisfy the above my answer ?
(I may not understand your intension correctly)
Quoted 12 lines
Quoted 5 lines
Do you mean UART Rx/Tx interrupt Enable/Disable ? If yes, the control hava been already implemented. For example, in
handle_tx(called from IRQ handler), you can seepch_uart_hal_disable_interrupt().I will take another look next time the patch is submitted. However if you are relying on disabling an IRQ source to avoid the interrupt running in parallel with another routine remember on a multithreaded CPU and on some hardware that it is possible for this to occur
Quoted 6 lines
CPU1 CPU2
Interrupt arrives
Disable interrupt
Interrupt routine runs Other code runs
Yes, I have used spin_lock_irqsave.
Do you mean we should implement another method for lock model ?
I will post the latst patch soon.
--
Thanks,
Tomoya MORINAGA
OKI SEMICONDUCTOR CO., LTD.