Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754889AbcKYPFm (ORCPT ); Fri, 25 Nov 2016 10:05:42 -0500 Received: from mga04.intel.com ([192.55.52.120]:50970 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754602AbcKYPFf (ORCPT ); Fri, 25 Nov 2016 10:05:35 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,547,1473145200"; d="scan'208";a="905406665" Subject: Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality To: Ulf Hansson , Ziji Hu References: <0390e7a05b6163deabb545f93729ea615eeaaee2.1477911954.git-series.gregory.clement@free-electrons.com> Cc: Gregory CLEMENT , "linux-mmc@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Rob Herring , "devicetree@vger.kernel.org" , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Jimmy Xu , Jisheng Zhang , Nadav Haklai , Ryan Gao , Doug Jones , Victor Gu , "Wei(SOCP) Liu" , Wilson Ding , Romain Perier , Yehuda Yitschak , Marcin Wojtas , Hanna Hawa , Kostya Porotchkin , "linux-kernel@vger.kernel.org" , Zhen Huang From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <88cb424b-7fbb-625f-1d83-be3418d4ae29@intel.com> Date: Fri, 25 Nov 2016 16:04:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1661 Lines: 37 On 24/11/16 15:34, Ulf Hansson wrote: > On 24 November 2016 at 13:41, Ziji Hu wrote: >> On 2016/11/24 18:43, Ulf Hansson wrote: >>> On 31 October 2016 at 12:09, Gregory CLEMENT >>> wrote: >>>> From: Ziji Hu >>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc, >>>> + struct mmc_ios *ios) >>>> +{ >>>> + struct sdhci_host *host = mmc_priv(mmc); >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); >>>> + >>>> + /* >>>> + * Before SD/SDIO set signal voltage, SD bus clock should be >>>> + * disabled. However, sdhci_set_clock will also disable the Internal >>>> + * clock in mmc_set_signal_voltage(). >>> >>> If that's the case then that is wrong in the generic sdhci code. >>> What's the reason why it can't be fixed there instead of having this >>> workaround? >>> >> In my very own opinion, SD Spec doesn't specify whether SDCLK should be >> enabled or not during power setting. >> Enabling SDCLK might be a special condition only required by our SDHC. >> I try to avoid breaking other vendors' SDHC functionality >> if their SDHCs require SDCLK disabled. >> Thus I prefer to keep it inside our SDHC driver. > > I let Adrian comment on this. > > For sure we should avoid breaking other sdhci variant, but on the > other hand *if* the generic code is wrong we should fix it! Yes, this looks like something that could perhaps be fixed in sdhci. I will look into it.