Message-ID: <98baa5ba-1254-86e5-de9b-ef1a03912a55@redhat.com>
Date:   Wed, 1 Mar 2023 11:51:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [RFC PATCH 2/4] fpga: add fake FPGA region
Content-Language: en-US
To:     Xu Yilun <yilun.xu@intel.com>
Cc:     linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230203170653.414990-1-marpagan@redhat.com>
 <20230203170653.414990-3-marpagan@redhat.com>
 <Y/Ckw+F/sb2er2oK@yilunxu-OptiPlex-7050>
 <3f17060a-69ce-fb39-61ab-a892b9a1e8c6@redhat.com>
 <Y/hlUOy26HBfdJFR@yilunxu-OptiPlex-7050>
From:   Marco Pagani <marpagan@redhat.com>
In-Reply-To: <Y/hlUOy26HBfdJFR@yilunxu-OptiPlex-7050>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@redhat.com>
>>>> ---
>>>>  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@redhat.com>
>>>> + */
>>>> +
>>>> +#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,
>>>> +				      &region_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