Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751642AbdFHOPa convert rfc822-to-8bit (ORCPT ); Thu, 8 Jun 2017 10:15:30 -0400 Received: from mail-out.m-online.net ([212.18.0.9]:44991 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751344AbdFHOP2 (ORCPT ); Thu, 8 Jun 2017 10:15:28 -0400 X-Auth-Info: nI6BueNK8wY/SIwsmUHL9qXZE75AbXMGYKacb7XjN1k= Date: Thu, 8 Jun 2017 16:15:19 +0200 From: Anatolij Gustschin To: Andy Shevchenko Cc: linux-fpga@vger.kernel.org, Alan Tull , Moritz Fischer , "linux-kernel@vger.kernel.org" , matthew.gerlach@linux.intel.com, yi1.li@linux.intel.com Subject: Re: [PATCH v5] fpga manager: Add Altera CvP driver Message-ID: <20170608161519.67e3908d@crub> In-Reply-To: References: <1494777082-13375-1-git-send-email-agust@denx.de> <20170608010928.3effdf38@crub> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3107 Lines: 97 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 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