| Message ID | 20231214201442.660447-1-tobias@waldekranz.com |
|---|---|
| Headers | show |
| Series | net: phy: marvell10g: Firmware loading and LED support for 88X3310 | expand |
On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote: > When probing, if a device is waiting for firmware to be loaded into > its RAM, ask userspace for the binary and load it over XMDIO. Does a device without firmware have valid ID registers? Is the driver going to probe via bus enumeration, or is it necessary to use a compatible with ID values? > + for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) { This validates that the firmware is big enough to hold the header... > + memcpy(&hdr, sect, sizeof(hdr)); > + hdr.data.size = cpu_to_le32(hdr.data.size); > + hdr.data.addr = cpu_to_le32(hdr.data.addr); > + hdr.data.csum = cpu_to_le16(hdr.data.csum); > + hdr.next_hdr = cpu_to_le32(hdr.next_hdr); I'm surprised sparse is not complaining about this. You have the same source and destination, and sparse probably wants the destination to be marked as little endian. > + hdr.csum = cpu_to_le16(hdr.csum); > + > + for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++) > + csum += sect[i]; > + > + if ((u16)~csum != hdr.csum) { > + dev_err(&phydev->mdio.dev, "Corrupt section header\n"); > + err = -EINVAL; > + break; > + } > + > + err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr)); What i don't see is any validation that the firmware left at sect + sizeof(hdr) big enough to contain hdr.data.size bytes. Andrew
> + for (i = 0, csum = 0; i < hdr->data.size; i++) > + csum += data[i]; > + for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) { > + memcpy(&hdr, sect, sizeof(hdr)); > + hdr.data.size = cpu_to_le32(hdr.data.size); hdr.data.size is little endian. Doing a for loop using a little endian test seems wrong. Should this actually be le32_to_cpu()? Andrew
On Fri, Dec 15, 2023 at 03:30:09PM +0100, Andrew Lunn wrote: > On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote: > > When probing, if a device is waiting for firmware to be loaded into > > its RAM, ask userspace for the binary and load it over XMDIO. > > Does a device without firmware have valid ID registers? Yes it does - remember one of the ZII dev boards had a 3310 without firmware, and that can be successfully probed by the driver, which is key to my userspace tooling being able to access the PHY (since userspace can only access devices on MDIO buses that are bound to a netdev.)
> +static int mv3310_led_funcs_from_flags(struct mv3310_led *led, > + unsigned long flags, > + enum mv3310_led_func *solid, > + enum mv3310_led_func *blink) > +{ > + unsigned long activity, duplex, link; > + > + if (flags & ~(BIT(TRIGGER_NETDEV_LINK) | > + BIT(TRIGGER_NETDEV_HALF_DUPLEX) | > + BIT(TRIGGER_NETDEV_FULL_DUPLEX) | > + BIT(TRIGGER_NETDEV_TX) | > + BIT(TRIGGER_NETDEV_RX))) > + return -EINVAL; This probably should be -EOPNOTSUPP. The trigger will then do the blinking in software. > + > + link = flags & BIT(TRIGGER_NETDEV_LINK); > + > + duplex = flags & (BIT(TRIGGER_NETDEV_HALF_DUPLEX) | > + BIT(TRIGGER_NETDEV_FULL_DUPLEX)); > + > + activity = flags & (BIT(TRIGGER_NETDEV_TX) | > + BIT(TRIGGER_NETDEV_RX)); > + > + if (link && duplex) > + return -EINVAL; It is an odd combination, but again, if the hardware cannot do it, return -EOPNOTSUPP and leave it to the software. Andrew
On Fri, Dec 15, 2023 at 03:44:07PM +0100, Andrew Lunn wrote: > > +static int mv3310_led_funcs_from_flags(struct mv3310_led *led, > > + unsigned long flags, > > + enum mv3310_led_func *solid, > > + enum mv3310_led_func *blink) > > +{ > > + unsigned long activity, duplex, link; > > + > > + if (flags & ~(BIT(TRIGGER_NETDEV_LINK) | > > + BIT(TRIGGER_NETDEV_HALF_DUPLEX) | > > + BIT(TRIGGER_NETDEV_FULL_DUPLEX) | > > + BIT(TRIGGER_NETDEV_TX) | > > + BIT(TRIGGER_NETDEV_RX))) > > + return -EINVAL; > > This probably should be -EOPNOTSUPP. The trigger will then do the > blinking in software. > > > + > > + link = flags & BIT(TRIGGER_NETDEV_LINK); > > + > > + duplex = flags & (BIT(TRIGGER_NETDEV_HALF_DUPLEX) | > > + BIT(TRIGGER_NETDEV_FULL_DUPLEX)); > > + > > + activity = flags & (BIT(TRIGGER_NETDEV_TX) | > > + BIT(TRIGGER_NETDEV_RX)); > > + > > + if (link && duplex) > > + return -EINVAL; > > It is an odd combination, but again, if the hardware cannot do it, > return -EOPNOTSUPP and leave it to the software. I don't recall how the LED triggers work (whether they logically OR or AND). The hardware supports indicating whether it has a half or full duplex link, and if the LED is programmed for that, then it will illuminate when it has link _and_ the duplex is of the specified type. If the link is down, or the duplex is of the other type, then it won't. I'm guessing that if link is set and duplex is set, then what userspace is asking for is the LED to be illuminated whenever the link is up _or_ the duplex is of the specified type, which basically logically resolves to _only_ "link is up" (because a link that is down has no duplex.) So, I'm not sure "leaving it to software" is good, that combination is effectively just "has link".
On Thu, Dec 14, 2023 at 09:14:40PM +0100, Tobias Waldekranz wrote: > On devices which are hardware strapped to start powered down (PDSTATE > == 1), make sure that we clear the power-down bit on all units > affected by this setting. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > drivers/net/phy/marvell10g.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > index 83233b30d7b0..1c1333d867fb 100644 > --- a/drivers/net/phy/marvell10g.c > +++ b/drivers/net/phy/marvell10g.c > @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev) > > static int mv3310_power_up(struct phy_device *phydev) > { > + static const u16 resets[][2] = { > + { MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1 }, > + { MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1 }, This is not necessary. The documentation states that the power down bit found at each of these is the same physical bit appearing in two different locations. So only one is necessary. > + { MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1 }, > + { MDIO_MMD_PMAPMD, MDIO_CTRL1 }, > + { MDIO_MMD_VEND2, MV_V2_PORT_CTRL }, > + }; > struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); > - int ret; > + int i, ret; > > - ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, > - MV_V2_PORT_CTRL_PWRDOWN); > + for (i = 0; i < ARRAY_SIZE(resets); i++) { > + ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1], > + MV_V2_PORT_CTRL_PWRDOWN); While MV_V2_PORT_CTRL_PWRDOWN may correspond with the correct bit for the MDIO CTRL1 register, we have MDIO_CTRL1_LPOWER which describes this bit. Probably the simplest solution would be to leave the existing phy_clear_bits_mmd(), remove the vendor 2 entry from the table, and run through that table first. Lastly, how does this impact a device which has firmware, and the firmware manages the power-down state (the manual states that unused blocks will be powered down - I assume by the firmware.) If this causes blocks which had been powered down by the firmware because they're not being used to then be powered up, that is a regression. Please check that this is not the case.
On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote: > + MV_PMA_BOOT_PRGS_MASK = 0x0006, > + MV_PMA_BOOT_PRGS_INIT = 0x0000, > + MV_PMA_BOOT_PRGS_WAIT = 0x0002, > + MV_PMA_BOOT_PRGS_CSUM = 0x0004, > + MV_PMA_BOOT_PRGS_JRAM = 0x0006, You only seem to use PRGS_WAIT, the rest seem unused. > +struct mv3310_fw_hdr { > + struct { > + u32 size; > + u32 addr; > + u16 csum; > + } __packed data; It's probably better to get rid of this embedded struct and just place the members in the parent struct (although csum woul dneed to be renamed). > + > + u8 flags; > +#define MV3310_FW_HDR_DATA_ONLY BIT(6) > + > + u8 port_skip; > + u32 next_hdr; > + u16 csum; > + > + u8 pad[14]; > +} __packed; > + > +static int mv3310_load_fw_sect(struct phy_device *phydev, > + const struct mv3310_fw_hdr *hdr, const u8 *data) > +{ > + int err = 0; > + size_t i; > + u16 csum; > + > + dev_dbg(&phydev->mdio.dev, "Loading %u byte %s section at 0x%08x\n", > + hdr->data.size, > + (hdr->flags & MV3310_FW_HDR_DATA_ONLY) ? "data" : "executable", > + hdr->data.addr); > + > + for (i = 0, csum = 0; i < hdr->data.size; i++) > + csum += data[i]; > + > + if ((u16)~csum != hdr->data.csum) { > + dev_err(&phydev->mdio.dev, "Corrupt section data\n"); > + return -EINVAL; > + } > + > + phy_lock_mdio_bus(phydev); > + > + /* Any existing checksum is cleared by a read */ > + __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM); > + > + __phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_LOW, hdr->data.addr & 0xffff); > + __phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_HIGH, hdr->data.addr >> 16); > + > + for (i = 0; i < hdr->data.size; i += 2) { > + __phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_DATA, > + (data[i + 1] << 8) | data[i]); > + } > + > + csum = __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM); > + if ((u16)~csum != hdr->data.csum) { > + dev_err(&phydev->mdio.dev, "Download failed\n"); > + err = -EIO; > + goto unlock; > + } > + > + if (hdr->flags & MV3310_FW_HDR_DATA_ONLY) > + goto unlock; > + > + __phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT, 0, MV_PMA_BOOT_APP_LOADED); > + mdelay(200); > + if (!(__phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT) & MV_PMA_BOOT_APP_STARTED)) { > + dev_err(&phydev->mdio.dev, "Application did not startup\n"); > + err = -ENODEV; > + } I'm confused why this is done here - after each section in the firmware file, rather than having loaded all sections in the firmware file and only then starting the application. Surely if there's multiple sections that we're going to load, we want to load _all_ sections before starting the application?
On Thu, Dec 14, 2023 at 09:14:41PM +0100, Tobias Waldekranz wrote: > Pickup the LEDs from the state in which the hardware reset or > bootloader left them, but also support further configuration via > device tree. This is primarily needed because the electrical polarity > and drive behavior is software controlled and not possible to set via > hardware strapping. > > Trigger support: > - "none" > - "timer": Map 60-100 ms periods to the fast rate (81ms) and 1000-1600 > ms periods to the slow rate (1300ms). Defer everything else to > software blinking > - "netdev": Offload link or duplex information to the solid behavior; > tx and/or rx activity to blink behavior. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> ... > +static int mv3310_leds_probe(struct phy_device *phydev) > +{ > + struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); > + struct device_node *node = phydev->mdio.dev.of_node; > + struct device_node *pnp, *np; > + int err, val, index; > + > + /* Save the config left by HW reset or bootloader, to make > + * sure that we do not loose any polarity config made by > + * firmware. This will be overridden by info from DT, if > + * available. > + */ > + for (index = 0; index < MV3310_N_LEDS; index++) { > + val = phy_read_mmd(phydev, MDIO_MMD_VEND2, > + MV_V2_LED0_CONTROL + index); > + if (val < 0) > + return val; > + > + priv->led[index] = (struct mv3310_led) { > + .index = index, > + .fw_ctrl = val, > + }; > + } > + > + if (!node) > + return 0; > + > + pnp = of_get_child_by_name(node, "leds"); > + if (!pnp) > + return 0; > + > + for_each_available_child_of_node(pnp, np) { > + err = mv3310_led_probe_of(phydev, np); > + if (err) Hi Tobias, I think a call to of_node_put(np) is required here to avoid leaking a reference. Flagged by Coccinelle. > + return err; > + } > + > + return 0; > +} ...
On fre, dec 15, 2023 at 15:30, Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote: >> When probing, if a device is waiting for firmware to be loaded into >> its RAM, ask userspace for the binary and load it over XMDIO. > > Does a device without firmware have valid ID registers? Is the driver > going to probe via bus enumeration, or is it necessary to use a > compatible with ID values? > >> + for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) { > > This validates that the firmware is big enough to hold the header... > >> + memcpy(&hdr, sect, sizeof(hdr)); >> + hdr.data.size = cpu_to_le32(hdr.data.size); >> + hdr.data.addr = cpu_to_le32(hdr.data.addr); >> + hdr.data.csum = cpu_to_le16(hdr.data.csum); >> + hdr.next_hdr = cpu_to_le32(hdr.next_hdr); > > I'm surprised sparse is not complaining about this. You have the same > source and destination, and sparse probably wants the destination to > be marked as little endian. > >> + hdr.csum = cpu_to_le16(hdr.csum); >> + >> + for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++) >> + csum += sect[i]; >> + >> + if ((u16)~csum != hdr.csum) { >> + dev_err(&phydev->mdio.dev, "Corrupt section header\n"); >> + err = -EINVAL; >> + break; >> + } >> + >> + err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr)); > > What i don't see is any validation that the firmware left at sect + > sizeof(hdr) big enough to contain hdr.data.size bytes. > Thanks Andrew and Russel, for the review! You both make valid points, I'll try to be less clever about this whole section in v2.
On fre, dec 15, 2023 at 15:44, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Thu, Dec 14, 2023 at 09:14:40PM +0100, Tobias Waldekranz wrote: >> On devices which are hardware strapped to start powered down (PDSTATE >> == 1), make sure that we clear the power-down bit on all units >> affected by this setting. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> drivers/net/phy/marvell10g.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c >> index 83233b30d7b0..1c1333d867fb 100644 >> --- a/drivers/net/phy/marvell10g.c >> +++ b/drivers/net/phy/marvell10g.c >> @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev) >> >> static int mv3310_power_up(struct phy_device *phydev) >> { >> + static const u16 resets[][2] = { >> + { MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1 }, >> + { MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1 }, > > This is not necessary. The documentation states that the power down > bit found at each of these is the same physical bit appearing in two > different locations. So only one is necessary. Right, I'll remove the entry for 1000BASE-X in v2. >> + { MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1 }, >> + { MDIO_MMD_PMAPMD, MDIO_CTRL1 }, >> + { MDIO_MMD_VEND2, MV_V2_PORT_CTRL }, >> + }; >> struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); >> - int ret; >> + int i, ret; >> >> - ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL, >> - MV_V2_PORT_CTRL_PWRDOWN); >> + for (i = 0; i < ARRAY_SIZE(resets); i++) { >> + ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1], >> + MV_V2_PORT_CTRL_PWRDOWN); > > While MV_V2_PORT_CTRL_PWRDOWN may correspond with the correct bit for > the MDIO CTRL1 register, we have MDIO_CTRL1_LPOWER which describes > this bit. Probably the simplest solution would be to leave the > existing phy_clear_bits_mmd(), remove the vendor 2 entry from the > table, and run through that table first. Yes, I'll fix this in v2. > Lastly, how does this impact a device which has firmware, and the > firmware manages the power-down state (the manual states that unused > blocks will be powered down - I assume by the firmware.) If this > causes blocks which had been powered down by the firmware because > they're not being used to then be powered up, that is a regression. > Please check that this is not the case. This will be very hard for me to test, as I only have access to boards without dedicated FLASHes. That said, I don't think I understand how this is related to how the devices load their firmware. As I understand it, we should pick up the device in the exact same state after the MDIO load as we would if it had booted on its own, via a serial FLASH. The selection of PDSTATE, based on the sample-at-reset pins, is independent of how firmware is loaded. From the manual: The following registers can be set to force the units to power down. I interpret this as the power-down bits only acting as gates to stop firmware from powering up a particular unit. Conversely, clearing one of these bits merely indicates that the firmware is free to power up the unit in question. On a device strapped to PDSTATE==0, I would expect all of these bits to already be cleared, since the manual states that the value of PDSTATE is latched into all these bits at reset.
On Thu, 14 Dec 2023 21:14:39 +0100
Tobias Waldekranz <tobias@waldekranz.com> wrote:
> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
And do you have permission to publish this firmware into linux-firmware?
Because when we tried this with Marvell, their lawyer guy said we can't
do that...
Marek
On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote: > On Thu, 14 Dec 2023 21:14:39 +0100 > Tobias Waldekranz <tobias@waldekranz.com> wrote: > >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr"); > > And do you have permission to publish this firmware into linux-firmware? No, I do not. > Because when we tried this with Marvell, their lawyer guy said we can't > do that... I don't even have good enough access to ask the question, much less get rejected by Marvell :) I just used that path so that it would line up with linux-firmware if Marvell was to publish it in the future. Should MODULE_FIRMWARE be avoided for things that are not in linux-firmware?
On Tue, 19 Dec 2023 11:15:41 +0100 Tobias Waldekranz <tobias@waldekranz.com> wrote: > On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote: > > On Thu, 14 Dec 2023 21:14:39 +0100 > > Tobias Waldekranz <tobias@waldekranz.com> wrote: > > > >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr"); > > > > And do you have permission to publish this firmware into linux-firmware? > > No, I do not. > > > Because when we tried this with Marvell, their lawyer guy said we can't > > do that... > > I don't even have good enough access to ask the question, much less get > rejected by Marvell :) I just used that path so that it would line up > with linux-firmware if Marvell was to publish it in the future. Yeah, it was pretty stupid in my opinion. The lawyer guy's reasoning was that to download the firmware from Marvell's Customer Portal you have to agree with Terms & Conditions, so it can't be distributed to people who did not agree to Terms & Conditions. We told him that anyone can get access to the firmware without agreeing anyway, since they can just read the SPI NOR module connected to the PHY if we burn the firmware in manufacture... > Should MODULE_FIRMWARE be avoided for things that are not in > linux-firmware? I don't know.
On tis, dec 19, 2023 at 11:49, Marek Behún <kabel@kernel.org> wrote: > On Tue, 19 Dec 2023 11:15:41 +0100 > Tobias Waldekranz <tobias@waldekranz.com> wrote: > >> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote: >> > On Thu, 14 Dec 2023 21:14:39 +0100 >> > Tobias Waldekranz <tobias@waldekranz.com> wrote: >> > >> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr"); >> > >> > And do you have permission to publish this firmware into linux-firmware? >> >> No, I do not. >> >> > Because when we tried this with Marvell, their lawyer guy said we can't >> > do that... >> >> I don't even have good enough access to ask the question, much less get >> rejected by Marvell :) I just used that path so that it would line up >> with linux-firmware if Marvell was to publish it in the future. > > Yeah, it was pretty stupid in my opinion. The lawyer guy's reasoning > was that to download the firmware from Marvell's Customer Portal you > have to agree with Terms & Conditions, so it can't be distributed to > people who did not agree to Terms & Conditions. We told him that anyone > can get access to the firmware without agreeing anyway, since they can > just read the SPI NOR module connected to the PHY if we burn the > firmware in manufacture... Yeah, they are needlessly secretive in lots of ways - much to their own detriment, IMO. They also protect their functional specs as if you could just run them through `pdf2rtl`, email the output to TSMC, and have your own 7nm ASIC in the mail the following week.
On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote: > On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote: > > On Thu, 14 Dec 2023 21:14:39 +0100 > > Tobias Waldekranz <tobias@waldekranz.com> wrote: > > > >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr"); > > > > And do you have permission to publish this firmware into linux-firmware? > > No, I do not. > > > Because when we tried this with Marvell, their lawyer guy said we can't > > do that... > > I don't even have good enough access to ask the question, much less get > rejected by Marvell :) I just used that path so that it would line up > with linux-firmware if Marvell was to publish it in the future. > > Should MODULE_FIRMWARE be avoided for things that are not in > linux-firmware? Without the firmware being published, what use is having this code in mainline kernels?
On tis, jan 02, 2024 at 10:12, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote: >> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote: >> > On Thu, 14 Dec 2023 21:14:39 +0100 >> > Tobias Waldekranz <tobias@waldekranz.com> wrote: >> > >> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr"); >> > >> > And do you have permission to publish this firmware into linux-firmware? >> >> No, I do not. >> >> > Because when we tried this with Marvell, their lawyer guy said we can't >> > do that... >> >> I don't even have good enough access to ask the question, much less get >> rejected by Marvell :) I just used that path so that it would line up >> with linux-firmware if Marvell was to publish it in the future. >> >> Should MODULE_FIRMWARE be avoided for things that are not in >> linux-firmware? > > Without the firmware being published, what use is having this code in > mainline kernels? Personally, I primarily want this merged so that future contributions to the driver are easier to develop, since I'll be able test them on top of a clean net-next base. More broadly, I suppose it will help others who are looking to support similar boards to run the latest kernel, without the need to juggle external patches which are likely to break as the driver evolves. Having a single, canonical, implementation of firmware loading, instead of multiple slightly-broken-in-different-ways ones floating around, also seems like a net positive.