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

[Christian Marangi]

On Sat, Nov 11, 2023 at 04:46:42PM +0100, Andrew Lunn wrote:
> 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?
>

The cast is a handy way to cut the other 16bit. We make them 0 anyway by
the FIELD_PREP. So yes I think I can just drop the cast there and put it
in the write_mmd. (it's the same thing just making it more clear)

I'm not including your tag in the next revision as I will make these
changes.

-- 
	Ansuel