Received-SPF: pass (google.com: domain of linux-kernel+bounces-123245-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 28 Mar 2024 18:22:05 +0200
From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Guanbing Huang <albanhuang@outlook.com>
Cc: gregkh@linuxfoundation.org, jirislaby@kernel.org,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	lvjianmin@loongson.cn, albanhuang@tencent.com,
	tombinfan@tencent.com
Subject: Re: [PATCH v3] serial: 8250_pnp: Support configurable reg shift
 property
Message-ID: <ZgWZLfKp9fqAHjb8@smile.fi.intel.com>
References: <SG2PR06MB4953D0E0A40FDAC34FF130C8C93B2@SG2PR06MB4953.apcprd06.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SG2PR06MB4953D0E0A40FDAC34FF130C8C93B2@SG2PR06MB4953.apcprd06.prod.outlook.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Thu, Mar 28, 2024 at 11:45:29AM +0800, Guanbing Huang wrote:
> From: Guanbing Huang <albanhuang@tencent.com>
> 
> The 16550a serial port based on the ACPI table requires obtaining the
> reg-shift attribute. In the ACPI scenario, If the reg-shift property
> is not configured like in DTS, the 16550a serial driver cannot read or
> write controller registers properly during initialization.

Thank you for the update!
Basically we have now a few issues with this (easy to fix though).

Note, below I described _two_ variants of what needs to be done,
but if you think it's too much for you, let's go in v4 to the code
of v2 (i.o.w. just adding what you need in 8250_pnp).

..

>  	memset(&uart, 0, sizeof(uart));
> -	if (pnp_irq_valid(dev, 0))
> -		uart.port.irq = pnp_irq(dev, 0);

This has checked for IRQ validness and left it 0 if not...

>  	if ((flags & CIR_PORT) && pnp_port_valid(dev, 2)) {
>  		uart.port.iobase = pnp_port_start(dev, 2);
> -		uart.port.iotype = UPIO_PORT;
>  	} else if (pnp_port_valid(dev, 0)) {
>  		uart.port.iobase = pnp_port_start(dev, 0);
> -		uart.port.iotype = UPIO_PORT;
>  	} else if (pnp_mem_valid(dev, 0)) {
>  		uart.port.mapbase = pnp_mem_start(dev, 0);

(The mapsize is also needed now to be initialized correctly)

> -		uart.port.iotype = UPIO_MEM;
>  		uart.port.flags = UPF_IOREMAP;
>  	} else
>  		return -ENODEV;
>  
> -	dev_dbg(&dev->dev,
> -		 "Setup PNP port: port %#lx, mem %#llx, irq %u, type %u\n",
> -		 uart.port.iobase, (unsigned long long)uart.port.mapbase,
> -		 uart.port.irq, uart.port.iotype);
> +	uart.port.uartclk = 1843200;
> +	uart.port.dev = &dev->dev;
> +
> +	ret = uart_read_port_properties(&uart.port);

..which means here you need to check for IRQ being absent. The example
is 8250_dw.c, where similar check is done.

> +	if (ret < 0)
> +		return ret;

Then the iotype should be preserved, most likely it will become UPIO_UNKNOWN
for those who do not provide reg-io-width property. That means you need an
additional local variable that you assign instead of port->iotype in the above
if-else-if chain and (re)assign port->iotype with it here.

..

Next point is that above API doesn't cover PNP devices, what you need is
to add a patch to that API in drivers/tty/serial/serial_port.c

        else if (dev_is_pnp(dev))
		ret = pnp_irq(to_pnp_dev(dev), 0);
		if (ret < 0)
			ret = -ENXIO;
	} else

You may try to fix pnp to return -ENXIO from pnp_irq(), but this might need
more careful check of users that none of them relies on the exact returned
value. (If you choose this direction, the mentioned change has to be done
separately.)

And also update include/linux/pnp.h to provide dev_is_pnp() macro
(somewhere before module_pnp_driver() I think)

#define dev_is_pnp(d)	((d)->bus == &pnp_bus_type)

(This should be done in a separate patch)

That said you should have the series out of 3+ patches:
1) (optional) Fix pnp_irq() to return -ENXIO.
2) Add dev_is_pnp() macro to include/linux/pnp.h.
3) Add support of PNP IRQ to __uart_read_properties().
4) Update 8250_pnp to support the additional properties.


Alternatively to the above you may simply try pnp_irq() after the call

	ret = uart_read_port_properties(&uart.port);
	if (ret == -ENXIO) {
		if (pnp_irq_valid(dev, 0))
			uart.port.irq = pnp_irq(dev, 0);
		if (pnp_irq_flags(dev, 0) & IORESOURCE_IRQ_SHAREABLE)
			uart.port.flags |= UPF_SHARE_IRQ;
	} else if (ret) {
		return ret;
	}

-- 
With Best Regards,
Andy Shevchenko