Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> +	MV_PMA_BOOT_PRGS_MASK	= 0x0006,
> +	MV_PMA_BOOT_PRGS_INIT	= 0x0000,
> +	MV_PMA_BOOT_PRGS_WAIT	= 0x0002,
> +	MV_PMA_BOOT_PRGS_CSUM	= 0x0004,
> +	MV_PMA_BOOT_PRGS_JRAM	= 0x0006,

You only seem to use PRGS_WAIT, the rest seem unused.

> +struct mv3310_fw_hdr {
> +	struct {
> +		u32 size;
> +		u32 addr;
> +		u16 csum;
> +	} __packed data;

It's probably better to get rid of this embedded struct and just place
the members in the parent struct (although csum woul dneed to be
renamed).

> +
> +	u8 flags;
> +#define MV3310_FW_HDR_DATA_ONLY BIT(6)
> +
> +	u8 port_skip;
> +	u32 next_hdr;
> +	u16 csum;
> +
> +	u8 pad[14];
> +} __packed;
> +
> +static int mv3310_load_fw_sect(struct phy_device *phydev,
> +			       const struct mv3310_fw_hdr *hdr, const u8 *data)
> +{
> +	int err = 0;
> +	size_t i;
> +	u16 csum;
> +
> +	dev_dbg(&phydev->mdio.dev, "Loading %u byte %s section at 0x%08x\n",
> +		hdr->data.size,
> +		(hdr->flags & MV3310_FW_HDR_DATA_ONLY) ? "data" : "executable",
> +		hdr->data.addr);
> +
> +	for (i = 0, csum = 0; i < hdr->data.size; i++)
> +		csum += data[i];
> +
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Corrupt section data\n");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +
> +	/* Any existing checksum is cleared by a read */
> +	__phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_LOW,  hdr->data.addr & 0xffff);
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_HIGH, hdr->data.addr >> 16);
> +
> +	for (i = 0; i < hdr->data.size; i += 2) {
> +		__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_DATA,
> +				(data[i + 1] << 8) | data[i]);
> +	}
> +
> +	csum = __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Download failed\n");
> +		err = -EIO;
> +		goto unlock;
> +	}
> +
> +	if (hdr->flags & MV3310_FW_HDR_DATA_ONLY)
> +		goto unlock;
> +
> +	__phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT, 0, MV_PMA_BOOT_APP_LOADED);
> +	mdelay(200);
> +	if (!(__phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT) & MV_PMA_BOOT_APP_STARTED)) {
> +		dev_err(&phydev->mdio.dev, "Application did not startup\n");
> +		err = -ENODEV;
> +	}

I'm confused why this is done here - after each section in the firmware
file, rather than having loaded all sections in the firmware file and
only then starting the application. Surely if there's multiple sections
that we're going to load, we want to load _all_ sections before starting
the application?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!