On Tue, 2015-09-15 at 13:41 +0300, Heikki Krogerus wrote:
> If the DW_apb_uart is configured with UART_16550_COMPATIBLE
> configuration parameter set, then the Busy Functionality is
> not available. These UARTs will never generate the Busy
> detect indication interrupt, and therefore don't need
> handling for it.
>
> This creates a small optimization for the DW_apb_uarts
> configured without the busy functionality, but more
> importantly, it removes the small but real risk of hitting
> potential issues caused by busy functionality handling when
> no busy functionality exist.
>
Couple of comments.
> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> .../devicetree/bindings/serial/snps-dw-apb-uart.txt | 3 +++
> drivers/tty/serial/8250/8250_dw.c | 21
> ++++++++++++++++++++-
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb
> -uart.txt b/Documentation/devicetree/bindings/serial/snps-dw-apb
> -uart.txt
> index 289c40e..12bbe9f 100644
> --- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> +++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> @@ -15,6 +15,9 @@ The supplying peripheral clock can also be handled,
> needing a second property
> Required elements: "baudclk", "apb_pclk"
>
> Optional properties:
> +- snps,uart-16550-compatible : reflects the value of
> UART_16550_COMPATIBLE
> + configuration parameter. Define this if your UART does not
> implement the busy
> + functionality.
> - resets : phandle to the parent reset controller.
> - reg-shift : quantity to shift the register offsets by. If this
> property is
> not present then the register offsets are not shifted.
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index 715215a..1ca7d75 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -65,6 +65,7 @@ struct dw8250_data {
> struct uart_8250_dma dma;
>
> unsigned skip_autocfg:1;
> + unsigned uart_16550_compatible:1;
Perhaps to follow the style of 8250? Is it okay to use plain 'unsigned'
here?
> };
>
> #define BYT_PRV_CLK 0x800
> @@ -314,11 +315,20 @@ static void dw8250_quirks(struct uart_port *p,
> struct dw8250_data *data)
> }
> #endif
> } else if (has_acpi_companion(p->dev)) {
> + const struct acpi_device_id *id;
> +
> p->iotype = UPIO_MEM32;
> p->regshift = 2;
> p->serial_in = dw8250_serial_in32;
> p->serial_out = dw8250_serial_out32;
> - p->set_termios = dw8250_set_termios;
> +
> + id = acpi_match_device(p->dev->driver
> ->acpi_match_table, p->dev);
> + if ((id && strcmp(id->id, "AMD0020") &&
> + strcmp(id->id, "APMC0D08")) || !id) {
Shouldn't we ask the actual owners of such devices if they have the
Busy Functionality enabled?
Otherwise can we use string array to match with?
> + /* Intel platforms don't have Busy
> Functionality */
> + p->set_termios = dw8250_set_termios;
> + data->uart_16550_compatible = true;
> + }
> }
>
> /* Platforms with iDMA */
> @@ -375,6 +385,9 @@ static int dw8250_probe(struct platform_device
> *pdev)
> data->usr_reg = DW_UART_USR;
> p->private_data = data;
>
> + data->uart_16550_compatible = device_property_read_bool(p
> ->dev,
> + "snps,uart-16550
> -compatible");
> +
> err = device_property_read_u32(p->dev, "reg-shift", &val);
> if (!err)
> p->regshift = val;
> @@ -461,6 +474,12 @@ static int dw8250_probe(struct platform_device
> *pdev)
>
> dw8250_quirks(p, data);
>
> + /* If the Busy Functionality is not implemented, don't
> handle it */
> + if (data->uart_16550_compatible) {
> + p->serial_out = NULL;
> + p->handle_irq = NULL;
> + }
> +
> if (!data->skip_autocfg)
> dw8250_setup_port(&uart);
>
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
