Message-ID: <07e71ce4-a78e-750c-6325-0a88891519d5@marvell.com>
Date:   Fri, 25 Nov 2016 16:45:47 +0800
From:   Ziji Hu <huziji@...vell.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>,
        Adrian Hunter <adrian.hunter@...el.com>
CC:     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/24 23:00, Ziji Hu wrote:
> Hi Ulf,
> 
> On 2016/11/24 21:34, Ulf Hansson wrote:
<snip>
>>>>> +
>>>>> +       /*
>>>>> +        * 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.

    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
> 

Powered by blists - more mailing lists