From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4864:20::141; helo=mail-it1-x141.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it1-x141.google.com (mail-it1-x141.google.com [IPv6:2607:f8b0:4864:20::141]) (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 6F14321189FB9 for ; Fri, 2 Nov 2018 01:21:55 -0700 (PDT) Received: by mail-it1-x141.google.com with SMTP id d6so2112054itl.4 for ; Fri, 02 Nov 2018 01:21:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=C9Ajk5p2QMdOC4uemCifDsx/gp/ZqzahwR613J9bN/I=; b=kZvWi+d+rXh8zg88ypb4asc0jCp723E+dFgykIlbYBI4SDlTNhWkcdS1urHkSH83Av qA+WWLltPlmNdhXoiRoMPMGBRyfr0O3l/UtzMfCvJPFSpYjbYENXo7dWw1Xy0/zU5D0a YjB9qbVrF8wTwUXNbO1YNnOb5xrEBKKACvNfokLttZI1rjI0bpUIZKcCX1IlSWZksZE0 leDEL/C7rIXdZWmGaVjidyU4f4I/nFzD5DtrhPwjU/wUmEhG14KEUTFIKziGOmDhKJqU fp8H2SYtRPZs7bB+UUGucQYaHtKivDK/WoLoucjHiMxQOu2O/bHke9vCbJ7YreVz4paJ WAeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=C9Ajk5p2QMdOC4uemCifDsx/gp/ZqzahwR613J9bN/I=; b=MEL66ghTE2NX1S7lG/K8VsSfBffF553cftmFbJa163wcKydDummqYheShyjRpaH8rU iXup3bq33r8HXTUr8cAWAcChahGq8Dedj52aL4t4G9PNlwI3tmEhH4iFFxwDnE7YuX3n KsImXGBFWWpRCrpfXQ5o8JkkFgeJwiKiS1SebDTH13UfnyM3z2npfCAY8sW7OwQUm7RT U/Qv8HbtevC8mwDLD5ERD7UkxBNbcwCW4Nc+yloPO8tIfGH7+Vnr/BuLuWwGCz5H0x1l Z8HoB+nqbU66+17U3ASgKskn2/WrEBcNlUH4XlH1vY0ywCL38tt5M6kbiXXSpoUfvwEw JuNw== X-Gm-Message-State: AGRZ1gKH9ztfApoKczUKihyKWcgsVCYP/H7tbXPac4/kREs/B+pOGCtS 5wCFLxzwL3UXYtWASPOrcq3bMqwMGCMXHxricnMqAg== X-Google-Smtp-Source: AJdET5ezbVzjlPPNUPIz7rG6NPL++eGY67ZJjNR8wpe3Or1Hjs06rneRvgYLm6xqfcz/bvo2MoXhT6Zjl89gOXUzBHg= X-Received: by 2002:a02:41d2:: with SMTP id n79-v6mr9341545jad.112.1541146914377; Fri, 02 Nov 2018 01:21:54 -0700 (PDT) MIME-Version: 1.0 References: <1538745911-22484-1-git-send-email-mw@semihalf.com> <1538745911-22484-3-git-send-email-mw@semihalf.com> In-Reply-To: From: Marcin Wojtas Date: Fri, 2 Nov 2018 09:21:43 +0100 Message-ID: To: hao.a.wu@intel.com Cc: edk2-devel-01 , "Kinney, Michael D" , "Gao, Liming" , Leif Lindholm , Ard Biesheuvel , nadavh@marvell.com, "jsd@semihalf.com" , Tomasz Michalec Subject: Re: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Nov 2018 08:21:55 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Hao, czw., 1 lis 2018 o 08:06 Wu, Hao A napisa=C5=82(a): > > Hi Marcin, > > > -----Original Message----- > > From: Marcin Wojtas [mailto:mw@semihalf.com] > > Sent: Friday, October 05, 2018 9:25 PM > > To: edk2-devel@lists.01.org > > Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindholm@linaro.or= g; Wu, > > Hao A; ard.biesheuvel@linaro.org; nadavh@marvell.com; > > mw@semihalf.com; jsd@semihalf.com; tm@semihalf.com > > Subject: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling > > to SdMmcOverride protocol > > > > From: Tomasz Michalec > > > > Some SD Host Controlers use different values in Host Control 2 Register > > to select UHS Mode. This patch adds a new UhsSignaling type routine to > > the NotifyPhase of the SdMmcOverride protocol. > > > > UHS signaling configuration is moved to a common, default routine > > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not > > cover this functionality. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Marcin Wojtas > > --- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 50 +++++++ > > MdeModulePkg/Include/Protocol/SdMmcOverride.h | 2 + > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 153 > > ++++++++++++-------- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 37 +++-- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 69 +++++++++ > > 5 files changed, 243 insertions(+), 68 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > index e389d52..a03160d 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF > > ANY KIND, EITHER EXPRESS OR IMPLIED. > > #define SD_MMC_HC_CTRL_VER 0xFE > > > > // > > +// SD Host Controler bits to HOST_CTRL2 register > > +// > > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007 > > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x0000 > > +#define SD_MMC_HC_CTRL_UHS_SDR25 0x0001 > > +#define SD_MMC_HC_CTRL_UHS_SDR50 0x0002 > > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003 > > +#define SD_MMC_HC_CTRL_UHS_DDR50 0x0004 > > +#define SD_MMC_HC_CTRL_MMC_DDR52 0x0004 > > +#define SD_MMC_HC_CTRL_MMC_SDR50 0x0002 > > I think SD_MMC_HC_CTRL_MMC_SDR50 is not needed here. > > Since according to the SD Physical Layer Simplified Specification, max cl= ock > frequency for SD bus mode SDR50 is 100MHz. And there is no eMMC bus mode = whose > max clock frequency is at 100MHz in Embedded Multi-Media Card Electrical > Standard (5.1). Ok, will drop it. > > > +#define SD_MMC_HC_CTRL_MMC_SDR25 0x0001 > > +#define SD_MMC_HC_CTRL_MMC_SDR12 0x0000 > > +#define SD_MMC_HC_CTRL_HS200 0x0003 > > +#define SD_MMC_HC_CTRL_HS400 0x0005 > > How about the below renames & reorder? > > SD_MMC_HC_CTRL_MMC_LEGACY 0x0000 > SD_MMC_HC_CTRL_MMC_HS_SDR 0x0001 > SD_MMC_HC_CTRL_MMC_HS_DDR 0x0004 > SD_MMC_HC_CTRL_MMC_HS200 0x0003 > SD_MMC_HC_CTRL_MMC_HS400 0x0005 Ok. > > > + > > +// > > +// Timing modes for uhs > > +// > > +typedef enum { > > + SdMmcUhsSdr12, > > + SdMmcUhsSdr25, > > + SdMmcUhsSdr50, > > + SdMmcUhsSdr104, > > + SdMmcUhsDdr50, > > + SdMmcMmcDdr52, > > + SdMmcMmcSdr50, > > + SdMmcMmcSdr25, > > + SdMmcMmcSdr12, > > + SdMmcMmcHs200, > > + SdMmcMmcHs400, > > +} SD_MMC_UHS_TIMING; > > Suggest a similar drop of 'SdMmcMmcSdr50' and rename according to the abo= ve > comments upon HOST_CTRL2 register value definitions. Also, how about a re= name > for enum to SD_MMC_BUS_MODE? Ok. > > > + > > +// > > // The transfer modes supported by SD Host Controller > > // Simplified Spec 3.0 Table 1-2 > > // > > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl ( > > IN UINT8 Slot > > ); > > > > +/** > > + Set SD Host Controler control 2 registry according to selected speed= . > > + > > + @param[in] PciIo The PCI IO protocol instance. > > + @param[in] Slot The slot number of the SD card to send the > > command to. > > + @param[in] Timing The timing to select. > > + > > + @retval EFI_SUCCESS The timing is set successfully. > > + @retval Others The timing isn't set successfully. > > +**/ > > +EFI_STATUS > > +SdMmcHcUhsSignaling ( > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > + IN UINT8 Slot, > > + IN SD_MMC_UHS_TIMING Timing > > + ); > > + > > #endif > > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h > > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h > > index 178945f..25db98a 100644 > > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h > > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h > > @@ -17,6 +17,7 @@ > > #ifndef __SD_MMC_OVERRIDE_H__ > > #define __SD_MMC_OVERRIDE_H__ > > > > +#include > > Please do not expose a module private header file here. > > One approach comes to me is to keep the SD/MMC bus mode enumeration struc= ture > in this protocol header file. SdMmcPciHcDxe driver and producers of the > Override protocol keep a version of the HOST_CTRL2 register value macros = of > their own. Agree. I will move necessary defines to Include/Protocol/SdMmcOverride.h instead of exposing SdMmcPciHci.h. HOST_CTRL2 contents will be a local define in producer code. > > > #include > > > > #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \ > > @@ -31,6 +32,7 @@ typedef enum { > > EdkiiSdMmcResetPost, > > EdkiiSdMmcInitHostPre, > > EdkiiSdMmcInitHostPost, > > + EdkiiSdMmcUhsSignaling, > > } EDKII_SD_MMC_PHASE_TYPE; > > > > /** > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > index c5fd214..05bd4a0 100755 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed ( > > IN UINT8 BusWidth > > ) > > { > > - EFI_STATUS Status; > > - UINT8 HsTiming; > > - UINT8 HostCtrl1; > > - UINT8 HostCtrl2; > > + EFI_STATUS Status; > > + UINT8 HsTiming; > > + UINT8 HostCtrl1; > > + SD_MMC_UHS_TIMING Timing; > > + SD_MMC_HC_PRIVATE_DATA *Private; > > + > > + Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, Bu= sWidth); > > if (EFI_ERROR (Status)) { > > @@ -758,27 +761,37 @@ EmmcSwitchToHighSpeed ( > > return Status; > > } > > > > - // > > - // Clean UHS Mode Select field of Host Control 2 reigster before upd= ate > > - // > > - HostCtrl2 =3D (UINT8)~0x7; > > - Status =3D SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, > > sizeof (HostCtrl2), &HostCtrl2); > > - if (EFI_ERROR (Status)) { > > - return Status; > > - } > > - // > > - // Set UHS Mode Select field of Host Control 2 reigster to SDR12/25/= 50 > > - // > > if (IsDdr) { > > - HostCtrl2 =3D BIT2; > > + Timing =3D SdMmcMmcDdr52; > > } else if (ClockFreq =3D=3D 52) { > > - HostCtrl2 =3D BIT0; > > + Timing =3D SdMmcMmcSdr50; > > + } else if (ClockFreq =3D=3D 26) { > > + Timing =3D SdMmcMmcSdr25; > > } else { > > - HostCtrl2 =3D 0; > > + Timing =3D SdMmcMmcSdr12; > > } > > As mentioned above, "SdMmcMmcSdr50" can be dropped here. > And considering the rename above, how about: > > if (IsDdr) { > Timing =3D SdMmcMmcHsDdr; > } else if (ClockFreq =3D=3D 52) { > Timing =3D SdMmcMmcHsSdr; > } else { > Timing =3D SdMmcMmcLegacy; > } Ok. > > > - Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof > > (HostCtrl2), &HostCtrl2); > > - if (EFI_ERROR (Status)) { > > - return Status; > > + > > + if (mOverride !=3D NULL && mOverride->NotifyPhase !=3D NULL) { > > + Status =3D mOverride->NotifyPhase ( > > + Private->ControllerHandle, > > + Slot, > > + EdkiiSdMmcUhsSignaling, > > + &Timing > > + ); > > + if (EFI_ERROR (Status)) { > > + DEBUG (( > > + DEBUG_ERROR, > > + "%a: SD/MMC uhs signaling notifier callback failed - %r\n", > > + __FUNCTION__, > > + Status > > + )); > > + return Status; > > + } > > + } else { > > + Status =3D SdMmcHcUhsSignaling (PciIo, Slot, Timing); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > } > > I have concern for this, current existing hook points for the NotifyPhase= () > service are performing additional operations during the controller > initialization process. > > Producer of the override protocol can simply return EFI_SUCCESS, if there= is > nothing to do for a specific hook point. > > But this hook here is to override the behavior when setting the "UHS Mode > Select" field of Host Control 2 Register. If the override protocol produc= er > does not want to override the behavior at the 'EdkiiSdMmcUhsSignaling' ho= ok, > but has to do something in other hooks, one cannot directly return EFI_SU= CCESS > for the 'EdkiiSdMmcUhsSignaling' hook. For this case, one has to implemen= t the > 'EdkiiSdMmcUhsSignaling' hook even if the behavior will be exactly the sa= me as > the SdMmcPciHcDxe driver. I see your point. It's not additional code, but replacing the default. IMO the easiest way to handle it is to go back to a separate UhsSignaling callback, so that it's either omitted or explicitly defined by Override protocol producer driver. What do you think? Best regards, Marcin