From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8AC7D21125010 for ; Fri, 7 Sep 2018 06:30:24 -0700 (PDT) Received: by mail-it0-x241.google.com with SMTP id h1-v6so19875316itj.4 for ; Fri, 07 Sep 2018 06:30:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=T65nYiGzYFpXlbYzbKfXeRVIufA2aUxgINZ+OhzQt+k=; b=pX2UyMqwo/hRim7i9NOS1+0BNhbJr3YmYdihpeKu8ctIzWmEhGznjdes52aDk46MOm XI/AghyCZ6enruG/WqS4YT5GmcMCAzE687yr5UbBwOG+mMTDstT2NCznBNYndr6ayXH7 jLfxFNveZdEFmq2TMMELIfRFb8xJ3/u4QdxNfQA1s4HZ27vI0LwgKy0BaC3o3NLJukeY WbyDt7F/TYnsGNtWO3mFKTyaD1MWQ5vHJQvxZidpHjYSUTQwPGBgf+3ZOQBJ2R3YZCZQ AxbXX1oZdxwhaR9DF+UEadrEjIu0c+Eniun2+q9NDlo2M1unzqzFSDQnzfhg/WQcUTI6 XoNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=T65nYiGzYFpXlbYzbKfXeRVIufA2aUxgINZ+OhzQt+k=; b=LTzrgOXaTjv/5EEdUUvVvYqNtSCdqdrU1Vf2TBDassdvru5puPF+cr7tBG3d8zylk9 qNLKXrlxU2hrjMPjicIWi4qxcU4dOomoe+WgoLxiD8H2jIkCtv8vb8EWRqVnUhh+4kJS zyC7dZuZmKwsJbqymuWWg8t0hlTwnQNkvae1speOu7M8Y4tu9sV9foAFLHBLqrMvdZrV gys092DgomAf/yst3SiCTlckzkxx3eNZy7QwGcEwfjhll8OFmrRVj7gJ1TWQiRVjQJqp i+qB6gE1ia4R5kpzFWW6V2Jrqfti2x3dh0EzVfVSskDAUus0C8UHf6ZunDpMHrUGQx1K JY3w== X-Gm-Message-State: APzg51DdMzKZYcOXzwUhlJ+D3RR7ZN0KzgZeutfUPehoMdtqCv2Ku6wJ LUrOQA8K1g54C14VB9ter3N9/QudGqVME4ZWZy5moQ== X-Google-Smtp-Source: ANB0VdYHzCMIO+aVVxv08JURZYrTc65u9H1vzMmqERkLVzN1ffVdmtJxLgT125aIUkFIg4fxi9HOBdXupaDofNAPglA= X-Received: by 2002:a24:e084:: with SMTP id c126-v6mr6837003ith.136.1536327023113; Fri, 07 Sep 2018 06:30:23 -0700 (PDT) MIME-Version: 1.0 References: <1535950443-27106-1-git-send-email-mw@semihalf.com> <1535950443-27106-2-git-send-email-mw@semihalf.com> In-Reply-To: From: Marcin Wojtas Date: Fri, 7 Sep 2018 15:30:08 +0200 Message-ID: To: Ard Biesheuvel Cc: edk2-devel-01 , Leif Lindholm , nadavh@marvell.com, "jsd@semihalf.com" , Grzegorz Jaszczyk , Tomasz Michalec Subject: Re: [platforms: PATCH 1/7] Silicon/SynQuacer/PlatformDxe: Modify initialization of SdMmcOverride X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Sep 2018 13:30:24 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Ard, pt., 7 wrz 2018 o 13:29 Ard Biesheuvel napisa= =C5=82(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 na= pisa=C5=82(a): > >>> > >>> On 6 September 2018 at 16:26, Marcin Wojtas wrote: > >>> > czw., 6 wrz 2018 o 16:04 Ard Biesheuvel = napisa=C5=82(a): > >>> >> > >>> >> On 3 September 2018 at 06:53, Marcin Wojtas wrot= e: > >>> >> > 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 membe= rs > >>> >> 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 onl= y be > >>> >> added after incrementing the version, and calling the new hooks sh= ould > >>> >> only occur if the runtime version of the protocol implementation i= s > >>> >> greater than or equal to the version where those hooks were first > >>> >> introduced. > >>> >> > >>> > > >>> > So even if the given SdMmcOverride protocol callback will be NULL f= or > >>> > 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 >=3D 0x2 && (p)->SwitchClockFreqPost != =3D 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 =3D 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