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.
> This is all hidden by a cast in VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR()
> This seems dangerous to me.
>
>
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > + VEND1_GLOBAL_MAILBOX_INTERFACE4,
> > + VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(addr));
>
> There seem to be similar issues with the use of addr here.
>
> > +
> > + /* We assume and enforce the size to be word aligned.
> > + * If a firmware that is not word aligned is found, please report upstream.
> > + */
> > + for (pos = 0; pos < len; pos += sizeof(u32)) {
> > + u32 word = get_unaligned((const u32 *)(data + 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 mailbox will
> > + * return a BE CRC.
> > + */
> > + word = cpu_to_be32(word);
>
> Similarly here, Sparse flags that a little-endian value is assigned to a
> host byte-order variable.
>
Same here, I'm solving by declaring a new __be32 variable but in
crc_ccitt_false the thing needs to be casted anyway, doesn't that makes
the check useless?
> > + crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
> > + }
>
> ...
>
> pw-bot: changes-requested
--
Ansuel
