Re: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 14 November 2016 at 20:52, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Ard,
>
> I tested your solution on Marvell Armada 70x0, whose upstream process
> to OpenPlatformPkg is being finalized now. So far I was able to check
> generic XHCI IP (MdeModulePkg/Bus/Pci/XhciDxe)  and custom SD/MMC
> driver - they both work fine with following modification in your code:
>
> --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
> @@ -942,7 +942,7 @@ InitializePciIoProtocol (
>      Dev->ConfigSpace.Hdr.ClassCode[1] = PCI_CLASS_SERIAL_USB;
>      Dev->ConfigSpace.Hdr.ClassCode[2] = PCI_CLASS_SERIAL;
>      Dev->BarIndex = 0;
> -    Dev->BarSize = SIZE_2KB;
> +    Dev->BarSize = SIZE_16KB;
>      break;
>
>    case NonDiscoverableDeviceTypeAhci:
> @@ -958,7 +958,7 @@ InitializePciIoProtocol (
>      Dev->ConfigSpace.Hdr.ClassCode[1] = PCI_SUBCLASS_SD_HOST_CONTROLLER;
>      Dev->ConfigSpace.Hdr.ClassCode[2] = PCI_CLASS_SYSTEM_PERIPHERAL;
>      Dev->BarIndex = 0;
> -    Dev->BarSize = 0x100;
> +    Dev->BarSize = 0x300;
>      break;
>
> Any value lower than SIZE_16KB of BarSize resulted in XhcReadExtCapReg
> function failing due to reaching out of declared space. So happened
> with custom SD/MMC driver. Is it a problem to extend the space?
>

No, not at all. It was just a ballpark figure derived from a quick
glance at the PCI drivers.

> Did you consider extending RegisterNonDiscoverableDevice() function
> with possible custom BarSize value? I think enabling the users
> overriding the default value could be possible. What do you think?
>

Well, it depends on whether the PCI drivers will actually access that
space. The idea is to make the non-discoverable device description as
simple as possible.

> I also reviewed the code during debug and have no objections.
>

Thanks!

Note to reviewers: Marcin's patch series to add AHCI, XHCI and SDHCI
support went from

 10 files changed, 1583 insertions(+), 3 deletions(-)

to

 7 files changed, 271 insertions(+)

which sounds like an improvement to me.

Thanks,
Ard.