Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754228AbcKYNG1 (ORCPT ); Fri, 25 Nov 2016 08:06:27 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:36073 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753481AbcKYNGU (ORCPT ); Fri, 25 Nov 2016 08:06:20 -0500 MIME-Version: 1.0 In-Reply-To: <07e71ce4-a78e-750c-6325-0a88891519d5@marvell.com> References: <0390e7a05b6163deabb545f93729ea615eeaaee2.1477911954.git-series.gregory.clement@free-electrons.com> <76ce72f8-4b86-3b83-544f-b9a7ef871393@marvell.com> <07e71ce4-a78e-750c-6325-0a88891519d5@marvell.com> From: Ulf Hansson Date: Fri, 25 Nov 2016 14:06:17 +0100 Message-ID: Subject: Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality To: Ziji Hu Cc: Adrian Hunter , 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" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4411 Lines: 106 [...] >>>>>> + >>>>>> + /* >>>>>> + * Xenon Specific property: >>>>>> + * emmc: explicitly indicate whether this slot is for eMMC >>>>>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register >>>>>> + * tun-count: the interval between re-tuning >>>>>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1" >>>>>> + */ >>>>>> + if (of_property_read_bool(np, "marvell,xenon-emmc")) >>>>>> + priv->emmc_slot = true; >>>>> >>>>> So, you need this because of the eMMC voltage switch behaviour, right? >>>>> >>>>> Then I would rather like to describe this a generic DT bindings for >>>>> the eMMC voltage level support. There have acutally been some earlier >>>>> discussions for this, but we haven't yet made some changes. >>>>> >>>>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set, >>>>> allows the host driver to accept I/O voltage switches to 3.3V. If not >>>>> supported the ->start_signal_voltage_switch() ops may return -EINVAL. >>>>> This would inform the mmc core to move on to the next supported >>>>> voltage level. There might be some minor additional changes to the mmc >>>>> card initialization sequence, but those should be simple. >>>>> >>>>> I can help out to look into this, unless you want to do it yourself of course!? >>>>> >>>> Yes. One of the reasons is to provide eMMC specific voltage setting. >>>> But in my very own opinion, it should be irrelevant to voltage level. >>>> The eMMC voltage setting on our SDHC is different from SD/SDIO voltage switch. >>>> It will become more complex with different SOC implementation details. >>> >>> Got it. Although I think we can cope with that fine just by using the >>> different SD/eMMC speed modes settings defined in DT (or from the >>> SDHCI caps register) >>> >> In my very opinion, I'm not sure if there is any corner case that driver cannot >> determine the eMMC card type from DT and SDHC caps. >> >>>> Unfortunately, MMC driver cannot determine the card type yet when eMMC voltage >>>> setting should be executed. >>>> Thus an flag is required here to tell driver to execute eMMC voltage setting. >>>> >>>> Besides, additional eMMC specific settings might be implemented in future, besides >>>> voltage setting. Most of them should be completed before MMC driver recognizes the >>>> card type. Thus I have to keep this flag to indicate current SDHC is for eMMC. >>> >>> I doubt you will need a generic "eMMC" flag, but let's see when we go forward. >>> >>> Currently it's clear you don't need such a flag, so I will submit a >>> change adding a DT binding for "mmc-ddr-3_3v" then we can take it from >>> there, to see if it suits your needs. >>> > > Another reason for a special "xenon-emmc" property is that our host IP usually can > support both eMMC and SD. Whether a host is used as eMMC or SD depends on the > final implementation of the actual product. > Thus our host driver needs to know whether current SDHC is fixed as eMMC or SD. > So far, It can only get the information from DT. As a matter of fact for mounted non-removable cards, such as eMMC, we already have the option to describe some of their characteristics in DT. Perhaps that's what you need? Please have a look at: Documentation/devicetree/bindings/mmc/mmc-card.txt > > After out host driver get the card type information from DT, it can prepare eMMC > specific voltage, set eMMC specific mmc->caps/caps2 flags and do other > vendor specific init, before card init procedure. > Otherwise, our host driver has to wait until card type is determined in mmc_rescan(). > > A generic "eMMC" flag is unnecessary. I just require a private property, > which is only used in our host driver and DT. > > Thank you. > > Best regards, > Hu Ziji > >> >> Actually, our eMMC is usually fixed as 1.8V. >> >> The pair "no-sd" + "no-sdio" can provide the similar information. >> But I'm not sure if it is proper to use those two property in such a way. >> >> Thank you. >> >> Best regards >> Hu Ziji >> >>> [...] >>> >>> Kind regards >>> Uffe >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Kind regards Uffe