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

[Christian Marangi <ansuelsmth@gmail.com>]

On Sat, Nov 11, 2023 at 07:16:55PM +0100, Christian Marangi wrote:
> 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@gmail.com>
> > > > > 
> > > > > 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@gmail.com>
> > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > 
> > > > 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.
>

Actually the cast in the define are needed for FIELD_PREP or build time
compilation error is triggered for addr being too big for the mask.

So the golden question is... Is it really a problem having the (u16)
cast in the header? 

-- 
	Ansuel