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

[Ulf Samuelsson]

Den 2023-02-12 kl. 23:40, skrev 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.

If I look at Altera SOCFPGAs, I find that 21 defconfigs have MTD support 
and 3 do not.
* socfpga_agilex_defconfig
* socfpga_chameleonv3_defconfig
* socfpga_n5x_defconfig
so in most cases there would be a reduction in code size.

>> 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 size of the FPGA will be in the devicetree, so there is no need
to supply that in the command itself.

>>  >> * 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 "altr,fpga-passive-serial" driver is very close to what I did.
My driver supports a superset of a devicetree for that driver.
It does not look like U-Boot is setup to use that.

I feel that the FPGA manager needs a total rework to make this work
and I cannot justify that. If someone adds devicetree support to the 
FPGA manager, I will certainly look to adopt to it.

>> 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 ?
> 
Every board using the Altera drivers have an Altera_desc which contains 
links to callbacks used by the altera driver.

cls; rgrep Altera_desc | grep -v -e drivers -e include
board/beckhoff/mx53cx9020/mx53cx9020.c:static Altera_desc ccat_fpga = {
board/astro/mcf5373l/fpga.c:Altera_desc altera_fpga[FPGA_COUNT] = {
board/theadorable/fpga.c:static Altera_desc altera_fpga[] = {

They all access the FPGA using spi, gpio etc. outside the driver model.

Not a trace of devicetree support unfortunately.


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

I do not see why other chips could not be supported by an MTD driver.
Conceptually, there is not much difference between reading/writing a
NAND flash and configuring an FPGA.

Obviously, someone would have to take the time to do it, but it is 
certainly possible.

Best Regards
Ulf Samuelsson