Message-ID: <523AD5DA.6060907@monstr.eu>
Date:	Thu, 19 Sep 2013 12:45:46 +0200
From:	Michal Simek <monstr@...str.eu>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
CC:	Jason Cooper <jason@...edaemon.net>,
	Michal Simek <michal.simek@...inx.com>,
	linux-kernel@...r.kernel.org, Alan Tull <atull@...era.com>,
	Pavel Machek <pavel@....cz>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Dinh Nguyen <dinguyen@...era.com>,
	Philip Balister <philip@...ister.org>,
	Alessandro Rubini <rubini@...dd.com>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Cesar Eduardo Barros <cesarb@...arb.net>,
	Joe Perches <joe@...ches.com>,
	"David S. Miller" <davem@...emloft.net>,
	Stephen Warren <swarren@...dia.com>,
	Arnd Bergmann <arnd@...db.de>,
	David Brown <davidb@...eaurora.org>,
	Dom Cobley <popcornmix@...il.com>
Subject: Re: [RFC PATCH] fpga: Introduce new fpga subsystem

Hi Jason,

On 09/18/2013 10:32 PM, Jason Gunthorpe wrote:
> On Wed, Sep 18, 2013 at 03:15:17PM -0400, Jason Cooper wrote:
> 
>> + Jason Gunthorpe
> 
> Thanks, looks interesting, we could possibly use this interface if it 
> met our needs..

That will be great.

>  
>> On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
>>> This new subsystem should unify all fpga drivers which
>>> do the same things. Load configuration data to fpga
>>> or another programmable logic through common interface.
>>> It doesn't matter if it is MMIO device, gpio bitbanging,
>>> etc. connection. The point is to have the same
>>> inteface for these drivers.
> 
> So, we have many years of in-field experience with this and this API
> doesn't really match what we do.

then we can talk about it to also provide you enough support to achieve
what you want to achieve.

> Here are the steps we perform, from userspace:
>  - Ask kernel to place FPGA into reset and prepare for programming
>    * Kernel can return an error (eg FPGA failed to erase, etc)
>    * this is the PROG_N low -> DONE high, PROG_N high -> INIT_N high
>      sequencing on Xilinx chips

Isn't it enough to do it in your custom driver in read_init/write_init phase?

>  - Ask kernel to load a bitstream.
>    * Userspace locats the bitstream file to load, and the mmaps it.
>    * Userspace passes the entire file in a single write() call to the
>      kernel which streams it over the configuration bus

ok


>    * The kernel can report an erro rhere (eg Xilinx can report CRC
>    error)

yep - you can do it in read_init/write_init phase.
For these generic features like CRC we should probably create
xilinx.c, altera.c, etc files which will provide these generic algorithms.

>  - Ask the kernel to verify that configuration is complete. 
>    * On Xilinx this wait for done to go high

that should be done by read_complete/write_complete.

>  - Ask the kernel to release the configuration bus (tristate
>    all drivers) (or sometimes we have to drive the bus low,
>    it depends on the bitfile, user space knows what to do)

complete phase should be also suitable for that.

> 
> It is very important that userspace know exactly which step fails
> because the resolution is different. We use this in a manufacturing
> setting, so failures are expected and need quick root cause
> determination.
> 
> You could probably address that need by very clearly defining a
> variety of errno values for the various cases. However, it would be a
> disaster if every driver did something a little different :|

Sure I agree with you and others that we have to improve that error values
but in general we can just exactly identify correct error values
which driver should return. I can't see any problem in that.

> 
> Using request_firmware exclusively is not useful for us. We
> format the bitfile with a header that contains our internal tracking
> information. Sometimes we need to bitswap the bitfile. Our userspace
> handles all of this and can pass a bitfile in memory to write().
> 
> request_firmware would be horrible to use :)
>
> Our API uses a binary sysfs attribute to stream the FPGA data, you
> might want to consider that.

It depends where are you going to add this functionality but anyway
in origin Altera driver they use char device but I think that using
request firmware is also one way to go.
But I am not saying that we have to use it exlusively.
As you see the part of fpga_manager struct is just simple write
function which can be used there.
It means if you don't like that device attributes that you are passing
just firmware name we can change that.
I have no problem to use binary sysfs attribute just for reading/writing data
directly to the end driver instead of passing there just information
about firmware name itself.

It should just end up in very similar calling
sequence where it will call one function which will write data
to fpga though any interface.
But the point is that user access to all these devices is the same
and doesn't matter if this zynq pl, altera socfpga, fpga connected
through gpio, etc. These end drivers will be unique but interface
should be the same.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



Download attachment "signature.asc" of type "application/pgp-signature" (264 bytes)

Powered by blists - more mailing lists