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

[Leif Lindholm <leif.lindholm@linaro.org>]

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