drivers: PL011: add ARM SBSA Generic UART support

Quoted 11 lines · Andre Przywara

drivers/tty/serial/amba-pl011.c +11 -0

+static void pl011_enable_interrupts(struct uart_amba_port *uap)
+{
+       spin_lock_irq(&uap->port.lock);
+
+       /* Clear out any spuriously appearing RX interrupts */
+       writew(UART011_RTIS | UART011_RXIS,
+              uap->port.membase + UART011_ICR);
+       uap->im = UART011_RTIM;
+       if (!pl011_dma_rx_running(uap))
+               uap->im |= UART011_RXIM;
+       writew(uap->im, uap->port.membase + UART011_IMSC);

drivers/tty/serial/amba-pl011.c static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h) · line 1583

Quoted 2 lines

drivers/tty/serial/amba-pl011.c +2 -0

+       spin_unlock_irq(&uap->port.lock);
+}

Shouldn't this function be using spin_lock_irqsave() and spin_unlock_irqrestore()? If interrupts are generally disabled before calling this function, then they will be enabled by the spin_unlock_irq() call, and I don't think we want that. This function is only supposed to enable pl011 interrupts, not all interrupts.

Hi Timur,

Quoted 12 lines · Timur Tabi

Quoted 11 lines · Andre Przywara

drivers/tty/serial/amba-pl011.c +11 -0

+static void pl011_enable_interrupts(struct uart_amba_port *uap)
+{
+       spin_lock_irq(&uap->port.lock);
+
+       /* Clear out any spuriously appearing RX interrupts */
+       writew(UART011_RTIS | UART011_RXIS,
+              uap->port.membase + UART011_ICR);
+       uap->im = UART011_RTIM;
+       if (!pl011_dma_rx_running(uap))
+               uap->im |= UART011_RXIM;
+       writew(uap->im, uap->port.membase + UART011_IMSC);

Quoted 8 lines

drivers/tty/serial/amba-pl011.c static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h) · line 1583

Quoted 2 lines

drivers/tty/serial/amba-pl011.c +2 -0

+       spin_unlock_irq(&uap->port.lock);
+}

Shouldn't this function be using spin_lock_irqsave() and spin_unlock_irqrestore()? If interrupts are generally disabled before calling this function, then they will be enabled by the spin_unlock_irq() call, and I don't think we want that. This function is only supposed to enable pl011 interrupts, not all interrupts.

Are you seeing an actual issue with this? Does changing it fix anything? Looking at the history I see that these locks predate git history. If I get this correctly, going from spin_{un,}lock_irq to the _irqsave variants should always be safe, but I'd like to hear more opinions on this.

(quoted diff) +10 -0

+static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
+{
+       struct uart_amba_port *uap;
+       struct vendor_data *vendor = id->data;
+       int portnr, ret;
+
+       portnr = pl011_find_free_port();
+       if (portnr < 0)
+               return portnr;
+

Quoted 16 lines · Andre Przywara

(quoted diff) +16 -0

+static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
+{
+       struct uart_amba_port *uap;
+       struct vendor_data *vendor = id->data;
+       int portnr, ret;
+
+       portnr = pl011_find_free_port();
+       if (portnr < 0)
+               return portnr;
+
+       uap = devm_kzalloc(&dev->dev, sizeof(struct uart_amba_port),
+                          GFP_KERNEL);
+       if (!uap)
+               return -ENOMEM;
+
+       uap->clk = devm_clk_get(&dev->dev, NULL);

Review

Quoted 4 lines

(quoted diff) +4 -0

+       if (IS_ERR(uap->clk))
+               return PTR_ERR(uap->clk);
+
+       = vendor;

I'm having trouble with this. Specifically, I can't get SBSA early console to work, because uap->vendor is uninitialized when pl011_early_write() is called.

I don't have a non-SBSA system to test, but looking at the code, I'm not sure how early console is supposed to work any more. In both SBSA and non-SBSA cases, uap->vendor is not initialized until the probe function is called. Isn't that too late? I thought early console was supposed to be running before the driver is probed.

If I make this change, then early console works on SBSA systems (with earlycon=pl011,<address>). But of course, this only works for SBSA and breaks regular PL011.

drivers/tty/serial/amba-pl011.c +6 -0

@@ -2326,9 +2326,15 @@ static void pl011_early_write(struct console *con, const
 static int __init pl011_early_console_setup(struct earlycon_device *device,
                                            const char *opt)
 {
+       struct uart_amba_port *uap =
+           container_of(&device->port, struct uart_amba_port, port);
+
        if (!device->port.membase)
                return -ENODEV;
+       if (!uap->vendor)
+               uap->vendor = &vendor_sbsa;
+
        device->con->write = pl011_early_write;
        return 0;
 }
 

I don't get where pl011_early_write() would need any kind of PL011 struct. It only references structures defined in the serial core, so it's completely dumb MMIO writes and reads, using only the base address provided on the command line.
Are you by any chance mixing up early console and the normal console? To me it looks like early console just uses the three tiny functions defined just before the declaration and it does not touch vendor at all, isn't it?

As for the broken part, any chance you were testing -next, which has some patches that broke early console, as we learned today?[1]

This is the fifth revision of the SBSA UART support series, based on the current tty-next branch.
Beside some minor formatting issues fixed (thanks ot Jakub for noticing) the SBSA UART set_termios() function has been reworked.

We now completely ignore the old value, instead we filter the new setting to match the SBSA UART's fixed parameters for word size, parity, stop bits and so on.
This should also address the issues people have seen with ACPI on Fedora systems.
Mark, Naresh, Graeme: can you confirm that this version works for you? If needed I can bake a follow-up patch on top of v4.
----

The ARM Server Base System Architecture[1] document describes a generic UART which is a subset of the ARM PL011 UART.
It lacks DMA support, baud rate control and modem status line control, among other things.
The idea is to move the UART initialization and setup into the firmware (which does this job today already) and let the kernel just use the UART for sending and receiving characters.

This patchset integrates support for this UART subset into the existing PL011 driver - basically by refactoring some functions and providing a new uart_ops structure for it. It also has a separate probe function to be not dependent on AMBA/PrimeCell. It provides a device tree and an ACPI binding.
Beside the obvious effect of code sharing reusing most of the PL011 code has the advantage of not introducing another serial device prefix, so it can go with ttyAMA, which seems to be pretty common. As changing the baudrate and other communication parameters is not specified for the SBSA UART, any userland attempt will be denied by the kernel. The device-tree provided fixed baud rate will be reported, so stty for instance prints the right value.

This series is based on Greg's tty-next branch, a git repo can be found at [2] (branch sbsa-uart/v5).

Patch 1/11 contains a bug fix which applies to the PL011 part also, it should be considered regardless of the rest of the series. According to Russell this is still racy, but still better than the current solution.
Patch 2-7 refactor some PL011 functions by splitting them up into smaller pieces, so that most of the code can be reused later by the SBSA part.
Patch 8 and 9 introduce two new properties for the vendor structure, this is for SBSA functionality which cannot be controlled by separate uart_ops members only.
Patch 10 then finally drops in the SBSA specific code, by providing a new uart_ops, vendor struct and probe function for it. Also the new device tree binding is documented.
Patch 11 adds the necessary ACPI boilerplate.

For testing you should be able to take any hardware which has a PL011 and change the DT to use a "arm,sbsa-uart" compatible string and the baud rate with the "current-speed" property.
Of course testing with a real SBSA Generic UART is welcomed - as well as regression testing with any PL011 implementation.
I tested this on a FastModel, a Juno and a Midway machine, both in PL011 and in (DT-emulated) SBSA mode.

Tested-by Robert Richter <[email protected]>

Tested-by Lorenzo Pieralisi <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Changelog:
v4 .. v5:
- fixed checkpatch --strict complaints
- rework set_termios:
- disallow changes or UART fixed communication parameters
- ignore old value to fix an issue with Fedora on ACPI

v3 .. v4:
- fixed rebase artifact
- moved memory allocation out of pl011_allocate_port() function (and consequently renamed that to pl011_find_free_port)
- added ACPI driver binding

v2 .. v3:
- rebased on top of tty-next and Dave's latest PL011 rework
- fixed module build
- removed redundant =NULL members in sbsa_uart_ops

v1 .. v2:
- rebased on top of 4.0-rc1 and Dave's newest PL011 fix [3]
- added mandatory current-speed property and report that to userland

Cheers,
Andre

[1] ARM-DEN-0029 Server Base System Architecture, available (click- thru...) from http://infocenter.arm.com
[2] http://www.linux-arm.org/git?p=linux-ap.git
git://linux-arm.org/linux-ap.git
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/327631.html

Andre Przywara (10):
drivers: PL011: avoid potential unregister_driver call
drivers: PL011: refactor pl011_startup()
drivers: PL011: refactor pl011_shutdown()
drivers: PL011: refactor pl011_set_termios()
drivers: PL011: refactor pl011_probe()
drivers: PL011: replace UART_MIS reading with _RIS & _IMSC
drivers: PL011: move cts_event workaround into separate function
drivers: PL011: allow avoiding UART enabling/disabling
drivers: PL011: allow to supply fixed option string
drivers: PL011: add support for the ARM SBSA generic UART

Graeme Gregory (1):
drivers: PL011: add ACPI probing for SBSA UART

  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |  10 +
  drivers/tty/serial/amba-pl011.c                    | 550 +++++++++++++++------
  2 files changed, 412 insertions(+), 148 deletions(-)

create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt

Quoted 4 lines · Andre Przywara

This is the fifth revision of the SBSA UART support series, based on the current tty-next branch.
Beside some minor formatting issues fixed (thanks ot Jakub for noticing) the SBSA UART set_termios() function has been reworked.

Quoted 8 lines

We now completely ignore the old value, instead we filter the new setting to match the SBSA UART's fixed parameters for word size, parity, stop bits and so on.
This should also address the issues people have seen with ACPI on Fedora systems.
Mark, Naresh, Graeme: can you confirm that this version works for you? If needed I can bake a follow-up patch on top of v4.
----

Works for me, thanks.

Tested-by Mark Langsdorf <[email protected]>

--Mark Langsdorf

Quoted 73 lines

The ARM Server Base System Architecture[1] document describes a generic UART which is a subset of the ARM PL011 UART.
It lacks DMA support, baud rate control and modem status line control, among other things.
The idea is to move the UART initialization and setup into the firmware (which does this job today already) and let the kernel just use the UART for sending and receiving characters.

This patchset integrates support for this UART subset into the existing PL011 driver - basically by refactoring some functions and providing a new uart_ops structure for it. It also has a separate probe function to be not dependent on AMBA/PrimeCell. It provides a device tree and an ACPI binding.
Beside the obvious effect of code sharing reusing most of the PL011 code has the advantage of not introducing another serial device prefix, so it can go with ttyAMA, which seems to be pretty common. As changing the baudrate and other communication parameters is not specified for the SBSA UART, any userland attempt will be denied by the kernel. The device-tree provided fixed baud rate will be reported, so stty for instance prints the right value.

This series is based on Greg's tty-next branch, a git repo can be found at [2] (branch sbsa-uart/v5).

Patch 1/11 contains a bug fix which applies to the PL011 part also, it should be considered regardless of the rest of the series. According to Russell this is still racy, but still better than the current solution.
Patch 2-7 refactor some PL011 functions by splitting them up into smaller pieces, so that most of the code can be reused later by the SBSA part.
Patch 8 and 9 introduce two new properties for the vendor structure, this is for SBSA functionality which cannot be controlled by separate uart_ops members only.
Patch 10 then finally drops in the SBSA specific code, by providing a new uart_ops, vendor struct and probe function for it. Also the new device tree binding is documented.
Patch 11 adds the necessary ACPI boilerplate.

For testing you should be able to take any hardware which has a PL011 and change the DT to use a "arm,sbsa-uart" compatible string and the baud rate with the "current-speed" property.
Of course testing with a real SBSA Generic UART is welcomed - as well as regression testing with any PL011 implementation.
I tested this on a FastModel, a Juno and a Midway machine, both in PL011 and in (DT-emulated) SBSA mode.

Tested-by Robert Richter <[email protected]>

Tested-by Lorenzo Pieralisi <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Changelog:
v4 .. v5:
- fixed checkpatch --strict complaints
- rework set_termios:
- disallow changes or UART fixed communication parameters
- ignore old value to fix an issue with Fedora on ACPI

v3 .. v4:
- fixed rebase artifact
- moved memory allocation out of pl011_allocate_port() function (and consequently renamed that to pl011_find_free_port)
- added ACPI driver binding

v2 .. v3:
- rebased on top of tty-next and Dave's latest PL011 rework
- fixed module build
- removed redundant =NULL members in sbsa_uart_ops

v1 .. v2:
- rebased on top of 4.0-rc1 and Dave's newest PL011 fix [3]
- added mandatory current-speed property and report that to userland

Quoted 8 lines

Cheers,
Andre

[1] ARM-DEN-0029 Server Base System Architecture, available (click- thru...) from http://infocenter.arm.com
[2] http://www.linux-arm.org/git?p=linux-ap.git
git://linux-arm.org/linux-ap.git
[3]

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/327631.html

Quoted 13 lines

Andre Przywara (10):
drivers: PL011: avoid potential unregister_driver call
drivers: PL011: refactor pl011_startup()
drivers: PL011: refactor pl011_shutdown()
drivers: PL011: refactor pl011_set_termios()
drivers: PL011: refactor pl011_probe()
drivers: PL011: replace UART_MIS reading with _RIS & _IMSC
drivers: PL011: move cts_event workaround into separate function
drivers: PL011: allow avoiding UART enabling/disabling
drivers: PL011: allow to supply fixed option string
drivers: PL011: add support for the ARM SBSA generic UART

Quoted 5 lines

Graeme Gregory (1):
drivers: PL011: add ACPI probing for SBSA UART

  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |  10 +
  drivers/tty/serial/amba-pl011.c                    | 550

+++++++++++++++------

Quoted 2 lines

  2 files changed, 412 insertions(+), 148 deletions(-)

create mode 100644

Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt

Quoted 1 line

This is the fifth revision of the SBSA UART support series, based on the current tty-next branch.
Beside some minor formatting issues fixed (thanks ot Jakub for noticing) the SBSA UART set_termios() function has been reworked. We now completely ignore the old value, instead we filter the new setting to match the SBSA UART's fixed parameters for word size, parity, stop bits and so on.
This should also address the issues people have seen with ACPI on Fedora systems.
Mark, Naresh, Graeme: can you confirm that this version works for you? If needed I can bake a follow-up patch on top of v4.

----

The ARM Server Base System Architecture[1] document describes a generic UART which is a subset of the ARM PL011 UART.
It lacks DMA support, baud rate control and modem status line control, among other things.
The idea is to move the UART initialization and setup into the firmware (which does this job today already) and let the kernel just use the UART for sending and receiving characters.

This patchset integrates support for this UART subset into the existing PL011 driver - basically by refactoring some functions and providing a new uart_ops structure for it. It also has a separate probe function to be not dependent on AMBA/PrimeCell. It provides a device tree and an ACPI binding.
Beside the obvious effect of code sharing reusing most of the PL011 code has the advantage of not introducing another serial device prefix, so it can go with ttyAMA, which seems to be pretty common. As changing the baudrate and other communication parameters is not specified for the SBSA UART, any userland attempt will be denied by the kernel. The device-tree provided fixed baud rate will be reported, so stty for instance prints the right value.

This series is based on Greg's tty-next branch, a git repo can be found at [2] (branch sbsa-uart/v5).

Patch 1/11 contains a bug fix which applies to the PL011 part also, it should be considered regardless of the rest of the series. According to Russell this is still racy, but still better than the current solution.
Patch 2-7 refactor some PL011 functions by splitting them up into smaller pieces, so that most of the code can be reused later by the SBSA part.
Patch 8 and 9 introduce two new properties for the vendor structure, this is for SBSA functionality which cannot be controlled by separate uart_ops members only.
Patch 10 then finally drops in the SBSA specific code, by providing a new uart_ops, vendor struct and probe function for it. Also the new device tree binding is documented.
Patch 11 adds the necessary ACPI boilerplate.

For testing you should be able to take any hardware which has a PL011 and change the DT to use a "arm,sbsa-uart" compatible string and the baud rate with the "current-speed" property.
Of course testing with a real SBSA Generic UART is welcomed - as well as regression testing with any PL011 implementation.
I tested this on a FastModel, a Juno and a Midway machine, both in PL011 and in (DT-emulated) SBSA mode.

Tested-by Robert Richter <[email protected]>

Tested-by Lorenzo Pieralisi <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Changelog:
v4 .. v5:
- fixed checkpatch --strict complaints
- rework set_termios:
- disallow changes or UART fixed communication parameters
- ignore old value to fix an issue with Fedora on ACPI

v3 .. v4:
- fixed rebase artifact
- moved memory allocation out of pl011_allocate_port() function (and consequently renamed that to pl011_find_free_port)
- added ACPI driver binding

v2 .. v3:
- rebased on top of tty-next and Dave's latest PL011 rework
- fixed module build
- removed redundant =NULL members in sbsa_uart_ops

v1 .. v2:
- rebased on top of 4.0-rc1 and Dave's newest PL011 fix [3]
- added mandatory current-speed property and report that to userland

Cheers,
Andre

[1] ARM-DEN-0029 Server Base System Architecture, available (click- thru...) from http://infocenter.arm.com
[2] http://www.linux-arm.org/git?p=linux-ap.git
git://linux-arm.org/linux-ap.git
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/327631.html

Andre Przywara (10):
drivers: PL011: avoid potential unregister_driver call
drivers: PL011: refactor pl011_startup()
drivers: PL011: refactor pl011_shutdown()
drivers: PL011: refactor pl011_set_termios()
drivers: PL011: refactor pl011_probe()
drivers: PL011: replace UART_MIS reading with _RIS & _IMSC
drivers: PL011: move cts_event workaround into separate function
drivers: PL011: allow avoiding UART enabling/disabling
drivers: PL011: allow to supply fixed option string
drivers: PL011: add support for the ARM SBSA generic UART

Graeme Gregory (1):
drivers: PL011: add ACPI probing for SBSA UART

 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |  10 +
 drivers/tty/serial/amba-pl011.c                    | 550 +++++++++++++++------
 2 files changed, 412 insertions(+), 148 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt

--
2.3.5