Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Williams, Dan J]

[ adding Arnd ]

On Mon, Apr 9, 2012 at 12:48 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> The "KT" serial port has another use case for a "received break" quirk,
> so before adding another special case to the 8250 core take this
> opportunity to push such quirks out of the core and into a uart_port op.
>
> Stephen says:
> "If the callback function is to no longer live in 8250.c itself,
>  arch/arm/mach-tegra/devices.c isn't logically a good place to put it,
>  and that file will be going away once we get rid of all the board files
>  and move solely to device tree."
>
> ...so since 8250_pci.c houses all the quirks for pci serial devices this
> quirk is similarly housed in of_serial.c.  Once the open firmware
> conversion completes the infrastructure details (CONFIG_TEGRA_SERIAL,
> include/linux/of_serial.h, and the export) can all be removed to make
> this self contained to of_serial.c.
>
> Cc: Nhan H Mai <nhan.h.mai@intel.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Arnd Bergmann arnd@arndb.de

Sorry, messed up Arnd's address, checkpatch would have caught this for me...

> Acked-by: Sudhakar Mamillapalli <sudhakar@fb.com>
> Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Acked-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  arch/arm/configs/tegra_defconfig      |    1 +
>  arch/arm/mach-tegra/board-harmony.c   |    4 ++++
>  arch/arm/mach-tegra/board-paz00.c     |    5 +++++
>  arch/arm/mach-tegra/board-seaboard.c  |    4 ++++
>  arch/arm/mach-tegra/board-trimslice.c |    4 ++++
>  arch/arm/mach-tegra/devices.h         |    1 -
>  drivers/tty/serial/8250/8250.c        |   34 +++------------------------------
>  drivers/tty/serial/Kconfig            |    8 ++++++++
>  drivers/tty/serial/of_serial.c        |   25 ++++++++++++++++++++++++
>  include/linux/of_serial.h             |   17 +++++++++++++++++
>  include/linux/serial_8250.h           |    1 +
>  include/linux/serial_core.h           |    5 +++++
>  12 files changed, 77 insertions(+), 32 deletions(-)
>  create mode 100644 include/linux/of_serial.h
>
> diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
> index 351d670..de7288a 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -97,6 +97,7 @@ CONFIG_INPUT_EVDEV=y
>  CONFIG_SERIAL_8250=y
>  CONFIG_SERIAL_8250_CONSOLE=y
>  CONFIG_SERIAL_OF_PLATFORM=y
> +CONFIG_SERIAL_TEGRA=y
>  # CONFIG_HW_RANDOM is not set
>  CONFIG_I2C=y
>  # CONFIG_I2C_COMPAT is not set
> diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
> index c00aadb..b8ceac3 100644
> --- a/arch/arm/mach-tegra/board-harmony.c
> +++ b/arch/arm/mach-tegra/board-harmony.c
> @@ -19,6 +19,7 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/serial_8250.h>
> +#include <linux/of_serial.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/pda_power.h>
> @@ -52,6 +53,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
>                .irq            = INT_UARTD,
>                .flags          = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
>                .type           = PORT_TEGRA,
> +               .handle_break   = tegra_serial_handle_break,
>                .iotype         = UPIO_MEM,
>                .regshift       = 2,
>                .uartclk        = 216000000,
> @@ -115,7 +117,9 @@ static void __init harmony_i2c_init(void)
>  }
>
>  static struct platform_device *harmony_devices[] __initdata = {
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
>        &debug_uart,
> +#endif
>        &tegra_sdhci_device1,
>        &tegra_sdhci_device2,
>        &tegra_sdhci_device4,
> diff --git a/arch/arm/mach-tegra/board-paz00.c b/arch/arm/mach-tegra/board-paz00.c
> index 330afdf..1113dab 100644
> --- a/arch/arm/mach-tegra/board-paz00.c
> +++ b/arch/arm/mach-tegra/board-paz00.c
> @@ -21,6 +21,7 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/serial_8250.h>
> +#include <linux/of_serial.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/gpio_keys.h>
> @@ -55,6 +56,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
>                .irq            = INT_UARTA,
>                .flags          = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
>                .type           = PORT_TEGRA,
> +               .handle_break   = tegra_serial_handle_break,
>                .iotype         = UPIO_MEM,
>                .regshift       = 2,
>                .uartclk        = 216000000,
> @@ -65,6 +67,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
>                .irq            = INT_UARTC,
>                .flags          = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
>                .type           = PORT_TEGRA,
> +               .handle_break   = tegra_serial_handle_break,
>                .iotype         = UPIO_MEM,
>                .regshift       = 2,
>                .uartclk        = 216000000,
> @@ -142,7 +145,9 @@ static struct platform_device gpio_keys_device = {
>  };
>
>  static struct platform_device *paz00_devices[] __initdata = {
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
>        &debug_uart,
> +#endif
>        &tegra_sdhci_device4,
>        &tegra_sdhci_device1,
>        &wifi_rfkill_device,
> diff --git a/arch/arm/mach-tegra/board-seaboard.c b/arch/arm/mach-tegra/board-seaboard.c
> index d669847..59a30ab 100644
> --- a/arch/arm/mach-tegra/board-seaboard.c
> +++ b/arch/arm/mach-tegra/board-seaboard.c
> @@ -18,6 +18,7 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/serial_8250.h>
> +#include <linux/of_serial.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/input.h>
> @@ -47,6 +48,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
>                /* Memory and IRQ filled in before registration */
>                .flags          = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
>                .type           = PORT_TEGRA,
> +               .handle_break   = tegra_serial_handle_break,
>                .iotype         = UPIO_MEM,
>                .regshift       = 2,
>                .uartclk        = 216000000,
> @@ -145,7 +147,9 @@ static struct platform_device seaboard_audio_device = {
>  };
>
>  static struct platform_device *seaboard_devices[] __initdata = {
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
>        &debug_uart,
> +#endif
>        &tegra_pmu_device,
>        &tegra_sdhci_device4,
>        &tegra_sdhci_device3,
> diff --git a/arch/arm/mach-tegra/board-trimslice.c b/arch/arm/mach-tegra/board-trimslice.c
> index cd52820..b156f55 100644
> --- a/arch/arm/mach-tegra/board-trimslice.c
> +++ b/arch/arm/mach-tegra/board-trimslice.c
> @@ -22,6 +22,7 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/serial_8250.h>
> +#include <linux/of_serial.h>
>  #include <linux/io.h>
>  #include <linux/i2c.h>
>  #include <linux/gpio.h>
> @@ -48,6 +49,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
>                .irq            = INT_UARTA,
>                .flags          = UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
>                .type           = PORT_TEGRA,
> +               .handle_break   = tegra_serial_handle_break,
>                .iotype         = UPIO_MEM,
>                .regshift       = 2,
>                .uartclk        = 216000000,
> @@ -81,7 +83,9 @@ static struct platform_device trimslice_audio_device = {
>  };
>
>  static struct platform_device *trimslice_devices[] __initdata = {
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
>        &debug_uart,
> +#endif
>        &tegra_sdhci_device1,
>        &tegra_sdhci_device4,
>        &tegra_i2s_device1,
> diff --git a/arch/arm/mach-tegra/devices.h b/arch/arm/mach-tegra/devices.h
> index ec45567..6e5f852 100644
> --- a/arch/arm/mach-tegra/devices.h
> +++ b/arch/arm/mach-tegra/devices.h
> @@ -53,5 +53,4 @@ extern struct platform_device tegra_i2s_device1;
>  extern struct platform_device tegra_i2s_device2;
>  extern struct platform_device tegra_das_device;
>  extern struct platform_device tegra_pcm_device;
> -
>  #endif
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 5c27f7e..cbd94c3 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -1332,27 +1332,6 @@ static void serial8250_enable_ms(struct uart_port *port)
>  }
>
>  /*
> - * Clear the Tegra rx fifo after a break
> - *
> - * FIXME: This needs to become a port specific callback once we have a
> - * framework for this
> - */
> -static void clear_rx_fifo(struct uart_8250_port *up)
> -{
> -       unsigned int status, tmout = 10000;
> -       do {
> -               status = serial_in(up, UART_LSR);
> -               if (status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS))
> -                       status = serial_in(up, UART_RX);
> -               else
> -                       break;
> -               if (--tmout == 0)
> -                       break;
> -               udelay(1);
> -       } while (1);
> -}
> -
> -/*
>  * serial8250_rx_chars: processes according to the passed in LSR
>  * value, and returns the remaining LSR bits not handled
>  * by this Rx routine.
> @@ -1386,20 +1365,10 @@ serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
>                up->lsr_saved_flags = 0;
>
>                if (unlikely(lsr & UART_LSR_BRK_ERROR_BITS)) {
> -                       /*
> -                        * For statistics only
> -                        */
>                        if (lsr & UART_LSR_BI) {
>                                lsr &= ~(UART_LSR_FE | UART_LSR_PE);
>                                port->icount.brk++;
>                                /*
> -                                * If tegra port then clear the rx fifo to
> -                                * accept another break/character.
> -                                */
> -                               if (port->type == PORT_TEGRA)
> -                                       clear_rx_fifo(up);
> -
> -                               /*
>                                 * We do the SysRQ and SAK checking
>                                 * here because otherwise the break
>                                 * may get masked by ignore_status_mask
> @@ -3037,6 +3006,7 @@ static int __devinit serial8250_probe(struct platform_device *dev)
>                port.serial_in          = p->serial_in;
>                port.serial_out         = p->serial_out;
>                port.handle_irq         = p->handle_irq;
> +               port.handle_break       = p->handle_break;
>                port.set_termios        = p->set_termios;
>                port.pm                 = p->pm;
>                port.dev                = &dev->dev;
> @@ -3209,6 +3179,8 @@ int serial8250_register_port(struct uart_port *port)
>                        uart->port.set_termios = port->set_termios;
>                if (port->pm)
>                        uart->port.pm = port->pm;
> +               if (port->handle_break)
> +                       uart->port.handle_break = port->handle_break;
>
>                if (serial8250_isa_config != NULL)
>                        serial8250_isa_config(0, &uart->port,
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 665beb6..33fa6ec 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -380,6 +380,14 @@ config SERIAL_PXA_CONSOLE
>          your boot loader (lilo or loadlin) about how to pass options to the
>          kernel at boot time.)
>
> +# FIXME remove this option when Tegra completes conversion to open firmware
> +config SERIAL_TEGRA
> +       bool "Tegra serial port support"
> +       depends on SERIAL_OF_PLATFORM=y
> +       help
> +         If you have a machine based on NVIDIA Tegra you can enable its
> +         onboard serial ports by enabling this option.
> +
>  config SERIAL_SA1100
>        bool "SA1100 serial port support"
>        depends on ARM && ARCH_SA1100
> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
> index e8c9cee..b692528 100644
> --- a/drivers/tty/serial/of_serial.c
> +++ b/drivers/tty/serial/of_serial.c
> @@ -12,8 +12,10 @@
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/delay.h>
>  #include <linux/serial_core.h>
>  #include <linux/serial_8250.h>
> +#include <linux/serial_reg.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
> @@ -24,6 +26,26 @@ struct of_serial_info {
>        int line;
>  };
>
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
> +void tegra_serial_handle_break(struct uart_port *p)
> +{
> +       unsigned int status, tmout = 10000;
> +
> +       do {
> +               status = p->serial_in(p, UART_LSR);
> +               if (status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS))
> +                       status = p->serial_in(p, UART_RX);
> +               else
> +                       break;
> +               if (--tmout == 0)
> +                       break;
> +               udelay(1);
> +       } while (1);
> +}
> +/* FIXME remove this export when tegra finishes conversion to open firmware */
> +EXPORT_SYMBOL_GPL(tegra_serial_handle_break);
> +#endif
> +
>  /*
>  * Fill a struct uart_port for a given device node
>  */
> @@ -84,6 +106,9 @@ static int __devinit of_platform_serial_setup(struct platform_device *ofdev,
>                | UPF_FIXED_PORT | UPF_FIXED_TYPE;
>        port->dev = &ofdev->dev;
>
> +       if (type == PORT_TEGRA)
> +               port->handle_break = tegra_serial_handle_break;
> +
>        return 0;
>  }
>
> diff --git a/include/linux/of_serial.h b/include/linux/of_serial.h
> new file mode 100644
> index 0000000..2035d96
> --- /dev/null
> +++ b/include/linux/of_serial.h
> @@ -0,0 +1,17 @@
> +#ifndef __LINUX_OF_SERIAL_H
> +#define __LINUX_OF_SERIAL_H
> +
> +/*
> + * FIXME remove this file when tegra finishes conversion to open firmware,
> + * expectation is that all quirks will then be self-contained in
> + * drivers/tty/serial/of_serial.c.
> + */
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
> +extern void tegra_serial_handle_break(struct uart_port *port);
> +#else
> +static inline void tegra_serial_handle_break(struct uart_port *port)
> +{
> +}
> +#endif
> +
> +#endif /* __LINUX_OF_SERIAL */
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 8f012f8..a522fd9 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -38,6 +38,7 @@ struct plat_serial8250_port {
>        int             (*handle_irq)(struct uart_port *);
>        void            (*pm)(struct uart_port *, unsigned int state,
>                              unsigned old);
> +       void            (*handle_break)(struct uart_port *);
>  };
>
>  /*
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 2db407a..65db992 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -310,6 +310,7 @@ struct uart_port {
>        int                     (*handle_irq)(struct uart_port *);
>        void                    (*pm)(struct uart_port *, unsigned int state,
>                                      unsigned int old);
> +       void                    (*handle_break)(struct uart_port *);
>        unsigned int            irq;                    /* irq number */
>        unsigned long           irqflags;               /* irq flags  */
>        unsigned int            uartclk;                /* base uart clock */
> @@ -533,6 +534,10 @@ uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>  static inline int uart_handle_break(struct uart_port *port)
>  {
>        struct uart_state *state = port->state;
> +
> +       if (port->handle_break)
> +               port->handle_break(port);
> +
>  #ifdef SUPPORT_SYSRQ
>        if (port->cons && port->cons->index == port->line) {
>                if (!port->sysrq) {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Stephen Warren]

On 04/09/2012 01:48 PM, Dan Williams wrote:
> The "KT" serial port has another use case for a "received break" quirk,
> so before adding another special case to the 8250 core take this
> opportunity to push such quirks out of the core and into a uart_port op.
> 
> Stephen says:
> "If the callback function is to no longer live in 8250.c itself,
>  arch/arm/mach-tegra/devices.c isn't logically a good place to put it,
>  and that file will be going away once we get rid of all the board files
>  and move solely to device tree."
> 
> ...so since 8250_pci.c houses all the quirks for pci serial devices this
> quirk is similarly housed in of_serial.c.  Once the open firmware
> conversion completes the infrastructure details (CONFIG_TEGRA_SERIAL,
> include/linux/of_serial.h, and the export) can all be removed to make
> this self contained to of_serial.c.

This still seems much to invasive.

Again, what's wrong with simply keeping the quirk implementation where
it is, and driving everything off the already-extant
plat_serial8250_port.type == PORT_TEGRA flag?

I believe all that's required here is to enhance struct struct
serial8250_config (and hence 8250.c's uart_config[] quirk table) to be
able to set the quirk function pointers as well as all the other exiting
quirked variables.

[PATCH v3 0/3] rework quirks for the "kt" serial port

[Dan Williams]

Changes since: v2: http://marc.info/?l=linux-serial&m=133399499209069&w=2

Fixed a brown-paper-bag bug caught by Arnd and dropped "of_serial: add
support for setup quirks" in favor of a simple "if (type == PORT_TEGRA)"
check.

---

Dan Williams (2):
      tegra, serial8250: add ->handle_break() uart_port op
      serial/8250_pci: fix suspend/resume vs init/exit quirks

Sudhakar Mamillapalli (1):
      serial/8250_pci: Clear FIFOs for Intel ME Serial Over Lan device on BI


 arch/arm/configs/tegra_defconfig      |    1 +
 arch/arm/mach-tegra/board-harmony.c   |    4 +++
 arch/arm/mach-tegra/board-paz00.c     |    5 ++++
 arch/arm/mach-tegra/board-seaboard.c  |    4 +++
 arch/arm/mach-tegra/board-trimslice.c |    4 +++
 arch/arm/mach-tegra/devices.h         |    1 -
 drivers/tty/serial/8250/8250.c        |   44 ++++++++++----------------------
 drivers/tty/serial/8250/8250.h        |    2 +
 drivers/tty/serial/8250/8250_pci.c    |   45 +++++++++++++++++++++++++++++++++
 drivers/tty/serial/Kconfig            |    8 ++++++
 drivers/tty/serial/of_serial.c        |   25 ++++++++++++++++++
 include/linux/of_serial.h             |   17 ++++++++++++
 include/linux/serial_8250.h           |    1 +
 include/linux/serial_core.h           |    5 ++++
 14 files changed, 134 insertions(+), 32 deletions(-)
 create mode 100644 include/linux/of_serial.h

[PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Dan Williams]

The "KT" serial port has another use case for a "received break" quirk,
so before adding another special case to the 8250 core take this
opportunity to push such quirks out of the core and into a uart_port op.

Stephen says:
"If the callback function is to no longer live in 8250.c itself,
 arch/arm/mach-tegra/devices.c isn't logically a good place to put it,
 and that file will be going away once we get rid of all the board files
 and move solely to device tree."

...so since 8250_pci.c houses all the quirks for pci serial devices this
quirk is similarly housed in of_serial.c.  Once the open firmware
conversion completes the infrastructure details (CONFIG_TEGRA_SERIAL,
include/linux/of_serial.h, and the export) can all be removed to make
this self contained to of_serial.c.

Cc: Nhan H Mai <nhan.h.mai@intel.com>
Cc: Colin Cross <ccross@android.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Sudhakar Mamillapalli <sudhakar@fb.com>
Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/arm/configs/tegra_defconfig      |    1 +
 arch/arm/mach-tegra/board-harmony.c   |    4 ++++
 arch/arm/mach-tegra/board-paz00.c     |    5 +++++
 arch/arm/mach-tegra/board-seaboard.c  |    4 ++++
 arch/arm/mach-tegra/board-trimslice.c |    4 ++++
 arch/arm/mach-tegra/devices.h         |    1 -
 drivers/tty/serial/8250/8250.c        |   34 +++------------------------------
 drivers/tty/serial/Kconfig            |    8 ++++++++
 drivers/tty/serial/of_serial.c        |   25 ++++++++++++++++++++++++
 include/linux/of_serial.h             |   17 +++++++++++++++++
 include/linux/serial_8250.h           |    1 +
 include/linux/serial_core.h           |    5 +++++
 12 files changed, 77 insertions(+), 32 deletions(-)
 create mode 100644 include/linux/of_serial.h

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 351d670..de7288a 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -97,6 +97,7 @@ CONFIG_INPUT_EVDEV=y
 CONFIG_SERIAL_8250=y
 CONFIG_SERIAL_8250_CONSOLE=y
 CONFIG_SERIAL_OF_PLATFORM=y
+CONFIG_SERIAL_TEGRA=y
 # CONFIG_HW_RANDOM is not set
 CONFIG_I2C=y
 # CONFIG_I2C_COMPAT is not set
diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
index c00aadb..b8ceac3 100644
--- a/arch/arm/mach-tegra/board-harmony.c
+++ b/arch/arm/mach-tegra/board-harmony.c
@@ -19,6 +19,7 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/serial_8250.h>
+#include <linux/of_serial.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/pda_power.h>
@@ -52,6 +53,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
 		.irq		= INT_UARTD,
 		.flags		= UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
 		.type		= PORT_TEGRA,
+		.handle_break	= tegra_serial_handle_break,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= 216000000,
@@ -115,7 +117,9 @@ static void __init harmony_i2c_init(void)
 }
 
 static struct platform_device *harmony_devices[] __initdata = {
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
 	&debug_uart,
+#endif
 	&tegra_sdhci_device1,
 	&tegra_sdhci_device2,
 	&tegra_sdhci_device4,
diff --git a/arch/arm/mach-tegra/board-paz00.c b/arch/arm/mach-tegra/board-paz00.c
index 330afdf..1113dab 100644
--- a/arch/arm/mach-tegra/board-paz00.c
+++ b/arch/arm/mach-tegra/board-paz00.c
@@ -21,6 +21,7 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/serial_8250.h>
+#include <linux/of_serial.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/gpio_keys.h>
@@ -55,6 +56,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
 		.irq		= INT_UARTA,
 		.flags		= UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
 		.type		= PORT_TEGRA,
+		.handle_break	= tegra_serial_handle_break,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= 216000000,
@@ -65,6 +67,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
 		.irq		= INT_UARTC,
 		.flags		= UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
 		.type		= PORT_TEGRA,
+		.handle_break	= tegra_serial_handle_break,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= 216000000,
@@ -142,7 +145,9 @@ static struct platform_device gpio_keys_device = {
 };
 
 static struct platform_device *paz00_devices[] __initdata = {
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
 	&debug_uart,
+#endif
 	&tegra_sdhci_device4,
 	&tegra_sdhci_device1,
 	&wifi_rfkill_device,
diff --git a/arch/arm/mach-tegra/board-seaboard.c b/arch/arm/mach-tegra/board-seaboard.c
index d669847..59a30ab 100644
--- a/arch/arm/mach-tegra/board-seaboard.c
+++ b/arch/arm/mach-tegra/board-seaboard.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/serial_8250.h>
+#include <linux/of_serial.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/input.h>
@@ -47,6 +48,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
 		/* Memory and IRQ filled in before registration */
 		.flags		= UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
 		.type		= PORT_TEGRA,
+		.handle_break	= tegra_serial_handle_break,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= 216000000,
@@ -145,7 +147,9 @@ static struct platform_device seaboard_audio_device = {
 };
 
 static struct platform_device *seaboard_devices[] __initdata = {
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
 	&debug_uart,
+#endif
 	&tegra_pmu_device,
 	&tegra_sdhci_device4,
 	&tegra_sdhci_device3,
diff --git a/arch/arm/mach-tegra/board-trimslice.c b/arch/arm/mach-tegra/board-trimslice.c
index cd52820..b156f55 100644
--- a/arch/arm/mach-tegra/board-trimslice.c
+++ b/arch/arm/mach-tegra/board-trimslice.c
@@ -22,6 +22,7 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/serial_8250.h>
+#include <linux/of_serial.h>
 #include <linux/io.h>
 #include <linux/i2c.h>
 #include <linux/gpio.h>
@@ -48,6 +49,7 @@ static struct plat_serial8250_port debug_uart_platform_data[] = {
 		.irq		= INT_UARTA,
 		.flags		= UPF_BOOT_AUTOCONF | UPF_FIXED_TYPE,
 		.type		= PORT_TEGRA,
+		.handle_break	= tegra_serial_handle_break,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= 216000000,
@@ -81,7 +83,9 @@ static struct platform_device trimslice_audio_device = {
 };
 
 static struct platform_device *trimslice_devices[] __initdata = {
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
 	&debug_uart,
+#endif
 	&tegra_sdhci_device1,
 	&tegra_sdhci_device4,
 	&tegra_i2s_device1,
diff --git a/arch/arm/mach-tegra/devices.h b/arch/arm/mach-tegra/devices.h
index ec45567..6e5f852 100644
--- a/arch/arm/mach-tegra/devices.h
+++ b/arch/arm/mach-tegra/devices.h
@@ -53,5 +53,4 @@ extern struct platform_device tegra_i2s_device1;
 extern struct platform_device tegra_i2s_device2;
 extern struct platform_device tegra_das_device;
 extern struct platform_device tegra_pcm_device;
-
 #endif
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 5c27f7e..cbd94c3 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -1332,27 +1332,6 @@ static void serial8250_enable_ms(struct uart_port *port)
 }
 
 /*
- * Clear the Tegra rx fifo after a break
- *
- * FIXME: This needs to become a port specific callback once we have a
- * framework for this
- */
-static void clear_rx_fifo(struct uart_8250_port *up)
-{
-	unsigned int status, tmout = 10000;
-	do {
-		status = serial_in(up, UART_LSR);
-		if (status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS))
-			status = serial_in(up, UART_RX);
-		else
-			break;
-		if (--tmout == 0)
-			break;
-		udelay(1);
-	} while (1);
-}
-
-/*
  * serial8250_rx_chars: processes according to the passed in LSR
  * value, and returns the remaining LSR bits not handled
  * by this Rx routine.
@@ -1386,20 +1365,10 @@ serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
 		up->lsr_saved_flags = 0;
 
 		if (unlikely(lsr & UART_LSR_BRK_ERROR_BITS)) {
-			/*
-			 * For statistics only
-			 */
 			if (lsr & UART_LSR_BI) {
 				lsr &= ~(UART_LSR_FE | UART_LSR_PE);
 				port->icount.brk++;
 				/*
-				 * If tegra port then clear the rx fifo to
-				 * accept another break/character.
-				 */
-				if (port->type == PORT_TEGRA)
-					clear_rx_fifo(up);
-
-				/*
 				 * We do the SysRQ and SAK checking
 				 * here because otherwise the break
 				 * may get masked by ignore_status_mask
@@ -3037,6 +3006,7 @@ static int __devinit serial8250_probe(struct platform_device *dev)
 		port.serial_in		= p->serial_in;
 		port.serial_out		= p->serial_out;
 		port.handle_irq		= p->handle_irq;
+		port.handle_break	= p->handle_break;
 		port.set_termios	= p->set_termios;
 		port.pm			= p->pm;
 		port.dev		= &dev->dev;
@@ -3209,6 +3179,8 @@ int serial8250_register_port(struct uart_port *port)
 			uart->port.set_termios = port->set_termios;
 		if (port->pm)
 			uart->port.pm = port->pm;
+		if (port->handle_break)
+			uart->port.handle_break = port->handle_break;
 
 		if (serial8250_isa_config != NULL)
 			serial8250_isa_config(0, &uart->port,
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 665beb6..33fa6ec 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -380,6 +380,14 @@ config SERIAL_PXA_CONSOLE
 	  your boot loader (lilo or loadlin) about how to pass options to the
 	  kernel at boot time.)
 
+# FIXME remove this option when Tegra completes conversion to open firmware
+config SERIAL_TEGRA
+	bool "Tegra serial port support"
+	depends on SERIAL_OF_PLATFORM=y
+	help
+	  If you have a machine based on NVIDIA Tegra you can enable its
+	  onboard serial ports by enabling this option.
+
 config SERIAL_SA1100
 	bool "SA1100 serial port support"
 	depends on ARM && ARCH_SA1100
diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index e8c9cee..b692528 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -12,8 +12,10 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
 #include <linux/serial_core.h>
 #include <linux/serial_8250.h>
+#include <linux/serial_reg.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
@@ -24,6 +26,26 @@ struct of_serial_info {
 	int line;
 };
 
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
+void tegra_serial_handle_break(struct uart_port *p)
+{
+	unsigned int status, tmout = 10000;
+
+	do {
+		status = p->serial_in(p, UART_LSR);
+		if (status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS))
+			status = p->serial_in(p, UART_RX);
+		else
+			break;
+		if (--tmout == 0)
+			break;
+		udelay(1);
+	} while (1);
+}
+/* FIXME remove this export when tegra finishes conversion to open firmware */
+EXPORT_SYMBOL_GPL(tegra_serial_handle_break);
+#endif
+
 /*
  * Fill a struct uart_port for a given device node
  */
@@ -84,6 +106,9 @@ static int __devinit of_platform_serial_setup(struct platform_device *ofdev,
 		| UPF_FIXED_PORT | UPF_FIXED_TYPE;
 	port->dev = &ofdev->dev;
 
+	if (type == PORT_TEGRA)
+		port->handle_break = tegra_serial_handle_break;
+
 	return 0;
 }
 
diff --git a/include/linux/of_serial.h b/include/linux/of_serial.h
new file mode 100644
index 0000000..2035d96
--- /dev/null
+++ b/include/linux/of_serial.h
@@ -0,0 +1,17 @@
+#ifndef __LINUX_OF_SERIAL_H
+#define __LINUX_OF_SERIAL_H
+
+/*
+ * FIXME remove this file when tegra finishes conversion to open firmware,
+ * expectation is that all quirks will then be self-contained in
+ * drivers/tty/serial/of_serial.c.
+ */
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
+extern void tegra_serial_handle_break(struct uart_port *port);
+#else
+static inline void tegra_serial_handle_break(struct uart_port *port)
+{
+}
+#endif
+
+#endif /* __LINUX_OF_SERIAL */
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 8f012f8..a522fd9 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -38,6 +38,7 @@ struct plat_serial8250_port {
 	int		(*handle_irq)(struct uart_port *);
 	void		(*pm)(struct uart_port *, unsigned int state,
 			      unsigned old);
+	void		(*handle_break)(struct uart_port *);
 };
 
 /*
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 2db407a..65db992 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -310,6 +310,7 @@ struct uart_port {
 	int			(*handle_irq)(struct uart_port *);
 	void			(*pm)(struct uart_port *, unsigned int state,
 				      unsigned int old);
+	void			(*handle_break)(struct uart_port *);
 	unsigned int		irq;			/* irq number */
 	unsigned long		irqflags;		/* irq flags  */
 	unsigned int		uartclk;		/* base uart clock */
@@ -533,6 +534,10 @@ uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 static inline int uart_handle_break(struct uart_port *port)
 {
 	struct uart_state *state = port->state;
+
+	if (port->handle_break)
+		port->handle_break(port);
+
 #ifdef SUPPORT_SYSRQ
 	if (port->cons && port->cons->index == port->line) {
 		if (!port->sysrq) {

[PATCH v3 2/3] serial/8250_pci: Clear FIFOs for Intel ME Serial Over Lan device on BI

[Dan Williams]

From: Sudhakar Mamillapalli <sudhakar@fb.com>

When using Serial Over Lan (SOL) over the virtual serial port in a Intel
management engine (ME) device, on device reset the serial FIFOs need to
be cleared to keep the FIFO indexes in-sync between the host and the
engine.

On a reset the serial device assertes BI, so using that as a cue FIFOs
are cleared.  So for this purpose a new handle_break callback has been
added.  One other problem is that the serial registers might temporarily
go to 0 on reset of this device.  So instead of using the IER register
read, if 0 returned use the ier value in uart_8250_port. This is hidden
under a custom serial_in.

Cc: Nhan H Mai <nhan.h.mai@intel.com>
Signed-off-by: Sudhakar Mamillapalli <sudhakar@fb.com>
Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/tty/serial/8250/8250.c     |   10 +++++++++
 drivers/tty/serial/8250/8250.h     |    2 ++
 drivers/tty/serial/8250/8250_pci.c |   39 ++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index cbd94c3..182efcc 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -568,6 +568,16 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
 	}
 }
 
+void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
+{
+	unsigned char fcr;
+
+	serial8250_clear_fifos(p);
+	fcr = uart_config[p->port.type].fcr;
+	serial_out(p, UART_FCR, fcr);
+}
+EXPORT_SYMBOL_GPL(serial8250_clear_and_reinit_fifos);
+
 /*
  * IER sleep support.  UARTs which have EFRs need the "extended
  * capability" bit enabled.  Note that on XR16C850s, we need to
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 2868a1d..c9d0ebe 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -96,6 +96,8 @@ static inline void serial_out(struct uart_8250_port *up, int offset, int value)
 	up->port.serial_out(&up->port, offset, value);
 }
 
+void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p);
+
 #if defined(__alpha__) && !defined(CONFIG_PCI)
 /*
  * Digital did something really horribly wrong with the OUT1 and OUT2
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 858dca8..024551a 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/tty.h>
+#include <linux/serial_reg.h>
 #include <linux/serial_core.h>
 #include <linux/8250_pci.h>
 #include <linux/bitops.h>
@@ -1092,11 +1093,49 @@ static int skip_tx_en_setup(struct serial_private *priv,
 	return pci_default_setup(priv, board, port, idx);
 }
 
+static void kt_handle_break(struct uart_port *p)
+{
+	struct uart_8250_port *up =
+		container_of(p, struct uart_8250_port, port);
+	/*
+	 * On receipt of a BI, serial device in Intel ME (Intel
+	 * management engine) needs to have its fifos cleared for sane
+	 * SOL (Serial Over Lan) output.
+	 */
+	serial8250_clear_and_reinit_fifos(up);
+}
+
+static unsigned int kt_serial_in(struct uart_port *p, int offset)
+{
+	struct uart_8250_port *up =
+		container_of(p, struct uart_8250_port, port);
+	unsigned int val;
+
+	/*
+	 * When the Intel ME (management engine) gets reset its serial
+	 * port registers could return 0 momentarily.  Functions like
+	 * serial8250_console_write, read and save the IER, perform
+	 * some operation and then restore it.  In order to avoid
+	 * setting IER register inadvertently to 0, if the value read
+	 * is 0, double check with ier value in uart_8250_port and use
+	 * that instead.  up->ier should be the same value as what is
+	 * currently configured.
+	 */
+	val = inb(p->iobase + offset);
+	if (offset == UART_IER) {
+		if (val == 0)
+			val = up->ier;
+	}
+	return val;
+}
+
 static int kt_serial_setup(struct serial_private *priv,
 			   const struct pciserial_board *board,
 			   struct uart_port *port, int idx)
 {
 	port->flags |= UPF_BUG_THRE;
+	port->serial_in = kt_serial_in;
+	port->handle_break = kt_handle_break;
 	return skip_tx_en_setup(priv, board, port, idx);
 }
 

[PATCH v3 3/3] serial/8250_pci: fix suspend/resume vs init/exit quirks

[Dan Williams]

Commit e86ff4a6 "serial/8250_pci: init-quirk msi support for kt serial
controller" introduced a regression in suspend/resume by causing msi's
to be enabled twice without an intervening disable.

00:16.3 Serial controller: Intel Corporation Patsburg KT Controller (rev 05) (prog-if 02 [16550])
       Subsystem: Intel Corporation Device 7270
       Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 72
       I/O ports at 4080 [size=8]
       Memory at d1c30000 (32-bit, non-prefetchable) [size=4K]
       Capabilities: [c8] Power Management version 3
       Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+
       Kernel driver in use: serial

[  365.250523] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:16.3/msi_irqs'
[  365.250525] Modules linked in: nls_utf8 ipv6 uinput sg iTCO_wdt
  iTCO_vendor_support ioatdma dca i2c_i801 i2c_core wmi sd_mod ahci libahci isci
  libsas libata scsi_transport_sas [last unloaded: scsi_wait_scan]
[  365.250540] Pid: 9030, comm: kworker/u:1 Tainted: G        W    3.3.0-isci-3.0.213+ #1
[  365.250542] Call Trace:
[  365.250545]  [<ffffffff8115e955>] ? sysfs_add_one+0x99/0xad
[  365.250548]  [<ffffffff8102db8b>] warn_slowpath_common+0x85/0x9e
[  365.250551]  [<ffffffff8102dc96>] warn_slowpath_fmt+0x6e/0x70
[  365.250555]  [<ffffffff8115e8fa>] ? sysfs_add_one+0x3e/0xad
[  365.250558]  [<ffffffff8115e8b4>] ? sysfs_pathname+0x3c/0x44
[  365.250561]  [<ffffffff8115e8b4>] ? sysfs_pathname+0x3c/0x44
[  365.250564]  [<ffffffff8115e8b4>] ? sysfs_pathname+0x3c/0x44
[  365.250567]  [<ffffffff8115e8b4>] ? sysfs_pathname+0x3c/0x44
[  365.250570]  [<ffffffff8115e955>] sysfs_add_one+0x99/0xad
[  365.250573]  [<ffffffff8115f031>] create_dir+0x72/0xa5
[  365.250577]  [<ffffffff8115f194>] sysfs_create_dir+0xa2/0xbe
[  365.250581]  [<ffffffff81262463>] kobject_add_internal+0x126/0x1f8
[  365.250585]  [<ffffffff8126255b>] kset_register+0x26/0x3f
[  365.250588]  [<ffffffff8126275a>] kset_create_and_add+0x62/0x7c
[  365.250592]  [<ffffffff81293619>] populate_msi_sysfs+0x34/0x103
[  365.250595]  [<ffffffff81293e1c>] pci_enable_msi_block+0x1b3/0x216
[  365.250599]  [<ffffffff81303f7c>] try_enable_msi+0x13/0x17
[  365.250603]  [<ffffffff81303fb3>] pciserial_resume_ports+0x21/0x42
[  365.250607]  [<ffffffff81304041>] pciserial_resume_one+0x50/0x57
[  365.250610]  [<ffffffff81283e1a>] pci_legacy_resume+0x38/0x47
[  365.250613]  [<ffffffff81283e7d>] pci_pm_restore+0x54/0x87
[  365.250616]  [<ffffffff81283e29>] ? pci_legacy_resume+0x47/0x47
[  365.250619]  [<ffffffff8131e9e8>] dpm_run_callback+0x48/0x7b
[  365.250623]  [<ffffffff8131f39a>] device_resume+0x342/0x394
[  365.250626]  [<ffffffff8131f5b7>] async_resume+0x21/0x49

That patch has since been reverted, but by inspection it seems that
pciserial_suspend_ports() should be invoking .exit() quirks to release
resources acquired during .init().

Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/tty/serial/8250/8250_pci.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 024551a..24ea98c 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -2814,6 +2814,12 @@ void pciserial_suspend_ports(struct serial_private *priv)
 	for (i = 0; i < priv->nr; i++)
 		if (priv->line[i] >= 0)
 			serial8250_suspend_port(priv->line[i]);
+
+	/*
+	 * Ensure that every init quirk is properly torn down
+	 */
+	if (priv->quirk->exit)
+		priv->quirk->exit(priv->dev);
 }
 EXPORT_SYMBOL_GPL(pciserial_suspend_ports);
 

Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Williams, Dan J]

On Mon, Apr 9, 2012 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/09/2012 01:48 PM, Dan Williams wrote:
>> The "KT" serial port has another use case for a "received break" quirk,
>> so before adding another special case to the 8250 core take this
>> opportunity to push such quirks out of the core and into a uart_port op.
>>
>> Stephen says:
>> "If the callback function is to no longer live in 8250.c itself,
>>  arch/arm/mach-tegra/devices.c isn't logically a good place to put it,
>>  and that file will be going away once we get rid of all the board files
>>  and move solely to device tree."
>>
>> ...so since 8250_pci.c houses all the quirks for pci serial devices this
>> quirk is similarly housed in of_serial.c.  Once the open firmware
>> conversion completes the infrastructure details (CONFIG_TEGRA_SERIAL,
>> include/linux/of_serial.h, and the export) can all be removed to make
>> this self contained to of_serial.c.
>
> This still seems much to invasive.

I agree that the arch/arm/mach-tegra/ and Kconfig touches are messy,
but those are also things that will just be deleted when Tegra
completes its transition to open firmware.

If you take those changes out of the equation this patch is actually
net-positive in terms deleting more lines than it adds.

 drivers/tty/serial/8250/8250.c |   34 +++-------------------------------
 drivers/tty/serial/of_serial.c |   25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 31 deletions(-)

> Again, what's wrong with simply keeping the quirk implementation where
> it is, and driving everything off the already-extant
> plat_serial8250_port.type == PORT_TEGRA flag?

Because Alan said to Sudhakar's new "break" quirk:

"This wants to be some kind of call back handled case not more stuff in
the core 8250.c which we are trying to drive all the special cases back
out of."

...and a certain Stephen Warren ;-) said in a comment:

" * FIXME: This needs to become a port specific callback once we have a
  * framework for this"

> I believe all that's required here is to enhance struct struct
> serial8250_config (and hence 8250.c's uart_config[] quirk table) to be
> able to set the quirk function pointers as well as all the other exiting
> quirked variables.

That does not provide the necessary amount of flexibility.  The "KT"
device is a good example it's a standard 8250 with a handle_break
quirk.  We could not express that if quirks were tied to
serial8250_config... outside of requiring a new type for every
variation of quirk.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Arnd Bergmann]

On Monday 09 April 2012, Dan Williams wrote:
> The "KT" serial port has another use case for a "received break" quirk,
> so before adding another special case to the 8250 core take this
> opportunity to push such quirks out of the core and into a uart_port op.
> 
> Stephen says:
> "If the callback function is to no longer live in 8250.c itself,
>  arch/arm/mach-tegra/devices.c isn't logically a good place to put it,
>  and that file will be going away once we get rid of all the board files
>  and move solely to device tree."
> 
> ...so since 8250_pci.c houses all the quirks for pci serial devices this
> quirk is similarly housed in of_serial.c.  Once the open firmware
> conversion completes the infrastructure details (CONFIG_TEGRA_SERIAL,
> include/linux/of_serial.h, and the export) can all be removed to make
> this self contained to of_serial.c.

Looks much better to me, thanks!

Acked-by: Arnd Bergmann <arnd@arndb.de>

> @@ -81,7 +83,9 @@ static struct platform_device trimslice_audio_device = {
>  };
>  
>  static struct platform_device *trimslice_devices[] __initdata = {
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
>  	&debug_uart,
> +#endif
>  	&tegra_sdhci_device1,
>  	&tegra_sdhci_device4,
>  	&tegra_i2s_device1,

FWIW, I'd prefer to see this without the #if, I generally think the devices
that are present should not depend on the drivers that are built into the
kernel. However, since this code is going away soon, I don't care all that
much about it.

	Arnd

Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Stephen Warren]

On 04/09/2012 01:48 PM, Dan Williams wrote:
> The "KT" serial port has another use case for a "received break" quirk,
> so before adding another special case to the 8250 core take this
> opportunity to push such quirks out of the core and into a uart_port op.

> diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
...
> +CONFIG_SERIAL_TEGRA=y

Instead of that,

> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
...
> +# FIXME remove this option when Tegra completes conversion to open firmware
> +config SERIAL_TEGRA
> +	bool "Tegra serial port support"
> +	depends on SERIAL_OF_PLATFORM=y
> +	help
> +	  If you have a machine based on NVIDIA Tegra you can enable its
> +	  onboard serial ports by enabling this option.
> +

Can we just make that default y if ARCH_TEGRA?

defconfig changes are apparently a little touchy.

Actually, why even introduce a new config variable; why not replace the
ifdefs in of_serial.[ch] with CONFIG_ARCH_TEGRA?

> diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
...
>  static struct platform_device *harmony_devices[] __initdata = {
> +#if IS_ENABLED(CONFIG_SERIAL_TEGRA)
>  	&debug_uart,
> +#endif

Yes, it'd be nice to avoid those ifdefs. If you used ARCH_TEGRA instead
of a new config variable for this, the ifdefs in the board files would
be guaranteed to be true.

> diff --git a/arch/arm/mach-tegra/devices.h b/arch/arm/mach-tegra/devices.h
> index ec45567..6e5f852 100644
> --- a/arch/arm/mach-tegra/devices.h
> +++ b/arch/arm/mach-tegra/devices.h
> @@ -53,5 +53,4 @@ extern struct platform_device tegra_i2s_device1;
>  extern struct platform_device tegra_i2s_device2;
>  extern struct platform_device tegra_das_device;
>  extern struct platform_device tegra_pcm_device;
> -
>  #endif

That's left over from a previous patch version.

> @@ -84,6 +106,9 @@ static int __devinit of_platform_serial_setup(struct platform_device *ofdev,
>  		| UPF_FIXED_PORT | UPF_FIXED_TYPE;
>  	port->dev = &ofdev->dev;
>  
> +	if (type == PORT_TEGRA)
> +		port->handle_break = tegra_serial_handle_break;

This is going to mean that everything in 8250.c:uart_config[] will move
into the exact same data structure in of_serial.c eventually, so I still
don't see the point of this exercise. But, I guess I won't argue against
it any more.

> diff --git a/include/linux/of_serial.h b/include/linux/of_serial.h
...
> + * FIXME remove this file when tegra finishes conversion to open firmware,

remove this *prototype* not *file*?

Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Dan Williams]

On Tue, Apr 10, 2012 at 8:53 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/09/2012 01:48 PM, Dan Williams wrote:
>> The "KT" serial port has another use case for a "received break" quirk,
>> so before adding another special case to the 8250 core take this
>> opportunity to push such quirks out of the core and into a uart_port op.
>
>> diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
> ...
>> +CONFIG_SERIAL_TEGRA=y
>
> Instead of that,
>
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> ...
>> +# FIXME remove this option when Tegra completes conversion to open firmware
>> +config SERIAL_TEGRA
>> +     bool "Tegra serial port support"
>> +     depends on SERIAL_OF_PLATFORM=y
>> +     help
>> +       If you have a machine based on NVIDIA Tegra you can enable its
>> +       onboard serial ports by enabling this option.
>> +
>
> Can we just make that default y if ARCH_TEGRA?
>
> defconfig changes are apparently a little touchy.
>
> Actually, why even introduce a new config variable; why not replace the
> ifdefs in of_serial.[ch] with CONFIG_ARCH_TEGRA?

Nice, yes, that makes things a bit cleaner.

>> --- a/arch/arm/mach-tegra/devices.h
>> +++ b/arch/arm/mach-tegra/devices.h
>> @@ -53,5 +53,4 @@ extern struct platform_device tegra_i2s_device1;
>>  extern struct platform_device tegra_i2s_device2;
>>  extern struct platform_device tegra_das_device;
>>  extern struct platform_device tegra_pcm_device;
>> -
>>  #endif
>
> That's left over from a previous patch version.

yep.

>> @@ -84,6 +106,9 @@ static int __devinit of_platform_serial_setup(struct platform_device *ofdev,
>>               | UPF_FIXED_PORT | UPF_FIXED_TYPE;
>>       port->dev = &ofdev->dev;
>>
>> +     if (type == PORT_TEGRA)
>> +             port->handle_break = tegra_serial_handle_break;
>
> This is going to mean that everything in 8250.c:uart_config[] will move
> into the exact same data structure in of_serial.c eventually, so I still
> don't see the point of this exercise. But, I guess I won't argue against
> it any more.

To me serial8250_config is about serial port geometries, uart_port ops
are about quirks.  Once open firmware needs to deal with a serial port
with a standard geometry but with a runtime quirk this approach of
just looking at the type will need to be revisited.  The fact that
'PORT_TEGRA' is enough to identify the geometry and the quirk is
serendipitous.

>> diff --git a/include/linux/of_serial.h b/include/linux/of_serial.h
> ...
>> + * FIXME remove this file when tegra finishes conversion to open firmware,
>
> remove this *prototype* not *file*?

The entire file should go away since it is only there to export
tegra_serial_handle_break to the deprecated board files.  Once this is
open firmware only tegra_serial_handle_break can be marked static,
right?

--
Dan

Re: [PATCH v3 1/3] tegra, serial8250: add ->handle_break() uart_port op

[Stephen Warren]

On 04/10/2012 11:36 AM, Dan Williams wrote:
> On Tue, Apr 10, 2012 at 8:53 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/09/2012 01:48 PM, Dan Williams wrote:
>>> The "KT" serial port has another use case for a "received break" quirk,
>>> so before adding another special case to the 8250 core take this
>>> opportunity to push such quirks out of the core and into a uart_port op.
...
>>> diff --git a/include/linux/of_serial.h b/include/linux/of_serial.h
>> ...
>>> + * FIXME remove this file when tegra finishes conversion to open firmware,
>>
>> remove this *prototype* not *file*?
> 
> The entire file should go away since it is only there to export
> tegra_serial_handle_break to the deprecated board files.  Once this is
> open firmware only tegra_serial_handle_break can be marked static,
> right?

Ah yes, right.