On 02/29/16 20:39, Ryan Harkin wrote:
> Hi Ard/Leif/anyone who cares,
>
> So I was trying to work out who broke MMC support in TC2 in the
> upstream EDK2 tree. It was difficult because the tree is borked on
> TC2 in so many interesting ways throughout history, but I eventually
> bisected down to this patch:
>
> 300fc77 2015-08-25 ArmPlatformPkg/PL180MciDxe: check PrimeCell ID
> before initializing [Ard Biesheuvel]
>
> Basically, TC2 reads 0x02 for MCI_PERIPH_ID_REG3, when, according to
> the spec, the register is supposed to read 0x00 in all cases. So the
> driver doesn't probe and is never initialised. I guess this is an
> FPGA bug in TC2? It's probably known about, but not to me ;-)
>
> Anyway, how to fix it??
>
> We could mask off the "stuck" bit, we could not check ID_REG3, there
> are other things we could do.
>
> I decided to mask off the bit rather than discard the register check
> in my patch below, just to get things working
>
> But would you like to do?
>
> For extra point.... this was extra fun to track down due to other
> problems. TC2 stopped booting since this patch was submitted
>
> d340ef7 2014-08-26 ArmPkg/ArmArchTimerLib: Remove non required
> [depex] and IoLib [Olivier Martin]
>
> I've always carried a revert patch in my tree because I was previously
> told I was wrong and that it wasn't a problem, even though it clearly
> is. TC2 is spewing out a constant stream of this message:
>
> IRQ Exception PC at 0xBFB74C20 CPSR 0x60000133
>
> It wasn't fixed until Ard's patch that broke MMC support. Ugh!
>
> I'm suspecting that the MMC support has a dependency on IoLib - for
> that is the part of the patch that broke TC2 in the first place. But
> I have yet to investigate that problem; I don't even know what IoLib
> is.
IoLib is a library class that lets you massage IO ports and MMIO registers.
MdePkg/Include/Library/IoLib.h
The patch you quoted does two things: it removes ArmArchTimerLib's
build-time dependency on the IoLib class, and it removes the runtime
(dispatch) dependency on EFI_CPU_ARCH_PROTOCOL of any module that is
linked against ArmArchTimerLib (unless that module has the same
dependency due to another library instance it links against, or due to
its own explicit [depex] section).
Removing the library class dependency could introduce such a problem
only if the actual library instance used for that dependency had a
constructor function that is henceforth no longer called, and this
function changed something related to interrupts. Very unlikely.
Removing the DXE dispatch dependency on EFI_CPU_ARCH_PROTOCOL is the
likely culprit, in my opinion. The driver that provides said
architectural protocol probably massages interrupt configuration on the
CPU or the GIC in its entry point function in such a way that
ArmArchTimerLib actually silently depends on, without explicitly calling
EFI_CPU_ARCH_PROTOCOL member functions. By removing the depex, the DXE
core may have reordered another driver (that links against
ArmArchTimerLib) versus the driver providing EFI_CPU_ARCH_PROTOCOL --
for which reason the timerlib functions may now run without the
necessary interrupt setup.
Instances of the TimerLib class have always been finicky. For example,
in OvmfPkg we have three instances (for various module types & firmware
phases). The two instances that get linked into early module types (SEC,
and PEI_CORE, PEIM, DXE_CORE) massage chipset registers directly,
because that was the only robust way to make sure that whichever of
these module (types) needed the ACPI timer could actually utilize it.
Through these library instances, every such "early" module (that needs
TimerLib) looks at the chipset registers, and sets the needed bits if
they are not in place yet.
Thanks
Laszlo
> So until this 2nd problem is fixed, I really don't want to submit a
> fix to the PL180 problem or I'll have a dead TC2 port again :-/
>
> Cheers,
> Ryan.
>
> ---
> ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.h | 3 +++
> ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c | 26 +++++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.h
> b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.h
> index ce38a9e..8d36456 100644
> --- a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.h
> +++ b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.h
> @@ -64,11 +64,14 @@
> #define MCI_PERIPH_ID1 0x11
> #define MCI_PERIPH_ID2 0x04
> #define MCI_PERIPH_ID3 0x00
> +#define MCI_PERIPH_ID3_TC2 0x02
> #define MCI_PCELL_ID0 0x0D
> #define MCI_PCELL_ID1 0xF0
> #define MCI_PCELL_ID2 0x05
> #define MCI_PCELL_ID3 0xB1
>
> +#define MCI_PERIPH_ID3_MASK (~0x02)
> +
> #define MCI_POWER_OFF 0
> #define MCI_POWER_UP BIT1
> #define MCI_POWER_ON (BIT1 | BIT0)
> diff --git a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
> b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
> index 688cd8a..8ae88b3 100644
> --- a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
> +++ b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
> @@ -520,6 +520,14 @@ PL180MciDxeInitialize (
> {
> EFI_STATUS Status;
> EFI_HANDLE Handle;
> + UINT8 r1 = MmioRead8 (MCI_PERIPH_ID_REG0);
> + UINT8 r2 = MmioRead8 (MCI_PERIPH_ID_REG1);
> + UINT8 r3 = MmioRead8 (MCI_PERIPH_ID_REG2);
> + UINT8 r4 = MmioRead8 (MCI_PERIPH_ID_REG3);
> + UINT8 r5 = MmioRead8 (MCI_PCELL_ID_REG0);
> + UINT8 r6 = MmioRead8 (MCI_PCELL_ID_REG1);
> + UINT8 r7 = MmioRead8 (MCI_PCELL_ID_REG2);
> + UINT8 r8 = MmioRead8 (MCI_PCELL_ID_REG3);
>
> DEBUG ((EFI_D_WARN, "Probing ID registers at 0x%lx for a PL180\n",
> MCI_PERIPH_ID_REG0));
> @@ -528,14 +536,30 @@ PL180MciDxeInitialize (
> if (MmioRead8 (MCI_PERIPH_ID_REG0) != MCI_PERIPH_ID0 ||
> MmioRead8 (MCI_PERIPH_ID_REG1) != MCI_PERIPH_ID1 ||
> MmioRead8 (MCI_PERIPH_ID_REG2) != MCI_PERIPH_ID2 ||
> - MmioRead8 (MCI_PERIPH_ID_REG3) != MCI_PERIPH_ID3 ||
> + (MmioRead8 (MCI_PERIPH_ID_REG3) & MCI_PERIPH_ID3_MASK) !=
> MCI_PERIPH_ID3 ||
> MmioRead8 (MCI_PCELL_ID_REG0) != MCI_PCELL_ID0 ||
> MmioRead8 (MCI_PCELL_ID_REG1) != MCI_PCELL_ID1 ||
> MmioRead8 (MCI_PCELL_ID_REG2) != MCI_PCELL_ID2 ||
> MmioRead8 (MCI_PCELL_ID_REG3) != MCI_PCELL_ID3) {
>
> + DEBUG ((EFI_D_ERROR, "PL180MciDxeInitialize(): Failed to probe
> PL180\n"));
> + DEBUG ((EFI_D_ERROR, "PL180MciDxeInitialize(): want:
> %x,%x,%x,%x,%x,%x,%x,%x\n",
> + MCI_PERIPH_ID0,
> + MCI_PERIPH_ID1,
> + MCI_PERIPH_ID2,
> + MCI_PERIPH_ID3,
> + MCI_PCELL_ID0,
> + MCI_PCELL_ID1,
> + MCI_PCELL_ID2,
> + MCI_PCELL_ID3
> + ));
> +
> + DEBUG ((EFI_D_ERROR, "PL180MciDxeInitialize(): read:
> %x,%x,%x,%x,%x,%x,%x,%x\n", r1, r2, r3, r4, r5, r6, r7, r8));
> return EFI_NOT_FOUND;
> }
> + else {
> + DEBUG ((EFI_D_ERROR, "PL180MciDxeInitialize(): Probe succeeded\n"));
> + }
>
> Handle = NULL;
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
