Re: [PATCH v5] fpga manager: Add Altera CvP driver

[Anatolij Gustschin <agust@denx.de>]

On Thu, 8 Jun 2017 02:38:55 +0300
Andy Shevchenko andy.shevchenko@gmail.com wrote:

>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@denx.de> wrote:
>> On Fri, 2 Jun 2017 20:43:21 +0300
>> Andy Shevchenko andy.shevchenko@gmail.com wrote:  
>
>Besides below comments, please, do
>
>s/VSEC_/VSE_/g
>
>for entire file.
>
>We are following PCI and Thunderbolt pattern for use of Vendor
>Specific Extended Capability.

I can do it, but I'm just not getting why. The registers are named as VSEC
registers in the documentation, why should the code name them differently?

...
>>>> +       if (!timeout_us)
>>>> +               return -ETIMEDOUT;  
>>>
>>>Hmm...
>>>What as a user I would expect here is at least one attempt (0 -- no
>>>timeout, but try once).  
>>
>> yes, this first attempt is above, please see original patch for full
>> context.  
>
>Ah, it means you don't correctly use do {} while approach.
>
>Remove everything above do { and move usleep after check for the
>status inside the loop.

Unfortunately, suggested approach has an unwanted side effect:

	do {
		check and return if done;

		usleep_range(10, 11);
		tout -= 10;

	} while (tout > 0);

For simplicity, let's say we were asked to wait with 20 µs timeout.
Assume, that the device reports ready status after 17 µs. The first
check is done, we don't return and sleep approx. 10 µs. Then, the 2nd
check is done and we continue to wait another 10 µs and the loop ends
signalling a timeout. But in the meantime the device reported ready
status. Additional check would be needed after the loop.
In some cases the device reports ready status immediately. That's
the reason why I check first and then loop with more wait&check cycles.

...
>See the magic below:
>
>        u32 iflags = info ? info->flags : 0;
>...
>        if (iflags & FPGA_MGR_PARTIAL_RECONFIG) {
>                 dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
>                 return -EINVAL;
>        }
>
>        if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM)
>                conf->numclks = 8; /* ratio for all compressed images */
>        else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM)
>                conf->numclks = 4; /* ratio for encrypted images */
>        else
>                conf->numclks = 1; /* clock to data ratio for
>uncompressed and unencrypted images */
>
>I would really recommend to double check the code every time you are
>about to send.
>A little thought can make a beauteful result!

ok, changed as suggested.

>>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>>> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },  
>>>
>>>Does it have dedicated PCI class?
>>>
>>>PCI_ANY_ID usually is too broad.  
>>
>> no, it doesn't have dedicated class.  
>
>Hmm... It means any device of this vendor will jump into this
>driver... Not good.

in an early patch version I was asked by Intel people to use PCI_ANY_ID
because these devices are not set in stone. The implemented FPGA PCIe
devices can have varying IDs. probe() checks for expected capability ID
and stops if we hit a not supported device.

Thanks,
Anatolij