Date:   Wed, 20 Dec 2017 18:58:01 -0600
From:   Alan Tull <atull@...nel.org>
To:     Wu Hao <hao.wu@...el.com>
Cc:     Moritz Fischer <mdf@...nel.org>, linux-fpga@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-api@...r.kernel.org, "Kang, Luwei" <luwei.kang@...el.com>,
        "Zhang, Yi Z" <yi.z.zhang@...el.com>,
        Tim Whisonant <tim.whisonant@...el.com>,
        Enno Luebbers <enno.luebbers@...el.com>,
        Shiva Rao <shiva.rao@...el.com>,
        Christopher Rauer <christopher.rauer@...el.com>
Subject: Re: [PATCH v3 04/21] fpga: add device feature list support

On Wed, Dec 20, 2017 at 4:29 PM, Alan Tull <atull@...nel.org> wrote:
> On Mon, Nov 27, 2017 at 12:42 AM, Wu Hao <hao.wu@...el.com> wrote:
>
> Hi Hao,
>
>> +
>> +enum port_feature_id {
>> +       PORT_FEATURE_ID_HEADER = 0x0,
>> +       PORT_FEATURE_ID_ERROR = 0x1,
>> +       PORT_FEATURE_ID_UMSG = 0x2,
>> +       PORT_FEATURE_ID_PR = 0x3,
>> +       PORT_FEATURE_ID_STP = 0x4,
>> +       PORT_FEATURE_ID_UAFU = 0x5,
>> +       PORT_FEATURE_ID_MAX = 0x6,
>> +};
>> +
>> +#define FME_FEATURE_NUM                        FME_FEATURE_ID_MAX
>> +#define PORT_FEATURE_NUM               PORT_FEATURE_ID_MAX
>> +
>> +#define FPGA_FEATURE_DEV_FME           "fpga-dfl-fme"
>> +#define FPGA_FEATURE_DEV_PORT          "fpga-dfl-port"
>> +
>> +static inline int feature_platform_data_size(const int num)
>> +{
>> +       return sizeof(struct feature_platform_data) +
>> +               num * sizeof(struct feature);
>> +}
>> +
>> +int fpga_port_id(struct platform_device *pdev);
>> +
>> +static inline int fpga_port_check_id(struct platform_device *pdev,
>> +                                    void *pport_id)
>> +{
>> +       return fpga_port_id(pdev) == *(int *)pport_id;
>> +}
>> +
>> +void __fpga_port_enable(struct platform_device *pdev);
>> +int __fpga_port_disable(struct platform_device *pdev);
>> +
>> +static inline void fpga_port_enable(struct platform_device *pdev)
>> +{
>> +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +
>> +       mutex_lock(&pdata->lock);
>> +       __fpga_port_enable(pdev);
>> +       mutex_unlock(&pdata->lock);
>> +}
>> +
>> +static inline int fpga_port_disable(struct platform_device *pdev)
>> +{
>> +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +       int ret;
>> +
>> +       mutex_lock(&pdata->lock);
>> +       ret = __fpga_port_disable(pdev);
>> +       mutex_unlock(&pdata->lock);
>> +
>> +       return ret;
>> +}
>> +
>> +static inline int __fpga_port_reset(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +
>> +       ret = __fpga_port_disable(pdev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       __fpga_port_enable(pdev);
>> +
>> +       return 0;
>> +}
>> +
>> +static inline int fpga_port_reset(struct platform_device *pdev)
>> +{
>> +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +       int ret;
>> +
>> +       mutex_lock(&pdata->lock);
>> +       ret = __fpga_port_reset(pdev);
>> +       mutex_unlock(&pdata->lock);
>> +
>> +       return ret;
>> +}
>
> I see that the port code is included as part of the enumeration code.
> This is not very future-proofed, if a different port needs to be
> supported.
>
> The port is a FPGA fabric based bridge with expanded functionality,
> right?  So it is similar to the altera freeze bridge, but adds the
> ability to reset the fabric and some other features are promised in
> the future, IIUC.  I still think that the port could be implemented in
> the bridge driver .c file instead of being here as part of the
> enumeration code.   For that to happen, some APIs would need to be
> added to the bridge framework and the FPGA region framework.  Then the
> reset can be requested through a new FPGA region API function.
>
> The advantage of this is that if this patchset evolves and there is
> some other v2 port driver needed, it can be a different driver if it
> needs to be.
>
> If the port reset is really a fabric reset,

Actually 'fabric reset' is probably not clear enough.  It's resetting
the hardware in a partial reconfiguration region, not just resetting
the bridge.  I'm trying to come up with a term that makes that clear
what is getting reset is the contents of the region.

> (correct me if I'm
> remembering wrongly) then it would be helpful to call it a
> fabric_reset.  This would be the first bridge driver supporting fabric
> reset.  I think it won't be the last.
>
> So what I'm proposing would be added/changed would be:
> * move all the bridge code to fpga-dfl-fme-br.c
> * add .fabric_reset to bridge ops
> * add fpga_bridges_reset to fpga-bridge.c (a new function that goes
> through a list of bridges and calls the reset ops if it exists,
> ignores the bridges where it doesn't exist)
> * add fpga_region_fabric_reset to fpga-region.c.  This function gets
> the region, gets the bridges, calls fpga_bridges_reset (can steal code
> from fpga_region_program_fpga)
> * the rest of the patchset can use fpga_region_fabric_reset instead of
> fpga_port_reset
>
> Alan

Powered by blists - more mailing lists