Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 4 Sep 2020 22:25:43 +0300 | Subject | Re: [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
| |