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

[Ulf Samuelsson]

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.


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

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


>>> - 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.
If it is right or wrong to use that as an MTD is a matter of opinion.


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

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

Anyone that designs a new system, would have to choose between
using the new driver, or write their own access method.
They cannot use what is in u-boot today, because it is in the board 
directory. 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.
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 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.


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

> [...]
Best Regards
Ulf Samuelsson