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

[Marek Vasut]

On 2/13/23 10:30, Ulf Samuelsson wrote:
> 
> 
> 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.

All these machines already have FPGA framework enabled, just use it or 
extend it.

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

That stops working once you start dealing with partial reconfiguration 
and the bitstream size varies. Then it is back to $filesize .

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

If there is an existing driver, extend it, rework it, but please don't 
introduce new driver that does the same thing.

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

Overloading the MTD subsystem only because it has a .write() callback is 
not the way to go either.

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

DT support would have to be added, but that shouldn't be all that hard. 
There are other simple subsystems which were extended the same way.

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

See above.

To sum it up:
- The only part of the MTD subsystem that is in use by this driver is 
the write callback, everything else is unused. That does not make MTD 
subsystem the right tool.
- There is existing FPGA subsystem both in U-Boot and Linux, extend it 
if necessary. That also covers the usecases which go past a simple full 
bitstream upload.

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

You could make the exact same argument about making the FPGA driver a 
MISC device, CHAR device, BLOCK device, they all have the write 
callback, but that does not mean it is the right fit.

[...]