Re: [PATCH] serial: 8250_dw: Prefer SRBR in bogus RX timeout workaround if available

[Andy Shevchenko]

On Mon, Jun 29, 2026 at 06:56:28PM +0300, Ilpo Järvinen wrote:
> On Mon, 29 Jun 2026, Yicong Yang wrote:

...

> > +			if (d->data.shadow_support)
> > +				serial_port_in(p, DW_UART_SRBR_0);
> > +			else
> > +				serial_port_in(p, UART_RX);
> 
> How about:
> 			serial_port_in(p, d->data.shadow_support ? DW_UART_SRBR_0
> 								 : UART_RX);

I was thinking of even just having the value of the register offset somewhere
in dw8250_data and call this unconditionally.

			serial_port_in(p, d->srbr);

Of course this has to be carefully initialised in time with UART_RX.

...

> >  /* Offsets for the DesignWare specific registers */
> > +#define DW_UART_SRBR_0	0x0c /* Shadow Receive Buffer Register */
> >  #define DW_UART_USR	0x1f /* UART Status Register */
> 
> It seems USR and now SRBR_0 are without regshift whereas the reset of 
> the register defines are with it.
> 
> Somewhat unrelated to this patch, I really hate this 
> dw8250_readl/writel_ext() mess even more now... Why are those needed 
> anyway, should the .serial_in/out callbacks handle those byte-order, etc. 
> variations just fine?

Aren't there is a possibility to have the register stride for the first ones as
1 for backward compatibility with 16550, while providing the features like CPR
and UCV that needs a 32-bit accessors? IIRC that was the reason for _ext() IO.

> Are those _ext calls only required for some early things during probe?
> I guess 8250_lpss hasn't setup the callbacks yet before calling 
> dw8250_setup_port() (I'm not even sure where it gets set with it after 
> looking for it for a few minutes)?


-- 
With Best Regards,
Andy Shevchenko