On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin <[email protected]> wrote:
> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
> and Arria-10 FPGAs via CvP.
Few comments from me.
> +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);
Is it too far beyond 80 characters? I would leave it in one line (~83
characters are okay).
> + char mgr_name[64];
> + u8 numclks;
> +};
> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
> +{
> + unsigned int i;
> + u32 val32;
Drop the useless suffix.
> +}
> +static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_msk,
> + u32 status_val, int timeout_us)
> +{
> + u32 val32;
Ditto.
> + if (!timeout_us)
> + return -ETIMEDOUT;
Hmm...
What as a user I would expect here is at least one attempt (0 -- no
timeout, but try once).
> +
> + do {
> + /* use small usleep value to re-check and break early */
> + usleep_range(10, 11);
> +
> + pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
> + if ((val32 & status_msk) == status_val)
> + return 0;
> +
> + timeout_us -= 10;
> + } while (timeout_us > 0);
> +
> + return -ETIMEDOUT;
> +}
> +static int altera_cvp_teardown(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + u32 val32;
Drop the suffix.
> + return ret;
> +}
> +static int altera_cvp_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + u32 val32;
Ditto.
> + int ret;
> +
> + /* clock to data ratio for uncompressed and unencrypted images */
> + conf->numclks = 1;
To else branch of below?
> + if (info) {
> + }
> + ret = altera_cvp_teardown(mgr, info);
> + if (ret < 0)
> + return ret;
What is the meaning of ret > 0?
Can't it be just if (ret) here?
> + ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY,
> + VSEC_CVP_STATUS_CFG_RDY, 10);
> + if (ret < 0) {
> + dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n");
> + return ret;
> + }
Ditto.
Also check another code like this above and below.
> + return 0;
> +}
> +static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
> +{
> + struct altera_cvp_conf *conf = mgr->priv;
> + u32 val32;
Suffix.
> +}
> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + u32 status_msk;
status_mask
> + u32 val32;
Drop the suffix.
> +}
> +static ssize_t show_chkcfg(struct device_driver *dev, char *buf)
> +{
> + return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);
Just altera_cvp_chkcfg.
> +}
> +static struct pci_device_id altera_cvp_id_tbl[] = {
> + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
Does it have dedicated PCI class?
PCI_ANY_ID usually is too broad.
> +static int altera_cvp_probe(struct pci_dev *pdev,
> + const struct pci_device_id *dev_id)
> +{
> + struct altera_cvp_conf *conf;
> + u16 cmd, val16;
Drop the suffix.
> + /*
> + * First check if this is the expected FPGA device. PCI config
> + * space access works without enabling the pci device, memory
pci -> PCI
> + * space access is enabled further down.
> + */
> + /*
> + * Enable memory BAR access. We cannot use pci_enable_device() here
> + * because it will make the driver unusable with FPGA devices that
> + * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit
iomem -> IOMEM
> + * platform. Such BARs will not have an assigned address range and
> + * pci_enable_device() will fail, complaining about not claimed BAR,
> + * even if the concerned BAR is not needed for FPGA configuration
> + * at all. Thus, enable the device via pci config space command.
pci -> PCI
> + */
> + ret = pci_request_region(pdev, CVP_BAR, "CVP");
> + if (ret < 0) {
if (ret)
> + dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
> + goto err;
> + }
> + snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d",
> + ALTERA_CVP_MGR_NAME,
pdev->bus->number,
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
pci_name() ?
> +err_unmap:
> + pci_iounmap(pdev, conf->map);
> + pci_release_region(pdev, CVP_BAR);
> +err:
err_disable:
> + cmd &= ~PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
On Fri, Jun 2, 2017 at 8:43 PM, Andy Shevchenko
<[email protected]> wrote:
> On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin <[email protected]> wrote:
>> Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V
>> and Arria-10 FPGAs via CvP.
>
> Few comments from me.
After addressing them, FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko [email protected] wrote:
...
>
>> + void (*write_data)(struct altera_cvp_conf *conf,
>> + u32 val);
>
>Is it too far beyond 80 characters? I would leave it in one line (~83
>characters are okay).
I changed to single line, (*write_data)(struct altera_cvp_conf *, u32);
...
>> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
>> +{
>> + unsigned int i;
>> + u32 val32;
>
>Drop the useless suffix.
ok, done.
>> + if (!timeout_us)
>> + return -ETIMEDOUT;
>
>Hmm...
>What as a user I would expect here is at least one attempt (0 -- no
>timeout, but try once).
yes, this first attempt is above, please see original patch for full
context.
...
>> + /* clock to data ratio for uncompressed and unencrypted images */
>> + conf->numclks = 1;
>
>To else branch of below?
>
>> + if (info) {
>
>> + }
then, the default numclks value would have to be set in the if branch
as well. It is easier to set it before.
...
>> + ret = altera_cvp_teardown(mgr, info);
>> + if (ret < 0)
>> + return ret;
>
>What is the meaning of ret > 0?
>
>Can't it be just if (ret) here?
return value > 0 is never used, here it can be if (ret).
...
>> + ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY,
>> + VSEC_CVP_STATUS_CFG_RDY, 10);
>> + if (ret < 0) {
>> + dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n");
>> + return ret;
>> + }
>
>Ditto.
>
>Also check another code like this above and below.
ok, done.
>> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
>> + struct fpga_image_info *info)
>> +{
>
>> + u32 status_msk;
>
>status_mask
>
>> + u32 val32;
>
>Drop the suffix.
done.
...
>> + return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);
>
>Just altera_cvp_chkcfg.
ok, done.
...
>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>> + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>
>Does it have dedicated PCI class?
>
>PCI_ANY_ID usually is too broad.
no, it doesn't have dedicated class.
...
>> + u16 cmd, val16;
>
>Drop the suffix.
>> + * space access works without enabling the pci device, memory
>
>pci -> PCI
ok, done.
...
>> + * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit
>
>iomem -> IOMEM
done.
...
>> + * at all. Thus, enable the device via pci config space command.
>
>pci -> PCI
ok.
...
>> + ret = pci_request_region(pdev, CVP_BAR, "CVP");
>> + if (ret < 0) {
>
>if (ret)
ok, done.
...
>> + snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d",
>> + ALTERA_CVP_MGR_NAME,
>
>pdev->bus->number,
>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>
>pci_name() ?
ok.
>> +err_unmap:
>> + pci_iounmap(pdev, conf->map);
>> + pci_release_region(pdev, CVP_BAR);
>
>> +err:
>
>err_disable:
done.
Thanks,
Anatolij
On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <[email protected]> wrote:
> On Fri, 2 Jun 2017 20:43:21 +0300
> Andy Shevchenko [email protected] wrote:
Besides below comments, please, do
s/VSEC_/VSE_/g
for entire file.
We are following PCI and Thunderbolt pattern for use of Vendor
Specific Extended Capability.
>>> + if (!timeout_us)
>>> + return -ETIMEDOUT;
>>
>>Hmm...
>>What as a user I would expect here is at least one attempt (0 -- no
>>timeout, but try once).
>
> yes, this first attempt is above, please see original patch for full
> context.
Ah, it means you don't correctly use do {} while approach.
Remove everything above do { and move usleep after check for the
status inside the loop.
>>> + /* clock to data ratio for uncompressed and unencrypted images */
>>> + conf->numclks = 1;
>>
>>To else branch of below?
>>
>>> + if (info) {
>>
>>> + }
>
> then, the default numclks value would have to be set in the if branch
> as well. It is easier to set it before.
See the magic below:
u32 iflags = info ? info->flags : 0;
...
if (iflags & FPGA_MGR_PARTIAL_RECONFIG) {
dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
return -EINVAL;
}
if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM)
conf->numclks = 8; /* ratio for all compressed images */
else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM)
conf->numclks = 4; /* ratio for encrypted images */
else
conf->numclks = 1; /* clock to data ratio for
uncompressed and unencrypted images */
I would really recommend to double check the code every time you are
about to send.
A little thought can make a beauteful result!
>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>> + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>>
>>Does it have dedicated PCI class?
>>
>>PCI_ANY_ID usually is too broad.
>
> no, it doesn't have dedicated class.
Hmm... It means any device of this vendor will jump into this
driver... Not good.
--
With Best Regards,
Andy Shevchenko
On Thu, 8 Jun 2017 02:38:55 +0300
Andy Shevchenko [email protected] wrote:
>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <[email protected]> wrote:
>> On Fri, 2 Jun 2017 20:43:21 +0300
>> Andy Shevchenko [email protected] wrote:
>
>Besides below comments, please, do
>
>s/VSEC_/VSE_/g
>
>for entire file.
>
>We are following PCI and Thunderbolt pattern for use of Vendor
>Specific Extended Capability.
I can do it, but I'm just not getting why. The registers are named as VSEC
registers in the documentation, why should the code name them differently?
...
>>>> + if (!timeout_us)
>>>> + return -ETIMEDOUT;
>>>
>>>Hmm...
>>>What as a user I would expect here is at least one attempt (0 -- no
>>>timeout, but try once).
>>
>> yes, this first attempt is above, please see original patch for full
>> context.
>
>Ah, it means you don't correctly use do {} while approach.
>
>Remove everything above do { and move usleep after check for the
>status inside the loop.
Unfortunately, suggested approach has an unwanted side effect:
do {
check and return if done;
usleep_range(10, 11);
tout -= 10;
} while (tout > 0);
For simplicity, let's say we were asked to wait with 20 µs timeout.
Assume, that the device reports ready status after 17 µs. The first
check is done, we don't return and sleep approx. 10 µs. Then, the 2nd
check is done and we continue to wait another 10 µs and the loop ends
signalling a timeout. But in the meantime the device reported ready
status. Additional check would be needed after the loop.
In some cases the device reports ready status immediately. That's
the reason why I check first and then loop with more wait&check cycles.
...
>See the magic below:
>
> u32 iflags = info ? info->flags : 0;
>...
> if (iflags & FPGA_MGR_PARTIAL_RECONFIG) {
> dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> return -EINVAL;
> }
>
> if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM)
> conf->numclks = 8; /* ratio for all compressed images */
> else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM)
> conf->numclks = 4; /* ratio for encrypted images */
> else
> conf->numclks = 1; /* clock to data ratio for
>uncompressed and unencrypted images */
>
>I would really recommend to double check the code every time you are
>about to send.
>A little thought can make a beauteful result!
ok, changed as suggested.
>>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>>> + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>>>
>>>Does it have dedicated PCI class?
>>>
>>>PCI_ANY_ID usually is too broad.
>>
>> no, it doesn't have dedicated class.
>
>Hmm... It means any device of this vendor will jump into this
>driver... Not good.
in an early patch version I was asked by Intel people to use PCI_ANY_ID
because these devices are not set in stone. The implemented FPGA PCIe
devices can have varying IDs. probe() checks for expected capability ID
and stops if we hit a not supported device.
Thanks,
Anatolij
On Thu, Jun 8, 2017 at 5:15 PM, Anatolij Gustschin <[email protected]> wrote:
> On Thu, 8 Jun 2017 02:38:55 +0300
> Andy Shevchenko [email protected] wrote:
>>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <[email protected]> wrote:
>>> On Fri, 2 Jun 2017 20:43:21 +0300
>>> Andy Shevchenko [email protected] wrote:
>>Besides below comments, please, do
>>
>>s/VSEC_/VSE_/g
>>
>>for entire file.
>>
>>We are following PCI and Thunderbolt pattern for use of Vendor
>>Specific Extended Capability.
>
> I can do it, but I'm just not getting why. The registers are named as VSEC
> registers in the documentation, why should the code name them differently?
Does your documentation decode VSEC abbreviation?
What C stands for? Capability?
In PCI and Thunderbolt we agreed to use word capability separately,
so, either XXX_... or XXX_CAP_... to use.
>>>>> + if (!timeout_us)
>>>>> + return -ETIMEDOUT;
>>>>
>>>>Hmm...
>>>>What as a user I would expect here is at least one attempt (0 -- no
>>>>timeout, but try once).
>>>
>>> yes, this first attempt is above, please see original patch for full
>>> context.
>>
>>Ah, it means you don't correctly use do {} while approach.
>>
>>Remove everything above do { and move usleep after check for the
>>status inside the loop.
>
> Unfortunately, suggested approach has an unwanted side effect:
How come? See below.
>
> do {
> check and return if done;
>
> usleep_range(10, 11);
> tout -= 10;
>
> } while (tout > 0);
>
> For simplicity, let's say we were asked to wait with 20 µs timeout.
> Assume, that the device reports ready status after 17 µs. The first
> check is done, we don't return and sleep approx. 10 µs. Then, the 2nd
> check is done and we continue to wait another 10 µs and the loop ends
> signalling a timeout. But in the meantime the device reported ready
> status. Additional check would be needed after the loop.
> In some cases the device reports ready status immediately. That's
> the reason why I check first and then loop with more wait&check cycles.
Just look to the rest of the code in kernel
Most of the timeout related loops we have the following pattern:
unsigned int retries = XXX;
do {
...check for something...
if (yes)
return YY;
...sleep for a while...
} while (--retries);
if (!retries)
return -ETIMEDOUT;
What I'm suggesting is to follow the pattern (adjust it for your exact
conditions and so on).
>>>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>>>> + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>>>>
>>>>Does it have dedicated PCI class?
>>>>
>>>>PCI_ANY_ID usually is too broad.
>>>
>>> no, it doesn't have dedicated class.
>>
>>Hmm... It means any device of this vendor will jump into this
>>driver... Not good.
>
> in an early patch version I was asked by Intel people to use PCI_ANY_ID
> because these devices are not set in stone. The implemented FPGA PCIe
> devices can have varying IDs. probe() checks for expected capability ID
> and stops if we hit a not supported device.
Yeah, the problem is that every device with a such Vendor ID would be
considered by this driver and PCI class would be helpful here just to
reduce an impact.
Capability approach works, though it's slightly more error prone.
I have no other comment on this. For now it seems the only choice
since such IPs are on the market, right?
--
With Best Regards,
Andy Shevchenko
On Thu, 8 Jun 2017 17:44:19 +0300
Andy Shevchenko [email protected] wrote:
>On Thu, Jun 8, 2017 at 5:15 PM, Anatolij Gustschin <[email protected]> wrote:
>> On Thu, 8 Jun 2017 02:38:55 +0300
>> Andy Shevchenko [email protected] wrote:
>>>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <[email protected]> wrote:
>>>> On Fri, 2 Jun 2017 20:43:21 +0300
>>>> Andy Shevchenko [email protected] wrote:
>
>>>Besides below comments, please, do
>>>
>>>s/VSEC_/VSE_/g
>>>
>>>for entire file.
>>>
>>>We are following PCI and Thunderbolt pattern for use of Vendor
>>>Specific Extended Capability.
>>
>> I can do it, but I'm just not getting why. The registers are named as VSEC
>> registers in the documentation, why should the code name them differently?
>
>Does your documentation decode VSEC abbreviation?
>What C stands for? Capability?
the CvP user guide talks about Vendor Specific Extended Capability (VSEC).
--
Anatolij