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

[Marcin Wojtas <mw@semihalf.com>]

Hi Ard,

2016-11-15 12:23 GMT+01:00 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.

Also Ahci should be extended to SIZE_8KB - according to the spec,
single controller can comprise up to 32 ports, which gives possible
0x1200 size.

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

If extending the size is not a problem, I'm also for leaving it as-is.

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

Right, it's way more simple, without code duplication.

Best regards,
Marcin