Date:   Fri, 25 Nov 2016 14:06:17 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Ziji Hu <huziji@...vell.com>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Gregory CLEMENT <gregory.clement@...e-electrons.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Jimmy Xu <zmxu@...vell.com>,
        Jisheng Zhang <jszhang@...vell.com>,
        Nadav Haklai <nadavh@...vell.com>, Ryan Gao <ygao@...vell.com>,
        Doug Jones <dougj@...vell.com>, Victor Gu <xigu@...vell.com>,
        "Wei(SOCP) Liu" <liuw@...vell.com>,
        Wilson Ding <dingwei@...vell.com>,
        Romain Perier <romain.perier@...e-electrons.com>,
        Yehuda Yitschak <yehuday@...vell.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Hanna Hawa <hannah@...vell.com>,
        Kostya Porotchkin <kostap@...vell.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[...]

>>>>>> +
>>>>>> +       /*
>>>>>> +        * 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@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Kind regards
Uffe

Powered by blists - more mailing lists