Message-ID: <CAD=FV=XMrurp4TLD7BCpVR9-+daHidHhkHG6P7u69HOBcvkMxA@mail.gmail.com>
Date:   Thu, 14 Oct 2021 09:55:24 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Mark Brown <broonie@...nel.org>, Wolfram Sang <wsa@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/5] spi: spi-geni-qcom: Add support for GPI dma

Hi,


On Thu, Oct 14, 2021 at 9:04 AM Vinod Koul <vkoul@...nel.org> wrote:
> > > +static bool geni_can_dma(struct spi_controller *ctlr,
> > > +                        struct spi_device *slv, struct spi_transfer *xfer)
> > > +{
> > > +       struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
> > > +
> > > +       /* check if dma is supported */
> > > +       if (mas->cur_xfer_mode == GENI_GPI_DMA)
> > > +               return true;
> > > +
> > > +       return false;
> > > +}
> >
> > nit: might as well handle GENI_SE_DMA as well since it's just as easy
> > to test against GENI_SE_FIFO?
>
> I think I will leave that for the person adding GENI_SE_DMA support :)

I was just thinking of changing the "if" statement:

if (mas->cur_xfer_mode != GENI_SE_FIFO)

It's no skin off your teeth and would make one fewer line to change
if/when the other DMA mode is supported. In any case, I won't push it.
;-)


> > > @@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev)
> > >         if (ret)
> > >                 goto spi_geni_probe_runtime_disable;
> > >
> > > +       /*
> > > +        * check the mode supported and set_cs for fifo mode only
> > > +        * for dma (gsi) mode, the gsi will set cs based on params passed in
> > > +        * TRE
> > > +        */
> > > +       if (mas->cur_xfer_mode == GENI_SE_FIFO)
> > > +               spi->set_cs = spi_geni_set_cs;
> >
> > I'm curious: is there no way to get set_cs() working in GPI mode? In
> > an off-thread conversation Qualcomm seemed to indicate that it was
> > possible, but maybe they didn't quite understand what I was asking.
> >
> > Without an implementation of set_cs() there will be drivers in Linux
> > that simply aren't compatible because they make the assumption that
> > they can lock the bus, set the CS, and do several transfers that are
> > part of some logical "transaction". I believe that both of the SPI
> > peripherals on boards that I work on, cros-ec and the SPI TPM make
> > this assumption. I don't even believe that the drivers can be "fixed"
> > because the requirement is more at the protocol level. The protocol
> > requires you to do things like:
> >
> > 0. Lock the bus.
> > 1. Set the CS.
> > 2. Transfer a few bytes, reading the response as you go.
> > 3. Once you see the other side respond that it's ready, transfer some
> > more bytes.
> > 4. Release the CS.
> > 5. Unlock the bus.
> >
> > You can't do this without a set_cs() implementation because of the
> > requirement to read the responses of the other side before moving on
> > to the next phase of the transfer.
> >
> > As I understand it this is roughly the equivalent of i2c clock
> > stretching but much more ad-hoc and defined peripherals-by-peripheral.
> >
> > In any case, I guess you must have examples of peripherals that need
> > GPI mode and don't need set_cs() so we shouldn't block your way
> > forward, but I'm just curious if you had more info on this.
>
> So I have asked some qcom folks, they tell me it is _possible_ to use
> the cs bit in the TRE and it can work. But TBH I am not yet convinced it
> would work as advertised. So do you enable the GPI mode for chrome books
> or it is SE DMA mode (i think SE DMA mode might be simpler to use for
> your case)

Sounds promising. I'm curious about why you're not convinced it would
work as advertised? Right now we have all our SPI devices running in
FIFO mode (!) and we've been trying to find a way to get them in DMA
mode. I think Qualcomm is trying to avoid supporting both SE DMA and
GPI DMA mode so they are saying that once GPI mode works then we can
just use that.

I don't really have a huge objection to that, but I also have zero
experience with GPI DMA mode. We don't have any need (at the moment)
to share our SPI bus with multiple execution environments, but it
seems like GPI mode _could_ still work as long as the chip select
problem is solved. I did manage to get some more documentation and I
do see a "LOCK" command, so maybe that combined with leaving the CS
asserted would solve the problem? Maybe Mark would allow your driver
to get called from spi_bus_lock() and spi_bus_unlock(). That seems
like it would be important to do anyway to match the Linux SPI client
model...

NOTE: in reality, we sorta paper over the "chip select" problem anyway
on Chromebooks. We just configure the chip select lines as GPIOs and
let Linux manage them. There is much less overhead in setting a GPIO
compared to messaging a QUP, so this improves performance. As long as
this continues to work, perhaps we don't care about whether we can
really tell GPI mode to leave the CS asserted.

-Doug

Powered by blists - more mailing lists