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:c06::231; helo=mail-io0-x231.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (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 01888203525EE for ; Thu, 26 Oct 2017 08:03:29 -0700 (PDT) Received: by mail-io0-x231.google.com with SMTP id 97so6057364iok.7 for ; Thu, 26 Oct 2017 08:07:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=5NkEvEg4YECOOfR2RUSOTbphZQC1RRwv9kmCH13VXCg=; b=wtlyCsmgnZeILqxE3zcg3/8HXA+OkYAiT7w5l7gZJuG6o42bBsjRXchKudmMPLA0jJ 1WYqgVGrG6ZBUbZt9Rl3X5j5VSHu5AUmp+PgUZY/qv0yK+gMDWZ7srOyIJU22rR6ujfd OANea+WzPJG5YnL1Ia2VptQEzyjRmmfCXNQDDizQgEdeHIc4BM6Ku3zm0zLTiAdvPopG HdR+VdsME6X1xMQ3q8Io7ddsHjPl5bCkN8NoMn7G9C3kUB7ysF7QMQPGKmxJRXKBxGNi uFbAPhoI3KeGToYngFCZahP/MHph5lTHZoYSp+Sa+8pI7UTpr4KjDGFvQvtzGY/7AZE8 qsRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=5NkEvEg4YECOOfR2RUSOTbphZQC1RRwv9kmCH13VXCg=; b=Ktue/UALpQHhdUPvNKL7GaHjt2bxAzxBrFq6EEd//e7a0xivDmOAghVP2RRG44L1MX F6LDmafot+tVupbyeR/pgjVUrVhNvrwdtDJybEnKacrfVBViZhe7zKObV0WPURC18rb4 EWfNJkoV0Z4WEmsYwEbQz2WKa3bumOLf68JayL0+MrkkHPJIz0XX92aC3oSXotRx9yok DyKQRVJw44OxwQSfU5g7jpxhQpVDKwd7GNtzx3/XjYZv719f5N4KP0CgVvVUxXd+MuwQ bvQojoe3o5690IFP6AiEPfVtB/NEVOILErn80Ha6OT78bybzR4G5oNZ+VT34R1mPoTeM WZJg== X-Gm-Message-State: AMCzsaVnOgJSNZ58dRUYfpiMNtX4oNqJ8we6y+8N1huixqBTCJI8+6UH Yla9OThHFrKsD20knJL+UfTHpzmQht+Obo0Z/4ulGg== X-Google-Smtp-Source: ABhQp+RiVibjb8DGCmHf4kcHnlr//XC8Jh5Ga2EgZfGCAgMqwB8fSzUaFKYRJ2SCu5xy5ijSwzN6C8qjVLMow+zLelI= X-Received: by 10.36.110.71 with SMTP id w68mr2996886itc.113.1509030435024; Thu, 26 Oct 2017 08:07:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.167.208 with HTTP; Thu, 26 Oct 2017 08:07:14 -0700 (PDT) In-Reply-To: <20171026145242.zyzx7tysxq653sex@bivouac.eciton.net> References: <1508980777-29006-1-git-send-email-mw@semihalf.com> <1508980777-29006-10-git-send-email-mw@semihalf.com> <20171026134612.nro2lhy2l3qvm7pq@bivouac.eciton.net> <20171026140200.v33dpi54ri2e3fuu@bivouac.eciton.net> <20171026145242.zyzx7tysxq653sex@bivouac.eciton.net> From: Marcin Wojtas Date: Thu, 26 Oct 2017 17:07:14 +0200 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan Subject: Re: [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Oct 2017 15:03:30 -0000 Content-Type: text/plain; charset="UTF-8" 2017-10-26 16:52 GMT+02:00 Leif Lindholm : > On Thu, Oct 26, 2017 at 04:29:39PM +0200, Marcin Wojtas wrote: >> 2017-10-26 16:02 GMT+02:00 Leif Lindholm : >> >> > Why is Capability.BaseClkFreq the wrong frequency to use? >> >> >> >> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz. >> >> An alternative would be change this generic type to UINT16 and update >> >> field properly during initialization - do you prefer that? >> > >> > No, I'm still dreaming we might be able to reintegrate this into the >> > MdeModulePkg driver in some glorious future. >> >> Yes, that would be great. I imagine some SDMMC_HOST_PROTOCOL exposing >> callbacks to set UHS, custom clock handling, etc (like it's done in >> the Linux). > > Yes, *waves hands*, something like that. Just 'a bit' spare time needed. > >> > So what you are basically saying is that this controller is running at >> > a higher frequency than is permitted (or even describable) by the >> > specification? If so, _that_ needs to be in the commit message (and >> > really, a comment by the code as well). >> >> Yes, this clock value is Xenon controller's quirk. I will mention it >> in commit log/comment, but please let know if you wish me to: >> a. extend BaseClkFreq to UINT16 and configure it properly during init >> b. leave as is with better description only >> I lean towards a., it's a very low-cost quirk to be applied. > > I would actually lean towards b. That way it stands out and is less > likely to actually _confuse_ someone in the future. > If we dream about possible merge with edk2 original code, I'd really suggest not to spoil (well, I'm criticizing my own patch:)) the SdMmcHcClockSupply and continue to rely on BaseClkFreq field there. This way the code can be common for various hosts. Let's take look at linux - there is SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN flag, thanks to which the host controller driver can override the capability register value during initialization. Most common method here ends up in clk_get_rate () call and this quirk is used by overall 10 different controllers. Modifying BaseClkFreq type and assignment during driver initialization (if needed) will be IMO better for maintenance than creating a custom version of SdMmcHcClockSupply routine. Is there any chance I might have convinced you?:) Thanks, Marcin