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

[Marek Vasut]

On 2/21/23 00:47, Ulf Samuelsson wrote:
> 
> 
> Den 2023-02-20 kl. 23:34, skrev Marek Vasut:
>> On 2/20/23 22:29, Ulf Samuelsson wrote:
>>
>> [...]
>>
>>>> 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.
>>
>> That does not justify overloading the subsystem with unrelated 
>> hardware support for which there is an existing subsystem with 
>> existing drivers for the functionality you would duplicate in the MTD 
>> subsystem. There are existing DT bindings for FPGAs, see Linux 
>> Documentation/devicetree/bindings/fpga/ , use those and add support 
>> for them into the FPGA subsystem if needed .
> 
> 
> The current u-boot FPGA support is incompatible with the linux 
> devicetree for FPGAs.
> There is simply nothing there.
> That is the reason for the proposal in the first place.

Add it, that seems like the best approach here.

>>> You can also get information about the FPGA when you list the mtd 
>>> devices.
>>
>> No, you cannot, because the information you may get out of the FPGA in 
>> some cases does not map to MTD devices .
> 
> Please give some examples.

Which FPGA bus bridge(s) are enabled for example . This is something 
present on SoCFPGA solutions , e.g. an AXI bridge into the fabric .

>>> 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.
>>
>> I am afraid it is not as simple as that, see Linux 
>> Documentation/devicetree/bindings/fpga/fpga-region.txt . Furthermore, 
>> those DT bindings are already part of Linux and Linux expects the DT 
>> to contain exactly those bindings, U-Boot tries not to diverge from 
>> what Linux does in DT.
>>
> 
> The device tree in my driver is a superset to what linux does.
> It tests more status pins, and provides a name for the FPGA.
> It would not be difficult to make it 100% compatible.

Extend the Linux DT bindings if they are insufficient, but please don't 
try and reinvent the wheel.

>>>> - 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.
>>
>> If you were to turn everything that needs read/write callback into an 
>> MTD device, you would be able to save even more space, but that does 
>> not make it right. You could try and reduce the FPGA command size if 
>> there is code which is unused for some usecases, that can be selected 
>> via Kconfig at build time.
>>
> 
>  From my point of view, Having the FPGA command set duplicates the 
> functionality of the MTD.
> That is the major contributor to code space expansion.
> 
> The thing you are writing to is SRAM which is memory.

There are flash based FPGAs , like Altera MAXV.
There are CPLDs which are not even flash based.

> If it is right or wrong to use that as an MTD is a matter of opinion.

I am still hoping the MTD maintainer would provide input here.

>>>>> 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.
>>
>> Yes, because that is the only callback this patch implements, the rest 
>> are empty.
> 
> You cannot read back the configuration from a Cyclone 10.

Therefore, the entire read part of whatever subsystem is unnecessary.

Erase as well for most FPGAs which are SRAM based, even for flash based 
FPGA, per-sector erase is not available, since there is no concept of 
sectors or erase blocks at all.

> A driver for an FPGA where configuration could be read back of course 
> would have a read routine as well.
> The MTD subsystem can display the information the driver provides.
> That is an important piece that would be missing from a MISC or CHAR 
> driver.
> You also have a "device" to work with.

All this is already in the FPGA subsystem, and it is designed toward the 
FPGA use case.

>>>> [...]
>>>
>>> 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.
>>
>> No, that would make things MUCH worse, because we would have a one-off 
>> driver used by one system, therefore poorly tested, which duplicates 
>> functionality of existing driver used by multiple systems and better 
>> tested. This also increases maintenance burden.
> 
> 
> Today you have a custom functionality to access the FPGA in each board 
> package.  That only gets tested when building that board.

No, at least the Altera SoCFPGA and Xilinx anything newer than 
Zynq/ZynqMP is tested regularly on actual hardware, and the code is 
common to all those systems, and lives in drivers/fpga/ .

> So if you have 'n' boards, you have 'n' different ways of accessing the 
> FPGA. Each poorly tested, if number of users are the measurement.
> The only thing in common is the multiplexing code.

Not really, on SoCFPGA systems, even the programming logic is common. 
For external FPGAs, sure, you need to provide the pin or interface 
mapping in some way, via platform data, DT, ACPI, whatever. If that is 
platform data, it should be moved to DT, the FPGA framework DT bindings 
should provide guidance how to do that. But even then, the driver itself 
should be generic, and reside in drivers/fpga/ .

> Anyone that designs a new system, would have to choose between
> using the new driver, or write their own access method.

I vote "reuse existing driver, plug in DT/platform_data/... as needed" .

> They cannot use what is in u-boot today, because it is in the board 
> directory.

This is not true for majority of FPGA systems, see drivers/fpga/ .

> Even if they have the same chip as a board with existing 
> support, they cannot add it in, since it is in the board directory.

Are you actually talking about platform data which are used during 
driver instantiation, or, full driver, in board/ ?

Platform data are fine-ish, full drivers in board/ should be on the way out.

> Only solution is to duplicate the code in their own board directory,
> but the likelyhood of using the same CPU is slim.
> 
> Are you happy with this?

I think we have a vastly different picture of the current situation .

> I wager that anyone wanting to use passive serial would adopt the new 
> driver.
> 
> One solution is of course to ask the maintainers of those boards
> if they would consider moving to a device tree solution.
> Then the new driver would be the only driver.

Put them on CC, there is scripts/get_maintainer.pl script, use -f 
parameter to point it to specific files. I have a feeling a lot of those 
boards with external FPGAs being configured by poking registers in 
board/ are going to be dropped sooner rather than later.

>> One single driver for this common functionality is always better as it 
>> gets more testing.
>>
>>> The result would of course be something which adds both complexity 
>>> and code size for us, without any benefit.
>>
>> Please also consider the MTD maintainer point of view.
>>
> I do not see that this adds much more complexity than any other MTD device.

Again, there is a separate subsystem/framework for FPGAs . I disagree 
one should conflate MTD and FPGA subsystems.