Re: [EXT] Re: [PATCH] mmc: sdhci-xenon: add gpio hard reset support

[Zhoujie Wu <zjwu@marvell.com>]

Hi Shawn,
Really appreciate your feedback.

On 08/14/2017 07:11 PM, Shawn Lin wrote:
> External Email
>
> ----------------------------------------------------------------------
> Hi
>
> On 2017/8/15 6:19, Zhoujie Wu wrote:
>> On some platforms, like armada3700,  SD card need to
>> do hard reset by gpio toggling to make it work properly
>> after warm reset the board.
>
> I don't get this that SD card need to do hard reset...
>
> I assume what you talk about is either for eMMC or a power-cycle
> for SD card.

The subject of the patch confused you. What I want is a power-cycle for 
the SD card. The gpio is  used to enable/disable the vdd power supply 
for sd card.
Actually on a3700, when warm reset the board, their is no power-cycle 
for SD card, which will lead sd card can't response correct ocr and 
never set S18A unless a power-cycle.
This is the purpose I submit this patch.

>
>> Add gpio hard reset feature for this purpose.
>>
>> Signed-off-by: Zhoujie Wu <zjwu@marvell.com>
>> ---
>>   drivers/mmc/host/sdhci-xenon.c | 49 
>> +++++++++++++++++++++++++++++++++++++++---
>>   drivers/mmc/host/sdhci-xenon.h |  4 ++++
>>   2 files changed, 50 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-xenon.c 
>> b/drivers/mmc/host/sdhci-xenon.c
>> index edd4d915..54a7057 100644
>> --- a/drivers/mmc/host/sdhci-xenon.c
>> +++ b/drivers/mmc/host/sdhci-xenon.c
>> @@ -18,6 +18,8 @@
>>   #include <linux/ktime.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +
>>     #include "sdhci-pltfm.h"
>>   #include "sdhci-xenon.h"
>> @@ -210,12 +212,25 @@ static void xenon_set_uhs_signaling(struct 
>> sdhci_host *host,
>>       sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>>   }
>>   +static void xenon_hw_reset(struct sdhci_host *host)
>> +{
>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +    struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +    if (priv->reset_gpio) {
>> +        gpiod_set_value_cansleep(priv->reset_gpio, 0);
>> +        msleep(30);
>> +        gpiod_set_value_cansleep(priv->reset_gpio, 1);
>> +    }
>> +}
>> +
>
> If that is not a functional IO wired out from your controller,
> and I would expect you $subject patch and could introduce pwrseq for you
> DT instead of adding these....
>
> Does that work for you?

Thanks for your suggestion about pwrseq, today I tried both 
pwrseq_simple and pwrseq_emmc, both of them have some issues on our 
platform.
1) pwrseq_simple has power_off callback, which will call 
pwrseq_power_off function to clear the gpio and cut the power for sd 
module. In this case, our card detection can't work. I believe the cd 
feature reply on this voltage as well since if I comment out the 
power_off function, the card detection can work without issue.
2) pwrseq_emmc, it uses gpio_set_value to reset the card, but our 
platform is using a expander gpio chip based on i2c bus, so I saw 
warning in gpio driver since the chip can sleep.
I don't know why pwrseq_emmc can't use gpio_set_value_cansleep. Even 
change the api to cansleep can solve the warning, but this pwrseq is 
designed for emmc card, it looks like not a good idea to use it for SD card.

I can use 1) but in xenon driver directly change pwr_off callback to 
null to solve issue. Or do you think any other better solution?

>
>
>>   static const struct sdhci_ops sdhci_xenon_ops = {
>>       .set_clock        = sdhci_set_clock,
>>       .set_bus_width        = sdhci_set_bus_width,
>>       .reset            = xenon_reset,
>>       .set_uhs_signaling    = xenon_set_uhs_signaling,
>>       .get_max_clock        = sdhci_pltfm_clk_get_max_clock,
>> +    .hw_reset        = xenon_hw_reset,
>>   };
>>     static const struct sdhci_pltfm_data sdhci_xenon_pdata = {
>> @@ -373,10 +388,15 @@ static int xenon_probe_dt(struct 
>> platform_device *pdev)
>>       struct device_node *np = pdev->dev.of_node;
>>       struct sdhci_host *host = platform_get_drvdata(pdev);
>>       struct mmc_host *mmc = host->mmc;
>> +    struct device *dev = mmc_dev(mmc);
>>       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>       struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>       u32 sdhc_id, nr_sdhc;
>>       u32 tuning_count;
>> +    int reset_gpio, ret;
>> +    enum of_gpio_flags flags;
>> +    char *reset_name;
>> +    unsigned long gpio_flags;
>>         /* Disable HS200 on Armada AP806 */
>>       if (of_device_is_compatible(np, "marvell,armada-ap806-sdhci"))
>> @@ -387,7 +407,7 @@ static int xenon_probe_dt(struct platform_device 
>> *pdev)
>>           nr_sdhc = sdhci_readl(host, XENON_SYS_CFG_INFO);
>>           nr_sdhc &= XENON_NR_SUPPORTED_SLOT_MASK;
>>           if (unlikely(sdhc_id > nr_sdhc)) {
>> -            dev_err(mmc_dev(mmc), "SDHC Index %d exceeds Number of 
>> SDHCs %d\n",
>> +            dev_err(dev, "SDHC Index %d exceeds Number of SDHCs %d\n",
>>                   sdhc_id, nr_sdhc);
>>               return -EINVAL;
>>           }
>> @@ -398,14 +418,37 @@ static int xenon_probe_dt(struct 
>> platform_device *pdev)
>>       if (!of_property_read_u32(np, "marvell,xenon-tun-count",
>>                     &tuning_count)) {
>>           if (unlikely(tuning_count >= XENON_TMR_RETUN_NO_PRESENT)) {
>> -            dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set 
>> default value %d\n",
>> +            dev_err(dev, "Wrong Re-tuning Count. Set default value 
>> %d\n",
>>                   XENON_DEF_TUNING_COUNT);
>>               tuning_count = XENON_DEF_TUNING_COUNT;
>>           }
>>       }
>>       priv->tuning_count = tuning_count;
>>   -    return xenon_phy_parse_dt(np, host);
>> +    ret = xenon_phy_parse_dt(np, host);
>> +
>> +    reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
>> +    if (gpio_is_valid(reset_gpio)) {
>> +        if (flags & OF_GPIO_ACTIVE_LOW)
>> +            gpio_flags = GPIOF_ACTIVE_LOW | GPIOF_OUT_INIT_HIGH;
>> +        else
>> +            gpio_flags = GPIOF_OUT_INIT_LOW;
>> +
>> +        reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
>> +                        dev_name(dev));
>> +        if (!reset_name)
>> +            return -ENOMEM;
>> +        ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
>> +                        reset_name);
>> +        if (ret) {
>> +            dev_err(dev, "Failed to request reset gpio\n");
>> +            return ret;
>> +        }
>> +
>> +        priv->reset_gpio = gpio_to_desc(reset_gpio);
>> +    }
>> +
>> +    return ret;
>>   }
>>     static int xenon_sdhc_prepare(struct sdhci_host *host)
>> diff --git a/drivers/mmc/host/sdhci-xenon.h 
>> b/drivers/mmc/host/sdhci-xenon.h
>> index 73debb4..cffb0fe 100644
>> --- a/drivers/mmc/host/sdhci-xenon.h
>> +++ b/drivers/mmc/host/sdhci-xenon.h
>> @@ -90,6 +90,10 @@ struct xenon_priv {
>>        */
>>       void        *phy_params;
>>       struct xenon_emmc_phy_regs *emmc_phy_regs;
>> +    /*
>> +     * gpio pin for hw reset
>> +     */
>> +    struct gpio_desc *reset_gpio;
>>   };
>>     int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios);
>>
>