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

[Anatolij Gustschin]

On Wed, 3 May 2017 18:01:19 +0300
Andy Shevchenko andy.shevche...@gmail.com wrote:
...
>> 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...?

then the final check below will catch it. And I doubt that config
images can be so small. The lowest size I've ever seen is more
than 1MiB.

...
 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, );
>> +   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).

other places set or clean additional pci command flags, use
different pci config accessors or contain debug code. And I reuse
pre-initialized cmd in the error path, so the usage pattern here
is not the same as in the atyfb_base driver.

--
Anatolij

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

[Anatolij Gustschin]

On Wed, 3 May 2017 18:01:19 +0300
Andy Shevchenko andy.shevche...@gmail.com wrote:
...
>> 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...?

then the final check below will catch it. And I doubt that config
images can be so small. The lowest size I've ever seen is more
than 1MiB.

...
 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, );
>> +   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).

other places set or clean additional pci command flags, use
different pci config accessors or contain debug code. And I reuse
pre-initialized cmd in the error path, so the usage pattern here
is not the same as in the atyfb_base driver.

--
Anatolij

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

[Andy Shevchenko]

On Wed, May 3, 2017 at 3:14 AM, Anatolij Gustschin  wrote:
> On Wed, 3 May 2017 00:28:17 +0300
> Andy Shevchenko andy.shevche...@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, );
> +   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, );
if (!(cmd & PCI_COMMAND_MEMORY)) {
cmd |= PCI_COMMAND_MEMORY;
pci_write_config_word(pdev, PCI_COMMAND, cmd);
}
}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, May 3, 2017 at 3:14 AM, Anatolij Gustschin  wrote:
> On Wed, 3 May 2017 00:28:17 +0300
> Andy Shevchenko andy.shevche...@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, );
> +   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, );
if (!(cmd & PCI_COMMAND_MEMORY)) {
cmd |= PCI_COMMAND_MEMORY;
pci_write_config_word(pdev, PCI_COMMAND, cmd);
}
}

-- 
With Best Regards,
Andy Shevchenko

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

[Alan Tull]

On Tue, May 2, 2017 at 7:14 PM, Anatolij Gustschin  wrote:
> 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.

Loading encrypted or compressed images can be keyed off of flags in
fpga_image_info.  Currently we have FPGA_MGR_ENCRYPTED_BITSTREAM  Will
need to add one for compressed such as FPGA_MGR_COMPRESSED_BITSTREAM

Alan

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

[Alan Tull]

On Tue, May 2, 2017 at 7:14 PM, Anatolij Gustschin  wrote:
> 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.

Loading encrypted or compressed images can be keyed off of flags in
fpga_image_info.  Currently we have FPGA_MGR_ENCRYPTED_BITSTREAM  Will
need to add one for compressed such as FPGA_MGR_COMPRESSED_BITSTREAM

Alan

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

[Anatolij Gustschin]

On Tue, 02 May 2017 16:36:54 -0700
Joe Perches j...@perches.com wrote:
...
>It would with command line option --strict, otherwise not.

ah, good to know. Thanks!

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

[Anatolij Gustschin]

On Tue, 02 May 2017 16:36:54 -0700
Joe Perches j...@perches.com wrote:
...
>It would with command line option --strict, otherwise not.

ah, good to know. Thanks!

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

[Anatolij Gustschin]

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 &= ~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, );
 +   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, );  
>>>
>>>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, );
+   if (!(cmd & PCI_COMMAND_MEMORY)) {
+   cmd |= PCI_COMMAND_MEMORY;
+   pci_write_config_word(pdev, PCI_COMMAND, cmd);
+   

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

[Anatolij Gustschin]

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 &= ~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, );
 +   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, );  
>>>
>>>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, );
+   if (!(cmd & PCI_COMMAND_MEMORY)) {
+   cmd |= PCI_COMMAND_MEMORY;
+   pci_write_config_word(pdev, PCI_COMMAND, cmd);
+   

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

[Joe Perches]

On Tue, 2017-05-02 at 11:53 +0200, Anatolij Gustschin wrote:
> On Mon, 1 May 2017 23:06:16 +0300 Andy Shevchenko andy.shevche...@gmail.com 
> wrote:
> > > +#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).

It would with command line option --strict, otherwise not.

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

[Joe Perches]

On Tue, 2017-05-02 at 11:53 +0200, Anatolij Gustschin wrote:
> On Mon, 1 May 2017 23:06:16 +0300 Andy Shevchenko andy.shevche...@gmail.com 
> wrote:
> > > +#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).

It would with command line option --strict, otherwise not.

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

[Andy Shevchenko]

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

>>> +#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 32bit */
>>> +#define VSEC_CVP_MODE_CTRL_CVP_MODEBIT(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_CFGBIT(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 &= ~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 &= ~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, );
>>> +   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;

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

[Andy Shevchenko]

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

>>> +#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 32bit */
>>> +#define VSEC_CVP_MODE_CTRL_CVP_MODEBIT(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_CFGBIT(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 &= ~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 &= ~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, );
>>> +   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;

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

[Anatolij Gustschin]

On Mon, 1 May 2017 23:06:16 +0300
Andy Shevchenko andy.shevche...@gmail.com wrote:

>On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin  wrote:
>> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
>> and Arria-10 FPGAs via CvP.  
>
>I think you need to spend time on polishing such code.
>
>See my comments below.
>
>> +#define CVP_BAR0   /* BAR used for data transfer in 
>> memory mode */
>> +#define CVP_DUMMY_WR   244 /* dummy writes to clear CvP state machine */
>> +#define TIMEOUT_IN_US  2000
>> +
>> +/* Vendor Specific Extended Capability Offset */
>> +#define VSEC_OFFSET0x200  
>
>> +#define VSEC_PCIE_EXT_CAP_ID_VAL   0x000b  
>
>> +#define VSEC_PCIE_EXT_CAP_ID   (VSEC_OFFSET + 0x00)/* 16bit */  
>
>It is not clear what is what. If the above is value, why it's located
>under offset "section"?

ok, will move VSEC_PCIE_EXT_CAP_ID_VAL below the offset.

>> +#define VSEC_CVP_STATUS(VSEC_OFFSET + 0x1c)/* 
>> 32bit */  
>
>Usually in the drivers we use just absolute value.

ok, will change it.

>> +#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 32bit */
>> +#define VSEC_CVP_MODE_CTRL_CVP_MODEBIT(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_CFGBIT(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).

>> +static int chkcfg; /* use value 1 for debugging only */
>> +module_param(chkcfg, int, 0664);
>> +MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking (default 
>> 0)");
>> +
>> +static int numclks = 1; /* default 1 for uncompressed and unencrypted */
>> +module_param(numclks, int, 0664);
>> +MODULE_PARM_DESC(numclks, "Clock to data ratio 1, 4 or 8 (default 1)");  
>
>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.

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.

>> +
>> +struct altera_cvp_conf {
>> +   struct fpga_manager *mgr;
>> +   struct pci_dev  *pci_dev;
>> +   void __iomem*map;
>> +   void(*write_data)(struct altera_cvp_conf *conf,
>> + u32 val);
>> +   charmgr_name[64];
>> +};
>> +  
>
>> +static inline void altera_cvp_chk_numclks(void)
>> +{
>> +   switch (numclks) {
>> +   case 1:
>> +   case 4:
>> +   case 8:
>> +   break;
>> +   default:
>> +   numclks = 1;
>> +   break;
>> +   }
>> +}  
>
>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.

>> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
>> +{
>> +   u32 val32;  
>
>> +   int i;  
>
>unsigned int i; ?

ok, will change.

>> +   /* set number of CVP clock cycles for every CVP Data Register Write 
>> */
>> +   pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, );
>> +   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?

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


>> +}  
>
>> +
>> +static int altera_cvp_teardown(struct fpga_manager *mgr,
>> +  struct fpga_image_info *info)
>> +{
>> +   struct altera_cvp_conf *conf = mgr->priv;
>> +   struct pci_dev *pdev = conf->pci_dev;
>> +   int status = 0;
>> +   int delay_us;
>> +   u32 val32;
>> +  
>
>> +   /*
>> +* STEP 12 - reset START_XFER bit
>> +*/  
>
>One line?
>(as well below)

ok, will change.

>> +   pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, );
>> +   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 

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

[Anatolij Gustschin]

On Mon, 1 May 2017 23:06:16 +0300
Andy Shevchenko andy.shevche...@gmail.com wrote:

>On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin  wrote:
>> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
>> and Arria-10 FPGAs via CvP.  
>
>I think you need to spend time on polishing such code.
>
>See my comments below.
>
>> +#define CVP_BAR0   /* BAR used for data transfer in 
>> memory mode */
>> +#define CVP_DUMMY_WR   244 /* dummy writes to clear CvP state machine */
>> +#define TIMEOUT_IN_US  2000
>> +
>> +/* Vendor Specific Extended Capability Offset */
>> +#define VSEC_OFFSET0x200  
>
>> +#define VSEC_PCIE_EXT_CAP_ID_VAL   0x000b  
>
>> +#define VSEC_PCIE_EXT_CAP_ID   (VSEC_OFFSET + 0x00)/* 16bit */  
>
>It is not clear what is what. If the above is value, why it's located
>under offset "section"?

ok, will move VSEC_PCIE_EXT_CAP_ID_VAL below the offset.

>> +#define VSEC_CVP_STATUS(VSEC_OFFSET + 0x1c)/* 
>> 32bit */  
>
>Usually in the drivers we use just absolute value.

ok, will change it.

>> +#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 32bit */
>> +#define VSEC_CVP_MODE_CTRL_CVP_MODEBIT(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_CFGBIT(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).

>> +static int chkcfg; /* use value 1 for debugging only */
>> +module_param(chkcfg, int, 0664);
>> +MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking (default 
>> 0)");
>> +
>> +static int numclks = 1; /* default 1 for uncompressed and unencrypted */
>> +module_param(numclks, int, 0664);
>> +MODULE_PARM_DESC(numclks, "Clock to data ratio 1, 4 or 8 (default 1)");  
>
>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.

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.

>> +
>> +struct altera_cvp_conf {
>> +   struct fpga_manager *mgr;
>> +   struct pci_dev  *pci_dev;
>> +   void __iomem*map;
>> +   void(*write_data)(struct altera_cvp_conf *conf,
>> + u32 val);
>> +   charmgr_name[64];
>> +};
>> +  
>
>> +static inline void altera_cvp_chk_numclks(void)
>> +{
>> +   switch (numclks) {
>> +   case 1:
>> +   case 4:
>> +   case 8:
>> +   break;
>> +   default:
>> +   numclks = 1;
>> +   break;
>> +   }
>> +}  
>
>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.

>> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
>> +{
>> +   u32 val32;  
>
>> +   int i;  
>
>unsigned int i; ?

ok, will change.

>> +   /* set number of CVP clock cycles for every CVP Data Register Write 
>> */
>> +   pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, );
>> +   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?

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


>> +}  
>
>> +
>> +static int altera_cvp_teardown(struct fpga_manager *mgr,
>> +  struct fpga_image_info *info)
>> +{
>> +   struct altera_cvp_conf *conf = mgr->priv;
>> +   struct pci_dev *pdev = conf->pci_dev;
>> +   int status = 0;
>> +   int delay_us;
>> +   u32 val32;
>> +  
>
>> +   /*
>> +* STEP 12 - reset START_XFER bit
>> +*/  
>
>One line?
>(as well below)

ok, will change.

>> +   pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, );
>> +   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.

>
>style. 

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

[Andy Shevchenko]

On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin  wrote:
> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
> and Arria-10 FPGAs via CvP.

I think you need to spend time on polishing such code.

See my comments below.

> +#define CVP_BAR0   /* BAR used for data transfer in 
> memory mode */
> +#define CVP_DUMMY_WR   244 /* dummy writes to clear CvP state machine */
> +#define TIMEOUT_IN_US  2000
> +
> +/* Vendor Specific Extended Capability Offset */
> +#define VSEC_OFFSET0x200

> +#define VSEC_PCIE_EXT_CAP_ID_VAL   0x000b

> +#define VSEC_PCIE_EXT_CAP_ID   (VSEC_OFFSET + 0x00)/* 16bit */

It is not clear what is what. If the above is value, why it's located
under offset "section"?

> +#define VSEC_CVP_STATUS(VSEC_OFFSET + 0x1c)/* 
> 32bit */

Usually in the drivers we use just absolute value.

> +#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 32bit */
> +#define VSEC_CVP_MODE_CTRL_CVP_MODEBIT(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_CFGBIT(2) /* CVP_FULLCONFIG */

> +#define VSEC_CVP_MODE_CTRL_NUMCLKS (0xff<<8) /* CVP_NUMCLKS */

Is 0xff a mask here? (Btw, you missed spaces around <<)

> +static int chkcfg; /* use value 1 for debugging only */
> +module_param(chkcfg, int, 0664);
> +MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking (default 
> 0)");
> +
> +static int numclks = 1; /* default 1 for uncompressed and unencrypted */
> +module_param(numclks, int, 0664);
> +MODULE_PARM_DESC(numclks, "Clock to data ratio 1, 4 or 8 (default 1)");

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

> +
> +struct altera_cvp_conf {
> +   struct fpga_manager *mgr;
> +   struct pci_dev  *pci_dev;
> +   void __iomem*map;
> +   void(*write_data)(struct altera_cvp_conf *conf,
> + u32 val);
> +   charmgr_name[64];
> +};
> +

> +static inline void altera_cvp_chk_numclks(void)
> +{
> +   switch (numclks) {
> +   case 1:
> +   case 4:
> +   case 8:
> +   break;
> +   default:
> +   numclks = 1;
> +   break;
> +   }
> +}

Why 2 is missed? Hardware limitation? Needs a comment about these magics.

> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
> +{
> +   u32 val32;

> +   int i;

unsigned int i; ?

> +
> +   /* set number of CVP clock cycles for every CVP Data Register Write */
> +   pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, );
> +   val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;

> +   val32 |= 0x01 << 8; /* 1 clock */

Yeah, needs more clear way to put clocks of choice.

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

> +}

> +
> +static int altera_cvp_teardown(struct fpga_manager *mgr,
> +  struct fpga_image_info *info)
> +{
> +   struct altera_cvp_conf *conf = mgr->priv;
> +   struct pci_dev *pdev = conf->pci_dev;
> +   int status = 0;
> +   int delay_us;
> +   u32 val32;
> +

> +   /*
> +* STEP 12 - reset START_XFER bit
> +*/

One line?
(as well below)

> +   pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, );
> +   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);

style. It will on smallest glance give reader a clue that the body
will go at least once.

> +   pci_read_config_dword(pdev, VSEC_CVP_STATUS, );
> +   if ((val32 & VSEC_CVP_STATUS_CFG_RDY) == 0)
> +   break;
> +

> +   udelay(1);  /* wait 1us */

Why not 10? Needs a comment.

> +
> +   if (delay_us++ > TIMEOUT_IN_US) {
> +   dev_warn(>dev, "CVP_CONFIG_READY == 0 
> timeout\n");
> +   status = -ETIMEDOUT;
> +   break;
> +   }
> +   }
> +
> +   return status;
> +}

> +
> +static int altera_cvp_write_init(struct fpga_manager *mgr,
> +struct fpga_image_info *info,
> +const char *buf, size_t count)
> +{
> +   struct altera_cvp_conf *conf = mgr->priv;
> +   struct pci_dev *pdev = conf->pci_dev;
> +   int delay_us;
> +   u32 val32;
> +   int ret;

> +   /*
> +* STEP 1 - read CVP status and check 

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

[Andy Shevchenko]

On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin  wrote:
> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
> and Arria-10 FPGAs via CvP.

I think you need to spend time on polishing such code.

See my comments below.

> +#define CVP_BAR0   /* BAR used for data transfer in 
> memory mode */
> +#define CVP_DUMMY_WR   244 /* dummy writes to clear CvP state machine */
> +#define TIMEOUT_IN_US  2000
> +
> +/* Vendor Specific Extended Capability Offset */
> +#define VSEC_OFFSET0x200

> +#define VSEC_PCIE_EXT_CAP_ID_VAL   0x000b

> +#define VSEC_PCIE_EXT_CAP_ID   (VSEC_OFFSET + 0x00)/* 16bit */

It is not clear what is what. If the above is value, why it's located
under offset "section"?

> +#define VSEC_CVP_STATUS(VSEC_OFFSET + 0x1c)/* 
> 32bit */

Usually in the drivers we use just absolute value.

> +#define VSEC_CVP_MODE_CTRL (VSEC_OFFSET + 0x20)/* 32bit */
> +#define VSEC_CVP_MODE_CTRL_CVP_MODEBIT(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_CFGBIT(2) /* CVP_FULLCONFIG */

> +#define VSEC_CVP_MODE_CTRL_NUMCLKS (0xff<<8) /* CVP_NUMCLKS */

Is 0xff a mask here? (Btw, you missed spaces around <<)

> +static int chkcfg; /* use value 1 for debugging only */
> +module_param(chkcfg, int, 0664);
> +MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking (default 
> 0)");
> +
> +static int numclks = 1; /* default 1 for uncompressed and unencrypted */
> +module_param(numclks, int, 0664);
> +MODULE_PARM_DESC(numclks, "Clock to data ratio 1, 4 or 8 (default 1)");

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

> +
> +struct altera_cvp_conf {
> +   struct fpga_manager *mgr;
> +   struct pci_dev  *pci_dev;
> +   void __iomem*map;
> +   void(*write_data)(struct altera_cvp_conf *conf,
> + u32 val);
> +   charmgr_name[64];
> +};
> +

> +static inline void altera_cvp_chk_numclks(void)
> +{
> +   switch (numclks) {
> +   case 1:
> +   case 4:
> +   case 8:
> +   break;
> +   default:
> +   numclks = 1;
> +   break;
> +   }
> +}

Why 2 is missed? Hardware limitation? Needs a comment about these magics.

> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
> +{
> +   u32 val32;

> +   int i;

unsigned int i; ?

> +
> +   /* set number of CVP clock cycles for every CVP Data Register Write */
> +   pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, );
> +   val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;

> +   val32 |= 0x01 << 8; /* 1 clock */

Yeah, needs more clear way to put clocks of choice.

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

> +}

> +
> +static int altera_cvp_teardown(struct fpga_manager *mgr,
> +  struct fpga_image_info *info)
> +{
> +   struct altera_cvp_conf *conf = mgr->priv;
> +   struct pci_dev *pdev = conf->pci_dev;
> +   int status = 0;
> +   int delay_us;
> +   u32 val32;
> +

> +   /*
> +* STEP 12 - reset START_XFER bit
> +*/

One line?
(as well below)

> +   pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, );
> +   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);

style. It will on smallest glance give reader a clue that the body
will go at least once.

> +   pci_read_config_dword(pdev, VSEC_CVP_STATUS, );
> +   if ((val32 & VSEC_CVP_STATUS_CFG_RDY) == 0)
> +   break;
> +

> +   udelay(1);  /* wait 1us */

Why not 10? Needs a comment.

> +
> +   if (delay_us++ > TIMEOUT_IN_US) {
> +   dev_warn(>dev, "CVP_CONFIG_READY == 0 
> timeout\n");
> +   status = -ETIMEDOUT;
> +   break;
> +   }
> +   }
> +
> +   return status;
> +}

> +
> +static int altera_cvp_write_init(struct fpga_manager *mgr,
> +struct fpga_image_info *info,
> +const char *buf, size_t count)
> +{
> +   struct altera_cvp_conf *conf = mgr->priv;
> +   struct pci_dev *pdev = conf->pci_dev;
> +   int delay_us;
> +   u32 val32;
> +   int ret;

> +   /*
> +* STEP 1 - read CVP status and check CVP_EN flag
> +   

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

[Alan Tull]

On Sun, Apr 30, 2017 at 2:08 PM, Anatolij Gustschin  wrote:

Hi Anatolij,

Looks good!

> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
> and Arria-10 FPGAs via CvP.
>
> Signed-off-by: Anatolij Gustschin 
Signed-off-by: Alan Tull 

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

[Alan Tull]

On Sun, Apr 30, 2017 at 2:08 PM, Anatolij Gustschin  wrote:

Hi Anatolij,

Looks good!

> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
> and Arria-10 FPGAs via CvP.
>
> Signed-off-by: Anatolij Gustschin 
Signed-off-by: Alan Tull