Hi Adrian,
Thank you very much for your review.
I will firstly fix the typo.
On 2016/10/11 20:37, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> 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 <huz...@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>
> I looked at a couple of things but you need to sort out the issues with
> card_candidate before going further.
>
Understood.
I will improve the card_candidate. Please help check the details in
below.
>> ---
<snip>
>> +
>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> + unsigned char voltage = ios->signal_voltage;
>> +
>> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>> + (voltage == MMC_SIGNAL_VOLTAGE_180))
>> + return __emmc_signal_voltage_switch(mmc, voltage);
>> +
>> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>> + voltage);
>> + return -EINVAL;
>> +}
>> +
>> +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);
>> +
>> + if (priv->card_candidate) {
>
> mmc_power_up() calls __mmc_set_signal_voltage() calls
> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
> invalid reference to an old card.
>
> So that's not going to work if the card changes - not only for removable
> cards but even for eMMC if init fails and retries.
>
As you point out, this piece of code have defects, even though it
actually works on Marvell multiple platforms, unless eMMC card is removable.
I can add a property to explicitly indicate eMMC type in DTS.
Then card_candidate access can be removed here.
Does it sounds more reasonable to you?
>> + if (mmc_card_mmc(priv->card_candidate))
>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>
> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>
I can add an eMMC type property in DTS, to remove the card_candidate
access here.
>> + }
>> +
>> + return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
>> + * After determining which slot is used for SDIO,
>> + * some additional task is required.
>> + */
>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> + u32 reg;
>> + u8 slot_idx;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + /* Link the card for delay adjustment */
>> + priv->card_candidate = card;
>
> You really need a better way to get the card. I suggest you take up the
> issue with Ulf. One possibility is to have mmc core set host->card = card
> much earlier.
>
Could you please tell me if any issue related to card_candidate still
exists, after the card_candidate is removed from
xenon_start_signal_voltage_switch() in above?
It seems that when init_card is called, the structure card has already
been updated and stable in MMC/SD/SDIO initialization sequence.
May I keep it here?
>> + /* Set tuning functionality of this slot */
>> + xenon_slot_tuning_setup(host);
>> +
>> + slot_idx = priv->slot_idx;
>> + if (!mmc_card_sdio(card)) {
>> + /* Re-enable the Auto-CMD12 cap flag. */
>> + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>> + host->flags |= SDHCI_AUTO_CMD12;
>> +
>> + /* Clear SDIO Card Inserted indication */
>> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + reg &= ~(1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>> +
>> + if (mmc_card_mmc(card)) {
>> + mmc->caps |= MMC_CAP_NONREMOVABLE;
>> + if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V))
>> + mmc->caps |= MMC_CAP_1_8V_DDR;
>> + /*
>> + * Force to clear BUS_TEST to
>> + * skip bus_test_pre and bus_test_post
>> + */
>> + mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>> + mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>> + MMC_CAP2_PACKED_CMD;
>> + if (mmc->caps & MMC_CAP_8_BIT_DATA)
>> + mmc->caps2 |= MMC_CAP2_HS400_1_8V;
>> + }
>> + } else {
>> + /*
>> + * Delete the Auto-CMD12 cap flag.
>> + * Otherwise, when sending multi-block CMD53,
>> + * Driver will set Transfer Mode Register to enable Auto CMD12.
>> + * However, SDIO device cannot recognize CMD12.
>> + * Thus SDHC will time-out for waiting for CMD12 response.
>> + */
>> + host->quirks &= ~SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>> + host->flags &= ~SDHCI_AUTO_CMD12;
>
> sdhci_set_transfer_mode() won't enable auto-CMD12 for CMD53 anyway, so is
> this needed?
>
In Xenon driver, Auto-CMD12 flag is set to enable full support to
Auto-CMD feature, both Auto-CMD12 and Auto-CMD23.
As a result, when Xenon SDHC slot can both support SD and SDIO,
Auto-CMD12 is disabled when SDIO card is inserted, and renabled when SD is
inserted.
I recheck the sdhci code to set Auto-CMD bit in Transfer Mode register,
in sdhci_set_transfer_mode():
if (mmc_op_multi(cmd->opcode) || data->blocks > 1)
As you can see, as long as it is CMD18/CMD25 OR there are multiple data
blocks, Auto-CMD field will be set.
CMD53 doesn't have CMD23. Thus Auto-CMD12 is selected since Auto-CMD12
flag is set.
Thus I have to clear Auto-CMD12 to avoid issuing Auto-CMD12 in SDIO
transfer.
I just meet a similar issue in RPMB.
When Auto-CMD12 flag is set, eMMC RPMB access will trigger Auto-CMD12,
since CMD25 is in use.
It will cause RPMB access failed.
One possible solution is to drop Auto-CMD12 support and use Auto-CMD23
only, in Xenon driver.
May I know you opinion, please?
>> +
>> + /*
>> + * Set SDIO Card Inserted indication
>> + * to inform that the current slot is for SDIO
>> + */
>> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + reg |= (1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>> + }
>> +}
>> +
>> +static int xenon_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> +
>> + if (host->timing == MMC_TIMING_UHS_DDR50)
>> + return 0;
>> +
>> + return sdhci_execute_tuning(mmc, opcode);
>> +}
>> +
>> +static void xenon_replace_mmc_host_ops(struct sdhci_host *host)
>> +{
>> + host->mmc_host_ops.set_ios = xenon_set_ios;
>> + host->mmc_host_ops.start_signal_voltage_switch =
>> + xenon_start_signal_voltage_switch;
>> + host->mmc_host_ops.init_card = xenon_init_card;
>> + host->mmc_host_ops.execute_tuning = xenon_execute_tuning;
>> +}
>> +
>> +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 sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + int err;
>> + u32 slot_idx, nr_slot;
>> + u32 tuning_count;
>> + u32 reg;
>> +
>> + /* Standard MMC property */
>> + err = mmc_of_parse(mmc);
>> + if (err)
>> + return err;
>> +
>> + /* Standard SDHCI property */
>> + sdhci_get_of_property(pdev);
>> +
>> + /*
>> + * Xenon Specific property:
>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>> + * tuning-count: the interval between re-tuning
>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>> + */
>> + if (!of_property_read_u32(np, "xenon,slotno", &slot_idx)) {
>> + nr_slot = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + nr_slot &= NR_SUPPORTED_SLOT_MASK;
>> + if (unlikely(slot_idx > nr_slot)) {
>> + dev_err(mmc_dev(mmc), "Slot Index %d exceeds Number of
>> slots %d\n",
>> + slot_idx, nr_slot);
>> + return -EINVAL;
>> + }
>> + } else {
>> + priv->slot_idx = 0x0;
>> + }
>> +
>> + if (!of_property_read_u32(np, "xenon,tuning-count", &tuning_count)) {
>> + if (unlikely(tuning_count >= TMR_RETUN_NO_PRESENT)) {
>> + dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set
>> default value %d\n",
>> + DEF_TUNING_COUNT);
>> + tuning_count = DEF_TUNING_COUNT;
>> + }
>> + } else {
>> + priv->tuning_count = DEF_TUNING_COUNT;
>> + }
>> +
>> + if (of_property_read_bool(np, "xenon,mask-conflict-err")) {
>> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
>> + reg |= MASK_CMD_CONFLICT_ERROR;
>> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
>> + }
>> +
>> + return err;
>> +}
>> +
>> +static int xenon_slot_probe(struct sdhci_host *host)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + u8 slot_idx = priv->slot_idx;
>> +
>> + /* Enable slot */
>> + xenon_enable_slot(host, slot_idx);
>> +
>> + /* Enable ACG */
>> + xenon_set_acg(host, true);
>> +
>> + /* Enable Parallel Transfer Mode */
>> + xenon_enable_slot_parallel_tran(host, slot_idx);
>> +
>> + priv->timing = MMC_TIMING_FAKE;
>> + priv->clock = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static void xenon_slot_remove(struct sdhci_host *host)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + u8 slot_idx = priv->slot_idx;
>> +
>> + /* disable slot */
>> + xenon_disable_slot(host, slot_idx);
>> +}
>> +
>> +static int sdhci_xenon_probe(struct platform_device *pdev)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host;
>> + struct sdhci_host *host;
>> + struct clk *clk, *axi_clk;
>> + struct sdhci_xenon_priv *priv;
>> + int err;
>> +
>> + host = sdhci_pltfm_init(pdev, &sdhci_xenon_pdata,
>> + sizeof(struct sdhci_xenon_priv));
>> + if (IS_ERR(host))
>> + return PTR_ERR(host);
>> +
>> + pltfm_host = sdhci_priv(host);
>> + priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + xenon_set_acg(host, false);
>> +
>> + /*
>> + * Link Xenon specific mmc_host_ops function,
>> + * to replace standard ones in sdhci_ops.
>> + */
>> + xenon_replace_mmc_host_ops(host);
>> +
>> + clk = devm_clk_get(&pdev->dev, "core");
>> + if (IS_ERR(clk)) {
>> + dev_err(&pdev->dev, "Failed to setup input clk.\n");
>> + err = PTR_ERR(clk);
>> + goto free_pltfm;
>> + }
>> + clk_prepare_enable(clk);
>> + pltfm_host->clk = clk;
>> +
>> + /*
>> + * Some SOCs require additional clock to
>> + * manage AXI bus clock.
>> + * It is optional.
>> + */
>> + axi_clk = devm_clk_get(&pdev->dev, "axi");
>> + if (!IS_ERR(axi_clk)) {
>> + clk_prepare_enable(axi_clk);
>> + priv->axi_clk = axi_clk;
>> + }
>> +
>> + err = xenon_probe_dt(pdev);
>> + if (err)
>> + goto err_clk;
>> +
>> + err = xenon_slot_probe(host);
>> + if (err)
>> + goto err_clk;
>> +
>> + err = sdhci_add_host(host);
>> + if (err)
>> + goto remove_slot;
>> +
>> + return 0;
>> +
>> +remove_slot:
>> + xenon_slot_remove(host);
>> +err_clk:
>> + clk_disable_unprepare(pltfm_host->clk);
>> + if (!IS_ERR(axi_clk))
>> + clk_disable_unprepare(axi_clk);
>> +free_pltfm:
>> + sdhci_pltfm_free(pdev);
>> + return err;
>> +}
>> +
>> +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);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xFFFFFFFF);
>> +
>> + xenon_slot_remove(host);
>> +
>> + sdhci_remove_host(host, dead);
>> +
>> + clk_disable_unprepare(pltfm_host->clk);
>> + clk_disable_unprepare(priv->axi_clk);
>> +
>> + sdhci_pltfm_free(pdev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id sdhci_xenon_dt_ids[] = {
>> + { .compatible = "marvell,sdhci-xenon",},
>> + { .compatible = "marvell,armada-3700-sdhci",},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
>> +
>> +static struct platform_driver sdhci_xenon_driver = {
>> + .driver = {
>> + .name = "sdhci-xenon",
>> + .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 <huz...@marvell.com>");
>> +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..c2370493fbe8
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon.h
>> @@ -0,0 +1,134 @@
>> +/*
>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>> + *
>> + * Author: Hu Ziji <huz...@marvell.com>
>> + * 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_
>> +
>> +#include <linux/clk.h>
>> +#include <linux/mmc/card.h>
>> +#include <linux/of.h>
>> +#include "sdhci.h"
>> +
>> +/* Register Offset of SD Host Controller SOCP self-defined register */
>> +#define SDHC_SYS_CFG_INFO 0x0104
>> +#define SLOT_TYPE_SDIO_SHIFT 24
>> +#define SLOT_TYPE_EMMC_MASK 0xFF
>> +#define SLOT_TYPE_EMMC_SHIFT 16
>> +#define SLOT_TYPE_SD_SDIO_MMC_MASK 0xFF
>> +#define SLOT_TYPE_SD_SDIO_MMC_SHIFT 8
>> +#define NR_SUPPORTED_SLOT_MASK 0x7
>> +
>> +#define SDHC_SYS_OP_CTRL 0x0108
>> +#define AUTO_CLKGATE_DISABLE_MASK BIT(20)
>> +#define SDCLK_IDLEOFF_ENABLE_SHIFT 8
>> +#define SLOT_ENABLE_SHIFT 0
>> +
>> +#define SDHC_SYS_EXT_OP_CTRL 0x010C
>> +#define MASK_CMD_CONFLICT_ERROR BIT(8)
>> +
>> +#define SDHC_SLOT_OP_STATUS_CTRL 0x0128
>> +#define DELAY_90_DEGREE_MASK_EMMC5 BIT(7)
>> +#define DELAY_90_DEGREE_SHIFT_EMMC5 7
>> +#define EMMC_5_0_PHY_FIXED_DELAY_MASK 0x7F
>> +#define EMMC_PHY_FIXED_DELAY_MASK 0xFF
>> +#define EMMC_PHY_FIXED_DELAY_WINDOW_MIN
>> (EMMC_PHY_FIXED_DELAY_MASK >> 3)
>> +#define SDH_PHY_FIXED_DELAY_MASK 0x1FF
>> +#define SDH_PHY_FIXED_DELAY_WINDOW_MIN
>> (SDH_PHY_FIXED_DELAY_MASK >> 4)
>> +
>> +#define TUN_CONSECUTIVE_TIMES_SHIFT 16
>> +#define TUN_CONSECUTIVE_TIMES_MASK 0x7
>> +#define TUN_CONSECUTIVE_TIMES 0x4
>> +#define TUNING_STEP_SHIFT 12
>> +#define TUNING_STEP_MASK 0xF
>> +#define TUNING_STEP_DIVIDER BIT(6)
>> +
>> +#define FORCE_SEL_INVERSE_CLK_SHIFT 11
>> +
>> +#define SDHC_SLOT_EMMC_CTRL 0x0130
>> +#define ENABLE_DATA_STROBE BIT(24)
>> +#define SET_EMMC_RSTN BIT(16)
>> +#define DISABLE_RD_DATA_CRC BIT(14)
>> +#define DISABLE_CRC_STAT_TOKEN BIT(13)
>> +#define EMMC_VCCQ_MASK 0x3
>> +#define EMMC_VCCQ_1_8V 0x1
>> +#define EMMC_VCCQ_3_3V 0x3
>> +
>> +#define SDHC_SLOT_RETUNING_REQ_CTRL 0x0144
>> +/* retuning compatible */
>> +#define RETUNING_COMPATIBLE 0x1
>> +
>> +#define SDHC_SLOT_EXT_PRESENT_STATE 0x014C
>> +#define LOCK_STATE 0x1
>> +
>> +#define SDHC_SLOT_DLL_CUR_DLY_VAL 0x0150
>> +
>> +/* Tuning Parameter */
>> +#define TMR_RETUN_NO_PRESENT 0xF
>> +#define DEF_TUNING_COUNT 0x9
>> +
>> +#define MMC_TIMING_FAKE 0xFF
>> +
>> +#define DEFAULT_SDCLK_FREQ (400000)
>> +
>> +/* Xenon specific Mode Select value */
>> +#define XENON_SDHCI_CTRL_HS200 0x5
>> +#define XENON_SDHCI_CTRL_HS400 0x6
>> +
>> +struct sdhci_xenon_priv {
>> + /*
>> + * The bus_width, timing, and clock fields in below
>> + * record the current setting of Xenon SDHC.
>> + * Driver will call a Sampling Fixed Delay Adjustment
>> + * if any setting is changed.
>> + */
>> + unsigned char bus_width;
>> + unsigned char timing;
>> + unsigned char tuning_count;
>> + unsigned int clock;
>> + struct clk *axi_clk;
>> +
>> + /* Slot idx */
>> + u8 slot_idx;
>> +
>> + /*
>> + * When initializing card, Xenon has to determine card type and
>> + * adjust Sampling Fixed delay.
>> + * However, at that time, card structure is not linked to mmc_host.
>> + * Thus a card pointer is added here to provide
>> + * the delay adjustment function with the card structure
>> + * of the card during initialization
>> + */
>> + struct mmc_card *card_candidate;
>> +};
>> +
>> +static inline int enable_xenon_internal_clk(struct sdhci_host *host)
>> +{
>> + u32 reg;
>> + u8 timeout;
>> +
>> + reg = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
>> + reg |= SDHCI_CLOCK_INT_EN;
>> + sdhci_writel(host, reg, SDHCI_CLOCK_CONTROL);
>> + /* Wait max 20 ms */
>> + timeout = 20;
>> + while (!((reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>> + & SDHCI_CLOCK_INT_STABLE)) {
>> + if (timeout == 0) {
>> + pr_err("%s: Internal clock never stabilised.\n",
>> + mmc_hostname(host->mmc));
>> + return -ETIMEDOUT;
>> + }
>> + timeout--;
>> + mdelay(1);
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>>
>
