Re: [PATCH v3 05/11] serial: sc16is7xx: fix broken port 0 uart init

[Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>]

On Thu, 25 May 2023, Hugo Villeneuve wrote:

> From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> 
> While experimenting with rs485 configuration on a SC16IS752 dual UART,

You can remove this intro, it's not necessary.

> I found that the sc16is7xx_config_rs485() function was called only for
> the second port (index 1, channel B), causing initialization problems
> for the first port.

Just start with:

sc16is7xx_config_rs485() function is called only for ...

> For the sc16is7xx driver, port->membase and port->mapbase are not set,
> and their default values are 0. And we set port->iobase to the device
> index. This means that when the first device is registered using the
> uart_add_one_port() function, the following values will be in the port
> structure:
>     port->membase = 0
>     port->mapbase = 0
>     port->iobase  = 0
> 
> Therefore, the function uart_configure_port() in serial_core.c will
> exit early because of the following check:
> 	/*
> 	 * If there isn't a port here, don't do anything further.
> 	 */
> 	if (!port->iobase && !port->mapbase && !port->membase)
> 		return;
> 
> Typically, I2C and SPI drivers do not set port->membase and
> port->mapbase. But I found that the max310x driver sets
> port->membase to ~0 (all ones).

The max310x driver sets port->membase to ~0 (all ones) to solve the same 
problem.

> By implementing the same change in our
> driver, uart_configure_port() is now correctly executed.

our driver -> this driver

This changelog was really well describing the problem! :-)

> Fixes: dfeae619d781 ("serial: sc16is7xx")
> Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> ---
> I am not sure if this change is the best long-term solution to this
> problem, and maybe uart_configure_port() itself could be modified to
> take into account the fact that some devices have all three *base
> values set to zero?

Yeah, some other solution should be devised. Maybe we should add another 
.iotype for thse kind of devices. But I'm fine with this for this fix.
After editing the changelog, feel free to add:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>

> Also, many drivers use port->iobase as an index, is it the correct way
> to use it?

"Many" for this and max310x? Besides that, uartlite.c has a comment which 
says "mark port in use".

-- 
 i.


> For example, for our driver, there was
> commit 5da6b1c079e6 ("sc16is7xx: Set iobase to device index") with the
> following explanation:
>     "Set the .iobase value to the relative index within the device to allow
>     infering the order through sysfs."
> 
>  drivers/tty/serial/sc16is7xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index af7e66db54b4..8a2fc6f89d36 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1443,6 +1443,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
>  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
>  		s->p[i].port.iobase	= i;
> +		s->p[i].port.membase	= (void __iomem *)~0;
>  		s->p[i].port.iotype	= UPIO_PORT;
>  		s->p[i].port.uartclk	= freq;
>  		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
>