Re: [PATCH v3 04/21] fpga: add device feature list support

[Alan Tull <atull@kernel.org>]

On Mon, Nov 27, 2017 at 12:42 AM, Wu Hao <hao.wu@intel.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, (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