* [PATCH 7/9] dt-bindings: serial: add compatible for stm32h7
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: Bich HEMON
In-Reply-To: <1499958494-19354-1-git-send-email-bich.hemon@st.com>
From: Fabrice Gasnier <fabrice.gasnier@st.com>
Introduce new compatibles for "st,stm32h7-usart" and "st,stm32h7-uart".
This new compatible allow to use optional wake-up interrupt.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Signed-off-by: Bich Hemon <bich.hemon@st.com>
---
.../devicetree/bindings/serial/st,stm32-usart.txt | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
index 85ec5f2..3657f9f 100644
--- a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
+++ b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
@@ -1,12 +1,19 @@
* STMicroelectronics STM32 USART
Required properties:
-- compatible: Can be either "st,stm32-usart", "st,stm32-uart",
-"st,stm32f7-usart" or "st,stm32f7-uart" depending on whether
-the device supports synchronous mode and is compatible with
-stm32(f4) or stm32f7.
+- compatible: can be either:
+ - "st,stm32-usart",
+ - "st,stm32-uart",
+ - "st,stm32f7-usart",
+ - "st,stm32f7-uart",
+ - "st,stm32h7-usart"
+ - "st,stm32h7-uart".
+ depending on whether the device supports synchronous mode
+ and is compatible with stm32(f4), stm32f7 or stm32h7.
- reg: The address and length of the peripheral registers space
-- interrupts: The interrupt line of the USART instance
+- interrupts:
+ - The interrupt line for the USART instance,
+ - An optional wake-up interrupt.
- clocks: The input clock of the USART instance
Optional properties:
--
1.9.1
^ permalink raw reply related
* [PATCH 6/9] serial: stm32: fix error handling in probe
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: Bich HEMON
In-Reply-To: <1499958494-19354-1-git-send-email-bich.hemon@st.com>
From: Fabrice Gasnier <fabrice.gasnier@st.com>
Disable clock properly in case of error.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Signed-off-by: Bich Hemon <bich.hemon@st.com>
---
drivers/tty/serial/stm32-usart.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 9158d31..413ff49 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -678,8 +678,10 @@ static int stm32_init_port(struct stm32_port *stm32port,
return ret;
stm32port->port.uartclk = clk_get_rate(stm32port->clk);
- if (!stm32port->port.uartclk)
+ if (!stm32port->port.uartclk) {
+ clk_disable_unprepare(stm32port->clk);
ret = -EINVAL;
+ }
return ret;
}
@@ -865,7 +867,7 @@ static int stm32_serial_probe(struct platform_device *pdev)
ret = uart_add_one_port(&stm32_usart_driver, &stm32port->port);
if (ret)
- return ret;
+ goto err_uninit;
ret = stm32_of_dma_rx_probe(stm32port, pdev);
if (ret)
@@ -878,6 +880,11 @@ static int stm32_serial_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, &stm32port->port);
return 0;
+
+err_uninit:
+ clk_disable_unprepare(stm32port->clk);
+
+ return ret;
}
static int stm32_serial_remove(struct platform_device *pdev)
--
1.9.1
^ permalink raw reply related
* [PATCH 5/9] serial: stm32: add RTS support
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: Bich HEMON
In-Reply-To: <1499958494-19354-1-git-send-email-bich.hemon@st.com>
Implement support of RTS in USART control register
Signed-off-by: Bich Hemon <bich.hemon@st.com>
---
drivers/tty/serial/stm32-usart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index ca61bfe..9158d31 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -518,7 +518,7 @@ static void stm32_set_termios(struct uart_port *port, struct ktermios *termios,
port->status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS);
if (cflag & CRTSCTS) {
port->status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
- cr3 |= USART_CR3_CTSE;
+ cr3 |= USART_CR3_CTSE | USART_CR3_RTSE;
}
usartdiv = DIV_ROUND_CLOSEST(port->uartclk, baud);
--
1.9.1
^ permalink raw reply related
* [PATCH 4/9] serial: stm32: fix pio transmit timeout
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: Bich HEMON
In-Reply-To: <1499958494-19354-1-git-send-email-bich.hemon@st.com>
From: Gerald Baeza <gerald.baeza@st.com>
100µs was too short for low speed transmission
(9600bps)
Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
Signed-off-by: Bich Hemon <bich.hemon@st.com>
---
drivers/tty/serial/stm32-usart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index a12d79c..ca61bfe 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -202,7 +202,7 @@ static void stm32_transmit_chars_pio(struct uart_port *port)
ret = readl_relaxed_poll_timeout_atomic(port->membase + ofs->isr,
isr,
(isr & USART_SR_TXE),
- 10, 100);
+ 10, 100000);
if (ret)
dev_err(port->dev, "tx empty not set\n");
--
1.9.1
^ permalink raw reply related
* [PATCH 3/9] serial: stm32: Increase maximum number of ports
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Bich HEMON
In-Reply-To: <1499958494-19354-1-git-send-email-bich.hemon-qxv4g6HH51o@public.gmane.org>
From: Gerald Baeza <gerald.baeza-qxv4g6HH51o@public.gmane.org>
Increase max number of ports for stm32h7
which supports up to 8 uart and usart instances.
Signed-off-by: Gerald Baeza <gerald.baeza-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Bich Hemon <bich.hemon-qxv4g6HH51o@public.gmane.org>
---
drivers/tty/serial/stm32-usart.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
index 65b4ffa..6092789 100644
--- a/drivers/tty/serial/stm32-usart.h
+++ b/drivers/tty/serial/stm32-usart.h
@@ -206,7 +206,7 @@ struct stm32_usart_info stm32f7_info = {
#define USART_ICR_CMCF BIT(17) /* F7 */
#define STM32_SERIAL_NAME "ttyS"
-#define STM32_MAX_PORTS 6
+#define STM32_MAX_PORTS 8
#define RX_BUF_L 200 /* dma rx buffer length */
#define RX_BUF_P RX_BUF_L /* dma rx buffer period */
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 2/9] serial: stm32: fix multi-ports management
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: Bich HEMON
In-Reply-To: <1499958494-19354-1-git-send-email-bich.hemon@st.com>
From: Gerald Baeza <gerald.baeza@st.com>
Correct management of multi-ports. Each port has
its own last residue value and its own alias.
Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
Signed-off-by: Bich Hemon <bich.hemon@st.com>
---
drivers/tty/serial/stm32-usart.c | 12 +++++++-----
drivers/tty/serial/stm32-usart.h | 1 +
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index ebc49e4..a12d79c 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -111,14 +111,13 @@ static void stm32_receive_chars(struct uart_port *port, bool threaded)
unsigned long c;
u32 sr;
char flag;
- static int last_res = RX_BUF_L;
if (port->irq_wake)
pm_wakeup_event(tport->tty->dev, 0);
- while (stm32_pending_rx(port, &sr, &last_res, threaded)) {
+ while (stm32_pending_rx(port, &sr, &stm32_port->last_res, threaded)) {
sr |= USART_SR_DUMMY_RX;
- c = stm32_get_char(port, &sr, &last_res);
+ c = stm32_get_char(port, &sr, &stm32_port->last_res);
flag = TTY_NORMAL;
port->icount.rx++;
@@ -694,8 +693,10 @@ static struct stm32_port *stm32_of_get_stm32_port(struct platform_device *pdev)
return NULL;
id = of_alias_get_id(np, "serial");
- if (id < 0)
- id = 0;
+ if (id < 0) {
+ dev_err(&pdev->dev, "failed to get alias id, errno %d\n", id);
+ return NULL;
+ }
if (WARN_ON(id >= STM32_MAX_PORTS))
return NULL;
@@ -703,6 +704,7 @@ static struct stm32_port *stm32_of_get_stm32_port(struct platform_device *pdev)
stm32_ports[id].hw_flow_control = of_property_read_bool(np,
"st,hw-flow-ctrl");
stm32_ports[id].port.line = id;
+ stm32_ports[id].last_res = RX_BUF_L;
return &stm32_ports[id];
}
diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
index 0532b2f..65b4ffa 100644
--- a/drivers/tty/serial/stm32-usart.h
+++ b/drivers/tty/serial/stm32-usart.h
@@ -222,6 +222,7 @@ struct stm32_port {
struct dma_chan *tx_ch; /* dma tx channel */
dma_addr_t tx_dma_buf; /* dma tx buffer bus address */
unsigned char *tx_buf; /* dma tx buffer cpu address */
+ int last_res;
bool tx_dma_busy; /* dma tx busy */
bool hw_flow_control;
};
--
1.9.1
^ permalink raw reply related
* [PATCH 1/9] serial: stm32: fix copyright
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: Bich HEMON
In-Reply-To: <1499958494-19354-1-git-send-email-bich.hemon@st.com>
Fix missing copyright for STMicroelectronics
Signed-off-by: Bich Hemon <bich.hemon@st.com>
---
drivers/tty/serial/stm32-usart.c | 1 +
drivers/tty/serial/stm32-usart.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 0338562..ebc49e4 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -1,5 +1,6 @@
/*
* Copyright (C) Maxime Coquelin 2015
+ * Copyright (C) STMicroelectronics SA 2017
* Authors: Maxime Coquelin <mcoquelin.stm32@gmail.com>
* Gerald Baeza <gerald.baeza@st.com>
* License terms: GNU General Public License (GPL), version 2
diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
index cd97ceb..0532b2f 100644
--- a/drivers/tty/serial/stm32-usart.h
+++ b/drivers/tty/serial/stm32-usart.h
@@ -1,5 +1,6 @@
/*
* Copyright (C) Maxime Coquelin 2015
+ * Copyright (C) STMicroelectronics SA 2017
* Authors: Maxime Coquelin <mcoquelin.stm32@gmail.com>
* Gerald Baeza <gerald_baeza@yahoo.fr>
* License terms: GNU General Public License (GPL), version 2
--
1.9.1
^ permalink raw reply related
* [PATCH 0/9] Add support for STM32H7 serial
From: Bich HEMON @ 2017-07-13 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: Bich HEMON
This patchset updates existing stm32 usart driver:
- Misc fixes: copyright, mutliport management and timeout issue
- Add support for STM32H7
- Add support for wake-up and fifo management (for STM32H7)
Bich Hemon (2):
serial: stm32: fix copyright
serial: stm32: add RTS support
Fabrice Gasnier (3):
serial: stm32: fix error handling in probe
dt-bindings: serial: add compatible for stm32h7
serial: stm32: add wakeup mechanism
Gerald Baeza (4):
serial: stm32: fix multi-ports management
serial: stm32: Increase maximum number of ports
serial: stm32: fix pio transmit timeout
serial: stm32: add fifo support
.../devicetree/bindings/serial/st,stm32-usart.txt | 17 ++-
drivers/tty/serial/stm32-usart.c | 123 +++++++++++++++++++--
drivers/tty/serial/stm32-usart.h | 37 ++++++-
3 files changed, 162 insertions(+), 15 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH] clk: gemini: Fix reset regression
From: Stephen Boyd @ 2017-07-12 23:08 UTC (permalink / raw)
To: Philipp Zabel
Cc: Linus Walleij, Michael Turquette, linux-clk, Janos Laube,
Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Joel Stanley, Greg Kroah-Hartman, linux-serial
In-Reply-To: <1499874682.6374.57.camel@pengutronix.de>
On 07/12, Philipp Zabel wrote:
> Hi Linus,
>
> On Tue, 2017-07-11 at 14:26 +0200, Linus Walleij wrote:
> > commit e2860e1f62f2 ("serial: 8250_of: Add reset support")
> > introduced reset support for the 8250_of driver.
> >
> > However it unconditionally uses the assert/deassert pair to
> > deassert reset on the device at probe and assert it at
> > remove. This does not work with systems that have a
> > self-deasserting reset controller, such as Gemini, that
> > recently added a reset controller.
> >
> > As a result, the console will not probe on the Gemini with
> > this message:
> >
> > Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
> > of_serial: probe of 42000000.serial failed with error -524
> >
> > This (-ENOTSUPP) is the error code returned by the
> > deassert() operation on self-deasserting reset controllers.
> >
> > To work around this, implement dummy .assert() and
> > .deassert() operations in the Gemini combined clock and
> > reset controller. This fixes the issue on this system.
> >
> > Cc: Joel Stanley <joel@jms.id.au>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: linux-serial@vger.kernel.org
> > Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > This is the solution suggested by Philipp, I think.
>
> It is what I suggested, yes, but now that I see it before me, I don't
> think this is the proper solution either. Reason below:
>
> > ---
> > drivers/clk/clk-gemini.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/clk/clk-gemini.c b/drivers/clk/clk-gemini.c
> > index c391a49aaaff..b4cf2f699a21 100644
> > --- a/drivers/clk/clk-gemini.c
> > +++ b/drivers/clk/clk-gemini.c
> > @@ -237,6 +237,18 @@ static int gemini_reset(struct reset_controller_dev *rcdev,
> > BIT(GEMINI_RESET_CPU1) | BIT(id));
> > }
> >
> > +static int gemini_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + return 0;
>
> This is valid behaviour for shared reset controls, as sharing users
> don't mind whether the reset line is actually asserted after this call,
> they just allow it.
>
> For an exclusive reset control this should return an error though, as
> the caller would expect the reset line to be asserted after this call.
> Unfortunately the core does not provide information whether the reset
> control is shared or exclusive to the reset drivers, and it could be
> argued that the drivers shouldn't have to care. I suppose I'll have to
> handle this in the core, after all. What do you think of the attached
> patch?
>
> Otherwise, as a regression fix, I think this would be ok. There isn't
> going to be any driver on the Gemini platform that requests an exclusive
> reset control and then calls reset_control_assert, expecting the reset
> line to stay asserted.
>
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
>
I'll queue this up for clk-fixes once -rc1 is out.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH] clk: gemini: Fix reset regression
From: Philipp Zabel @ 2017-07-12 15:51 UTC (permalink / raw)
To: Linus Walleij
Cc: Michael Turquette, Stephen Boyd, linux-clk, Janos Laube,
Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Joel Stanley, Greg Kroah-Hartman, linux-serial
In-Reply-To: <20170711122601.17745-1-linus.walleij@linaro.org>
Hi Linus,
On Tue, 2017-07-11 at 14:26 +0200, Linus Walleij wrote:
> commit e2860e1f62f2 ("serial: 8250_of: Add reset support")
> introduced reset support for the 8250_of driver.
>
> However it unconditionally uses the assert/deassert pair to
> deassert reset on the device at probe and assert it at
> remove. This does not work with systems that have a
> self-deasserting reset controller, such as Gemini, that
> recently added a reset controller.
>
> As a result, the console will not probe on the Gemini with
> this message:
>
> Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
> of_serial: probe of 42000000.serial failed with error -524
>
> This (-ENOTSUPP) is the error code returned by the
> deassert() operation on self-deasserting reset controllers.
>
> To work around this, implement dummy .assert() and
> .deassert() operations in the Gemini combined clock and
> reset controller. This fixes the issue on this system.
>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-serial@vger.kernel.org
> Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This is the solution suggested by Philipp, I think.
It is what I suggested, yes, but now that I see it before me, I don't
think this is the proper solution either. Reason below:
> ---
> drivers/clk/clk-gemini.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/clk/clk-gemini.c b/drivers/clk/clk-gemini.c
> index c391a49aaaff..b4cf2f699a21 100644
> --- a/drivers/clk/clk-gemini.c
> +++ b/drivers/clk/clk-gemini.c
> @@ -237,6 +237,18 @@ static int gemini_reset(struct reset_controller_dev *rcdev,
> BIT(GEMINI_RESET_CPU1) | BIT(id));
> }
>
> +static int gemini_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + return 0;
This is valid behaviour for shared reset controls, as sharing users
don't mind whether the reset line is actually asserted after this call,
they just allow it.
For an exclusive reset control this should return an error though, as
the caller would expect the reset line to be asserted after this call.
Unfortunately the core does not provide information whether the reset
control is shared or exclusive to the reset drivers, and it could be
argued that the drivers shouldn't have to care. I suppose I'll have to
handle this in the core, after all. What do you think of the attached
patch?
Otherwise, as a regression fix, I think this would be ok. There isn't
going to be any driver on the Gemini platform that requests an exclusive
reset control and then calls reset_control_assert, expecting the reset
line to stay asserted.
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
> +}
> +
> +static int gemini_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + return 0;
This is valid behaviour for a self-deasserting controller with the reset
lines initially deasserted for both shared and exclusive reset controls:
after this call the reset line is guaranteed to be deasserted.
regards
Philipp
----------8<----------
>From fab3a9a697e9797ba1c24874d7c43c09dd812e77 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Wed, 12 Jul 2017 17:29:28 +0200
Subject: [PATCH] reset: make (de)assert report succeess for self-deasserting
reset drivers
By now there are drivers using shared reset controls and (de)assert
calls on platforms with self-deasserting reset lines and thus reset
drivers that do not implement .assert() and .deassert().
As long as the initial state of the reset line is deasserted, there
is no reason for a reset_control_assert call to return an error for
shared reset controls, or for a reset_control_deassert call to return
an error for either shared or exclusive reset controls: after a call
to reset_control_deassert the reset line is guaranteed to be deasserted,
and after a call to reset_control_assert it is valid for the reset
line to stay deasserted for shared reset controls.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/reset/core.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index cd739d2fa1603..6c182c173f9b4 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -201,9 +201,6 @@ int reset_control_assert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
- if (!rstc->rcdev->ops->assert)
- return -ENOTSUPP;
-
if (rstc->shared) {
if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
return -EINVAL;
@@ -213,6 +210,21 @@ int reset_control_assert(struct reset_control *rstc)
if (atomic_dec_return(&rstc->deassert_count) != 0)
return 0;
+
+ /*
+ * Shared reset controls allow the reset line to be in any state
+ * after this call, so doing nothing is a valid option.
+ */
+ if (!rstc->rcdev->ops->assert)
+ return 0;
+ } else {
+ /*
+ * If the reset controller does not implement .assert(), there
+ * is no way to guarantee that the reset line is asserted after
+ * this call.
+ */
+ if (!rstc->rcdev->ops->assert)
+ return -ENOTSUPP;
}
return rstc->rcdev->ops->assert(rstc->rcdev, rstc->id);
@@ -239,9 +251,6 @@ int reset_control_deassert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
- if (!rstc->rcdev->ops->deassert)
- return -ENOTSUPP;
-
if (rstc->shared) {
if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
return -EINVAL;
@@ -250,6 +259,16 @@ int reset_control_deassert(struct reset_control *rstc)
return 0;
}
+ /*
+ * If the reset controller does not implement .deassert(), we assume
+ * that it handles self-deasserting reset lines via .reset(). In that
+ * case, the reset lines are deasserted by default. If that is not the
+ * case, the reset controller driver should implement .deassert() and
+ * return -ENOTSUPP.
+ */
+ if (!rstc->rcdev->ops->deassert)
+ return 0;
+
return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
}
EXPORT_SYMBOL_GPL(reset_control_deassert);
--
2.11.0
---------->8----------
^ permalink raw reply related
* Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
From: Petr Mladek @ 2017-07-12 11:11 UTC (permalink / raw)
To: Matt Redfearn
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Greg Kroah-Hartman,
Jiri Slaby, linux-serial, Steven Rostedt, linux-kernel
In-Reply-To: <86ff1193-8485-4b14-492f-627720c1d868@imgtec.com>
On Tue 2017-07-11 15:41:50, Matt Redfearn wrote:
> On 11/07/17 13:43, Petr Mladek wrote:
> >IMHO, the reasonable solution is to move early console code and data
> >out of the init sections. We should do this for the early consoles
> >where the corresponding real console is registered using a deferred
> >probe. Others should be already replaced by the real console when
> >printk_late_init() is called. At least this is how I understand it.
>
> This seems like the most reasonable way forward to me as well,
> though sadly will lead to some post-init kernel bloat.
>
> I still think, however, that this patch is a reasonable change to
> make.
The thing is that this patch "silently" makes the keep_bootcon
option almost unusable.
The remaining use in register_console() does not make much sense.
It keeps the boot console after the preferred real one was registered.
> My other patch, placing serial earlycon in the init section is then
> the wrong way round and we need to remove __init from e.g. early8250
> and so on.
Yup.
> >Matt, is there any chance that you look into this possibility?
>
> Sure, I can look at fixing up console drivers which we use on the
> MIPS architecture.
I have discussed this with my colleagues. It seems that implementation
of keep_bootcon option is pretty broken. It allows to debug broken
system using unreliable consoles (already freed). Therefore enabling
the option adds more problems on its own.
It would be nice to fix it. But it looks rather complicated. There
are several possibilities that are more or less hacky. It is hard
to guess the best solution if we do not know who is using it, how
it helps or sucks for them.
Before we spend a lot of time on this. Let me first try to send an RFC
to remove the feature and put more people into CC. It will either
be accepted or we will get arguments that would justify the needed
changes to fix it.
Best Regards,
Petr
PS: If you already stared migrating some consoles from the init
section, I would be interested into feedback about what problems
you did meet so far.
^ permalink raw reply
* Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
From: Matt Redfearn @ 2017-07-11 14:41 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Greg Kroah-Hartman,
Jiri Slaby, linux-serial, Steven Rostedt, linux-kernel
In-Reply-To: <20170711124308.GA3393@pathway.suse.cz>
Hi Petr,
On 11/07/17 13:43, Petr Mladek wrote:
> Hi all,
>
> let's first make sure that we understand the code the same way.
>
> On Fri 2017-07-07 08:58:01, Matt Redfearn wrote:
>> On 07/07/17 05:45, Sergey Senozhatsky wrote:
>>> On (07/06/17 11:38), Matt Redfearn wrote:
>>>> Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in
>>>> printk_late_init() if keep_bootcon") added a check on keep_bootcon to
>>>> ensure that boot consoles were kept around until the real console is
>>>> registered.
>>>>
>>>> This can lead to problems if the boot console data and code are in the
>>>> init section, since it can be freed before the boot console is
>>>> deregistered.
> Yes.
>
>>>> This was fixed by commit 81cc26f2bd11 ("printk: only
>>>> unregister boot consoles when necessary").
> This does not make sense to me in this context. This commit has an
> effect only when keep_bootcon is false. While the commit 4c30c6f566c0
> ("kernel/printk: do not turn off boot console in printk_late_init()
> if keep_bootcon") _causes problems only_ when keep_bootcon is true.
>
> What I want to say is that the two commits have effect when
> keep_bootcon has different value. Therefore they could not fix
> each other.
Yeah, what I meant was the situation of the boot console being freed
while still active (if keep_bootcon is false) was fixed by 81cc26f2bd11.
After 4c30c6f566c0, if keep_bootcon is true, then you run in to the
issue I had. The boot console is still active. It's code & data are
freed and poisoned with the SLAB poison. Some of that section is then
executed when the boot console is accessed. By happy coincidence(!), on
MIPS, the poison value 0xCCCCCCCC happens to be a valid opcode with no
side effect (it's a memory prefetch), so the CPU merrily runs off into
the weeds executing Mb's worth of them before finally executing
something that causes an exception of some kind. You get a kernel panic
with a cause and address that make no sense and have to backtrack trying
to figure out at what point the actual error occurred, and get nicely
sidetracked form the issue you were actually trying to debug that
necessitated keep_bootcon ;-).
>
>>>> The keep_bootcon flag prevents the unregistration of a boot console,
>>>> even if it's data and code reside in the init section and are about to
>>>> be freed. This can lead to crashes and weird panics when the bootconsole
>>>> is accessed after free, especially if page poisoning is in use and the
>>>> code / data have been overwritten with a poison value.
>>>> To prevent this, always free the boot console if it is within the init
>>>> section.
>>> if someone asked to `keep_bootcon' but we actually don't keep it, then
>>> what's the purpose of the flag and
>>> pr_info("debug: skip boot console de-registration.\n")?
> Exactly. The important information is in the commit 7bf693951a8e5f7e
> ("console: allow to retain boot console via boot option keep_bootcon"):
>
> On some architectures, the boot process involves de-registering the boot
> console (early boot), initialize drivers and then re-register the console.
>
> This mechanism introduces a window in which no printk can happen on the
> console and messages are buffered and then printed once the new console is
> available.
>
> If a kernel crashes during this window, all it's left on the boot console
> is "console [foo] enabled, boot console disabled" making debug of the crash
> rather 'interesting'.
>
>
>>> keeping `early_printk' sometimes can be helpful and people even want to
>>> use `early_printk' as a panic() console fallback (because of those nasty
>>> deadlocks that spoil printk->call_console_drivers()).
>>>
>> Sure, but as a user, how are you supposed to know that?
> Good point! I wonder if the authors of the keep_bootcon option
> actually knew about it. I do not see this risk mentioned anywhere
> and the early consoles might work long enough by chance.
>
> One problem is that real consoles might be registered much later
> when it is done using an async probe calls. It might open a big window
> when there is no visible output and debugging is "impossible".
>
> I am not comfortable with removing the only way to debug some type
> of bugs. But the current state is broken as well.
>
> IMHO, the reasonable solution is to move early console code and data
> out of the init sections. We should do this for the early consoles
> where the corresponding real console is registered using a deferred
> probe. Others should be already replaced by the real console when
> printk_late_init() is called. At least this is how I understand it.
This seems like the most reasonable way forward to me as well, though
sadly will lead to some post-init kernel bloat.
I still think, however, that this patch is a reasonable change to make.
Though perhaps some warning (if keep_bootcon is active?) would be
appropriate so that a user knows that the boot console is being
unregistered (because it may no longer be functional after being freed)
and therefore silence can be expected - and we know that a driver needs
fixing. We can then proceed as you suggest and migrate away from placing
early console drivers which will not already have an active real console
in the init section.
My other patch, placing serial earlycon in the init section is then the
wrong way round and we need to remove __init from e.g. early8250 and so on.
>
> Matt, is there any chance that you look into this possibility?
Sure, I can look at fixing up console drivers which we use on the MIPS
architecture.
Thanks,
Matt
>
> Best Regards,
> Petr
^ permalink raw reply
* Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
From: Petr Mladek @ 2017-07-11 12:43 UTC (permalink / raw)
To: Matt Redfearn
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Greg Kroah-Hartman,
Jiri Slaby, linux-serial, Steven Rostedt, linux-kernel
In-Reply-To: <0212d098-e307-2ea1-cb41-1e81d545da56@imgtec.com>
Hi all,
let's first make sure that we understand the code the same way.
On Fri 2017-07-07 08:58:01, Matt Redfearn wrote:
> On 07/07/17 05:45, Sergey Senozhatsky wrote:
> >On (07/06/17 11:38), Matt Redfearn wrote:
> >>Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in
> >>printk_late_init() if keep_bootcon") added a check on keep_bootcon to
> >>ensure that boot consoles were kept around until the real console is
> >>registered.
> >>
> >>This can lead to problems if the boot console data and code are in the
> >>init section, since it can be freed before the boot console is
> >>deregistered.
Yes.
> >>This was fixed by commit 81cc26f2bd11 ("printk: only
> >>unregister boot consoles when necessary").
This does not make sense to me in this context. This commit has an
effect only when keep_bootcon is false. While the commit 4c30c6f566c0
("kernel/printk: do not turn off boot console in printk_late_init()
if keep_bootcon") _causes problems only_ when keep_bootcon is true.
What I want to say is that the two commits have effect when
keep_bootcon has different value. Therefore they could not fix
each other.
> >>The keep_bootcon flag prevents the unregistration of a boot console,
> >>even if it's data and code reside in the init section and are about to
> >>be freed. This can lead to crashes and weird panics when the bootconsole
> >>is accessed after free, especially if page poisoning is in use and the
> >>code / data have been overwritten with a poison value.
> >>To prevent this, always free the boot console if it is within the init
> >>section.
> >if someone asked to `keep_bootcon' but we actually don't keep it, then
> >what's the purpose of the flag and
> > pr_info("debug: skip boot console de-registration.\n")?
Exactly. The important information is in the commit 7bf693951a8e5f7e
("console: allow to retain boot console via boot option keep_bootcon"):
On some architectures, the boot process involves de-registering the boot
console (early boot), initialize drivers and then re-register the console.
This mechanism introduces a window in which no printk can happen on the
console and messages are buffered and then printed once the new console is
available.
If a kernel crashes during this window, all it's left on the boot console
is "console [foo] enabled, boot console disabled" making debug of the crash
rather 'interesting'.
> >keeping `early_printk' sometimes can be helpful and people even want to
> >use `early_printk' as a panic() console fallback (because of those nasty
> >deadlocks that spoil printk->call_console_drivers()).
> >
>
> Sure, but as a user, how are you supposed to know that?
Good point! I wonder if the authors of the keep_bootcon option
actually knew about it. I do not see this risk mentioned anywhere
and the early consoles might work long enough by chance.
One problem is that real consoles might be registered much later
when it is done using an async probe calls. It might open a big window
when there is no visible output and debugging is "impossible".
I am not comfortable with removing the only way to debug some type
of bugs. But the current state is broken as well.
IMHO, the reasonable solution is to move early console code and data
out of the init sections. We should do this for the early consoles
where the corresponding real console is registered using a deferred
probe. Others should be already replaced by the real console when
printk_late_init() is called. At least this is how I understand it.
Matt, is there any chance that you look into this possibility?
Best Regards,
Petr
^ permalink raw reply
* [PATCH] clk: gemini: Fix reset regression
From: Linus Walleij @ 2017-07-11 12:26 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Philipp Zabel, linux-clk
Cc: Janos Laube, Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Linus Walleij, Joel Stanley, Greg Kroah-Hartman,
linux-serial
commit e2860e1f62f2 ("serial: 8250_of: Add reset support")
introduced reset support for the 8250_of driver.
However it unconditionally uses the assert/deassert pair to
deassert reset on the device at probe and assert it at
remove. This does not work with systems that have a
self-deasserting reset controller, such as Gemini, that
recently added a reset controller.
As a result, the console will not probe on the Gemini with
this message:
Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
of_serial: probe of 42000000.serial failed with error -524
This (-ENOTSUPP) is the error code returned by the
deassert() operation on self-deasserting reset controllers.
To work around this, implement dummy .assert() and
.deassert() operations in the Gemini combined clock and
reset controller. This fixes the issue on this system.
Cc: Joel Stanley <joel@jms.id.au>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-serial@vger.kernel.org
Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
This is the solution suggested by Philipp, I think.
---
drivers/clk/clk-gemini.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/clk/clk-gemini.c b/drivers/clk/clk-gemini.c
index c391a49aaaff..b4cf2f699a21 100644
--- a/drivers/clk/clk-gemini.c
+++ b/drivers/clk/clk-gemini.c
@@ -237,6 +237,18 @@ static int gemini_reset(struct reset_controller_dev *rcdev,
BIT(GEMINI_RESET_CPU1) | BIT(id));
}
+static int gemini_reset_assert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ return 0;
+}
+
+static int gemini_reset_deassert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ return 0;
+}
+
static int gemini_reset_status(struct reset_controller_dev *rcdev,
unsigned long id)
{
@@ -253,6 +265,8 @@ static int gemini_reset_status(struct reset_controller_dev *rcdev,
static const struct reset_control_ops gemini_reset_ops = {
.reset = gemini_reset,
+ .assert = gemini_reset_assert,
+ .deassert = gemini_reset_deassert,
.status = gemini_reset_status,
};
--
2.9.4
^ permalink raw reply related
* Re: [PATCH] serial: 8250_of: Fix regression in reset support
From: Philipp Zabel @ 2017-07-11 8:29 UTC (permalink / raw)
To: Linus Walleij
Cc: Greg Kroah-Hartman, linux-serial, linux-kernel, Joel Stanley
In-Reply-To: <20170710152451.5773-1-linus.walleij@linaro.org>
Hi Linus,
On Mon, 2017-07-10 at 17:24 +0200, Linus Walleij wrote:
> commit e2860e1f62f2 ("serial: 8250_of: Add reset support")
> introduced reset support for the 8250_of driver.
>
> However it unconditionally uses the assert/deassert pair to
> deassert reset on the device at probe and assert it at
> remove. This does not work with systems that have a
> self-deasserting reset controller, such as Gemini, that
> recently added a reset controller.
Self-deasserting resets are fundamentally incompatible with
assert/deassert in the current shared reset model (as there is no
framework for deferring or or vetoing reset assertions while one of the
devices sharing the reset line could be negatively affected).
The way it is implemented right now, after reset() is triggered once,
assert/deassert will return -EINVAL for the lifetime of the reset
control instance. Also, reset() returns -EINVAL between another users'
deassert() and assert() calls.
> As a result, the console will not probe on the Gemini with
> this message:
>
> Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
> of_serial: probe of 42000000.serial failed with error -524
>
> This (-ENOTSUPP) is the error code returned by the
> deassert() operation on self-deasserting reset controllers.
One easy workaround would be to implement deassert() to be a no-op (or
to divert the call to reset()) in your reset controller driver. A more
intrusive variant would be to do the same in the core, as you suggest
below.
> Add some code and comments that will:
>
> - In probe() avoid to bail out if -ENOTSUPP is returned
> from the reset_deassert() call.
I think it would be better not to put this burden on the drivers.
> - If reset_assert() bails out with -ENOTSUPP on remove(),
> try the self-deasserting method as a fallback.
I don't understand the purpose of this, see below.
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Philipp: please comment on this or ACK if it is the right
> approach. It sort of sets a precedent for handling
> different reset controllers from the consumer side.
>
> Another possibility is to modify the reset core such
> that .deassert() bails out silently if the controller only
> has .reset(), and .assert() just calls .reset() if the
> controller does not have .assert().
>
> Actually I think the latter is more intuitive but it is
> also more intrusive.
I have been thinking about this. Due to the diversity of reset
controller implementations, I'm hesitant to do this in the core.
For starters, why would reset() be called during assert() and not during
deassert()? Can we be sure that this is the right thing for all reset
controllers?
The deassert() call guarantees that the reset line is deasserted after
the call (and as long as deassert_count > 0). What if somebody ends up
building a self-deasserting reset controller that has the reset line
initially asserted?
Also it is implied that after the call to deassert() the device is in a
working state, obviously. This might only be the case after the first
reset() for some devices.
> ---
> drivers/tty/serial/8250/8250_of.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> index 0cf95fddccfc..927ee8561c8d 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -138,7 +138,12 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
> if (IS_ERR(info->rst))
> goto out;
> ret = reset_control_deassert(info->rst);
> - if (ret)
> + /*
> + * If the deassert operation is not supported, this could be because
> + * the reset controller is self-deasserting and onlt supports the
> + * .reset() operation, so this is not a probe error.
> + */
> + if (ret && (ret != -ENOTSUPP))
I don't like to have to do this in the drivers.
The driver just requests the reset line to be deasserted after the call,
if there is one. If the reset controller driver can guarantee that,
deassert() should not return an error.
> goto out;
>
> port->type = type;
> @@ -235,10 +240,14 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
> static int of_platform_serial_remove(struct platform_device *ofdev)
> {
> struct of_serial_info *info = platform_get_drvdata(ofdev);
> + int ret;
>
> serial8250_unregister_port(info->line);
>
> - reset_control_assert(info->rst);
> + ret = reset_control_assert(info->rst);
> + /* If the assert operation is not supported, use pure reset() */
> + if (ret == -ENOTSUPP)
> + reset_control_reset(info->rst);
> if (info->clk)
> clk_disable_unprepare(info->clk);
> kfree(info);
What is the purpose of triggering the assert during remove?
I would expect the self-deasserting reset to be triggered during probe,
before the device is used, if it is necessary at all.
regards
Philipp
^ permalink raw reply
* Re: [PATCH] serial: 8250_of: Fix regression in reset support
From: Andy Shevchenko @ 2017-07-10 17:09 UTC (permalink / raw)
To: Linus Walleij
Cc: Philipp Zabel, Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, Joel Stanley
In-Reply-To: <20170710152451.5773-1-linus.walleij@linaro.org>
On Mon, Jul 10, 2017 at 6:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> commit e2860e1f62f2 ("serial: 8250_of: Add reset support")
> introduced reset support for the 8250_of driver.
>
> However it unconditionally uses the assert/deassert pair to
> deassert reset on the device at probe and assert it at
> remove. This does not work with systems that have a
> self-deasserting reset controller, such as Gemini, that
> recently added a reset controller.
>
> As a result, the console will not probe on the Gemini with
> this message:
>
> Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
> of_serial: probe of 42000000.serial failed with error -524
>
> This (-ENOTSUPP) is the error code returned by the
> deassert() operation on self-deasserting reset controllers.
>
> Add some code and comments that will:
>
> - In probe() avoid to bail out if -ENOTSUPP is returned
> from the reset_deassert() call.
>
> - If reset_assert() bails out with -ENOTSUPP on remove(),
> try the self-deasserting method as a fallback.
>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Philipp: please comment on this or ACK if it is the right
> approach. It sort of sets a precedent for handling
> different reset controllers from the consumer side.
>
> Another possibility is to modify the reset core such
> that .deassert() bails out silently if the controller only
> has .reset(), and .assert() just calls .reset() if the
> controller does not have .assert().
>
> Actually I think the latter is more intuitive but it is
> also more intrusive.
> ---
> drivers/tty/serial/8250/8250_of.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> index 0cf95fddccfc..927ee8561c8d 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -138,7 +138,12 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
> if (IS_ERR(info->rst))
> goto out;
> ret = reset_control_deassert(info->rst);
> - if (ret)
> + /*
> + * If the deassert operation is not supported, this could be because
> + * the reset controller is self-deasserting and onlt supports the
only
> + * .reset() operation, so this is not a probe error.
> + */
> + if (ret && (ret != -ENOTSUPP))
> goto out;
>
> port->type = type;
> @@ -235,10 +240,14 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
> static int of_platform_serial_remove(struct platform_device *ofdev)
> {
> struct of_serial_info *info = platform_get_drvdata(ofdev);
> + int ret;
>
> serial8250_unregister_port(info->line);
>
> - reset_control_assert(info->rst);
> + ret = reset_control_assert(info->rst);
> + /* If the assert operation is not supported, use pure reset() */
> + if (ret == -ENOTSUPP)
> + reset_control_reset(info->rst);
> if (info->clk)
> clk_disable_unprepare(info->clk);
> kfree(info);
> --
> 2.9.4
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] serial: 8250_of: Fix regression in reset support
From: Linus Walleij @ 2017-07-10 15:24 UTC (permalink / raw)
To: Philipp Zabel, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Linus Walleij, Joel Stanley
commit e2860e1f62f2 ("serial: 8250_of: Add reset support")
introduced reset support for the 8250_of driver.
However it unconditionally uses the assert/deassert pair to
deassert reset on the device at probe and assert it at
remove. This does not work with systems that have a
self-deasserting reset controller, such as Gemini, that
recently added a reset controller.
As a result, the console will not probe on the Gemini with
this message:
Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
of_serial: probe of 42000000.serial failed with error -524
This (-ENOTSUPP) is the error code returned by the
deassert() operation on self-deasserting reset controllers.
Add some code and comments that will:
- In probe() avoid to bail out if -ENOTSUPP is returned
from the reset_deassert() call.
- If reset_assert() bails out with -ENOTSUPP on remove(),
try the self-deasserting method as a fallback.
Cc: Joel Stanley <joel@jms.id.au>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Philipp: please comment on this or ACK if it is the right
approach. It sort of sets a precedent for handling
different reset controllers from the consumer side.
Another possibility is to modify the reset core such
that .deassert() bails out silently if the controller only
has .reset(), and .assert() just calls .reset() if the
controller does not have .assert().
Actually I think the latter is more intuitive but it is
also more intrusive.
---
drivers/tty/serial/8250/8250_of.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 0cf95fddccfc..927ee8561c8d 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -138,7 +138,12 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
if (IS_ERR(info->rst))
goto out;
ret = reset_control_deassert(info->rst);
- if (ret)
+ /*
+ * If the deassert operation is not supported, this could be because
+ * the reset controller is self-deasserting and onlt supports the
+ * .reset() operation, so this is not a probe error.
+ */
+ if (ret && (ret != -ENOTSUPP))
goto out;
port->type = type;
@@ -235,10 +240,14 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
static int of_platform_serial_remove(struct platform_device *ofdev)
{
struct of_serial_info *info = platform_get_drvdata(ofdev);
+ int ret;
serial8250_unregister_port(info->line);
- reset_control_assert(info->rst);
+ ret = reset_control_assert(info->rst);
+ /* If the assert operation is not supported, use pure reset() */
+ if (ret == -ENOTSUPP)
+ reset_control_reset(info->rst);
if (info->clk)
clk_disable_unprepare(info->clk);
kfree(info);
--
2.9.4
^ permalink raw reply related
* [PATCH v5] tty: serial: owl: Implement console driver
From: Andreas Färber @ 2017-07-09 20:29 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial
Cc: linux-arm-kernel, Thomas Liau, 张东风,
刘炜, 张天益, 梅利,
96boards, support, support, lee, Alan Cox, Andy Shevchenko,
linux-kernel, Andreas Färber, Jiri Slaby
Implement serial console driver to complement earlycon.
Based on LeMaker linux-actions tree.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
v4 -> v5:
* Don't clear HUPCL, IGNBRK or BRKINT (Alan)
* Don't set CLOCAL (Alan)
* Added tty_termios_encode_baud_rate() call (Alan)
* Extended get_mctrl with RTS/CTS
* Adopted GENMASK() macro (Andy)
* Dropped cast from void * (Andy)
* Adopted devm_clk_get() (Andy)
* Checked for uartclk == 0 (Andy)
* Changed port ID from 117 to unused 36 (Andy)
* Adopted spin_lock_irqsave() in owl_uart_irq() (Andy)
* Adopted platform_get_irq() (Andy)
* Implemented TIOCM_LOOP in set_mctrl
* Switched platform driver to s500/s900 compatibles from owl
* Set TX fifosize based on compatible (16 -> 32 for S900)
v3 -> v4: Unchanged
v2 -> v3:
* Adopted BIT() macro
* Implemented baudrate setting
v2: new
drivers/tty/serial/Kconfig | 4 +-
drivers/tty/serial/owl-uart.c | 635 ++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/serial_core.h | 1 +
3 files changed, 636 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 1f096e2bb398..b788fee54249 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1689,7 +1689,7 @@ config SERIAL_MVEBU_CONSOLE
Otherwise, say 'N'.
config SERIAL_OWL
- bool "Actions Semi Owl serial port support"
+ tristate "Actions Semi Owl serial port support"
depends on ARCH_ACTIONS || COMPILE_TEST
select SERIAL_CORE
help
@@ -1705,7 +1705,7 @@ config SERIAL_OWL_CONSOLE
default y
help
Say 'Y' here if you wish to use Actions Semiconductor S500/S900 UART
- as the system console. Only earlycon is implemented currently.
+ as the system console.
endmenu
diff --git a/drivers/tty/serial/owl-uart.c b/drivers/tty/serial/owl-uart.c
index 1b8008797a1b..683b05478ade 100644
--- a/drivers/tty/serial/owl-uart.c
+++ b/drivers/tty/serial/owl-uart.c
@@ -20,6 +20,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include <linux/clk.h>
#include <linux/console.h>
#include <linux/delay.h>
#include <linux/io.h>
@@ -28,22 +29,66 @@
#include <linux/platform_device.h>
#include <linux/serial.h>
#include <linux/serial_core.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+#define OWL_UART_PORT_NUM 7
+#define OWL_UART_DEV_NAME "ttyOWL"
#define OWL_UART_CTL 0x000
+#define OWL_UART_RXDAT 0x004
#define OWL_UART_TXDAT 0x008
#define OWL_UART_STAT 0x00c
+#define OWL_UART_CTL_DWLS_MASK GENMASK(1, 0)
+#define OWL_UART_CTL_DWLS_5BITS (0x0 << 0)
+#define OWL_UART_CTL_DWLS_6BITS (0x1 << 0)
+#define OWL_UART_CTL_DWLS_7BITS (0x2 << 0)
+#define OWL_UART_CTL_DWLS_8BITS (0x3 << 0)
+#define OWL_UART_CTL_STPS_2BITS BIT(2)
+#define OWL_UART_CTL_PRS_MASK GENMASK(6, 4)
+#define OWL_UART_CTL_PRS_NONE (0x0 << 4)
+#define OWL_UART_CTL_PRS_ODD (0x4 << 4)
+#define OWL_UART_CTL_PRS_MARK (0x5 << 4)
+#define OWL_UART_CTL_PRS_EVEN (0x6 << 4)
+#define OWL_UART_CTL_PRS_SPACE (0x7 << 4)
+#define OWL_UART_CTL_AFE BIT(12)
#define OWL_UART_CTL_TRFS_TX BIT(14)
#define OWL_UART_CTL_EN BIT(15)
+#define OWL_UART_CTL_RXDE BIT(16)
+#define OWL_UART_CTL_TXDE BIT(17)
#define OWL_UART_CTL_RXIE BIT(18)
#define OWL_UART_CTL_TXIE BIT(19)
+#define OWL_UART_CTL_LBEN BIT(20)
#define OWL_UART_STAT_RIP BIT(0)
#define OWL_UART_STAT_TIP BIT(1)
+#define OWL_UART_STAT_RXER BIT(2)
+#define OWL_UART_STAT_TFER BIT(3)
+#define OWL_UART_STAT_RXST BIT(4)
+#define OWL_UART_STAT_RFEM BIT(5)
#define OWL_UART_STAT_TFFU BIT(6)
-#define OWL_UART_STAT_TRFL_MASK (0x1f << 11)
+#define OWL_UART_STAT_CTSS BIT(7)
+#define OWL_UART_STAT_RTSS BIT(8)
+#define OWL_UART_STAT_TFES BIT(10)
+#define OWL_UART_STAT_TRFL_MASK GENMASK(16, 11)
#define OWL_UART_STAT_UTBB BIT(17)
+static struct uart_driver owl_uart_driver;
+
+struct owl_uart_info {
+ unsigned int tx_fifosize;
+};
+
+struct owl_uart_port {
+ struct uart_port port;
+ struct clk *clk;
+};
+
+#define to_owl_uart_port(prt) container_of(prt, struct owl_uart_port, prt)
+
+static struct owl_uart_port *owl_uart_ports[OWL_UART_PORT_NUM];
+
static inline void owl_uart_write(struct uart_port *port, u32 val, unsigned int off)
{
writel(val, port->membase + off);
@@ -54,6 +99,397 @@ static inline u32 owl_uart_read(struct uart_port *port, unsigned int off)
return readl(port->membase + off);
}
+static void owl_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+ u32 ctl;
+
+ ctl = owl_uart_read(port, OWL_UART_CTL);
+
+ if (mctrl & TIOCM_LOOP)
+ ctl |= OWL_UART_CTL_LBEN;
+ else
+ ctl &= ~OWL_UART_CTL_LBEN;
+
+ owl_uart_write(port, ctl, OWL_UART_CTL);
+}
+
+static unsigned int owl_uart_get_mctrl(struct uart_port *port)
+{
+ unsigned int mctrl = TIOCM_CAR | TIOCM_DSR;
+ u32 stat, ctl;
+
+ ctl = owl_uart_read(port, OWL_UART_CTL);
+ stat = owl_uart_read(port, OWL_UART_STAT);
+ if (stat & OWL_UART_STAT_RTSS)
+ mctrl |= TIOCM_RTS;
+ if ((stat & OWL_UART_STAT_CTSS) || !(ctl & OWL_UART_CTL_AFE))
+ mctrl |= TIOCM_CTS;
+ return mctrl;
+}
+
+static unsigned int owl_uart_tx_empty(struct uart_port *port)
+{
+ unsigned long flags;
+ u32 val;
+ unsigned int ret;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ val = owl_uart_read(port, OWL_UART_STAT);
+ ret = (val & OWL_UART_STAT_TFES) ? TIOCSER_TEMT : 0;
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ return ret;
+}
+
+static void owl_uart_stop_rx(struct uart_port *port)
+{
+ u32 val;
+
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val &= ~(OWL_UART_CTL_RXIE | OWL_UART_CTL_RXDE);
+ owl_uart_write(port, val, OWL_UART_CTL);
+
+ val = owl_uart_read(port, OWL_UART_STAT);
+ val |= OWL_UART_STAT_RIP;
+ owl_uart_write(port, val, OWL_UART_STAT);
+}
+
+static void owl_uart_stop_tx(struct uart_port *port)
+{
+ u32 val;
+
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val &= ~(OWL_UART_CTL_TXIE | OWL_UART_CTL_TXDE);
+ owl_uart_write(port, val, OWL_UART_CTL);
+
+ val = owl_uart_read(port, OWL_UART_STAT);
+ val |= OWL_UART_STAT_TIP;
+ owl_uart_write(port, val, OWL_UART_STAT);
+}
+
+static void owl_uart_start_tx(struct uart_port *port)
+{
+ u32 val;
+
+ if (uart_tx_stopped(port)) {
+ owl_uart_stop_tx(port);
+ return;
+ }
+
+ val = owl_uart_read(port, OWL_UART_STAT);
+ val |= OWL_UART_STAT_TIP;
+ owl_uart_write(port, val, OWL_UART_STAT);
+
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val |= OWL_UART_CTL_TXIE;
+ owl_uart_write(port, val, OWL_UART_CTL);
+}
+
+static void owl_uart_send_chars(struct uart_port *port)
+{
+ struct circ_buf *xmit = &port->state->xmit;
+ unsigned int ch;
+
+ if (uart_tx_stopped(port))
+ return;
+
+ if (port->x_char) {
+ while (!(owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU))
+ cpu_relax();
+ owl_uart_write(port, port->x_char, OWL_UART_TXDAT);
+ port->icount.tx++;
+ port->x_char = 0;
+ }
+
+ while (!(owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU)) {
+ if (uart_circ_empty(xmit))
+ break;
+
+ ch = xmit->buf[xmit->tail];
+ owl_uart_write(port, ch, OWL_UART_TXDAT);
+ xmit->tail = (xmit->tail + 1) & (SERIAL_XMIT_SIZE - 1);
+ port->icount.tx++;
+ }
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+
+ if (uart_circ_empty(xmit))
+ owl_uart_stop_tx(port);
+}
+
+static void owl_uart_receive_chars(struct uart_port *port)
+{
+ u32 stat, val;
+
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val &= ~OWL_UART_CTL_TRFS_TX;
+ owl_uart_write(port, val, OWL_UART_CTL);
+
+ stat = owl_uart_read(port, OWL_UART_STAT);
+ while (!(stat & OWL_UART_STAT_RFEM)) {
+ char flag = TTY_NORMAL;
+
+ if (stat & OWL_UART_STAT_RXER)
+ port->icount.overrun++;
+
+ if (stat & OWL_UART_STAT_RXST) {
+ /* We are not able to distinguish the error type. */
+ port->icount.brk++;
+ port->icount.frame++;
+
+ stat &= port->read_status_mask;
+ if (stat & OWL_UART_STAT_RXST)
+ flag = TTY_PARITY;
+ } else
+ port->icount.rx++;
+
+ val = owl_uart_read(port, OWL_UART_RXDAT);
+ val &= 0xff;
+
+ if ((stat & port->ignore_status_mask) == 0)
+ tty_insert_flip_char(&port->state->port, val, flag);
+
+ stat = owl_uart_read(port, OWL_UART_STAT);
+ }
+
+ spin_unlock(&port->lock);
+ tty_flip_buffer_push(&port->state->port);
+ spin_lock(&port->lock);
+}
+
+static irqreturn_t owl_uart_irq(int irq, void *dev_id)
+{
+ struct uart_port *port = dev_id;
+ unsigned long flags;
+ u32 stat;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ stat = owl_uart_read(port, OWL_UART_STAT);
+
+ if (stat & OWL_UART_STAT_RIP)
+ owl_uart_receive_chars(port);
+
+ if (stat & OWL_UART_STAT_TIP)
+ owl_uart_send_chars(port);
+
+ stat = owl_uart_read(port, OWL_UART_STAT);
+ stat |= OWL_UART_STAT_RIP | OWL_UART_STAT_TIP;
+ owl_uart_write(port, stat, OWL_UART_STAT);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ return IRQ_HANDLED;
+}
+
+static void owl_uart_shutdown(struct uart_port *port)
+{
+ u32 val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val &= ~(OWL_UART_CTL_TXIE | OWL_UART_CTL_RXIE
+ | OWL_UART_CTL_TXDE | OWL_UART_CTL_RXDE | OWL_UART_CTL_EN);
+ owl_uart_write(port, val, OWL_UART_CTL);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ free_irq(port->irq, port);
+}
+
+static int owl_uart_startup(struct uart_port *port)
+{
+ u32 val;
+ unsigned long flags;
+ int ret;
+
+ ret = request_irq(port->irq, owl_uart_irq, IRQF_TRIGGER_HIGH,
+ "owl-uart", port);
+ if (ret)
+ return ret;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ val = owl_uart_read(port, OWL_UART_STAT);
+ val |= OWL_UART_STAT_RIP | OWL_UART_STAT_TIP
+ | OWL_UART_STAT_RXER | OWL_UART_STAT_TFER | OWL_UART_STAT_RXST;
+ owl_uart_write(port, val, OWL_UART_STAT);
+
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val |= OWL_UART_CTL_RXIE | OWL_UART_CTL_TXIE;
+ val |= OWL_UART_CTL_EN;
+ owl_uart_write(port, val, OWL_UART_CTL);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ return 0;
+}
+
+static void owl_uart_change_baudrate(struct owl_uart_port *owl_port,
+ unsigned long baud)
+{
+ clk_set_rate(owl_port->clk, baud * 8);
+}
+
+static void owl_uart_set_termios(struct uart_port *port,
+ struct ktermios *termios,
+ struct ktermios *old)
+{
+ struct owl_uart_port *owl_port = to_owl_uart_port(port);
+ unsigned int baud;
+ u32 ctl;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ ctl = owl_uart_read(port, OWL_UART_CTL);
+
+ ctl &= ~OWL_UART_CTL_DWLS_MASK;
+ switch (termios->c_cflag & CSIZE) {
+ case CS5:
+ ctl |= OWL_UART_CTL_DWLS_5BITS;
+ break;
+ case CS6:
+ ctl |= OWL_UART_CTL_DWLS_6BITS;
+ break;
+ case CS7:
+ ctl |= OWL_UART_CTL_DWLS_7BITS;
+ break;
+ case CS8:
+ default:
+ ctl |= OWL_UART_CTL_DWLS_8BITS;
+ break;
+ }
+
+ if (termios->c_cflag & CSTOPB)
+ ctl |= OWL_UART_CTL_STPS_2BITS;
+ else
+ ctl &= ~OWL_UART_CTL_STPS_2BITS;
+
+ ctl &= ~OWL_UART_CTL_PRS_MASK;
+ if (termios->c_cflag & PARENB) {
+ if (termios->c_cflag & CMSPAR) {
+ if (termios->c_cflag & PARODD)
+ ctl |= OWL_UART_CTL_PRS_MARK;
+ else
+ ctl |= OWL_UART_CTL_PRS_SPACE;
+ } else if (termios->c_cflag & PARODD)
+ ctl |= OWL_UART_CTL_PRS_ODD;
+ else
+ ctl |= OWL_UART_CTL_PRS_EVEN;
+ } else
+ ctl |= OWL_UART_CTL_PRS_NONE;
+
+ if (termios->c_cflag & CRTSCTS)
+ ctl |= OWL_UART_CTL_AFE;
+ else
+ ctl &= ~OWL_UART_CTL_AFE;
+
+ owl_uart_write(port, ctl, OWL_UART_CTL);
+
+ baud = uart_get_baud_rate(port, termios, old, 9600, 3200000);
+ owl_uart_change_baudrate(owl_port, baud);
+
+ /* Don't rewrite B0 */
+ if (tty_termios_baud_rate(termios))
+ tty_termios_encode_baud_rate(termios, baud, baud);
+
+ port->read_status_mask |= OWL_UART_STAT_RXER;
+ if (termios->c_iflag & INPCK)
+ port->read_status_mask |= OWL_UART_STAT_RXST;
+
+ uart_update_timeout(port, termios->c_cflag, baud);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void owl_uart_release_port(struct uart_port *port)
+{
+ struct platform_device *pdev = to_platform_device(port->dev);
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return;
+
+ if (port->flags & UPF_IOREMAP) {
+ devm_release_mem_region(port->dev, port->mapbase,
+ resource_size(res));
+ devm_iounmap(port->dev, port->membase);
+ port->membase = NULL;
+ }
+}
+
+static int owl_uart_request_port(struct uart_port *port)
+{
+ struct platform_device *pdev = to_platform_device(port->dev);
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENXIO;
+
+ if (!devm_request_mem_region(port->dev, port->mapbase,
+ resource_size(res), dev_name(port->dev)))
+ return -EBUSY;
+
+ if (port->flags & UPF_IOREMAP) {
+ port->membase = devm_ioremap_nocache(port->dev, port->mapbase,
+ resource_size(res));
+ if (!port->membase)
+ return -EBUSY;
+ }
+
+ return 0;
+}
+
+static const char *owl_uart_type(struct uart_port *port)
+{
+ return (port->type == PORT_OWL) ? "owl-uart" : NULL;
+}
+
+static int owl_uart_verify_port(struct uart_port *port,
+ struct serial_struct *ser)
+{
+ if (port->type != PORT_OWL)
+ return -EINVAL;
+
+ if (port->irq != ser->irq)
+ return -EINVAL;
+
+ return 0;
+}
+
+static void owl_uart_config_port(struct uart_port *port, int flags)
+{
+ if (flags & UART_CONFIG_TYPE) {
+ port->type = PORT_OWL;
+ owl_uart_request_port(port);
+ }
+}
+
+static struct uart_ops owl_uart_ops = {
+ .set_mctrl = owl_uart_set_mctrl,
+ .get_mctrl = owl_uart_get_mctrl,
+ .tx_empty = owl_uart_tx_empty,
+ .start_tx = owl_uart_start_tx,
+ .stop_rx = owl_uart_stop_rx,
+ .stop_tx = owl_uart_stop_tx,
+ .startup = owl_uart_startup,
+ .shutdown = owl_uart_shutdown,
+ .set_termios = owl_uart_set_termios,
+ .type = owl_uart_type,
+ .config_port = owl_uart_config_port,
+ .request_port = owl_uart_request_port,
+ .release_port = owl_uart_release_port,
+ .verify_port = owl_uart_verify_port,
+};
+
#ifdef CONFIG_SERIAL_OWL_CONSOLE
static void owl_console_putchar(struct uart_port *port, int ch)
@@ -110,6 +546,57 @@ static void owl_uart_port_write(struct uart_port *port, const char *s,
local_irq_restore(flags);
}
+static void owl_uart_console_write(struct console *co, const char *s,
+ u_int count)
+{
+ struct owl_uart_port *owl_port;
+
+ owl_port = owl_uart_ports[co->index];
+ if (!owl_port)
+ return;
+
+ owl_uart_port_write(&owl_port->port, s, count);
+}
+
+static int owl_uart_console_setup(struct console *co, char *options)
+{
+ struct owl_uart_port *owl_port;
+ int baud = 115200;
+ int bits = 8;
+ int parity = 'n';
+ int flow = 'n';
+
+ if (co->index < 0 || co->index >= OWL_UART_PORT_NUM)
+ return -EINVAL;
+
+ owl_port = owl_uart_ports[co->index];
+ if (!owl_port || !owl_port->port.membase)
+ return -ENODEV;
+
+ if (options)
+ uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+ return uart_set_options(&owl_port->port, co, baud, parity, bits, flow);
+}
+
+static struct console owl_uart_console = {
+ .name = OWL_UART_DEV_NAME,
+ .write = owl_uart_console_write,
+ .device = uart_console_device,
+ .setup = owl_uart_console_setup,
+ .flags = CON_PRINTBUFFER,
+ .index = -1,
+ .data = &owl_uart_driver,
+};
+
+static int __init owl_uart_console_init(void)
+{
+ register_console(&owl_uart_console);
+
+ return 0;
+}
+console_initcall(owl_uart_console_init);
+
static void owl_uart_early_console_write(struct console *co,
const char *s,
u_int count)
@@ -132,4 +619,148 @@ owl_uart_early_console_setup(struct earlycon_device *device, const char *opt)
OF_EARLYCON_DECLARE(owl, "actions,owl-uart",
owl_uart_early_console_setup);
-#endif /* CONFIG_SERIAL_OWL_CONSOLE */
+#define OWL_UART_CONSOLE (&owl_uart_console)
+#else
+#define OWL_UART_CONSOLE NULL
+#endif
+
+static struct uart_driver owl_uart_driver = {
+ .owner = THIS_MODULE,
+ .driver_name = "owl-uart",
+ .dev_name = OWL_UART_DEV_NAME,
+ .nr = OWL_UART_PORT_NUM,
+ .cons = OWL_UART_CONSOLE,
+};
+
+static const struct owl_uart_info owl_s500_info = {
+ .tx_fifosize = 16,
+};
+
+static const struct owl_uart_info owl_s900_info = {
+ .tx_fifosize = 32,
+};
+
+static const struct of_device_id owl_uart_dt_matches[] = {
+ { .compatible = "actions,s500-uart", .data = &owl_s500_info },
+ { .compatible = "actions,s900-uart", .data = &owl_s900_info },
+ { }
+};
+MODULE_DEVICE_TABLE(of, owl_uart_dt_matches);
+
+static int owl_uart_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *match;
+ const struct owl_uart_info *info = NULL;
+ struct resource *res_mem;
+ struct owl_uart_port *owl_port;
+ int ret, irq;
+
+ if (pdev->dev.of_node) {
+ pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
+ match = of_match_node(owl_uart_dt_matches, pdev->dev.of_node);
+ if (match)
+ info = match->data;
+ }
+
+ if (pdev->id < 0 || pdev->id >= OWL_UART_PORT_NUM) {
+ dev_err(&pdev->dev, "id %d out of range\n", pdev->id);
+ return -EINVAL;
+ }
+
+ res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res_mem) {
+ dev_err(&pdev->dev, "could not get mem\n");
+ return -ENODEV;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "could not get irq\n");
+ return irq;
+ }
+
+ if (owl_uart_ports[pdev->id]) {
+ dev_err(&pdev->dev, "port %d already allocated\n", pdev->id);
+ return -EBUSY;
+ }
+
+ owl_port = devm_kzalloc(&pdev->dev, sizeof(*owl_port), GFP_KERNEL);
+ if (!owl_port)
+ return -ENOMEM;
+
+ owl_port->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(owl_port->clk)) {
+ dev_err(&pdev->dev, "could not get clk\n");
+ return PTR_ERR(owl_port->clk);
+ }
+
+ owl_port->port.dev = &pdev->dev;
+ owl_port->port.line = pdev->id;
+ owl_port->port.type = PORT_OWL;
+ owl_port->port.iotype = UPIO_MEM;
+ owl_port->port.mapbase = res_mem->start;
+ owl_port->port.irq = irq;
+ owl_port->port.uartclk = clk_get_rate(owl_port->clk);
+ if (owl_port->port.uartclk == 0) {
+ dev_err(&pdev->dev, "clock rate is zero\n");
+ return -EINVAL;
+ }
+ owl_port->port.flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
+ owl_port->port.x_char = 0;
+ owl_port->port.fifosize = (info) ? info->tx_fifosize : 16;
+ owl_port->port.ops = &owl_uart_ops;
+
+ owl_uart_ports[pdev->id] = owl_port;
+ platform_set_drvdata(pdev, owl_port);
+
+ ret = uart_add_one_port(&owl_uart_driver, &owl_port->port);
+ if (ret)
+ owl_uart_ports[pdev->id] = NULL;
+
+ return ret;
+}
+
+static int owl_uart_remove(struct platform_device *pdev)
+{
+ struct owl_uart_port *owl_port = platform_get_drvdata(pdev);
+
+ uart_remove_one_port(&owl_uart_driver, &owl_port->port);
+ owl_uart_ports[pdev->id] = NULL;
+
+ return 0;
+}
+
+static struct platform_driver owl_uart_platform_driver = {
+ .probe = owl_uart_probe,
+ .remove = owl_uart_remove,
+ .driver = {
+ .name = "owl-uart",
+ .of_match_table = owl_uart_dt_matches,
+ },
+};
+
+static int __init owl_uart_init(void)
+{
+ int ret;
+
+ ret = uart_register_driver(&owl_uart_driver);
+ if (ret)
+ return ret;
+
+ ret = platform_driver_register(&owl_uart_platform_driver);
+ if (ret)
+ uart_unregister_driver(&owl_uart_driver);
+
+ return ret;
+}
+
+static void __init owl_uart_exit(void)
+{
+ platform_driver_unregister(&owl_uart_platform_driver);
+ uart_unregister_driver(&owl_uart_driver);
+}
+
+module_init(owl_uart_init);
+module_exit(owl_uart_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index c34a2a3eeff5..38bea3217ead 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -70,6 +70,7 @@
#define PORT_CLPS711X 33
#define PORT_SA1100 34
#define PORT_UART00 35
+#define PORT_OWL 36
#define PORT_21285 37
/* Sparc type numbers. */
--
2.12.3
^ permalink raw reply related
* Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
From: Matt Redfearn @ 2017-07-07 7:58 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby,
linux-serial, Steven Rostedt, linux-kernel
In-Reply-To: <20170707044537.GB7478@jagdpanzerIV.localdomain>
On 07/07/17 05:45, Sergey Senozhatsky wrote:
> On (07/06/17 11:38), Matt Redfearn wrote:
>> Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in
>> printk_late_init() if keep_bootcon") added a check on keep_bootcon to
>> ensure that boot consoles were kept around until the real console is
>> registered.
>> This can lead to problems if the boot console data and code are in the
>> init section, since it can be freed before the boot console is
>> deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
>> unregister boot consoles when necessary").
>> The keep_bootcon flag prevents the unregistration of a boot console,
>> even if it's data and code reside in the init section and are about to
>> be freed. This can lead to crashes and weird panics when the bootconsole
>> is accessed after free, especially if page poisoning is in use and the
>> code / data have been overwritten with a poison value.
>> To prevent this, always free the boot console if it is within the init
>> section.
>>
>> Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
>> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
> well... hm. don't specify `keep_bootcon' if your bootcon access initdata?
> keeping `early_printk' sometimes can be helpful and people even want to
> use `early_printk' as a panic() console fallback (because of those nasty
> deadlocks that spoil printk->call_console_drivers()).
>
> if someone asked to `keep_bootcon' but we actually don't keep it, then
> what's the purpose of the flag and
> pr_info("debug: skip boot console de-registration.\n")?
Sure, but as a user, how are you supposed to know that? And if your
kernel is configured with SLAB poisoning, when the earlycon resident in
initdata is freed all of it's text and data is replaced with a poison
value, meaning it definitely won't work anymore anyway, and will instead
likely just panic the kernel, often with a completely meaningless
message (I know, because that's how I tracked down the need for this patch).
With this patch, the earlycon will only be unregistered if it is in the
init section and will no longer be functional anyway. Perhaps it would
be better to change the structure to make that more explicit, something
like:
if ((con->flags & CON_BOOT) &&
init_section_intersects(con, sizeof(*con))) {
/*
* Make sure to unregister boot consoles whose
data
* resides in the init section before the init
section
* is discarded. Boot consoles whose data will
stick
* around will automatically be unregistered
when the
* proper console replaces them.
*/
unregister_console(con);
}
Thanks,
Matt
>
>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index a1db38abac5b..b5411f44eed4 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2660,7 +2660,7 @@ static int __init printk_late_init(void)
>> int ret;
>>
>> for_each_console(con) {
>> - if (!keep_bootcon && con->flags & CON_BOOT) {
>> + if (con->flags & CON_BOOT) {
>> /*
>> * Make sure to unregister boot consoles whose data
>> * resides in the init section before the init section
> what about bootconsoles that are still not getting de-registered due
> to keep_bootcon in register_console()?
>
> -ss
^ permalink raw reply
* Re: [PATCH 2/2] serial: earlycon: Make early_con as __initdata
From: Sergey Senozhatsky @ 2017-07-07 4:47 UTC (permalink / raw)
To: Matt Redfearn
Cc: Petr Mladek, Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby,
linux-serial, linux-kernel
In-Reply-To: <1499337481-19397-2-git-send-email-matt.redfearn@imgtec.com>
On (07/06/17 11:38), Matt Redfearn wrote:
> All early console drivers that may be registered as the earlycon are
> marked __init to be placed in the init section. The drivers' code and
> data are freed during free_initmem_default() but the early console is
> not unregistered in printk_late_init() as the init_section_intersects()
> test fails. This leads to the earlycon still being active, potentially
> with dangling pointers into the freed init section, which may have been
> poisoned by the slab debugger. Attempting to use the boot console after
> this will likely lead to weird behaviour and kernel crashes.
people want to use early-con as a panic() console fallback.
-ss
> Fix this by marking the early_con struct __initdata so that the
> init_section_intersects() test succeeds and the console is unregistered
> in printk_late_init() before the init section is freed.
>
> The 8250 earlycon, on which the generic earlycon was based, had these
> attributes on it's console struct. The switch to the generic
> implementation in commit d2fd6810a823 ("tty/serial: convert 8250 to
> generic earlycon") appears to have broken unregistraton of the boot
> console when its code and data are in __init.
>
> Fixes: d2fd6810a823 ("tty/serial: convert 8250 to generic earlycon")
> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
>
> ---
>
> drivers/tty/serial/earlycon.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index c3651540e1ba..388aaafcca2b 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -29,13 +29,13 @@
>
> #include <asm/serial.h>
>
> -static struct console early_con = {
> +static struct console early_con __initdata = {
> .name = "uart", /* fixed up at earlycon registration */
> .flags = CON_PRINTBUFFER | CON_BOOT,
> .index = 0,
> };
>
> -static struct earlycon_device early_console_dev = {
> +static struct earlycon_device early_console_dev __initdata = {
> .con = &early_con,
> };
>
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
From: Sergey Senozhatsky @ 2017-07-07 4:45 UTC (permalink / raw)
To: Matt Redfearn
Cc: Petr Mladek, Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby,
linux-serial, Steven Rostedt, linux-kernel
In-Reply-To: <1499337481-19397-1-git-send-email-matt.redfearn@imgtec.com>
On (07/06/17 11:38), Matt Redfearn wrote:
> Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in
> printk_late_init() if keep_bootcon") added a check on keep_bootcon to
> ensure that boot consoles were kept around until the real console is
> registered.
> This can lead to problems if the boot console data and code are in the
> init section, since it can be freed before the boot console is
> deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
> unregister boot consoles when necessary").
> The keep_bootcon flag prevents the unregistration of a boot console,
> even if it's data and code reside in the init section and are about to
> be freed. This can lead to crashes and weird panics when the bootconsole
> is accessed after free, especially if page poisoning is in use and the
> code / data have been overwritten with a poison value.
> To prevent this, always free the boot console if it is within the init
> section.
>
> Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
well... hm. don't specify `keep_bootcon' if your bootcon access initdata?
keeping `early_printk' sometimes can be helpful and people even want to
use `early_printk' as a panic() console fallback (because of those nasty
deadlocks that spoil printk->call_console_drivers()).
if someone asked to `keep_bootcon' but we actually don't keep it, then
what's the purpose of the flag and
pr_info("debug: skip boot console de-registration.\n")?
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index a1db38abac5b..b5411f44eed4 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2660,7 +2660,7 @@ static int __init printk_late_init(void)
> int ret;
>
> for_each_console(con) {
> - if (!keep_bootcon && con->flags & CON_BOOT) {
> + if (con->flags & CON_BOOT) {
> /*
> * Make sure to unregister boot consoles whose data
> * resides in the init section before the init section
what about bootconsoles that are still not getting de-registered due
to keep_bootcon in register_console()?
-ss
^ permalink raw reply
* Re: [PATCH v2 2/6] serial: imx: Simplify DMA disablement
From: kbuild test robot @ 2017-07-07 1:45 UTC (permalink / raw)
Cc: kbuild-all, Greg Kroah-Hartman, linux-serial, linux-arm-kernel,
linux-kernel, Nandor Han, Romain Perier
In-Reply-To: <20170705130706.10388-3-romain.perier@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]
Hi Nandor,
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.12 next-20170706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Romain-Perier/serial-imx-various-improvements/20170706-153226
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arm-multi_v5_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: the linux-review/Romain-Perier/serial-imx-various-improvements/20170706-153226 HEAD c99e6e0fd195719d99b5ce9a4f9a155243772578 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/tty/serial/imx.c: In function 'imx_disable_dma':
>> drivers/tty/serial/imx.c:1250:2: error: 'temp' undeclared (first use in this function)
temp = readl(sport->port.membase + UCR2);
^~~~
drivers/tty/serial/imx.c:1250:2: note: each undeclared identifier is reported only once for each function it appears in
vim +/temp +1250 drivers/tty/serial/imx.c
b4cdc8f6 Huang Shijie 2013-07-08 1244 static void imx_disable_dma(struct imx_port *sport)
b4cdc8f6 Huang Shijie 2013-07-08 1245 {
52fe0ee0 Nandor Han 2017-07-05 1246 imx_stop_rx_dma(sport);
52fe0ee0 Nandor Han 2017-07-05 1247 imx_stop_tx_dma(sport);
b4cdc8f6 Huang Shijie 2013-07-08 1248
b4cdc8f6 Huang Shijie 2013-07-08 1249 /* clear UCR2 */
b4cdc8f6 Huang Shijie 2013-07-08 @1250 temp = readl(sport->port.membase + UCR2);
86a04ba6 Lucas Stach 2015-09-04 1251 temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
b4cdc8f6 Huang Shijie 2013-07-08 1252 writel(temp, sport->port.membase + UCR2);
b4cdc8f6 Huang Shijie 2013-07-08 1253
:::::: The code at line 1250 was first introduced by commit
:::::: b4cdc8f61beb2a55c8c3d22dfcaf5f34a919fe9b serial: imx: add DMA support for imx6q
:::::: TO: Huang Shijie <b32955@freescale.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29150 bytes --]
^ permalink raw reply
* [PATCH 2/2] serial: earlycon: Make early_con as __initdata
From: Matt Redfearn @ 2017-07-06 10:38 UTC (permalink / raw)
To: Petr Mladek, Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, Matt Redfearn, linux-kernel
In-Reply-To: <1499337481-19397-1-git-send-email-matt.redfearn@imgtec.com>
All early console drivers that may be registered as the earlycon are
marked __init to be placed in the init section. The drivers' code and
data are freed during free_initmem_default() but the early console is
not unregistered in printk_late_init() as the init_section_intersects()
test fails. This leads to the earlycon still being active, potentially
with dangling pointers into the freed init section, which may have been
poisoned by the slab debugger. Attempting to use the boot console after
this will likely lead to weird behaviour and kernel crashes.
Fix this by marking the early_con struct __initdata so that the
init_section_intersects() test succeeds and the console is unregistered
in printk_late_init() before the init section is freed.
The 8250 earlycon, on which the generic earlycon was based, had these
attributes on it's console struct. The switch to the generic
implementation in commit d2fd6810a823 ("tty/serial: convert 8250 to
generic earlycon") appears to have broken unregistraton of the boot
console when its code and data are in __init.
Fixes: d2fd6810a823 ("tty/serial: convert 8250 to generic earlycon")
Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
---
drivers/tty/serial/earlycon.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index c3651540e1ba..388aaafcca2b 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -29,13 +29,13 @@
#include <asm/serial.h>
-static struct console early_con = {
+static struct console early_con __initdata = {
.name = "uart", /* fixed up at earlycon registration */
.flags = CON_PRINTBUFFER | CON_BOOT,
.index = 0,
};
-static struct earlycon_device early_console_dev = {
+static struct earlycon_device early_console_dev __initdata = {
.con = &early_con,
};
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
From: Matt Redfearn @ 2017-07-06 10:38 UTC (permalink / raw)
To: Petr Mladek, Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, Matt Redfearn, Steven Rostedt, linux-kernel
Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in
printk_late_init() if keep_bootcon") added a check on keep_bootcon to
ensure that boot consoles were kept around until the real console is
registered.
This can lead to problems if the boot console data and code are in the
init section, since it can be freed before the boot console is
deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
unregister boot consoles when necessary").
The keep_bootcon flag prevents the unregistration of a boot console,
even if it's data and code reside in the init section and are about to
be freed. This can lead to crashes and weird panics when the bootconsole
is accessed after free, especially if page poisoning is in use and the
code / data have been overwritten with a poison value.
To prevent this, always free the boot console if it is within the init
section.
Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
---
kernel/printk/printk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a1db38abac5b..b5411f44eed4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,7 @@ static int __init printk_late_init(void)
int ret;
for_each_console(con) {
- if (!keep_bootcon && con->flags & CON_BOOT) {
+ if (con->flags & CON_BOOT) {
/*
* Make sure to unregister boot consoles whose data
* resides in the init section before the init section
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
From: Uwe Kleine-König @ 2017-07-06 10:02 UTC (permalink / raw)
To: Romain Perier
Cc: Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel,
Nandor Han
In-Reply-To: <49bfa666-b036-3ed9-8209-9ea59468371b@collabora.com>
On Thu, Jul 06, 2017 at 10:31:38AM +0200, Romain Perier wrote:
> Hello,
>
>
> Le 05/07/2017 à 15:29, Uwe Kleine-König a écrit :
> > Hello,
> >
> > On Wed, Jul 05, 2017 at 03:07:01PM +0200, Romain Perier wrote:
> >> From: Nandor Han <nandor.han@ge.com>
> >>
> >> Avoid the situation when `dma_is_rxing` could incorrectly signal that
> >> DMA RX channel is receiving data in case DMA preparation or sg mapping
> >> fails.
> >>
> >> This commit fixes the issues by moving the assignment of dma_is_rxing
> >> out of imx_disable_rx_int(), then the variable is set to 1 from
> >> start_rx_dma() only when the preparation is correctly done.
> > I'd write:
> >
> > There are a few issues with setting dma_is_rxing to 1 in
> > imx_disable_rx_int:
> >
> > - Currently always after imx_disable_rx_int() the function
> > start_rx_dma() is called. This dependency isn't obvious though.
> > - start_rx_dma() does error checking and might exit without enabling
> > DMA but keeping dma_is_rxing 1.
> >
> > So the more natural place for setting dma_is_rxing to 1 is in
> > start_rx_dma after all errors are checked.
> >
> > If you use this, there is nothing left of Nandor Han's patch and you can
> > drop his authorship.
> >
> > Best regards
> > Uwe
> >
> Ok, will do. No other feedback for the rest of the series ? (just to
> know if I send a v3 or If I wait a bit...)
I didn't come around looking in the patches I didn't comment yet.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ 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