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 <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> >> Reviewed-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> >> Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> > > 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 <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..c2370493fbe8 >> --- /dev/null >> +++ b/drivers/mmc/host/sdhci-xenon.h >> @@ -0,0 +1,134 @@ >> +/* >> + * 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_ >> + >> +#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 >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html