[PATCH] fpga: altera-cvp: allow interrupt to continue next time

[PATCH] fpga: altera-cvp: allow interrupt to continue next time

[tien.sung.ang]

From: Dinh Nguyen <dinh.nguyen@intel.com>

CFG_READY signal/bit may time-out due to firmware not responding
within the given time-out. This time varies due to numerous
factors like size of bitstream and others.
This time-out error does not impact the result of the CvP
previous transactions. The CvP driver shall then, respond with
EAGAIN instead Time out error.

Signed-off-by: Dinh Nguyen <dinh.nguyen@intel.com>
Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
---
 drivers/fpga/altera-cvp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 4ffb9da537d8..d74ff63c61e8 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -309,10 +309,22 @@ static int altera_cvp_teardown(struct fpga_manager *mgr,
 	/* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */
 	ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0,
 				     conf->priv->poll_time_us);
-	if (ret)
+	if (ret) {
 		dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n");
+		goto error_path;
+	}
 
 	return ret;
+
+error_path:
+	/* reset CVP_MODE and HIP_CLK_SEL bit */
+	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;
+	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
+
+	return -EAGAIN;
+
 }
 
 static int altera_cvp_write_init(struct fpga_manager *mgr,
-- 
2.25.1

Re: [PATCH] fpga: altera-cvp: allow interrupt to continue next time

[Tom Rix]

On 5/18/22 12:38 AM, tien.sung.ang@intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@intel.com>
>
> CFG_READY signal/bit may time-out due to firmware not responding
> within the given time-out. This time varies due to numerous
> factors like size of bitstream and others.
> This time-out error does not impact the result of the CvP
> previous transactions. The CvP driver shall then, respond with
> EAGAIN instead Time out error.
>
> Signed-off-by: Dinh Nguyen <dinh.nguyen@intel.com>
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
>   drivers/fpga/altera-cvp.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..d74ff63c61e8 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -309,10 +309,22 @@ static int altera_cvp_teardown(struct fpga_manager *mgr,
>   	/* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */
>   	ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0,
>   				     conf->priv->poll_time_us);
> -	if (ret)
> +	if (ret) {
>   		dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n");
> +		goto error_path;
> +	}
>   
>   	return ret;
> +
> +error_path:
> +	/* reset CVP_MODE and HIP_CLK_SEL bit */
> +	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;
> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
> +
> +	return -EAGAIN;

This will set fpga_mgr->state to *_ERR.

Is this ok or do you think we need a couple new of *_BUSY enums ?

Tom

> +
>   }
>   
>   static int altera_cvp_write_init(struct fpga_manager *mgr,

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

Thanks for bringing this up. Yes, you are right that the fpga_mgr sees this
as an error irrespective of the value. The CvP driver is changed now to just
indicate the correct error which recommends a retry. To me understanding,
EAGAIN was this. The fpga manager now looks like is going to return a CvP
failure in short. 
A BUSY state does not seem to be able to solve this issue. 
Even an extended time-out didn't resolve this error state. The current time-out
is set to 10seconds.   
However, the main objective is to also handle the error if the CvP firmware
is not responsive. The error_path flow is to reset the CVP mode and HIP_CLK_SEL bit
as recommended by the firmware engineers. 
The flow prescribed here is also an identical copy of working CvP driver 
which is also owned by Intel. This driver is a downstream driver which is 
not part of the Linux kernel. We are now porting this differences over to 
the current upstream CvP driver. 

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Thu, May 19, 2022 at 05:39:07PM +0800, tien.sung.ang@intel.com wrote:
> Thanks for bringing this up. Yes, you are right that the fpga_mgr sees this
> as an error irrespective of the value. The CvP driver is changed now to just
> indicate the correct error which recommends a retry. To me understanding,
> EAGAIN was this. The fpga manager now looks like is going to return a CvP
> failure in short. 
> A BUSY state does not seem to be able to solve this issue. 
> Even an extended time-out didn't resolve this error state. The current time-out
> is set to 10seconds.   
> However, the main objective is to also handle the error if the CvP firmware
> is not responsive. The error_path flow is to reset the CVP mode and HIP_CLK_SEL bit

Please add your main objective to commit message.

Thanks,
Yilun

> as recommended by the firmware engineers. 
> The flow prescribed here is also an identical copy of working CvP driver 
> which is also owned by Intel. This driver is a downstream driver which is 
> not part of the Linux kernel. We are now porting this differences over to 
> the current upstream CvP driver. 

[PATCH v2] fpga: altera-cvp: allow interrupt to continue next time

[tien.sung.ang]

From: Dinh Nguyen <dinh.nguyen@intel.com>

The main objective of this change is to perform error handling
if the CvP firmware becomes unresponsive. The error_path flow
resets the CvP mode and HIP_CLK_SEL bit.

CFG_READY signal/bit may time-out due to firmware not responding
within the given time-out. This time varies due to numerous
factors like size of bitstream and others.
This time-out error may or may not impact the result of the CvP
previous transactions. The CvP driver shall then, respond with
EAGAIN instead Time out error.

Signed-off-by: Dinh Nguyen <dinh.nguyen@intel.com>
Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
---

changelog v2:
* Amend the commit message

---
 drivers/fpga/altera-cvp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 4ffb9da537d8..d74ff63c61e8 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -309,10 +309,22 @@ static int altera_cvp_teardown(struct fpga_manager *mgr,
 	/* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */
 	ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0,
 				     conf->priv->poll_time_us);
-	if (ret)
+	if (ret) {
 		dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n");
+		goto error_path;
+	}
 
 	return ret;
+
+error_path:
+	/* reset CVP_MODE and HIP_CLK_SEL bit */
+	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;
+	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
+
+	return -EAGAIN;
+
 }
 
 static int altera_cvp_write_init(struct fpga_manager *mgr,
-- 
2.25.1

Re: [PATCH v2] fpga: altera-cvp: allow interrupt to continue next time

[Xu Yilun]

On Wed, Jun 01, 2022 at 09:40:27AM +0800, tien.sung.ang@intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@intel.com>
> 
> The main objective of this change is to perform error handling
> if the CvP firmware becomes unresponsive. The error_path flow
> resets the CvP mode and HIP_CLK_SEL bit.
> 
> CFG_READY signal/bit may time-out due to firmware not responding
> within the given time-out. This time varies due to numerous
> factors like size of bitstream and others.
> This time-out error may or may not impact the result of the CvP
> previous transactions. The CvP driver shall then, respond with
> EAGAIN instead Time out error.
> 
> Signed-off-by: Dinh Nguyen <dinh.nguyen@intel.com>
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
> 
> changelog v2:
> * Amend the commit message
> 
> ---
>  drivers/fpga/altera-cvp.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..d74ff63c61e8 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -309,10 +309,22 @@ static int altera_cvp_teardown(struct fpga_manager *mgr,
>  	/* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */
>  	ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0,
>  				     conf->priv->poll_time_us);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n");
> +		goto error_path;

I assume the error handling is specific to CFG_RDY timeout, is it? Then it
could be embedded in this code block.

And also the -EAGAIN ret, please only return it in this code block.

Usually the goto error path is for common fail out.

> +	}
>  
>  	return ret;
> +
> +error_path:
> +	/* reset CVP_MODE and HIP_CLK_SEL bit */
> +	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;
> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
> +
> +	return -EAGAIN;

Please still specify the reason for -EAGAIN rather than timeout.

Thanks,
Yilun

> +
>  }
>  
>  static int altera_cvp_write_init(struct fpga_manager *mgr,
> -- 
> 2.25.1

Re: [PATCH] fpga: altera-cvp: allow interrupt to continue next time

[Xu Yilun]

On Wed, May 18, 2022 at 03:38:44PM +0800, tien.sung.ang@intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@intel.com>
> 
> CFG_READY signal/bit may time-out due to firmware not responding
> within the given time-out. This time varies due to numerous
> factors like size of bitstream and others.
> This time-out error does not impact the result of the CvP
> previous transactions. The CvP driver shall then, respond with

Do you mean the reprogramming is successful even if you find the time
out in write_complete()? Then return 0 is better?

And could you specify what the time-out mean on write_init() phase?

Thanks,
Yilun

> EAGAIN instead Time out error.
> 
> Signed-off-by: Dinh Nguyen <dinh.nguyen@intel.com>
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
>  drivers/fpga/altera-cvp.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..d74ff63c61e8 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -309,10 +309,22 @@ static int altera_cvp_teardown(struct fpga_manager *mgr,
>  	/* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */
>  	ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0,
>  				     conf->priv->poll_time_us);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n");
> +		goto error_path;
> +	}
>  
>  	return ret;
> +
> +error_path:
> +	/* reset CVP_MODE and HIP_CLK_SEL bit */
> +	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;
> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
> +
> +	return -EAGAIN;
> +
>  }
>  
>  static int altera_cvp_write_init(struct fpga_manager *mgr,
> -- 
> 2.25.1

Re: [PATCH] fpga: altera-cvp: allow interrupt to continue next time

[tien.sung.ang]

>> CFG_READY signal/bit may time-out due to firmware not responding
>> within the given time-out. This time varies due to numerous
>> factors like size of bitstream and others.
>> This time-out error does not impact the result of the CvP
>> previous transactions. The CvP driver shall then, respond with

>Do you mean the reprogramming is successful even if you find the time
>out in write_complete()? Then return 0 is better?
Based on the information given by the Intel FPGA firmware team,
CFG_READY is essential to indicate if the current FPGA 
configuration session is indeed a success. There are 
cases we test in the lab whereby, CFG_READY stays invalid and
the tests performed subsequently to verify the FPGA functionality
could not detect the failed session. A failed FPGA 
configuration session means, the new bitstream wasn't 
successfully configured and tests ran later will just be passing
on the previous working bitstream version. In short, CFG_READY
is esential, and an error indicating the time-out is a must.
Another example, using an incorrect SOF/Design FPGA results
in CFG_READY being invalid. The user must be informed of a 
potential error. 
I will correct the wordings i used earlier that says that
the timoeut error does not impact the results of the CvP
previous transactions. It may so if the firmware has some sort
of error. 

>And could you specify what the time-out mean on write_init() phase?
I could not really understand your question. We set huge 
time-outs of ~10seconds. Every wait for the firmware to respond
is potentially a hazard. The firmware CvP is has it's limitation
unfortunately. 

Re: [PATCH] fpga: altera-cvp: allow interrupt to continue next time

[Xu Yilun]

On Tue, May 31, 2022 at 10:20:04AM +0800, tien.sung.ang@intel.com wrote:
> >> CFG_READY signal/bit may time-out due to firmware not responding
> >> within the given time-out. This time varies due to numerous
> >> factors like size of bitstream and others.
> >> This time-out error does not impact the result of the CvP
> >> previous transactions. The CvP driver shall then, respond with
> 
> >Do you mean the reprogramming is successful even if you find the time
> >out in write_complete()? Then return 0 is better?
> Based on the information given by the Intel FPGA firmware team,
> CFG_READY is essential to indicate if the current FPGA 
> configuration session is indeed a success. There are 
> cases we test in the lab whereby, CFG_READY stays invalid and
> the tests performed subsequently to verify the FPGA functionality
> could not detect the failed session. A failed FPGA 
> configuration session means, the new bitstream wasn't 
> successfully configured and tests ran later will just be passing
> on the previous working bitstream version. In short, CFG_READY
> is esential, and an error indicating the time-out is a must.
> Another example, using an incorrect SOF/Design FPGA results
> in CFG_READY being invalid. The user must be informed of a 
> potential error. 
> I will correct the wordings i used earlier that says that
> the timoeut error does not impact the results of the CvP
> previous transactions. It may so if the firmware has some sort
> of error. 

Understood. But with your new comment why you must change the error
code to -EAGAIN rather than timeout?

I think you may change your commit message. The main change is adding
the error handling. The error code change is minor, even not necessary
if you don't have a strong reason.

Thanks,
Yilun

> 
> >And could you specify what the time-out mean on write_init() phase?
> I could not really understand your question. We set huge 
> time-outs of ~10seconds. Every wait for the firmware to respond
> is potentially a hazard. The firmware CvP is has it's limitation
> unfortunately. 

[PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

From: Ang Tien Sung <tien.sung.ang@intel.com>

To support the error handling of a truncated bitstream sent.
The current AIB CvP firmware is not capable of handling a
data stream smaller than 4096bytes. The firmware's limitation
causes a hung-up as it's DMA engine waits forever for the
completion of the instructed 4096bytes.
To resolve this design limitation, both firmware and CvP
driver made several changes. At the CvP driver, we just
have to ensure that anything lesser than 4096bytes are
padded with extra bytes. The CvP will then, initiate the
tear-down by clearing the START_XFER and CVP_CONFIG bits.
We should also check for CVP_ERROR during the CvP completion.
A send_buf which is always 4096bytes is used to copy the
data during every transaction.

Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
---
 drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 4ffb9da537d8..80edcfb5e5fc 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -81,6 +81,7 @@ struct altera_cvp_conf {
 	u8			numclks;
 	u32			sent_packets;
 	u32			vsec_offset;
+	u8			*send_buf;
 	const struct cvp_priv	*priv;
 };
 
@@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
 		}
 
 		len = min(conf->priv->block_size, remaining);
-		altera_cvp_send_block(conf, data, len);
+		/* Copy the requested host data into the transmit buffer */
+
+		memcpy(conf->send_buf, data, len);
+		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
+		conf->priv->block_size);
 		data += len / sizeof(u32);
 		done += len;
 		remaining -= len;
@@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
 	if (ret)
 		return ret;
 
-	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
-	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");
+	/*
+	 * STEP 16 - If bitstream error (truncated/miss-matched),
+	 * we shall exit here.
+	 */
+	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
+	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
+		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
 		return -EPROTO;
 	}
 
@@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, mgr);
 
+	/* Allocate the 4096 block size transmit buffer */
+	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
+	if (!conf->send_buf) {
+		ret = -ENOMEM;
+		goto err_unmap;
+	}
 	return 0;
 
 err_unmap:
-- 
2.25.1

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Tom Rix]

On 5/17/22 11:48 PM, tien.sung.ang@intel.com wrote:
> From: Ang Tien Sung <tien.sung.ang@intel.com>
>
> To support the error handling of a truncated bitstream sent.
> The current AIB CvP firmware is not capable of handling a
> data stream smaller than 4096bytes. The firmware's limitation
> causes a hung-up as it's DMA engine waits forever for the
> completion of the instructed 4096bytes.
> To resolve this design limitation, both firmware and CvP
> driver made several changes. At the CvP driver, we just
> have to ensure that anything lesser than 4096bytes are
> padded with extra bytes. The CvP will then, initiate the
> tear-down by clearing the START_XFER and CVP_CONFIG bits.
> We should also check for CVP_ERROR during the CvP completion.
> A send_buf which is always 4096bytes is used to copy the
> data during every transaction.
>
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
>   drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..80edcfb5e5fc 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -81,6 +81,7 @@ struct altera_cvp_conf {
>   	u8			numclks;
>   	u32			sent_packets;
>   	u32			vsec_offset;
> +	u8			*send_buf;

Why is it necessary to hold  the send_buf in this structure ?

If it is used only in *_write, could it alloc/freed there ?

Because the write happens rarely, my preference is to alloc/free in 
*_write().

>   	const struct cvp_priv	*priv;
>   };
>   
> @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>   		}
>   
>   		len = min(conf->priv->block_size, remaining);
> -		altera_cvp_send_block(conf, data, len);
> +		/* Copy the requested host data into the transmit buffer */
> +
> +		memcpy(conf->send_buf, data, len);
Is a memset needed for a short buffer ?
> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> +		conf->priv->block_size);
>   		data += len / sizeof(u32);
>   		done += len;
>   		remaining -= len;
> @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>   	if (ret)
>   		return ret;
>   
> -	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
> -	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");
> +	/*
> +	 * STEP 16 - If bitstream error (truncated/miss-matched),
> +	 * we shall exit here.
> +	 */
> +	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
Should this be STEP 17 ? the old 16 checked something else.

Tom

> +	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
> +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
>   		return -EPROTO;
>   	}
>   
> @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
>   
>   	pci_set_drvdata(pdev, mgr);
>   
> +	/* Allocate the 4096 block size transmit buffer */
> +	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
> +	if (!conf->send_buf) {
> +		ret = -ENOMEM;
> +		goto err_unmap;
> +	}
>   	return 0;
>   
>   err_unmap:

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

The send_buf is always used throughout the life-span of the CvP driver.
Hence, we thought it would be wise to just pre-allocate it one time
at the start of the probe/init.
It is also fine if we do it in the altera_cvp_write. The only issue we
see in this is that, a minor hit on the performance as you need to 
then, allocate the buffer on every new CvP FPGA configuration write.

As for STEP 16, the previous implementation checks the Error latch bit
which stores the previous transaction's result. If an error occurs
prior to this, the driver would throw an error which is not right.
The correct step is to just check for the current CvP error status 
from the register.
Hope that is fine with you. Thanks

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Thu, May 19, 2022 at 12:34:12PM +0800, tien.sung.ang@intel.com wrote:
> The send_buf is always used throughout the life-span of the CvP driver.
> Hence, we thought it would be wise to just pre-allocate it one time
> at the start of the probe/init.
> It is also fine if we do it in the altera_cvp_write. The only issue we
> see in this is that, a minor hit on the performance as you need to 
> then, allocate the buffer on every new CvP FPGA configuration write.
> 
> As for STEP 16, the previous implementation checks the Error latch bit
> which stores the previous transaction's result. If an error occurs
> prior to this, the driver would throw an error which is not right.
> The correct step is to just check for the current CvP error status 
> from the register.
> Hope that is fine with you. Thanks

Please comment inline in the mail, like others do.

Thanks,
Yilun

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Wed, May 18, 2022 at 01:08:31PM -0700, Tom Rix wrote:
> 
> On 5/17/22 11:48 PM, tien.sung.ang@intel.com wrote:
> > From: Ang Tien Sung <tien.sung.ang@intel.com>
> > 
> > To support the error handling of a truncated bitstream sent.
> > The current AIB CvP firmware is not capable of handling a
> > data stream smaller than 4096bytes. The firmware's limitation
> > causes a hung-up as it's DMA engine waits forever for the
> > completion of the instructed 4096bytes.
> > To resolve this design limitation, both firmware and CvP
> > driver made several changes. At the CvP driver, we just
> > have to ensure that anything lesser than 4096bytes are
> > padded with extra bytes. The CvP will then, initiate the
> > tear-down by clearing the START_XFER and CVP_CONFIG bits.
> > We should also check for CVP_ERROR during the CvP completion.
> > A send_buf which is always 4096bytes is used to copy the
> > data during every transaction.
> > 
> > Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> > ---
> >   drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
> >   1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> > index 4ffb9da537d8..80edcfb5e5fc 100644
> > --- a/drivers/fpga/altera-cvp.c
> > +++ b/drivers/fpga/altera-cvp.c
> > @@ -81,6 +81,7 @@ struct altera_cvp_conf {
> >   	u8			numclks;
> >   	u32			sent_packets;
> >   	u32			vsec_offset;
> > +	u8			*send_buf;
> 
> Why is it necessary to hold  the send_buf in this structure ?
> 
> If it is used only in *_write, could it alloc/freed there ?
> 
> Because the write happens rarely, my preference is to alloc/free in
> *_write().

Is it better alloc in write_init()?

Thanks,
Yilun

> 
> >   	const struct cvp_priv	*priv;
> >   };
> > @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
> >   		}
> >   		len = min(conf->priv->block_size, remaining);
> > -		altera_cvp_send_block(conf, data, len);
> > +		/* Copy the requested host data into the transmit buffer */
> > +
> > +		memcpy(conf->send_buf, data, len);
> Is a memset needed for a short buffer ?
> > +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> > +		conf->priv->block_size);
> >   		data += len / sizeof(u32);
> >   		done += len;
> >   		remaining -= len;
> > @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
> >   	if (ret)
> >   		return ret;
> > -	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
> > -	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");
> > +	/*
> > +	 * STEP 16 - If bitstream error (truncated/miss-matched),
> > +	 * we shall exit here.
> > +	 */
> > +	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
> Should this be STEP 17 ? the old 16 checked something else.
> 
> Tom
> 
> > +	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
> > +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
> >   		return -EPROTO;
> >   	}
> > @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
> >   	pci_set_drvdata(pdev, mgr);
> > +	/* Allocate the 4096 block size transmit buffer */
> > +	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
> > +	if (!conf->send_buf) {
> > +		ret = -ENOMEM;
> > +		goto err_unmap;
> > +	}
> >   	return 0;
> >   err_unmap:

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Christophe JAILLET]

Le 18/05/2022 à 08:48, tien.sung.ang@intel.com a écrit :
> From: Ang Tien Sung <tien.sung.ang@intel.com>
> 
> To support the error handling of a truncated bitstream sent.
> The current AIB CvP firmware is not capable of handling a
> data stream smaller than 4096bytes. The firmware's limitation
> causes a hung-up as it's DMA engine waits forever for the
> completion of the instructed 4096bytes.
> To resolve this design limitation, both firmware and CvP
> driver made several changes. At the CvP driver, we just
> have to ensure that anything lesser than 4096bytes are
> padded with extra bytes. The CvP will then, initiate the
> tear-down by clearing the START_XFER and CVP_CONFIG bits.
> We should also check for CVP_ERROR during the CvP completion.
> A send_buf which is always 4096bytes is used to copy the
> data during every transaction.
> 
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
>   drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..80edcfb5e5fc 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -81,6 +81,7 @@ struct altera_cvp_conf {
>   	u8			numclks;
>   	u32			sent_packets;
>   	u32			vsec_offset;
> +	u8			*send_buf;
>   	const struct cvp_priv	*priv;
>   };
>   
> @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>   		}
>   
>   		len = min(conf->priv->block_size, remaining);
> -		altera_cvp_send_block(conf, data, len);
> +		/* Copy the requested host data into the transmit buffer */
> +
> +		memcpy(conf->send_buf, data, len);
> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> +		conf->priv->block_size);

Nitpicking: this should be aligned with 'conf'.

CJ

>   		data += len / sizeof(u32);
>   		done += len;
>   		remaining -= len;

[...]

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

Thanks for pointing that out. It is always good to get all the alignments right.
I will add this to the next revision of the patch.

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Thu, May 19, 2022 at 12:21:35PM +0800, tien.sung.ang@intel.com wrote:
> Thanks for pointing that out. It is always good to get all the alignments right.

Please run checkpatch --strict before sending, it helps find out most of
alignment issues.

Thanks,
Yilun

> I will add this to the next revision of the patch.