Re: [PATCH] PL180: set variable length for CMD6 and ACMD51

[Ryan Harkin <ryan.harkin@linaro.org>]

Hi Haojian,

On 7 November 2016 at 15:49, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
> Since CMD6 & ACMD51 needs to read data size less than 512, proper
> variable length should be set.
>

Yay!  Thanks for working out what the problem was with TC2.  I've
tested this patch on top of your v3 series of your MMC patches and
everything seems to be working now.

I tested in release and debug builds too.


> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

However, I have a minor comment about this patch below...

I would like to see a series pushed that includes this patch in the
correct place so that TC2 does not break at all during the series.  At
the moment, TC2 will not be bisect-able if this patch is pushed after
the series.


> ---
>  ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
> index 4d839e7..c3e8830 100644
> --- a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
> +++ b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
> @@ -63,7 +63,7 @@ MciIsReadOnly (
>    return (MmioRead32 (FixedPcdGet32 (PcdPL180SysMciRegAddress)) & SYS_MCI_WPROT);
>  }
>
> -#if 0
> +#if 1

I think it would be better to remove the "#if 0" rather than change it
to 1.  Leif, do you have a stronger opinion about it either way?


>  //Note: This function has been commented out because it is not used yet.

This comment is no longer correct and should be removed.


>  //      This function could be used to remove the hardcoded BlockLen used
>  //      in MciPrepareDataPath
> @@ -129,12 +129,20 @@ MciSendCommand (
>    } else if (MmcCmd == MMC_CMD6) {
>      MmioWrite32 (MCI_DATA_TIMER_REG, 0xFFFFFFF);
>      MmioWrite32 (MCI_DATA_LENGTH_REG, 64);
> +#ifndef USE_STREAM

Seeing this #ifdef made me look at other uses of USE_STREAM in the
file.  If you are going to enable a variable size BlockLen, do you
need to do it in the other places where the USE_STREAM occurs?

But I guess these lines make the whole USE_STREAM thing look like a
bad idea anyway:

// Untested ...
//#define USE_STREAM

;-)


> +    MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | GetPow2BlockLen (64));
> +#else
>      MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | MCI_DATACTL_STREAM_TRANS);
> +#endif
>    } else if (MmcCmd == MMC_ACMD51) {
>      MmioWrite32 (MCI_DATA_TIMER_REG, 0xFFFFFFF);
>      /* SCR register is 8 bytes long. */
>      MmioWrite32 (MCI_DATA_LENGTH_REG, 8);
> +#ifndef USE_STREAM
> +    MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | GetPow2BlockLen (8));
> +#else
>      MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | MCI_DATACTL_STREAM_TRANS);
> +#endif
>    }
>
>    // Create Command for PL180
> --
> 2.7.4
>