Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 2 Jun 2017 20:43:21 +0300 | Subject | Re: [PATCH v5] fpga manager: Add Altera CvP driver |
| |
On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin <agust@denx.de> 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
| |