Hi Jaehoon On 02/02/2017 09:00 AM, Jaehoon Chung wrote: > Hi, > > On 01/21/2017 01:05 AM, patrice.chot...@st.com wrote: >> From: Patrice Chotard <patrice.chot...@st.com> > > I want to put minimum commit message at here. > You need to rework this on latest u-boot. > > Before sending patch, run checkpatch.pl at least. Also add the Maintainers. > After that, i will review more. > >> >> Signed-off-by: Patrice Chotard <patrice.chot...@st.com> >> --- >> arch/arm/Kconfig | 2 + >> arch/arm/include/asm/arch-stih410/sdhci.h | 69 +++++++++++++++ >> arch/arm/include/asm/arch-stih410/sti.h | 5 ++ >> board/st/stih410-b2260/board.c | 3 + >> drivers/mmc/Kconfig | 7 ++ >> drivers/mmc/Makefile | 1 + >> drivers/mmc/sti_sdhci.c | 137 >> ++++++++++++++++++++++++++++++ >> include/configs/stih410-b2260.h | 4 + >> include/dm/platform_data/sti_sdhci.h | 17 ++++ >> 9 files changed, 245 insertions(+) >> create mode 100644 arch/arm/include/asm/arch-stih410/sdhci.h >> create mode 100644 drivers/mmc/sti_sdhci.c >> create mode 100644 include/dm/platform_data/sti_sdhci.h >> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig >> index 98546ae..9a472e6 100644 >> --- a/arch/arm/Kconfig >> +++ b/arch/arm/Kconfig >> @@ -897,6 +897,8 @@ config ARCH_STI >> select CPU_V7 >> select DM >> select DM_SERIAL >> + select BLK >> + select DM_MMC >> help >> Support for STMicroelectronics STiH407/10 SoC family. >> This SoC is used on Linaro 96Board STiH410-B2260 >> diff --git a/arch/arm/include/asm/arch-stih410/sdhci.h >> b/arch/arm/include/asm/arch-stih410/sdhci.h >> new file mode 100644 >> index 0000000..f45b961 >> --- /dev/null >> +++ b/arch/arm/include/asm/arch-stih410/sdhci.h >> @@ -0,0 +1,69 @@ >> +/* >> + * (C) Copyright 2017 Patrice Chotard <patrice.chot...@st.com> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#ifndef __STI_SDHCI_H__ >> +#define __STI_SDHCI_H__ >> + >> +#define FLASHSS_MMC_CORE_CONFIG_1 0x400 >> +#define FLASHSS_MMC_CORECFG_TIMEOUT_CLK_UNIT_MHZ BIT(24) >> +#define FLASHSS_MMC_CORECFG_TIMEOUT_CLK_FREQ_MIN BIT(12) >> + >> +#define STI_FLASHSS_MMC_CORE_CONFIG_1 \ >> + (FLASHSS_MMC_CORECFG_TIMEOUT_CLK_UNIT_MHZ | \ >> + FLASHSS_MMC_CORECFG_TIMEOUT_CLK_FREQ_MIN) >> + >> +#define FLASHSS_MMC_CORE_CONFIG_2 0x404 >> +#define FLASHSS_MMC_CORECFG_HIGH_SPEED BIT(28) >> +#define FLASHSS_MMC_CORECFG_8BIT_EMMC BIT(20) >> +#define MAX_BLK_LENGTH_1024 BIT(16) >> +#define BASE_CLK_FREQ_200 0xc8 >> + >> + >> +#define STI_FLASHSS_MMC_CORE_CONFIG2 \ >> + (FLASHSS_MMC_CORECFG_HIGH_SPEED | \ >> + FLASHSS_MMC_CORECFG_8BIT_EMMC | \ >> + MAX_BLK_LENGTH_1024 | \ >> + BASE_CLK_FREQ_200 << 0) >> + >> +#define STI_FLASHSS_SDCARD_CORE_CONFIG2 \ >> + (FLASHSS_MMC_CORECFG_HIGH_SPEED | \ >> + MAX_BLK_LENGTH_1024 | \ >> + BASE_CLK_FREQ_200) >> + >> +#define FLASHSS_MMC_CORE_CONFIG_3 0x408 >> +#define FLASHSS_MMC_CORECFG_SLOT_TYPE_EMMC BIT(28) >> +#define FLASHSS_MMC_CORECFG_ASYNCH_INTR_SUPPORT BIT(20) >> +#define FLASHSS_MMC_CORECFG_3P3_VOLT BIT(8) >> +#define FLASHSS_MMC_CORECFG_SUSP_RES_SUPPORT BIT(4) >> +#define FLASHSS_MMC_CORECFG_SDMA BIT(0) >> + >> +#define STI_FLASHSS_MMC_CORE_CONFIG3 \ >> + (FLASHSS_MMC_CORECFG_SLOT_TYPE_EMMC | \ >> + FLASHSS_MMC_CORECFG_ASYNCH_INTR_SUPPORT | \ >> + FLASHSS_MMC_CORECFG_3P3_VOLT | \ >> + FLASHSS_MMC_CORECFG_SUSP_RES_SUPPORT | \ >> + FLASHSS_MMC_CORECFG_SDMA) >> + >> +#define STI_FLASHSS_SDCARD_CORE_CONFIG3 \ >> + (FLASHSS_MMC_CORECFG_ASYNCH_INTR_SUPPORT | \ >> + FLASHSS_MMC_CORECFG_3P3_VOLT | \ >> + FLASHSS_MMC_CORECFG_SUSP_RES_SUPPORT | \ >> + FLASHSS_MMC_CORECFG_SDMA) >> + >> +#define FLASHSS_MMC_CORE_CONFIG_4 0x40c >> +#define FLASHSS_MMC_CORECFG_D_DRIVER_SUPPORT BIT(20) >> +#define FLASHSS_MMC_CORECFG_C_DRIVER_SUPPORT BIT(16) >> +#define FLASHSS_MMC_CORECFG_A_DRIVER_SUPPORT BIT(12) >> + >> +#define STI_FLASHSS_MMC_CORE_CONFIG4 \ >> + (FLASHSS_MMC_CORECFG_D_DRIVER_SUPPORT | \ >> + FLASHSS_MMC_CORECFG_C_DRIVER_SUPPORT | \ >> + FLASHSS_MMC_CORECFG_A_DRIVER_SUPPORT) >> + >> +#define ST_MMC_CCONFIG_REG_5 0x210 >> +#define SYSCONF_MMC1_ENABLE_BIT 3 >> + >> +#endif /* _STI_SDHCI_H_ */ >> diff --git a/arch/arm/include/asm/arch-stih410/sti.h >> b/arch/arm/include/asm/arch-stih410/sti.h >> index f167560..d9e6f03 100644 >> --- a/arch/arm/include/asm/arch-stih410/sti.h >> +++ b/arch/arm/include/asm/arch-stih410/sti.h >> @@ -17,4 +17,9 @@ >> /* ASC UART located in the main "COMMs" block */ >> #define STIH410_ASC1_BASE (STIH410_COMMS_BASE + 0x00031000) >> >> +/* MMC Controller Base (in the FlashSS) */ >> +#define STIH410_FLASH_IF_REG1_BASE 0x09060000 >> +#define STIH410_CONFIG_SYS_FLASHSS_PORT1_BASE >> STIH410_FLASH_IF_REG1_BASE >> +#define STIH410_CONFIG_SYS_MMC0_BASE >> STIH410_CONFIG_SYS_FLASHSS_PORT1_BASE /* MMC #0 */ >> + >> #endif /* _STI_H_ */ >> diff --git a/board/st/stih410-b2260/board.c b/board/st/stih410-b2260/board.c >> index 72b0042..607a52f 100644 >> --- a/board/st/stih410-b2260/board.c >> +++ b/board/st/stih410-b2260/board.c >> @@ -7,9 +7,12 @@ >> */ >> >> #include <common.h> >> +#include <mmc.h> >> +#include <sdhci.h> >> #include <asm/arch/sti.h> >> #include <dm/platdata.h> >> #include <dm/platform_data/serial_sti_asc.h> >> +#include <dm/platform_data/sti_sdhci.h> > > Why adds these header at here?
right, it's useless > >> >> DECLARE_GLOBAL_DATA_PTR; >> >> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig >> index 9ed8da3..e1197b4 100644 >> --- a/drivers/mmc/Kconfig >> +++ b/drivers/mmc/Kconfig >> @@ -296,6 +296,13 @@ config MMC_SDHCI_TEGRA >> platform with SD or MMC devices, say Y here. >> >> If unsure, say N. >> + >> +config MMC_SDHCI_STI >> + bool "SDHCI support for STMicroelectronics SoC" >> + depends on MMC_SDHCI >> + help >> + This selects the Secure Digital Host Controller Interface (SDHCI) >> + on STMicroelectronics STiH410 SoC. > > Do the alphabet ordering will fix it > >> >> config MMC_SUNXI >> bool "Allwinner sunxi SD/MMC Host Controller support" >> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile >> index 4dca09c..78c0a73 100644 >> --- a/drivers/mmc/Makefile >> +++ b/drivers/mmc/Makefile >> @@ -67,6 +67,7 @@ obj-$(CONFIG_MMC_SDHCI_KONA) += kona_sdhci.o >> 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_STI) += sti_sdhci.o >> obj-$(CONFIG_MMC_SDHCI_TEGRA) += tegra_mmc.o >> >> obj-$(CONFIG_MMC_SUNXI) += sunxi_mmc.o >> diff --git a/drivers/mmc/sti_sdhci.c b/drivers/mmc/sti_sdhci.c >> new file mode 100644 >> index 0000000..ca4013c >> --- /dev/null >> +++ b/drivers/mmc/sti_sdhci.c >> @@ -0,0 +1,137 @@ >> +/* >> + * Copyright (c) 2017 >> + * Patrice Chotard <patrice.chot...@st.com> >> + * >> + * SPDX-License-Identifier: GPL-2.0 >> + */ >> + >> +#include <common.h> >> +#include <dm.h> >> +#include <mmc.h> >> +#include <sdhci.h> >> +#include <asm/arch/gpio.h> >> +#include <asm/arch/sdhci.h> >> +#include <asm/arch/sti.h> >> +#include <asm/arch/syscfg.h> >> +#include <dm/platform_data/sti_sdhci.h> >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + >> +/** >> + * sti_mmc_core_config: configure the Arasan HC >> + * @ioaddr: base address > > Where is ioaddr argument? i forgot to update the param list, i will fix it > >> + * Description: this function is to configure the Arasan MMC HC. >> + * This should be called when the system starts in case of, on the SoC, >> + * it is needed to configure the host controller. >> + * This happens on some SoCs, i.e. StiH410, where the MMC0 inside the >> flashSS >> + * needs to be configured as MMC 4.5 to have full capabilities. >> + * W/o these settings the SDHCI could configure and use the embedded >> controller >> + * with limited features. >> + */ >> +static void sti_mmc_core_config(const u32 regbase, int mmc_instance) >> +{ >> + unsigned long sysconf; >> + >> + /* reset MMC1 */ >> + if (mmc_instance) { >> + sysconf = readl(STIH410_SYSCONF5_BASE + ST_MMC_CCONFIG_REG_5); >> + SET_SYSCONF_BIT(sysconf, 1, SYSCONF_MMC1_ENABLE_BIT); >> + writel(sysconf, STIH410_SYSCONF5_BASE + ST_MMC_CCONFIG_REG_5); >> + } >> + >> + writel(STI_FLASHSS_MMC_CORE_CONFIG_1, regbase + >> FLASHSS_MMC_CORE_CONFIG_1); >> + >> + if (mmc_instance) { >> + writel(STI_FLASHSS_MMC_CORE_CONFIG2, regbase + >> FLASHSS_MMC_CORE_CONFIG_2); >> + writel(STI_FLASHSS_MMC_CORE_CONFIG3, regbase + >> FLASHSS_MMC_CORE_CONFIG_3); >> + } else { >> + writel(STI_FLASHSS_SDCARD_CORE_CONFIG2, regbase + >> FLASHSS_MMC_CORE_CONFIG_2); >> + writel(STI_FLASHSS_SDCARD_CORE_CONFIG3, regbase + >> FLASHSS_MMC_CORE_CONFIG_3); >> + } >> + writel(STI_FLASHSS_MMC_CORE_CONFIG4, regbase + >> FLASHSS_MMC_CORE_CONFIG_4); >> +} >> + >> +int sti_sdhci_probe(struct udevice *dev) >> +{ >> + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >> + struct sti_sdhci_plat *plat = dev_get_platdata(dev); >> + struct sdhci_host *host = dev_get_priv(dev); >> + int node = dev->of_offset; >> + const void *blob = gd->fdt_blob; > > Not need to use blob and node variables. i will remove them and use directly dev->of_offset and gd->fdt_blob > >> + int ret, count, mmc_instance; >> + >> + /* identify current mmc instance, mmc1 has a reset, not mmc0 */ > > MMC1 is which card..? You have to add comment more corret. MMC0 is wired to the SD slot, MMC1 is wired on the high speed connector I will add an additionnal comment > >> + fdt_getprop(blob, node, "resets", &count); >> + if (count < 0) { >> + mmc_instance = 0; >> + host->voltages = MMC_VDD_165_195; >> + } else { >> + mmc_instance = 1; >> + host->voltages = MMC_VDD_32_33 | MMC_VDD_33_34; >> + } > > if sdhci capabilities register is support VDD_330 and VDD_300 and VDD_180? > Your sdhci controller is broken CAPABILITES register? This part of code was extracted from our internal legacy U-Boot software which is quite old and which has not followed the community evolution. After double checking, the host->voltage settings can be removed. > > what related with having "resets"? resets is for just checking mmc1? Yes, for the moment i identify MMC instances with reset presence > >> + >> + sti_mmc_core_config((const u32) host->ioaddr, mmc_instance); >> + >> + host->name = "sti_sdhci"; > > dev->name, not "sti_sdhci". ok > >> + host->quirks = SDHCI_QUIRK_BROKEN_VOLTAGE | >> + SDHCI_QUIRK_BROKEN_R1B | >> + SDHCI_QUIRK_WAIT_SEND_CMD | >> + SDHCI_QUIRK_32BIT_DMA_ADDR | >> + SDHCI_QUIRK_NO_HISPD_BIT; > > Really need to set SDHCI_QUIRK_BROKEN_R1B? As explained above, i extracted these settings from our internal legacy U-boot software. I tested without BROKEN_VOLTAGE and BROKEN_R1B and it works. I will update this too > >> + >> + host->host_caps = MMC_MODE_DDR_52MHz; >> + >> + ret = sdhci_setup_cfg(&plat->cfg, host, 50000000, 400000); >> + >> + host->mmc = &plat->mmc; > > Why do you put this at here? and after this, checking "ret"? right, will put host->mmc = &plat->mmc after ret test > >> + if (ret) >> + return ret; >> + >> + host->mmc->priv = host; >> + host->mmc->dev = dev; >> + upriv->mmc = host->mmc; >> + >> + return sdhci_probe(dev); >> +} >> + >> +static int sti_sdhci_ofdata_to_platdata(struct udevice *dev) >> +{ >> + struct sdhci_host *host = dev_get_priv(dev); >> + >> + host->name = strdup(dev->name); > > duplicated host->name = "sti_sdhci" ok > >> + host->ioaddr = (void *)dev_get_addr(dev); >> + >> + host->bus_width = fdtdec_get_int(gd->fdt_blob, dev->of_offset, >> + "bus-width", 4); > > default 4bit? i don't think so. In both case, MMC0 or MMC1 are wired to a SD slot, so only 4 data lines > >> + >> + return 0; >> +} >> + >> +static int sti_sdhci_bind(struct udevice *dev) >> +{ >> + struct sti_sdhci_plat *plat = dev_get_platdata(dev); >> + int ret; >> + >> + ret = sdhci_bind(dev, &plat->mmc, &plat->cfg); >> + if (ret) >> + return ret; >> + >> + return 0; > > just resturn sdhci_bind(); right, i will fix this > >> +} >> + >> +static const struct udevice_id sti_sdhci_ids[] = { >> + { .compatible = "st,sdhci" }, >> + { } >> +}; >> + >> +U_BOOT_DRIVER(sti_mmc) = { >> + .name = "sti_sdhci", >> + .id = UCLASS_MMC, >> + .of_match = sti_sdhci_ids, >> + .bind = sti_sdhci_bind, >> + .ops = &sdhci_ops, >> + .ofdata_to_platdata = sti_sdhci_ofdata_to_platdata, >> + .probe = sti_sdhci_probe, >> + .priv_auto_alloc_size = sizeof(struct sdhci_host), >> + .platdata_auto_alloc_size = sizeof(struct sti_sdhci_plat), >> +}; >> diff --git a/include/configs/stih410-b2260.h >> b/include/configs/stih410-b2260.h >> index b4b3d75..7d97847 100644 >> --- a/include/configs/stih410-b2260.h >> +++ b/include/configs/stih410-b2260.h >> @@ -36,9 +36,13 @@ >> #define CONFIG_ENV_SIZE 0x4000 >> #define CONFIG_SYS_NO_FLASH >> >> +#define CONFIG_GENERIC_MMC > > Not need. ok, i will remove it > >> /* Extra Commands */ >> #define CONFIG_CMD_ASKENV >> >> +#define CONFIG_DOS_PARTITION >> +#define CONFIG_SETUP_MEMORY_TAGS > > what are these related with SDHCI driver? Right there are not related, i will dispatch these flags in correct patches > >> + >> /* Size of malloc() pool */ >> #define CONFIG_SYS_MALLOC_LEN 0x1800000 >> #define CONFIG_SYS_GBL_DATA_SIZE 1024 /* Global data structures */ >> diff --git a/include/dm/platform_data/sti_sdhci.h >> b/include/dm/platform_data/sti_sdhci.h >> new file mode 100644 >> index 0000000..d8cc170 >> --- /dev/null >> +++ b/include/dm/platform_data/sti_sdhci.h >> @@ -0,0 +1,17 @@ >> + >> + * (C) Copyright 2017 >> + * Patrice Chotard <patrice.chot...@st.com> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#ifndef __STI_SDHCI_H >> +#define __STI_SDHCI_H >> + >> +struct sti_sdhci_plat { >> + unsigned long *base; /* address of registers in physical memory */ > > I didn't find this usage. > >> + struct mmc_config cfg; >> + struct mmc mmc; >> +}; > > I want to put this structure into your sdhci driver. > There is no specific sti_sdhci platform data. ok i will remove include/dm/platform_data/sti_sdhci.h > > Best Regards, > Jaehoon Chung Thanks Patrice > >> + >> +#endif /* __STI_SDHCI_H */ >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot