Any comments about this patch?
On 2018/3/2 14:36, Ming Huang wrote: > > On 2018/2/27 18:01, Ni, Ruiyu wrote: >> On 2/27/2018 5:25 PM, Ming Huang wrote: >>> >>> On 2018/2/27 13:25, Ni, Ruiyu wrote: >>>> I don't prefer to add PCD, unless we cannot find: >>>> 1. spec content to describe the max/min blocks >>> There is no spec about the max/min blocks in my mind. I had checked this in >>> several pdf document like >>> Universal Serial Bus Mass Storage Class Specification Overview, >>> Universal Serial Bus Mass Storage Specification For Bootability, >>> Universal Serial Bus Mass Storage Class Bulk-Only Transport. >>>> 2. error handling when the blocks number is bigger than HW expects. >>> Where should error handling add to? Error handing can't add to HW >>> (end-point device), >>> because HW is not in our control scope. >> I mean maybe spec describes an error status could be returned from HW when >> using 128. So that we can use 64, 32, and smaller value until HW is happy. >> >> I am curious how the other USB storage drivers handle this. >> PCD is a static way. Dynamic way is more preferred. >> > When using 128, after waiting 5x5(USB_BOOT_COMMAND_RETRY=5, > USB_BOOT_GENERAL_CMD_TIMEOUT=5) seconds, > the UsbBootReadBlocks ->UsbBootExecCmdWithRetry retrun TimeOut. I don't know > why HW return Timeout. > Booting time is to long if using Dynamic way to fix the issue. > When using 64, It works and booting from HW succeed. > May be using PCD is a simple and effective mean. > > Thanks > Ming >>>> Thanks/Ray >>>> >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:[email protected]] On Behalf Of >>>>> Ming Huang >>>>> Sent: Saturday, February 24, 2018 5:30 PM >>>>> To: [email protected]; [email protected]; edk2- >>>>> [email protected]; [email protected]; Zeng, Star >>>>> <[email protected]>; Dong, Eric <[email protected]> >>>>> Cc: [email protected]; [email protected]; Ming Huang >>>>> <[email protected]>; Gao, Liming <[email protected]>; >>>>> [email protected]; [email protected]; >>>>> [email protected]; Kinney, Michael D >>>>> <[email protected]>; [email protected]; >>>>> [email protected]; [email protected] >>>>> Subject: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use Pcd for >>>>> UsbBootIoBlocks >>>>> >>>>> Booting from USB may fail while the macro USB_BOOT_IO_BLOCKS set to 128 >>>>> because 128 blocks is exceeded the maximun blocks of some USB >>>>> devices,like some virtual CD-ROM from BMC. So, give a chance to set the >>>>> value of USB_BOOT_IO_BLOCKS by adding a Pcd. >>>>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Ming Huang <[email protected]> >>>>> --- >>>>> MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h | 7 >>>>> +++++-- >>>>> MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf | 4 >>>>> ++++ >>>>> MdeModulePkg/MdeModulePkg.dec | 4 ++++ >>>>> MdeModulePkg/MdeModulePkg.uni | 4 ++++ >>>>> 4 files changed, 17 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h >>>>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h >>>>> index 5ee50ac52a21..ca9240adbd5f 100644 >>>>> --- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h >>>>> +++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.h >>>>> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY >>>>> KIND, EITHER EXPRESS OR IMPLIED. >>>>> #ifndef _EFI_USB_MASS_BOOT_H_ >>>>> #define _EFI_USB_MASS_BOOT_H_ >>>>> >>>>> +#include <Library/PcdLib.h> >>>>> + >>>>> // >>>>> // The opcodes of various USB boot commands: >>>>> // INQUIRY/REQUEST_SENSE are "No Timeout Commands" as specified @@ >>>>> -66,9 +68,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY >>>>> KIND, EITHER EXPRESS OR IMPLIED. >>>>> #define USB_PDT_SIMPLE_DIRECT 0x0E ///< Simplified >>>>> direct access >>>>> device >>>>> >>>>> // >>>>> -// Other parameters, Max carried size is 512B * 128 = 64KB >>>>> +// Other parameters, Max carried size is depanded on Pcd. >>>>> +// The default of PcdUsbBootIoBlocks is 128. 512B * 128 = 64KB >>>>> // >>>>> -#define USB_BOOT_IO_BLOCKS 128 >>>>> +#define USB_BOOT_IO_BLOCKS (FixedPcdGet32 >>>>> (PcdUsbBootIoBlocks)) >>>>> >>>>> // >>>>> // Retry mass command times, set by experience diff --git >>>>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf >>>>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf >>>>> index 26d15c7679bf..40426512f884 100644 >>>>> --- >>>>> a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf >>>>> +++ >>>>> b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf >>>>> @@ -60,6 +60,7 @@ [Sources] >>>>> UsbMassDiskInfo.c >>>>> >>>>> [Packages] >>>>> + MdeModulePkg/MdeModulePkg.dec >>>>> MdePkg/MdePkg.dec >>>>> >>>>> [LibraryClasses] >>>>> @@ -83,5 +84,8 @@ [Protocols] >>>>> # EVENT_TYPE_RELATIVE_TIMER ## CONSUMES >>>>> # >>>>> >>>>> +[FixedPcd] >>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdUsbBootIoBlocks >>>>> + >>>>> [UserExtensions.TianoCore."ExtraFiles"] >>>>> UsbMassStorageDxeExtra.uni >>>>> diff --git a/MdeModulePkg/MdeModulePkg.dec >>>>> b/MdeModulePkg/MdeModulePkg.dec index 455979386e3f..fc40745315a0 >>>>> 100644 >>>>> --- a/MdeModulePkg/MdeModulePkg.dec >>>>> +++ b/MdeModulePkg/MdeModulePkg.dec >>>>> @@ -999,6 +999,10 @@ [PcdsFixedAtBuild] >>>>> # @Prompt Enable UEFI Stack Guard. >>>>> >>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0 >>>>> x30001055 >>>>> >>>>> +## The Max blocks of usb transfer. The default is 128. >>>>> +# @Prompt The Max blocks of usb transfer >>>>> +gEfiMdeModulePkgTokenSpaceGuid.PcdUsbBootIoBlocks|128|UINT32|0x0 >>>>> 000010B >>>>> + >>>>> [PcdsFixedAtBuild, PcdsPatchableInModule] >>>>> ## Dynamic type PCD can be registered callback function for Pcd >>>>> setting >>>>> action. >>>>> # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum >>>>> number of callback function diff --git a/MdeModulePkg/MdeModulePkg.uni >>>>> b/MdeModulePkg/MdeModulePkg.uni index f3fa616438b0..c996d6b4ebe0 >>>>> 100644 >>>>> --- a/MdeModulePkg/MdeModulePkg.uni >>>>> +++ b/MdeModulePkg/MdeModulePkg.uni >>>>> @@ -1243,3 +1243,7 @@ >>>>> #string >>>>> STR_gEfiMdeModulePkgTokenSpaceGuid_PcdEdkiiFpdtStringRecordEnableO >>>>> nly_HELP #language en-US "Control which FPDT record format will be used >>>>> to store the performance entry.\n" >>>>> >>>>> "On TRUE, the string FPDT >>>>> record will be used to store every performance entry.\n" >>>>> >>>>> "On FALSE, the different >>>>> FPDT record will be used to store the different performance entries." >>>>> + >>>>> +#string >>>>> STR_gEfiMdeModulePkgTokenSpaceGuid_PcdUsbBootIoBlocks_PROMPT >>>>> #language en-US "The Max blocks of usb transfer." >>>>> + >>>>> +#string >>>>> STR_gEfiMdeModulePkgTokenSpaceGuid_PcdUsbBootIoBlocks_HELP >>>>> #language en-US "The Max blocks of usb transfer. The default is 128." >>>>> -- >>>>> 1.9.1 >>>>> >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> [email protected] >>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>> _______________________________________________ >>> edk2-devel mailing list >>> [email protected] >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
