by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCH v2 1/7] soc: qcom: geni-se: Add GP_LENGTH/IRQ_EN_SET/IRQ_EN_CLEAR registers

On Thu, May 30, 2024 at 03:45:53PM GMT, Douglas Anderson wrote:
> For UART devices the M_GP_LENGTH is the TX word count. For other
> devices this is the transaction word count.
>
> For UART devices the S_GP_LENGTH is the RX word count.
>
> The IRQ_EN set/clear registers allow you to set or clear bits in the
> IRQ_EN register without needing a read-modify-write.
>

Acked-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> Since these new definitions are used in the future UART patches the
> hope is that they could be acked by Qualcomm folks and then go through
> the same tree as the UART patches that need them.
>
> Changes in v2:
> - New
>
> include/linux/soc/qcom/geni-se.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index 0f038a1a0330..8d07c442029b 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -88,11 +88,15 @@ struct geni_se {
> #define SE_GENI_M_IRQ_STATUS 0x610
> #define SE_GENI_M_IRQ_EN 0x614
> #define SE_GENI_M_IRQ_CLEAR 0x618
> +#define SE_GENI_M_IRQ_EN_SET 0x61c
> +#define SE_GENI_M_IRQ_EN_CLEAR 0x620
> #define SE_GENI_S_CMD0 0x630
> #define SE_GENI_S_CMD_CTRL_REG 0x634
> #define SE_GENI_S_IRQ_STATUS 0x640
> #define SE_GENI_S_IRQ_EN 0x644
> #define SE_GENI_S_IRQ_CLEAR 0x648
> +#define SE_GENI_S_IRQ_EN_SET 0x64c
> +#define SE_GENI_S_IRQ_EN_CLEAR 0x650
> #define SE_GENI_TX_FIFOn 0x700
> #define SE_GENI_RX_FIFOn 0x780
> #define SE_GENI_TX_FIFO_STATUS 0x800
> @@ -101,6 +105,8 @@ struct geni_se {
> #define SE_GENI_RX_WATERMARK_REG 0x810
> #define SE_GENI_RX_RFR_WATERMARK_REG 0x814
> #define SE_GENI_IOS 0x908
> +#define SE_GENI_M_GP_LENGTH 0x910
> +#define SE_GENI_S_GP_LENGTH 0x914
> #define SE_DMA_TX_IRQ_STAT 0xc40
> #define SE_DMA_TX_IRQ_CLR 0xc44
> #define SE_DMA_TX_FSM_RST 0xc58
> --
> 2.45.1.288.g0e0cd299f1-goog
>

2024-06-04 16:06:24

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] serial: qcom-geni: Fix suspend while active UART xfer

Hi,

On Fri, May 31, 2024 at 8:13 AM Ilpo Järvinen
<[email protected]> wrote:
>
> On Thu, 30 May 2024, Douglas Anderson wrote:
>
> > On devices using Qualcomm's GENI UART it is possible to get the UART
> > stuck such that it no longer outputs data. Specifically, I could
> > reproduce this problem by logging in via an agetty on the debug serial
> > port (which was _not_ used for kernel console) and running:
> > cat /var/log/messages
> > ...and then (via an SSH session) forcing a few suspend/resume cycles.
> >
> > Digging into this showed a number of problems that are all related.
> >
> > The root of the problems was with qcom_geni_serial_stop_tx_fifo()
> > which is called as part of the suspend process. Specific problems with
> > that function:
> > - When we cancel an in-progress "tx" command it doesn't appear to
> > fully drain the FIFO. That meant qcom_geni_serial_tx_empty()
> > continued to report that the FIFO wasn't empty. The
> > qcom_geni_serial_start_tx_fifo() function didn't re-enable
> > interrupts in this case so we'd never start transferring again.
> > - We cancelled the current "tx" command but we forgot to zero out
> > "tx_remaining". This confused logic elsewhere in the driver
> > - From experimentation, it appears that cancelling the "tx" command
> > could drop some of the queued up bytes. While maybe not the end of
> > the world, it doesn't seem like we should be dropping bytes when
> > stopping the FIFO, which is defined more of a "pause".
> >
> > One idea to fix the above would be to add FIFO draining to
> > qcom_geni_serial_stop_tx_fifo(). However, digging into the
> > documentation in serial_core.h for stop_tx() makes this seem like the
> > wrong choice. Specifically stop_tx() is called with local interrupts
> > disabled. Waiting for a FIFO (which might be 64 bytes big) to drain at
> > 115.2 kbps doesn't seem like a wise move.
> >
> > Ideally qcom_geni_serial_stop_tx_fifo() would be able to pause the
> > transmitter, but nothing in the documentation for the GENI UART makes
> > me believe that is possible.
> >
> > Given the lack of better choices, we'll change
> > qcom_geni_serial_stop_tx_fifo() to simply disable the
> > TX_FIFO_WATERMARK interrupt and call it a day. This seems OK as per
> > the serial core docs since stop_tx() is supposed to stop transferring
> > bytes "as soon as possible" and there doesn't seem to be any possible
> > way to stop transferring sooner. As part of this, get rid of some of
> > the extra conditions on qcom_geni_serial_start_tx_fifo() which simply
> > weren't needed and are now getting in the way. It's always fine to
> > turn the interrupts on if we want to receive and it'll be up to the
> > IRQ handler to turn them back off if somehow they're not needed. This
> > works fine.
> >
> > Unfortunately, doing just the above change causes new/different
> > problems with suspend/resume. Now if you suspend while an active
> > transfer is happening you can find that after resume time you're no
> > longer receiving UART interrupts at all. It appears to be important to
> > drain the FIFO and send a "cancel" command if the UART is active to
> > avoid this. Since we've already decided that
> > qcom_geni_serial_stop_tx_fifo() shouldn't be doing this, let's add the
> > draining / cancelling logic to the shutdown() call where it should be
> > OK to delay a bit. This is called as part of the suspend process via
> > uart_suspend_port().
> >
> > Finally, with all of the above, the test case where we're spamming the
> > UART with data and going through suspend/resume cycles doesn't kill
> > the UART and doesn't drop bytes.
> >
> > NOTE: though I haven't gone back and validated on ancient code, it
> > appears from code inspection that many of these problems have existed
> > since the start of the driver. In the very least, I could reproduce
> > the problems on vanilla v5.15. The problems don't seem to reproduce
> > when using the serial port for kernel console output and also don't
> > seem to reproduce if nothing is being printed to the console at
> > suspend time, so this is presumably why they were not noticed until
> > now.
>
> Hi,
>
> This was quite tiring to read. :-) It's has lots of useful information but
> it could be structured better.
>
> Could you try to rewrite this entire description so that it's easier to
> find the problem and final solution information from it. Start with those
> two things, and in that part, try to avoid detouring to extra branches you
> took while finding and solving the problem.
>
> You can place how the problem can be reproduced after you've described the
> root cause & final solution first. Extra information why some other
> approaches do not work is also useful information, but please place it
> after the final solution has been covered first.
>
> Also, try to avoid I/you/we, use imperative tone.

Sure. I'll try. It's always a tradeoff between providing too much
information and not providing enough. In general I find that providing
the thought process can help someone else who is likely going to go
through the same thing as they're trying to understand the patch, but
I agree it can also be overwhelming.

Sure. I've attempted to use the imperative tone when possible. In
general (unless my understanding is flawed) it's not possible to use
imperative when explaining to the reader how the hardware/driver works
or what the problem is and (IMO) we shouldn't fully remove these types
of explanations from the commit message. When describing what the
patch actually does, though, I've tried to make sure it's in
imperative form. If you have wording changes on v3 then please suggest
specific changes.

-Doug