| Message ID | 1691583100-15689-1-git-send-email-quic_vnivarth@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [RESEND] tty: serial: qcom-geni-serial: Poll primary sequencer irq status after cancel_tx | expand |
On Wed, 9 Aug 2023 17:41:40 +0530 Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > TX is handled by primary sequencer. After cancelling primary command, poll > primary sequencer's irq status instead of that of secondary. Hi, it is not clear to me if this is a bug fix or an improvement? > While at it, also remove a couple of redundant lines that read from IRQ_EN > register and write back same. This should go into a separate patch. Hugo Villeneuve. > Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA") > Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > --- > drivers/tty/serial/qcom_geni_serial.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 3ca5db2..b8aa4c1 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -591,7 +591,6 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport) > { > struct qcom_geni_serial_port *port = to_dev_port(uport); > bool done; > - u32 m_irq_en; > > if (!qcom_geni_serial_main_active(uport)) > return; > @@ -603,12 +602,10 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport) > port->tx_remaining = 0; > } > > - m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN); > - writel(m_irq_en, uport->membase + SE_GENI_M_IRQ_EN); > geni_se_cancel_m_cmd(&port->se); > > - done = qcom_geni_serial_poll_bit(uport, SE_GENI_S_IRQ_STATUS, > - S_CMD_CANCEL_EN, true); > + done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, > + M_CMD_CANCEL_EN, true); > if (!done) { > geni_se_abort_m_cmd(&port->se); > done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, > -- > 2.7.4 >
Hi, Thank you very much for the review... On 8/9/2023 6:49 PM, Hugo Villeneuve wrote: > On Wed, 9 Aug 2023 17:41:40 +0530 > Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > >> TX is handled by primary sequencer. After cancelling primary command, poll >> primary sequencer's irq status instead of that of secondary. > Hi, > it is not clear to me if this is a bug fix or an improvement? This is a bug fix. > >> While at it, also remove a couple of redundant lines that read from IRQ_EN >> register and write back same. > This should go into a separate patch. The changes were too close by so I wasn't sure it could be split into 2 patches. I see that the earlier patch has already been signed off by Greg. (I did a RESEND after realising that I had Bjorn Andersson's email address incorrect) Will post another version if original patch doesn't get merged for any reason. Thank you, Vijay/ > > Hugo Villeneuve. > > >> Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA") >> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> >> --- >> drivers/tty/serial/qcom_geni_serial.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >> index 3ca5db2..b8aa4c1 100644 >> --- a/drivers/tty/serial/qcom_geni_serial.c >> +++ b/drivers/tty/serial/qcom_geni_serial.c >> @@ -591,7 +591,6 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport) >> { >> struct qcom_geni_serial_port *port = to_dev_port(uport); >> bool done; >> - u32 m_irq_en; >> >> if (!qcom_geni_serial_main_active(uport)) >> return; >> @@ -603,12 +602,10 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport) >> port->tx_remaining = 0; >> } >> >> - m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN); >> - writel(m_irq_en, uport->membase + SE_GENI_M_IRQ_EN); >> geni_se_cancel_m_cmd(&port->se); >> >> - done = qcom_geni_serial_poll_bit(uport, SE_GENI_S_IRQ_STATUS, >> - S_CMD_CANCEL_EN, true); >> + done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, >> + M_CMD_CANCEL_EN, true); >> if (!done) { >> geni_se_abort_m_cmd(&port->se); >> done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, >> -- >> 2.7.4 >>
On 09/08/2023 13:11, Vijaya Krishna Nivarthi wrote: > While at it, also remove a couple of redundant lines that read from IRQ_EN > register and write back same. > > Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA") > Signed-off-by: Vijaya Krishna Nivarthi<quic_vnivarth@quicinc.com> The "while at it" should be put into a separate patch. I wonder if the read/write cycle is there to ensure an io-completion ? Either way please break this up into two individual patches. One thing changing where you poll and the other removing the read/write cycle, so the changes may be evaluated individually. --- bod
On Wed, Aug 09, 2023 at 10:51:54PM +0530, Vijaya Krishna Nivarthi wrote: > Hi, > > Thank you very much for the review... > Thank you for the bug fix, Vijaya. > > On 8/9/2023 6:49 PM, Hugo Villeneuve wrote: > > On Wed, 9 Aug 2023 17:41:40 +0530 > > Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > > > > > TX is handled by primary sequencer. After cancelling primary command, poll > > > primary sequencer's irq status instead of that of secondary. > > Hi, > > it is not clear to me if this is a bug fix or an improvement? > This is a bug fix. Please describe the actual problem you're solving, to allow others working in and around this driver to know what issue(s) are corrected. This will save others debugging time, and it will teach others to help you maintain this driver. The section in the documentation on how to describe your changes is good, please read it: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > > > > > While at it, also remove a couple of redundant lines that read from IRQ_EN > > > register and write back same. > > This should go into a separate patch. > > The changes were too close by so I wasn't sure it could be split into 2 > patches. > > I see that the earlier patch has already been signed off by Greg. (I did a > RESEND after realising that I had Bjorn Andersson's email address incorrect) Please use ./scripts/get_maintainer.pl on the upstream tree, as this uses up to date information about recipients. > > Will post another version if original patch doesn't get merged for any > reason. > Please double check linux-next [1], if it's unclear if Greg picked up your previous patch (he's usually quite explicit about it...). I really would like some more details on the bug fix... [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ Regards, Bjorn
Thank you very much, Hugo Villeneuve and Bjorn, for the review and time. If this one doesn't get merged, I will post another patch series incorporating suggestions (including splitting) along with few more changes. -Vijay/ On 8/10/2023 8:06 PM, Bjorn Andersson wrote: > On Wed, Aug 09, 2023 at 10:51:54PM +0530, Vijaya Krishna Nivarthi wrote: >> Hi, >> >> Thank you very much for the review... >> > Thank you for the bug fix, Vijaya. > >> On 8/9/2023 6:49 PM, Hugo Villeneuve wrote: >>> On Wed, 9 Aug 2023 17:41:40 +0530 >>> Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: >>> >>>> TX is handled by primary sequencer. After cancelling primary command, poll >>>> primary sequencer's irq status instead of that of secondary. >>> Hi, >>> it is not clear to me if this is a bug fix or an improvement? >> This is a bug fix. > Please describe the actual problem you're solving, to allow others > working in and around this driver to know what issue(s) are corrected. > > This will save others debugging time, and it will teach others to help > you maintain this driver. > > The section in the documentation on how to describe your changes is > good, please read it: > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > >>>> While at it, also remove a couple of redundant lines that read from IRQ_EN >>>> register and write back same. >>> This should go into a separate patch. >> The changes were too close by so I wasn't sure it could be split into 2 >> patches. >> >> I see that the earlier patch has already been signed off by Greg. (I did a >> RESEND after realising that I had Bjorn Andersson's email address incorrect) > Please use ./scripts/get_maintainer.pl on the upstream tree, as this > uses up to date information about recipients. > >> Will post another version if original patch doesn't get merged for any >> reason. >> > Please double check linux-next [1], if it's unclear if Greg picked up > your previous patch (he's usually quite explicit about it...). I really > would like some more details on the bug fix... > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ > > Regards, > Bjorn
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 3ca5db2..b8aa4c1 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -591,7 +591,6 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport) { struct qcom_geni_serial_port *port = to_dev_port(uport); bool done; - u32 m_irq_en; if (!qcom_geni_serial_main_active(uport)) return; @@ -603,12 +602,10 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport) port->tx_remaining = 0; } - m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN); - writel(m_irq_en, uport->membase + SE_GENI_M_IRQ_EN); geni_se_cancel_m_cmd(&port->se); - done = qcom_geni_serial_poll_bit(uport, SE_GENI_S_IRQ_STATUS, - S_CMD_CANCEL_EN, true); + done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, + M_CMD_CANCEL_EN, true); if (!done) { geni_se_abort_m_cmd(&port->se); done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
TX is handled by primary sequencer. After cancelling primary command, poll primary sequencer's irq status instead of that of secondary. While at it, also remove a couple of redundant lines that read from IRQ_EN register and write back same. Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA") Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> --- drivers/tty/serial/qcom_geni_serial.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)