This series reworks DW UART's write logic such that the write in
->serial_out() is reused for LCR retries which allows removing those
ugly duplicated writes in dw8250_check_lcr() (renamed to
dw8250_verify_write() by this series).
I've used label+goto since the retry is really an exceptional thing. If
somebody insists, I can convert those to do {} while (); but I feel it
will give wrong impression that there's a "loop" there.
Ilpo Järvinen (4):
serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x()
serial: 8250_dw: Rename offset to reg_offset
serial: 8250_dw: Move 16550 compatible & LCR checks to
dw8250_verify_write()
serial: 8250_dw: Rework ->serial_out() LCR write retry logic
drivers/tty/serial/8250/8250_dw.c | 90 ++++++++++++++++---------------
1 file changed, 47 insertions(+), 43 deletions(-)
--
2.30.2
Place dw8250_serial_out() before dw8250_serial_out38x() so that it can
be called from dw8250_serial_out38x() to do the actual write.
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_dw.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 167a691c7b19..41bf063396e4 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -143,29 +143,23 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
}
}
-static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out(struct uart_port *p, int offset, int value)
{
struct dw8250_data *d = to_dw8250_data(p->private_data);
- /* Allow the TX to drain before we reconfigure */
- if (offset == UART_LCR)
- dw8250_tx_wait_empty(p);
-
writeb(value, p->membase + (offset << p->regshift));
if (offset == UART_LCR && !d->uart_16550_compatible)
dw8250_check_lcr(p, value);
}
-
-static void dw8250_serial_out(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
{
- struct dw8250_data *d = to_dw8250_data(p->private_data);
-
- writeb(value, p->membase + (offset << p->regshift));
+ /* Allow the TX to drain before we reconfigure */
+ if (offset == UART_LCR)
+ dw8250_tx_wait_empty(p);
- if (offset == UART_LCR && !d->uart_16550_compatible)
- dw8250_check_lcr(p, value);
+ dw8250_serial_out(p, offset, value);
}
static unsigned int dw8250_serial_in(struct uart_port *p, int offset)
--
2.30.2
Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
benefit from differentiated ->serial_out() by having big if tree to
select correct write type.
Rework the logic such that the LCR write can be retried within the
relevant ->serial_out() handler:
1. Move retries counter on the caller level and pass as pointer to
dw8250_verify_write()
2. Make dw8250_verify_write() return bool
3. Retry the write on caller level (if needed)
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_dw.c | 59 ++++++++++++++++++-------------
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index fc367d44f86d..f6846363341b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -92,41 +92,36 @@ static void dw8250_force_idle(struct uart_port *p)
* UART_16550_COMPATIBLE=NO or version prior to introducing that option).
* If BUSY is set while writing to LCR register, the write is ignored and
* needs to be retries.
+ *
+ * Returns: false if the caller should retry the write.
*/
-static void dw8250_verify_write(struct uart_port *p, int offset, int value)
+static bool dw8250_verify_write(struct uart_port *p, int offset, int value,
+ unsigned int *retries)
{
- void __iomem *reg_offset = p->membase + (offset << p->regshift);
struct dw8250_data *d = to_dw8250_data(p->private_data);
- int tries = 1000;
+ unsigned int lcr;
if ((offset != UART_LCR) || !d->uart_16550_compatible)
- return;
+ return true;
/* Make sure LCR write wasn't ignored */
- while (tries--) {
- unsigned int lcr = p->serial_in(p, offset);
+ lcr = p->serial_in(p, offset);
- if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
- return;
+ if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
+ return true;
- dw8250_force_idle(p);
+ dw8250_force_idle(p);
-#ifdef CONFIG_64BIT
- if (p->type == PORT_OCTEON)
- __raw_writeq(value & 0xff, reg_offset);
- else
-#endif
- if (p->iotype == UPIO_MEM32)
- writel(value, reg_offset);
- else if (p->iotype == UPIO_MEM32BE)
- iowrite32be(value, reg_offset);
- else
- writeb(value, reg_offset);
+ if (*retries) {
+ *retries -= 1;
+ return false;
}
+
/*
* FIXME: this deadlocks if port->lock is already held
* dev_err(p->dev, "Couldn't set LCR to %d\n", value);
*/
+ return true;
}
/* Returns once the transmitter is empty or we run out of retries */
@@ -155,9 +150,13 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
static void dw8250_serial_out(struct uart_port *p, int offset, int value)
{
+ unsigned int retries = 1000;
+
+retry:
writeb(value, p->membase + (offset << p->regshift));
- dw8250_verify_write(p, offset, value);
+ if (!dw8250_verify_write(p, offset, value, &retries))
+ goto retry;
}
static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
@@ -188,20 +187,29 @@ static unsigned int dw8250_serial_inq(struct uart_port *p, int offset)
static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
{
+ unsigned int retries = 1000;
+
value &= 0xff;
+
+retry:
__raw_writeq(value, p->membase + (offset << p->regshift));
/* Read back to ensure register write ordering. */
__raw_readq(p->membase + (UART_LCR << p->regshift));
- dw8250_verify_write(p, offset, value);
+ if (!dw8250_verify_write(p, offset, value, &retries))
+ goto retry;
}
#endif /* CONFIG_64BIT */
static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
{
+ unsigned int retries = 1000;
+
+retry:
writel(value, p->membase + (offset << p->regshift));
- dw8250_verify_write(p, offset, value);
+ if (!dw8250_verify_write(p, offset, value, &retries))
+ goto retry;
}
static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
@@ -213,10 +221,13 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
{
+ unsigned int retries = 1000;
+retry:
iowrite32be(value, p->membase + (offset << p->regshift));
- dw8250_verify_write(p, offset, value);
+ if (!dw8250_verify_write(p, offset, value, &retries))
+ goto retry;
}
static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
--
2.30.2
On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
<[email protected]> wrote:
>
> Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> benefit from differentiated ->serial_out() by having big if tree to
> select correct write type.
>
> Rework the logic such that the LCR write can be retried within the
> relevant ->serial_out() handler:
> 1. Move retries counter on the caller level and pass as pointer to
> dw8250_verify_write()
> 2. Make dw8250_verify_write() return bool
> 3. Retry the write on caller level (if needed)
I'm wondering if it's possible to utilize one of iopoll.h macro here
instead of copying retries and that not-so-obvious IO poll write.
--
With Best Regards,
Andy Shevchenko
On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 3:43 PM Ilpo J?rvinen
> <[email protected]> wrote:
> >
> > Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> > benefit from differentiated ->serial_out() by having big if tree to
> > select correct write type.
> >
> > Rework the logic such that the LCR write can be retried within the
> > relevant ->serial_out() handler:
> > 1. Move retries counter on the caller level and pass as pointer to
> > dw8250_verify_write()
> > 2. Make dw8250_verify_write() return bool
> > 3. Retry the write on caller level (if needed)
>
> I'm wondering if it's possible to utilize one of iopoll.h macro here
> instead of copying retries and that not-so-obvious IO poll write.
Eh, are you suggesting I should do write as a side-effect inside one of
the iopoll.h macros? Because those available seem to only read?
Or should I create another macro there which writes too?
--
i.
On Wed, 29 Jun 2022, Andy Shevchenko wrote:
> On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
> <[email protected]> wrote:
> > On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> > > On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> > > <[email protected]> wrote:
> > > >
> > > > Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> > > > benefit from differentiated ->serial_out() by having big if tree to
> > > > select correct write type.
> > > >
> > > > Rework the logic such that the LCR write can be retried within the
> > > > relevant ->serial_out() handler:
> > > > 1. Move retries counter on the caller level and pass as pointer to
> > > > dw8250_verify_write()
> > > > 2. Make dw8250_verify_write() return bool
> > > > 3. Retry the write on caller level (if needed)
> > >
> > > I'm wondering if it's possible to utilize one of iopoll.h macro here
> > > instead of copying retries and that not-so-obvious IO poll write.
> >
> > Eh, are you suggesting I should do write as a side-effect inside one of
> > the iopoll.h macros? Because those available seem to only read?
> >
> > Or should I create another macro there which writes too?
>
> It seems to me that it would be a macro on top of iopoll's one which
> will take an op read and op write arguments depending on the case.
The thing is those iopoll macros don't return until the timeout is
exhausted so I don't think I can reuse them easily for this task ("on top
of iopoll's one")? That is, w/o some major side-effect hack (which is
IMHO a no-go).
--
i.
> Note, for that special case you would need a custom write op instead
> of simple __raw_writeq().
>
> Try and if it looks better, convert, otherwise it would be nice to
> hear why it won't fly in your opinion.
On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
<[email protected]> wrote:
> On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> > On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> > <[email protected]> wrote:
> > >
> > > Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> > > benefit from differentiated ->serial_out() by having big if tree to
> > > select correct write type.
> > >
> > > Rework the logic such that the LCR write can be retried within the
> > > relevant ->serial_out() handler:
> > > 1. Move retries counter on the caller level and pass as pointer to
> > > dw8250_verify_write()
> > > 2. Make dw8250_verify_write() return bool
> > > 3. Retry the write on caller level (if needed)
> >
> > I'm wondering if it's possible to utilize one of iopoll.h macro here
> > instead of copying retries and that not-so-obvious IO poll write.
>
> Eh, are you suggesting I should do write as a side-effect inside one of
> the iopoll.h macros? Because those available seem to only read?
>
> Or should I create another macro there which writes too?
It seems to me that it would be a macro on top of iopoll's one which
will take an op read and op write arguments depending on the case.
Note, for that special case you would need a custom write op instead
of simple __raw_writeq().
Try and if it looks better, convert, otherwise it would be nice to
hear why it won't fly in your opinion.
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 29, 2022 at 11:40 AM Ilpo Järvinen
<[email protected]> wrote:
> On Wed, 29 Jun 2022, Andy Shevchenko wrote:
> > On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
> > <[email protected]> wrote:
> > > On Tue, 28 Jun 2022, Andy Shevchenko wrote:
...
> > > Eh, are you suggesting I should do write as a side-effect inside one of
> > > the iopoll.h macros? Because those available seem to only read?
> > >
> > > Or should I create another macro there which writes too?
> >
> > It seems to me that it would be a macro on top of iopoll's one which
> > will take an op read and op write arguments depending on the case.
>
> The thing is those iopoll macros don't return until the timeout is
> exhausted
It returns when the condition is true (in your case verify_lcr is OK).
> so I don't think I can reuse them easily for this task ("on top
> of iopoll's one")? That is, w/o some major side-effect hack (which is
> IMHO a no-go).
Basically what we need is a write-read type of polling.
With your current approach I don't like that retries assignment is
duplicated several times and decrement happens in the callee. What I'm
trying to suggest is to research alternatives.
--
With Best Regards,
Andy Shevchenko