On 2023-02-24 08:20, Xu Yilun wrote:
> On 2023-02-21 at 15:53:20 +0100, Marco Pagani wrote:
>>
>>
>> On 2023-02-18 11:13, Xu Yilun wrote:
>>> On 2023-02-03 at 18:06:51 +0100, Marco Pagani wrote:
>>>> Add fake FPGA region platform driver with support functions. This
>>>> module is part of the KUnit test suite for the FPGA subsystem.
>>>>
>>>> Signed-off-by: Marco Pagani <marpagan@xxxxxxxxxx>
>>>> ---
>>>> drivers/fpga/tests/fake-fpga-region.c | 186 ++++++++++++++++++++++++++
>>>> drivers/fpga/tests/fake-fpga-region.h | 37 +++++
>>>> 2 files changed, 223 insertions(+)
>>>> create mode 100644 drivers/fpga/tests/fake-fpga-region.c
>>>> create mode 100644 drivers/fpga/tests/fake-fpga-region.h
>>>>
>>>> diff --git a/drivers/fpga/tests/fake-fpga-region.c b/drivers/fpga/tests/fake-fpga-region.c
>>>> new file mode 100644
>>>> index 000000000000..095397e41837
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/tests/fake-fpga-region.c
>>>> @@ -0,0 +1,186 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Driver for fake FPGA region
>>>> + *
>>>> + * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
>>>> + *
>>>> + * Author: Marco Pagani <marpagan@xxxxxxxxxx>
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/fpga/fpga-mgr.h>
>>>> +#include <linux/fpga/fpga-region.h>
>>>> +#include <linux/fpga/fpga-bridge.h>
>>>> +#include <kunit/test.h>
>>>> +
>>>> +#include "fake-fpga-region.h"
>>>> +
>>>> +#define FAKE_FPGA_REGION_DEV_NAME "fake_fpga_region"
>>>> +
>>>> +struct fake_region_priv {
>>>> + int id;
>>>> + struct kunit *test;
>>>> +};
>>>> +
>>>> +struct fake_region_data {
>>>> + struct fpga_manager *mgr;
>>>> + struct kunit *test;
>>>> +};
>>>> +
>>>> +/**
>>>> + * fake_fpga_region_register - register a fake FPGA region
>>>> + * @region_ctx: fake FPGA region context data structure.
>>>> + * @test: KUnit test context object.
>>>> + *
>>>> + * Return: 0 if registration succeeded, an error code otherwise.
>>>> + */
>>>> +int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
>>>> + struct fpga_manager *mgr, struct kunit *test)
>>>> +{
>>>> + struct fake_region_data pdata;
>>>> + struct fake_region_priv *priv;
>>>> + int ret;
>>>> +
>>>> + pdata.mgr = mgr;
>>>> + pdata.test = test;
>>>> +
>>>> + region_ctx->pdev = platform_device_alloc(FAKE_FPGA_REGION_DEV_NAME,
>>>> + PLATFORM_DEVID_AUTO);
>>>> + if (IS_ERR(region_ctx->pdev)) {
>>>> + pr_err("Fake FPGA region device allocation failed\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + platform_device_add_data(region_ctx->pdev, &pdata, sizeof(pdata));
>>>> +
>>>> + ret = platform_device_add(region_ctx->pdev);
>>>> + if (ret) {
>>>> + pr_err("Fake FPGA region device add failed\n");
>>>> + platform_device_put(region_ctx->pdev);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + region_ctx->region = platform_get_drvdata(region_ctx->pdev);
>>>> +
>>>> + if (test) {
>>>> + priv = region_ctx->region->priv;
>>>> + kunit_info(test, "Fake FPGA region %d registered\n", priv->id);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_register);
>>>> +
>>>> +/**
>>>> + * fake_fpga_region_unregister - unregister a fake FPGA region
>>>> + * @region_ctx: fake FPGA region context data structure.
>>>> + */
>>>> +void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx)
>>>> +{
>>>> + struct fake_region_priv *priv;
>>>> + struct kunit *test;
>>>> + int id;
>>>> +
>>>> + priv = region_ctx->region->priv;
>>>> + test = priv->test;
>>>> + id = priv->id;
>>>> +
>>>> + if (region_ctx->pdev) {
>>>> + platform_device_unregister(region_ctx->pdev);
>>>> + if (test)
>>>> + kunit_info(test, "Fake FPGA region %d unregistered\n", id);
>>>> + }
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_unregister);
>>>> +
>>>> +/**
>>>> + * fake_fpga_region_add_bridge - add a bridge to a fake FPGA region
>>>> + * @region_ctx: fake FPGA region context data structure.
>>>> + * @bridge: FPGA bridge.
>>>> + *
>>>> + * Return: 0 if registration succeeded, an error code otherwise.
>>>> + */
>>>> +int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
>>>> + struct fpga_bridge *bridge)
>>>> +{
>>>> + struct fake_region_priv *priv;
>>>> + int ret;
>>>> +
>>>> + priv = region_ctx->region->priv;
>>>> +
>>>> + ret = fpga_bridge_get_to_list(bridge->dev.parent, NULL,
>>>> + ®ion_ctx->region->bridge_list);
>>>> +
>>>> + if (priv->test && !ret)
>>>> + kunit_info(priv->test, "Bridge added to fake FPGA region %d\n",
>>>> + priv->id);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_add_bridge);
>>>> +
>>>> +static int fake_fpga_region_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev;
>>>> + struct fpga_region *region;
>>>> + struct fpga_manager *mgr;
>>>> + struct fake_region_data *pdata;
>>>> + struct fake_region_priv *priv;
>>>> + static int id_count;
>>>> +
>>>> + dev = &pdev->dev;
>>>> + pdata = dev_get_platdata(dev);
>>>> +
>>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> + if (!priv)
>>>> + return -ENOMEM;
>>>> +
>>>> + mgr = fpga_mgr_get(pdata->mgr->dev.parent);
>>>> + if (IS_ERR(mgr))
>>>> + return PTR_ERR(mgr);
>>>> +
>>>> + /*
>>>> + * No get_bridges() method since the bridges list is
>>>> + * pre-built using fake_fpga_region_add_bridge()
>>>> + */
>>>
>>> This is not the common use for drivers to associate the region & bridge,
>>> Better to realize the get_bridges() method.
>>
>> Initially, I was using a get_bridges() method to create the list of bridges
>> before each reconfiguration. However, this required having a "duplicated"
>> list of bridges in the context of the fake region low-level driver.
>>
>> Since I couldn't find a reason to keep a duplicate list of bridges in the
>> fake region driver, I decided then to change the approach and build the
>> list of bridges at device instantiation time.
>>
>> In my understanding, the approach of creating the list of bridges just
>> before reconfiguration with a get_bridges() method works best for the
>> OF case, where the topology is already encoded in the DT. I feel using
>> this approach on platforms without OF support would increase complexity
>> and create unnecessary duplication.
>
> I'm not fully get your point. My understanding is we don't have to
> always grab the bridge driver module if we don't reprogram. In many
> cases, we just work with the existing bitstream before Linux is started.
> So generally I prefer not to have an example that gets all bridges at
> initialization unless there is a real need.
The fake region can be used without bridges to model the scenario where
the FPGA is statically configured by the bootloader.
I was referring to the choice between building the bridge list of the
region (fpga_region->bridge_list) ahead of programming vs. just before
programming.
Currently, fake_fpga_region_add_bridge() attaches the bridge directly
to the bridge_list of the fpga_region struct.
Alternatively, I could change fake_fpga_region_add_bridge() to attach
the bridge to a secondary list in the low-level driver. The secondary
list would then be copied to the fpga_region->bridge_list by a
get_bridges() method just before programming.
However, I feel that using this approach would make test code more
complicated than necessary. Ideally, I would like to keep fake modules
as simple as possible.
Thanks,
Marco
