From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x229.google.com (mail-wm0-x229.google.com [IPv6:2a00:1450:400c:c09::229]) (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 0D69481D4F for ; Tue, 8 Nov 2016 06:58:01 -0800 (PST) Received: by mail-wm0-x229.google.com with SMTP id p190so251503590wmp.1 for ; Tue, 08 Nov 2016 06:58:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=39FWdJY/zgp2aNVhdinYLPDQIX0ldS02TpUewaF/EN4=; b=dPVERS9FrwzLLI9ciBiLmOXRWu5IHrJFRq4WUeq/Tpxps1evQaOYukC6EvhicdMS55 ZIdTGnLWfNuzzszvdZZ5N+63vgSREg6vWjfLTHcQEzUd8ecfqYj+8aRbhiHy5e/rRyzU 506AzdxBI+AwDZjGV5t2u2IkzHP77edjkvbRs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=39FWdJY/zgp2aNVhdinYLPDQIX0ldS02TpUewaF/EN4=; b=CRLhuHFYzSOt8QA9BYgEoQHXIY8ItDM6ZRR5DUuw6HIPImFhf8GwMHheC3FeAKxteW 2ZB0p8quMkATB6qCeUjTtdW9Eg+j3J81QgwZ1/qrjLOJ0S7Vip/AxMr+w7kmYtxCXSsx deBDcER/YK5egfEFLk6dH0HmHSzeFxDqwTcx4QHqmjfDMYzPO/nIca8nBYtNSGaCx+0w lSL6fhrFavVJKrz7Q8QvYMKWsIyuAnVgPMw4DakfcWJVLm9kIiCE8wNrQ6noSErULkl7 7H739RuUfrJPCkj35PjFtTPfEE12MRQgIEtCxowayv2fF3618X3vtxjoMyeJNdMkSvjC 5LVg== X-Gm-Message-State: ABUngveJmmufMuzn/VIDEWRN4juJEYKcgWvdxjbEKMIIo62t7DH55G1K0Z3UsOtHIck6nL5k X-Received: by 10.28.144.14 with SMTP id s14mr16048655wmd.26.1478617082598; Tue, 08 Nov 2016 06:58:02 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id l67sm1419870wmf.20.2016.11.08.06.58.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Nov 2016 06:58:02 -0800 (PST) Date: Tue, 8 Nov 2016 14:58:00 +0000 From: Leif Lindholm To: Ryan Harkin Cc: Haojian Zhuang , edk2-devel-01 , Ard Biesheuvel Message-ID: <20161108145800.GH27644@bivouac.eciton.net> References: <1478533749-17387-1-git-send-email-haojian.zhuang@linaro.org> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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 14:58:01 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Nov 08, 2016 at 12:37:12PM +0000, Ryan Harkin wrote: > 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. Sweet! Seconded! Great work, Haojian. > 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. 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 > >