fpga manager: Add Altera CvP driver
PATCH v4 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.
---
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
drivers/fpga/Kconfig | 7 + drivers/fpga/Makefile | 1 + drivers/fpga/altera-cvp.c | 549 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 557 insertions(+) create mode 100644 drivers/fpga/altera-cvp.c
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 +549 -0
549 lines not on this page · raw message
Signature · 1 line
2.7.4
On Sun, Apr 30, 2017 at 2:08 PM, Anatolij Gustschin <[email protected]> wrote:
Hi Anatolij,
Looks good!
Quoted 4 lines
Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V and Arria-10 FPGAs via CvP.
Signed-off-by Anatolij Gustschin <[email protected]>
Quoted 2 lines · Anatolij Gustschin
Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V and Arria-10 FPGAs via CvP.
I think you need to spend time on polishing such code.
See my comments below.
drivers/fpga/altera-cvp.c line 36
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +8
-0+#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_IN_US 2000 + +/* Vendor Specific Extended Capability Offset */ +#define VSEC_OFFSET 0x200 +#define VSEC_PCIE_EXT_CAP_ID_VAL 0x000b +#define VSEC_PCIE_EXT_CAP_ID (VSEC_OFFSET + 0x00)/* 16bit */
It is not clear what is what. If the above is value, why it's located under offset "section"?
Quoted 1 line
+#define
VSEC_CVP_STATUS(VSEC_OFFSET+0x1c) /* 32bit */
Usually in the drivers we use just absolute value.
drivers/fpga/altera-cvp.c line 58
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 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_FULL_CFG BIT(2)/* CVP_FULLCONFIG */ +#define VSEC_CVP_MODE_CTRL_NUMCLKS (0xff<<8)/* CVP_NUMCLKS */
Is 0xff a mask here? (Btw, you missed spaces around <<)
drivers/fpga/altera-cvp.c line 78
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +7
-0+static int chkcfg;/* use value 1 for debugging only */ +module_param (chkcfg, int,0664 ); +MODULE_PARM_DESC (chkcfg,"1 - check CvP status, 0 - skip checking (default 0)" ); + +static int numclks =1 ;/* default 1 for uncompressed and unencrypted */ +module_param (numclks, int,0664 ); +MODULE_PARM_DESC (numclks,"Clock to data ratio 1, 4 or 8 (default 1)" );
Why do we need that?!
In new drivers we try to avoid new module parameters. We have enough interfaces nowadays to let driver know some details (quirks).
Quoted 13 lines
drivers/fpga/altera-cvp.c new file +12
-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); +char mgr_name[64 ]; +}; + +static inline void altera_cvp_chk_numclks (void ) +{
drivers/fpga/altera-cvp.c line 100
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +10
-0+switch (numclks) { +case 1 : +case 4 : +case 8 : +break ; +default : + numclks =1 ; +break ; + } +}
Why 2 is missed? Hardware limitation? Needs a comment about these magics.
drivers/fpga/altera-cvp.c line 131
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+static void altera_cvp_dummy_write (struct altera_cvp_conf *conf) +{ +u32 val32; +int i;
unsigned int i; ?
drivers/fpga/altera-cvp.c line 136
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+ +/* set number of CVP clock cycles for every CVP Data Register Write */ +pci_read_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , &val32); + val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS ; + val32 |=0x01 <<8 ;/* 1 clock */
Yeah, needs more clear way to put clocks of choice.
drivers/fpga/altera-cvp.c line 140
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+pci_write_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , val32); + +for (i =0 ; i <CVP_DUMMY_WR ; i++) + conf->write_data (conf,0xdeadbeef );
Why this dummy is chosen?
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+} + +static int altera_cvp_teardown (struct fpga_manager *mgr, +struct fpga_image_info *info) +{
drivers/fpga/altera-cvp.c line 154
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +9
-0+struct altera_cvp_conf *conf = mgr->priv; +struct pci_dev *pdev = conf->pci_dev; +int status =0 ; +int delay_us; +u32 val32; + +/* +* STEP 12 - reset START_XFER bit +*/
One line?
(as well below)
drivers/fpga/altera-cvp.c line 176
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +6
-0+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); + + delay_us =0 ; +while (1 ) {
Oh, no. It's a red flag.
And better to do
do {} while (--retries);
style. It will on smallest glance give reader a clue that the body will go at least once.
drivers/fpga/altera-cvp.c line 181
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 &VSEC_CVP_STATUS_CFG_RDY ) ==0 ) +break ; + +udelay (1 );/* wait 1us */
Why not 10? Needs a comment.
Quoted 15 lines
drivers/fpga/altera-cvp.c new file +14
-0+ +if (delay_us++ >TIMEOUT_IN_US ) { +dev_warn (&mgr->dev,"CVP_CONFIG_READY == 0 timeout\n" ); + status = -ETIMEDOUT ; +break ; + } + } + +return status; +} + +static int altera_cvp_write_init (struct fpga_manager *mgr, +struct fpga_image_info *info, +const char *buf,size_t count)
drivers/fpga/altera-cvp.c line 210
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +9
-0+{ +struct altera_cvp_conf *conf = mgr->priv; +struct pci_dev *pdev = conf->pci_dev; +int delay_us; +u32 val32; +int ret; +/* +* STEP 1 - read CVP status and check CVP_EN flag +*/
Ditto about comments.
Review
Quoted 2 lines
(quoted diff) +2
-0+ delay_us = 0; + while (1) {
Ditto about loop style.
Quoted 17 lines
drivers/fpga/altera-cvp.c new file +15
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 &VSEC_CVP_STATUS_CFG_RDY ) == +VSEC_CVP_STATUS_CFG_RDY ) +break ; + +udelay (1 );/* wait 1us */ + +if (delay_us++ >TIMEOUT_IN_US ) { +dev_warn (&mgr->dev,"CVP_CONFIG_READY == 1 timeout\n" ); +return -ETIMEDOUT ; + } + } +return 0 ; +} +
drivers/fpga/altera-cvp.c line 306
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +8
-0+static inline int altera_cvp_chk_error (struct fpga_manager *mgr,size_t bytes) +{ +struct altera_cvp_conf *conf = mgr->priv; +u32 val32; +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 %zi bytes!\n" , + bytes);
%zu?
Quoted 20 lines
drivers/fpga/altera-cvp.c new file +18
-0+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 bytes; +int status =0 ; +u32 mask; + + data = (u32 *)buf; + bytes = count; + +while (bytes >=4 ) {
drivers/fpga/altera-cvp.c line 337
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +9
-0+ conf->write_data (conf, *data++); + bytes -=4 ; + +/* +* STEP 10 (optional) and STEP 11 +* - check error flag +* - loop until data transfer completed +*/ +if (chkcfg && !(bytes %SZ_4K )) {
Is 4k comes from PCI spec, or is it page size?
Quoted 13 lines
drivers/fpga/altera-cvp.c new file +12
-0+size_t done = count - bytes; + + status =altera_cvp_chk_error (mgr, done); +if (status <0 ) +return status; + } + } + +switch (bytes) { +case 3 : + mask =0x00ffffff ; +break ;
drivers/fpga/altera-cvp.c line 359
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +10
-0+case 2 : + mask =0x0000ffff ; +break ; +case 1 : + mask =0x000000ff ; +break ; +case 0 : + mask =0x00000000 ; +break ; + }
Why not to use simple formula?
mask = BIT(bytes * 8) - 1;
Quoted 8 lines
drivers/fpga/altera-cvp.c new file +8
-0+ +if (mask) { + conf->write_data (conf, *data & mask); + +if (chkcfg) + status =altera_cvp_chk_error (mgr, count); + } +
drivers/fpga/altera-cvp.c line 407
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +8
-0+return status; +} +static int altera_cvp_write_complete (struct fpga_manager *mgr, +struct fpga_image_info *info) +{ + delay_us =0 ; + status_msk =VSEC_CVP_STATUS_PLD_CLK_IN_USE |VSEC_CVP_STATUS_USERMODE ; +while (1 ) {
Same comment about loop style.
Quoted 14 lines
drivers/fpga/altera-cvp.c new file +14
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 & status_msk) == status_msk) +break ; + +udelay (1 );/* wait 1us */ + +if (delay_us++ >TIMEOUT_IN_US ) { +dev_warn (&mgr->dev,"PLD_CLK_IN_USE|USERMODE timeout\n" ); + status = -ETIMEDOUT ; +break ; + } + } + +return status;
drivers/fpga/altera-cvp.c line 438
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +9
-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; +int ret; + +pci_read_config_word (pdev,VSEC_PCIE_EXT_CAP_ID , &val16);
Are you foing to do this without enabling device? Needs comment why if so.
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +6
-0+if (val16 !=VSEC_PCIE_EXT_CAP_ID_VAL ) { +dev_err (&pdev->dev,"Wrong EXT_CAP_ID value 0x%x\n" , val16); + ret = -ENODEV ; + goto err; + } +
drivers/fpga/altera-cvp.c line 460
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +9
-0+ conf =devm_kzalloc (&pdev->dev,sizeof (*conf),GFP_KERNEL ); +if (!conf) { + ret = -ENOMEM ; + goto err; + } + + conf->pci_dev = pdev; + +if (pci_request_region (pdev,CVP_BAR ,"CVP" ) <0 ) {
So, you are using devm_ above, but avoid pcim_ here. Please clarify enabling device case and use if possible pcim_
drivers/fpga/altera-cvp.c line 470
Quoted 11 lines
drivers/fpga/altera-cvp.c new file +10
-0+dev_err (&pdev->dev,"Requesting CVP BAR region failed\n" ); + ret = -ENODEV ; + goto err; + } + + 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" );
Not needed in pcim_ case.
drivers/fpga/altera-cvp.c line 486
Quoted 8 lines
drivers/fpga/altera-cvp.c new file +6
-0+ conf->write_data = altera_cvp_write_data_config; + } + conf->mgr =fpga_mgr_get (&pdev->dev); +if (IS_ERR (conf->mgr)) { +dev_err (&pdev->dev,"Getting fpga mgr reference failed\n" ); + ret = -ENODEV ;
Why error is shadowed here?
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+ goto err_unreg; + } +fpga_mgr_put (conf->mgr); +static int __init altera_cvp_init (void ) +{
drivers/fpga/altera-cvp.c line 545
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +10
-0+return pci_register_driver (&altera_cvp_driver); +} + +static void __exit altera_cvp_exit(void) +{ +pci_unregister_driver (&altera_cvp_driver); +} + +module_init (altera_cvp_init); +module_exit (altera_cvp_exit);
I dunno for how many (3+ I think) years we have macros like module_pci_driver();
Please, use it instead of above.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 15 lines · On Mon, 1 May 2017 23:06:16 +0300 Andy Shevchenko [email protected]…
Quoted 2 lines · Anatolij Gustschin
Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V and Arria-10 FPGAs via CvP.
I think you need to spend time on polishing such code.
See my comments below.
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +6
-0+#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_IN_US 2000 + +/* Vendor Specific Extended Capability Offset */ +#define VSEC_OFFSET 0x200
Quoted 6 lines
drivers/fpga/altera-cvp.c line 36
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +2
-0+#define VSEC_PCIE_EXT_CAP_ID_VAL 0x000b +#define VSEC_PCIE_EXT_CAP_ID (VSEC_OFFSET + 0x00)/* 16bit */ It is not clear what is what. If the above is value, why it's located under offset "section"?
ok, will move VSEC_PCIE_EXT_CAP_ID_VAL below the offset.
Quoted 3 lines
Quoted 1 line
+#define
VSEC_CVP_STATUS(VSEC_OFFSET+0x1c) /* 32bit */Usually in the drivers we use just absolute value.
ok, will change it.
Quoted 8 lines
drivers/fpga/altera-cvp.c line 58
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 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_FULL_CFG BIT(2)/* CVP_FULLCONFIG */ +#define VSEC_CVP_MODE_CTRL_NUMCLKS (0xff<<8)/* CVP_NUMCLKS */ Is
0xffa mask here? (Btw, you missed spaces around <<)
yes, it is. Will add spaces (checkpatch didn't warn here).
Quoted 11 lines
drivers/fpga/altera-cvp.c line 78
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +7
-0+static int chkcfg;/* use value 1 for debugging only */ +module_param (chkcfg, int,0664 ); +MODULE_PARM_DESC (chkcfg,"1 - check CvP status, 0 - skip checking (default 0)" ); + +static int numclks =1 ;/* default 1 for uncompressed and unencrypted */ +module_param (numclks, int,0664 ); +MODULE_PARM_DESC (numclks,"Clock to data ratio 1, 4 or 8 (default 1)" );Why do we need that?!
In new drivers we try to avoid new module parameters. We have enough interfaces nowadays to let driver know some details (quirks).
Which interface is preffered here? Do you suggest sysfs? Won't be able to pass the parameter on kernel command line, then.
I'll drop the numclks parameter here and will use a fixed value. I do not need it for current configurations and if anyone needs it and actually has the devices and bitstreams to test it, feel free to implement it using the preferred interfaces.
Quoted 11 lines
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +10
-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); +char mgr_name[64 ]; +}; +
Quoted 14 lines
drivers/fpga/altera-cvp.c line 100
Quoted 12 lines
drivers/fpga/altera-cvp.c new file +12
-0+static inline void altera_cvp_chk_numclks (void ) +{ +switch (numclks) { +case 1 : +case 4 : +case 8 : +break ; +default : + numclks =1 ; +break ; + } +}Why 2 is missed? Hardware limitation? Needs a comment about these magics.
it is not missed, other values are just not valid and filtered out here.
Quoted 7 lines
drivers/fpga/altera-cvp.c line 131
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+static void altera_cvp_dummy_write (struct altera_cvp_conf *conf) +{ +u32 val32; +int i;unsigned int i; ?
ok, will change.
Quoted 7 lines
drivers/fpga/altera-cvp.c line 136
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+/* set number of CVP clock cycles for every CVP Data Register Write */ +pci_read_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , &val32); + val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS ; + val32 |=0x01 <<8 ;/* 1 clock */ Yeah, needs more clear way to put clocks of choice.
what exactly is not clear here?
Quoted 7 lines
drivers/fpga/altera-cvp.c line 140
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+pci_write_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , val32); + +for (i =0 ; i <CVP_DUMMY_WR ; i++) + conf->write_data (conf,0xdeadbeef );Why this dummy is chosen?
it is a dummy and can be anything. So why not? I re-used some code where this value was chosen. Can change it to 0.
Quoted 13 lines
Quoted 12 lines
drivers/fpga/altera-cvp.c new file +11
-0+} + +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 status =0 ; +int delay_us; +u32 val32; +
Quoted 6 lines
drivers/fpga/altera-cvp.c line 154
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +3
-0+/* +* STEP 12 - reset START_XFER bit +*/ One line?
(as well below)
ok, will change.
Quoted 5 lines
Quoted 4 lines
drivers/fpga/altera-cvp.c new file +4
-0+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); +
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ delay_us = 0; + while (1) {Oh, no. It's a red flag.
And better to do
do {} while (--retries);
can change it if its preferred.
Quoted 4 lines
style. It will on smallest glance give reader a clue that the body will go at least once.
Quoted 8 lines
drivers/fpga/altera-cvp.c line 181
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 &VSEC_CVP_STATUS_CFG_RDY ) ==0 ) +break ; + +udelay (1 );/* wait 1us */ Why not 10? Needs a comment.
if this is not obvious, we want to start the configuration early and want to avoid unneeded delays when polling ready status. For 10 I would have to use usleep_range() adding more delay.
Quoted 27 lines
Quoted 16 lines
drivers/fpga/altera-cvp.c new file +14
-0+ +if (delay_us++ >TIMEOUT_IN_US ) { +dev_warn (&mgr->dev,"CVP_CONFIG_READY == 0 timeout\n" ); + status = -ETIMEDOUT ; +break ; + } + } + +return status; +} + +static int altera_cvp_write_init (struct fpga_manager *mgr, +struct fpga_image_info *info, +const char *buf,size_t count)drivers/fpga/altera-cvp.c line 210
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +9
-0+{ +struct altera_cvp_conf *conf = mgr->priv; +struct pci_dev *pdev = conf->pci_dev; +int delay_us; +u32 val32; +int ret; +/* +* STEP 1 - read CVP status and check CVP_EN flag +*/
Quoted 6 lines
Ditto about comments.
Review
Quoted 2 lines
(quoted diff) +2
-0+ delay_us = 0; + while (1) {Ditto about loop style.
ok, will change.
Quoted 22 lines
Quoted 21 lines
drivers/fpga/altera-cvp.c new file +19
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 &VSEC_CVP_STATUS_CFG_RDY ) == +VSEC_CVP_STATUS_CFG_RDY ) +break ; + +udelay (1 );/* wait 1us */ + +if (delay_us++ >TIMEOUT_IN_US ) { +dev_warn (&mgr->dev,"CVP_CONFIG_READY == 1 timeout\n" ); +return -ETIMEDOUT ; + } + } +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;
Quoted 7 lines
drivers/fpga/altera-cvp.c line 306
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+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 %zi bytes!\n" , + bytes);%zu?
ok, will fix.
Quoted 29 lines
Quoted 28 lines
drivers/fpga/altera-cvp.c new file +26
-0+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 bytes; +int status =0 ; +u32 mask; + + data = (u32 *)buf; + bytes = count; + +while (bytes >=4 ) { + conf->write_data (conf, *data++); + bytes -=4 ; + +/* +* STEP 10 (optional) and STEP 11 +* - check error flag +* - loop until data transfer completed +*/
Quoted 3 lines
Quoted 1 line
+ if (chkcfg && !(bytes %
SZ_4K)) {Is 4k comes from PCI spec, or is it page size?
no, it is more an arbitrary value. It was suggested to check for error status after writing a data block and not after each data write to speed-up the config process. The config images can be big (above 36 MiB) and often checking will slow down the configuration.
Quoted 24 lines
Quoted 13 lines
drivers/fpga/altera-cvp.c new file +12
-0+size_t done = count - bytes; + + status =altera_cvp_chk_error (mgr, done); +if (status <0 ) +return status; + } + } + +switch (bytes) { +case 3 : + mask =0x00ffffff ; +break ;drivers/fpga/altera-cvp.c line 359
Quoted 10 lines
drivers/fpga/altera-cvp.c new file +10
-0+case 2 : + mask =0x0000ffff ; +break ; +case 1 : + mask =0x000000ff ; +break ; +case 0 : + mask =0x00000000 ; +break ; + }
Quoted 3 lines
Why not to use simple formula?
mask =
BIT(bytes * 8)- 1;
ok, will use it.
Quoted 15 lines
Quoted 14 lines
drivers/fpga/altera-cvp.c new file +13
-0+ +if (mask) { + conf->write_data (conf, *data & mask); + +if (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) +{
Quoted 5 lines
drivers/fpga/altera-cvp.c line 407
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +3
-0+ delay_us =0 ; + status_msk =VSEC_CVP_STATUS_PLD_CLK_IN_USE |VSEC_CVP_STATUS_USERMODE ; +while (1 ) {Same comment about loop style.
ok, will change.
Quoted 16 lines
Quoted 15 lines
drivers/fpga/altera-cvp.c new file +15
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 & status_msk) == status_msk) +break ; + +udelay (1 );/* wait 1us */ + +if (delay_us++ >TIMEOUT_IN_US ) { +dev_warn (&mgr->dev,"PLD_CLK_IN_USE|USERMODE timeout\n" ); + status = -ETIMEDOUT ; +break ; + } + } + +return status; +}
Quoted 10 lines
drivers/fpga/altera-cvp.c line 438
Quoted 8 lines
drivers/fpga/altera-cvp.c new file +8
-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; +int ret; + +pci_read_config_word (pdev,VSEC_PCIE_EXT_CAP_ID , &val16);Are you foing to do this without enabling device? Needs comment why if so.
pci config space access works without enabling the pci device, writing commands to config space enables the device first. It is done some lines below which you deleted when commenting (please see original patch).
Quoted 16 lines
Quoted 15 lines
drivers/fpga/altera-cvp.c new file +14
-0+if (val16 !=VSEC_PCIE_EXT_CAP_ID_VAL ) { +dev_err (&pdev->dev,"Wrong EXT_CAP_ID value 0x%x\n" , val16); + ret = -ENODEV ; + goto err; + } + + conf =devm_kzalloc (&pdev->dev,sizeof (*conf),GFP_KERNEL ); +if (!conf) { + ret = -ENOMEM ; + goto err; + } + + conf->pci_dev = pdev; +
Quoted 4 lines
Quoted 1 line
+ if (
pci_request_region(pdev, CVP_BAR, "CVP")< 0) {So, you are using
devm_above, but avoidpcim_here. Please clarify enabling device case and use if possiblepcim_
I can't use pcim_enable_device(), it will make the driver unusable on some platforms. The driver is only for re-configuring FPGAs and there can be unlimited variations of different PCIe devices implemented in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for which an address range cannot be assigned on embedded 32-bit platforms. pcim_enable_device() will fail here complaining about not claimed BAR, even if the concerned BAR is not needed for FPGA configuration at all. This makes the driver unusable.
Quoted 8 lines
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +7
-0+dev_err (&pdev->dev,"Requesting CVP BAR region failed\n" ); + ret = -ENODEV ; + goto err; + } + + conf->write_data = altera_cvp_write_data_iomem; +
Quoted 5 lines
drivers/fpga/altera-cvp.c line 470
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +3
-0+ conf->map =pci_iomap (pdev,CVP_BAR ,0 ); +if (!conf->map) { +dev_warn (&pdev->dev,"Mapping CVP BAR failed\n" );Not needed in
pcim_case.
can I use other pcim_ functions if pcim_enalbe_device is not used?
Quoted 10 lines
drivers/fpga/altera-cvp.c line 486
Quoted 8 lines
drivers/fpga/altera-cvp.c new file +6
-0+ conf->write_data = altera_cvp_write_data_config; + } + conf->mgr =fpga_mgr_get (&pdev->dev); +if (IS_ERR (conf->mgr)) { +dev_err (&pdev->dev,"Getting fpga mgr reference failed\n" ); + ret = -ENODEV ;Why error is shadowed here?
oh, actually this mgr_get/mgr_put sequence is not needed here any more, just forgot to remove it.
...
Quoted 7 lines
drivers/fpga/altera-cvp.c line 545
Quoted 2 lines
drivers/fpga/altera-cvp.c new file +2
-0+module_init (altera_cvp_init); +module_exit (altera_cvp_exit);I dunno for how many (3+ I think) years we have macros like
module_pci_driver();Please, use it instead of above.
ok, will do.
Thanks,
Anatolij
Quoted 9 lines · Anatolij Gustschin
Quoted 7 lines · On Mon, 1 May 2017 23:06:16 +0300 Andy Shevchenko [email protected]…
Quoted 4 lines · Anatolij Gustschin
drivers/fpga/altera-cvp.c new file +4
-0+#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 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_FULL_CFG BIT(2)/* CVP_FULLCONFIG */
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+#define
VSEC_CVP_MODE_CTRL_NUMCLKS(0xff<<8) /*CVP_NUMCLKS*/Is
0xffa mask here? (Btw, you missed spaces around <<)yes, it is. Will add spaces (checkpatch didn't warn here).
Then it makes sense to add _MASK and use GENMASK() instead of direct value.
Quoted 6 lines
Quoted 3 lines
Why do we need that?!
In new drivers we try to avoid new module parameters. We have enough interfaces nowadays to let driver know some details (quirks).Which interface is preffered here? Do you suggest sysfs? Won't be able to pass the parameter on kernel command line, then.
Yes, my question here is to understand what so important that driver needs module parameter.
Can you elaborate?
Quoted 4 lines
I'll drop the numclks parameter here and will use a fixed value. I do not need it for current configurations and if anyone needs it and actually has the devices and bitstreams to test it, feel free to implement it using the preferred interfaces.
This would work to me.
Quoted 3 lines
Quoted 1 line
Why 2 is missed? Hardware limitation? Needs a comment about these magics.
it is not missed, other values are just not valid and filtered out here.
That's my suggestion.
So, if reviewer asks such a question it's a flag like "Okay, here is an additional comment required".
Quoted 9 lines
Quoted 7 lines
drivers/fpga/altera-cvp.c line 136
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+/* set number of CVP clock cycles for every CVP Data Register Write */ +pci_read_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , &val32); + val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS ; + val32 |=0x01 <<8 ;/* 1 clock */ Yeah, needs more clear way to put clocks of choice.
what exactly is not clear here?
Magics. And extra prefixes where it's not needed.
0x0 - makes reader to think "A-ha, this is probably address or some interesting data. Here is just 1.
8 - why? Usually we do ..._SHIFT costant for a such.
Quoted 10 lines
Quoted 7 lines
drivers/fpga/altera-cvp.c line 140
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+pci_write_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , val32); + +for (i =0 ; i <CVP_DUMMY_WR ; i++) + conf->write_data (conf,0xdeadbeef );Why this dummy is chosen?
it is a dummy and can be anything. So why not? I re-used some code where this value was chosen. Can change it to 0.
Not need to be changed, but, please add a comment.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
drivers/fpga/altera-cvp.c new file +6
-0+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); + + delay_us =0 ; +while (1 ) {
Quoted 8 lines
Quoted 5 lines
Oh, no. It's a red flag.
And better to do
do {} while (--retries);
can change it if its preferred.
Just think about it:
while (1) {
...100500 lines of code...
}
Reader needs to go through the entire body to understand 2 things:
- what is the exit condition if any? do we have only one exit condition?
- how many iterrations are supposed to be
It takes time even more that took for writing above lines.
Good code would be read fast.
Quoted 10 lines
Quoted 8 lines
drivers/fpga/altera-cvp.c line 181
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 &VSEC_CVP_STATUS_CFG_RDY ) ==0 ) +break ; + +udelay (1 );/* wait 1us */ Why not 10? Needs a comment.
if this is not obvious,
No, it's not. Especially after what you wrote below.
Quoted 3 lines
we want to start the configuration early and want
to avoid unneeded delays when polling ready status. For 10 I would have to useusleep_range()adding more delay.
usleep_range() has one big difference to udelay: it's not atomic. This makes me to ask even more questions instead of understanding what's going on here.
So, what kind of this function is? Is it supposed to be run in atomic context, not atomic, or any?
Depends on answer we need to choose best API to allow minimum delays _and_ CPU resource waste.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ if (chkcfg && !(bytes %
SZ_4K)) {Is 4k comes from PCI spec, or is it page size?
no, it is more an arbitrary value. It was suggested to check for error status after writing a data block and not after each data write to speed-up the config process. The config images can be big (above 36 MiB) and often checking will slow down the configuration.
Your comment didn't make it more clearer to me. So, you take bytes value and check that 12 LSBs are 0. Why?
Quoted 11 lines
Quoted 10 lines
drivers/fpga/altera-cvp.c line 438
Quoted 8 lines
drivers/fpga/altera-cvp.c new file +8
-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; +int ret; + +pci_read_config_word (pdev,VSEC_PCIE_EXT_CAP_ID , &val16);Are you foing to do this without enabling device? Needs comment why if so.
Quoted 4 lines
pci config space access works without enabling the pci device, writing commands to config space enables the device first. It is done some lines below which you deleted when commenting (please see original patch).
Your comment didn't clarify what's going on along these lines.
I checked original patch, I didn't find any type of pci_enable_device() call.
So,
- can you use it?
- if no, elaborate why not
Okay, looks like below you answer this somehow.
Quoted 6 lines
Quoted 2 lines
So, you are using
devm_above, but avoidpcim_here. Please clarify enabling device case and use if possiblepcim_I can't use
pcim_enable_device(), it will make the driver unusable on some platforms.
Quoted 7 lines
The driver is only for re-configuring FPGAs and
there can be unlimited variations of different PCIe devices implemented in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for which an address range cannot be assigned on embedded 32-bit platforms.pcim_enable_device()will fail here complaining about not claimed BAR, even if the concerned BAR is not needed for FPGA configuration at all. This makes the driver unusable.
Please put something like above in the comment in ->probe() before first call to pci_...().
Quoted 7 lines
Quoted 5 lines
drivers/fpga/altera-cvp.c line 470
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +3
-0+ conf->map =pci_iomap (pdev,CVP_BAR ,0 ); +if (!conf->map) { +dev_warn (&pdev->dev,"Mapping CVP BAR failed\n" );Not needed in
pcim_case.can I use other
pcim_functions ifpcim_enalbe_deviceis not used?
You may check yourselt, IIRC no, you can't use pcim_ioremap*().
Signature · 2 lines
With Best Regards, Andy Shevchenko
On Wed, 3 May 2017 00:28:17 +0300
Andy Shevchenko [email protected] wrote:
...
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Is
0xffa mask here? (Btw, you missed spaces around <<)yes, it is. Will add spaces (checkpatch didn't warn here).
Then it makes sense to add
_MASKand useGENMASK()instead of direct value.
ok, will do.
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Why do we need that?!
In new drivers we try to avoid new module parameters. We have enough interfaces nowadays to let driver know some details (quirks).Which interface is preffered here? Do you suggest sysfs? Won't be able to pass the parameter on kernel command line, then.
Yes, my question here is to understand what so important that driver needs module parameter.
Can you elaborate?
the driver doesn't need this parameter, but it could help testing the loading of compressed or encrypted images.
...
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Why 2 is missed? Hardware limitation? Needs a comment about these magics.
it is not missed, other values are just not valid and filtered out here.
That's my suggestion.
So, if reviewer asks such a question it's a flag like "Okay, here is an additional comment required".
ok
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
drivers/fpga/altera-cvp.c line 136
Quoted 5 lines
drivers/fpga/altera-cvp.c new file +4
-0+/* set number of CVP clock cycles for every CVP Data Register Write */ +pci_read_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , &val32); + val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS ; + val32 |=0x01 <<8 ;/* 1 clock */ Yeah, needs more clear way to put clocks of choice.
Quoted 7 lines
Quoted 1 line
what exactly is not clear here?
Magics. And extra prefixes where it's not needed.
0x0- makes reader to think "A-ha, this is probably address or some interesting data. Here is just 1.
8 - why? Usually we do ..._SHIFT costant for a such.
ok, will change.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
drivers/fpga/altera-cvp.c new file +3
-0+pci_write_config_dword (conf->pci_dev,VSEC_CVP_MODE_CTRL , val32); + +for (i =0 ; i <CVP_DUMMY_WR ; i++)
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+
conf->write_data(conf, 0xdeadbeef);Why this dummy is chosen?
it is a dummy and can be anything. So why not? I re-used some code where this value was chosen. Can change it to 0.
Not need to be changed, but, please add a comment.
ok, will do.
...
Quoted 6 lines
Just think about it:
while (1) {
...100500 lines of code...
}
Quoted 7 lines
Reader needs to go through the entire body to understand 2 things:
- what is the exit condition if any? do we have only one exit condition?
- how many iterrations are supposed to beIt takes time even more that took for writing above lines.
Good code would be read fast.
ok
Quoted 17 lines
Quoted 10 lines
Quoted 8 lines
drivers/fpga/altera-cvp.c line 181
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +5
-0+pci_read_config_dword (pdev,VSEC_CVP_STATUS , &val32); +if ((val32 &VSEC_CVP_STATUS_CFG_RDY ) ==0 ) +break ; + +udelay (1 );/* wait 1us */ Why not 10? Needs a comment.
if this is not obvious,
No, it's not. Especially after what you wrote below.
Quoted 3 lines
we want to start the configuration early and want
to avoid unneeded delays when polling ready status. For 10 I would have to useusleep_range()adding more delay.
Quoted 6 lines
usleep_range()has one big difference to udelay: it's not atomic. This makes me to ask even more questions instead of understanding what's going on here.So, what kind of this function is? Is it supposed to be run in atomic context, not atomic, or any?
not atomic, a callback always running in a process context.
Quoted 7 lines
Depends on answer we need to choose best API to allow minimum delays _and_ CPU resource waste.
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
+ if (chkcfg && !(bytes %
SZ_4K)) {Is 4k comes from PCI spec, or is it page size?
Quoted 7 lines
Quoted 4 lines
no, it is more an arbitrary value. It was suggested to check for error status after writing a data block and not after each data write to speed-up the config process. The config images can be big (above 36 MiB) and often checking will slow down the configuration.
Your comment didn't make it more clearer to me. So, you take bytes value and check that 12 LSBs are 0. Why?
when 12 LSBs are zero, the bytes value has been decremented by 4k, meaning that a new 4k data block has been written. Only then the error checking is performed.
...
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+
pci_read_config_word(pdev, VSEC_PCIE_EXT_CAP_ID, &val16);Are you foing to do this without enabling device? Needs comment why if so.
pci config space access works without enabling the pci device, writing commands to config space enables the device first. It is done some lines below which you deleted when commenting (please see original patch).
Quoted 4 lines
Your comment didn't clarify what's going on along these lines.
I checked original patch, I didn't find any type of
pci_enable_device()call.
I mean this part (instead of pci_enable_device()):
+ /* Enable memory BAR access */
+ 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);
+ }
Quoted 20 lines
So,
- can you use it?
- if no, elaborate why notOkay, looks like below you answer this somehow.
Quoted 6 lines
Quoted 2 lines
So, you are using
devm_above, but avoidpcim_here. Please clarify enabling device case and use if possiblepcim_I can't use
pcim_enable_device(), it will make the driver unusable on some platforms.Quoted 7 lines
The driver is only for re-configuring FPGAs and
there can be unlimited variations of different PCIe devices implemented in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for which an address range cannot be assigned on embedded 32-bit platforms.pcim_enable_device()will fail here complaining about not claimed BAR, even if the concerned BAR is not needed for FPGA configuration at all. This makes the driver unusable.
Quoted 2 lines
Please put something like above in the comment in ->probe() before first call to pci_...().
ok
Thanks,
Anatolij
Quoted 18 lines · Anatolij Gustschin
On Wed, 3 May 2017 00:28:17 +0300
Andy Shevchenko [email protected] wrote:
...Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Is
0xffa mask here? (Btw, you missed spaces around <<)yes, it is. Will add spaces (checkpatch didn't warn here).
Then it makes sense to add
_MASKand useGENMASK()instead of direct value.ok, will do.
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
Why do we need that?!
In new drivers we try to avoid new module parameters. We have enough interfaces nowadays to let driver know some details (quirks).Which interface is preffered here? Do you suggest sysfs? Won't be able to pass the parameter on kernel command line, then.
Quoted 6 lines
Quoted 3 lines
Yes, my question here is to understand what so important that driver needs module parameter.
Can you elaborate?the driver doesn't need this parameter, but it could help testing the loading of compressed or encrypted images.
Loading encrypted or compressed images can be keyed off of flags in fpga_image_info. Currently we have FPGA_MGR_ENCRYPTED_BITSTREAM Will need to add one for compressed such as FPGA_MGR_COMPRESSED_BITSTREAM
Alan
Quoted 15 lines · Anatolij Gustschin
Quoted 12 lines · On Wed, 3 May 2017 00:28:17 +0300 Andy Shevchenko [email protected]…
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+ udelay(1); /* wait 1us */
Why not 10? Needs a comment.
if this is not obvious,
No, it's not. Especially after what you wrote below.
Quoted 3 lines
we want to start the configuration early and want
to avoid unneeded delays when polling ready status. For 10 I would have to useusleep_range()adding more delay.
Quoted 8 lines
Quoted 6 lines
usleep_range()has one big difference to udelay: it's not atomic. This makes me to ask even more questions instead of understanding what's going on here.So, what kind of this function is? Is it supposed to be run in atomic context, not atomic, or any?
not atomic, a callback always running in a process context.
So, then it would be good trade off to use usleep_range(10, 11) or alike to allow others to get a resource.
udelay() is a busy loop and use of it costs us CPU resource.
Quoted 12 lines
Quoted 12 lines
Depends on answer we need to choose best API to allow minimum delays _and_ CPU resource waste.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ if (chkcfg && !(bytes %
SZ_4K)) {Is 4k comes from PCI spec, or is it page size?
no, it is more an arbitrary value. It was suggested to check for error status after writing a data block and not after each data write to speed-up the config process. The config images can be big (above 36 MiB) and often checking will slow down the configuration.
Quoted 6 lines
Quoted 2 lines
Your comment didn't make it more clearer to me. So, you take bytes value and check that 12 LSBs are 0. Why?
when 12 LSBs are zero, the bytes value has been decremented by 4k, meaning that a new 4k data block has been written. Only then the error checking is performed.
If the size is less than 4k...?
Quoted 15 lines
Quoted 11 lines
Quoted 6 lines
Quoted 1 line
Are you foing to do this without enabling device? Needs comment why if so.
pci config space access works without enabling the pci device, writing commands to config space enables the device first. It is done some lines below which you deleted when commenting (please see original patch).
Your comment didn't clarify what's going on along these lines.
I checked original patch, I didn't find any type of
pci_enable_device()call.I mean this part (instead of
pci_enable_device()):
drivers/fpga/altera-cvp.c line 450
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +6
-0+/* Enable memory BAR access */ +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); + }
I see this code is used somewhere else (several places I suppose, drivers/video/fbdev/aty/atyfb_base.c is one of them).
For me it makes sense to split it to a helper in pci.h for broader use.
static inline void pci_enable_memory(struct pci_dev *dev)
{
u16 cmd;
/* Enable memory BAR access */
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);
}
}Signature · 2 lines
With Best Regards, Andy Shevchenko
On Wed, 3 May 2017 18:01:19 +0300
Andy Shevchenko [email protected] wrote:
...
Quoted 5 lines
Quoted 3 lines
when 12 LSBs are zero, the bytes value has been decremented by 4k, meaning that a new 4k data block has been written. Only then the error checking is performed.
If the size is less than 4k...?
then the final check below will catch it. And I doubt that config images can be so small. The lowest size I've ever seen is more than 1MiB.
...
Quoted 20 lines
Quoted 13 lines
Quoted 9 lines
Quoted 4 lines
pci config space access works without enabling the pci device, writing commands to config space enables the device first. It is done some lines below which you deleted when commenting (please see original patch).
Your comment didn't clarify what's going on along these lines.
I checked original patch, I didn't find any type of
pci_enable_device()call.I mean this part (instead of
pci_enable_device()):drivers/fpga/altera-cvp.c line 450
Quoted 6 lines
drivers/fpga/altera-cvp.c new file +6
-0+/* Enable memory BAR access */ +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); + }
Quoted 2 lines
I see this code is used somewhere else (several places I suppose,
drivers/video/fbdev/aty/atyfb_base.cis one of them).
other places set or clean additional pci command flags, use different pci config accessors or contain debug code. And I reuse pre-initialized cmd in the error path, so the usage pattern here is not the same as in the atyfb_base driver.
--
Anatolij
Quoted 6 lines · Anatolij Gustschin
Quoted 3 lines · On Mon, 1 May 2017 23:06:16 +0300 Andy Shevchenko [email protected]…
Quoted 1 line
+#define
VSEC_CVP_MODE_CTRL_NUMCLKS(0xff<<8) /*CVP_NUMCLKS*/Is
0xffa mask here? (Btw, you missed spaces around <<)yes, it is. Will add spaces (checkpatch didn't warn here).
It would with command line option --strict, otherwise not.
On Tue, 02 May 2017 16:36:54 -0700
Joe Perches [email protected] wrote:
...
Quoted 1 line
It would with command line option --strict, otherwise not.
ah, good to know. Thanks!