On Saturday 30 August 2014 00:10:39 Andre Przywara wrote: > On 08/29/2014 07:59 PM, Arnd Bergmann wrote: > > On Friday 29 August 2014 17:13:23 Andre Przywara 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@xxxxxxx> > > > > > > Hi Andre, > > > > Thanks for getting this driver ready. There is one high-level comment > > I have: As mentioned in the discussion in > > https://lkml.org/lkml/2014/7/28/386 , I think this should really be > > a tty driver using tty_port, not a serial driver using uart_port. > > > > What is the reason you chose to do a uart_port driver? > > Mainly because the SBSA is more of an UART than I originally > anticipated. It's intention may be more for debugging only, but it's > implementation is that of a real UART. > So the goldfish driver for instance seems to be tailored for a virtual > device, where TX/RX actually does not cost much. Also it supports > transmitting large chunks of data at once, an UART cannot do that. I > didn't find an obvious or easy way of implementing IRQ based > transmission. So if someone throws 10 KB at the driver, it will hog the > CPU for a second :-( I think the driver is supposed to just return '0' from its tty->write function when there is no room for more data, and then call tty_wakeup() from the interrupt handler once some buffer space has been freed up. Note that this is actually much simpler to do than the circ_buf handling in uart_port. > Also there is this console line ending issue. The UART layer takes care > about changing LF into CR/LF, but in a pure TTY driver this needs to be > done explicitly. Hooking this into the code was a real nightmare. It should not do that. Did you forget to set tty_std_termios? > Also the error conditions the UART supports (frame error, break) are > hard to model in a pure TTY driver. The register subset doesn't even support flow control, so what's the point in trying to support get_icount? > So after having coded it based on goldfish I decided to go for a real > UART driver instead, and the result is much better. > > A few more details below: > > > >> +} > >> +EARLYCON_DECLARE(pl011, sbsa_uart_early_console_setup); > >> +OF_EARLYCON_DECLARE(pl011, "arm,sbsa-uart", sbsa_uart_early_console_setup); > > > > Stray 'pl011' left from copying the code? > > Actually left in deliberately (to reuse existing kernel command lines), > but I know see that this was silly. > The earlycon routines of PL011 are actually the same as for the SBSA > UART, so both can use one implementation. And registering them twice > under the same name triggers a warning during boot. > I have to check how this can be shared if only one driver is compiled in. > > >> +static struct uart_driver sbsa_uart_reg = { > >> + .owner = THIS_MODULE, > >> + .driver_name = "sbsa_uart", > >> + .dev_name = "ttyAMA", > >> + .nr = UART_NR, > >> + .cons = SBSA_UART_CONSOLE, > >> +}; > > > > I don't think we should overload the ttyAMA name. > > That triggered a lot of discussion here. Actually most people don't want > to introduce yet another serial prefix. Also since the both devices are > so similar and this driver can drive a full PL011 also, I decided to > reuse it. > This still has issues if both drivers are active, but I consider this > saner for the user this way. I'm not convinced. It sounds absolutely possible that someone makes a system that needs both drivers simultaneously, sbsa_uart for the console (with the settings in place from the boot loader and no registers to change them) and a proper uart for talking to other devices. If the earlycon line or the device names are conflicting, you get into trouble. > >> +#ifdef CONFIG_OF > >> + > >> +static int dt_probe_serial_alias(int index, struct device *dev) > >> +{ > >> + struct device_node *np; > >> + static bool seen_dev_with_alias; > >> + static bool seen_dev_without_alias; > >> + int ret = index; > >> + > >> + if (!IS_ENABLED(CONFIG_OF)) > >> + return ret; > > > > The #ifdef should go away since you already have the if > > (!IS_ENABLED(CONFIG_OF)) logic here. > > Right, this is redundant, but I'd rather remove the IS_ENABLED() line, > since I provide a non-DT implementation of that routine below. That would mean you don't get any build-time coverage if CONFIG_OF is disabled. Please just remove all the #ifdefs you can so that the compiler gets to see the code and discard all unused parts of it. Arnd -- 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
