Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 8 Jun 2017 17:44:19 +0300 | Subject | Re: [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
| |