From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::243; helo=mail-wr0-x243.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x243.google.com (mail-wr0-x243.google.com [IPv6:2a00:1450:400c:c0c::243]) (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 6B67C2034A890 for ; Fri, 27 Oct 2017 07:48:01 -0700 (PDT) Received: by mail-wr0-x243.google.com with SMTP id k62so6398811wrc.9 for ; Fri, 27 Oct 2017 07:51:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=J298wCqELIGKxWcFQHJMwJRIPAtajr2RbwPiXSdW9z8=; b=eecErEjag6E2AhMa5D8KjosC/IxgDKN1a60F/w9TfttsY+sXiiEmGAOqoFCFOSsUZ4 HLEYWvPExz6Gz+cjbiaPH3OWg4DOHjzm9Oao5Pc/qwfl9xCHXp5IS3bLO+LlaGft/Ja9 QH62f6N4oMPG+9k+lqpRwjMxMkv9HI2YxpIpo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=J298wCqELIGKxWcFQHJMwJRIPAtajr2RbwPiXSdW9z8=; b=nZuRBdDU9Xl7p7tkhfeAQgSa5qeItNbll2k6mlGVPWAp+ro0tmKzHOE4Vk6rkV7STj e6uAj9FCzMjDxyHdsESvegLgP3HKEBeG4Ow6o5fefMhA7vtzfVSaAV43oHPLuYJp9Lg4 +AyvYCtmnaRV2jKAJ5AD8gYzBb/u7vvI6Qtp/8Wg93aXYe88GU9N4DzRqcMniIlkl3o9 PjvdukK4dtYjyzBCi0o/vpG7rN3SPZsC8KhUuOXone7wyUoSaZvscLCJ1G7/vo4eEb54 3Giqk8oyBqTg6lw7Bf8gSbik1mt4upIrGbfGjkoe8cFs+aTt70+Vm29UPP4M/A8OnkAN bjKw== X-Gm-Message-State: AMCzsaX41S8qdUnOf1mW6vx2/9zepmcUyf/1vCA/uXiytWpWQbcVUIYe Nc6IBYl5YIDcndAfNmC8NtG/+A== X-Google-Smtp-Source: ABhQp+RcCn97FTb+MpEAI18eVNhN+t71yJLCEhdI87sMkegoEC/Sc7WJkATDg4ysegVX4n/1O5SlKA== X-Received: by 10.223.170.139 with SMTP id h11mr707309wrc.167.1509115907081; Fri, 27 Oct 2017 07:51:47 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id y144sm1237071wmd.18.2017.10.27.07.51.45 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 27 Oct 2017 07:51:46 -0700 (PDT) Date: Fri, 27 Oct 2017 15:51:44 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, neta@marvell.com, kostap@marvell.com, jinghua@marvell.com, jsd@semihalf.com Message-ID: <20171027145144.v26za4zpw4rh5tuo@bivouac.eciton.net> References: <1509066832-5285-1-git-send-email-mw@semihalf.com> <1509066832-5285-10-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1509066832-5285-10-git-send-email-mw@semihalf.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH v2 09/10] Marvell/Drivers: XenonDxe: Allow overriding 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: Fri, 27 Oct 2017 14:48:01 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Oct 27, 2017 at 03:13:51AM +0200, Marcin Wojtas wrote: > Some SdMmc host controllers are run by clocks with different > frequency than it is reflected in Capabilities Register 1. > Because the bitfield is only 8 bits wide, a maximum value > that could be obtained from hardware is 255(MHz). > > In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq > member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient > to be used for setting the clock speed in SdMmcHcClockSupply > function. > > This patch adds new UINT32 array ('BaseClkFreq[]') to > SD_MMC_HC_PRIVATE_DATA structure for specifying > the input clock speed for each slot of the host controller. > All routines that are used for clock configuration are > updated accordingly. > > Thanks to above the Xenon host controller driver > could be modified to configure clock speed relatively > to actual 400MHz input. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marcin Wojtas > --- > Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c | 4 ++-- > Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c | 4 ++-- > Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c | 13 ++++++++---- > Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h | 6 ++++++ > Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 22 ++++++++++---------- > Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h | 12 ++++++----- > 6 files changed, 37 insertions(+), 24 deletions(-) > > diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c > index 4d4833f..530a01c 100755 > --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c > +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/EmmcDevice.c > @@ -705,7 +705,7 @@ EmmcSwitchClockFreq ( > // > // Convert the clock freq unit from MHz to KHz. > // > - Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]); > + Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]); > > return Status; > } > @@ -1007,7 +1007,7 @@ EmmcSetBusMode ( > return Status; > } > > - ASSERT (Private->Capability[Slot].BaseClkFreq != 0); > + ASSERT (Private->BaseClkFreq[Slot] != 0); > // > // Check if the Host Controller support 8bits bus width. > // > diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c > index 9122848..ea7eed7 100644 > --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c > +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdDevice.c > @@ -972,7 +972,7 @@ SdCardSetBusMode ( > return Status; > } > > - Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability); > + Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]); > if (EFI_ERROR (Status)) { > return Status; > } > @@ -1144,7 +1144,7 @@ SdCardIdentification ( > goto Error; > } > > - SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]); > + SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]); > > gBS->Stall (1000); > > diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c > index 981eab5..10e15c5 100644 > --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c > +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.c > @@ -291,7 +291,10 @@ SdMmcPciHcEnumerateDevice ( > // > // Reinitialize slot and restart identification process for the new attached device > // > - Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]); > + Status = SdMmcHcInitHost (Private->PciIo, > + Slot, > + Private->Capability[Slot], > + Private->BaseClkFreq[Slot]); > if (EFI_ERROR (Status)) { > continue; > } > @@ -617,9 +620,11 @@ SdMmcPciHcDriverBindingStart ( > Private->Capability[Slot].Sdr50 = 0; > Private->Capability[Slot].BusWidth8 = 0; > > - if (Private->Capability[Slot].BaseClkFreq == 0) { > - Private->Capability[Slot].BaseClkFreq = 0xff; > - } > + // > + // Override inappropriate base clock frequency from Capabilities Register 1. > + // Actual clock speed of Xenon controller is 400MHz. > + // > + Private->BaseClkFreq[Slot] = XENON_MMC_MAX_CLK / 1000 / 1000; > > DumpCapabilityReg (Slot, &Private->Capability[Slot]); > > diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h > index 6a2a279..067b9ac 100644 > --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h > +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHcDxe.h > @@ -115,6 +115,12 @@ typedef struct { > UINT64 MaxCurrent[SD_MMC_HC_MAX_SLOT]; > > UINT32 ControllerVersion; > + > + // > + // Some controllers may require to override base clock frequency > + // value stored in Capabilities Register 1. > + // > + UINT32 BaseClkFreq[SD_MMC_HC_MAX_SLOT]; > } SD_MMC_HC_PRIVATE_DATA; > > #define SD_MMC_HC_TRB_SIG SIGNATURE_32 ('T', 'R', 'B', 'T') > diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c > index ccbf355..706618d 100644 > --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c > +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c If doing this rework, there should probably be an addition to DumpCapabilityReg to at least point out that Capability->BaseClkFreq is ignored, rather than just printing the (now unused) value. Otherwise, this looks sort of OK. / Leif > @@ -678,7 +678,7 @@ SdMmcHcStopClock ( > @param[in] PciIo The PCI IO protocol instance. > @param[in] Slot The slot number of the SD card to send the command to. > @param[in] ClockFreq The max clock frequency to be set. The unit is KHz. > - @param[in] Capability The capability of the slot. > + @param[in] BaseClkFreq The base clock frequency of host controller in MHz. > > @retval EFI_SUCCESS The clock is supplied successfully. > @retval Others The clock isn't supplied successfully. > @@ -689,11 +689,10 @@ SdMmcHcClockSupply ( > IN EFI_PCI_IO_PROTOCOL *PciIo, > IN UINT8 Slot, > IN UINT64 ClockFreq, > - IN SD_MMC_HC_SLOT_CAP Capability > + IN UINT32 BaseClkFreq > ) > { > EFI_STATUS Status; > - UINT32 BaseClkFreq; > UINT32 SettingFreq; > UINT32 Divisor; > UINT32 Remainder; > @@ -703,9 +702,8 @@ SdMmcHcClockSupply ( > // > // Calculate a divisor for SD clock frequency > // > - ASSERT (Capability.BaseClkFreq != 0); > + ASSERT (BaseClkFreq != 0); > > - BaseClkFreq = Capability.BaseClkFreq; > if (ClockFreq == 0) { > return EFI_INVALID_PARAMETER; > } > @@ -896,7 +894,7 @@ SdMmcHcSetBusWidth ( > > @param[in] PciIo The PCI IO protocol instance. > @param[in] Slot The slot number of the SD card to send the command to. > - @param[in] Capability The capability of the slot. > + @param[in] BaseClkFreq The base clock frequency of host controller in MHz. > > @retval EFI_SUCCESS The clock is supplied successfully. > @retval Others The clock isn't supplied successfully. > @@ -906,7 +904,7 @@ EFI_STATUS > SdMmcHcInitClockFreq ( > IN EFI_PCI_IO_PROTOCOL *PciIo, > IN UINT8 Slot, > - IN SD_MMC_HC_SLOT_CAP Capability > + IN UINT32 BaseClkFreq > ) > { > EFI_STATUS Status; > @@ -915,7 +913,7 @@ SdMmcHcInitClockFreq ( > // > // Calculate a divisor for SD clock frequency > // > - if (Capability.BaseClkFreq == 0) { > + if (BaseClkFreq == 0) { > // > // Don't support get Base Clock Frequency information via another method > // > @@ -925,7 +923,7 @@ SdMmcHcInitClockFreq ( > // Supply 400KHz clock frequency at initialization phase. > // > InitFreq = 400; > - Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability); > + Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq); > return Status; > } > > @@ -1024,6 +1022,7 @@ SdMmcHcInitTimeoutCtrl ( > @param[in] PciIo The PCI IO protocol instance. > @param[in] Slot The slot number of the SD card to send the command to. > @param[in] Capability The capability of the slot. > + @param[in] BaseClkFreq The base clock frequency of host controller in MHz. > > @retval EFI_SUCCESS The host controller is initialized successfully. > @retval Others The host controller isn't initialized successfully. > @@ -1033,12 +1032,13 @@ EFI_STATUS > SdMmcHcInitHost ( > IN EFI_PCI_IO_PROTOCOL *PciIo, > IN UINT8 Slot, > - IN SD_MMC_HC_SLOT_CAP Capability > + IN SD_MMC_HC_SLOT_CAP Capability, > + IN UINT32 BaseClkFreq > ) > { > EFI_STATUS Status; > > - Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability); > + Status = SdMmcHcInitClockFreq (PciIo, Slot, BaseClkFreq); > if (EFI_ERROR (Status)) { > return Status; > } > diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h > index fb62758..a4ec4fe 100644 > --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h > +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.h > @@ -414,7 +414,7 @@ SdMmcHcStopClock ( > @param[in] PciIo The PCI IO protocol instance. > @param[in] Slot The slot number of the SD card to send the command to. > @param[in] ClockFreq The max clock frequency to be set. The unit is KHz. > - @param[in] Capability The capability of the slot. > + @param[in] BaseClkFreq The base clock frequency of host controller in MHz. > > @retval EFI_SUCCESS The clock is supplied successfully. > @retval Others The clock isn't supplied successfully. > @@ -425,7 +425,7 @@ SdMmcHcClockSupply ( > IN EFI_PCI_IO_PROTOCOL *PciIo, > IN UINT8 Slot, > IN UINT64 ClockFreq, > - IN SD_MMC_HC_SLOT_CAP Capability > + IN UINT32 BaseClockFreq > ); > > /** > @@ -473,7 +473,7 @@ SdMmcHcSetBusWidth ( > > @param[in] PciIo The PCI IO protocol instance. > @param[in] Slot The slot number of the SD card to send the command to. > - @param[in] Capability The capability of the slot. > + @param[in] BaseClkFreq The base clock frequency of host controller in MHz. > > @retval EFI_SUCCESS The clock is supplied successfully. > @retval Others The clock isn't supplied successfully. > @@ -483,7 +483,7 @@ EFI_STATUS > SdMmcHcInitClockFreq ( > IN EFI_PCI_IO_PROTOCOL *PciIo, > IN UINT8 Slot, > - IN SD_MMC_HC_SLOT_CAP Capability > + IN UINT32 BaseClockFreq > ); > > /** > @@ -531,6 +531,7 @@ SdMmcHcInitTimeoutCtrl ( > @param[in] PciIo The PCI IO protocol instance. > @param[in] Slot The slot number of the SD card to send the command to. > @param[in] Capability The capability of the slot. > + @param[in] BaseClkFreq The base clock frequency of host controller in MHz. > > @retval EFI_SUCCESS The host controller is initialized successfully. > @retval Others The host controller isn't initialized successfully. > @@ -540,7 +541,8 @@ EFI_STATUS > SdMmcHcInitHost ( > IN EFI_PCI_IO_PROTOCOL *PciIo, > IN UINT8 Slot, > - IN SD_MMC_HC_SLOT_CAP Capability > + IN SD_MMC_HC_SLOT_CAP Capability, > + IN UINT32 BaseClockFreq > ); > > #endif > -- > 2.7.4 >