* Re: [PATCH v1 2/7] serdev: add dev_pm_domain_attach|detach()
From: Sean Wang @ 2018-04-26 5:29 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Rob Herring, Mark Rutland, Johan Hedberg, devicetree,
Bluez mailing list, linux-arm-kernel, linux-mediatek, LKML,
Rob Herring, Ulf Hansson, Greg Kroah-Hartman, Jiri Slaby,
linux-serial
In-Reply-To: <FD85DED4-589C-4113-8A8B-BC5A39494130@holtmann.org>
On Tue, 2018-04-03 at 12:29 +0200, Marcel Holtmann wrote:
> Hi Sean,
>
> > In order to open up the required power gate before any operation can be
> > effectively performed over the serial bus between CPU and serdev, it's
> > clearly essential to add common attach functions for PM domains to serdev
> > at the probe phase.
> >
> > Similarly, the relevant dettach function for the PM domains should be
> > properly and reversely added at the remove phase.
> >
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: linux-serial@vger.kernel.org
> > ---
> > drivers/tty/serdev/core.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index df93b72..c93d8ee 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/serdev.h>
> > #include <linux/slab.h>
> >
> > @@ -330,8 +331,16 @@ EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
> > static int serdev_drv_probe(struct device *dev)
> > {
> > const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> > + int ret;
> > +
> > + ret = dev_pm_domain_attach(dev, true);
> > + if (ret != -EPROBE_DEFER) {
> > + ret = sdrv->probe(to_serdev_device(dev));
> > + if (ret)
> > + dev_pm_domain_detach(dev, true);
> > + }
>
> so if this is deferred, when does the serdev device gets probed?
>
driver probe deferral mechanism is supported in driver core
deferred_probe_initcall makes sure that deferred probing is
delayed until late_initcall time.
Below is a few of word I got from drivers/base/core.c I thought it helps
to understand the mechanism in complete picture
* If a required resource is not available yet, a driver can
request probing to be deferred by returning -EPROBE_DEFER from
its probe hook.
* A driver returning -EPROBE_DEFER causes the device to be added to the
pending list. A successful driver probe will trigger moving all devices
from the pending to the active list so that the workqueue will
eventually retry them.
> >
> > - return sdrv->probe(to_serdev_device(dev));
> > + return ret;
> > }
> >
> > static int serdev_drv_remove(struct device *dev)
> > @@ -339,6 +348,9 @@ static int serdev_drv_remove(struct device *dev)
> > const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> > if (sdrv->remove)
> > sdrv->remove(to_serdev_device(dev));
> > +
> > + dev_pm_domain_detach(dev, true);
> > +
> > return 0;
> > }
>
> Regards
>
> Marcel
>
^ permalink raw reply
* Re: [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
From: Andrew Jones @ 2018-04-25 14:59 UTC (permalink / raw)
To: Dave Martin
Cc: Peter Maydell, Ciro Santilli, Linus Walleij, Russell King, Wei Xu,
linux-serial, arm-mail-list
In-Reply-To: <20180425141010.GO16308@e103592.cambridge.arm.com>
On Wed, Apr 25, 2018 at 03:10:14PM +0100, Dave Martin wrote:
> On Wed, Apr 25, 2018 at 01:47:42PM +0100, Ciro Santilli wrote:
> > On Wed, Apr 25, 2018 at 1:20 PM, Andrew Jones <drjones@redhat.com> wrote:
> > > On Mon, Apr 23, 2018 at 02:49:30PM +0100, Peter Maydell wrote:
> > >> On 23 April 2018 at 14:41, Dave Martin <Dave.Martin@arm.com> wrote:
> > >> > This is an update to a previous RFC v2 [1], to fix a problem observed by
> > >> > the qemu community that causes serial input to hang when booting a
> > >> > simulated system with data already queued in the UART FIFO [2].
> > >> >
> > >> > After discussion, I decided that the approach in [1] was over-
> > >> > engineered: it tries to preserve a guarantee that people shouldn't be
> > >> > relying on anyway, namely that data sent to the UART prior to kernel
> > >> > boot will be received by the kernel; or more generally that data
> > >> > received by the UART while the pl011 driver is not opened will be
> > >> > received (either intact or at all) by the driver.
> > >> >
> > >> > If anyone can please test the following and let me know the results,
> > >> > that would be much appreciated!
> > >> >
> > >> > a) Check that you can still reproduce the bug on mainline without this
> > >> > patch.
> > >> >
> > >> > b) Check whether this patch fixes the problem.
> > >>
> > >> Thanks. I'm cc'ing Ciro and Drew, who are the two people I
> > >> recall reporting this issue to me.
> > >> Link to the patch for their benefit:
> > >> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html
> > >>
> > >
> > > Hi Dave,
> > >
> > > v3 does not resolve the issue for me. v2 does, and, fwiw, v3 applied on
> > > top of v2 (i.e. applying both), still works.
> > >
> >
> > I also confirm that v2 + v3 on top of Linux kernel v4.16, QEMU v2.11.0
> > solves the problem on arm and aarch64, otherwise if I hit Ctrl + C
> > during boot my terminal become irresponsive after boot. Test setup:
> > https://github.com/cirosantilli/linux-kernel-module-cheat/tree/14965a40d27c8d9d1ff5b023ace827b288a024ef
>
> Hmmm, interesting.
>
> Looking at the code, it looks like RXIS is explicitly cleared twice,
> once in pl011_hwinit() and once in pl011_startup(). The CONFIG_POLL
> code uses hwinit alone, and I'm not sure exactly what properties it
> relies on.
>
> To be most robust, perhaps we should drain the RX FIFO in both places.
>
> Can you try applying this on top of the branch and see what happens?
>
> Cheers
> ---Dave
>
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 73e6f44..841afbd 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1652,6 +1652,19 @@ static void pl011_put_poll_char(struct uart_port *port,
>
> #endif /* CONFIG_CONSOLE_POLL */
>
> +/*
> + * RXIS is asserted only when the RX FIFO transitions from below to
> + * above the trigger threshold. If the RX FIFO is already full to the
> + * threshold this can't happen and RXIS will now be stuck off.
> + * Unless polling the UART, use this function to drain the RX FIFO
> + * explicitly after clearing RXIS.
> + */
> +static void pl011_drain_rx_fifo(struct uart_amba_port *uap)
> +{
> + while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
> + pl011_read(uap, REG_DR);
> +}
> +
> static int pl011_hwinit(struct uart_port *port)
> {
> struct uart_amba_port *uap =
> @@ -1674,15 +1687,7 @@ static int pl011_hwinit(struct uart_port *port)
> pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS |
> UART011_FEIS | UART011_RTIS | UART011_RXIS,
> uap, REG_ICR);
> -
> - /*
> - * RXIS is asserted only when the RX FIFO transitions from below
> - * to above the trigger threshold. If the RX FIFO is already
> - * full to the threshold this can't happen and RXIS will now be
> - * stuck off. Drain the FIFO explicitly to fix this:
> - */
> - while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
> - pl011_read(uap, REG_DR);
> + pl011_drain_rx_fifo(uap);
>
> /*
> * Save interrupts enable mask, and enable RX interrupts in case if
> @@ -1740,6 +1745,8 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
>
> /* Clear out any spuriously appearing RX interrupts */
> pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
> + pl011_drain_rx_fifo(uap);
> +
> uap->im = UART011_RTIM;
> if (!pl011_dma_rx_running(uap))
> uap->im |= UART011_RXIM;
>
>
>
This worked for me. To be clear, I applied the following, and nothing
else, to my base kernel for the test
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 111e6a9..d64b64f 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1672,6 +1672,19 @@ static void pl011_put_poll_char(struct uart_port *port,
#endif /* CONFIG_CONSOLE_POLL */
+/*
+ * RXIS is asserted only when the RX FIFO transitions from below to
+ * above the trigger threshold. If the RX FIFO is already full to the
+ * threshold this can't happen and RXIS will now be stuck off.
+ * Unless polling the UART, use this function to drain the RX FIFO
+ * explicitly after clearing RXIS.
+ */
+static void pl011_drain_rx_fifo(struct uart_amba_port *uap)
+{
+ while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
+ pl011_read(uap, REG_DR);
+}
+
static int pl011_hwinit(struct uart_port *port)
{
struct uart_amba_port *uap =
@@ -1695,6 +1708,8 @@ static int pl011_hwinit(struct uart_port *port)
UART011_FEIS | UART011_RTIS | UART011_RXIS,
uap, REG_ICR);
+ pl011_drain_rx_fifo(uap);
+
/*
* Save interrupts enable mask, and enable RX interrupts in case if
* the interrupt is used for NMI entry.
@@ -1751,6 +1766,8 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
/* Clear out any spuriously appearing RX interrupts */
pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
+ pl011_drain_rx_fifo(uap);
+
uap->im = UART011_RTIM;
if (!pl011_dma_rx_running(uap))
uap->im |= UART011_RXIM;
Thanks,
drew
^ permalink raw reply related
* Re: [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
From: Dave Martin @ 2018-04-25 14:10 UTC (permalink / raw)
To: Ciro Santilli
Cc: Peter Maydell, Andrew Jones, Linus Walleij, Russell King, Wei Xu,
linux-serial, arm-mail-list
In-Reply-To: <CAFXrp_djBYwrp664A8tLrqZDw=3L6w4Uadk-Rin_ZTwfE2QwGw@mail.gmail.com>
On Wed, Apr 25, 2018 at 01:47:42PM +0100, Ciro Santilli wrote:
> On Wed, Apr 25, 2018 at 1:20 PM, Andrew Jones <drjones@redhat.com> wrote:
> > On Mon, Apr 23, 2018 at 02:49:30PM +0100, Peter Maydell wrote:
> >> On 23 April 2018 at 14:41, Dave Martin <Dave.Martin@arm.com> wrote:
> >> > This is an update to a previous RFC v2 [1], to fix a problem observed by
> >> > the qemu community that causes serial input to hang when booting a
> >> > simulated system with data already queued in the UART FIFO [2].
> >> >
> >> > After discussion, I decided that the approach in [1] was over-
> >> > engineered: it tries to preserve a guarantee that people shouldn't be
> >> > relying on anyway, namely that data sent to the UART prior to kernel
> >> > boot will be received by the kernel; or more generally that data
> >> > received by the UART while the pl011 driver is not opened will be
> >> > received (either intact or at all) by the driver.
> >> >
> >> > If anyone can please test the following and let me know the results,
> >> > that would be much appreciated!
> >> >
> >> > a) Check that you can still reproduce the bug on mainline without this
> >> > patch.
> >> >
> >> > b) Check whether this patch fixes the problem.
> >>
> >> Thanks. I'm cc'ing Ciro and Drew, who are the two people I
> >> recall reporting this issue to me.
> >> Link to the patch for their benefit:
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html
> >>
> >
> > Hi Dave,
> >
> > v3 does not resolve the issue for me. v2 does, and, fwiw, v3 applied on
> > top of v2 (i.e. applying both), still works.
> >
>
> I also confirm that v2 + v3 on top of Linux kernel v4.16, QEMU v2.11.0
> solves the problem on arm and aarch64, otherwise if I hit Ctrl + C
> during boot my terminal become irresponsive after boot. Test setup:
> https://github.com/cirosantilli/linux-kernel-module-cheat/tree/14965a40d27c8d9d1ff5b023ace827b288a024ef
Hmmm, interesting.
Looking at the code, it looks like RXIS is explicitly cleared twice,
once in pl011_hwinit() and once in pl011_startup(). The CONFIG_POLL
code uses hwinit alone, and I'm not sure exactly what properties it
relies on.
To be most robust, perhaps we should drain the RX FIFO in both places.
Can you try applying this on top of the branch and see what happens?
Cheers
---Dave
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 73e6f44..841afbd 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1652,6 +1652,19 @@ static void pl011_put_poll_char(struct uart_port *port,
#endif /* CONFIG_CONSOLE_POLL */
+/*
+ * RXIS is asserted only when the RX FIFO transitions from below to
+ * above the trigger threshold. If the RX FIFO is already full to the
+ * threshold this can't happen and RXIS will now be stuck off.
+ * Unless polling the UART, use this function to drain the RX FIFO
+ * explicitly after clearing RXIS.
+ */
+static void pl011_drain_rx_fifo(struct uart_amba_port *uap)
+{
+ while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
+ pl011_read(uap, REG_DR);
+}
+
static int pl011_hwinit(struct uart_port *port)
{
struct uart_amba_port *uap =
@@ -1674,15 +1687,7 @@ static int pl011_hwinit(struct uart_port *port)
pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS |
UART011_FEIS | UART011_RTIS | UART011_RXIS,
uap, REG_ICR);
-
- /*
- * RXIS is asserted only when the RX FIFO transitions from below
- * to above the trigger threshold. If the RX FIFO is already
- * full to the threshold this can't happen and RXIS will now be
- * stuck off. Drain the FIFO explicitly to fix this:
- */
- while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
- pl011_read(uap, REG_DR);
+ pl011_drain_rx_fifo(uap);
/*
* Save interrupts enable mask, and enable RX interrupts in case if
@@ -1740,6 +1745,8 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
/* Clear out any spuriously appearing RX interrupts */
pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
+ pl011_drain_rx_fifo(uap);
+
uap->im = UART011_RTIM;
if (!pl011_dma_rx_running(uap))
uap->im |= UART011_RXIM;
^ permalink raw reply related
* [PATCH] earlycon: Remove hardcoded port->uartclk initialization in of_setup_earlycon
From: Michal Simek @ 2018-04-25 13:48 UTC (permalink / raw)
To: linux-kernel, monstr, devicetree, Rob Herring, matt.redfearn
Cc: Greg Kroah-Hartman, linux-serial, Jiri Slaby
There is no reason to initialize uartclk to BASE_BAUD * 16 for DT based
systems.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Tested-by: Matt Redfearn <matt.redfearn@mips.com>
---
It looks like from history that portclk = BASE_BAUD * 16 was setup to
get on calculation on x86 (divisor = 1) but it shouldn't be needed on DT based
system. That's why I think that there is no DT based system which really
requires this line.
---
drivers/tty/serial/earlycon.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a24278380fec..c12b1edcdd8e 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -244,7 +244,6 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
return -ENXIO;
}
port->mapbase = addr;
- port->uartclk = BASE_BAUD * 16;
val = of_get_flat_dt_prop(node, "reg-offset", NULL);
if (val)
--
2.17.0
^ permalink raw reply related
* Re: [PATCH] serial: 8250_early: Setup divider when uartclk is passed
From: Greg Kroah-Hartman @ 2018-04-25 12:51 UTC (permalink / raw)
To: Michal Simek
Cc: linux-kernel, monstr, stable, Matt Redfearn, Jiri Slaby,
Douglas Anderson, linux-serial, Jeffy Chen
In-Reply-To: <93a7871c8654a6273b1ab35a8071e9f830e2a0c8.1524475087.git.michal.simek@xilinx.com>
On Mon, Apr 23, 2018 at 11:18:11AM +0200, Michal Simek wrote:
> device->baud is always non zero value because it is checked already in
> early_serial8250_setup() before init_port is called.
>
> Fixes: 0ff3ab701963 ("serial: 8250_early: Only set divisor if valid clk & baud")
> Cc: stable <stable@vger.kernel.org>
This isn't "fixing" anything, as the logic is identical before, or
after, this patch is committed. So I'll apply this, but will drop these
two tags.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
From: Ciro Santilli @ 2018-04-25 12:47 UTC (permalink / raw)
To: Andrew Jones
Cc: Peter Maydell, Linus Walleij, Russell King, Wei Xu, linux-serial,
Dave Martin, arm-mail-list
In-Reply-To: <20180425122000.cts26gtvjuje4j7f@kamzik.brq.redhat.com>
On Wed, Apr 25, 2018 at 1:20 PM, Andrew Jones <drjones@redhat.com> wrote:
> On Mon, Apr 23, 2018 at 02:49:30PM +0100, Peter Maydell wrote:
>> On 23 April 2018 at 14:41, Dave Martin <Dave.Martin@arm.com> wrote:
>> > This is an update to a previous RFC v2 [1], to fix a problem observed by
>> > the qemu community that causes serial input to hang when booting a
>> > simulated system with data already queued in the UART FIFO [2].
>> >
>> > After discussion, I decided that the approach in [1] was over-
>> > engineered: it tries to preserve a guarantee that people shouldn't be
>> > relying on anyway, namely that data sent to the UART prior to kernel
>> > boot will be received by the kernel; or more generally that data
>> > received by the UART while the pl011 driver is not opened will be
>> > received (either intact or at all) by the driver.
>> >
>> > If anyone can please test the following and let me know the results,
>> > that would be much appreciated!
>> >
>> > a) Check that you can still reproduce the bug on mainline without this
>> > patch.
>> >
>> > b) Check whether this patch fixes the problem.
>>
>> Thanks. I'm cc'ing Ciro and Drew, who are the two people I
>> recall reporting this issue to me.
>> Link to the patch for their benefit:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html
>>
>
> Hi Dave,
>
> v3 does not resolve the issue for me. v2 does, and, fwiw, v3 applied on
> top of v2 (i.e. applying both), still works.
>
I also confirm that v2 + v3 on top of Linux kernel v4.16, QEMU v2.11.0
solves the problem on arm and aarch64, otherwise if I hit Ctrl + C
during boot my terminal become irresponsive after boot. Test setup:
https://github.com/cirosantilli/linux-kernel-module-cheat/tree/14965a40d27c8d9d1ff5b023ace827b288a024ef
> If you choose to go with v2, then you can add my tested-by.
>
> Thanks,
> drew
^ permalink raw reply
* Re: [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
From: Andrew Jones @ 2018-04-25 12:20 UTC (permalink / raw)
To: Peter Maydell
Cc: Ciro Santilli, Linus Walleij, Russell King, Wei Xu, linux-serial,
Dave Martin, arm-mail-list
In-Reply-To: <CAFEAcA8Q97YNKomCgkujgoY0MoES3edHoFEk_jjXGMCdzBpd9w@mail.gmail.com>
On Mon, Apr 23, 2018 at 02:49:30PM +0100, Peter Maydell wrote:
> On 23 April 2018 at 14:41, Dave Martin <Dave.Martin@arm.com> wrote:
> > This is an update to a previous RFC v2 [1], to fix a problem observed by
> > the qemu community that causes serial input to hang when booting a
> > simulated system with data already queued in the UART FIFO [2].
> >
> > After discussion, I decided that the approach in [1] was over-
> > engineered: it tries to preserve a guarantee that people shouldn't be
> > relying on anyway, namely that data sent to the UART prior to kernel
> > boot will be received by the kernel; or more generally that data
> > received by the UART while the pl011 driver is not opened will be
> > received (either intact or at all) by the driver.
> >
> > If anyone can please test the following and let me know the results,
> > that would be much appreciated!
> >
> > a) Check that you can still reproduce the bug on mainline without this
> > patch.
> >
> > b) Check whether this patch fixes the problem.
>
> Thanks. I'm cc'ing Ciro and Drew, who are the two people I
> recall reporting this issue to me.
> Link to the patch for their benefit:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html
>
Hi Dave,
v3 does not resolve the issue for me. v2 does, and, fwiw, v3 applied on
top of v2 (i.e. applying both), still works.
If you choose to go with v2, then you can add my tested-by.
Thanks,
drew
^ permalink raw reply
* Re: [RFC][PATCH 0/1] serdev: Support HS-UART serdev slaves over tty
From: Johan Hovold @ 2018-04-25 9:12 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Johan Hovold, Shrirang Bagul, Hans de Goede, linux-serial, gregkh,
Rob Herring, Frédéric Danis, Lukas Wunner, linux-kernel,
linux-acpi
In-Reply-To: <1821148.enBAg0iGPe@aspire.rjw.lan>
On Wed, Apr 25, 2018 at 11:00:47AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, April 25, 2018 9:47:36 AM CEST Johan Hovold wrote:
> > On Tue, Apr 24, 2018 at 11:16:38PM +0200, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 24-04-18 19:18, Johan Hovold wrote:
> > > > [ Adding some more people on CC. ]
> > > >
> > > > On Tue, Apr 24, 2018 at 04:29:53PM +0800, Shrirang Bagul wrote:
> > > >> On systems using Intel Atom (Baytrail-I) SoC's, slave devices connected on
> > > >> HSUART1/2 ports are described by the ACPI BIOS as virtual hardware using
> > > >> HID's INT3511/INT3512 [1].
> > > >>
> > > >> As a consequence, HW manufacturers have complete freedom to install any
> > > >> devices on-board as long as they can be accessed over serial tty
> > > >> interface. Once such device is Dell Edge 3002 IoT Gateway which sports
> > > >> ZigBee & GPS devices on the HS-UART ports 1 & 2 respectively.
> > > >>
> > > >> In kernels before the introduction of 'Serial Device Bus (serdev)'
> > > >> subsystem, these devices were accessible using /dev/ttySx nodes. But,
> > > >> kernels since 4.15 can no longer do so.
> > > >>
> > > >> Post 4.15, with CONFIG_SERIAL_DEV_BUS=y, serdev port controller driver
> > > >> handles the enumeration for the slaves connected on these ports. Also,
> > > >> /dev/ttySx device nodes for these ports are no longer exposed to the
> > > >> userspace.
> > > >>
> > > >> This patch implements a new driver which binds to the ACPI serdev slaves
> > > >> enumerated by the serdev port controller and exposes /dev/ttyHSx device
> > > >> nodes which the userspace applications can use. Otherwise, upgrades to 4.15
> > > >> or higher kernels would certainly render these devices unusable.
> > > >>
> > > >> Considering serdev is new and evolving, this is one approach to solving
> > > >> the problem at hand. An obvious drawback is the change in the tty device
> > > >> node name from ttySx => ttyHSx, which means userspace applications have to
> > > >> be modified (I know that this is strongly discouraged). For the same
> > > >> reason, I am submitting these patches as RFC.
> > > >>
> > > >> If there are other/better ways of solving this or improving on the
> > > >> proposed solution, that will be most helpful.
> > > >
> > > > Yeah, I don't think this is the right solution to this problem. It seems
> > > > we need to blacklist (or maybe even use whitelists) ACPI-ids until there
> > > > are drivers for the slave devices that would otherwise be claimed by
> > > > serdev.
> > >
> > > FWIW I've been using this patch for a while for realtek UART attached bluetooth:
> > > https://github.com/jwrdegoede/linux-sunxi/commit/bc904e3703940600ca66c65fcdb0a8cb01dff55d
> > > which is a gross hack.
> > >
> > > If we're going to do a whitelist for this, it better support some sort of
> > > wildcards as there are a LOT of BCM2E?? devices which need to be on the
> > > whitelist. I think a blacklist would actually be better though, this also
> > > documents which devices are lacking a proper kernel (where applicable).
> >
> > Yeah, you guys know the ACPI space better than I do. I just fear that
> > if we go with the blacklist approach, we'll be playing a whack-a-mole
> > with this for a long time when people start upgrading there systems to
> > 4.15 and discover that their serial ports are gone.
> >
> > Since this would qualify as a severe regression, me may need to consider
> > adding a serdev whitelist for every ACPI id we add to a serdev driver
> > instead.
>
> OK, so let's have the ACPI discussion on linux-acpi pretty please.
Oh, sorry, I forgot to add linux-acpi.
> Could this be resent with a CC to linux-acpi for some more complete
> context?
Fortunately, I think the relevant context is still there above.
In short, when we added ACPI support to serdev, serdev started claiming
all slave devices to UARTs that happen to described by ACPI. Regardless
of whether there's a kernel driver for them or not. This means that
serial ports (/dev/ttySn) will simply start disappearing on such systems
when updating to 4.15 unless we act somehow (blacklist or whitelist).
Thanks,
Johan
^ permalink raw reply
* Re: [RFC][PATCH 0/1] serdev: Support HS-UART serdev slaves over tty
From: Rafael J. Wysocki @ 2018-04-25 9:00 UTC (permalink / raw)
To: Johan Hovold, Shrirang Bagul
Cc: Hans de Goede, linux-serial, gregkh, Rob Herring,
Frédéric Danis, Lukas Wunner, linux-kernel, linux-acpi
In-Reply-To: <20180425074736.GK4615@localhost>
On Wednesday, April 25, 2018 9:47:36 AM CEST Johan Hovold wrote:
> On Tue, Apr 24, 2018 at 11:16:38PM +0200, Hans de Goede wrote:
> > Hi,
> >
> > On 24-04-18 19:18, Johan Hovold wrote:
> > > [ Adding some more people on CC. ]
> > >
> > > On Tue, Apr 24, 2018 at 04:29:53PM +0800, Shrirang Bagul wrote:
> > >> On systems using Intel Atom (Baytrail-I) SoC's, slave devices connected on
> > >> HSUART1/2 ports are described by the ACPI BIOS as virtual hardware using
> > >> HID's INT3511/INT3512 [1].
> > >>
> > >> As a consequence, HW manufacturers have complete freedom to install any
> > >> devices on-board as long as they can be accessed over serial tty
> > >> interface. Once such device is Dell Edge 3002 IoT Gateway which sports
> > >> ZigBee & GPS devices on the HS-UART ports 1 & 2 respectively.
> > >>
> > >> In kernels before the introduction of 'Serial Device Bus (serdev)'
> > >> subsystem, these devices were accessible using /dev/ttySx nodes. But,
> > >> kernels since 4.15 can no longer do so.
> > >>
> > >> Post 4.15, with CONFIG_SERIAL_DEV_BUS=y, serdev port controller driver
> > >> handles the enumeration for the slaves connected on these ports. Also,
> > >> /dev/ttySx device nodes for these ports are no longer exposed to the
> > >> userspace.
> > >>
> > >> This patch implements a new driver which binds to the ACPI serdev slaves
> > >> enumerated by the serdev port controller and exposes /dev/ttyHSx device
> > >> nodes which the userspace applications can use. Otherwise, upgrades to 4.15
> > >> or higher kernels would certainly render these devices unusable.
> > >>
> > >> Considering serdev is new and evolving, this is one approach to solving
> > >> the problem at hand. An obvious drawback is the change in the tty device
> > >> node name from ttySx => ttyHSx, which means userspace applications have to
> > >> be modified (I know that this is strongly discouraged). For the same
> > >> reason, I am submitting these patches as RFC.
> > >>
> > >> If there are other/better ways of solving this or improving on the
> > >> proposed solution, that will be most helpful.
> > >
> > > Yeah, I don't think this is the right solution to this problem. It seems
> > > we need to blacklist (or maybe even use whitelists) ACPI-ids until there
> > > are drivers for the slave devices that would otherwise be claimed by
> > > serdev.
> >
> > FWIW I've been using this patch for a while for realtek UART attached bluetooth:
> > https://github.com/jwrdegoede/linux-sunxi/commit/bc904e3703940600ca66c65fcdb0a8cb01dff55d
> > which is a gross hack.
> >
> > If we're going to do a whitelist for this, it better support some sort of
> > wildcards as there are a LOT of BCM2E?? devices which need to be on the
> > whitelist. I think a blacklist would actually be better though, this also
> > documents which devices are lacking a proper kernel (where applicable).
>
> Yeah, you guys know the ACPI space better than I do. I just fear that
> if we go with the blacklist approach, we'll be playing a whack-a-mole
> with this for a long time when people start upgrading there systems to
> 4.15 and discover that their serial ports are gone.
>
> Since this would qualify as a severe regression, me may need to consider
> adding a serdev whitelist for every ACPI id we add to a serdev driver
> instead.
OK, so let's have the ACPI discussion on linux-acpi pretty please.
Could this be resent with a CC to linux-acpi for some more complete context?
^ permalink raw reply
* Re: [RFC][PATCH 0/1] serdev: Support HS-UART serdev slaves over tty
From: Johan Hovold @ 2018-04-25 7:47 UTC (permalink / raw)
To: Hans de Goede
Cc: Johan Hovold, Shrirang Bagul, linux-serial, gregkh, Rob Herring,
Frédéric Danis, Rafael J. Wysocki, Lukas Wunner,
linux-kernel
In-Reply-To: <b4947b3c-15b3-15ea-e1b7-38169f11d0f9@redhat.com>
On Tue, Apr 24, 2018 at 11:16:38PM +0200, Hans de Goede wrote:
> Hi,
>
> On 24-04-18 19:18, Johan Hovold wrote:
> > [ Adding some more people on CC. ]
> >
> > On Tue, Apr 24, 2018 at 04:29:53PM +0800, Shrirang Bagul wrote:
> >> On systems using Intel Atom (Baytrail-I) SoC's, slave devices connected on
> >> HSUART1/2 ports are described by the ACPI BIOS as virtual hardware using
> >> HID's INT3511/INT3512 [1].
> >>
> >> As a consequence, HW manufacturers have complete freedom to install any
> >> devices on-board as long as they can be accessed over serial tty
> >> interface. Once such device is Dell Edge 3002 IoT Gateway which sports
> >> ZigBee & GPS devices on the HS-UART ports 1 & 2 respectively.
> >>
> >> In kernels before the introduction of 'Serial Device Bus (serdev)'
> >> subsystem, these devices were accessible using /dev/ttySx nodes. But,
> >> kernels since 4.15 can no longer do so.
> >>
> >> Post 4.15, with CONFIG_SERIAL_DEV_BUS=y, serdev port controller driver
> >> handles the enumeration for the slaves connected on these ports. Also,
> >> /dev/ttySx device nodes for these ports are no longer exposed to the
> >> userspace.
> >>
> >> This patch implements a new driver which binds to the ACPI serdev slaves
> >> enumerated by the serdev port controller and exposes /dev/ttyHSx device
> >> nodes which the userspace applications can use. Otherwise, upgrades to 4.15
> >> or higher kernels would certainly render these devices unusable.
> >>
> >> Considering serdev is new and evolving, this is one approach to solving
> >> the problem at hand. An obvious drawback is the change in the tty device
> >> node name from ttySx => ttyHSx, which means userspace applications have to
> >> be modified (I know that this is strongly discouraged). For the same
> >> reason, I am submitting these patches as RFC.
> >>
> >> If there are other/better ways of solving this or improving on the
> >> proposed solution, that will be most helpful.
> >
> > Yeah, I don't think this is the right solution to this problem. It seems
> > we need to blacklist (or maybe even use whitelists) ACPI-ids until there
> > are drivers for the slave devices that would otherwise be claimed by
> > serdev.
>
> FWIW I've been using this patch for a while for realtek UART attached bluetooth:
> https://github.com/jwrdegoede/linux-sunxi/commit/bc904e3703940600ca66c65fcdb0a8cb01dff55d
> which is a gross hack.
>
> If we're going to do a whitelist for this, it better support some sort of
> wildcards as there are a LOT of BCM2E?? devices which need to be on the
> whitelist. I think a blacklist would actually be better though, this also
> documents which devices are lacking a proper kernel (where applicable).
Yeah, you guys know the ACPI space better than I do. I just fear that
if we go with the blacklist approach, we'll be playing a whack-a-mole
with this for a long time when people start upgrading there systems to
4.15 and discover that their serial ports are gone.
Since this would qualify as a severe regression, me may need to consider
adding a serdev whitelist for every ACPI id we add to a serdev driver
instead.
Thanks,
Johan
^ permalink raw reply
* Re: [RFC][PATCH 0/1] serdev: Support HS-UART serdev slaves over tty
From: Hans de Goede @ 2018-04-24 21:16 UTC (permalink / raw)
To: Johan Hovold, Shrirang Bagul
Cc: linux-serial, gregkh, Rob Herring, Frédéric Danis,
Rafael J. Wysocki, Lukas Wunner, linux-kernel
In-Reply-To: <20180424171846.GD4615@localhost>
Hi,
On 24-04-18 19:18, Johan Hovold wrote:
> [ Adding some more people on CC. ]
>
> On Tue, Apr 24, 2018 at 04:29:53PM +0800, Shrirang Bagul wrote:
>> On systems using Intel Atom (Baytrail-I) SoC's, slave devices connected on
>> HSUART1/2 ports are described by the ACPI BIOS as virtual hardware using
>> HID's INT3511/INT3512 [1].
>>
>> As a consequence, HW manufacturers have complete freedom to install any
>> devices on-board as long as they can be accessed over serial tty
>> interface. Once such device is Dell Edge 3002 IoT Gateway which sports
>> ZigBee & GPS devices on the HS-UART ports 1 & 2 respectively.
>>
>> In kernels before the introduction of 'Serial Device Bus (serdev)'
>> subsystem, these devices were accessible using /dev/ttySx nodes. But,
>> kernels since 4.15 can no longer do so.
>>
>> Post 4.15, with CONFIG_SERIAL_DEV_BUS=y, serdev port controller driver
>> handles the enumeration for the slaves connected on these ports. Also,
>> /dev/ttySx device nodes for these ports are no longer exposed to the
>> userspace.
>>
>> This patch implements a new driver which binds to the ACPI serdev slaves
>> enumerated by the serdev port controller and exposes /dev/ttyHSx device
>> nodes which the userspace applications can use. Otherwise, upgrades to 4.15
>> or higher kernels would certainly render these devices unusable.
>>
>> Considering serdev is new and evolving, this is one approach to solving
>> the problem at hand. An obvious drawback is the change in the tty device
>> node name from ttySx => ttyHSx, which means userspace applications have to
>> be modified (I know that this is strongly discouraged). For the same
>> reason, I am submitting these patches as RFC.
>>
>> If there are other/better ways of solving this or improving on the
>> proposed solution, that will be most helpful.
>
> Yeah, I don't think this is the right solution to this problem. It seems
> we need to blacklist (or maybe even use whitelists) ACPI-ids until there
> are drivers for the slave devices that would otherwise be claimed by
> serdev.
FWIW I've been using this patch for a while for realtek UART attached bluetooth:
https://github.com/jwrdegoede/linux-sunxi/commit/bc904e3703940600ca66c65fcdb0a8cb01dff55d
which is a gross hack.
If we're going to do a whitelist for this, it better support some sort of
wildcards as there are a LOT of BCM2E?? devices which need to be on the
whitelist. I think a blacklist would actually be better though, this also
documents which devices are lacking a proper kernel (where applicable).
Regards,
Hans
>
>> This patch is based on:
>> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git v4.17-rc2
>>
>> [1] Enabling Multi-COM Port for Microsoft Windows OS 8.1 & 10 / IoT Core [Sec. 4.1]
>> (https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/enabling-multi-com-port-white-paper.pdf)
>
> Thanks,
> Johan
>
^ permalink raw reply
* Re: [RFC][PATCH 0/1] serdev: Support HS-UART serdev slaves over tty
From: Johan Hovold @ 2018-04-24 17:18 UTC (permalink / raw)
To: Shrirang Bagul
Cc: johan, linux-serial, gregkh, Rob Herring,
Frédéric Danis, Hans de Goede, Rafael J. Wysocki,
Lukas Wunner, linux-kernel
In-Reply-To: <20180424082954.11453-1-shrirang.bagul@canonical.com>
[ Adding some more people on CC. ]
On Tue, Apr 24, 2018 at 04:29:53PM +0800, Shrirang Bagul wrote:
> On systems using Intel Atom (Baytrail-I) SoC's, slave devices connected on
> HSUART1/2 ports are described by the ACPI BIOS as virtual hardware using
> HID's INT3511/INT3512 [1].
>
> As a consequence, HW manufacturers have complete freedom to install any
> devices on-board as long as they can be accessed over serial tty
> interface. Once such device is Dell Edge 3002 IoT Gateway which sports
> ZigBee & GPS devices on the HS-UART ports 1 & 2 respectively.
>
> In kernels before the introduction of 'Serial Device Bus (serdev)'
> subsystem, these devices were accessible using /dev/ttySx nodes. But,
> kernels since 4.15 can no longer do so.
>
> Post 4.15, with CONFIG_SERIAL_DEV_BUS=y, serdev port controller driver
> handles the enumeration for the slaves connected on these ports. Also,
> /dev/ttySx device nodes for these ports are no longer exposed to the
> userspace.
>
> This patch implements a new driver which binds to the ACPI serdev slaves
> enumerated by the serdev port controller and exposes /dev/ttyHSx device
> nodes which the userspace applications can use. Otherwise, upgrades to 4.15
> or higher kernels would certainly render these devices unusable.
>
> Considering serdev is new and evolving, this is one approach to solving
> the problem at hand. An obvious drawback is the change in the tty device
> node name from ttySx => ttyHSx, which means userspace applications have to
> be modified (I know that this is strongly discouraged). For the same
> reason, I am submitting these patches as RFC.
>
> If there are other/better ways of solving this or improving on the
> proposed solution, that will be most helpful.
Yeah, I don't think this is the right solution to this problem. It seems
we need to blacklist (or maybe even use whitelists) ACPI-ids until there
are drivers for the slave devices that would otherwise be claimed by
serdev.
> This patch is based on:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git v4.17-rc2
>
> [1] Enabling Multi-COM Port for Microsoft Windows OS 8.1 & 10 / IoT Core [Sec. 4.1]
> (https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/enabling-multi-com-port-white-paper.pdf)
Thanks,
Johan
^ permalink raw reply
* [PATCH v2] sc16is7xx: Check for an error when the clock is enabled.
From: Stefan Potyra @ 2018-04-24 16:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, ldv-project, sil2review
In-Reply-To: <20180423075519.GA13262@kroah.com>
When the clock is enabled, check if there is an error. Otherwise
clk_get_rate() can be called without enabled clock.
Found by Linux Driver Verification project (linuxtesting.org).
Fixes: 0814e8d5da2b ("sc16is7xx: enable the clock")
Signed-off-by: Stefan Potyra <Stefan.Potyra@elektrobit.com>
---
drivers/tty/serial/sc16is7xx.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 65792a3539d0..243c96025053 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1168,7 +1168,10 @@ static int sc16is7xx_probe(struct device *dev,
else
return PTR_ERR(s->clk);
} else {
- clk_prepare_enable(s->clk);
+ ret = clk_prepare_enable(s->clk);
+ if (ret)
+ return ret;
+
freq = clk_get_rate(s->clk);
}
--
2.17.0
^ permalink raw reply related
* Re: [PATCH] serial: 8250_early: Setup divider when uartclk is passed
From: Michal Simek @ 2018-04-24 12:27 UTC (permalink / raw)
To: Matt Redfearn, Michal Simek, linux-kernel, monstr
Cc: stable, Jiri Slaby, Douglas Anderson, linux-serial,
Greg Kroah-Hartman, Jeffy Chen
In-Reply-To: <f8c69fee-8889-f0aa-1235-b7a65068238b@mips.com>
Hi Matt,
On 23.4.2018 15:21, Matt Redfearn wrote:
> Hi Michal
>
> On 23/04/18 10:18, Michal Simek wrote:
>> device->baud is always non zero value because it is checked already in
>> early_serial8250_setup() before init_port is called.
>
> True, currently init_port is only called from the one location and so
> the test is a little redundant, though I don't see the harm in testing
> both inputs to the divisor calculation immediately before use such that
> any future call path avoids setting a bad divisor.
I will let others to decide.
>
>>
>> Fixes: 0ff3ab701963 ("serial: 8250_early: Only set divisor if valid
>> clk & baud")
>> Cc: stable <stable@vger.kernel.org>
>
> Even if the test is dropped going forward, I wouldn't consider it's
> presence a "bug" such that a fix needs to be backported.
Not a problem with not adding this to stable.
Thanks,
Michal
^ permalink raw reply
* Re: [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
From: Peter Maydell @ 2018-04-23 13:49 UTC (permalink / raw)
To: Dave Martin
Cc: Andrew Jones, Ciro Santilli, Linus Walleij, Russell King, Wei Xu,
linux-serial, arm-mail-list
In-Reply-To: <1524490888-849-1-git-send-email-Dave.Martin@arm.com>
On 23 April 2018 at 14:41, Dave Martin <Dave.Martin@arm.com> wrote:
> This is an update to a previous RFC v2 [1], to fix a problem observed by
> the qemu community that causes serial input to hang when booting a
> simulated system with data already queued in the UART FIFO [2].
>
> After discussion, I decided that the approach in [1] was over-
> engineered: it tries to preserve a guarantee that people shouldn't be
> relying on anyway, namely that data sent to the UART prior to kernel
> boot will be received by the kernel; or more generally that data
> received by the UART while the pl011 driver is not opened will be
> received (either intact or at all) by the driver.
>
> If anyone can please test the following and let me know the results,
> that would be much appreciated!
>
> a) Check that you can still reproduce the bug on mainline without this
> patch.
>
> b) Check whether this patch fixes the problem.
Thanks. I'm cc'ing Ciro and Drew, who are the two people I
recall reporting this issue to me.
Link to the patch for their benefit:
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html
-- PMM
^ permalink raw reply
* [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
From: Dave Martin @ 2018-04-23 13:41 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Peter Maydell, Linus Walleij, Wei Xu, Russell King, linux-serial
In-Reply-To: <1524490888-849-1-git-send-email-Dave.Martin@arm.com>
Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts")
clears the RX and receive timeout interrupts on pl011 startup, to
avoid a screaming-interrupt scenario that can occur when the
firmware or bootloader leaves these interrupts asserted.
This has been noted as an issue when running Linux on qemu [1].
Unfortunately, the above fix seems to lead to potential
misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously
on driver startup, if the RX FIFO is also already full to the
trigger level.
Clearing the RX FIFO interrupt does not change the FIFO fill level.
In this scenario, because the interrupt is now clear and because
the FIFO is already full to the trigger level, no new assertion of
the RX FIFO interrupt can occur unless the FIFO is drained back
below the trigger level. This never occurs because the pl011
driver is waiting for an RX FIFO interrupt to tell it that there is
something to read, and does not read the FIFO at all until that
interrupt occurs.
Thus, simply clearing "spurious" interrupts on startup may be
misguided, since there is no way to be sure that the interrupts are
truly spurious, and things can go wrong if they are not.
This patch instead clears the interrupt condition by draining the
RX FIFO during UART startup, after clearing any potentially
spurious interrupt. This should ensure that an interrupt will
definitely be asserted if the RX FIFO subsequently becomes
sufficiently full.
This patch does not attempt to address the case where the RX FIFO
fills faster than it can be drained: that is a pathological
condition that is beyond the scope of the driver to work around.
Users cannot expect this to work unless they enable hardware flow
control.
[1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
before enabled the interruption
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
Reported-by: Wei Xu <xuwei5@hisilicon.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
Changes since RFC v2:
* Drain the RX FIFO explicitly on startup.
This reduces the complexity of the changes, but should still
fix the underlying problem.
Characters present in the RX FIFO before the kernel boots will
now be lost. However it was always poor practice to rely on
such characters being processed by the kernel, since UART
initialisation and RX FIFO overruns can always lead to data
loss or corruption in any case.
---
drivers/tty/serial/amba-pl011.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 4b40a5b..73e6f44 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1676,6 +1676,15 @@ static int pl011_hwinit(struct uart_port *port)
uap, REG_ICR);
/*
+ * RXIS is asserted only when the RX FIFO transitions from below
+ * to above the trigger threshold. If the RX FIFO is already
+ * full to the threshold this can't happen and RXIS will now be
+ * stuck off. Drain the FIFO explicitly to fix this:
+ */
+ while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
+ pl011_read(uap, REG_DR);
+
+ /*
* Save interrupts enable mask, and enable RX interrupts in case if
* the interrupt is used for NMI entry.
*/
--
2.1.4
^ permalink raw reply related
* [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
From: Dave Martin @ 2018-04-23 13:41 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Peter Maydell, Linus Walleij, Wei Xu, Russell King, linux-serial
This is an update to a previous RFC v2 [1], to fix a problem observed by
the qemu community that causes serial input to hang when booting a
simulated system with data already queued in the UART FIFO [2].
After discussion, I decided that the approach in [1] was over-
engineered: it tries to preserve a guarantee that people shouldn't be
relying on anyway, namely that data sent to the UART prior to kernel
boot will be received by the kernel; or more generally that data
received by the UART while the pl011 driver is not opened will be
received (either intact or at all) by the driver.
If anyone can please test the following and let me know the results,
that would be much appreciated!
a) Check that you can still reproduce the bug on mainline without this
patch.
b) Check whether this patch fixes the problem.
Cheers
---Dave
[1] [RFC PATCH v2] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/556897.html
[2] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
before enabled the interruption
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
Dave Martin (1):
tty: pl011: Avoid spuriously stuck-off interrupts
drivers/tty/serial/amba-pl011.c | 9 +++++++++
1 file changed, 9 insertions(+)
--
2.1.4
^ permalink raw reply
* Re: [RFC. PATCH] earlycon: Remove hardcoded port->uartclk initialization in of_setup_earlycon
From: Michal Simek @ 2018-04-23 13:22 UTC (permalink / raw)
To: Matt Redfearn, Michal Simek, linux-kernel, monstr, devicetree,
Rob Herring
Cc: Greg Kroah-Hartman, linux-serial, Jiri Slaby
In-Reply-To: <6626f66e-e1a6-9e54-0011-95ab4949bd3b@mips.com>
Hi,
On 23.4.2018 15:16, Matt Redfearn wrote:
>
>
> On 23/04/18 10:27, Michal Simek wrote:
>> There is no reason to initialize uartclk to BASE_BAUD * 16 for DT based
>> systems.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> It looks like from history that portclk = BASE_BAUD * 16 was setup to
>> get on calculation on x86 (divisor = 1) but it shouldn't be needed on
>> DT based
>> system. That's why I think that there is no DT based system which really
>> requires this line.
>
> Hi Michal,
>
> This is fine for the MIPS generic platform (tested on Boston board)
> which was broken by some earlycon changes a few versions ago. Also
> tested on a MIPS pistachio board. As long as the bootloader has
> configured the uart divisor, earlycon should work as long as my patch
> "serial: 8250_early: Only set divisor if valid clk & baud" is applied to
> avoid a bad divisor getting calculated.
I have sent one patch to fix your patch because you don't need to check
device->baud twice.
[PATCH] serial: 8250_early: Setup divider when uartclk is passed
> Tested-by: Matt Redfearn <matt.redfearn@mips.com>
Thanks,
Michal
^ permalink raw reply
* Re: [PATCH] serial: 8250_early: Setup divider when uartclk is passed
From: Matt Redfearn @ 2018-04-23 13:21 UTC (permalink / raw)
To: Michal Simek, linux-kernel, monstr
Cc: stable, Jiri Slaby, Douglas Anderson, linux-serial,
Greg Kroah-Hartman, Jeffy Chen
In-Reply-To: <93a7871c8654a6273b1ab35a8071e9f830e2a0c8.1524475087.git.michal.simek@xilinx.com>
Hi Michal
On 23/04/18 10:18, Michal Simek wrote:
> device->baud is always non zero value because it is checked already in
> early_serial8250_setup() before init_port is called.
True, currently init_port is only called from the one location and so
the test is a little redundant, though I don't see the harm in testing
both inputs to the divisor calculation immediately before use such that
any future call path avoids setting a bad divisor.
>
> Fixes: 0ff3ab701963 ("serial: 8250_early: Only set divisor if valid clk & baud")
> Cc: stable <stable@vger.kernel.org>
Even if the test is dropped going forward, I wouldn't consider it's
presence a "bug" such that a fix needs to be backported.
Thanks,
Matt
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> drivers/tty/serial/8250/8250_early.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
> index ae6a256524d8..5cd8c36c8fcc 100644
> --- a/drivers/tty/serial/8250/8250_early.c
> +++ b/drivers/tty/serial/8250/8250_early.c
> @@ -122,7 +122,7 @@ static void __init init_port(struct earlycon_device *device)
> serial8250_early_out(port, UART_FCR, 0); /* no fifo */
> serial8250_early_out(port, UART_MCR, 0x3); /* DTR + RTS */
>
> - if (port->uartclk && device->baud) {
> + if (port->uartclk) {
> divisor = DIV_ROUND_CLOSEST(port->uartclk, 16 * device->baud);
> c = serial8250_early_in(port, UART_LCR);
> serial8250_early_out(port, UART_LCR, c | UART_LCR_DLAB);
>
^ permalink raw reply
* Re: [RFC. PATCH] earlycon: Remove hardcoded port->uartclk initialization in of_setup_earlycon
From: Matt Redfearn @ 2018-04-23 13:16 UTC (permalink / raw)
To: Michal Simek, linux-kernel, monstr, devicetree, Rob Herring
Cc: Greg Kroah-Hartman, linux-serial, Jiri Slaby
In-Reply-To: <b05e4d329c556357bf7393075c6c1602509eb9c2.1524475668.git.michal.simek@xilinx.com>
On 23/04/18 10:27, Michal Simek wrote:
> There is no reason to initialize uartclk to BASE_BAUD * 16 for DT based
> systems.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> It looks like from history that portclk = BASE_BAUD * 16 was setup to
> get on calculation on x86 (divisor = 1) but it shouldn't be needed on DT based
> system. That's why I think that there is no DT based system which really
> requires this line.
Hi Michal,
This is fine for the MIPS generic platform (tested on Boston board)
which was broken by some earlycon changes a few versions ago. Also
tested on a MIPS pistachio board. As long as the bootloader has
configured the uart divisor, earlycon should work as long as my patch
"serial: 8250_early: Only set divisor if valid clk & baud" is applied to
avoid a bad divisor getting calculated.
Tested-by: Matt Redfearn <matt.redfearn@mips.com>
Thanks,
Matt
> ---
> drivers/tty/serial/earlycon.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index a24278380fec..c12b1edcdd8e 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -244,7 +244,6 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
> return -ENXIO;
> }
> port->mapbase = addr;
> - port->uartclk = BASE_BAUD * 16;
>
> val = of_get_flat_dt_prop(node, "reg-offset", NULL);
> if (val)
>
^ permalink raw reply
* [PATCH 4/4] serial: uartps: Remove static port array
From: Michal Simek @ 2018-04-23 9:51 UTC (permalink / raw)
To: linux-kernel, monstr, Alan Cox, geert+renesas
Cc: Jiri Slaby, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
In-Reply-To: <cover.1524477059.git.michal.simek@xilinx.com>
Allocate uart port structure dynamically.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/serial/xilinx_uartps.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index f7d527a1c532..0fc9de20843c 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1097,8 +1097,6 @@ static const struct uart_ops cdns_uart_ops = {
#endif
};
-static struct uart_port cdns_uart_port[CDNS_UART_NR_PORTS];
-
#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
/**
* cdns_uart_console_wait_tx - Wait for the TX to be full
@@ -1438,6 +1436,9 @@ static int cdns_uart_probe(struct platform_device *pdev)
GFP_KERNEL);
if (!cdns_uart_data)
return -ENOMEM;
+ port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return -ENOMEM;
match = of_match_node(cdns_uart_of_match, pdev->dev.of_node);
if (match && match->data) {
@@ -1503,16 +1504,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
if (id < 0)
id = 0;
- /* Try the given port id if failed use default method */
- if (id < CDNS_UART_NR_PORTS && cdns_uart_port[id].mapbase != 0) {
- /* Find the next unused port */
- for (id = 0; id < CDNS_UART_NR_PORTS; id++)
- if (cdns_uart_port[id].mapbase == 0)
- break;
- }
-
- port = &cdns_uart_port[id];
- if (!port || id >= CDNS_UART_NR_PORTS) {
+ if (id >= CDNS_UART_NR_PORTS) {
dev_err(&pdev->dev, "Cannot get uart_port structure\n");
rc = -ENODEV;
goto err_out_notif_unreg;
--
2.17.0
^ permalink raw reply related
* [PATCH 3/4] serial: uartps: Move cnds_uart_get_port to probe
From: Michal Simek @ 2018-04-23 9:51 UTC (permalink / raw)
To: linux-kernel, monstr, Alan Cox, geert+renesas
Cc: Jiri Slaby, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
In-Reply-To: <cover.1524477059.git.michal.simek@xilinx.com>
c&p this function to probe as preparation for removing
cdns_uart_port[] static array.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/serial/xilinx_uartps.c | 61 ++++++++++--------------------
1 file changed, 21 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 789423d081ed..f7d527a1c532 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1099,43 +1099,6 @@ static const struct uart_ops cdns_uart_ops = {
static struct uart_port cdns_uart_port[CDNS_UART_NR_PORTS];
-/**
- * cdns_uart_get_port - Configure the port from platform device resource info
- * @id: Port id
- *
- * Return: a pointer to a uart_port or NULL for failure
- */
-static struct uart_port *cdns_uart_get_port(int id)
-{
- struct uart_port *port;
-
- /* Try the given port id if failed use default method */
- if (id < CDNS_UART_NR_PORTS && cdns_uart_port[id].mapbase != 0) {
- /* Find the next unused port */
- for (id = 0; id < CDNS_UART_NR_PORTS; id++)
- if (cdns_uart_port[id].mapbase == 0)
- break;
- }
-
- if (id >= CDNS_UART_NR_PORTS)
- return NULL;
-
- port = &cdns_uart_port[id];
-
- /* At this point, we've got an empty uart_port struct, initialize it */
- spin_lock_init(&port->lock);
- port->membase = NULL;
- port->irq = 0;
- port->type = PORT_UNKNOWN;
- port->iotype = UPIO_MEM32;
- port->flags = UPF_BOOT_AUTOCONF;
- port->ops = &cdns_uart_ops;
- port->fifosize = CDNS_UART_FIFO_SIZE;
- port->line = id;
- port->dev = NULL;
- return port;
-}
-
#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
/**
* cdns_uart_console_wait_tx - Wait for the TX to be full
@@ -1540,15 +1503,33 @@ static int cdns_uart_probe(struct platform_device *pdev)
if (id < 0)
id = 0;
- /* Initialize the port structure */
- port = cdns_uart_get_port(id);
+ /* Try the given port id if failed use default method */
+ if (id < CDNS_UART_NR_PORTS && cdns_uart_port[id].mapbase != 0) {
+ /* Find the next unused port */
+ for (id = 0; id < CDNS_UART_NR_PORTS; id++)
+ if (cdns_uart_port[id].mapbase == 0)
+ break;
+ }
- if (!port) {
+ port = &cdns_uart_port[id];
+ if (!port || id >= CDNS_UART_NR_PORTS) {
dev_err(&pdev->dev, "Cannot get uart_port structure\n");
rc = -ENODEV;
goto err_out_notif_unreg;
}
+ /* At this point, we've got an empty uart_port struct, initialize it */
+ spin_lock_init(&port->lock);
+ port->membase = NULL;
+ port->irq = 0;
+ port->type = PORT_UNKNOWN;
+ port->iotype = UPIO_MEM32;
+ port->flags = UPF_BOOT_AUTOCONF;
+ port->ops = &cdns_uart_ops;
+ port->fifosize = CDNS_UART_FIFO_SIZE;
+ port->line = id;
+ port->dev = NULL;
+
/*
* Register the port.
* This function also registers this device with the tty layer
--
2.17.0
^ permalink raw reply related
* [PATCH 2/4] serial: uartps: Use dynamic array for console port
From: Michal Simek @ 2018-04-23 9:51 UTC (permalink / raw)
To: linux-kernel, monstr, Alan Cox, geert+renesas
Cc: Jiri Slaby, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
In-Reply-To: <cover.1524477059.git.michal.simek@xilinx.com>
Driver console functions are using pointer to static array with fixed
size. There can be only one serial console at the time which is found
by register_console(). register_console() is filling cons->index to
port->line value.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/serial/xilinx_uartps.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 932210331484..789423d081ed 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1206,6 +1206,10 @@ OF_EARLYCON_DECLARE(cdns, "cdns,uart-r1p8", cdns_early_console_setup);
OF_EARLYCON_DECLARE(cdns, "cdns,uart-r1p12", cdns_early_console_setup);
OF_EARLYCON_DECLARE(cdns, "xlnx,zynqmp-uart", cdns_early_console_setup);
+
+/* Static pointer to console port */
+static struct uart_port *console_port;
+
/**
* cdns_uart_console_write - perform write operation
* @co: Console handle
@@ -1215,7 +1219,7 @@ OF_EARLYCON_DECLARE(cdns, "xlnx,zynqmp-uart", cdns_early_console_setup);
static void cdns_uart_console_write(struct console *co, const char *s,
unsigned int count)
{
- struct uart_port *port = &cdns_uart_port[co->index];
+ struct uart_port *port = console_port;
unsigned long flags;
unsigned int imr, ctrl;
int locked = 1;
@@ -1261,15 +1265,13 @@ static void cdns_uart_console_write(struct console *co, const char *s,
*/
static int __init cdns_uart_console_setup(struct console *co, char *options)
{
- struct uart_port *port = &cdns_uart_port[co->index];
+ struct uart_port *port = console_port;
+
int baud = 9600;
int bits = 8;
int parity = 'n';
int flow = 'n';
- if (co->index < 0 || co->index >= CDNS_UART_NR_PORTS)
- return -EINVAL;
-
if (!port->membase) {
pr_debug("console on " CDNS_UART_TTY_NAME "%i not present\n",
co->index);
@@ -1565,6 +1567,17 @@ static int cdns_uart_probe(struct platform_device *pdev)
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
+#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
+ /*
+ * If console hasn't been found yet try to assign this port
+ * because it is required to be assigned for console setup function.
+ * If register_console() don't assign value, then console_port pointer
+ * is cleanup.
+ */
+ if (cdns_uart_uart_driver.cons->index == -1)
+ console_port = port;
+#endif
+
rc = uart_add_one_port(&cdns_uart_uart_driver, port);
if (rc) {
dev_err(&pdev->dev,
@@ -1572,6 +1585,12 @@ static int cdns_uart_probe(struct platform_device *pdev)
goto err_out_pm_disable;
}
+#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
+ /* This is not port which is used for console that's why clean it up */
+ if (cdns_uart_uart_driver.cons->index == -1)
+ console_port = NULL;
+#endif
+
return 0;
err_out_pm_disable:
--
2.17.0
^ permalink raw reply related
* [PATCH 1/4] serial: uartps: Remove console_initcall from the driver
From: Michal Simek @ 2018-04-23 9:51 UTC (permalink / raw)
To: linux-kernel, monstr, Alan Cox, geert+renesas
Cc: Jiri Slaby, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
In-Reply-To: <cover.1524477059.git.michal.simek@xilinx.com>
register_console() is called from
uart_add_one_port()->uart_configure_port()
that's why register_console() is called twice.
This patch remove console_initcall to call register_console() only from
one location.
Also based on my tests cdns_uart_console_setup() is not called
from the first register_console() call.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Alan Cox <alan@linux.intel.com>
---
I am not quite sure about this because console_initcall is called
early but I can see any difference in usage.
pl011 is not calling this but others are doing it.
---
drivers/tty/serial/xilinx_uartps.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index bd72dd843338..932210331484 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1293,20 +1293,6 @@ static struct console cdns_uart_console = {
.index = -1, /* Specified on the cmdline (e.g. console=ttyPS ) */
.data = &cdns_uart_uart_driver,
};
-
-/**
- * cdns_uart_console_init - Initialization call
- *
- * Return: 0 on success, negative errno otherwise
- */
-static int __init cdns_uart_console_init(void)
-{
- register_console(&cdns_uart_console);
- return 0;
-}
-
-console_initcall(cdns_uart_console_init);
-
#endif /* CONFIG_SERIAL_XILINX_PS_UART_CONSOLE */
static struct uart_driver cdns_uart_uart_driver = {
--
2.17.0
^ permalink raw reply related
* [PATCH 0/4] serial: uartps: Dynamic allocation
From: Michal Simek @ 2018-04-23 9:50 UTC (permalink / raw)
To: linux-kernel, monstr, Alan Cox, geert+renesas
Cc: Jiri Slaby, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
Hi,
this is the initial version before next step which is move
uart_register_driver to probe function.
I was able to get rid of static array with uart_port structures.
It was wired with console which is also fixed.
And the next step is the most complicated one handle .nr in uart_driver
structure in more generic way.
I am sending these patches as v1. RFC was sent in past
(https://www.spinics.net/lists/linux-serial/msg27106.html)
and I promised to wait till Geert's
"serial: xuartps: Fix out-of-bounds access through DT
alias" (sha1: e7d75e18d0fc3f7193b65282b651f980c778d935) is applied.
Thanks,
Michal
Michal Simek (4):
serial: uartps: Remove console_initcall from the driver
serial: uartps: Use dynamic array for console port
serial: uartps: Move cnds_uart_get_port to probe
serial: uartps: Remove static port array
drivers/tty/serial/xilinx_uartps.c | 102 +++++++++++------------------
1 file changed, 40 insertions(+), 62 deletions(-)
--
2.17.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox