Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <651b26a5.050a0220.213bf.e11b@mx.google.com>
Date:   Mon, 2 Oct 2023 22:22:59 +0200
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
References: <20230930104008.234831-1-robimarko@gmail.com>
 <df89a28e-0886-4db0-9e68-5f9af5bec888@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <df89a28e-0886-4db0-9e68-5f9af5bec888@lunn.ch>
Precedence: bulk

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