* [PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic
@ 2022-06-28 13:42 Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 1/4] serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x() Ilpo Järvinen
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-28 13:42 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby
Cc: Andy Shevchenko, linux-kernel, Ilpo Järvinen
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/4] serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x()
2022-06-28 13:42 [PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic Ilpo Järvinen
@ 2022-06-28 13:42 ` Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset Ilpo Järvinen
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-28 13:42 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby, Ilpo Järvinen,
Andy Shevchenko, linux-kernel
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 <ilpo.jarvinen@linux.intel.com>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset
2022-06-28 13:42 [PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 1/4] serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x() Ilpo Järvinen
@ 2022-06-28 13:42 ` Ilpo Järvinen
2022-06-28 21:25 ` Andy Shevchenko
2022-06-28 13:42 ` [PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write() Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic Ilpo Järvinen
3 siblings, 1 reply; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-28 13:42 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby, Ilpo Järvinen,
Andy Shevchenko, linux-kernel
Get 'offset' variable out of the way of parameter named 'offset',
rename it to 'reg_offset'. This is very short lived change as
reg_offset is going to be soon removed.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/tty/serial/8250/8250_dw.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 41bf063396e4..f18975b4d2c7 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -89,7 +89,7 @@ static void dw8250_force_idle(struct uart_port *p)
static void dw8250_check_lcr(struct uart_port *p, int value)
{
- void __iomem *offset = p->membase + (UART_LCR << p->regshift);
+ void __iomem *reg_offset = p->membase + (UART_LCR << p->regshift);
int tries = 1000;
/* Make sure LCR write wasn't ignored */
@@ -103,15 +103,15 @@ static void dw8250_check_lcr(struct uart_port *p, int value)
#ifdef CONFIG_64BIT
if (p->type == PORT_OCTEON)
- __raw_writeq(value & 0xff, offset);
+ __raw_writeq(value & 0xff, reg_offset);
else
#endif
if (p->iotype == UPIO_MEM32)
- writel(value, offset);
+ writel(value, reg_offset);
else if (p->iotype == UPIO_MEM32BE)
- iowrite32be(value, offset);
+ iowrite32be(value, reg_offset);
else
- writeb(value, offset);
+ writeb(value, reg_offset);
}
/*
* FIXME: this deadlocks if port->lock is already held
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write()
2022-06-28 13:42 [PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 1/4] serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x() Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset Ilpo Järvinen
@ 2022-06-28 13:42 ` Ilpo Järvinen
2022-06-28 20:06 ` Andy Shevchenko
2022-06-28 13:42 ` [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic Ilpo Järvinen
3 siblings, 1 reply; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-28 13:42 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby, Ilpo Järvinen,
Andy Shevchenko, linux-kernel
Rename dw8250_check_lcr() -> dw8250_verify_write() and add comment.
Move LCR and 16550_compatible checks there. As offset is now passed and
dw8250_verify_write() ensures it's UART_LCR, offset can use used
instead of explicit UART_LCR.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/tty/serial/8250/8250_dw.c | 35 +++++++++++++++----------------
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f18975b4d2c7..fc367d44f86d 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -87,14 +87,24 @@ static void dw8250_force_idle(struct uart_port *p)
(void)p->serial_in(p, UART_RX);
}
-static void dw8250_check_lcr(struct uart_port *p, int value)
+/*
+ * DW UART can be configured to indicate BUSY in USR (with
+ * 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.
+ */
+static void dw8250_verify_write(struct uart_port *p, int offset, int value)
{
- void __iomem *reg_offset = p->membase + (UART_LCR << p->regshift);
+ void __iomem *reg_offset = p->membase + (offset << p->regshift);
+ struct dw8250_data *d = to_dw8250_data(p->private_data);
int tries = 1000;
+ if ((offset != UART_LCR) || !d->uart_16550_compatible)
+ return;
+
/* Make sure LCR write wasn't ignored */
while (tries--) {
- unsigned int lcr = p->serial_in(p, UART_LCR);
+ unsigned int lcr = p->serial_in(p, offset);
if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
return;
@@ -145,12 +155,9 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
static void dw8250_serial_out(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));
- if (offset == UART_LCR && !d->uart_16550_compatible)
- dw8250_check_lcr(p, value);
+ dw8250_verify_write(p, offset, value);
}
static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
@@ -181,26 +188,20 @@ 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)
{
- struct dw8250_data *d = to_dw8250_data(p->private_data);
-
value &= 0xff;
__raw_writeq(value, p->membase + (offset << p->regshift));
/* Read back to ensure register write ordering. */
__raw_readq(p->membase + (UART_LCR << p->regshift));
- if (offset == UART_LCR && !d->uart_16550_compatible)
- dw8250_check_lcr(p, value);
+ dw8250_verify_write(p, offset, value);
}
#endif /* CONFIG_64BIT */
static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
{
- struct dw8250_data *d = to_dw8250_data(p->private_data);
-
writel(value, p->membase + (offset << p->regshift));
- if (offset == UART_LCR && !d->uart_16550_compatible)
- dw8250_check_lcr(p, value);
+ dw8250_verify_write(p, offset, value);
}
static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
@@ -212,12 +213,10 @@ 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)
{
- struct dw8250_data *d = to_dw8250_data(p->private_data);
iowrite32be(value, p->membase + (offset << p->regshift));
- if (offset == UART_LCR && !d->uart_16550_compatible)
- dw8250_check_lcr(p, value);
+ dw8250_verify_write(p, offset, value);
}
static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic
2022-06-28 13:42 [PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic Ilpo Järvinen
` (2 preceding siblings ...)
2022-06-28 13:42 ` [PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write() Ilpo Järvinen
@ 2022-06-28 13:42 ` Ilpo Järvinen
2022-06-28 20:11 ` Andy Shevchenko
3 siblings, 1 reply; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-28 13:42 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby, Ilpo Järvinen,
Andy Shevchenko, linux-kernel
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 <ilpo.jarvinen@linux.intel.com>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write()
2022-06-28 13:42 ` [PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write() Ilpo Järvinen
@ 2022-06-28 20:06 ` Andy Shevchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2022-06-28 20:06 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: open list:SERIAL DRIVERS, Greg KH, Jiri Slaby, Andy Shevchenko,
Linux Kernel Mailing List
On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> Rename dw8250_check_lcr() -> dw8250_verify_write() and add comment.
> Move LCR and 16550_compatible checks there. As offset is now passed and
> dw8250_verify_write() ensures it's UART_LCR, offset can use used
> instead of explicit UART_LCR.
...
> +/*
> + * DW UART can be configured to indicate BUSY in USR (with
> + * 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.
retried
> + */
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic
2022-06-28 13:42 ` [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic Ilpo Järvinen
@ 2022-06-28 20:11 ` Andy Shevchenko
2022-06-29 8:47 ` Ilpo Järvinen
0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2022-06-28 20:11 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: open list:SERIAL DRIVERS, Greg KH, Jiri Slaby, Andy Shevchenko,
Linux Kernel Mailing List
On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset
2022-06-28 13:42 ` [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset Ilpo Järvinen
@ 2022-06-28 21:25 ` Andy Shevchenko
2022-06-29 7:47 ` Ilpo Järvinen
0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2022-06-28 21:25 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: linux-serial, Greg KH, Jiri Slaby, linux-kernel
On Tue, Jun 28, 2022 at 04:42:32PM +0300, Ilpo Järvinen wrote:
> Get 'offset' variable out of the way of parameter named 'offset',
> rename it to 'reg_offset'. This is very short lived change as
> reg_offset is going to be soon removed.
I'm not sure why this change then even needed...
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset
2022-06-28 21:25 ` Andy Shevchenko
@ 2022-06-29 7:47 ` Ilpo Järvinen
2022-06-29 9:38 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-29 7:47 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-serial, Greg KH, Jiri Slaby, LKML
[-- Attachment #1: Type: text/plain, Size: 833 bytes --]
On Wed, 29 Jun 2022, Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 04:42:32PM +0300, Ilpo Järvinen wrote:
> > Get 'offset' variable out of the way of parameter named 'offset',
> > rename it to 'reg_offset'. This is very short lived change as
> > reg_offset is going to be soon removed.
>
> I'm not sure why this change then even needed...
I could either:
1) create one large patch doing many thing (2+3 or 2+3+4)
or
2) add the 'offset' parameter with some other name first and rename it
to its final name after local var 'offset' is eliminated by patch 4
or
3) rename local var 'offset' first out of the way so that I can add
'offset' parameter in patch 3 (=this patch)
If I just drop patch 2 and only do 3, it won't build because 'offset'
variable appears twice (as arg and local var).
--
i.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic
2022-06-28 20:11 ` Andy Shevchenko
@ 2022-06-29 8:47 ` Ilpo Järvinen
2022-06-29 9:33 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-29 8:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:SERIAL DRIVERS, Greg KH, Jiri Slaby, Andy Shevchenko,
Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 980 bytes --]
On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic
2022-06-29 8:47 ` Ilpo Järvinen
@ 2022-06-29 9:33 ` Andy Shevchenko
2022-06-29 9:40 ` Ilpo Järvinen
0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2022-06-29 9:33 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: open list:SERIAL DRIVERS, Greg KH, Jiri Slaby, Andy Shevchenko,
Linux Kernel Mailing List
On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> > On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset
2022-06-29 7:47 ` Ilpo Järvinen
@ 2022-06-29 9:38 ` Andy Shevchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2022-06-29 9:38 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Andy Shevchenko, linux-serial, Greg KH, Jiri Slaby, LKML
On Wed, Jun 29, 2022 at 9:52 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Wed, 29 Jun 2022, Andy Shevchenko wrote:
>
> > On Tue, Jun 28, 2022 at 04:42:32PM +0300, Ilpo Järvinen wrote:
> > > Get 'offset' variable out of the way of parameter named 'offset',
> > > rename it to 'reg_offset'. This is very short lived change as
a very
> > > reg_offset is going to be soon removed.
> >
> > I'm not sure why this change then even needed...
>
> I could either:
> 1) create one large patch doing many thing (2+3 or 2+3+4)
> or
> 2) add the 'offset' parameter with some other name first and rename it
> to its final name after local var 'offset' is eliminated by patch 4
> or
> 3) rename local var 'offset' first out of the way so that I can add
> 'offset' parameter in patch 3 (=this patch)
>
> If I just drop patch 2 and only do 3, it won't build because 'offset'
> variable appears twice (as arg and local var).
Now I got it, thanks. See one remark above.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic
2022-06-29 9:33 ` Andy Shevchenko
@ 2022-06-29 9:40 ` Ilpo Järvinen
2022-06-29 9:51 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Ilpo Järvinen @ 2022-06-29 9:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:SERIAL DRIVERS, Greg KH, Jiri Slaby, Andy Shevchenko,
Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]
On Wed, 29 Jun 2022, Andy Shevchenko wrote:
> On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> > > On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic
2022-06-29 9:40 ` Ilpo Järvinen
@ 2022-06-29 9:51 ` Andy Shevchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2022-06-29 9:51 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: open list:SERIAL DRIVERS, Greg KH, Jiri Slaby, Andy Shevchenko,
Linux Kernel Mailing List
On Wed, Jun 29, 2022 at 11:40 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Wed, 29 Jun 2022, Andy Shevchenko wrote:
> > On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-06-29 9:52 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-28 13:42 [PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 1/4] serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x() Ilpo Järvinen
2022-06-28 13:42 ` [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset Ilpo Järvinen
2022-06-28 21:25 ` Andy Shevchenko
2022-06-29 7:47 ` Ilpo Järvinen
2022-06-29 9:38 ` Andy Shevchenko
2022-06-28 13:42 ` [PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write() Ilpo Järvinen
2022-06-28 20:06 ` Andy Shevchenko
2022-06-28 13:42 ` [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic Ilpo Järvinen
2022-06-28 20:11 ` Andy Shevchenko
2022-06-29 8:47 ` Ilpo Järvinen
2022-06-29 9:33 ` Andy Shevchenko
2022-06-29 9:40 ` Ilpo Järvinen
2022-06-29 9:51 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).