[PATCH] serial: dw8250: Make DLF feature independent of ADDITIONAL_FEATURE

[PATCH] serial: dw8250: Make DLF feature independent of ADDITIONAL_FEATURE

[Dawei Li]

DW apb uart databook defines couples of configuration parameters of
dw8250 IP, among which there are 2 of them:
- ADDTIONAL_FEATURE
" Configure the peripheral to have the option to include FIFO status
  registers, shadow registers and encoded parameter register. Also
  configures the peripheral to have UART component version and the
  peripheral ID registers. "

- FRACTIONAL_BAUD_DIVISOR_EN
" Configures the peripheral to have Fractional Baud Rate Divisor.
  .... "

These two parameters are completely irrelevant, and supposed to be
independent of each other. However, in current dw8250 driver
implementation, they are hooked together.

The bug was hit when we are bringing up dw8250 IP on our hardware
platform, in which parameters are configured in such combination:
- ADDTIONAL_FEATURE disabled;
- FRACTIONAL_BAUD_DIVISOR_EN enabled;

Fixes: 701c5e73b296 ("serial: 8250_dw: add fractional divisor support")
Cc: stable@kernel.org
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
 drivers/tty/serial/8250/8250_dwlib.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 84843e204a5e..136ad093c5b6 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -259,17 +259,6 @@ void dw8250_setup_port(struct uart_port *p)
 	}
 	up->capabilities |= UART_CAP_NOTEMT;
 
-	/*
-	 * If the Component Version Register returns zero, we know that
-	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
-	 */
-	reg = dw8250_readl_ext(p, DW_UART_UCV);
-	if (!reg)
-		return;
-
-	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
-		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
-
 	/* Preserve value written by firmware or bootloader  */
 	old_dlf = dw8250_readl_ext(p, DW_UART_DLF);
 	dw8250_writel_ext(p, DW_UART_DLF, ~0U);
@@ -282,6 +271,17 @@ void dw8250_setup_port(struct uart_port *p)
 		p->set_divisor = dw8250_set_divisor;
 	}
 
+	/*
+	 * If the Component Version Register returns zero, we know that
+	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
+	 */
+	reg = dw8250_readl_ext(p, DW_UART_UCV);
+	if (!reg)
+		return;
+
+	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
+		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
+
 	reg = dw8250_readl_ext(p, DW_UART_CPR);
 	if (!reg) {
 		reg = data->pdata->cpr_val;
-- 
2.27.0

Re: [PATCH] serial: dw8250: Make DLF feature independent of ADDITIONAL_FEATURE

[Ilpo Järvinen]

On Mon, 4 Dec 2023, Dawei Li wrote:

> DW apb uart databook defines couples of configuration parameters of
> dw8250 IP, among which there are 2 of them:
> - ADDTIONAL_FEATURE
> " Configure the peripheral to have the option to include FIFO status
>   registers, shadow registers and encoded parameter register. Also
>   configures the peripheral to have UART component version and the
>   peripheral ID registers. "
> 
> - FRACTIONAL_BAUD_DIVISOR_EN
> " Configures the peripheral to have Fractional Baud Rate Divisor.
>   .... "
> 
> These two parameters are completely irrelevant, and supposed to be
> independent of each other. However, in current dw8250 driver
> implementation, they are hooked together.
> 
> The bug was hit when we are bringing up dw8250 IP on our hardware
> platform, in which parameters are configured in such combination:
> - ADDTIONAL_FEATURE disabled;
> - FRACTIONAL_BAUD_DIVISOR_EN enabled;
> 
> Fixes: 701c5e73b296 ("serial: 8250_dw: add fractional divisor support")
> Cc: stable@kernel.org
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
>  drivers/tty/serial/8250/8250_dwlib.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
> index 84843e204a5e..136ad093c5b6 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -259,17 +259,6 @@ void dw8250_setup_port(struct uart_port *p)
>  	}
>  	up->capabilities |= UART_CAP_NOTEMT;
>  
> -	/*
> -	 * If the Component Version Register returns zero, we know that
> -	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
> -	 */
> -	reg = dw8250_readl_ext(p, DW_UART_UCV);
> -	if (!reg)
> -		return;
> -
> -	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
> -		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
> -
>  	/* Preserve value written by firmware or bootloader  */
>  	old_dlf = dw8250_readl_ext(p, DW_UART_DLF);
>  	dw8250_writel_ext(p, DW_UART_DLF, ~0U);
> @@ -282,6 +271,17 @@ void dw8250_setup_port(struct uart_port *p)
>  		p->set_divisor = dw8250_set_divisor;
>  	}
>  
> +	/*
> +	 * If the Component Version Register returns zero, we know that
> +	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
> +	 */
> +	reg = dw8250_readl_ext(p, DW_UART_UCV);
> +	if (!reg)
> +		return;
> +
> +	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
> +		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
> +
>  	reg = dw8250_readl_ext(p, DW_UART_CPR);
>  	if (!reg) {
>  		reg = data->pdata->cpr_val;
> 

The very same code change is already in tty-next (from another author).

-- 
 i.

Re: [PATCH] serial: dw8250: Make DLF feature independent of ADDITIONAL_FEATURE

[Andy Shevchenko]

On Mon, Dec 04, 2023 at 03:20:09PM +0200, Ilpo Järvinen wrote:
> On Mon, 4 Dec 2023, Dawei Li wrote:
> 
> > DW apb uart databook defines couples of configuration parameters of

DW_apb_uart (as it's part of file name, or spell this fully).

> > dw8250 IP, among which there are 2 of them:

DesignWare 8250 IP

...

> > The bug was hit when we are bringing up dw8250 IP on our hardware

Ditto.

> > platform, in which parameters are configured in such combination:
> > - ADDTIONAL_FEATURE disabled;
> > - FRACTIONAL_BAUD_DIVISOR_EN enabled;

...

> The very same code change is already in tty-next (from another author).

For your convenience:

d804987153e7 ("serial: 8250_dw: Decouple DLF register check from UCV")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: dw8250: Make DLF feature independent of ADDITIONAL_FEATURE

[Dawei Li]

Hi Ilpo, Andy,

On Mon, Dec 04, 2023 at 03:29:35PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 04, 2023 at 03:20:09PM +0200, Ilpo Järvinen wrote:
> > On Mon, 4 Dec 2023, Dawei Li wrote:
> > 
> > > DW apb uart databook defines couples of configuration parameters of
> 
> DW_apb_uart (as it's part of file name, or spell this fully).
> 
> > > dw8250 IP, among which there are 2 of them:
> 
> DesignWare 8250 IP
> 
> ..
> 
> > > The bug was hit when we are bringing up dw8250 IP on our hardware
> 
> Ditto.
> 
> > > platform, in which parameters are configured in such combination:
> > > - ADDTIONAL_FEATURE disabled;
> > > - FRACTIONAL_BAUD_DIVISOR_EN enabled;
> 
> ..
> 
> > The very same code change is already in tty-next (from another author).
> 
> For your convenience:
> 
> d804987153e7 ("serial: 8250_dw: Decouple DLF register check from UCV")

[sigh]

Apparently I thought tty/for-linus is the "bug fixing" branch for tty by
mistake, and didn't realize the same fix patch has been landed in tty-next
for a while.

Thanks for the update.

Anyway, I believe the fixing patch should cc stable and be getting backported?
I hit the bug on 5.10.x, it's possible the bug has been there before that.

Thanks,
   dawei

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH] serial: dw8250: Make DLF feature independent of ADDITIONAL_FEATURE

[Andy Shevchenko]

On Tue, Dec 05, 2023 at 09:55:44AM +0800, Dawei Li wrote:
> On Mon, Dec 04, 2023 at 03:29:35PM +0200, Andy Shevchenko wrote:
> > On Mon, Dec 04, 2023 at 03:20:09PM +0200, Ilpo Järvinen wrote:
> > > On Mon, 4 Dec 2023, Dawei Li wrote:

...

> > > The very same code change is already in tty-next (from another author).
> > 
> > For your convenience:
> > 
> > d804987153e7 ("serial: 8250_dw: Decouple DLF register check from UCV")
> 
> [sigh]
> 
> Apparently I thought tty/for-linus is the "bug fixing" branch for tty by
> mistake, and didn't realize the same fix patch has been landed in tty-next
> for a while.
> 
> Thanks for the update.
> 
> Anyway, I believe the fixing patch should cc stable and be getting backported?
> I hit the bug on 5.10.x, it's possible the bug has been there before that.

When that commit meets upstream you can always resend it for stable. The process
is documented.

-- 
With Best Regards,
Andy Shevchenko