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

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

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).
Is mmio16 not supported for command line parsing?

Ok, I will drop the above hunk.

Cheers
Jon