Re: [PATCH 2/3] serial: 8250_early: Add support for regshift

[Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>]

On 01/12/2016 10:28 AM, Jon Hunter wrote:
> Hi Peter,
> 
> On 12/01/16 16:15, Peter Hurley wrote:
>> Hi Jon,
>>
>> On 01/12/2016 02:33 AM, Jon Hunter wrote:
>>> The 8250 early console assumes a default regshift for each iotype. This
>>> does not work for all devices. For example, Tegra UARTs use a iotype of
>>> UPIO_MEM with a regshift of 2 because the registers are 32-bit aligned
>>> and permit byte access.
>>>
>>> If the regshift is specified (for example, via device-tree), then use the
>>> value provided and otherwise revert to the defaults assumed for each
>>> iotype.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> Please note that today for Tegra, early console is supported by passing
>>> the boot parameter "earlycon=uart8250,mmio32,0xXXXXXXXX". While this works
>>> and we could use UPIO_MEM32 for tegra, this would mean changing all the
>>> DT source files for Tegra to add the "reg-io-width" property. IMO it seems
>>> better to make early console for 8250 work in the same way as the normal
>>> 8250 console and support regshift.
>>>
>>>  drivers/tty/serial/8250/8250_early.c | 36 ++++++++++++++++++++++++++----------
>>>  1 file changed, 26 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
>>> index af62131af21e..758054957788 100644
>>> --- a/drivers/tty/serial/8250/8250_early.c
>>> +++ b/drivers/tty/serial/8250/8250_early.c
>>> @@ -41,15 +41,15 @@ static unsigned int __init serial8250_early_in(struct uart_port *port, int offse
>>>  {
>>>  	switch (port->iotype) {
>>>  	case UPIO_MEM:
>>> -		return readb(port->membase + offset);
>>> +		return readb(port->membase + (offset << port->regshift));
>>>  	case UPIO_MEM16:
>>> -		return readw(port->membase + (offset << 1));
>>> +		return readw(port->membase + (offset << port->regshift));
>>>  	case UPIO_MEM32:
>>> -		return readl(port->membase + (offset << 2));
>>> +		return readl(port->membase + (offset << port->regshift));
>>>  	case UPIO_MEM32BE:
>>> -		return ioread32be(port->membase + (offset << 2));
>>> +		return ioread32be(port->membase + (offset << port->regshift));
>>>  	case UPIO_PORT:
>>> -		return inb(port->iobase + offset);
>>> +		return inb(port->iobase + (offset << port->regshift));
>>>  	default:
>>>  		return 0;
>>>  	}
>>> @@ -59,19 +59,19 @@ static void __init serial8250_early_out(struct uart_port *port, int offset, int
>>>  {
>>>  	switch (port->iotype) {
>>>  	case UPIO_MEM:
>>> -		writeb(value, port->membase + offset);
>>> +		writeb(value, port->membase + (offset << port->regshift));
>>>  		break;
>>>  	case UPIO_MEM16:
>>> -		writew(value, port->membase + (offset << 1));
>>> +		writew(value, port->membase + (offset << port->regshift));
>>>  		break;
>>>  	case UPIO_MEM32:
>>> -		writel(value, port->membase + (offset << 2));
>>> +		writel(value, port->membase + (offset << port->regshift));
>>>  		break;
>>>  	case UPIO_MEM32BE:
>>> -		iowrite32be(value, port->membase + (offset << 2));
>>> +		iowrite32be(value, port->membase + (offset << port->regshift));
>>>  		break;
>>>  	case UPIO_PORT:
>>> -		outb(value, port->iobase + offset);
>>> +		outb(value, port->iobase + (offset << port->regshift));
>>>  		break;
>>>  	}
>>>  }
>>> @@ -128,6 +128,22 @@ int __init early_serial8250_setup(struct earlycon_device *device,
>>>  	if (!(device->port.membase || device->port.iobase))
>>>  		return -ENODEV;
>>>  
>>> +	/*
>>> +	 * If regshift is not specified, then assume the
>>> +	 * following defaults for the below iotypes.
>>> +	 */
>>> +	if (!device->port.regshift) {
>>> +		switch (device->port.iotype) {
>>> +		case UPIO_MEM16:
>>> +			device->port.regshift = 1;
>>> +			break;
>>> +		case UPIO_MEM32:
>>> +		case UPIO_MEM32BE:
>>> +			device->port.regshift = 2;
>>> +			break;
>>> +		}
>>> +	}
>>
>> Since the earlycon command line parsing sets port->regshift, the only possible
>> path requiring defaults would be DT, but 8250 DT should not have default
>> regshift based on the iotype; the 8250 port driver doesn't.
> 
> Ah, I see, I missed that. However, for command line parsing, I only see
> regshift being set to 2 for mmio32 (and I guess it will be 0 otherwise).

Exactly. Currently unrepresented is 8-bit wide, 32-bit aligned mmio.

> Is mmio16 not supported for command line parsing?

In Greg's tty-next branch @
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git

commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
Author: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
Date:   Wed Oct 28 12:46:05 2015 +0900

    serial: support 16-bit register interface for console
    
    Currently, 8-bit (MMIO) and 32-bit (MMIO32) register interfaces are
    supported for the 8250 console, but the 16-bit (MMIO16) is not.
    The 8250 UART device on my board is connected to a 16-bit bus and
    my main motivation is to use earlycon with it.
    (Refer to arch/arm/boot/dts/uniphier-support-card.dtsi)
    
    Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
    Reviewed-by: Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>

Regards,
Peter Hurley

> Ok, I will drop the above hunk.
> 
> Cheers
> Jon
>