Re: [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride

[Marcin Wojtas <mw@semihalf.com>]

Hi Ard,

pt., 7 wrz 2018 o 13:29 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> On 6 September 2018 at 16:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 6 September 2018 at 16:38, Marcin Wojtas <mw@semihalf.com> wrote:
> >> czw., 6 wrz 2018 o 16:31 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
> >>>
> >>> On 6 September 2018 at 16:26, Marcin Wojtas <mw@semihalf.com> wrote:
> >>> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
> >>> >>
> >>> >> On 3 September 2018 at 06:53, Marcin Wojtas <mw@semihalf.com> wrote:
> >>> >> > From: Tomasz Michalec <tm@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