Re: [PATCH] firmware: Add Lattice ECP3 FPGA configuration via SPI

[Stefan Roese <stefan.roese@gmail.com>]

On 11/26/2012 02:35 AM, Ming Lei wrote:
> On Fri, Nov 23, 2012 at 3:58 PM, Stefan Roese <sr@denx.de> wrote:
>> This patch adds support for bitstream configuration (programming /
>> loading) of the Lattice ECP3 FPGA's via the SPI bus.
>>
>> Here an example on my custom MPC5200 based board:
>>
>> $ echo 1 > /sys/class/firmware/lattice-ecp3.0/loading
>> $ cat fpga_a4m2k.bit > /sys/class/firmware/lattice-ecp3.0/data
>> $ echo 0 > /sys/class/firmware/lattice-ecp3.0/loading
>>
>> leads to these messages:
>>
>> lattice-ecp3 spi32766.0: FPGA Lattice ECP3-35 detected
>> lattice-ecp3 spi32766.0: Configuring the FPGA...
>> lattice-ecp3 spi32766.0: FPGA succesfully configured!
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Ming Lei <ming.lei@canonical.com>
>> ---
>>  arch/powerpc/Kconfig                   |   2 +
>>  drivers/firmware/Kconfig               |  11 ++
>>  drivers/firmware/Makefile              |   1 +
>>  drivers/firmware/lattice-ecp3-config.c | 254 +++++++++++++++++++++++++++++++++
> 
> The driver is just a firmware loader, so looks 'drivers/firmware/' is not
> a good place for it. And it is better to make the driver as part  the
> of the FPGA driver, such as other drivers which need downloading firmware,
> or you can put it under 'drivers/spi' if there is no such fpga driver.

This FPGA loading via the firmware infrastructure is completely
independent from the FPGA device driver. Its common for all Lattice ECP3
FPGA's. So I would like to place this loading driver into a generic
directory. If firmware is wrong then I'll move this driver for the next
version to drivers/spi.

> BTW, you'd better to CC maintainers of this directory.

Will do.

<snip>

>> +static struct platform_device pseudo_dev = {
>> +       .name = DRIVER_NAME,
>> +       .id   = 0,
>> +       .num_resources = 0,
>> +       .dev  = {
>> +               .release = dev_release,
>> +       }
>> +};
> 
> Why do you introduce one such device? If you just make it
> as the parent of firmware device, it is not correct and unnecessary,
> see below.

Good point. Will fix in next version.

>> +
>> +static struct device *ecp3_device = &pseudo_dev.dev;
>> +
>> +static void firmware_load(const struct firmware *fw, void *context)
>> +{
>> +       struct spi_device *spi = (struct spi_device *)context;
>> +       static u8 *buffer;
> 
> Defining the buffer as static is dangerous and will make the buffer shared by
> more than one such FPGA device, so buffer data may become broken and
> cause downloading a mistaken firmware.

Fixed.

<snip>

>> +static int __devinit lattice_ecp3_probe(struct spi_device *spi)
>> +{
>> +       int err;
>> +
>> +       if (platform_device_register(&pseudo_dev))
>> +               return -ENODEV;
>> +
>> +       err = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
>> +                                     FIRMWARE_NAME, ecp3_device,
> 
> The &spi->dev should be passed to request_firmware_nowait() instead of
> the pseudo-device, which is wrong. It is a device lifetime thing. When you
> download firmware via spi device, you should make sure the spi device
> is live during firmware downloading, so the spi device should be passed to
> request_firmware_nowait() to make it as the parent of the firmware device.

Fixed.

Thanks for the review,
Stefan