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

Reply via email to