| Message ID | 1493579317-28608-1-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sun, Apr 30, 2017 at 2:08 PM, Anatolij Gustschin <agust@denx.de> 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 <agust@denx.de> Signed-off-by: Alan Tull <atull@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Apr 30, 2017 at 10:08 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. 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.
On Mon, 1 May 2017 23:06:16 +0300 Andy Shevchenko andy.shevchenko@gmail.com wrote: >On Sun, Apr 30, 2017 at 10:08 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. > >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"? 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_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 <<) 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); >> + 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. 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); >> + 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); >> + 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. 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 this is not obvious, 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. > >> + >> + 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. ok, will change. >> + 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? ok, will fix. >> + 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? 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. >> + 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; ok, will use it. >> + >> + 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. ok, will change. >> + 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. 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). >> + 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_ I can't use pcim_enable_device(), it will make the driver unusable on some platforms. The driver is only for re-configuring FPGAs and there can be unlimited variations of different PCIe devices implemented in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for which an address range cannot be assigned on embedded 32-bit platforms. pcim_enable_device() will fail here complaining about not claimed BAR, even if the concerned BAR is not needed for FPGA configuration at all. This makes the driver unusable. >> + 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. can I use other pcim_ functions if pcim_enalbe_device is not used? >> + 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? oh, actually this mgr_get/mgr_put sequence is not needed here any more, just forgot to remove it. ... >> +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. ok, will do. Thanks, Anatolij -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 2, 2017 at 12:53 PM, Anatolij Gustschin <agust@denx.de> wrote: > On Mon, 1 May 2017 23:06:16 +0300 > Andy Shevchenko andy.shevchenko@gmail.com wrote: >>On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin <agust@denx.de> wrote: >>> +#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 <<) > > 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); >>> + 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); >>> + 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, &val32); >>> + 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; >>> + 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. > > 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. So, - can you use it? - if no, elaborate why not Okay, looks like below you answer this somehow. >>So, you are using devm_ above, but avoid pcim_ here. Please clarify >>enabling device case and use if possible pcim_ > > I can't use pcim_enable_device(), it will make the driver unusable > on some platforms. > The driver is only for re-configuring FPGAs and > there can be unlimited variations of different PCIe devices implemented > in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for > which an address range cannot be assigned on embedded 32-bit > platforms. pcim_enable_device() will fail here complaining about > not claimed BAR, even if the concerned BAR is not needed for FPGA > configuration at all. This makes the driver unusable. Please put something like above in the comment in ->probe() before first call to pci_...(). >>> + 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. > > can I use other pcim_ functions if pcim_enalbe_device is not used? You may check yourselt, IIRC no, you can't use pcim_ioremap*().
On Tue, 2017-05-02 at 11:53 +0200, Anatolij Gustschin wrote: > On Mon, 1 May 2017 23:06:16 +0300 Andy Shevchenko andy.shevchenko@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. -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 3 May 2017 00:28:17 +0300 Andy Shevchenko andy.shevchenko@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); >>>> + 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, &val32); >>>> + 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, &val16); >>> >>>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, &cmd); + if (!(cmd & PCI_COMMAND_MEMORY)) { + cmd |= PCI_COMMAND_MEMORY; + pci_write_config_word(pdev, PCI_COMMAND, cmd); + } >So, >- can you use it? >- if no, elaborate why not > >Okay, looks like below you answer this somehow. > >>>So, you are using devm_ above, but avoid pcim_ here. Please clarify >>>enabling device case and use if possible pcim_ >> >> I can't use pcim_enable_device(), it will make the driver unusable >> on some platforms. > >> The driver is only for re-configuring FPGAs and >> there can be unlimited variations of different PCIe devices implemented >> in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for >> which an address range cannot be assigned on embedded 32-bit >> platforms. pcim_enable_device() will fail here complaining about >> not claimed BAR, even if the concerned BAR is not needed for FPGA >> configuration at all. This makes the driver unusable. > >Please put something like above in the comment in ->probe() before >first call to pci_...(). ok Thanks, Anatolij -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 02 May 2017 16:36:54 -0700
Joe Perches joe@perches.com wrote:
...
>It would with command line option --strict, otherwise not.
ah, good to know. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 2, 2017 at 7:14 PM, Anatolij Gustschin <agust@denx.de> wrote: > On Wed, 3 May 2017 00:28:17 +0300 > Andy Shevchenko andy.shevchenko@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 -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 3, 2017 at 3:14 AM, Anatolij Gustschin <agust@denx.de> wrote: > On Wed, 3 May 2017 00:28:17 +0300 > Andy Shevchenko andy.shevchenko@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, &cmd); > + 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, &cmd); if (!(cmd & PCI_COMMAND_MEMORY)) { cmd |= PCI_COMMAND_MEMORY; pci_write_config_word(pdev, PCI_COMMAND, cmd); } }
On Wed, 3 May 2017 18:01:19 +0300 Andy Shevchenko andy.shevchenko@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, &cmd); >> + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 161ba9d..895529e 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -26,6 +26,13 @@ config FPGA_MGR_ICE40_SPI help FPGA manager driver support for Lattice iCE40 FPGAs over SPI. +config FPGA_MGR_ALTERA_CVP + tristate "Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager" + depends on PCI + help + FPGA manager driver support for Arria-V, Cyclone-V, Stratix-V + and Arria 10 Altera FPGAs using the CvP interface over PCIe. + config FPGA_MGR_SOCFPGA tristate "Altera SOCFPGA FPGA Manager" depends on ARCH_SOCFPGA || COMPILE_TEST diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index 2a4f021..2e5c8b6 100644 --- a/drivers/fpga/Makefile +++ b/drivers/fpga/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_FPGA) += fpga-mgr.o # FPGA Manager Drivers +obj-$(CONFIG_FPGA_MGR_ALTERA_CVP) += altera-cvp.o obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.o diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c new file mode 100644 index 0000000..4903231 --- /dev/null +++ b/drivers/fpga/altera-cvp.c @@ -0,0 +1,549 @@ +/* + * FPGA Manager Driver for Altera Arria/Cyclone/Stratix CvP + * + * Copyright (C) 2017 DENX Software Engineering + * + * Anatolij Gustschin <agust@denx.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Manage Altera FPGA firmware using PCIe CvP. + * Firmware must be in binary "rbf" format. + */ + +#include <linux/delay.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/sizes.h> + +#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 + +/* offsets from VSEC_OFFSET */ +#define VSEC_PCIE_EXT_CAP_ID (VSEC_OFFSET + 0x00) /* 16bit */ +#define VSEC_VERSION 0x02 /* 8bit */ +#define VSEC_NEXT_CAP_OFF 0x03 /* 8bit */ +#define VSEC_ID 0x04 /* 16bit */ +#define VSEC_REV 0x06 /* 8bit */ +#define VSEC_LENGTH 0x07 /* 8bit */ +#define VSEC_ALTERA_MARKER 0x08 /* 32bit */ + +#define VSEC_CVP_STATUS (VSEC_OFFSET + 0x1c) /* 32bit */ +#define VSEC_CVP_STATUS_DATA_ENC BIT(16) /* is treated as encrypted */ +#define VSEC_CVP_STATUS_DATA_COMP BIT(17) /* is treated as compressed */ +#define VSEC_CVP_STATUS_CFG_RDY BIT(18) /* CVP_CONFIG_READY */ +#define VSEC_CVP_STATUS_CFG_ERR BIT(19) /* CVP_CONFIG_ERROR */ +#define VSEC_CVP_STATUS_CVP_EN BIT(20) /* ctrl block is enabling CVP */ +#define VSEC_CVP_STATUS_USERMODE BIT(21) /* USERMODE */ +#define VSEC_CVP_STATUS_CFG_DONE BIT(23) /* CVP_CONFIG_DONE */ +#define VSEC_CVP_STATUS_PLD_CLK_IN_USE BIT(24) /* PLD_CLK_IN_USE */ + +#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 */ + +#define VSEC_CVP_DATA (VSEC_OFFSET + 0x28) /* 32bit */ +#define VSEC_CVP_PROG_CTRL (VSEC_OFFSET + 0x2c) /* 32bit */ +#define VSEC_CVP_PROG_CTRL_CONFIG BIT(0) +#define VSEC_CVP_PROG_CTRL_START_XFER BIT(1) + +#define VSEC_UNCOR_ERR_STATUS (VSEC_OFFSET + 0x34) /* 32bit */ +#define VSEC_UNCOR_ERR_MASK (VSEC_OFFSET + 0x38) /* 32bit */ +#define VSEC_UNCOR_ERR_CVP_CFG_ERR BIT(5) /* CVP_CONFIG_ERROR_LATCHED */ + +#define DRV_NAME "altera-cvp" +#define ALTERA_CVP_MGR_NAME "Altera CvP FPGA Manager" + +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)"); + +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; + } +} + +static enum fpga_mgr_states altera_cvp_state(struct fpga_manager *mgr) +{ + struct altera_cvp_conf *conf = mgr->priv; + u32 status; + + pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &status); + + if (status & VSEC_CVP_STATUS_CFG_DONE) + return FPGA_MGR_STATE_OPERATING; + + if (status & VSEC_CVP_STATUS_CVP_EN) + return FPGA_MGR_STATE_POWER_UP; + + return FPGA_MGR_STATE_UNKNOWN; +} + +static void altera_cvp_write_data_iomem(struct altera_cvp_conf *conf, u32 val) +{ + writel(val, conf->map); +} + +static void altera_cvp_write_data_config(struct altera_cvp_conf *conf, u32 val) +{ + pci_write_config_dword(conf->pci_dev, VSEC_CVP_DATA, val); +} + +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf) +{ + u32 val32; + 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 */ + 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); +} + +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 + */ + 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); + + /* + * STEP 13 - reset CVP_CONFIG bit + */ + val32 &= ~VSEC_CVP_PROG_CTRL_CONFIG; + pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32); + + /* + * STEP 14 + * - set CVP_NUMCLKS to 0x01 and then issue CVP_DUMMY_WR dummy + * writes to the HIP + */ + altera_cvp_dummy_write(conf); /* from CVP clock to internal clock */ + + /* + * STEP 15 - poll CVP_CONFIG_READY bit + */ + delay_us = 0; + while (1) { + pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32); + if ((val32 & VSEC_CVP_STATUS_CFG_RDY) == 0) + break; + + udelay(1); /* wait 1us */ + + 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; + + if (info && info->flags & FPGA_MGR_PARTIAL_RECONFIG) { + dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); + return -EINVAL; + } + + /* + * STEP 1 - read CVP status and check CVP_EN flag + */ + pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32); + if (!(val32 & VSEC_CVP_STATUS_CVP_EN)) { + dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val32); + return -ENODEV; + } + + if (val32 & VSEC_CVP_STATUS_CFG_RDY) { + dev_warn(&mgr->dev, "CvP already started, teardown first\n"); + ret = altera_cvp_teardown(mgr, info); + if (ret < 0) + return ret; + } + + /* + * STEP 2 + * - set HIP_CLK_SEL and CVP_MODE (must be set in the order mentioned) + */ + /* switch from fabric to PMA clock */ + pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32); + val32 |= VSEC_CVP_MODE_CTRL_HIP_CLK_SEL; + pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32); + + /* set CVP mode */ + pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32); + val32 |= VSEC_CVP_MODE_CTRL_CVP_MODE; + pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32); + + /* + * STEP 3 + * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP + */ + altera_cvp_dummy_write(conf); + + /* + * STEP 4 - set CVP_CONFIG bit + */ + pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32); + /* request control block to begin transfer using CVP */ + val32 |= VSEC_CVP_PROG_CTRL_CONFIG; + pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32); + + /* + * STEP 5 - poll CVP_CONFIG READY + */ + delay_us = 0; + while (1) { + 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; + } + } + + /* + * STEP 6 + * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP + */ + altera_cvp_dummy_write(conf); /* from internal clock to CVP clock */ + + /* + * STEP 7 - set START_XFER + */ + 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); + + /* + * STEP 8 - start transfer (set CVP_NUMCLKS) + */ + altera_cvp_chk_numclks(); + pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32); + val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS; + val32 |= numclks << 8; /* bitstream specific clock count */ + pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32); + + 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; + + /* + * STEP 10 (optional) - check CVP_CONFIG_ERROR flag + */ + 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); + 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; + + /* + * STEP 9 + * - write 32-bit configuration data from RBF file to CVP data register + */ + 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)) { + 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; + } + + 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) +{ + struct altera_cvp_conf *conf = mgr->priv; + struct pci_dev *pdev = conf->pci_dev; + int status; + int delay_us; + u32 status_msk; + u32 val32; + + status = altera_cvp_teardown(mgr, info); + if (status < 0) + return status; + + /* + * STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit + */ + pci_read_config_dword(pdev, VSEC_UNCOR_ERR_STATUS, &val32); + if (val32 & VSEC_UNCOR_ERR_CVP_CFG_ERR) { + dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n"); + return -EFAULT; + } + + /* + * STEP 17 - reset CVP_MODE and HIP_CLK_SEL bit + */ + pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32); + val32 &= ~VSEC_CVP_MODE_CTRL_HIP_CLK_SEL; + val32 &= ~VSEC_CVP_MODE_CTRL_CVP_MODE; + pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32); + + /* + * STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits + */ + delay_us = 0; + status_msk = VSEC_CVP_STATUS_PLD_CLK_IN_USE | VSEC_CVP_STATUS_USERMODE; + while (1) { + 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 const struct fpga_manager_ops altera_cvp_ops = { + .state = altera_cvp_state, + .write_init = altera_cvp_write_init, + .write = altera_cvp_write, + .write_complete = altera_cvp_write_complete, +}; + +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); + 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; + } + + /* Enable memory BAR access */ + pci_read_config_word(pdev, PCI_COMMAND, &cmd); + if (!(cmd & PCI_COMMAND_MEMORY)) { + cmd |= PCI_COMMAND_MEMORY; + pci_write_config_word(pdev, PCI_COMMAND, cmd); + } + + 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) { + 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"); + conf->write_data = altera_cvp_write_data_config; + } + + 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)); + + ret = fpga_mgr_register(&pdev->dev, conf->mgr_name, + &altera_cvp_ops, conf); + if (ret) + goto err_unmap; + + conf->mgr = fpga_mgr_get(&pdev->dev); + if (IS_ERR(conf->mgr)) { + dev_err(&pdev->dev, "Getting fpga mgr reference failed\n"); + ret = -ENODEV; + goto err_unreg; + } + fpga_mgr_put(conf->mgr); + + return 0; + +err_unreg: + fpga_mgr_unregister(&pdev->dev); +err_unmap: + pci_iounmap(pdev, conf->map); + pci_release_region(pdev, CVP_BAR); +err: + cmd &= ~PCI_COMMAND_MEMORY; + pci_write_config_word(pdev, PCI_COMMAND, cmd); + return ret; +} + +static void altera_cvp_remove(struct pci_dev *pdev) +{ + struct fpga_manager *mgr = pci_get_drvdata(pdev); + struct altera_cvp_conf *conf = mgr->priv; + u16 cmd; + + fpga_mgr_unregister(&pdev->dev); + pci_iounmap(pdev, conf->map); + pci_release_region(pdev, CVP_BAR); + pci_read_config_word(pdev, PCI_COMMAND, &cmd); + cmd &= ~PCI_COMMAND_MEMORY; + pci_write_config_word(pdev, PCI_COMMAND, cmd); +} + +#define PCI_VENDOR_ID_ALTERA 0x1172 + +static struct pci_device_id altera_cvp_id_tbl[] = { + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) }, + { } +}; + +MODULE_DEVICE_TABLE(pci, altera_cvp_id_tbl); + +static struct pci_driver altera_cvp_driver = { + .name = DRV_NAME, + .id_table = altera_cvp_id_tbl, + .probe = altera_cvp_probe, + .remove = altera_cvp_remove, +}; + +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); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Anatolij Gustschin <agust@denx.de>"); +MODULE_DESCRIPTION("Module to load Altera FPGA over CvP");
Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V and Arria-10 FPGAs via CvP. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- Changes in v4: - Update description about supported FPGA models in Kconfig and commit log - use writel() for iomem accesses - factor out dummy write code to altera_cvp_dummy_write() - add data register accessor function (iomem and pci config space variants) to reduce code Changes in v3: - removed V-series from description (since the driver works also with Arria-10). Also renamed functions, config option and driver file name. Changed module description in Kconfig - dropped 'firmware=' module option (does not allow to bypass the fpga framework any more) - fixed build warning with newer gcc - changed to use hex numbers for PCI bus/device in registered fpga manager name. Use more generic fpga manager name (no V-series in the name any more) - moved steps 16 to 18 from teardown func to write_complete() as suggested by Yi - rebased on linux-next and tested with Arria-10/Stratix-V devices Changes in v2: - rebase for v4.10, change subject ("Stratix V" to "V series") and add GPL header - use BIT() in register bit macro definitions - change .state() to return status or FPGA_MGR_STATE_UNKNOWN - use unique name for registered FPGA manager (append "@B:D.F") - use PCI_ANY_ID for device ID, users can set custom ID using new_id sysfs interface - remove map_base/map_len init and use pci_iomap() instead - simplify to use pci_read_config_word() instead of pci_bus_read_config_word() - run fpga_mgr_put() after usage, so the module can be removed without unbinding the driver - access CVP_STATUS as a 32-bit register, update CVP_STATUS bits macros - check CONFIG_READY bit in write_init and perform a teardown if this bit is set. Move teardown code to separate function as suggested - change function names to altera_v_*() as we can use the driver with other V-series FPGAs. Also change the driver file and Kconfig option names accordingly - pad written firmware data if the file size is not a multiple of 4 - when config error checking is enabled, run checks after a 4KiB chunk is written - remove single built-in firmware string, add module parameter to allow setting default firmware for multiple cards as a Bus:Device.Function specific strings (optional) - remove polling of non-existing data bits DATA_ENC, DATA_COMP. Instead, add module parameter for bitstream specific clock to data ratio setting drivers/fpga/Kconfig | 7 + drivers/fpga/Makefile | 1 + drivers/fpga/altera-cvp.c | 549 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 557 insertions(+) create mode 100644 drivers/fpga/altera-cvp.c