On Fri, Sep 4, 2020 at 7:52 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/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
