Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Feb 10, 2023 at 07:14:54AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART
> > On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote:
> > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Sent: Thursday, February 9, 2023 2:09 PM On Thu, 9 Feb 2023, Biju
> > > > Das wrote:

...

> > > > > +	[PORT_16750] = {
> > > > > +		.name		= "Renesas RZ/V2M 16750",
> > > > > +		.fifo_size	= 64,
> > > > > +		.tx_loadsz	= 64,
> > > > > +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> > > > > +				  UART_FCR7_64BYTE,
> > > > > +		.rxtrig_bytes	= {1, 16, 32, 56},
> > > > > +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> > > > > +	},
> > > >
> > > > Eh, how can you reuse [PORT_16750] again in the initializer like that?
> > >
> > > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead
> > > as PORT_16750 is used by TI16750?
> > 
> > What the difference to the 16750 from TI that prevents you from using it?
> 
> Mostly it is identical.
> 
> The main difference is detection method, and we don't have UART_IERX_SLEEP bit in IER.
> 
> On TI, it sets bit 5 of the IIR when 64-byte FIFO mode is enabled when DLAB is set.
> 
> Whereas in our case DLAB does n't have any role for Identification, 
> 
> It set bit 5 of the IIR when 64 byte FIFO mode is enabled.
> and it clears bit 5 of the IIR when 64 byte FIFO mode is disabled.

So the question here is do these minor deviations affect the actual functionality?

Note, on Intel hardware we use directly TI16750 while we have no sleep
functionality available IIRC. Ilpo may correct me if I'm wrong.

> Other than that, when I use PORT_16750 type and capabilities in 8250_em driver and 
> add identification method for Renesas UART in 8250_port driver,
> 
> It detected as PORT_16750 UART, but I get below prints during autoconf which is confusing for the end user
> 
> [    0.214926] serial8250-em a4040000.serial: detected caps 00000900 should be 00000d00
> [    0.214975] a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a TI16750
> 
> 
> Modification in 8250_em driver
> 
> +		    up.port.type = PORT_16750;
> +               up.port.name            = "Renesas RZ/V2M 16750";
> +               up.port.fifosize        = 64;
> +               up.tx_loadsz = 64;
> +               up.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> 
> Identification method in 8250_port.c driver
> 
> +       /*
> +        * No EFR.  Try to detect a Renesas RZ/V2M 16750, which only sets bit 5
> +        * of the IIR when 64 byte FIFO mode is enabled.
> +        * Try setting/clear bit5 of FCR.
> +        */
> +       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> +       status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
> +
> +       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE);
> +       status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
> +
> +       if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
> +           status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) {
> +               up->port.type = PORT_16750;
> +               up->capabilities |= UART_CAP_AFE;
> +               return;
> +       }

What I don't like is increasing quirks in the 8250_port. Can't you simply use FIXED_PORT facility?
Again, look how 8250_mid is written.

-- 
With Best Regards,
Andy Shevchenko