On Wed, 3 May 2017 00:28:17 +0300
Andy Shevchenko andy.shevche...@gmail.com 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
