* [PATCHv2 0/3] fpga: altera-cvp: Add Stratix10 Support
@ 2019-07-16 22:48 thor.thayer
2019-07-16 22:48 ` [PATCHv2 1/3] fpga: altera-cvp: Discover Vendor Specific offset thor.thayer
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: thor.thayer @ 2019-07-16 22:48 UTC (permalink / raw)
To: mdf, richard.gong, agust; +Cc: linux-fpga, linux-kernel, Thor Thayer
From: Thor Thayer <thor.thayer@linux.intel.com>
Newer versions (V2) of Altera/Intel FPGAs CvP have different PCI
Vendor Specific Capability offsets than the older (V1) Altera/FPGAs.
Most of the CvP registers and their bitfields remain the same
between both the older parts and the newer parts.
This patchset implements changes to discover the Vendor Specific
Capability offset and then add Stratix10 CvP support.
V2 Changes:
Remove inline designator from abstraction functions.
Reverse Christmas Tree format for local variables
Remove redundant mask from credit calculation
Add commment for the delay(1) function in wait_for_credit()
Thor Thayer (3):
fpga: altera-cvp: Discover Vendor Specific offset
fpga: altera-cvp: Preparation for V2 parts.
fpga: altera-cvp: Add Stratix10 (V2) Support
drivers/fpga/altera-cvp.c | 326 ++++++++++++++++++++++++++++++++++++----------
1 file changed, 255 insertions(+), 71 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCHv2 1/3] fpga: altera-cvp: Discover Vendor Specific offset 2019-07-16 22:48 [PATCHv2 0/3] fpga: altera-cvp: Add Stratix10 Support thor.thayer @ 2019-07-16 22:48 ` thor.thayer 2019-07-16 22:48 ` [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts thor.thayer 2019-07-16 22:48 ` [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support thor.thayer 2 siblings, 0 replies; 11+ messages in thread From: thor.thayer @ 2019-07-16 22:48 UTC (permalink / raw) To: mdf, richard.gong, agust; +Cc: linux-fpga, linux-kernel, Thor Thayer From: Thor Thayer <thor.thayer@linux.intel.com> Newer Intel FPGAs have different Vendor Specific offsets than legacy parts. Use PCI discovery to find the CvP registers. Since the register positions remain the same, change the hard coded address to a more flexible way of indexing registers from the offset. Adding new PCI read and write abstraction functions to handle the offset (altera_read_config_dword() and altera_write_config_dword()). Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> --- v2 Remove inline designator from abstraction functions. Reverse Christmas Tree format for local variables --- drivers/fpga/altera-cvp.c | 94 ++++++++++++++++++++++++++++------------------- 1 file changed, 57 insertions(+), 37 deletions(-) diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c index 770915fb97f9..b78c90580071 100644 --- a/drivers/fpga/altera-cvp.c +++ b/drivers/fpga/altera-cvp.c @@ -22,10 +22,10 @@ #define TIMEOUT_US 2000 /* CVP STATUS timeout for USERMODE polling */ /* Vendor Specific Extended Capability Registers */ -#define VSE_PCIE_EXT_CAP_ID 0x200 +#define VSE_PCIE_EXT_CAP_ID 0x0 #define VSE_PCIE_EXT_CAP_ID_VAL 0x000b /* 16bit */ -#define VSE_CVP_STATUS 0x21c /* 32bit */ +#define VSE_CVP_STATUS 0x1c /* 32bit */ #define VSE_CVP_STATUS_CFG_RDY BIT(18) /* CVP_CONFIG_READY */ #define VSE_CVP_STATUS_CFG_ERR BIT(19) /* CVP_CONFIG_ERROR */ #define VSE_CVP_STATUS_CVP_EN BIT(20) /* ctrl block is enabling CVP */ @@ -33,18 +33,18 @@ #define VSE_CVP_STATUS_CFG_DONE BIT(23) /* CVP_CONFIG_DONE */ #define VSE_CVP_STATUS_PLD_CLK_IN_USE BIT(24) /* PLD_CLK_IN_USE */ -#define VSE_CVP_MODE_CTRL 0x220 /* 32bit */ +#define VSE_CVP_MODE_CTRL 0x20 /* 32bit */ #define VSE_CVP_MODE_CTRL_CVP_MODE BIT(0) /* CVP (1) or normal mode (0) */ #define VSE_CVP_MODE_CTRL_HIP_CLK_SEL BIT(1) /* PMA (1) or fabric clock (0) */ #define VSE_CVP_MODE_CTRL_NUMCLKS_OFF 8 /* NUMCLKS bits offset */ #define VSE_CVP_MODE_CTRL_NUMCLKS_MASK GENMASK(15, 8) -#define VSE_CVP_DATA 0x228 /* 32bit */ -#define VSE_CVP_PROG_CTRL 0x22c /* 32bit */ +#define VSE_CVP_DATA 0x28 /* 32bit */ +#define VSE_CVP_PROG_CTRL 0x2c /* 32bit */ #define VSE_CVP_PROG_CTRL_CONFIG BIT(0) #define VSE_CVP_PROG_CTRL_START_XFER BIT(1) -#define VSE_UNCOR_ERR_STATUS 0x234 /* 32bit */ +#define VSE_UNCOR_ERR_STATUS 0x34 /* 32bit */ #define VSE_UNCOR_ERR_CVP_CFG_ERR BIT(5) /* CVP_CONFIG_ERROR_LATCHED */ #define DRV_NAME "altera-cvp" @@ -60,14 +60,27 @@ struct altera_cvp_conf { void (*write_data)(struct altera_cvp_conf *, u32); char mgr_name[64]; u8 numclks; + u32 vsec_offset; }; +static void altera_read_config_dword(struct altera_cvp_conf *conf, + int where, u32 *val) +{ + pci_read_config_dword(conf->pci_dev, conf->vsec_offset + where, val); +} + +static void altera_write_config_dword(struct altera_cvp_conf *conf, + int where, u32 val) +{ + pci_write_config_dword(conf->pci_dev, conf->vsec_offset + where, val); +} + 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, VSE_CVP_STATUS, &status); + altera_read_config_dword(conf, VSE_CVP_STATUS, &status); if (status & VSE_CVP_STATUS_CFG_DONE) return FPGA_MGR_STATE_OPERATING; @@ -85,7 +98,8 @@ static void altera_cvp_write_data_iomem(struct altera_cvp_conf *conf, u32 val) static void altera_cvp_write_data_config(struct altera_cvp_conf *conf, u32 val) { - pci_write_config_dword(conf->pci_dev, VSE_CVP_DATA, val); + pci_write_config_dword(conf->pci_dev, conf->vsec_offset + VSE_CVP_DATA, + val); } /* switches between CvP clock and internal clock */ @@ -95,10 +109,10 @@ static void altera_cvp_dummy_write(struct altera_cvp_conf *conf) u32 val; /* set 1 CVP clock cycle for every CVP Data Register Write */ - pci_read_config_dword(conf->pci_dev, VSE_CVP_MODE_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; val |= 1 << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; - pci_write_config_dword(conf->pci_dev, VSE_CVP_MODE_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); for (i = 0; i < CVP_DUMMY_WR; i++) conf->write_data(conf, 0); /* dummy data, could be any value */ @@ -115,7 +129,7 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask, retries++; do { - pci_read_config_dword(conf->pci_dev, VSE_CVP_STATUS, &val); + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); if ((val & status_mask) == status_val) return 0; @@ -130,18 +144,17 @@ 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 val; /* STEP 12 - reset START_XFER bit */ - pci_read_config_dword(pdev, VSE_CVP_PROG_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); val &= ~VSE_CVP_PROG_CTRL_START_XFER; - pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); /* STEP 13 - reset CVP_CONFIG bit */ val &= ~VSE_CVP_PROG_CTRL_CONFIG; - pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); /* * STEP 14 @@ -163,7 +176,6 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, const char *buf, size_t count) { struct altera_cvp_conf *conf = mgr->priv; - struct pci_dev *pdev = conf->pci_dev; u32 iflags, val; int ret; @@ -183,7 +195,7 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, conf->numclks = 1; /* for uncompressed and unencrypted images */ /* STEP 1 - read CVP status and check CVP_EN flag */ - pci_read_config_dword(pdev, VSE_CVP_STATUS, &val); + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); if (!(val & VSE_CVP_STATUS_CVP_EN)) { dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val); return -ENODEV; @@ -201,14 +213,14 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, * - 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, VSE_CVP_MODE_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); val |= VSE_CVP_MODE_CTRL_HIP_CLK_SEL; - pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); /* set CVP mode */ - pci_read_config_dword(pdev, VSE_CVP_MODE_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); val |= VSE_CVP_MODE_CTRL_CVP_MODE; - pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); /* * STEP 3 @@ -217,10 +229,10 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, altera_cvp_dummy_write(conf); /* STEP 4 - set CVP_CONFIG bit */ - pci_read_config_dword(pdev, VSE_CVP_PROG_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); /* request control block to begin transfer using CVP */ val |= VSE_CVP_PROG_CTRL_CONFIG; - pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); /* STEP 5 - poll CVP_CONFIG READY for 1 with 10us timeout */ ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, @@ -237,15 +249,15 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, altera_cvp_dummy_write(conf); /* STEP 7 - set START_XFER */ - pci_read_config_dword(pdev, VSE_CVP_PROG_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); val |= VSE_CVP_PROG_CTRL_START_XFER; - pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); /* STEP 8 - start transfer (set CVP_NUMCLKS for bitstream) */ - pci_read_config_dword(pdev, VSE_CVP_MODE_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; - pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); return 0; } @@ -256,7 +268,7 @@ static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) u32 val; /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ - pci_read_config_dword(conf->pci_dev, VSE_CVP_STATUS, &val); + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); if (val & VSE_CVP_STATUS_CFG_ERR) { dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n", bytes); @@ -315,27 +327,25 @@ 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; + u32 mask, val; int ret; - u32 mask; - u32 val; ret = altera_cvp_teardown(mgr, info); if (ret) return ret; /* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */ - pci_read_config_dword(pdev, VSE_UNCOR_ERR_STATUS, &val); + altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val); if (val & VSE_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, VSE_CVP_MODE_CTRL, &val); + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); val &= ~VSE_CVP_MODE_CTRL_HIP_CLK_SEL; val &= ~VSE_CVP_MODE_CTRL_CVP_MODE; - pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val); + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); /* STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits */ mask = VSE_CVP_STATUS_PLD_CLK_IN_USE | VSE_CVP_STATUS_USERMODE; @@ -394,22 +404,29 @@ static int altera_cvp_probe(struct pci_dev *pdev, { struct altera_cvp_conf *conf; struct fpga_manager *mgr; + int ret, offset; u16 cmd, val; u32 regval; - int ret; + + /* Discover the Vendor Specific Offset for this device */ + offset = pci_find_next_ext_capability(pdev, 0, PCI_EXT_CAP_ID_VNDR); + if (!offset) { + dev_err(&pdev->dev, "No Vendor Specific Offset.\n"); + return -ENODEV; + } /* * 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, VSE_PCIE_EXT_CAP_ID, &val); + pci_read_config_word(pdev, offset + VSE_PCIE_EXT_CAP_ID, &val); if (val != VSE_PCIE_EXT_CAP_ID_VAL) { dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val); return -ENODEV; } - pci_read_config_dword(pdev, VSE_CVP_STATUS, ®val); + pci_read_config_dword(pdev, offset + VSE_CVP_STATUS, ®val); if (!(regval & VSE_CVP_STATUS_CVP_EN)) { dev_err(&pdev->dev, "CVP is disabled for this device: CVP_STATUS Reg 0x%x\n", @@ -421,6 +438,9 @@ static int altera_cvp_probe(struct pci_dev *pdev, if (!conf) return -ENOMEM; + conf->vsec_offset = offset; + dev_dbg(&pdev->dev, "Vendor Specific Data Offset at 0x%x\n", offset); + /* * Enable memory BAR access. We cannot use pci_enable_device() here * because it will make the driver unusable with FPGA devices that -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts. 2019-07-16 22:48 [PATCHv2 0/3] fpga: altera-cvp: Add Stratix10 Support thor.thayer 2019-07-16 22:48 ` [PATCHv2 1/3] fpga: altera-cvp: Discover Vendor Specific offset thor.thayer @ 2019-07-16 22:48 ` thor.thayer 2019-07-22 0:59 ` Moritz Fischer 2019-07-16 22:48 ` [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support thor.thayer 2 siblings, 1 reply; 11+ messages in thread From: thor.thayer @ 2019-07-16 22:48 UTC (permalink / raw) To: mdf, richard.gong, agust; +Cc: linux-fpga, linux-kernel, Thor Thayer From: Thor Thayer <thor.thayer@linux.intel.com> In preparation for adding newer V2 parts that use a FIFO, reorganize altera_cvp_chk_error() and change the write function to block based. V2 parts have a block size matching the FIFO while older V1 parts write a 32 bit word at a time. Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> --- v2 Remove inline function declaration Reverse Christmas Tree format for local variables --- drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 26 deletions(-) diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c index b78c90580071..37419d6b9915 100644 --- a/drivers/fpga/altera-cvp.c +++ b/drivers/fpga/altera-cvp.c @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask, return -ETIMEDOUT; } +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) +{ + struct altera_cvp_conf *conf = mgr->priv; + u32 val; + + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); + if (val & VSE_CVP_STATUS_CFG_ERR) { + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n", + bytes); + return -EPROTO; + } + return 0; +} + +static int altera_cvp_send_block(struct altera_cvp_conf *conf, + const u32 *data, size_t len) +{ + u32 mask, words = len / sizeof(u32); + int i, remainder; + + for (i = 0; i < words; i++) + conf->write_data(conf, *data++); + + /* write up to 3 trailing bytes, if any */ + remainder = len % sizeof(u32); + if (remainder) { + mask = BIT(remainder * 8) - 1; + if (mask) + conf->write_data(conf, *data & mask); + } + + return 0; +} + static int altera_cvp_teardown(struct fpga_manager *mgr, struct fpga_image_info *info) { @@ -262,39 +297,29 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, return 0; } -static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) -{ - struct altera_cvp_conf *conf = mgr->priv; - u32 val; - - /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ - altera_read_config_dword(conf, VSE_CVP_STATUS, &val); - if (val & VSE_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; + size_t done, remaining, len; 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; + while (remaining) { + if (remaining >= sizeof(u32)) + len = sizeof(u32); + else + len = remaining; + + altera_cvp_send_block(conf, data, len); + data++; + done += len; + remaining -= len; /* * STEP 10 (optional) and STEP 11 @@ -312,11 +337,6 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, } } - /* 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts. 2019-07-16 22:48 ` [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts thor.thayer @ 2019-07-22 0:59 ` Moritz Fischer 2019-07-23 14:40 ` Thor Thayer 0 siblings, 1 reply; 11+ messages in thread From: Moritz Fischer @ 2019-07-22 0:59 UTC (permalink / raw) To: thor.thayer; +Cc: mdf, richard.gong, agust, linux-fpga, linux-kernel Thor, On Tue, Jul 16, 2019 at 05:48:06PM -0500, thor.thayer@linux.intel.com wrote: > From: Thor Thayer <thor.thayer@linux.intel.com> > > In preparation for adding newer V2 parts that use a FIFO, > reorganize altera_cvp_chk_error() and change the write > function to block based. > V2 parts have a block size matching the FIFO while older > V1 parts write a 32 bit word at a time. > > Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> > --- > v2 Remove inline function declaration > Reverse Christmas Tree format for local variables > --- > drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++----------------- > 1 file changed, 46 insertions(+), 26 deletions(-) > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c > index b78c90580071..37419d6b9915 100644 > --- a/drivers/fpga/altera-cvp.c > +++ b/drivers/fpga/altera-cvp.c > @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask, > return -ETIMEDOUT; > } > > +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) > +{ > + struct altera_cvp_conf *conf = mgr->priv; > + u32 val; > + > + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ > + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); Same as in the other email, why can we ignore return values here. I think the original code probably did that already. > + if (val & VSE_CVP_STATUS_CFG_ERR) { > + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n", > + bytes); > + return -EPROTO; > + } > + return 0; > +} > + > +static int altera_cvp_send_block(struct altera_cvp_conf *conf, > + const u32 *data, size_t len) > +{ > + u32 mask, words = len / sizeof(u32); > + int i, remainder; > + > + for (i = 0; i < words; i++) > + conf->write_data(conf, *data++); > + > + /* write up to 3 trailing bytes, if any */ > + remainder = len % sizeof(u32); > + if (remainder) { > + mask = BIT(remainder * 8) - 1; > + if (mask) > + conf->write_data(conf, *data & mask); > + } > + > + return 0; > +} > + > static int altera_cvp_teardown(struct fpga_manager *mgr, > struct fpga_image_info *info) > { > @@ -262,39 +297,29 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, > return 0; > } > > -static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) > -{ > - struct altera_cvp_conf *conf = mgr->priv; > - u32 val; > - > - /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ > - altera_read_config_dword(conf, VSE_CVP_STATUS, &val); > - if (val & VSE_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; > + size_t done, remaining, len; > 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; > + while (remaining) { > + if (remaining >= sizeof(u32)) > + len = sizeof(u32); > + else > + len = remaining; > + > + altera_cvp_send_block(conf, data, len); > + data++; > + done += len; > + remaining -= len; > > /* > * STEP 10 (optional) and STEP 11 > @@ -312,11 +337,6 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, > } > } > > - /* 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); > > -- > 2.7.4 > Cheers, Moritz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts. 2019-07-22 0:59 ` Moritz Fischer @ 2019-07-23 14:40 ` Thor Thayer 2019-07-24 14:57 ` Moritz Fischer 0 siblings, 1 reply; 11+ messages in thread From: Thor Thayer @ 2019-07-23 14:40 UTC (permalink / raw) To: Moritz Fischer; +Cc: richard.gong, agust, linux-fpga, linux-kernel Hi Moritz, On 7/21/19 7:59 PM, Moritz Fischer wrote: > Thor, > > On Tue, Jul 16, 2019 at 05:48:06PM -0500, thor.thayer@linux.intel.com wrote: >> From: Thor Thayer <thor.thayer@linux.intel.com> >> >> In preparation for adding newer V2 parts that use a FIFO, >> reorganize altera_cvp_chk_error() and change the write >> function to block based. >> V2 parts have a block size matching the FIFO while older >> V1 parts write a 32 bit word at a time. >> >> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> >> --- >> v2 Remove inline function declaration >> Reverse Christmas Tree format for local variables >> --- >> drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++----------------- >> 1 file changed, 46 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c >> index b78c90580071..37419d6b9915 100644 >> --- a/drivers/fpga/altera-cvp.c >> +++ b/drivers/fpga/altera-cvp.c >> @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask, >> return -ETIMEDOUT; >> } >> >> +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) >> +{ >> + struct altera_cvp_conf *conf = mgr->priv; >> + u32 val; >> + >> + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ >> + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); > Same as in the other email, why can we ignore return values here. I > think the original code probably did that already. Yes, I actually didn't make any changes to this function. You can see I moved it from below since it is used in the following function. I'm not checking the return code from any of the read/write functions since the original driver didn't. Would you prefer I check and issue a warning? Thanks for reviewing! >> + if (val & VSE_CVP_STATUS_CFG_ERR) { >> + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n", >> + bytes); >> + return -EPROTO; >> + } >> + return 0; >> +} >> + >> +static int altera_cvp_send_block(struct altera_cvp_conf *conf, >> + const u32 *data, size_t len) >> +{ >> + u32 mask, words = len / sizeof(u32); >> + int i, remainder; >> + >> + for (i = 0; i < words; i++) >> + conf->write_data(conf, *data++); >> + >> + /* write up to 3 trailing bytes, if any */ >> + remainder = len % sizeof(u32); >> + if (remainder) { >> + mask = BIT(remainder * 8) - 1; >> + if (mask) >> + conf->write_data(conf, *data & mask); >> + } >> + >> + return 0; >> +} >> + >> static int altera_cvp_teardown(struct fpga_manager *mgr, >> struct fpga_image_info *info) >> { >> @@ -262,39 +297,29 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, >> return 0; >> } >> >> -static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) >> -{ >> - struct altera_cvp_conf *conf = mgr->priv; >> - u32 val; >> - >> - /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ >> - altera_read_config_dword(conf, VSE_CVP_STATUS, &val); >> - if (val & VSE_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; >> + size_t done, remaining, len; >> 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; >> + while (remaining) { >> + if (remaining >= sizeof(u32)) >> + len = sizeof(u32); >> + else >> + len = remaining; >> + >> + altera_cvp_send_block(conf, data, len); >> + data++; >> + done += len; >> + remaining -= len; >> >> /* >> * STEP 10 (optional) and STEP 11 >> @@ -312,11 +337,6 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, >> } >> } >> >> - /* 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); >> >> -- >> 2.7.4 >> > Cheers, > Moritz > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts. 2019-07-23 14:40 ` Thor Thayer @ 2019-07-24 14:57 ` Moritz Fischer 2019-07-24 16:59 ` Thor Thayer 0 siblings, 1 reply; 11+ messages in thread From: Moritz Fischer @ 2019-07-24 14:57 UTC (permalink / raw) To: Thor Thayer; +Cc: Moritz Fischer, richard.gong, agust, linux-fpga, linux-kernel On Tue, Jul 23, 2019 at 09:40:51AM -0500, Thor Thayer wrote: > Hi Moritz, > > On 7/21/19 7:59 PM, Moritz Fischer wrote: > > Thor, > > > > On Tue, Jul 16, 2019 at 05:48:06PM -0500, thor.thayer@linux.intel.com wrote: > > > From: Thor Thayer <thor.thayer@linux.intel.com> > > > > > > In preparation for adding newer V2 parts that use a FIFO, > > > reorganize altera_cvp_chk_error() and change the write > > > function to block based. > > > V2 parts have a block size matching the FIFO while older > > > V1 parts write a 32 bit word at a time. > > > > > > Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> > > > --- > > > v2 Remove inline function declaration > > > Reverse Christmas Tree format for local variables > > > --- > > > drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++----------------- > > > 1 file changed, 46 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c > > > index b78c90580071..37419d6b9915 100644 > > > --- a/drivers/fpga/altera-cvp.c > > > +++ b/drivers/fpga/altera-cvp.c > > > @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask, > > > return -ETIMEDOUT; > > > } > > > +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) > > > +{ > > > + struct altera_cvp_conf *conf = mgr->priv; > > > + u32 val; > > > + > > > + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ > > > + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); > > Same as in the other email, why can we ignore return values here. I > > think the original code probably did that already. > > Yes, I actually didn't make any changes to this function. You can see I > moved it from below since it is used in the following function. > > I'm not checking the return code from any of the read/write functions since > the original driver didn't. Would you prefer I check and issue a warning? Not sure a warning would change much here. We should probably look at why it was ok in the first place. > > Thanks for reviewing! > > > > + if (val & VSE_CVP_STATUS_CFG_ERR) { > > > + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n", > > > + bytes); > > > + return -EPROTO; > > > + } > > > + return 0; > > > +} > > > + > > > +static int altera_cvp_send_block(struct altera_cvp_conf *conf, > > > + const u32 *data, size_t len) > > > +{ > > > + u32 mask, words = len / sizeof(u32); > > > + int i, remainder; > > > + > > > + for (i = 0; i < words; i++) > > > + conf->write_data(conf, *data++); > > > + > > > + /* write up to 3 trailing bytes, if any */ > > > + remainder = len % sizeof(u32); > > > + if (remainder) { > > > + mask = BIT(remainder * 8) - 1; > > > + if (mask) > > > + conf->write_data(conf, *data & mask); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static int altera_cvp_teardown(struct fpga_manager *mgr, > > > struct fpga_image_info *info) > > > { > > > @@ -262,39 +297,29 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, > > > return 0; > > > } > > > -static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) > > > -{ > > > - struct altera_cvp_conf *conf = mgr->priv; > > > - u32 val; > > > - > > > - /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ > > > - altera_read_config_dword(conf, VSE_CVP_STATUS, &val); > > > - if (val & VSE_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; > > > + size_t done, remaining, len; > > > 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; > > > + while (remaining) { > > > + if (remaining >= sizeof(u32)) > > > + len = sizeof(u32); > > > + else > > > + len = remaining; > > > + > > > + altera_cvp_send_block(conf, data, len); > > > + data++; > > > + done += len; > > > + remaining -= len; > > > /* > > > * STEP 10 (optional) and STEP 11 > > > @@ -312,11 +337,6 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, > > > } > > > } > > > - /* 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); > > > -- > > > 2.7.4 > > > > > Cheers, > > Moritz > > > Moritz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts. 2019-07-24 14:57 ` Moritz Fischer @ 2019-07-24 16:59 ` Thor Thayer 2019-07-24 17:05 ` Moritz Fischer 0 siblings, 1 reply; 11+ messages in thread From: Thor Thayer @ 2019-07-24 16:59 UTC (permalink / raw) To: Moritz Fischer; +Cc: richard.gong, agust, linux-fpga, linux-kernel Hi Moritz, On 7/24/19 9:57 AM, Moritz Fischer wrote: > On Tue, Jul 23, 2019 at 09:40:51AM -0500, Thor Thayer wrote: >> Hi Moritz, >> >> On 7/21/19 7:59 PM, Moritz Fischer wrote: >>> Thor, >>> >>> On Tue, Jul 16, 2019 at 05:48:06PM -0500, thor.thayer@linux.intel.com wrote: >>>> From: Thor Thayer <thor.thayer@linux.intel.com> >>>> >>>> In preparation for adding newer V2 parts that use a FIFO, >>>> reorganize altera_cvp_chk_error() and change the write >>>> function to block based. >>>> V2 parts have a block size matching the FIFO while older >>>> V1 parts write a 32 bit word at a time. >>>> >>>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> >>>> --- >>>> v2 Remove inline function declaration >>>> Reverse Christmas Tree format for local variables >>>> --- >>>> drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++----------------- >>>> 1 file changed, 46 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c >>>> index b78c90580071..37419d6b9915 100644 >>>> --- a/drivers/fpga/altera-cvp.c >>>> +++ b/drivers/fpga/altera-cvp.c >>>> @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask, >>>> return -ETIMEDOUT; >>>> } >>>> +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) >>>> +{ >>>> + struct altera_cvp_conf *conf = mgr->priv; >>>> + u32 val; >>>> + >>>> + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ >>>> + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); >>> Same as in the other email, why can we ignore return values here. I >>> think the original code probably did that already. >> >> Yes, I actually didn't make any changes to this function. You can see I >> moved it from below since it is used in the following function. >> >> I'm not checking the return code from any of the read/write functions since >> the original driver didn't. Would you prefer I check and issue a warning? > > Not sure a warning would change much here. We should probably look at > why it was ok in the first place. A quick grep of the drivers directory shows that an overwhelming majority of pci_read_config_dword() and pci_write_config_dword() calls do not check the return code. For robustness, I agree with you that checking and returning the return code in this error checking function is important. I will return the error code if the read fails. It shouldn't be necessary to change the rest of the code though unless you feel strongly about updating the existing codebase. >> >> Thanks for reviewing! >> >>>> + if (val & VSE_CVP_STATUS_CFG_ERR) { >>>> + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n", >>>> + bytes); >>>> + return -EPROTO; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +static int altera_cvp_send_block(struct altera_cvp_conf *conf, >>>> + const u32 *data, size_t len) >>>> +{ >>>> + u32 mask, words = len / sizeof(u32); >>>> + int i, remainder; >>>> + >>>> + for (i = 0; i < words; i++) >>>> + conf->write_data(conf, *data++); >>>> + >>>> + /* write up to 3 trailing bytes, if any */ >>>> + remainder = len % sizeof(u32); >>>> + if (remainder) { >>>> + mask = BIT(remainder * 8) - 1; >>>> + if (mask) >>>> + conf->write_data(conf, *data & mask); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int altera_cvp_teardown(struct fpga_manager *mgr, >>>> struct fpga_image_info *info) >>>> { >>>> @@ -262,39 +297,29 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, >>>> return 0; >>>> } >>>> -static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) >>>> -{ >>>> - struct altera_cvp_conf *conf = mgr->priv; >>>> - u32 val; >>>> - >>>> - /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ >>>> - altera_read_config_dword(conf, VSE_CVP_STATUS, &val); >>>> - if (val & VSE_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; >>>> + size_t done, remaining, len; >>>> 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; >>>> + while (remaining) { >>>> + if (remaining >= sizeof(u32)) >>>> + len = sizeof(u32); >>>> + else >>>> + len = remaining; >>>> + >>>> + altera_cvp_send_block(conf, data, len); >>>> + data++; >>>> + done += len; >>>> + remaining -= len; >>>> /* >>>> * STEP 10 (optional) and STEP 11 >>>> @@ -312,11 +337,6 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, >>>> } >>>> } >>>> - /* 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); >>>> -- >>>> 2.7.4 >>>> >>> Cheers, >>> Moritz >>> >> > > Moritz > Thor ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts. 2019-07-24 16:59 ` Thor Thayer @ 2019-07-24 17:05 ` Moritz Fischer 0 siblings, 0 replies; 11+ messages in thread From: Moritz Fischer @ 2019-07-24 17:05 UTC (permalink / raw) To: Thor Thayer; +Cc: Moritz Fischer, richard.gong, agust, linux-fpga, linux-kernel Hi Thor, On Wed, Jul 24, 2019 at 11:59:12AM -0500, Thor Thayer wrote: > Hi Moritz, > > On 7/24/19 9:57 AM, Moritz Fischer wrote: > > On Tue, Jul 23, 2019 at 09:40:51AM -0500, Thor Thayer wrote: > > > Hi Moritz, > > > > > > On 7/21/19 7:59 PM, Moritz Fischer wrote: > > > > Thor, > > > > > > > > On Tue, Jul 16, 2019 at 05:48:06PM -0500, thor.thayer@linux.intel.com wrote: > > > > > From: Thor Thayer <thor.thayer@linux.intel.com> > > > > > > > > > > In preparation for adding newer V2 parts that use a FIFO, > > > > > reorganize altera_cvp_chk_error() and change the write > > > > > function to block based. > > > > > V2 parts have a block size matching the FIFO while older > > > > > V1 parts write a 32 bit word at a time. > > > > > > > > > > Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> > > > > > --- > > > > > v2 Remove inline function declaration > > > > > Reverse Christmas Tree format for local variables > > > > > --- > > > > > drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++----------------- > > > > > 1 file changed, 46 insertions(+), 26 deletions(-) > > > > > > > > > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c > > > > > index b78c90580071..37419d6b9915 100644 > > > > > --- a/drivers/fpga/altera-cvp.c > > > > > +++ b/drivers/fpga/altera-cvp.c > > > > > @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask, > > > > > return -ETIMEDOUT; > > > > > } > > > > > +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) > > > > > +{ > > > > > + struct altera_cvp_conf *conf = mgr->priv; > > > > > + u32 val; > > > > > + > > > > > + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */ > > > > > + altera_read_config_dword(conf, VSE_CVP_STATUS, &val); > > > > Same as in the other email, why can we ignore return values here. I > > > > think the original code probably did that already. > > > > > > Yes, I actually didn't make any changes to this function. You can see I > > > moved it from below since it is used in the following function. > > > > > > I'm not checking the return code from any of the read/write functions since > > > the original driver didn't. Would you prefer I check and issue a warning? > > > > Not sure a warning would change much here. We should probably look at > > why it was ok in the first place. > > A quick grep of the drivers directory shows that an overwhelming majority of > pci_read_config_dword() and pci_write_config_dword() calls do not check the > return code. Yeah I came to the same conclusion after looking around the codebase. > > For robustness, I agree with you that checking and returning the return code > in this error checking function is important. I will return the error code > if the read fails. Ok. > > It shouldn't be necessary to change the rest of the code though unless you > feel strongly about updating the existing codebase. Yeah not in this patchset. We'll look at it separately. Cheers, Moritz ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support 2019-07-16 22:48 [PATCHv2 0/3] fpga: altera-cvp: Add Stratix10 Support thor.thayer 2019-07-16 22:48 ` [PATCHv2 1/3] fpga: altera-cvp: Discover Vendor Specific offset thor.thayer 2019-07-16 22:48 ` [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts thor.thayer @ 2019-07-16 22:48 ` thor.thayer 2019-07-22 0:56 ` Moritz Fischer 2 siblings, 1 reply; 11+ messages in thread From: thor.thayer @ 2019-07-16 22:48 UTC (permalink / raw) To: mdf, richard.gong, agust; +Cc: linux-fpga, linux-kernel, Thor Thayer From: Thor Thayer <thor.thayer@linux.intel.com> Add Stratix10 specific functions that use a credit mechanism to throttle data to the CvP FIFOs. Add a private structure with function pointers for V1 vs V2 functions. Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> --- v2 Remove inline function declaration Reverse Christmas Tree format for local variables Remove mask from credit calculation Add commment for the delay(1) function in wait_for_credit() --- drivers/fpga/altera-cvp.c | 174 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 159 insertions(+), 15 deletions(-) diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c index 37419d6b9915..b4aa973ea046 100644 --- a/drivers/fpga/altera-cvp.c +++ b/drivers/fpga/altera-cvp.c @@ -43,16 +43,32 @@ #define VSE_CVP_PROG_CTRL 0x2c /* 32bit */ #define VSE_CVP_PROG_CTRL_CONFIG BIT(0) #define VSE_CVP_PROG_CTRL_START_XFER BIT(1) +#define VSE_CVP_PROG_CTRL_MASK GENMASK(1, 0) #define VSE_UNCOR_ERR_STATUS 0x34 /* 32bit */ #define VSE_UNCOR_ERR_CVP_CFG_ERR BIT(5) /* CVP_CONFIG_ERROR_LATCHED */ +/* V2 Defines */ +#define VSE_CVP_TX_CREDITS 0x49 /* 8bit */ + +#define CREDIT_TIMEOUT_US 20000 +#define V2_POLL_TIMEOUT_US 1000000 +#define V2_USER_TIMEOUT_US 500000 + +#define V1_POLL_TIMEOUT_US 10 + #define DRV_NAME "altera-cvp" #define ALTERA_CVP_MGR_NAME "Altera CvP FPGA Manager" +/* Write block sizes */ +#define ALTERA_CVP_V1_SIZE 4 +#define ALTERA_CVP_V2_SIZE 4096 + /* Optional CvP config error status check for debugging */ static bool altera_cvp_chkcfg; +struct cvp_priv; + struct altera_cvp_conf { struct fpga_manager *mgr; struct pci_dev *pci_dev; @@ -60,9 +76,26 @@ struct altera_cvp_conf { void (*write_data)(struct altera_cvp_conf *, u32); char mgr_name[64]; u8 numclks; + u8 current_credit_byte; u32 vsec_offset; + const struct cvp_priv *priv; +}; + +struct cvp_priv { + void (*switch_clk)(struct altera_cvp_conf *conf); + int (*clear_state)(struct altera_cvp_conf *conf); + int (*wait_credit)(struct fpga_manager *mgr, u32 blocks); + int block_size; + int poll_time_us; + int user_time_us; }; +static void altera_read_config_byte(struct altera_cvp_conf *conf, + int where, u8 *val) +{ + pci_read_config_byte(conf->pci_dev, conf->vsec_offset + where, val); +} + static void altera_read_config_dword(struct altera_cvp_conf *conf, int where, u32 *val) { @@ -155,6 +188,58 @@ static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) return 0; } +/* + * CvP Version2 Functions + * Recent Intel FPGAs use a credit mechanism to throttle incoming + * bitstreams and a different method of clearing the state. + */ + +static int altera_cvp_v2_clear_state(struct altera_cvp_conf *conf) +{ + u32 val; + + /* Clear the START_XFER and CVP_CONFIG bits */ + altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); + val &= ~VSE_CVP_PROG_CTRL_MASK; + altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); + + return altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, + conf->priv->poll_time_us); +} + +static int altera_cvp_v2_wait_for_credit(struct fpga_manager *mgr, + u32 blocks) +{ + struct altera_cvp_conf *conf = mgr->priv; + u8 val, delta_credit; + u32 count = 0; + int ret; + + do { + altera_read_config_byte(conf, VSE_CVP_TX_CREDITS, &val); + delta_credit = val - conf->current_credit_byte; + + ret = altera_cvp_chk_error(mgr, blocks * ALTERA_CVP_V2_SIZE); + if (ret) { + dev_err(&conf->pci_dev->dev, + "CE Bit error credits host[0x%x]:dev[0x%x]\n", + conf->current_credit_byte, val); + return -EAGAIN; + } + + if (count++ >= CREDIT_TIMEOUT_US) { + dev_err(&conf->pci_dev->dev, + "Timeout waiting for credit\n"); + return -ETIMEDOUT; + } + + /* Limit the traffic & ensure a timeout in usec */ + udelay(1); + } while (!delta_credit); + + return 0; +} + static int altera_cvp_send_block(struct altera_cvp_conf *conf, const u32 *data, size_t len) { @@ -196,10 +281,12 @@ static int altera_cvp_teardown(struct fpga_manager *mgr, * - 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 */ + if (conf->priv->switch_clk) + conf->priv->switch_clk(conf); /* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */ - ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, 10); + ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, + conf->priv->poll_time_us); if (ret) dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n"); @@ -261,7 +348,16 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, * STEP 3 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP */ - altera_cvp_dummy_write(conf); + if (conf->priv->switch_clk) + conf->priv->switch_clk(conf); + + if (conf->priv->clear_state) { + ret = conf->priv->clear_state(conf); + if (ret) { + dev_err(&mgr->dev, "Problem clearing out state\n"); + return ret; + } + } /* STEP 4 - set CVP_CONFIG bit */ altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); @@ -269,9 +365,10 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, val |= VSE_CVP_PROG_CTRL_CONFIG; altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); - /* STEP 5 - poll CVP_CONFIG READY for 1 with 10us timeout */ + /* STEP 5 - poll CVP_CONFIG READY for 1 with timeout */ ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, - VSE_CVP_STATUS_CFG_RDY, 10); + VSE_CVP_STATUS_CFG_RDY, + conf->priv->poll_time_us); if (ret) { dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n"); return ret; @@ -281,7 +378,16 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, * STEP 6 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP */ - altera_cvp_dummy_write(conf); + if (conf->priv->switch_clk) + conf->priv->switch_clk(conf); + + if (altera_cvp_chkcfg) { + ret = altera_cvp_chk_error(mgr, 0); + if (ret) { + dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n"); + return ret; + } + } /* STEP 7 - set START_XFER */ altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); @@ -289,11 +395,12 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); /* STEP 8 - start transfer (set CVP_NUMCLKS for bitstream) */ - altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); - val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; - val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; - altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); - + if (conf->priv->switch_clk) { + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); + val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; + val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); + } return 0; } @@ -311,15 +418,26 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, done = 0; while (remaining) { - if (remaining >= sizeof(u32)) - len = sizeof(u32); + /* Use credit throttling if available */ + if (conf->priv->wait_credit) { + status = conf->priv->wait_credit(mgr, done); + if (status) { + dev_err(&conf->pci_dev->dev, + "Wait Credit ERR: 0x%x\n", status); + return status; + } + } + + if (remaining >= conf->priv->block_size) + len = conf->priv->block_size; else len = remaining; altera_cvp_send_block(conf, data, len); - data++; + data += len / sizeof(u32); done += len; remaining -= len; + conf->current_credit_byte++; /* * STEP 10 (optional) and STEP 11 @@ -369,7 +487,8 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr, /* STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits */ mask = VSE_CVP_STATUS_PLD_CLK_IN_USE | VSE_CVP_STATUS_USERMODE; - ret = altera_cvp_wait_status(conf, mask, mask, TIMEOUT_US); + ret = altera_cvp_wait_status(conf, mask, mask, + conf->priv->user_time_us); if (ret) dev_err(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n"); @@ -383,6 +502,24 @@ static const struct fpga_manager_ops altera_cvp_ops = { .write_complete = altera_cvp_write_complete, }; +static const struct cvp_priv cvp_priv_v1 = { + .switch_clk = altera_cvp_dummy_write, + .clear_state = NULL, + .wait_credit = NULL, + .block_size = ALTERA_CVP_V1_SIZE, + .poll_time_us = V1_POLL_TIMEOUT_US, + .user_time_us = TIMEOUT_US, +}; + +static const struct cvp_priv cvp_priv_v2 = { + .switch_clk = NULL, + .clear_state = altera_cvp_v2_clear_state, + .wait_credit = altera_cvp_v2_wait_for_credit, + .block_size = ALTERA_CVP_V2_SIZE, + .poll_time_us = V2_POLL_TIMEOUT_US, + .user_time_us = V2_USER_TIMEOUT_US, +}; + static ssize_t chkcfg_show(struct device_driver *dev, char *buf) { return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg); @@ -485,6 +622,13 @@ static int altera_cvp_probe(struct pci_dev *pdev, conf->pci_dev = pdev; conf->write_data = altera_cvp_write_data_iomem; + if (conf->vsec_offset == 0x200) + conf->priv = &cvp_priv_v1; + else + conf->priv = &cvp_priv_v2; + + conf->current_credit_byte = 0; + conf->map = pci_iomap(pdev, CVP_BAR, 0); if (!conf->map) { dev_warn(&pdev->dev, "Mapping CVP BAR failed\n"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support 2019-07-16 22:48 ` [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support thor.thayer @ 2019-07-22 0:56 ` Moritz Fischer 2019-07-23 14:38 ` Thor Thayer 0 siblings, 1 reply; 11+ messages in thread From: Moritz Fischer @ 2019-07-22 0:56 UTC (permalink / raw) To: thor.thayer; +Cc: mdf, richard.gong, agust, linux-fpga, linux-kernel Hi Thor, looks mostly good. On Tue, Jul 16, 2019 at 05:48:07PM -0500, thor.thayer@linux.intel.com wrote: > From: Thor Thayer <thor.thayer@linux.intel.com> > > Add Stratix10 specific functions that use a credit mechanism > to throttle data to the CvP FIFOs. Add a private structure > with function pointers for V1 vs V2 functions. > > Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> > --- > v2 Remove inline function declaration > Reverse Christmas Tree format for local variables > Remove mask from credit calculation > Add commment for the delay(1) function in wait_for_credit() > --- > drivers/fpga/altera-cvp.c | 174 ++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 159 insertions(+), 15 deletions(-) > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c > index 37419d6b9915..b4aa973ea046 100644 > --- a/drivers/fpga/altera-cvp.c > +++ b/drivers/fpga/altera-cvp.c > @@ -43,16 +43,32 @@ > #define VSE_CVP_PROG_CTRL 0x2c /* 32bit */ > #define VSE_CVP_PROG_CTRL_CONFIG BIT(0) > #define VSE_CVP_PROG_CTRL_START_XFER BIT(1) > +#define VSE_CVP_PROG_CTRL_MASK GENMASK(1, 0) > > #define VSE_UNCOR_ERR_STATUS 0x34 /* 32bit */ > #define VSE_UNCOR_ERR_CVP_CFG_ERR BIT(5) /* CVP_CONFIG_ERROR_LATCHED */ > > +/* V2 Defines */ > +#define VSE_CVP_TX_CREDITS 0x49 /* 8bit */ > + > +#define CREDIT_TIMEOUT_US 20000 > +#define V2_POLL_TIMEOUT_US 1000000 > +#define V2_USER_TIMEOUT_US 500000 > + > +#define V1_POLL_TIMEOUT_US 10 > + > #define DRV_NAME "altera-cvp" > #define ALTERA_CVP_MGR_NAME "Altera CvP FPGA Manager" > > +/* Write block sizes */ > +#define ALTERA_CVP_V1_SIZE 4 > +#define ALTERA_CVP_V2_SIZE 4096 > + > /* Optional CvP config error status check for debugging */ > static bool altera_cvp_chkcfg; > > +struct cvp_priv; > + > struct altera_cvp_conf { > struct fpga_manager *mgr; > struct pci_dev *pci_dev; > @@ -60,9 +76,26 @@ struct altera_cvp_conf { > void (*write_data)(struct altera_cvp_conf *, u32); > char mgr_name[64]; > u8 numclks; > + u8 current_credit_byte; > u32 vsec_offset; > + const struct cvp_priv *priv; > +}; > + > +struct cvp_priv { > + void (*switch_clk)(struct altera_cvp_conf *conf); > + int (*clear_state)(struct altera_cvp_conf *conf); > + int (*wait_credit)(struct fpga_manager *mgr, u32 blocks); > + int block_size; > + int poll_time_us; > + int user_time_us; > }; > > +static void altera_read_config_byte(struct altera_cvp_conf *conf, > + int where, u8 *val) > +{ > + pci_read_config_byte(conf->pci_dev, conf->vsec_offset + where, val); What happens to the return values here? I don't recall pci_read_config_byte being a void function. > +} > + > static void altera_read_config_dword(struct altera_cvp_conf *conf, > int where, u32 *val) > { > @@ -155,6 +188,58 @@ static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) > return 0; > } > > +/* > + * CvP Version2 Functions > + * Recent Intel FPGAs use a credit mechanism to throttle incoming > + * bitstreams and a different method of clearing the state. > + */ > + > +static int altera_cvp_v2_clear_state(struct altera_cvp_conf *conf) > +{ > + u32 val; > + > + /* Clear the START_XFER and CVP_CONFIG bits */ > + altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); > + val &= ~VSE_CVP_PROG_CTRL_MASK; > + altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); > + > + return altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, > + conf->priv->poll_time_us); > +} > + > +static int altera_cvp_v2_wait_for_credit(struct fpga_manager *mgr, > + u32 blocks) > +{ > + struct altera_cvp_conf *conf = mgr->priv; > + u8 val, delta_credit; > + u32 count = 0; > + int ret; > + > + do { > + altera_read_config_byte(conf, VSE_CVP_TX_CREDITS, &val); > + delta_credit = val - conf->current_credit_byte; > + > + ret = altera_cvp_chk_error(mgr, blocks * ALTERA_CVP_V2_SIZE); > + if (ret) { > + dev_err(&conf->pci_dev->dev, > + "CE Bit error credits host[0x%x]:dev[0x%x]\n", > + conf->current_credit_byte, val); > + return -EAGAIN; > + } > + > + if (count++ >= CREDIT_TIMEOUT_US) { > + dev_err(&conf->pci_dev->dev, > + "Timeout waiting for credit\n"); > + return -ETIMEDOUT; > + } > + > + /* Limit the traffic & ensure a timeout in usec */ > + udelay(1); > + } while (!delta_credit); > + > + return 0; > +} > + > static int altera_cvp_send_block(struct altera_cvp_conf *conf, > const u32 *data, size_t len) > { > @@ -196,10 +281,12 @@ static int altera_cvp_teardown(struct fpga_manager *mgr, > * - 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 */ > + if (conf->priv->switch_clk) > + conf->priv->switch_clk(conf); > > /* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */ > - ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, 10); > + ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, > + conf->priv->poll_time_us); > if (ret) > dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n"); > > @@ -261,7 +348,16 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, > * STEP 3 > * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP > */ > - altera_cvp_dummy_write(conf); > + if (conf->priv->switch_clk) > + conf->priv->switch_clk(conf); > + > + if (conf->priv->clear_state) { > + ret = conf->priv->clear_state(conf); > + if (ret) { > + dev_err(&mgr->dev, "Problem clearing out state\n"); > + return ret; > + } > + } > > /* STEP 4 - set CVP_CONFIG bit */ > altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); > @@ -269,9 +365,10 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, > val |= VSE_CVP_PROG_CTRL_CONFIG; > altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); > > - /* STEP 5 - poll CVP_CONFIG READY for 1 with 10us timeout */ > + /* STEP 5 - poll CVP_CONFIG READY for 1 with timeout */ > ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, > - VSE_CVP_STATUS_CFG_RDY, 10); > + VSE_CVP_STATUS_CFG_RDY, > + conf->priv->poll_time_us); > if (ret) { > dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n"); > return ret; > @@ -281,7 +378,16 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, > * STEP 6 > * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP > */ > - altera_cvp_dummy_write(conf); > + if (conf->priv->switch_clk) > + conf->priv->switch_clk(conf); > + > + if (altera_cvp_chkcfg) { > + ret = altera_cvp_chk_error(mgr, 0); > + if (ret) { > + dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n"); > + return ret; > + } > + } > > /* STEP 7 - set START_XFER */ > altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); > @@ -289,11 +395,12 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, > altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); > > /* STEP 8 - start transfer (set CVP_NUMCLKS for bitstream) */ > - altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); > - val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; > - val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; > - altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); > - > + if (conf->priv->switch_clk) { > + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); > + val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; > + val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; > + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); > + } > return 0; > } > > @@ -311,15 +418,26 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, > done = 0; > > while (remaining) { > - if (remaining >= sizeof(u32)) > - len = sizeof(u32); > + /* Use credit throttling if available */ > + if (conf->priv->wait_credit) { > + status = conf->priv->wait_credit(mgr, done); > + if (status) { > + dev_err(&conf->pci_dev->dev, > + "Wait Credit ERR: 0x%x\n", status); > + return status; > + } > + } > + > + if (remaining >= conf->priv->block_size) > + len = conf->priv->block_size; > else > len = remaining; > > altera_cvp_send_block(conf, data, len); > - data++; > + data += len / sizeof(u32); > done += len; > remaining -= len; > + conf->current_credit_byte++; > > /* > * STEP 10 (optional) and STEP 11 > @@ -369,7 +487,8 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr, > > /* STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits */ > mask = VSE_CVP_STATUS_PLD_CLK_IN_USE | VSE_CVP_STATUS_USERMODE; > - ret = altera_cvp_wait_status(conf, mask, mask, TIMEOUT_US); > + ret = altera_cvp_wait_status(conf, mask, mask, > + conf->priv->user_time_us); > if (ret) > dev_err(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n"); > > @@ -383,6 +502,24 @@ static const struct fpga_manager_ops altera_cvp_ops = { > .write_complete = altera_cvp_write_complete, > }; > > +static const struct cvp_priv cvp_priv_v1 = { > + .switch_clk = altera_cvp_dummy_write, > + .clear_state = NULL, > + .wait_credit = NULL, > + .block_size = ALTERA_CVP_V1_SIZE, > + .poll_time_us = V1_POLL_TIMEOUT_US, > + .user_time_us = TIMEOUT_US, > +}; > + > +static const struct cvp_priv cvp_priv_v2 = { > + .switch_clk = NULL, > + .clear_state = altera_cvp_v2_clear_state, > + .wait_credit = altera_cvp_v2_wait_for_credit, > + .block_size = ALTERA_CVP_V2_SIZE, > + .poll_time_us = V2_POLL_TIMEOUT_US, > + .user_time_us = V2_USER_TIMEOUT_US, > +}; > + > static ssize_t chkcfg_show(struct device_driver *dev, char *buf) > { > return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg); > @@ -485,6 +622,13 @@ static int altera_cvp_probe(struct pci_dev *pdev, > conf->pci_dev = pdev; > conf->write_data = altera_cvp_write_data_iomem; > > + if (conf->vsec_offset == 0x200) > + conf->priv = &cvp_priv_v1; > + else > + conf->priv = &cvp_priv_v2; > + > + conf->current_credit_byte = 0; > + > conf->map = pci_iomap(pdev, CVP_BAR, 0); > if (!conf->map) { > dev_warn(&pdev->dev, "Mapping CVP BAR failed\n"); > -- > 2.7.4 > Thanks, Moritz ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support 2019-07-22 0:56 ` Moritz Fischer @ 2019-07-23 14:38 ` Thor Thayer 0 siblings, 0 replies; 11+ messages in thread From: Thor Thayer @ 2019-07-23 14:38 UTC (permalink / raw) To: Moritz Fischer; +Cc: richard.gong, agust, linux-fpga, linux-kernel Hi Moritz, On 7/21/19 7:56 PM, Moritz Fischer wrote: > Hi Thor, > > looks mostly good. > > On Tue, Jul 16, 2019 at 05:48:07PM -0500, thor.thayer@linux.intel.com wrote: >> From: Thor Thayer <thor.thayer@linux.intel.com> >> >> Add Stratix10 specific functions that use a credit mechanism >> to throttle data to the CvP FIFOs. Add a private structure >> with function pointers for V1 vs V2 functions. >> >> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> >> --- >> v2 Remove inline function declaration >> Reverse Christmas Tree format for local variables >> Remove mask from credit calculation >> Add commment for the delay(1) function in wait_for_credit() >> --- >> drivers/fpga/altera-cvp.c | 174 ++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 159 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c >> index 37419d6b9915..b4aa973ea046 100644 >> --- a/drivers/fpga/altera-cvp.c >> +++ b/drivers/fpga/altera-cvp.c >> @@ -43,16 +43,32 @@ >> #define VSE_CVP_PROG_CTRL 0x2c /* 32bit */ >> #define VSE_CVP_PROG_CTRL_CONFIG BIT(0) >> #define VSE_CVP_PROG_CTRL_START_XFER BIT(1) >> +#define VSE_CVP_PROG_CTRL_MASK GENMASK(1, 0) >> >> #define VSE_UNCOR_ERR_STATUS 0x34 /* 32bit */ >> #define VSE_UNCOR_ERR_CVP_CFG_ERR BIT(5) /* CVP_CONFIG_ERROR_LATCHED */ >> >> +/* V2 Defines */ >> +#define VSE_CVP_TX_CREDITS 0x49 /* 8bit */ >> + >> +#define CREDIT_TIMEOUT_US 20000 >> +#define V2_POLL_TIMEOUT_US 1000000 >> +#define V2_USER_TIMEOUT_US 500000 >> + >> +#define V1_POLL_TIMEOUT_US 10 >> + >> #define DRV_NAME "altera-cvp" >> #define ALTERA_CVP_MGR_NAME "Altera CvP FPGA Manager" >> >> +/* Write block sizes */ >> +#define ALTERA_CVP_V1_SIZE 4 >> +#define ALTERA_CVP_V2_SIZE 4096 >> + >> /* Optional CvP config error status check for debugging */ >> static bool altera_cvp_chkcfg; >> >> +struct cvp_priv; >> + >> struct altera_cvp_conf { >> struct fpga_manager *mgr; >> struct pci_dev *pci_dev; >> @@ -60,9 +76,26 @@ struct altera_cvp_conf { >> void (*write_data)(struct altera_cvp_conf *, u32); >> char mgr_name[64]; >> u8 numclks; >> + u8 current_credit_byte; >> u32 vsec_offset; >> + const struct cvp_priv *priv; >> +}; >> + >> +struct cvp_priv { >> + void (*switch_clk)(struct altera_cvp_conf *conf); >> + int (*clear_state)(struct altera_cvp_conf *conf); >> + int (*wait_credit)(struct fpga_manager *mgr, u32 blocks); >> + int block_size; >> + int poll_time_us; >> + int user_time_us; >> }; >> >> +static void altera_read_config_byte(struct altera_cvp_conf *conf, >> + int where, u8 *val) >> +{ >> + pci_read_config_byte(conf->pci_dev, conf->vsec_offset + where, val); > > What happens to the return values here? I don't recall > pci_read_config_byte being a void function. Yes, you are right. These should return the same return code since it is a veneer. I was copying the format of the write_data() function pointer in the altera_cvp_conf structure which returned void but that was sloppy. I will fix and submit v3. Thank you for reviewing! Thor > >> +} >> + >> static void altera_read_config_dword(struct altera_cvp_conf *conf, >> int where, u32 *val) >> { >> @@ -155,6 +188,58 @@ static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) >> return 0; >> } >> >> +/* >> + * CvP Version2 Functions >> + * Recent Intel FPGAs use a credit mechanism to throttle incoming >> + * bitstreams and a different method of clearing the state. >> + */ >> + >> +static int altera_cvp_v2_clear_state(struct altera_cvp_conf *conf) >> +{ >> + u32 val; >> + >> + /* Clear the START_XFER and CVP_CONFIG bits */ >> + altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); >> + val &= ~VSE_CVP_PROG_CTRL_MASK; >> + altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); >> + >> + return altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, >> + conf->priv->poll_time_us); >> +} >> + >> +static int altera_cvp_v2_wait_for_credit(struct fpga_manager *mgr, >> + u32 blocks) >> +{ >> + struct altera_cvp_conf *conf = mgr->priv; >> + u8 val, delta_credit; >> + u32 count = 0; >> + int ret; >> + >> + do { >> + altera_read_config_byte(conf, VSE_CVP_TX_CREDITS, &val); >> + delta_credit = val - conf->current_credit_byte; >> + >> + ret = altera_cvp_chk_error(mgr, blocks * ALTERA_CVP_V2_SIZE); >> + if (ret) { >> + dev_err(&conf->pci_dev->dev, >> + "CE Bit error credits host[0x%x]:dev[0x%x]\n", >> + conf->current_credit_byte, val); >> + return -EAGAIN; >> + } >> + >> + if (count++ >= CREDIT_TIMEOUT_US) { >> + dev_err(&conf->pci_dev->dev, >> + "Timeout waiting for credit\n"); >> + return -ETIMEDOUT; >> + } >> + >> + /* Limit the traffic & ensure a timeout in usec */ >> + udelay(1); >> + } while (!delta_credit); >> + >> + return 0; >> +} >> + >> static int altera_cvp_send_block(struct altera_cvp_conf *conf, >> const u32 *data, size_t len) >> { >> @@ -196,10 +281,12 @@ static int altera_cvp_teardown(struct fpga_manager *mgr, >> * - 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 */ >> + if (conf->priv->switch_clk) >> + conf->priv->switch_clk(conf); >> >> /* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */ >> - ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, 10); >> + ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0, >> + conf->priv->poll_time_us); >> if (ret) >> dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n"); >> >> @@ -261,7 +348,16 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, >> * STEP 3 >> * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP >> */ >> - altera_cvp_dummy_write(conf); >> + if (conf->priv->switch_clk) >> + conf->priv->switch_clk(conf); >> + >> + if (conf->priv->clear_state) { >> + ret = conf->priv->clear_state(conf); >> + if (ret) { >> + dev_err(&mgr->dev, "Problem clearing out state\n"); >> + return ret; >> + } >> + } >> >> /* STEP 4 - set CVP_CONFIG bit */ >> altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); >> @@ -269,9 +365,10 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, >> val |= VSE_CVP_PROG_CTRL_CONFIG; >> altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); >> >> - /* STEP 5 - poll CVP_CONFIG READY for 1 with 10us timeout */ >> + /* STEP 5 - poll CVP_CONFIG READY for 1 with timeout */ >> ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, >> - VSE_CVP_STATUS_CFG_RDY, 10); >> + VSE_CVP_STATUS_CFG_RDY, >> + conf->priv->poll_time_us); >> if (ret) { >> dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n"); >> return ret; >> @@ -281,7 +378,16 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, >> * STEP 6 >> * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP >> */ >> - altera_cvp_dummy_write(conf); >> + if (conf->priv->switch_clk) >> + conf->priv->switch_clk(conf); >> + >> + if (altera_cvp_chkcfg) { >> + ret = altera_cvp_chk_error(mgr, 0); >> + if (ret) { >> + dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n"); >> + return ret; >> + } >> + } >> >> /* STEP 7 - set START_XFER */ >> altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val); >> @@ -289,11 +395,12 @@ static int altera_cvp_write_init(struct fpga_manager *mgr, >> altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val); >> >> /* STEP 8 - start transfer (set CVP_NUMCLKS for bitstream) */ >> - altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); >> - val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; >> - val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; >> - altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); >> - >> + if (conf->priv->switch_clk) { >> + altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val); >> + val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK; >> + val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF; >> + altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val); >> + } >> return 0; >> } >> >> @@ -311,15 +418,26 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf, >> done = 0; >> >> while (remaining) { >> - if (remaining >= sizeof(u32)) >> - len = sizeof(u32); >> + /* Use credit throttling if available */ >> + if (conf->priv->wait_credit) { >> + status = conf->priv->wait_credit(mgr, done); >> + if (status) { >> + dev_err(&conf->pci_dev->dev, >> + "Wait Credit ERR: 0x%x\n", status); >> + return status; >> + } >> + } >> + >> + if (remaining >= conf->priv->block_size) >> + len = conf->priv->block_size; >> else >> len = remaining; >> >> altera_cvp_send_block(conf, data, len); >> - data++; >> + data += len / sizeof(u32); >> done += len; >> remaining -= len; >> + conf->current_credit_byte++; >> >> /* >> * STEP 10 (optional) and STEP 11 >> @@ -369,7 +487,8 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr, >> >> /* STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits */ >> mask = VSE_CVP_STATUS_PLD_CLK_IN_USE | VSE_CVP_STATUS_USERMODE; >> - ret = altera_cvp_wait_status(conf, mask, mask, TIMEOUT_US); >> + ret = altera_cvp_wait_status(conf, mask, mask, >> + conf->priv->user_time_us); >> if (ret) >> dev_err(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n"); >> >> @@ -383,6 +502,24 @@ static const struct fpga_manager_ops altera_cvp_ops = { >> .write_complete = altera_cvp_write_complete, >> }; >> >> +static const struct cvp_priv cvp_priv_v1 = { >> + .switch_clk = altera_cvp_dummy_write, >> + .clear_state = NULL, >> + .wait_credit = NULL, >> + .block_size = ALTERA_CVP_V1_SIZE, >> + .poll_time_us = V1_POLL_TIMEOUT_US, >> + .user_time_us = TIMEOUT_US, >> +}; >> + >> +static const struct cvp_priv cvp_priv_v2 = { >> + .switch_clk = NULL, >> + .clear_state = altera_cvp_v2_clear_state, >> + .wait_credit = altera_cvp_v2_wait_for_credit, >> + .block_size = ALTERA_CVP_V2_SIZE, >> + .poll_time_us = V2_POLL_TIMEOUT_US, >> + .user_time_us = V2_USER_TIMEOUT_US, >> +}; >> + >> static ssize_t chkcfg_show(struct device_driver *dev, char *buf) >> { >> return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg); >> @@ -485,6 +622,13 @@ static int altera_cvp_probe(struct pci_dev *pdev, >> conf->pci_dev = pdev; >> conf->write_data = altera_cvp_write_data_iomem; >> >> + if (conf->vsec_offset == 0x200) >> + conf->priv = &cvp_priv_v1; >> + else >> + conf->priv = &cvp_priv_v2; >> + >> + conf->current_credit_byte = 0; >> + >> conf->map = pci_iomap(pdev, CVP_BAR, 0); >> if (!conf->map) { >> dev_warn(&pdev->dev, "Mapping CVP BAR failed\n"); >> -- >> 2.7.4 >> > > Thanks, > Moritz > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-07-24 17:05 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-16 22:48 [PATCHv2 0/3] fpga: altera-cvp: Add Stratix10 Support thor.thayer 2019-07-16 22:48 ` [PATCHv2 1/3] fpga: altera-cvp: Discover Vendor Specific offset thor.thayer 2019-07-16 22:48 ` [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts thor.thayer 2019-07-22 0:59 ` Moritz Fischer 2019-07-23 14:40 ` Thor Thayer 2019-07-24 14:57 ` Moritz Fischer 2019-07-24 16:59 ` Thor Thayer 2019-07-24 17:05 ` Moritz Fischer 2019-07-16 22:48 ` [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support thor.thayer 2019-07-22 0:56 ` Moritz Fischer 2019-07-23 14:38 ` Thor Thayer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).