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@xxxxxxx>
> 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@xxxxxxx>
> ---
> 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.
