diff mbox series

[v2,2/4] serial: 8250_dw: Only accept a contiguous DLF readback

Message ID a79114e72d029732db6764935dde12aa5d497706.1792439398.git.vinicius.eduardo.pedrosa@gmail.com (mailing list archive)
State New
Headers
Series serial: 8250_dw: Allwinner A733 UART support |

Commit Message

Vinicius Pedrosa Oct. 19, 2026, 8:06 p.m. UTC 
dw8250_setup_port() writes all ones to offset 0xc0 and treats any
nonzero readback as the DLF register. DLF is a bit field starting at
bit 0, so a real one reads back as a contiguous 2^n - 1 mask. On some
Allwinner SoCs offset 0xc0 is the RS485 control register instead
(UART_485_CTL in the A733 User Manual), whose implemented bits are not
contiguous. The probe still takes its readback for a DLF of fls(readback)
bits, and every later divisor change writes the fractional divisor into
the RS485 control register.

Only accept a readback of the form 2^n - 1.

The probe itself is unchanged: it still writes all ones to offset 0xc0
and then restores the old value, so this only changes how the readback
is interpreted.

The A733 reads back 0x19f, and Andre Przywara observed 0x9f on the
A523. Without this check the A733 RS485 control register reads 0xb after
boot; with it, the register stays 0 across 300 termios toggle and
restore cycles.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
---
 drivers/tty/serial/8250/8250_dwlib.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jiri Slaby Oct. 20, 2026, 6:11 a.m. UTC | #1 
On 19. 10. 26, 22:06, Vinicius Pedrosa wrote:
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -225,7 +225,11 @@ void dw8250_setup_port(struct uart_port *p)
>   	reg = dw8250_readl_ext(p, DW_UART_DLF);
>   	dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
>   
> -	if (reg) {
> +	/*
> +	 * DLF is a bit field starting at bit 0, so a real one reads back as
> +	 * 2^n - 1. Any other value means this offset is something else.
> +	 */
> +	if (reg && !(reg & (reg + 1))) {

Do I understand correctly, that this equivalent to:
   if (is_power_of_2(reg + 1))
?

thanks,
Andy Shevchenko Oct. 20, 2026, 8:31 a.m. UTC | #2 
On Tue, Oct 20, 2026 at 08:11:06AM +0200, Jiri Slaby wrote:
> On 19. 10. 26, 22:06, Vinicius Pedrosa wrote:

...

> > -	if (reg) {
> > +	/*
> > +	 * DLF is a bit field starting at bit 0, so a real one reads back as
> > +	 * 2^n - 1. Any other value means this offset is something else.
> > +	 */
> > +	if (reg && !(reg & (reg + 1))) {
> 
> Do I understand correctly, that this equivalent to:
>   if (is_power_of_2(reg + 1))
> ?

Yes, and since we check for reg to be "not zero", this will work the rest.
Vinicius Pedrosa Oct. 20, 2026, 10:47 a.m. UTC | #3 
Hi Jiri,

On Tue, 20 Oct 2026, Jiri Slaby wrote:

> Do I understand correctly, that this equivalent to:
>   if (is_power_of_2(reg + 1))
> ?

Yes, given the existing reg != 0 check. I kept the explicit form in v2
to keep the change minimal, but I'll use

  reg && is_power_of_2(reg + 1)

in v3, it reads better.

The two forms only differ for U32_MAX, where reg + 1 wraps to 0. But that
isn't a valid DLF readback here.

Thanks,
Vinicius
Ilpo Järvinen Oct. 20, 2026, 11:50 a.m. UTC | #4 
On Tue, 20 Oct 2026, Andy Shevchenko wrote:

> On Tue, Oct 20, 2026 at 08:11:06AM +0200, Jiri Slaby wrote:
> > On 19. 10. 26, 22:06, Vinicius Pedrosa wrote:
> 
> ...
> 
> > > -	if (reg) {
> > > +	/*
> > > +	 * DLF is a bit field starting at bit 0, so a real one reads back as
> > > +	 * 2^n - 1. Any other value means this offset is something else.
> > > +	 */
> > > +	if (reg && !(reg & (reg + 1))) {
> > 
> > Do I understand correctly, that this equivalent to:
> >   if (is_power_of_2(reg + 1))
> > ?
> 
> Yes, and since we check for reg to be "not zero", this will work the rest.

For is_power_of_2(), 0 is not a power of two. And it also wouldn't make 
sense to have zero bits in the bit field as it's same as no bit field so 
using just is_power_of_2() should be enough.

It could be that's what Andy meant, but the wording left too much room 
for interpretation.
Vinicius Pedrosa Oct. 20, 2026, 12:27 p.m. UTC | #5 
Hi Ilpo,

On Tue, 20 Oct 2026, Ilpo Järvinen wrote:

> For is_power_of_2(), 0 is not a power of two. And it also wouldn't make
> sense to have zero bits in the bit field as it's same as no bit field so
> using just is_power_of_2() should be enough.

I think the reg != 0 check still needs to stay here, since the test is
is_power_of_2(reg + 1), rather than is_power_of_2(reg).

For reg == 0, reg + 1 is 1, and is_power_of_2(1) is true. So without
the explicit reg check, a zero readback, meaning no DLF, would be
accepted as a valid DLF.

I'll keep it as:

    reg && is_power_of_2(reg + 1)

in v3.

Thanks,
Vinicius
Ilpo Järvinen Oct. 20, 2026, 1:02 p.m. UTC | #6 
On Tue, 20 Oct 2026, Vinicius Pedrosa wrote:

> Hi Ilpo,
> 
> On Tue, 20 Oct 2026, Ilpo Järvinen wrote:
> 
> > For is_power_of_2(), 0 is not a power of two. And it also wouldn't make
> > sense to have zero bits in the bit field as it's same as no bit field so
> > using just is_power_of_2() should be enough.
> 
> I think the reg != 0 check still needs to stay here, since the test is
> is_power_of_2(reg + 1), rather than is_power_of_2(reg).
> 
> For reg == 0, reg + 1 is 1, and is_power_of_2(1) is true. So without
> the explicit reg check, a zero readback, meaning no DLF, would be
> accepted as a valid DLF.
> 
> I'll keep it as:
> 
>     reg && is_power_of_2(reg + 1)
> 
> in v3.

Ah, you're right.
Andy Shevchenko Oct. 21, 2026, 6:43 a.m. UTC | #7 
On Tue, Oct 20, 2026 at 04:02:29PM +0300, Ilpo Järvinen wrote:
> On Tue, 20 Oct 2026, Vinicius Pedrosa wrote:
> > On Tue, 20 Oct 2026, Ilpo Järvinen wrote:
> > 
> > > For is_power_of_2(), 0 is not a power of two. And it also wouldn't make
> > > sense to have zero bits in the bit field as it's same as no bit field so
> > > using just is_power_of_2() should be enough.
> > 
> > I think the reg != 0 check still needs to stay here, since the test is
> > is_power_of_2(reg + 1), rather than is_power_of_2(reg).
> > 
> > For reg == 0, reg + 1 is 1, and is_power_of_2(1) is true. So without
> > the explicit reg check, a zero readback, meaning no DLF, would be
> > accepted as a valid DLF.
> > 
> > I'll keep it as:
> > 
> >     reg && is_power_of_2(reg + 1)
> > 
> > in v3.
> 
> Ah, you're right.

Sorry for being unclear, yes, that's what I meant, reg != 0 should stay.
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 325793f20256..1b0192efe162 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -225,7 +225,11 @@  void dw8250_setup_port(struct uart_port *p)
 	reg = dw8250_readl_ext(p, DW_UART_DLF);
 	dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
 
-	if (reg) {
+	/*
+	 * DLF is a bit field starting at bit 0, so a real one reads back as
+	 * 2^n - 1. Any other value means this offset is something else.
+	 */
+	if (reg && !(reg & (reg + 1))) {
 		pd->dlf_size = fls(reg);
 		p->get_divisor = dw8250_get_divisor;
 		/* Keep a hook the driver has already installed */