Re: [RFC PATCH 1/1] drivers: introduce ARM SBSA generic UART driver

Mark Rutland
2014-09-02T10:47:47Z

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:
> > On Fri, Aug 29, 2014 at 11:13 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> >> 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)
>
> >> 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.