Re: [EXT] Re: [PATCH 1/1] EmbeddedPkg/MmcDxe: Added MaxBlock Transfer Limit 65535 in R/W.

["Gaurav Jain" <gaurav.jain@nxp.com>]

Hi Tien Hock

Can you help to review the patch?

Regards
Gaurav Jain

> -----Original Message-----
> From: Loh, Tien Hock <tien.hock.loh@intel.com>
> Sent: Tuesday, April 7, 2020 1:23 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>; Ard Biesheuvel
> <ard.biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>
> Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>;
> Haojian Zhuang <haojian.zhuang@linaro.org>
> Subject: RE: [EXT] Re: [PATCH 1/1] EmbeddedPkg/MmcDxe: Added MaxBlock
> Transfer Limit 65535 in R/W.
> 
> Caution: EXT Email
> 
> Hi Leif, Gaurav,
> 
> The changes look good to me, but I haven't tested it on Intel's SoCFPGA
> platform.
> I will need some time to test it as I'm working on some other tasks, maybe in
> a week or so.
> 
> Thanks
> 
> > -----Original Message-----
> > From: Gaurav Jain <gaurav.jain@nxp.com>
> > Sent: Tuesday, April 7, 2020 3:02 PM
> > To: Ard Biesheuvel <ard.biesheuvel@arm.com>; Leif Lindholm
> > <leif@nuviainc.com>
> > Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>;
> > Haojian Zhuang <haojian.zhuang@linaro.org>; Loh, Tien Hock
> > <tien.hock.loh@intel.com>
> > Subject: RE: [EXT] Re: [PATCH 1/1] EmbeddedPkg/MmcDxe: Added
> MaxBlock
> > Transfer Limit 65535 in R/W.
> >
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > Sent: Monday, April 6, 2020 7:42 PM
> > > To: Leif Lindholm <leif@nuviainc.com>; Gaurav Jain
> > > <gaurav.jain@nxp.com>
> > > Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>;
> > > Haojian Zhuang <haojian.zhuang@linaro.org>; Loh, Tien Hock
> > > <tien.hock.loh@intel.com>
> > > Subject: [EXT] Re: [PATCH 1/1] EmbeddedPkg/MmcDxe: Added MaxBlock
> > > Transfer Limit 65535 in R/W.
> > >
> > > Caution: EXT Email
> > >
> > > On 4/6/20 4:08 PM, Leif Lindholm wrote:
> > > > Hi Gaurav,
> > > >
> > > > Haojian, Tien Hock - can you help review/test this change?
> > > >
> > > > Best Regards,
> > > >
> > > > Leif
> > > >
> > > > On Fri, Apr 03, 2020 at 14:54:07 +0530, Gaurav Jain wrote:
> > > >> Moved BlockCount calculation below BufferSize Validation checks.
> > > >> First Ensure Buffersize is Not Zero and multiple of Media BlockSize.
> > > >> then calculate BlockCount and perform Block checks.
> > > >>
> > > >> Corrected BlockCount calculation, as BufferSize is multiple of
> > > >> BlockSize, So adding (BlockSize-1) bytes to BufferSize and then
> > > >> divide by BlockSize will have no impact on BlockCount.
> > > >>
> > > >> Reading Large Images from MMC causes errors.
> > > >> As per SD Host Controller Spec version 4.20, Restriction of
> > > >> 16-bit Block Count transfer is 65535.
> > > >> Max block transfer limit in single cmd is 65535 blocks.
> > > >> Added Max Block check that can be processed is 0xFFFF.
> > > >> then Update BlockCount on the basis of MaxBlock.
> > > >>
> > > >> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> > >
> > >
> > > Hello Gaurav,
> > >
> > > Could you please elaborate on the underlying need for this change?
> > > If you are considering using this driver for future NXP platforms, I
> > > should point out that this legacy driver is only kept around for
> > > existing users, and new users should use the driver stack in
> > > MdeModulePkg, which is based on the UEFI spec.
> > >
> > > --
> > > Ard.
> >
> > Hello Ard
> >
> > This change is for existing Platforms as well, that are using
> > EmbeddedPkg driver.
> > I can see Max Block Transfer Limit in MdeModulePkg also.
> > This Limit is not defined in EmbeddedPkg, which is causing errors on
> > NXP existing platform While reading Large images from MMC.
> > Block transfer limit is defined in SD spec.
> >
> > Regards
> > Gaurav Jain
> > >
> > >
> > >
> > > >> ---
> > > >>   EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c | 38
> > > ++++++++++++++++++++-----------
> > > >>   1 file changed, 25 insertions(+), 13 deletions(-)
> > > >>
> > > >> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> > > >> b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> > > >> index 17c20c0159ba..b508c466d9c5 100644
> > > >> --- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> > > >> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> > > >> @@ -242,6 +242,8 @@ MmcIoBlocks (
> > > >>     UINTN                   BytesRemainingToBeTransfered;
> > > >>     UINTN                   BlockCount;
> > > >>     UINTN                   ConsumeSize;
> > > >> +  UINT32                  MaxBlock;
> > > >> +  UINTN                   RemainingBlock;
> > > >>
> > > >>     BlockCount = 1;
> > > >>     MmcHostInstance = MMC_HOST_INSTANCE_FROM_BLOCK_IO_THIS
> > > (This); @@
> > > >> -262,19 +264,6 @@ MmcIoBlocks (
> > > >>       return EFI_NO_MEDIA;
> > > >>     }
> > > >>
> > > >> -  if (MMC_HOST_HAS_ISMULTIBLOCK(MmcHost) && MmcHost-
> > > >IsMultiBlock(MmcHost)) {
> > > >> -    BlockCount = (BufferSize + This->Media->BlockSize - 1) / This-
> >Media-
> > > >BlockSize;
> > > >> -  }
> > > >> -
> > > >> -  // All blocks must be within the device
> > > >> -  if ((Lba + (BufferSize / This->Media->BlockSize)) >
> > > >> (This->Media-
> > > >LastBlock + 1)) {
> > > >> -    return EFI_INVALID_PARAMETER;
> > > >> -  }
> > > >> -
> > > >> -  if ((Transfer == MMC_IOBLOCKS_WRITE) && (This->Media-
> >ReadOnly
> > > == TRUE)) {
> > > >> -    return EFI_WRITE_PROTECTED;
> > > >> -  }
> > > >> -
> > > >>     // Reading 0 Byte is valid
> > > >>     if (BufferSize == 0) {
> > > >>       return EFI_SUCCESS;
> > > >> @@ -285,14 +274,36 @@ MmcIoBlocks (
> > > >>       return EFI_BAD_BUFFER_SIZE;
> > > >>     }
> > > >>
> > > >> +  if (MMC_HOST_HAS_ISMULTIBLOCK(MmcHost) && MmcHost-
> > > >IsMultiBlock(MmcHost)) {
> > > >> +    BlockCount = BufferSize / This->Media->BlockSize;  }
> > > >> +
> > > >> +  // All blocks must be within the device  if ((Lba +
> > > >> + (BufferSize /
> > > >> + This->Media->BlockSize)) > (This->Media->LastBlock + 1)) {
> > > >> +    return EFI_INVALID_PARAMETER;  }
> > > >> +
> > > >> +  if ((Transfer == MMC_IOBLOCKS_WRITE) && (This->Media-
> >ReadOnly
> > > == TRUE)) {
> > > >> +    return EFI_WRITE_PROTECTED;
> > > >> +  }
> > > >> +
> > > >>     // Check the alignment
> > > >>     if ((This->Media->IoAlign > 2) && (((UINTN)Buffer &
> > > >> (This->Media-
> > > >IoAlign - 1)) != 0)) {
> > > >>       return EFI_INVALID_PARAMETER;
> > > >>     }
> > > >>
> > > >> +  // Max block number in single cmd is 65535 blocks.
> > > >> +  MaxBlock = 0xFFFF;
> > > >> +  RemainingBlock = BlockCount;
> > > >>     BytesRemainingToBeTransfered = BufferSize;
> > > >>     while (BytesRemainingToBeTransfered > 0) {
> > > >>
> > > >> +    if (RemainingBlock <= MaxBlock) {
> > > >> +      BlockCount = RemainingBlock;
> > > >> +    } else {
> > > >> +      BlockCount = MaxBlock;
> > > >> +    }
> > > >> +
> > > >>       // Check if the Card is in Ready status
> > > >>       CmdArg = MmcHostInstance->CardInfo.RCA << 16;
> > > >>       Response[0] = 0;
> > > >> @@ -338,6 +349,7 @@ MmcIoBlocks (
> > > >>         DEBUG ((EFI_D_ERROR, "%a(): Failed to transfer block and
> > > Status:%r\n", __func__, Status));
> > > >>       }
> > > >>
> > > >> +    RemainingBlock -= BlockCount;
> > > >>       BytesRemainingToBeTransfered -= ConsumeSize;
> > > >>       if (BytesRemainingToBeTransfered > 0) {
> > > >>         Lba    += BlockCount;
> > > >> --
> > > >> 2.7.4
> > > >>