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

[Ulf Samuelsson]

Den 2023-02-20 kl. 17:17, skrev 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.

The argument was that some boards have FPGA framework without MTD
and that gives overhead for the those board.
Since 21 out of 24 have MTD, the FPGA framework gives overhead
for those boards, which could removed if FPGAs were
installed as MTDs.

> 
>>>> 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 MTD subsystem supports the concept of partitions.
I suspect that this would allow partial configuration to work quite OK.

> 
>>>>  >> * 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.
> 
The current passive serial solutions does not use the existing SPI 
drivers. They access the SPI peripheral outside the driver.
A new driver would build on the existing SPI drivers.

It is not possible to test if a modified driver break the function  on 
those boards, if you do not have access to the boards.

>> 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.
>
As I said, I cannot justify to do that change.

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

No, the MTD subsystem is compatible with the device tree.
You can also get information about the FPGA when you list the mtd devices.
It allows you to support partial reconfiguration through the partition 
feature. You can write any size block to any part of the MTD, and you
can express that in a device tree. That is what is needed.

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

And for use, that introduces a command set, which we do not need
and adds 50-100kB to the image.

> 
>> 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.
> 
That is because you see the "write" as the only function.

> [...]

What would work is to keep the driver as is, replacing
the MTD stuff internally with an fpga_add after generating a struct 
based on the device tree.

Then it would be part of the FPGA subsystem.
To add this as an Altera passive serial is high risk,
since it cannot be tested on boards using that.
It would simplify things a lot to add it as another driver.

The result would of course be something which adds both complexity and 
code size for us, without any benefit.


Best Regards
Ulf Samuelsson