Re: [net-next RFC PATCH v6 3/4] net: phy: aquantia: add firmware load support

[Andrew Lunn]

On Fri, Nov 10, 2023 at 11:28:36PM +0100, Christian Marangi wrote:
> On Fri, Nov 10, 2023 at 07:57:02PM +0000, Simon Horman wrote:
> > On Thu, Nov 09, 2023 at 01:32:52PM +0100, Christian Marangi wrote:
> > > From: Robert Marko <robimarko@xxxxxxxxx>
> > > 
> > > Aquantia PHY-s require firmware to be loaded before they start operating.
> > > It can be automatically loaded in case when there is a SPI-NOR connected
> > > to Aquantia PHY-s or can be loaded from the host via MDIO.
> > > 
> > > This patch adds support for loading the firmware via MDIO as in most cases
> > > there is no SPI-NOR being used to save on cost.
> > > Firmware loading code itself is ported from mainline U-boot with cleanups.
> > > 
> > > The firmware has mixed values both in big and little endian.
> > > PHY core itself is big-endian but it expects values to be in little-endian.
> > > The firmware is little-endian but CRC-16 value for it is stored at the end
> > > of firmware in big-endian.
> > > 
> > > It seems the PHY does the conversion internally from firmware that is
> > > little-endian to the PHY that is big-endian on using the mailbox
> > > but mailbox returns a big-endian CRC-16 to verify the written data
> > > integrity.
> > > 
> > > Co-developed-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > > Signed-off-by: Robert Marko <robimarko@xxxxxxxxx>
> > > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > 
> > Hi Christian and Robert,
> > 
> > thanks for your patch-set.
> > 
> > I spotted some minor endien issues which I have highlighted below.
> > 
> > ...
> >
> 
> Hi Simon,
> 
> thanks for the check!
> 
> > > +/* load data into the phy's memory */
> > > +static int aqr_fw_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);
> > 
> > The type of addr is host byte-order,
> > but here it is assigned a little-endian value.
> > 
> > Flagged by Sparse.
> > 
> > > +
> > > +	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));
> > 
> > VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR() performs a bit-shift on addr,
> > and applies a mask which is in host-byte order.
> > But, as highlighted above, addr is a little-endian value.
> > This does not seem right.
> >
> 
> It's really just some magic to split the addr and swap if we are not
> in little-endian. The passed addr are defined here in the code and are
> hardcoded, they doesn't come from the firmware. What I can do is just
> recast __le32 to u32 again with __force to mute the warning...
> 
> Resulting in this snippet:
> 
> 	__le32 addr;
> 	size_t pos;
> 
> 	/* PHY expect addr in LE */
> 	addr = cpu_to_le32(load_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((__force u32)addr));
> 	phy_write_mmd(phydev, MDIO_MMD_VEND1,
> 		      VEND1_GLOBAL_MAILBOX_INTERFACE4,
> 		      VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR((__force u32)addr));
> 
> Also things needs to be casted to u16 anyway as phy_write_mmd expect a
> u16. And as you said FILED_PREP will use int (from the define) so I
> wonder if a more clean way would be just addr = (__force u32)cpu_to_le32(load_addr)
> resulting in a simple bswap32 if we are in big-endian.
> 
> Would love some feedback about this.

I don't think sparse is giving much value here. As you say,
phy_write_mmd() expects a u16, host endian. The endianness of the bus
is well defined in 802.3, and we expect the MDIO bus driver to take
care of converting host endian to whatever is needed by the
hardware. And typically, that is nothing since it is all integrated.

There does not appear to be a cpu_to_le32() without sparse markup. So
i think you are forced to use the ugly __force. I would do that as
soon as possible, as part of the cpu_to_le32() line.

> > This is all hidden by a cast in VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR()
> > This seems dangerous to me.

That cast could be made more visible. The macro itself looks safe on
different endians. It uses > and & operations. So try taking the cast
out of the macro and make it part of the phy_write_mmd() call? I
assume the cast is needed because you get a compiler warning, passing
a u32 when a u16 is expected?

	Andrew