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

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

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

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

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

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

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

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.

[...]