| Message ID | 1685729609-26871-1-git-send-email-quic_vnivarth@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | soc: qcom: geni-se: Do not bother about enable/disable of interrupts in secondary sequencer for non-uart modes | expand |
Hi, On Fri, Jun 2, 2023 at 11:13 AM Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > > The select_fifo/dma_mode() functions in geni driver enable/disable > interrupts (secondary included) conditionally for non-uart modes, while > uart is supposed to manage this internally. > However, only uart uses secondary IRQs while spi, i2c do not care about > these at all making their enablement (or disablement) totally unnecessary > for these protos. > > Drop enabling/disabling secondary IRQs for non-uart modes. > This doesn't solve any observed problem but only gets rid of code pieces > that are not required. > > Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > --- > drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- > 1 file changed, 4 insertions(+), 20 deletions(-) This seems like a nice cleanup to me. It's still odd that the general code has a special case for UART, but I guess the alternative is to duplicate the exact same logic for both the i2c and SPI drivers. I won't insist on that, though I wouldn't be opposed to it either. I guess one comment, though: should we also remove the code that messes with "SE_GENI_S_IRQ_EN" in geni_se_select_gpi_mode()? -Doug
Hi, Thank you very much for the review... On 6/7/2023 9:41 PM, Doug Anderson wrote: > Hi, > > On Fri, Jun 2, 2023 at 11:13 AM Vijaya Krishna Nivarthi > <quic_vnivarth@quicinc.com> wrote: >> The select_fifo/dma_mode() functions in geni driver enable/disable >> interrupts (secondary included) conditionally for non-uart modes, while >> uart is supposed to manage this internally. >> However, only uart uses secondary IRQs while spi, i2c do not care about >> these at all making their enablement (or disablement) totally unnecessary >> for these protos. >> >> Drop enabling/disabling secondary IRQs for non-uart modes. >> This doesn't solve any observed problem but only gets rid of code pieces >> that are not required. >> >> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> >> --- >> drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- >> 1 file changed, 4 insertions(+), 20 deletions(-) > This seems like a nice cleanup to me. It's still odd that the general > code has a special case for UART, but I guess the alternative is to > duplicate the exact same logic for both the i2c and SPI drivers. I > won't insist on that, though I wouldn't be opposed to it either. > > I guess one comment, though: should we also remove the code that > messes with "SE_GENI_S_IRQ_EN" in geni_se_select_gpi_mode()? Right now we have gpi-dma mode support for i2c and spi but not for uart. Even when gpi-dma support is added, uart driver's interrupt handler would not be invoked so I believe it would be safe to drop that code from geni_se_select_gpi_mode() too. I will post v2 dropping same after some more lookup. -Vijay/ > > -Doug
Hi, On 6/12/2023 7:09 PM, Vijaya Krishna Nivarthi wrote: > Hi, > > Thank you very much for the review... > > > On 6/7/2023 9:41 PM, Doug Anderson wrote: >> Hi, >> >> On Fri, Jun 2, 2023 at 11:13 AM Vijaya Krishna Nivarthi >> <quic_vnivarth@quicinc.com> wrote: >>> The select_fifo/dma_mode() functions in geni driver enable/disable >>> interrupts (secondary included) conditionally for non-uart modes, while >>> uart is supposed to manage this internally. >>> However, only uart uses secondary IRQs while spi, i2c do not care about >>> these at all making their enablement (or disablement) totally >>> unnecessary >>> for these protos. >>> >>> Drop enabling/disabling secondary IRQs for non-uart modes. >>> This doesn't solve any observed problem but only gets rid of code >>> pieces >>> that are not required. >>> >>> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> >>> --- >>> drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- >>> 1 file changed, 4 insertions(+), 20 deletions(-) >> This seems like a nice cleanup to me. It's still odd that the general >> code has a special case for UART, but I guess the alternative is to >> duplicate the exact same logic for both the i2c and SPI drivers. I >> won't insist on that, though I wouldn't be opposed to it either. >> >> I guess one comment, though: should we also remove the code that >> messes with "SE_GENI_S_IRQ_EN" in geni_se_select_gpi_mode()? > > > Right now we have gpi-dma mode support for i2c and spi but not for uart. > > Even when gpi-dma support is added, uart driver's interrupt handler > would not be invoked so I believe it would be safe to drop that code > from geni_se_select_gpi_mode() too. > > I will post v2 dropping same after some more lookup. Looking at this once again, I am more inclined towards leaving alone gpi_mode() for now. It probably needs cleanup but we want to take that up at a later time. Meanwhile its not causing much harm as we don't switch modes dynamically for gpi case, so we are not losing much time there reading from and writing to registers. Please let know your thoughts. Thank you, Vijay/ > > -Vijay/ > > >> >> -Doug
Hi, On Tue, Jun 13, 2023 at 9:07 AM Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > > Hi, > > > On 6/12/2023 7:09 PM, Vijaya Krishna Nivarthi wrote: > > Hi, > > > > Thank you very much for the review... > > > > > > On 6/7/2023 9:41 PM, Doug Anderson wrote: > >> Hi, > >> > >> On Fri, Jun 2, 2023 at 11:13 AM Vijaya Krishna Nivarthi > >> <quic_vnivarth@quicinc.com> wrote: > >>> The select_fifo/dma_mode() functions in geni driver enable/disable > >>> interrupts (secondary included) conditionally for non-uart modes, while > >>> uart is supposed to manage this internally. > >>> However, only uart uses secondary IRQs while spi, i2c do not care about > >>> these at all making their enablement (or disablement) totally > >>> unnecessary > >>> for these protos. > >>> > >>> Drop enabling/disabling secondary IRQs for non-uart modes. > >>> This doesn't solve any observed problem but only gets rid of code > >>> pieces > >>> that are not required. > >>> > >>> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > >>> --- > >>> drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- > >>> 1 file changed, 4 insertions(+), 20 deletions(-) > >> This seems like a nice cleanup to me. It's still odd that the general > >> code has a special case for UART, but I guess the alternative is to > >> duplicate the exact same logic for both the i2c and SPI drivers. I > >> won't insist on that, though I wouldn't be opposed to it either. > >> > >> I guess one comment, though: should we also remove the code that > >> messes with "SE_GENI_S_IRQ_EN" in geni_se_select_gpi_mode()? > > > > > > Right now we have gpi-dma mode support for i2c and spi but not for uart. > > > > Even when gpi-dma support is added, uart driver's interrupt handler > > would not be invoked so I believe it would be safe to drop that code > > from geni_se_select_gpi_mode() too. > > > > I will post v2 dropping same after some more lookup. > > > Looking at this once again, I am more inclined towards leaving alone > gpi_mode() for now. > > It probably needs cleanup but we want to take that up at a later time. > > Meanwhile its not causing much harm as we don't switch modes dynamically > for gpi case, so we are not losing much time there reading from and > writing to registers. > > Please let know your thoughts. It's not really about the time needed for the extra register writes, but just someone trying to understand the code who might be trying to figure out what bits are relevant. The bits related to the secondary sequencer's interrupts don't do anything useful when we're using the controller for i2c/spi, so why not delete them? -Doug
Hi, On 6/13/2023 11:27 PM, Doug Anderson wrote: > Hi, > > On Tue, Jun 13, 2023 at 9:07 AM Vijaya Krishna Nivarthi > <quic_vnivarth@quicinc.com> wrote: >> Hi, >> >> >> On 6/12/2023 7:09 PM, Vijaya Krishna Nivarthi wrote: >>> Hi, >>> >>> Thank you very much for the review... >>> >>> >>> On 6/7/2023 9:41 PM, Doug Anderson wrote: >>>> Hi, >>>> >>>> On Fri, Jun 2, 2023 at 11:13 AM Vijaya Krishna Nivarthi >>>> <quic_vnivarth@quicinc.com> wrote: >>>>> The select_fifo/dma_mode() functions in geni driver enable/disable >>>>> interrupts (secondary included) conditionally for non-uart modes, while >>>>> uart is supposed to manage this internally. >>>>> However, only uart uses secondary IRQs while spi, i2c do not care about >>>>> these at all making their enablement (or disablement) totally >>>>> unnecessary >>>>> for these protos. >>>>> >>>>> Drop enabling/disabling secondary IRQs for non-uart modes. >>>>> This doesn't solve any observed problem but only gets rid of code >>>>> pieces >>>>> that are not required. >>>>> >>>>> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> >>>>> --- >>>>> drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- >>>>> 1 file changed, 4 insertions(+), 20 deletions(-) >>>> This seems like a nice cleanup to me. It's still odd that the general >>>> code has a special case for UART, but I guess the alternative is to >>>> duplicate the exact same logic for both the i2c and SPI drivers. I >>>> won't insist on that, though I wouldn't be opposed to it either. >>>> >>>> I guess one comment, though: should we also remove the code that >>>> messes with "SE_GENI_S_IRQ_EN" in geni_se_select_gpi_mode()? >>> >>> Right now we have gpi-dma mode support for i2c and spi but not for uart. >>> >>> Even when gpi-dma support is added, uart driver's interrupt handler >>> would not be invoked so I believe it would be safe to drop that code >>> from geni_se_select_gpi_mode() too. >>> >>> I will post v2 dropping same after some more lookup. >> >> Looking at this once again, I am more inclined towards leaving alone >> gpi_mode() for now. >> >> It probably needs cleanup but we want to take that up at a later time. >> >> Meanwhile its not causing much harm as we don't switch modes dynamically >> for gpi case, so we are not losing much time there reading from and >> writing to registers. >> >> Please let know your thoughts. > It's not really about the time needed for the extra register writes, > but just someone trying to understand the code who might be trying to > figure out what bits are relevant. The bits related to the secondary > sequencer's interrupts don't do anything useful when we're using the > controller for i2c/spi, so why not delete them? Agreed the s_irqs are not useful for spi/i2c but Right now I am not really sure how uart + gsi mode is going to look like. So how about we move the part that messes with s_irq in gpi_mode() into a *if(proto == GENI_SE_UART)* conditional? Understand we are adding to weirdness but Would that be ok for now? thank you, Vijay/ > > -Doug
Hi, On Tue, Jun 13, 2023 at 11:24 AM Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > > Hi, > > > On 6/13/2023 11:27 PM, Doug Anderson wrote: > > Hi, > > > > On Tue, Jun 13, 2023 at 9:07 AM Vijaya Krishna Nivarthi > > <quic_vnivarth@quicinc.com> wrote: > >> Hi, > >> > >> > >> On 6/12/2023 7:09 PM, Vijaya Krishna Nivarthi wrote: > >>> Hi, > >>> > >>> Thank you very much for the review... > >>> > >>> > >>> On 6/7/2023 9:41 PM, Doug Anderson wrote: > >>>> Hi, > >>>> > >>>> On Fri, Jun 2, 2023 at 11:13 AM Vijaya Krishna Nivarthi > >>>> <quic_vnivarth@quicinc.com> wrote: > >>>>> The select_fifo/dma_mode() functions in geni driver enable/disable > >>>>> interrupts (secondary included) conditionally for non-uart modes, while > >>>>> uart is supposed to manage this internally. > >>>>> However, only uart uses secondary IRQs while spi, i2c do not care about > >>>>> these at all making their enablement (or disablement) totally > >>>>> unnecessary > >>>>> for these protos. > >>>>> > >>>>> Drop enabling/disabling secondary IRQs for non-uart modes. > >>>>> This doesn't solve any observed problem but only gets rid of code > >>>>> pieces > >>>>> that are not required. > >>>>> > >>>>> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > >>>>> --- > >>>>> drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- > >>>>> 1 file changed, 4 insertions(+), 20 deletions(-) > >>>> This seems like a nice cleanup to me. It's still odd that the general > >>>> code has a special case for UART, but I guess the alternative is to > >>>> duplicate the exact same logic for both the i2c and SPI drivers. I > >>>> won't insist on that, though I wouldn't be opposed to it either. > >>>> > >>>> I guess one comment, though: should we also remove the code that > >>>> messes with "SE_GENI_S_IRQ_EN" in geni_se_select_gpi_mode()? > >>> > >>> Right now we have gpi-dma mode support for i2c and spi but not for uart. > >>> > >>> Even when gpi-dma support is added, uart driver's interrupt handler > >>> would not be invoked so I believe it would be safe to drop that code > >>> from geni_se_select_gpi_mode() too. > >>> > >>> I will post v2 dropping same after some more lookup. > >> > >> Looking at this once again, I am more inclined towards leaving alone > >> gpi_mode() for now. > >> > >> It probably needs cleanup but we want to take that up at a later time. > >> > >> Meanwhile its not causing much harm as we don't switch modes dynamically > >> for gpi case, so we are not losing much time there reading from and > >> writing to registers. > >> > >> Please let know your thoughts. > > It's not really about the time needed for the extra register writes, > > but just someone trying to understand the code who might be trying to > > figure out what bits are relevant. The bits related to the secondary > > sequencer's interrupts don't do anything useful when we're using the > > controller for i2c/spi, so why not delete them? > > > Agreed the s_irqs are not useful for spi/i2c but Right now I am not > really sure how uart + gsi mode is going to look like. > > So how about we move the part that messes with s_irq in gpi_mode() into > a *if(proto == GENI_SE_UART)* conditional? > > Understand we are adding to weirdness but Would that be ok for now? For the non-GPI DMA path and for the PIO path we don't touch the "S_IRQ" for UART either (we don't touch any IRQs for the UART). ...so it doesn't seem right to be tweaking with the S_IRQ for UART. I would say delete it and if/when the UART needs GPI mode then we can figure out what to do. I'd actually wonder if GPI will ever be implemented for UART anyway. The whole idea of GPI is to allow sharing a port between more than one user. Each user could submit a "transaction" and they'd get the port for the duration of that transaction. After the transaction was done then another user would be able to grab the port. Typically UART isn't used like this. I'm not saying that it couldn't be done, but just saying that it would be a pretty non-standard way of using a UART... -Doug
Hi, On 6/14/2023 12:44 AM, Doug Anderson wrote: > Hi, > > On Tue, Jun 13, 2023 at 11:24 AM Vijaya Krishna Nivarthi > <quic_vnivarth@quicinc.com> wrote: >> Hi, >> >> >> On 6/13/2023 11:27 PM, Doug Anderson wrote: >>> Hi, >>> >>> On Tue, Jun 13, 2023 at 9:07 AM Vijaya Krishna Nivarthi >>> <quic_vnivarth@quicinc.com> wrote: >>>> Hi, >>>> >>>> >>>> On 6/12/2023 7:09 PM, Vijaya Krishna Nivarthi wrote: >>>>> Hi, >>>>> >>>>> Thank you very much for the review... >>>>> >>>>> >>>>> On 6/7/2023 9:41 PM, Doug Anderson wrote: >>>>>> Hi, >>>>>> >>>>>> On Fri, Jun 2, 2023 at 11:13 AM Vijaya Krishna Nivarthi >>>>>> <quic_vnivarth@quicinc.com> wrote: >>>>>>> The select_fifo/dma_mode() functions in geni driver enable/disable >>>>>>> interrupts (secondary included) conditionally for non-uart modes, while >>>>>>> uart is supposed to manage this internally. >>>>>>> However, only uart uses secondary IRQs while spi, i2c do not care about >>>>>>> these at all making their enablement (or disablement) totally >>>>>>> unnecessary >>>>>>> for these protos. >>>>>>> >>>>>>> Drop enabling/disabling secondary IRQs for non-uart modes. >>>>>>> This doesn't solve any observed problem but only gets rid of code >>>>>>> pieces >>>>>>> that are not required. >>>>>>> >>>>>>> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> >>>>>>> --- >>>>>>> drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- >>>>>>> 1 file changed, 4 insertions(+), 20 deletions(-) >>>>>> This seems like a nice cleanup to me. It's still odd that the general >>>>>> code has a special case for UART, but I guess the alternative is to >>>>>> duplicate the exact same logic for both the i2c and SPI drivers. I >>>>>> won't insist on that, though I wouldn't be opposed to it either. >>>>>> >>>>>> I guess one comment, though: should we also remove the code that >>>>>> messes with "SE_GENI_S_IRQ_EN" in geni_se_select_gpi_mode()? >>>>> Right now we have gpi-dma mode support for i2c and spi but not for uart. >>>>> >>>>> Even when gpi-dma support is added, uart driver's interrupt handler >>>>> would not be invoked so I believe it would be safe to drop that code >>>>> from geni_se_select_gpi_mode() too. >>>>> >>>>> I will post v2 dropping same after some more lookup. >>>> Looking at this once again, I am more inclined towards leaving alone >>>> gpi_mode() for now. >>>> >>>> It probably needs cleanup but we want to take that up at a later time. >>>> >>>> Meanwhile its not causing much harm as we don't switch modes dynamically >>>> for gpi case, so we are not losing much time there reading from and >>>> writing to registers. >>>> >>>> Please let know your thoughts. >>> It's not really about the time needed for the extra register writes, >>> but just someone trying to understand the code who might be trying to >>> figure out what bits are relevant. The bits related to the secondary >>> sequencer's interrupts don't do anything useful when we're using the >>> controller for i2c/spi, so why not delete them? >> >> Agreed the s_irqs are not useful for spi/i2c but Right now I am not >> really sure how uart + gsi mode is going to look like. >> >> So how about we move the part that messes with s_irq in gpi_mode() into >> a *if(proto == GENI_SE_UART)* conditional? >> >> Understand we are adding to weirdness but Would that be ok for now? > For the non-GPI DMA path and for the PIO path we don't touch the > "S_IRQ" for UART either (we don't touch any IRQs for the UART). ...so > it doesn't seem right to be tweaking with the S_IRQ for UART. I would > say delete it and if/when the UART needs GPI mode then we can figure > out what to do. > > I'd actually wonder if GPI will ever be implemented for UART anyway. > The whole idea of GPI is to allow sharing a port between more than one > user. Each user could submit a "transaction" and they'd get the port > for the duration of that transaction. After the transaction was done > then another user would be able to grab the port. Typically UART isn't > used like this. I'm not saying that it couldn't be done, but just > saying that it would be a pretty non-standard way of using a UART... Agreed on both. Uploading v2 dropping from gpi_mode() Thank you. -Vijay/ > > -Doug
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c index 795a2e1..7111661 100644 --- a/drivers/soc/qcom/qcom-geni-se.c +++ b/drivers/soc/qcom/qcom-geni-se.c @@ -281,27 +281,14 @@ static void geni_se_select_fifo_mode(struct geni_se *se) geni_se_irq_clear(se); - /* - * The RX path for the UART is asynchronous and so needs more - * complex logic for enabling / disabling its interrupts. - * - * Specific notes: - * - The done and TX-related interrupts are managed manually. - * - We don't RX from the main sequencer (we use the secondary) so - * we don't need the RX-related interrupts enabled in the main - * sequencer for UART. - */ + /* UART driver manages enabling / disabling interrupts internally */ if (proto != GENI_SE_UART) { + /* Non-UART use only primary sequencer so dont bother about S_IRQ */ val_old = val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN); val |= M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN; val |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN; if (val != val_old) writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN); - - val_old = val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN); - val |= S_CMD_DONE_EN; - if (val != val_old) - writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN); } val_old = val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN); @@ -317,17 +304,14 @@ static void geni_se_select_dma_mode(struct geni_se *se) geni_se_irq_clear(se); + /* UART driver manages enabling / disabling interrupts internally */ if (proto != GENI_SE_UART) { + /* Non-UART use only primary sequencer so dont bother about S_IRQ */ val_old = val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN); val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN); val &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); if (val != val_old) writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN); - - val_old = val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN); - val &= ~S_CMD_DONE_EN; - if (val != val_old) - writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN); } val_old = val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
The select_fifo/dma_mode() functions in geni driver enable/disable interrupts (secondary included) conditionally for non-uart modes, while uart is supposed to manage this internally. However, only uart uses secondary IRQs while spi, i2c do not care about these at all making their enablement (or disablement) totally unnecessary for these protos. Drop enabling/disabling secondary IRQs for non-uart modes. This doesn't solve any observed problem but only gets rid of code pieces that are not required. Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> --- drivers/soc/qcom/qcom-geni-se.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-)