Re: [PATCH] soc: qcom: geni-se: Do not bother about enable/disable of interrupts in secondary sequencer for non-uart modes

[Vijaya Krishna Nivarthi]

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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > ---
> > > > >    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