[PATCH v1 00/12] serial: 8250: split LPSS to 8250_lpss, enable DMA on Quark

[PATCH v1 00/12] serial: 8250: split LPSS to 8250_lpss, enable DMA on Quark

[Andy Shevchenko]

This is combined series of two things:
 - split out the Intel LPSS specific driver from 8250_pci into 8250_lpss
 - enable DMA support on Intel Quark UART

The patch has been tested on few Intel SoCs / platforms.

This is targeting serial subsystem, thus it would be nice to get and Ack from Vinod first.
Moreover, the series depends on series [1] that is now under review.

That's why I ask Vinod to create immutable tag / branch for the [1] and the
dependants (at least one more, which is sata_dwc_460ex).

[1] http://www.spinics.net/lists/dmaengine/msg08709.html

Andy Shevchenko (12):
  dmaengine: dw: keep copy of custom slave config in dwc
  dmaengine: dw: provide probe(), remove() stubs for users
  dmaengine: dw: set polarity of handshake interface
  dmaengine: dw: override LLP support if asked in platform data
  serial: 8250_dma: switch to new dmaengine_terminate_* API
  serial: 8250_dma: stop ongoing RX DMA on exception
  serial: 8250_dma: adjust DMA address of the UART
  serial: 8250: enable AFE on ports where FIFO is 16 bytes
  serial: 8250_lpss: split LPSS driver to separate module
  serial: 8250_lpss: move Quark code from PCI driver
  serial: 8250_lpss: enable MSI for Intel Quark
  serial: 8250_lpss: enable DMA on Intel Quark UART

 drivers/dma/dw/core.c                |  41 ++--
 drivers/dma/dw/regs.h                |   5 +-
 drivers/tty/serial/8250/8250.h       |   2 +
 drivers/tty/serial/8250/8250_dma.c   |  26 ++-
 drivers/tty/serial/8250/8250_lpss.c  | 356 +++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_pci.c   | 242 +-----------------------
 drivers/tty/serial/8250/8250_port.c  |   4 +-
 drivers/tty/serial/8250/Kconfig      |  14 +-
 drivers/tty/serial/8250/Makefile     |   1 +
 include/linux/dma/dw.h               |   5 +
 include/linux/platform_data/dma-dw.h |   4 +
 11 files changed, 429 insertions(+), 271 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_lpss.c

-- 
2.8.0.rc3

[PATCH v1 01/12] dmaengine: dw: keep copy of custom slave config in dwc

[Andy Shevchenko]

It seems we need to extend custom slave configuration by one more member to
support Intel Quart UART. It becomes a burden to manage all members of struct
dw_dma_slave one-by-one.

Replace set of fields by embedding struct dw_dma_slave into struct dw_dma_chan.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw/core.c | 29 ++++++++++-------------------
 drivers/dma/dw/regs.h |  5 +----
 2 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 86e55ab..840f0da 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -46,9 +46,9 @@
 		u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :	\
 			DW_DMA_MSIZE_16;			\
 		u8 _dms = (_dwc->direction == DMA_MEM_TO_DEV) ?		\
-			_dwc->p_master : _dwc->m_master;		\
+			_dwc->dws.p_master : _dwc->dws.m_master;	\
 		u8 _sms = (_dwc->direction == DMA_DEV_TO_MEM) ?		\
-			_dwc->p_master : _dwc->m_master;		\
+			_dwc->dws.p_master : _dwc->dws.m_master;	\
 								\
 		(DWC_CTLL_DST_MSIZE(_dmsize)			\
 		 | DWC_CTLL_SRC_MSIZE(_smsize)			\
@@ -140,8 +140,8 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 	if (test_bit(DW_DMA_IS_INITIALIZED, &dwc->flags))
 		return;
 
-	cfghi |= DWC_CFGH_DST_PER(dwc->dst_id);
-	cfghi |= DWC_CFGH_SRC_PER(dwc->src_id);
+	cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
+	cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
 
 	channel_writel(dwc, CFG_LO, cfglo);
 	channel_writel(dwc, CFG_HI, cfghi);
@@ -202,7 +202,7 @@ static inline void dwc_do_single_block(struct dw_dma_chan *dwc,
 static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 {
 	struct dw_dma	*dw = to_dw_dma(dwc->chan.device);
-	u8		lms = DWC_LLP_LMS(dwc->m_master);
+	u8		lms = DWC_LLP_LMS(dwc->dws.m_master);
 	unsigned long	was_soft_llp;
 
 	/* ASSERT:  channel is idle */
@@ -686,7 +686,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 	unsigned int		src_width;
 	unsigned int		dst_width;
 	u32			ctllo;
-	u8			lms = DWC_LLP_LMS(dwc->m_master);
+	u8			lms = DWC_LLP_LMS(dwc->dws.m_master);
 
 	dev_vdbg(chan2dev(chan),
 			"%s: d%pad s%pad l0x%zx f0x%lx\n", __func__,
@@ -759,7 +759,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	struct dw_desc		*prev;
 	struct dw_desc		*first;
 	u32			ctllo;
-	u8			lms = DWC_LLP_LMS(dwc->m_master);
+	u8			lms = DWC_LLP_LMS(dwc->dws.m_master);
 	dma_addr_t		reg;
 	unsigned int		reg_width;
 	unsigned int		mem_width;
@@ -912,12 +912,7 @@ bool dw_dma_filter(struct dma_chan *chan, void *param)
 		return false;
 
 	/* We have to copy data since dws can be temporary storage */
-
-	dwc->src_id = dws->src_id;
-	dwc->dst_id = dws->dst_id;
-
-	dwc->m_master = dws->m_master;
-	dwc->p_master = dws->p_master;
+	memcpy(&dwc->dws, dws, sizeof(struct dw_dma_slave));
 
 	return true;
 }
@@ -1221,11 +1216,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	dwc->descs_allocated = 0;
 
 	/* Clear custom channel configuration */
-	dwc->src_id = 0;
-	dwc->dst_id = 0;
-
-	dwc->m_master = 0;
-	dwc->p_master = 0;
+	memset(&dwc->dws, 0, sizeof(struct dw_dma_slave));
 
 	clear_bit(DW_DMA_IS_INITIALIZED, &dwc->flags);
 
@@ -1323,7 +1314,7 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 	struct dw_cyclic_desc		*retval = NULL;
 	struct dw_desc			*desc;
 	struct dw_desc			*last = NULL;
-	u8				lms = DWC_LLP_LMS(dwc->m_master);
+	u8				lms = DWC_LLP_LMS(dwc->dws.m_master);
 	unsigned long			was_cyclic;
 	unsigned int			reg_width;
 	unsigned int			periods;
diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
index 5b3ea2d..f9e56f9 100644
--- a/drivers/dma/dw/regs.h
+++ b/drivers/dma/dw/regs.h
@@ -246,10 +246,7 @@ struct dw_dma_chan {
 	bool			nollp;
 
 	/* custom slave configuration */
-	u8			src_id;
-	u8			dst_id;
-	u8			m_master;
-	u8			p_master;
+	struct dw_dma_slave	dws;
 
 	/* configuration passed via .device_config */
 	struct dma_slave_config dma_sconfig;
-- 
2.8.0.rc3

[PATCH v1 02/12] dmaengine: dw: provide probe(), remove() stubs for users

[Andy Shevchenko]

Some users consider DMA optional, thus when driver is not compiled we shouldn't
prevent compilation of the users. Add stubs for dw_dma_probe() and
dw_dma_remove().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/dma/dw.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/dma/dw.h b/include/linux/dma/dw.h
index f2e538a..ccfd0c3 100644
--- a/include/linux/dma/dw.h
+++ b/include/linux/dma/dw.h
@@ -40,8 +40,13 @@ struct dw_dma_chip {
 };
 
 /* Export to the platform drivers */
+#if IS_ENABLED(CONFIG_DW_DMAC_CORE)
 int dw_dma_probe(struct dw_dma_chip *chip);
 int dw_dma_remove(struct dw_dma_chip *chip);
+#else
+static inline int dw_dma_probe(struct dw_dma_chip *chip) { return -ENODEV; }
+static inline int dw_dma_remove(struct dw_dma_chip *chip) { return 0; }
+#endif /* CONFIG_DW_DMAC_CORE */
 
 /* DMA API extensions */
 struct dw_desc;
-- 
2.8.0.rc3

[PATCH v1 03/12] dmaengine: dw: set polarity of handshake interface

[Andy Shevchenko]

Intel Quark UART uses DesignWare DMA IP. Though the DMA IP is connected in such
way that handshake interface uses inverted polarity. We have to provide a
possibility to set this in the DMA driver when configuring a channel.

Introduce a new member of custom slave configuration called 'polarity' and set
active low polarity in case this value is 'true'.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw/core.c                | 2 ++
 include/linux/platform_data/dma-dw.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 840f0da..fc77f4e 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -143,6 +143,8 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 	cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
 	cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
 
+	cfglo |= dwc->dws.polarity ? DWC_CFGL_HS_DST_POL | DWC_CFGL_HS_SRC_POL : 0;
+
 	channel_writel(dwc, CFG_LO, cfglo);
 	channel_writel(dwc, CFG_HI, cfghi);
 
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 06227d0..6196139 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -23,6 +23,7 @@
  * @dst_id:	dst request line
  * @m_master:	memory master for transfers on allocated channel
  * @p_master:	peripheral master for transfers on allocated channel
+ * @polarity:	set active low polarity of handshake interface
  */
 struct dw_dma_slave {
 	struct device		*dma_dev;
@@ -30,6 +31,7 @@ struct dw_dma_slave {
 	u8			dst_id;
 	u8			m_master;
 	u8			p_master;
+	bool			polarity;
 };
 
 /**
-- 
2.8.0.rc3

[PATCH v1 04/12] dmaengine: dw: override LLP support if asked in platform data

[Andy Shevchenko]

There is at least one known device, i.e. UART on Intel Galileo, that works
unreliably in case of use of multi block transfer support in DMA mode.

Override autodetection by user provided data.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw/core.c                | 10 +++++++---
 include/linux/platform_data/dma-dw.h |  2 ++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index fc77f4e..0617880 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1631,9 +1631,13 @@ int dw_dma_probe(struct dw_dma_chip *chip)
 			dwc->block_size = pdata->block_size;
 
 			/* Check if channel supports multi block transfer */
-			channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
-			dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
-			channel_writel(dwc, LLP, 0);
+			if (pdata->is_nollp) {
+				dwc->nollp = pdata->is_nollp;
+			} else {
+				channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
+				dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
+				channel_writel(dwc, LLP, 0);
+			}
 		}
 	}
 
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 6196139..8a9d132 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -40,6 +40,7 @@ struct dw_dma_slave {
  * @is_private: The device channels should be marked as private and not for
  *	by the general purpose DMA channel allocator.
  * @is_memcpy: The device channels do support memory-to-memory transfers.
+ * @is_nollp: The device channels does not support multi block transfers.
  * @chan_allocation_order: Allocate channels starting from 0 or 7
  * @chan_priority: Set channel priority increasing from 0 to 7 or 7 to 0.
  * @block_size: Maximum block size supported by the controller
@@ -50,6 +51,7 @@ struct dw_dma_platform_data {
 	unsigned int	nr_channels;
 	bool		is_private;
 	bool		is_memcpy;
+	bool		is_nollp;
 #define CHAN_ALLOCATION_ASCENDING	0	/* zero to seven */
 #define CHAN_ALLOCATION_DESCENDING	1	/* seven to zero */
 	unsigned char	chan_allocation_order;
-- 
2.8.0.rc3

[PATCH v1 05/12] serial: 8250_dma: switch to new dmaengine_terminate_* API

[Andy Shevchenko]

Convert dmaengine_terminate_all() calls to synchronous and asynchronous
versions where appropriate.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 78259d3c..9d80bb1 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -127,7 +127,7 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 		if (dma->rx_running) {
 			dmaengine_pause(dma->rxchan);
 			__dma_rx_complete(p);
-			dmaengine_terminate_all(dma->rxchan);
+			dmaengine_terminate_async(dma->rxchan);
 		}
 		return -ETIMEDOUT;
 	default:
@@ -230,14 +230,14 @@ void serial8250_release_dma(struct uart_8250_port *p)
 		return;
 
 	/* Release RX resources */
-	dmaengine_terminate_all(dma->rxchan);
+	dmaengine_terminate_sync(dma->rxchan);
 	dma_free_coherent(dma->rxchan->device->dev, dma->rx_size, dma->rx_buf,
 			  dma->rx_addr);
 	dma_release_channel(dma->rxchan);
 	dma->rxchan = NULL;
 
 	/* Release TX resources */
-	dmaengine_terminate_all(dma->txchan);
+	dmaengine_terminate_sync(dma->txchan);
 	dma_unmap_single(dma->txchan->device->dev, dma->tx_addr,
 			 UART_XMIT_SIZE, DMA_TO_DEVICE);
 	dma_release_channel(dma->txchan);
-- 
2.8.0.rc3

Re: [PATCH v1 05/12] serial: 8250_dma: switch to new dmaengine_terminate_* API

[Peter Hurley]

On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> Convert dmaengine_terminate_all() calls to synchronous and asynchronous
> versions where appropriate.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

[Andy Shevchenko]

If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
transfer otherwise it might generates more spurious interrupts and make port
unavailable anymore.

As has been seen on Intel Broxton system:
...
[  168.526281] serial8250: too much work for irq5
[  168.535908] serial8250: too much work for irq5
[  173.449464] serial8250_interrupt: 4439 callbacks suppressed
[  173.455694] serial8250: too much work for irq5
...

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 9d80bb1..b134bec 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -110,6 +110,16 @@ err:
 	return ret;
 }
 
+static void __dma_rx_stop(struct uart_8250_port *p, struct uart_8250_dma *dma)
+{
+	if (!dma->rx_running)
+		return;
+
+	dmaengine_pause(dma->rxchan);
+	__dma_rx_complete(p);
+	dmaengine_terminate_async(dma->rxchan);
+}
+
 int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -118,17 +128,14 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 	switch (iir & 0x3f) {
 	case UART_IIR_RLSI:
 		/* 8250_core handles errors and break interrupts */
+		__dma_rx_stop(p, dma);
 		return -EIO;
 	case UART_IIR_RX_TIMEOUT:
 		/*
 		 * If RCVR FIFO trigger level was not reached, complete the
 		 * transfer and let 8250_core copy the remaining data.
 		 */
-		if (dma->rx_running) {
-			dmaengine_pause(dma->rxchan);
-			__dma_rx_complete(p);
-			dmaengine_terminate_async(dma->rxchan);
-		}
+		__dma_rx_stop(p, dma);
 		return -ETIMEDOUT;
 	default:
 		break;
-- 
2.8.0.rc3

Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

[Peter Hurley]

On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
> transfer otherwise it might generates more spurious interrupts and make port
> unavailable anymore.

Then how to know which rx byte the error is for if dma continues anyway?
What if there are multiple error bytes?


> As has been seen on Intel Broxton system:

This system shouldn't be setup for UART DMA imo.


> ...
> [  168.526281] serial8250: too much work for irq5
> [  168.535908] serial8250: too much work for irq5
> [  173.449464] serial8250_interrupt: 4439 callbacks suppressed
> [  173.455694] serial8250: too much work for irq5
> ...
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 9d80bb1..b134bec 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -110,6 +110,16 @@ err:
>  	return ret;
>  }
>  
> +static void __dma_rx_stop(struct uart_8250_port *p, struct uart_8250_dma *dma)
> +{
> +	if (!dma->rx_running)
> +		return;
> +
> +	dmaengine_pause(dma->rxchan);
> +	__dma_rx_complete(p);
> +	dmaengine_terminate_async(dma->rxchan);
> +}
> +
>  int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
>  {
>  	struct uart_8250_dma		*dma = p->dma;
> @@ -118,17 +128,14 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
>  	switch (iir & 0x3f) {
>  	case UART_IIR_RLSI:
>  		/* 8250_core handles errors and break interrupts */
> +		__dma_rx_stop(p, dma);
>  		return -EIO;
>  	case UART_IIR_RX_TIMEOUT:
>  		/*
>  		 * If RCVR FIFO trigger level was not reached, complete the
>  		 * transfer and let 8250_core copy the remaining data.
>  		 */
> -		if (dma->rx_running) {
> -			dmaengine_pause(dma->rxchan);
> -			__dma_rx_complete(p);
> -			dmaengine_terminate_async(dma->rxchan);
> -		}
> +		__dma_rx_stop(p, dma);
>  		return -ETIMEDOUT;
>  	default:
>  		break;
> 

Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

[Andy Shevchenko]

On Fri, Apr 8, 2016 at 2:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
>> If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
>> transfer otherwise it might generates more spurious interrupts and make port
>> unavailable anymore.
>
> Then how to know which rx byte the error is for if dma continues anyway?
> What if there are multiple error bytes?

And how should it work?
We get an interrupt during DMA, if we don't stop DMA it will be racy
with direct readings.

>
>
>> As has been seen on Intel Broxton system:
>
> This system shouldn't be setup for UART DMA imo.

Same approach is done in 8250_omap.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

[Peter Hurley]

On 04/08/2016 01:07 AM, Andy Shevchenko wrote:
> On Fri, Apr 8, 2016 at 2:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
>>> If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
>>> transfer otherwise it might generates more spurious interrupts and make port
>>> unavailable anymore.
>>
>> Then how to know which rx byte the error is for if dma continues anyway?
>> What if there are multiple error bytes?
> 
> And how should it work?
> We get an interrupt during DMA, if we don't stop DMA it will be racy
> with direct readings.

It makes sense to me that the ongoing DMA needs paused, flushed & terminated,
but the UART should have already aborted the DMA at the first error byte,
so it doesn't make sense to me that the DMA hardware went sideways.

Have you verified that the actual byte in error is reported as the frame/parity
byte and that error-free data is unmangled? Like with a data pattern and a logic
analyzer?


>>
>>
>>> As has been seen on Intel Broxton system:
>>
>> This system shouldn't be setup for UART DMA imo.
> 
> Same approach is done in 8250_omap.

Well, omap8250 has totally different (and possibly unnecessary) rx dma flow.

During the development of the omap8250 driver, it was discovered that the
normal 8250 rx dma flow didn't work reliably on OMAP; ie., the rx dma wouldn't
start once rx uart interrupt had already happened.

*So omap8250 sets up rx dma before any data has been received*
That's the dma that is cancelled when an RLSI interrupt is received;
on OMAP the residue is always 0.

Well, it turns out that the omap8250 rx dma flow *may* be limited to only
1 specific design, the am335x, which has a bunch of other dma issues, with
both tx and rx dma. So all that omap8250 dma handling might be going
away anyway.

IOW, omap8250 is a terrible dma model; do not use.
[Granted the current model needs some work as well; eg., using ping-pong
dma buffers to weather dmaengine descriptor completion latency).

Regards,
Peter Hurley

[PATCH v1 07/12] serial: 8250_dma: adjust DMA address of the UART

[Andy Shevchenko]

Some UARTs, e.g. one is used in Intel Quark, have a different address base for
DMA operations. Introduce an additional field in struct uart_8250_dma to cover
those cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250.h     | 2 ++
 drivers/tty/serial/8250/8250_dma.c | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 047a7ba..040deb2 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -31,6 +31,8 @@ struct uart_8250_dma {
 	struct dma_chan		*rxchan;
 	struct dma_chan		*txchan;
 
+	phys_addr_t		dma_addr;
+
 	dma_addr_t		rx_addr;
 	dma_addr_t		tx_addr;
 
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index b134bec..a314492 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -164,16 +164,17 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 int serial8250_request_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma	*dma = p->dma;
+	phys_addr_t dma_addr = dma->dma_addr ? dma->dma_addr : p->port.mapbase;
 	dma_cap_mask_t		mask;
 
 	/* Default slave configuration parameters */
 	dma->rxconf.direction		= DMA_DEV_TO_MEM;
 	dma->rxconf.src_addr_width	= DMA_SLAVE_BUSWIDTH_1_BYTE;
-	dma->rxconf.src_addr		= p->port.mapbase + UART_RX;
+	dma->rxconf.src_addr		= dma_addr + UART_RX;
 
 	dma->txconf.direction		= DMA_MEM_TO_DEV;
 	dma->txconf.dst_addr_width	= DMA_SLAVE_BUSWIDTH_1_BYTE;
-	dma->txconf.dst_addr		= p->port.mapbase + UART_TX;
+	dma->txconf.dst_addr		= dma_addr + UART_TX;
 
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_SLAVE, mask);
-- 
2.8.0.rc3

Re: [PATCH v1 07/12] serial: 8250_dma: adjust DMA address of the UART

[Peter Hurley]

On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> Some UARTs, e.g. one is used in Intel Quark, have a different address base for
> DMA operations. Introduce an additional field in struct uart_8250_dma to cover
> those cases.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250.h     | 2 ++
>  drivers/tty/serial/8250/8250_dma.c | 5 +++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 047a7ba..040deb2 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -31,6 +31,8 @@ struct uart_8250_dma {
>  	struct dma_chan		*rxchan;
>  	struct dma_chan		*txchan;
>  
> +	phys_addr_t		dma_addr;
> +

Let's add separate rx and tx device addrs.


>  	dma_addr_t		rx_addr;
>  	dma_addr_t		tx_addr;
>  
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index b134bec..a314492 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -164,16 +164,17 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
>  int serial8250_request_dma(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma	*dma = p->dma;
> +	phys_addr_t dma_addr = dma->dma_addr ? dma->dma_addr : p->port.mapbase;
>  	dma_cap_mask_t		mask;
>  
>  	/* Default slave configuration parameters */
>  	dma->rxconf.direction		= DMA_DEV_TO_MEM;
>  	dma->rxconf.src_addr_width	= DMA_SLAVE_BUSWIDTH_1_BYTE;
> -	dma->rxconf.src_addr		= p->port.mapbase + UART_RX;
> +	dma->rxconf.src_addr		= dma_addr + UART_RX;
>  
>  	dma->txconf.direction		= DMA_MEM_TO_DEV;
>  	dma->txconf.dst_addr_width	= DMA_SLAVE_BUSWIDTH_1_BYTE;
> -	dma->txconf.dst_addr		= p->port.mapbase + UART_TX;
> +	dma->txconf.dst_addr		= dma_addr + UART_TX;
>  
>  	dma_cap_zero(mask);
>  	dma_cap_set(DMA_SLAVE, mask);
> 

[PATCH v1 08/12] serial: 8250: enable AFE on ports where FIFO is 16 bytes

[Andy Shevchenko]

Intel Quark has 16550A compatible UART with autoflow feature enabled. It has
only 16 bytes of FIFO. Currently serial8250_do_set_termios() prevents to enable
autoflow since the minimum requirement of 32 bytes of FIFO size.

Decrease a FIFO size limitation to 16 bytes to allow autoflow control be
enabled on such UARTs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index e213da0..3f8121e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2522,9 +2522,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	 * the trigger, or the MCR RTS bit is cleared.  In the case where
 	 * the remote UART is not using CTS auto flow control, we must
 	 * have sufficient FIFO entries for the latency of the remote
-	 * UART to respond.  IOW, at least 32 bytes of FIFO.
+	 * UART to respond.  IOW, at least 16 bytes of FIFO.
 	 */
-	if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
+	if (up->capabilities & UART_CAP_AFE && port->fifosize >= 16) {
 		up->mcr &= ~UART_MCR_AFE;
 		if (termios->c_cflag & CRTSCTS)
 			up->mcr |= UART_MCR_AFE;
-- 
2.8.0.rc3

Re: [PATCH v1 08/12] serial: 8250: enable AFE on ports where FIFO is 16 bytes

[Peter Hurley]

On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> Intel Quark has 16550A compatible UART with autoflow feature enabled. It has
> only 16 bytes of FIFO. Currently serial8250_do_set_termios() prevents to enable
> autoflow since the minimum requirement of 32 bytes of FIFO size.
> 
> Decrease a FIFO size limitation to 16 bytes to allow autoflow control be
> enabled on such UARTs.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e213da0..3f8121e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2522,9 +2522,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>  	 * the trigger, or the MCR RTS bit is cleared.  In the case where
>  	 * the remote UART is not using CTS auto flow control, we must
>  	 * have sufficient FIFO entries for the latency of the remote
> -	 * UART to respond.  IOW, at least 32 bytes of FIFO.
> +	 * UART to respond.  IOW, at least 16 bytes of FIFO.
>  	 */
> -	if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
> +	if (up->capabilities & UART_CAP_AFE && port->fifosize >= 16) {

Let's just remove the fifosize test and rely on UART_CAP_AFE to enable
AFE.  Please remove comment from "In the case where ..."

Also, I think the PORT_A7 port type should have UART_CAP_AFE commented out,
especially since/if the trigger level is 1 byte.

Regards,
Peter Hurley

>  		up->mcr &= ~UART_MCR_AFE;
>  		if (termios->c_cflag & CRTSCTS)
>  			up->mcr |= UART_MCR_AFE;
> 

[PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module

[Andy Shevchenko]

Intes SoCs, such as Braswell, have DesignWare UART. Split out to separate
module which also will be used for Intel Quark later.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 279 ++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_pci.c  | 227 ++---------------------------
 drivers/tty/serial/8250/Kconfig     |  14 +-
 drivers/tty/serial/8250/Makefile    |   1 +
 4 files changed, 301 insertions(+), 220 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_lpss.c

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
new file mode 100644
index 0000000..bca4adb
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -0,0 +1,279 @@
+/*
+ * 8250_lpss.c - Driver for UART on Intel Braswell and various other Intel SoCs
+ *
+ * Copyright (C) 2016 Intel Corporation
+ * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/rational.h>
+
+#include <linux/dmaengine.h>
+#include <linux/platform_data/dma-dw.h>
+
+#include "8250.h"
+
+#define PCI_DEVICE_ID_INTEL_BYT_UART1	0x0f0a
+#define PCI_DEVICE_ID_INTEL_BYT_UART2	0x0f0c
+
+#define PCI_DEVICE_ID_INTEL_BSW_UART1	0x228a
+#define PCI_DEVICE_ID_INTEL_BSW_UART2	0x228c
+
+#define PCI_DEVICE_ID_INTEL_BDW_UART1	0x9ce3
+#define PCI_DEVICE_ID_INTEL_BDW_UART2	0x9ce4
+
+/* Intel LPSS specific registers */
+
+#define BYT_PRV_CLK			0x800
+#define BYT_PRV_CLK_EN			BIT(0)
+#define BYT_PRV_CLK_M_VAL_SHIFT		1
+#define BYT_PRV_CLK_N_VAL_SHIFT		16
+#define BYT_PRV_CLK_UPDATE		BIT(31)
+
+#define BYT_TX_OVF_INT			0x820
+#define BYT_TX_OVF_INT_MASK		BIT(1)
+
+struct lpss8250;
+
+struct lpss8250_board {
+	unsigned long freq;
+	unsigned int base_baud;
+	int (*setup)(struct lpss8250 *, struct uart_port *p);
+};
+
+struct lpss8250 {
+	int line;
+	struct lpss8250_board *board;
+
+	/* DMA parameters */
+	struct uart_8250_dma dma;
+	struct dw_dma_slave dma_param;
+	u8 dma_maxburst;
+};
+
+/*****************************************************************************/
+
+static void lpss8250_set_termios(struct uart_port *p,
+				 struct ktermios *termios,
+				 struct ktermios *old)
+{
+	unsigned int baud = tty_termios_baud_rate(termios);
+	struct lpss8250 *lpss = p->private_data;
+	unsigned long fref = lpss->board->freq, fuart = baud * 16;
+	unsigned long w = BIT(15) - 1;
+	unsigned long m, n;
+	u32 reg;
+
+	/* Get Fuart closer to Fref */
+	fuart *= rounddown_pow_of_two(fref / fuart);
+
+	/*
+	 * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
+	 * dividers must be adjusted.
+	 *
+	 * uartclk = (m / n) * 100 MHz, where m <= n
+	 */
+	rational_best_approximation(fuart, fref, w, w, &m, &n);
+	p->uartclk = fuart;
+
+	/* Reset the clock */
+	reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
+	writel(reg, p->membase + BYT_PRV_CLK);
+	reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
+	writel(reg, p->membase + BYT_PRV_CLK);
+
+	p->status &= ~UPSTAT_AUTOCTS;
+	if (termios->c_cflag & CRTSCTS)
+		p->status |= UPSTAT_AUTOCTS;
+
+	serial8250_do_set_termios(p, termios, old);
+}
+
+/*****************************************************************************/
+
+static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
+{
+	struct dw_dma_slave *param = &lpss->dma_param;
+	struct uart_8250_port *up = up_to_u8250p(port);
+	struct pci_dev *pdev = to_pci_dev(port->dev);
+	struct pci_dev *dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
+
+	switch (pdev->device) {
+	case PCI_DEVICE_ID_INTEL_BYT_UART1:
+	case PCI_DEVICE_ID_INTEL_BSW_UART1:
+	case PCI_DEVICE_ID_INTEL_BDW_UART1:
+		param->src_id = 3;
+		param->dst_id = 2;
+		break;
+	case PCI_DEVICE_ID_INTEL_BYT_UART2:
+	case PCI_DEVICE_ID_INTEL_BSW_UART2:
+	case PCI_DEVICE_ID_INTEL_BDW_UART2:
+		param->src_id = 5;
+		param->dst_id = 4;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	param->dma_dev = &dma_dev->dev;
+	param->m_master = 0;
+	param->p_master = 1;
+
+	/* TODO: Detect FIFO size automaticaly for DesignWare 8250 */
+	port->fifosize = 64;
+	up->tx_loadsz = 64;
+
+	lpss->dma_maxburst = 16;
+
+	port->set_termios = lpss8250_set_termios;
+
+	/* Disable TX counter interrupts */
+	writel(BYT_TX_OVF_INT_MASK, port->membase + BYT_TX_OVF_INT);
+
+	return 0;
+}
+
+/*****************************************************************************/
+
+static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
+{
+	struct dw_dma_slave *dws = param;
+
+	if (dws->dma_dev != chan->device->dev)
+		return false;
+
+	chan->private = dws;
+	return true;
+}
+
+static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port)
+{
+	struct uart_8250_dma *dma = &lpss->dma;
+	struct dw_dma_slave *param = &lpss->dma_param;
+	struct dw_dma_slave *rx_param, *tx_param;
+	struct device *dev = port->port.dev;
+
+	rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
+	if (!rx_param)
+		return -ENOMEM;
+
+	tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
+	if (!tx_param)
+		return -ENOMEM;
+
+	*rx_param = *param;
+	dma->rxconf.src_maxburst = lpss->dma_maxburst;
+
+	*tx_param = *param;
+	dma->txconf.dst_maxburst = lpss->dma_maxburst;
+
+	dma->fn = lpss8250_dma_filter;
+	dma->rx_param = rx_param;
+	dma->tx_param = tx_param;
+
+	port->dma = dma;
+	return 0;
+}
+
+/*****************************************************************************/
+
+static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct uart_8250_port uart;
+	struct lpss8250 *lpss;
+	int ret;
+
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	pci_set_master(pdev);
+
+	lpss = devm_kzalloc(&pdev->dev, sizeof(*lpss), GFP_KERNEL);
+	if (!lpss)
+		return -ENOMEM;
+
+	lpss->board = (struct lpss8250_board *)id->driver_data;
+
+	memset(&uart, 0, sizeof(struct uart_8250_port));
+
+	uart.port.dev = &pdev->dev;
+	uart.port.irq = pdev->irq;
+	uart.port.private_data = lpss;
+	uart.port.type = PORT_16550A;
+	uart.port.iotype = UPIO_MEM32;
+	uart.port.regshift = 2;
+	uart.port.uartclk = lpss->board->base_baud * 16;
+	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
+
+	uart.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
+
+	uart.port.mapbase = pci_resource_start(pdev, 0);
+	uart.port.membase = pcim_iomap(pdev, 0, 0);
+	if (!uart.port.membase)
+		return -ENOMEM;
+
+	if (lpss->board->setup) {
+		ret = lpss->board->setup(lpss, &uart.port);
+		if (ret)
+			return ret;
+	}
+
+	ret = lpss8250_dma_setup(lpss, &uart);
+	if (ret)
+		return ret;
+
+	ret = serial8250_register_8250_port(&uart);
+	if (ret < 0)
+		return ret;
+
+	lpss->line = ret;
+
+	pci_set_drvdata(pdev, lpss);
+	return 0;
+}
+
+static void lpss8250_remove(struct pci_dev *pdev)
+{
+	struct lpss8250 *lpss = pci_get_drvdata(pdev);
+
+	serial8250_unregister_port(lpss->line);
+}
+
+static const struct lpss8250_board byt_board = {
+	.freq = 100000000,
+	.base_baud = 2764800,
+	.setup = byt_serial_setup,
+};
+
+#define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
+
+static const struct pci_device_id pci_ids[] = {
+	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART1, byt_board),
+	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART2, byt_board),
+	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART1, byt_board),
+	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART2, byt_board),
+	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART1, byt_board),
+	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART2, byt_board),
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, pci_ids);
+
+static struct pci_driver lpss8250_pci_driver = {
+	.name           = "8250_lpss",
+	.id_table       = pci_ids,
+	.probe          = lpss8250_probe,
+	.remove         = lpss8250_remove,
+};
+
+module_pci_driver(lpss8250_pci_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel LPSS UART driver");
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 5eea74d..bb4df5d 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -21,14 +21,10 @@
 #include <linux/serial_core.h>
 #include <linux/8250_pci.h>
 #include <linux/bitops.h>
-#include <linux/rational.h>
 
 #include <asm/byteorder.h>
 #include <asm/io.h>
 
-#include <linux/dmaengine.h>
-#include <linux/platform_data/dma-dw.h>
-
 #include "8250.h"
 
 /*
@@ -1349,145 +1345,6 @@ ce4100_serial_setup(struct serial_private *priv,
 	return ret;
 }
 
-#define PCI_DEVICE_ID_INTEL_BYT_UART1	0x0f0a
-#define PCI_DEVICE_ID_INTEL_BYT_UART2	0x0f0c
-
-#define PCI_DEVICE_ID_INTEL_BSW_UART1	0x228a
-#define PCI_DEVICE_ID_INTEL_BSW_UART2	0x228c
-
-#define PCI_DEVICE_ID_INTEL_BDW_UART1	0x9ce3
-#define PCI_DEVICE_ID_INTEL_BDW_UART2	0x9ce4
-
-#define BYT_PRV_CLK			0x800
-#define BYT_PRV_CLK_EN			(1 << 0)
-#define BYT_PRV_CLK_M_VAL_SHIFT		1
-#define BYT_PRV_CLK_N_VAL_SHIFT		16
-#define BYT_PRV_CLK_UPDATE		(1 << 31)
-
-#define BYT_TX_OVF_INT			0x820
-#define BYT_TX_OVF_INT_MASK		(1 << 1)
-
-static void
-byt_set_termios(struct uart_port *p, struct ktermios *termios,
-		struct ktermios *old)
-{
-	unsigned int baud = tty_termios_baud_rate(termios);
-	unsigned long fref = 100000000, fuart = baud * 16;
-	unsigned long w = BIT(15) - 1;
-	unsigned long m, n;
-	u32 reg;
-
-	/* Get Fuart closer to Fref */
-	fuart *= rounddown_pow_of_two(fref / fuart);
-
-	/*
-	 * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
-	 * dividers must be adjusted.
-	 *
-	 * uartclk = (m / n) * 100 MHz, where m <= n
-	 */
-	rational_best_approximation(fuart, fref, w, w, &m, &n);
-	p->uartclk = fuart;
-
-	/* Reset the clock */
-	reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
-	writel(reg, p->membase + BYT_PRV_CLK);
-	reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
-	writel(reg, p->membase + BYT_PRV_CLK);
-
-	p->status &= ~UPSTAT_AUTOCTS;
-	if (termios->c_cflag & CRTSCTS)
-		p->status |= UPSTAT_AUTOCTS;
-
-	serial8250_do_set_termios(p, termios, old);
-}
-
-static bool byt_dma_filter(struct dma_chan *chan, void *param)
-{
-	struct dw_dma_slave *dws = param;
-
-	if (dws->dma_dev != chan->device->dev)
-		return false;
-
-	chan->private = dws;
-	return true;
-}
-
-static int
-byt_serial_setup(struct serial_private *priv,
-		 const struct pciserial_board *board,
-		 struct uart_8250_port *port, int idx)
-{
-	struct pci_dev *pdev = priv->dev;
-	struct device *dev = port->port.dev;
-	struct uart_8250_dma *dma;
-	struct dw_dma_slave *tx_param, *rx_param;
-	struct pci_dev *dma_dev;
-	int ret;
-
-	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
-	if (!dma)
-		return -ENOMEM;
-
-	tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
-	if (!tx_param)
-		return -ENOMEM;
-
-	rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
-	if (!rx_param)
-		return -ENOMEM;
-
-	switch (pdev->device) {
-	case PCI_DEVICE_ID_INTEL_BYT_UART1:
-	case PCI_DEVICE_ID_INTEL_BSW_UART1:
-	case PCI_DEVICE_ID_INTEL_BDW_UART1:
-		rx_param->src_id = 3;
-		tx_param->dst_id = 2;
-		break;
-	case PCI_DEVICE_ID_INTEL_BYT_UART2:
-	case PCI_DEVICE_ID_INTEL_BSW_UART2:
-	case PCI_DEVICE_ID_INTEL_BDW_UART2:
-		rx_param->src_id = 5;
-		tx_param->dst_id = 4;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	rx_param->m_master = 0;
-	rx_param->p_master = 1;
-
-	dma->rxconf.src_maxburst = 16;
-
-	tx_param->m_master = 0;
-	tx_param->p_master = 1;
-
-	dma->txconf.dst_maxburst = 16;
-
-	dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
-	rx_param->dma_dev = &dma_dev->dev;
-	tx_param->dma_dev = &dma_dev->dev;
-
-	dma->fn = byt_dma_filter;
-	dma->rx_param = rx_param;
-	dma->tx_param = tx_param;
-
-	ret = pci_default_setup(priv, board, port, idx);
-	port->port.iotype = UPIO_MEM;
-	port->port.type = PORT_16550A;
-	port->port.flags = (port->port.flags | UPF_FIXED_PORT | UPF_FIXED_TYPE);
-	port->port.set_termios = byt_set_termios;
-	port->port.fifosize = 64;
-	port->tx_loadsz = 64;
-	port->dma = dma;
-	port->capabilities = UART_CAP_FIFO | UART_CAP_AFE;
-
-	/* Disable Tx counter interrupts */
-	writel(BYT_TX_OVF_INT_MASK, port->port.membase + BYT_TX_OVF_INT);
-
-	return ret;
-}
-
 static int
 pci_omegapci_setup(struct serial_private *priv,
 		      const struct pciserial_board *board,
@@ -2015,48 +1872,6 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
 		.subdevice	= PCI_ANY_ID,
 		.setup		= kt_serial_setup,
 	},
-	{
-		.vendor		= PCI_VENDOR_ID_INTEL,
-		.device		= PCI_DEVICE_ID_INTEL_BYT_UART1,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= byt_serial_setup,
-	},
-	{
-		.vendor		= PCI_VENDOR_ID_INTEL,
-		.device		= PCI_DEVICE_ID_INTEL_BYT_UART2,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= byt_serial_setup,
-	},
-	{
-		.vendor		= PCI_VENDOR_ID_INTEL,
-		.device		= PCI_DEVICE_ID_INTEL_BSW_UART1,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= byt_serial_setup,
-	},
-	{
-		.vendor		= PCI_VENDOR_ID_INTEL,
-		.device		= PCI_DEVICE_ID_INTEL_BSW_UART2,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= byt_serial_setup,
-	},
-	{
-		.vendor		= PCI_VENDOR_ID_INTEL,
-		.device		= PCI_DEVICE_ID_INTEL_BDW_UART1,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= byt_serial_setup,
-	},
-	{
-		.vendor		= PCI_VENDOR_ID_INTEL,
-		.device		= PCI_DEVICE_ID_INTEL_BDW_UART2,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= byt_serial_setup,
-	},
 	/*
 	 * ITE
 	 */
@@ -2921,7 +2736,6 @@ enum pci_board_num_t {
 	pbn_ADDIDATA_PCIe_4_3906250,
 	pbn_ADDIDATA_PCIe_8_3906250,
 	pbn_ce4100_1_115200,
-	pbn_byt,
 	pbn_qrk,
 	pbn_omegapci,
 	pbn_NETMOS9900_2s_115200,
@@ -3698,12 +3512,6 @@ static struct pciserial_board pci_boards[] = {
 		.base_baud	= 921600,
 		.reg_shift      = 2,
 	},
-	[pbn_byt] = {
-		.flags		= FL_BASE0,
-		.num_ports	= 1,
-		.base_baud	= 2764800,
-		.reg_shift      = 2,
-	},
 	[pbn_qrk] = {
 		.flags		= FL_BASE0,
 		.num_ports	= 1,
@@ -3820,6 +3628,14 @@ static const struct pci_device_id blacklist[] = {
 	{ PCI_VDEVICE(INTEL, 0x081d), },
 	{ PCI_VDEVICE(INTEL, 0x1191), },
 	{ PCI_VDEVICE(INTEL, 0x19d8), },
+
+	/* Intel platforms with DesignWare UART */
+	{ PCI_VDEVICE(INTEL, 0x0f0a), },
+	{ PCI_VDEVICE(INTEL, 0x0f0c), },
+	{ PCI_VDEVICE(INTEL, 0x228a), },
+	{ PCI_VDEVICE(INTEL, 0x228c), },
+	{ PCI_VDEVICE(INTEL, 0x9ce3), },
+	{ PCI_VDEVICE(INTEL, 0x9ce4), },
 };
 
 /*
@@ -5485,33 +5301,6 @@ static struct pci_device_id serial_pci_tbl[] = {
 	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART,
 		PCI_ANY_ID,  PCI_ANY_ID, 0, 0,
 		pbn_ce4100_1_115200 },
-	/* Intel BayTrail */
-	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART1,
-		PCI_ANY_ID,  PCI_ANY_ID,
-		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
-		pbn_byt },
-	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART2,
-		PCI_ANY_ID,  PCI_ANY_ID,
-		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
-		pbn_byt },
-	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART1,
-		PCI_ANY_ID,  PCI_ANY_ID,
-		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
-		pbn_byt },
-	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART2,
-		PCI_ANY_ID,  PCI_ANY_ID,
-		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
-		pbn_byt },
-
-	/* Intel Broadwell */
-	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART1,
-		PCI_ANY_ID,  PCI_ANY_ID,
-		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
-		pbn_byt },
-	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART2,
-		PCI_ANY_ID,  PCI_ANY_ID,
-		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
-		pbn_byt },
 
 	/*
 	 * Intel Quark x1000
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 64742a0..6fde7d2 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -108,7 +108,6 @@ config SERIAL_8250_PCI
 	tristate "8250/16550 PCI device support" if EXPERT
 	depends on SERIAL_8250 && PCI
 	default SERIAL_8250
-	select RATIONAL
 	help
 	  This builds standard PCI serial support. You may be able to
 	  disable this feature if you only need legacy serial support.
@@ -398,6 +397,19 @@ config SERIAL_8250_INGENIC
 	  If you have a system using an Ingenic SoC and wish to make use of
 	  its UARTs, say Y to this option. If unsure, say N.
 
+config SERIAL_8250_LPSS
+	tristate "Support for serial ports on Intel LPSS platforms" if EXPERT
+	default SERIAL_8250
+	depends on SERIAL_8250 && PCI
+	depends on X86 || COMPILE_TEST
+	select DW_DMAC_CORE if SERIAL_8250_DMA
+	select DW_DMAC_PCI if X86_INTEL_LPSS
+	select RATIONAL
+	help
+	  Selecting this option will enable handling of the extra features
+	  present on the UART found on Intel Braswell SoC and various other
+	  Intel platforms.
+
 config SERIAL_8250_MID
 	tristate "Support for serial ports on Intel MID platforms"
 	depends on SERIAL_8250 && PCI
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index c9a2d6e..ca37d1c 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_LPC18XX)	+= 8250_lpc18xx.o
 obj-$(CONFIG_SERIAL_8250_MT6577)	+= 8250_mtk.o
 obj-$(CONFIG_SERIAL_8250_UNIPHIER)	+= 8250_uniphier.o
 obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
+obj-$(CONFIG_SERIAL_8250_LPSS)		+= 8250_lpss.o
 obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
 obj-$(CONFIG_SERIAL_8250_MOXA)		+= 8250_moxa.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
-- 
2.8.0.rc3

Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module

[Peter Hurley]

On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> Intes SoCs, such as Braswell, have DesignWare UART. Split out to separate
> module which also will be used for Intel Quark later.

What's the rationale?

And this really isn't a split; this patch introduces a number of significant
changes from the pci version.


> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_lpss.c | 279 ++++++++++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/8250_pci.c  | 227 ++---------------------------
>  drivers/tty/serial/8250/Kconfig     |  14 +-
>  drivers/tty/serial/8250/Makefile    |   1 +
>  4 files changed, 301 insertions(+), 220 deletions(-)
>  create mode 100644 drivers/tty/serial/8250/8250_lpss.c
> 
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> new file mode 100644
> index 0000000..bca4adb
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -0,0 +1,279 @@
> +/*
> + * 8250_lpss.c - Driver for UART on Intel Braswell and various other Intel SoCs
> + *
> + * Copyright (C) 2016 Intel Corporation
> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/rational.h>
> +
> +#include <linux/dmaengine.h>
> +#include <linux/platform_data/dma-dw.h>
> +
> +#include "8250.h"
> +
> +#define PCI_DEVICE_ID_INTEL_BYT_UART1	0x0f0a
> +#define PCI_DEVICE_ID_INTEL_BYT_UART2	0x0f0c
> +
> +#define PCI_DEVICE_ID_INTEL_BSW_UART1	0x228a
> +#define PCI_DEVICE_ID_INTEL_BSW_UART2	0x228c
> +
> +#define PCI_DEVICE_ID_INTEL_BDW_UART1	0x9ce3
> +#define PCI_DEVICE_ID_INTEL_BDW_UART2	0x9ce4
> +
> +/* Intel LPSS specific registers */
> +
> +#define BYT_PRV_CLK			0x800
> +#define BYT_PRV_CLK_EN			BIT(0)
> +#define BYT_PRV_CLK_M_VAL_SHIFT		1
> +#define BYT_PRV_CLK_N_VAL_SHIFT		16
> +#define BYT_PRV_CLK_UPDATE		BIT(31)
> +
> +#define BYT_TX_OVF_INT			0x820
> +#define BYT_TX_OVF_INT_MASK		BIT(1)
> +
> +struct lpss8250;
> +
> +struct lpss8250_board {
> +	unsigned long freq;
> +	unsigned int base_baud;
> +	int (*setup)(struct lpss8250 *, struct uart_port *p);
> +};

New concept.

> +
> +struct lpss8250 {
> +	int line;
> +	struct lpss8250_board *board;
> +
> +	/* DMA parameters */
> +	struct uart_8250_dma dma;
> +	struct dw_dma_slave dma_param;
> +	u8 dma_maxburst;
> +};
> +
> +/*****************************************************************************/

Please remove.

> +
> +static void lpss8250_set_termios(struct uart_port *p,
> +				 struct ktermios *termios,
> +				 struct ktermios *old)
> +{
> +	unsigned int baud = tty_termios_baud_rate(termios);
> +	struct lpss8250 *lpss = p->private_data;
> +	unsigned long fref = lpss->board->freq, fuart = baud * 16;
> +	unsigned long w = BIT(15) - 1;
> +	unsigned long m, n;
> +	u32 reg;
> +
> +	/* Get Fuart closer to Fref */
> +	fuart *= rounddown_pow_of_two(fref / fuart);
> +
> +	/*
> +	 * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
> +	 * dividers must be adjusted.
> +	 *
> +	 * uartclk = (m / n) * 100 MHz, where m <= n
> +	 */
> +	rational_best_approximation(fuart, fref, w, w, &m, &n);
> +	p->uartclk = fuart;
> +
> +	/* Reset the clock */
> +	reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
> +	writel(reg, p->membase + BYT_PRV_CLK);
> +	reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
> +	writel(reg, p->membase + BYT_PRV_CLK);
> +
> +	p->status &= ~UPSTAT_AUTOCTS;
> +	if (termios->c_cflag & CRTSCTS)
> +		p->status |= UPSTAT_AUTOCTS;
> +
> +	serial8250_do_set_termios(p, termios, old);
> +}
> +
> +/*****************************************************************************/

Please remove.

> +
> +static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)

This would have been much easier to review if you had simply moved it here
and done the rework in a follow-on patch.


> +{
> +	struct dw_dma_slave *param = &lpss->dma_param;
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	struct pci_dev *pdev = to_pci_dev(port->dev);
> +	struct pci_dev *dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
> +
> +	switch (pdev->device) {
> +	case PCI_DEVICE_ID_INTEL_BYT_UART1:
> +	case PCI_DEVICE_ID_INTEL_BSW_UART1:
> +	case PCI_DEVICE_ID_INTEL_BDW_UART1:
> +		param->src_id = 3;
> +		param->dst_id = 2;
> +		break;
> +	case PCI_DEVICE_ID_INTEL_BYT_UART2:
> +	case PCI_DEVICE_ID_INTEL_BSW_UART2:
> +	case PCI_DEVICE_ID_INTEL_BDW_UART2:
> +		param->src_id = 5;
> +		param->dst_id = 4;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	param->dma_dev = &dma_dev->dev;
> +	param->m_master = 0;
> +	param->p_master = 1;
> +
> +	/* TODO: Detect FIFO size automaticaly for DesignWare 8250 */
> +	port->fifosize = 64;
> +	up->tx_loadsz = 64;
> +
> +	lpss->dma_maxburst = 16;
> +
> +	port->set_termios = lpss8250_set_termios;
> +
> +	/* Disable TX counter interrupts */
> +	writel(BYT_TX_OVF_INT_MASK, port->membase + BYT_TX_OVF_INT);
> +
> +	return 0;
> +}
> +
> +/*****************************************************************************/

Please remove.

> +
> +static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
> +{
> +	struct dw_dma_slave *dws = param;
> +
> +	if (dws->dma_dev != chan->device->dev)
> +		return false;
> +
> +	chan->private = dws;
> +	return true;
> +}
> +
> +static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port)
> +{
> +	struct uart_8250_dma *dma = &lpss->dma;
> +	struct dw_dma_slave *param = &lpss->dma_param;

Unnecessary alias.

> +	struct dw_dma_slave *rx_param, *tx_param;
> +	struct device *dev = port->port.dev;
> +
> +	rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
> +	if (!rx_param)
> +		return -ENOMEM;
> +
> +	tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
> +	if (!tx_param)
> +		return -ENOMEM;
> +
> +	*rx_param = *param;
> +	dma->rxconf.src_maxburst = lpss->dma_maxburst;
> +
> +	*tx_param = *param;
> +	dma->txconf.dst_maxburst = lpss->dma_maxburst;
> +
> +	dma->fn = lpss8250_dma_filter;
> +	dma->rx_param = rx_param;
> +	dma->tx_param = tx_param;
> +
> +	port->dma = dma;
> +	return 0;
> +}
> +
> +/*****************************************************************************/

Please remove.

> +
> +static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct uart_8250_port uart;
> +	struct lpss8250 *lpss;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	pci_set_master(pdev);
> +
> +	lpss = devm_kzalloc(&pdev->dev, sizeof(*lpss), GFP_KERNEL);
> +	if (!lpss)
> +		return -ENOMEM;
> +
> +	lpss->board = (struct lpss8250_board *)id->driver_data;
> +
> +	memset(&uart, 0, sizeof(struct uart_8250_port));
> +
> +	uart.port.dev = &pdev->dev;
> +	uart.port.irq = pdev->irq;
> +	uart.port.private_data = lpss;
> +	uart.port.type = PORT_16550A;
> +	uart.port.iotype = UPIO_MEM32;

UPIO_MEM


> +	uart.port.regshift = 2;
> +	uart.port.uartclk = lpss->board->base_baud * 16;
> +	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
> +

Please remove empty line

> +	uart.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> +

Same

> +	uart.port.mapbase = pci_resource_start(pdev, 0);
> +	uart.port.membase = pcim_iomap(pdev, 0, 0);
> +	if (!uart.port.membase)
> +		return -ENOMEM;
> +
> +	if (lpss->board->setup) {

There's no design that doesn't have a setup method.


> +		ret = lpss->board->setup(lpss, &uart.port);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = lpss8250_dma_setup(lpss, &uart);
> +	if (ret)
> +		return ret;

I would fold this call into board setup which avoids the
ugliness when this error pathway is reworked in the
follow-on patches.


> +
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0)
> +		return ret;
> +
> +	lpss->line = ret;
> +
> +	pci_set_drvdata(pdev, lpss);
> +	return 0;
> +}
> +
> +static void lpss8250_remove(struct pci_dev *pdev)
> +{
> +	struct lpss8250 *lpss = pci_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(lpss->line);
> +}
> +
> +static const struct lpss8250_board byt_board = {
> +	.freq = 100000000,
> +	.base_baud = 2764800,
> +	.setup = byt_serial_setup,
> +};
> +
> +#define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
> +
> +static const struct pci_device_id pci_ids[] = {
> +	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART1, byt_board),
> +	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART2, byt_board),
> +	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART1, byt_board),
> +	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART2, byt_board),
> +	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART1, byt_board),
> +	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART2, byt_board),
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(pci, pci_ids);
> +
> +static struct pci_driver lpss8250_pci_driver = {
> +	.name           = "8250_lpss",
> +	.id_table       = pci_ids,
> +	.probe          = lpss8250_probe,
> +	.remove         = lpss8250_remove,

No power management?


> +};
> +
> +module_pci_driver(lpss8250_pci_driver);
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Intel LPSS UART driver");
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 5eea74d..bb4df5d 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -21,14 +21,10 @@
>  #include <linux/serial_core.h>
>  #include <linux/8250_pci.h>
>  #include <linux/bitops.h>
> -#include <linux/rational.h>
>  
>  #include <asm/byteorder.h>
>  #include <asm/io.h>
>  
> -#include <linux/dmaengine.h>
> -#include <linux/platform_data/dma-dw.h>
> -
>  #include "8250.h"
>  
>  /*
> @@ -1349,145 +1345,6 @@ ce4100_serial_setup(struct serial_private *priv,
>  	return ret;
>  }
>  
> -#define PCI_DEVICE_ID_INTEL_BYT_UART1	0x0f0a
> -#define PCI_DEVICE_ID_INTEL_BYT_UART2	0x0f0c
> -
> -#define PCI_DEVICE_ID_INTEL_BSW_UART1	0x228a
> -#define PCI_DEVICE_ID_INTEL_BSW_UART2	0x228c
> -
> -#define PCI_DEVICE_ID_INTEL_BDW_UART1	0x9ce3
> -#define PCI_DEVICE_ID_INTEL_BDW_UART2	0x9ce4
> -
> -#define BYT_PRV_CLK			0x800
> -#define BYT_PRV_CLK_EN			(1 << 0)
> -#define BYT_PRV_CLK_M_VAL_SHIFT		1
> -#define BYT_PRV_CLK_N_VAL_SHIFT		16
> -#define BYT_PRV_CLK_UPDATE		(1 << 31)
> -
> -#define BYT_TX_OVF_INT			0x820
> -#define BYT_TX_OVF_INT_MASK		(1 << 1)
> -
> -static void
> -byt_set_termios(struct uart_port *p, struct ktermios *termios,
> -		struct ktermios *old)
> -{
> -	unsigned int baud = tty_termios_baud_rate(termios);
> -	unsigned long fref = 100000000, fuart = baud * 16;
> -	unsigned long w = BIT(15) - 1;
> -	unsigned long m, n;
> -	u32 reg;
> -
> -	/* Get Fuart closer to Fref */
> -	fuart *= rounddown_pow_of_two(fref / fuart);
> -
> -	/*
> -	 * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
> -	 * dividers must be adjusted.
> -	 *
> -	 * uartclk = (m / n) * 100 MHz, where m <= n
> -	 */
> -	rational_best_approximation(fuart, fref, w, w, &m, &n);
> -	p->uartclk = fuart;
> -
> -	/* Reset the clock */
> -	reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
> -	writel(reg, p->membase + BYT_PRV_CLK);
> -	reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
> -	writel(reg, p->membase + BYT_PRV_CLK);
> -
> -	p->status &= ~UPSTAT_AUTOCTS;
> -	if (termios->c_cflag & CRTSCTS)
> -		p->status |= UPSTAT_AUTOCTS;
> -
> -	serial8250_do_set_termios(p, termios, old);
> -}
> -
> -static bool byt_dma_filter(struct dma_chan *chan, void *param)
> -{
> -	struct dw_dma_slave *dws = param;
> -
> -	if (dws->dma_dev != chan->device->dev)
> -		return false;
> -
> -	chan->private = dws;
> -	return true;
> -}
> -
> -static int
> -byt_serial_setup(struct serial_private *priv,
> -		 const struct pciserial_board *board,
> -		 struct uart_8250_port *port, int idx)
> -{
> -	struct pci_dev *pdev = priv->dev;
> -	struct device *dev = port->port.dev;
> -	struct uart_8250_dma *dma;
> -	struct dw_dma_slave *tx_param, *rx_param;
> -	struct pci_dev *dma_dev;
> -	int ret;
> -
> -	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
> -	if (!dma)
> -		return -ENOMEM;
> -
> -	tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
> -	if (!tx_param)
> -		return -ENOMEM;
> -
> -	rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
> -	if (!rx_param)
> -		return -ENOMEM;
> -
> -	switch (pdev->device) {
> -	case PCI_DEVICE_ID_INTEL_BYT_UART1:
> -	case PCI_DEVICE_ID_INTEL_BSW_UART1:
> -	case PCI_DEVICE_ID_INTEL_BDW_UART1:
> -		rx_param->src_id = 3;
> -		tx_param->dst_id = 2;
> -		break;
> -	case PCI_DEVICE_ID_INTEL_BYT_UART2:
> -	case PCI_DEVICE_ID_INTEL_BSW_UART2:
> -	case PCI_DEVICE_ID_INTEL_BDW_UART2:
> -		rx_param->src_id = 5;
> -		tx_param->dst_id = 4;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	rx_param->m_master = 0;
> -	rx_param->p_master = 1;
> -
> -	dma->rxconf.src_maxburst = 16;
> -
> -	tx_param->m_master = 0;
> -	tx_param->p_master = 1;
> -
> -	dma->txconf.dst_maxburst = 16;
> -
> -	dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
> -	rx_param->dma_dev = &dma_dev->dev;
> -	tx_param->dma_dev = &dma_dev->dev;
> -
> -	dma->fn = byt_dma_filter;
> -	dma->rx_param = rx_param;
> -	dma->tx_param = tx_param;
> -
> -	ret = pci_default_setup(priv, board, port, idx);
> -	port->port.iotype = UPIO_MEM;
> -	port->port.type = PORT_16550A;
> -	port->port.flags = (port->port.flags | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> -	port->port.set_termios = byt_set_termios;
> -	port->port.fifosize = 64;
> -	port->tx_loadsz = 64;
> -	port->dma = dma;
> -	port->capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> -
> -	/* Disable Tx counter interrupts */
> -	writel(BYT_TX_OVF_INT_MASK, port->port.membase + BYT_TX_OVF_INT);
> -
> -	return ret;
> -}
> -
>  static int
>  pci_omegapci_setup(struct serial_private *priv,
>  		      const struct pciserial_board *board,
> @@ -2015,48 +1872,6 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>  		.subdevice	= PCI_ANY_ID,
>  		.setup		= kt_serial_setup,
>  	},
> -	{
> -		.vendor		= PCI_VENDOR_ID_INTEL,
> -		.device		= PCI_DEVICE_ID_INTEL_BYT_UART1,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.setup		= byt_serial_setup,
> -	},
> -	{
> -		.vendor		= PCI_VENDOR_ID_INTEL,
> -		.device		= PCI_DEVICE_ID_INTEL_BYT_UART2,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.setup		= byt_serial_setup,
> -	},
> -	{
> -		.vendor		= PCI_VENDOR_ID_INTEL,
> -		.device		= PCI_DEVICE_ID_INTEL_BSW_UART1,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.setup		= byt_serial_setup,
> -	},
> -	{
> -		.vendor		= PCI_VENDOR_ID_INTEL,
> -		.device		= PCI_DEVICE_ID_INTEL_BSW_UART2,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.setup		= byt_serial_setup,
> -	},
> -	{
> -		.vendor		= PCI_VENDOR_ID_INTEL,
> -		.device		= PCI_DEVICE_ID_INTEL_BDW_UART1,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.setup		= byt_serial_setup,
> -	},
> -	{
> -		.vendor		= PCI_VENDOR_ID_INTEL,
> -		.device		= PCI_DEVICE_ID_INTEL_BDW_UART2,
> -		.subvendor	= PCI_ANY_ID,
> -		.subdevice	= PCI_ANY_ID,
> -		.setup		= byt_serial_setup,
> -	},
>  	/*
>  	 * ITE
>  	 */
> @@ -2921,7 +2736,6 @@ enum pci_board_num_t {
>  	pbn_ADDIDATA_PCIe_4_3906250,
>  	pbn_ADDIDATA_PCIe_8_3906250,
>  	pbn_ce4100_1_115200,
> -	pbn_byt,
>  	pbn_qrk,
>  	pbn_omegapci,
>  	pbn_NETMOS9900_2s_115200,
> @@ -3698,12 +3512,6 @@ static struct pciserial_board pci_boards[] = {
>  		.base_baud	= 921600,
>  		.reg_shift      = 2,
>  	},
> -	[pbn_byt] = {
> -		.flags		= FL_BASE0,
> -		.num_ports	= 1,
> -		.base_baud	= 2764800,
> -		.reg_shift      = 2,
> -	},
>  	[pbn_qrk] = {
>  		.flags		= FL_BASE0,
>  		.num_ports	= 1,
> @@ -3820,6 +3628,14 @@ static const struct pci_device_id blacklist[] = {
>  	{ PCI_VDEVICE(INTEL, 0x081d), },
>  	{ PCI_VDEVICE(INTEL, 0x1191), },
>  	{ PCI_VDEVICE(INTEL, 0x19d8), },
> +
> +	/* Intel platforms with DesignWare UART */
> +	{ PCI_VDEVICE(INTEL, 0x0f0a), },
> +	{ PCI_VDEVICE(INTEL, 0x0f0c), },
> +	{ PCI_VDEVICE(INTEL, 0x228a), },
> +	{ PCI_VDEVICE(INTEL, 0x228c), },
> +	{ PCI_VDEVICE(INTEL, 0x9ce3), },
> +	{ PCI_VDEVICE(INTEL, 0x9ce4), },
>  };
>  
>  /*
> @@ -5485,33 +5301,6 @@ static struct pci_device_id serial_pci_tbl[] = {
>  	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART,
>  		PCI_ANY_ID,  PCI_ANY_ID, 0, 0,
>  		pbn_ce4100_1_115200 },
> -	/* Intel BayTrail */
> -	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART1,
> -		PCI_ANY_ID,  PCI_ANY_ID,
> -		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
> -		pbn_byt },
> -	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART2,
> -		PCI_ANY_ID,  PCI_ANY_ID,
> -		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
> -		pbn_byt },
> -	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART1,
> -		PCI_ANY_ID,  PCI_ANY_ID,
> -		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
> -		pbn_byt },
> -	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART2,
> -		PCI_ANY_ID,  PCI_ANY_ID,
> -		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
> -		pbn_byt },
> -
> -	/* Intel Broadwell */
> -	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART1,
> -		PCI_ANY_ID,  PCI_ANY_ID,
> -		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
> -		pbn_byt },
> -	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART2,
> -		PCI_ANY_ID,  PCI_ANY_ID,
> -		PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
> -		pbn_byt },
>  
>  	/*
>  	 * Intel Quark x1000
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 64742a0..6fde7d2 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -108,7 +108,6 @@ config SERIAL_8250_PCI
>  	tristate "8250/16550 PCI device support" if EXPERT
>  	depends on SERIAL_8250 && PCI
>  	default SERIAL_8250
> -	select RATIONAL
>  	help
>  	  This builds standard PCI serial support. You may be able to
>  	  disable this feature if you only need legacy serial support.
> @@ -398,6 +397,19 @@ config SERIAL_8250_INGENIC
>  	  If you have a system using an Ingenic SoC and wish to make use of
>  	  its UARTs, say Y to this option. If unsure, say N.
>  
> +config SERIAL_8250_LPSS
> +	tristate "Support for serial ports on Intel LPSS platforms" if EXPERT
> +	default SERIAL_8250
> +	depends on SERIAL_8250 && PCI
> +	depends on X86 || COMPILE_TEST
> +	select DW_DMAC_CORE if SERIAL_8250_DMA
> +	select DW_DMAC_PCI if X86_INTEL_LPSS
> +	select RATIONAL
> +	help
> +	  Selecting this option will enable handling of the extra features
> +	  present on the UART found on Intel Braswell SoC and various other
> +	  Intel platforms.
> +
>  config SERIAL_8250_MID
>  	tristate "Support for serial ports on Intel MID platforms"
>  	depends on SERIAL_8250 && PCI
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index c9a2d6e..ca37d1c 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_LPC18XX)	+= 8250_lpc18xx.o
>  obj-$(CONFIG_SERIAL_8250_MT6577)	+= 8250_mtk.o
>  obj-$(CONFIG_SERIAL_8250_UNIPHIER)	+= 8250_uniphier.o
>  obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
> +obj-$(CONFIG_SERIAL_8250_LPSS)		+= 8250_lpss.o
>  obj-$(CONFIG_SERIAL_8250_MID)		+= 8250_mid.o
>  obj-$(CONFIG_SERIAL_8250_MOXA)		+= 8250_moxa.o
>  obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
> 

Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module

[Andy Shevchenko]

On Fri, Apr 8, 2016 at 4:42 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
>> Intes SoCs, such as Braswell, have DesignWare UART. Split out to separate
>> module which also will be used for Intel Quark later.
>
> What's the rationale?

1. Not poison 8250_pci with too many quirks.
2. They all use same DMA engine, otherwise we might end up in all
possible DMA engines included in one file.
3. All of them are actually DesignWare, so, in the future we might
share code between 8250_dw and 8250_lpss.

>
> And this really isn't a split; this patch introduces a number of significant
> changes from the pci version.

Some style changes, yes, but "significant"?
For example?

>
>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/tty/serial/8250/8250_lpss.c | 279 ++++++++++++++++++++++++++++++++++++
>>  drivers/tty/serial/8250/8250_pci.c  | 227 ++---------------------------
>>  drivers/tty/serial/8250/Kconfig     |  14 +-
>>  drivers/tty/serial/8250/Makefile    |   1 +
>>  4 files changed, 301 insertions(+), 220 deletions(-)
>>  create mode 100644 drivers/tty/serial/8250/8250_lpss.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
>> new file mode 100644
>> index 0000000..bca4adb
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_lpss.c
>> @@ -0,0 +1,279 @@
>> +/*
>> + * 8250_lpss.c - Driver for UART on Intel Braswell and various other Intel SoCs
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/rational.h>
>> +
>> +#include <linux/dmaengine.h>
>> +#include <linux/platform_data/dma-dw.h>
>> +
>> +#include "8250.h"
>> +
>> +#define PCI_DEVICE_ID_INTEL_BYT_UART1        0x0f0a
>> +#define PCI_DEVICE_ID_INTEL_BYT_UART2        0x0f0c
>> +
>> +#define PCI_DEVICE_ID_INTEL_BSW_UART1        0x228a
>> +#define PCI_DEVICE_ID_INTEL_BSW_UART2        0x228c
>> +
>> +#define PCI_DEVICE_ID_INTEL_BDW_UART1        0x9ce3
>> +#define PCI_DEVICE_ID_INTEL_BDW_UART2        0x9ce4
>> +
>> +/* Intel LPSS specific registers */
>> +
>> +#define BYT_PRV_CLK                  0x800
>> +#define BYT_PRV_CLK_EN                       BIT(0)
>> +#define BYT_PRV_CLK_M_VAL_SHIFT              1
>> +#define BYT_PRV_CLK_N_VAL_SHIFT              16
>> +#define BYT_PRV_CLK_UPDATE           BIT(31)
>> +
>> +#define BYT_TX_OVF_INT                       0x820
>> +#define BYT_TX_OVF_INT_MASK          BIT(1)
>> +
>> +struct lpss8250;
>> +
>> +struct lpss8250_board {
>> +     unsigned long freq;
>> +     unsigned int base_baud;
>> +     int (*setup)(struct lpss8250 *, struct uart_port *p);
>> +};
>
> New concept.
>
>> +
>> +struct lpss8250 {
>> +     int line;
>> +     struct lpss8250_board *board;
>> +
>> +     /* DMA parameters */
>> +     struct uart_8250_dma dma;
>> +     struct dw_dma_slave dma_param;
>> +     u8 dma_maxburst;
>> +};
>> +
>> +/*****************************************************************************/
>
> Please remove.
>
>> +
>> +static void lpss8250_set_termios(struct uart_port *p,
>> +                              struct ktermios *termios,
>> +                              struct ktermios *old)
>> +{
>> +     unsigned int baud = tty_termios_baud_rate(termios);
>> +     struct lpss8250 *lpss = p->private_data;
>> +     unsigned long fref = lpss->board->freq, fuart = baud * 16;
>> +     unsigned long w = BIT(15) - 1;
>> +     unsigned long m, n;
>> +     u32 reg;
>> +
>> +     /* Get Fuart closer to Fref */
>> +     fuart *= rounddown_pow_of_two(fref / fuart);
>> +
>> +     /*
>> +      * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
>> +      * dividers must be adjusted.
>> +      *
>> +      * uartclk = (m / n) * 100 MHz, where m <= n
>> +      */
>> +     rational_best_approximation(fuart, fref, w, w, &m, &n);
>> +     p->uartclk = fuart;
>> +
>> +     /* Reset the clock */
>> +     reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
>> +     writel(reg, p->membase + BYT_PRV_CLK);
>> +     reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
>> +     writel(reg, p->membase + BYT_PRV_CLK);
>> +
>> +     p->status &= ~UPSTAT_AUTOCTS;
>> +     if (termios->c_cflag & CRTSCTS)
>> +             p->status |= UPSTAT_AUTOCTS;
>> +
>> +     serial8250_do_set_termios(p, termios, old);
>> +}
>> +
>> +/*****************************************************************************/
>
> Please remove.
>
>> +
>> +static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
>
> This would have been much easier to review if you had simply moved it here
> and done the rework in a follow-on patch.

I didn't quite get this one. How series should look like?

>
>
>> +{
>> +     struct dw_dma_slave *param = &lpss->dma_param;
>> +     struct uart_8250_port *up = up_to_u8250p(port);
>> +     struct pci_dev *pdev = to_pci_dev(port->dev);
>> +     struct pci_dev *dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
>> +
>> +     switch (pdev->device) {
>> +     case PCI_DEVICE_ID_INTEL_BYT_UART1:
>> +     case PCI_DEVICE_ID_INTEL_BSW_UART1:
>> +     case PCI_DEVICE_ID_INTEL_BDW_UART1:
>> +             param->src_id = 3;
>> +             param->dst_id = 2;
>> +             break;
>> +     case PCI_DEVICE_ID_INTEL_BYT_UART2:
>> +     case PCI_DEVICE_ID_INTEL_BSW_UART2:
>> +     case PCI_DEVICE_ID_INTEL_BDW_UART2:
>> +             param->src_id = 5;
>> +             param->dst_id = 4;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     param->dma_dev = &dma_dev->dev;
>> +     param->m_master = 0;
>> +     param->p_master = 1;
>> +
>> +     /* TODO: Detect FIFO size automaticaly for DesignWare 8250 */
>> +     port->fifosize = 64;
>> +     up->tx_loadsz = 64;
>> +
>> +     lpss->dma_maxburst = 16;
>> +
>> +     port->set_termios = lpss8250_set_termios;
>> +
>> +     /* Disable TX counter interrupts */
>> +     writel(BYT_TX_OVF_INT_MASK, port->membase + BYT_TX_OVF_INT);
>> +
>> +     return 0;
>> +}
>> +
>> +/*****************************************************************************/
>
> Please remove.
>
>> +
>> +static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
>> +{
>> +     struct dw_dma_slave *dws = param;
>> +
>> +     if (dws->dma_dev != chan->device->dev)
>> +             return false;
>> +
>> +     chan->private = dws;
>> +     return true;
>> +}
>> +
>> +static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port)
>> +{
>> +     struct uart_8250_dma *dma = &lpss->dma;
>> +     struct dw_dma_slave *param = &lpss->dma_param;
>
> Unnecessary alias.
>
>> +     struct dw_dma_slave *rx_param, *tx_param;
>> +     struct device *dev = port->port.dev;
>> +
>> +     rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
>> +     if (!rx_param)
>> +             return -ENOMEM;
>> +
>> +     tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
>> +     if (!tx_param)
>> +             return -ENOMEM;
>> +
>> +     *rx_param = *param;
>> +     dma->rxconf.src_maxburst = lpss->dma_maxburst;
>> +
>> +     *tx_param = *param;
>> +     dma->txconf.dst_maxburst = lpss->dma_maxburst;
>> +
>> +     dma->fn = lpss8250_dma_filter;
>> +     dma->rx_param = rx_param;
>> +     dma->tx_param = tx_param;
>> +
>> +     port->dma = dma;
>> +     return 0;
>> +}
>> +
>> +/*****************************************************************************/
>
> Please remove.
>
>> +
>> +static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> +{
>> +     struct uart_8250_port uart;
>> +     struct lpss8250 *lpss;
>> +     int ret;
>> +
>> +     ret = pcim_enable_device(pdev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     pci_set_master(pdev);
>> +
>> +     lpss = devm_kzalloc(&pdev->dev, sizeof(*lpss), GFP_KERNEL);
>> +     if (!lpss)
>> +             return -ENOMEM;
>> +
>> +     lpss->board = (struct lpss8250_board *)id->driver_data;
>> +
>> +     memset(&uart, 0, sizeof(struct uart_8250_port));
>> +
>> +     uart.port.dev = &pdev->dev;
>> +     uart.port.irq = pdev->irq;
>> +     uart.port.private_data = lpss;
>> +     uart.port.type = PORT_16550A;
>> +     uart.port.iotype = UPIO_MEM32;
>
> UPIO_MEM
>
>
>> +     uart.port.regshift = 2;
>> +     uart.port.uartclk = lpss->board->base_baud * 16;
>> +     uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
>> +
>
> Please remove empty line
>
>> +     uart.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
>> +
>
> Same
>
>> +     uart.port.mapbase = pci_resource_start(pdev, 0);
>> +     uart.port.membase = pcim_iomap(pdev, 0, 0);
>> +     if (!uart.port.membase)
>> +             return -ENOMEM;
>> +
>> +     if (lpss->board->setup) {
>
> There's no design that doesn't have a setup method.

For now, indeed. Okay, I will call it directly.

>
>
>> +             ret = lpss->board->setup(lpss, &uart.port);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     ret = lpss8250_dma_setup(lpss, &uart);
>> +     if (ret)
>> +             return ret;
>
> I would fold this call into board setup which avoids the
> ugliness when this error pathway is reworked in the
> follow-on patches.

Each of them?

>
>
>> +
>> +     ret = serial8250_register_8250_port(&uart);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     lpss->line = ret;
>> +
>> +     pci_set_drvdata(pdev, lpss);
>> +     return 0;
>> +}
>> +
>> +static void lpss8250_remove(struct pci_dev *pdev)
>> +{
>> +     struct lpss8250 *lpss = pci_get_drvdata(pdev);
>> +
>> +     serial8250_unregister_port(lpss->line);
>> +}
>> +
>> +static const struct lpss8250_board byt_board = {
>> +     .freq = 100000000,
>> +     .base_baud = 2764800,
>> +     .setup = byt_serial_setup,
>> +};
>> +
>> +#define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
>> +
>> +static const struct pci_device_id pci_ids[] = {
>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART1, byt_board),
>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART2, byt_board),
>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART1, byt_board),
>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART2, byt_board),
>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART1, byt_board),
>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART2, byt_board),
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(pci, pci_ids);
>> +
>> +static struct pci_driver lpss8250_pci_driver = {
>> +     .name           = "8250_lpss",
>> +     .id_table       = pci_ids,
>> +     .probe          = lpss8250_probe,
>> +     .remove         = lpss8250_remove,
>
> No power management?

PCI does the trick, no *special* power management treatment required, yes.

>
>
>> +};
>> +
>> +module_pci_driver(lpss8250_pci_driver);
>> +
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Intel LPSS UART driver");
>> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
>> index 5eea74d..bb4df5d 100644
>> --- a/drivers/tty/serial/8250/8250_pci.c
>> +++ b/drivers/tty/serial/8250/8250_pci.c
>> @@ -21,14 +21,10 @@
>>  #include <linux/serial_core.h>
>>  #include <linux/8250_pci.h>
>>  #include <linux/bitops.h>
>> -#include <linux/rational.h>
>>
>>  #include <asm/byteorder.h>
>>  #include <asm/io.h>
>>
>> -#include <linux/dmaengine.h>
>> -#include <linux/platform_data/dma-dw.h>
>> -
>>  #include "8250.h"
>>
>>  /*
>> @@ -1349,145 +1345,6 @@ ce4100_serial_setup(struct serial_private *priv,
>>       return ret;
>>  }
>>
>> -#define PCI_DEVICE_ID_INTEL_BYT_UART1        0x0f0a
>> -#define PCI_DEVICE_ID_INTEL_BYT_UART2        0x0f0c
>> -
>> -#define PCI_DEVICE_ID_INTEL_BSW_UART1        0x228a
>> -#define PCI_DEVICE_ID_INTEL_BSW_UART2        0x228c
>> -
>> -#define PCI_DEVICE_ID_INTEL_BDW_UART1        0x9ce3
>> -#define PCI_DEVICE_ID_INTEL_BDW_UART2        0x9ce4
>> -
>> -#define BYT_PRV_CLK                  0x800
>> -#define BYT_PRV_CLK_EN                       (1 << 0)
>> -#define BYT_PRV_CLK_M_VAL_SHIFT              1
>> -#define BYT_PRV_CLK_N_VAL_SHIFT              16
>> -#define BYT_PRV_CLK_UPDATE           (1 << 31)
>> -
>> -#define BYT_TX_OVF_INT                       0x820
>> -#define BYT_TX_OVF_INT_MASK          (1 << 1)
>> -
>> -static void
>> -byt_set_termios(struct uart_port *p, struct ktermios *termios,
>> -             struct ktermios *old)
>> -{
>> -     unsigned int baud = tty_termios_baud_rate(termios);
>> -     unsigned long fref = 100000000, fuart = baud * 16;
>> -     unsigned long w = BIT(15) - 1;
>> -     unsigned long m, n;
>> -     u32 reg;
>> -
>> -     /* Get Fuart closer to Fref */
>> -     fuart *= rounddown_pow_of_two(fref / fuart);
>> -
>> -     /*
>> -      * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
>> -      * dividers must be adjusted.
>> -      *
>> -      * uartclk = (m / n) * 100 MHz, where m <= n
>> -      */
>> -     rational_best_approximation(fuart, fref, w, w, &m, &n);
>> -     p->uartclk = fuart;
>> -
>> -     /* Reset the clock */
>> -     reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
>> -     writel(reg, p->membase + BYT_PRV_CLK);
>> -     reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
>> -     writel(reg, p->membase + BYT_PRV_CLK);
>> -
>> -     p->status &= ~UPSTAT_AUTOCTS;
>> -     if (termios->c_cflag & CRTSCTS)
>> -             p->status |= UPSTAT_AUTOCTS;
>> -
>> -     serial8250_do_set_termios(p, termios, old);
>> -}
>> -
>> -static bool byt_dma_filter(struct dma_chan *chan, void *param)
>> -{
>> -     struct dw_dma_slave *dws = param;
>> -
>> -     if (dws->dma_dev != chan->device->dev)
>> -             return false;
>> -
>> -     chan->private = dws;
>> -     return true;
>> -}
>> -
>> -static int
>> -byt_serial_setup(struct serial_private *priv,
>> -              const struct pciserial_board *board,
>> -              struct uart_8250_port *port, int idx)
>> -{
>> -     struct pci_dev *pdev = priv->dev;
>> -     struct device *dev = port->port.dev;
>> -     struct uart_8250_dma *dma;
>> -     struct dw_dma_slave *tx_param, *rx_param;
>> -     struct pci_dev *dma_dev;
>> -     int ret;
>> -
>> -     dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
>> -     if (!dma)
>> -             return -ENOMEM;
>> -
>> -     tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
>> -     if (!tx_param)
>> -             return -ENOMEM;
>> -
>> -     rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
>> -     if (!rx_param)
>> -             return -ENOMEM;
>> -
>> -     switch (pdev->device) {
>> -     case PCI_DEVICE_ID_INTEL_BYT_UART1:
>> -     case PCI_DEVICE_ID_INTEL_BSW_UART1:
>> -     case PCI_DEVICE_ID_INTEL_BDW_UART1:
>> -             rx_param->src_id = 3;
>> -             tx_param->dst_id = 2;
>> -             break;
>> -     case PCI_DEVICE_ID_INTEL_BYT_UART2:
>> -     case PCI_DEVICE_ID_INTEL_BSW_UART2:
>> -     case PCI_DEVICE_ID_INTEL_BDW_UART2:
>> -             rx_param->src_id = 5;
>> -             tx_param->dst_id = 4;
>> -             break;
>> -     default:
>> -             return -EINVAL;
>> -     }
>> -
>> -     rx_param->m_master = 0;
>> -     rx_param->p_master = 1;
>> -
>> -     dma->rxconf.src_maxburst = 16;
>> -
>> -     tx_param->m_master = 0;
>> -     tx_param->p_master = 1;
>> -
>> -     dma->txconf.dst_maxburst = 16;
>> -
>> -     dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
>> -     rx_param->dma_dev = &dma_dev->dev;
>> -     tx_param->dma_dev = &dma_dev->dev;
>> -
>> -     dma->fn = byt_dma_filter;
>> -     dma->rx_param = rx_param;
>> -     dma->tx_param = tx_param;
>> -
>> -     ret = pci_default_setup(priv, board, port, idx);
>> -     port->port.iotype = UPIO_MEM;
>> -     port->port.type = PORT_16550A;
>> -     port->port.flags = (port->port.flags | UPF_FIXED_PORT | UPF_FIXED_TYPE);
>> -     port->port.set_termios = byt_set_termios;
>> -     port->port.fifosize = 64;
>> -     port->tx_loadsz = 64;
>> -     port->dma = dma;
>> -     port->capabilities = UART_CAP_FIFO | UART_CAP_AFE;
>> -
>> -     /* Disable Tx counter interrupts */
>> -     writel(BYT_TX_OVF_INT_MASK, port->port.membase + BYT_TX_OVF_INT);
>> -
>> -     return ret;
>> -}
>> -
>>  static int
>>  pci_omegapci_setup(struct serial_private *priv,
>>                     const struct pciserial_board *board,
>> @@ -2015,48 +1872,6 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>>               .subdevice      = PCI_ANY_ID,
>>               .setup          = kt_serial_setup,
>>       },
>> -     {
>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>> -             .device         = PCI_DEVICE_ID_INTEL_BYT_UART1,
>> -             .subvendor      = PCI_ANY_ID,
>> -             .subdevice      = PCI_ANY_ID,
>> -             .setup          = byt_serial_setup,
>> -     },
>> -     {
>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>> -             .device         = PCI_DEVICE_ID_INTEL_BYT_UART2,
>> -             .subvendor      = PCI_ANY_ID,
>> -             .subdevice      = PCI_ANY_ID,
>> -             .setup          = byt_serial_setup,
>> -     },
>> -     {
>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>> -             .device         = PCI_DEVICE_ID_INTEL_BSW_UART1,
>> -             .subvendor      = PCI_ANY_ID,
>> -             .subdevice      = PCI_ANY_ID,
>> -             .setup          = byt_serial_setup,
>> -     },
>> -     {
>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>> -             .device         = PCI_DEVICE_ID_INTEL_BSW_UART2,
>> -             .subvendor      = PCI_ANY_ID,
>> -             .subdevice      = PCI_ANY_ID,
>> -             .setup          = byt_serial_setup,
>> -     },
>> -     {
>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>> -             .device         = PCI_DEVICE_ID_INTEL_BDW_UART1,
>> -             .subvendor      = PCI_ANY_ID,
>> -             .subdevice      = PCI_ANY_ID,
>> -             .setup          = byt_serial_setup,
>> -     },
>> -     {
>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>> -             .device         = PCI_DEVICE_ID_INTEL_BDW_UART2,
>> -             .subvendor      = PCI_ANY_ID,
>> -             .subdevice      = PCI_ANY_ID,
>> -             .setup          = byt_serial_setup,
>> -     },
>>       /*
>>        * ITE
>>        */
>> @@ -2921,7 +2736,6 @@ enum pci_board_num_t {
>>       pbn_ADDIDATA_PCIe_4_3906250,
>>       pbn_ADDIDATA_PCIe_8_3906250,
>>       pbn_ce4100_1_115200,
>> -     pbn_byt,
>>       pbn_qrk,
>>       pbn_omegapci,
>>       pbn_NETMOS9900_2s_115200,
>> @@ -3698,12 +3512,6 @@ static struct pciserial_board pci_boards[] = {
>>               .base_baud      = 921600,
>>               .reg_shift      = 2,
>>       },
>> -     [pbn_byt] = {
>> -             .flags          = FL_BASE0,
>> -             .num_ports      = 1,
>> -             .base_baud      = 2764800,
>> -             .reg_shift      = 2,
>> -     },
>>       [pbn_qrk] = {
>>               .flags          = FL_BASE0,
>>               .num_ports      = 1,
>> @@ -3820,6 +3628,14 @@ static const struct pci_device_id blacklist[] = {
>>       { PCI_VDEVICE(INTEL, 0x081d), },
>>       { PCI_VDEVICE(INTEL, 0x1191), },
>>       { PCI_VDEVICE(INTEL, 0x19d8), },
>> +
>> +     /* Intel platforms with DesignWare UART */
>> +     { PCI_VDEVICE(INTEL, 0x0f0a), },
>> +     { PCI_VDEVICE(INTEL, 0x0f0c), },
>> +     { PCI_VDEVICE(INTEL, 0x228a), },
>> +     { PCI_VDEVICE(INTEL, 0x228c), },
>> +     { PCI_VDEVICE(INTEL, 0x9ce3), },
>> +     { PCI_VDEVICE(INTEL, 0x9ce4), },
>>  };
>>
>>  /*
>> @@ -5485,33 +5301,6 @@ static struct pci_device_id serial_pci_tbl[] = {
>>       {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART,
>>               PCI_ANY_ID,  PCI_ANY_ID, 0, 0,
>>               pbn_ce4100_1_115200 },
>> -     /* Intel BayTrail */
>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART1,
>> -             PCI_ANY_ID,  PCI_ANY_ID,
>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>> -             pbn_byt },
>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART2,
>> -             PCI_ANY_ID,  PCI_ANY_ID,
>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>> -             pbn_byt },
>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART1,
>> -             PCI_ANY_ID,  PCI_ANY_ID,
>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>> -             pbn_byt },
>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART2,
>> -             PCI_ANY_ID,  PCI_ANY_ID,
>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>> -             pbn_byt },
>> -
>> -     /* Intel Broadwell */
>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART1,
>> -             PCI_ANY_ID,  PCI_ANY_ID,
>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>> -             pbn_byt },
>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART2,
>> -             PCI_ANY_ID,  PCI_ANY_ID,
>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>> -             pbn_byt },
>>
>>       /*
>>        * Intel Quark x1000
>> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>> index 64742a0..6fde7d2 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -108,7 +108,6 @@ config SERIAL_8250_PCI
>>       tristate "8250/16550 PCI device support" if EXPERT
>>       depends on SERIAL_8250 && PCI
>>       default SERIAL_8250
>> -     select RATIONAL
>>       help
>>         This builds standard PCI serial support. You may be able to
>>         disable this feature if you only need legacy serial support.
>> @@ -398,6 +397,19 @@ config SERIAL_8250_INGENIC
>>         If you have a system using an Ingenic SoC and wish to make use of
>>         its UARTs, say Y to this option. If unsure, say N.
>>
>> +config SERIAL_8250_LPSS
>> +     tristate "Support for serial ports on Intel LPSS platforms" if EXPERT
>> +     default SERIAL_8250
>> +     depends on SERIAL_8250 && PCI
>> +     depends on X86 || COMPILE_TEST
>> +     select DW_DMAC_CORE if SERIAL_8250_DMA
>> +     select DW_DMAC_PCI if X86_INTEL_LPSS
>> +     select RATIONAL
>> +     help
>> +       Selecting this option will enable handling of the extra features
>> +       present on the UART found on Intel Braswell SoC and various other
>> +       Intel platforms.
>> +
>>  config SERIAL_8250_MID
>>       tristate "Support for serial ports on Intel MID platforms"
>>       depends on SERIAL_8250 && PCI
>> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
>> index c9a2d6e..ca37d1c 100644
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_LPC18XX)   += 8250_lpc18xx.o
>>  obj-$(CONFIG_SERIAL_8250_MT6577)     += 8250_mtk.o
>>  obj-$(CONFIG_SERIAL_8250_UNIPHIER)   += 8250_uniphier.o
>>  obj-$(CONFIG_SERIAL_8250_INGENIC)    += 8250_ingenic.o
>> +obj-$(CONFIG_SERIAL_8250_LPSS)               += 8250_lpss.o
>>  obj-$(CONFIG_SERIAL_8250_MID)                += 8250_mid.o
>>  obj-$(CONFIG_SERIAL_8250_MOXA)               += 8250_moxa.o
>>  obj-$(CONFIG_SERIAL_OF_PLATFORM)     += 8250_of.o
>>
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module

[Peter Hurley]

On 04/08/2016 01:17 AM, Andy Shevchenko wrote:
> On Fri, Apr 8, 2016 at 4:42 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
>>> Intes SoCs, such as Braswell, have DesignWare UART. Split out to separate
>>> module which also will be used for Intel Quark later.
>>
>> What's the rationale?
> 
> 1. Not poison 8250_pci with too many quirks.
> 2. They all use same DMA engine, otherwise we might end up in all
> possible DMA engines included in one file.
> 3. All of them are actually DesignWare, so, in the future we might
> share code between 8250_dw and 8250_lpss.

Just my opinion, but I like to see the rationale in the changelog.


>> And this really isn't a split; this patch introduces a number of significant
>> changes from the pci version.
> 
> Some style changes, yes, but "significant"?
> For example?

I'm just pointing out the changelog doesn't really match the
commit. I'm not suggesting necessarily to redo the series, but just more
adequately reflect the change. See below.


>>
>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>  drivers/tty/serial/8250/8250_lpss.c | 279 ++++++++++++++++++++++++++++++++++++
>>>  drivers/tty/serial/8250/8250_pci.c  | 227 ++---------------------------
>>>  drivers/tty/serial/8250/Kconfig     |  14 +-
>>>  drivers/tty/serial/8250/Makefile    |   1 +
>>>  4 files changed, 301 insertions(+), 220 deletions(-)
>>>  create mode 100644 drivers/tty/serial/8250/8250_lpss.c
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
>>> new file mode 100644
>>> index 0000000..bca4adb
>>> --- /dev/null
>>> +++ b/drivers/tty/serial/8250/8250_lpss.c
>>> @@ -0,0 +1,279 @@
>>> +/*
>>> + * 8250_lpss.c - Driver for UART on Intel Braswell and various other Intel SoCs
>>> + *
>>> + * Copyright (C) 2016 Intel Corporation
>>> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/module.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/rational.h>
>>> +
>>> +#include <linux/dmaengine.h>
>>> +#include <linux/platform_data/dma-dw.h>
>>> +
>>> +#include "8250.h"
>>> +
>>> +#define PCI_DEVICE_ID_INTEL_BYT_UART1        0x0f0a
>>> +#define PCI_DEVICE_ID_INTEL_BYT_UART2        0x0f0c
>>> +
>>> +#define PCI_DEVICE_ID_INTEL_BSW_UART1        0x228a
>>> +#define PCI_DEVICE_ID_INTEL_BSW_UART2        0x228c
>>> +
>>> +#define PCI_DEVICE_ID_INTEL_BDW_UART1        0x9ce3
>>> +#define PCI_DEVICE_ID_INTEL_BDW_UART2        0x9ce4
>>> +
>>> +/* Intel LPSS specific registers */
>>> +
>>> +#define BYT_PRV_CLK                  0x800
>>> +#define BYT_PRV_CLK_EN                       BIT(0)
>>> +#define BYT_PRV_CLK_M_VAL_SHIFT              1
>>> +#define BYT_PRV_CLK_N_VAL_SHIFT              16
>>> +#define BYT_PRV_CLK_UPDATE           BIT(31)
>>> +
>>> +#define BYT_TX_OVF_INT                       0x820
>>> +#define BYT_TX_OVF_INT_MASK          BIT(1)
>>> +
>>> +struct lpss8250;
>>> +
>>> +struct lpss8250_board {
>>> +     unsigned long freq;
>>> +     unsigned int base_baud;
>>> +     int (*setup)(struct lpss8250 *, struct uart_port *p);
>>> +};
>>
>> New concept.
>>
>>> +
>>> +struct lpss8250 {
>>> +     int line;
>>> +     struct lpss8250_board *board;
>>> +
>>> +     /* DMA parameters */
>>> +     struct uart_8250_dma dma;
>>> +     struct dw_dma_slave dma_param;
>>> +     u8 dma_maxburst;
>>> +};
>>> +
>>> +/*****************************************************************************/
>>
>> Please remove.
>>
>>> +
>>> +static void lpss8250_set_termios(struct uart_port *p,
>>> +                              struct ktermios *termios,
>>> +                              struct ktermios *old)
>>> +{
>>> +     unsigned int baud = tty_termios_baud_rate(termios);
>>> +     struct lpss8250 *lpss = p->private_data;
>>> +     unsigned long fref = lpss->board->freq, fuart = baud * 16;
>>> +     unsigned long w = BIT(15) - 1;
>>> +     unsigned long m, n;
>>> +     u32 reg;
>>> +
>>> +     /* Get Fuart closer to Fref */
>>> +     fuart *= rounddown_pow_of_two(fref / fuart);
>>> +
>>> +     /*
>>> +      * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
>>> +      * dividers must be adjusted.
>>> +      *
>>> +      * uartclk = (m / n) * 100 MHz, where m <= n
>>> +      */
>>> +     rational_best_approximation(fuart, fref, w, w, &m, &n);
>>> +     p->uartclk = fuart;
>>> +
>>> +     /* Reset the clock */
>>> +     reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
>>> +     writel(reg, p->membase + BYT_PRV_CLK);
>>> +     reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
>>> +     writel(reg, p->membase + BYT_PRV_CLK);
>>> +
>>> +     p->status &= ~UPSTAT_AUTOCTS;
>>> +     if (termios->c_cflag & CRTSCTS)
>>> +             p->status |= UPSTAT_AUTOCTS;
>>> +
>>> +     serial8250_do_set_termios(p, termios, old);
>>> +}
>>> +
>>> +/*****************************************************************************/
>>
>> Please remove.
>>
>>> +
>>> +static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
>>
>> This would have been much easier to review if you had simply moved it here
>> and done the rework in a follow-on patch.
> 
> I didn't quite get this one.

Well, just comparing byt_serial_setup() from the two versions:
1) dma setup is in a completely separate function
2) the tx & rx dma parameters are folded together
3) the port setup is split out
4) introduction of struct lpss8250
...


> How series should look like?

I would have just moved byt_serial_setup() without any of the other changes
except perhaps replacing pciserial_board with lpss8250_board, and
then made the other changes on top before the Quark patches.

There is no changelog describing the purpose of struct lpss8250_board, or
struct lpss8250. Or of the dma changes. Or why dma_maxburst was parameterized.
...

Naturally, I can figure all of that out on my own, but it would have been
better to read your reasoning.

It looks alot of work to split out now, so I guess what's done is done, and I'll
just review this *really* carefully. But imagine if you hadn't wrote it, and
were reviewing this: it's very difficult to mentally separate out the changes
and keep track of them while reviewing. Side-by-side diff is nearly useless...



>>
>>
>>> +{
>>> +     struct dw_dma_slave *param = &lpss->dma_param;
>>> +     struct uart_8250_port *up = up_to_u8250p(port);
>>> +     struct pci_dev *pdev = to_pci_dev(port->dev);
>>> +     struct pci_dev *dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
>>> +
>>> +     switch (pdev->device) {
>>> +     case PCI_DEVICE_ID_INTEL_BYT_UART1:
>>> +     case PCI_DEVICE_ID_INTEL_BSW_UART1:
>>> +     case PCI_DEVICE_ID_INTEL_BDW_UART1:
>>> +             param->src_id = 3;
>>> +             param->dst_id = 2;
>>> +             break;
>>> +     case PCI_DEVICE_ID_INTEL_BYT_UART2:
>>> +     case PCI_DEVICE_ID_INTEL_BSW_UART2:
>>> +     case PCI_DEVICE_ID_INTEL_BDW_UART2:
>>> +             param->src_id = 5;
>>> +             param->dst_id = 4;
>>> +             break;
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     param->dma_dev = &dma_dev->dev;
>>> +     param->m_master = 0;
>>> +     param->p_master = 1;
>>> +
>>> +     /* TODO: Detect FIFO size automaticaly for DesignWare 8250 */
>>> +     port->fifosize = 64;
>>> +     up->tx_loadsz = 64;
>>> +
>>> +     lpss->dma_maxburst = 16;
>>> +
>>> +     port->set_termios = lpss8250_set_termios;
>>> +
>>> +     /* Disable TX counter interrupts */
>>> +     writel(BYT_TX_OVF_INT_MASK, port->membase + BYT_TX_OVF_INT);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +/*****************************************************************************/
>>
>> Please remove.
>>
>>> +
>>> +static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
>>> +{
>>> +     struct dw_dma_slave *dws = param;
>>> +
>>> +     if (dws->dma_dev != chan->device->dev)
>>> +             return false;
>>> +
>>> +     chan->private = dws;
>>> +     return true;
>>> +}
>>> +
>>> +static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port)
>>> +{
>>> +     struct uart_8250_dma *dma = &lpss->dma;
>>> +     struct dw_dma_slave *param = &lpss->dma_param;
>>
>> Unnecessary alias.
>>
>>> +     struct dw_dma_slave *rx_param, *tx_param;
>>> +     struct device *dev = port->port.dev;
>>> +
>>> +     rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
>>> +     if (!rx_param)
>>> +             return -ENOMEM;
>>> +
>>> +     tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
>>> +     if (!tx_param)
>>> +             return -ENOMEM;
>>> +
>>> +     *rx_param = *param;
>>> +     dma->rxconf.src_maxburst = lpss->dma_maxburst;
>>> +
>>> +     *tx_param = *param;
>>> +     dma->txconf.dst_maxburst = lpss->dma_maxburst;
>>> +
>>> +     dma->fn = lpss8250_dma_filter;
>>> +     dma->rx_param = rx_param;
>>> +     dma->tx_param = tx_param;
>>> +
>>> +     port->dma = dma;
>>> +     return 0;
>>> +}
>>> +
>>> +/*****************************************************************************/
>>
>> Please remove.
>>
>>> +
>>> +static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>> +{
>>> +     struct uart_8250_port uart;
>>> +     struct lpss8250 *lpss;
>>> +     int ret;
>>> +
>>> +     ret = pcim_enable_device(pdev);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     pci_set_master(pdev);
>>> +
>>> +     lpss = devm_kzalloc(&pdev->dev, sizeof(*lpss), GFP_KERNEL);
>>> +     if (!lpss)
>>> +             return -ENOMEM;
>>> +
>>> +     lpss->board = (struct lpss8250_board *)id->driver_data;
>>> +
>>> +     memset(&uart, 0, sizeof(struct uart_8250_port));
>>> +
>>> +     uart.port.dev = &pdev->dev;
>>> +     uart.port.irq = pdev->irq;
>>> +     uart.port.private_data = lpss;
>>> +     uart.port.type = PORT_16550A;
>>> +     uart.port.iotype = UPIO_MEM32;
>>
>> UPIO_MEM
>>
>>
>>> +     uart.port.regshift = 2;
>>> +     uart.port.uartclk = lpss->board->base_baud * 16;
>>> +     uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
>>> +
>>
>> Please remove empty line
>>
>>> +     uart.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
>>> +
>>
>> Same
>>
>>> +     uart.port.mapbase = pci_resource_start(pdev, 0);
>>> +     uart.port.membase = pcim_iomap(pdev, 0, 0);
>>> +     if (!uart.port.membase)
>>> +             return -ENOMEM;
>>> +
>>> +     if (lpss->board->setup) {
>>
>> There's no design that doesn't have a setup method.
> 
> For now, indeed. Okay, I will call it directly.
> 
>>
>>
>>> +             ret = lpss->board->setup(lpss, &uart.port);
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>> +
>>> +     ret = lpss8250_dma_setup(lpss, &uart);
>>> +     if (ret)
>>> +             return ret;
>>
>> I would fold this call into board setup which avoids the
>> ugliness when this error pathway is reworked in the
>> follow-on patches.
> 
> Each of them?

I'm assuming there's just the two: byt_serial_setup()
and qrk_serial_setup()?

Did I overlook something? Perhaps I missed some design goal?


>>
>>
>>> +
>>> +     ret = serial8250_register_8250_port(&uart);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     lpss->line = ret;
>>> +
>>> +     pci_set_drvdata(pdev, lpss);
>>> +     return 0;
>>> +}
>>> +
>>> +static void lpss8250_remove(struct pci_dev *pdev)
>>> +{
>>> +     struct lpss8250 *lpss = pci_get_drvdata(pdev);
>>> +
>>> +     serial8250_unregister_port(lpss->line);
>>> +}
>>> +
>>> +static const struct lpss8250_board byt_board = {
>>> +     .freq = 100000000,
>>> +     .base_baud = 2764800,
>>> +     .setup = byt_serial_setup,
>>> +};
>>> +
>>> +#define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
>>> +
>>> +static const struct pci_device_id pci_ids[] = {
>>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART1, byt_board),
>>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART2, byt_board),
>>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART1, byt_board),
>>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART2, byt_board),
>>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART1, byt_board),
>>> +     LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART2, byt_board),
>>> +     { },
>>> +};
>>> +MODULE_DEVICE_TABLE(pci, pci_ids);
>>> +
>>> +static struct pci_driver lpss8250_pci_driver = {
>>> +     .name           = "8250_lpss",
>>> +     .id_table       = pci_ids,
>>> +     .probe          = lpss8250_probe,
>>> +     .remove         = lpss8250_remove,
>>
>> No power management?
> 
> PCI does the trick, no *special* power management treatment required, yes.

I realized that later this am; sorry about that.
[Maybe just put a small note in the changelog about that though?]

Regards,
Peter Hurley


>>
>>
>>> +};
>>> +
>>> +module_pci_driver(lpss8250_pci_driver);
>>> +
>>> +MODULE_AUTHOR("Intel Corporation");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DESCRIPTION("Intel LPSS UART driver");
>>> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
>>> index 5eea74d..bb4df5d 100644
>>> --- a/drivers/tty/serial/8250/8250_pci.c
>>> +++ b/drivers/tty/serial/8250/8250_pci.c
>>> @@ -21,14 +21,10 @@
>>>  #include <linux/serial_core.h>
>>>  #include <linux/8250_pci.h>
>>>  #include <linux/bitops.h>
>>> -#include <linux/rational.h>
>>>
>>>  #include <asm/byteorder.h>
>>>  #include <asm/io.h>
>>>
>>> -#include <linux/dmaengine.h>
>>> -#include <linux/platform_data/dma-dw.h>
>>> -
>>>  #include "8250.h"
>>>
>>>  /*
>>> @@ -1349,145 +1345,6 @@ ce4100_serial_setup(struct serial_private *priv,
>>>       return ret;
>>>  }
>>>
>>> -#define PCI_DEVICE_ID_INTEL_BYT_UART1        0x0f0a
>>> -#define PCI_DEVICE_ID_INTEL_BYT_UART2        0x0f0c
>>> -
>>> -#define PCI_DEVICE_ID_INTEL_BSW_UART1        0x228a
>>> -#define PCI_DEVICE_ID_INTEL_BSW_UART2        0x228c
>>> -
>>> -#define PCI_DEVICE_ID_INTEL_BDW_UART1        0x9ce3
>>> -#define PCI_DEVICE_ID_INTEL_BDW_UART2        0x9ce4
>>> -
>>> -#define BYT_PRV_CLK                  0x800
>>> -#define BYT_PRV_CLK_EN                       (1 << 0)
>>> -#define BYT_PRV_CLK_M_VAL_SHIFT              1
>>> -#define BYT_PRV_CLK_N_VAL_SHIFT              16
>>> -#define BYT_PRV_CLK_UPDATE           (1 << 31)
>>> -
>>> -#define BYT_TX_OVF_INT                       0x820
>>> -#define BYT_TX_OVF_INT_MASK          (1 << 1)
>>> -
>>> -static void
>>> -byt_set_termios(struct uart_port *p, struct ktermios *termios,
>>> -             struct ktermios *old)
>>> -{
>>> -     unsigned int baud = tty_termios_baud_rate(termios);
>>> -     unsigned long fref = 100000000, fuart = baud * 16;
>>> -     unsigned long w = BIT(15) - 1;
>>> -     unsigned long m, n;
>>> -     u32 reg;
>>> -
>>> -     /* Get Fuart closer to Fref */
>>> -     fuart *= rounddown_pow_of_two(fref / fuart);
>>> -
>>> -     /*
>>> -      * For baud rates 0.5M, 1M, 1.5M, 2M, 2.5M, 3M, 3.5M and 4M the
>>> -      * dividers must be adjusted.
>>> -      *
>>> -      * uartclk = (m / n) * 100 MHz, where m <= n
>>> -      */
>>> -     rational_best_approximation(fuart, fref, w, w, &m, &n);
>>> -     p->uartclk = fuart;
>>> -
>>> -     /* Reset the clock */
>>> -     reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) | (n << BYT_PRV_CLK_N_VAL_SHIFT);
>>> -     writel(reg, p->membase + BYT_PRV_CLK);
>>> -     reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
>>> -     writel(reg, p->membase + BYT_PRV_CLK);
>>> -
>>> -     p->status &= ~UPSTAT_AUTOCTS;
>>> -     if (termios->c_cflag & CRTSCTS)
>>> -             p->status |= UPSTAT_AUTOCTS;
>>> -
>>> -     serial8250_do_set_termios(p, termios, old);
>>> -}
>>> -
>>> -static bool byt_dma_filter(struct dma_chan *chan, void *param)
>>> -{
>>> -     struct dw_dma_slave *dws = param;
>>> -
>>> -     if (dws->dma_dev != chan->device->dev)
>>> -             return false;
>>> -
>>> -     chan->private = dws;
>>> -     return true;
>>> -}
>>> -
>>> -static int
>>> -byt_serial_setup(struct serial_private *priv,
>>> -              const struct pciserial_board *board,
>>> -              struct uart_8250_port *port, int idx)
>>> -{
>>> -     struct pci_dev *pdev = priv->dev;
>>> -     struct device *dev = port->port.dev;
>>> -     struct uart_8250_dma *dma;
>>> -     struct dw_dma_slave *tx_param, *rx_param;
>>> -     struct pci_dev *dma_dev;
>>> -     int ret;
>>> -
>>> -     dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
>>> -     if (!dma)
>>> -             return -ENOMEM;
>>> -
>>> -     tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
>>> -     if (!tx_param)
>>> -             return -ENOMEM;
>>> -
>>> -     rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
>>> -     if (!rx_param)
>>> -             return -ENOMEM;
>>> -
>>> -     switch (pdev->device) {
>>> -     case PCI_DEVICE_ID_INTEL_BYT_UART1:
>>> -     case PCI_DEVICE_ID_INTEL_BSW_UART1:
>>> -     case PCI_DEVICE_ID_INTEL_BDW_UART1:
>>> -             rx_param->src_id = 3;
>>> -             tx_param->dst_id = 2;
>>> -             break;
>>> -     case PCI_DEVICE_ID_INTEL_BYT_UART2:
>>> -     case PCI_DEVICE_ID_INTEL_BSW_UART2:
>>> -     case PCI_DEVICE_ID_INTEL_BDW_UART2:
>>> -             rx_param->src_id = 5;
>>> -             tx_param->dst_id = 4;
>>> -             break;
>>> -     default:
>>> -             return -EINVAL;
>>> -     }
>>> -
>>> -     rx_param->m_master = 0;
>>> -     rx_param->p_master = 1;
>>> -
>>> -     dma->rxconf.src_maxburst = 16;
>>> -
>>> -     tx_param->m_master = 0;
>>> -     tx_param->p_master = 1;
>>> -
>>> -     dma->txconf.dst_maxburst = 16;
>>> -
>>> -     dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
>>> -     rx_param->dma_dev = &dma_dev->dev;
>>> -     tx_param->dma_dev = &dma_dev->dev;
>>> -
>>> -     dma->fn = byt_dma_filter;
>>> -     dma->rx_param = rx_param;
>>> -     dma->tx_param = tx_param;
>>> -
>>> -     ret = pci_default_setup(priv, board, port, idx);
>>> -     port->port.iotype = UPIO_MEM;
>>> -     port->port.type = PORT_16550A;
>>> -     port->port.flags = (port->port.flags | UPF_FIXED_PORT | UPF_FIXED_TYPE);
>>> -     port->port.set_termios = byt_set_termios;
>>> -     port->port.fifosize = 64;
>>> -     port->tx_loadsz = 64;
>>> -     port->dma = dma;
>>> -     port->capabilities = UART_CAP_FIFO | UART_CAP_AFE;
>>> -
>>> -     /* Disable Tx counter interrupts */
>>> -     writel(BYT_TX_OVF_INT_MASK, port->port.membase + BYT_TX_OVF_INT);
>>> -
>>> -     return ret;
>>> -}
>>> -
>>>  static int
>>>  pci_omegapci_setup(struct serial_private *priv,
>>>                     const struct pciserial_board *board,
>>> @@ -2015,48 +1872,6 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>>>               .subdevice      = PCI_ANY_ID,
>>>               .setup          = kt_serial_setup,
>>>       },
>>> -     {
>>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>>> -             .device         = PCI_DEVICE_ID_INTEL_BYT_UART1,
>>> -             .subvendor      = PCI_ANY_ID,
>>> -             .subdevice      = PCI_ANY_ID,
>>> -             .setup          = byt_serial_setup,
>>> -     },
>>> -     {
>>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>>> -             .device         = PCI_DEVICE_ID_INTEL_BYT_UART2,
>>> -             .subvendor      = PCI_ANY_ID,
>>> -             .subdevice      = PCI_ANY_ID,
>>> -             .setup          = byt_serial_setup,
>>> -     },
>>> -     {
>>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>>> -             .device         = PCI_DEVICE_ID_INTEL_BSW_UART1,
>>> -             .subvendor      = PCI_ANY_ID,
>>> -             .subdevice      = PCI_ANY_ID,
>>> -             .setup          = byt_serial_setup,
>>> -     },
>>> -     {
>>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>>> -             .device         = PCI_DEVICE_ID_INTEL_BSW_UART2,
>>> -             .subvendor      = PCI_ANY_ID,
>>> -             .subdevice      = PCI_ANY_ID,
>>> -             .setup          = byt_serial_setup,
>>> -     },
>>> -     {
>>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>>> -             .device         = PCI_DEVICE_ID_INTEL_BDW_UART1,
>>> -             .subvendor      = PCI_ANY_ID,
>>> -             .subdevice      = PCI_ANY_ID,
>>> -             .setup          = byt_serial_setup,
>>> -     },
>>> -     {
>>> -             .vendor         = PCI_VENDOR_ID_INTEL,
>>> -             .device         = PCI_DEVICE_ID_INTEL_BDW_UART2,
>>> -             .subvendor      = PCI_ANY_ID,
>>> -             .subdevice      = PCI_ANY_ID,
>>> -             .setup          = byt_serial_setup,
>>> -     },
>>>       /*
>>>        * ITE
>>>        */
>>> @@ -2921,7 +2736,6 @@ enum pci_board_num_t {
>>>       pbn_ADDIDATA_PCIe_4_3906250,
>>>       pbn_ADDIDATA_PCIe_8_3906250,
>>>       pbn_ce4100_1_115200,
>>> -     pbn_byt,
>>>       pbn_qrk,
>>>       pbn_omegapci,
>>>       pbn_NETMOS9900_2s_115200,
>>> @@ -3698,12 +3512,6 @@ static struct pciserial_board pci_boards[] = {
>>>               .base_baud      = 921600,
>>>               .reg_shift      = 2,
>>>       },
>>> -     [pbn_byt] = {
>>> -             .flags          = FL_BASE0,
>>> -             .num_ports      = 1,
>>> -             .base_baud      = 2764800,
>>> -             .reg_shift      = 2,
>>> -     },
>>>       [pbn_qrk] = {
>>>               .flags          = FL_BASE0,
>>>               .num_ports      = 1,
>>> @@ -3820,6 +3628,14 @@ static const struct pci_device_id blacklist[] = {
>>>       { PCI_VDEVICE(INTEL, 0x081d), },
>>>       { PCI_VDEVICE(INTEL, 0x1191), },
>>>       { PCI_VDEVICE(INTEL, 0x19d8), },
>>> +
>>> +     /* Intel platforms with DesignWare UART */
>>> +     { PCI_VDEVICE(INTEL, 0x0f0a), },
>>> +     { PCI_VDEVICE(INTEL, 0x0f0c), },
>>> +     { PCI_VDEVICE(INTEL, 0x228a), },
>>> +     { PCI_VDEVICE(INTEL, 0x228c), },
>>> +     { PCI_VDEVICE(INTEL, 0x9ce3), },
>>> +     { PCI_VDEVICE(INTEL, 0x9ce4), },
>>>  };
>>>
>>>  /*
>>> @@ -5485,33 +5301,6 @@ static struct pci_device_id serial_pci_tbl[] = {
>>>       {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART,
>>>               PCI_ANY_ID,  PCI_ANY_ID, 0, 0,
>>>               pbn_ce4100_1_115200 },
>>> -     /* Intel BayTrail */
>>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART1,
>>> -             PCI_ANY_ID,  PCI_ANY_ID,
>>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>>> -             pbn_byt },
>>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT_UART2,
>>> -             PCI_ANY_ID,  PCI_ANY_ID,
>>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>>> -             pbn_byt },
>>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART1,
>>> -             PCI_ANY_ID,  PCI_ANY_ID,
>>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>>> -             pbn_byt },
>>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW_UART2,
>>> -             PCI_ANY_ID,  PCI_ANY_ID,
>>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>>> -             pbn_byt },
>>> -
>>> -     /* Intel Broadwell */
>>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART1,
>>> -             PCI_ANY_ID,  PCI_ANY_ID,
>>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>>> -             pbn_byt },
>>> -     {       PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BDW_UART2,
>>> -             PCI_ANY_ID,  PCI_ANY_ID,
>>> -             PCI_CLASS_COMMUNICATION_SERIAL << 8, 0xff0000,
>>> -             pbn_byt },
>>>
>>>       /*
>>>        * Intel Quark x1000
>>> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>>> index 64742a0..6fde7d2 100644
>>> --- a/drivers/tty/serial/8250/Kconfig
>>> +++ b/drivers/tty/serial/8250/Kconfig
>>> @@ -108,7 +108,6 @@ config SERIAL_8250_PCI
>>>       tristate "8250/16550 PCI device support" if EXPERT
>>>       depends on SERIAL_8250 && PCI
>>>       default SERIAL_8250
>>> -     select RATIONAL
>>>       help
>>>         This builds standard PCI serial support. You may be able to
>>>         disable this feature if you only need legacy serial support.
>>> @@ -398,6 +397,19 @@ config SERIAL_8250_INGENIC
>>>         If you have a system using an Ingenic SoC and wish to make use of
>>>         its UARTs, say Y to this option. If unsure, say N.
>>>
>>> +config SERIAL_8250_LPSS
>>> +     tristate "Support for serial ports on Intel LPSS platforms" if EXPERT
>>> +     default SERIAL_8250
>>> +     depends on SERIAL_8250 && PCI
>>> +     depends on X86 || COMPILE_TEST
>>> +     select DW_DMAC_CORE if SERIAL_8250_DMA
>>> +     select DW_DMAC_PCI if X86_INTEL_LPSS
>>> +     select RATIONAL
>>> +     help
>>> +       Selecting this option will enable handling of the extra features
>>> +       present on the UART found on Intel Braswell SoC and various other
>>> +       Intel platforms.
>>> +
>>>  config SERIAL_8250_MID
>>>       tristate "Support for serial ports on Intel MID platforms"
>>>       depends on SERIAL_8250 && PCI
>>> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
>>> index c9a2d6e..ca37d1c 100644
>>> --- a/drivers/tty/serial/8250/Makefile
>>> +++ b/drivers/tty/serial/8250/Makefile
>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_LPC18XX)   += 8250_lpc18xx.o
>>>  obj-$(CONFIG_SERIAL_8250_MT6577)     += 8250_mtk.o
>>>  obj-$(CONFIG_SERIAL_8250_UNIPHIER)   += 8250_uniphier.o
>>>  obj-$(CONFIG_SERIAL_8250_INGENIC)    += 8250_ingenic.o
>>> +obj-$(CONFIG_SERIAL_8250_LPSS)               += 8250_lpss.o
>>>  obj-$(CONFIG_SERIAL_8250_MID)                += 8250_mid.o
>>>  obj-$(CONFIG_SERIAL_8250_MOXA)               += 8250_moxa.o
>>>  obj-$(CONFIG_SERIAL_OF_PLATFORM)     += 8250_of.o
>>>
>>
> 
> 
> 

Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module

[Andy Shevchenko]

On Fri, 2016-04-08 at 15:44 -0700, Peter Hurley wrote:
> On 04/08/2016 01:17 AM, Andy Shevchenko wrote:
> > 
> > On Fri, Apr 8, 2016 at 4:42 AM, Peter Hurley <peter@hurleysoftware.c
> > om> wrote:
> > > 
> > > On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> > > > 
> > > > Intes SoCs, such as Braswell, have DesignWare UART. Split out to
> > > > separate
> > > > module which also will be used for Intel Quark later.
> > > What's the rationale?
> > 1. Not poison 8250_pci with too many quirks.
> > 2. They all use same DMA engine, otherwise we might end up in all
> > possible DMA engines included in one file.
> > 3. All of them are actually DesignWare, so, in the future we might
> > share code between 8250_dw and 8250_lpss.
> Just my opinion, but I like to see the rationale in the changelog.

Agreed. Already did locally.

> 
> > > And this really isn't a split; this patch introduces a number of
> > > significant
> > > changes from the pci version.
> > Some style changes, yes, but "significant"?
> > For example?
> I'm just pointing out the changelog doesn't really match the
> commit. I'm not suggesting necessarily to redo the series, but just
> more
> adequately reflect the change. See below.


> +struct lpss8250_board {
> > > > +     unsigned long freq;
> > > > +     unsigned int base_baud;
> > > > +     int (*setup)(struct lpss8250 *, struct uart_port *p);
> > > > +};
> > > New concept.

> > > +struct lpss8250 {
> > > > +     int line;
> > > > +     struct lpss8250_board *board;
> > > > +
> > > > +     /* DMA parameters */
> > > > +     struct uart_8250_dma dma;
> > > > +     struct dw_dma_slave dma_param;
> > > > +     u8 dma_maxburst;
> > > > +};

> > > > +static int byt_serial_setup(struct lpss8250 *lpss, struct
> > > > uart_port *port)
> > > This would have been much easier to review if you had simply moved
> > > it here
> > > and done the rework in a follow-on patch.
> > I didn't quite get this one.
> Well, just comparing byt_serial_setup() from the two versions:
> 1) dma setup is in a completely separate function
> 2) the tx & rx dma parameters are folded together
> 3) the port setup is split out
> 4) introduction of struct lpss8250
> ...


> 
> > 
> > How series should look like?
> I would have just moved byt_serial_setup() without any of the other
> changes
> except perhaps replacing pciserial_board with lpss8250_board, and
> then made the other changes on top before the Quark patches.
> 
> There is no changelog describing the purpose of struct lpss8250_board,
> or
> struct lpss8250. Or of the dma changes. Or why dma_maxburst was
> parameterized.
> ...
> 
> Naturally, I can figure all of that out on my own, but it would have
> been
> better to read your reasoning.
> 
> It looks alot of work to split out now, so I guess what's done is
> done, and I'll
> just review this *really* carefully. But imagine if you hadn't wrote
> it, and
> were reviewing this: it's very difficult to mentally separate out the
> changes
> and keep track of them while reviewing. Side-by-side diff is nearly
> useless...
> 

I sent new version, please, review.

> > > > 
> > > > +             ret = lpss->board->setup(lpss, &uart.port);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     ret = lpss8250_dma_setup(lpss, &uart);
> > > > +     if (ret)
> > > > +             return ret;
> > > I would fold this call into board setup which avoids the
> > > ugliness when this error pathway is reworked in the
> > > follow-on patches.
> > Each of them?
> I'm assuming there's just the two: byt_serial_setup()
> and qrk_serial_setup()?

For now yes.

> 
> Did I overlook something? Perhaps I missed some design goal?

Nope. 
But I still prefer to have separate _dma_setup() helper. I didn't see
too much ugliness in the next patches.

> > > > +     .probe          = lpss8250_probe,
> > > > +     .remove         = lpss8250_remove,
> > > No power management?
> > PCI does the trick, no *special* power management treatment
> > required, yes.
> I realized that later this am; sorry about that.
> [Maybe just put a small note in the changelog about that though?]

OK.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v1 10/12] serial: 8250_lpss: move Quark code from PCI driver

[Andy Shevchenko]

Intel Quark has DesignWare UART. Move the code from 8250_pci to 8250_lpss.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 11 +++++++++++
 drivers/tty/serial/8250/8250_pci.c  | 15 +--------------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index bca4adb..1095612 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -25,6 +25,8 @@
 #define PCI_DEVICE_ID_INTEL_BSW_UART1	0x228a
 #define PCI_DEVICE_ID_INTEL_BSW_UART2	0x228c
 
+#define PCI_DEVICE_ID_INTEL_QRK_UARTx	0x0936
+
 #define PCI_DEVICE_ID_INTEL_BDW_UART1	0x9ce3
 #define PCI_DEVICE_ID_INTEL_BDW_UART2	0x9ce4
 
@@ -159,6 +161,9 @@ static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port
 	struct dw_dma_slave *rx_param, *tx_param;
 	struct device *dev = port->port.dev;
 
+	if (!param->dma_dev)
+		return 0;
+
 	rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
 	if (!rx_param)
 		return -ENOMEM;
@@ -252,6 +257,11 @@ static const struct lpss8250_board byt_board = {
 	.setup = byt_serial_setup,
 };
 
+static const struct lpss8250_board qrk_board = {
+	.freq = 44236800,
+	.base_baud = 2764800,
+};
+
 #define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
 
 static const struct pci_device_id pci_ids[] = {
@@ -259,6 +269,7 @@ static const struct pci_device_id pci_ids[] = {
 	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BYT_UART2, byt_board),
 	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART1, byt_board),
 	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BSW_UART2, byt_board),
+	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_QRK_UARTx, qrk_board),
 	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART1, byt_board),
 	LPSS_DEVICE(PCI_DEVICE_ID_INTEL_BDW_UART2, byt_board),
 	{ },
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index bb4df5d..b94b1ee 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1765,7 +1765,6 @@ pci_wch_ch38x_setup(struct serial_private *priv,
 #define PCI_DEVICE_ID_COMMTECH_4222PCIE	0x0022
 #define PCI_DEVICE_ID_BROADCOM_TRUMANAGE 0x160a
 #define PCI_DEVICE_ID_AMCC_ADDIDATA_APCI7800 0x818e
-#define PCI_DEVICE_ID_INTEL_QRK_UART	0x0936
 
 #define PCI_VENDOR_ID_SUNIX		0x1fd4
 #define PCI_DEVICE_ID_SUNIX_1999	0x1999
@@ -2736,7 +2735,6 @@ enum pci_board_num_t {
 	pbn_ADDIDATA_PCIe_4_3906250,
 	pbn_ADDIDATA_PCIe_8_3906250,
 	pbn_ce4100_1_115200,
-	pbn_qrk,
 	pbn_omegapci,
 	pbn_NETMOS9900_2s_115200,
 	pbn_brcm_trumanage,
@@ -3512,12 +3510,6 @@ static struct pciserial_board pci_boards[] = {
 		.base_baud	= 921600,
 		.reg_shift      = 2,
 	},
-	[pbn_qrk] = {
-		.flags		= FL_BASE0,
-		.num_ports	= 1,
-		.base_baud	= 2764800,
-		.reg_shift	= 2,
-	},
 	[pbn_omegapci] = {
 		.flags		= FL_BASE0,
 		.num_ports	= 8,
@@ -3634,6 +3626,7 @@ static const struct pci_device_id blacklist[] = {
 	{ PCI_VDEVICE(INTEL, 0x0f0c), },
 	{ PCI_VDEVICE(INTEL, 0x228a), },
 	{ PCI_VDEVICE(INTEL, 0x228c), },
+	{ PCI_VDEVICE(INTEL, 0x0936), },
 	{ PCI_VDEVICE(INTEL, 0x9ce3), },
 	{ PCI_VDEVICE(INTEL, 0x9ce4), },
 };
@@ -5303,12 +5296,6 @@ static struct pci_device_id serial_pci_tbl[] = {
 		pbn_ce4100_1_115200 },
 
 	/*
-	 * Intel Quark x1000
-	 */
-	{	PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_QRK_UART,
-		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		pbn_qrk },
-	/*
 	 * Cronyx Omega PCI
 	 */
 	{	PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_CRONYX_OMEGA,
-- 
2.8.0.rc3

[PATCH v1 11/12] serial: 8250_lpss: enable MSI for Intel Quark

[Andy Shevchenko]

Intel Quark SoC supports MSI for LPSS, in particular for UART. Enable MSI for
Intel Quark.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 1095612..1b36d32 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -141,6 +141,17 @@ static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 	return 0;
 }
 
+static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
+{
+	struct pci_dev *pdev = to_pci_dev(port->dev);
+
+	pci_enable_msi(pdev);
+
+	port->irq = pdev->irq;
+
+	return 0;
+}
+
 /*****************************************************************************/
 
 static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
@@ -260,6 +271,7 @@ static const struct lpss8250_board byt_board = {
 static const struct lpss8250_board qrk_board = {
 	.freq = 44236800,
 	.base_baud = 2764800,
+	.setup = qrk_serial_setup,
 };
 
 #define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
-- 
2.8.0.rc3

[PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Andy Shevchenko]

DMA on Intel Quark SoC is a part of UART IP block. Enable it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 60 +++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 1b36d32..bc55738 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -15,7 +15,7 @@
 #include <linux/rational.h>
 
 #include <linux/dmaengine.h>
-#include <linux/platform_data/dma-dw.h>
+#include <linux/dma/dw.h>
 
 #include "8250.h"
 
@@ -47,6 +47,7 @@ struct lpss8250_board {
 	unsigned long freq;
 	unsigned int base_baud;
 	int (*setup)(struct lpss8250 *, struct uart_port *p);
+	void (*exit)(struct lpss8250 *);
 };
 
 struct lpss8250 {
@@ -55,6 +56,7 @@ struct lpss8250 {
 
 	/* DMA parameters */
 	struct uart_8250_dma dma;
+	struct dw_dma_chip dma_chip;
 	struct dw_dma_slave dma_param;
 	u8 dma_maxburst;
 };
@@ -141,17 +143,60 @@ static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 	return 0;
 }
 
+static const struct dw_dma_platform_data qrk_serial_dma_pdata = {
+	.nr_channels = 2,
+	.is_private = true,
+	.is_nollp = true,
+	.chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
+	.chan_priority = CHAN_PRIORITY_ASCENDING,
+	.block_size = 4095,
+	.nr_masters = 1,
+	.data_width = 4,
+};
+
 static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 {
+	struct uart_8250_dma *dma = &lpss->dma;
+	struct dw_dma_chip *chip = &lpss->dma_chip;
+	struct dw_dma_slave *param = &lpss->dma_param;
 	struct pci_dev *pdev = to_pci_dev(port->dev);
+	int ret;
 
 	pci_enable_msi(pdev);
 
 	port->irq = pdev->irq;
 
+	chip->dev = &pdev->dev;
+	chip->irq = pdev->irq;
+	chip->regs = pci_ioremap_bar(pdev, 1);
+	chip->pdata = &qrk_serial_dma_pdata;
+
+	/* Falling back to PIO mode if DMA probing fails */
+	ret = dw_dma_probe(chip);
+	if (ret)
+		return 0;
+
+	/* Special DMA address for UART */
+	dma->dma_addr = 0xfffff000;
+
+	param->dma_dev = &pdev->dev;
+	param->src_id = 0;
+	param->dst_id = 1;
+	param->polarity = true;
+
+	lpss->dma_maxburst = 8;
 	return 0;
 }
 
+static void qrk_serial_exit(struct lpss8250 *lpss)
+{
+	struct dw_dma_slave *param = &lpss->dma_param;
+
+	if (!param->dma_dev)
+		return;
+	dw_dma_remove(&lpss->dma_chip);
+}
+
 /*****************************************************************************/
 
 static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
@@ -243,22 +288,30 @@ static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	ret = lpss8250_dma_setup(lpss, &uart);
 	if (ret)
-		return ret;
+		goto err_exit;
 
 	ret = serial8250_register_8250_port(&uart);
 	if (ret < 0)
-		return ret;
+		goto err_exit;
 
 	lpss->line = ret;
 
 	pci_set_drvdata(pdev, lpss);
 	return 0;
+
+err_exit:
+	if (lpss->board->exit)
+		lpss->board->exit(lpss);
+	return ret;
 }
 
 static void lpss8250_remove(struct pci_dev *pdev)
 {
 	struct lpss8250 *lpss = pci_get_drvdata(pdev);
 
+	if (lpss->board->exit)
+		lpss->board->exit(lpss);
+
 	serial8250_unregister_port(lpss->line);
 }
 
@@ -272,6 +325,7 @@ static const struct lpss8250_board qrk_board = {
 	.freq = 44236800,
 	.base_baud = 2764800,
 	.setup = qrk_serial_setup,
+	.exit = qrk_serial_exit,
 };
 
 #define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
-- 
2.8.0.rc3

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Bryan O'Donoghue]

On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:

Preface. I tried this on Galileo and it appears to work. I'll do some
throughput testing to verify but, initially the results are positive :)


> +       lpss->dma_maxburst = 8;

Are these dwords ? If those are bytes then the maxburst value looks
small. In the BSP the max burst is 32 bytes.

---
bod

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Andy Shevchenko]

On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:
>
> Preface. I tried this on Galileo and it appears to work. I'll do some
> throughput testing to verify but, initially the results are positive :)

I submitted (and pushed into my branch) a bit changed version (see my v2).

>> +       lpss->dma_maxburst = 8;
>
> Are these dwords ? If those are bytes then the maxburst value looks
> small. In the BSP the max burst is 32 bytes.

max_burst is in items of given size (here is 32 bytes for memory and 8
bytes for UART). I took this value from Quark BSP, but I'll be happy
to adjust to more optimal values.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Andy Shevchenko]

On Mon, Apr 11, 2016 at 6:51 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
>> On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:
>>
>> Preface. I tried this on Galileo and it appears to work. I'll do some
>> throughput testing to verify but, initially the results are positive :)
>
> I submitted (and pushed into my branch) a bit changed version (see my v2).
>
>>> +       lpss->dma_maxburst = 8;
>>
>> Are these dwords ? If those are bytes then the maxburst value looks
>> small. In the BSP the max burst is 32 bytes.
>
> max_burst is in items of given size (here is 32 bytes for memory and 8
> bytes for UART). I took this value from Quark BSP, but I'll be happy
> to adjust to more optimal values.

+Jarkko.

Oy vei, seems we don't set memory side of transfer neither in
8250_dma, nor in spi-pxa2xx-dma.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Bryan O'Donoghue]

On Mon, 2016-04-11 at 18:51 +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
> > On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:
> > 
> > Preface. I tried this on Galileo and it appears to work. I'll do
> > some
> > throughput testing to verify but, initially the results are
> > positive :)
> 
> I submitted (and pushed into my branch) a bit changed version (see my
> v2).
> 
> > > +       lpss->dma_maxburst = 8;
> > 
> > Are these dwords ? If those are bytes then the maxburst value looks
> > small. In the BSP the max burst is 32 bytes.
> 
> max_burst is in items of given size (here is 32 bytes for memory and 
> 8

I haven't read your V2 yet but on this, I'd suggest raising the burst
size to 32 bytes for UART (no higher) we found during bringup that
larger sizes "fall-over and die" but, anything up to 32 bytes is OK -
and therefore you should be able to reduce the number of
bursts/interrupts etc.

---
bod

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Andy Shevchenko]

On Tue, 2016-04-12 at 17:25 +0100, Bryan O'Donoghue wrote:
> On Mon, 2016-04-11 at 18:51 +0300, Andy Shevchenko wrote:
> > 
> > On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue
> > <pure.logic@nexus-software.ie> wrote:
> > > 
> > > On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:
> > > 
> > > Preface. I tried this on Galileo and it appears to work. I'll do
> > > some
> > > throughput testing to verify but, initially the results are
> > > positive :)
> > I submitted (and pushed into my branch) a bit changed version (see
> > my
> > v2).
> > 
> > > 
> > > > 
> > > > +       lpss->dma_maxburst = 8;
> > > Are these dwords ? If those are bytes then the maxburst value
> > > looks
> > > small. In the BSP the max burst is 32 bytes.
> > max_burst is in items of given size (here is 32 bytes for memory
> > and 
> > 8
> I haven't read your V2 yet but on this, I'd suggest raising the burst
> size to 32 bytes for UART (no higher) we found during bringup that
> larger sizes "fall-over and die" but, anything up to 32 bytes is OK -
> and therefore you should be able to reduce the number of
> bursts/interrupts etc.

It can't be more that FIFO size and recommendation as far as I know is
FIFO/2, which is exactly 8 bytes.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Bryan O'Donoghue]

On Tue, 2016-04-12 at 19:50 +0300, Andy Shevchenko wrote:
> > I haven't read your V2 yet but on this, I'd suggest raising the
> burst
> > size to 32 bytes for UART (no higher) we found during bringup that
> > larger sizes "fall-over and die" but, anything up to 32 bytes is OK
> -
> > and therefore you should be able to reduce the number of
> > bursts/interrupts etc.
> 
> It can't be more that FIFO size and recommendation as far as I know
> is
> FIFO/2, which is exactly 8 bytes.

Why not ?

We went as high as 32 bytes previously in the BSP with no obvious
errors.

At 8 bytes or 1/2 of the FIFO size I'd ask the question is DMA even
worth it i.e. does it take more time to setup and execute a DMA
transaction @ 1/2 FIFO size than just writing straight into the FIFO ?

---
bod

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Andy Shevchenko]

On Wed, 2016-04-13 at 12:22 +0100, Bryan O'Donoghue wrote:
> On Tue, 2016-04-12 at 19:50 +0300, Andy Shevchenko wrote:
> > 
> > > 
> > > I haven't read your V2 yet but on this, I'd suggest raising the
> > burst
> > > 
> > > size to 32 bytes for UART (no higher) we found during bringup that
> > > larger sizes "fall-over and die" but, anything up to 32 bytes is
> > > OK
> > -
> > > 
> > > and therefore you should be able to reduce the number of
> > > bursts/interrupts etc.
> > It can't be more that FIFO size and recommendation as far as I know
> > is
> > FIFO/2, which is exactly 8 bytes.
> Why not ?

Because a probability of FIFO overrun.

There is a big chapter ("Peripheral Burst Transaction Requests") in
dw_apb_dmac_db.pdf covering this.

> 
> We went as high as 32 bytes previously in the BSP with no obvious
> errors.
> 
> At 8 bytes or 1/2 of the FIFO size I'd ask the question is DMA even
> worth it i.e. does it take more time to setup and execute a DMA
> transaction @ 1/2 FIFO size than just writing straight into the FIFO ?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Bryan O'Donoghue]

On Wed, 2016-04-13 at 15:03 +0300, Andy Shevchenko wrote:
> Because a probability of FIFO overrun.
> 
> There is a big chapter ("Peripheral Burst Transaction Requests") in
> dw_apb_dmac_db.pdf covering this.

I thought there was flow control between the controller and the FIFO
here ? I don't have the spec SoC spec for the UART to hand but, if
memory serves...

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Andy Shevchenko]

On Wed, 2016-04-13 at 15:34 +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-04-13 at 15:03 +0300, Andy Shevchenko wrote:
> > 
> > Because a probability of FIFO overrun.
> > 
> > There is a big chapter ("Peripheral Burst Transaction Requests") in
> > dw_apb_dmac_db.pdf covering this.
> I thought there was flow control between the controller and the FIFO
> here ? I don't have the spec SoC spec for the UART to hand but, if
> memory serves...

Wait, you mean flow control between DMA controller and UART FIFO, or I
misread you?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 12/12] serial: 8250_lpss: enable DMA on Intel Quark UART

[Bryan O'Donoghue]

On Wed, 2016-04-13 at 17:48 +0300, Andy Shevchenko wrote:
> Wait, you mean flow control between DMA controller and UART FIFO, or
> I
> misread you?

Yup.

It's a while since I read the spec and talked to the relevant people
but... I have this memory that the FIFO fill signal and DMA block were
'wired up' @ the AHB level. That would be how the UART and DMA block
would flow-control each other for descriptor chaining at any rate and
so one assumes that its active at the block-to-fifo layer.

Meh I don't have the UART EAS anymore to comment in detail.. 

I think the right thing to do is to be safe (so I'll ACK your series)
and then run an experiment to push the burst size upwards. If you have
the EAS handy though it might be worthwhile working out when the DMA
block will flow-control w/r to the FIFO fill level - I *think* (but
can't prove since I don't have the EAS anymore) that it's safe to push
that value higher.

---
bod

Re: [PATCH v1 00/12] serial: 8250: split LPSS to 8250_lpss, enable DMA on Quark

[Bryan O'Donoghue]

On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:
> This is combined series of two things:
>  - split out the Intel LPSS specific driver from 8250_pci into
> 8250_lpss
>  - enable DMA support on Intel Quark UART
> 
> The patch has been tested on few Intel SoCs / platforms.
> 
> This is targeting serial subsystem, thus it would be nice to get and
> Ack from Vinod first.
> Moreover, the series depends on series [1] that is now under review.
> 
> That's why I ask Vinod to create immutable tag / branch for the [1]
> and the
> dependants (at least one more, which is sata_dwc_460ex).
> 
> [1] http://www.spinics.net/lists/dmaengine/msg08709.html

Andy.

Which tree/branch exactly are you applying this against ? I'd like to
test it out on Galileo.

Could you give a link ?

---
bod

Re: [PATCH v1 00/12] serial: 8250: split LPSS to 8250_lpss, enable DMA on Quark

[Andy Shevchenko]

On Sat, Apr 9, 2016 at 7:53 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:
>> This is combined series of two things:
>>  - split out the Intel LPSS specific driver from 8250_pci into
>> 8250_lpss
>>  - enable DMA support on Intel Quark UART
>>
>> The patch has been tested on few Intel SoCs / platforms.
>>
>> This is targeting serial subsystem, thus it would be nice to get and
>> Ack from Vinod first.
>> Moreover, the series depends on series [1] that is now under review.
>>
>> That's why I ask Vinod to create immutable tag / branch for the [1]
>> and the
>> dependants (at least one more, which is sata_dwc_460ex).
>>
>> [1] http://www.spinics.net/lists/dmaengine/msg08709.html
>
> Andy.
>
> Which tree/branch exactly are you applying this against ? I'd like to
> test it out on Galileo.
>

It's based on linux-next. it would be nice to have your tag / comments!

> Could you give a link ?
>

You may try my branch
https://bitbucket.org/andy-shev/linux/branch/topic%2Fdw%2Fqrk

> ---
> bod
>



-- 
With Best Regards,
Andy Shevchenko