* Re: 答复: Re: [PATCH] tty: max3100: Fix oops while 'cat/proc/tty/driver/ttyMAX'
From: Jiri Slaby @ 2018-09-14 11:28 UTC (permalink / raw)
To: chen.lin5; +Cc: gregkh, linux-kernel, linux-serial
In-Reply-To: <201809131432456792094@zte.com.cn>
On 09/13/2018, 08:32 AM, chen.lin5@zte.com.cn wrote:
>
>
> but 'get_mctrl' is already protected by the upper layer by spin lock,
> so, will the races happen?
>
>
> for example: in /drivers/tty/serial/serial_core.c
>
> spin_lock_irq(&uport->lock);
>
> result |= uport->ops->get_mctrl(uport);
>
> spin_unlock_irq(&uport->lock);
Right, but creation and destroy of the workqueue is not locked as far as
I can see. Am I missing something?
thanks,.
--
js
suse labs
^ permalink raw reply
* Re: 答复: Re: 答复: Re: [PATCH] tty: max3100: Fix oops while 'cat/proc/tty/driver/ttyMAX'
From: Jiri Slaby @ 2018-09-16 8:09 UTC (permalink / raw)
To: chen.lin5; +Cc: gregkh, linux-kernel, linux-serial
In-Reply-To: <201809151014546700030@zte.com.cn>
On 09/15/2018, 04:14 AM, chen.lin5@zte.com.cn wrote:
> yes, creation and destroy of the workqueue is not locked, I think
> maybe there is some
>
> remainder work to do in destroy-wq, so I cannot sure if there is
> any usage about lock destroy-wq.
>
>
> What you worried of the races is about this ?
>
> --> when max3100_shutdown, destroy_workqueue is doing, s->workqueue
> is not NULL, at this moment, get_mctrl is executed, destroying wq is
> queued again.
>
> bu this cannot happen, becasue s->force_end_work = 1
> before destroy_workqueue , so max3100_dowork do nothing.
Oh, so this relies on flush_workqueue or destroy_workqueue to be a
barrier (so that the assignment to end_work is not reordered), correct?
> static void max3100_shutdown(struct uart_port *port)
>
> {
>
> ...
>
> s->force_end_work = 1;
>
>
> if (s->poll_time > 0)
>
> del_timer_sync(&s->timer);
>
>
> if (s->workqueue) {
>
> flush_workqueue(s->workqueue);
>
> destroy_workqueue(s->workqueue);
>
> s->workqueue = NULL;
>
> }
>
>
>
> static void max3100_dowork(struct max3100_port *s)
>
> {
>
> if (!s->force_end_work && !freezing(current) && !s->suspending)
>
> queue_work(s->workqueue, &s->work);
>
> }
Also, on the first open:
s->force_end_work = 0;
s->parity = 0;
s->rts = 0;
sprintf(b, "max3100-%d", s->minor);
s->workqueue = create_freezable_workqueue(b);
if (!s->workqueue) {
dev_warn(&s->spi->dev, "cannot create workqueue\n");
return -EBUSY;
}
Here, s->force_end_work is 0, s->workqueue is non-NULL, but s->work is
garbage until:
INIT_WORK(&s->work, max3100_work);
INIT_WORK should be in max3100_probe as far as I can see (or at least
before create_freezable_workqueue), right? But those variable
assignments also rely on some implicit barrier which is not there.
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH v2] serial: 8250_of: Fix for lack of interrupt support
From: John Garry @ 2018-09-17 14:55 UTC (permalink / raw)
To: gregkh
Cc: Alexander Sverdlin, jslaby, robh, joel, kurt, yamada.masahiro,
linux-serial, linux-kernel, linuxarm
In-Reply-To: <055f5ee8-4905-c229-0e33-6379cf65dfbc@nokia.com>
On 30/08/2018 19:21, Alexander Sverdlin wrote:
> Hello John!
>
Hi Greg,
Can you kindly consider picking up this patch?
It still applies to the branches today in your tty git, but let me know
if you still rather I resend.
Thanks,
John
> On 30/08/2018 11:08, John Garry wrote:
>> In commit c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ"), a
>> check was added for the UART driver being probed prior to the parent IRQ
>> controller.
>>
>> Unfortunately this breaks certain boards which have no interrupt support,
>> like Huawei D03.
>>
>> Indeed, the 8250 DT bindings state that interrupts should be supported -
>> not must.
>>
>> To fix, switch from irq_of_parse_and_map() to of_irq_get(), which
>> does relay whether the IRQ host controller domain is not ready, i.e.
>> defer probe, instead of assuming it.
>>
>> Fixes: c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ")
>> Signed-off-by: John Garry <john.garry@huawei.com>
>
> This indeed looks like a proper way to handle both cases:
>
> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>
>> ---
>>
>> Change in v2:
>> - fix check on irq value
>>
>> Note: I think that it would better if we could try to get the interrupt
>> before clk+pm enabling, so we don't need to disable later when
>> deferring, but this is not a fix.
>>
>> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
>> index af8beef..877fd7f 100644
>> --- a/drivers/tty/serial/8250/8250_of.c
>> +++ b/drivers/tty/serial/8250/8250_of.c
>> @@ -58,7 +58,7 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>> struct resource resource;
>> struct device_node *np = ofdev->dev.of_node;
>> u32 clk, spd, prop;
>> - int ret;
>> + int ret, irq;
>>
>> memset(port, 0, sizeof *port);
>>
>> @@ -143,21 +143,27 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>> if (ret >= 0)
>> port->line = ret;
>>
>> - port->irq = irq_of_parse_and_map(np, 0);
>> - if (!port->irq) {
>> - ret = -EPROBE_DEFER;
>> - goto err_unprepare;
>> + irq = of_irq_get(np, 0);
>> + if (irq < 0) {
>> + if (irq == -EPROBE_DEFER) {
>> + ret = -EPROBE_DEFER;
>> + goto err_unprepare;
>> + }
>> + /* IRQ support not mandatory */
>> + irq = 0;
>> }
>>
>> + port->irq = irq;
>> +
>> info->rst = devm_reset_control_get_optional_shared(&ofdev->dev, NULL);
>> if (IS_ERR(info->rst)) {
>> ret = PTR_ERR(info->rst);
>> - goto err_dispose;
>> + goto err_unprepare;
>> }
>>
>> ret = reset_control_deassert(info->rst);
>> if (ret)
>> - goto err_dispose;
>> + goto err_unprepare;
>>
>> port->type = type;
>> port->uartclk = clk;
>> @@ -184,8 +190,6 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>> port->handle_irq = fsl8250_handle_irq;
>>
>> return 0;
>> -err_dispose:
>> - irq_dispose_mapping(port->irq);
>> err_unprepare:
>> clk_disable_unprepare(info->clk);
>> err_pmruntime:
>>
>
^ permalink raw reply
* RE: [PATCH 1/2] Revert "serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE"
From: Chris Brandt @ 2018-09-17 17:10 UTC (permalink / raw)
To: Geert Uytterhoeven, Greg Kroah-Hartman
Cc: Jiri Slaby, Wolfram Sang, Ulrich Hecht,
linux-serial@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20180830125404.21192-2-geert+renesas@glider.be>
On Thursday, August 30, 2018, Geert Uytterhoeven wrote:
> This reverts commit 7acece71a517cad83a0842a94d94c13f271b680c.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> drivers/tty/serial/sh-sci.c | 31 +++++++++++++++++++++++++++++++
> include/linux/serial_sci.h | 1 +
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index ac4424bf6b136cc4..5d42c9a63001575a 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -291,6 +291,33 @@ static const struct sci_port_params
> sci_port_params[SCIx_NR_REGTYPES] = {
> .error_clear = SCIF_ERROR_CLEAR,
> },
>
> + /*
> + * The "SCIFA" that is in RZ/T and RZ/A2.
> + * It looks like a normal SCIF with FIFO data, but with a
> + * compressed address space. Also, the break out of interrupts
> + * are different: ERI/BRI, RXI, TXI, TEI, DRI.
> + */
> + [SCIx_RZ_SCIFA_REGTYPE] = {
> + .regs = {
> + [SCSMR] = { 0x00, 16 },
> + [SCBRR] = { 0x02, 8 },
> + [SCSCR] = { 0x04, 16 },
> + [SCxTDR] = { 0x06, 8 },
> + [SCxSR] = { 0x08, 16 },
> + [SCxRDR] = { 0x0A, 8 },
> + [SCFCR] = { 0x0C, 16 },
> + [SCFDR] = { 0x0E, 16 },
> + [SCSPTR] = { 0x10, 16 },
> + [SCLSR] = { 0x12, 16 },
> + },
> + .fifosize = 16,
> + .overrun_reg = SCLSR,
> + .overrun_mask = SCLSR_ORER,
> + .sampling_rate_mask = SCI_SR(32),
> + .error_mask = SCIF_DEFAULT_ERROR_MASK,
> + .error_clear = SCIF_ERROR_CLEAR,
> + },
> +
> /*
> * Common SH-3 SCIF definitions.
> */
> @@ -3110,6 +3137,10 @@ static const struct of_device_id of_sci_match[] =
> {
> .compatible = "renesas,scif-r7s72100",
> .data = SCI_OF_DATA(PORT_SCIF,
> SCIx_SH2_SCIF_FIFODATA_REGTYPE),
> },
> + {
> + .compatible = "renesas,scif-r7s9210",
> + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> + },
> /* Family-specific types */
> {
> .compatible = "renesas,rcar-gen1-scif",
> diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h
> index c0e795d95477daea..1c89611e0e0634ae 100644
> --- a/include/linux/serial_sci.h
> +++ b/include/linux/serial_sci.h
> @@ -36,6 +36,7 @@ enum {
> SCIx_SH4_SCIF_FIFODATA_REGTYPE,
> SCIx_SH7705_SCIF_REGTYPE,
> SCIx_HSCIF_REGTYPE,
> + SCIx_RZ_SCIFA_REGTYPE,
>
> SCIx_NR_REGTYPES,
> };
> --
> 2.17.1
^ permalink raw reply
* RE: [PATCH 2/2] Revert "serial: sh-sci: Allow for compressed SCIF address"
From: Chris Brandt @ 2018-09-17 17:11 UTC (permalink / raw)
To: Geert Uytterhoeven, Greg Kroah-Hartman
Cc: Jiri Slaby, Wolfram Sang, Ulrich Hecht,
linux-serial@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20180830125404.21192-3-geert+renesas@glider.be>
On Thursday, August 30, 2018, Geert Uytterhoeven wrote:
> This reverts commit 2d4dd0da45401c7ae7332b4d1eb7bbb1348edde9.
>
> This broke earlycon on all Renesas ARM platforms using a SCIF port for
> the
> serial console (R-Car, RZ/A1, RZ/G1, RZ/G2 SoCs), due to an incorrect
> value
> of port->regshift.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> drivers/tty/serial/sh-sci.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 5d42c9a63001575a..ab3f6e91853da3c2 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -346,15 +346,15 @@ static const struct sci_port_params
> sci_port_params[SCIx_NR_REGTYPES] = {
> [SCIx_SH4_SCIF_REGTYPE] = {
> .regs = {
> [SCSMR] = { 0x00, 16 },
> - [SCBRR] = { 0x02, 8 },
> - [SCSCR] = { 0x04, 16 },
> - [SCxTDR] = { 0x06, 8 },
> - [SCxSR] = { 0x08, 16 },
> - [SCxRDR] = { 0x0a, 8 },
> - [SCFCR] = { 0x0c, 16 },
> - [SCFDR] = { 0x0e, 16 },
> - [SCSPTR] = { 0x10, 16 },
> - [SCLSR] = { 0x12, 16 },
> + [SCBRR] = { 0x04, 8 },
> + [SCSCR] = { 0x08, 16 },
> + [SCxTDR] = { 0x0c, 8 },
> + [SCxSR] = { 0x10, 16 },
> + [SCxRDR] = { 0x14, 8 },
> + [SCFCR] = { 0x18, 16 },
> + [SCFDR] = { 0x1c, 16 },
> + [SCSPTR] = { 0x20, 16 },
> + [SCLSR] = { 0x24, 16 },
> },
> .fifosize = 16,
> .overrun_reg = SCLSR,
> @@ -2837,7 +2837,7 @@ static int sci_init_single(struct platform_device
> *dev,
> {
> struct uart_port *port = &sci_port->port;
> const struct resource *res;
> - unsigned int i, regtype;
> + unsigned int i;
> int ret;
>
> sci_port->cfg = p;
> @@ -2874,7 +2874,6 @@ static int sci_init_single(struct platform_device
> *dev,
> if (unlikely(sci_port->params == NULL))
> return -EINVAL;
>
> - regtype = sci_port->params - sci_port_params;
> switch (p->type) {
> case PORT_SCIFB:
> sci_port->rx_trigger = 48;
> @@ -2929,10 +2928,6 @@ static int sci_init_single(struct platform_device
> *dev,
> port->regshift = 1;
> }
>
> - if (regtype == SCIx_SH4_SCIF_REGTYPE)
> - if (sci_port->reg_size >= 0x20)
> - port->regshift = 1;
> -
> /*
> * The UART port needs an IRQ value, so we peg this to the RX IRQ
> * for the multi-IRQ ports, which is where we are primarily
> --
> 2.17.1
^ permalink raw reply
* [PATCH v2 1/5] serial: sprd: Remove unused structure
From: Baolin Wang @ 2018-09-17 18:33 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
Remove the unused reg_backup structure.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Acked-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
Changes from v1:
- Add acked tag from Chunyan.
---
drivers/tty/serial/sprd_serial.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 828f114..1b0e3fb 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -95,19 +95,8 @@
#define SPRD_IMSR_BREAK_DETECT BIT(7)
#define SPRD_IMSR_TIMEOUT BIT(13)
-struct reg_backup {
- u32 ien;
- u32 ctrl0;
- u32 ctrl1;
- u32 ctrl2;
- u32 clkd0;
- u32 clkd1;
- u32 dspwait;
-};
-
struct sprd_uart_port {
struct uart_port port;
- struct reg_backup reg_bak;
char name[16];
};
--
1.9.1
^ permalink raw reply related
* [PATCH v2 2/5] serial: sprd: Use readable macros instead of magic number
From: Baolin Wang @ 2018-09-17 18:33 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <96e34fdadaf3f354f5d8e0d2895508e7c7918839.1537208449.git.baolin.wang@linaro.org>
Define readable macros instead of magic number to make code more readable.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Acked-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
Changes from v1:
- Add acked tag from Chunyan.
---
drivers/tty/serial/sprd_serial.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 1b0e3fb..e18d8af 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -45,6 +45,8 @@
/* data number in TX and RX fifo */
#define SPRD_STS1 0x000C
+#define SPRD_RX_FIFO_CNT_MASK GENMASK(7, 0)
+#define SPRD_TX_FIFO_CNT_MASK GENMASK(15, 8)
/* interrupt enable register and its BITs */
#define SPRD_IEN 0x0010
@@ -82,11 +84,15 @@
/* fifo threshold register */
#define SPRD_CTL2 0x0020
#define THLD_TX_EMPTY 0x40
+#define THLD_TX_EMPTY_SHIFT 8
#define THLD_RX_FULL 0x40
/* config baud rate register */
#define SPRD_CLKD0 0x0024
+#define SPRD_CLKD0_MASK GENMASK(15, 0)
#define SPRD_CLKD1 0x0028
+#define SPRD_CLKD1_MASK GENMASK(20, 16)
+#define SPRD_CLKD1_SHIFT 16
/* interrupt mask status register */
#define SPRD_IMSR 0x002C
@@ -115,7 +121,7 @@ static inline void serial_out(struct uart_port *port, int offset, int value)
static unsigned int sprd_tx_empty(struct uart_port *port)
{
- if (serial_in(port, SPRD_STS1) & 0xff00)
+ if (serial_in(port, SPRD_STS1) & SPRD_TX_FIFO_CNT_MASK)
return 0;
else
return TIOCSER_TEMT;
@@ -213,7 +219,8 @@ static inline void sprd_rx(struct uart_port *port)
struct tty_port *tty = &port->state->port;
unsigned int ch, flag, lsr, max_count = SPRD_TIMEOUT;
- while ((serial_in(port, SPRD_STS1) & 0x00ff) && max_count--) {
+ while ((serial_in(port, SPRD_STS1) & SPRD_RX_FIFO_CNT_MASK) &&
+ max_count--) {
lsr = serial_in(port, SPRD_LSR);
ch = serial_in(port, SPRD_RXD);
flag = TTY_NORMAL;
@@ -303,16 +310,17 @@ static int sprd_startup(struct uart_port *port)
struct sprd_uart_port *sp;
unsigned long flags;
- serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
+ serial_out(port, SPRD_CTL2,
+ THLD_TX_EMPTY << THLD_TX_EMPTY_SHIFT | THLD_RX_FULL);
/* clear rx fifo */
timeout = SPRD_TIMEOUT;
- while (timeout-- && serial_in(port, SPRD_STS1) & 0x00ff)
+ while (timeout-- && serial_in(port, SPRD_STS1) & SPRD_RX_FIFO_CNT_MASK)
serial_in(port, SPRD_RXD);
/* clear tx fifo */
timeout = SPRD_TIMEOUT;
- while (timeout-- && serial_in(port, SPRD_STS1) & 0xff00)
+ while (timeout-- && serial_in(port, SPRD_STS1) & SPRD_TX_FIFO_CNT_MASK)
cpu_relax();
/* clear interrupt */
@@ -433,10 +441,11 @@ static void sprd_set_termios(struct uart_port *port,
}
/* clock divider bit0~bit15 */
- serial_out(port, SPRD_CLKD0, quot & 0xffff);
+ serial_out(port, SPRD_CLKD0, quot & SPRD_CLKD0_MASK);
/* clock divider bit16~bit20 */
- serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
+ serial_out(port, SPRD_CLKD1,
+ (quot & SPRD_CLKD1_MASK) >> SPRD_CLKD1_SHIFT);
serial_out(port, SPRD_LCR, lcr);
fc |= RX_TOUT_THLD_DEF | RX_HFC_THLD_DEF;
serial_out(port, SPRD_CTL1, fc);
@@ -510,7 +519,7 @@ static void wait_for_xmitr(struct uart_port *port)
if (--tmout == 0)
break;
udelay(1);
- } while (status & 0xff00);
+ } while (status & SPRD_TX_FIFO_CNT_MASK);
}
static void sprd_console_putchar(struct uart_port *port, int ch)
--
1.9.1
^ permalink raw reply related
* [PATCH v2 3/5] serial: sprd: Remove unnecessary resource validation
From: Baolin Wang @ 2018-09-17 18:33 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <96e34fdadaf3f354f5d8e0d2895508e7c7918839.1537208449.git.baolin.wang@linaro.org>
The devm_ioremap_resource() will valid the resources, thus remove the
unnecessary resource validation in the driver.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Acked-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
Changes from v1:
- Add acked tag from Chunyan.
---
drivers/tty/serial/sprd_serial.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index e18d8af..03b0cd4 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -710,15 +710,12 @@ static int sprd_probe(struct platform_device *pdev)
up->uartclk = clk_get_rate(clk);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev, "not provide mem resource\n");
- return -ENODEV;
- }
- up->mapbase = res->start;
up->membase = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(up->membase))
return PTR_ERR(up->membase);
+ up->mapbase = res->start;
+
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
dev_err(&pdev->dev, "not provide irq resource: %d\n", irq);
--
1.9.1
^ permalink raw reply related
* [PATCH v2 4/5] serial: sprd: Change 'int' to 'unsigned int'
From: Baolin Wang @ 2018-09-17 18:33 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <96e34fdadaf3f354f5d8e0d2895508e7c7918839.1537208449.git.baolin.wang@linaro.org>
The register offset value should be 'unsigned int' type.
Moreover, prefer 'unsigned int' to bare use of 'unsigned'.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Acked-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
Changes from v1:
- Add acked tag from Chunyan.
---
drivers/tty/serial/sprd_serial.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 03b0cd4..8d5c9cd 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -109,12 +109,14 @@ struct sprd_uart_port {
static struct sprd_uart_port *sprd_port[UART_NR_MAX];
static int sprd_ports_num;
-static inline unsigned int serial_in(struct uart_port *port, int offset)
+static inline unsigned int serial_in(struct uart_port *port,
+ unsigned int offset)
{
return readl_relaxed(port->membase + offset);
}
-static inline void serial_out(struct uart_port *port, int offset, int value)
+static inline void serial_out(struct uart_port *port, unsigned int offset,
+ int value)
{
writel_relaxed(value, port->membase + offset);
}
@@ -598,8 +600,7 @@ static void sprd_putc(struct uart_port *port, int c)
writeb(c, port->membase + SPRD_TXD);
}
-static void sprd_early_write(struct console *con, const char *s,
- unsigned n)
+static void sprd_early_write(struct console *con, const char *s, unsigned int n)
{
struct earlycon_device *dev = con->data;
--
1.9.1
^ permalink raw reply related
* [PATCH v2 5/5] serial: sprd: Fix the indentation issue
From: Baolin Wang @ 2018-09-17 18:33 UTC (permalink / raw)
To: gregkh, jslaby
Cc: orsonzhai, baolin.wang, zhang.lyra, broonie, linux-serial,
linux-kernel
In-Reply-To: <96e34fdadaf3f354f5d8e0d2895508e7c7918839.1537208449.git.baolin.wang@linaro.org>
Make the macros' definition and code have the same correct indentation.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Acked-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
Changes from v1:
- Add acked tag from Chunyan.
---
drivers/tty/serial/sprd_serial.c | 46 +++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 8d5c9cd..4287ca3 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -68,24 +68,24 @@
#define SPRD_LCR_DATA_LEN6 0x4
#define SPRD_LCR_DATA_LEN7 0x8
#define SPRD_LCR_DATA_LEN8 0xc
-#define SPRD_LCR_PARITY (BIT(0) | BIT(1))
+#define SPRD_LCR_PARITY (BIT(0) | BIT(1))
#define SPRD_LCR_PARITY_EN 0x2
#define SPRD_LCR_EVEN_PAR 0x0
#define SPRD_LCR_ODD_PAR 0x1
/* control register 1 */
-#define SPRD_CTL1 0x001C
+#define SPRD_CTL1 0x001C
#define RX_HW_FLOW_CTL_THLD BIT(6)
#define RX_HW_FLOW_CTL_EN BIT(7)
#define TX_HW_FLOW_CTL_EN BIT(8)
#define RX_TOUT_THLD_DEF 0x3E00
-#define RX_HFC_THLD_DEF 0x40
+#define RX_HFC_THLD_DEF 0x40
/* fifo threshold register */
#define SPRD_CTL2 0x0020
-#define THLD_TX_EMPTY 0x40
+#define THLD_TX_EMPTY 0x40
#define THLD_TX_EMPTY_SHIFT 8
-#define THLD_RX_FULL 0x40
+#define THLD_RX_FULL 0x40
/* config baud rate register */
#define SPRD_CLKD0 0x0024
@@ -95,11 +95,11 @@
#define SPRD_CLKD1_SHIFT 16
/* interrupt mask status register */
-#define SPRD_IMSR 0x002C
-#define SPRD_IMSR_RX_FIFO_FULL BIT(0)
+#define SPRD_IMSR 0x002C
+#define SPRD_IMSR_RX_FIFO_FULL BIT(0)
#define SPRD_IMSR_TX_FIFO_EMPTY BIT(1)
-#define SPRD_IMSR_BREAK_DETECT BIT(7)
-#define SPRD_IMSR_TIMEOUT BIT(13)
+#define SPRD_IMSR_BREAK_DETECT BIT(7)
+#define SPRD_IMSR_TIMEOUT BIT(13)
struct sprd_uart_port {
struct uart_port port;
@@ -229,7 +229,7 @@ static inline void sprd_rx(struct uart_port *port)
port->icount.rx++;
if (lsr & (SPRD_LSR_BI | SPRD_LSR_PE |
- SPRD_LSR_FE | SPRD_LSR_OE))
+ SPRD_LSR_FE | SPRD_LSR_OE))
if (handle_lsr_errors(port, &lsr, &flag))
continue;
if (uart_handle_sysrq_char(port, ch))
@@ -292,8 +292,8 @@ static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
if (ims & SPRD_IMSR_TIMEOUT)
serial_out(port, SPRD_ICLR, SPRD_ICLR_TIMEOUT);
- if (ims & (SPRD_IMSR_RX_FIFO_FULL |
- SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
+ if (ims & (SPRD_IMSR_RX_FIFO_FULL | SPRD_IMSR_BREAK_DETECT |
+ SPRD_IMSR_TIMEOUT))
sprd_rx(port);
if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
@@ -333,7 +333,7 @@ static int sprd_startup(struct uart_port *port)
sp = container_of(port, struct sprd_uart_port, port);
snprintf(sp->name, sizeof(sp->name), "sprd_serial%d", port->line);
ret = devm_request_irq(port->dev, port->irq, sprd_handle_irq,
- IRQF_SHARED, sp->name, port);
+ IRQF_SHARED, sp->name, port);
if (ret) {
dev_err(port->dev, "fail to request serial irq %d, ret=%d\n",
port->irq, ret);
@@ -361,8 +361,8 @@ static void sprd_shutdown(struct uart_port *port)
}
static void sprd_set_termios(struct uart_port *port,
- struct ktermios *termios,
- struct ktermios *old)
+ struct ktermios *termios,
+ struct ktermios *old)
{
unsigned int baud, quot;
unsigned int lcr = 0, fc;
@@ -480,8 +480,7 @@ static void sprd_config_port(struct uart_port *port, int flags)
port->type = PORT_SPRD;
}
-static int sprd_verify_port(struct uart_port *port,
- struct serial_struct *ser)
+static int sprd_verify_port(struct uart_port *port, struct serial_struct *ser)
{
if (ser->type != PORT_SPRD)
return -EINVAL;
@@ -531,7 +530,7 @@ static void sprd_console_putchar(struct uart_port *port, int ch)
}
static void sprd_console_write(struct console *co, const char *s,
- unsigned int count)
+ unsigned int count)
{
struct uart_port *port = &sprd_port[co->index]->port;
int locked = 1;
@@ -594,7 +593,7 @@ static void sprd_putc(struct uart_port *port, int c)
unsigned int timeout = SPRD_TIMEOUT;
while (timeout-- &&
- !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
+ !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
cpu_relax();
writeb(c, port->membase + SPRD_TXD);
@@ -607,9 +606,8 @@ static void sprd_early_write(struct console *con, const char *s, unsigned int n)
uart_console_write(&dev->port, s, n, sprd_putc);
}
-static int __init sprd_early_console_setup(
- struct earlycon_device *device,
- const char *opt)
+static int __init sprd_early_console_setup(struct earlycon_device *device,
+ const char *opt)
{
if (!device->port.membase)
return -ENODEV;
@@ -691,8 +689,8 @@ static int sprd_probe(struct platform_device *pdev)
index = sprd_probe_dt_alias(index, &pdev->dev);
- sprd_port[index] = devm_kzalloc(&pdev->dev,
- sizeof(*sprd_port[index]), GFP_KERNEL);
+ sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
+ GFP_KERNEL);
if (!sprd_port[index])
return -ENOMEM;
--
1.9.1
^ permalink raw reply related
* [PATCH v2 2/2] serial: uartps: Change uart ID port allocation
From: Michal Simek @ 2018-09-18 11:51 UTC (permalink / raw)
To: linux-kernel, monstr
Cc: Jiri Slaby, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
In-Reply-To: <6b07b21f60dcf07e1f6d6e39bae1f3750c4aa804.1537271478.git.michal.simek@xilinx.com>
For IPs which have alias algorightm all the time using that alias and
minor number. It means serial20 alias ends up as ttyPS20.
If alias is not setup for probed IP instance the first unused position is
used but that needs to be checked if it is really empty because another
instance doesn't need to be probed at that time. of_alias_get_alias_list()
fills alias bitmap which exactly shows which ID is free.
If alias pointing to different not compatible IP, it is free to use.
cdns_get_id() call is placed below structure allocation to simplify
error path.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Add handle of return value from of_alias_get_alias_list
Needs to be applied on the top of
https://lkml.org/lkml/2018/9/3/404
https://lkml.org/lkml/2018/9/3/400 (minor that's why just in case)
---
drivers/tty/serial/xilinx_uartps.c | 109 ++++++++++++++++++++++++++++++++-----
1 file changed, 96 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 71c032744dae..f77200a0f461 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -30,7 +30,6 @@
#define CDNS_UART_TTY_NAME "ttyPS"
#define CDNS_UART_NAME "xuartps"
#define CDNS_UART_MAJOR 0 /* use dynamic node allocation */
-#define CDNS_UART_NR_PORTS 2
#define CDNS_UART_FIFO_SIZE 64 /* FIFO size */
#define CDNS_UART_REGISTER_SPACE 0x1000
@@ -1370,6 +1369,88 @@ static int __maybe_unused cdns_runtime_resume(struct device *dev)
};
MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
+/*
+ * Maximum number of instances without alias IDs but if there is alias
+ * which target "< MAX_UART_INSTANCES" range this ID can't be used.
+ */
+#define MAX_UART_INSTANCES 32
+
+/* Stores static aliases list */
+static DECLARE_BITMAP(alias_bitmap, MAX_UART_INSTANCES);
+static int alias_bitmap_initialized;
+
+/* Stores actual bitmap of allocated IDs with alias IDs together */
+static DECLARE_BITMAP(bitmap, MAX_UART_INSTANCES);
+/* Protect bitmap operations to have unique IDs */
+static DEFINE_MUTEX(bitmap_lock);
+
+static int cdns_get_id(struct platform_device *pdev)
+{
+ int id, ret;
+
+ mutex_lock(&bitmap_lock);
+
+ /* Alias list is stable that's why get alias bitmap only once */
+ if (!alias_bitmap_initialized) {
+ ret = of_alias_get_alias_list(cdns_uart_of_match, "serial",
+ alias_bitmap, MAX_UART_INSTANCES);
+ if (ret)
+ return ret;
+
+ alias_bitmap_initialized++;
+ }
+
+ /* Make sure that alias ID is not taken by instance without alias */
+ bitmap_or(bitmap, bitmap, alias_bitmap, MAX_UART_INSTANCES);
+
+ dev_dbg(&pdev->dev, "Alias bitmap: %*pb\n",
+ MAX_UART_INSTANCES, bitmap);
+
+ /* Look for a serialN alias */
+ id = of_alias_get_id(pdev->dev.of_node, "serial");
+ if (id < 0) {
+ dev_warn(&pdev->dev,
+ "No serial alias passed. Using the first free id\n");
+
+ /*
+ * Start with id 0 and check if there is no serial0 alias
+ * which points to device which is compatible with this driver.
+ * If alias exists then try next free position.
+ */
+ id = 0;
+
+ for (;;) {
+ dev_info(&pdev->dev, "Checking id %d\n", id);
+ id = find_next_zero_bit(bitmap, MAX_UART_INSTANCES, id);
+
+ /* No free empty instance */
+ if (id == MAX_UART_INSTANCES) {
+ dev_err(&pdev->dev, "No free ID\n");
+ mutex_unlock(&bitmap_lock);
+ return -EINVAL;
+ }
+
+ dev_dbg(&pdev->dev, "The empty id is %d\n", id);
+ /* Check if ID is empty */
+ if (!test_and_set_bit(id, bitmap)) {
+ /* Break the loop if bit is taken */
+ dev_dbg(&pdev->dev,
+ "Selected ID %d allocation passed\n",
+ id);
+ break;
+ }
+ dev_dbg(&pdev->dev,
+ "Selected ID %d allocation failed\n", id);
+ /* if taking bit fails then try next one */
+ id++;
+ }
+ }
+
+ mutex_unlock(&bitmap_lock);
+
+ return id;
+}
+
/**
* cdns_uart_probe - Platform driver probe
* @pdev: Pointer to the platform device structure
@@ -1403,21 +1484,17 @@ static int cdns_uart_probe(struct platform_device *pdev)
if (!cdns_uart_uart_driver)
return -ENOMEM;
- /* Look for a serialN alias */
- cdns_uart_data->id = of_alias_get_id(pdev->dev.of_node, "serial");
+ cdns_uart_data->id = cdns_get_id(pdev);
if (cdns_uart_data->id < 0)
- cdns_uart_data->id = 0;
-
- if (cdns_uart_data->id >= CDNS_UART_NR_PORTS) {
- dev_err(&pdev->dev, "Cannot get uart_port structure\n");
- return -ENODEV;
- }
+ return cdns_uart_data->id;
/* There is a need to use unique driver name */
driver_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s%d",
CDNS_UART_NAME, cdns_uart_data->id);
- if (!driver_name)
- return -ENOMEM;
+ if (!driver_name) {
+ rc = -ENOMEM;
+ goto err_out_id;
+ }
cdns_uart_uart_driver->owner = THIS_MODULE;
cdns_uart_uart_driver->driver_name = driver_name;
@@ -1446,7 +1523,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
rc = uart_register_driver(cdns_uart_uart_driver);
if (rc < 0) {
dev_err(&pdev->dev, "Failed to register driver\n");
- return rc;
+ goto err_out_id;
}
cdns_uart_data->cdns_uart_driver = cdns_uart_uart_driver;
@@ -1587,7 +1664,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
clk_disable_unprepare(cdns_uart_data->pclk);
err_out_unregister_driver:
uart_unregister_driver(cdns_uart_data->cdns_uart_driver);
-
+err_out_id:
+ mutex_lock(&bitmap_lock);
+ clear_bit(cdns_uart_data->id, bitmap);
+ mutex_unlock(&bitmap_lock);
return rc;
}
@@ -1610,6 +1690,9 @@ static int cdns_uart_remove(struct platform_device *pdev)
#endif
rc = uart_remove_one_port(cdns_uart_data->cdns_uart_driver, port);
port->mapbase = 0;
+ mutex_lock(&bitmap_lock);
+ clear_bit(cdns_uart_data->id, bitmap);
+ mutex_unlock(&bitmap_lock);
clk_disable_unprepare(cdns_uart_data->uartclk);
clk_disable_unprepare(cdns_uart_data->pclk);
pm_runtime_disable(&pdev->dev);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3 00/10] serial: uartps: Add run time support for more IPs than hardcoded 2
From: Michal Simek @ 2018-09-18 11:54 UTC (permalink / raw)
To: Michal Simek, linux-kernel, gnomes, Alexander Graf, shubhraj,
robh, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <cover.1535980253.git.michal.simek@xilinx.com>
Hi Greg,
On 3.9.2018 15:10, Michal Simek wrote:
> Hi,
>
> this series is trying to address discussion I had with Alan in past
> https://patchwork.kernel.org/patch/9738445/ and also with Rob in v1
> https://lkml.org/lkml/2018/4/26/551.
>
> These patches are doing preparation to enable dynamic ID port allocation
> which is capable to create devices with higher IDs.
>
> For example this is how it works.
> uart0 on higher alias
> serial0 = &uart1;
> serial30 = &uart0;
>
> ~# ls -la /dev/ttyPS*
> crw------- 1 root root 252, 0 Jun 6 12:19 /dev/ttyPS0
> crw--w---- 1 root root 253, 100 Jan 1 1970 /dev/ttyPS30
>
> Thanks,
> Michal
>
> Changes in v3:
> - New patch found by testing
> - New patch - can be sent separately but there is dependency that's why
> sent in this series
> - Fix uart_unregister_driver() in error path or when driver is removed.
> - Change commit message
> - s/,/;/ in filling structure
> - get cdns_uart_uart_driver out of PS_UART_CONSOLE ifdefs
> - New patch in series
> - Rebase on the top of previous broken patch
> - Change patch subject ("was serial: uartps: Remove CDNS_UART_NR_PORTS
> macro")
> - Keep CDNS_UART_NR_PORTS in this patch and remove it in next one and
> align commit message to reflect this
> - Allocate struct console dynamically too to be unique for every
> instance
> - Cleanup error path
> - New patch in series
>
> Changes in v2:
> - new patch - it can be sent separately too
> - new patch - it can be sent separately too
> - new patch - it can be sent separately too
> - Remove nr field logic
> - new patch - it can be sent separately too
> - Register one uart_driver with unique minor at probe time
>
> Michal Simek (9):
> serial: uartps: console_setup() can't be placed to init section
> serial: uartps: Do not initialize field to zero again
> serial: uartps: Do not use static struct uart_driver out of probe()
> serial: uartps: Move alias reading higher in probe()
> serial: uartps: Move register to probe based on run time detection
> serial: uartps: Fill struct uart_driver in probe()
> serial: uartps: Change logic how console_port is setup
> serial: uartps: Register own uart console and driver structures
> serial: uartps: Move Port ID to device data structure
>
> Nava kishore Manne (1):
> serial: uartps: Fix suspend functionality
>
> drivers/tty/serial/xilinx_uartps.c | 203 +++++++++++++++++++------------------
> 1 file changed, 107 insertions(+), 96 deletions(-)
>
Can you please look at this series?
I have sent 2 more patches on the top of this series where OF patch was
reviewed by Rob already.
Thanks,
Michal
^ permalink raw reply
* Re: [PATCH v3 00/10] serial: uartps: Add run time support for more IPs than hardcoded 2
From: Greg Kroah-Hartman @ 2018-09-18 12:01 UTC (permalink / raw)
To: Michal Simek
Cc: linux-kernel, gnomes, Alexander Graf, shubhraj, robh, Jiri Slaby,
linux-serial, linux-arm-kernel
In-Reply-To: <ac23383e-3d54-1cd2-c1f4-219c8ee306d2@xilinx.com>
On Tue, Sep 18, 2018 at 01:54:31PM +0200, Michal Simek wrote:
> Hi Greg,
>
> On 3.9.2018 15:10, Michal Simek wrote:
> > Hi,
> >
> > this series is trying to address discussion I had with Alan in past
> > https://patchwork.kernel.org/patch/9738445/ and also with Rob in v1
> > https://lkml.org/lkml/2018/4/26/551.
> >
> > These patches are doing preparation to enable dynamic ID port allocation
> > which is capable to create devices with higher IDs.
> >
> > For example this is how it works.
> > uart0 on higher alias
> > serial0 = &uart1;
> > serial30 = &uart0;
> >
> > ~# ls -la /dev/ttyPS*
> > crw------- 1 root root 252, 0 Jun 6 12:19 /dev/ttyPS0
> > crw--w---- 1 root root 253, 100 Jan 1 1970 /dev/ttyPS30
> >
> > Thanks,
> > Michal
> >
> > Changes in v3:
> > - New patch found by testing
> > - New patch - can be sent separately but there is dependency that's why
> > sent in this series
> > - Fix uart_unregister_driver() in error path or when driver is removed.
> > - Change commit message
> > - s/,/;/ in filling structure
> > - get cdns_uart_uart_driver out of PS_UART_CONSOLE ifdefs
> > - New patch in series
> > - Rebase on the top of previous broken patch
> > - Change patch subject ("was serial: uartps: Remove CDNS_UART_NR_PORTS
> > macro")
> > - Keep CDNS_UART_NR_PORTS in this patch and remove it in next one and
> > align commit message to reflect this
> > - Allocate struct console dynamically too to be unique for every
> > instance
> > - Cleanup error path
> > - New patch in series
> >
> > Changes in v2:
> > - new patch - it can be sent separately too
> > - new patch - it can be sent separately too
> > - new patch - it can be sent separately too
> > - Remove nr field logic
> > - new patch - it can be sent separately too
> > - Register one uart_driver with unique minor at probe time
> >
> > Michal Simek (9):
> > serial: uartps: console_setup() can't be placed to init section
> > serial: uartps: Do not initialize field to zero again
> > serial: uartps: Do not use static struct uart_driver out of probe()
> > serial: uartps: Move alias reading higher in probe()
> > serial: uartps: Move register to probe based on run time detection
> > serial: uartps: Fill struct uart_driver in probe()
> > serial: uartps: Change logic how console_port is setup
> > serial: uartps: Register own uart console and driver structures
> > serial: uartps: Move Port ID to device data structure
> >
> > Nava kishore Manne (1):
> > serial: uartps: Fix suspend functionality
> >
> > drivers/tty/serial/xilinx_uartps.c | 203 +++++++++++++++++++------------------
> > 1 file changed, 107 insertions(+), 96 deletions(-)
> >
>
> Can you please look at this series?
> I have sent 2 more patches on the top of this series where OF patch was
> reviewed by Rob already.
My tty queue is a bit lagging right now, sorry, will get to that soon...
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] sc16is7xx: Fix for multi-channel stall
From: Greg Kroah-Hartman @ 2018-09-18 13:02 UTC (permalink / raw)
To: Phil Elwell
Cc: Jiri Slaby, linux-serial, linux-kernel, Alexander Graf,
Stefan Wahren
In-Reply-To: <1536762716-30673-2-git-send-email-phil@raspberrypi.org>
On Wed, Sep 12, 2018 at 03:31:55PM +0100, Phil Elwell wrote:
> The SC16IS752 is a dual-channel device. The two channels are largely
> independent, but the IRQ signals are wired together as an open-drain,
> active low signal which will be driven low while either of the
> channels requires attention, which can be for significant periods of
> time until operations complete and the interrupt can be acknowledged.
> In that respect it is should be treated as a true level-sensitive IRQ.
>
> The kernel, however, needs to be able to exit interrupt context in
> order to use I2C or SPI to access the device registers (which may
> involve sleeping). Therefore the interrupt needs to be masked out or
> paused in some way.
>
> The usual way to manage sleeping from within an interrupt handler
> is to use a threaded interrupt handler - a regular interrupt routine
> does the minimum amount of work needed to triage the interrupt before
> waking the interrupt service thread. If the threaded IRQ is marked as
> IRQF_ONESHOT the kernel will automatically mask out the interrupt
> until the thread runs to completion. The sc16is7xx driver used to
> use a threaded IRQ, but a patch switched to using a kthread_worker
> in order to set realtime priorities on the handler thread and for
> other optimisations. The end result is non-threaded IRQ that
> schedules some work then returns IRQ_HANDLED, making the kernel
> think that all IRQ processing has completed.
>
> The work-around to prevent a constant stream of interrupts is to
> mark the interrupt as edge-sensitive rather than level-sensitive,
> but interpreting an active-low source as a falling-edge source
> requires care to prevent a total cessation of interrupts. Whereas
> an edge-triggering source will generate a new edge for every interrupt
> condition a level-triggering source will keep the signal at the
> interrupting level until it no longer requires attention; in other
> words, the host won't see another edge until all interrupt conditions
> are cleared. It is therefore vital that the interrupt handler does not
> exit with an outstanding interrupt condition, otherwise the kernel
> will not receive another interrupt unless some other operation causes
> the interrupt state on the device to be cleared.
>
> The existing sc16is7xx driver has a very simple interrupt "thread"
> (kthread_work job) that processes interrupts on each channel in turn
> until there are no more. If both channels are active and the first
> channel starts interrupting while the handler for the second channel
> is running then it will not be detected and an IRQ stall ensues. This
> could be handled easily if there was a shared IRQ status register, or
> a convenient way to determine if the IRQ had been deasserted for any
> length of time, but both appear to be lacking.
>
> Avoid this problem (or at least make it much less likely to happen)
> by reducing the granularity of per-channel interrupt processing
> to one condition per iteration, only exiting the overall loop when
> both channels are no longer interrupting.
>
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
> drivers/tty/serial/sc16is7xx.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 243c960..47b4115 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -657,7 +657,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
> uart_write_wakeup(port);
> }
>
> -static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> +static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> {
> struct uart_port *port = &s->p[portno].port;
>
> @@ -666,7 +666,7 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
>
> iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
> if (iir & SC16IS7XX_IIR_NO_INT_BIT)
> - break;
> + return false;
>
> iir &= SC16IS7XX_IIR_ID_MASK;
>
> @@ -688,16 +688,23 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> port->line, iir);
> break;
> }
> - } while (1);
> + } while (0);
> + return true;
> }
>
> static void sc16is7xx_ist(struct kthread_work *ws)
> {
> struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
> - int i;
>
> - for (i = 0; i < s->devtype->nr_uart; ++i)
> - sc16is7xx_port_irq(s, i);
> + while (1) {
> + bool keep_polling = false;
> + int i;
> +
> + for (i = 0; i < s->devtype->nr_uart; ++i)
> + keep_polling |= sc16is7xx_port_irq(s, i);
> + if (!keep_polling)
> + break;
This makes me worried, there is no "timeout" now? What happens if this
never happens, will you just sit and spin forever? What prevents that?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] sc16is7xx: Fix for multi-channel stall
From: Phil Elwell @ 2018-09-18 13:13 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Alexander Graf,
Stefan Wahren
In-Reply-To: <20180918130243.GA25253@kroah.com>
Hi Greg,
On 18/09/2018 14:02, Greg Kroah-Hartman wrote:
> On Wed, Sep 12, 2018 at 03:31:55PM +0100, Phil Elwell wrote:
>> The SC16IS752 is a dual-channel device. The two channels are largely
>> independent, but the IRQ signals are wired together as an open-drain,
>> active low signal which will be driven low while either of the
>> channels requires attention, which can be for significant periods of
>> time until operations complete and the interrupt can be acknowledged.
>> In that respect it is should be treated as a true level-sensitive IRQ.
>>
>> The kernel, however, needs to be able to exit interrupt context in
>> order to use I2C or SPI to access the device registers (which may
>> involve sleeping). Therefore the interrupt needs to be masked out or
>> paused in some way.
>>
>> The usual way to manage sleeping from within an interrupt handler
>> is to use a threaded interrupt handler - a regular interrupt routine
>> does the minimum amount of work needed to triage the interrupt before
>> waking the interrupt service thread. If the threaded IRQ is marked as
>> IRQF_ONESHOT the kernel will automatically mask out the interrupt
>> until the thread runs to completion. The sc16is7xx driver used to
>> use a threaded IRQ, but a patch switched to using a kthread_worker
>> in order to set realtime priorities on the handler thread and for
>> other optimisations. The end result is non-threaded IRQ that
>> schedules some work then returns IRQ_HANDLED, making the kernel
>> think that all IRQ processing has completed.
>>
>> The work-around to prevent a constant stream of interrupts is to
>> mark the interrupt as edge-sensitive rather than level-sensitive,
>> but interpreting an active-low source as a falling-edge source
>> requires care to prevent a total cessation of interrupts. Whereas
>> an edge-triggering source will generate a new edge for every interrupt
>> condition a level-triggering source will keep the signal at the
>> interrupting level until it no longer requires attention; in other
>> words, the host won't see another edge until all interrupt conditions
>> are cleared. It is therefore vital that the interrupt handler does not
>> exit with an outstanding interrupt condition, otherwise the kernel
>> will not receive another interrupt unless some other operation causes
>> the interrupt state on the device to be cleared.
>>
>> The existing sc16is7xx driver has a very simple interrupt "thread"
>> (kthread_work job) that processes interrupts on each channel in turn
>> until there are no more. If both channels are active and the first
>> channel starts interrupting while the handler for the second channel
>> is running then it will not be detected and an IRQ stall ensues. This
>> could be handled easily if there was a shared IRQ status register, or
>> a convenient way to determine if the IRQ had been deasserted for any
>> length of time, but both appear to be lacking.
>>
>> Avoid this problem (or at least make it much less likely to happen)
>> by reducing the granularity of per-channel interrupt processing
>> to one condition per iteration, only exiting the overall loop when
>> both channels are no longer interrupting.
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>> drivers/tty/serial/sc16is7xx.c | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>> index 243c960..47b4115 100644
>> --- a/drivers/tty/serial/sc16is7xx.c
>> +++ b/drivers/tty/serial/sc16is7xx.c
>> @@ -657,7 +657,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
>> uart_write_wakeup(port);
>> }
>>
>> -static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
>> +static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
>> {
>> struct uart_port *port = &s->p[portno].port;
>>
>> @@ -666,7 +666,7 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
>>
>> iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
>> if (iir & SC16IS7XX_IIR_NO_INT_BIT)
>> - break;
>> + return false;
>>
>> iir &= SC16IS7XX_IIR_ID_MASK;
>>
>> @@ -688,16 +688,23 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
>> port->line, iir);
>> break;
>> }
>> - } while (1);
>> + } while (0);
>> + return true;
>> }
>>
>> static void sc16is7xx_ist(struct kthread_work *ws)
>> {
>> struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
>> - int i;
>>
>> - for (i = 0; i < s->devtype->nr_uart; ++i)
>> - sc16is7xx_port_irq(s, i);
>> + while (1) {
>> + bool keep_polling = false;
>> + int i;
>> +
>> + for (i = 0; i < s->devtype->nr_uart; ++i)
>> + keep_polling |= sc16is7xx_port_irq(s, i);
>> + if (!keep_polling)
>> + break;
>
> This makes me worried, there is no "timeout" now? What happens if this
> never happens, will you just sit and spin forever? What prevents that?
The patch is keeping to the spirit of the original, which also has a
potentially infinite loop (in sc16is7xx_port_irq) - this just moves it
up one level.
I could add a limit on the number of iterations, but if the limit is ever hit,
leading to an early exit, the port is basically dead because it will never
receive another interrupt.
Phil
^ permalink raw reply
* Re: [PATCH 1/2] sc16is7xx: Fix for multi-channel stall
From: Greg Kroah-Hartman @ 2018-09-18 13:26 UTC (permalink / raw)
To: Phil Elwell
Cc: Jiri Slaby, linux-serial, linux-kernel, Alexander Graf,
Stefan Wahren
In-Reply-To: <07ee2375-382c-154a-4c47-abb1a81b3351@raspberrypi.org>
On Tue, Sep 18, 2018 at 02:13:15PM +0100, Phil Elwell wrote:
> Hi Greg,
>
> On 18/09/2018 14:02, Greg Kroah-Hartman wrote:
> > On Wed, Sep 12, 2018 at 03:31:55PM +0100, Phil Elwell wrote:
> >> The SC16IS752 is a dual-channel device. The two channels are largely
> >> independent, but the IRQ signals are wired together as an open-drain,
> >> active low signal which will be driven low while either of the
> >> channels requires attention, which can be for significant periods of
> >> time until operations complete and the interrupt can be acknowledged.
> >> In that respect it is should be treated as a true level-sensitive IRQ.
> >>
> >> The kernel, however, needs to be able to exit interrupt context in
> >> order to use I2C or SPI to access the device registers (which may
> >> involve sleeping). Therefore the interrupt needs to be masked out or
> >> paused in some way.
> >>
> >> The usual way to manage sleeping from within an interrupt handler
> >> is to use a threaded interrupt handler - a regular interrupt routine
> >> does the minimum amount of work needed to triage the interrupt before
> >> waking the interrupt service thread. If the threaded IRQ is marked as
> >> IRQF_ONESHOT the kernel will automatically mask out the interrupt
> >> until the thread runs to completion. The sc16is7xx driver used to
> >> use a threaded IRQ, but a patch switched to using a kthread_worker
> >> in order to set realtime priorities on the handler thread and for
> >> other optimisations. The end result is non-threaded IRQ that
> >> schedules some work then returns IRQ_HANDLED, making the kernel
> >> think that all IRQ processing has completed.
> >>
> >> The work-around to prevent a constant stream of interrupts is to
> >> mark the interrupt as edge-sensitive rather than level-sensitive,
> >> but interpreting an active-low source as a falling-edge source
> >> requires care to prevent a total cessation of interrupts. Whereas
> >> an edge-triggering source will generate a new edge for every interrupt
> >> condition a level-triggering source will keep the signal at the
> >> interrupting level until it no longer requires attention; in other
> >> words, the host won't see another edge until all interrupt conditions
> >> are cleared. It is therefore vital that the interrupt handler does not
> >> exit with an outstanding interrupt condition, otherwise the kernel
> >> will not receive another interrupt unless some other operation causes
> >> the interrupt state on the device to be cleared.
> >>
> >> The existing sc16is7xx driver has a very simple interrupt "thread"
> >> (kthread_work job) that processes interrupts on each channel in turn
> >> until there are no more. If both channels are active and the first
> >> channel starts interrupting while the handler for the second channel
> >> is running then it will not be detected and an IRQ stall ensues. This
> >> could be handled easily if there was a shared IRQ status register, or
> >> a convenient way to determine if the IRQ had been deasserted for any
> >> length of time, but both appear to be lacking.
> >>
> >> Avoid this problem (or at least make it much less likely to happen)
> >> by reducing the granularity of per-channel interrupt processing
> >> to one condition per iteration, only exiting the overall loop when
> >> both channels are no longer interrupting.
> >>
> >> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> >> ---
> >> drivers/tty/serial/sc16is7xx.c | 19 +++++++++++++------
> >> 1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >> index 243c960..47b4115 100644
> >> --- a/drivers/tty/serial/sc16is7xx.c
> >> +++ b/drivers/tty/serial/sc16is7xx.c
> >> @@ -657,7 +657,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
> >> uart_write_wakeup(port);
> >> }
> >>
> >> -static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> >> +static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> >> {
> >> struct uart_port *port = &s->p[portno].port;
> >>
> >> @@ -666,7 +666,7 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> >>
> >> iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
> >> if (iir & SC16IS7XX_IIR_NO_INT_BIT)
> >> - break;
> >> + return false;
> >>
> >> iir &= SC16IS7XX_IIR_ID_MASK;
> >>
> >> @@ -688,16 +688,23 @@ static void sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> >> port->line, iir);
> >> break;
> >> }
> >> - } while (1);
> >> + } while (0);
> >> + return true;
> >> }
> >>
> >> static void sc16is7xx_ist(struct kthread_work *ws)
> >> {
> >> struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
> >> - int i;
> >>
> >> - for (i = 0; i < s->devtype->nr_uart; ++i)
> >> - sc16is7xx_port_irq(s, i);
> >> + while (1) {
> >> + bool keep_polling = false;
> >> + int i;
> >> +
> >> + for (i = 0; i < s->devtype->nr_uart; ++i)
> >> + keep_polling |= sc16is7xx_port_irq(s, i);
> >> + if (!keep_polling)
> >> + break;
> >
> > This makes me worried, there is no "timeout" now? What happens if this
> > never happens, will you just sit and spin forever? What prevents that?
>
> The patch is keeping to the spirit of the original, which also has a
> potentially infinite loop (in sc16is7xx_port_irq) - this just moves it
> up one level.
>
> I could add a limit on the number of iterations, but if the limit is ever hit,
> leading to an early exit, the port is basically dead because it will never
> receive another interrupt.
Ok, it's your hardware, good luck with it! :)
greg k-h
^ permalink raw reply
* Re: [PATCH v4 2/2] tty/serial: atmel: add ISO7816 support
From: Greg KH @ 2018-09-18 13:28 UTC (permalink / raw)
To: Ludovic Desroches
Cc: linux-serial, linux-arch, linux-arm-kernel, jslaby, arnd,
richard.genoud, nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <20180906134214.29031-3-ludovic.desroches@microchip.com>
On Thu, Sep 06, 2018 at 03:42:14PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>
> When mode is set in atmel_config_iso7816() we backup last RS232 mode
> for coming back to this mode if requested.
> Also allow setup of T=0 and T=1 parameter and basic support in set_termios
> function as well.
>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> [ludovic.desroches@microchip.com: rebase, add check on fidi ratio, checkpatch fixes]
> Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> Acked-by: Richard Genoud <richard.genoud@gmail.com>
> ---
> drivers/tty/serial/atmel_serial.c | 190 +++++++++++++++++++++++++++++++++++---
> drivers/tty/serial/atmel_serial.h | 3 +-
> 2 files changed, 181 insertions(+), 12 deletions(-)
This doesn't apply to my tree :(
Also, shouldn't this new ioctl be documented somewhere?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] sc16is7xx: Fix for multi-channel stall
From: Rogier Wolff @ 2018-09-18 13:37 UTC (permalink / raw)
To: Phil Elwell
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Alexander Graf, Stefan Wahren
In-Reply-To: <07ee2375-382c-154a-4c47-abb1a81b3351@raspberrypi.org>
On Tue, Sep 18, 2018 at 02:13:15PM +0100, Phil Elwell wrote:
> I could add a limit on the number of iterations, but if the limit is ever hit,
> leading to an early exit, the port is basically dead because it will never
> receive another interrupt.
Especially if you print something like: "<driver name>: Too many
iterations with work-to-do bailing out" while bailing out, then
hanging just one driver/piece of hardware as opposed to the whole
system when somehow the hardware never indicates "all work done"
would be preferable.
Under normal circumstances you never expect to hit that number of
iterations. But if the card keeps hitting the driver with "more work
to do" then you'd hang the system. Better try and recover, and provide
debug info for the user who knows where to look.
Best would be to ignore the driver for say a second and start handling
interrupts again a while later. Should the system be overloaded with
work, (and a slow CPU?) then you can recover and just make things slow
down a bit without hanging the system.
Getting edge-triggered interrupts right is VERY difficult. In general
I'd advise against them. It looks like a nice solution, but in reality
the chances for difficult-to-debug race conditions is enormous. In
those race conditions the card will get "new work to do" and
(re-)assert the interrupt line when the driver is already on the "no
more work to do" path.
Roger.
--
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2600998 **
** Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
The plan was simple, like my brother-in-law Phil. But unlike
Phil, this plan just might work.
^ permalink raw reply
* [PATCH v3 2/2] serial: uartps: Change uart ID port allocation
From: Michal Simek @ 2018-09-18 14:57 UTC (permalink / raw)
To: linux-kernel, monstr
Cc: Jiri Slaby, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
In-Reply-To: <b1772d99d4e8b788448cde0fc5c7dccb1f63b507.1537282676.git.michal.simek@xilinx.com>
For IPs which have alias algorightm all the time using that alias and
minor number. It means serial20 alias ends up as ttyPS20.
If alias is not setup for probed IP instance the first unused position is
used but that needs to be checked if it is really empty because another
instance doesn't need to be probed at that time. of_alias_get_alias_list()
fills alias bitmap which exactly shows which ID is free.
If alias pointing to different not compatible IP, it is free to use.
cdns_get_id() call is placed below structure allocation to simplify
error path.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v3: None
Changes in v2:
- Add handle of return value from of_alias_get_alias_list
Needs to be applied on the top of
https://lkml.org/lkml/2018/9/3/404
https://lkml.org/lkml/2018/9/3/400 (minor that's why just in case)
Greg: Please apply it with
"of: base: Introduce of_alias_get_alias_list() to check alias IDs
Rob is ok with that.
---
drivers/tty/serial/xilinx_uartps.c | 109 ++++++++++++++++++++++++++++++++-----
1 file changed, 96 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 71c032744dae..f77200a0f461 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -30,7 +30,6 @@
#define CDNS_UART_TTY_NAME "ttyPS"
#define CDNS_UART_NAME "xuartps"
#define CDNS_UART_MAJOR 0 /* use dynamic node allocation */
-#define CDNS_UART_NR_PORTS 2
#define CDNS_UART_FIFO_SIZE 64 /* FIFO size */
#define CDNS_UART_REGISTER_SPACE 0x1000
@@ -1370,6 +1369,88 @@ static int __maybe_unused cdns_runtime_resume(struct device *dev)
};
MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
+/*
+ * Maximum number of instances without alias IDs but if there is alias
+ * which target "< MAX_UART_INSTANCES" range this ID can't be used.
+ */
+#define MAX_UART_INSTANCES 32
+
+/* Stores static aliases list */
+static DECLARE_BITMAP(alias_bitmap, MAX_UART_INSTANCES);
+static int alias_bitmap_initialized;
+
+/* Stores actual bitmap of allocated IDs with alias IDs together */
+static DECLARE_BITMAP(bitmap, MAX_UART_INSTANCES);
+/* Protect bitmap operations to have unique IDs */
+static DEFINE_MUTEX(bitmap_lock);
+
+static int cdns_get_id(struct platform_device *pdev)
+{
+ int id, ret;
+
+ mutex_lock(&bitmap_lock);
+
+ /* Alias list is stable that's why get alias bitmap only once */
+ if (!alias_bitmap_initialized) {
+ ret = of_alias_get_alias_list(cdns_uart_of_match, "serial",
+ alias_bitmap, MAX_UART_INSTANCES);
+ if (ret)
+ return ret;
+
+ alias_bitmap_initialized++;
+ }
+
+ /* Make sure that alias ID is not taken by instance without alias */
+ bitmap_or(bitmap, bitmap, alias_bitmap, MAX_UART_INSTANCES);
+
+ dev_dbg(&pdev->dev, "Alias bitmap: %*pb\n",
+ MAX_UART_INSTANCES, bitmap);
+
+ /* Look for a serialN alias */
+ id = of_alias_get_id(pdev->dev.of_node, "serial");
+ if (id < 0) {
+ dev_warn(&pdev->dev,
+ "No serial alias passed. Using the first free id\n");
+
+ /*
+ * Start with id 0 and check if there is no serial0 alias
+ * which points to device which is compatible with this driver.
+ * If alias exists then try next free position.
+ */
+ id = 0;
+
+ for (;;) {
+ dev_info(&pdev->dev, "Checking id %d\n", id);
+ id = find_next_zero_bit(bitmap, MAX_UART_INSTANCES, id);
+
+ /* No free empty instance */
+ if (id == MAX_UART_INSTANCES) {
+ dev_err(&pdev->dev, "No free ID\n");
+ mutex_unlock(&bitmap_lock);
+ return -EINVAL;
+ }
+
+ dev_dbg(&pdev->dev, "The empty id is %d\n", id);
+ /* Check if ID is empty */
+ if (!test_and_set_bit(id, bitmap)) {
+ /* Break the loop if bit is taken */
+ dev_dbg(&pdev->dev,
+ "Selected ID %d allocation passed\n",
+ id);
+ break;
+ }
+ dev_dbg(&pdev->dev,
+ "Selected ID %d allocation failed\n", id);
+ /* if taking bit fails then try next one */
+ id++;
+ }
+ }
+
+ mutex_unlock(&bitmap_lock);
+
+ return id;
+}
+
/**
* cdns_uart_probe - Platform driver probe
* @pdev: Pointer to the platform device structure
@@ -1403,21 +1484,17 @@ static int cdns_uart_probe(struct platform_device *pdev)
if (!cdns_uart_uart_driver)
return -ENOMEM;
- /* Look for a serialN alias */
- cdns_uart_data->id = of_alias_get_id(pdev->dev.of_node, "serial");
+ cdns_uart_data->id = cdns_get_id(pdev);
if (cdns_uart_data->id < 0)
- cdns_uart_data->id = 0;
-
- if (cdns_uart_data->id >= CDNS_UART_NR_PORTS) {
- dev_err(&pdev->dev, "Cannot get uart_port structure\n");
- return -ENODEV;
- }
+ return cdns_uart_data->id;
/* There is a need to use unique driver name */
driver_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s%d",
CDNS_UART_NAME, cdns_uart_data->id);
- if (!driver_name)
- return -ENOMEM;
+ if (!driver_name) {
+ rc = -ENOMEM;
+ goto err_out_id;
+ }
cdns_uart_uart_driver->owner = THIS_MODULE;
cdns_uart_uart_driver->driver_name = driver_name;
@@ -1446,7 +1523,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
rc = uart_register_driver(cdns_uart_uart_driver);
if (rc < 0) {
dev_err(&pdev->dev, "Failed to register driver\n");
- return rc;
+ goto err_out_id;
}
cdns_uart_data->cdns_uart_driver = cdns_uart_uart_driver;
@@ -1587,7 +1664,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
clk_disable_unprepare(cdns_uart_data->pclk);
err_out_unregister_driver:
uart_unregister_driver(cdns_uart_data->cdns_uart_driver);
-
+err_out_id:
+ mutex_lock(&bitmap_lock);
+ clear_bit(cdns_uart_data->id, bitmap);
+ mutex_unlock(&bitmap_lock);
return rc;
}
@@ -1610,6 +1690,9 @@ static int cdns_uart_remove(struct platform_device *pdev)
#endif
rc = uart_remove_one_port(cdns_uart_data->cdns_uart_driver, port);
port->mapbase = 0;
+ mutex_lock(&bitmap_lock);
+ clear_bit(cdns_uart_data->id, bitmap);
+ mutex_unlock(&bitmap_lock);
clk_disable_unprepare(cdns_uart_data->uartclk);
clk_disable_unprepare(cdns_uart_data->pclk);
pm_runtime_disable(&pdev->dev);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Vinod @ 2018-09-18 16:35 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
In-Reply-To: <20180901164215.3683-3-manivannan.sadhasivam@linaro.org>
On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> OWL_DMA_MODE_DAM_INC;
>
> + break;
> + case DMA_MEM_TO_DEV:
> + mode |= OWL_DMA_MODE_TS(vchan->drq)
> + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> +
> + /* Handle bus width for UART */
> + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> + mode |= OWL_DMA_MODE_NDDBW_8BIT;
this is fine per se, but not correct way to handle in dmaengine driver.
You should be agnostic to user of dmaengine, so handle all the buswidths
the IP block supports and update the values accordingly. That way new
uses can be added w/o requiring change in dmaengine driver
--
~Vinod
^ permalink raw reply
* Re: [PATCH 2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Manivannan Sadhasivam @ 2018-09-18 22:52 UTC (permalink / raw)
To: Vinod
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
In-Reply-To: <20180918163512.GE2613@vkoul-mobl>
On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
>
> > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > OWL_DMA_MODE_DAM_INC;
> >
> > + break;
> > + case DMA_MEM_TO_DEV:
> > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > +
> > + /* Handle bus width for UART */
> > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
>
> this is fine per se, but not correct way to handle in dmaengine driver.
> You should be agnostic to user of dmaengine, so handle all the buswidths
> the IP block supports and update the values accordingly. That way new
> uses can be added w/o requiring change in dmaengine driver
>
Hi Vinod,
Currently, all members of Owl family supports only 32bit and 8bit
bus widths. 32bit is common for all peripherals and 8bit applies to only
UART since the internal buffer is 8bit wide. So, this makes sense to me!
Thanks,
Mani
> --
> ~Vinod
^ permalink raw reply
* Re: [PATCH 2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Vinod @ 2018-09-18 23:32 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
In-Reply-To: <20180918225236.GA17306@Mani-XPS-13-9360>
Hi Mani,
On 18-09-18, 15:52, Manivannan Sadhasivam wrote:
> On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> > On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> >
> > > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > > OWL_DMA_MODE_DAM_INC;
> > >
> > > + break;
> > > + case DMA_MEM_TO_DEV:
> > > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > > +
> > > + /* Handle bus width for UART */
> > > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
> >
> > this is fine per se, but not correct way to handle in dmaengine driver.
> > You should be agnostic to user of dmaengine, so handle all the buswidths
> > the IP block supports and update the values accordingly. That way new
> > uses can be added w/o requiring change in dmaengine driver
>
> Currently, all members of Owl family supports only 32bit and 8bit
> bus widths. 32bit is common for all peripherals and 8bit applies to only
> UART since the internal buffer is 8bit wide. So, this makes sense to me!
Above you are onky handing DMA_SLAVE_BUSWIDTH_1_BYTE and not 32bit which
this IP supports.. You should handle all widths supported vt hardware..
--
~Vinod
^ permalink raw reply
* Re: [PATCH 2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Manivannan Sadhasivam @ 2018-09-18 23:34 UTC (permalink / raw)
To: Vinod
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
In-Reply-To: <20180918233200.GK2613@vkoul-mobl>
On Tue, Sep 18, 2018 at 04:32:00PM -0700, Vinod wrote:
> Hi Mani,
>
> On 18-09-18, 15:52, Manivannan Sadhasivam wrote:
> > On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> > > On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> > >
> > > > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > > > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > > > OWL_DMA_MODE_DAM_INC;
> > > >
> > > > + break;
> > > > + case DMA_MEM_TO_DEV:
> > > > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > > > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > > > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > > > +
> > > > + /* Handle bus width for UART */
> > > > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > > > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
> > >
> > > this is fine per se, but not correct way to handle in dmaengine driver.
> > > You should be agnostic to user of dmaengine, so handle all the buswidths
> > > the IP block supports and update the values accordingly. That way new
> > > uses can be added w/o requiring change in dmaengine driver
> >
> > Currently, all members of Owl family supports only 32bit and 8bit
> > bus widths. 32bit is common for all peripherals and 8bit applies to only
> > UART since the internal buffer is 8bit wide. So, this makes sense to me!
>
> Above you are onky handing DMA_SLAVE_BUSWIDTH_1_BYTE and not 32bit which
> this IP supports.. You should handle all widths supported vt hardware..
>
Hi Vinod,
Default width is 32bit and we will only override it for UART... Should I
add a comment stating this?
Thanks,
Mani
> --
> ~Vinod
^ permalink raw reply
* Re: [PATCH 2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Manivannan Sadhasivam @ 2018-09-18 23:56 UTC (permalink / raw)
To: Vinod
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
In-Reply-To: <20180918233414.GA19844@Mani-XPS-13-9360>
On Tue, Sep 18, 2018 at 04:34:14PM -0700, Manivannan Sadhasivam wrote:
> On Tue, Sep 18, 2018 at 04:32:00PM -0700, Vinod wrote:
> > Hi Mani,
> >
> > On 18-09-18, 15:52, Manivannan Sadhasivam wrote:
> > > On Tue, Sep 18, 2018 at 09:35:12AM -0700, Vinod wrote:
> > > > On 01-09-18, 22:12, Manivannan Sadhasivam wrote:
> > > >
> > > > > @@ -364,6 +372,26 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
> > > > > OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
> > > > > OWL_DMA_MODE_DAM_INC;
> > > > >
> > > > > + break;
> > > > > + case DMA_MEM_TO_DEV:
> > > > > + mode |= OWL_DMA_MODE_TS(vchan->drq)
> > > > > + | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
> > > > > + | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
> > > > > +
> > > > > + /* Handle bus width for UART */
> > > > > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
> > > > > + mode |= OWL_DMA_MODE_NDDBW_8BIT;
> > > >
> > > > this is fine per se, but not correct way to handle in dmaengine driver.
> > > > You should be agnostic to user of dmaengine, so handle all the buswidths
> > > > the IP block supports and update the values accordingly. That way new
> > > > uses can be added w/o requiring change in dmaengine driver
> > >
> > > Currently, all members of Owl family supports only 32bit and 8bit
> > > bus widths. 32bit is common for all peripherals and 8bit applies to only
> > > UART since the internal buffer is 8bit wide. So, this makes sense to me!
> >
> > Above you are onky handing DMA_SLAVE_BUSWIDTH_1_BYTE and not 32bit which
> > this IP supports.. You should handle all widths supported vt hardware..
> >
>
> Hi Vinod,
>
> Default width is 32bit and we will only override it for UART... Should I
> add a comment stating this?
>
I think it is better to select 32bit mode eventhough it is the default one.
Will update it in next revision.
Thanks,
Mani
> Thanks,
> Mani
>
> > --
> > ~Vinod
^ permalink raw reply
* [PATCH 0/3] serial: 8250_uniphier: remove unused code and add auto-flow-control
From: Masahiro Yamada @ 2018-09-19 5:28 UTC (permalink / raw)
To: Greg KH, linux-serial
Cc: Dai Okamura, devicetree, linux-kernel, Masahiro Yamada,
Rob Herring, Jiri Slaby, Mark Rutland, linux-arm-kernel
- Remove never-used DT property
- Refactor code
- Add auto-flow-control support
Dai Okamura (1):
serial: 8250_uniphier: add auto-flow-control support
Masahiro Yamada (2):
serial: 8250_uniphier: remove unused "fifo-size" property
serial: 8250_uniphier: flatten probe function
.../devicetree/bindings/serial/uniphier-uart.txt | 3 +-
drivers/tty/serial/8250/8250_uniphier.c | 62 ++++++++--------------
2 files changed, 22 insertions(+), 43 deletions(-)
--
2.7.4
^ 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