Re: [edk2] [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride
Hi Ard, pt., 7 wrz 2018 o 13:29 Ard Biesheuvel napisał(a): > > On 6 September 2018 at 16:45, Ard Biesheuvel > wrote: > > On 6 September 2018 at 16:38, Marcin Wojtas wrote: > >> czw., 6 wrz 2018 o 16:31 Ard Biesheuvel > >> napisał(a): > >>> > >>> On 6 September 2018 at 16:26, Marcin Wojtas wrote: > >>> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel > >>> > napisał(a): > >>> >> > >>> >> On 3 September 2018 at 06:53, Marcin Wojtas wrote: > >>> >> > From: Tomasz Michalec > >>> >> > > >>> >> > 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 >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
Re: [edk2] [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride
On 6 September 2018 at 16:45, Ard Biesheuvel wrote: > On 6 September 2018 at 16:38, Marcin Wojtas wrote: >> czw., 6 wrz 2018 o 16:31 Ard Biesheuvel >> napisał(a): >>> >>> On 6 September 2018 at 16:26, Marcin Wojtas wrote: >>> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel >>> > napisał(a): >>> >> >>> >> On 3 September 2018 at 06:53, Marcin Wojtas wrote: >>> >> > From: Tomasz Michalec >>> >> > >>> >> > 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. In any case, it might be useful if you could provide an overview of all the quirks needed by Xenon ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride
On 6 September 2018 at 16:38, Marcin Wojtas wrote: > czw., 6 wrz 2018 o 16:31 Ard Biesheuvel > napisał(a): >> >> On 6 September 2018 at 16:26, Marcin Wojtas wrote: >> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel >> > napisał(a): >> >> >> >> On 3 September 2018 at 06:53, Marcin Wojtas wrote: >> >> > From: Tomasz Michalec >> >> > >> >> > 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. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride
czw., 6 wrz 2018 o 16:31 Ard Biesheuvel napisał(a): > > On 6 September 2018 at 16:26, Marcin Wojtas wrote: > > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel > > napisał(a): > >> > >> On 3 September 2018 at 06:53, Marcin Wojtas wrote: > >> > From: Tomasz Michalec > >> > > >> > 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? Marcin ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride
On 6 September 2018 at 16:26, Marcin Wojtas wrote: > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel > napisał(a): >> >> On 3 September 2018 at 06:53, Marcin Wojtas wrote: >> > From: Tomasz Michalec >> > >> > 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? >> >> 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. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride
czw., 6 wrz 2018 o 16:04 Ard Biesheuvel napisał(a):
>
> On 3 September 2018 at 06:53, Marcin Wojtas wrote:
> > From: Tomasz Michalec
> >
> > 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.
>
> 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?
Best regards,
Marcin
>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas
> > ---
> > Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c | 20
> > +---
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> > b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> > index e0987c9..1ad8b88 100644
> > --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> > +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> > @@ -59,6 +59,8 @@
> >
> > STATIC EFI_HANDLE mSdMmcControllerHandle;
> >
> > +STATIC EDKII_SD_MMC_OVERRIDE *mSdMmcOverride;
> > +
> > STATIC EFI_ACPI_DESCRIPTION_HEADER *mSsdt;
> > STATIC UINTNmSsdtSize;
> > STATIC VOID *mEventRegistration;
> > @@ -180,12 +182,6 @@ SynQuacerSdMmcNotifyPhase (
> >return EFI_SUCCESS;
> > }
> >
> > -STATIC EDKII_SD_MMC_OVERRIDE mSdMmcOverride = {
> > - EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION,
> > - SynQuacerSdMmcCapability,
> > - SynQuacerSdMmcNotifyPhase,
> > -};
> > -
> > STATIC
> > VOID
> > EFIAPI
> > @@ -255,10 +251,20 @@ RegisterEmmc (
> > SYNQUACER_EMMC_BASE, SYNQUACER_EMMC_BASE_SZ);
> >ASSERT_EFI_ERROR (Status);
> >
> > + mSdMmcOverride = AllocateZeroPool (sizeof (EDKII_SD_MMC_OVERRIDE));
> > + if (mSdMmcOverride == NULL) {
> > +DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> > +return EFI_OUT_OF_RESOURCES;
> > + }
> > +
> > + mSdMmcOverride->Version = EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION;
> > + mSdMmcOverride->Capability = SynQuacerSdMmcCapability;
> > + mSdMmcOverride->NotifyPhase = SynQuacerSdMmcNotifyPhase;
> > +
> >Handle = NULL;
> >Status = gBS->InstallProtocolInterface (,
> >,
> > - EFI_NATIVE_INTERFACE, (VOID **));
> > + EFI_NATIVE_INTERFACE, mSdMmcOverride);
> >ASSERT_EFI_ERROR (Status);
> >
> >return EFI_SUCCESS;
> > --
> > 2.7.4
> >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride
On 3 September 2018 at 06:53, Marcin Wojtas wrote:
> From: Tomasz Michalec
>
> 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.
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.
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.
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas
> ---
> Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c | 20
> +---
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> index e0987c9..1ad8b88 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> @@ -59,6 +59,8 @@
>
> STATIC EFI_HANDLE mSdMmcControllerHandle;
>
> +STATIC EDKII_SD_MMC_OVERRIDE *mSdMmcOverride;
> +
> STATIC EFI_ACPI_DESCRIPTION_HEADER *mSsdt;
> STATIC UINTNmSsdtSize;
> STATIC VOID *mEventRegistration;
> @@ -180,12 +182,6 @@ SynQuacerSdMmcNotifyPhase (
>return EFI_SUCCESS;
> }
>
> -STATIC EDKII_SD_MMC_OVERRIDE mSdMmcOverride = {
> - EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION,
> - SynQuacerSdMmcCapability,
> - SynQuacerSdMmcNotifyPhase,
> -};
> -
> STATIC
> VOID
> EFIAPI
> @@ -255,10 +251,20 @@ RegisterEmmc (
> SYNQUACER_EMMC_BASE, SYNQUACER_EMMC_BASE_SZ);
>ASSERT_EFI_ERROR (Status);
>
> + mSdMmcOverride = AllocateZeroPool (sizeof (EDKII_SD_MMC_OVERRIDE));
> + if (mSdMmcOverride == NULL) {
> +DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +return EFI_OUT_OF_RESOURCES;
> + }
> +
> + mSdMmcOverride->Version = EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION;
> + mSdMmcOverride->Capability = SynQuacerSdMmcCapability;
> + mSdMmcOverride->NotifyPhase = SynQuacerSdMmcNotifyPhase;
> +
>Handle = NULL;
>Status = gBS->InstallProtocolInterface (,
>,
> - EFI_NATIVE_INTERFACE, (VOID **));
> + EFI_NATIVE_INTERFACE, mSdMmcOverride);
>ASSERT_EFI_ERROR (Status);
>
>return EFI_SUCCESS;
> --
> 2.7.4
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
