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

[Andy Shevchenko]

On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan <vadym.kochan@xxxxxxxxxxx> 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/mrvl/prestera/mvsw_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.

...

> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/pci.h>
> +#include <linux/circ_buf.h>
> +#include <linux/firmware.h>
> +#include <linux/iopoll.h>

Perhaps sorted?

...

> +enum prestera_pci_bar_t {
> +       PRESTERA_PCI_BAR_FW = 2,
> +       PRESTERA_PCI_BAR_PP = 4

Comma?

> +};

...

> +       return readl_poll_timeout(addr, rd_idx,
> +                                CIRC_SPACE(wr_idx, rd_idx, buf_len) >= len,
> +                                1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);

> +       return 0;

dead code.

...

> +       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;

return err;

> +}

...

> +       status = prestera_ldr_read(fw, PRESTERA_LDR_STATUS_REG);

> +       if (status != PRESTERA_LDR_STATUS_START_FW) {

Point of this check?

> +               switch (status) {
> +               case PRESTERA_LDR_STATUS_INVALID_IMG:
> +                       dev_err(fw->dev.dev, "FW img has bad CRC\n");
> +                       return -EINVAL;
> +               case PRESTERA_LDR_STATUS_NOMEM:
> +                       dev_err(fw->dev.dev, "Loader has no enough mem\n");
> +                       return -ENOMEM;
> +               default:
> +                       break;
> +               }
> +       }
> +
> +       return 0;

...

> +       err = prestera_ldr_wait_reg32(fw, PRESTERA_LDR_READY_REG,
> +                                     PRESTERA_LDR_READY_MAGIC, 5 * 1000);

1000? MSEC_PER_SEC?

> +       if (err) {
> +               dev_err(fw->dev.dev, "waiting for FW loader is timed out");
> +               return err;
> +       }

...

> +       if (!IS_ALIGNED(f->size, 4)) {
> +               dev_err(fw->dev.dev, "FW image file is not aligned");

> +               release_firmware(f);
> +               return -EINVAL;

 err = -EINVAL;
 goto out_release;
?

> +       }

Is it really fatal?

> +
> +       err = prestera_fw_hdr_parse(fw, f);
> +       if (err) {
> +               dev_err(fw->dev.dev, "FW image header is invalid\n");

> +               release_firmware(f);
> +               return err;

goto out_release; ?

> +       }
> +
> +       prestera_ldr_write(fw, PRESTERA_LDR_IMG_SIZE_REG, f->size - hlen);
> +       prestera_ldr_write(fw, PRESTERA_LDR_CTL_REG, PRESTERA_LDR_CTL_DL_START);
> +
> +       dev_info(fw->dev.dev, "Loading %s ...", fw_path);
> +
> +       err = prestera_ldr_fw_send(fw, f->data + hlen, f->size - hlen);

out_release: ?

> +       release_firmware(f);
> +       return err;

-- 
With Best Regards,
Andy Shevchenko