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

[Anatolij Gustschin]

On Wed, 3 May 2017 00:28:17 +0300
Andy Shevchenko andy.shevchenko@xxxxxxxxx wrote:
...
>>>Is 0xff a mask here? (Btw, you missed spaces around <<)  
>>
>> yes, it is. Will add spaces (checkpatch didn't warn here).  
>
>Then it makes sense to add _MASK and use GENMASK() instead of direct value.

ok, will do.

>>>Why do we need that?!
>>>In new drivers we try to avoid new module parameters. We have enough
>>>interfaces nowadays to let driver know some details (quirks).  
>>
>> Which interface is preffered here? Do you suggest sysfs? Won't be able
>> to pass the parameter on kernel command line, then.  
>
>Yes, my question here is to understand what so important that driver
>needs module parameter.
>Can you elaborate?

the driver doesn't need this parameter, but it could help testing
the loading of compressed or encrypted images.

...
>>>Why 2 is missed? Hardware limitation? Needs a comment about these magics.  
>>
>> it is not missed, other values are just not valid and filtered out here.  
>
>That's my suggestion.
>So, if reviewer asks such a question it's a flag like "Okay, here is
>an additional comment required".

ok

>>>> +       /* set number of CVP clock cycles for every CVP Data Register Write */
>>>> +       pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, &val32);
>>>> +       val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;  
>>>  
>>>> +       val32 |= 0x01 << 8;     /* 1 clock */  
>>>
>>>Yeah, needs more clear way to put clocks of choice.  
>>
>> what exactly is not clear here?  
>
>Magics. And extra prefixes where it's not needed.
>
>0x0 - makes reader to think "A-ha, this is probably address or some
>interesting data. Here is just 1.
>8 - why? Usually we do ..._SHIFT costant for a such.

ok, will change.

>>>> +       pci_write_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, val32);
>>>> +
>>>> +       for (i = 0; i < CVP_DUMMY_WR; i++)  
>>>  
>>>> +               conf->write_data(conf, 0xdeadbeef);  
>>>
>>>Why this dummy is chosen?  
>>
>> it is a dummy and can be anything. So why not? I re-used some code
>> where this value was chosen. Can change it to 0.  
>
>Not need to be changed, but, please add a comment.

ok, will do.

...
>Just think about it:
>
>while (1) {
> ...100500 lines of code...
>}
>
>Reader needs to go through the entire body to understand 2 things:
>- what is the exit condition if any? do we have only one exit condition?
>- how many iterrations are supposed to be
>
>It takes time even more that took for writing above lines.
>
>Good code would be read fast.

ok

>>>> +               pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
>>>> +               if ((val32 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>>>> +                       break;
>>>> +  
>>>  
>>>> +               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.

>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.

...
>>>> +       pci_read_config_word(pdev, VSEC_PCIE_EXT_CAP_ID, &val16);  
>>>
>>>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);
+	}

>So,
>- can you use it?
>- if no, elaborate why not
>
>Okay, looks like below you answer this somehow.
>
>>>So, you are using devm_ above, but avoid pcim_ here. Please clarify
>>>enabling device case and use if possible pcim_  
>>
>> I can't use pcim_enable_device(), it will make the driver unusable
>> on some platforms.  
>
>> The driver is only for re-configuring FPGAs and
>> there can be unlimited variations of different PCIe devices implemented
>> in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for
>> which an address range cannot be assigned on embedded 32-bit
>> platforms. pcim_enable_device() will fail here complaining about
>> not claimed BAR, even if the concerned BAR is not needed for FPGA
>> configuration at all. This makes the driver unusable.  
>
>Please put something like above in the comment in ->probe() before
>first call to pci_...().

ok

Thanks,

Anatolij