Re: [PATCH v5 2/4] fpga pr ip: Core driver support for Altera Partial Reconfiguration IP.

[matthew . gerlach]

On Sat, 18 Mar 2017, Anatolij Gustschin wrote:

> Hi Matthew,

Hi Anatolij,

More good feedback.  See below.


> thanks for the patches. Please see some comments below.
>
> On Fri, 10 Mar 2017 11:40:25 -0800
> matthew.gerlach@xxxxxxxxxxxxxxx matthew.gerlach@xxxxxxxxxxxxxxx wrote:
>
> ...
> > +	if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> > +		pr_err("%s Partial Reconfiguration flag not set\n", __func__);
>
> please use dev_err() here.
>
> ...
> > +	if (val & ALT_PR_CSR_PR_START) {
> > +		pr_err("%s Partial Reconfiguration already started\n",
>
> dev_err(), too.

Good catch on both of these pr_err().  dev_err() would be better.
> ...
> > +static int alt_pr_fpga_write_complete(struct fpga_manager *mgr,
> > +				      struct fpga_image_info *info)
> > +{
> > +	u32 i;
> > +
> > +	for (i = 0; i < info->config_complete_timeout_us; i++) {
> > +		switch (alt_pr_fpga_state(mgr)) {
> > +		case FPGA_MGR_STATE_WRITE_ERR:
> > +			return -EIO;
> > +
> > +		case FPGA_MGR_STATE_OPERATING:
> > +			dev_info(&mgr->dev,
> > +				 "successful partial reconfiguration\n");
> > +			return 0;
> > +
> > +		default:
> > +			break;
> > +		}
> > +		udelay(1);
> > +	}
> > +	dev_err(&mgr->dev, "timed out waiting for write to complete\n");
> > +	return -ETIMEDOUT;
> > +}
>
> we will always get timed out error if info->config_complete_timeout_us
> is zero. Can we change to
> 	u32 i = 0;
> 	...
> 	do {
> 		...
> 	} while (info->config_complete_timeout_us > i++);
> ?

This seems more than reasonable to me.

> ...
> > diff --git a/drivers/fpga/altera-pr-ip-core.h b/drivers/fpga/altera-pr-ip-core.h
> > new file mode 100644
> > index 0000000..3810a90
> > --- /dev/null
> > +++ b/drivers/fpga/altera-pr-ip-core.h
>
> Should we move this header to include/linux/? We can use register/
> unregister functions in other drivers (PCIe) outside drivers/fpga
> then.
>
> Thanks,
>
> Anatolij