mbox series

[v3,0/2] drivers: serial: Aspeed VUART driver

Message ID 20170410040400.5509-1-joel@jms.id.au
Headers show
Series drivers: serial: Aspeed VUART driver | expand

Message

Joel Stanley April 10, 2017, 4:03 a.m. UTC
This is v3 of a driver for the Aspeed VUART. This version addresses feedback
from Andy and Greg, and includes Rob's ack for the bindings change.

The VUART is a serial device on the BMC side of the LPC bus that connects a BMC
to it's host processor.

We add a flag to the serial core to allow the driver to skip probing of the
THRE irq behaviour, which could hang due to the host not reading bytes out of
the buffer.

We've been using this on systems for over a year, so it has seen a good amount
of testing.

Cheers,

Joel

Jeremy Kerr (1):
  drivers/serial: Add driver for Aspeed virtual UART

Joel Stanley (1):
  serial: 8250: Add flag so drivers can avoid THRE probe

 Documentation/ABI/stable/sysfs-driver-aspeed-vuart |  15 +
 Documentation/devicetree/bindings/serial/8250.txt  |   2 +
 drivers/tty/serial/8250/8250_aspeed_vuart.c        | 323 +++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c                |   2 +-
 drivers/tty/serial/8250/Kconfig                    |  10 +
 drivers/tty/serial/8250/Makefile                   |   1 +
 include/linux/serial_core.h                        |   1 +
 7 files changed, 353 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/stable/sysfs-driver-aspeed-vuart
 create mode 100644 drivers/tty/serial/8250/8250_aspeed_vuart.c

-- 
2.11.0

--
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

Comments

Andy Shevchenko April 11, 2017, 12:15 p.m. UTC | #1
On Mon, Apr 10, 2017 at 7:04 AM, Joel Stanley <joel@jms.id.au> wrote:
> From: Jeremy Kerr <jk@ozlabs.org>

>

> This change adds a driver for the 16550-based Aspeed virtual UART

> device. We use a similar process to the of_serial driver for device

> probe, but expose some VUART-specific functions through sysfs too.

>

> The VUART is two UART 'front ends' connected by their FIFO (no actual

> serial line in between). One is on the BMC side (management controller)

> and one is on the host CPU side.

>

> This driver is for the BMC side. The sysfs files allow the BMC

> userspace, which owns the system configuration policy, to specify at

> what IO port and interrupt number the host side will appear to the host

> on the Host <-> BMC LPC bus. It could be different on a different system

> (though most of them use 3f8/4).

>

> OpenPOWER host firmware doesn't like it when the host-side of the

> VUART's FIFO is not drained. This driver only disables host TX discard

> mode when the port is in use. We set the VUART enabled bit when we bind

> to the device, and clear it on unbind.

>

> We don't want to do this on open/release, as the host may be using this

> bit to configure serial output modes, which is independent of whether

> the devices has been opened by BMC userspace.


> +static void aspeed_vuart_set_enabled(struct aspeed_vuart *vuart, bool enabled)

> +{

> +       u8 reg;

> +

> +       reg = readb(vuart->regs + ASPEED_VUART_GCRA);


> +       reg &= ~ASPEED_VUART_GCRA_VUART_EN;

> +       if (enabled)

> +               reg |= ASPEED_VUART_GCRA_VUART_EN;


Usually the pattern is
if (something)
 set x bit;
else
 clear x bit;

It would make it one operation in any case and a bit more understandable.

> +       writeb(reg, vuart->regs + ASPEED_VUART_GCRA);

> +}

> +

> +static void aspeed_vuart_set_host_tx_discard(struct aspeed_vuart *vuart,

> +                                            bool discard)

> +{

> +       u8 reg;

> +

> +       reg = readb(vuart->regs + ASPEED_VUART_GCRA);

> +

> +       /* If the DISABLE_HOST_TX_DISCARD bit is set, discard is disabled */


> +       reg &= ~ASPEED_VUART_GCRA_DISABLE_HOST_TX_DISCARD;

> +       if (!discard)

> +               reg |= ASPEED_VUART_GCRA_DISABLE_HOST_TX_DISCARD;


Ditto.

> +

> +       writeb(reg, vuart->regs + ASPEED_VUART_GCRA);

> +}


> +       /* The 8510 core creates the mapping, which we grab a reference to

> +        * for VUART-specific registers */


Hmm... What about multi-line style?

> +       port.port.irq = irq_of_parse_and_map(np, 0);


The benefit of use platform_get_irq() is to get rid of some OF specific headers.

-- 
With Best Regards,
Andy Shevchenko
--
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
Joel Stanley May 2, 2017, 7:45 a.m. UTC | #2
On Tue, Apr 11, 2017 at 9:45 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Apr 10, 2017 at 7:04 AM, Joel Stanley <joel@jms.id.au> wrote:

>> From: Jeremy Kerr <jk@ozlabs.org>

>>

>> This change adds a driver for the 16550-based Aspeed virtual UART

>> device. We use a similar process to the of_serial driver for device

>> probe, but expose some VUART-specific functions through sysfs too.

>>

>> The VUART is two UART 'front ends' connected by their FIFO (no actual

>> serial line in between). One is on the BMC side (management controller)

>> and one is on the host CPU side.

>>

>> This driver is for the BMC side. The sysfs files allow the BMC

>> userspace, which owns the system configuration policy, to specify at

>> what IO port and interrupt number the host side will appear to the host

>> on the Host <-> BMC LPC bus. It could be different on a different system

>> (though most of them use 3f8/4).

>>

>> OpenPOWER host firmware doesn't like it when the host-side of the

>> VUART's FIFO is not drained. This driver only disables host TX discard

>> mode when the port is in use. We set the VUART enabled bit when we bind

>> to the device, and clear it on unbind.

>>

>> We don't want to do this on open/release, as the host may be using this

>> bit to configure serial output modes, which is independent of whether

>> the devices has been opened by BMC userspace.

>

>> +static void aspeed_vuart_set_enabled(struct aspeed_vuart *vuart, bool enabled)

>> +{

>> +       u8 reg;

>> +

>> +       reg = readb(vuart->regs + ASPEED_VUART_GCRA);

>

>> +       reg &= ~ASPEED_VUART_GCRA_VUART_EN;

>> +       if (enabled)

>> +               reg |= ASPEED_VUART_GCRA_VUART_EN;

>

> Usually the pattern is

> if (something)

>  set x bit;

> else

>  clear x bit;

>

> It would make it one operation in any case and a bit more understandable.


I have made these ordering changes you requested.

>

>> +       port.port.irq = irq_of_parse_and_map(np, 0);

>

> The benefit of use platform_get_irq() is to get rid of some OF specific headers.


It's an of driver, and this function does exactly what we require. I
have left it in for v4.

Cheers,

Joel
--
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