| Message ID | 20231101123608.11157-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/2] net: phy: aquantia: add firmware load support | expand |
On Wed, Nov 01, 2023 at 02:01:33PM +0100, Heiner Kallweit wrote: > On 01.11.2023 13:36, 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> > > --- > > Changes v2: > > - Move out of RFC > > - Address sanity check for offsets > > - Add additional comments on firmware load check > > - Fix some typo > > - Capitalize CRC in comments > > - Rename load_sysfs to load_fs > > > > To make the driver better maintainable: can the firmware handling code > be placed in a separate source code file, similar to what has been done > for the hwmon part? > If yes, then this could also be the right time to move the aquantia > driver to an own subdirectory. > Sure! Np for me just is it really worth it? hwmod is a bigger one but this is really a few functions. Anyway if requested, I will move in v3 the driver to a dedicated directory and move the function to a separate file!
On 01.11.2023 13:36, 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> > --- > Changes v2: > - Move out of RFC > - Address sanity check for offsets > - Add additional comments on firmware load check > - Fix some typo > - Capitalize CRC in comments > - Rename load_sysfs to load_fs > To make the driver better maintainable: can the firmware handling code be placed in a separate source code file, similar to what has been done for the hwmon part? If yes, then this could also be the right time to move the aquantia driver to an own subdirectory.
On Wed, Nov 01, 2023 at 01:36:07PM +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> > --- > Changes v2: > - Move out of RFC Actually, since we are in the merge window, RFC would be correct. > - Address sanity check for offsets > - Add additional comments on firmware load check > - Fix some typo > - Capitalize CRC in comments > - Rename load_sysfs to load_fs > > drivers/net/phy/Kconfig | 1 + > drivers/net/phy/aquantia_main.c | 304 ++++++++++++++++++++++++++++++++ > 2 files changed, 305 insertions(+) > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 421d2b62918f..46c7194efcea 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -98,6 +98,7 @@ config ADIN1100_PHY > > config AQUANTIA_PHY > tristate "Aquantia PHYs" > + select CRC_CCITT > help > Currently supports the Aquantia AQ1202, AQ2104, AQR105, AQR405 > > diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c > index 334a6904ca5a..0f1b8d75cca0 100644 > --- a/drivers/net/phy/aquantia_main.c > +++ b/drivers/net/phy/aquantia_main.c > @@ -12,6 +12,10 @@ > #include <linux/delay.h> > #include <linux/bitfield.h> > #include <linux/phy.h> > +#include <linux/of.h> > +#include <linux/firmware.h> > +#include <linux/crc-ccitt.h> > +#include <linux/nvmem-consumer.h> > > #include "aquantia.h" > > @@ -92,10 +96,40 @@ > #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES 0xd31b > > /* Vendor specific 1, MDIO_MMD_VEND1 */ > +#define VEND1_GLOBAL_SC 0x0 > +#define VEND1_GLOBAL_SC_SOFT_RESET BIT(15) > +#define VEND1_GLOBAL_SC_LOW_POWER BIT(11) > + > #define VEND1_GLOBAL_FW_ID 0x0020 > #define VEND1_GLOBAL_FW_ID_MAJOR GENMASK(15, 8) > #define VEND1_GLOBAL_FW_ID_MINOR GENMASK(7, 0) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1 0x0200 > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE BIT(15) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE BIT(14) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET BIT(12) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_BUSY BIT(8) > + > +#define VEND1_GLOBAL_MAILBOX_INTERFACE2 0x0201 > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3 0x0202 > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK GENMASK(15, 0) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK, (u16)((x) >> 16)) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4 0x0203 > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK GENMASK(15, 2) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK, (u16)(x)) > + > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5 0x0204 > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK GENMASK(15, 0) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK, (u16)((x) >> 16)) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6 0x0205 > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK GENMASK(15, 0) > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK, (u16)(x)) > + > +#define VEND1_GLOBAL_CONTROL2 0xc001 > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST BIT(15) > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD BIT(6) > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL BIT(0) > + > #define VEND1_GLOBAL_GEN_STAT2 0xc831 > #define VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG BIT(15) > > @@ -152,6 +186,30 @@ > #define AQR107_OP_IN_PROG_SLEEP 1000 > #define AQR107_OP_IN_PROG_TIMEOUT 100000 > > +#define UP_RESET_SLEEP 100 > + > +/* addresses of memory segments in the phy */ > +#define DRAM_BASE_ADDR 0x3FFE0000 > +#define IRAM_BASE_ADDR 0x40000000 > + > +/* firmware image format constants */ > +#define VERSION_STRING_SIZE 0x40 > +#define VERSION_STRING_OFFSET 0x0200 > +/* primary offset is written at an offset from the start of the fw blob */ > +#define PRIMARY_OFFSET_OFFSET 0x8 > +/* primary offset needs to be then added to a base offset */ > +#define PRIMARY_OFFSET_SHIFT 12 > +#define PRIMARY_OFFSET(x) ((x) << PRIMARY_OFFSET_SHIFT) > +#define HEADER_OFFSET 0x300 > + > +struct aqr_fw_header { > + u32 padding; > + u8 iram_offset[3]; > + u8 iram_size[3]; > + u8 dram_offset[3]; > + u8 dram_size[3]; > +} __packed; > + > struct aqr107_hw_stat { > const char *name; > int reg; > @@ -677,6 +735,166 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev) > return 0; > } > > +/* load data into the phy's memory */ > +static int aquantia_load_memory(struct phy_device *phydev, u32 addr, > + const u8 *data, size_t len) > +{ > + for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) { > + u32 word = 0; > + > + memcpy(&word, data + pos, min(sizeof(u32), len - pos)); Rather than do a memcpy, use the get_unaligned_ macros. They might map to a memcpy(), but some architectures can do unaligned accesses without problems. > +static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size) > +{ > + const struct aqr_fw_header *header; > + u32 iram_offset = 0, iram_size = 0; > + u32 dram_offset = 0, dram_size = 0; > + char version[VERSION_STRING_SIZE]; > + u16 calculated_crc, read_crc; > + u32 primary_offset = 0; > + int ret; > + > + /* extract saved CRC at the end of the fw */ > + memcpy(&read_crc, data + size - 2, sizeof(read_crc)); Say size == 1. You just had a buffer underrun. > + /* CRC is saved in big-endian as PHY is BE */ > + read_crc = be16_to_cpu(read_crc); > + calculated_crc = crc_ccitt_false(0, data, size - 2); > + if (read_crc != calculated_crc) { > + phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n", > + read_crc, calculated_crc); > + return -EINVAL; > + } > + > + /* Get the primary offset to extract DRAM and IRAM sections. */ > + memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16)); What if PRIMARY_OFFSET_OFFSET + sizeof(u16) is greater than size? A buffer overrun. Assume the firmware is evil and is trying to hack you. Always test everything. I would suggest some helpers, something like int aqr_fw_get_u16(const u8 *data, size_t size, size_t offset, u16 *value) Check that offset + sizeof(u16) is within the firmware, and if not return -EINVAL. Otherwise set *value to the u16 from the firmware and return 0. This is where Rust would be nice :-) Andrew --- pw-bot: cr
On Wed, Nov 01, 2023 at 02:13:05PM +0100, Andrew Lunn wrote: > On Wed, Nov 01, 2023 at 01:36:07PM +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> > > --- > > Changes v2: > > - Move out of RFC > > Actually, since we are in the merge window, RFC would be correct. > My bad! > > - Address sanity check for offsets > > - Add additional comments on firmware load check > > - Fix some typo > > - Capitalize CRC in comments > > - Rename load_sysfs to load_fs > > > > drivers/net/phy/Kconfig | 1 + > > drivers/net/phy/aquantia_main.c | 304 ++++++++++++++++++++++++++++++++ > > 2 files changed, 305 insertions(+) > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > index 421d2b62918f..46c7194efcea 100644 > > --- a/drivers/net/phy/Kconfig > > +++ b/drivers/net/phy/Kconfig > > @@ -98,6 +98,7 @@ config ADIN1100_PHY > > > > config AQUANTIA_PHY > > tristate "Aquantia PHYs" > > + select CRC_CCITT > > help > > Currently supports the Aquantia AQ1202, AQ2104, AQR105, AQR405 > > > > diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c > > index 334a6904ca5a..0f1b8d75cca0 100644 > > --- a/drivers/net/phy/aquantia_main.c > > +++ b/drivers/net/phy/aquantia_main.c > > @@ -12,6 +12,10 @@ > > #include <linux/delay.h> > > #include <linux/bitfield.h> > > #include <linux/phy.h> > > +#include <linux/of.h> > > +#include <linux/firmware.h> > > +#include <linux/crc-ccitt.h> > > +#include <linux/nvmem-consumer.h> > > > > #include "aquantia.h" > > > > @@ -92,10 +96,40 @@ > > #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES 0xd31b > > > > /* Vendor specific 1, MDIO_MMD_VEND1 */ > > +#define VEND1_GLOBAL_SC 0x0 > > +#define VEND1_GLOBAL_SC_SOFT_RESET BIT(15) > > +#define VEND1_GLOBAL_SC_LOW_POWER BIT(11) > > + > > #define VEND1_GLOBAL_FW_ID 0x0020 > > #define VEND1_GLOBAL_FW_ID_MAJOR GENMASK(15, 8) > > #define VEND1_GLOBAL_FW_ID_MINOR GENMASK(7, 0) > > > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1 0x0200 > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE BIT(15) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE BIT(14) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET BIT(12) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_BUSY BIT(8) > > + > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE2 0x0201 > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3 0x0202 > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK GENMASK(15, 0) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK, (u16)((x) >> 16)) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4 0x0203 > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK GENMASK(15, 2) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK, (u16)(x)) > > + > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5 0x0204 > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK GENMASK(15, 0) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK, (u16)((x) >> 16)) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6 0x0205 > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK GENMASK(15, 0) > > +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK, (u16)(x)) > > + > > +#define VEND1_GLOBAL_CONTROL2 0xc001 > > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST BIT(15) > > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD BIT(6) > > +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL BIT(0) > > + > > #define VEND1_GLOBAL_GEN_STAT2 0xc831 > > #define VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG BIT(15) > > > > @@ -152,6 +186,30 @@ > > #define AQR107_OP_IN_PROG_SLEEP 1000 > > #define AQR107_OP_IN_PROG_TIMEOUT 100000 > > > > +#define UP_RESET_SLEEP 100 > > + > > +/* addresses of memory segments in the phy */ > > +#define DRAM_BASE_ADDR 0x3FFE0000 > > +#define IRAM_BASE_ADDR 0x40000000 > > + > > +/* firmware image format constants */ > > +#define VERSION_STRING_SIZE 0x40 > > +#define VERSION_STRING_OFFSET 0x0200 > > +/* primary offset is written at an offset from the start of the fw blob */ > > +#define PRIMARY_OFFSET_OFFSET 0x8 > > +/* primary offset needs to be then added to a base offset */ > > +#define PRIMARY_OFFSET_SHIFT 12 > > +#define PRIMARY_OFFSET(x) ((x) << PRIMARY_OFFSET_SHIFT) > > +#define HEADER_OFFSET 0x300 > > + > > +struct aqr_fw_header { > > + u32 padding; > > + u8 iram_offset[3]; > > + u8 iram_size[3]; > > + u8 dram_offset[3]; > > + u8 dram_size[3]; > > +} __packed; > > + > > struct aqr107_hw_stat { > > const char *name; > > int reg; > > @@ -677,6 +735,166 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev) > > return 0; > > } > > > > +/* load data into the phy's memory */ > > +static int aquantia_load_memory(struct phy_device *phydev, u32 addr, > > + const u8 *data, size_t len) > > +{ > > > + for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) { > > + u32 word = 0; > > + > > + memcpy(&word, data + pos, min(sizeof(u32), len - pos)); > > Rather than do a memcpy, use the get_unaligned_ macros. They might map > to a memcpy(), but some architectures can do unaligned accesses > without problems. > I don't think this is doable for this loop, think we would end up in some funny situation where for the last run we have to copy less than u32. (get_unaligned would always take u32 of data and that would end up reading more than requested) Am I wrong? Aside from this, in the other part of the code I can use the macro and skip having to convert them. > > +static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size) > > +{ > > + const struct aqr_fw_header *header; > > + u32 iram_offset = 0, iram_size = 0; > > + u32 dram_offset = 0, dram_size = 0; > > + char version[VERSION_STRING_SIZE]; > > + u16 calculated_crc, read_crc; > > + u32 primary_offset = 0; > > + int ret; > > + > > + /* extract saved CRC at the end of the fw */ > > + memcpy(&read_crc, data + size - 2, sizeof(read_crc)); > > Say size == 1. You just had a buffer underrun. > > > + /* CRC is saved in big-endian as PHY is BE */ > > + read_crc = be16_to_cpu(read_crc); > > + calculated_crc = crc_ccitt_false(0, data, size - 2); > > + if (read_crc != calculated_crc) { > > + phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n", > > + read_crc, calculated_crc); > > + return -EINVAL; > > + } > > + > > + /* Get the primary offset to extract DRAM and IRAM sections. */ > > + memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16)); > > What if PRIMARY_OFFSET_OFFSET + sizeof(u16) is greater than size? A > buffer overrun. > > Assume the firmware is evil and is trying to hack you. Always test > everything. > > I would suggest some helpers, something like > > int aqr_fw_get_u16(const u8 *data, size_t size, size_t offset, u16 *value) > > Check that offset + sizeof(u16) is within the firmware, and if not return -EINVAL. > Otherwise set *value to the u16 from the firmware and return 0. > > This is where Rust would be nice :-) > > Andrew > > --- > pw-bot: cr
> > > + for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) { > > > + u32 word = 0; > > > + > > > + memcpy(&word, data + pos, min(sizeof(u32), len - pos)); > > > > Rather than do a memcpy, use the get_unaligned_ macros. They might map > > to a memcpy(), but some architectures can do unaligned accesses > > without problems. > > > > I don't think this is doable for this loop, think we would end up in > some funny situation where for the last run we have to copy less than > u32. (get_unaligned would always take u32 of data and that would end up > reading more than requested) Am I wrong? Does it happen in practice that the last chunk is not 4 bytes? Since this is firmware, its probably produced by some sort of linker, and they often round segments to words. Could you take a look at the firmware images you have access to and see if this is true? It could be we do need to keep with the memcpy, but it would be nice if we could limit it to words, at least until somebody has a firmware which is not word aligned. Andrew
On Wed, Nov 01, 2023 at 05:32:29PM +0100, Andrew Lunn wrote: > > > > + for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) { > > > > + u32 word = 0; > > > > + > > > > + memcpy(&word, data + pos, min(sizeof(u32), len - pos)); > > > > > > Rather than do a memcpy, use the get_unaligned_ macros. They might map > > > to a memcpy(), but some architectures can do unaligned accesses > > > without problems. > > > > > > > I don't think this is doable for this loop, think we would end up in > > some funny situation where for the last run we have to copy less than > > u32. (get_unaligned would always take u32 of data and that would end up > > reading more than requested) Am I wrong? > > Does it happen in practice that the last chunk is not 4 bytes? Since > this is firmware, its probably produced by some sort of linker, and > they often round segments to words. Could you take a look at the > firmware images you have access to and see if this is true? > > It could be we do need to keep with the memcpy, but it would be nice > if we could limit it to words, at least until somebody has a firmware > which is not word aligned. > There are plenty of firmware around so it can be checked by from what I have, it looks like they are word aligned... Ok I will use the get_unaligned and add a comment saying that we assume the iram and dram section are always word aligned. Is it ok?
> There are plenty of firmware around so it can be checked by from what I > have, it looks like they are word aligned... Ok I will use the > get_unaligned and add a comment saying that we assume the iram and dram > section are always word aligned. We probably want to know if there is firmware out there which is not word aligned. So i would probably do phydev_err() and return -EINVAL. Andrew
On 01.11.2023 13:57, Christian Marangi wrote: > On Wed, Nov 01, 2023 at 02:01:33PM +0100, Heiner Kallweit wrote: >> On 01.11.2023 13:36, 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> >>> --- >>> Changes v2: >>> - Move out of RFC >>> - Address sanity check for offsets >>> - Add additional comments on firmware load check >>> - Fix some typo >>> - Capitalize CRC in comments >>> - Rename load_sysfs to load_fs >>> >> >> To make the driver better maintainable: can the firmware handling code >> be placed in a separate source code file, similar to what has been done >> for the hwmon part? >> If yes, then this could also be the right time to move the aquantia >> driver to an own subdirectory. >> > > Sure! Np for me just is it really worth it? hwmod is a bigger one but > this is really a few functions. > r8169_firmware.c is even smaller and I've never regretted having it factored out. Whether it makes sense depends on how much you share with the main module and how the API is structured that you provide to the main module. So I don't say you have to do it, I'm just saying it's worth considering it. > Anyway if requested, I will move in v3 the driver to a dedicated > directory and move the function to a separate file! >
On Wed, Nov 01, 2023 at 05:54:50PM +0100, Andrew Lunn wrote: > > There are plenty of firmware around so it can be checked by from what I > > have, it looks like they are word aligned... Ok I will use the > > get_unaligned and add a comment saying that we assume the iram and dram > > section are always word aligned. > > We probably want to know if there is firmware out there which is not > word aligned. So i would probably do phydev_err() and return -EINVAL. > Do we have API to check this? Or I think I should just check the iram and dram size and see if iram_size % sizeof(u32) is zero and return error otherwise.
On Wed, Nov 01, 2023 at 05:57:50PM +0100, Heiner Kallweit wrote: > On 01.11.2023 13:57, Christian Marangi wrote: > > On Wed, Nov 01, 2023 at 02:01:33PM +0100, Heiner Kallweit wrote: > >> On 01.11.2023 13:36, 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> > >>> --- > >>> Changes v2: > >>> - Move out of RFC > >>> - Address sanity check for offsets > >>> - Add additional comments on firmware load check > >>> - Fix some typo > >>> - Capitalize CRC in comments > >>> - Rename load_sysfs to load_fs > >>> > >> > >> To make the driver better maintainable: can the firmware handling code > >> be placed in a separate source code file, similar to what has been done > >> for the hwmon part? > >> If yes, then this could also be the right time to move the aquantia > >> driver to an own subdirectory. > >> > > > > Sure! Np for me just is it really worth it? hwmod is a bigger one but > > this is really a few functions. > > > r8169_firmware.c is even smaller and I've never regretted having it factored > out. Whether it makes sense depends on how much you share with the main module > and how the API is structured that you provide to the main module. > So I don't say you have to do it, I'm just saying it's worth considering it. > Already done! Will be part of this series with v3 :D > > Anyway if requested, I will move in v3 the driver to a dedicated > > directory and move the function to a separate file! > > >
> Do we have API to check this? Or I think I should just check the iram > and dram size and see if iram_size % sizeof(u32) is zero and return > error otherwise. Yes, that sounds correct. Andrew
Hi Christian, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/dt-bindings-Document-bindings-for-Marvell-Aquantia-PHY/20231101-203944 base: net-next/main patch link: https://lore.kernel.org/r/20231101123608.11157-1-ansuelsmth%40gmail.com patch subject: [net-next PATCH v2 1/2] net: phy: aquantia: add firmware load support config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20231103/202311030347.asaThH7R-lkp@intel.com/config) compiler: arceb-elf-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/202311030347.asaThH7R-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311030347.asaThH7R-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/net/phy/aquantia_main.c: In function 'aqr_fw_boot': >> drivers/net/phy/aquantia_main.c:857:13: warning: the address of 'version' will always evaluate as 'true' [-Waddress] 857 | if (!version) { | ^ during RTL pass: mach drivers/net/phy/aquantia_main.c: In function 'aqr107_chip_info': drivers/net/phy/aquantia_main.c:619:1: internal compiler error: in arc_ifcvt, at config/arc/arc.cc:9703 619 | } | ^ 0x5b78c1 arc_ifcvt /tmp/build-crosstools-gcc-13.2.0-binutils-2.41/gcc/gcc-13.2.0/gcc/config/arc/arc.cc:9703 0xe431b4 arc_reorg /tmp/build-crosstools-gcc-13.2.0-binutils-2.41/gcc/gcc-13.2.0/gcc/config/arc/arc.cc:8552 0xaed299 execute /tmp/build-crosstools-gcc-13.2.0-binutils-2.41/gcc/gcc-13.2.0/gcc/reorg.cc:3927 Please submit a full bug report, with preprocessed source (by using -freport-bug). Please include the complete backtrace with any bug report. See <https://gcc.gnu.org/bugs/> for instructions. vim +857 drivers/net/phy/aquantia_main.c 789 790 static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size) 791 { 792 const struct aqr_fw_header *header; 793 u32 iram_offset = 0, iram_size = 0; 794 u32 dram_offset = 0, dram_size = 0; 795 char version[VERSION_STRING_SIZE]; 796 u16 calculated_crc, read_crc; 797 u32 primary_offset = 0; 798 int ret; 799 800 /* extract saved CRC at the end of the fw */ 801 memcpy(&read_crc, data + size - 2, sizeof(read_crc)); 802 /* CRC is saved in big-endian as PHY is BE */ 803 read_crc = be16_to_cpu(read_crc); 804 calculated_crc = crc_ccitt_false(0, data, size - 2); 805 if (read_crc != calculated_crc) { 806 phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n", 807 read_crc, calculated_crc); 808 return -EINVAL; 809 } 810 811 /* Get the primary offset to extract DRAM and IRAM sections. */ 812 memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16)); 813 if (!primary_offset) { 814 phydev_err(phydev, "bad primary offset in firmware\n"); 815 return -EINVAL; 816 } 817 primary_offset = PRIMARY_OFFSET(le32_to_cpu(primary_offset)); 818 819 /* Find the DRAM and IRAM sections within the firmware file. */ 820 header = (struct aqr_fw_header *)(data + primary_offset + HEADER_OFFSET); 821 memcpy(&iram_offset, &header->iram_offset, sizeof(u8) * 3); 822 if (!iram_offset) { 823 phydev_err(phydev, "bad iram offset in firmware\n"); 824 return -EINVAL; 825 } 826 memcpy(&iram_size, &header->iram_size, sizeof(u8) * 3); 827 if (!iram_size) { 828 phydev_err(phydev, "invalid iram size in firmware\n"); 829 return -EINVAL; 830 } 831 memcpy(&dram_offset, &header->dram_offset, sizeof(u8) * 3); 832 if (!dram_offset) { 833 phydev_err(phydev, "bad dram offset in firmware\n"); 834 return -EINVAL; 835 } 836 memcpy(&dram_size, &header->dram_size, sizeof(u8) * 3); 837 if (!dram_size) { 838 phydev_err(phydev, "invalid dram size in firmware\n"); 839 return -EINVAL; 840 } 841 842 /* offset are in LE and values needs to be converted to cpu endian */ 843 iram_offset = le32_to_cpu(iram_offset); 844 iram_size = le32_to_cpu(iram_size); 845 dram_offset = le32_to_cpu(dram_offset); 846 dram_size = le32_to_cpu(dram_size); 847 848 /* Increment the offset with the primary offset. */ 849 iram_offset += primary_offset; 850 dram_offset += primary_offset; 851 852 phydev_dbg(phydev, "primary %d IRAM offset=%d size=%d DRAM offset=%d size=%d\n", 853 primary_offset, iram_offset, iram_size, dram_offset, dram_size); 854 855 strscpy(version, (char *)data + dram_offset + VERSION_STRING_OFFSET, 856 VERSION_STRING_SIZE); > 857 if (!version) { 858 phydev_err(phydev, "invalid version in firmware\n"); 859 return -EINVAL; 860 } 861 phydev_info(phydev, "loading firmware version '%s'\n", version); 862 863 /* stall the microcprocessor */ 864 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, 865 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD); 866 867 phydev_dbg(phydev, "loading DRAM 0x%08x from offset=%d size=%d\n", 868 DRAM_BASE_ADDR, dram_offset, dram_size); 869 ret = aquantia_load_memory(phydev, DRAM_BASE_ADDR, data + dram_offset, 870 dram_size); 871 if (ret) 872 return ret; 873 874 phydev_dbg(phydev, "loading IRAM 0x%08x from offset=%d size=%d\n", 875 IRAM_BASE_ADDR, iram_offset, iram_size); 876 ret = aquantia_load_memory(phydev, IRAM_BASE_ADDR, data + iram_offset, 877 iram_size); 878 if (ret) 879 return ret; 880 881 /* make sure soft reset and low power mode are clear */ 882 phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC, 883 VEND1_GLOBAL_SC_SOFT_RESET | VEND1_GLOBAL_SC_LOW_POWER); 884 885 /* Release the microprocessor. UP_RESET must be held for 100 usec. */ 886 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, 887 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | 888 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD | 889 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST); 890 usleep_range(UP_RESET_SLEEP, UP_RESET_SLEEP * 2); 891 892 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, 893 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD); 894 895 return 0; 896 } 897
Hi Christian, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/dt-bindings-Document-bindings-for-Marvell-Aquantia-PHY/20231101-203944 base: net-next/main patch link: https://lore.kernel.org/r/20231101123608.11157-1-ansuelsmth%40gmail.com patch subject: [net-next PATCH v2 1/2] net: phy: aquantia: add firmware load support config: i386-randconfig-062-20231102 (https://download.01.org/0day-ci/archive/20231103/202311030505.vv0uoWBW-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/202311030505.vv0uoWBW-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311030505.vv0uoWBW-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/net/phy/aquantia_main.c:746:14: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] addr @@ got restricted __le32 [usertype] @@ drivers/net/phy/aquantia_main.c:746:14: sparse: expected unsigned int [usertype] addr drivers/net/phy/aquantia_main.c:746:14: sparse: got restricted __le32 [usertype] >> drivers/net/phy/aquantia_main.c:776:22: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [addressable] [usertype] word @@ got restricted __be32 [usertype] @@ drivers/net/phy/aquantia_main.c:776:22: sparse: expected unsigned int [addressable] [usertype] word drivers/net/phy/aquantia_main.c:776:22: sparse: got restricted __be32 [usertype] >> drivers/net/phy/aquantia_main.c:803:20: sparse: sparse: cast to restricted __be16 >> drivers/net/phy/aquantia_main.c:817:26: sparse: sparse: cast to restricted __le32 drivers/net/phy/aquantia_main.c:843:23: sparse: sparse: cast to restricted __le32 drivers/net/phy/aquantia_main.c:844:21: sparse: sparse: cast to restricted __le32 drivers/net/phy/aquantia_main.c:845:23: sparse: sparse: cast to restricted __le32 drivers/net/phy/aquantia_main.c:846:21: sparse: sparse: cast to restricted __le32 vim +746 drivers/net/phy/aquantia_main.c 737 738 /* load data into the phy's memory */ 739 static int aquantia_load_memory(struct phy_device *phydev, u32 addr, 740 const u8 *data, size_t len) 741 { 742 u16 crc = 0, up_crc; 743 size_t pos; 744 745 /* PHY expect addr in LE */ > 746 addr = cpu_to_le32(addr); 747 748 phy_write_mmd(phydev, MDIO_MMD_VEND1, 749 VEND1_GLOBAL_MAILBOX_INTERFACE1, 750 VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET); 751 phy_write_mmd(phydev, MDIO_MMD_VEND1, 752 VEND1_GLOBAL_MAILBOX_INTERFACE3, 753 VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(addr)); 754 phy_write_mmd(phydev, MDIO_MMD_VEND1, 755 VEND1_GLOBAL_MAILBOX_INTERFACE4, 756 VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(addr)); 757 758 for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) { 759 u32 word = 0; 760 761 memcpy(&word, data + pos, min(sizeof(u32), len - pos)); 762 763 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5, 764 VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word)); 765 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE6, 766 VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(word)); 767 768 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE1, 769 VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE | 770 VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE); 771 772 /* calculate CRC as we load data to the mailbox. 773 * We convert word to big-endiang as PHY is BE and mailbox will 774 * return a BE CRC. 775 */ > 776 word = cpu_to_be32(word); 777 crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); 778 } 779 780 up_crc = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE2); 781 if (crc != up_crc) { 782 phydev_err(phydev, "CRC mismatch: calculated 0x%04x PHY 0x%04x\n", 783 crc, up_crc); 784 return -EINVAL; 785 } 786 787 return 0; 788 } 789 790 static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size) 791 { 792 const struct aqr_fw_header *header; 793 u32 iram_offset = 0, iram_size = 0; 794 u32 dram_offset = 0, dram_size = 0; 795 char version[VERSION_STRING_SIZE]; 796 u16 calculated_crc, read_crc; 797 u32 primary_offset = 0; 798 int ret; 799 800 /* extract saved CRC at the end of the fw */ 801 memcpy(&read_crc, data + size - 2, sizeof(read_crc)); 802 /* CRC is saved in big-endian as PHY is BE */ > 803 read_crc = be16_to_cpu(read_crc); 804 calculated_crc = crc_ccitt_false(0, data, size - 2); 805 if (read_crc != calculated_crc) { 806 phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n", 807 read_crc, calculated_crc); 808 return -EINVAL; 809 } 810 811 /* Get the primary offset to extract DRAM and IRAM sections. */ 812 memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16)); 813 if (!primary_offset) { 814 phydev_err(phydev, "bad primary offset in firmware\n"); 815 return -EINVAL; 816 } > 817 primary_offset = PRIMARY_OFFSET(le32_to_cpu(primary_offset)); 818 819 /* Find the DRAM and IRAM sections within the firmware file. */ 820 header = (struct aqr_fw_header *)(data + primary_offset + HEADER_OFFSET); 821 memcpy(&iram_offset, &header->iram_offset, sizeof(u8) * 3); 822 if (!iram_offset) { 823 phydev_err(phydev, "bad iram offset in firmware\n"); 824 return -EINVAL; 825 } 826 memcpy(&iram_size, &header->iram_size, sizeof(u8) * 3); 827 if (!iram_size) { 828 phydev_err(phydev, "invalid iram size in firmware\n"); 829 return -EINVAL; 830 } 831 memcpy(&dram_offset, &header->dram_offset, sizeof(u8) * 3); 832 if (!dram_offset) { 833 phydev_err(phydev, "bad dram offset in firmware\n"); 834 return -EINVAL; 835 } 836 memcpy(&dram_size, &header->dram_size, sizeof(u8) * 3); 837 if (!dram_size) { 838 phydev_err(phydev, "invalid dram size in firmware\n"); 839 return -EINVAL; 840 } 841 842 /* offset are in LE and values needs to be converted to cpu endian */ 843 iram_offset = le32_to_cpu(iram_offset); 844 iram_size = le32_to_cpu(iram_size); 845 dram_offset = le32_to_cpu(dram_offset); 846 dram_size = le32_to_cpu(dram_size); 847 848 /* Increment the offset with the primary offset. */ 849 iram_offset += primary_offset; 850 dram_offset += primary_offset; 851 852 phydev_dbg(phydev, "primary %d IRAM offset=%d size=%d DRAM offset=%d size=%d\n", 853 primary_offset, iram_offset, iram_size, dram_offset, dram_size); 854 855 strscpy(version, (char *)data + dram_offset + VERSION_STRING_OFFSET, 856 VERSION_STRING_SIZE); 857 if (!version) { 858 phydev_err(phydev, "invalid version in firmware\n"); 859 return -EINVAL; 860 } 861 phydev_info(phydev, "loading firmware version '%s'\n", version); 862 863 /* stall the microcprocessor */ 864 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, 865 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD); 866 867 phydev_dbg(phydev, "loading DRAM 0x%08x from offset=%d size=%d\n", 868 DRAM_BASE_ADDR, dram_offset, dram_size); 869 ret = aquantia_load_memory(phydev, DRAM_BASE_ADDR, data + dram_offset, 870 dram_size); 871 if (ret) 872 return ret; 873 874 phydev_dbg(phydev, "loading IRAM 0x%08x from offset=%d size=%d\n", 875 IRAM_BASE_ADDR, iram_offset, iram_size); 876 ret = aquantia_load_memory(phydev, IRAM_BASE_ADDR, data + iram_offset, 877 iram_size); 878 if (ret) 879 return ret; 880 881 /* make sure soft reset and low power mode are clear */ 882 phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC, 883 VEND1_GLOBAL_SC_SOFT_RESET | VEND1_GLOBAL_SC_LOW_POWER); 884 885 /* Release the microprocessor. UP_RESET must be held for 100 usec. */ 886 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, 887 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | 888 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD | 889 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST); 890 usleep_range(UP_RESET_SLEEP, UP_RESET_SLEEP * 2); 891 892 phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, 893 VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD); 894 895 return 0; 896 } 897
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 421d2b62918f..46c7194efcea 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -98,6 +98,7 @@ config ADIN1100_PHY config AQUANTIA_PHY tristate "Aquantia PHYs" + select CRC_CCITT help Currently supports the Aquantia AQ1202, AQ2104, AQR105, AQR405 diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index 334a6904ca5a..0f1b8d75cca0 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -12,6 +12,10 @@ #include <linux/delay.h> #include <linux/bitfield.h> #include <linux/phy.h> +#include <linux/of.h> +#include <linux/firmware.h> +#include <linux/crc-ccitt.h> +#include <linux/nvmem-consumer.h> #include "aquantia.h" @@ -92,10 +96,40 @@ #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES 0xd31b /* Vendor specific 1, MDIO_MMD_VEND1 */ +#define VEND1_GLOBAL_SC 0x0 +#define VEND1_GLOBAL_SC_SOFT_RESET BIT(15) +#define VEND1_GLOBAL_SC_LOW_POWER BIT(11) + #define VEND1_GLOBAL_FW_ID 0x0020 #define VEND1_GLOBAL_FW_ID_MAJOR GENMASK(15, 8) #define VEND1_GLOBAL_FW_ID_MINOR GENMASK(7, 0) +#define VEND1_GLOBAL_MAILBOX_INTERFACE1 0x0200 +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE BIT(15) +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE BIT(14) +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET BIT(12) +#define VEND1_GLOBAL_MAILBOX_INTERFACE1_BUSY BIT(8) + +#define VEND1_GLOBAL_MAILBOX_INTERFACE2 0x0201 +#define VEND1_GLOBAL_MAILBOX_INTERFACE3 0x0202 +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK GENMASK(15, 0) +#define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK, (u16)((x) >> 16)) +#define VEND1_GLOBAL_MAILBOX_INTERFACE4 0x0203 +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK GENMASK(15, 2) +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK, (u16)(x)) + +#define VEND1_GLOBAL_MAILBOX_INTERFACE5 0x0204 +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK GENMASK(15, 0) +#define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK, (u16)((x) >> 16)) +#define VEND1_GLOBAL_MAILBOX_INTERFACE6 0x0205 +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK GENMASK(15, 0) +#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK, (u16)(x)) + +#define VEND1_GLOBAL_CONTROL2 0xc001 +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST BIT(15) +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD BIT(6) +#define VEND1_GLOBAL_CONTROL2_UP_RUN_STALL BIT(0) + #define VEND1_GLOBAL_GEN_STAT2 0xc831 #define VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG BIT(15) @@ -152,6 +186,30 @@ #define AQR107_OP_IN_PROG_SLEEP 1000 #define AQR107_OP_IN_PROG_TIMEOUT 100000 +#define UP_RESET_SLEEP 100 + +/* addresses of memory segments in the phy */ +#define DRAM_BASE_ADDR 0x3FFE0000 +#define IRAM_BASE_ADDR 0x40000000 + +/* firmware image format constants */ +#define VERSION_STRING_SIZE 0x40 +#define VERSION_STRING_OFFSET 0x0200 +/* primary offset is written at an offset from the start of the fw blob */ +#define PRIMARY_OFFSET_OFFSET 0x8 +/* primary offset needs to be then added to a base offset */ +#define PRIMARY_OFFSET_SHIFT 12 +#define PRIMARY_OFFSET(x) ((x) << PRIMARY_OFFSET_SHIFT) +#define HEADER_OFFSET 0x300 + +struct aqr_fw_header { + u32 padding; + u8 iram_offset[3]; + u8 iram_size[3]; + u8 dram_offset[3]; + u8 dram_size[3]; +} __packed; + struct aqr107_hw_stat { const char *name; int reg; @@ -677,6 +735,166 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev) return 0; } +/* load data into the phy's memory */ +static int aquantia_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); + + 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)); + phy_write_mmd(phydev, MDIO_MMD_VEND1, + VEND1_GLOBAL_MAILBOX_INTERFACE4, + VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(addr)); + + for (pos = 0; pos < len; pos += min(sizeof(u32), len - pos)) { + u32 word = 0; + + memcpy(&word, data + pos, min(sizeof(u32), len - 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); + crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); + } + + up_crc = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE2); + if (crc != up_crc) { + phydev_err(phydev, "CRC mismatch: calculated 0x%04x PHY 0x%04x\n", + crc, up_crc); + return -EINVAL; + } + + return 0; +} + +static int aqr_fw_boot(struct phy_device *phydev, const u8 *data, size_t size) +{ + const struct aqr_fw_header *header; + u32 iram_offset = 0, iram_size = 0; + u32 dram_offset = 0, dram_size = 0; + char version[VERSION_STRING_SIZE]; + u16 calculated_crc, read_crc; + u32 primary_offset = 0; + int ret; + + /* extract saved CRC at the end of the fw */ + memcpy(&read_crc, data + size - 2, sizeof(read_crc)); + /* CRC is saved in big-endian as PHY is BE */ + read_crc = be16_to_cpu(read_crc); + calculated_crc = crc_ccitt_false(0, data, size - 2); + if (read_crc != calculated_crc) { + phydev_err(phydev, "bad firmware CRC: file 0x%04x calculated 0x%04x\n", + read_crc, calculated_crc); + return -EINVAL; + } + + /* Get the primary offset to extract DRAM and IRAM sections. */ + memcpy(&primary_offset, data + PRIMARY_OFFSET_OFFSET, sizeof(u16)); + if (!primary_offset) { + phydev_err(phydev, "bad primary offset in firmware\n"); + return -EINVAL; + } + primary_offset = PRIMARY_OFFSET(le32_to_cpu(primary_offset)); + + /* Find the DRAM and IRAM sections within the firmware file. */ + header = (struct aqr_fw_header *)(data + primary_offset + HEADER_OFFSET); + memcpy(&iram_offset, &header->iram_offset, sizeof(u8) * 3); + if (!iram_offset) { + phydev_err(phydev, "bad iram offset in firmware\n"); + return -EINVAL; + } + memcpy(&iram_size, &header->iram_size, sizeof(u8) * 3); + if (!iram_size) { + phydev_err(phydev, "invalid iram size in firmware\n"); + return -EINVAL; + } + memcpy(&dram_offset, &header->dram_offset, sizeof(u8) * 3); + if (!dram_offset) { + phydev_err(phydev, "bad dram offset in firmware\n"); + return -EINVAL; + } + memcpy(&dram_size, &header->dram_size, sizeof(u8) * 3); + if (!dram_size) { + phydev_err(phydev, "invalid dram size in firmware\n"); + return -EINVAL; + } + + /* offset are in LE and values needs to be converted to cpu endian */ + iram_offset = le32_to_cpu(iram_offset); + iram_size = le32_to_cpu(iram_size); + dram_offset = le32_to_cpu(dram_offset); + dram_size = le32_to_cpu(dram_size); + + /* Increment the offset with the primary offset. */ + iram_offset += primary_offset; + dram_offset += primary_offset; + + phydev_dbg(phydev, "primary %d IRAM offset=%d size=%d DRAM offset=%d size=%d\n", + primary_offset, iram_offset, iram_size, dram_offset, dram_size); + + strscpy(version, (char *)data + dram_offset + VERSION_STRING_OFFSET, + VERSION_STRING_SIZE); + if (!version) { + phydev_err(phydev, "invalid version in firmware\n"); + return -EINVAL; + } + phydev_info(phydev, "loading firmware version '%s'\n", version); + + /* stall the microcprocessor */ + phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, + VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD); + + phydev_dbg(phydev, "loading DRAM 0x%08x from offset=%d size=%d\n", + DRAM_BASE_ADDR, dram_offset, dram_size); + ret = aquantia_load_memory(phydev, DRAM_BASE_ADDR, data + dram_offset, + dram_size); + if (ret) + return ret; + + phydev_dbg(phydev, "loading IRAM 0x%08x from offset=%d size=%d\n", + IRAM_BASE_ADDR, iram_offset, iram_size); + ret = aquantia_load_memory(phydev, IRAM_BASE_ADDR, data + iram_offset, + iram_size); + if (ret) + return ret; + + /* make sure soft reset and low power mode are clear */ + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC, + VEND1_GLOBAL_SC_SOFT_RESET | VEND1_GLOBAL_SC_LOW_POWER); + + /* Release the microprocessor. UP_RESET must be held for 100 usec. */ + phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, + VEND1_GLOBAL_CONTROL2_UP_RUN_STALL | + VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD | + VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_RST); + usleep_range(UP_RESET_SLEEP, UP_RESET_SLEEP * 2); + + phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_CONTROL2, + VEND1_GLOBAL_CONTROL2_UP_RUN_STALL_OVD); + + return 0; +} + static int aqr107_get_rate_matching(struct phy_device *phydev, phy_interface_t iface) { @@ -711,13 +929,99 @@ static int aqr107_resume(struct phy_device *phydev) return aqr107_wait_processor_intensive_op(phydev); } +static int aqr_firmware_load_nvmem(struct phy_device *phydev) +{ + struct nvmem_cell *cell; + size_t size; + u8 *buf; + int ret; + + cell = nvmem_cell_get(&phydev->mdio.dev, "firmware"); + if (IS_ERR(cell)) + return PTR_ERR(cell); + + buf = nvmem_cell_read(cell, &size); + if (IS_ERR(buf)) { + ret = PTR_ERR(buf); + goto exit; + } + + ret = aqr_fw_boot(phydev, buf, size); + if (ret) + phydev_err(phydev, "firmware loading failed: %d\n", ret); + +exit: + nvmem_cell_put(cell); + + return ret; +} + +static int aqr_firmware_load_fs(struct phy_device *phydev) +{ + struct device *dev = &phydev->mdio.dev; + const struct firmware *fw; + const char *fw_name; + int ret; + + ret = of_property_read_string(dev->of_node, "firmware-name", + &fw_name); + if (ret) + return ret; + + ret = request_firmware(&fw, fw_name, dev); + if (ret) { + phydev_err(phydev, "failed to find FW file %s (%d)\n", + fw_name, ret); + goto exit; + } + + ret = aqr_fw_boot(phydev, fw->data, fw->size); + if (ret) + phydev_err(phydev, "firmware loading failed: %d\n", ret); + +exit: + release_firmware(fw); + + return ret; +} + +static int aqr_firmware_load(struct phy_device *phydev) +{ + int ret; + + /* Check if the firmware is not already loaded by pooling + * the current version returned by the PHY. If 0 is returned, + * no firmware is loaded. + */ + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID); + if (ret > 0) + goto exit; + + ret = aqr_firmware_load_nvmem(phydev); + if (!ret) + goto exit; + + ret = aqr_firmware_load_fs(phydev); + if (ret) + return ret; + +exit: + return 0; +} + static int aqr107_probe(struct phy_device *phydev) { + int ret; + phydev->priv = devm_kzalloc(&phydev->mdio.dev, sizeof(struct aqr107_priv), GFP_KERNEL); if (!phydev->priv) return -ENOMEM; + ret = aqr_firmware_load(phydev); + if (ret) + return ret; + return aqr_hwmon_probe(phydev); }