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

[Andy Shevchenko]

On Tue, May 2, 2017 at 12:53 PM, Anatolij Gustschin <agust@xxxxxxx> wrote:
> On Mon, 1 May 2017 23:06:16 +0300
> Andy Shevchenko andy.shevchenko@xxxxxxxxx wrote:
>>On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin <agust@xxxxxxx> wrote:

>>> +#define VSEC_CVP_MODE_CTRL             (VSEC_OFFSET + 0x20)    /* 32bit */
>>> +#define VSEC_CVP_MODE_CTRL_CVP_MODE    BIT(0) /* CVP (1) or normal mode (0) */
>>> +#define VSEC_CVP_MODE_CTRL_HIP_CLK_SEL BIT(1) /* PMA (1) or fabric clock (0) */
>>> +#define VSEC_CVP_MODE_CTRL_FULL_CFG    BIT(2) /* CVP_FULLCONFIG */
>>
>>> +#define VSEC_CVP_MODE_CTRL_NUMCLKS     (0xff<<8) /* CVP_NUMCLKS */
>>
>>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.

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

> I'll drop the numclks parameter here and will use a fixed value. I do not
> need it for current configurations and if anyone needs it and actually has
> the devices and bitstreams to test it, feel free to implement it using the
> preferred interfaces.

This would work to me.

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

>>> +       /* 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.

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

>>> +       pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
>>> +       val32 &= ~VSEC_CVP_PROG_CTRL_START_XFER;
>>> +       pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>>> +
>>
>>> +       delay_us = 0;
>>> +       while (1) {
>>
>>Oh, no. It's a red flag.
>>
>>And better to do
>>
>>do {} while (--retries);
>

> can change it if its preferred.

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.

>>> +               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?

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?

>>> +static int altera_cvp_probe(struct pci_dev *pdev,
>>> +                           const struct pci_device_id *dev_id)
>>> +{
>>> +       struct altera_cvp_conf *conf;
>>> +       u16 cmd, val16;
>>> +       int ret;
>>> +
>>> +       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.

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_...().

>>> +       conf->map = pci_iomap(pdev, CVP_BAR, 0);
>>> +       if (!conf->map) {
>>> +               dev_warn(&pdev->dev, "Mapping CVP BAR failed\n");
>>
>>Not needed in pcim_ case.
>
> can I use other pcim_ functions if pcim_enalbe_device is not used?

You may check yourselt, IIRC no, you can't use pcim_ioremap*().

-- 
With Best Regards,
Andy Shevchenko