lkml.org 
[lkml]   [2017]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5] fpga manager: Add Altera CvP driver
On Thu, Jun 8, 2017 at 5:15 PM, Anatolij Gustschin <agust@denx.de> wrote:
> 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?

Does your documentation decode VSEC abbreviation?
What C stands for? Capability?

In PCI and Thunderbolt we agreed to use word capability separately,
so, either XXX_... or XXX_CAP_... to use.

>>>>> + 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:

How come? See below.

>
> 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.

Just look to the rest of the code in kernel
Most of the timeout related loops we have the following pattern:

unsigned int retries = XXX;

do {
...check for something...
if (yes)
return YY;

...sleep for a while...
} while (--retries);
if (!retries)
return -ETIMEDOUT;

What I'm suggesting is to follow the pattern (adjust it for your exact
conditions and so on).

>>>>> +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.

Yeah, the problem is that every device with a such Vendor ID would be
considered by this driver and PCI class would be helpful here just to
reduce an impact.
Capability approach works, though it's slightly more error prone.

I have no other comment on this. For now it seems the only choice
since such IPs are on the market, right?

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2017-06-12 01:30    [W:0.051 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site