Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support

From: Christian Marangi
Date: Mon Oct 02 2023 - 16:23:08 EST


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