lkml.org 
[lkml]   [2020]   [Sep]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH net-next v7 2/6] net: marvell: prestera: Add PCI interface support
On Fri, Sep 4, 2020 at 7:52 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/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

\
 
 \ /
  Last update: 2020-09-04 21:26    [W:0.140 / U:12.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site