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

  From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

While experimenting with rs485 configuration on a SC16IS752 dual UART,
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.

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). By implementing the same change in our
driver, uart_configure_port() is now correctly executed.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
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?

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

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(+)
  

On Thu, 25 May 2023, Hugo Villeneuve wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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@dimonoff.com>
> ---
> 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@linux.intel.com>

> 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".
  

On Thu, 25 May 2023 14:20:59 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 25 May 2023, Hugo Villeneuve wrote:
> 
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > While experimenting with rs485 configuration on a SC16IS752 dual UART,
> 
> You can remove this intro, it's not necessary.

Fixed.

 
> > 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.

Fixed.


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

Fixed.

 
> This changelog was really well describing the problem! :-)
> 
> > Fixes: dfeae619d781 ("serial: sc16is7xx")
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> > 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@linux.intel.com>

Added.

 
> > 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".

Ok, 
anyway with your approval I will remove these comments which will not part of the final commit message anyway.

Hugo.


> > 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;
> >