On Tue, Sep 02, 2014 at 11:06:30AM +0100, Andre Przywara wrote:

Hi Rob,

thanks for looking at this.

On 02/09/14 04:06, Rob Herring wrote:

quoted

On Fri, Aug 29, 2014 at 11:13 AM, Andre Przywara [off-list ref] wrote:

quoted

The ARM Server Base System Architecture (SBSA) describes a generic
UART which all compliant level 1 systems should implement. This is
actually a PL011 subset, so a full PL011 implementation will satisfy
this requirement.
However if a system does not have a PL011, a very stripped down
implementation complying to the SBSA defined specification will
suffice. The Linux PL011 driver is not guaranteed to drive this
limited device (and indeed the fast model implentation hangs the
kernel if driven by the PL011 driver).
So introduce a new driver just implementing the part specified by the
SBSA (which lacks DMA, the modem control signals and many of the
registers including baud rate control). This driver has been derived
by the actual PL011 one, removing all unnecessary code.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    6 +
 drivers/tty/serial/Kconfig                         |   28 +
 drivers/tty/serial/Makefile                        |    1 +
 drivers/tty/serial/sbsa_uart.c                     |  793 ++++++++++++++++++++
 include/uapi/linux/serial_core.h                   |    1 +

Sorry, but I think this is all wrong. We've now just duplicated some
subset of the pl011 driver leaving out the parts like setting baudrate
which can never be added since those things could be different for
every vendor.

The original intent of the SBSA uart was to provide a common early
debug uart. It was not to have a full fledged driver. I think the SBSA
has failed in this area and created the potential to create a mess of
serial drivers different for every vendor. Reality will hopefully not
be that extreme and most vendors will just use the pl011 and create
their value add somewhere besides the uart. For the purpose of debug
output, we already support that as the pl011 earlycon only touches
SBSA compatible registers.

I agree that we don't want a proliferation of not-quite-pl011 devices
that we end up having to drive differently.

If we do have such devices I wouldn't want to call them a pl011s for the
sake of earlycon if they aren't actually pl011s.

I see your point (and was actually looking for those kind of comments
when posting this).
I agree to that debug aspect and understand that earlycon already does
this, but I think we need some support beyond earlycon, to be able to
login and use it as a console (which is not possible with earlycon,
right?) This is probably still for debugging or emergency access to the
system only, but maybe also for logging - actually quite similar to how
UARTs are used on today's x86 servers.
So after having written three incarnations of this driver (goldfish
based, PL010 based, PL011 based) I wonder if supporting the SBSA subset
in the real PL011 driver is an option. Either this would be enabled by a
new explicit DT property or preferably by a clever compatible string.
Ideally we would just provide a different set of "struct uart_ops"
members, with some pointing to generic PL011 routines, some to SBSA UART
specific ones.
Maybe we make the full featured PL011 support a config option
(defaulting to y), allowing people to only use the SBSA subset in their
kernel?

Does that make more sense? (for a general SBSA h/w rationale see below)

quoted

quoted

 5 files changed, 829 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
 create mode 100644 drivers/tty/serial/sbsa_uart.c

diff --git a/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
new file mode 100644
index 0000000..8e2c5d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt

@@ -0,0 +1,6 @@
+* ARM SBSA defined generic UART
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"

This alone is not okay. There is no such implementation of hardware.

Do you want something like:

- compatible: must contain an "arm,sbsa-uart" following a more specific
  entry from the following list:

  * "arm,pl011"

Or do we need to restructure the pl011 docs entirely?

Mark.