serial8250: can not change baudrate while the controller is busy

serial8250: can not change baudrate while the controller is busy

[qianfan]

Hi:

My custom board is based on allwinner R40, the uart is compatibled with
serial8250. Based on it's datasheet:

 > When TX transmit data, or RX receives data, or TX FIFO is not empty, 
then the
 > BUSY flag bit can be set to 1 by hardware, which indicates the UART
 > controller is busy.

We cannot write LCR and DLL to update UART params such as baudrate and 
partity
while the UART is busy, however `serial8250_do_set_termios` is a void 
function,
the upper level always assume the uart params is updated.

The upper level `uart_set_termios` do noting if ktermios params is not 
changed,
it will not update when the user space program running tcsetattr set a same
baudrate again. So we can not fix the baudrate when 
`serial8250_do_set_termios`
failed.

Allwinner R40's datasheet provided a way for this case.

 > CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also 
set UART
 > controller when UART is busy, such as the LCR, DLH, DLL register.
 > CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and 
CHANGE_UPDATE
 > is written to 1, the configuration of UART controller can be updated.
 > After completed update, the bit is cleared to 0 automatically.

I can't know this feature is expanded by allwinner, or it is a common 
functiton
of serial8250. Perhaps the serial8250 driver need this.

Thanks

Re: serial8250: can not change baudrate while the controller is busy

[Ilpo Järvinen]

On Fri, 14 Apr 2023, qianfan wrote:

> Hi:
> 
> My custom board is based on allwinner R40, the uart is compatibled with
> serial8250. Based on it's datasheet:
> 
> > When TX transmit data, or RX receives data, or TX FIFO is not empty, then
> the
> > BUSY flag bit can be set to 1 by hardware, which indicates the UART
> > controller is busy.
> 
> We cannot write LCR and DLL to update UART params such as baudrate and partity
> while the UART is busy, however `serial8250_do_set_termios` is a void
> function,
> the upper level always assume the uart params is updated.
> 
> The upper level `uart_set_termios` do noting if ktermios params is not
> changed,
> it will not update when the user space program running tcsetattr set a same
> baudrate again.
>
> So we can not fix the baudrate when
> `serial8250_do_set_termios`
> failed.
> 
> Allwinner R40's datasheet provided a way for this case.
> 
> > CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also set UART
> > controller when UART is busy, such as the LCR, DLH, DLL register.
> > CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and CHANGE_UPDATE
> > is written to 1, the configuration of UART controller can be updated.
> > After completed update, the bit is cleared to 0 automatically.
> 
> I can't know this feature is expanded by allwinner, or it is a common
> functiton
> of serial8250. Perhaps the serial8250 driver need this.

tcsetattr() can be given a flag which enforces TX empty condition before 
core calls into the lower layer HW set_termios function. Would that be 
enough to solve the case you're interested in?

Obviously, nothing can prevent Rx from occuring as it's not under local 
UART's control (e.g. a busy flag check would still be racy). But does 
writing those registers actually break something or just corrupts the 
character under Tx/Rx (which can be handled by flushing)?

-- 
 i.

Re: serial8250: can not change baudrate while the controller is busy

[qianfan]

在 2023/4/14 20:10, Ilpo Järvinen 写道:
> On Fri, 14 Apr 2023, qianfan wrote:
>
>> Hi:
>>
>> My custom board is based on allwinner R40, the uart is compatibled with
>> serial8250. Based on it's datasheet:
>>
>>> When TX transmit data, or RX receives data, or TX FIFO is not empty, then
>> the
>>> BUSY flag bit can be set to 1 by hardware, which indicates the UART
>>> controller is busy.
>> We cannot write LCR and DLL to update UART params such as baudrate and partity
>> while the UART is busy, however `serial8250_do_set_termios` is a void
>> function,
>> the upper level always assume the uart params is updated.
>>
>> The upper level `uart_set_termios` do noting if ktermios params is not
>> changed,
>> it will not update when the user space program running tcsetattr set a same
>> baudrate again.
>>
>> So we can not fix the baudrate when
>> `serial8250_do_set_termios`
>> failed.
>>
>> Allwinner R40's datasheet provided a way for this case.
>>
>>> CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also set UART
>>> controller when UART is busy, such as the LCR, DLH, DLL register.
>>> CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and CHANGE_UPDATE
>>> is written to 1, the configuration of UART controller can be updated.
>>> After completed update, the bit is cleared to 0 automatically.
>> I can't know this feature is expanded by allwinner, or it is a common
>> functiton
>> of serial8250. Perhaps the serial8250 driver need this.
> tcsetattr() can be given a flag which enforces TX empty condition before
> core calls into the lower layer HW set_termios function. Would that be
> enough to solve the case you're interested in?
>
> Obviously, nothing can prevent Rx from occuring as it's not under local
> UART's control (e.g. a busy flag check would still be racy). But does
> writing those registers actually break something or just corrupts the
> character under Tx/Rx (which can be handled by flushing)?
I think flushing can't resolv this problem. We can control the time to 
sending
but can not control when we are recving.

The RX busy is always exists.
>

Re: serial8250: can not change baudrate while the controller is busy

[qianfan]

在 2023/4/14 20:10, Ilpo Järvinen 写道:
> On Fri, 14 Apr 2023, qianfan wrote:
>
>> Hi:
>>
>> My custom board is based on allwinner R40, the uart is compatibled with
>> serial8250. Based on it's datasheet:
>>
>>> When TX transmit data, or RX receives data, or TX FIFO is not empty, then
>> the
>>> BUSY flag bit can be set to 1 by hardware, which indicates the UART
>>> controller is busy.
>> We cannot write LCR and DLL to update UART params such as baudrate and partity
>> while the UART is busy, however `serial8250_do_set_termios` is a void
>> function,
>> the upper level always assume the uart params is updated.
>>
>> The upper level `uart_set_termios` do noting if ktermios params is not
>> changed,
>> it will not update when the user space program running tcsetattr set a same
>> baudrate again.
>>
>> So we can not fix the baudrate when
>> `serial8250_do_set_termios`
>> failed.
>>
>> Allwinner R40's datasheet provided a way for this case.
>>
>>> CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also set UART
>>> controller when UART is busy, such as the LCR, DLH, DLL register.
>>> CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and CHANGE_UPDATE
>>> is written to 1, the configuration of UART controller can be updated.
>>> After completed update, the bit is cleared to 0 automatically.
>> I can't know this feature is expanded by allwinner, or it is a common
>> functiton
>> of serial8250. Perhaps the serial8250 driver need this.
> tcsetattr() can be given a flag which enforces TX empty condition before
> core calls into the lower layer HW set_termios function. Would that be
> enough to solve the case you're interested in?
>
> Obviously, nothing can prevent Rx from occuring as it's not under local
> UART's control (e.g. a busy flag check would still be racy). But does
> writing those registers actually break something or just corrupts the
> character under Tx/Rx (which can be handled by flushing)?
Hi:

I speed long times to create a common solution for this problem.

(I had create two commit, the first one add some sysfs debug interface
and the second one try solve this problem. So the next following patch
has only patch-2. Let's we discuess this solution and I will send all
patches if it is good.)

Allwinner introduce some bits in HALT_TX register which can change
baudrate while the serial is busy. But that is not a common feature
of dw-uart. Rockchip's uart is also based on dw-uart and they doesn't
has such feature.

The loopback is a common feature of 16450/16550 serial, so we can set
loopback mode to cut down the external serial line to force the serial
to idle.

Next is the second patch:

 From 171e981c3695e3efcc76a2c4f0d0937d366d6e2a Mon Sep 17 00:00:00 2001
From: qianfan Zhao <qianfanguijin@163.com>
Date: Fri, 5 May 2023 08:46:50 +0800
Subject: [PATCH] drivers: serial: 8250_dw: Make uart idle before set 
baudrate

Some registers which control the baudrate such as DLL, DLM can not
write while the uart is busy. So set the controller to loopback mode
and clear fifos to force idle before change baudrate.

Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
---
  drivers/tty/serial/8250/8250_dw.c | 57 ++++++++++++++++++++++++++++++-
  1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c 
b/drivers/tty/serial/8250/8250_dw.c
index 3dca344ca19c..4eaa4d05a43e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -35,6 +35,7 @@
  #define DW_UART_USR    0x1f /* UART Status Register */

  /* DesignWare specific register fields */
+#define DW_UART_USR_BUSY        BIT(0)
  #define DW_UART_MCR_SIRE        BIT(6)

  struct dw8250_data {
@@ -43,6 +44,8 @@ struct dw8250_data {
      u8            usr_reg;
      int            msr_mask_on;
      int            msr_mask_off;
+    u8            dll;
+    u8            dlm;
      struct clk        *clk;
      struct clk        *pclk;
      struct notifier_block    clk_notifier;
@@ -52,7 +55,9 @@ struct dw8250_data {
      unsigned int        skip_autocfg:1;
      unsigned int        uart_16550_compatible:1;

+    unsigned int        last_loopback_waiting_time;
      unsigned long        iir_busy_count;
+    unsigned long        lcr_busy_count;
  };

  static inline struct dw8250_data *to_dw8250_data(struct 
dw8250_port_data *data)
@@ -93,6 +98,7 @@ static void dw8250_force_idle(struct uart_port *p)

  static void dw8250_check_lcr(struct uart_port *p, int value)
  {
+    struct dw8250_data *d = to_dw8250_data(p->private_data);
      void __iomem *offset = p->membase + (UART_LCR << p->regshift);
      int tries = 1000;

@@ -121,6 +127,7 @@ static void dw8250_check_lcr(struct uart_port *p, 
int value)
       * FIXME: this deadlocks if port->lock is already held
       * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
       */
+    d->lcr_busy_count++;
  }

  /* Returns once the transmitter is empty or we run out of retries */
@@ -360,6 +367,46 @@ static void dw8250_set_termios(struct uart_port *p, 
struct ktermios *termios,
      serial8250_do_set_termios(p, termios, old);
  }

+static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
+                   unsigned int quot, unsigned int quot_frac)
+{
+    struct uart_8250_port *up = up_to_u8250p(p);
+    struct dw8250_data *d = to_dw8250_data(p->private_data);
+    unsigned int usr;
+    int retries;
+
+    /*
+     * LCR, DLL, DLM registers can not write while the uart is busy,
+     * set uart to loopback mode, clear fifos to force idle.
+     * The loopback mode doesn't take effect immediately, it will waiting
+     * current byte received done, the lower baudrate the longer waiting
+     * time.
+     */
+    p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
+    for (retries = 0; retries < 10000; retries++) {
+        dw8250_force_idle(p);
+
+        usr = p->serial_in(p, d->usr_reg);
+        if (!(usr & DW_UART_USR_BUSY))
+            break;
+        udelay(1);
+    }
+
+    d->last_loopback_waiting_time = retries;
+
+    p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
+    if (p->serial_in(p, UART_LCR) & UART_LCR_DLAB) {
+        d->dll = quot & 0xff;
+        d->dlm = (quot >> 8) & 0xff;
+
+        p->serial_out(p, UART_DLL, d->dll);
+        p->serial_out(p, UART_DLM, d->dlm);
+        p->serial_out(p, UART_LCR, up->lcr);
+    }
+
+    p->serial_out(p, UART_MCR, up->mcr);
+}
+
  static void dw8250_set_ldisc(struct uart_port *p, struct ktermios 
*termios)
  {
      struct uart_8250_port *up = up_to_u8250p(p);
@@ -449,6 +496,8 @@ static ssize_t register_show(struct device *dev, 
struct device_attribute *attr,

      spin_lock_irqsave(&p->lock, flags);
      ret = scnprintf(buf, PAGE_SIZE,
+            "%s: %02x\n"
+            "%s: %02x\n"
              "%s: %02x/%02x\n"
              "%s: %02x\n"
              "%s: %02x\n"
@@ -458,6 +507,8 @@ static ssize_t register_show(struct device *dev, 
struct device_attribute *attr,
              "%s: %02x\n"
              "%s: %02x\n"
              ,
+            "DLL", d->dll,
+            "DLM", d->dlm,
              "IER", up->ier, p->serial_in(p, UART_IER),
              "IIR", p->serial_in(p, UART_IIR),
              "FCR", up->fcr,
@@ -478,7 +529,10 @@ static ssize_t busy_show(struct device *dev, struct 
device_attribute *attr,
  {
      struct dw8250_data *d = platform_get_drvdata(to_platform_device(dev));

-    return scnprintf(buf, PAGE_SIZE, "%lu\n", d->iir_busy_count);
+    return scnprintf(buf, PAGE_SIZE, "%lu %lu %u\n",
+             d->iir_busy_count,
+             d->lcr_busy_count,
+             d->last_loopback_waiting_time);
  }
  static DEVICE_ATTR_RO(busy);

@@ -570,6 +624,7 @@ static int dw8250_probe(struct platform_device *pdev)
      p->serial_out    = dw8250_serial_out;
      p->set_ldisc    = dw8250_set_ldisc;
      p->set_termios    = dw8250_set_termios;
+    p->set_divisor    = dw8250_set_divisor;

      p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
      if (!p->membase)
-- 
2.25.1

Re: serial8250: can not change baudrate while the controller is busy

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8033 bytes --]

On Fri, 5 May 2023, qianfan wrote:
> 在 2023/4/14 20:10, Ilpo Järvinen 写道:
> > On Fri, 14 Apr 2023, qianfan wrote:
> > > 
> > > My custom board is based on allwinner R40, the uart is compatibled with
> > > serial8250. Based on it's datasheet:
> > > 
> > > > When TX transmit data, or RX receives data, or TX FIFO is not empty,
> > > > then
> > > the
> > > > BUSY flag bit can be set to 1 by hardware, which indicates the UART
> > > > controller is busy.
> > > We cannot write LCR and DLL to update UART params such as baudrate and
> > > partity
> > > while the UART is busy, however `serial8250_do_set_termios` is a void
> > > function,
> > > the upper level always assume the uart params is updated.
> > > 
> > > The upper level `uart_set_termios` do noting if ktermios params is not
> > > changed,
> > > it will not update when the user space program running tcsetattr set a
> > > same
> > > baudrate again.
> > > 
> > > So we can not fix the baudrate when
> > > `serial8250_do_set_termios`
> > > failed.
> > > 
> > > Allwinner R40's datasheet provided a way for this case.
> > > 
> > > > CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also set
> > > > UART
> > > > controller when UART is busy, such as the LCR, DLH, DLL register.
> > > > CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and
> > > > CHANGE_UPDATE
> > > > is written to 1, the configuration of UART controller can be updated.
> > > > After completed update, the bit is cleared to 0 automatically.
> > > I can't know this feature is expanded by allwinner, or it is a common
> > > functiton
> > > of serial8250. Perhaps the serial8250 driver need this.
> > tcsetattr() can be given a flag which enforces TX empty condition before
> > core calls into the lower layer HW set_termios function. Would that be
> > enough to solve the case you're interested in?
> > 
> > Obviously, nothing can prevent Rx from occuring as it's not under local
> > UART's control (e.g. a busy flag check would still be racy). But does
> > writing those registers actually break something or just corrupts the
> > character under Tx/Rx (which can be handled by flushing)?
> Hi:
> 
> I speed long times to create a common solution for this problem.
> 
> (I had create two commit, the first one add some sysfs debug interface
> and the second one try solve this problem. So the next following patch
> has only patch-2. Let's we discuess this solution and I will send all
> patches if it is good.)

Thanks a lot, it's much easier to discuss now with something concrete at 
hand.

> Allwinner introduce some bits in HALT_TX register which can change
> baudrate while the serial is busy. But that is not a common feature
> of dw-uart. Rockchip's uart is also based on dw-uart and they doesn't
> has such feature.
> 
> The loopback is a common feature of 16450/16550 serial, so we can set
> loopback mode to cut down the external serial line to force the serial
> to idle.
> 
> Next is the second patch:
> 
> From 171e981c3695e3efcc76a2c4f0d0937d366d6e2a Mon Sep 17 00:00:00 2001
> From: qianfan Zhao <qianfanguijin@163.com>
> Date: Fri, 5 May 2023 08:46:50 +0800
> Subject: [PATCH] drivers: serial: 8250_dw: Make uart idle before set baudrate
> 
> Some registers which control the baudrate such as DLL, DLM can not
> write while the uart is busy. So set the controller to loopback mode
> and clear fifos to force idle before change baudrate.
> 
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 57 ++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index 3dca344ca19c..4eaa4d05a43e 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -35,6 +35,7 @@
>  #define DW_UART_USR    0x1f /* UART Status Register */
> 
>  /* DesignWare specific register fields */
> +#define DW_UART_USR_BUSY        BIT(0)
>  #define DW_UART_MCR_SIRE        BIT(6)
> 
>  struct dw8250_data {
> @@ -43,6 +44,8 @@ struct dw8250_data {
>      u8            usr_reg;
>      int            msr_mask_on;
>      int            msr_mask_off;
> +    u8            dll;
> +    u8            dlm;

In general, there's something wrong with the formatting of your patch, 
something corrupted tabs. You should fix that before doing any official 
submission.

>      struct clk        *clk;
>      struct clk        *pclk;
>      struct notifier_block    clk_notifier;
> @@ -52,7 +55,9 @@ struct dw8250_data {
>      unsigned int        skip_autocfg:1;
>      unsigned int        uart_16550_compatible:1;
> 
> +    unsigned int        last_loopback_waiting_time;
>      unsigned long        iir_busy_count;
> +    unsigned long        lcr_busy_count;
>  };
> 
>  static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data
> *data)
> @@ -93,6 +98,7 @@ static void dw8250_force_idle(struct uart_port *p)
> 
>  static void dw8250_check_lcr(struct uart_port *p, int value)
>  {
> +    struct dw8250_data *d = to_dw8250_data(p->private_data);
>      void __iomem *offset = p->membase + (UART_LCR << p->regshift);
>      int tries = 1000;
> 
> @@ -121,6 +127,7 @@ static void dw8250_check_lcr(struct uart_port *p, int
> value)
>       * FIXME: this deadlocks if port->lock is already held
>       * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
>       */
> +    d->lcr_busy_count++;
>  }
> 
>  /* Returns once the transmitter is empty or we run out of retries */
> @@ -360,6 +367,46 @@ static void dw8250_set_termios(struct uart_port *p,
> struct ktermios *termios,
>      serial8250_do_set_termios(p, termios, old);
>  }
> 
> +static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
> +                   unsigned int quot, unsigned int quot_frac)
> +{
> +    struct uart_8250_port *up = up_to_u8250p(p);
> +    struct dw8250_data *d = to_dw8250_data(p->private_data);
> +    unsigned int usr;
> +    int retries;
> +
> +    /*
> +     * LCR, DLL, DLM registers can not write while the uart is busy,

According to DW databook, this is not entirely true. The databook 
explicitly states that if BUSY is not configured 
(UART_16550_COMPATIBLE=YES), those are always writable. And I know for 
sure that there are devices on the field do not come with BUSY.
Thus, it looks something that should be decided based on BUSY 
availability.

I had one time a patch which generalized uart_16550_compatible to 
struct dw8250_port_data but in the end I didn't need it.

> +     * set uart to loopback mode, clear fifos to force idle.
> +     * The loopback mode doesn't take effect immediately, it will waiting
> +     * current byte received done, the lower baudrate the longer waiting
> +     * time.
> +     */
> +    p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> +    for (retries = 0; retries < 10000; retries++) {
> +        dw8250_force_idle(p);
> +
> +        usr = p->serial_in(p, d->usr_reg);
> +        if (!(usr & DW_UART_USR_BUSY))
> +            break;
> +        udelay(1);
> +    }

This loop is overkill, ndelay(p->frame_time) is all you need to wait for
the maximum time a single frame needs.

> +    d->last_loopback_waiting_time = retries;
> +
> +    p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> +    if (p->serial_in(p, UART_LCR) & UART_LCR_DLAB) {

Can this still fail? Why?

> +        d->dll = quot & 0xff;
> +        d->dlm = (quot >> 8) & 0xff;
> +
> +        p->serial_out(p, UART_DLL, d->dll);
> +        p->serial_out(p, UART_DLM, d->dlm);

serial_dl_write()

> +        p->serial_out(p, UART_LCR, up->lcr);
> +    }
> +
> +    p->serial_out(p, UART_MCR, up->mcr);
> +}
> +

-- 
 i.

Re: serial8250: can not change baudrate while the controller is busy

[qianfan]

在 2023/5/8 18:17, Ilpo Järvinen 写道:
> On Fri, 5 May 2023, qianfan wrote:
>> 在 2023/4/14 20:10, Ilpo Järvinen 写道:
>>> On Fri, 14 Apr 2023, qianfan wrote:
>>>> My custom board is based on allwinner R40, the uart is compatibled with
>>>> serial8250. Based on it's datasheet:
>>>>
>>>>> When TX transmit data, or RX receives data, or TX FIFO is not empty,
>>>>> then
>>>> the
>>>>> BUSY flag bit can be set to 1 by hardware, which indicates the UART
>>>>> controller is busy.
>>>> We cannot write LCR and DLL to update UART params such as baudrate and
>>>> partity
>>>> while the UART is busy, however `serial8250_do_set_termios` is a void
>>>> function,
>>>> the upper level always assume the uart params is updated.
>>>>
>>>> The upper level `uart_set_termios` do noting if ktermios params is not
>>>> changed,
>>>> it will not update when the user space program running tcsetattr set a
>>>> same
>>>> baudrate again.
>>>>
>>>> So we can not fix the baudrate when
>>>> `serial8250_do_set_termios`
>>>> failed.
>>>>
>>>> Allwinner R40's datasheet provided a way for this case.
>>>>
>>>>> CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also set
>>>>> UART
>>>>> controller when UART is busy, such as the LCR, DLH, DLL register.
>>>>> CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and
>>>>> CHANGE_UPDATE
>>>>> is written to 1, the configuration of UART controller can be updated.
>>>>> After completed update, the bit is cleared to 0 automatically.
>>>> I can't know this feature is expanded by allwinner, or it is a common
>>>> functiton
>>>> of serial8250. Perhaps the serial8250 driver need this.
>>> tcsetattr() can be given a flag which enforces TX empty condition before
>>> core calls into the lower layer HW set_termios function. Would that be
>>> enough to solve the case you're interested in?
>>>
>>> Obviously, nothing can prevent Rx from occuring as it's not under local
>>> UART's control (e.g. a busy flag check would still be racy). But does
>>> writing those registers actually break something or just corrupts the
>>> character under Tx/Rx (which can be handled by flushing)?
>> Hi:
>>
>> I speed long times to create a common solution for this problem.
>>
>> (I had create two commit, the first one add some sysfs debug interface
>> and the second one try solve this problem. So the next following patch
>> has only patch-2. Let's we discuess this solution and I will send all
>> patches if it is good.)
> Thanks a lot, it's much easier to discuss now with something concrete at
> hand.
>
>> Allwinner introduce some bits in HALT_TX register which can change
>> baudrate while the serial is busy. But that is not a common feature
>> of dw-uart. Rockchip's uart is also based on dw-uart and they doesn't
>> has such feature.
>>
>> The loopback is a common feature of 16450/16550 serial, so we can set
>> loopback mode to cut down the external serial line to force the serial
>> to idle.
>>
>> Next is the second patch:
>>
>>  From 171e981c3695e3efcc76a2c4f0d0937d366d6e2a Mon Sep 17 00:00:00 2001
>> From: qianfan Zhao <qianfanguijin@163.com>
>> Date: Fri, 5 May 2023 08:46:50 +0800
>> Subject: [PATCH] drivers: serial: 8250_dw: Make uart idle before set baudrate
>>
>> Some registers which control the baudrate such as DLL, DLM can not
>> write while the uart is busy. So set the controller to loopback mode
>> and clear fifos to force idle before change baudrate.
>>
>> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
>> ---
>>   drivers/tty/serial/8250/8250_dw.c | 57 ++++++++++++++++++++++++++++++-
>>   1 file changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>> b/drivers/tty/serial/8250/8250_dw.c
>> index 3dca344ca19c..4eaa4d05a43e 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -35,6 +35,7 @@
>>   #define DW_UART_USR    0x1f /* UART Status Register */
>>
>>   /* DesignWare specific register fields */
>> +#define DW_UART_USR_BUSY        BIT(0)
>>   #define DW_UART_MCR_SIRE        BIT(6)
>>
>>   struct dw8250_data {
>> @@ -43,6 +44,8 @@ struct dw8250_data {
>>       u8            usr_reg;
>>       int            msr_mask_on;
>>       int            msr_mask_off;
>> +    u8            dll;
>> +    u8            dlm;
> In general, there's something wrong with the formatting of your patch,
> something corrupted tabs. You should fix that before doing any official
> submission.
OK, I will check it on the next patches.
>
>>       struct clk        *clk;
>>       struct clk        *pclk;
>>       struct notifier_block    clk_notifier;
>> @@ -52,7 +55,9 @@ struct dw8250_data {
>>       unsigned int        skip_autocfg:1;
>>       unsigned int        uart_16550_compatible:1;
>>
>> +    unsigned int        last_loopback_waiting_time;
>>       unsigned long        iir_busy_count;
>> +    unsigned long        lcr_busy_count;
>>   };
>>
>>   static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data
>> *data)
>> @@ -93,6 +98,7 @@ static void dw8250_force_idle(struct uart_port *p)
>>
>>   static void dw8250_check_lcr(struct uart_port *p, int value)
>>   {
>> +    struct dw8250_data *d = to_dw8250_data(p->private_data);
>>       void __iomem *offset = p->membase + (UART_LCR << p->regshift);
>>       int tries = 1000;
>>
>> @@ -121,6 +127,7 @@ static void dw8250_check_lcr(struct uart_port *p, int
>> value)
>>        * FIXME: this deadlocks if port->lock is already held
>>        * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
>>        */
>> +    d->lcr_busy_count++;
>>   }
>>
>>   /* Returns once the transmitter is empty or we run out of retries */
>> @@ -360,6 +367,46 @@ static void dw8250_set_termios(struct uart_port *p,
>> struct ktermios *termios,
>>       serial8250_do_set_termios(p, termios, old);
>>   }
>>
>> +static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
>> +                   unsigned int quot, unsigned int quot_frac)
>> +{
>> +    struct uart_8250_port *up = up_to_u8250p(p);
>> +    struct dw8250_data *d = to_dw8250_data(p->private_data);
>> +    unsigned int usr;
>> +    int retries;
>> +
>> +    /*
>> +     * LCR, DLL, DLM registers can not write while the uart is busy,
> According to DW databook, this is not entirely true. The databook
> explicitly states that if BUSY is not configured
> (UART_16550_COMPATIBLE=YES), those are always writable. And I know for
> sure that there are devices on the field do not come with BUSY.
> Thus, it looks something that should be decided based on BUSY
> availability.
>
> I had one time a patch which generalized uart_16550_compatible to
> struct dw8250_port_data but in the end I didn't need it.
Or we should registerdw8250_set_divisor callback only when 
!d->uart_16550_compatible
in probe function, that is a easy way to handle this.
>> +     * set uart to loopback mode, clear fifos to force idle.
>> +     * The loopback mode doesn't take effect immediately, it will waiting
>> +     * current byte received done, the lower baudrate the longer waiting
>> +     * time.
>> +     */
>> +    p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
>> +    for (retries = 0; retries < 10000; retries++) {
>> +        dw8250_force_idle(p);
>> +
>> +        usr = p->serial_in(p, d->usr_reg);
>> +        if (!(usr & DW_UART_USR_BUSY))
>> +            break;
>> +        udelay(1);
>> +    }
> This loop is overkill, ndelay(p->frame_time) is all you need to wait for
> the maximum time a single frame needs.
Sorry but I can not find the p->frame_time variable.
And the total waiting time is not a const value so we need polling.
>
>> +    d->last_loopback_waiting_time = retries;
>> +
>> +    p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
>> +    if (p->serial_in(p, UART_LCR) & UART_LCR_DLAB) {
> Can this still fail? Why?
If the waiting time is enough this should not fail.

But under my test before this patch, set UART_LCR register maybe failed 
due to busy,
if we write DLM without check DLAB bit, that will write data to UART_IER 
register,
different baudrate will write different value, that will cause some 
interrupt disabled
and cause strange problem.
>
>> +        d->dll = quot & 0xff;
>> +        d->dlm = (quot >> 8) & 0xff;
>> +
>> +        p->serial_out(p, UART_DLL, d->dll);
>> +        p->serial_out(p, UART_DLM, d->dlm);
> serial_dl_write()
OK
>
>> +        p->serial_out(p, UART_LCR, up->lcr);
>> +    }
>> +
>> +    p->serial_out(p, UART_MCR, up->mcr);
>> +}
>> +

Re: serial8250: can not change baudrate while the controller is busy

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8849 bytes --]

On Mon, 8 May 2023, qianfan wrote:
> 在 2023/5/8 18:17, Ilpo Järvinen 写道:
> > On Fri, 5 May 2023, qianfan wrote:
> > > 在 2023/4/14 20:10, Ilpo Järvinen 写道:
> > > > On Fri, 14 Apr 2023, qianfan wrote:
> > > > > My custom board is based on allwinner R40, the uart is compatibled
> > > > > with
> > > > > serial8250. Based on it's datasheet:
> > > > > 
> > > > > > When TX transmit data, or RX receives data, or TX FIFO is not empty,
> > > > > > then
> > > > > the
> > > > > > BUSY flag bit can be set to 1 by hardware, which indicates the UART
> > > > > > controller is busy.
> > > > > We cannot write LCR and DLL to update UART params such as baudrate and
> > > > > partity
> > > > > while the UART is busy, however `serial8250_do_set_termios` is a void
> > > > > function,
> > > > > the upper level always assume the uart params is updated.
> > > > > 
> > > > > The upper level `uart_set_termios` do noting if ktermios params is not
> > > > > changed,
> > > > > it will not update when the user space program running tcsetattr set a
> > > > > same
> > > > > baudrate again.
> > > > > 
> > > > > So we can not fix the baudrate when
> > > > > `serial8250_do_set_termios`
> > > > > failed.
> > > > > 
> > > > > Allwinner R40's datasheet provided a way for this case.
> > > > > 
> > > > > > CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also
> > > > > > set
> > > > > > UART
> > > > > > controller when UART is busy, such as the LCR, DLH, DLL register.
> > > > > > CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and
> > > > > > CHANGE_UPDATE
> > > > > > is written to 1, the configuration of UART controller can be
> > > > > > updated.
> > > > > > After completed update, the bit is cleared to 0 automatically.
> > > > > I can't know this feature is expanded by allwinner, or it is a common
> > > > > functiton
> > > > > of serial8250. Perhaps the serial8250 driver need this.
> > > > tcsetattr() can be given a flag which enforces TX empty condition before
> > > > core calls into the lower layer HW set_termios function. Would that be
> > > > enough to solve the case you're interested in?
> > > > 
> > > > Obviously, nothing can prevent Rx from occuring as it's not under local
> > > > UART's control (e.g. a busy flag check would still be racy). But does
> > > > writing those registers actually break something or just corrupts the
> > > > character under Tx/Rx (which can be handled by flushing)?
> > > Hi:
> > > 
> > > I speed long times to create a common solution for this problem.
> > > 
> > > (I had create two commit, the first one add some sysfs debug interface
> > > and the second one try solve this problem. So the next following patch
> > > has only patch-2. Let's we discuess this solution and I will send all
> > > patches if it is good.)
> > Thanks a lot, it's much easier to discuss now with something concrete at
> > hand.
> > 
> > > Allwinner introduce some bits in HALT_TX register which can change
> > > baudrate while the serial is busy. But that is not a common feature
> > > of dw-uart. Rockchip's uart is also based on dw-uart and they doesn't
> > > has such feature.
> > > 
> > > The loopback is a common feature of 16450/16550 serial, so we can set
> > > loopback mode to cut down the external serial line to force the serial
> > > to idle.
> > > 
> > > Next is the second patch:
> > > 
> > >  From 171e981c3695e3efcc76a2c4f0d0937d366d6e2a Mon Sep 17 00:00:00 2001
> > > From: qianfan Zhao <qianfanguijin@163.com>
> > > Date: Fri, 5 May 2023 08:46:50 +0800
> > > Subject: [PATCH] drivers: serial: 8250_dw: Make uart idle before set
> > > baudrate
> > > 
> > > Some registers which control the baudrate such as DLL, DLM can not
> > > write while the uart is busy. So set the controller to loopback mode
> > > and clear fifos to force idle before change baudrate.
> > > 
> > > Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> > > ---

> > > @@ -360,6 +367,46 @@ static void dw8250_set_termios(struct uart_port *p,
> > > struct ktermios *termios,
> > >       serial8250_do_set_termios(p, termios, old);
> > >   }
> > > 
> > > +static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
> > > +                   unsigned int quot, unsigned int quot_frac)
> > > +{
> > > +    struct uart_8250_port *up = up_to_u8250p(p);
> > > +    struct dw8250_data *d = to_dw8250_data(p->private_data);
> > > +    unsigned int usr;
> > > +    int retries;
> > > +
> > > +    /*
> > > +     * LCR, DLL, DLM registers can not write while the uart is busy,
> > According to DW databook, this is not entirely true. The databook
> > explicitly states that if BUSY is not configured
> > (UART_16550_COMPATIBLE=YES), those are always writable. And I know for
> > sure that there are devices on the field do not come with BUSY.
> > Thus, it looks something that should be decided based on BUSY
> > availability.
> > 
> > I had one time a patch which generalized uart_16550_compatible to
> > struct dw8250_port_data but in the end I didn't need it.
>
> Or we should register dw8250_set_divisor callback only when
> !d->uart_16550_compatible
> in probe function, that is a easy way to handle this.

Yes, registering it based on the compatible might makes sense, but please 
see my comment below wrt. dw8250_check_lcr().

> > > +     * set uart to loopback mode, clear fifos to force idle.
> > > +     * The loopback mode doesn't take effect immediately, it will waiting
> > > +     * current byte received done, the lower baudrate the longer waiting
> > > +     * time.
> > > +     */
> > > +    p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > > +    for (retries = 0; retries < 10000; retries++) {
> > > +        dw8250_force_idle(p);
> > > +
> > > +        usr = p->serial_in(p, d->usr_reg);
> > > +        if (!(usr & DW_UART_USR_BUSY))
> > > +            break;
> > > +        udelay(1);
> > > +    }
> > This loop is overkill, ndelay(p->frame_time) is all you need to wait for
> > the maximum time a single frame needs.
> Sorry but I can not find the p->frame_time variable.

Then you're probably using some old kernel... Please base your work on 
tty repo's tty-next branch when working with serial upstream.

> And the total waiting time is not a const value so we need polling.

It's not constant, agreed. But your comment states that it's at most one 
frame worth of time so that should be the worst-case waiting time. Once 
the UART starts to do things like temporary switch to loopback and/or 
reinit/clearing FIFO, it doesn't seem that harmful to wait slightly 
longer for the worst-case frame time, so essentially you'd only need this 
(+ comment):

	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
	dw8250_force_idle(p);
	ndelay(p->frame_time);
	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);

If you insist, ndelay() could be replaced by:
	ret = read_poll_timeout_atomic(p->serial_in, usr, !(usr & DW_UART_USR_BUSY),
				       1, DIV_ROUND_UP(p->frame_time, NSEC_PER_USEC), false,
				       p, d->usr_reg);

You also don't explain why force_idle() is needed inside to loop, why 
doing it once before the loop is not enough? I can see the need for that 
in loop for dw8250_check_lcr() because it doesn't enable loopback, but 
here, so why?

I started to wonder whether dw8250_check_lcr() should also temporarily 
switch to loopback mode to ensure the UART becomes idle. Some common macro 
could be created which wraps the idle forcing for both use cases + 
restoring LCR & MCR. That is, dw8250_force_idle() + little bit extra -> 
__dw8250_idle_enter() and __dw8250_idle_exit() that are called by the 
macro.

I also recall there was something with RS485 mode that it didn't do 
something with loopback but the wording in the databook seems rather 
vague.

> > > +    d->last_loopback_waiting_time = retries;
> > > +
> > > +    p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > > +    if (p->serial_in(p, UART_LCR) & UART_LCR_DLAB) {
> > Can this still fail? Why?
> If the waiting time is enough this should not fail.
> 
> But under my test before this patch, set UART_LCR register maybe failed 
> due to busy,

So your patch doesn't work or what are you saying? I see you're using 
"before this patch" but how is that relevant to the case with the patch
my question was about, I don't understand?

> if we write DLM without check DLAB bit, that will write data to UART_IER
> register,
> different baudrate will write different value, that will cause some interrupt
> disabled
> and cause strange problem.

Ah, I see. You should comment it then it's there just for safety purposes.


It might also make sense to add delayed error reporting for the failures. 
The error printing can safely happen only after releasing port's lock.


-- 
 i.

Re: serial8250: can not change baudrate while the controller is busy

[qianfan]

在 2023/5/8 21:34, Ilpo Järvinen 写道:
> On Mon, 8 May 2023, qianfan wrote:
>> 在 2023/5/8 18:17, Ilpo Järvinen 写道:
>>> On Fri, 5 May 2023, qianfan wrote:
>>>> 在 2023/4/14 20:10, Ilpo Järvinen 写道:
>>>>> On Fri, 14 Apr 2023, qianfan wrote:
>>>>>> My custom board is based on allwinner R40, the uart is compatibled
>>>>>> with
>>>>>> serial8250. Based on it's datasheet:
>>>>>>
>>>>>>> When TX transmit data, or RX receives data, or TX FIFO is not empty,
>>>>>>> then
>>>>>> the
>>>>>>> BUSY flag bit can be set to 1 by hardware, which indicates the UART
>>>>>>> controller is busy.
>>>>>> We cannot write LCR and DLL to update UART params such as baudrate and
>>>>>> partity
>>>>>> while the UART is busy, however `serial8250_do_set_termios` is a void
>>>>>> function,
>>>>>> the upper level always assume the uart params is updated.
>>>>>>
>>>>>> The upper level `uart_set_termios` do noting if ktermios params is not
>>>>>> changed,
>>>>>> it will not update when the user space program running tcsetattr set a
>>>>>> same
>>>>>> baudrate again.
>>>>>>
>>>>>> So we can not fix the baudrate when
>>>>>> `serial8250_do_set_termios`
>>>>>> failed.
>>>>>>
>>>>>> Allwinner R40's datasheet provided a way for this case.
>>>>>>
>>>>>>> CHCFG_AT_BUSY(configure at busy): Enable the bit, software can also
>>>>>>> set
>>>>>>> UART
>>>>>>> controller when UART is busy, such as the LCR, DLH, DLL register.
>>>>>>> CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and
>>>>>>> CHANGE_UPDATE
>>>>>>> is written to 1, the configuration of UART controller can be
>>>>>>> updated.
>>>>>>> After completed update, the bit is cleared to 0 automatically.
>>>>>> I can't know this feature is expanded by allwinner, or it is a common
>>>>>> functiton
>>>>>> of serial8250. Perhaps the serial8250 driver need this.
>>>>> tcsetattr() can be given a flag which enforces TX empty condition before
>>>>> core calls into the lower layer HW set_termios function. Would that be
>>>>> enough to solve the case you're interested in?
>>>>>
>>>>> Obviously, nothing can prevent Rx from occuring as it's not under local
>>>>> UART's control (e.g. a busy flag check would still be racy). But does
>>>>> writing those registers actually break something or just corrupts the
>>>>> character under Tx/Rx (which can be handled by flushing)?
>>>> Hi:
>>>>
>>>> I speed long times to create a common solution for this problem.
>>>>
>>>> (I had create two commit, the first one add some sysfs debug interface
>>>> and the second one try solve this problem. So the next following patch
>>>> has only patch-2. Let's we discuess this solution and I will send all
>>>> patches if it is good.)
>>> Thanks a lot, it's much easier to discuss now with something concrete at
>>> hand.
>>>
>>>> Allwinner introduce some bits in HALT_TX register which can change
>>>> baudrate while the serial is busy. But that is not a common feature
>>>> of dw-uart. Rockchip's uart is also based on dw-uart and they doesn't
>>>> has such feature.
>>>>
>>>> The loopback is a common feature of 16450/16550 serial, so we can set
>>>> loopback mode to cut down the external serial line to force the serial
>>>> to idle.
>>>>
>>>> Next is the second patch:
>>>>
>>>>   From 171e981c3695e3efcc76a2c4f0d0937d366d6e2a Mon Sep 17 00:00:00 2001
>>>> From: qianfan Zhao <qianfanguijin@163.com>
>>>> Date: Fri, 5 May 2023 08:46:50 +0800
>>>> Subject: [PATCH] drivers: serial: 8250_dw: Make uart idle before set
>>>> baudrate
>>>>
>>>> Some registers which control the baudrate such as DLL, DLM can not
>>>> write while the uart is busy. So set the controller to loopback mode
>>>> and clear fifos to force idle before change baudrate.
>>>>
>>>> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
>>>> ---
>>>> @@ -360,6 +367,46 @@ static void dw8250_set_termios(struct uart_port *p,
>>>> struct ktermios *termios,
>>>>        serial8250_do_set_termios(p, termios, old);
>>>>    }
>>>>
>>>> +static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
>>>> +                   unsigned int quot, unsigned int quot_frac)
>>>> +{
>>>> +    struct uart_8250_port *up = up_to_u8250p(p);
>>>> +    struct dw8250_data *d = to_dw8250_data(p->private_data);
>>>> +    unsigned int usr;
>>>> +    int retries;
>>>> +
>>>> +    /*
>>>> +     * LCR, DLL, DLM registers can not write while the uart is busy,
>>> According to DW databook, this is not entirely true. The databook
>>> explicitly states that if BUSY is not configured
>>> (UART_16550_COMPATIBLE=YES), those are always writable. And I know for
>>> sure that there are devices on the field do not come with BUSY.
>>> Thus, it looks something that should be decided based on BUSY
>>> availability.
>>>
>>> I had one time a patch which generalized uart_16550_compatible to
>>> struct dw8250_port_data but in the end I didn't need it.
>> Or we should register dw8250_set_divisor callback only when
>> !d->uart_16550_compatible
>> in probe function, that is a easy way to handle this.
> Yes, registering it based on the compatible might makes sense, but please
> see my comment below wrt. dw8250_check_lcr().
>
>>>> +     * set uart to loopback mode, clear fifos to force idle.
>>>> +     * The loopback mode doesn't take effect immediately, it will waiting
>>>> +     * current byte received done, the lower baudrate the longer waiting
>>>> +     * time.
>>>> +     */
>>>> +    p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
>>>> +    for (retries = 0; retries < 10000; retries++) {
>>>> +        dw8250_force_idle(p);
>>>> +
>>>> +        usr = p->serial_in(p, d->usr_reg);
>>>> +        if (!(usr & DW_UART_USR_BUSY))
>>>> +            break;
>>>> +        udelay(1);
>>>> +    }
>>> This loop is overkill, ndelay(p->frame_time) is all you need to wait for
>>> the maximum time a single frame needs.
>> Sorry but I can not find the p->frame_time variable.
> Then you're probably using some old kernel... Please base your work on
> tty repo's tty-next branch when working with serial upstream.
Thanks and I found it now.
>
>> And the total waiting time is not a const value so we need polling.
> It's not constant, agreed. But your comment states that it's at most one
> frame worth of time so that should be the worst-case waiting time. Once
> the UART starts to do things like temporary switch to loopback and/or
> reinit/clearing FIFO, it doesn't seem that harmful to wait slightly
> longer for the worst-case frame time, so essentially you'd only need this
> (+ comment):
>
> 	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> 	dw8250_force_idle(p);
> 	ndelay(p->frame_time);
> 	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
>
> If you insist, ndelay() could be replaced by:
> 	ret = read_poll_timeout_atomic(p->serial_in, usr, !(usr & DW_UART_USR_BUSY),
> 				       1, DIV_ROUND_UP(p->frame_time, NSEC_PER_USEC), false,
> 				       p, d->usr_reg);
>
> You also don't explain why force_idle() is needed inside to loop, why
> doing it once before the loop is not enough? I can see the need for that
> in loop for dw8250_check_lcr() because it doesn't enable loopback, but
> here, so why?
Under my test, this code will not work:

p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
dw8250_force_idle(p);
/* waiting until not busy... */

Current byte maybe not finished when we set UART_MCR_LOOP and reset fifo,
dw-uart will continue receive even if LOOP bit is set. After this byte
is finished it will push data to rx fifo and remark BUSY flag again.
That's why I leave dw8250_force_idle inside to loop.

Or maybe we should make dw8250_force_idle after ndelay such as:

p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
ndelay(p->frame_time);
dw8250_force_idle(p);

But that requires a test to see if it works.
>
> I started to wonder whether dw8250_check_lcr() should also temporarily
> switch to loopback mode to ensure the UART becomes idle. Some common macro
> could be created which wraps the idle forcing for both use cases +
> restoring LCR & MCR. That is, dw8250_force_idle() + little bit extra ->
> __dw8250_idle_enter() and __dw8250_idle_exit() that are called by the
> macro.
>
> I also recall there was something with RS485 mode that it didn't do
> something with loopback but the wording in the databook seems rather
> vague.
>
>>>> +    d->last_loopback_waiting_time = retries;
>>>> +
>>>> +    p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
>>>> +    if (p->serial_in(p, UART_LCR) & UART_LCR_DLAB) {
>>> Can this still fail? Why?
>> If the waiting time is enough this should not fail.
>>
>> But under my test before this patch, set UART_LCR register maybe failed
>> due to busy,
> So your patch doesn't work or what are you saying? I see you're using
> "before this patch" but how is that relevant to the case with the patch
> my question was about, I don't understand?
I means set LCR maybe failed without this patch.
>
>> if we write DLM without check DLAB bit, that will write data to UART_IER
>> register,
>> different baudrate will write different value, that will cause some interrupt
>> disabled
>> and cause strange problem.
> Ah, I see. You should comment it then it's there just for safety purposes.
OK.
>
>
> It might also make sense to add delayed error reporting for the failures.
> The error printing can safely happen only after releasing port's lock.
Yes, this is really useful.
>
>

Re: serial8250: can not change baudrate while the controller is busy

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8463 bytes --]

On Tue, 9 May 2023, qianfan wrote:
> 在 2023/5/8 21:34, Ilpo Järvinen 写道:
> > On Mon, 8 May 2023, qianfan wrote:
> > > 在 2023/5/8 18:17, Ilpo Järvinen 写道:
> > > > On Fri, 5 May 2023, qianfan wrote:
> > > > > 在 2023/4/14 20:10, Ilpo Järvinen 写道:
> > > > > > On Fri, 14 Apr 2023, qianfan wrote:
> > > > > > > My custom board is based on allwinner R40, the uart is compatibled
> > > > > > > with
> > > > > > > serial8250. Based on it's datasheet:
> > > > > > > 
> > > > > > > > When TX transmit data, or RX receives data, or TX FIFO is not
> > > > > > > > empty,
> > > > > > > > then
> > > > > > > the
> > > > > > > > BUSY flag bit can be set to 1 by hardware, which indicates the
> > > > > > > > UART
> > > > > > > > controller is busy.
> > > > > > > We cannot write LCR and DLL to update UART params such as baudrate
> > > > > > > and
> > > > > > > partity
> > > > > > > while the UART is busy, however `serial8250_do_set_termios` is a
> > > > > > > void
> > > > > > > function,
> > > > > > > the upper level always assume the uart params is updated.
> > > > > > > 
> > > > > > > The upper level `uart_set_termios` do noting if ktermios params is
> > > > > > > not
> > > > > > > changed,
> > > > > > > it will not update when the user space program running tcsetattr
> > > > > > > set a
> > > > > > > same
> > > > > > > baudrate again.
> > > > > > > 
> > > > > > > So we can not fix the baudrate when
> > > > > > > `serial8250_do_set_termios`
> > > > > > > failed.
> > > > > > > 
> > > > > > > Allwinner R40's datasheet provided a way for this case.
> > > > > > > 
> > > > > > > > CHCFG_AT_BUSY(configure at busy): Enable the bit, software can
> > > > > > > > also
> > > > > > > > set
> > > > > > > > UART
> > > > > > > > controller when UART is busy, such as the LCR, DLH, DLL
> > > > > > > > register.
> > > > > > > > CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and
> > > > > > > > CHANGE_UPDATE
> > > > > > > > is written to 1, the configuration of UART controller can be
> > > > > > > > updated.
> > > > > > > > After completed update, the bit is cleared to 0 automatically.
> > > > > > > I can't know this feature is expanded by allwinner, or it is a
> > > > > > > common
> > > > > > > functiton
> > > > > > > of serial8250. Perhaps the serial8250 driver need this.
> > > > > > tcsetattr() can be given a flag which enforces TX empty condition
> > > > > > before
> > > > > > core calls into the lower layer HW set_termios function. Would that
> > > > > > be
> > > > > > enough to solve the case you're interested in?
> > > > > > 
> > > > > > Obviously, nothing can prevent Rx from occuring as it's not under
> > > > > > local
> > > > > > UART's control (e.g. a busy flag check would still be racy). But
> > > > > > does
> > > > > > writing those registers actually break something or just corrupts
> > > > > > the
> > > > > > character under Tx/Rx (which can be handled by flushing)?
> > > > > Hi:
> > > > > 
> > > > > I speed long times to create a common solution for this problem.
> > > > > 
> > > > > (I had create two commit, the first one add some sysfs debug interface
> > > > > and the second one try solve this problem. So the next following patch
> > > > > has only patch-2. Let's we discuess this solution and I will send all
> > > > > patches if it is good.)
> > > > Thanks a lot, it's much easier to discuss now with something concrete at
> > > > hand.
> > > > 
> > > > > Allwinner introduce some bits in HALT_TX register which can change
> > > > > baudrate while the serial is busy. But that is not a common feature
> > > > > of dw-uart. Rockchip's uart is also based on dw-uart and they doesn't
> > > > > has such feature.
> > > > > 
> > > > > The loopback is a common feature of 16450/16550 serial, so we can set
> > > > > loopback mode to cut down the external serial line to force the serial
> > > > > to idle.
> > > > > 
> > > > > Next is the second patch:
> > > > > 
> > > > >   From 171e981c3695e3efcc76a2c4f0d0937d366d6e2a Mon Sep 17 00:00:00
> > > > > 2001
> > > > > From: qianfan Zhao <qianfanguijin@163.com>
> > > > > Date: Fri, 5 May 2023 08:46:50 +0800
> > > > > Subject: [PATCH] drivers: serial: 8250_dw: Make uart idle before set
> > > > > baudrate
> > > > > 
> > > > > Some registers which control the baudrate such as DLL, DLM can not
> > > > > write while the uart is busy. So set the controller to loopback mode
> > > > > and clear fifos to force idle before change baudrate.
> > > > > 
> > > > > Signed-off-by: qianfan Zhao <qianfanguijin@163.com>

> > > And the total waiting time is not a const value so we need polling.
> > It's not constant, agreed. But your comment states that it's at most one
> > frame worth of time so that should be the worst-case waiting time. Once
> > the UART starts to do things like temporary switch to loopback and/or
> > reinit/clearing FIFO, it doesn't seem that harmful to wait slightly
> > longer for the worst-case frame time, so essentially you'd only need this
> > (+ comment):
> > 
> > 	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > 	dw8250_force_idle(p);
> > 	ndelay(p->frame_time);
> > 	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > 
> > If you insist, ndelay() could be replaced by:
> > 	ret = read_poll_timeout_atomic(p->serial_in, usr, !(usr &
> > DW_UART_USR_BUSY),
> > 				       1, DIV_ROUND_UP(p->frame_time,
> > NSEC_PER_USEC), false,
> > 				       p, d->usr_reg);
> > 
> > You also don't explain why force_idle() is needed inside to loop, why
> > doing it once before the loop is not enough? I can see the need for that
> > in loop for dw8250_check_lcr() because it doesn't enable loopback, but
> > here, so why?
> Under my test, this code will not work:
> 
> p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> dw8250_force_idle(p);
> /* waiting until not busy... */
> 
> Current byte maybe not finished when we set UART_MCR_LOOP and reset fifo,
> dw-uart will continue receive even if LOOP bit is set. After this byte
> is finished it will push data to rx fifo and remark BUSY flag again.
> That's why I leave dw8250_force_idle inside to loop.

It's a bit odd because BUSY should indicate a character is being received 
(asserted around middle of the start bit according to the databook), not 
that it was pushed to FIFO. Is it trying to indicate BUSY due to rxing 
another character but then the hypothesis that enabling loopback takes at 
most 1 frame would be false.

To understand better what's going on here during debugging, it might be 
useful the check DR in LSR in the case when the BUSY is still held to 
exclude data-in-RBR condition which is another condition that could 
assert BUSY. Since the FIFO is being toggled off/on, there's window of 
time when BUSY could be held due to something arriving into RBR.

IMHO, it would be nice if the FIFO reset turns out to be entirely 
unnecessary for enforcing not-busy because of that very window of 
receiving something into RBR. A character in RBR would need to be handled 
by an extra read from UART_RX which is what dw8250_force_idle() actually 
does, it looked earlier pretty magic to me but now realizing this race 
window w/o FIFO, maybe that's the explanation. If FIFO reset is not 
necessary, it would take away the need to understand how instanteneous 
toggling UART_FCR_ENABLE_FIFO is wrt. arriving character.

> Or maybe we should make dw8250_force_idle after ndelay such as:
> 
> p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> ndelay(p->frame_time);
> dw8250_force_idle(p);
> 
> But that requires a test to see if it works.

Is Tx side empty during your test or can it interfere too here? (Just 
thinking out loud, you probably considered that yourself).

> > I started to wonder whether dw8250_check_lcr() should also temporarily
> > switch to loopback mode to ensure the UART becomes idle. Some common macro
> > could be created which wraps the idle forcing for both use cases +
> > restoring LCR & MCR. That is, dw8250_force_idle() + little bit extra ->
> > __dw8250_idle_enter() and __dw8250_idle_exit() that are called by the
> > macro.

> > It might also make sense to add delayed error reporting for the failures.
> > The error printing can safely happen only after releasing port's lock.
> Yes, this is really useful.

I checked it briefly yesterday and only the break_ctl would need to be 
wrapped besides set_divisor and set_termios. In any case, it should be 
done in a separate patch but definitely looked doable.

-- 
 i.

Re: serial8250: can not change baudrate while the controller is busy

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 11656 bytes --]

On Tue, 9 May 2023, qianfan wrote:

> It's not constant, agreed. But your comment states that it's at most one
> frame worth of time so that should be the worst-case waiting time. Once
> the UART starts to do things like temporary switch to loopback and/or
> reinit/clearing FIFO, it doesn't seem that harmful to wait slightly
> longer for the worst-case frame time, so essentially you'd only need this
> (+ comment):
> 
> 	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> 	dw8250_force_idle(p);
> 	ndelay(p->frame_time);
> 	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> 
> If you insist, ndelay() could be replaced by:
> 	ret = read_poll_timeout_atomic(p->serial_in, usr, !(usr &
> DW_UART_USR_BUSY),
> 				       1, DIV_ROUND_UP(p->frame_time,
> NSEC_PER_USEC), false,
> 				       p, d->usr_reg);
> 
> You also don't explain why force_idle() is needed inside to loop, why
> doing it once before the loop is not enough? I can see the need for that
> in loop for dw8250_check_lcr() because it doesn't enable loopback, but
> here, so why?
> 
> Under my test, this code will not work:
> 
> p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> dw8250_force_idle(p);
> /* waiting until not busy... */
> 
> Current byte maybe not finished when we set UART_MCR_LOOP and reset fifo,
> dw-uart will continue receive even if LOOP bit is set. After this byte
> is finished it will push data to rx fifo and remark BUSY flag again.
> That's why I leave dw8250_force_idle inside to loop.
> 
> It's a bit odd because BUSY should indicate a character is being received 
> (asserted around middle of the start bit according to the databook), not 
> 
> Is the databoot is public and may I read it?

I'm not sure. The title is "DesignWare DW_apb_uart Databook" in case that 
helps.

> that it was pushed to FIFO. Is it trying to indicate BUSY due to rxing 
> another character but then the hypothesis that enabling loopback takes at 
> most 1 frame would be false.
> 
> To understand better what's going on here during debugging, it might be 
> useful the check DR in LSR in the case when the BUSY is still held to 
> exclude data-in-RBR condition which is another condition that could 
> assert BUSY. Since the FIFO is being toggled off/on, there's window of 
> time when BUSY could be held due to something arriving into RBR.
> 
> IMHO, it would be nice if the FIFO reset turns out to be entirely 
> unnecessary for enforcing not-busy because of that very window of 
> receiving something into RBR. A character in RBR would need to be handled 
> by an extra read from UART_RX which is what dw8250_force_idle() actually 
> does, it looked earlier pretty magic to me but now realizing this race 
> window w/o FIFO, maybe that's the explanation. If FIFO reset is not 
> necessary, it would take away the need to understand how instanteneous 
> toggling UART_FCR_ENABLE_FIFO is wrt. arriving character.
> 
> Thanks and now I understand why we should read UART_RX register when dw8250_
> force_idle,
> the loopback mode takes affect right now when we set LOOP bit, the fifo is r
> eseted and
> the next incoming data will goto RBR register, not FIFO.
> 
> So the most important code in dw8250_force_idle after set loopback mode is r
> ead UART_RX, 
> not clear fifo. Is it right?
> 
> So the next code is better:
> p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> if (p->serial_in(p, d->usr_reg) & DW_UART_USR_BUSY)
> 	ndelay(p->frame_time);
> dw8250_force_idle(p);
>
> Or maybe we should make dw8250_force_idle after ndelay such as:
> 
> p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> ndelay(p->frame_time);
> dw8250_force_idle(p);
> 
> But that requires a test to see if it works.
> 
> Is Tx side empty during your test or can it interfere too here? (Just 
> thinking out loud, you probably considered that yourself).
> 
> I started to wonder whether dw8250_check_lcr() should also temporarily
> switch to loopback mode to ensure the UART becomes idle. Some common macro
> could be created which wraps the idle forcing for both use cases +
> restoring LCR & MCR. That is, dw8250_force_idle() + little bit extra ->
> __dw8250_idle_enter() and __dw8250_idle_exit() that are called by the
> macro.
> 
> Switch to loopback mode in dw8250_check_lcr is not enough. We also need intr
> oduce 
> sometings such as dw8250_check_dll() and dw8250_check_dlm(). That is not a g
> ood
> idea.

No. My point was that it would be nice to figure out a sequence of 
operations that is guaranteed to get BUSY deasserted. It helps both LCR 
(only) write problem and the divisor update problem because the cause for 
them seems to be the same.

So replacing dw8250_force_idle() with something like what is below might 
be useful.

I still don't understand why the FIFO needs to be disabled but included it 
into the idle sequence anyway.

In contrast to earlier:
  - FIFO is not reset but it could be added into the idle exit function if 
    it's a desirable feature.
  - IER is zeroed to prevent RBR filling triggering an interrupt (with 
    port's lock held here it's going to just keep attempt to acquire the 
    lock).
  - LCR write is handled as well (w/o now triggering extra FIFO reinits).
  - Flush DMA Rx


[PATCH 1/1] serial: 8250_dw: Ensure BUSY is deasserted

DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
Existance of BUSY depends on uart_16550_compatible, if UART HW is
configured with 16550 compatible those registers can always be
written.

There currently is dw8250_force_idle() which attempts to archive
non-BUSY state by disabling FIFO, however, the solution is unreliable
when Rx keeps getting more and more characters.

Create a sequence of operations to enforce that ensures UART cannot
keep BUSY asserted indefinitely. The new sequence relies on enabling
loopback mode temporarily to prevent incoming Rx characters keeping
UART BUSY.

Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
writes.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c    | 141 ++++++++++++++++++++-------
 drivers/tty/serial/8250/8250_dwlib.h |   1 +
 2 files changed, 106 insertions(+), 36 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 7db51781289e..c9d9b4bda36a 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -44,6 +44,8 @@
 /* DesignWare specific register fields */
 #define DW_UART_MCR_SIRE		BIT(6)
 
+#define DW_UART_USR_BUSY		BIT(0)
+
 /* Renesas specific register fields */
 #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
 #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
@@ -80,57 +82,123 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
 	return value;
 }
 
-static void dw8250_force_idle(struct uart_port *p)
+/*
+ * Ensure BUSY is not asserted. If DW UART is configured with
+ * !uart_16550_compatible, the writes to LCR, DLL, and DLH fail while
+ * BUSY is asserted.
+ *
+ * Context: port's lock must be held
+ */
+static int dw8250_idle_enter(struct uart_port *p)
 {
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	struct uart_8250_port *up = up_to_u8250p(p);
-	unsigned int lsr;
+	u32 lsr;
 
-	serial8250_clear_and_reinit_fifos(up);
+	if (d->uart_16550_compatible)
+		return 0;
 
-	/*
-	 * With PSLVERR_RESP_EN parameter set to 1, the device generates an
-	 * error response when an attempt to read an empty RBR with FIFO
-	 * enabled.
-	 */
-	if (up->fcr & UART_FCR_ENABLE_FIFO) {
-		lsr = p->serial_in(p, UART_LSR);
-		if (!(lsr & UART_LSR_DR))
-			return;
+	d->in_idle = 1;
+
+	/* Prevent triggering interrupt from RBR filling */
+	p->serial_out(p, UART_IER, 0);
+
+	serial8250_rx_dma_flush(up);
+	// What about Tx DMA? Should probably pause that too and resume
+	// afterwards.
+
+	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
+	if (up->capabilities & UART_CAP_FIFO)
+		p->serial_out(p, UART_FCR, 0);
+
+	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
+		ndelay(p->frame_time);
+
+	lsr = serial_lsr_in(up);
+	if (lsr & UART_LSR_DR) {
+		p->serial_in(p, UART_RX);
+		up->lsr_saved_flags = 0;
 	}
 
-	(void)p->serial_in(p, UART_RX);
+	/* Now guaranteed to have BUSY deasserted? Just sanity check */
+	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
+		return -EBUSY;
+
+	return 0;
+}
+
+static void dw8250_idle_exit(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	struct uart_8250_port *up = up_to_u8250p(p);
+
+	if (d->uart_16550_compatible)
+		return;
+
+	if (up->capabilities & UART_CAP_FIFO)
+		p->serial_out(p, UART_FCR, up->fcr);
+	p->serial_out(p, UART_MCR, up->mcr);
+	p->serial_out(p, UART_IER, up->ier);
+
+	// Maybe move the DMA Rx restart check in dma_rx_complete() to own
+	// function (serial8250_rx_dma_restart()) and call it from here.
+	// DMA Tx resume
+
+	d->in_idle = 0;
+}
+
+static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
+			       unsigned int quot, unsigned int quot_frac)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+	int ret;
+
+	ret = dw8250_idle_enter(p);
+	if (ret < 0)
+		goto idle_failed;
+
+	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
+	if (!(p->serial_in(p, UART_LCR) & UART_LCR_DLAB))
+		goto idle_failed;
+
+	serial_dl_write(up, quot);
+	p->serial_out(p, UART_LCR, up->lcr);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 static void dw8250_check_lcr(struct uart_port *p, int value)
 {
-	void __iomem *offset = p->membase + (UART_LCR << p->regshift);
-	int tries = 1000;
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	unsigned int lcr = p->serial_in(p, UART_LCR);
+	int ret;
 
 	/* Make sure LCR write wasn't ignored */
-	while (tries--) {
-		unsigned int lcr = p->serial_in(p, UART_LCR);
-
-		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
-			return;
+	if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
+		return;
 
-		dw8250_force_idle(p);
+	if (d->in_idle) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		return;
+	}
 
-#ifdef CONFIG_64BIT
-		if (p->type == PORT_OCTEON)
-			__raw_writeq(value & 0xff, offset);
-		else
-#endif
-		if (p->iotype == UPIO_MEM32)
-			writel(value, offset);
-		else if (p->iotype == UPIO_MEM32BE)
-			iowrite32be(value, offset);
-		else
-			writeb(value, offset);
+	ret = dw8250_idle_enter(p);
+	if (ret < 0) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		goto idle_failed;
 	}
-	/*
-	 * FIXME: this deadlocks if port->lock is already held
-	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
-	 */
+
+	p->serial_out(p, UART_LCR, value);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 /* Returns once the transmitter is empty or we run out of retries */
@@ -540,6 +608,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->set_divisor	= dw8250_set_divisor;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index f13e91f2cace..bafacd327fc4 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -45,6 +45,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		in_idle:1;
 };
 
 void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, const struct ktermios *old);
-- 
2.30.2

Re: serial8250: can not change baudrate while the controller is busy

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 12507 bytes --]

Hi,

Did you ever manage to take a look at the approach I tried in the patch 
below or perhaps even test if it works?

On Wed, 10 May 2023, Ilpo Järvinen wrote:

> On Tue, 9 May 2023, qianfan wrote:
> 
> > It's not constant, agreed. But your comment states that it's at most one
> > frame worth of time so that should be the worst-case waiting time. Once
> > the UART starts to do things like temporary switch to loopback and/or
> > reinit/clearing FIFO, it doesn't seem that harmful to wait slightly
> > longer for the worst-case frame time, so essentially you'd only need this
> > (+ comment):
> > 
> > 	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > 	dw8250_force_idle(p);
> > 	ndelay(p->frame_time);
> > 	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > 
> > If you insist, ndelay() could be replaced by:
> > 	ret = read_poll_timeout_atomic(p->serial_in, usr, !(usr &
> > DW_UART_USR_BUSY),
> > 				       1, DIV_ROUND_UP(p->frame_time,
> > NSEC_PER_USEC), false,
> > 				       p, d->usr_reg);
> > 
> > You also don't explain why force_idle() is needed inside to loop, why
> > doing it once before the loop is not enough? I can see the need for that
> > in loop for dw8250_check_lcr() because it doesn't enable loopback, but
> > here, so why?
> > 
> > Under my test, this code will not work:
> > 
> > p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > dw8250_force_idle(p);
> > /* waiting until not busy... */
> > 
> > Current byte maybe not finished when we set UART_MCR_LOOP and reset fifo,
> > dw-uart will continue receive even if LOOP bit is set. After this byte
> > is finished it will push data to rx fifo and remark BUSY flag again.
> > That's why I leave dw8250_force_idle inside to loop.
> > 
> > It's a bit odd because BUSY should indicate a character is being received 
> > (asserted around middle of the start bit according to the databook), not 
> > 
> > Is the databoot is public and may I read it?
> 
> I'm not sure. The title is "DesignWare DW_apb_uart Databook" in case that 
> helps.
> 
> > that it was pushed to FIFO. Is it trying to indicate BUSY due to rxing 
> > another character but then the hypothesis that enabling loopback takes at 
> > most 1 frame would be false.
> > 
> > To understand better what's going on here during debugging, it might be 
> > useful the check DR in LSR in the case when the BUSY is still held to 
> > exclude data-in-RBR condition which is another condition that could 
> > assert BUSY. Since the FIFO is being toggled off/on, there's window of 
> > time when BUSY could be held due to something arriving into RBR.
> > 
> > IMHO, it would be nice if the FIFO reset turns out to be entirely 
> > unnecessary for enforcing not-busy because of that very window of 
> > receiving something into RBR. A character in RBR would need to be handled 
> > by an extra read from UART_RX which is what dw8250_force_idle() actually 
> > does, it looked earlier pretty magic to me but now realizing this race 
> > window w/o FIFO, maybe that's the explanation. If FIFO reset is not 
> > necessary, it would take away the need to understand how instanteneous 
> > toggling UART_FCR_ENABLE_FIFO is wrt. arriving character.
> > 
> > Thanks and now I understand why we should read UART_RX register when dw8250_
> > force_idle,
> > the loopback mode takes affect right now when we set LOOP bit, the fifo is r
> > eseted and
> > the next incoming data will goto RBR register, not FIFO.
> > 
> > So the most important code in dw8250_force_idle after set loopback mode is r
> > ead UART_RX, 
> > not clear fifo. Is it right?
> > 
> > So the next code is better:
> > p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > if (p->serial_in(p, d->usr_reg) & DW_UART_USR_BUSY)
> > 	ndelay(p->frame_time);
> > dw8250_force_idle(p);
> >
> > Or maybe we should make dw8250_force_idle after ndelay such as:
> > 
> > p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > ndelay(p->frame_time);
> > dw8250_force_idle(p);
> > 
> > But that requires a test to see if it works.
> > 
> > Is Tx side empty during your test or can it interfere too here? (Just 
> > thinking out loud, you probably considered that yourself).
> > 
> > I started to wonder whether dw8250_check_lcr() should also temporarily
> > switch to loopback mode to ensure the UART becomes idle. Some common macro
> > could be created which wraps the idle forcing for both use cases +
> > restoring LCR & MCR. That is, dw8250_force_idle() + little bit extra ->
> > __dw8250_idle_enter() and __dw8250_idle_exit() that are called by the
> > macro.
> > 
> > Switch to loopback mode in dw8250_check_lcr is not enough. We also need intr
> > oduce 
> > sometings such as dw8250_check_dll() and dw8250_check_dlm(). That is not a g
> > ood
> > idea.
> 
> No. My point was that it would be nice to figure out a sequence of 
> operations that is guaranteed to get BUSY deasserted. It helps both LCR 
> (only) write problem and the divisor update problem because the cause for 
> them seems to be the same.
> 
> So replacing dw8250_force_idle() with something like what is below might 
> be useful.
> 
> I still don't understand why the FIFO needs to be disabled but included it 
> into the idle sequence anyway.
> 
> In contrast to earlier:
>   - FIFO is not reset but it could be added into the idle exit function if 
>     it's a desirable feature.
>   - IER is zeroed to prevent RBR filling triggering an interrupt (with 
>     port's lock held here it's going to just keep attempt to acquire the 
>     lock).
>   - LCR write is handled as well (w/o now triggering extra FIFO reinits).
>   - Flush DMA Rx
> 
> 
> [PATCH 1/1] serial: 8250_dw: Ensure BUSY is deasserted
> 
> DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
> Existance of BUSY depends on uart_16550_compatible, if UART HW is
> configured with 16550 compatible those registers can always be
> written.
> 
> There currently is dw8250_force_idle() which attempts to archive
> non-BUSY state by disabling FIFO, however, the solution is unreliable
> when Rx keeps getting more and more characters.
> 
> Create a sequence of operations to enforce that ensures UART cannot
> keep BUSY asserted indefinitely. The new sequence relies on enabling
> loopback mode temporarily to prevent incoming Rx characters keeping
> UART BUSY.
> 
> Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
> writes.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c    | 141 ++++++++++++++++++++-------
>  drivers/tty/serial/8250/8250_dwlib.h |   1 +
>  2 files changed, 106 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 7db51781289e..c9d9b4bda36a 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -44,6 +44,8 @@
>  /* DesignWare specific register fields */
>  #define DW_UART_MCR_SIRE		BIT(6)
>  
> +#define DW_UART_USR_BUSY		BIT(0)
> +
>  /* Renesas specific register fields */
>  #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
>  #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
> @@ -80,57 +82,123 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
>  	return value;
>  }
>  
> -static void dw8250_force_idle(struct uart_port *p)
> +/*
> + * Ensure BUSY is not asserted. If DW UART is configured with
> + * !uart_16550_compatible, the writes to LCR, DLL, and DLH fail while
> + * BUSY is asserted.
> + *
> + * Context: port's lock must be held
> + */
> +static int dw8250_idle_enter(struct uart_port *p)
>  {
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
>  	struct uart_8250_port *up = up_to_u8250p(p);
> -	unsigned int lsr;
> +	u32 lsr;
>  
> -	serial8250_clear_and_reinit_fifos(up);
> +	if (d->uart_16550_compatible)
> +		return 0;
>  
> -	/*
> -	 * With PSLVERR_RESP_EN parameter set to 1, the device generates an
> -	 * error response when an attempt to read an empty RBR with FIFO
> -	 * enabled.
> -	 */
> -	if (up->fcr & UART_FCR_ENABLE_FIFO) {
> -		lsr = p->serial_in(p, UART_LSR);
> -		if (!(lsr & UART_LSR_DR))
> -			return;
> +	d->in_idle = 1;
> +
> +	/* Prevent triggering interrupt from RBR filling */
> +	p->serial_out(p, UART_IER, 0);
> +
> +	serial8250_rx_dma_flush(up);
> +	// What about Tx DMA? Should probably pause that too and resume
> +	// afterwards.
> +
> +	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> +	if (up->capabilities & UART_CAP_FIFO)
> +		p->serial_out(p, UART_FCR, 0);
> +
> +	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
> +		ndelay(p->frame_time);
> +
> +	lsr = serial_lsr_in(up);
> +	if (lsr & UART_LSR_DR) {
> +		p->serial_in(p, UART_RX);
> +		up->lsr_saved_flags = 0;
>  	}
>  
> -	(void)p->serial_in(p, UART_RX);
> +	/* Now guaranteed to have BUSY deasserted? Just sanity check */
> +	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +static void dw8250_idle_exit(struct uart_port *p)
> +{
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +
> +	if (d->uart_16550_compatible)
> +		return;
> +
> +	if (up->capabilities & UART_CAP_FIFO)
> +		p->serial_out(p, UART_FCR, up->fcr);
> +	p->serial_out(p, UART_MCR, up->mcr);
> +	p->serial_out(p, UART_IER, up->ier);
> +
> +	// Maybe move the DMA Rx restart check in dma_rx_complete() to own
> +	// function (serial8250_rx_dma_restart()) and call it from here.
> +	// DMA Tx resume
> +
> +	d->in_idle = 0;
> +}
> +
> +static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
> +			       unsigned int quot, unsigned int quot_frac)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +	int ret;
> +
> +	ret = dw8250_idle_enter(p);
> +	if (ret < 0)
> +		goto idle_failed;
> +
> +	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> +	if (!(p->serial_in(p, UART_LCR) & UART_LCR_DLAB))
> +		goto idle_failed;
> +
> +	serial_dl_write(up, quot);
> +	p->serial_out(p, UART_LCR, up->lcr);
> +
> +idle_failed:
> +	dw8250_idle_exit(p);
>  }
>  
>  static void dw8250_check_lcr(struct uart_port *p, int value)
>  {
> -	void __iomem *offset = p->membase + (UART_LCR << p->regshift);
> -	int tries = 1000;
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> +	unsigned int lcr = p->serial_in(p, UART_LCR);
> +	int ret;
>  
>  	/* Make sure LCR write wasn't ignored */
> -	while (tries--) {
> -		unsigned int lcr = p->serial_in(p, UART_LCR);
> -
> -		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> -			return;
> +	if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> +		return;
>  
> -		dw8250_force_idle(p);
> +	if (d->in_idle) {
> +		/*
> +		 * FIXME: this deadlocks if port->lock is already held
> +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> +		 */
> +		return;
> +	}
>  
> -#ifdef CONFIG_64BIT
> -		if (p->type == PORT_OCTEON)
> -			__raw_writeq(value & 0xff, offset);
> -		else
> -#endif
> -		if (p->iotype == UPIO_MEM32)
> -			writel(value, offset);
> -		else if (p->iotype == UPIO_MEM32BE)
> -			iowrite32be(value, offset);
> -		else
> -			writeb(value, offset);
> +	ret = dw8250_idle_enter(p);
> +	if (ret < 0) {
> +		/*
> +		 * FIXME: this deadlocks if port->lock is already held
> +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> +		 */
> +		goto idle_failed;
>  	}
> -	/*
> -	 * FIXME: this deadlocks if port->lock is already held
> -	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> -	 */
> +
> +	p->serial_out(p, UART_LCR, value);
> +
> +idle_failed:
> +	dw8250_idle_exit(p);
>  }
>  
>  /* Returns once the transmitter is empty or we run out of retries */
> @@ -540,6 +608,7 @@ static int dw8250_probe(struct platform_device *pdev)
>  	p->serial_out	= dw8250_serial_out;
>  	p->set_ldisc	= dw8250_set_ldisc;
>  	p->set_termios	= dw8250_set_termios;
> +	p->set_divisor	= dw8250_set_divisor;
>  
>  	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
>  	if (!p->membase)
> diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
> index f13e91f2cace..bafacd327fc4 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.h
> +++ b/drivers/tty/serial/8250/8250_dwlib.h
> @@ -45,6 +45,7 @@ struct dw8250_data {
>  
>  	unsigned int		skip_autocfg:1;
>  	unsigned int		uart_16550_compatible:1;
> +	unsigned int		in_idle:1;
>  };
>  
>  void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, const struct ktermios *old);
> 


-- 
 i.