[edk2] PL180MciDxe problem with TC2

Message ID CAD0U-hJ33hjmBDekCTs7Kg-swM9U9H9BkGsfGiApUPaYgF4Mfg@mail.gmail.com
State New
Headers show

Commit Message

Ryan Harkin Feb. 29, 2016, 7:39 p.m.
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.

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(-)


   Handle = NULL;

-- 
2.5.0
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Feb. 29, 2016, 8:05 p.m. | #1
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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 29, 2016, 8:59 p.m. | #2
On 29 February 2016 at 21:05, Laszlo Ersek <lersek@redhat.com> wrote:
> 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 for the analysis

It appears that PL180MciDxe's dependency on gEfiCpuArchProtocolGuid
was transitively fulfilled by its dependency on TimerLib, which is
implemented by ArmArchTimerLib on TC2, and the patch removes it from
the depex

Simply replacing TRUE with gEfiCpuArchProtocolGuid in the Depex
section of PL180MciDxe.inf should do the trick.

As far as the Primecell ID is concerned, let's just whitelist whatever
TC2 exposes, even if in error.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryan Harkin March 1, 2016, 7:50 a.m. | #3
Thanks for jumping in!

On 29 February 2016 at 20:59, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 29 February 2016 at 21:05, Laszlo Ersek <lersek@redhat.com> wrote:

>> 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

>>>


^^ That only happens on a release build.  I suspect there's a there
bug too.  The IRQ exception shouldn't happen infinitely.  I think it
should appear once and never again.

But anyway, on a debug build, I see this once I've fixed the probe function:

Probing ID registers at 0x11C050FE0 for a PL180

IRQ Exception PC at 0xBF71FC58  CPSR 0x60000133 nZCveAifT_svc
/working/platforms/uefi/edk2/Build/ArmVExpress-CTA15-A7/DEBUG_GCC49/ARM/ArmPlatformPkg/Drivers/PL180MciDxe/PL180MciDxe/DEBUG/PL180MciDxe.dll
loaded at 0xBF71E000 (PE/COFF offset) 0x1C58 (ELF or Mach-O offset) 0x1A38
0x9303       STR    r3, [sp, #0xC]
  R0 0x00000001   R1 0x00000400   R2 0x00000800   R3 0x00000080
  R4 0xBF810391   R5 0xBF831000   R6 0x00000000   R7 0xB000021C
  R8 0x80000100   R9 0xB8000000  R10 0xBFFEC000  R11 0x00000000
 R12 0x00000000   SP 0xBFFFFB40   LR 0xBF71FC39   PC 0xBF71FC58
DFSR 0x00001E17 DFAR 0xE1D3C103 IFSR 0x00001236 IFAR 0xE9404847
 No function: write to 0xE1D3C103
 Instruction Access Flag fault on Page at 0xE9404847

ASSERT [ArmCpuDxe]
/working/platforms/uefi/edk2/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c(258):
((BOOLEAN)(0==1))



>>> 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 for the analysis

>

> It appears that PL180MciDxe's dependency on gEfiCpuArchProtocolGuid

> was transitively fulfilled by its dependency on TimerLib, which is

> implemented by ArmArchTimerLib on TC2, and the patch removes it from

> the depex

>


It looks like both of you are right:  If I only revert the Depex part
of the commit, all works.

+[Depex]
+  gEfiCpuArchProtocolGuid

If I only revert the IoLib part, then the board still dies with the same ASSERT.


> Simply replacing TRUE with gEfiCpuArchProtocolGuid in the Depex

> section of PL180MciDxe.inf should do the trick.

>


Yes, that works, thanks.  It looks like a better fix than adding it back into


> As far as the Primecell ID is concerned, let's just whitelist whatever

> TC2 exposes, even if in error.


You mean rather than mask the "stuck bit", specifically check if the
register is 0 or 2?  (Where 2 is added as a #define to the header).

I intend to also add a patch that prints debug if the probed registers
don't match, sound reasonable?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 1, 2016, 8:39 a.m. | #4
On 1 March 2016 at 08:50, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> Thanks for jumping in!

>

> On 29 February 2016 at 20:59, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> On 29 February 2016 at 21:05, Laszlo Ersek <lersek@redhat.com> wrote:

>>> 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

>>>>

>

> ^^ That only happens on a release build.  I suspect there's a there

> bug too.  The IRQ exception shouldn't happen infinitely.  I think it

> should appear once and never again.

>


My suspicion is that this driver depends on the CPU arch protocol so
that interrupts are disabled at the GIC for everything except the
timer before it starts poking the MMC hardware.

> But anyway, on a debug build, I see this once I've fixed the probe function:

>

> Probing ID registers at 0x11C050FE0 for a PL180

>

> IRQ Exception PC at 0xBF71FC58  CPSR 0x60000133 nZCveAifT_svc

> /working/platforms/uefi/edk2/Build/ArmVExpress-CTA15-A7/DEBUG_GCC49/ARM/ArmPlatformPkg/Drivers/PL180MciDxe/PL180MciDxe/DEBUG/PL180MciDxe.dll

> loaded at 0xBF71E000 (PE/COFF offset) 0x1C58 (ELF or Mach-O offset) 0x1A38

> 0x9303       STR    r3, [sp, #0xC]

>   R0 0x00000001   R1 0x00000400   R2 0x00000800   R3 0x00000080

>   R4 0xBF810391   R5 0xBF831000   R6 0x00000000   R7 0xB000021C

>   R8 0x80000100   R9 0xB8000000  R10 0xBFFEC000  R11 0x00000000

>  R12 0x00000000   SP 0xBFFFFB40   LR 0xBF71FC39   PC 0xBF71FC58

> DFSR 0x00001E17 DFAR 0xE1D3C103 IFSR 0x00001236 IFAR 0xE9404847

>  No function: write to 0xE1D3C103

>  Instruction Access Flag fault on Page at 0xE9404847

>

> ASSERT [ArmCpuDxe]

> /working/platforms/uefi/edk2/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c(258):

> ((BOOLEAN)(0==1))

>


Here, it hits the default exception handler because no handler has
been registered for whichever interrupt the MMC hardware raises. The
same happens in a RELEASE build, but there the output is less verbose
and the ASSERT() is compiled out, so execution proceeds.

>

>

>>>> 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 for the analysis

>>

>> It appears that PL180MciDxe's dependency on gEfiCpuArchProtocolGuid

>> was transitively fulfilled by its dependency on TimerLib, which is

>> implemented by ArmArchTimerLib on TC2, and the patch removes it from

>> the depex

>>

>

> It looks like both of you are right:  If I only revert the Depex part

> of the commit, all works.

>

> +[Depex]

> +  gEfiCpuArchProtocolGuid

>

> If I only revert the IoLib part, then the board still dies with the same ASSERT.

>

>

>> Simply replacing TRUE with gEfiCpuArchProtocolGuid in the Depex

>> section of PL180MciDxe.inf should do the trick.

>>

>

> Yes, that works, thanks.  It looks like a better fix than adding it back into

>


ArmArchTimerLib.inf? I agree

>

>> As far as the Primecell ID is concerned, let's just whitelist whatever

>> TC2 exposes, even if in error.

>

> You mean rather than mask the "stuck bit", specifically check if the

> register is 0 or 2?  (Where 2 is added as a #define to the header).

>


I don't care deeply either way, but supporting the documented and the
actual ID explicitly seems more correct, unless the difference is in
the revision field?

> I intend to also add a patch that prints debug if the probed registers

> don't match, sound reasonable?


Sure, that makes sense. On Foundation model, I already see a
notification related to the probing of the (non-existent, in that
case) Primecell ID registers, but it makes sense to have the same in
UEFI's own DEBUG output.

-- 
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm March 1, 2016, 9:36 a.m. | #5
On Tue, Mar 01, 2016 at 09:39:18AM +0100, Ard Biesheuvel wrote:
> >> As far as the Primecell ID is concerned, let's just whitelist whatever

> >> TC2 exposes, even if in error.

> >

> > You mean rather than mask the "stuck bit", specifically check if the

> > register is 0 or 2?  (Where 2 is added as a #define to the header).

> 

> I don't care deeply either way, but supporting the documented and the

> actual ID explicitly seems more correct, unless the difference is in

> the revision field?


So ... PeriphID3 holds the "Configuration" bits, so in any case, they
would be irrelevant for identification.

We really should macroise this up a bit better, like Linux does:
/* Some drivers don't use the struct amba_device */
#define AMBA_CONFIG_BITS(a) (((a) >> 24) & 0xff)
#define AMBA_REV_BITS(a) (((a) >> 20) & 0x0f)
#define AMBA_MANF_BITS(a) (((a) >> 12) & 0xff)
#define AMBA_PART_BITS(a) ((a) & 0xfff)

#define amba_config(d)  AMBA_CONFIG_BITS((d)->periphid)
#define amba_rev(d)     AMBA_REV_BITS((d)->periphid)
#define amba_manf(d)    AMBA_MANF_BITS((d)->periphid)
#define amba_part(d)    AMBA_PART_BITS((d)->periphid)

(after proper extraction of the 32-bit value from the 4 registers of
course)

Amusingly, the Linux driver ignores pretty much everything, and
accepts any primecell with ARM as designer.
Opens up for cute devicetree hacks, I guess...

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 1, 2016, 9:40 a.m. | #6
On 1 March 2016 at 10:36, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Mar 01, 2016 at 09:39:18AM +0100, Ard Biesheuvel wrote:

>> >> As far as the Primecell ID is concerned, let's just whitelist whatever

>> >> TC2 exposes, even if in error.

>> >

>> > You mean rather than mask the "stuck bit", specifically check if the

>> > register is 0 or 2?  (Where 2 is added as a #define to the header).

>>

>> I don't care deeply either way, but supporting the documented and the

>> actual ID explicitly seems more correct, unless the difference is in

>> the revision field?

>

> So ... PeriphID3 holds the "Configuration" bits, so in any case, they

> would be irrelevant for identification.

>


Well, the problem is not what they are called, it is what the
refmanual of the IP block explicitly states its value can be expected
to be.

> We really should macroise this up a bit better, like Linux does:

> /* Some drivers don't use the struct amba_device */

> #define AMBA_CONFIG_BITS(a) (((a) >> 24) & 0xff)

> #define AMBA_REV_BITS(a) (((a) >> 20) & 0x0f)

> #define AMBA_MANF_BITS(a) (((a) >> 12) & 0xff)

> #define AMBA_PART_BITS(a) ((a) & 0xfff)

>

> #define amba_config(d)  AMBA_CONFIG_BITS((d)->periphid)

> #define amba_rev(d)     AMBA_REV_BITS((d)->periphid)

> #define amba_manf(d)    AMBA_MANF_BITS((d)->periphid)

> #define amba_part(d)    AMBA_PART_BITS((d)->periphid)

>

> (after proper extraction of the 32-bit value from the 4 registers of

> course)

>

> Amusingly, the Linux driver ignores pretty much everything, and

> accepts any primecell with ARM as designer.

> Opens up for cute devicetree hacks, I guess...

>


As I said, I don't care deeply. If part and manufacturer is
sufficient, then let's drop the other bits
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm March 1, 2016, 9:52 a.m. | #7
On Tue, Mar 01, 2016 at 10:40:53AM +0100, Ard Biesheuvel wrote:
> On 1 March 2016 at 10:36, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Tue, Mar 01, 2016 at 09:39:18AM +0100, Ard Biesheuvel wrote:

> >> >> As far as the Primecell ID is concerned, let's just whitelist whatever

> >> >> TC2 exposes, even if in error.

> >> >

> >> > You mean rather than mask the "stuck bit", specifically check if the

> >> > register is 0 or 2?  (Where 2 is added as a #define to the header).

> >>

> >> I don't care deeply either way, but supporting the documented and the

> >> actual ID explicitly seems more correct, unless the difference is in

> >> the revision field?

> >

> > So ... PeriphID3 holds the "Configuration" bits, so in any case, they

> > would be irrelevant for identification.

> 

> Well, the problem is not what they are called, it is what the

> refmanual of the IP block explicitly states its value can be expected

> to be.


Yes, and the public documentation found in the obvious place is not
for the part actually used on the platform, or at least not for the
same revision. More recent ARM documents tend to be less confusing on
which fields may be revision-sensitive or not.

The point is that the "configuration" bits have no bearing on which
component it is, they merely indicate implementation-time decisions
(things like configurable fifo depths, inclusion of randomly bolted
onto the side GPIO block, ...). These are not even necessarily
software-visible, and the lack of interest on behalf of the Linux
driver suggests this is the case here.

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryan Harkin March 1, 2016, 11:03 a.m. | #8
On 1 March 2016 at 09:52, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Mar 01, 2016 at 10:40:53AM +0100, Ard Biesheuvel wrote:

>> On 1 March 2016 at 10:36, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>> > On Tue, Mar 01, 2016 at 09:39:18AM +0100, Ard Biesheuvel wrote:

>> >> >> As far as the Primecell ID is concerned, let's just whitelist whatever

>> >> >> TC2 exposes, even if in error.

>> >> >

>> >> > You mean rather than mask the "stuck bit", specifically check if the

>> >> > register is 0 or 2?  (Where 2 is added as a #define to the header).

>> >>

>> >> I don't care deeply either way, but supporting the documented and the

>> >> actual ID explicitly seems more correct, unless the difference is in

>> >> the revision field?

>> >

>> > So ... PeriphID3 holds the "Configuration" bits, so in any case, they

>> > would be irrelevant for identification.

>>

>> Well, the problem is not what they are called, it is what the

>> refmanual of the IP block explicitly states its value can be expected

>> to be.

>

> Yes, and the public documentation found in the obvious place is not

> for the part actually used on the platform, or at least not for the

> same revision. More recent ARM documents tend to be less confusing on

> which fields may be revision-sensitive or not.

>


Is there a version available publicly for the revision found on
Versatile Express?  A link would be handy for future reference.  I'm
looking at this doc:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html

... which is assume is for pre r1p0, which is the version that is
claimed to be in the IOFPGA on the Versatile Express motherboard doc:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html


> The point is that the "configuration" bits have no bearing on which

> component it is, they merely indicate implementation-time decisions

> (things like configurable fifo depths, inclusion of randomly bolted

> onto the side GPIO block, ...). These are not even necessarily

> software-visible, and the lack of interest on behalf of the Linux

> driver suggests this is the case here.

>


I'm happy to fix it "properly", but perhaps properly should be via
functions, not macros?

Although, actually, isn't the PrimeCell identification mechanism
common across all Prime Cells and if so, perhaps a small library is in
order?

So I propose a series of patches to get things working:
- depex fix to get TC2 working properly
- add debug to indicate PL180 probe failure
- simple fix to the probe, ignoring MCI_PERIPH_ID_REG3

Then another series:
- create a small PrimeCell library for common functions and only add
in the identification code
- a patch per PrimeCell (PL180, PL111, PL061, ...) to use the library

For the library, we could have two functions:

PrimeCellDesigner(baseaddr)
PrimeCellPartNumber(baseaddr)

Do we want to return enums of known designers and part numbers or are
#defines preferable?

Or perhaps just one TRUE/FALSE function IsPrimeCell(baseaddr, enum id)
is better?

Thoughts?

Cheers,
Ryan.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 1, 2016, 11:08 a.m. | #9
On 1 March 2016 at 12:03, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 1 March 2016 at 09:52, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>> On Tue, Mar 01, 2016 at 10:40:53AM +0100, Ard Biesheuvel wrote:

>>> On 1 March 2016 at 10:36, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>>> > On Tue, Mar 01, 2016 at 09:39:18AM +0100, Ard Biesheuvel wrote:

>>> >> >> As far as the Primecell ID is concerned, let's just whitelist whatever

>>> >> >> TC2 exposes, even if in error.

>>> >> >

>>> >> > You mean rather than mask the "stuck bit", specifically check if the

>>> >> > register is 0 or 2?  (Where 2 is added as a #define to the header).

>>> >>

>>> >> I don't care deeply either way, but supporting the documented and the

>>> >> actual ID explicitly seems more correct, unless the difference is in

>>> >> the revision field?

>>> >

>>> > So ... PeriphID3 holds the "Configuration" bits, so in any case, they

>>> > would be irrelevant for identification.

>>>

>>> Well, the problem is not what they are called, it is what the

>>> refmanual of the IP block explicitly states its value can be expected

>>> to be.

>>

>> Yes, and the public documentation found in the obvious place is not

>> for the part actually used on the platform, or at least not for the

>> same revision. More recent ARM documents tend to be less confusing on

>> which fields may be revision-sensitive or not.

>>

>

> Is there a version available publicly for the revision found on

> Versatile Express?  A link would be handy for future reference.  I'm

> looking at this doc:

>

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html

>

> ... which is assume is for pre r1p0, which is the version that is

> claimed to be in the IOFPGA on the Versatile Express motherboard doc:

>

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html

>

>

>> The point is that the "configuration" bits have no bearing on which

>> component it is, they merely indicate implementation-time decisions

>> (things like configurable fifo depths, inclusion of randomly bolted

>> onto the side GPIO block, ...). These are not even necessarily

>> software-visible, and the lack of interest on behalf of the Linux

>> driver suggests this is the case here.

>>

>

> I'm happy to fix it "properly", but perhaps properly should be via

> functions, not macros?

>

> Although, actually, isn't the PrimeCell identification mechanism

> common across all Prime Cells and if so, perhaps a small library is in

> order?

>

> So I propose a series of patches to get things working:

> - depex fix to get TC2 working properly

> - add debug to indicate PL180 probe failure

> - simple fix to the probe, ignoring MCI_PERIPH_ID_REG3

>


Ack

> Then another series:

> - create a small PrimeCell library for common functions and only add

> in the identification code

> - a patch per PrimeCell (PL180, PL111, PL061, ...) to use the library

>

> For the library, we could have two functions:

>

> PrimeCellDesigner(baseaddr)

> PrimeCellPartNumber(baseaddr)

>

> Do we want to return enums of known designers and part numbers or are

> #defines preferable?

>

> Or perhaps just one TRUE/FALSE function IsPrimeCell(baseaddr, enum id)

> is better?

>


Actually, it makes sense to defer the second part until we have some
progress on the 'platform bus' discussion we had last week (between
Charles, Leif, Eugene Cohen and myself, among others). The problem
here is that many of these drivers should be UEFI drivers, not DXE
drivers, and should be instantiated on demand.

This means there should be some kind of Primecell bus, which is seeded
statically with a list of base addresses, and then proceeds to
instantiate driver instances for each primecell id that can be
supported by any of the UEFI drivers that claims to support it.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm March 1, 2016, 11:18 a.m. | #10
On Tue, Mar 01, 2016 at 11:03:18AM +0000, Ryan Harkin wrote:
> > Yes, and the public documentation found in the obvious place is not

> > for the part actually used on the platform, or at least not for the

> > same revision. More recent ARM documents tend to be less confusing on

> > which fields may be revision-sensitive or not.

> 

> Is there a version available publicly for the revision found on

> Versatile Express?  A link would be handy for future reference.  I'm

> looking at this doc:

> 

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html

> 

> ... which is assume is for pre r1p0, which is the version that is

> claimed to be in the IOFPGA on the Versatile Express motherboard doc:

> 

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html


I intend to go on a minor archeological expedition when I'm working in
the office tomorrow.

> > The point is that the "configuration" bits have no bearing on which

> > component it is, they merely indicate implementation-time decisions

> > (things like configurable fifo depths, inclusion of randomly bolted

> > onto the side GPIO block, ...). These are not even necessarily

> > software-visible, and the lack of interest on behalf of the Linux

> > driver suggests this is the case here.

> 

> I'm happy to fix it "properly", but perhaps properly should be via

> functions, not macros?

> 

> Although, actually, isn't the PrimeCell identification mechanism

> common across all Prime Cells and if so, perhaps a small library is in

> order?


Good call. I was thinking in Linux mode.

> So I propose a series of patches to get things working:

> - depex fix to get TC2 working properly

> - add debug to indicate PL180 probe failure

> - simple fix to the probe, ignoring MCI_PERIPH_ID_REG3


Agreed.

> Then another series:

> - create a small PrimeCell library for common functions and only add

> in the identification code


Sounds lovely.

> - a patch per PrimeCell (PL180, PL111, PL061, ...) to use the library


Taking Ard's point into consideration, we may or may not need this -
but the first part would certainly be real handy regardless.

> For the library, we could have two functions:

> 

> PrimeCellDesigner(baseaddr)

> PrimeCellPartNumber(baseaddr)


I think we'd want the Configuration and Revision field as well (not
really any extra effort).

> Do we want to return enums of known designers and part numbers or are

> #defines preferable?


Umm, *shrug*?

> Or perhaps just one TRUE/FALSE function IsPrimeCell(baseaddr, enum id)

> is better?


That _would_ preclude the Configuration/Revision fields, or require
always passing pointers even when the driver doesn't care - so my
preference would be against.

Thanks!

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryan Harkin March 1, 2016, 12:08 p.m. | #11
On 1 March 2016 at 11:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Mar 01, 2016 at 11:03:18AM +0000, Ryan Harkin wrote:

>> > Yes, and the public documentation found in the obvious place is not

>> > for the part actually used on the platform, or at least not for the

>> > same revision. More recent ARM documents tend to be less confusing on

>> > which fields may be revision-sensitive or not.

>>

>> Is there a version available publicly for the revision found on

>> Versatile Express?  A link would be handy for future reference.  I'm

>> looking at this doc:

>>

>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html

>>

>> ... which is assume is for pre r1p0, which is the version that is

>> claimed to be in the IOFPGA on the Versatile Express motherboard doc:

>>

>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0172a/index.html

>

> I intend to go on a minor archeological expedition when I'm working in

> the office tomorrow.

>

>> > The point is that the "configuration" bits have no bearing on which

>> > component it is, they merely indicate implementation-time decisions

>> > (things like configurable fifo depths, inclusion of randomly bolted

>> > onto the side GPIO block, ...). These are not even necessarily

>> > software-visible, and the lack of interest on behalf of the Linux

>> > driver suggests this is the case here.

>>

>> I'm happy to fix it "properly", but perhaps properly should be via

>> functions, not macros?

>>

>> Although, actually, isn't the PrimeCell identification mechanism

>> common across all Prime Cells and if so, perhaps a small library is in

>> order?

>

> Good call. I was thinking in Linux mode.

>

>> So I propose a series of patches to get things working:

>> - depex fix to get TC2 working properly

>> - add debug to indicate PL180 probe failure

>> - simple fix to the probe, ignoring MCI_PERIPH_ID_REG3

>

> Agreed.

>


Good, I'll do that today.


>> Then another series:

>> - create a small PrimeCell library for common functions and only add

>> in the identification code

>

> Sounds lovely.

>

>> - a patch per PrimeCell (PL180, PL111, PL061, ...) to use the library

>

> Taking Ard's point into consideration, we may or may not need this -

> but the first part would certainly be real handy regardless.

>

>> For the library, we could have two functions:

>>

>> PrimeCellDesigner(baseaddr)

>> PrimeCellPartNumber(baseaddr)

>

> I think we'd want the Configuration and Revision field as well (not

> really any extra effort).

>

>> Do we want to return enums of known designers and part numbers or are

>> #defines preferable?

>

> Umm, *shrug*?

>

>> Or perhaps just one TRUE/FALSE function IsPrimeCell(baseaddr, enum id)

>> is better?

>

> That _would_ preclude the Configuration/Revision fields, or require

> always passing pointers even when the driver doesn't care - so my

> preference would be against.

>


If there is going to be a bus implementation, which sounds like a good
plan, I reckon it would be better for a PrimeCell library to be part
of that series.  I expect the requirements are likely to shift during
implementation.

So I'll hold fire till I/we know more.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch hide | download patch | download mbox

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"));
+  }