fpga manager: Add Altera CvP driver
PATCH v5 1/1 fpga manager: Add Altera CvP driver
Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V and Arria-10 FPGAs via CvP.
---
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
Documentation/ABI/testing/sysfs-driver-altera-cvp new file +8 -0
@@ -0,0 +1,8 @@ +What: /sys/bus/pci/drivers/altera-cvp/chkcfg +Date: May 2017 +Kernel Version: 4.13 +Contact: Anatolij Gustschin <[email protected]> +Description: + Contains either 1 or 0 and controls if configuration + error checking in altera-cvp driver is turned on or + off.
drivers/fpga/Kconfig +7 -0
@@ -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
drivers/fpga/Makefile +1 -0
@@ -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
drivers/fpga/altera-cvp.c new file +508 -0
508 lines not on this page · raw message
Signature · 1 line
2.7.4
kbuild test robot
build boturl: 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…
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
Quoted 15 lines · kbuild test robot
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
Quoted 7 lines
All errors (new ones prefixed by >>):
drivers//fpga/altera-cvp.c: In function 'altera_cvp_write_init':Quoted 1 line
Quoted 1 line
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).
Quoted 17 lines
vim +/FPGA_MGR_COMPRESSED_BITSTREAM +196
drivers//fpga/altera-cvp.c190 return -EINVAL; 191 } 192 193 if (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM) 194 conf->numclks = 4; /* ratio for encrypted images */ 195Quoted 1 line
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
Signature · 2 lines
With Best Regards, Andy Shevchenko
Hi Alan,
Quoted 2 lines · On Sun, 14 May 2017 17:51:22 +0200 Anatolij Gustschin [email protected] 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
Quoted 2 lines · Anatolij Gustschin
Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V and Arria-10 FPGAs via CvP.
Few comments from me.
drivers/fpga/altera-cvp.c line 69
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +6
-0+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).
drivers/fpga/altera-cvp.c line 104
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +7
-0+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.
drivers/fpga/altera-cvp.c line 119
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+} +static int altera_cvp_wait_status (struct altera_cvp_conf *conf,u32 status_msk, +u32 status_val,int timeout_us) +{ +u32 val32;
Ditto.
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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).
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +10
-0+ + 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;
drivers/fpga/altera-cvp.c line 148
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +8
-0+ }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.
drivers/fpga/altera-cvp.c line 180
Quoted 9 lines
drivers/fpga/altera-cvp.c new file +7
-0+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.
drivers/fpga/altera-cvp.c line 184
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+int ret; + +/* clock to data ratio for uncompressed and unencrypted images */ + conf->numclks =1 ;
To else branch of below?
drivers/fpga/altera-cvp.c line 211
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +5
-0+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?
drivers/fpga/altera-cvp.c line 246
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +6
-0+ 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.
drivers/fpga/altera-cvp.c line 271
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +6
-0+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.
drivers/fpga/altera-cvp.c line 335
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +5
-0+} +static int altera_cvp_write_complete (struct fpga_manager *mgr, +struct fpga_image_info *info) +{ +u32 status_msk;
status_mask
Quoted 1 line
+ u32 val32;
Drop the suffix.
drivers/fpga/altera-cvp.c line 373
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+} +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.
drivers/fpga/altera-cvp.c line 397
Quoted 4 lines
drivers/fpga/altera-cvp.c new file +3
-0+} +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.
drivers/fpga/altera-cvp.c line 413
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +5
-0+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.
drivers/fpga/altera-cvp.c line 418
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +3
-0+/* +* First check if this is the expected FPGA device. PCI config +* space access works without enabling the pci device, memory
pci -> PCI
drivers/fpga/altera-cvp.c line 434
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +6
-0+* 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
drivers/fpga/altera-cvp.c line 438
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+* 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
drivers/fpga/altera-cvp.c line 447
Quoted 4 lines
drivers/fpga/altera-cvp.c new file +3
-0+*/ + ret =pci_request_region (pdev,CVP_BAR ,"CVP" ); +if (ret <0 ) {
if (ret)
drivers/fpga/altera-cvp.c
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+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,
Quoted 1 line
+
PCI_SLOT(pdev->devfn),PCI_FUNC(pdev->devfn));
pci_name() ?
drivers/fpga/altera-cvp.c line 483
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+err_unmap: +pci_iounmap (pdev, conf->map); +pci_release_region (pdev,CVP_BAR ); +err:
err_disable:
Quoted 4 lines
drivers/fpga/altera-cvp.c new file +4
-0+ cmd &= ~PCI_COMMAND_MEMORY ; +pci_write_config_word (pdev,PCI_COMMAND , cmd); +return ret; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Andy Shevchenko
Reviewed-byQuoted 5 lines · Andy Shevchenko
Quoted 2 lines · Anatolij Gustschin
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,
Signature · 2 lines
With Best Regards, Andy Shevchenko
On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko [email protected] wrote:
...
Quoted 6 lines
drivers/fpga/altera-cvp.c line 69
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +2
-0+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);
...
Quoted 6 lines
drivers/fpga/altera-cvp.c line 104
Quoted 4 lines
drivers/fpga/altera-cvp.c new file +4
-0+static void altera_cvp_dummy_write (struct altera_cvp_conf *conf) +{ +unsigned int i; +u32 val32;Drop the useless suffix.
ok, done.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
...
Quoted 8 lines
drivers/fpga/altera-cvp.c line 184
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+/* clock to data ratio for uncompressed and unencrypted images */ + conf->numclks =1 ;To else branch of below?
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +2
-0+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.
...
Quoted 7 lines
drivers/fpga/altera-cvp.c line 211
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +3
-0+ 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).
...
Quoted 10 lines
drivers/fpga/altera-cvp.c line 246
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +6
-0+ 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.
Quoted 11 lines
drivers/fpga/altera-cvp.c line 335
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+static int altera_cvp_write_complete (struct fpga_manager *mgr, +struct fpga_image_info *info) +{ +u32 status_msk;
status_maskQuoted 1 line
+ u32 val32;
Drop the suffix.
done.
...
Quoted 3 lines
Quoted 1 line
+ return
snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);Just
altera_cvp_chkcfg.
ok, done.
...
Quoted 6 lines
drivers/fpga/altera-cvp.c line 397
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+static struct pci_device_id altera_cvp_id_tbl[] = { + {PCI_VDEVICE (ALTERA ,PCI_ANY_ID ) },Does it have dedicated PCI class?
PCI_ANY_IDusually is too broad.
no, it doesn't have dedicated class.
...
Quoted 7 lines
Quoted 1 line
+ u16 cmd, val16;
Drop the suffix.
Quoted 1 line
+ * space access works without enabling the pci device, memory
pci -> PCI
ok, done.
...
Quoted 3 lines
Quoted 1 line
+ * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit
iomem -> IOMEM
done.
...
Quoted 3 lines
Quoted 1 line
+ * at all. Thus, enable the device via pci config space command.
pci -> PCI
ok.
...
Quoted 4 lines
drivers/fpga/altera-cvp.c line 447
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+ ret =pci_request_region (pdev,CVP_BAR ,"CVP" ); +if (ret <0 ) {if (ret)
ok, done.
...
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d", + ALTERA_CVP_MGR_NAME,
pdev->bus->number,Quoted 1 line
+
PCI_SLOT(pdev->devfn),PCI_FUNC(pdev->devfn));
pci_name()?
ok.
Quoted 7 lines
drivers/fpga/altera-cvp.c line 483
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+err_unmap: +pci_iounmap (pdev, conf->map); +pci_release_region (pdev,CVP_BAR ); +err:
err_disable:
done.
Thanks,
Anatolij
Quoted 2 lines · Anatolij Gustschin
On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko [email protected] 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.
Quoted 9 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 11 lines
Quoted 8 lines
drivers/fpga/altera-cvp.c line 184
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+/* clock to data ratio for uncompressed and unencrypted images */ + conf->numclks =1 ;To else branch of below?
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +2
-0+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 foruncompressed 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!
Quoted 8 lines
Quoted 6 lines
drivers/fpga/altera-cvp.c line 397
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+static struct pci_device_id altera_cvp_id_tbl[] = { + {PCI_VDEVICE (ALTERA ,PCI_ANY_ID ) },Does it have dedicated PCI class?
PCI_ANY_IDusually 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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 4 lines · On Thu, 8 Jun 2017 02:38:55 +0300 Andy Shevchenko [email protected]…
Quoted 2 lines · Anatolij Gustschin
On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko [email protected] wrote:
Quoted 8 lines
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?
...
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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).
Quoted 7 lines
Quoted 2 lines
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.
...
Quoted 15 lines
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
Quoted 5 lines
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.
Quoted 11 lines
Quoted 8 lines
Quoted 6 lines
drivers/fpga/altera-cvp.c line 397
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+static struct pci_device_id altera_cvp_id_tbl[] = { + {PCI_VDEVICE (ALTERA ,PCI_ANY_ID ) },Does it have dedicated PCI class?
PCI_ANY_IDusually 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
Quoted 10 lines · Anatolij Gustschin
Quoted 8 lines · On Thu, 8 Jun 2017 02:38:55 +0300 Andy Shevchenko [email protected]…
Quoted 2 lines · Anatolij Gustschin
On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko [email protected] wrote:Besides below comments, please, do
s/VSEC_/VSE_/g
Quoted 7 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 9 lines
do {
check and return if done;
usleep_range(10, 11);
tout -= 10;
} while (tout > 0);Quoted 8 lines
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).
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines
drivers/fpga/altera-cvp.c line 397
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+static struct pci_device_id altera_cvp_id_tbl[] = { + {PCI_VDEVICE (ALTERA ,PCI_ANY_ID ) },Does it have dedicated PCI class?
PCI_ANY_IDusually is too broad.no, it doesn't have dedicated class.
Quoted 7 lines
Quoted 2 lines
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_IDbecause 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?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 13 lines · On Thu, 8 Jun 2017 17:44:19 +0300 Andy Shevchenko [email protected]…
Quoted 12 lines · Anatolij Gustschin
Quoted 10 lines · On Thu, 8 Jun 2017 02:38:55 +0300 Andy Shevchenko [email protected]…
Quoted 2 lines · Anatolij Gustschin
On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko [email protected] wrote:Besides below comments, please, do
s/VSEC_/VSE_/g
for entire file.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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