From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x231.google.com (mail-lf0-x231.google.com [IPv6:2a00:1450:4010:c07::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CDFFB81C90 for ; Tue, 8 Nov 2016 04:37:12 -0800 (PST) Received: by mail-lf0-x231.google.com with SMTP id b14so138014268lfg.2 for ; Tue, 08 Nov 2016 04:37:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=R9NSs7/vn4Brdshz+XjTmUcZlypMx2/esR0BGQEmY/8=; b=kvXukrd/Ptm0Yi4kWoLzPCG46qaLm1RDu9Np6VNt/YGsXP51b2LZGfIjrfZk6PeNGs YYDdJm/Q2rpf/jYPF3ZLdSkApWZjDl3Orh8uswaPXO92YcTtUo/ZPnMh2AsP9R0bHn1t tHVCYF/51thvYqCQGN/ZpABBD0y5npYdJM4XA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=R9NSs7/vn4Brdshz+XjTmUcZlypMx2/esR0BGQEmY/8=; b=efJ1cYGNNh2VmtpSDuqJqcpNayJkfSdqu6W2JthgLsvAb6QOn1rTwqsfqRKQWfBOuM cCE694PfpNomGZuM0/QD7c/l5hkrXu6JFEWrpzokv5aCzmnDNMKROK+QuEPKyj8TEatt WUjO/4vO0uzLnx7+LQwk0rm45wrKUKt8s1ZH8eeWv8bH9vteHVZAuqKcsiLT/iqorLeS DVVDFXQ69pcLzoP4b0cN2c2hcPKYY9EVDN3m0rokvjIifPW7Dyp8RsH5Jqrc3h19xmcY 8zL+Be9EzeHoTBxQi/BqrEre5cI+QM0WGKMHnG7ggxCoiw6BxCKEpphSS2PRvJAGUngA M7yA== X-Gm-Message-State: ABUngvcaS8x2+o7VBkKUjxQOu2cErp24KbFL7HEvJrPFObGQJ9c5M6pW8fTmC9zAN7mynGPLmATgaJAqCOBCoQoX X-Received: by 10.25.153.75 with SMTP id b72mr5661325lfe.112.1478608633640; Tue, 08 Nov 2016 04:37:13 -0800 (PST) MIME-Version: 1.0 Received: by 10.25.27.68 with HTTP; Tue, 8 Nov 2016 04:37:12 -0800 (PST) In-Reply-To: <1478533749-17387-1-git-send-email-haojian.zhuang@linaro.org> References: <1478533749-17387-1-git-send-email-haojian.zhuang@linaro.org> From: Ryan Harkin Date: Tue, 8 Nov 2016 12:37:12 +0000 Message-ID: To: Haojian Zhuang Cc: edk2-devel-01 , Leif Lindholm , Ard Biesheuvel Subject: Re: [PATCH] PL180: set variable length for CMD6 and ACMD51 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Nov 2016 12:37:13 -0000 Content-Type: text/plain; charset=UTF-8 Hi Haojian, On 7 November 2016 at 15:49, Haojian Zhuang 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 Tested-by: Ryan Harkin 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 >