From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751261AbdEAUGU (ORCPT ); Mon, 1 May 2017 16:06:20 -0400 Received: from mail-qt0-f194.google.com ([209.85.216.194]:35966 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750704AbdEAUGS (ORCPT ); Mon, 1 May 2017 16:06:18 -0400 MIME-Version: 1.0 In-Reply-To: <1493579317-28608-1-git-send-email-agust@denx.de> References: <1493579317-28608-1-git-send-email-agust@denx.de> From: Andy Shevchenko Date: Mon, 1 May 2017 23:06:16 +0300 Message-ID: Subject: Re: [PATCH v4] fpga manager: Add Altera CvP driver To: Anatolij Gustschin Cc: linux-fpga@vger.kernel.org, Alan Tull , Moritz Fischer , matthew.gerlach@linux.intel.com, yi1.li@linux.intel.com, "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Apr 30, 2017 at 10:08 PM, Anatolij Gustschin 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