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

[Marek Vasut]

On 2/12/23 23:07, Ulf Samuelsson wrote:
> 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.

FPGA and MTD support is orthogonal, you cannot make that assumption.
Consider SoC-FPGA machine booting from eMMC, Altera Cyclone V SoC does 
support that mode of operation, and MTD support can be disabled.

> 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?

You can use DT /aliases node to enumerate the FPGAs the same way i2c 
busses, SPI NORs, SD/MMC devices etc. are enumerated . Also have a look 
at e.g. 'net list' command, similar functionality can be added to the 
FPGA command to list all registered FPGAs.

> Why should I have to remember the size of the FPGA bitstream?

Because it is not always possible to extract that information from the 
bitstream blob, remember, some of those blobs may be just raw binaries 
of the SRAM/flash content .

>  >> * 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?

Have a look at the Linux FPGA DT bindings in 
Documentation/devicetree/bindings/fpga/ . You can implement parsing of 
those bindings into the U-Boot FPGA framework and then add your FPGA 
device configuration interface into the DT.

> 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

These two ^ cover very much every zynq 7000 and zynqmp device in 
existence, since the way those are used is in combination with provided 
custom board DT.

> * bitmain_antminer_s9_defconfig

There is also arch/arm/mach-socfpga/Kconfig: imply FPGA_SOCFPGA , which 
activates the CONFIG_FPGA on all of Altera Cyclone V/Arria V/Stratix 
10/Agilex and whatever new SoCFPGA Intel has.

> 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.

That's correct

> 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.

Which boards do you refer to ?

>  > 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.

Update the existing FPGA framework to parse the necessary DT bindings.

Please do not overload existing framework (MTD or whatever) with 
unrelated functionality only because it provides the .write callback. 
While this may look like a good idea to cover one very specific use 
case, it would fail once all the other use cases which are already 
covered by the FPGA framework come into picture.