Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts.
From: Thor Thayer
Date: Wed Jul 24 2019 - 12:57:16 EST
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@xxxxxxxxxxxxxxx wrote:
From: Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
---
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