On Tue, Jan 30, 2018 at 11:14:31AM +0000, Ard Biesheuvel wrote: > On 30 January 2018 at 11:00, Leif Lindholm <leif.lindh...@linaro.org> wrote: > > On Tue, Jan 30, 2018 at 10:32:40AM +0000, Ard Biesheuvel wrote: > >> Implement support for the SynQuacer eMMC controller. This involves an > >> implementation of the SD/MMC override protocol to handle a couple of > >> quirks that would otherwise prevent this IP from being driven by the > >> generic SDHCI driver. > >> > >> Also, add a HII page to the PlatformDxe driver that allows eMMC support > >> to be enabled, and wire it up for both DeveloperBox and EVB. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > >> --- > >> Now that the core support for the SD/MMC override protocol is finally > >> merged, resubmit this again. I dropped Leif's R-b given that I have > >> now added DeveloperBox, as well as a HII option to enable eMMC. > > > > Couple of minor comments/suggestions below and a question. > > > >> Platform/Socionext/DeveloperBox/DeveloperBox.dsc > >> | 8 + > >> Platform/Socionext/DeveloperBox/DeveloperBox.fdf > >> | 7 + > >> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc > >> | 8 + > >> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf > >> | 7 + > >> Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi > >> | 1 - > >> Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts > >> | 4 - > >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c > >> | 203 ++++++++++++++++++++ > >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c > >> | 5 + > >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h > >> | 9 + > >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf > >> | 4 + > >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni > >> | 6 + > >> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr > >> | 8 + > >> Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h > >> | 6 +- > >> > >> Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c > >> | 23 ++- > >> > >> Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf > >> | 1 + > >> 15 files changed, 287 insertions(+), 13 deletions(-) > >> > > > >> +/** > >> + > >> + Override function for SDHCI capability bits > >> + > >> + @param[in] PassThru A pointer to the > >> + EFI_SD_MMC_PASS_THRU_PROTOCOL > >> instance. > >> + @param[in] ControllerHandle The EFI_HANDLE of the controller. > >> + @param[in] Slot The 0 based slot index. > >> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure. > >> + > >> + @retval EFI_SUCCESS The override function completed > >> successfully. > >> + @retval EFI_NOT_FOUND The specified controller or slot does not > >> exist. > >> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL > >> + > >> +**/ > >> +STATIC > >> +EFI_STATUS > >> +EFIAPI > >> +SynQuacerSdMmcCapability ( > >> + IN EFI_HANDLE ControllerHandle, > >> + IN UINT8 Slot, > >> + IN OUT VOID *SdMmcHcSlotCapability > >> + ) > >> +{ > >> + UINT64 Capability; > >> + > >> + if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) { > > > > This test pattern repeats below, does it suggest a macro? > > > > I don't see how that would clear things up tbh, and the pattern occurs > only twice > > #define IS_OUR_QUIRKY_SDMMC_CONTROLLER(Handle, Slot) \ > ((Handle) == mSdMmcControllerHandle && (Slot) == 0) > > if (!IS_OUR_QUIRKY_SDMMC_CONTROLLER(ControllerHandle, Slot) { > return EFI_SUCCESS; > } > > I can change it if you want, or add a comment if the condition is not > self-explanatory enough.
It's just an awful lot of logical operations on a single line. 'ControllerHandle != mSdMmcControllerHandle' is reasonably easy to figure out, but '|| Slot != 0' looks a bit random there. A comment would be sufficient. Another option would be to reduce the number of logical operations by two by doing if (ControllerHandle == mSdMmcControllerHandle && Slot == 0) { and doing the body inside the if-statement. That's a bit uglier in the next function, but I would expect it follows the paradigm of "handle most likely case first"? > >> diff --git > >> a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni > >> b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni > >> index b274d12ed2c6..2eca8bbba8c3 100644 > >> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni > >> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni > >> @@ -27,3 +27,9 @@ > >> > >> #string STR_PCIE_MAX_SPEED_UNLIMITED #language en-US "Unlimited" > >> #string STR_PCIE_MAX_SPEED_GEN1 #language en-US "Gen1 (2.5 GT/s)" > >> + > >> +#string STR_EMMC_ENABLE_PROMPT #language en-US "Enable on-board > >> eMMC" > >> +#string STR_EMMC_ENABLE_HELP #language en-US "Enable the > >> on-board eMMC for booting and for use by the OS." > >> + > >> +#string STR_EMMC_DISABLED #language en-US "Disabled" > >> +#string STR_EMMC_ENABLED #language en-US "Enabled" > > > > Perhaps a random question, but ... > > Why am I seeing this in cleartext in the patch? Is it really a unicode file? > > Given that we support UTF-8 in .uni files these days, and the fact > that all characters used are in the 7-bit ASCII range, it doesn't > really make a difference, I guess. Fair enough. / Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel