Re: [PATCH v4] fpga manager: Add Altera CvP driver
From: Andy Shevchenko
Date: Mon May 01 2017 - 16:06:25 EST
On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin <agust@xxxxxxx> 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_BAR 0 /* 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_OFFSET 0x200
> +#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_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 <<)
> +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);
> + char mgr_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);
> + 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);
> + 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, &val32);
> + 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(&mgr->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
> + */
Ditto about comments.
> + delay_us = 0;
> + while (1) {
Ditto about loop style.
> + pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
> + if ((val32 & VSEC_CVP_STATUS_CFG_RDY) ==
> + VSEC_CVP_STATUS_CFG_RDY)
> + break;
> +
> + udelay(1); /* wait 1us */
> +
> + if (delay_us++ > TIMEOUT_IN_US) {
> + dev_warn(&mgr->dev, "CVP_CONFIG_READY == 1 timeout\n");
> + return -ETIMEDOUT;
> + }
> + }
> + 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;
> + pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
> + if (val32 & VSEC_CVP_STATUS_CFG_ERR) {
> + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zi bytes!\n",
> + bytes);
%zu?
> + return -EPROTO;
> + }
> + return 0;
> +}
> +
> +static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
> + size_t count)
> +{
> + struct altera_cvp_conf *conf = mgr->priv;
> + const u32 *data;
> + size_t bytes;
> + int status = 0;
> + u32 mask;
> +
> + data = (u32 *)buf;
> + bytes = count;
> +
> + while (bytes >= 4) {
> + conf->write_data(conf, *data++);
> + bytes -= 4;
> +
> + /*
> + * STEP 10 (optional) and STEP 11
> + * - check error flag
> + * - loop until data transfer completed
> + */
> + if (chkcfg && !(bytes % SZ_4K)) {
Is 4k comes from PCI spec, or is it page size?
> + size_t done = count - bytes;
> +
> + status = altera_cvp_chk_error(mgr, done);
> + if (status < 0)
> + return status;
> + }
> + }
> +
> + switch (bytes) {
> + case 3:
> + mask = 0x00ffffff;
> + break;
> + case 2:
> + mask = 0x0000ffff;
> + break;
> + case 1:
> + mask = 0x000000ff;
> + break;
> + case 0:
> + mask = 0x00000000;
> + break;
> + }
Why not to use simple formula?
mask = BIT(bytes * 8) - 1;
> +
> + if (mask) {
> + conf->write_data(conf, *data & mask);
> +
> + if (chkcfg)
> + status = altera_cvp_chk_error(mgr, count);
> + }
> +
> + return status;
> +}
> +static int altera_cvp_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + delay_us = 0;
> + status_msk = VSEC_CVP_STATUS_PLD_CLK_IN_USE | VSEC_CVP_STATUS_USERMODE;
> + while (1) {
Same comment about loop style.
> + pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
> + if ((val32 & status_msk) == status_msk)
> + break;
> +
> + udelay(1); /* wait 1us */
> +
> + if (delay_us++ > TIMEOUT_IN_US) {
> + dev_warn(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
> + status = -ETIMEDOUT;
> + break;
> + }
> + }
> +
> + return status;
> +}
> +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.
> + if (val16 != VSEC_PCIE_EXT_CAP_ID_VAL) {
> + dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val16);
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
> + if (!conf) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + conf->pci_dev = pdev;
> +
> + if (pci_request_region(pdev, CVP_BAR, "CVP") < 0) {
So, you are using devm_ above, but avoid pcim_ here. Please clarify
enabling device case and use if possible pcim_
> + dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + conf->write_data = altera_cvp_write_data_iomem;
> +
> + 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.
> + conf->write_data = altera_cvp_write_data_config;
> + }
> + conf->mgr = fpga_mgr_get(&pdev->dev);
> + if (IS_ERR(conf->mgr)) {
> + dev_err(&pdev->dev, "Getting fpga mgr reference failed\n");
> + ret = -ENODEV;
Why error is shadowed here?
> + goto err_unreg;
> + }
> + fpga_mgr_put(conf->mgr);
> +static int __init altera_cvp_init(void)
> +{
> + return pci_register_driver(&altera_cvp_driver);
> +}
> +
> +static void __exit altera_cvp_exit(void)
> +{
> + pci_unregister_driver(&altera_cvp_driver);
> +}
> +
> +module_init(altera_cvp_init);
> +module_exit(altera_cvp_exit);
I dunno for how many (3+ I think) years we have macros like
module_pci_driver();
Please, use it instead of above.
--
With Best Regards,
Andy Shevchenko