From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=xSn3m0XS; spf=pass (domain: linaro.org, ip: 209.85.128.68, mailfrom: leif.lindholm@linaro.org) Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by groups.io with SMTP; Fri, 03 May 2019 04:51:58 -0700 Received: by mail-wm1-f68.google.com with SMTP id b10so6787845wmj.4 for ; Fri, 03 May 2019 04:51:57 -0700 (PDT) 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=Kk7J7BtmmPWqBkBrnJ3HCCN5tJns1YdOz/RaK6HxwQo=; b=xSn3m0XSgvm/rInwkBfHnK+AJplbP/uxyIQUTGH1LNB6hm5FWdBOJMnMsXQRnX0VLL GSd6PfBUQqtYlR6XeQU0pgZeuanXg50plUyyXLGdDEHA/80usr6PuwMCFpMIFoGYzY4a 4rr8plUAU6YnPVeM+FbJJMmcq3bZOUp3+stvyXJyJtumxBMbga5RmdP+EuTxd3uAEBcV ip2bIh+LUWNy0REnRWuubnRXgc0Fic57f+4d+XmV72O6zZXYR2RnLwUch46+Lamh6tvQ NR/m932n4MIZlgo0FTdm+AJz20fcEySUjDAw1oG2rY3Bf09Zu1Nigo/ZcZFNQa32U9gO A6/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Kk7J7BtmmPWqBkBrnJ3HCCN5tJns1YdOz/RaK6HxwQo=; b=qNDsqChzKa0+XwG028XjUzI26+VP4J7XJhSWCleSwiYEqelzkYJgSRP6ulTtgbOIfg yflgGZcUWEJSR4QZtS9mNZJeQhGD5Sh++VL+9Jqfq/WZ5nB8iC7Vaojkp3OS3HMqyXOb jORFz3Czy+O+Atf1YYF2/alUNHXCCgnyKbbHlVSk46YhYWqq4k38QjGDmeDBOOGCN/Sh vQjGQqJvgY6HDcFHwWf+Ij1T263uB29TXA5X8/JVZT6saf95V2+Mu5sxb7tcKMUXF6K9 SnhD9Lo+q8HIymQH2owtERLGx4b1C2M5sQaI5wlh/bIkYU1n86n0PRNLRgZ6Cwl4bqqz Lwqw== X-Gm-Message-State: APjAAAWHuNHTzmZWmr2FbpxYZkxGyu+9UJJ2bNdzp3fN8tjb8bXdiv1Q Nz08jhF4jkSnK+YrvO4XdvNSqQ== X-Google-Smtp-Source: APXvYqwxlrFqtLDHfU4/zIvpdaX8YmqALXIqOqA8EL8XgvbKUAwXrR6uX67E/pMGzyi2Fn5jkcyqaw== X-Received: by 2002:a05:600c:506:: with SMTP id i6mr3869972wmc.3.1556884316504; Fri, 03 May 2019 04:51:56 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id u14sm1357159wrn.30.2019.05.03.04.51.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 03 May 2019 04:51:55 -0700 (PDT) Date: Fri, 3 May 2019 12:51:54 +0100 From: "Leif Lindholm" To: tien.hock.loh@intel.com Cc: devel@edk2.groups.io, thloh85@gmail.com, Ard Biesheuvel Subject: Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs Message-ID: <20190503115154.zc6x37xupwl7v7jr@bivouac.eciton.net> References: <1556854023-5486-1-git-send-email-tien.hock.loh@intel.com> <1556854023-5486-2-git-send-email-tien.hock.loh@intel.com> MIME-Version: 1.0 In-Reply-To: <1556854023-5486-2-git-send-email-tien.hock.loh@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Tien Hock, Thanks for splitting the patches back up, and sorry for taking so long to get around to reviewing. But could you please add some descriptive subject lines back as well? Most of my comments on this series are purely coding style related, a couple are not. On Fri, May 03, 2019 at 11:26:57AM +0800, tien.hock.loh@intel.com wrote: > From: "Tien Hock, Loh" > > Added ACMD6 for SD support. For SD, after CMD55 is sent, the next command > should be an application command, which should not expect data > > Signed-off-by: "Tien Hock, Loh" > Cc: Leif Lindholm > Cc: Ard Biesheuvel > --- > EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c > index adc6b06..fa24802 100644 > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c > @@ -39,6 +39,7 @@ DWEMMC_IDMAC_DESCRIPTOR *gpIdmacDesc; > EFI_GUID mDwEmmcDevicePathGuid = EFI_CALLER_ID_GUID; > STATIC UINT32 mDwEmmcCommand; > STATIC UINT32 mDwEmmcArgument; > +STATIC BOOLEAN mIsACmd = FALSE; Could we move this variable into DwEmmcSendCommand and drop the 'm'? > > EFI_STATUS > DwEmmcReadBlockData ( > @@ -321,6 +322,15 @@ DwEmmcSendCommand ( > Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC | > BIT_CMD_SEND_INIT; > break; > + case MMC_INDX(6): > + if(mIsACmd) { > + Cmd = BIT_CMD_RESPONSE_EXPECT ; Drop space before ;. > + } > + else { } else { > + Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_DATA_EXPECTED | > + BIT_CMD_READ; > + } > + break; > case MMC_INDX(7): > if (Argument) > Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC; > @@ -371,6 +381,11 @@ DwEmmcSendCommand ( > } > > Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG | BIT_CMD_START; > + > + if(MMC_INDX(55) == MMC_GET_INDX(MmcCmd)) > + mIsACmd = TRUE; > + else > + mIsACmd = FALSE; if () { } else { } There should also be spaces between MMC_INDX and (55), as well as between MMC_GET_INDX (MmcCmd). Surrounding code does not follow the style, but I would still prefer to keep to this for new/modified code. I also think we should add IsACmd = FALSE to the default: case. / Leif > if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) { > mDwEmmcCommand = Cmd; > mDwEmmcArgument = Argument; > -- > 2.2.2 >