On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
> This new fpga subsystem core should unify all fpga drivers/managers 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
> interface for these drivers.
Does this interface support partial reprogramming/configuration
for FPGAs that can do that?
trivial notes:
There are a _lot_ of dev_dbg statements.
I hope some of these would be removed one day,
especially the function tracing style ones, because
there's already a generic kernel mechanism for that.
Maybe perf/trace support could be added eventually.
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
[]
> +/**
> + * fpga_mgr_status_write - Write fpga manager status
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + * @count: Number of characters in @buf
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_status_write(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + int ret;
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + ret = strcmp(buf, "write_init");
> + if (!ret) {
> + ret = fpga_mgr_write_init(mgr);
> + goto out;
> + }
> +
> + ret = strcmp(buf, "write_complete");
> + if (!ret) {
> + ret = fpga_mgr_write_complete(mgr);
> + goto out;
> + }
> +
> + ret = strcmp(buf, "read_init");
> + if (!ret) {
> + ret = fpga_mgr_read_init(mgr);
> + goto out;
> + }
> +
> + ret = strcmp(buf, "read_complete");
> + if (!ret) {
> + ret = fpga_mgr_read_complete(mgr);
> + goto out;
> + }
> +
> + ret = -EINVAL;
> +out:
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return ret ? : count;
> +}
I think this style is awkward and this would be
better written as:
if (!strcmp(buf, "write_init"))
ret = fpga_mgr_write_init(mgr);
else if (!strcmp(buf, "write_complete"))
ret = fpga_mgr_write_complete(mgr);
else if (!strcmp(buf, "read_init"))
ret = fpga_mgr_read_init(mgr);
else if (!strcmp(buf, "read_complete"))
ret = fpga_mgr_read_complete(mgr);
else
ret = -EINVAL;
clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
if (ret)
return ret;
return count;
}
Maybe use (strcmp(...) == 0) if you prefer that.
Both styles are commonly used in linux.
Probably all of the "return ret ?: count;" uses
would be more easily understood on 3 lines.
On Wed, Oct 02, 2013 at 05:35:58PM +0200, Michal Simek wrote:
> +What: /sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
> +Date: October 2013
> +KernelVersion: 3.12
> +Contact: Michal Simek <[email protected]>
> +Description:
> + By reading this file you will get current fpga manager state.
> + Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
> + By writing to this file you can change fpga manager state.
> + Valid options are: write_init, write_complete, read_init,
> + read_complete.
This shouldn't be asymmetric - read/write should be in the same
format.
I strongly encourage you to use text strings to indicate the state of
the configuration FSM, and I *really* think you should rework things
to have an explicit configuration FSM rather than trying to bodge one
together with a bunch of bit flags.
Plus error handling is missing, failures need to be reported back.
Noticed several typos:
> +
> + if (mgr->mops->read_init) {
> + ret = mgr->mops->read_init(mgr);
> + if (ret) {
> + dev_dbg(mgr->dev, "Failed to write_init\n");
^^^^^^^^
read_init
> + if (mgr->mops->write) {
^^^^^^^^^^
read
> + ret = mgr->mops->read(mgr, buf, count);
> + if (ret) {
> + dev_dbg(mgr->dev, "Failed to write\n");
^^^^^^^^^^^^^^^^^
read
> +
> + if (mgr->mops->write_complete) {
^^^^^^^^^^
read
> + ret = mgr->mops->read_complete(mgr);
> + if (ret) {
> + dev_dbg(mgr->dev, "Failed to write_complete\n");
^^^^^^^^^^^^
read
> +static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
> + ssize_t count)
> +{
> + int bit, ret;
> +
> + dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
> +
> + /* FPGE init has to be done to be able to continue */
^^^^^^
FPGA
> +static struct device_attribute fpga_mgr_attrs[] = {
> + __ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
> + __ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
> + __ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
> + fpga_mgr_status_read, fpga_mgr_status_write),
> + __ATTR_NULL
> +};
AFAIK it is preferred to use DEVICE_ATTR_RO(), ATTRIBUTE_GROUPS()
and struct class.dev_groups
eg see this note in linux/device.h
struct class {
struct device_attribute *dev_attrs; /* use dev_groups instead */
const struct attribute_group **dev_groups;
}
> + struct fpga_manager *mgr;
> + int ret;
> +
> + if (!mops) {
> + dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
> + return -EINVAL;
> + }
> +
> + if (!name || !strlen(name)) {
> + dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
> + return -EINVAL;
> + }
> +
> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> + if (!mgr)
> + return -ENOMEM;
I wonder if this is right, it seems like a strange way to make a class
subsystem, usually the struct fpga_manager would contain the 'struct
device' (not a pointer, so you can use container_of) and drvdata would
be reserved for something else.
This seems to create lifetime issues since the devm above will be
free'd when the platform driver is released, but the class device will
live on with the stray pointer. Better to allocate everything against
the class device below.
Plus you need to ensure the device is fully functionally before
device_register is called, otherwise you race with notifications to
userspace.
> +/**
> + * fpga_mgr_unregister: Remove fpga manager
> + * @pdev: Pointer to the platform device of fpga manager
> + *
> + * Function unregister fpga manager and release all temporary structures
> + *
> + * Returns 0 for all cases
> + */
> +int fpga_mgr_unregister(struct platform_device *pdev)
> +{
> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +
> + if (mgr && mgr->mops && mgr->mops->fpga_remove)
> + mgr->mops->fpga_remove(mgr);
> +
> + device_unregister(mgr->dev);
> +
> + spin_lock(&fpga_mgr_idr_lock);
> + idr_remove(&fpga_mgr_idr, mgr->nr);
> + spin_unlock(&fpga_mgr_idr_lock);
What happens when userspace is holding one of the sysfs files open and
you unload the module? Looks like bad things?
Regards,
Jason
On 10/02/2013 08:35 AM, Michal Simek wrote:
>
> Based on my discussion at ELC with Greg KH the new driver should
> support firmware interface for loading bitstream.
>
As I have previously stated, I think this is a mistake simply because
the firmware interface is a bad mapping on requirements for an FPGA,
especially once you account for the vast number of ways an FPGA can
get loaded and you take partial reconfiguration into account. I
happen to be at a face to face meeting with Greg today, so I'll pick
his brain a bit.
-hpa
On Wed 2013-10-02 12:00:52, H. Peter Anvin wrote:
> On 10/02/2013 08:35 AM, Michal Simek wrote:
> >
> > Based on my discussion at ELC with Greg KH the new driver should
> > support firmware interface for loading bitstream.
> >
>
> As I have previously stated, I think this is a mistake simply because
> the firmware interface is a bad mapping on requirements for an FPGA,
> especially once you account for the vast number of ways an FPGA can
> get loaded and you take partial reconfiguration into account. I
> happen to be at a face to face meeting with Greg today, so I'll pick
> his brain a bit.
Umm. Could the discussion be kept on line?
Yes, more than one bitstream makes sense for fpga, but more than one
firmware makes sense for most other devices, too...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
>
> Through firmware interface:
> cat /sys/class/fpga_manager/fpga0/name
> echo -n fpga.bin > /sys/class/fpga_manager/fpga0/firmware
>
> Through sysfs bin file:
> cat /sys/class/fpga_manager/fpga0/fpga_config_state
> echo -n write_init > /sys/class/fpga_manager/fpga0/fpga_config_state
> cat /lib/firmware/fpga.bin > /sys/class/fpga_manager/fpga0/fpga_config_data
> echo -n write_complete > /sys/class/fpga_manager/fpga0/fpga_config_state
>
Hi Michal,
I have v2 working for me with Altera socfpga and had some feedback.
Add me and Dinh as maintainers.
This driver now has two interfaces for programming the image.
I don't think things in the kernel usually have multiple interfaces.
Does the fpga community in general find that the firmware class is
suitable for all our use cases? I think it only supports the most simple
use cases.
My original fpga framework that you started with supported writing the
fpga device through the devnode, i.e.
cat fpga.bin > /dev/fpga0
I think we should get back to that basic char driver interface like that.
It seems like if you have a char driver, you would open and write to the
devnode instead of adding an attribute under /sys.
The 'flags' implementation is a nice way to do some locking. But it doesn't
replace the status op to get fpga manager status which vanished in v2.
So please add that back. Its interface was that catting the 'status'
attribute got a status description from the low level driver such as
'power up phase' or 'reset phase'. Too useful to just get rid of.
Alan
> Subsystem supports working with phandles for cases where you want to load
> bitstreams for particular device through defined device.
> For example:
> mngr@0 {
> compatible = "whatever";
> fpga-mgr = <&ps7_dev_cfg_0>;
> ...
> } ;
>
> With these lines you can get easily load bitstream to the device.
>
> struct fpga_manager *mgr;
> mgr = of_find_fpga_mgr_by_phandle(pdev->dev.of_node, "fpga-mgr");
> if (mgr)
> mgr->fpga_firmware_write(mgr, "filename");
>
> NOTE: I have added there of_find_fpga_mgr_by_node()
> and of_find_fpga_mgr_by_phandle() but maybe they should be added
> separately to drivers/of/of_fpga.c.
>
> Alessandro: I haven't looked at your FMC cases but maybe this
> could be also worth for your cases.
>
> TODO:
> - Probably make sense to create doc in Documentation folder too.
> - When interface is fine also send zynq devcfg driver
> - Properly test reading (we have problem with zynq devcfg driver now)
> - Not sure if firmware interface also provide option to create
> files from kernel space.
>
> Thanks for your comments,
> Michal
>
> Changes in v2:
> - Remove ! from all error message not to be shouty
> - Fix error codes
> - Add sysfs-class-fpga description
> - Use read/write helper functions with bit protection
> - s/fpga_mgr_status_show/fpga_mgr_status_read/g
> - Do not all end driver status just show core status
> - Extract firmware support to specific sysfs firmware file
> - Add support for sysfs bin attributes (fpga_config_state, fpga_config_data)
> - Allocate space for name dynamically
> - Introduce new flags bits (INIT_DONE, READ, WRITE)
>
> Michal Simek (1):
> fpga: Introduce new fpga subsystem
>
> Documentation/ABI/testing/sysfs-class-fpga | 33 ++
> MAINTAINERS | 7 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/fpga/Kconfig | 18 +
> drivers/fpga/Makefile | 5 +
> drivers/fpga/fpga-mgr.c | 753 +++++++++++++++++++++++++++++
> include/linux/fpga.h | 110 +++++
> 8 files changed, 929 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-fpga
> create mode 100644 drivers/fpga/Kconfig
> create mode 100644 drivers/fpga/Makefile
> create mode 100644 drivers/fpga/fpga-mgr.c
> create mode 100644 include/linux/fpga.h
>
> --
> 1.8.2.3
>
On 10/03/2013 08:49 AM, Pavel Machek wrote:
> On Wed 2013-10-02 12:00:52, H. Peter Anvin wrote:
>> On 10/02/2013 08:35 AM, Michal Simek wrote:
>>>
>>> Based on my discussion at ELC with Greg KH the new driver should
>>> support firmware interface for loading bitstream.
>>>
>>
>> As I have previously stated, I think this is a mistake simply because
>> the firmware interface is a bad mapping on requirements for an FPGA,
>> especially once you account for the vast number of ways an FPGA can
>> get loaded and you take partial reconfiguration into account. I
>> happen to be at a face to face meeting with Greg today, so I'll pick
>> his brain a bit.
>
> Umm. Could the discussion be kept on line?
I agree with you.
But anyway what was resolution from that meeting?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
> But anyway what was resolution from that meeting?
It never happened, we got distracted by lunch :)
Yes; I never got too corner Greg ;)
Greg Kroah-Hartman <[email protected]> wrote:
>On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>> But anyway what was resolution from that meeting?
>
>It never happened, we got distracted by lunch :)
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
> Yes; I never got too corner Greg ;)
>
> Greg Kroah-Hartman <[email protected]> wrote:
>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>>> But anyway what was resolution from that meeting?
>>
>> It never happened, we got distracted by lunch :)
Then why not to have it here?
M
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
Hi,
On 10/03/2013 11:46 PM, Alan Tull wrote:
> On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
>
>>
>> Through firmware interface:
>> cat /sys/class/fpga_manager/fpga0/name
>> echo -n fpga.bin > /sys/class/fpga_manager/fpga0/firmware
>>
>> Through sysfs bin file:
>> cat /sys/class/fpga_manager/fpga0/fpga_config_state
>> echo -n write_init > /sys/class/fpga_manager/fpga0/fpga_config_state
>> cat /lib/firmware/fpga.bin > /sys/class/fpga_manager/fpga0/fpga_config_data
>> echo -n write_complete > /sys/class/fpga_manager/fpga0/fpga_config_state
>>
>
> Hi Michal,
>
> I have v2 working for me with Altera socfpga and had some feedback.
>
> Add me and Dinh as maintainers.
why not just one? What about you?
>
> This driver now has two interfaces for programming the image.
> I don't think things in the kernel usually have multiple interfaces.
The question here is if this is a problem. i2c create char devices
and also provide sysfs access too. It is done through notification.
> Does the fpga community in general find that the firmware class is
> suitable for all our use cases? I think it only supports the most simple
> use cases.
Let's continue with this on that second thread and we will see what happen.
> My original fpga framework that you started with supported writing the
> fpga device through the devnode, i.e.
> cat fpga.bin > /dev/fpga0
> I think we should get back to that basic char driver interface like that.
> It seems like if you have a char driver, you would open and write to the
> devnode instead of adding an attribute under /sys.
It is the same as above. As you know we can simple add support for char
device with the current set of functions without changing logic in the driver.
>
> The 'flags' implementation is a nice way to do some locking. But it doesn't
> replace the status op to get fpga manager status which vanished in v2.
> So please add that back. Its interface was that catting the 'status'
> attribute got a status description from the low level driver such as
> 'power up phase' or 'reset phase'. Too useful to just get rid of.
No problem to add it back but it means that core will loose control
about values which can be returned back to the user. It is probably better
to create set of return values.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
Hi Joe,
On 10/02/2013 06:06 PM, Joe Perches wrote:
> On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
>> This new fpga subsystem core should unify all fpga drivers/managers 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
>> interface for these drivers.
>
> Does this interface support partial reprogramming/configuration
> for FPGAs that can do that?
Currently we have two interfaces and third one is around (char driver)
that's why I didn't look at this support.
But for Xilinx devices init and complete phase is different.
We can use one more flag parameter for init and complete functions.
It means we can simple extend config states with write_init_partial, etc
Or the second option is to create new sysfs file to indicate
that we will work with partial bitstreams.
> trivial notes:
>
> There are a _lot_ of dev_dbg statements.
>
> I hope some of these would be removed one day,
> especially the function tracing style ones, because
> there's already a generic kernel mechanism for that.
>
> Maybe perf/trace support could be added eventually.
Are you talking about trace_printk?
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> []
>> +/**
>> + * fpga_mgr_status_write - Write fpga manager status
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + * @count: Number of characters in @buf
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_status_write(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
>> + return -EBUSY;
>> +
>> + ret = strcmp(buf, "write_init");
>> + if (!ret) {
>> + ret = fpga_mgr_write_init(mgr);
>> + goto out;
>> + }
>> +
>> + ret = strcmp(buf, "write_complete");
>> + if (!ret) {
>> + ret = fpga_mgr_write_complete(mgr);
>> + goto out;
>> + }
>> +
>> + ret = strcmp(buf, "read_init");
>> + if (!ret) {
>> + ret = fpga_mgr_read_init(mgr);
>> + goto out;
>> + }
>> +
>> + ret = strcmp(buf, "read_complete");
>> + if (!ret) {
>> + ret = fpga_mgr_read_complete(mgr);
>> + goto out;
>> + }
>> +
>> + ret = -EINVAL;
>> +out:
>> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>> +
>> + return ret ? : count;
>> +}
>
> I think this style is awkward and this would be
> better written as:
>
> if (!strcmp(buf, "write_init"))
> ret = fpga_mgr_write_init(mgr);
> else if (!strcmp(buf, "write_complete"))
> ret = fpga_mgr_write_complete(mgr);
> else if (!strcmp(buf, "read_init"))
> ret = fpga_mgr_read_init(mgr);
> else if (!strcmp(buf, "read_complete"))
> ret = fpga_mgr_read_complete(mgr);
> else
> ret = -EINVAL;
>
> clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>
> if (ret)
> return ret;
>
> return count;
> }
>
> Maybe use (strcmp(...) == 0) if you prefer that.
> Both styles are commonly used in linux.
sure.
> Probably all of the "return ret ?: count;" uses
> would be more easily understood on 3 lines.
This structure is also quite common in the kernel
git grep "? :" | wc -l
415
git grep "?:" | wc -l
541
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On Fri, Oct 04, 2013 at 06:15:28PM +0200, Michal Simek wrote:
> > Probably all of the "return ret ?: count;" uses
> > would be more easily understood on 3 lines.
>
> This structure is also quite common in the kernel
> git grep "? :" | wc -l
> 415
>
> git grep "?:" | wc -l
> 541
And it should all be removed and fixed up properly to make it easier to
read and understand. Please don't add new instances of it, I sure will
not take it.
greg k-h
On 10/02/2013 07:46 PM, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2013 at 05:35:58PM +0200, Michal Simek wrote:
>
>> +What: /sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
>> +Date: October 2013
>> +KernelVersion: 3.12
>> +Contact: Michal Simek <[email protected]>
>> +Description:
>> + By reading this file you will get current fpga manager state.
>> + Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
>> + By writing to this file you can change fpga manager state.
>> + Valid options are: write_init, write_complete, read_init,
>> + read_complete.
>
> This shouldn't be asymmetric - read/write should be in the same
> format.
>
> I strongly encourage you to use text strings to indicate the state of
> the configuration FSM, and I *really* think you should rework things
> to have an explicit configuration FSM rather than trying to bodge one
> together with a bunch of bit flags.
Any favourite names for states?
Or ready, write_init, write_complete is enough for now?
Alan: This should be unified way for user to get proper states from the
driver. It means from my point of view will be better to have set of states
which this function can return instead of calling origin status function
where we can't control states for all these drivers.
> Plus error handling is missing, failures need to be reported back.
Will fix this.
> Noticed several typos:
Ah yeah c&p. :-(
>
>> +
>> + if (mgr->mops->read_init) {
>> + ret = mgr->mops->read_init(mgr);
>> + if (ret) {
>> + dev_dbg(mgr->dev, "Failed to write_init\n");
> ^^^^^^^^
> read_init
>
>> + if (mgr->mops->write) {
> ^^^^^^^^^^
> read
>
>> + ret = mgr->mops->read(mgr, buf, count);
>> + if (ret) {
>> + dev_dbg(mgr->dev, "Failed to write\n");
> ^^^^^^^^^^^^^^^^^
> read
>
>> +
>> + if (mgr->mops->write_complete) {
> ^^^^^^^^^^
> read
>> + ret = mgr->mops->read_complete(mgr);
>> + if (ret) {
>> + dev_dbg(mgr->dev, "Failed to write_complete\n");
> ^^^^^^^^^^^^
> read
>
>> +static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
>> + ssize_t count)
>> +{
>> + int bit, ret;
>> +
>> + dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
>> +
>> + /* FPGE init has to be done to be able to continue */
> ^^^^^^
> FPGA
>
>> +static struct device_attribute fpga_mgr_attrs[] = {
>> + __ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
>> + __ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
>> + __ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
>> + fpga_mgr_status_read, fpga_mgr_status_write),
>> + __ATTR_NULL
>> +};
>
> AFAIK it is preferred to use DEVICE_ATTR_RO(), ATTRIBUTE_GROUPS()
> and struct class.dev_groups
>
> eg see this note in linux/device.h
>
> struct class {
> struct device_attribute *dev_attrs; /* use dev_groups instead */
> const struct attribute_group **dev_groups;
> }
>
This will be fixed in v3. I have already changed this.
>> + struct fpga_manager *mgr;
>> + int ret;
>> +
>> + if (!mops) {
>> + dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!name || !strlen(name)) {
>> + dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
>> + return -EINVAL;
>> + }
>> +
>> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
>> + if (!mgr)
>> + return -ENOMEM;
>
> I wonder if this is right, it seems like a strange way to make a class
> subsystem, usually the struct fpga_manager would contain the 'struct
> device' (not a pointer, so you can use container_of) and drvdata would
> be reserved for something else.
I am not following you here. mrg structure is connected with the driver
it means when driver is removed then this structure is freed.
>
> This seems to create lifetime issues since the devm above will be
> free'd when the platform driver is released, but the class device will
> live on with the stray pointer. Better to allocate everything against
> the class device below.
device in unregistered before this structure is freed.
fpga_mgr_unregister() is called in the platform driver in remove function.
>
> Plus you need to ensure the device is fully functionally before
> device_register is called, otherwise you race with notifications to
> userspace.
fpga_mgr_register() should be called as the latest function in the probe
in platform_driver. At least it is what I have for zynq.
>> +/**
>> + * fpga_mgr_unregister: Remove fpga manager
>> + * @pdev: Pointer to the platform device of fpga manager
>> + *
>> + * Function unregister fpga manager and release all temporary structures
>> + *
>> + * Returns 0 for all cases
>> + */
>> +int fpga_mgr_unregister(struct platform_device *pdev)
>> +{
>> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
>> +
>> + if (mgr && mgr->mops && mgr->mops->fpga_remove)
>> + mgr->mops->fpga_remove(mgr);
>> +
>> + device_unregister(mgr->dev);
>> +
>> + spin_lock(&fpga_mgr_idr_lock);
>> + idr_remove(&fpga_mgr_idr, mgr->nr);
>> + spin_unlock(&fpga_mgr_idr_lock);
>
> What happens when userspace is holding one of the sysfs files open and
> you unload the module? Looks like bad things?
I didn't test this but feel free to check it.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On 10/04/2013 07:28 AM, Michal Simek wrote:
> On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
>> Yes; I never got too corner Greg ;)
>>
>> Greg Kroah-Hartman <[email protected]> wrote:
>>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>>>> But anyway what was resolution from that meeting?
>>>
>>> It never happened, we got distracted by lunch :)
>
> Then why not to have it here?
>
The essential question is if the firmware interface really is
appropriate for FPGAs. It definitely has a feel of a "square peg in a
round hole", especially when you consider the myriad ways FPGAs can be
configured (some persistent, some not, some which takes effect now,
some which come later, some which involve bytecode interpreters...)
and considering reconfiguration and partial reconfiguration.
-hpa
On Fri, Oct 04, 2013 at 06:28:23PM +0200, Michal Simek wrote:
> > I strongly encourage you to use text strings to indicate the state of
> > the configuration FSM, and I *really* think you should rework things
> > to have an explicit configuration FSM rather than trying to bodge one
> > together with a bunch of bit flags.
>
> Any favourite names for states?
> Or ready, write_init, write_complete is enough for now?
Doesnt really matter to me, don't forget error states. Transisionts to
ready, write_init and write_complete can all fail.
> > I wonder if this is right, it seems like a strange way to make a class
> > subsystem, usually the struct fpga_manager would contain the 'struct
> > device' (not a pointer, so you can use container_of) and drvdata would
> > be reserved for something else.
>
> I am not following you here. mrg structure is connected with the driver
> it means when driver is removed then this structure is freed.
You've got your mgr structure and then later you allocate a struct
device as well, the mgr can be free'd before the struct device is
released, due to the way ref counting works. You are not doing
anything to compensate for that.
> > This seems to create lifetime issues since the devm above will be
> > free'd when the platform driver is released, but the class device will
> > live on with the stray pointer. Better to allocate everything against
> > the class device below.
>
> device in unregistered before this structure is freed.
> fpga_mgr_unregister() is called in the platform driver in remove function.
Which is the problem, device_unregister dosen't delete 'mgr->dev', and it
doesn't mean the sysfs call backs are uncallable, so you have a free'd
dangling pointer in mgr->dev, and an object lifetime issue.
> > What happens when userspace is holding one of the sysfs files open and
> > you unload the module? Looks like bad things?
>
> I didn't test this but feel free to check it.
You should fix these problems before your driver reaches Linus.
Jason
On 10/04/2013 06:46 PM, H. Peter Anvin wrote:
> On 10/04/2013 07:28 AM, Michal Simek wrote:
>> On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
>>> Yes; I never got too corner Greg ;)
>>>
>>> Greg Kroah-Hartman <[email protected]> wrote:
>>>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>>>>> But anyway what was resolution from that meeting?
>>>>
>>>> It never happened, we got distracted by lunch :)
>>
>> Then why not to have it here?
>>
>
> The essential question is if the firmware interface really is
> appropriate for FPGAs. It definitely has a feel of a "square peg in a
> round hole", especially when you consider the myriad ways FPGAs can be
> configured (some persistent, some not, some which takes effect now,
> some which come later, some which involve bytecode interpreters...)
> and considering reconfiguration and partial reconfiguration.
If you look at it in general I believe that there is wide range
of applications which just contain one bitstream per fpga and the bitstream is replaced
by newer version in upgrade. For them firmware interface should be pretty useful.
Just setup firmware name with bitstream and it will be automatically loaded in
startup phase.
Then there is another set of applications especially in connection to partial reconfiguration
where this can be done statically by pregenerated partial bitstreams
or automatically generated on target cpu. For doing everything on the target
firmware interface is not the best because everything can be handled by user application
and it is easier just to push this bitstream to do device and not to save it
to the fs.
I think the question here is if this subsystem could have several interfaces.
For example Alan is asking for adding char support.
Does it even make sense to have more interfaces with the same backend driver?
When this is answered then we can talk which one make sense to have.
In v2 is sysfs and firmware one. Adding char is also easy to do.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On 10/04/2013 10:44 AM, Michal Simek wrote:
>
> If you look at it in general I believe that there is wide range of
> applications which just contain one bitstream per fpga and the
> bitstream is replaced by newer version in upgrade. For them
> firmware interface should be pretty useful. Just setup firmware
> name with bitstream and it will be automatically loaded in startup
> phase.
>
> Then there is another set of applications especially in connection
> to partial reconfiguration where this can be done statically by
> pregenerated partial bitstreams or automatically generated on
> target cpu. For doing everything on the target firmware interface
> is not the best because everything can be handled by user
> application and it is easier just to push this bitstream to do
> device and not to save it to the fs.
>
> I think the question here is if this subsystem could have several
> interfaces. For example Alan is asking for adding char support.
> Does it even make sense to have more interfaces with the same
> backend driver? When this is answered then we can talk which one
> make sense to have. In v2 is sysfs and firmware one. Adding char
> is also easy to do.
>
Greg, what do you think?
I agree that the firmware interface makes sense when the use of the
FPGA is an implementation detail in a fixed hardware configuration,
but that is a fairly restricted use case all things considered.
-hpa
On Fri, 2013-10-04 at 19:44 +0200, Michal Simek wrote:
> On 10/04/2013 06:46 PM, H. Peter Anvin wrote:
> > On 10/04/2013 07:28 AM, Michal Simek wrote:
> >> On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
> >>> Yes; I never got too corner Greg ;)
> >>>
> >>> Greg Kroah-Hartman <[email protected]> wrote:
> >>>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
> >>>>> But anyway what was resolution from that meeting?
> >>>>
> >>>> It never happened, we got distracted by lunch :)
> >>
> >> Then why not to have it here?
> >>
> >
> > The essential question is if the firmware interface really is
> > appropriate for FPGAs. It definitely has a feel of a "square peg in a
> > round hole", especially when you consider the myriad ways FPGAs can be
> > configured (some persistent, some not, some which takes effect now,
> > some which come later, some which involve bytecode interpreters...)
> > and considering reconfiguration and partial reconfiguration.
>
> If you look at it in general I believe that there is wide range
> of applications which just contain one bitstream per fpga and the bitstream is replaced
> by newer version in upgrade. For them firmware interface should be pretty useful.
> Just setup firmware name with bitstream and it will be automatically loaded in
> startup phase.
>
> Then there is another set of applications especially in connection to partial reconfiguration
> where this can be done statically by pregenerated partial bitstreams
> or automatically generated on target cpu. For doing everything on the target
> firmware interface is not the best because everything can be handled by user application
> and it is easier just to push this bitstream to do device and not to save it
> to the fs.
>
> I think the question here is if this subsystem could have several interfaces.
> For example Alan is asking for adding char support.
> Does it even make sense to have more interfaces with the same backend driver?
Just for clarification, I'm asking for just one way to write the fpga
image data, not two or three.
I like being able to directly write the fpga image buffer from
userspace; that will support the superset of use cases. v2 accepts the
binary image data from a sysfs attribute (cat image.bin
> /sys/class/fpga_manager/fpga0/fpga_config_data). My original fpga
manager framework allowed writing the image data to the device node (cat
image.bin > /dev/fpga0) rather than sysfs. My point is that it that if
you are writing data, that goes to the file ops, not to sysfs
attributes. sysfs is for text communication (Documentation/sysfs.txt:
"Attributes should be ASCII text files...")
> When this is answered then we can talk which one make sense to have.
> In v2 is sysfs and firmware one. Adding char is also easy to do.
Please, not three ways to do the same thing. If you change from having
the fpga_config_data attribute to having a file_operations' write, that
would be what I think is more standard for char drivers in the kernel,
if I'm not mistaken.
>
> Thanks,
> Michal
>
On Fri, 2013-10-04 at 17:27 +0200, Michal Simek wrote:
> Hi,
>
> On 10/03/2013 11:46 PM, Alan Tull wrote:
> > On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
> >
> >>
> >> Through firmware interface:
> >> cat /sys/class/fpga_manager/fpga0/name
> >> echo -n fpga.bin > /sys/class/fpga_manager/fpga0/firmware
> >>
> >> Through sysfs bin file:
> >> cat /sys/class/fpga_manager/fpga0/fpga_config_state
> >> echo -n write_init > /sys/class/fpga_manager/fpga0/fpga_config_state
> >> cat /lib/firmware/fpga.bin > /sys/class/fpga_manager/fpga0/fpga_config_data
> >> echo -n write_complete > /sys/class/fpga_manager/fpga0/fpga_config_state
> >>
> >
> > Hi Michal,
> >
> > I have v2 working for me with Altera socfpga and had some feedback.
> >
> > Add me and Dinh as maintainers.
>
> why not just one? What about you?
That's fine. Just add "Alan Tull <[email protected]>" So we can expect
that in v3, right?
> >
> > This driver now has two interfaces for programming the image.
> > I don't think things in the kernel usually have multiple interfaces.
>
> The question here is if this is a problem. i2c create char devices
> and also provide sysfs access too. It is done through notification.
i2c_master_send() is called as the fops write or from an i2c client, but
not by writing data to some attribute under sysfs. So there aren't two
direct paths from userspace to the i2c core. I am advocating against
multiple ways for userspace to request/send fpga images for programming.
>
> > Does the fpga community in general find that the firmware class is
> > suitable for all our use cases? I think it only supports the most simple
> > use cases.
>
> Let's continue with this on that second thread and we will see what happen.
Yes, let's continue this particular discussion on the other thread.
>
>
> > My original fpga framework that you started with supported writing the
> > fpga device through the devnode, i.e.
> > cat fpga.bin > /dev/fpga0
> > I think we should get back to that basic char driver interface like that.
> > It seems like if you have a char driver, you would open and write to the
> > devnode instead of adding an attribute under /sys.
>
> It is the same as above. As you know we can simple add support for char
> device with the current set of functions without changing logic in the driver.
>
> >
> > The 'flags' implementation is a nice way to do some locking. But it doesn't
> > replace the status op to get fpga manager status which vanished in v2.
> > So please add that back. Its interface was that catting the 'status'
> > attribute got a status description from the low level driver such as
> > 'power up phase' or 'reset phase'. Too useful to just get rid of.
>
> No problem to add it back but it means that core will loose control
> about values which can be returned back to the user. It is probably better
> to create set of return values.
>
> Thanks,
> Michal
>
On Fri, 2013-10-04 at 18:28 +0200, Michal Simek wrote:
> On 10/02/2013 07:46 PM, Jason Gunthorpe wrote:
> > On Wed, Oct 02, 2013 at 05:35:58PM +0200, Michal Simek wrote:
> >
> >> +What: /sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
> >> +Date: October 2013
> >> +KernelVersion: 3.12
> >> +Contact: Michal Simek <[email protected]>
> >> +Description:
> >> + By reading this file you will get current fpga manager state.
> >> + Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
> >> + By writing to this file you can change fpga manager state.
> >> + Valid options are: write_init, write_complete, read_init,
> >> + read_complete.
> >
> > This shouldn't be asymmetric - read/write should be in the same
> > format.
> >
> > I strongly encourage you to use text strings to indicate the state of
> > the configuration FSM, and I *really* think you should rework things
> > to have an explicit configuration FSM rather than trying to bodge one
> > together with a bunch of bit flags.
>
> Any favourite names for states?
> Or ready, write_init, write_complete is enough for now?
Currently the only state I want to attempt to *set* from sysfs would be
reset.
For Altera fpga, the states I want to *report* are:
static const char *const altera_fpga_state_str[] = {
[ALT_FPGAMGR_STAT_POWER_UP] = "power up phase",
[ALT_FPGAMGR_STAT_RESET] = "reset phase",
[ALT_FPGAMGR_STAT_CFG] = "configuration phase",
[ALT_FPGAMGR_STAT_INIT] = "initialization phase",
[ALT_FPGAMGR_STAT_USER_MODE] = "user mode",
[ALT_FPGAMGR_STAT_UNKNOWN] = "undetermined",
[ALT_FPGAMGR_STAT_POWER_OFF] = "powered off",
};
Setting write_init and write_complete explicitly is fine, but not needed
for me. I'm fine with the driver knowing that if I request to write
data, it knows that it needs to write_init and write_complete. Either
way works for me here.
>
> Alan: This should be unified way for user to get proper states from the
> driver. It means from my point of view will be better to have set of states
> which this function can return instead of calling origin status function
> where we can't control states for all these drivers.
I agree with both you and Jason on this :) I agree that it should be
strings, and also that it probably should come from the framework
instead of directly from the low level driver. The framework should
query the low level driver for state. Not all vendors' fpgas support
exactly the same set of states, so we can hash out a superset that we
can agree on that fits all our needs (mine are above).
I think this is separate from your 'flags' implementation. I like that
implementation for locking, but I want something that actually goes to
the low level driver to find out what state the fpga manager has to
report.
>
> > Plus error handling is missing, failures need to be reported back.
>
> Will fix this.
>
> > Noticed several typos:
>
> Ah yeah c&p. :-(
>
> >
> >> +
> >> + if (mgr->mops->read_init) {
> >> + ret = mgr->mops->read_init(mgr);
> >> + if (ret) {
> >> + dev_dbg(mgr->dev, "Failed to write_init\n");
> > ^^^^^^^^
> > read_init
> >
> >> + if (mgr->mops->write) {
> > ^^^^^^^^^^
> > read
> >
> >> + ret = mgr->mops->read(mgr, buf, count);
> >> + if (ret) {
> >> + dev_dbg(mgr->dev, "Failed to write\n");
> > ^^^^^^^^^^^^^^^^^
> > read
> >
> >> +
> >> + if (mgr->mops->write_complete) {
> > ^^^^^^^^^^
> > read
> >> + ret = mgr->mops->read_complete(mgr);
> >> + if (ret) {
> >> + dev_dbg(mgr->dev, "Failed to write_complete\n");
> > ^^^^^^^^^^^^
> > read
> >
> >> +static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
> >> + ssize_t count)
> >> +{
> >> + int bit, ret;
> >> +
> >> + dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
> >> +
> >> + /* FPGE init has to be done to be able to continue */
> > ^^^^^^
> > FPGA
> >
> >> +static struct device_attribute fpga_mgr_attrs[] = {
> >> + __ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
> >> + __ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
> >> + __ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
> >> + fpga_mgr_status_read, fpga_mgr_status_write),
> >> + __ATTR_NULL
> >> +};
> >
> > AFAIK it is preferred to use DEVICE_ATTR_RO(), ATTRIBUTE_GROUPS()
> > and struct class.dev_groups
> >
> > eg see this note in linux/device.h
> >
> > struct class {
> > struct device_attribute *dev_attrs; /* use dev_groups instead */
> > const struct attribute_group **dev_groups;
> > }
> >
>
> This will be fixed in v3. I have already changed this.
>
> >> + struct fpga_manager *mgr;
> >> + int ret;
> >> +
> >> + if (!mops) {
> >> + dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (!name || !strlen(name)) {
> >> + dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> >> + if (!mgr)
> >> + return -ENOMEM;
> >
> > I wonder if this is right, it seems like a strange way to make a class
> > subsystem, usually the struct fpga_manager would contain the 'struct
> > device' (not a pointer, so you can use container_of) and drvdata would
> > be reserved for something else.
>
> I am not following you here. mrg structure is connected with the driver
> it means when driver is removed then this structure is freed.
>
>
> >
> > This seems to create lifetime issues since the devm above will be
> > free'd when the platform driver is released, but the class device will
> > live on with the stray pointer. Better to allocate everything against
> > the class device below.
>
> device in unregistered before this structure is freed.
> fpga_mgr_unregister() is called in the platform driver in remove function.
>
> >
> > Plus you need to ensure the device is fully functionally before
> > device_register is called, otherwise you race with notifications to
> > userspace.
>
> fpga_mgr_register() should be called as the latest function in the probe
> in platform_driver. At least it is what I have for zynq.
>
> >> +/**
> >> + * fpga_mgr_unregister: Remove fpga manager
> >> + * @pdev: Pointer to the platform device of fpga manager
> >> + *
> >> + * Function unregister fpga manager and release all temporary structures
> >> + *
> >> + * Returns 0 for all cases
> >> + */
> >> +int fpga_mgr_unregister(struct platform_device *pdev)
> >> +{
> >> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> >> +
> >> + if (mgr && mgr->mops && mgr->mops->fpga_remove)
> >> + mgr->mops->fpga_remove(mgr);
> >> +
> >> + device_unregister(mgr->dev);
> >> +
> >> + spin_lock(&fpga_mgr_idr_lock);
> >> + idr_remove(&fpga_mgr_idr, mgr->nr);
> >> + spin_unlock(&fpga_mgr_idr_lock);
> >
> > What happens when userspace is holding one of the sysfs files open and
> > you unload the module? Looks like bad things?
>
> I didn't test this but feel free to check it.
>
> Thanks,
> Michal
>
On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> On 10/04/2013 10:44 AM, Michal Simek wrote:
> >
> > If you look at it in general I believe that there is wide range of
> > applications which just contain one bitstream per fpga and the
> > bitstream is replaced by newer version in upgrade. For them
> > firmware interface should be pretty useful. Just setup firmware
> > name with bitstream and it will be automatically loaded in startup
> > phase.
> >
> > Then there is another set of applications especially in connection
> > to partial reconfiguration where this can be done statically by
> > pregenerated partial bitstreams or automatically generated on
> > target cpu. For doing everything on the target firmware interface
> > is not the best because everything can be handled by user
> > application and it is easier just to push this bitstream to do
> > device and not to save it to the fs.
> >
> > I think the question here is if this subsystem could have several
> > interfaces. For example Alan is asking for adding char support.
> > Does it even make sense to have more interfaces with the same
> > backend driver? When this is answered then we can talk which one
> > make sense to have. In v2 is sysfs and firmware one. Adding char
> > is also easy to do.
> >
>
> Greg, what do you think?
>
> I agree that the firmware interface makes sense when the use of the
> FPGA is an implementation detail in a fixed hardware configuration,
> but that is a fairly restricted use case all things considered.
Ideally I thought this would be just like "firmware", you dump the file
to the FPGA, it validates it and away you go with a new image running in
the chip.
But, it sounds like this is much more complicated, so much so that
configfs might be the correct interface for it, as you can do lots of
things there, and it is very flexible (some say too flexible...)
A char device, with a zillion different custom ioctls is also a way to
do it, but one that I really want to avoid as that gets messy really
quickly.
thanks,
greg k-h
On Fri, Oct 04, 2013 at 04:33:41PM -0700, Greg Kroah-Hartman wrote:
> > I agree that the firmware interface makes sense when the use of the
> > FPGA is an implementation detail in a fixed hardware configuration,
> > but that is a fairly restricted use case all things considered.
>
> Ideally I thought this would be just like "firmware", you dump the file
> to the FPGA, it validates it and away you go with a new image running in
> the chip.
That is 99% of the use cases. The other stuff people are talking about
is fringe.
I've been doing FPGAs for > 10 years and I've never once used read back
via the config bus. In fact all my FPGAs turn that feature off once
they are loaded.
Partial reconfiguration is very specialized, and hard to use from a
FPGA design standpoint.
I also think it is sensible to focus this interface on simple SRAM
FPGAs, not FLASH based stuff, or whatever complex device required a
byte code interpreter (never heard of that before).
Jason
On 10/04/2013 04:33 PM, Greg Kroah-Hartman wrote:
>
> Ideally I thought this would be just like "firmware", you dump the file
> to the FPGA, it validates it and away you go with a new image running in
> the chip.
>
> But, it sounds like this is much more complicated, so much so that
> configfs might be the correct interface for it, as you can do lots of
> things there, and it is very flexible (some say too flexible...)
>
> A char device, with a zillion different custom ioctls is also a way to
> do it, but one that I really want to avoid as that gets messy really
> quickly.
>
I'm not sure that a zillion custom ioctls are necessary, but I think we
really need to get a better understanding of the various usage cases
(and there are going to be ones where an "FPGA driver" simply makes no
sense at all.)
-hpa
Every FPGA toolchain I know of has a way to emit JAM/STAPL bytecode files... and a fair number of programming scenarios need them.
Jason Gunthorpe <[email protected]> wrote:
>On Fri, Oct 04, 2013 at 04:33:41PM -0700, Greg Kroah-Hartman wrote:
>
>> > I agree that the firmware interface makes sense when the use of the
>> > FPGA is an implementation detail in a fixed hardware configuration,
>> > but that is a fairly restricted use case all things considered.
>>
>> Ideally I thought this would be just like "firmware", you dump the
>file
>> to the FPGA, it validates it and away you go with a new image running
>in
>> the chip.
>
>That is 99% of the use cases. The other stuff people are talking about
>is fringe.
>
>I've been doing FPGAs for > 10 years and I've never once used read back
>via the config bus. In fact all my FPGAs turn that feature off once
>they are loaded.
>
>Partial reconfiguration is very specialized, and hard to use from a
>FPGA design standpoint.
>
>I also think it is sensible to focus this interface on simple SRAM
>FPGAs, not FLASH based stuff, or whatever complex device required a
>byte code interpreter (never heard of that before).
>
>Jason
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On Fri, Oct 04, 2013 at 09:00:28PM -0700, H. Peter Anvin wrote:
> Every FPGA toolchain I know of has a way to emit JAM/STAPL bytecode
> files... and a fair number of programming scenarios need them.
Yes, but now you are talking about JTAG.
JTAG is a very different problem than configuring over the
configuration bus, I don't think it makes much sense to try and
combine those two things into a single subsystem.
The majority use of JAM/STAPL output is for manufacturing
automation. In system, In field programming of SRAM FPGAs via JTAG is
uncommon.
Jason
I do it all the time.
JAM/STAPL seems to me to be more used for exotic connections to serial flash for persistent programming.
Jason Gunthorpe <[email protected]> wrote:
>On Fri, Oct 04, 2013 at 09:00:28PM -0700, H. Peter Anvin wrote:
>
>> Every FPGA toolchain I know of has a way to emit JAM/STAPL bytecode
>> files... and a fair number of programming scenarios need them.
>
>Yes, but now you are talking about JTAG.
>
>JTAG is a very different problem than configuring over the
>configuration bus, I don't think it makes much sense to try and
>combine those two things into a single subsystem.
>
>The majority use of JAM/STAPL output is for manufacturing
>automation. In system, In field programming of SRAM FPGAs via JTAG is
>uncommon.
>
>Jason
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On 10/05/2013 01:50 AM, H. Peter Anvin wrote:
> On 10/04/2013 04:33 PM, Greg Kroah-Hartman wrote:
>>
>> Ideally I thought this would be just like "firmware", you dump the file
>> to the FPGA, it validates it and away you go with a new image running in
>> the chip.
>>
>> But, it sounds like this is much more complicated, so much so that
>> configfs might be the correct interface for it, as you can do lots of
>> things there, and it is very flexible (some say too flexible...)
>>
>> A char device, with a zillion different custom ioctls is also a way to
>> do it, but one that I really want to avoid as that gets messy really
>> quickly.
>>
>
> I'm not sure that a zillion custom ioctls are necessary, but I think we
> really need to get a better understanding of the various usage cases
> (and there are going to be ones where an "FPGA driver" simply makes no
> sense at all.)
Isn't this a reason to add this to staging area a see where this can go?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On 10/05/2013 07:34 AM, H. Peter Anvin wrote:
> I do it all the time.
>
> JAM/STAPL seems to me to be more used for exotic connections to serial flash for persistent programming.
ok. I expect you have any code which you are using.
Why not to share it with us to see how you are using it?
I will try to find out some people at Xilinx to have generic overview
about all these methods.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On 10/05/2013 01:49 AM, Jason Gunthorpe wrote:
> On Fri, Oct 04, 2013 at 04:33:41PM -0700, Greg Kroah-Hartman wrote:
>
>>> I agree that the firmware interface makes sense when the use of the
>>> FPGA is an implementation detail in a fixed hardware configuration,
>>> but that is a fairly restricted use case all things considered.
>>
>> Ideally I thought this would be just like "firmware", you dump the file
>> to the FPGA, it validates it and away you go with a new image running in
>> the chip.
>
> That is 99% of the use cases. The other stuff people are talking about
> is fringe.
yep.
> I've been doing FPGAs for > 10 years and I've never once used read back
> via the config bus. In fact all my FPGAs turn that feature off once
> they are loaded.
>
> Partial reconfiguration is very specialized, and hard to use from a
> FPGA design standpoint.
And also from software point of view in connection to drivers handling.
It means tools supports partial reconfiguration and even Xilinx produce
TRD design where partial reconfiguration is used but it is long way
to go to get this work nicely.
> I also think it is sensible to focus this interface on simple SRAM
> FPGAs, not FLASH based stuff, or whatever complex device required a
> byte code interpreter (never heard of that before).
Agree.
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On Fri, Oct 04, 2013 at 10:34:10PM -0700, H. Peter Anvin wrote:
> I do it all the time.
>
> JAM/STAPL seems to me to be more used for exotic connections to
> serial flash for persistent programming.
The FPGA tools write two kinds of SVF/JAM/STAPL files, one is ment to
be replayed the to FPGA itself (what I was talking about), and one is
ment to be replayed to the vendor's family of configuration PROMS (aka
funky serial FLASH)
Programming the PROM in-ciruit/in-field via JTAG is normal, while
programming the FPGA via JTAG is uncommon. This proposed FPGA
susbsytem is about programming SRAM FPGAs, not FLASH ...
Fortunately these PROMs are on their way out, modern FPGAs can be
directly connected to normal NOR FLASH and you can program the NOR
via drivers/mtd :)
Regards,
Jason
On 10/05/2013 08:56 AM, H. Peter Anvin wrote:
> I would, but in my case it was employer-owned and closed.
ok. But I believe general concept for this can be shared.
If you used char device, sysfs, etc.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
If I recall correctly we simply poked at the FPGA directly from userspace. Not ideal by any means and also meant we had to have a backup recovery mechanism as it meant that the FPGA had to be programmed already as the bus interface was in soft IP.
Michal Simek <[email protected]> wrote:
>On 10/05/2013 08:56 AM, H. Peter Anvin wrote:
>> I would, but in my case it was employer-owned and closed.
>
>ok. But I believe general concept for this can be shared.
>If you used char device, sysfs, etc.
>
>Thanks,
>Michal
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On 10/07/2013 04:55 PM, H. Peter Anvin wrote:
> If I recall correctly we simply poked at the FPGA directly from userspace.
Not ideal by any means and also meant we had to have a backup recovery mechanism as it meant
that the FPGA had to be programmed already as the bus interface was in soft IP.
ok. How was that physical hardware connection to device you wanted to talk?
Was it any special IP with MMIO? Or gpio jtag emulation or similar?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
Special soft IP presenting a PCI device to the host.
Michal Simek <[email protected]> wrote:
>On 10/07/2013 04:55 PM, H. Peter Anvin wrote:
>> If I recall correctly we simply poked at the FPGA directly from
>userspace.
>Not ideal by any means and also meant we had to have a backup recovery
>mechanism as it meant
>that the FPGA had to be programmed already as the bus interface was in
>soft IP.
>
>ok. How was that physical hardware connection to device you wanted to
>talk?
>Was it any special IP with MMIO? Or gpio jtag emulation or similar?
>
>Thanks,
>Michal
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On 10/07/2013 05:07 PM, H. Peter Anvin wrote:
> Special soft IP presenting a PCI device to the host.
ok. It means that you should need just different backend for this device
which is able to communicate over PCI.
I still can't see why this case should be problematic for this fpga
manager.
As Jason pointed if this is just about JTAG emulation and your
data is in different format then you have to create your backend
which will support this configuration.
I will want to look at gpio jtag emulation to be able to program
different board. We have this support for u-boot and doing in Linux
should be also possible.
I think the question is if we can live with 2/3 user interfaces.
I tend to keep firmware one because it is covering a lot of common
use cases and it can be easily to use.
And then I don't have any preference if sysfs or char device
is better.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
On Tue, 2013-10-08 at 15:00 +0200, Michal Simek wrote:
> On 10/07/2013 05:07 PM, H. Peter Anvin wrote:
> > Special soft IP presenting a PCI device to the host.
>
> ok. It means that you should need just different backend for this device
> which is able to communicate over PCI.
>
> I still can't see why this case should be problematic for this fpga
> manager.
> As Jason pointed if this is just about JTAG emulation and your
> data is in different format then you have to create your backend
> which will support this configuration.
> I will want to look at gpio jtag emulation to be able to program
> different board. We have this support for u-boot and doing in Linux
> should be also possible.
>
> I think the question is if we can live with 2/3 user interfaces.
> I tend to keep firmware one because it is covering a lot of common
> use cases and it can be easily to use.
> And then I don't have any preference if sysfs or char device
The sysfs and char device interface are equal, except I don't think it
is right to write binary data to a sysfs attribute.
The difference between these 3 options is that firmware will work for
some fixed use cases, but either the sysfs or char interface will work
for all the use cases.
Alan
> is better.
>
> Thanks,
> Michal
>
On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
> On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> > On 10/04/2013 10:44 AM, Michal Simek wrote:
> > >
> > > If you look at it in general I believe that there is wide range of
> > > applications which just contain one bitstream per fpga and the
> > > bitstream is replaced by newer version in upgrade. For them
> > > firmware interface should be pretty useful. Just setup firmware
> > > name with bitstream and it will be automatically loaded in startup
> > > phase.
> > >
> > > Then there is another set of applications especially in connection
> > > to partial reconfiguration where this can be done statically by
> > > pregenerated partial bitstreams or automatically generated on
> > > target cpu. For doing everything on the target firmware interface
> > > is not the best because everything can be handled by user
> > > application and it is easier just to push this bitstream to do
> > > device and not to save it to the fs.
> > >
> > > I think the question here is if this subsystem could have several
> > > interfaces. For example Alan is asking for adding char support.
> > > Does it even make sense to have more interfaces with the same
> > > backend driver? When this is answered then we can talk which one
> > > make sense to have. In v2 is sysfs and firmware one. Adding char
> > > is also easy to do.
> > >
> >
> > Greg, what do you think?
> >
> > I agree that the firmware interface makes sense when the use of the
> > FPGA is an implementation detail in a fixed hardware configuration,
> > but that is a fairly restricted use case all things considered.
>
> Ideally I thought this would be just like "firmware", you dump the file
> to the FPGA, it validates it and away you go with a new image running in
> the chip.
>
> But, it sounds like this is much more complicated, so much so that
> configfs might be the correct interface for it, as you can do lots of
> things there, and it is very flexible (some say too flexible...)
>
> A char device, with a zillion different custom ioctls is also a way to
> do it, but one that I really want to avoid as that gets messy really
> quickly.
Hi Greg,
We are discussing a char device that has very few interfaces:
- a way of writing the image to fpga
- a way of getting fpga manager status
- a way of setting fpga manager state
This all looks like standard char driver interface to me. Writing the
image could be writing to the devnode (cat image.bin > /dev/fpga0). The
status stuff would be sysfs attributes. All normal stuff any char
driver in the kernel would do. Why not just go with that?
>
> thanks,
>
> greg k-h
>
On Tue, Oct 08, 2013 at 11:49:46AM -0500, Alan Tull wrote:
> On Tue, 2013-10-08 at 15:00 +0200, Michal Simek wrote:
> > On 10/07/2013 05:07 PM, H. Peter Anvin wrote:
> > > Special soft IP presenting a PCI device to the host.
> >
> > ok. It means that you should need just different backend for this device
> > which is able to communicate over PCI.
> >
> > I still can't see why this case should be problematic for this fpga
> > manager.
> > As Jason pointed if this is just about JTAG emulation and your
> > data is in different format then you have to create your backend
> > which will support this configuration.
> > I will want to look at gpio jtag emulation to be able to program
> > different board. We have this support for u-boot and doing in Linux
> > should be also possible.
> >
> > I think the question is if we can live with 2/3 user interfaces.
> > I tend to keep firmware one because it is covering a lot of common
> > use cases and it can be easily to use.
> > And then I don't have any preference if sysfs or char device
>
> The sysfs and char device interface are equal, except I don't think it
> is right to write binary data to a sysfs attribute.
That's exactly what binary sysfs files are for :)
> The difference between these 3 options is that firmware will work for
> some fixed use cases, but either the sysfs or char interface will work
> for all the use cases.
I don't understand how one will work but the other will not, please
explain.
thanks,
greg k-h
On Tue, Oct 08, 2013 at 12:00:14PM -0500, Alan Tull wrote:
> On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
> > On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> > > On 10/04/2013 10:44 AM, Michal Simek wrote:
> > > >
> > > > If you look at it in general I believe that there is wide range of
> > > > applications which just contain one bitstream per fpga and the
> > > > bitstream is replaced by newer version in upgrade. For them
> > > > firmware interface should be pretty useful. Just setup firmware
> > > > name with bitstream and it will be automatically loaded in startup
> > > > phase.
> > > >
> > > > Then there is another set of applications especially in connection
> > > > to partial reconfiguration where this can be done statically by
> > > > pregenerated partial bitstreams or automatically generated on
> > > > target cpu. For doing everything on the target firmware interface
> > > > is not the best because everything can be handled by user
> > > > application and it is easier just to push this bitstream to do
> > > > device and not to save it to the fs.
> > > >
> > > > I think the question here is if this subsystem could have several
> > > > interfaces. For example Alan is asking for adding char support.
> > > > Does it even make sense to have more interfaces with the same
> > > > backend driver? When this is answered then we can talk which one
> > > > make sense to have. In v2 is sysfs and firmware one. Adding char
> > > > is also easy to do.
> > > >
> > >
> > > Greg, what do you think?
> > >
> > > I agree that the firmware interface makes sense when the use of the
> > > FPGA is an implementation detail in a fixed hardware configuration,
> > > but that is a fairly restricted use case all things considered.
> >
> > Ideally I thought this would be just like "firmware", you dump the file
> > to the FPGA, it validates it and away you go with a new image running in
> > the chip.
> >
> > But, it sounds like this is much more complicated, so much so that
> > configfs might be the correct interface for it, as you can do lots of
> > things there, and it is very flexible (some say too flexible...)
> >
> > A char device, with a zillion different custom ioctls is also a way to
> > do it, but one that I really want to avoid as that gets messy really
> > quickly.
>
> Hi Greg,
>
> We are discussing a char device that has very few interfaces:
> - a way of writing the image to fpga
> - a way of getting fpga manager status
> - a way of setting fpga manager state
>
> This all looks like standard char driver interface to me. Writing the
> image could be writing to the devnode (cat image.bin > /dev/fpga0). The
> status stuff would be sysfs attributes. All normal stuff any char
> driver in the kernel would do. Why not just go with that?
Because we really hate to add new ioctls to the kernel if at all
possible. Using sysfs (and it's one-value-per-file rule), makes
userspace tools easier, and managing the different devices in the system
easier (you know _exactly_ which device you are talking to, you don't
have to guess based on minor number).
thanks,
greg k-h
On Tue, Oct 8, 2013 at 4:44 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Tue, Oct 08, 2013 at 12:00:14PM -0500, Alan Tull wrote:
>> On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
>> > On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
>> > > On 10/04/2013 10:44 AM, Michal Simek wrote:
>> > > >
>> > > > If you look at it in general I believe that there is wide range of
>> > > > applications which just contain one bitstream per fpga and the
>> > > > bitstream is replaced by newer version in upgrade. For them
>> > > > firmware interface should be pretty useful. Just setup firmware
>> > > > name with bitstream and it will be automatically loaded in startup
>> > > > phase.
>> > > >
>> > > > Then there is another set of applications especially in connection
>> > > > to partial reconfiguration where this can be done statically by
>> > > > pregenerated partial bitstreams or automatically generated on
>> > > > target cpu. For doing everything on the target firmware interface
>> > > > is not the best because everything can be handled by user
>> > > > application and it is easier just to push this bitstream to do
>> > > > device and not to save it to the fs.
>> > > >
>> > > > I think the question here is if this subsystem could have several
>> > > > interfaces. For example Alan is asking for adding char support.
>> > > > Does it even make sense to have more interfaces with the same
>> > > > backend driver? When this is answered then we can talk which one
>> > > > make sense to have. In v2 is sysfs and firmware one. Adding char
>> > > > is also easy to do.
>> > > >
>> > >
>> > > Greg, what do you think?
>> > >
>> > > I agree that the firmware interface makes sense when the use of the
>> > > FPGA is an implementation detail in a fixed hardware configuration,
>> > > but that is a fairly restricted use case all things considered.
>> >
>> > Ideally I thought this would be just like "firmware", you dump the file
>> > to the FPGA, it validates it and away you go with a new image running in
>> > the chip.
>> >
>> > But, it sounds like this is much more complicated, so much so that
>> > configfs might be the correct interface for it, as you can do lots of
>> > things there, and it is very flexible (some say too flexible...)
>> >
>> > A char device, with a zillion different custom ioctls is also a way to
>> > do it, but one that I really want to avoid as that gets messy really
>> > quickly.
>>
>> Hi Greg,
>>
>> We are discussing a char device that has very few interfaces:
>> - a way of writing the image to fpga
>> - a way of getting fpga manager status
>> - a way of setting fpga manager state
>>
>> This all looks like standard char driver interface to me. Writing the
>> image could be writing to the devnode (cat image.bin > /dev/fpga0). The
>> status stuff would be sysfs attributes. All normal stuff any char
>> driver in the kernel would do. Why not just go with that?
>
> Because we really hate to add new ioctls to the kernel if at all
> possible.
I don't see any need for adding any ioctls.
> Using sysfs (and it's one-value-per-file rule), makes
> userspace tools easier, and managing the different devices in the system
> easier (you know _exactly_ which device you are talking to, you don't
> have to guess based on minor number).
That's cool. The interface we could use is writing the raw fpga data
to /sys/class/fpga_manager/fpga0/fpga_config_data
Reading or setting the fpga state could be from
/sys/class/fpga_manager/fpga0/fpga_config_state
Or do I misunderstand? Do you include sysfs attributes when you
are talking about ioctls?
Alan
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Oct 08, 2013 at 06:47:41PM -0500, delicious quinoa wrote:
> On Tue, Oct 8, 2013 at 4:44 PM, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Tue, Oct 08, 2013 at 12:00:14PM -0500, Alan Tull wrote:
> >> On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
> >> > On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> >> > > On 10/04/2013 10:44 AM, Michal Simek wrote:
> >> > > >
> >> > > > If you look at it in general I believe that there is wide range of
> >> > > > applications which just contain one bitstream per fpga and the
> >> > > > bitstream is replaced by newer version in upgrade. For them
> >> > > > firmware interface should be pretty useful. Just setup firmware
> >> > > > name with bitstream and it will be automatically loaded in startup
> >> > > > phase.
> >> > > >
> >> > > > Then there is another set of applications especially in connection
> >> > > > to partial reconfiguration where this can be done statically by
> >> > > > pregenerated partial bitstreams or automatically generated on
> >> > > > target cpu. For doing everything on the target firmware interface
> >> > > > is not the best because everything can be handled by user
> >> > > > application and it is easier just to push this bitstream to do
> >> > > > device and not to save it to the fs.
> >> > > >
> >> > > > I think the question here is if this subsystem could have several
> >> > > > interfaces. For example Alan is asking for adding char support.
> >> > > > Does it even make sense to have more interfaces with the same
> >> > > > backend driver? When this is answered then we can talk which one
> >> > > > make sense to have. In v2 is sysfs and firmware one. Adding char
> >> > > > is also easy to do.
> >> > > >
> >> > >
> >> > > Greg, what do you think?
> >> > >
> >> > > I agree that the firmware interface makes sense when the use of the
> >> > > FPGA is an implementation detail in a fixed hardware configuration,
> >> > > but that is a fairly restricted use case all things considered.
> >> >
> >> > Ideally I thought this would be just like "firmware", you dump the file
> >> > to the FPGA, it validates it and away you go with a new image running in
> >> > the chip.
> >> >
> >> > But, it sounds like this is much more complicated, so much so that
> >> > configfs might be the correct interface for it, as you can do lots of
> >> > things there, and it is very flexible (some say too flexible...)
> >> >
> >> > A char device, with a zillion different custom ioctls is also a way to
> >> > do it, but one that I really want to avoid as that gets messy really
> >> > quickly.
> >>
> >> Hi Greg,
> >>
> >> We are discussing a char device that has very few interfaces:
> >> - a way of writing the image to fpga
> >> - a way of getting fpga manager status
> >> - a way of setting fpga manager state
> >>
> >> This all looks like standard char driver interface to me. Writing the
> >> image could be writing to the devnode (cat image.bin > /dev/fpga0). The
> >> status stuff would be sysfs attributes. All normal stuff any char
> >> driver in the kernel would do. Why not just go with that?
> >
> > Because we really hate to add new ioctls to the kernel if at all
> > possible.
>
> I don't see any need for adding any ioctls.
>
> > Using sysfs (and it's one-value-per-file rule), makes
> > userspace tools easier, and managing the different devices in the system
> > easier (you know _exactly_ which device you are talking to, you don't
> > have to guess based on minor number).
>
> That's cool. The interface we could use is writing the raw fpga data
> to /sys/class/fpga_manager/fpga0/fpga_config_data
>
> Reading or setting the fpga state could be from
> /sys/class/fpga_manager/fpga0/fpga_config_state
Ok, that's fine, I don't object to that, but you are giving up the
notification and loading ability of the kernel for the image files by
doing this, which will require you to use/write/maintain userspace
tools. If you use the firmware interface, no userspace tool is needed
at all, which I can see some people really wanting, right?
> Or do I misunderstand? Do you include sysfs attributes when you
> are talking about ioctls?
You can't do ioctls on sysfs files, so no.
greg k-h
On Wed, Oct 09, 2013 at 01:37:05PM -0700, H. Peter Anvin wrote:
> A very common use case would be where a device contains an FPGA but is
> presented to the user as a product, often having its own device driver
> to drive the programmed device and/or additional logic. From *that*
> point of view it would be nice if the FPGA subsystem had the capability
> for the *device driver* to trigger a firmware load request which is then
> fed to the FPGA subsystem for programming. This would be an in-kernel
> interface, in other words.
That is sort of backwards though, how does the driver know it should
load and start fpga progamming?
The way we are working driver attach today is to program the FPGA,
under control of user space, and then do a PCI rescan, which discovers
the FPGA device and triggers driver binding of the PCI FPGA driver.
Please keep in mind that loading the wrong FPGA could permanently
destroy the system. This is why we have meta-data encoded with the
bitstream. The user space loader does some sanity checks :)
Jason
On 10/09/2013 02:07 PM, Jason Gunthorpe wrote:
> That is sort of backwards though, how does the driver know it should
> load and start fpga progamming?
A common way is for there to be a bitstream stored in flash which
presents an interface to download the data. I think some FPGAs with
hard bus IP even has that built in.
Another variant -- common on USB -- is to use a simple USB interface
chip like an FTDI which can be used (sometimes in conjunction with a
CPLD) to (in effect) bitbang in a bitstream into the FPGA. After
configuration, the programming pins are used for the USB interface.
-hpa