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

[Ulf Samuelsson]

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

As I said, this is not an option for me.

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

Is that not something which you handle by placing your FPGA 
configuration block under the bus bridge in the device tree?

In the proposed driver, the FPGA is under the SPI bus and the
FPGA driver calls the access functions of the bus.

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

That is the plan.

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

AFAIK, they are never configured by a CPU.
They are not supported by U-Boot.

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

There are maintainers of MTD memories but
there is no maintainer of the MTD uclass in MAINTAINERS..

The last major contributors were
Name                    Year            lines
Heiko Schocher          2014-2015       1600
Stefan Roese            2009             811
Boris Brezillon		2017-2018	 600
Miquel Raynal		2018-2019	 564
Sergej Lapin            2013             215
Kyungmin Park           2008             197
Marek Behún		2021	         178

Miquel Raynal is already on copy.

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

The choice is for passive serial solutions.
You have the choice between
* using the proposed driver (which will use the SPI driver)
* write your own access routine in your board files.

You do not have a choice to use an existing driver, because there
is no existing driver. That is the problem with passive serial.

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

The proposed driver replaces the passive serial driver only.
The statement is true for all passive serial.
Please have a look at how it is implemented today.

>> 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/ ?
> 
When you use passive serial and connect using SPI, you cannot use
the U-Boot SPI driver. Everyone are accessing their SPI peripheral
to configure the FPGA in the board files.
Each solution is unique to that board.

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

I agree totally.

> 
>> 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 speak of passive serial only.

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

There are only two boards that use the current passive serial driver.

$ rgrep _Passive_Serial_fns
include/ACEX1K.h:} Altera_ACEX1K_Passive_Serial_fns;
include/ACEX1K.h:} Altera_CYC2_Passive_Serial_fns;
drivers/fpga/cyclon2.c:	Altera_CYC2_Passive_Serial_fns ...
drivers/fpga/ACEX1K.c:	Altera_ACEX1K_Passive_Serial_fns ...

board/beckhoff/mx53cx9020/mx53cx9020.c:...Altera_CYC2_Passive_Serial ...
board/astro/mcf5373l/fpga.c:Altera_CYC2_Passive_Serial ...

$ scripts/get_maintainer.pl board/beckhoff/mx53cx9020/mx53cx9020.c
Patrick Bruenn <p.bruenn at beckhoff.com> (maintainer:MX53 CX9020)
$ scripts/get_maintainer.pl board/astro/mcf5373l/fpga.c
Wolfgang Wegner <w.wegner at astro-kom.de> (maintainer:MCF5373L BOARD)

Patrick Bruenn <p.bruenn at beckhoff.com>
Wolfgang Wegner <w.wegner at astro-kom.de>

I will ask them in a separate email.

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

It works well for passive serial.

Best Regards
Ulf Samuelsson