[U-Boot] [PATCH 3/9 v2] mmc: Add Marvell Xenon SDHCI controller driver
Stefan Roese
sr at denx.de
Mon Jan 23 09:39:42 CET 2017
Hi Jaehoon,
On 23.01.2017 04:52, Jaehoon Chung wrote:
> On 01/21/2017 06:20 PM, Stefan Roese wrote:
>> This driver implementes platform specific code for the Xenon SDHCI
>> controller which is integrated in the Marvell MVEBU Armada 37xx and
>> Armada 7k / 8K SoCs.
>>
>> History:
>> This driver is ported from the Marvell U-Boot version 2015.01 which is
>> written by Victor Gu <xigu at marvell.com> with minor changes ported from
>> the Linux driver which is written by Ziji Hu <huziji at marvell.com>.
>
> Why this is [PATCH 3/9]? Sorry. i didn't find the other patches.
> Could you share the information for other patches?
This is v2 of this patchset. Since only this patch 3/9 has some
changes in v2, I didn't send the complete patchset again. Here
the links to the other patches. You are cc'ed for patches 1...3
as they change code in the mmc directory:
http://patchwork.ozlabs.org/patch/716972/
http://patchwork.ozlabs.org/patch/716970/
http://patchwork.ozlabs.org/patch/716967/
http://patchwork.ozlabs.org/patch/716969/
http://patchwork.ozlabs.org/patch/716965/
http://patchwork.ozlabs.org/patch/716964/
http://patchwork.ozlabs.org/patch/716968/
http://patchwork.ozlabs.org/patch/716968/
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Jaehoon Chung <jh80.chung at samsung.com>
>> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
>> ---
>> v2:
>> - Renamed MMC_XENON_SDHCI to MMC_SDHCI_XENON as requested by Masahiro
>> for the new consistant MMC naming
>>
>> drivers/mmc/Kconfig | 11 +
>> drivers/mmc/Makefile | 1 +
>> drivers/mmc/xenon_sdhci.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 601 insertions(+)
>> create mode 100644 drivers/mmc/xenon_sdhci.c
>>
>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>> index 9ed8da39ef..147e52d332 100644
>> --- a/drivers/mmc/Kconfig
>> +++ b/drivers/mmc/Kconfig
>> @@ -287,6 +287,17 @@ config MMC_SDHCI_SPEAR
>>
>> If unsure, say N.
>>
>> +config MMC_SDHCI_XENON
>> + bool "SDHCI support for the Xenon SDHCI controller"
>> + depends on MMC_SDHCI && DM_MMC && OF_CONTROL
>> + help
>> + Support for Xenon SDHCI host controller on Marvell Armada 3700
>> + 7k/8k ARM SoCs platforms
>> +
>> + If you have a controller with this interface, say Y here.
>> +
>> + If unsure, say N.
>> +
>> config MMC_SDHCI_TEGRA
>> bool "SDHCI platform support for the Tegra SD/MMC Controller"
>> depends on TEGRA
>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>> index 4dca09c955..6af7f79ff8 100644
>> --- a/drivers/mmc/Makefile
>> +++ b/drivers/mmc/Makefile
>> @@ -68,6 +68,7 @@ obj-$(CONFIG_MMC_SDHCI_MV) += mv_sdhci.o
>> obj-$(CONFIG_MMC_SDHCI_S5P) += s5p_sdhci.o
>> obj-$(CONFIG_MMC_SDHCI_SPEAR) += spear_sdhci.o
>> obj-$(CONFIG_MMC_SDHCI_TEGRA) += tegra_mmc.o
>> +obj-$(CONFIG_MMC_SDHCI_XENON) += xenon_sdhci.o
>>
>> obj-$(CONFIG_MMC_SUNXI) += sunxi_mmc.o
>> obj-$(CONFIG_MMC_UNIPHIER) += uniphier-sd.o
>> diff --git a/drivers/mmc/xenon_sdhci.c b/drivers/mmc/xenon_sdhci.c
>> new file mode 100644
>> index 0000000000..f36b482288
>> --- /dev/null
>> +++ b/drivers/mmc/xenon_sdhci.c
>> @@ -0,0 +1,589 @@
>> +/*
>> + * Driver for Marvell SOC Platform Group Xenon SDHC as a platform device
>> + *
>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>
> 2016-2017?
Thats the copyright notice from the Marvell code. Marvell might change
this if appropriate, but for now I would like to keep it.
>> + *
>> + * Author: Victor Gu <xigu at marvell.com>
>> + * Date: 2016-8-24
>> + *
>> + * Included parts of the Linux driver version which was written by:
>> + * Hu Ziji <huziji at marvell.com>
>> + *
>> + * Ported to from Marvell 2015.01 to mainline U-Boot 2017.01:
>> + * Stefan Roese <sr at denx.de>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <fdtdec.h>
>> +#include <libfdt.h>
>> +#include <malloc.h>
>> +#include <sdhci.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +/* 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
>
> Does it need to shift with 0?
Its not needed of course, but it makes to code more consistent
for my taste. And please note that the Linux driver also uses
these shifts, so we are more in-line with the Linux driver by
using it this way.
>> +
>> +#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)
>
> It's strange...0xFF >> 3? is it 0x1F?
Not used in this driver currently. I'll remove it in v3.
>> +#define SDH_PHY_FIXED_DELAY_MASK 0x1FF
>> +#define SDH_PHY_FIXED_DELAY_WINDOW_MIN (SDH_PHY_FIXED_DELAY_MASK >> 4)
>
> Ditto. why shift the 0x1FF?
Removed in v3.
>> +
>> +#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_FIFO_CTRL 0x012c
>> +
>> +#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 XENON_MAX_TUN_COUNT 0xb
>> +#define DEF_TUNING_COUNT 0x9
>> +
>> +#define MMC_TIMING_FAKE 0xFF
>
> What is fake?
Not used, to removed in v3.
>> +
>> +#define DEFAULT_SDCLK_FREQ 400000
>> +#define LOWEST_SDCLK_FREQ 100000
>
> Doesn't use these values.
Yes, thanks. Will remove.
>> +
>> +/* Xenon specific Mode Select value */
>> +#define XENON_SDHCI_CTRL_HS200 0x5
>> +#define XENON_SDHCI_CTRL_HS400 0x6
>> +
>> +#define EMMC_PHY_REG_BASE 0x170
>> +#define EMMC_PHY_TIMING_ADJUST EMMC_PHY_REG_BASE
>> +#define OUTPUT_QSN_PHASE_SELECT (1 << 17)
>> +#define SAMPL_INV_QSP_PHASE_SELECT (1 << 18)
>> +#define SAMPL_INV_QSP_PHASE_SELECT_SHIFT 18
>> +#define EMMC_PHY_SLOW_MODE (1 << 29)
>> +#define PHY_INITIALIZAION (1 << 31)
>
> You're using BIT() about some bit operation..but some values don't use it.
> Use the BIT() API about other bit operation.
Will do.
>> +#define WAIT_CYCLE_BEFORE_USING_MASK 0xf
>> +#define WAIT_CYCLE_BEFORE_USING_SHIFT 12
>> +#define FC_SYNC_EN_DURATION_MASK 0xf
>> +#define FC_SYNC_EN_DURATION_SHIFT 8
>> +#define FC_SYNC_RST_EN_DURATION_MASK 0xf
>> +#define FC_SYNC_RST_EN_DURATION_SHIFT 4
>> +#define FC_SYNC_RST_DURATION_MASK 0xf
>> +#define FC_SYNC_RST_DURATION_SHIFT 0
>> +
>> +#define EMMC_PHY_FUNC_CONTROL (EMMC_PHY_REG_BASE + 0x4)
>> +#define DQ_ASYNC_MODE (1 << 4)
>> +#define DQ_DDR_MODE_SHIFT 8
>> +#define DQ_DDR_MODE_MASK 0xff
>> +#define CMD_DDR_MODE (1 << 16)
>> +
>> +#define EMMC_PHY_PAD_CONTROL (EMMC_PHY_REG_BASE + 0x8)
>> +#define REC_EN_SHIFT 24
>> +#define REC_EN_MASK 0xf
>> +#define FC_DQ_RECEN (1 << 24)
>> +#define FC_CMD_RECEN (1 << 25)
>> +#define FC_QSP_RECEN (1 << 26)
>> +#define FC_QSN_RECEN (1 << 27)
>> +#define OEN_QSN (1 << 28)
>> +#define AUTO_RECEN_CTRL (1 << 30)
>
> Use BIT() API.
Yes.
>> +
>> +#define EMMC_PHY_PAD_CONTROL1 (EMMC_PHY_REG_BASE + 0xc)
>> +#define EMMC5_1_FC_QSP_PD BIT(9)
>> +#define EMMC5_1_FC_QSP_PU BIT(25)
>> +#define EMMC5_1_FC_CMD_PD BIT(8)
>> +#define EMMC5_1_FC_CMD_PU BIT(24)
>> +#define EMMC5_1_FC_DQ_PD 0xff
>> +#define EMMC5_1_FC_DQ_PU (0xff << 16)
>> +
>> +#define EMMC_PHY_PAD_CONTROL2 (EMMC_PHY_REG_BASE + 0x10)
>> +#define EMMC_PHY_DLL_CONTROL (EMMC_PHY_REG_BASE + 0x14)
>> +#define DLL_DELAY_TEST_LOWER_SHIFT 8
>> +#define DLL_DELAY_TEST_LOWER_MASK 0xff
>> +#define DLL_BYPASS_EN 0x1
>> +
>> +#define EMMC_LOGIC_TIMING_ADJUST (EMMC_PHY_REG_BASE + 0x18)
>> +#define EMMC_LOGIC_TIMING_ADJUST_LOW (EMMC_PHY_REG_BASE + 0x1c)
>> +
>> +/* Recommend by HW team */
>> +#define LOGIC_TIMING_VALUE 0x5a54
>
> also doesn't use this. why put this?
>
>> +
>> +#define SDHCI_RETUNE_EVT_INTSIG 0x00001000
>> +
>> +/* Hyperion only have one slot 0 */
>> +#define XENON_MMC_SLOT_ID_HYPERION 0
>> +
>> +#define MMC_TIMING_LEGACY 0
>> +#define MMC_TIMING_MMC_HS 1
>> +#define MMC_TIMING_SD_HS 2
>> +#define MMC_TIMING_UHS_SDR12 3
>> +#define MMC_TIMING_UHS_SDR25 4
>> +#define MMC_TIMING_UHS_SDR50 5
>> +#define MMC_TIMING_UHS_SDR104 6
>> +#define MMC_TIMING_UHS_DDR50 7
>> +#define MMC_TIMING_MMC_DDR52 8
>> +#define MMC_TIMING_MMC_HS200 9
>> +#define MMC_TIMING_MMC_HS400 10
>
> Why do you need to this? even HS200/400 don't support on u-boot.
Its taken from the Linux driver. As you might know, the SDHCI core
saves the current timing in this "ios->timing" variable. I was trying
to emulate this in the Xenon driver so long as the U-Boot
infrastructure does not implement it this way. I could remove the
currently unused timing defines for now but would prefer to keep
them.
>> +
>> +#define XENON_MMC_MAX_CLK 400000000
>> +
>> +enum soc_pad_ctrl_type {
>> + SOC_PAD_SD,
>> + SOC_PAD_FIXED_1_8V,
>> +};
>> +
>> +struct xenon_sdhci_plat {
>> + struct mmc_config cfg;
>> + struct mmc mmc;
>> +};
>> +
>> +struct xenon_sdhci_priv {
>> + struct sdhci_host host;
>> +
>> + u8 timing;
>> +
>> + unsigned int clock;
>> +
>> + void *pad_ctrl_reg;
>> + int pad_type;
>> +};
>> +
>> +static int xenon_mmc_phy_init(struct sdhci_host *host)
>> +{
>> + struct xenon_sdhci_priv *priv = host->mmc->priv;
>> + u32 clock = priv->clock;
>> + u32 wait;
>> + u32 time;
>> + u32 var;
>
> Make one line. u32 wait, time, var?
I personally prefer this coding style, one variable in one line. So
I would like to keep this.
>> +
>> + /* Enable QSP PHASE SELECT */
>> + var = sdhci_readl(host, EMMC_PHY_TIMING_ADJUST);
>> + var |= SAMPL_INV_QSP_PHASE_SELECT;
>> + if ((priv->timing == MMC_TIMING_UHS_SDR50) ||
>> + (priv->timing == MMC_TIMING_UHS_SDR25) ||
>> + (priv->timing == MMC_TIMING_UHS_SDR12) ||
>> + (priv->timing == MMC_TIMING_SD_HS) ||
>> + (priv->timing == MMC_TIMING_LEGACY))
>> + var |= EMMC_PHY_SLOW_MODE;
>> + sdhci_writel(host, var, EMMC_PHY_TIMING_ADJUST);
>> +
>> + /* Poll for host MMC PHY clock init to be stable */
>> + /* Wait up to 10ms */
>> + time = 100;
>> + while (time--) {
>> + var = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
>> + if (var & SDHCI_CLOCK_INT_STABLE)
>> + break;
>> +
>> + udelay(100);
>> + }
>> +
>> + if (time <= 0) {
>> + error("Failed to enable MMC internal clock in time\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + /* Init PHY */
>> + var = sdhci_readl(host, EMMC_PHY_TIMING_ADJUST);
>> + var |= PHY_INITIALIZAION;
>> + sdhci_writel(host, var, EMMC_PHY_TIMING_ADJUST);
>> +
>> + /* Add duration of FC_SYNC_RST */
>> + wait = (var >> FC_SYNC_RST_DURATION_SHIFT) & FC_SYNC_RST_DURATION_MASK;
>> + /* Add interval between FC_SYNC_EN and FC_SYNC_RST */
>> + wait += (var >> FC_SYNC_RST_EN_DURATION_SHIFT) &
>> + FC_SYNC_RST_EN_DURATION_MASK;
>> + /* Add duration of asserting FC_SYNC_EN */
>> + wait += (var >> FC_SYNC_EN_DURATION_SHIFT) & FC_SYNC_EN_DURATION_MASK;
>> + /* Add duration of waiting for PHY */
>> + wait += (var >> WAIT_CYCLE_BEFORE_USING_SHIFT) &
>> + WAIT_CYCLE_BEFORE_USING_MASK;
>> + /*
>> + * According to Moyang, 4 addtional bus clock and 4 AXI bus clock
>> + * are required
>> + */
>> + /* left shift 20 bits */
>> + wait += 8;
>> + wait <<= 20;
>> +
>> + if (clock == 0) {
>> + /* Use the possibly slowest bus frequency value */
>> + clock = 100000;
>> + }
>> +
>> + /* Get the wait time in unit of ms */
>> + wait = wait / clock;
>> + wait++;
>
> Where does "wait" variable use?
Good catch, thanks. Something that got mixed up while rebasing.
Will get fixed in v3.
>> +
>> + /* Poll for host eMMC PHY init to complete */
>> + /* Wait up to 10ms */
>> + time = 100;
>> + while (time--) {
>> + var = sdhci_readl(host, EMMC_PHY_TIMING_ADJUST);
>> + var &= PHY_INITIALIZAION;
>> + if (!var)
>> + break;
>> +
>> + /* wait for host eMMC PHY init to complete */
>> + udelay(100);
>> + }
>> +
>> + if (time <= 0) {
>> + error("Failed to init MMC PHY in time\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#define ARMADA_3700_SOC_PAD_1_8V 0x1
>> +#define ARMADA_3700_SOC_PAD_3_3V 0x0
>> +
>> +static void armada_3700_soc_pad_voltage_set(struct sdhci_host *host)
>> +{
>> + struct xenon_sdhci_priv *priv = host->mmc->priv;
>> +
>> + if (priv->pad_type == SOC_PAD_FIXED_1_8V)
>> + writel(ARMADA_3700_SOC_PAD_1_8V, priv->pad_ctrl_reg);
>> + else if (priv->pad_type == SOC_PAD_SD)
>> + writel(ARMADA_3700_SOC_PAD_3_3V, priv->pad_ctrl_reg);
>> +}
>> +
>> +static void xenon_mmc_phy_set(struct sdhci_host *host)
>> +{
>> + struct xenon_sdhci_priv *priv = host->mmc->priv;
>> + u32 var;
>> +
>> + /* Setup pad, set bit[30], bit[28] and bits[26:24] */
>> + var = sdhci_readl(host, EMMC_PHY_PAD_CONTROL);
>> + var |= AUTO_RECEN_CTRL | OEN_QSN | FC_QSP_RECEN |
>> + FC_CMD_RECEN | FC_DQ_RECEN;
>> + sdhci_writel(host, var, EMMC_PHY_PAD_CONTROL);
>> +
>> + /* Set CMD and DQ Pull Up */
>> + var = sdhci_readl(host, EMMC_PHY_PAD_CONTROL1);
>> + var |= (EMMC5_1_FC_CMD_PU | EMMC5_1_FC_DQ_PU);
>> + var &= ~(EMMC5_1_FC_CMD_PD | EMMC5_1_FC_DQ_PD);
>> + sdhci_writel(host, var, EMMC_PHY_PAD_CONTROL1);
>> +
>> + /*
>> + * If timing belongs to high speed, set bit[17] of
>> + * EMMC_PHY_TIMING_ADJUST register
>> + */
>> + if ((priv->timing == MMC_TIMING_MMC_HS400) ||
>> + (priv->timing == MMC_TIMING_MMC_HS200) ||
>> + (priv->timing == MMC_TIMING_UHS_SDR50) ||
>> + (priv->timing == MMC_TIMING_UHS_SDR104) ||
>> + (priv->timing == MMC_TIMING_UHS_DDR50) ||
>> + (priv->timing == MMC_TIMING_UHS_SDR25) ||
>> + (priv->timing == MMC_TIMING_MMC_DDR52)) {
>> + var = sdhci_readl(host, EMMC_PHY_TIMING_ADJUST);
>> + var |= OUTPUT_QSN_PHASE_SELECT;
>> + sdhci_writel(host, var, EMMC_PHY_TIMING_ADJUST);
>> + }
>> +
>> + /*
>> + * When setting EMMC_PHY_FUNC_CONTROL register,
>> + * SD clock should be disabled
>> + */
>> + var = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
>> + var &= ~SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, var, SDHCI_CLOCK_CONTROL);
>> +
>> + var = sdhci_readl(host, EMMC_PHY_FUNC_CONTROL);
>> + if (host->mmc->ddr_mode) {
>> + var |= (DQ_DDR_MODE_MASK << DQ_DDR_MODE_SHIFT) | CMD_DDR_MODE;
>> + } else {
>> + var &= ~((DQ_DDR_MODE_MASK << DQ_DDR_MODE_SHIFT) |
>> + CMD_DDR_MODE);
>> + }
>> + sdhci_writel(host, var, EMMC_PHY_FUNC_CONTROL);
>> +
>> + /* Enable bus clock */
>> + var = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
>> + var |= SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, var, SDHCI_CLOCK_CONTROL);
>> +
>> + xenon_mmc_phy_init(host);
>> +}
>> +
>> +/* Enable/Disable the Auto Clock Gating function of this slot */
>> +static void xenon_mmc_set_acg(struct sdhci_host *host, bool enable)
>> +{
>> + u32 var;
>> +
>> + var = sdhci_readl(host, SDHC_SYS_OP_CTRL);
>> + if (enable)
>> + var &= ~AUTO_CLKGATE_DISABLE_MASK;
>> + else
>> + var |= AUTO_CLKGATE_DISABLE_MASK;
>> +
>> + sdhci_writel(host, var, SDHC_SYS_OP_CTRL);
>> +}
>> +
>> +/* Enable specific slot */
>> +static void xenon_mmc_set_slot(struct sdhci_host *host, u8 slot, bool enable)
>> +{
>> + u32 var;
>> +
>> + var = sdhci_readl(host, SDHC_SYS_OP_CTRL);
>> + if (enable)
>> + var |= (0x1 << slot) << SLOT_ENABLE_SHIFT;
>> + else
>> + var &= ~((0x1 << slot) << SLOT_ENABLE_SHIFT);
>
> What is 0x1?
Its "(0x1 << slot)" masks the current slot. I can change this to
use a macro, like:
#define SLOT_MASK(slot) (1 << slot)
var |= SLOT_MASK(slot) << SLOT_ENABLE_SHIFT;
Would you prefer this?
>> + sdhci_writel(host, var, SDHC_SYS_OP_CTRL);
>> +}
>> +
>> +/* Enable Parallel Transfer Mode */
>> +static void xenon_mmc_set_parallel_tran(struct sdhci_host *host, u8 slot,
>> + bool enable)
>> +{
>> + u32 var;
>> +
>> + var = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
>> + if (enable)
>> + var |= (0x1 << slot);
>> + else
>> + var &= ~(0x1 << slot);
>
> Ditto.
>
>> + sdhci_writel(host, var, SDHC_SYS_EXT_OP_CTRL);
>> +}
>> +
>> +static void xenon_mmc_set_tuning(struct sdhci_host *host, u8 slot, bool enable)
>> +{
>> + u32 var;
>> +
>> + /* Set the Re-Tuning Request functionality */
>> + var = sdhci_readl(host, SDHC_SLOT_RETUNING_REQ_CTRL);
>> + if (enable)
>> + var |= RETUNING_COMPATIBLE;
>> + else
>> + var &= ~RETUNING_COMPATIBLE;
>> + sdhci_writel(host, var, SDHC_SLOT_RETUNING_REQ_CTRL);
>> +
>> + /* Set the Re-tuning Event Signal Enable */
>> + var = sdhci_readl(host, SDHCI_SIGNAL_ENABLE);
>> + if (enable)
>> + var |= SDHCI_RETUNE_EVT_INTSIG;
>> + else
>> + var &= ~SDHCI_RETUNE_EVT_INTSIG;
>> + sdhci_writel(host, var, SDHCI_SIGNAL_ENABLE);
>> +}
>> +
>> +/* Mask command conflict error */
>> +static void xenon_mask_cmd_conflict_err(struct sdhci_host *host)
>> +{
>> + u32 reg;
>> +
>> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
>> + reg |= MASK_CMD_CONFLICT_ERROR;
>> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
>> +}
>> +
>> +/* Platform specific function for post set_ios configuration */
>> +static void xenon_sdhci_set_ios_post(struct sdhci_host *host)
>> +{
>> + struct xenon_sdhci_priv *priv = host->mmc->priv;
>> + uint speed = host->mmc->tran_speed;
>> + int pwr_18v = 0;
>> +
>> + if ((sdhci_readb(host, SDHCI_POWER_CONTROL) & ~SDHCI_POWER_ON) ==
>> + SDHCI_POWER_180)
>> + pwr_18v = 1;
>> +
>> + /* Set timing variable according to the configured speed */
>> + if (IS_SD(host->mmc)) {
>> + /* SD/SDIO */
>> + if (pwr_18v) {
>> + if (host->mmc->ddr_mode)
>> + priv->timing = MMC_TIMING_UHS_DDR50;
>> + else if (speed <= 25000000)
>> + priv->timing = MMC_TIMING_UHS_SDR25;
>> + else
>> + priv->timing = MMC_TIMING_UHS_SDR50;
>> + } else {
>> + if (speed <= 25000000)
>> + priv->timing = MMC_TIMING_LEGACY;
>> + else
>> + priv->timing = MMC_TIMING_SD_HS;
>> + }
>> + } else {
>> + /* eMMC */
>> + if (host->mmc->ddr_mode)
>> + priv->timing = MMC_TIMING_MMC_DDR52;
>> + else if (speed <= 26000000)
>> + priv->timing = MMC_TIMING_LEGACY;
>> + else
>> + priv->timing = MMC_TIMING_MMC_HS;
>> + }
>
> Why do you assign to timing in host controller?
> MMC Timing is already assigned in mmc core.
As mentioned above, this driver is partly based on the Linux driver
and this one uses this timing variable. Please correct me if I am
wrong, but AFAICT we currently have no such "timing" variable in
the core U-Boot SDHCI code.
>> +
>> + /* Re-init the PHY */
>> + xenon_mmc_phy_set(host);
>> +}
>> +
>> +/* Install a driver specific handler for post set_ios configuration */
>> +static const struct sdhci_ops xenon_sdhci_ops = {
>> + .set_ios_post = xenon_sdhci_set_ios_post
>
> Where is ".set_ios_post" define in sdhci_ops structure?
Please see patch 2/9:
http://patchwork.ozlabs.org/patch/716970/
>> +};
>> +
>> +static int xenon_sdhci_probe(struct udevice *dev)
>> +{
>> + struct xenon_sdhci_plat *plat = dev_get_platdata(dev);
>> + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
>> + struct xenon_sdhci_priv *priv = dev_get_priv(dev);
>> + struct sdhci_host *host = dev_get_priv(dev);
>> + int ret;
>> +
>> + host->mmc = &plat->mmc;
>> + host->mmc->priv = host;
>> + host->mmc->dev = dev;
>> + upriv->mmc = host->mmc;
>> +
>> + /* Set quirks */
>> + host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD | SDHCI_QUIRK_32BIT_DMA_ADDR;
>> +
>> + /* Set default timing */
>> + priv->timing = MMC_TIMING_LEGACY;
>> +
>> + /* Disable auto clock gating during init */
>> + xenon_mmc_set_acg(host, false);
>> +
>> + /* Enable slot */
>> + xenon_mmc_set_slot(host, XENON_MMC_SLOT_ID_HYPERION, true);
>> +
>> + /*
>> + * Set default power on SoC PHY PAD register (currently only
>> + * available on the Armada 3700)
>> + */
>> + if (priv->pad_ctrl_reg)
>> + armada_3700_soc_pad_voltage_set(host);
>> +
>> + ret = sdhci_setup_cfg(&plat->cfg, host, XENON_MMC_MAX_CLK, 0);
>> + if (ret)
>> + return ret;
>> +
>> + plat->cfg.host_caps = MMC_MODE_HS | MMC_MODE_HS_52MHz |
>> + MMC_MODE_DDR_52MHz;
>
> In sdhci_setup_cfg(), host_caps is assigned to some value.
I'll take a look at it.
>> +
>> + switch (fdtdec_get_int(gd->fdt_blob, dev->of_offset, "bus-width", 1)) {
>> + case 8:
>> + plat->cfg.host_caps |= MMC_MODE_8BIT;
>> + break;
>> + case 4:
>> + plat->cfg.host_caps |= MMC_MODE_4BIT;
>> + break;
>> + case 1:
>> + break;
>> + default:
>> + printf("Invalid \"bus-width\" value\n");
>> + return -EINVAL;
>> + }
>> +
>> + host->ops = &xenon_sdhci_ops;
>> +
>> + ret = sdhci_probe(dev);
>> + if (ret)
>> + return ret;
>> +
>> + /* Enable parallel transfer */
>> + xenon_mmc_set_parallel_tran(host, XENON_MMC_SLOT_ID_HYPERION, true);
>> +
>> + /* Disable tuning functionality of this slot */
>> + xenon_mmc_set_tuning(host, XENON_MMC_SLOT_ID_HYPERION, false);
>
> Does it need to pass "false"? I didn't see where xenon_mmc_set_tuning()
> called with "true".
Will remove this parameter.
>> +
>> + /* Enable auto clock gating after init */
>> + xenon_mmc_set_acg(host, true);
>> +
>> + xenon_mask_cmd_conflict_err(host);
>
> Why put this function? this function didn't resue anywhere.
Copied from the Linux driver. So it more in-line with this driver
this way.
>> +
>> + return ret;
>> +}
>> +
>> +static int xenon_sdhci_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + struct sdhci_host *host = dev_get_priv(dev);
>> + struct xenon_sdhci_priv *priv = dev_get_priv(dev);
>> + const char *name;
>> +
>> + host->name = dev->name;
>> + host->ioaddr = (void *)dev_get_addr(dev);
>> +
>> + if (of_device_is_compatible(dev, "marvell,armada-3700-sdhci"))
>> + priv->pad_ctrl_reg = (void *)dev_get_addr_index(dev, 1);
>> +
>> + name = fdt_getprop(gd->fdt_blob, dev->of_offset, "marvell,pad-type",
>> + NULL);
>> + if (name) {
>> + if (0 == strcmp(name, "sd")) {
>
> use strncmp instead of strcmp.
Will do.
Thanks for the review,
Stefan
More information about the U-Boot
mailing list