On Tue, Jan 14, 2014 at 01:37:50AM -0800, Insop Song wrote:
> On Tue, Jan 14, 2014 at 1:18 AM, Insop Song <insop.song@xxxxxxxxx> wrote:
> > On Mon, Jan 13, 2014 at 10:44 AM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >> On Sat, Jan 11, 2014 at 04:37:32PM -0800, Insop Song wrote:
> >>> On Sat, Jan 11, 2014 at 4:10 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >>> > On Sat, Jan 11, 2014 at 03:56:48PM -0800, Insop Song wrote:
> >>> >> On Sat, Jan 11, 2014 at 1:05 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >>> >> > On Sat, Jan 11, 2014 at 12:51:28PM -0800, Greg KH wrote:
> >>> >> >> On Thu, Jan 09, 2014 at 11:48:04AM -0800, Insop Song wrote:
> >>> >> >> > This driver downloads Xilinx FPGA firmware using gpio pins.
> >>> >> >> > It loads Xilinx FPGA bitstream format firmware image and
> >>> >> >> > program the Xilinx FPGA using SelectMAP (parallel) mode.
> >>> >> >> >
> >>> >> >> > Signed-off-by: Insop Song <insop.song@xxxxxxxxxxxxx>
> >>> >> >>
> >>> >> >> This patch breaks the build on my machine, please be more careful:
> >>> >> >>
> >>> >> >> drivers/staging/gs_fpgaboot/gs_fpgaboot.c:30:28: fatal error: include/asm/io.h: No such file or directory
> >>> >> >> #include <include/asm/io.h>
> >>> >> >>
> >>> >> >> Please fix this up, test it, and resend.
> >>> >> >
> >>> >> > Also, while you are fixing things up, please just remove the character
> >>> >> > device node from the driver, you aren't doing anything with it, so it's
> >>> >> > not needed.
> >>> >> >
> >>> >>
> >>> >> Thank you for the suggestion, makes a lot of sense and code cleaner.
> >>> >>
> >>> >> Could you take a quick look?
> >>> >> If it is good, will send out a new patch, v5, shortly.
> >>> >>
> >>> >> Main change is
> >>> >>
> >>> >> +/* fake device for request_firmware */
> >>> >> +static struct platform_device *firmware_pdev;
> >>> >>
> >>> >> + firmware_pdev = platform_device_register_simple("fpgaboot", -1,
> >>> >> + NULL, 0);
> >>> >
> >>> > Why do you need a platform device? Don't you have something on the
> >>> > hardware that describes the fact that this device is in the system, or
> >>> > not? Like a PCI id? DMI id? Device Tree entry?
> >>> >
> >>> > You must have something that you can trigger off of.
> >>> >
> >>>
> >>> This driver is only to download fpga firmware, not intended to keep
> >>> managing the fpga device after the downloading. The device we use is
> >>> not part of the device tree or PCI.
> >>
> >> But how does the system know to load it or not, automatically?
> >>
> >>> It downloads the image and after that no need to be existing.
> >>
> >> Then why not have the module remove itself?
> >>
> >
> > The following (insmod to load firmware and rmmod remove the module)
> > commands are triggered from user space application. They are wrapped
> > as a script and triggered from an application.
> > Once the firmware is loaded, then this driver's job is done.
Yes, but that's not the "normal" way kernel drivers interact with
userspace. Please don't try to create something that is different than
the thousands of other drivers out there, and rely on custom userspace
scripts. Let's use the infrastructure we already have in place for
doing this exact thing.
> >>> Here is how we use
> >>>
> >>> $ insmod gs_fpga.ko file="xlinx_fpga_top_bitstream.bit"
> >>> $ rmmod gs_fpga
> >>
> >> Linux modules should be able to be autoloaded based on hardware present
> >> in the system, it's been that way for over a decade now.
> >>
> >
> >
> Greg, I think it's better to add more explanation for FPGA.
I know what a FPGA is, thanks :)
> FPGA (Field-programmable gate array) is not functional until the
> firmware is downloaded.
> That means that it is not easy, if not possible, to auto detect the
> device. This is why "insmod" is manually called.
That's not true, there has to be some way to detect the device is in the
system, otherwise what's to prevent you from loading the module on a
system that doesn't have the hardware and having bad things happen?
There has to be some way to detect this device is present, right? What
is it? Where is it located in the device tree description?
> FPGA firmware download happen ever time system boots, so this is
> different from occasional "upgrade".
Just like all other modules that use the firmware interface for their
hardware :)
> To sum up, FPGA download occurs every boot, and before this download
> is done, the device in question is not functional.
It's functional enough to get the data to it, so it must be operating
somehow...
> > Understood, however the use case for this driver is to load firmware, only that.
> > The other way to do could have been use a user space program read file
> > and access gpio (or other means) which could end up taking lot longer
> > firmware programming time.
> >
> >>> I still needed "device" to use request_firmware() even though I don't
> >>> use it. I've looked at other drivers which uses request_firmware()
> >>> after your suggestion of removing char device node, and I've found
> >>> platform_device can be a simple one to use.
> >>
> >> Please base your device on a real one in the system, like your FPGA
> >> device, surely there is some way it can be detected to be present or
> >> not?
> >>
> >
> > Do you still think a fake platform device doesn't cut? then what do you suggest?
Use a "real" platform device that is created based on the device tree
information that describes the FPGA device in the system. You have to
have some way to describe the memory location to be able to talk to the
FPGA, right? Just use that.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
