[PATCH v9 12/16] serial: amba-pl011: Pass FIQ information to KGDB.

[PATCH v9 12/16] serial: amba-pl011: Pass FIQ information to KGDB.

[Daniel Thompson]

Speculatively register a FIQ resource with KGDB. KGDB will only
accept it if the kgdb/fiq feature is enabled (both with compile time and
runtime switches) and the interrupt controller supports FIQ.

By providing this information to KGDB the serial driver offers
"permission" for KGDB to route the UART interrupt signal from the
drivers own handler to KGDBs FIQ handler (which will eventually use the
UART's polled I/O callbacks to interact with the user). This permission
also implies the amba-pl011 driver has already unmasked RX interrupts
(otherwise the FIQ handler will never trigger).

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/amba-pl011.c | 94 ++++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8572f2a..ec8ddc7 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -58,6 +58,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/sizes.h>
 #include <linux/io.h>
+#include <linux/kgdb.h>
 
 #define UART_NR			14
 
@@ -1416,8 +1417,61 @@ static void pl011_break_ctl(struct uart_port *port, int break_state)
 	spin_unlock_irqrestore(&uap->port.lock, flags);
 }
 
+static int pl011_hwinit(struct uart_port *port)
+{
+	struct uart_amba_port *uap = (struct uart_amba_port *)port;
+	int retval;
+
+	/* Optionaly enable pins to be muxed in and configured */
+	pinctrl_pm_select_default_state(port->dev);
+
+	/*
+	 * Try to enable the clock producer.
+	 */
+	retval = clk_prepare_enable(uap->clk);
+	if (retval)
+		return retval;
+
+	uap->port.uartclk = clk_get_rate(uap->clk);
+
+	/* Clear pending error and receive interrupts */
+	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+
+	/*
+	 * Save interrupts enable mask, and enable RX interrupts in case if
+	 * the interrupt is used for NMI entry.
+	 */
+	uap->im = readw(uap->port.membase + UART011_IMSC);
+	writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
+
+	if (dev_get_platdata(uap->port.dev)) {
+		struct amba_pl011_data *plat;
+
+		plat = dev_get_platdata(uap->port.dev);
+		if (plat->init)
+			plat->init();
+	}
+	return 0;
+}
+
 #ifdef CONFIG_CONSOLE_POLL
 
+static int pl011_poll_init(struct uart_port *port)
+{
+	struct uart_amba_port *uap = (struct uart_amba_port *)port;
+	int retval;
+
+	retval = pl011_hwinit(port);
+
+#ifdef CONFIG_KGDB_FIQ
+	if (retval == 0)
+		kgdb_register_fiq(uap->port.irq);
+#endif
+
+	return retval;
+}
+
 static void pl011_quiesce_irqs(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
@@ -1471,44 +1525,6 @@ static void pl011_put_poll_char(struct uart_port *port,
 
 #endif /* CONFIG_CONSOLE_POLL */
 
-static int pl011_hwinit(struct uart_port *port)
-{
-	struct uart_amba_port *uap = (struct uart_amba_port *)port;
-	int retval;
-
-	/* Optionaly enable pins to be muxed in and configured */
-	pinctrl_pm_select_default_state(port->dev);
-
-	/*
-	 * Try to enable the clock producer.
-	 */
-	retval = clk_prepare_enable(uap->clk);
-	if (retval)
-		return retval;
-
-	uap->port.uartclk = clk_get_rate(uap->clk);
-
-	/* Clear pending error and receive interrupts */
-	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
-	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
-
-	/*
-	 * Save interrupts enable mask, and enable RX interrupts in case if
-	 * the interrupt is used for NMI entry.
-	 */
-	uap->im = readw(uap->port.membase + UART011_IMSC);
-	writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
-
-	if (dev_get_platdata(uap->port.dev)) {
-		struct amba_pl011_data *plat;
-
-		plat = dev_get_platdata(uap->port.dev);
-		if (plat->init)
-			plat->init();
-	}
-	return 0;
-}
-
 static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
 {
 	writew(lcr_h, uap->port.membase + uap->lcrh_rx);
@@ -1888,7 +1904,7 @@ static struct uart_ops amba_pl011_pops = {
 	.config_port	= pl011_config_port,
 	.verify_port	= pl011_verify_port,
 #ifdef CONFIG_CONSOLE_POLL
-	.poll_init     = pl011_hwinit,
+	.poll_init     = pl011_poll_init,
 	.poll_get_char = pl011_get_poll_char,
 	.poll_put_char = pl011_put_poll_char,
 #endif
-- 
1.9.3

Re: [PATCH v9 12/16] serial: amba-pl011: Pass FIQ information to KGDB.

[Peter Hurley]

Hi Daniel,

On 08/18/2014 10:28 AM, Daniel Thompson wrote:
> Speculatively register a FIQ resource with KGDB. KGDB will only
> accept it if the kgdb/fiq feature is enabled (both with compile time and
> runtime switches) and the interrupt controller supports FIQ.
> 
> By providing this information to KGDB the serial driver offers
> "permission" for KGDB to route the UART interrupt signal from the
> drivers own handler to KGDBs FIQ handler (which will eventually use the
> UART's polled I/O callbacks to interact with the user). This permission
> also implies the amba-pl011 driver has already unmasked RX interrupts
> (otherwise the FIQ handler will never trigger).
> 
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/amba-pl011.c | 94 ++++++++++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8572f2a..ec8ddc7 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -58,6 +58,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/sizes.h>
>  #include <linux/io.h>
> +#include <linux/kgdb.h>
>  
>  #define UART_NR			14
>  
> @@ -1416,8 +1417,61 @@ static void pl011_break_ctl(struct uart_port *port, int break_state)
>  	spin_unlock_irqrestore(&uap->port.lock, flags);
>  }
>  

Is pl011_hwinit() just being relocated in source to avoid the forward
declaration? If so, this is usually split into its own commit.

> +static int pl011_hwinit(struct uart_port *port)
> +{
> +	struct uart_amba_port *uap = (struct uart_amba_port *)port;
> +	int retval;
> +
> +	/* Optionaly enable pins to be muxed in and configured */
> +	pinctrl_pm_select_default_state(port->dev);
> +
> +	/*
> +	 * Try to enable the clock producer.
> +	 */
> +	retval = clk_prepare_enable(uap->clk);
> +	if (retval)
> +		return retval;
> +
> +	uap->port.uartclk = clk_get_rate(uap->clk);
> +
> +	/* Clear pending error and receive interrupts */
> +	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
> +	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
> +
> +	/*
> +	 * Save interrupts enable mask, and enable RX interrupts in case if
> +	 * the interrupt is used for NMI entry.
> +	 */
> +	uap->im = readw(uap->port.membase + UART011_IMSC);
> +	writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
> +
> +	if (dev_get_platdata(uap->port.dev)) {
> +		struct amba_pl011_data *plat;
> +
> +		plat = dev_get_platdata(uap->port.dev);
> +		if (plat->init)
> +			plat->init();
> +	}
> +	return 0;
> +}
> +
>  #ifdef CONFIG_CONSOLE_POLL
>  
> +static int pl011_poll_init(struct uart_port *port)
> +{
> +	struct uart_amba_port *uap = (struct uart_amba_port *)port;

Please use container_of() in new code.

> +	int retval;
> +
> +	retval = pl011_hwinit(port);
> +
> +#ifdef CONFIG_KGDB_FIQ
> +	if (retval == 0)
> +		kgdb_register_fiq(uap->port.irq);

The uap->port dereference is unnecessary since the port parameter
is the same thing.

		kgdb_register_fiq(port->irq);

Regards,
Peter Hurley

> +#endif
> +
> +	return retval;
> +}
> +
>  static void pl011_quiesce_irqs(struct uart_port *port)
>  {
>  	struct uart_amba_port *uap = (struct uart_amba_port *)port;
> @@ -1471,44 +1525,6 @@ static void pl011_put_poll_char(struct uart_port *port,
>  
>  #endif /* CONFIG_CONSOLE_POLL */
>  
> -static int pl011_hwinit(struct uart_port *port)
> -{
> -	struct uart_amba_port *uap = (struct uart_amba_port *)port;
> -	int retval;
> -
> -	/* Optionaly enable pins to be muxed in and configured */
> -	pinctrl_pm_select_default_state(port->dev);
> -
> -	/*
> -	 * Try to enable the clock producer.
> -	 */
> -	retval = clk_prepare_enable(uap->clk);
> -	if (retval)
> -		return retval;
> -
> -	uap->port.uartclk = clk_get_rate(uap->clk);
> -
> -	/* Clear pending error and receive interrupts */
> -	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
> -	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
> -
> -	/*
> -	 * Save interrupts enable mask, and enable RX interrupts in case if
> -	 * the interrupt is used for NMI entry.
> -	 */
> -	uap->im = readw(uap->port.membase + UART011_IMSC);
> -	writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
> -
> -	if (dev_get_platdata(uap->port.dev)) {
> -		struct amba_pl011_data *plat;
> -
> -		plat = dev_get_platdata(uap->port.dev);
> -		if (plat->init)
> -			plat->init();
> -	}
> -	return 0;
> -}
> -
>  static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
>  {
>  	writew(lcr_h, uap->port.membase + uap->lcrh_rx);
> @@ -1888,7 +1904,7 @@ static struct uart_ops amba_pl011_pops = {
>  	.config_port	= pl011_config_port,
>  	.verify_port	= pl011_verify_port,
>  #ifdef CONFIG_CONSOLE_POLL
> -	.poll_init     = pl011_hwinit,
> +	.poll_init     = pl011_poll_init,
>  	.poll_get_char = pl011_get_poll_char,
>  	.poll_put_char = pl011_put_poll_char,
>  #endif
> 

Re: [PATCH v9 12/16] serial: amba-pl011: Pass FIQ information to KGDB.

[Daniel Thompson]

On 18/08/14 19:30, Peter Hurley wrote:
> Hi Daniel,
> 
> On 08/18/2014 10:28 AM, Daniel Thompson wrote:
>> Speculatively register a FIQ resource with KGDB. KGDB will only
>> accept it if the kgdb/fiq feature is enabled (both with compile time and
>> runtime switches) and the interrupt controller supports FIQ.
>>
>> By providing this information to KGDB the serial driver offers
>> "permission" for KGDB to route the UART interrupt signal from the
>> drivers own handler to KGDBs FIQ handler (which will eventually use the
>> UART's polled I/O callbacks to interact with the user). This permission
>> also implies the amba-pl011 driver has already unmasked RX interrupts
>> (otherwise the FIQ handler will never trigger).
>>
>> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jslaby@suse.cz>
>> Cc: linux-serial@vger.kernel.org
>> ---
>>  drivers/tty/serial/amba-pl011.c | 94 ++++++++++++++++++++++++-----------------
>>  1 file changed, 55 insertions(+), 39 deletions(-)#

Thanks for the review.


>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 8572f2a..ec8ddc7 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -58,6 +58,7 @@
>>  #include <linux/pinctrl/consumer.h>
>>  #include <linux/sizes.h>
>>  #include <linux/io.h>
>> +#include <linux/kgdb.h>
>>  
>>  #define UART_NR			14
>>  
>> @@ -1416,8 +1417,61 @@ static void pl011_break_ctl(struct uart_port *port, int break_state)
>>  	spin_unlock_irqrestore(&uap->port.lock, flags);
>>  }
>>  
> 
> Is pl011_hwinit() just being relocated in source to avoid the forward
> declaration? If so, this is usually split into its own commit.

Ok. I'll do this.


>> +static int pl011_hwinit(struct uart_port *port)
>> +{
>> +	struct uart_amba_port *uap = (struct uart_amba_port *)port;
>> +	int retval;
>> +
>> +	/* Optionaly enable pins to be muxed in and configured */
>> +	pinctrl_pm_select_default_state(port->dev);
>> +
>> +	/*
>> +	 * Try to enable the clock producer.
>> +	 */
>> +	retval = clk_prepare_enable(uap->clk);
>> +	if (retval)
>> +		return retval;
>> +
>> +	uap->port.uartclk = clk_get_rate(uap->clk);
>> +
>> +	/* Clear pending error and receive interrupts */
>> +	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
>> +	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
>> +
>> +	/*
>> +	 * Save interrupts enable mask, and enable RX interrupts in case if
>> +	 * the interrupt is used for NMI entry.
>> +	 */
>> +	uap->im = readw(uap->port.membase + UART011_IMSC);
>> +	writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
>> +
>> +	if (dev_get_platdata(uap->port.dev)) {
>> +		struct amba_pl011_data *plat;
>> +
>> +		plat = dev_get_platdata(uap->port.dev);
>> +		if (plat->init)
>> +			plat->init();
>> +	}
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_CONSOLE_POLL
>>  
>> +static int pl011_poll_init(struct uart_port *port)
>> +{
>> +	struct uart_amba_port *uap = (struct uart_amba_port *)port;
> 
> Please use container_of() in new code.

Ok.

Personally I dislike a file that mixes casts and conatiner_of but I
guess I can make both of us happy by switching the whole driver to
container_of. Separate patch again?


>> +	int retval;
>> +
>> +	retval = pl011_hwinit(port);
>> +
>> +#ifdef CONFIG_KGDB_FIQ
>> +	if (retval == 0)
>> +		kgdb_register_fiq(uap->port.irq);
> 
> The uap->port dereference is unnecessary since the port parameter
> is the same thing.
> 
> 		kgdb_register_fiq(port->irq);

Ok.

Re: [PATCH v9 12/16] serial: amba-pl011: Pass FIQ information to KGDB.

[Peter Hurley]

On 08/19/2014 05:08 AM, Daniel Thompson wrote:
> On 18/08/14 19:30, Peter Hurley wrote:

[cut]
  
>>> +static int pl011_poll_init(struct uart_port *port)
>>> +{
>>> +	struct uart_amba_port *uap = (struct uart_amba_port *)port;
>>
>> Please use container_of() in new code.
> 
> Ok.
> 
> Personally I dislike a file that mixes casts and conatiner_of but I
> guess I can make both of us happy by switching the whole driver to
> container_of. Separate patch again?

The change below makes the uap local unnecessary, so you can skip the
container_of() change, if you'd prefer.

>>> +	int retval;
>>> +
>>> +	retval = pl011_hwinit(port);
>>> +
>>> +#ifdef CONFIG_KGDB_FIQ
>>> +	if (retval == 0)
>>> +		kgdb_register_fiq(uap->port.irq);
>>
>> The uap->port dereference is unnecessary since the port parameter
>> is the same thing.
>>
>> 		kgdb_register_fiq(port->irq);
> 
> Ok.

Thanks,
Peter Hurley

Re: [PATCH v9 12/16] serial: amba-pl011: Pass FIQ information to KGDB.

[Daniel Thompson]

On 19/08/14 12:58, Peter Hurley wrote:
> On 08/19/2014 05:08 AM, Daniel Thompson wrote:
>> On 18/08/14 19:30, Peter Hurley wrote:
> 
> [cut]
>   
>>>> +static int pl011_poll_init(struct uart_port *port)
>>>> +{
>>>> +	struct uart_amba_port *uap = (struct uart_amba_port *)port;
>>>
>>> Please use container_of() in new code.
>>
>> Ok.
>>
>> Personally I dislike a file that mixes casts and conatiner_of but I
>> guess I can make both of us happy by switching the whole driver to
>> container_of. Separate patch again?
> 
> The change below makes the uap local unnecessary, so you can skip the
> container_of() change, if you'd prefer.

I realized that myself although not until after I'd replaced all the
casts with container_of()...

So I'll keep the patch for now but can drop it if anyone takes against it.



Thanks

Daniel.




> 
>>>> +	int retval;
>>>> +
>>>> +	retval = pl011_hwinit(port);
>>>> +
>>>> +#ifdef CONFIG_KGDB_FIQ
>>>> +	if (retval == 0)
>>>> +		kgdb_register_fiq(uap->port.irq);
>>>
>>> The uap->port dereference is unnecessary since the port parameter
>>> is the same thing.
>>>
>>> 		kgdb_register_fiq(port->irq);
>>
>> Ok.
> 
> Thanks,
> Peter Hurley
> 

[PATCH v9 13/16] serial: asc: Add support for KGDB's FIQ/NMI mode

[Daniel Thompson]

Add a .poll_init() function that enables UART RX and registers the
UART's irq with KGDB. By providing this information to KGDB the serial
driver offers "permission" for KGDB to route the UART interrupt signal
from the drivers own handler to KGDBs FIQ handler (which will eventually
use the UART's polled I/O callbacks to interact with the user).

Note that the RX is not only enabled but also unmasked. This is required
because otherwise the FIQ handler could never trigger. This unmask is
copied from similar code in amba-pl011.c .

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
Cc: Maxime Coquelin <maxime.coquelin@st.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: kernel@stlinux.com
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/st-asc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index 8b2d735..2b5eb6e 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -30,6 +30,7 @@
 #include <linux/of_platform.h>
 #include <linux/serial_core.h>
 #include <linux/clk.h>
+#include <linux/kgdb.h>
 
 #define DRIVER_NAME "st-asc"
 #define ASC_SERIAL_NAME "ttyAS"
@@ -607,6 +608,25 @@ asc_verify_port(struct uart_port *port, struct serial_struct *ser)
 }
 
 #ifdef CONFIG_CONSOLE_POLL
+
+#ifdef CONFIG_KGDB_FIQ
+/*
+ * Prepare the UART to be used from kgdb's NMI support.
+ */
+static int asc_poll_init(struct uart_port *port)
+{
+	struct asc_port *ascport = container_of(port, struct asc_port, port);
+
+	/* register the FIQ with kgdb */
+	kgdb_register_fiq(ascport->port.irq);
+
+	/* enable RX interrupts in case the interrupt is used for NMI entry. */
+	asc_enable_rx_interrupts(port);
+
+	return 0;
+}
+#endif /* CONFIG_KGDB_FIQ */
+
 /*
  * Console polling routines for writing and reading from the uart while
  * in an interrupt or debug context (i.e. kgdb).
@@ -649,6 +669,9 @@ static struct uart_ops asc_uart_ops = {
 	.verify_port	= asc_verify_port,
 	.pm		= asc_pm,
 #ifdef CONFIG_CONSOLE_POLL
+#ifdef CONFIG_KGDB_FIQ
+	.poll_init     = asc_poll_init,
+#endif /* CONFIG_KGDB_FIQ */
 	.poll_get_char = asc_get_poll_char,
 	.poll_put_char = asc_put_poll_char,
 #endif /* CONFIG_CONSOLE_POLL */
-- 
1.9.3

[PATCH v9 14/16] serial: asc: Adopt readl_/writel_relaxed()

[Daniel Thompson]

The architectures where this peripheral exists (ARM and SH) have expensive
implementations of writel(), reliant on spin locks and explicit L2 cache
management. These architectures provide a cheaper writel_relaxed() which
is much better suited to peripherals that do not perform DMA. The
situation with readl()/readl_relaxed()is similar although less acute.

This driver does not use DMA and will be more power efficient and more
robust (due to absense of spin locks during console I/O) if it uses the
relaxed variants.

This change means the driver is no longer portable and therefore no
longer suitable for compile testing.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
Cc: Maxime Coquelin <maxime.coquelin@st.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: kernel@stlinux.com
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/Kconfig  | 2 +-
 drivers/tty/serial/st-asc.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 26cec64..e9b1735 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1527,7 +1527,7 @@ config SERIAL_FSL_LPUART_CONSOLE
 config SERIAL_ST_ASC
 	tristate "ST ASC serial port support"
 	select SERIAL_CORE
-	depends on ARM || COMPILE_TEST
+	depends on ARM
 	help
 	  This driver is for the on-chip Asychronous Serial Controller on
 	  STMicroelectronics STi SoCs.
diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index 2b5eb6e..df709ee 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -152,12 +152,12 @@ static inline struct asc_port *to_asc_port(struct uart_port *port)
 
 static inline u32 asc_in(struct uart_port *port, u32 offset)
 {
-	return readl(port->membase + offset);
+	return readl_relaxed(port->membase + offset);
 }
 
 static inline void asc_out(struct uart_port *port, u32 offset, u32 value)
 {
-	writel(value, port->membase + offset);
+	writel_relaxed(value, port->membase + offset);
 }
 
 /*
-- 
1.9.3

[PATCH v9 15/16] serial: imx: clean up imx_poll_get_char()

[Daniel Thompson]

From: Dirk Behme <dirk.behme@de.bosch.com>

Looking at the get_poll_char() function of the 8250.c serial driver,
we learn:

* poll_get_char() doesn't have to save/disable/restore the interrupt
  registers. No interrupt handling is needed in this function at all.
  Remove it.

* Don't block in case there is no data available. So instead blocking
  in the do {} while loop, just return with NO_POLL_CHAR, immediately .

Additionally, while the i.MX6 register URXD[7-0] contain the RX_DATA,
the upper bits of this register (URXD[15-10]) might contain some
control flags. To ensure that these are not returned with the data
read, just mask out URXD[7-0].

These changes fix the 'hang' working with kdb:

$ echo ttymxc3 > /sys/module/kgdboc/parameters/kgdboc
$ echo g >/proc/sysrq-trigger
[0]kdb> help
...
<hang>

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/imx.c | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 044e86d..983668a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -80,6 +80,7 @@
 #define URXD_FRMERR	(1<<12)
 #define URXD_BRK	(1<<11)
 #define URXD_PRERR	(1<<10)
+#define URXD_RX_DATA	(0xFF<<0)
 #define UCR1_ADEN	(1<<15) /* Auto detect interrupt */
 #define UCR1_ADBR	(1<<14) /* Auto detect baud rate */
 #define UCR1_TRDYEN	(1<<13) /* Transmitter ready interrupt enable */
@@ -1506,32 +1507,10 @@ imx_verify_port(struct uart_port *port, struct serial_struct *ser)
 #if defined(CONFIG_CONSOLE_POLL)
 static int imx_poll_get_char(struct uart_port *port)
 {
-	struct imx_port_ucrs old_ucr;
-	unsigned int status;
-	unsigned char c;
-
-	/* save control registers */
-	imx_port_ucrs_save(port, &old_ucr);
-
-	/* disable interrupts */
-	writel(UCR1_UARTEN, port->membase + UCR1);
-	writel(old_ucr.ucr2 & ~(UCR2_ATEN | UCR2_RTSEN | UCR2_ESCI),
-	       port->membase + UCR2);
-	writel(old_ucr.ucr3 & ~(UCR3_DCD | UCR3_RI | UCR3_DTREN),
-	       port->membase + UCR3);
-
-	/* poll */
-	do {
-		status = readl(port->membase + USR2);
-	} while (~status & USR2_RDR);
-
-	/* read */
-	c = readl(port->membase + URXD0);
-
-	/* restore control registers */
-	imx_port_ucrs_restore(port, &old_ucr);
+	if (!(readl(port->membase + USR2) & USR2_RDR))
+		return NO_POLL_CHAR;
 
-	return c;
+	return readl(port->membase + URXD0) & URXD_RX_DATA;
 }
 
 static void imx_poll_put_char(struct uart_port *port, unsigned char c)
-- 
1.9.3

[PATCH v9 16/16] serial: imx: Add support for KGDB's FIQ/NMI mode

[Daniel Thompson]

This patch makes it possible to use the imx uart with KGDB's FIQ/NMI
mode.

Main changes are:

.poll_init() will, if KGDB+FIQ are enabled, perform deeper hardware
initialization to ensure the serial port is always active (required
otherwise FIQ is not triggered by UART activity). This has an impact on
power usage so it is conservatively enabled.

imx_put_poll_char() has been simplified to remove the code to disable
interrupts. The present code can corrupt register state when re-entered
from FIQ handler.

Both imx_put_poll_char() and imx_get_poll_char() adopt _relaxed()
MMIO functions (which are safe for polled I/O and needed to avoid taking
spin locks).

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: linux-serial@vger.kernel.org
---
 drivers/tty/serial/imx.c | 71 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 983668a..a201c61 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -49,6 +49,7 @@
 #include <linux/of_device.h>
 #include <linux/io.h>
 #include <linux/dma-mapping.h>
+#include <linux/kgdb.h>
 
 #include <asm/irq.h>
 #include <linux/platform_data/serial-imx.h>
@@ -1505,44 +1506,73 @@ imx_verify_port(struct uart_port *port, struct serial_struct *ser)
 }
 
 #if defined(CONFIG_CONSOLE_POLL)
+
+#if defined(CONFIG_KGDB_FIQ)
+/*
+ * Prepare the UART to be used from kgdb's NMI support.
+ */
+static int imx_poll_init(struct uart_port *port)
+{
+	struct imx_port *sport = (struct imx_port *)port;
+	unsigned long flags;
+	unsigned long temp;
+	int retval;
+
+	retval = clk_prepare_enable(sport->clk_ipg);
+	if (retval)
+		return retval;
+	retval = clk_prepare_enable(sport->clk_per);
+	if (retval)
+		clk_disable_unprepare(sport->clk_ipg);
+
+	imx_setup_ufcr(sport, 0);
+
+	spin_lock_irqsave(&sport->port.lock, flags);
+
+	temp = readl(sport->port.membase + UCR1);
+	if (is_imx1_uart(sport))
+		temp |= IMX1_UCR1_UARTCLKEN;
+	temp |= UCR1_UARTEN | UCR1_RRDYEN;
+	temp &= ~(UCR1_TXMPTYEN | UCR1_RTSDEN);
+	writel(temp, sport->port.membase + UCR1);
+
+	temp = readl(sport->port.membase + UCR2);
+	temp |= UCR2_RXEN;
+	writel(temp, sport->port.membase + UCR2);
+
+	spin_unlock_irqrestore(&sport->port.lock, flags);
+
+	/* register the FIQ with kgdb */
+	kgdb_register_fiq(sport->port.irq);
+
+	return 0;
+}
+#endif /* CONFIG_KGDB_FIQ */
+
 static int imx_poll_get_char(struct uart_port *port)
 {
-	if (!(readl(port->membase + USR2) & USR2_RDR))
+	if (!(readl_relaxed(port->membase + USR2) & USR2_RDR))
 		return NO_POLL_CHAR;
 
-	return readl(port->membase + URXD0) & URXD_RX_DATA;
+	return readl_relaxed(port->membase + URXD0) & URXD_RX_DATA;
 }
 
 static void imx_poll_put_char(struct uart_port *port, unsigned char c)
 {
-	struct imx_port_ucrs old_ucr;
 	unsigned int status;
 
-	/* save control registers */
-	imx_port_ucrs_save(port, &old_ucr);
-
-	/* disable interrupts */
-	writel(UCR1_UARTEN, port->membase + UCR1);
-	writel(old_ucr.ucr2 & ~(UCR2_ATEN | UCR2_RTSEN | UCR2_ESCI),
-	       port->membase + UCR2);
-	writel(old_ucr.ucr3 & ~(UCR3_DCD | UCR3_RI | UCR3_DTREN),
-	       port->membase + UCR3);
-
 	/* drain */
 	do {
-		status = readl(port->membase + USR1);
+		status = readl_relaxed(port->membase + USR1);
 	} while (~status & USR1_TRDY);
 
 	/* write */
-	writel(c, port->membase + URTX0);
+	writel_relaxed(c, port->membase + URTX0);
 
 	/* flush */
 	do {
-		status = readl(port->membase + USR2);
+		status = readl_relaxed(port->membase + USR2);
 	} while (~status & USR2_TXDC);
-
-	/* restore control registers */
-	imx_port_ucrs_restore(port, &old_ucr);
 }
 #endif
 
@@ -1563,6 +1593,9 @@ static struct uart_ops imx_pops = {
 	.config_port	= imx_config_port,
 	.verify_port	= imx_verify_port,
 #if defined(CONFIG_CONSOLE_POLL)
+#if defined(CONFIG_KGDB_FIQ)
+	.poll_init      = imx_poll_init,
+#endif
 	.poll_get_char  = imx_poll_get_char,
 	.poll_put_char  = imx_poll_put_char,
 #endif
-- 
1.9.3

Re: [PATCH v9 16/16] serial: imx: Add support for KGDB's FIQ/NMI mode

[Dirk Behme]

On 18.08.2014 16:28, Daniel Thompson wrote:
> This patch makes it possible to use the imx uart with KGDB's FIQ/NMI
> mode.
>
> Main changes are:
>
> .poll_init() will, if KGDB+FIQ are enabled, perform deeper hardware
> initialization to ensure the serial port is always active (required
> otherwise FIQ is not triggered by UART activity). This has an impact on
> power usage so it is conservatively enabled.
>
> imx_put_poll_char() has been simplified to remove the code to disable
> interrupts. The present code can corrupt register state when re-entered
> from FIQ handler.
>
> Both imx_put_poll_char() and imx_get_poll_char() adopt _relaxed()
> MMIO functions (which are safe for polled I/O and needed to avoid taking
> spin locks).
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: linux-serial@vger.kernel.org

Acked-by: Dirk Behme <dirk.behme@de.bosch.com>

Thanks

Dirk


> ---
>   drivers/tty/serial/imx.c | 71 +++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 983668a..a201c61 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -49,6 +49,7 @@
>   #include <linux/of_device.h>
>   #include <linux/io.h>
>   #include <linux/dma-mapping.h>
> +#include <linux/kgdb.h>
>
>   #include <asm/irq.h>
>   #include <linux/platform_data/serial-imx.h>
> @@ -1505,44 +1506,73 @@ imx_verify_port(struct uart_port *port, struct serial_struct *ser)
>   }
>
>   #if defined(CONFIG_CONSOLE_POLL)
> +
> +#if defined(CONFIG_KGDB_FIQ)
> +/*
> + * Prepare the UART to be used from kgdb's NMI support.
> + */
> +static int imx_poll_init(struct uart_port *port)
> +{
> +	struct imx_port *sport = (struct imx_port *)port;
> +	unsigned long flags;
> +	unsigned long temp;
> +	int retval;
> +
> +	retval = clk_prepare_enable(sport->clk_ipg);
> +	if (retval)
> +		return retval;
> +	retval = clk_prepare_enable(sport->clk_per);
> +	if (retval)
> +		clk_disable_unprepare(sport->clk_ipg);
> +
> +	imx_setup_ufcr(sport, 0);
> +
> +	spin_lock_irqsave(&sport->port.lock, flags);
> +
> +	temp = readl(sport->port.membase + UCR1);
> +	if (is_imx1_uart(sport))
> +		temp |= IMX1_UCR1_UARTCLKEN;
> +	temp |= UCR1_UARTEN | UCR1_RRDYEN;
> +	temp &= ~(UCR1_TXMPTYEN | UCR1_RTSDEN);
> +	writel(temp, sport->port.membase + UCR1);
> +
> +	temp = readl(sport->port.membase + UCR2);
> +	temp |= UCR2_RXEN;
> +	writel(temp, sport->port.membase + UCR2);
> +
> +	spin_unlock_irqrestore(&sport->port.lock, flags);
> +
> +	/* register the FIQ with kgdb */
> +	kgdb_register_fiq(sport->port.irq);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_KGDB_FIQ */
> +
>   static int imx_poll_get_char(struct uart_port *port)
>   {
> -	if (!(readl(port->membase + USR2) & USR2_RDR))
> +	if (!(readl_relaxed(port->membase + USR2) & USR2_RDR))
>   		return NO_POLL_CHAR;
>
> -	return readl(port->membase + URXD0) & URXD_RX_DATA;
> +	return readl_relaxed(port->membase + URXD0) & URXD_RX_DATA;
>   }
>
>   static void imx_poll_put_char(struct uart_port *port, unsigned char c)
>   {
> -	struct imx_port_ucrs old_ucr;
>   	unsigned int status;
>
> -	/* save control registers */
> -	imx_port_ucrs_save(port, &old_ucr);
> -
> -	/* disable interrupts */
> -	writel(UCR1_UARTEN, port->membase + UCR1);
> -	writel(old_ucr.ucr2 & ~(UCR2_ATEN | UCR2_RTSEN | UCR2_ESCI),
> -	       port->membase + UCR2);
> -	writel(old_ucr.ucr3 & ~(UCR3_DCD | UCR3_RI | UCR3_DTREN),
> -	       port->membase + UCR3);
> -
>   	/* drain */
>   	do {
> -		status = readl(port->membase + USR1);
> +		status = readl_relaxed(port->membase + USR1);
>   	} while (~status & USR1_TRDY);
>
>   	/* write */
> -	writel(c, port->membase + URTX0);
> +	writel_relaxed(c, port->membase + URTX0);
>
>   	/* flush */
>   	do {
> -		status = readl(port->membase + USR2);
> +		status = readl_relaxed(port->membase + USR2);
>   	} while (~status & USR2_TXDC);
> -
> -	/* restore control registers */
> -	imx_port_ucrs_restore(port, &old_ucr);
>   }
>   #endif
>
> @@ -1563,6 +1593,9 @@ static struct uart_ops imx_pops = {
>   	.config_port	= imx_config_port,
>   	.verify_port	= imx_verify_port,
>   #if defined(CONFIG_CONSOLE_POLL)
> +#if defined(CONFIG_KGDB_FIQ)
> +	.poll_init      = imx_poll_init,
> +#endif
>   	.poll_get_char  = imx_poll_get_char,
>   	.poll_put_char  = imx_poll_put_char,
>   #endif
>