From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Albecki, Mateusz" <mateusz.albecki@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe Fix eMMC HS400 switch sequence
Date: Mon, 18 Feb 2019 06:00:50 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C897AB1@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20190215144458.1812-2-mateusz.albecki@intel.com>
Hi Mateusz,
I found that the patch proposed is actually handling one of the issues
reported in the below BZ tracker:
https://bugzilla.tianocore.org/show_bug.cgi?id=1140
According to the discussion within the tracker, there are 2 fixes needed
for the SdMmcPciHcDxe:
1. Move the clock supply before doing send status
2. More robust handle to the send status CRC error on the switch to HS200
I think the proposed patch is handling the 1st issue listed above.
So could you help to add the below line:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1140
at the beginning of your commit log message?
Also, please see the below inline comments below:
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Albecki, Mateusz
> Sent: Friday, February 15, 2019 10:45 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A
> Subject: [edk2] [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe Fix eMMC
> HS400 switch sequence
>
> In eMMC HS400 switch sequence flow eMMC driver attampted
'attampted' -> 'attempted'
> to execute SEND_STATUS just after switching bus timing to high
> speed and before downgrading clock frequency to 52MHz. Since link
> was at that time in incorrect state SEND_STATUS was failing which
> made driver think switch to HS400 failed.
> This change makes driver always change clock frequency after
> switching bus timing and before executing SEND_STATUS.
>
> Change-Id: Ib1f2c78a8693c2a4ef6e1f1b5da2f4133f6210d2
Please help to remove the above 'Change-Id' information.
> Cc: Hao Wu <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> ---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 37
> +++++++++++++------------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index 4ef849fd0962..22200f806f26 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -642,7 +642,7 @@ EmmcSwitchBusWidth (
> }
>
> /**
> - Switch the clock frequency to the specified value.
> + Switch the bus timing and clock frequency.
>
> Refer to EMMC Electrical Standard Spec 5.1 Section 6.6 and SD Host
> Controller
> Simplified Spec 3.0 Figure 3-3 for details.
> @@ -660,7 +660,7 @@ EmmcSwitchBusWidth (
>
> **/
> EFI_STATUS
> -EmmcSwitchClockFreq (
> +EmmcSwitchBusTiming (
Could you help to update the debug messages that include keyword
'EmmcSwitchClockFreq' to 'EmmcSwitchBusTiming' to align with the function
name change?
Best Regards,
Hao Wu
> IN EFI_PCI_IO_PROTOCOL *PciIo,
> IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> IN UINT8 Slot,
> @@ -693,18 +693,6 @@ EmmcSwitchClockFreq (
> return Status;
> }
>
> - Status = EmmcSendStatus (PassThru, Slot, Rca, &DevStatus);
> - if (EFI_ERROR (Status)) {
> - DEBUG ((DEBUG_ERROR, "EmmcSwitchClockFreq: Send status fails
> with %r\n", Status));
> - return Status;
> - }
> - //
> - // Check the switch operation is really successful or not.
> - //
> - if ((DevStatus & BIT7) != 0) {
> - DEBUG ((DEBUG_ERROR, "EmmcSwitchClockFreq: The switch operation
> fails as DevStatus is 0x%08x\n", DevStatus));
> - return EFI_DEVICE_ERROR;
> - }
> //
> // Convert the clock freq unit from MHz to KHz.
> //
> @@ -713,6 +701,19 @@ EmmcSwitchClockFreq (
> return Status;
> }
>
> + Status = EmmcSendStatus (PassThru, Slot, Rca, &DevStatus);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "EmmcSwitchClockFreq: Send status fails
> with %r\n", Status));
> + return Status;
> + }
> + //
> + // Check the switch operation is really successful or not.
> + //
> + if ((DevStatus & BIT7) != 0) {
> + DEBUG ((DEBUG_ERROR, "EmmcSwitchClockFreq: The switch operation
> fails as DevStatus is 0x%08x\n", DevStatus));
> + return EFI_DEVICE_ERROR;
> + }
> +
> if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> Status = mOverride->NotifyPhase (
> Private->ControllerHandle,
> @@ -799,7 +800,7 @@ EmmcSwitchToHighSpeed (
> }
>
> HsTiming = 1;
> - Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing,
> ClockFreq);
> + Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming,
> Timing, ClockFreq);
>
> return Status;
> }
> @@ -887,7 +888,7 @@ EmmcSwitchToHS200 (
> Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof
> (ClockCtrl), &ClockCtrl);
>
> HsTiming = 2;
> - Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing,
> ClockFreq);
> + Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming,
> Timing, ClockFreq);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> @@ -937,7 +938,7 @@ EmmcSwitchToHS400 (
> // Set to Hight Speed timing and set the clock frequency to a value less than
> 52MHz.
> //
> HsTiming = 1;
> - Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> SdMmcMmcHsSdr, 52);
> + Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming,
> SdMmcMmcHsSdr, 52);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> @@ -957,7 +958,7 @@ EmmcSwitchToHS400 (
> }
>
> HsTiming = 3;
> - Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing,
> ClockFreq);
> + Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming,
> Timing, ClockFreq);
>
> return Status;
> }
> --
> 2.14.1.windows.1
>
> --------------------------------------------------------------------
>
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII
> Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-
> 07-52-316 | Kapital zakladowy 200.000 PLN.
>
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego
> adresata i moze zawierac informacje poufne. W razie przypadkowego
> otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale
> jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the
> sole use of the intended recipient(s). If you are not the intended recipient,
> please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
prev parent reply other threads:[~2019-02-18 6:00 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-15 14:44 [PATCH 0/1] Correcting eMMC HS400 switch sequence Albecki, Mateusz
2019-02-15 14:44 ` [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe Fix " Albecki, Mateusz
2019-02-18 6:00 ` Wu, Hao A [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C897AB1@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox