RE: [PATCH v2 07/22] fpga: intel: pcie: parse feature list and create platform device for features.

[Wu, Hao]

> On Sun, Jun 25, 2017 at 8:52 PM, Wu Hao <hao.wu@xxxxxxxxx> wrote:
> > From: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> >
> > Device Feature List structure creates a link list of feature headers
> > within the MMIO space to provide an extensible way of adding features.
> >
> > The Intel FPGA PCIe driver walks through the feature headers to enumerate
> > feature devices, FPGA Management Engine (FME) and FPGA Port for
> Accelerated
> > Function Unit (AFU), and their private sub features. For feature devices,
> > it creates the platform devices and linked the private sub features into
> > their platform data.
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@xxxxxxxxx>
> > Signed-off-by: Enno Luebbers <enno.luebbers@xxxxxxxxx>
> > Signed-off-by: Shiva Rao <shiva.rao@xxxxxxxxx>
> > Signed-off-by: Christopher Rauer <christopher.rauer@xxxxxxxxx>
> > Signed-off-by: Kang Luwei <luwei.kang@xxxxxxxxx>
> > Signed-off-by: Zhang Yi <yi.z.zhang@xxxxxxxxx>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> > ---
> > v2: moved the code to drivers/fpga folder as suggested by Alan Tull.
> >     switched to GPLv2 license.
> >     fixed comments from Moritz Fischer.
> >     fixed kbuild warning, typos and clean up the code.
> > ---
> >  drivers/fpga/Makefile            |   2 +-
> >  drivers/fpga/intel-feature-dev.c | 130 ++++++
> >  drivers/fpga/intel-feature-dev.h | 341 ++++++++++++++++
> >  drivers/fpga/intel-pcie.c        | 841
> ++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 1311 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/fpga/intel-feature-dev.c
> >  create mode 100644 drivers/fpga/intel-feature-dev.h
> >
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 5613133..ad24b3d 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -31,4 +31,4 @@ obj-$(CONFIG_OF_FPGA_REGION)          += of-fpga-
> region.o
> >  # Intel FPGA Support
> >  obj-$(CONFIG_INTEL_FPGA_PCI)           += intel-fpga-pci.o
> >
> > -intel-fpga-pci-objs := intel-pcie.o
> > +intel-fpga-pci-objs := intel-pcie.o intel-feature-dev.o
> > diff --git a/drivers/fpga/intel-feature-dev.c b/drivers/fpga/intel-feature-dev.c
> > new file mode 100644
> > index 0000000..68f9cba
> > --- /dev/null
> > +++ b/drivers/fpga/intel-feature-dev.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * Intel FPGA Feature Device Driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + *   Kang Luwei <luwei.kang@xxxxxxxxx>
> > + *   Zhang Yi <yi.z.zhang@xxxxxxxxx>
> > + *   Wu Hao <hao.wu@xxxxxxxxx>
> > + *   Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> > + *
> > + * This work is licensed under the terms of the GNU GPL version 2. See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "intel-feature-dev.h"
> > +
> > +void feature_platform_data_add(struct feature_platform_data *pdata,
> > +                              int index, const char *name,
> > +                              int resource_index, void __iomem *ioaddr)
> > +{
> > +       WARN_ON(index >= pdata->num);
> > +
> > +       pdata->features[index].name = name;
> > +       pdata->features[index].resource_index = resource_index;
> > +       pdata->features[index].ioaddr = ioaddr;
> > +}
> > +
> > +struct feature_platform_data *
> > +feature_platform_data_alloc_and_init(struct platform_device *dev, int num)
> > +{
> > +       struct feature_platform_data *pdata;
> > +
> > +       pdata = kzalloc(feature_platform_data_size(num), GFP_KERNEL);
> > +       if (pdata) {
> > +               pdata->dev = dev;
> > +               pdata->num = num;
> > +               mutex_init(&pdata->lock);
> > +       }
> > +
> > +       return pdata;
> > +}
> > +
> > +int fme_feature_num(void)
> > +{
> > +       return FME_FEATURE_ID_MAX;
> > +}
> > +
> > +int port_feature_num(void)
> > +{
> > +       return PORT_FEATURE_ID_MAX;
> > +}
> > +
> > +int fpga_port_id(struct platform_device *pdev)
> > +{
> > +       struct feature_port_header *port_hdr;
> > +       struct feature_port_capability capability;
> > +
> > +       port_hdr = get_feature_ioaddr_by_index(&pdev->dev,
> > +                                              PORT_FEATURE_ID_HEADER);
> > +       WARN_ON(!port_hdr);
> > +
> > +       capability.csr = readq(&port_hdr->capability);
> > +       return capability.port_number;
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_port_id);
> > +
> > +/*
> > + * Enable Port by clear the port soft reset bit, which is set by default.
> > + * The User AFU is unable to respond to any MMIO access while in reset.
> > + * __fpga_port_enable function should only be used after
> __fpga_port_disable
> > + * function.
> > + */
> > +void __fpga_port_enable(struct platform_device *pdev)
> > +{
> > +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +       struct feature_port_header *port_hdr;
> > +       struct feature_port_control control;
> > +
> > +       WARN_ON(!pdata->disable_count);
> > +
> > +       if (--pdata->disable_count != 0)
> > +               return;
> > +
> > +       port_hdr = get_feature_ioaddr_by_index(&pdev->dev,
> > +                                              PORT_FEATURE_ID_HEADER);
> > +       WARN_ON(!port_hdr);
> > +
> > +       control.csr = readq(&port_hdr->control);
> > +       control.port_sftrst = 0x0;
> > +       writeq(control.csr, &port_hdr->control);
> > +}
> > +EXPORT_SYMBOL_GPL(__fpga_port_enable);
> > +
> > +#define RST_POLL_INVL 10 /* us */
> > +#define RST_POLL_TIMEOUT 1000 /* us */
> > +
> > +int __fpga_port_disable(struct platform_device *pdev)
> > +{
> > +       struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +       struct feature_port_header *port_hdr;
> > +       struct feature_port_control control;
> > +
> > +       if (pdata->disable_count++ != 0)
> > +               return 0;
> > +
> > +       port_hdr = get_feature_ioaddr_by_index(&pdev->dev,
> > +                                              PORT_FEATURE_ID_HEADER);
> > +       WARN_ON(!port_hdr);
> > +
> > +       /* Set port soft reset */
> > +       control.csr = readq(&port_hdr->control);
> > +       control.port_sftrst = 0x1;
> > +       writeq(control.csr, &port_hdr->control);
> > +
> > +       /*
> > +        * HW sets ack bit to 1 when all outstanding requests have been drained
> > +        * on this port and minimum soft reset pulse width has elapsed.
> > +        * Driver polls port_soft_reset_ack to determine if reset done by HW.
> > +        */
> > +       if (readq_poll_timeout(&port_hdr->control, control.csr,
> > +                              (control.port_sftrst_ack == 1),
> > +                              RST_POLL_INVL, RST_POLL_TIMEOUT)) {
> > +               dev_err(&pdev->dev, "timeout, fail to reset device\n");
> > +               return -ETIMEDOUT;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(__fpga_port_disable);
> > diff --git a/drivers/fpga/intel-feature-dev.h b/drivers/fpga/intel-feature-dev.h
> > new file mode 100644
> > index 0000000..f67784a
> > --- /dev/null
> > +++ b/drivers/fpga/intel-feature-dev.h
> > @@ -0,0 +1,341 @@
> > +/*
> > + * Intel FPGA Feature Device Driver Header File
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + *   Kang Luwei <luwei.kang@xxxxxxxxx>
> > + *   Zhang Yi <yi.z.zhang@xxxxxxxxx>
> > + *   Wu Hao <hao.wu@xxxxxxxxx>
> > + *   Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> > + *
> > + * This work is licensed under the terms of the GNU GPL version 2. See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef __INTEL_FPGA_FEATURE_H
> > +#define __INTEL_FPGA_FEATURE_H
> > +
> > +#include <linux/fs.h>
> > +#include <linux/pci.h>
> > +#include <linux/uuid.h>
> > +#include <linux/delay.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/platform_device.h>
> > +
> > +#ifndef readq
> > +static inline u64 readq(void __iomem *addr)
> > +{
> > +       return readl(addr) + ((u64)readl(addr + 4) << 32);
> > +}
> > +#endif
> > +
> > +#ifndef writeq
> > +static inline void writeq(u64 val, void __iomem *addr)
> > +{
> > +       writel((u32) (val), addr);
> > +       writel((u32) (val >> 32), (addr + 4));
> > +}
> > +#endif
> 
> Hi Hao,
> 
> Was there a reason readq and writeq had to be created for this?  Do
> these get used?
> 

Hi Alan

Thanks for your review.

Driver uses writeq and readq to access HW registers.

Actually this is a problem reported by kbuild against the patch set v1[1].
Writeq/readq are missed in some arch, so I added writeq/readq definitions to
resolve this problem following the same way done by some existing drivers.
After recheck this today, I found I should use linux/io-64-nonatomic-lo-hi.h
instead of rewriting them.  I will fix it in the next version.

[1] http://marc.info/?l=linux-kernel&m=149100396816882&w=2

Thanks
Hao

> Alan