| Message ID | 20231214201442.660447-2-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| 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.)
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?
Hi Tobias, 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/Tobias-Waldekranz/net-phy-marvell10g-Support-firmware-loading-on-88X3310/20231215-041703 base: net-next/main patch link: https://lore.kernel.org/r/20231214201442.660447-2-tobias%40waldekranz.com patch subject: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310 config: x86_64-randconfig-123-20231216 (https://download.01.org/0day-ci/archive/20231216/202312162238.aJCgm39Y-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312162238.aJCgm39Y-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/202312162238.aJCgm39Y-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/net/phy/marvell10g.c:620:31: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [addressable] [usertype] size @@ got restricted __le32 [usertype] @@ drivers/net/phy/marvell10g.c:620:31: sparse: expected unsigned int [addressable] [usertype] size drivers/net/phy/marvell10g.c:620:31: sparse: got restricted __le32 [usertype] >> drivers/net/phy/marvell10g.c:621:31: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [addressable] [usertype] addr @@ got restricted __le32 [usertype] @@ drivers/net/phy/marvell10g.c:621:31: sparse: expected unsigned int [addressable] [usertype] addr drivers/net/phy/marvell10g.c:621:31: sparse: got restricted __le32 [usertype] >> drivers/net/phy/marvell10g.c:622:31: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [addressable] [usertype] csum @@ got restricted __le16 [usertype] @@ drivers/net/phy/marvell10g.c:622:31: sparse: expected unsigned short [addressable] [usertype] csum drivers/net/phy/marvell10g.c:622:31: sparse: got restricted __le16 [usertype] >> drivers/net/phy/marvell10g.c:623:30: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [addressable] [usertype] next_hdr @@ got restricted __le32 [usertype] @@ drivers/net/phy/marvell10g.c:623:30: sparse: expected unsigned int [addressable] [usertype] next_hdr drivers/net/phy/marvell10g.c:623:30: sparse: got restricted __le32 [usertype] drivers/net/phy/marvell10g.c:624:26: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [addressable] [usertype] csum @@ got restricted __le16 [usertype] @@ drivers/net/phy/marvell10g.c:624:26: sparse: expected unsigned short [addressable] [usertype] csum drivers/net/phy/marvell10g.c:624:26: sparse: got restricted __le16 [usertype] vim +620 drivers/net/phy/marvell10g.c 595 596 static int mv3310_load_fw(struct phy_device *phydev) 597 { 598 const struct mv3310_chip *chip = to_mv3310_chip(phydev); 599 const struct firmware *fw; 600 struct mv3310_fw_hdr hdr; 601 const u8 *sect; 602 size_t i; 603 u16 csum; 604 int err; 605 606 if (!chip->firmware_path) 607 return -EOPNOTSUPP; 608 609 err = request_firmware(&fw, chip->firmware_path, &phydev->mdio.dev); 610 if (err) 611 return err; 612 613 if (fw->size & 1) { 614 err = -EINVAL; 615 goto release; 616 } 617 618 for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) { 619 memcpy(&hdr, sect, sizeof(hdr)); > 620 hdr.data.size = cpu_to_le32(hdr.data.size); > 621 hdr.data.addr = cpu_to_le32(hdr.data.addr); > 622 hdr.data.csum = cpu_to_le16(hdr.data.csum); > 623 hdr.next_hdr = cpu_to_le32(hdr.next_hdr); 624 hdr.csum = cpu_to_le16(hdr.csum); 625 626 for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++) 627 csum += sect[i]; 628 629 if ((u16)~csum != hdr.csum) { 630 dev_err(&phydev->mdio.dev, "Corrupt section header\n"); 631 err = -EINVAL; 632 break; 633 } 634 635 err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr)); 636 if (err) 637 break; 638 639 if (!hdr.next_hdr) 640 break; 641 642 sect = fw->data + hdr.next_hdr; 643 } 644 645 release: 646 release_firmware(fw); 647 return err; 648 } 649
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 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.
Hi Tobias, On Tue, Jan 02, 2024 at 02:09:24PM +0100, Tobias Waldekranz wrote: > 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. I've been pointed to your series by Krzysztof Kozlowski who had reviewed the DT part of it. Are you still working on that or going to eventually re-submit it? I understand that the suggested LED support pre-dates commit 7ae215ee7bb8 net: phy: add support for PHY LEDs polarity modes which would allow using generic properties 'active-low' and 'inactive-high-impedance'. I assume that would be applicable to the LED patch which was part of this series as well? In that case, we would no longer need a vendor-specific property for that purpose. If the LEDs are active-low by default (or early boot firmware setting) and you would need a property for setting them to 'active-high' instead, I just suggested that in https://patchwork.kernel.org/project/netdevbpf/patch/e91ca84ac836fc40c94c52733f8fc607bcbed64c.1728145095.git.daniel@makrotopia.org/ which is why I'm now contacting you, as I was a bit confused by Krzysztof's suggestion to take a look at marvell,marvell10g.yaml which would have been introduced by your series. Imho it would be better to use the (now existing) generic properties than resorting to a vendor-specific one. In every case, if you have a minute to look at commit 7ae215ee7bb8 and let us know whether that structure, with or without my suggested addition, would be suitable for your case as well, that would be nice. Thank you for your time and support! Daniel
On sön, okt 06, 2024 at 17:15, Daniel Golle <daniel@makrotopia.org> wrote: > Hi Tobias, Hi Daniel, > On Tue, Jan 02, 2024 at 02:09:24PM +0100, Tobias Waldekranz wrote: >> 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. > > I've been pointed to your series by Krzysztof Kozlowski who had reviewed > the DT part of it. Are you still working on that or going to eventually > re-submit it? I'm not actively working on it, no, but I still want to get it merged. I, perhaps wrongly, interpreted Russell's lack of reply to my motivation for accepting the firmware loading without having the binary in linux-firmware, as a NAK, and moved on to other things. > I understand that the suggested LED support pre-dates commit > > 7ae215ee7bb8 net: phy: add support for PHY LEDs polarity modes > > which would allow using generic properties 'active-low' and > 'inactive-high-impedance'. I assume that would be applicable to the LED > patch which was part of this series as well? > > In that case, we would no longer need a vendor-specific property for that > purpose. If the LEDs are active-low by default (or early boot firmware > setting) and you would need a property for setting them to 'active-high' > instead, I just suggested that in > > https://patchwork.kernel.org/project/netdevbpf/patch/e91ca84ac836fc40c94c52733f8fc607bcbed64c.1728145095.git.daniel@makrotopia.org/ > > which is why I'm now contacting you, as I was a bit confused by Krzysztof's > suggestion to take a look at marvell,marvell10g.yaml which would have been > introduced by your series. > > Imho it would be better to use the (now existing) generic properties than > resorting to a vendor-specific one. > > In every case, if you have a minute to look at commit 7ae215ee7bb8 and let > us know whether that structure, with or without my suggested addition, > would be suitable for your case as well, that would be nice. To me, it seems cleaner to have a single attribute that defines the behavior you want on the pin (as a string enum). That way you can also explicitly declare that the kernel shouldn't mess with the settings (e.g., `polarity = "keep";`, like you can do with the initial brightness). If you go that way, I would prefer if the "old" attributes where deprecated and only evaluated in case the new attribute is not available. As for how generic it should be: To me it doesn't seem like there's anything PHY-specific about it. I suppose where it might be confusing would be when you have a gpio-led, when that is already taken care of at the GPIO layer.
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index ad43e280930c..83233b30d7b0 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -25,6 +25,7 @@ #include <linux/bitfield.h> #include <linux/ctype.h> #include <linux/delay.h> +#include <linux/firmware.h> #include <linux/hwmon.h> #include <linux/marvell_phy.h> #include <linux/phy.h> @@ -50,6 +51,13 @@ enum { MV_PMA_21X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH = 0x6, MV_PMA_BOOT = 0xc050, MV_PMA_BOOT_FATAL = BIT(0), + 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, + MV_PMA_BOOT_APP_STARTED = BIT(4), + MV_PMA_BOOT_APP_LOADED = BIT(6), MV_PCS_BASE_T = 0x0000, MV_PCS_BASE_R = 0x1000, @@ -96,6 +104,12 @@ enum { MV_PCS_PORT_INFO_NPORTS_MASK = 0x0380, MV_PCS_PORT_INFO_NPORTS_SHIFT = 7, + /* Firmware downloading */ + MV_PCS_FW_ADDR_LOW = 0xd0f0, + MV_PCS_FW_ADDR_HIGH = 0xd0f1, + MV_PCS_FW_DATA = 0xd0f2, + MV_PCS_FW_CSUM = 0xd0f3, + /* SerDes reinitialization 88E21X0 */ MV_AN_21X0_SERDES_CTRL2 = 0x800f, MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS = BIT(13), @@ -156,6 +170,7 @@ struct mv3310_chip { const struct mv3310_mactype *mactypes; size_t n_mactypes; + const char *firmware_path; #ifdef CONFIG_HWMON int (*hwmon_read_temp_reg)(struct phy_device *phydev); @@ -506,6 +521,132 @@ static const struct sfp_upstream_ops mv3310_sfp_ops = { .module_insert = mv3310_sfp_insert, }; +struct mv3310_fw_hdr { + struct { + u32 size; + u32 addr; + u16 csum; + } __packed data; + + 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; + } + +unlock: + phy_unlock_mdio_bus(phydev); + return err; +} + +static int mv3310_load_fw(struct phy_device *phydev) +{ + const struct mv3310_chip *chip = to_mv3310_chip(phydev); + const struct firmware *fw; + struct mv3310_fw_hdr hdr; + const u8 *sect; + size_t i; + u16 csum; + int err; + + if (!chip->firmware_path) + return -EOPNOTSUPP; + + err = request_firmware(&fw, chip->firmware_path, &phydev->mdio.dev); + if (err) + return err; + + if (fw->size & 1) { + err = -EINVAL; + goto release; + } + + 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.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); + 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)); + if (err) + break; + + if (!hdr.next_hdr) + break; + + sect = fw->data + hdr.next_hdr; + } + +release: + release_firmware(fw); + return err; +} + static int mv3310_probe(struct phy_device *phydev) { const struct mv3310_chip *chip = to_mv3310_chip(phydev); @@ -527,6 +668,12 @@ static int mv3310_probe(struct phy_device *phydev) return -ENODEV; } + if ((ret & MV_PMA_BOOT_PRGS_MASK) == MV_PMA_BOOT_PRGS_WAIT) { + ret = mv3310_load_fw(phydev); + if (ret) + return ret; + } + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; @@ -1219,6 +1366,7 @@ static const struct mv3310_chip mv3310_type = { .mactypes = mv3310_mactypes, .n_mactypes = ARRAY_SIZE(mv3310_mactypes), + .firmware_path = "mrvl/x3310fw.hdr", #ifdef CONFIG_HWMON .hwmon_read_temp_reg = mv3310_hwmon_read_temp_reg, @@ -1489,4 +1637,5 @@ static struct mdio_device_id __maybe_unused mv3310_tbl[] = { }; MODULE_DEVICE_TABLE(mdio, mv3310_tbl); MODULE_DESCRIPTION("Marvell Alaska X/M multi-gigabit Ethernet PHY driver"); +MODULE_FIRMWARE("mrvl/x3310fw.hdr"); MODULE_LICENSE("GPL");
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. We have no choice but to bail out of the probe if firmware is not available, as the device does not have any built-in image on which to fall back. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/phy/marvell10g.c | 149 +++++++++++++++++++++++++++++++++++ 1 file changed, 149 insertions(+)