* [PATCH V2 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
@ 2019-10-10 9:46 Akash Asthana
2019-10-10 14:19 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Akash Asthana @ 2019-10-10 9:46 UTC (permalink / raw)
To: gregkh
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson, swboyd,
Akash Asthana
Move ISR registration from startup to probe function to avoid registering
it everytime when the port open is called for driver.
Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
drivers/tty/serial/qcom_geni_serial.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 14c6306..5180cd8 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -9,6 +9,7 @@
#include <linux/console.h>
#include <linux/io.h>
#include <linux/iopoll.h>
+#include <linux/irq.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
@@ -830,7 +831,7 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
if (uart_console(uport))
console_stop(uport->cons);
- free_irq(uport->irq, uport);
+ disable_irq(uport->irq);
spin_lock_irqsave(&uport->lock, flags);
qcom_geni_serial_stop_tx(uport);
qcom_geni_serial_stop_rx(uport);
@@ -890,21 +891,14 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
int ret;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
- scnprintf(port->name, sizeof(port->name),
- "qcom_serial_%s%d",
- (uart_console(uport) ? "console" : "uart"), uport->line);
-
if (!port->setup) {
ret = qcom_geni_serial_port_setup(uport);
if (ret)
return ret;
}
+ enable_irq(uport->irq);
- ret = request_irq(uport->irq, qcom_geni_serial_isr, IRQF_TRIGGER_HIGH,
- port->name, uport);
- if (ret)
- dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
- return ret;
+ return 0;
}
static unsigned long get_clk_cfg(unsigned long clk_freq)
@@ -1297,11 +1291,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
+ scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
+ (uart_console(uport) ? "console" : "uart"), uport->line);
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;
uport->irq = irq;
+ irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
+ ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
+ IRQF_TRIGGER_HIGH, port->name, uport);
+ if (ret) {
+ dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
+ return ret;
+ }
+
uport->private_data = drv;
platform_set_drvdata(pdev, port);
port->handle_rx = console ? handle_rx_console : handle_rx_uart;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
2019-10-10 9:46 [PATCH V2 1/2] tty: serial: qcom_geni_serial: IRQ cleanup Akash Asthana
@ 2019-10-10 14:19 ` Stephen Boyd
2019-10-11 7:39 ` Akash Asthana
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2019-10-10 14:19 UTC (permalink / raw)
To: Akash Asthana, gregkh
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson, Akash Asthana
Quoting Akash Asthana (2019-10-10 02:46:03)
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 14c6306..5180cd8 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1297,11 +1291,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>
> + scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
> + (uart_console(uport) ? "console" : "uart"), uport->line);
Drop useless parenthesis. Also, it might make more sense to make this a
devm_kasprintf() call now.
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> return irq;
> uport->irq = irq;
>
> + irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
Is there a reason why we can't always leave the irq enabled and request
it later once the uart structure has been fully initialized?
> + ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
> + IRQF_TRIGGER_HIGH, port->name, uport);
> + if (ret) {
> + dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
> + return ret;
> + }
> +
> uport->private_data = drv;
> platform_set_drvdata(pdev, port);
> port->handle_rx = console ? handle_rx_console : handle_rx_uart;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
2019-10-10 14:19 ` Stephen Boyd
@ 2019-10-11 7:39 ` Akash Asthana
2019-10-15 20:11 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Akash Asthana @ 2019-10-11 7:39 UTC (permalink / raw)
To: Stephen Boyd, gregkh
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson
On 10/10/2019 7:49 PM, Stephen Boyd wrote:
> Quoting Akash Asthana (2019-10-10 02:46:03)
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 14c6306..5180cd8 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -1297,11 +1291,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>> port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>> port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>>
>> + scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
>> + (uart_console(uport) ? "console" : "uart"), uport->line);
> Drop useless parenthesis. Also, it might make more sense to make this a
> devm_kasprintf() call now.
OK
>> irq = platform_get_irq(pdev, 0);
>> if (irq < 0)
>> return irq;
>> uport->irq = irq;
>>
>> + irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
> Is there a reason why we can't always leave the irq enabled and request
> it later once the uart structure has been fully initialized?
According to current design we are requesting IRQ handler in probe, and
we enable and disable it from the startup(port open) and shutdown(port
close) function respectively.
We need to call for disable_irq in shutdown function because client has
closed the port and we don't expect any transfer requests after it.
>>request it later once the uart structure has been fully initialized?
Is the ask is to move request irq later in probe after the uport is
fully initialized?
>> + ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
>> + IRQF_TRIGGER_HIGH, port->name, uport);
>> + if (ret) {
>> + dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
>> + return ret;
>> + }
>> +
>> uport->private_data = drv;
>> platform_set_drvdata(pdev, port);
>> port->handle_rx = console ? handle_rx_console : handle_rx_uart;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
2019-10-11 7:39 ` Akash Asthana
@ 2019-10-15 20:11 ` Stephen Boyd
2019-10-17 11:10 ` Akash Asthana
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2019-10-15 20:11 UTC (permalink / raw)
To: Akash Asthana, gregkh
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson
Quoting Akash Asthana (2019-10-11 00:39:06)
>
> On 10/10/2019 7:49 PM, Stephen Boyd wrote:
> > Quoting Akash Asthana (2019-10-10 02:46:03)
> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> >> index 14c6306..5180cd8 100644
> >> --- a/drivers/tty/serial/qcom_geni_serial.c
> >> +++ b/drivers/tty/serial/qcom_geni_serial.c
> >> @@ -1297,11 +1291,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> >> port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> >> port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> >>
> >> + scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
> >> + (uart_console(uport) ? "console" : "uart"), uport->line);
> > Drop useless parenthesis. Also, it might make more sense to make this a
> > devm_kasprintf() call now.
>
> OK
>
> >> irq = platform_get_irq(pdev, 0);
> >> if (irq < 0)
> >> return irq;
> >> uport->irq = irq;
> >>
> >> + irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
> > Is there a reason why we can't always leave the irq enabled and request
> > it later once the uart structure has been fully initialized?
>
> According to current design we are requesting IRQ handler in probe, and
> we enable and disable it from the startup(port open) and shutdown(port
> close) function respectively.
>
> We need to call for disable_irq in shutdown function because client has
> closed the port and we don't expect any transfer requests after it.
>
> >>request it later once the uart structure has been fully initialized?
>
> Is the ask is to move request irq later in probe after the uport is
> fully initialized?
Yes I'm wondering if we can get rid of the IRQ_NOAUTOEN and
irq_enable/disable bits in here and leave the interrupt enabled all the
time.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
2019-10-15 20:11 ` Stephen Boyd
@ 2019-10-17 11:10 ` Akash Asthana
0 siblings, 0 replies; 5+ messages in thread
From: Akash Asthana @ 2019-10-17 11:10 UTC (permalink / raw)
To: Stephen Boyd, gregkh
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson
On 10/16/2019 1:41 AM, Stephen Boyd wrote:
> Quoting Akash Asthana (2019-10-11 00:39:06)
>> On 10/10/2019 7:49 PM, Stephen Boyd wrote:
>>> Quoting Akash Asthana (2019-10-10 02:46:03)
>>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>>> index 14c6306..5180cd8 100644
>>>> --- a/drivers/tty/serial/qcom_geni_serial.c
>>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>>> @@ -1297,11 +1291,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>>> port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>>>> port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>>>>
>>>> + scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
>>>> + (uart_console(uport) ? "console" : "uart"), uport->line);
>>> Drop useless parenthesis. Also, it might make more sense to make this a
>>> devm_kasprintf() call now.
>> OK
>>
>>>> irq = platform_get_irq(pdev, 0);
>>>> if (irq < 0)
>>>> return irq;
>>>> uport->irq = irq;
>>>>
>>>> + irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
>>> Is there a reason why we can't always leave the irq enabled and request
>>> it later once the uart structure has been fully initialized?
>> According to current design we are requesting IRQ handler in probe, and
>> we enable and disable it from the startup(port open) and shutdown(port
>> close) function respectively.
>>
>> We need to call for disable_irq in shutdown function because client has
>> closed the port and we don't expect any transfer requests after it.
>>
>>>> request it later once the uart structure has been fully initialized?
>> Is the ask is to move request irq later in probe after the uport is
>> fully initialized?
> Yes I'm wondering if we can get rid of the IRQ_NOAUTOEN and
> irq_enable/disable bits in here and leave the interrupt enabled all the
> time.
Ideally it should work, I will run few experiments to make sure there
isn't any spurious interrupts problem after port close is called.
If it works, I will post a separate patch for it.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-17 11:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 9:46 [PATCH V2 1/2] tty: serial: qcom_geni_serial: IRQ cleanup Akash Asthana
2019-10-10 14:19 ` Stephen Boyd
2019-10-11 7:39 ` Akash Asthana
2019-10-15 20:11 ` Stephen Boyd
2019-10-17 11:10 ` Akash Asthana
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).