| Message ID | 1478533749-17387-1-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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
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(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel