linux-kernel - Re: [PATCH 1/3] fpga: Add Efinix Trion & Titanium serial SPI programming driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKrir7gB+cdnHJu6ZgpEOA-rYLpVK0wJOG=9zChmA6fMCyZRQw@mail.gmail.com>
Date: Mon, 21 Oct 2024 15:23:10 +0200
From: Ian Dannapel <iansdannapel@...il.com>
To: Conor Dooley <conor@...nel.org>
Cc: Xu Yilun <yilun.xu@...ux.intel.com>, mdf@...nel.org, hao.wu@...el.com, 
	yilun.xu@...el.com, trix@...hat.com, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, neil.armstrong@...aro.org, heiko.stuebner@...rry.de, 
	rafal@...ecki.pl, linus.walleij@...aro.org, linux-fpga@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] fpga: Add Efinix Trion & Titanium serial SPI
 programming driver

Am Mo., 21. Okt. 2024 um 14:18 Uhr schrieb Conor Dooley <conor@...nel.org>:
>
> On Mon, Oct 21, 2024 at 10:10:20AM +0800, Xu Yilun wrote:
> > On Fri, Oct 18, 2024 at 05:58:44PM +0100, Conor Dooley wrote:
> > > On Fri, Oct 18, 2024 at 09:37:22AM +0800, Xu Yilun wrote:
> > > > On Fri, Sep 27, 2024 at 04:14:42PM +0200, iansdannapel@...il.com wrote:
> > > > > From: Ian Dannapel <iansdannapel@...il.com>
> > > > >
> > > > > Add a new driver for loading binary firmware to volatile
> > > > > configuration RAM using "SPI passive programming" on Efinix FPGAs.
> > > > >
> > > > > Signed-off-by: Ian Dannapel <iansdannapel@...il.com>
> > > > > ---
> > > > >  drivers/fpga/Kconfig                    |  10 ++
> > > > >  drivers/fpga/Makefile                   |   1 +
> > > > >  drivers/fpga/efinix-trion-spi-passive.c | 211 ++++++++++++++++++++++++
> > > > >  3 files changed, 222 insertions(+)
> > > > >  create mode 100644 drivers/fpga/efinix-trion-spi-passive.c
> > > > >
> > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > > index 37b35f58f0df..eb1e44c4e3e0 100644
> > > > > --- a/drivers/fpga/Kconfig
> > > > > +++ b/drivers/fpga/Kconfig
> > > > > @@ -83,6 +83,16 @@ config FPGA_MGR_XILINX_SPI
> > > > >           FPGA manager driver support for Xilinx FPGA configuration
> > > > >           over slave serial interface.
> > > > >
> > > > > +config FPGA_MGR_EFINIX_SPI
> > > > > +       tristate "Efinix FPGA configuration over SPI passive"
> > > > > +       depends on SPI
> > > > > +       help
> > > > > +         This option enables support for the FPGA manager driver to
> > > > > +         configure Efinix Trion and Titanium Series FPGAs over SPI
> > > > > +         using passive serial mode.
> > > > > +         Warning: Do not activate this if there are other SPI devices
> > > > > +         on the same bus as it might interfere with the transmission.
> > > >
> > > > Sorry, this won't work. As you can see, the conflict usage of CS causes
> > > > several concerns. Just a text here is far from enough.
> > > >
> > > > You need to actively work with SPI core/controller drivers to find a
> > > > solution that coordinate the usage of this pin.
> > >
> > > Why does it even impact other SPI devices on the bus? It's not /their/
> > > CS line that is being modified here, it is the line for the FPGA's
> > > programming interface, right?
> > > What am I missing here that makes it any different to any other SPI
> > > device that may need it's CS toggled?
> >
> > IIUC, now spi core or controller driver should fully responsible for
> > HW operations of CS. And every good behaved spi device driver should
> > declare their logical CS index defined by SPI controller and let SPI
> > core/controller driver to proxy the CS change.
> >
> > But if this spi device driver directly aquires CS, it conflicts with
> > the controller and just fails.
>
> Right, I don't think you answered my question here at all, but just
> reading over the kconfig text again I think I understand what it means.
> I'd interpreted this as other devices being impacted by what this driver
> is doing, but actually it is talking about other devices on the bus
> interfering with this one because of how it handles the chip select.

Correct, the problem lies when other devices initiate a transfer in
between the device programming:
If the CS goes inactive in between, the fpga won't be programmed
correctly since it requires that all bytes are transferred within the
same CS active.
If the CS remains active, the fpga will be programmed with random payloads.

But I might have found a solution to coordinate the CS with the
controller (set_cs in struct spi_controller), since the cs state must
be set before any transfer to enter the programming mode.

Regards,
Ian