From: Christian Marangi <ansuelsmth@gmail•com>
To: Andrew Lunn <andrew@lunn•ch>
Cc: Robert Marko <robimarko@gmail•com>,
hkallweit1@gmail•com, linux@armlinux•org.uk, davem@davemloft•net,
edumazet@google•com, kuba@kernel•org, pabeni@redhat•com,
netdev@vger•kernel.org, linux-kernel@vger•kernel.org
Subject: Re: [RFC PATCH net-next] net: phy: aquantia: add firmware load support
Date: Mon, 2 Oct 2023 22:22:59 +0200 [thread overview]
Message-ID: <651b26a5.050a0220.213bf.e11b@mx.google.com> (raw)
In-Reply-To: <df89a28e-0886-4db0-9e68-5f9af5bec888@lunn.ch>
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
next prev parent reply other threads:[~2023-10-02 20:23 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-30 10:39 [RFC PATCH net-next] net: phy: aquantia: add firmware load support Robert Marko
2023-10-02 20:18 ` Andrew Lunn
2023-10-02 20:22 ` Christian Marangi [this message]
2023-10-02 21:07 ` Andrew Lunn
2023-10-03 10:21 ` Christian Marangi
2023-10-03 15:20 ` Simon Horman
2023-10-04 23:28 ` Jakub Kicinski
2023-10-05 2:43 ` Andrew Lunn
2023-10-05 14:24 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=651b26a5.050a0220.213bf.e11b@mx.google.com \
--to=ansuelsmth@gmail$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=hkallweit1@gmail$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux@armlinux$(echo .)org.uk \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=robimarko@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox