* [PATCH] PL180: set variable length for CMD6 and ACMD51 @ 2016-11-07 15:49 Haojian Zhuang 2016-11-08 12:37 ` Ryan Harkin 0 siblings, 1 reply; 4+ messages in thread From: Haojian Zhuang @ 2016-11-07 15:49 UTC (permalink / raw) To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PL180: set variable length for CMD6 and ACMD51 2016-11-07 15:49 [PATCH] PL180: set variable length for CMD6 and ACMD51 Haojian Zhuang @ 2016-11-08 12:37 ` Ryan Harkin 2016-11-08 14:58 ` Leif Lindholm 0 siblings, 1 reply; 4+ messages in thread From: Ryan Harkin @ 2016-11-08 12:37 UTC (permalink / raw) To: Haojian Zhuang; +Cc: edk2-devel-01, Leif Lindholm, Ard Biesheuvel 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PL180: set variable length for CMD6 and ACMD51 2016-11-08 12:37 ` Ryan Harkin @ 2016-11-08 14:58 ` Leif Lindholm 2016-11-08 15:08 ` Haojian Zhuang 0 siblings, 1 reply; 4+ messages in thread From: Leif Lindholm @ 2016-11-08 14:58 UTC (permalink / raw) To: Ryan Harkin; +Cc: Haojian Zhuang, edk2-devel-01, Ard Biesheuvel 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PL180: set variable length for CMD6 and ACMD51 2016-11-08 14:58 ` Leif Lindholm @ 2016-11-08 15:08 ` Haojian Zhuang 0 siblings, 0 replies; 4+ messages in thread From: Haojian Zhuang @ 2016-11-08 15:08 UTC (permalink / raw) To: Leif Lindholm; +Cc: Ryan Harkin, edk2-devel-01, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-08 15:08 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-07 15:49 [PATCH] PL180: set variable length for CMD6 and ACMD51 Haojian Zhuang 2016-11-08 12:37 ` Ryan Harkin 2016-11-08 14:58 ` Leif Lindholm 2016-11-08 15:08 ` Haojian Zhuang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox