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

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

[Haojian Zhuang]

Since CMD6 & ACMD51 needs to read data size less than 512, proper
variable length should be set.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 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
 //Note: This function has been commented out because it is not used yet.
 //      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
+    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

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

[Ryan Harkin]

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
>

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

[Leif Lindholm]

On Tue, Nov 08, 2016 at 12:37:12PM +0000, Ryan Harkin wrote:
> 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.

Sweet!
Seconded!

Great work, Haojian.

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

Agreed.

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

Agreed. 

> >  //Note: This function has been commented out because it is not used yet.
> 
> This comment is no longer correct and should be removed.

Agreed!

> >  //      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
> 
> ;-)

Eew...

It is unrelated to this patch set, so does not need to be addressed
for that, but certainly looks like it could do with some love (or
fire).

Anyway - Haojian, since neither MMC_CMD6 or MMC_ACMD51 are supported
by the upstream driver, could you fold these changes, including Ryan's
comments on:
- #if 1
- Outdated comment

Into your original set, where these commands were added?
And then resend your series?

Regards,

Leif

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

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

[Haojian Zhuang]

On 8 November 2016 at 22:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Nov 08, 2016 at 12:37:12PM +0000, Ryan Harkin wrote:
>> 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.
>
> Sweet!
> Seconded!
>
> Great work, Haojian.
>
>> 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.
>
> Agreed.
>
>
> Anyway - Haojian, since neither MMC_CMD6 or MMC_ACMD51 are supported
> by the upstream driver, could you fold these changes, including Ryan's
> comments on:
> - #if 1
> - Outdated comment
>
> Into your original set, where these commands were added?
> And then resend your series?
>
> Regards,
>
> Leif
>

No problem. I'll format them and resend them right now.

Best Regards
Haojian