On 04/01/17 10:51, Ziji Hu wrote:
> Hi Adrian,
>
> On 2017/1/4 15:26, Adrian Hunter wrote:
>> On 13/12/16 19:48, Gregory CLEMENT wrote:
>>> From: Hu Ziji <huziji@xxxxxxxxxxx>
>>>
>>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>>> Add Xenon specific intialization process.
>>> Add Xenon specific mmc_host_ops APIs.
>>> Add Xenon specific register definitions.
>>>
>>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>>
>>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>>> Version 3.01 and is designed according to the guidelines provided
>>> in the SD Host Controller Standard Specification Version 3.00.
>>>
>>> Signed-off-by: Hu Ziji <huziji@xxxxxxxxxxx>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
>
> <snip>
>
>>> +static void xenon_sdhc_tuning_setup(struct sdhci_host *host)
>>> +{
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> + u32 reg;
>>> +
>>> + /* Disable the Re-Tuning Request functionality */
>>> + reg = sdhci_readl(host, SDHCI_SLOT_RETUNING_REQ_CTRL);
>>> + reg &= ~SDHCI_RETUNING_COMPATIBLE;
>>> + sdhci_writel(host, reg, SDHCI_SLOT_RETUNING_REQ_CTRL);
>>> +
>>> + /* Disable the Re-tuning Event Signal Enable */
>>> + reg = sdhci_readl(host, SDHCI_SIGNAL_ENABLE);
>>> + reg &= ~SDHCI_INT_RETUNE;
>>> + sdhci_writel(host, reg, SDHCI_SIGNAL_ENABLE);
>>> +
>>> + /* Force to use Tuning Mode 1 */
>>> + host->tuning_mode = SDHCI_TUNING_MODE_1;
>>> + /* Set re-tuning period */
>>> + host->tuning_count = 1 << (priv->tuning_count - 1);
>>
>> host->tuning_mode and host->tuning_count get overwritten in
>> sdhci_setup_host() called by sdhci_add_host()
>>
>
> You are correct.
> I will move it after sdhci_add_host().
>
>>> +}
>>> +
> <snip>
>
>>> +/*
>>> + * Xenon Specific Operations in mmc_host_ops
>>> + */
>>> +static void xenon_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> + unsigned long flags;
>>> + u32 reg;
>>> +
>>> + /*
>>> + * HS400/HS200/eMMC HS doesn't have Preset Value register.
>>> + * However, sdhci_set_ios will read HS400/HS200 Preset register.
>>> + * Disable Preset Value register for HS400/HS200.
>>> + * eMMC HS with preset_enabled set will trigger a bug in
>>> + * get_preset_value().
>>> + */
>>> + spin_lock_irqsave(&host->lock, flags);
>>> + if ((ios->timing == MMC_TIMING_MMC_HS400) ||
>>> + (ios->timing == MMC_TIMING_MMC_HS200) ||
>>> + (ios->timing == MMC_TIMING_MMC_HS)) {
>>> + host->preset_enabled = false;
>>> + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
>>> +
>>> + reg = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> + reg &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
>>> + sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>> + } else {
>>> + host->quirks2 &= ~SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
>>> + }
>>> + spin_unlock_irqrestore(&host->lock, flags);
>>
>> At some point we will have to get rid of SDHCI_QUIRK2_PRESET_VALUE_BROKEN
>> and add a callback instead.
>>
>
> Thanks for the information.
> I would like to keep this workaround here, before the detailed patch is brought up.
> Is it OK to you?
Sure
>
>>> +
> <snip>
>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + /*
>>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>>> + * disabled. However, sdhci_set_clock will also disable the Internal
>>> + * clock in mmc_set_signal_voltage().
>>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>>> + * Thus here manually enable internal clock.
>>> + *
>>> + * After switch completes, it is unnecessary to disable internal clock,
>>> + * since keeping internal clock active obeys SD spec.
>>> + */
>>> + enable_xenon_internal_clk(host);
>>
>> We could try the attached patch.
>>
>
> I test your patch. It can work on my platforms.
> Thanks a lot.
>
> May I keep this workaround now?
> I would like to remove this workaround after your attached patch is applied.
Ok
>
>>> +
> <snip>
>>> +static int sdhci_xenon_remove(struct platform_device *pdev)
>>> +{
>>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xFFFFFFFF);
>>
>> This 'dead' check was originally for PCI I think. Unless you know it makes
>> sense for your device, I would leave it out. i.e. just do
>> sdhci_remove_host(host, 0);
>>
>
> Got it. I will remove it.
>
>>> +
>>> + xenon_sdhc_remove(host);
>>> +
>>> + sdhci_remove_host(host, dead);
>>> +
>>> + clk_disable_unprepare(pltfm_host->clk);
>>> +
>>> + sdhci_pltfm_free(pdev);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id sdhci_xenon_dt_ids[] = {
>>> + { .compatible = "marvell,armada-7000-sdhci",},
>>> + { .compatible = "marvell,armada-3700-sdhci",},
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
>>> +
>>> +static struct platform_driver sdhci_xenon_driver = {
>>> + .driver = {
>>> + .name = "xenon-sdhci",
>>> + .of_match_table = sdhci_xenon_dt_ids,
>>> + .pm = &sdhci_pltfm_pmops,
>>> + },
>>> + .probe = sdhci_xenon_probe,
>>> + .remove = sdhci_xenon_remove,
>>> +};
>>> +
>>> +module_platform_driver(sdhci_xenon_driver);
>>> +
>>> +MODULE_DESCRIPTION("SDHCI platform driver for Marvell Xenon SDHC");
>>> +MODULE_AUTHOR("Hu Ziji <huziji@xxxxxxxxxxx>");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
>>> new file mode 100644
>>> index 000000000000..d50cd663a265
>>> --- /dev/null
>>> +++ b/drivers/mmc/host/sdhci-xenon.h
>>> @@ -0,0 +1,70 @@
>>> +/*
>>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>>> + *
>>> + * Author: Hu Ziji <huziji@xxxxxxxxxxx>
>>> + * Date: 2016-8-24
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation version 2.
>>> + */
>>> +#ifndef SDHCI_XENON_H_
>>> +#define SDHCI_XENON_H_
>>> +
>>> +
>>
>> Double blank line
>>
> Will remove it.
>
>>> +/* Register Offset of Xenon SDHC self-defined register */
>>> +#define SDHCI_SYS_CFG_INFO 0x0104
>>
>> A lot of these defines look like they could be just in sdhci-xenon.c or
>> sdhci-xenon-phy.c. It is also a little odd that they are prefixed by
>> "SDHCI" because they are not standard. "XENON" would be better.
>>
>
> Some registers are accessed by bother sdhci-xenon.c and sdhci-xenon-phy.c.
> As a result, I list all the registers here in sdhci-xenon.h, for convenience.
>
> Previously, Ulf asked me to prefix them all with "SDHCI".
> I would like to know which prefix sounds more reasonable, "XENON_" or "SDHCI_XENON_"?
I would use "XENON_"
>
>>> +#define SDHCI_SLOT_TYPE_SDIO_SHIFT 24
>>> +#define SDHCI_NR_SUPPORTED_SLOT_MASK 0x7
>>> +
>>> +#define SDHCI_SYS_OP_CTRL 0x0108
>>> +#define SDHCI_AUTO_CLKGATE_DISABLE_MASK BIT(20)
>>> +#define SDHCI_SDCLK_IDLEOFF_ENABLE_SHIFT 8
>>> +#define SDHCI_SLOT_ENABLE_SHIFT 0
>>> +
>>> +#define SDHCI_SYS_EXT_OP_CTRL 0x010C
>>> +
>>> +#define SDHCI_SLOT_EMMC_CTRL 0x0130
>>> +#define SDHCI_EMMC_VCCQ_MASK 0x3
>>> +#define SDHCI_EMMC_VCCQ_1_8V 0x1
>>> +#define SDHCI_EMMC_VCCQ_3_3V 0x3
>>> +
>>> +#define SDHCI_SLOT_RETUNING_REQ_CTRL 0x0144
>>> +/* retuning compatible */
>>> +#define SDHCI_RETUNING_COMPATIBLE 0x1
>>> +
>>> +/* Tuning Parameter */
>>> +#define SDHCI_TMR_RETUN_NO_PRESENT 0xF
>>> +#define SDHCI_DEF_TUNING_COUNT 0x9
>>> +
>>> +#define SDHCI_DEFAULT_SDCLK_FREQ (400000)
>>
>> Unnecessary ()
>>
> Will fix it.
>
> Thanks a lot for the review.
>
> Best regards,
> Hu Ziji
>
>>> +
>>> +/* Xenon specific Mode Select value */
>>> +#define SDHCI_XENON_CTRL_HS200 0x5
>>> +#define SDHCI_XENON_CTRL_HS400 0x6
>>> +
>>> +/* Indicate Card Type is not clear yet */
>>> +#define SDHCI_CARD_TYPE_UNKNOWN 0xF
>>> +
>>> +struct sdhci_xenon_priv {
>>> + unsigned char tuning_count;
>>> + /* idx of SDHC */
>>> + u8 sdhc_id;
>>> +
>>> + /*
>>> + * eMMC/SD/SDIO require different PHY settings or
>>> + * voltage control. It's necessary for Xenon driver to
>>> + * recognize card type during, or even before initialization.
>>> + * However, mmc_host->card is not available yet at that time.
>>> + * This field records the card type during init.
>>> + * For eMMC, it is updated in dt parse. For SD/SDIO, it is
>>> + * updated in xenon_init_card().
>>> + *
>>> + * It is only valid during initialization after it is updated.
>>> + * Do not access this variable in normal transfers after
>>> + * initialization completes.
>>> + */
>>> + unsigned int init_card_type;
>>> +};
>>> +
>>> +#endif
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
