Re: [PATCH 0/6] NXP DSPI bugfixes and support for LS1028A

[Michael Walle]

Am 2020-03-09 19:48, schrieb Vladimir Oltean:
> On Mon, 9 Mar 2020 at 20:31, Michael Walle <michael@xxxxxxxx> wrote:
> > Am 2020-03-09 19:14, schrieb Vladimir Oltean:
> > > On Mon, 9 Mar 2020 at 20:03, Michael Walle <michael@xxxxxxxx> wrote:
> > >> Am 2020-03-09 15:56, schrieb Vladimir Oltean:
> > >> > From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> > >> >
> > >> > This series addresses a few issues that were missed during the previous
> > >> > series "[PATCH 00/12] TCFQ to XSPI migration for NXP DSPI driver", on
> > >> > SoCs other than LS1021A and LS1043A. DMA mode has been completely
> > >> > broken
> > >> > by that series, and XSPI mode never worked on little-endian
> > >> > controllers.
> > >> >
> > >> > Then it introduces support for the LS1028A chip, whose compatible has
> > >> > recently been documented here:
> > >> >
> > >> > https://lore.kernel.org/linux-devicetree/20200218171418.18297-1-michael@xxxxxxxx/
> > >>
> > >> If it is not compatible with the LS1021A the second compatible string
> > >> should be removed. Depending on the other remark about the endianess,
> > >> it might still be compatible, though.
> > >>
> > >
> > > Please feel free to remove it. I wasn't actually planning to add it in
> > > the first place, but now it that it's there it doesn't really bother
> > > anybody either.
> >
> > But it won't work if the endianess depends on the compatible string ;)
>
> True.

So another reason to not have the endianess (read the CMD/TX register 
offset)
depends on the compatible string. Because then it should work also with 
the
ls1021a version, correct?

-michael


> > >>
> > >> > The device tree for the LS1028A SoC is extended with DMA channels
> > >> > definition, such that even though the default operating mode is XSPI,
> > >> > one can simply change DSPI_XSPI_MODE to DSPI_DMA_MODE in the
> > >> > devtype_data structure of the driver and use that instead.
> > >>
> > >> wouldn't it make more sense, to use DMA is the dma node is present
> > >> in the device tree? otherwise use XSPI mode? I don't think it is
> > >> really handy to change the mode inside the driver.
> > >>
> > >
> > > Let's keep it simple. The driver should configure the hardware in the
> > > most efficient and least buggy mode available. Right now that is XSPI.
> > > The hardware description (aka the device tree) is a separate topic. If
> > > there ever arises any situation where there are corner cases with XSPI
> > > mode, it's good to have a fallback in the form of DMA mode, and not
> > > have to worry about yet another problem (which is that there are 2
> > > sets of device tree blobs in deployment).
> >
> > Point taken. But this is not how other drivers behave, which uses the
> > DMA if its given in the device node.
>
> Also true.
>
> > Btw. do other SoCs perform better with DMA?
>
> Not that I know of.
> My general rule of thumb for this controller is: if it supports XSPI
> then use that, otherwise use DMA. Luckily there is just one controller
> that supports none of those, and that would be Coldfire, which uses
> the braindead EOQ mode. I don't have the hardware to do testing on
> that, but in principle if I did, I would have converted that as well
> to the more functional but less efficient TCFQ mode (now removed).
>
> > -michael
> >
> > > TL;DR: These DMA channels don't really bother anybody but you never
> > > know when they might come in handy.
> > >
> > >> -michael
> > >>
> > >> >
> > >> > For testing, benchmarking and debugging, the mikroBUS connector on the
> > >> > LS1028A-RDB is made available via spidev.
> > >> >
> > >> > Vladimir Oltean (6):
> > >> >   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
> > >> >   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
> > >> >   spi: spi-fsl-dspi: Fix oper_word_size of zero for DMA mode
> > >> >   spi: spi-fsl-dspi: Add support for LS1028A
> > >> >   arm64: dts: ls1028a: Specify the DMA channels for the DSPI
> > >> > controllers
> > >> >   arm64: dts: ls1028a-rdb: Add a spidev node for the mikroBUS
> > >> >
> > >> >  .../boot/dts/freescale/fsl-ls1028a-rdb.dts    | 14 +++++
> > >> >  .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi |  6 +++
> > >> >  drivers/spi/spi-fsl-dspi.c                    | 54 +++++++++++++++----
> > >> >  3 files changed, 64 insertions(+), 10 deletions(-)
> > >
> > > Thanks,
> > > -Vladimir
>
> -Vladimir