From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753327AbdECPB2 (ORCPT ); Wed, 3 May 2017 11:01:28 -0400 Received: from mail-qt0-f193.google.com ([209.85.216.193]:35697 "EHLO mail-qt0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753492AbdECPBU (ORCPT ); Wed, 3 May 2017 11:01:20 -0400 MIME-Version: 1.0 In-Reply-To: <20170503021439.6d812223@crub> References: <1493579317-28608-1-git-send-email-agust@denx.de> <20170502115308.1e2f1233@crub> <20170503021439.6d812223@crub> From: Andy Shevchenko Date: Wed, 3 May 2017 18:01:19 +0300 Message-ID: Subject: Re: [PATCH v4] fpga manager: Add Altera CvP driver To: Anatolij Gustschin Cc: linux-fpga@vger.kernel.org, Alan Tull , Moritz Fischer , matthew.gerlach@linux.intel.com, yi1.li@linux.intel.com, "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 3, 2017 at 3:14 AM, Anatolij Gustschin wrote: > On Wed, 3 May 2017 00:28:17 +0300 > Andy Shevchenko andy.shevchenko@gmail.com wrote: >>>>> + udelay(1); /* wait 1us */ >>>> >>>>Why not 10? Needs a comment. >>> >>> if this is not obvious, >> >>No, it's not. Especially after what you wrote below. >> >>> we want to start the configuration early and want >>> to avoid unneeded delays when polling ready status. For 10 I would have >>> to use usleep_range() adding more delay. >> >>usleep_range() has one big difference to udelay: it's not atomic. This >>makes me to ask even more questions instead of understanding what's >>going on here. >> >>So, what kind of this function is? Is it supposed to be run in atomic >>context, not atomic, or any? > > not atomic, a callback always running in a process context. So, then it would be good trade off to use usleep_range(10, 11) or alike to allow others to get a resource. udelay() is a busy loop and use of it costs us CPU resource. >>Depends on answer we need to choose best API to allow minimum delays >>_and_ CPU resource waste. >> >>>>> + if (chkcfg && !(bytes % SZ_4K)) { >>>> >>>>Is 4k comes from PCI spec, or is it page size? >>> >>> no, it is more an arbitrary value. It was suggested to check for >>> error status after writing a data block and not after each data write >>> to speed-up the config process. The config images can be big (above >>> 36 MiB) and often checking will slow down the configuration. >> >>Your comment didn't make it more clearer to me. >>So, you take bytes value and check that 12 LSBs are 0. Why? > > when 12 LSBs are zero, the bytes value has been decremented by > 4k, meaning that a new 4k data block has been written. Only > then the error checking is performed. If the size is less than 4k...? >>>>Are you foing to do this without enabling device? Needs comment why if so. >>> >>> pci config space access works without enabling the pci device, >>> writing commands to config space enables the device first. It is done >>> some lines below which you deleted when commenting (please see original >>> patch). >> >>Your comment didn't clarify what's going on along these lines. >> >>I checked original patch, I didn't find any type of >>pci_enable_device() call. > > I mean this part (instead of pci_enable_device()): > + /* Enable memory BAR access */ > + pci_read_config_word(pdev, PCI_COMMAND, &cmd); > + if (!(cmd & PCI_COMMAND_MEMORY)) { > + cmd |= PCI_COMMAND_MEMORY; > + pci_write_config_word(pdev, PCI_COMMAND, cmd); > + } I see this code is used somewhere else (several places I suppose, drivers/video/fbdev/aty/atyfb_base.c is one of them). For me it makes sense to split it to a helper in pci.h for broader use. static inline void pci_enable_memory(struct pci_dev *dev) { u16 cmd; /* Enable memory BAR access */ pci_read_config_word(pdev, PCI_COMMAND, &cmd); if (!(cmd & PCI_COMMAND_MEMORY)) { cmd |= PCI_COMMAND_MEMORY; pci_write_config_word(pdev, PCI_COMMAND, cmd); } } -- With Best Regards, Andy Shevchenko