diff mbox series

[PATCHv2,2/3] fpga: altera-cvp: Preparation for V2 parts.

Message ID 1563317287-18834-3-git-send-email-thor.thayer@linux.intel.com (mailing list archive)
State Superseded, archived
Delegated to: Moritz Fischer
Headers show
Series fpga: altera-cvp: Add Stratix10 Support | expand

Commit Message

Thor Thayer July 16, 2019, 10:48 p.m. UTC 
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(-)

Comments

Moritz Fischer July 22, 2019, 12:59 a.m. UTC | #1 
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
Thor Thayer July 23, 2019, 2:40 p.m. UTC | #2 
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
>
Moritz Fischer July 24, 2019, 2:57 p.m. UTC | #3 
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
Thor Thayer July 24, 2019, 4:59 p.m. UTC | #4 
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
Moritz Fischer July 24, 2019, 5:05 p.m. UTC | #5 
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
diff mbox series

Patch

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);