Re: [net-next v4 2/6] net: marvell: prestera: Add PCI interface support

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Jul 27, 2020 at 3:23 PM Vadym Kochan <vadym.kochan@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