[PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-12 kl. 21:01, skrev Marek Vasut:
 > On 2/12/23 20:52, Ulf Samuelsson wrote:
 >>
 >>
 >> Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
 >>  > On 2/11/23 11:07, u-boot at emagii.com wrote:
 >>  >
 >>  > [...]
 >>  >
 >>  >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, size_t len,
 >>  >> +                 size_t *retlen, const u_char *buf)
 >>  >> +{
 >>  >> +    struct udevice *dev = mtd->dev;
 >>  >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
 >>  >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
 >>  >> +    int ret;
 >>  >
 >>  > Do I read this right, that the 'write' callback is the only one doing
 >>  > meaningful work, all the other callbacks are just empty stubs ?
 >>  > Yes, you cannot read back the configuration data.
 >
 > That makes it look like any framework which supports "write" callback 
would be suitable, not just MTD framework.
 >
 >>  > Why not update drivers/fpga/cyclon2.c which is Passive Serial
 >>  > implementation already present in U-Boot for Altera Cyclone II FPGA ,
 >>  > with Cyclone 10 FPGA support ? I believe the PS protocol changed very
 >>  > little.
 >> Since the MTD command set is enough to configure the FPGA, the FPGA 
commands can be removed from the build. The FPGA command set requires 
you to supply addresses, but the MTD command set uses devices.
 >>
 >> So:
 >> * Smaller U-Boot image
 >
 > The MTD framework is rather large, compared to the trivial FPGA 
framework. Can you back this claim with any numbers ?

I am assuming that the MTD framework is needed anyway.
We certainly need it.
 >
 >> * Simplified user interface.
 >
 > If I am to select between 'fpga load' and 'mtd write' for FPGA 
bitstream loading , my obvious choice would be 'fpga load' . How is 
using 'mtd write' any better or simpler ?
 >
fpga load 0   ${loadaddr} ${filesize}
mtd write spy ${loadaddr}

The questions I ask myself.
So is "0" the "spy" FPGA or the "spx" FPGA?
Why should I have to remember the size of the FPGA bitstream?

 >> * The FPGA is an SPI peripheral, so why not add it to the SPI part 
of the device tree?
 >
 > You can add the device into DT and still operate it using the U-Boot 
FPGA framework, just add the DT support. Why not do it that way ?

I don't think you can add the device into DT in U-Boot as it is today.
You can create FPGA contents and add that to the device tree, but not
the configuration itself. At least, I have not seen it.
If I have missed it, where is an example?

The FPGA framework is not really setup to use device-tree to describe
configuration.

Only 5 defconfigs in U-Boot uses the FPGA framework.
* astro_mcf5373l_defconfig
* syzygy_hub_defconfig
* xilinx_zynqmp_virt_defconfig
* xilinx_zynq_virt_defconfig
* bitmain_antminer_s9_defconfig

Their device trees have leafs for configuration:
* compatible = "fpga-region";
* compatible = "xlnx,zynq-devcfg-1.0";
Neither "fpga-region" or "xlnx,zynq-devcfg-1.0" have any compatible 
drivers, so there is really no support for configuration based on 
device-tree at the moment.

If I look at boards using the Altera, no board uses a driver
to configure the FPGA. Instead they implement a number of callbacks
in the board files which manually handles the SPI bus outside the SPI 
driver. No trace of device tree.


 >
 > Let me be blunt about this, I have this feeling that what is 
happening here is just overloading of MTD framework with unrelated 
functionality (FPGA bitstream loading). MTD framework simply is not the 
right place, esp. if there is dedicated FPGA framework, with existing 
Altera PS driver no less.

When you are configuring an FPGA, you are writing a bitstream
to an SRAM inside the FPGA. SRAM are memories.

The MTD framework does exactly what is needed for passive serial.
I do not see that there is another place to add a device-tree based
FPGA configuration, so you basically have to replicate the MTD 
framework, in order to have FPGA configuration in the device tree.


Best Regards
Ulf Samuelsson