Re: [PATCH v4] fpga manager: Add Altera CvP driver

[Andy Shevchenko]

On Wed, May 3, 2017 at 3:14 AM, Anatolij Gustschin <agust@xxxxxxx> wrote:
> On Wed, 3 May 2017 00:28:17 +0300
> Andy Shevchenko andy.shevchenko@xxxxxxxxx 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