Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 3 May 2017 18:01:19 +0300 | Subject | Re: [PATCH v4] fpga manager: Add Altera CvP driver |
| |
On Wed, May 3, 2017 at 3:14 AM, Anatolij Gustschin <agust@denx.de> 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
| |