[U-Boot] [PATCH v2 1/1] mmc: Add MMC support for stm32h7 Socs

[Patrice CHOTARD <patrice.chotard@st.com>]

Hi Jaehoon

On 07/20/2017 06:22 AM, Jaehoon Chung wrote:
> Hi Patrice
> 
> On 07/19/2017 09:20 PM, Patrice CHOTARD wrote:
>> Hi Jaehoon
>>
>> On 07/19/2017 12:11 PM, Jaehoon Chung wrote:
>>> Hi,
>>>
>>> On 07/19/2017 04:27 PM, patrice.chotard at st.com wrote:
>>>> From: Patrice Chotard <patrice.chotard@st.com>
>>>>
>>>> This patch adds SD/MMC support for STM32H7 SoCs.
>>>>
>>>> Here is an extraction of SDMMC main features, embedded in
>>>> STM32H7 SoCs.
>>>> The SD/MMC block include the following:
>>>>    _ Full compliance with MultiMediaCard System Specification
>>>>      Version 4.51. Card support for three different databus modes:
>>>>      1-bit (default), 4-bit and 8-bit.
>>>>    _ Full compatibility with previous versions of MultiMediaCards
>>>>      (backward compatibility).
>>>>    _ Full compliance with SD memory card specifications version 4.1.
>>>>      (SDR104 SDMMC_CK speed limited to maximum allowed IO speed,
>>>>       SPI mode and UHS-II mode not supported).
>>>>    _ Full compliance with SDIO card specification version 4.0.
>>>>      Card support for two different databus modes: 1-bit (default)
>>>>      and 4-bit. (SDR104 SDMMC_CK speed limited to maximum allowed IO
>>>>      speed, SPI mode and UHS-II mode not supported).
>>>>    _ Data transfer up to 208 Mbyte/s for the 8 bit mode.
>>>>      (depending maximum allowed IO speed).
>>>>    _ Data and command output enable signals to control external
>>>>      bidirectional drivers.
>>>>
>>>> The current version of the SDMMC supports only one SD/SDIO/MMC card
>>>> at any one time and a stack of MMC Version 4.51 or previous.
>>>>
>>>> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>>> ---
>>>>
>>>> v2: _ add .get_cd() callback support
>>>>
>>>>    drivers/mmc/Kconfig        |   8 +
>>>>    drivers/mmc/Makefile       |   1 +
>>>>    drivers/mmc/stm32_sdmmc2.c | 619 +++++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 628 insertions(+)
>>>>    create mode 100644 drivers/mmc/stm32_sdmmc2.c
>>>>
>>>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>>>> index 82b8d75..f2e4c26 100644
>>>> --- a/drivers/mmc/Kconfig
>>>> +++ b/drivers/mmc/Kconfig
>>>> @@ -377,6 +377,14 @@ config GENERIC_ATMEL_MCI
>>>>    	  the SD Memory Card Specification V2.0, the SDIO V2.0 specification
>>>>    	  and CE-ATA V1.1.
>>>>    
>>>> +config STM32_SDMMC2
>>>
>>> Why add the SDMMC2? not SDMMC?
>>> Is there a special reason?
>>
>> It's simply the IP name
>>
>>>
>>>> +	bool "STMicroelectronics STM32H7 SD/MMC Host Controller support"
>>>> +	depends on DM_MMC && OF_CONTROL && DM_MMC_OPS
>>>> +	help
>>>> +	  This selects support for the SD/MMC controller on STM32H7 SoCs.
>>>> +	  If you have a board based on such a SoC and with a SD/MMC slot,
>>>> +	  say Y or M here.
>>>> +
>>>>    endif
>>>>    
>>>>    config TEGRA124_MMC_DISABLE_EXT_LOOPBACK
>>>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>>>> index 2d781c3..2584663 100644
>>>> --- a/drivers/mmc/Makefile
>>>> +++ b/drivers/mmc/Makefile
>>>> @@ -43,6 +43,7 @@ obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
>>>>    obj-$(CONFIG_MMC_SANDBOX)		+= sandbox_mmc.o
>>>>    obj-$(CONFIG_SH_MMCIF) += sh_mmcif.o
>>>>    obj-$(CONFIG_SH_SDHI) += sh_sdhi.o
>>>> +obj-$(CONFIG_STM32_SDMMC2) += stm32_sdmmc2.o
>>>>    
>>>>    # SDHCI
>>>>    obj-$(CONFIG_MMC_SDHCI)			+= sdhci.o
>>>> diff --git a/drivers/mmc/stm32_sdmmc2.c b/drivers/mmc/stm32_sdmmc2.c
>>>> new file mode 100644
>>>> index 0000000..84f96e5
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/stm32_sdmmc2.c
>>>> @@ -0,0 +1,619 @@
>>>> +/*
>>>> + *  Copyright (c) 2017 STMicrelectronics
>>>> + *
>>>> + * SPDX-License-Identifier:	GPL-2.0
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <clk.h>
>>>> +#include <dm.h>
>>>> +#include <fdtdec.h>
>>>> +#include <libfdt.h>
>>>> +#include <mmc.h>
>>>> +#include <reset.h>
>>>> +#include <asm/io.h>
>>>> +#include <asm/gpio.h>
>>>> +
>>>> +struct stm32_sdmmc2 {
>>>> +	u32 power;		/* SDMMC power control             [0x00] */
>>>> +	u32 clkcr;		/* SDMMC clock control             [0x04] */
>>>> +	u32 arg;		/* SDMMC argument                  [0x08] */
>>>> +	u32 cmd;		/* SDMMC command                   [0x0C] */
>>>> +	u32 respcmd;		/* SDMMC command response          [0x10] */
>>>> +	u32 resp1;		/* SDMMC response 1                [0x14] */
>>>> +	u32 resp2;		/* SDMMC response 2                [0x18] */
>>>> +	u32 resp3;		/* SDMMC response 3                [0x1C] */
>>>> +	u32 resp4;		/* SDMMC response 4                [0x20] */
>>>> +	u32 dtimer;		/* SDMMC data timer                [0x24] */
>>>> +	u32 dlen;		/* SDMMC data length               [0x28] */
>>>> +	u32 dctrl;		/* SDMMC data control              [0x2C] */
>>>> +	u32 dcount;		/* SDMMC data counter              [0x30] */
>>>> +	u32 sta;		/* SDMMC status                    [0x34] */
>>>> +	u32 icr;		/* SDMMC interrupt clear           [0x38] */
>>>> +	u32 mask;		/* SDMMC mask                      [0x3C] */
>>>> +	u32 acktime;		/* SDMMC Acknowledgment timer      [0x40] */
>>>> +	u32 reserved0[3];	/* Reserved, 0x44 - 0x4C - 0x4C           */
>>>> +	u32 idmactrl;		/* SDMMC DMA control               [0x50] */
>>>> +	u32 idmabsize;		/* SDMMC DMA buffer size           [0x54] */
>>>> +	u32 idmabase0;		/* SDMMC DMA buffer 0 base address [0x58] */
>>>> +	u32 idmabase1;		/* SDMMC DMA buffer 1 base address [0x5C] */
>>>> +	u32 reserved1[8];	/* Reserved, 0x4C-0x7C                    */
>>>> +	u32 fifo;		/* SDMMC data FIFO                 [0x80] */
>>>> +};
>>>
>>> This is for register offset, right?
>>
>> yes
>>
>>>
>>> I want to use the defined value..likes "#define SDMMC_POWER_CONTROL 0x00"
>>> (It's my preference.)
>>> I'm not sure but i have remembered that was difficult to debug.
>>
>> But on http://www.denx.de/wiki/U-Boot/CodingStyle, it's recommended to
>> use structures for I/O access, see "Use structures for I/O access " chapter.
> 
> It's recommended, not mandatory.
> Already used the "define" style in mmc subsystem on u-boot.
> e.g) sdhci/dwmmc controller etc...
> 
> I think it's no problem to use "define" style..
> In my opinion, it's more complex than define..
> 
> For example, some controllers can be changed the register offset according to IP version.
> Then we needs to make new structure for it.
> 
> And if someone calculated wrong offset, then it's possible that all register should be accessed to wrong offset.
> That's why it's my preference.

Ok i will convert this driver with registers offset.

> 
>>
>>
>>>
>>>> +
>>>> +struct stm32_sdmmc2_host {
>>>> +	struct stm32_sdmmc2 *base;
>>>> +	struct mmc_config cfg;
>>>> +	struct clk clk;
>>>> +	struct reset_ctl reset_ctl;
>>>> +	struct gpio_desc cd_gpio;
>>>> +	u32 clk_reg_add;
>>>> +	u32 pwr_reg_add;
>>>> +};
>>>> +
> 
> [..snip..]
> 
>>>> +		cfg->host_caps |= MMC_MODE_4BIT;
>>>> +		break;
>>>> +	case 1:
>>>> +		break;
>>>> +	default:
>>>> +		printf("%s: invalid \"bus-width\" property\n", __func__);
>>>> +		ret = -ENOENT;
>>>> +		goto reset_free;
>>>
>>> Maybe default value can be 1..
>>
>> Default value is already set to 1 in case the property "bus-width" is
>> not present in DT.
>>
>> The "default" case is just a protection in case of "bus-width" property
>> is set with other values than 1, 4 or 8.
> 
> Then i think it can be just displayed wrong dt property value, not returned error
> and host controller and card can be worked with 1bit buswidth..
> how about? :)

Agree, i will fix it

Thanks

Patrice

> 
> Best Regards,
> Jaehoon Chung
> 
>>
>>>
>>>> +	}
>>>> +
>>>> +	mmc = mmc_create(cfg, host);
>>>> +	if (!mmc) {
>>>> +		ret = -ENOMEM;
>>>> +		goto reset_free;
>>>> +	}
>>>> +
>>>> +	mmc->block_dev.removable = !dev_read_bool(dev, "non-removable");
>>>> +	mmc->dev = dev;
>>>> +	upriv->mmc = mmc;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +reset_free:
>>>> +	reset_free(&host->reset_ctl);
>>>> +clk_disable:
>>>> +	clk_disable(&host->clk);
>>>> +clk_free:
>>>> +	clk_free(&host->clk);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static const struct udevice_id stm32_sdmmc2_ids[] = {
>>>> +	{ .compatible = "st,stm32-sdmmc2" },
>>>> +	{ }
>>>> +};
>>>> +
>>>> +U_BOOT_DRIVER(stm32_sdmmc2) = {
>>>> +	.name = "stm32_sdmmc2",
>>>> +	.id = UCLASS_MMC,
>>>> +	.of_match = stm32_sdmmc2_ids,
>>>> +	.ops = &stm32_sdmmc2_ops,
>>>> +	.probe = stm32_sdmmc2_probe,
>>>> +	.priv_auto_alloc_size = sizeof(struct stm32_sdmmc2_host),
>>>> +};
>>>>
>>>
>>
>>
>