Hi Hao, czw., 1 lis 2018 o 08:06 Wu, Hao A <hao.a...@intel.com> napisaĆ(a): > > Hi Marcin, > > > -----Original Message----- > > From: Marcin Wojtas [mailto:m...@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.lindh...@linaro.org; > > Wu, > > Hao A; ard.biesheu...@linaro.org; nad...@marvell.com; > > m...@semihalf.com; j...@semihalf.com; t...@semihalf.com > > Subject: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling > > to SdMmcOverride protocol > > > > From: Tomasz Michalec <t...@semihalf.com> > > > > 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 <m...@semihalf.com> > > --- > > 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 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 above > 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 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 <Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h> > > Please do not expose a module private header file here. > > One approach comes to me is to keep the SD/MMC bus mode enumeration structure > 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 <Protocol/SdMmcPassThru.h> > > > > #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 = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > Status = 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 update > > - // > > - HostCtrl2 = (UINT8)~0x7; > > - Status = 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 = BIT2; > > + Timing = SdMmcMmcDdr52; > > } else if (ClockFreq == 52) { > > - HostCtrl2 = BIT0; > > + Timing = SdMmcMmcSdr50; > > + } else if (ClockFreq == 26) { > > + Timing = SdMmcMmcSdr25; > > } else { > > - HostCtrl2 = 0; > > + Timing = SdMmcMmcSdr12; > > } > > As mentioned above, "SdMmcMmcSdr50" can be dropped here. > And considering the rename above, how about: > > if (IsDdr) { > Timing = SdMmcMmcHsDdr; > } else if (ClockFreq == 52) { > Timing = SdMmcMmcHsSdr; > } else { > Timing = SdMmcMmcLegacy; > } Ok. > > > - Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof > > (HostCtrl2), &HostCtrl2); > > - if (EFI_ERROR (Status)) { > > - return Status; > > + > > + if (mOverride != NULL && mOverride->NotifyPhase != NULL) { > > + Status = 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 = 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 producer > 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 implement 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? Best regards, Marcin _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel