RFC - [intel] Wait until reset is completed

[dagmcr]

Refactor intel_reset function and make it wait until reset is completed
by checking STATUS.PF_RST_DONE and EEC.Auto_RD bits are set to 1b.

Signed-off-by: Daniel Gomez [email protected]

[NiKiZe]

Are these not lining up intentional?

[dagmcr]

Sorry, I'll update the patch. I wanted to lining them up and then I did the opposite.

[dagmcr]

Hi ipxe team,

We had problems with the ipxe on an i210 network card because of the reset mechanism being used. We've noticed there was some workaround in the code that didn't match with the datasheet, so we have replaced it based on the intel reference driver and manual.

According to the reference driver, the reset mechanism should wait at least 3 ms (code waits 20 ms but we've kept that) and do a busy wait on CTRL.RST and EEC.Auto_RD bits. After that, the Software Reset is done and the link is up.

If you think we should keep the 'original' code path and only do this for the i210 (or specific families) it's okay, we can revert/add that in the patch. But we haven't been able to see the current reset mechanism in the intel reference driver or linux kernel.

Any feedback is very much appreciated.

Thanks

[mcb30]

This patch appears to destroy the generic reset mechanism that works on a variety of Intel NICs (including workarounds for specific models), and replaces it with a mechanism that works only on the single piece of hardware that you are using. This is obviously not something that could be merged.

Please submit a new PR that does not deliberately break support for existing hardware.

[mcb30]

@dagmcr I took a quick look around this issue. The Linux driver seems to ignore STATUS.PF_RST_DONE, but does poll for EEC.AUTO_RD. It's very plausible that STATUS.PF_RST_DONE will always be set after our existing 20ms delay, but that the EEPROM read (which is a slow serial device read) may take longer.

The EEC register is present at offset 0x00010 on all relevant cards, as far as I can tell. (The 0x12010 offset is specific to the i210, but even there the register is aliased to 0x00010.)

The problem is that the EEC.AUTO_RD bit is not present on the very earliest models: for the original e1000, EEC bit 9 is the EEPROM size bit.

One possible approach would be to add a per-PCI device ID flag (similar to INTEL_I219, INTEL_NO_PHY_RST, etc) to indicate the models that do not have the AUTO_RD bit present. This would require you to identify the relevant PCI IDs, which is not always easy to do.

A cleaner approach would be to find something else that could be done with the EEC register to determine whether or not the AUTO_RD bit is present. For example: the original e1000 (with no AUTO_RD) also defines EEC bits 31:14 as "reserved, will read as zero", but on the i210 and e1000e (which do have AUTO_RD) we are guaranteed that bits 16:15 cannot both be 0. Unfortunately this doesn't quite work: the igb (which also does have AUTO_RD) can have bits 16:15 read as both 0. But this may give you some ideas on where to start looking.

[dagmcr]

@mcb30, thanks for your comments. I'm sorry for the initial confusion here, as what I wanted was to open a discussion about the intel reset mechanism so, I should have renamed the patch as RFC. Apologies for that.

We have an I210 network card that has some issues when booting from (uefi) ipxe. The reset appears to be broken and seems to be related to the comment/workaround in the code. The card is not able to link up at the speed of 1000 Mbps and when the dhcp client starts, it does not send any package out. So, what we tried to do was to 'restore' the mechanism we saw in the intel reference driver which btw you had it at some point the ipxe sources: 945e428. I know it's already +10y but I'm curious, why did you move to a unified driver? I guess it was to clean it up and to ease maintenance.

According to the intel manual for the I210, 'Software Reset should wait at least 3ms before accessing any register and then verify EEC.Auto_RD and STATUS.PF_RST_DONE is set to 1b'. Also, when performing a Software Reset
'most registers (receive, transmit, interrupt, statistics) and state machines are set to their power-on reset values'.

Following intel's reference driver code, this busy-wait (at least for the EEC.Auto_RD) is required to be done for several intel network families. This is performed in 'e1000_ich8lan.c' (82562*, 8256*, 8257*, I217*, I218*), 'e1000_82575.c' (8257*, 8258*, I350) and 'e1000_80003es2lan.c' (80003ES2LAN).

The ipxe intel reset code seems to perform a Software Reset twice (except when skipping) and then, after the first one, it individually handles some bits (SLU, LRST, FRCSPD and FRCDPLX). So, is it needed to execute a Software Reset, configure these registers and execute another Software Reset + PHY Reset (again)?

We have 'simplified' this (wrongly perhaps) and perform only one Software Reset and wait for the EEC.Auto_RD and STATUS.PF_RST_DONE bits. After that, the reset mechanism seems to be solved (for the I210 at least), so the network card links up properly at 1000 Mbps and the dhcp client starts some network traffic.

After all this, I managed to test vanilla ipxe and vanilla ipxe + our patch in several boards:

• QT5222 (custom board mounting an I210 NIC with 2 ports): Only works with our custom patch.

Note: EEprom 3.25 version.

• Intel eth SERVER ADPTR I210-T1 board: works with vanilla ipxe and vanilla ipxe + custom patch.

Note: EEprom 3.25 version.

• Intel PRO-1000 PT Server Adapter - PCIe: none of the version works.
• Intel Ethernet Server Adapter I2350 - T2: none of the version works.

It's very plausible that STATUS.PF_RST_DONE will always be set after our existing 20ms delay, but that the EEPROM read (which is a slow serial device read) may take longer.

When I did the tests, the PF_RST_DONE was always set after 20ms but not the Auto_RD. So, we should probably try to wait for this when required.

The EEC register is present at offset 0x00010 on all relevant cards, as far as I can tell. (The 0x12010 offset is specific to the i210, but even there the register is aliased to 0x00010.)

That might explained why I couldn't make it work (read properly) on the other NICs and the Software Reset was timeout.

The problem is that the EEC.AUTO_RD bit is not present on the very earliest models: for the original e1000, EEC bit 9 is the EEPROM size bit.

That's okay, I guess we can make the busy wait only on the boards that apply.

One possible approach would be to add a per-PCI device ID flag (similar to INTEL_I219, INTEL_NO_PHY_RST, etc) to indicate the models that do not have the AUTO_RD bit present. This would require you to identify the relevant PCI IDs, which is not always easy to do.

Thanks for the tip here. I'll take a look.

A cleaner approach would be to find something else that could be done with the EEC register to determine whether or not the AUTO_RD bit is present...

I agree. If we can verify that dynamically we can make the code a bit cleaner.

Thanks a lot for your feedback!

[mcb30]

We have an I210 network card that has some issues when booting from (uefi) ipxe. The reset appears to be broken and seems to be related to the comment/workaround in the code. The card is not able to link up at the speed of 1000 Mbps and when the dhcp client starts, it does not send any package out. So, what we tried to do was to 'restore' the mechanism we saw in the intel reference driver which btw you had it at some point the ipxe sources: 945e428. I know it's already +10y but I'm curious, why did you move to a unified driver? I guess it was to clean it up and to ease maintenance.

The primary motivation was to reduce the number of lines of code for the Intel driver by 97%, both to reduce the maintenance burden and to shrink the binary size.

One possible approach would be to add a per-PCI device ID flag (similar to INTEL_I219, INTEL_NO_PHY_RST, etc) to indicate the models that do not have the AUTO_RD bit present. This would require you to identify the relevant PCI IDs, which is not always easy to do.

Thanks for the tip here. I'll take a look.

A cleaner approach would be to find something else that could be done with the EEC register to determine whether or not the AUTO_RD bit is present...

I agree. If we can verify that dynamically we can make the code a bit cleaner.

Thanks for taking a look, and good luck!