On Mon, Jul 27, 2020 at 3:23 PM Vadym Kochan <vadym.koc...@plvision.eu> wrote:
>
> Add PCI interface driver for Prestera Switch ASICs family devices, which
> provides:
>
> - Firmware loading mechanism
> - Requests & events handling to/from the firmware
> - Access to the firmware on the bus level
>
> The firmware has to be loaded each time the device is reset. The driver
> is loading it from:
>
> /lib/firmware/marvell/prestera_fw-v{MAJOR}.{MINOR}.img
>
> The full firmware image version is located within the internal header
> and consists of 3 numbers - MAJOR.MINOR.PATCH. Additionally, driver has
> hard-coded minimum supported firmware version which it can work with:
>
> MAJOR - reflects the support on ABI level between driver and loaded
> firmware, this number should be the same for driver and loaded
> firmware.
>
> MINOR - this is the minimum supported version between driver and the
> firmware.
>
> PATCH - indicates only fixes, firmware ABI is not changed.
>
> Firmware image file name contains only MAJOR and MINOR numbers to make
> driver be compatible with any PATCH version.
I have to admit that memcpy_toio() / memcpy_fromio() may not be good
for this driver.
Please, consider two things:
- they are native endianess (it's good if it's your case)
- they are behaving interestingly when buffer is not aligned
Sorry I didn't think well about this.
...
> +struct prestera_ldr_regs {
> + u32 ldr_ready;
> + u32 pad1;
> +
> + u32 ldr_img_size;
> + u32 ldr_ctl_flags;
> +
> + u32 ldr_buf_offs;
> + u32 ldr_buf_size;
> +
> + u32 ldr_buf_rd;
> + u32 pad2;
> + u32 ldr_buf_wr;
> +
> + u32 ldr_status;
> +} __packed __aligned(4);
Do these attributes change the struct anyhow?
...
> +static int prestera_fw_wait_reg32(struct prestera_fw *fw, u32 reg, u32 cmp,
> + unsigned int waitms)
> +{
> + u8 __iomem *addr = PRESTERA_FW_REG_ADDR(fw, reg);
> + u32 val;
> +
> + return readl_poll_timeout(addr, val, cmp == val, 1000 * 10, waitms *
> 1000);
Hmm... If waitms (better to spell it fully like wait_timeout or so)
less than 10?
And all those magic numbers in each of readl_poll_timeout() calls.
Also consider to use predefined constants like NSEC_PER_SEC.
> +}
...
> + err = readl_poll_timeout(addr, val, val & mask, 1000 * 10, waitus);
> + if (err) {
> + dev_err(fw->dev.dev, "Timeout to load FW img [state=%d]",
> + prestera_ldr_read(fw, PRESTERA_LDR_STATUS_REG));
> + return err;
> + }
> +
> + return 0;
if (err)
dev_err();
return err;
...
> +static int prestera_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + const char *driver_name = pdev->driver->name;
> + struct prestera_fw *fw;
> + int err;
> +
> + err = pcim_enable_device(pdev);
> + if (err)
> + return err;
> +
> + err = pcim_iomap_regions(pdev, BIT(PRESTERA_PCI_BAR_FW) |
> + BIT(PRESTERA_PCI_BAR_PP),
> + pci_name(pdev));
> + if (err)
> + return err;
> +
> + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(30))) {
> + pci_err(pdev, "fail to set DMA mask\n");
pci_err() is more for PCI core and Co. I think dev_err() and other
dev_*() are good enough here.
> + goto err_dma_mask;
> + }
> +
> + pci_set_master(pdev);
> +
> + fw = devm_kzalloc(&pdev->dev, sizeof(*fw), GFP_KERNEL);
> + if (!fw) {
> + err = -ENOMEM;
> + goto err_pci_dev_alloc;
> + }
> +
> + fw->dev.ctl_regs = pcim_iomap_table(pdev)[PRESTERA_PCI_BAR_FW];
> + fw->dev.pp_regs = pcim_iomap_table(pdev)[PRESTERA_PCI_BAR_PP];
> + fw->dev.dev = &pdev->dev;
> + fw->pci_dev = pdev;
Seems like the second one is redundant. You may always derive struct
pci_dev from struct dev if needed.
> + pci_set_drvdata(pdev, fw);
> +
> + err = prestera_fw_init(fw);
> + if (err)
> + goto err_prestera_fw_init;
> +
> + dev_info(fw->dev.dev, "Switch FW is ready\n");
> +
> + fw->wq = alloc_workqueue("prestera_fw_wq", WQ_HIGHPRI, 1);
> + if (!fw->wq)
> + goto err_wq_alloc;
> +
> + INIT_WORK(&fw->evt_work, prestera_fw_evt_work_fn);
> +
> + err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> + if (err < 0) {
> + pci_err(pdev, "MSI IRQ init failed\n");
> + goto err_irq_alloc;
> + }
> +
> + err = request_irq(pci_irq_vector(pdev, 0), prestera_pci_irq_handler,
> + 0, driver_name, fw);
> + if (err) {
> + pci_err(pdev, "fail to request IRQ\n");
> + goto err_request_irq;
> + }
> +
> + err = prestera_device_register(&fw->dev);
> + if (err)
> + goto err_prestera_dev_register;
> +
> + return 0;
> +
> +err_prestera_dev_register:
> + free_irq(pci_irq_vector(pdev, 0), fw);
> +err_request_irq:
> + pci_free_irq_vectors(pdev);
> +err_irq_alloc:
> + destroy_workqueue(fw->wq);
> +err_wq_alloc:
> + prestera_fw_uninit(fw);
> +err_prestera_fw_init:
> +err_pci_dev_alloc:
> +err_dma_mask:
All three are useless.
> + return err;
> +}
...
> +static struct pci_driver prestera_pci_driver = {
> + .name = "Prestera DX",
> + .id_table = prestera_pci_devices,
> + .probe = prestera_pci_probe,
> + .remove = prestera_pci_remove,
> +};
> +
Redundant blank line.
> +module_pci_driver(prestera_pci_driver);
--
With Best Regards,
Andy Shevchenko
