Hi Ard, pt., 7 wrz 2018 o 13:29 Ard Biesheuvel <ard.biesheu...@linaro.org> napisał(a): > > On 6 September 2018 at 16:45, Ard Biesheuvel <ard.biesheu...@linaro.org> > wrote: > > On 6 September 2018 at 16:38, Marcin Wojtas <m...@semihalf.com> wrote: > >> czw., 6 wrz 2018 o 16:31 Ard Biesheuvel <ard.biesheu...@linaro.org> > >> napisał(a): > >>> > >>> On 6 September 2018 at 16:26, Marcin Wojtas <m...@semihalf.com> wrote: > >>> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel <ard.biesheu...@linaro.org> > >>> > napisał(a): > >>> >> > >>> >> On 3 September 2018 at 06:53, Marcin Wojtas <m...@semihalf.com> wrote: > >>> >> > From: Tomasz Michalec <t...@semihalf.com> > >>> >> > > >>> >> > This patch changes way the EDKII_SD_MMC_OVERRIDE protocol > >>> >> > sturcture is allocated. Using AllocateZeroPool and then > >>> >> > seting callbacks in the structure allow driver to be immune to > >>> >> > adding new callbacks in SdMmcOveride protocol in future. > >>> >> > > >>> >> > >>> >> What is the point of this patch? > >>> >> > >>> >> Statically allocating the structure will zero initialize the members > >>> >> that are not initialized explicitly, but only the members that are > >>> >> known to exist at compile time. > >>> >> > >>> > > >>> > In such case this patch is really not needed. > >>> > > >>> >> I guess the idea of this patch is to work around the latter > >>> >> limitation, but unfortunately, using sizeof(EDKII_SD_MMC_OVERRIDE) > >>> >> puts you in the exact same situation. > >>> > > >>> > If the newly added callback are zero-initialized, the situation is > >>> > fine as they won't be executed. > >>> > > >>> > >>> Yes, but this patch does not change that situation at all. > >>> > >>> So please, explain which problem is fixed by this patch? > >> > >> None, we only forgot, the static initializer will zero non-declared > >> fields by default. > >> > >>> > >>> >> > >>> >> This is the reason I added the version field. New hooks should only be > >>> >> added after incrementing the version, and calling the new hooks should > >>> >> only occur if the runtime version of the protocol implementation is > >>> >> greater than or equal to the version where those hooks were first > >>> >> introduced. > >>> >> > >>> > > >>> > So even if the given SdMmcOverride protocol callback will be NULL for > >>> > Synquacer controller, is there still a risk that anything could be > >>> > broken without the version check? > >>> > > >>> > >>> Yes. In EDK2, you can combine binary drivers with drivers build from > >>> source. If a binary driver was built against an older version of the > >>> SdMmcOverride header, it may have non-NULL values in the locations of > >>> the new methods. This patch does not help against that scenario. > >> > >> Indeed, this is why it will disappear from v2. So, when adding the new > >> callbacks, the version should be increased and checked in relevant > >> places of the main EDK2 driver, right? > >> > >> Because a couple of the new callbacks are introduced, would it be ok, > >> to increment the version only once, i.e. v2 of the SdMmcOverride will > >> support 4 new routines? > >> > > > > Yes, that is preferred in my opinion. > > > > Also, perhaps add some helper macros, e.g., > > > > #define EDKII_SD_MMC_OVERRIDE_HAVE_POST_CLOCK_FREQ_SWITCH(p) \ > > ((p)->Version >= 0x2 && (p)->SwitchClockFreqPost != NULL) > > > > so that the version handling is completely contained in the header file. > > Actually, would it be possible to define a new phase for this and use > the existing NotifyPhase hook? I know you need the timing parameter, > but I'm not thrilled by all the API changes you require there, so > perhaps we can solve that differently.
Actually the NotifyPhase was the first choice, but the problem I faced was additional parameters to be passed in the callbacks. I think adding a new generic field (VOID *) would solve the problem for xenon and all future controllers. I wanted to avoid the need of modifying your driver. Please see answer to the second question, in order to get better understanding. > > In any case, it might be useful if you could provide an overview of > all the quirks needed by Xenon There are a couple of quirks required: 1. Quirked initialization - done via existing SdMmcNotifyPhase - EdkiiSdMmcInitHostPre 2. Capabilities update depending on voltage supply, SlotType, and controller type (so called "SlowMode") - done via existing SdMmcCapability 3. Custom value of UHS Mode field in Host Control 2 Register - done with the new UhsSignaling callback. Additional Parameter needed - Timing. 4. Additional HW configuration after switching clock frequency - done with the new SwitchClockFreqPost. Additional Parameter needed - Timing. 5. BaseClockFreq = 400MHz. Maximum available in the Capability register is 255[MHz] stored in 7bit field. This is done with the new callback and new *Private structure field. If we were able to pass *Private instead of &Private->Capability[Slot], the new callback could be replaced with new usage of mOverride->Capability. However this would also force updating your driver... I hope now the Xenon demands are clear. I'm looking forward to your feedback and how you see the need of reimplementing our solutions. Best regards, Marcin _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel