Messages in this thread | | | Date | Mon, 2 Oct 2023 22:22:59 +0200 | From | Christian Marangi <> | Subject | Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support |
| |
On Mon, Oct 02, 2023 at 10:18:05PM +0200, Andrew Lunn wrote: > > +/* load data into the phy's memory */ > > +static int aquantia_load_memory(struct phy_device *phydev, u32 addr, > > + const u8 *data, size_t len) > > +{ > > + u16 crc = 0, up_crc; > > + size_t pos; > > + > > + /* PHY expect addr in LE */ > > + addr = cpu_to_le32(addr); > > + > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_MAILBOX_INTERFACE1, > > + VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET); > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_MAILBOX_INTERFACE3, > > + VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(addr)); > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_MAILBOX_INTERFACE4, > > + VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(addr)); > > + > > + for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) { > > + u32 word = 0; > > + > > + memcpy(&word, data + pos, min(sizeof(u32), len - pos)); > > + > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5, > > + VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word)); > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE6, > > + VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(word)); > > + > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE1, > > + VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE | > > + VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE); > > + > > + /* calculate CRC as we load data to the mailbox. > > + * We convert word to big-endiang as PHY is BE and ailbox will > > + * return a BE crc. > > _m_ailbox. > > And i would consistently uses CRC in comments. > > > +static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size) > > +{ > > + const struct aqr_fw_header *header; > > + u32 iram_offset = 0, iram_size = 0; > > + u32 dram_offset = 0, dram_size = 0; > > + char version[VERSION_STRING_SIZE]; > > + u16 calculated_crc, read_crc; > > + u32 primary_offset = 0; > > + int ret; > > + > > + /* extract saved crc at the end of the fw */ > > + memcpy(&read_crc, data + size - 2, sizeof(read_crc)); > > + /* crc is saved in big-endian as PHY is BE */ > > + read_crc = be16_to_cpu(read_crc); > > + calculated_crc = crc_ccitt_false(0, data, size - 2); > > + if (read_crc != calculated_crc) { > > + phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n", > > + read_crc, calculated_crc); > > + return -EINVAL; > > + } > > + > > + /* Get the primary offset to extract DRAM and IRAM sections. */ > > + memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16)); > > Please add some sanity checks. We should not fully trust the > firmware. Is PRIMARY_OFFSET_OFFSET + sizeof(u16) actually inside the > firmware blob? > > > + primary_offset = PRIMARY_OFFSET(le32_to_cpu(primary_offset)); > > + > > + /* Find the DRAM and IRAM sections within the firmware file. */ > > + header = (struct aqr_fw_header *)(data + primary_offset + HEADER_OFFSET); > > Is header actually inside the firmware blob? > > > + memcpy(&iram_offset, &header->iram_offset, sizeof(u8) * 3); > > + memcpy(&iram_size, &header->iram_size, sizeof(u8) * 3); > > + memcpy(&dram_offset, &header->dram_offset, sizeof(u8) * 3); > > + memcpy(&dram_size, &header->dram_size, sizeof(u8) * 3); > > + > > + /* offset are in LE and values needs to be converted to cpu endian */ > > + iram_offset = le32_to_cpu(iram_offset); > > + iram_size = le32_to_cpu(iram_size); > > + dram_offset = le32_to_cpu(dram_offset); > > + dram_size = le32_to_cpu(dram_size); > > + > > + /* Increment the offset with the primary offset. */ > > + iram_offset += primary_offset; > > + dram_offset += primary_offset; > > + > > + phydev_dbg(phydev, "primary %d IRAM offset=%d size=%d DRAM offset=%d size=%d\n", > > + primary_offset, iram_offset, iram_size, dram_offset, dram_size); > > + > > + strscpy(version, (char *)data + dram_offset + VERSION_STRING_OFFSET, > > + VERSION_STRING_SIZE); > > Is version inside the blob.... > > > +static int aqr_firmware_load_nvmem(struct phy_device *phydev) > > +{ > > + struct nvmem_cell *cell; > > + size_t size; > > + u8 *buf; > > + int ret; > > + > > + cell = nvmem_cell_get(&phydev->mdio.dev, "firmware"); > > Does this need properties in device tree? Please update the binding. >
This is problematic... Since this is a plain standard PHY and we don't have a compatible (as it's matched with the PHY id) we don't have DT to add this... Sooo how to add this? Should we update the generic-phy dt?
Should we create a dummy dt and add a compatible adding ethernet-phy.ID... just for this properties?
This is why we were a bit confused about adding a DT commit to this.
> > + > > +static int aqr_firmware_load_sysfs(struct phy_device *phydev) > > _sysfs seems a bit odd here. Does request_firmware still use the user > mode helper? I _thought_ it just went direct to the filesystem? > > > +{ > > + struct device *dev = &phydev->mdio.dev; > > + const struct firmware *fw; > > + const char *fw_name; > > + int ret; > > + > > + ret = of_property_read_string(dev->of_node, "firmware-name", > > + &fw_name); > > Please update the device tree binding. > > > +static int aqr_firmware_load(struct phy_device *phydev) > > +{ > > + int ret; > > + > > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID); > > + if (ret > 0) > > + goto exit; > > I assume this means a value of 0 indicates there is no firmware > running? Maybe a comment or a #define for 0? > > Andrew
-- Ansuel
| |