diff mbox

[v5] fpga manager: Add Altera CvP driver

Message ID 1494777082-13375-1-git-send-email-agust@denx.de (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Anatolij Gustschin May 14, 2017, 3:51 p.m. UTC
Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
and Arria-10 FPGAs via CvP.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
For building this patch requires https://lkml.org/lkml/2017/5/14/73

Changes in v5:

  - use absolute register offset values
  - move register bit macros below offset defines
  - use _MASK and use GENMASK()
  - remove module paramerers, use image flags in fpga_image_info
    struct and add FPGA_MGR_COMPRESSED_BITSTREAM macro. Use sysfs
    file for config error checking flag instead of module parameter.
    Add sysfs driver interface documentation
  - use unsigned int for loop index
  - add comment about dummy value
  - fix single-line comment style
  - use do {} while (cond) loop style
  - factor out status bit polling code and use more suitable
    timeout values, use usleep_range() instead of udelay()
  - use %zu instead of %zi
  - add comment for modulo 4k op in error checking code, use
    more descriptive variable names to make code more readable
  - use 'mask = BIT(bytes * 8) - 1;' for trailing bytes instead
    of switch statement, add comment about trailing bytes
  - add comments about pci config space access and pci memory
    space enabling in ->probe()
  - remove not needed fpga_mgr_get()/fpga_mgr_put() testing code
  - use module_pci_driver() macro

Changes in v4:

  - Update description about supported FPGA models in Kconfig
    and commit log
  - use writel() for iomem accesses
  - factor out dummy write code to altera_cvp_dummy_write()
  - add data register accessor function (iomem and pci config
    space variants) to reduce code

Changes in v3:

  - removed V-series from description (since the driver works
    also with Arria-10). Also renamed functions, config option
    and driver file name. Changed module description in Kconfig
  - dropped 'firmware=' module option (does not allow to bypass
    the fpga framework any more)
  - fixed build warning with newer gcc
  - changed to use hex numbers for PCI bus/device in registered
    fpga manager name. Use more generic fpga manager name (no
    V-series in the name any more)
  - moved steps 16 to 18 from teardown func to write_complete()
    as suggested by Yi
  - rebased on linux-next and tested with Arria-10/Stratix-V
    devices

Changes in v2:

  - rebase for v4.10, change subject ("Stratix V" to "V series")
    and add GPL header
  - use BIT() in register bit macro definitions
  - change .state() to return status or FPGA_MGR_STATE_UNKNOWN
  - use unique name for registered FPGA manager (append "@B:D.F")
  - use PCI_ANY_ID for device ID, users can set custom ID using
    new_id sysfs interface
  - remove map_base/map_len init and use pci_iomap() instead
  - simplify to use pci_read_config_word() instead of
    pci_bus_read_config_word()
  - run fpga_mgr_put() after usage, so the module can be removed
    without unbinding the driver
  - access CVP_STATUS as a 32-bit register, update CVP_STATUS
    bits macros
  - check CONFIG_READY bit in write_init and perform a teardown
    if this bit is set. Move teardown code to separate function
    as suggested
  - change function names to altera_v_*() as we can use
    the driver with other V-series FPGAs. Also change
    the driver file and Kconfig option names accordingly
  - pad written firmware data if the file size is not a multiple of 4
  - when config error checking is enabled, run checks after a 4KiB
    chunk is written
  - remove single built-in firmware string, add module parameter
    to allow setting default firmware for multiple cards as a
    Bus:Device.Function specific strings (optional)
  - remove polling of non-existing data bits DATA_ENC, DATA_COMP.
    Instead, add module parameter for bitstream specific clock to
    data ratio setting

 Documentation/ABI/testing/sysfs-driver-altera-cvp |   8 +
 drivers/fpga/Kconfig                              |   7 +
 drivers/fpga/Makefile                             |   1 +
 drivers/fpga/altera-cvp.c                         | 508 ++++++++++++++++++++++
 4 files changed, 524 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-altera-cvp
 create mode 100644 drivers/fpga/altera-cvp.c

Comments

kernel test robot May 14, 2017, 7:01 p.m. UTC | #1
Hi Anatolij,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc1 next-20170512]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Anatolij-Gustschin/fpga-manager-Add-Altera-CvP-driver/20170515-000247
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc 

All errors (new ones prefixed by >>):

   drivers//fpga/altera-cvp.c: In function 'altera_cvp_write_init':
>> drivers//fpga/altera-cvp.c:196:21: error: 'FPGA_MGR_COMPRESSED_BITSTREAM' undeclared (first use in this function)
      if (info->flags & FPGA_MGR_COMPRESSED_BITSTREAM)
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers//fpga/altera-cvp.c:196:21: note: each undeclared identifier is reported only once for each function it appears in

vim +/FPGA_MGR_COMPRESSED_BITSTREAM +196 drivers//fpga/altera-cvp.c

   190				return -EINVAL;
   191			}
   192	
   193			if (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM)
   194				conf->numclks = 4; /* ratio for encrypted images */
   195	
 > 196			if (info->flags & FPGA_MGR_COMPRESSED_BITSTREAM)
   197				conf->numclks = 8; /* ratio for all compressed images */
   198		}
   199	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Anatolij Gustschin June 2, 2017, 5:01 p.m. UTC | #2
Hi Alan,

On Sun, 14 May 2017 17:51:22 +0200
Anatolij Gustschin agust@denx.de wrote:

>Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
>and Arria-10 FPGAs via CvP.

any comments to this patch? I'll rebase to apply on top of Altera
PS-SPI driver series, so that it can be queued for merging in v4.13.

Thanks,
Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko June 2, 2017, 5:43 p.m. UTC | #3
On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin <agust@denx.de> wrote:
> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
> and Arria-10 FPGAs via CvP.

Few comments from me.

> +struct altera_cvp_conf {
> +       struct fpga_manager     *mgr;
> +       struct pci_dev          *pci_dev;
> +       void __iomem            *map;

> +       void                    (*write_data)(struct altera_cvp_conf *conf,
> +                                             u32 val);

Is it too far beyond 80 characters? I would leave it in one line (~83
characters are okay).

> +       char                    mgr_name[64];
> +       u8                      numclks;
> +};
> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
> +{
> +       unsigned int i;
> +       u32 val32;

Drop the useless suffix.

> +}

> +static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_msk,
> +                                 u32 status_val, int timeout_us)
> +{
> +       u32 val32;

Ditto.

> +       if (!timeout_us)
> +               return -ETIMEDOUT;

Hmm...
What as a user I would expect here is at least one attempt (0 -- no
timeout, but try once).

> +
> +       do {
> +               /* use small usleep value to re-check and break early */
> +               usleep_range(10, 11);
> +
> +               pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
> +               if ((val32 & status_msk) == status_val)
> +                       return 0;
> +
> +               timeout_us -= 10;
> +       } while (timeout_us > 0);
> +
> +       return -ETIMEDOUT;
> +}

> +static int altera_cvp_teardown(struct fpga_manager *mgr,
> +                              struct fpga_image_info *info)
> +{

> +       u32 val32;

Drop the suffix.

> +       return ret;
> +}

> +static int altera_cvp_write_init(struct fpga_manager *mgr,
> +                                struct fpga_image_info *info,
> +                                const char *buf, size_t count)
> +{

> +       u32 val32;

Ditto.

> +       int ret;
> +

> +       /* clock to data ratio for uncompressed and unencrypted images */
> +       conf->numclks = 1;

To else branch of below?

> +       if (info) {

> +       }

> +               ret = altera_cvp_teardown(mgr, info);
> +               if (ret < 0)
> +                       return ret;

What is the meaning of ret > 0?

Can't it be just if (ret) here?


> +       ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY,
> +                                    VSEC_CVP_STATUS_CFG_RDY, 10);
> +       if (ret < 0) {
> +               dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n");
> +               return ret;
> +       }

Ditto.

Also check another code like this above and below.

> +       return 0;
> +}

> +static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
> +{
> +       struct altera_cvp_conf *conf = mgr->priv;
> +       u32 val32;

Suffix.

> +}

> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
> +                                    struct fpga_image_info *info)
> +{

> +       u32 status_msk;

status_mask

> +       u32 val32;

Drop the suffix.

> +}

> +static ssize_t show_chkcfg(struct device_driver *dev, char *buf)
> +{
> +       return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);

Just altera_cvp_chkcfg.

> +}

> +static struct pci_device_id altera_cvp_id_tbl[] = {
> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },

Does it have dedicated PCI class?

PCI_ANY_ID usually is too broad.

> +static int altera_cvp_probe(struct pci_dev *pdev,
> +                           const struct pci_device_id *dev_id)
> +{
> +       struct altera_cvp_conf *conf;
> +       u16 cmd, val16;

Drop the suffix.

> +       /*
> +        * First check if this is the expected FPGA device. PCI config
> +        * space access works without enabling the pci device, memory

pci -> PCI

> +        * space access is enabled further down.
> +        */

> +       /*
> +        * Enable memory BAR access. We cannot use pci_enable_device() here
> +        * because it will make the driver unusable with FPGA devices that
> +        * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit

iomem -> IOMEM

> +        * platform. Such BARs will not have an assigned address range and
> +        * pci_enable_device() will fail, complaining about not claimed BAR,
> +        * even if the concerned BAR is not needed for FPGA configuration

> +        * at all. Thus, enable the device via pci config space command.

pci -> PCI

> +        */

> +       ret = pci_request_region(pdev, CVP_BAR, "CVP");
> +       if (ret < 0) {

if (ret)

> +               dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
> +               goto err;
> +       }

> +       snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d",
> +                ALTERA_CVP_MGR_NAME,

pdev->bus->number,
> +                PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));

pci_name() ?

> +err_unmap:
> +       pci_iounmap(pdev, conf->map);
> +       pci_release_region(pdev, CVP_BAR);

> +err:

err_disable:

> +       cmd &= ~PCI_COMMAND_MEMORY;
> +       pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +       return ret;
> +}
Andy Shevchenko June 2, 2017, 5:44 p.m. UTC | #4
On Fri, Jun 2, 2017 at 8:43 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin <agust@denx.de> wrote:
>> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
>> and Arria-10 FPGAs via CvP.
>
> Few comments from me.

After addressing them, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Anatolij Gustschin June 7, 2017, 11:09 p.m. UTC | #5
On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko andy.shevchenko@gmail.com wrote:
...
>
>> +       void                    (*write_data)(struct altera_cvp_conf *conf,
>> +                                             u32 val);
>
>Is it too far beyond 80 characters? I would leave it in one line (~83
>characters are okay).

I changed to single line, (*write_data)(struct altera_cvp_conf *, u32);

...
>> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
>> +{
>> +       unsigned int i;
>> +       u32 val32;  
>
>Drop the useless suffix.

ok, done.

>> +       if (!timeout_us)
>> +               return -ETIMEDOUT;  
>
>Hmm...
>What as a user I would expect here is at least one attempt (0 -- no
>timeout, but try once).

yes, this first attempt is above, please see original patch for full
context.

...
>> +       /* clock to data ratio for uncompressed and unencrypted images */
>> +       conf->numclks = 1;  
>
>To else branch of below?
>
>> +       if (info) {  
>
>> +       }

then, the default numclks value would have to be set in the if branch
as well. It is easier to set it before.

...
>> +               ret = altera_cvp_teardown(mgr, info);
>> +               if (ret < 0)
>> +                       return ret;  
>
>What is the meaning of ret > 0?
>
>Can't it be just if (ret) here?

return value > 0 is never used, here it can be if (ret).

...
>> +       ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY,
>> +                                    VSEC_CVP_STATUS_CFG_RDY, 10);
>> +       if (ret < 0) {
>> +               dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n");
>> +               return ret;
>> +       }  
>
>Ditto.
>
>Also check another code like this above and below.

ok, done.

>> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
>> +                                    struct fpga_image_info *info)
>> +{  
>
>> +       u32 status_msk;  
>
>status_mask
>
>> +       u32 val32;  
>
>Drop the suffix.

done.

...
>> +       return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);  
>
>Just altera_cvp_chkcfg.

ok, done.

...
>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },  
>
>Does it have dedicated PCI class?
>
>PCI_ANY_ID usually is too broad.

no, it doesn't have dedicated class.

...
>> +       u16 cmd, val16;  
>
>Drop the suffix.

>> +        * space access works without enabling the pci device, memory  
>
>pci -> PCI

ok, done.

...
>> +        * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit  
>
>iomem -> IOMEM

done.

...
>> +        * at all. Thus, enable the device via pci config space command.  
>
>pci -> PCI

ok.

...
>> +       ret = pci_request_region(pdev, CVP_BAR, "CVP");
>> +       if (ret < 0) {  
>
>if (ret)

ok, done.

...
>> +       snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d",
>> +                ALTERA_CVP_MGR_NAME,  
>
>pdev->bus->number,
>> +                PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));  
>
>pci_name() ?

ok.

>> +err_unmap:
>> +       pci_iounmap(pdev, conf->map);
>> +       pci_release_region(pdev, CVP_BAR);  
>
>> +err:  
>
>err_disable:

done.

Thanks,
Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko June 7, 2017, 11:38 p.m. UTC | #6
On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@denx.de> wrote:
> On Fri, 2 Jun 2017 20:43:21 +0300
> Andy Shevchenko andy.shevchenko@gmail.com wrote:

Besides below comments, please, do

s/VSEC_/VSE_/g

for entire file.

We are following PCI and Thunderbolt pattern for use of Vendor
Specific Extended Capability.

>>> +       if (!timeout_us)
>>> +               return -ETIMEDOUT;
>>
>>Hmm...
>>What as a user I would expect here is at least one attempt (0 -- no
>>timeout, but try once).
>
> yes, this first attempt is above, please see original patch for full
> context.

Ah, it means you don't correctly use do {} while approach.

Remove everything above do { and move usleep after check for the
status inside the loop.

>>> +       /* clock to data ratio for uncompressed and unencrypted images */
>>> +       conf->numclks = 1;
>>
>>To else branch of below?
>>
>>> +       if (info) {
>>
>>> +       }
>
> then, the default numclks value would have to be set in the if branch
> as well. It is easier to set it before.

See the magic below:

        u32 iflags = info ? info->flags : 0;
...
        if (iflags & FPGA_MGR_PARTIAL_RECONFIG) {
                 dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
                 return -EINVAL;
        }

        if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM)
                conf->numclks = 8; /* ratio for all compressed images */
        else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM)
                conf->numclks = 4; /* ratio for encrypted images */
        else
                conf->numclks = 1; /* clock to data ratio for
uncompressed and unencrypted images */

I would really recommend to double check the code every time you are
about to send.
A little thought can make a beauteful result!

>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>>
>>Does it have dedicated PCI class?
>>
>>PCI_ANY_ID usually is too broad.
>
> no, it doesn't have dedicated class.

Hmm... It means any device of this vendor will jump into this
driver... Not good.
Andy Shevchenko June 7, 2017, 11:51 p.m. UTC | #7
On Sun, May 14, 2017 at 10:01 PM, kbuild test robot <lkp@intel.com> wrote:
> Hi Anatolij,
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.12-rc1 next-20170512]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Anatolij-Gustschin/fpga-manager-Add-Altera-CvP-driver/20170515-000247
> config: sparc-allyesconfig (attached as .config)
> compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=sparc
>
> All errors (new ones prefixed by >>):
>
>    drivers//fpga/altera-cvp.c: In function 'altera_cvp_write_init':
>>> drivers//fpga/altera-cvp.c:196:21: error: 'FPGA_MGR_COMPRESSED_BITSTREAM' undeclared (first use in this function)
>       if (info->flags & FPGA_MGR_COMPRESSED_BITSTREAM)
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    drivers//fpga/altera-cvp.c:196:21: note: each undeclared identifier is reported only once for each function it appears in


Anatolij, this is clearly bisectability error (I assume it compiles on
your side, thus means you have more patches on top of it).
Needs to be fixed and supplied along with you driver (I guess it's
FPGA Manager flags in fpga-mgr.h).

>
> vim +/FPGA_MGR_COMPRESSED_BITSTREAM +196 drivers//fpga/altera-cvp.c
>
>    190                          return -EINVAL;
>    191                  }
>    192
>    193                  if (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM)
>    194                          conf->numclks = 4; /* ratio for encrypted images */
>    195
>  > 196                  if (info->flags & FPGA_MGR_COMPRESSED_BITSTREAM)
>    197                          conf->numclks = 8; /* ratio for all compressed images */
>    198          }
>    199
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Anatolij Gustschin June 8, 2017, 2:15 p.m. UTC | #8
On Thu, 8 Jun 2017 02:38:55 +0300
Andy Shevchenko andy.shevchenko@gmail.com wrote:

>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@denx.de> wrote:
>> On Fri, 2 Jun 2017 20:43:21 +0300
>> Andy Shevchenko andy.shevchenko@gmail.com wrote:  
>
>Besides below comments, please, do
>
>s/VSEC_/VSE_/g
>
>for entire file.
>
>We are following PCI and Thunderbolt pattern for use of Vendor
>Specific Extended Capability.

I can do it, but I'm just not getting why. The registers are named as VSEC
registers in the documentation, why should the code name them differently?

...
>>>> +       if (!timeout_us)
>>>> +               return -ETIMEDOUT;  
>>>
>>>Hmm...
>>>What as a user I would expect here is at least one attempt (0 -- no
>>>timeout, but try once).  
>>
>> yes, this first attempt is above, please see original patch for full
>> context.  
>
>Ah, it means you don't correctly use do {} while approach.
>
>Remove everything above do { and move usleep after check for the
>status inside the loop.

Unfortunately, suggested approach has an unwanted side effect:

	do {
		check and return if done;

		usleep_range(10, 11);
		tout -= 10;

	} while (tout > 0);

For simplicity, let's say we were asked to wait with 20 µs timeout.
Assume, that the device reports ready status after 17 µs. The first
check is done, we don't return and sleep approx. 10 µs. Then, the 2nd
check is done and we continue to wait another 10 µs and the loop ends
signalling a timeout. But in the meantime the device reported ready
status. Additional check would be needed after the loop.
In some cases the device reports ready status immediately. That's
the reason why I check first and then loop with more wait&check cycles.

...
>See the magic below:
>
>        u32 iflags = info ? info->flags : 0;
>...
>        if (iflags & FPGA_MGR_PARTIAL_RECONFIG) {
>                 dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
>                 return -EINVAL;
>        }
>
>        if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM)
>                conf->numclks = 8; /* ratio for all compressed images */
>        else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM)
>                conf->numclks = 4; /* ratio for encrypted images */
>        else
>                conf->numclks = 1; /* clock to data ratio for
>uncompressed and unencrypted images */
>
>I would really recommend to double check the code every time you are
>about to send.
>A little thought can make a beauteful result!

ok, changed as suggested.

>>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>>> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },  
>>>
>>>Does it have dedicated PCI class?
>>>
>>>PCI_ANY_ID usually is too broad.  
>>
>> no, it doesn't have dedicated class.  
>
>Hmm... It means any device of this vendor will jump into this
>driver... Not good.

in an early patch version I was asked by Intel people to use PCI_ANY_ID
because these devices are not set in stone. The implemented FPGA PCIe
devices can have varying IDs. probe() checks for expected capability ID
and stops if we hit a not supported device.

Thanks,
Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko June 8, 2017, 2:44 p.m. UTC | #9
On Thu, Jun 8, 2017 at 5:15 PM, Anatolij Gustschin <agust@denx.de> wrote:
> On Thu, 8 Jun 2017 02:38:55 +0300
> Andy Shevchenko andy.shevchenko@gmail.com wrote:
>>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@denx.de> wrote:
>>> On Fri, 2 Jun 2017 20:43:21 +0300
>>> Andy Shevchenko andy.shevchenko@gmail.com wrote:

>>Besides below comments, please, do
>>
>>s/VSEC_/VSE_/g
>>
>>for entire file.
>>
>>We are following PCI and Thunderbolt pattern for use of Vendor
>>Specific Extended Capability.
>
> I can do it, but I'm just not getting why. The registers are named as VSEC
> registers in the documentation, why should the code name them differently?

Does your documentation decode VSEC abbreviation?
What C stands for? Capability?

In PCI and Thunderbolt we agreed to use word capability separately,
so, either XXX_... or XXX_CAP_... to use.

>>>>> +       if (!timeout_us)
>>>>> +               return -ETIMEDOUT;
>>>>
>>>>Hmm...
>>>>What as a user I would expect here is at least one attempt (0 -- no
>>>>timeout, but try once).
>>>
>>> yes, this first attempt is above, please see original patch for full
>>> context.
>>
>>Ah, it means you don't correctly use do {} while approach.
>>
>>Remove everything above do { and move usleep after check for the
>>status inside the loop.
>
> Unfortunately, suggested approach has an unwanted side effect:

How come? See below.

>
>         do {
>                 check and return if done;
>
>                 usleep_range(10, 11);
>                 tout -= 10;
>
>         } while (tout > 0);
>
> For simplicity, let's say we were asked to wait with 20 µs timeout.
> Assume, that the device reports ready status after 17 µs. The first
> check is done, we don't return and sleep approx. 10 µs. Then, the 2nd
> check is done and we continue to wait another 10 µs and the loop ends
> signalling a timeout. But in the meantime the device reported ready
> status. Additional check would be needed after the loop.
> In some cases the device reports ready status immediately. That's
> the reason why I check first and then loop with more wait&check cycles.

Just look to the rest of the code in kernel
Most of the timeout related loops we have the following pattern:

unsigned int retries = XXX;

do {
...check for something...
if (yes)
  return YY;

...sleep for a while...
} while (--retries);
if (!retries)
 return -ETIMEDOUT;

What I'm suggesting is to follow the pattern (adjust it for your exact
conditions and so on).

>>>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>>>> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>>>>
>>>>Does it have dedicated PCI class?
>>>>
>>>>PCI_ANY_ID usually is too broad.
>>>
>>> no, it doesn't have dedicated class.
>>
>>Hmm... It means any device of this vendor will jump into this
>>driver... Not good.
>
> in an early patch version I was asked by Intel people to use PCI_ANY_ID
> because these devices are not set in stone. The implemented FPGA PCIe
> devices can have varying IDs. probe() checks for expected capability ID
> and stops if we hit a not supported device.

Yeah, the problem is that every device with a such Vendor ID would be
considered by this driver and PCI class would be helpful here just to
reduce an impact.
Capability approach works, though it's slightly more error prone.

I have no other comment on this. For now it seems the only choice
since such IPs are on the market, right?
Anatolij Gustschin June 8, 2017, 3 p.m. UTC | #10
On Thu, 8 Jun 2017 17:44:19 +0300
Andy Shevchenko andy.shevchenko@gmail.com wrote:

>On Thu, Jun 8, 2017 at 5:15 PM, Anatolij Gustschin <agust@denx.de> wrote:
>> On Thu, 8 Jun 2017 02:38:55 +0300
>> Andy Shevchenko andy.shevchenko@gmail.com wrote:  
>>>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@denx.de> wrote:  
>>>> On Fri, 2 Jun 2017 20:43:21 +0300
>>>> Andy Shevchenko andy.shevchenko@gmail.com wrote:  
>
>>>Besides below comments, please, do
>>>
>>>s/VSEC_/VSE_/g
>>>
>>>for entire file.
>>>
>>>We are following PCI and Thunderbolt pattern for use of Vendor
>>>Specific Extended Capability.  
>>
>> I can do it, but I'm just not getting why. The registers are named as VSEC
>> registers in the documentation, why should the code name them differently?  
>
>Does your documentation decode VSEC abbreviation?
>What C stands for? Capability?

the CvP user guide talks about Vendor Specific Extended Capability (VSEC).

--
Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/ABI/testing/sysfs-driver-altera-cvp b/Documentation/ABI/testing/sysfs-driver-altera-cvp
new file mode 100644
index 0000000..8cde64a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-altera-cvp
@@ -0,0 +1,8 @@ 
+What:		/sys/bus/pci/drivers/altera-cvp/chkcfg
+Date:		May 2017
+Kernel Version:	4.13
+Contact:	Anatolij Gustschin <agust@denx.de>
+Description:
+		Contains either 1 or 0 and controls if configuration
+		error checking in altera-cvp driver is turned on or
+		off.
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 161ba9d..895529e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -26,6 +26,13 @@  config FPGA_MGR_ICE40_SPI
 	help
 	  FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
 
+config FPGA_MGR_ALTERA_CVP
+	tristate "Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager"
+	depends on PCI
+	help
+	  FPGA manager driver support for Arria-V, Cyclone-V, Stratix-V
+	  and Arria 10 Altera FPGAs using the CvP interface over PCIe.
+
 config FPGA_MGR_SOCFPGA
 	tristate "Altera SOCFPGA FPGA Manager"
 	depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 2a4f021..2e5c8b6 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -6,6 +6,7 @@ 
 obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 
 # FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)	+= altera-cvp.o
 obj-$(CONFIG_FPGA_MGR_ICE40_SPI)	+= ice40-spi.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
new file mode 100644
index 0000000..e60a414
--- /dev/null
+++ b/drivers/fpga/altera-cvp.c
@@ -0,0 +1,508 @@ 
+/*
+ * FPGA Manager Driver for Altera Arria/Cyclone/Stratix CvP
+ *
+ * Copyright (C) 2017 DENX Software Engineering
+ *
+ * Anatolij Gustschin <agust@denx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Manage Altera FPGA firmware using PCIe CvP.
+ * Firmware must be in binary "rbf" format.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/sizes.h>
+
+#define CVP_BAR		0	/* BAR used for data transfer in memory mode */
+#define CVP_DUMMY_WR	244	/* dummy writes to clear CvP state machine */
+#define TIMEOUT_US	2000	/* CVP STATUS timeout for USERMODE polling */
+
+/* Vendor Specific Extended Capability Registers */
+#define VSEC_PCIE_EXT_CAP_ID		0x200
+#define VSEC_PCIE_EXT_CAP_ID_VAL	0x000b		/* 16bit */
+
+#define VSEC_CVP_STATUS			0x21c	/* 32bit */
+#define VSEC_CVP_STATUS_CFG_RDY		BIT(18)	/* CVP_CONFIG_READY */
+#define VSEC_CVP_STATUS_CFG_ERR		BIT(19)	/* CVP_CONFIG_ERROR */
+#define VSEC_CVP_STATUS_CVP_EN		BIT(20)	/* ctrl block is enabling CVP */
+#define VSEC_CVP_STATUS_USERMODE	BIT(21)	/* USERMODE */
+#define VSEC_CVP_STATUS_CFG_DONE	BIT(23)	/* CVP_CONFIG_DONE */
+#define VSEC_CVP_STATUS_PLD_CLK_IN_USE	BIT(24)	/* PLD_CLK_IN_USE */
+
+#define VSEC_CVP_MODE_CTRL		0x220	/* 32bit */
+#define VSEC_CVP_MODE_CTRL_CVP_MODE	BIT(0)	/* CVP (1) or normal mode (0) */
+#define VSEC_CVP_MODE_CTRL_HIP_CLK_SEL	BIT(1) /* PMA (1) or fabric clock (0) */
+#define VSEC_CVP_MODE_CTRL_NUMCLKS_OFF	8	/* NUMCLKS bits offset */
+#define VSEC_CVP_MODE_CTRL_NUMCLKS_MASK	GENMASK(15, 8)
+
+#define VSEC_CVP_DATA			0x228	/* 32bit */
+#define VSEC_CVP_PROG_CTRL		0x22c	/* 32bit */
+#define VSEC_CVP_PROG_CTRL_CONFIG	BIT(0)
+#define VSEC_CVP_PROG_CTRL_START_XFER	BIT(1)
+
+#define VSEC_UNCOR_ERR_STATUS		0x234	/* 32bit */
+#define	VSEC_UNCOR_ERR_CVP_CFG_ERR	BIT(5)	/* CVP_CONFIG_ERROR_LATCHED */
+
+#define	DRV_NAME		"altera-cvp"
+#define ALTERA_CVP_MGR_NAME	"Altera CvP FPGA Manager"
+
+/* Optional CvP config error status check for debugging */
+static bool altera_cvp_chkcfg;
+
+struct altera_cvp_conf {
+	struct fpga_manager	*mgr;
+	struct pci_dev		*pci_dev;
+	void __iomem		*map;
+	void			(*write_data)(struct altera_cvp_conf *conf,
+					      u32 val);
+	char			mgr_name[64];
+	u8			numclks;
+};
+
+static enum fpga_mgr_states altera_cvp_state(struct fpga_manager *mgr)
+{
+	struct altera_cvp_conf *conf = mgr->priv;
+	u32 status;
+
+	pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &status);
+
+	if (status & VSEC_CVP_STATUS_CFG_DONE)
+		return FPGA_MGR_STATE_OPERATING;
+
+	if (status & VSEC_CVP_STATUS_CVP_EN)
+		return FPGA_MGR_STATE_POWER_UP;
+
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static void altera_cvp_write_data_iomem(struct altera_cvp_conf *conf, u32 val)
+{
+	writel(val, conf->map);
+}
+
+static void altera_cvp_write_data_config(struct altera_cvp_conf *conf, u32 val)
+{
+	pci_write_config_dword(conf->pci_dev, VSEC_CVP_DATA, val);
+}
+
+/* switches between CvP clock and internal clock */
+static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
+{
+	unsigned int i;
+	u32 val32;
+
+	/* set 1 CVP clock cycle for every CVP Data Register Write */
+	pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS_MASK;
+	val32 |= 1 << VSEC_CVP_MODE_CTRL_NUMCLKS_OFF;
+	pci_write_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, val32);
+
+	for (i = 0; i < CVP_DUMMY_WR; i++)
+		conf->write_data(conf, 0); /* dummy data, could be any value */
+}
+
+static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_msk,
+				  u32 status_val, int timeout_us)
+{
+	u32 val32;
+
+	pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
+	if ((val32 & status_msk) == status_val)
+		return 0;
+
+	if (!timeout_us)
+		return -ETIMEDOUT;
+
+	do {
+		/* use small usleep value to re-check and break early */
+		usleep_range(10, 11);
+
+		pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
+		if ((val32 & status_msk) == status_val)
+			return 0;
+
+		timeout_us -= 10;
+	} while (timeout_us > 0);
+
+	return -ETIMEDOUT;
+}
+
+static int altera_cvp_teardown(struct fpga_manager *mgr,
+			       struct fpga_image_info *info)
+{
+	struct altera_cvp_conf *conf = mgr->priv;
+	struct pci_dev *pdev = conf->pci_dev;
+	int ret;
+	u32 val32;
+
+	/* STEP 12 - reset START_XFER bit */
+	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
+	val32 &= ~VSEC_CVP_PROG_CTRL_START_XFER;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/* STEP 13 - reset CVP_CONFIG bit */
+	val32 &= ~VSEC_CVP_PROG_CTRL_CONFIG;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/*
+	 * STEP 14
+	 * - set CVP_NUMCLKS to 1 and then issue CVP_DUMMY_WR dummy
+	 *   writes to the HIP
+	 */
+	altera_cvp_dummy_write(conf); /* from CVP clock to internal clock */
+
+	/* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */
+	ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY, 0, 10);
+	if (ret < 0)
+		dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n");
+
+	return ret;
+}
+
+static int altera_cvp_write_init(struct fpga_manager *mgr,
+				 struct fpga_image_info *info,
+				 const char *buf, size_t count)
+{
+	struct altera_cvp_conf *conf = mgr->priv;
+	struct pci_dev *pdev = conf->pci_dev;
+	u32 val32;
+	int ret;
+
+	/* clock to data ratio for uncompressed and unencrypted images */
+	conf->numclks = 1;
+
+	if (info) {
+		if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+			dev_err(&mgr->dev,
+				"Partial reconfiguration not supported.\n");
+			return -EINVAL;
+		}
+
+		if (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM)
+			conf->numclks = 4; /* ratio for encrypted images */
+
+		if (info->flags & FPGA_MGR_COMPRESSED_BITSTREAM)
+			conf->numclks = 8; /* ratio for all compressed images */
+	}
+
+	/* STEP 1 - read CVP status and check CVP_EN flag */
+	pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
+	if (!(val32 & VSEC_CVP_STATUS_CVP_EN)) {
+		dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val32);
+		return -ENODEV;
+	}
+
+	if (val32 & VSEC_CVP_STATUS_CFG_RDY) {
+		dev_warn(&mgr->dev, "CvP already started, teardown first\n");
+		ret = altera_cvp_teardown(mgr, info);
+		if (ret < 0)
+			return ret;
+	}
+
+	/*
+	 * STEP 2
+	 * - set HIP_CLK_SEL and CVP_MODE (must be set in the order mentioned)
+	 */
+	/* switch from fabric to PMA clock */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 |= VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/* set CVP mode */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 |= VSEC_CVP_MODE_CTRL_CVP_MODE;
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/*
+	 * STEP 3
+	 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP
+	 */
+	altera_cvp_dummy_write(conf);
+
+	/* STEP 4 - set CVP_CONFIG bit */
+	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
+	/* request control block to begin transfer using CVP */
+	val32 |= VSEC_CVP_PROG_CTRL_CONFIG;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/* STEP 5 - poll CVP_CONFIG READY for 1 with 10us timeout */
+	ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY,
+				     VSEC_CVP_STATUS_CFG_RDY, 10);
+	if (ret < 0) {
+		dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n");
+		return ret;
+	}
+
+	/*
+	 * STEP 6
+	 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP
+	 */
+	altera_cvp_dummy_write(conf);
+
+	/* STEP 7 - set START_XFER */
+	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
+	val32 |= VSEC_CVP_PROG_CTRL_START_XFER;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/* STEP 8 - start transfer (set CVP_NUMCLKS for bitstream) */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS_MASK;
+	val32 |= conf->numclks << VSEC_CVP_MODE_CTRL_NUMCLKS_OFF;
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	return 0;
+}
+
+static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
+{
+	struct altera_cvp_conf *conf = mgr->priv;
+	u32 val32;
+
+	/* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */
+	pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
+	if (val32 & VSEC_CVP_STATUS_CFG_ERR) {
+		dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n",
+			bytes);
+		return -EPROTO;
+	}
+	return 0;
+}
+
+static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
+			    size_t count)
+{
+	struct altera_cvp_conf *conf = mgr->priv;
+	const u32 *data;
+	size_t done, remaining;
+	int status = 0;
+	u32 mask;
+
+	/* STEP 9 - write 32-bit data from RBF file to CVP data register */
+	data = (u32 *)buf;
+	remaining = count;
+	done = 0;
+
+	while (remaining >= 4) {
+		conf->write_data(conf, *data++);
+		done += 4;
+		remaining -= 4;
+
+		/*
+		 * STEP 10 (optional) and STEP 11
+		 * - check error flag
+		 * - loop until data transfer completed
+		 * Config images can be huge (more than 40 MiB), so
+		 * only check after a new 4k data block has been written.
+		 * This reduces the number of checks and speeds up the
+		 * configuration process.
+		 */
+		if (altera_cvp_chkcfg && !(done % SZ_4K)) {
+			status = altera_cvp_chk_error(mgr, done);
+			if (status < 0)
+				return status;
+		}
+	}
+
+	/* write up to 3 trailing bytes, if any */
+	mask = BIT(remaining * 8) - 1;
+	if (mask)
+		conf->write_data(conf, *data & mask);
+
+	if (altera_cvp_chkcfg)
+		status = altera_cvp_chk_error(mgr, count);
+
+	return status;
+}
+
+static int altera_cvp_write_complete(struct fpga_manager *mgr,
+				     struct fpga_image_info *info)
+{
+	struct altera_cvp_conf *conf = mgr->priv;
+	struct pci_dev *pdev = conf->pci_dev;
+	int ret;
+	u32 status_msk;
+	u32 val32;
+
+	ret = altera_cvp_teardown(mgr, info);
+	if (ret < 0)
+		return ret;
+
+	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
+	pci_read_config_dword(pdev, VSEC_UNCOR_ERR_STATUS, &val32);
+	if (val32 & VSEC_UNCOR_ERR_CVP_CFG_ERR) {
+		dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n");
+		return -EPROTO;
+	}
+
+	/* STEP 17 - reset CVP_MODE and HIP_CLK_SEL bit */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
+	val32 &= ~VSEC_CVP_MODE_CTRL_CVP_MODE;
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/* STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits */
+	status_msk = VSEC_CVP_STATUS_PLD_CLK_IN_USE | VSEC_CVP_STATUS_USERMODE;
+	ret = altera_cvp_wait_status(conf, status_msk, status_msk, TIMEOUT_US);
+	if (ret < 0)
+		dev_err(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
+
+	return ret;
+}
+
+static const struct fpga_manager_ops altera_cvp_ops = {
+	.state		= altera_cvp_state,
+	.write_init	= altera_cvp_write_init,
+	.write		= altera_cvp_write,
+	.write_complete	= altera_cvp_write_complete,
+};
+
+static ssize_t show_chkcfg(struct device_driver *dev, char *buf)
+{
+	return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);
+}
+
+static ssize_t store_chkcfg(struct device_driver *drv, const char *buf,
+			    size_t count)
+{
+	int ret;
+
+	ret = kstrtobool(buf, &altera_cvp_chkcfg);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static DRIVER_ATTR(chkcfg, 0600, show_chkcfg, store_chkcfg);
+
+static int altera_cvp_probe(struct pci_dev *pdev,
+			    const struct pci_device_id *dev_id);
+static void altera_cvp_remove(struct pci_dev *pdev);
+
+#define PCI_VENDOR_ID_ALTERA	0x1172
+
+static struct pci_device_id altera_cvp_id_tbl[] = {
+	{ PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, altera_cvp_id_tbl);
+
+static struct pci_driver altera_cvp_driver = {
+	.name   = DRV_NAME,
+	.id_table = altera_cvp_id_tbl,
+	.probe  = altera_cvp_probe,
+	.remove = altera_cvp_remove,
+};
+
+static int altera_cvp_probe(struct pci_dev *pdev,
+			    const struct pci_device_id *dev_id)
+{
+	struct altera_cvp_conf *conf;
+	u16 cmd, val16;
+	int ret;
+
+	/*
+	 * First check if this is the expected FPGA device. PCI config
+	 * space access works without enabling the pci device, memory
+	 * space access is enabled further down.
+	 */
+	pci_read_config_word(pdev, VSEC_PCIE_EXT_CAP_ID, &val16);
+	if (val16 != VSEC_PCIE_EXT_CAP_ID_VAL) {
+		dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val16);
+		return -ENODEV;
+	}
+
+	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
+	if (!conf)
+		return -ENOMEM;
+
+	/*
+	 * Enable memory BAR access. We cannot use pci_enable_device() here
+	 * because it will make the driver unusable with FPGA devices that
+	 * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit
+	 * platform. Such BARs will not have an assigned address range and
+	 * pci_enable_device() will fail, complaining about not claimed BAR,
+	 * even if the concerned BAR is not needed for FPGA configuration
+	 * at all. Thus, enable the device via pci config space command.
+	 */
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (!(cmd & PCI_COMMAND_MEMORY)) {
+		cmd |= PCI_COMMAND_MEMORY;
+		pci_write_config_word(pdev, PCI_COMMAND, cmd);
+	}
+
+	ret = pci_request_region(pdev, CVP_BAR, "CVP");
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
+		goto err;
+	}
+
+	conf->pci_dev = pdev;
+	conf->write_data = altera_cvp_write_data_iomem;
+
+	conf->map = pci_iomap(pdev, CVP_BAR, 0);
+	if (!conf->map) {
+		dev_warn(&pdev->dev, "Mapping CVP BAR failed\n");
+		conf->write_data = altera_cvp_write_data_config;
+	}
+
+	snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d",
+		 ALTERA_CVP_MGR_NAME, pdev->bus->number,
+		 PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+	ret = fpga_mgr_register(&pdev->dev, conf->mgr_name,
+				&altera_cvp_ops, conf);
+	if (ret)
+		goto err_unmap;
+
+	ret = driver_create_file(&altera_cvp_driver.driver,
+				 &driver_attr_chkcfg);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't create sysfs chkcfg file\n");
+		fpga_mgr_unregister(&pdev->dev);
+		goto err_unmap;
+	}
+
+	return 0;
+
+err_unmap:
+	pci_iounmap(pdev, conf->map);
+	pci_release_region(pdev, CVP_BAR);
+err:
+	cmd &= ~PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+	return ret;
+}
+
+static void altera_cvp_remove(struct pci_dev *pdev)
+{
+	struct fpga_manager *mgr = pci_get_drvdata(pdev);
+	struct altera_cvp_conf *conf = mgr->priv;
+	u16 cmd;
+
+	driver_remove_file(&altera_cvp_driver.driver, &driver_attr_chkcfg);
+	fpga_mgr_unregister(&pdev->dev);
+	pci_iounmap(pdev, conf->map);
+	pci_release_region(pdev, CVP_BAR);
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	cmd &= ~PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+}
+
+module_pci_driver(altera_cvp_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Anatolij Gustschin <agust@denx.de>");
+MODULE_DESCRIPTION("Module to load Altera FPGA over CvP");