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::d44; helo=mail-io1-xd44.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd44.google.com (mail-io1-xd44.google.com [IPv6:2607:f8b0:4864:20::d44]) (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 09C852118847C for ; Fri, 2 Nov 2018 05:17:06 -0700 (PDT) Received: by mail-io1-xd44.google.com with SMTP id n11-v6so1207365iob.6 for ; Fri, 02 Nov 2018 05:17:06 -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=NW+5RnWoHc40uWBrGOUaoT9BXdwSZIk52ViczpPaBwM=; b=pDdiQfk1H6zvk7U6X55zANW+GQv3dq5G6mQ0mFxGlzOclYJWJ4Kx1nWlzm8Zdbm/Ew FRp3Ad8/X0iaB4bwyXkJv59HxQZk0tZf0pf94dAs8nF/WXixIrVTSRkv0NvGVC7BABNC 4Jn9REqNRdlrOZ5vvTKB7OrLWoXYDvu/cw6zJv5PRM+uwMPK6r4ZePu2NThmX8hLRud2 cKcMcZCfQUEOQBdL0sPqxAdKgU000eQk8Yry5NfOB7WlbJtY83Dlx2ZMDHSspENu6vGK NNs1OXvk73ECfT76uXgEdA349C++IEdzR/ms/yTh2KEhsVm9i2LGAavkuJ4eltB9r/Oo oErg== 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=NW+5RnWoHc40uWBrGOUaoT9BXdwSZIk52ViczpPaBwM=; b=mTCLPzpbBJO9+L+bYNLAMiKBAZ+DabM9LfhsInFKC9WH6V2lMbwo9RT3UBuUpSrNMS StAzLtxqKIPrKu9F1syzcq6iv/DHzWPMcNvzDllnmOInY5ARdu98+k12DyUaAu7I7nV7 nXHRVBCvQI73yOyqG2x0iYiFOrgwbgnCkyDUOVWVDlnsbd5vXz2KoQpPyfI4voLQthi/ ETOt9Lc5UuJR1oIRowuYLlaP71YnnH2Y8wkTk8S4SnBFZTyeUWjj3zfYgP+RB7kDw9vf YHvkBd3zU52zl3SW1+pPm4q1jexoquI6R7jG3uqsTv42+boFgMMG3J37aywiYuleoVJv wfqA== X-Gm-Message-State: AGRZ1gI6X1eZRUn5abySpVwgM2kFdw4burB9lBPlmFDVQY2gSgjhVb0T w5hFIvY/TyMKD0rmbh5l+JNIeQzBD8+qG+PzB5JPsA== X-Google-Smtp-Source: AJdET5eoj6f23smfxZTAsxLzAIJwwnstz9Y+oTBqv9gP8d/i4bpEkFjM0672Q78iUAGuFFz4JiXh93akLPMznHHtptY= X-Received: by 2002:a6b:8ec9:: with SMTP id q192-v6mr8528940iod.248.1541161026173; Fri, 02 Nov 2018 05:17:06 -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 13:16:49 +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 12:17:07 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable pt., 2 lis 2018 o 09:21 Marcin Wojtas napisa=C5=82(a): > > 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.= org; 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 Regist= er > > > to select UHS Mode. This patch adds a new UhsSignaling type routine t= o > > > 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 = clock > > frequency for SD bus mode SDR50 is 100MHz. And there is no eMMC bus mod= e whose > > max clock frequency is at 100MHz in Embedded Multi-Media Card Electrica= l > > 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 a= bove > > comments upon HOST_CTRL2 register value definitions. Also, how about a = rename > > 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 spe= ed. > > > + > > > + @param[in] PciIo The PCI IO protocol instance. > > > + @param[in] Slot The slot number of the SD card to send t= he > > > 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 str= ucture > > in this protocol header file. SdMmcPciHcDxe driver and producers of the > > Override protocol keep a version of the HOST_CTRL2 register value macro= s 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, = BusWidth); > > > if (EFI_ERROR (Status)) { > > > @@ -758,27 +761,37 @@ EmmcSwitchToHighSpeed ( > > > return Status; > > > } > > > > > > - // > > > - // Clean UHS Mode Select field of Host Control 2 reigster before u= pdate > > > - // > > > - 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/2= 5/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, sizeo= f > > > (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 NotifyPha= se() > > service are performing additional operations during the controller > > initialization process. > > > > Producer of the override protocol can simply return EFI_SUCCESS, if the= re is > > nothing to do for a specific hook point. > > > > But this hook here is to override the behavior when setting the "UHS Mo= de > > Select" field of Host Control 2 Register. If the override protocol prod= ucer > > does not want to override the behavior at the 'EdkiiSdMmcUhsSignaling' = hook, > > but has to do something in other hooks, one cannot directly return EFI_= SUCCESS > > for the 'EdkiiSdMmcUhsSignaling' hook. For this case, one has to implem= ent the > > 'EdkiiSdMmcUhsSignaling' hook even if the behavior will be exactly the = same 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? > Second thought here, as previously I discussed a lot with Ard, how to keep only Notify and Capability callbacks. I think the best compromise would be to call SdMmcHcUhsSignaling unconditionally before SdMmcOverride callback. In case the producer does nothing, the defaults would be used. This way, in case of deviations from standard, the the producer driver would have to handle updating only affected timing values. Best regards, Marcin