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

From: Andy Shevchenko
Date: Fri Jun 02 2017 - 13:43:29 EST


On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin <agust@xxxxxxx> 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