Message-ID: <07d4a4e2-1fbe-6495-12be-d3977db13120@marvell.com>
Date:   Fri, 25 Nov 2016 21:43:10 +0800
From:   Ziji Hu <huziji@...vell.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
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

Hi Ulf,

On 2016/11/25 21:06, Ulf Hansson wrote:
> [...]
> 
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * 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
> 
   Great!
   I will try this mmc-card sub-node.

   Thank you very much.

Best regards,
Hu Ziji

>>
>>     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