diff mbox series

[RFC,v2,05/13] mmc: add nexell driver

Message ID 1585388636-5404-6-git-send-email-stefan_b@posteo.net
State RFC
Delegated to: Tom Rini
Headers show
Series arm: add support for SoC S5P4418 | expand

Commit Message

Stefan Bosch March 28, 2020, 9:43 a.m. UTC
Changes in relation to FriendlyARM's U-Boot nanopi2-v2016.01:
- mmc: nexell_dw_mmc.c changed to nexell_dw_mmc_dm.c (switched to DM).

Signed-off-by: Stefan Bosch <stefan_b@posteo.net>
---

Changes in v2:
- commit "i2c: mmc: add nexell driver (gpio, i2c, mmc, pwm)" splitted
  into separate commits for gpio, i2c, mmc, pwm.

 drivers/mmc/Kconfig            |   6 +
 drivers/mmc/Makefile           |   1 +
 drivers/mmc/nexell_dw_mmc_dm.c | 350 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 357 insertions(+)
 create mode 100644 drivers/mmc/nexell_dw_mmc_dm.c

Comments

Jaehoon Chung April 2, 2020, 11:03 a.m. UTC | #1
Hi,

On 3/28/20 6:43 PM, Stefan Bosch wrote:
> Changes in relation to FriendlyARM's U-Boot nanopi2-v2016.01:
> - mmc: nexell_dw_mmc.c changed to nexell_dw_mmc_dm.c (switched to DM).

It doesn't need to add postfix as _dm.

> 
> Signed-off-by: Stefan Bosch <stefan_b@posteo.net>
> ---
> 
> Changes in v2:
> - commit "i2c: mmc: add nexell driver (gpio, i2c, mmc, pwm)" splitted
>   into separate commits for gpio, i2c, mmc, pwm.
> 
>  drivers/mmc/Kconfig            |   6 +
>  drivers/mmc/Makefile           |   1 +
>  drivers/mmc/nexell_dw_mmc_dm.c | 350 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 357 insertions(+)
>  create mode 100644 drivers/mmc/nexell_dw_mmc_dm.c
> 
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index 2f0eedc..bb8e7c0 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -253,6 +253,12 @@ config MMC_DW_SNPS
>  	  This selects support for Synopsys DesignWare Memory Card Interface driver
>  	  extensions used in various Synopsys ARC devboards.
>  
> +config NEXELL_DWMMC
> +	bool "Nexell SD/MMC controller support"
> +	depends on ARCH_NEXELL
> +	depends on MMC_DW
> +	default y

Not depends on DM_MMC?

> +
>  config MMC_MESON_GX
>  	bool "Meson GX EMMC controller support"
>  	depends on DM_MMC && BLK && ARCH_MESON
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index 9c1f8e5..a7b5a7b 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_SH_MMCIF) += sh_mmcif.o
>  obj-$(CONFIG_SH_SDHI) += sh_sdhi.o
>  obj-$(CONFIG_STM32_SDMMC2) += stm32_sdmmc2.o
>  obj-$(CONFIG_JZ47XX_MMC) += jz_mmc.o
> +obj-$(CONFIG_NEXELL_DWMMC) += nexell_dw_mmc_dm.o
>  
>  # SDHCI
>  obj-$(CONFIG_MMC_SDHCI)			+= sdhci.o
> diff --git a/drivers/mmc/nexell_dw_mmc_dm.c b/drivers/mmc/nexell_dw_mmc_dm.c
> new file mode 100644
> index 0000000..b06b60d
> --- /dev/null
> +++ b/drivers/mmc/nexell_dw_mmc_dm.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2016 Nexell
> + * Youngbok, Park <park@nexell.co.kr>
> + *
> + * (C) Copyright 2019 Stefan Bosch <stefan_b@posteo.net>
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <dt-structs.h>
> +#include <dwmmc.h>
> +#include <syscon.h>
> +#include <asm/gpio.h>
> +#include <asm/arch/nx_gpio.h>
> +#include <asm/arch/reset.h>
> +
> +#define DWMCI_CLKSEL			0x09C
> +#define DWMCI_SHIFT_0			0x0
> +#define DWMCI_SHIFT_1			0x1
> +#define DWMCI_SHIFT_2			0x2
> +#define DWMCI_SHIFT_3			0x3
> +#define DWMCI_SET_SAMPLE_CLK(x)	(x)
> +#define DWMCI_SET_DRV_CLK(x)	((x) << 16)
> +#define DWMCI_SET_DIV_RATIO(x)	((x) << 24)
> +#define DWMCI_CLKCTRL			0x114
> +#define NX_MMC_CLK_DELAY(x, y, a, b)	((((x) & 0xFF) << 0) |\
> +					(((y) & 0x03) << 16) |\
> +					(((a) & 0xFF) << 8)  |\
> +					(((b) & 0x03) << 24))
> +
> +struct nexell_mmc_plat {
> +	struct mmc_config cfg;
> +	struct mmc mmc;
> +};
> +
> +struct nexell_dwmmc_priv {
> +	struct clk *clk;
> +	struct dwmci_host host;
> +	int fifo_size;
> +	bool fifo_mode;
> +	int frequency;
> +	u32 min_freq;
> +	u32 max_freq;
> +	int d_delay;
> +	int d_shift;
> +	int s_delay;
> +	int s_shift;
> +
> +};
> +
> +struct clk *clk_get(const char *id);
> +
> +static void set_pin_stat(int index, int bit, int value)
> +{
> +#if !defined(CONFIG_SPL_BUILD)
> +	nx_gpio_set_pad_function(index, bit, value);
> +#else
> +#if defined(CONFIG_ARCH_S5P4418) ||	\
> +	defined(CONFIG_ARCH_S5P6818)
> +
> +	unsigned long base[5] = {
> +		PHY_BASEADDR_GPIOA, PHY_BASEADDR_GPIOB,
> +		PHY_BASEADDR_GPIOC, PHY_BASEADDR_GPIOD,
> +		PHY_BASEADDR_GPIOE,
> +	};

I don't understand why gpio pin is set in mmc driver?
If nexell soc will change the gpio map and function value, does it needs to add other gpio control?

> +
> +	dw_mmc_set_pin(base[index], bit, value);
> +#endif
> +#endif
> +}
> +
> +static void nx_dw_mmc_set_pin(struct dwmci_host *host)
> +{
> +	debug("  %s(): dev_index == %d", __func__, host->dev_index);
> +
> +	switch (host->dev_index) {
> +	case 0:
> +		set_pin_stat(0, 29, 1);
> +		set_pin_stat(0, 31, 1);
> +		set_pin_stat(1, 1, 1);
> +		set_pin_stat(1, 3, 1);
> +		set_pin_stat(1, 5, 1);
> +		set_pin_stat(1, 7, 1);
> +		break;
> +	case 1:
> +		set_pin_stat(3, 22, 1);
> +		set_pin_stat(3, 23, 1);
> +		set_pin_stat(3, 24, 1);
> +		set_pin_stat(3, 25, 1);
> +		set_pin_stat(3, 26, 1);
> +		set_pin_stat(3, 27, 1);
> +		break;
> +	case 2:
> +		set_pin_stat(2, 18, 2);
> +		set_pin_stat(2, 19, 2);
> +		set_pin_stat(2, 20, 2);
> +		set_pin_stat(2, 21, 2);
> +		set_pin_stat(2, 22, 2);
> +		set_pin_stat(2, 23, 2);
> +		if (host->buswidth == 8) {
> +			set_pin_stat(4, 21, 2);
> +			set_pin_stat(4, 22, 2);
> +			set_pin_stat(4, 23, 2);
> +			set_pin_stat(4, 24, 2);
> +		}
> +		break;
> +	default:
> +		debug(" is invalid!");

Is invalid..then will not work fine? 

i don't check your patch fully.
Your patch doesn't control gpio with dt? 
Well, i don't agree this concept. it need to get opinion how to think about this.

> +	}
> +	debug("\n");
> +}
> +
> +static void nx_dw_mmc_clksel(struct dwmci_host *host)
> +{
> +	u32 val;
> +
> +#ifdef CONFIG_BOOST_MMC

What is BOOST_MMC?

> +	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
> +	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(1);
> +#else
> +	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
> +	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(3);
> +#endif
> +
> +	dwmci_writel(host, DWMCI_CLKSEL, val);
> +}
> +
> +static void nx_dw_mmc_reset(int ch)
> +{
> +	int rst_id = RESET_ID_SDMMC0 + ch;

RESET_ID_SDMMC0? 

> +
> +	nx_rstcon_setrst(rst_id, 0);
> +	nx_rstcon_setrst(rst_id, 1);
> +}
> +
> +static void nx_dw_mmc_clk_delay(struct udevice *dev)
> +{
> +	unsigned int delay;
> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
> +	struct dwmci_host *host = &priv->host;
> +
> +	delay = NX_MMC_CLK_DELAY(priv->d_delay,
> +				 priv->d_shift, priv->s_delay, priv->s_shift);
> +
> +	writel(delay, (host->ioaddr + DWMCI_CLKCTRL));
> +	debug("%s(): Values set: d_delay==%d, d_shift==%d, s_delay==%d, "
> +	      "s_shift==%d\n", __func__, priv->d_delay, priv->d_shift,
> +	      priv->s_delay, priv->s_shift);
> +}
> +
> +static unsigned int nx_dw_mmc_get_clk(struct dwmci_host *host, uint freq)
> +{
> +	struct clk *clk;
> +	struct udevice *dev = host->priv;
> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
> +
> +	int index = host->dev_index;
> +	char name[50] = { 0, };
> +
> +	clk = priv->clk;
> +	if (!clk) {
> +		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);

DEV_NAME_SDHC ???

> +		clk = clk_get((const char *)name);
> +		if (!clk)
> +			return 0;
> +		priv->clk = clk;
> +	}
> +
> +	return clk_get_rate(clk) / 2;
> +}
> +
> +static unsigned long nx_dw_mmc_set_clk(struct dwmci_host *host,
> +				       unsigned int rate)
> +{
> +	struct clk *clk;
> +	char name[50] = { 0, };
> +	struct udevice *dev = host->priv;
> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
> +
> +	int index = host->dev_index;
> +
> +	clk = priv->clk;
> +	if (!clk) {
> +		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
> +		clk = clk_get((const char *)name);
> +		if (!clk)
> +			return 0;
> +		priv->clk = clk;
> +	}
> +
> +	clk_disable(clk);
> +	rate = clk_set_rate(clk, rate);
> +	clk_enable(clk);
> +
> +	return rate;
> +}
> +
> +static int nexell_dwmmc_ofdata_to_platdata(struct udevice *dev)
> +{
> +	/* if (dev): *priv = dev->priv, else: *priv = NULL */
> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
> +	struct dwmci_host *host = &priv->host;
> +	int val = -1;
> +
> +	debug("%s()\n", __func__);
> +
> +	host->name = dev->name;
> +	host->ioaddr = dev_read_addr_ptr(dev);
> +
> +	val = dev_read_u32_default(dev, "nexell,bus-width", -1);
> +	if (val < 0) {
> +		debug("  'nexell,bus-width' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	host->buswidth = val;
> +	host->get_mmc_clk = nx_dw_mmc_get_clk;
> +	host->clksel = nx_dw_mmc_clksel;
> +	host->priv = dev;
> +
> +	val = dev_read_u32_default(dev, "index", -1);
> +	if (val < 0) {
> +		debug("  'index' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	host->dev_index = val;
> +
> +	val = dev_read_u32_default(dev, "fifo-size", 0x20);
> +	if (val <= 0) {
> +		debug("  'fifo-size' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	priv->fifo_size = val;
> +
> +	priv->fifo_mode = dev_read_bool(dev, "fifo-mode");
> +
> +	val = dev_read_u32_default(dev, "frequency", -1);
> +	if (val < 0) {
> +		debug("  'frequency' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	priv->frequency = val;
> +
> +	val = dev_read_u32_default(dev, "max-frequency", -1);
> +	if (val < 0) {
> +		debug("  'max-frequency' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	priv->max_freq = val;
> +	priv->min_freq = 400000;  /* 400 kHz */
> +
> +	val = dev_read_u32_default(dev, "nexell,drive_dly", -1);
> +	if (val < 0) {
> +		debug("  'nexell,drive_dly' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	priv->d_delay = val;
> +
> +	val = dev_read_u32_default(dev, "nexell,drive_shift", -1);
> +	if (val < 0) {
> +		debug("  'nexell,drive_shift' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	priv->d_shift = val;
> +
> +	val = dev_read_u32_default(dev, "nexell,sample_dly", -1);
> +	if (val < 0) {
> +		debug("  'nexell,sample_dly' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	priv->s_delay = val;
> +
> +	val = dev_read_u32_default(dev, "nexell,sample_shift", -1);
> +	if (val < 0) {
> +		debug("  'nexell,sample_shift' missing/invalid!\n");
> +		return -EINVAL;
> +	}
> +	priv->s_shift = val;
> +
> +	debug("  index==%d, name==%s, ioaddr==0x%08x, buswidth==%d, "
> +		  "fifo_size==%d, fifo_mode==%d, frequency==%d\n",
> +		  host->dev_index, host->name, (u32)host->ioaddr,
> +		  host->buswidth, priv->fifo_size, priv->fifo_mode,
> +		  priv->frequency);
> +	debug("  min_freq==%d, max_freq==%d, delay: "
> +		  "0x%02x:0x%02x:0x%02x:0x%02x\n",
> +		  priv->min_freq, priv->max_freq, priv->d_delay,
> +		  priv->d_shift, priv->s_delay, priv->s_shift);

entirely not readable. not need to assign again with val.

priv->s_deley = dev_read_u32_default();


> +
> +	return 0;
> +}
> +
> +static int nexell_dwmmc_probe(struct udevice *dev)
> +{
> +	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
> +	struct dwmci_host *host = &priv->host;
> +	struct udevice *pwr_dev __maybe_unused;
> +
> +	debug("%s():\n", __func__);

Don't add unnecessary debug code. it's meaningless even if it's enabled.

Well, i didn't check other patches..but i think that your patches seems to based on older u-boot version.

Best Regards,
Jaehoon Chung


> +
> +	host->fifoth_val = MSIZE(0x2) |
> +		RX_WMARK(priv->fifo_size / 2 - 1) |
> +		TX_WMARK(priv->fifo_size / 2);
> +
> +	host->fifo_mode = priv->fifo_mode;
> +
> +	dwmci_setup_cfg(&plat->cfg, host, priv->max_freq, priv->min_freq);
> +	host->mmc = &plat->mmc;
> +	host->mmc->priv = &priv->host;
> +	host->mmc->dev = dev;
> +	upriv->mmc = host->mmc;
> +
> +	nx_dw_mmc_set_pin(host);
> +
> +	debug("  nx_dw_mmc_set_clk(host, frequency * 4 == %d)\n",
> +	      priv->frequency * 4);
> +	nx_dw_mmc_set_clk(host, priv->frequency * 4);
> +
> +	nx_dw_mmc_reset(host->dev_index);
> +	nx_dw_mmc_clk_delay(dev);
> +
> +	return dwmci_probe(dev);
> +}
> +
> +static int nexell_dwmmc_bind(struct udevice *dev)
> +{
> +	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
> +
> +	return dwmci_bind(dev, &plat->mmc, &plat->cfg);
> +}
> +
> +static const struct udevice_id nexell_dwmmc_ids[] = {
> +	{ .compatible = "nexell,nexell-dwmmc" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(nexell_dwmmc_drv) = {
> +	.name		= "nexell_dwmmc",
> +	.id		= UCLASS_MMC,
> +	.of_match	= nexell_dwmmc_ids,
> +	.ofdata_to_platdata = nexell_dwmmc_ofdata_to_platdata,
> +	.ops		= &dm_dwmci_ops,
> +	.bind		= nexell_dwmmc_bind,
> +	.probe		= nexell_dwmmc_probe,
> +	.priv_auto_alloc_size = sizeof(struct nexell_dwmmc_priv),
> +	.platdata_auto_alloc_size = sizeof(struct nexell_mmc_plat),
> +};
>
Stefan Bosch April 2, 2020, 4:51 p.m. UTC | #2
Hi,

thanks a lot for your reply. As you already guessed I have ported the 
outdated U-Boot from FriendlyARM, see:
https://github.com/friendlyarm/u-boot/tree/nanopi2-v2016.01

The original MMC-driver has been nexell_dw_mmc.c, so I renamed it to 
nexell_dw_mmc_dm.c after changing to DM.

I will have a closer look at your suggestions and give you feedback ASAP.


Regards
Stefan Bosch


Am 02.04.20 um 13:03 schrieb Jaehoon Chung:
> Hi,
> 
> On 3/28/20 6:43 PM, Stefan Bosch wrote:
>> Changes in relation to FriendlyARM's U-Boot nanopi2-v2016.01:
>> - mmc: nexell_dw_mmc.c changed to nexell_dw_mmc_dm.c (switched to DM).
> 
> It doesn't need to add postfix as _dm.
> 
>>
>> Signed-off-by: Stefan Bosch <stefan_b@posteo.net>
>> ---
>>
>> Changes in v2:
>> - commit "i2c: mmc: add nexell driver (gpio, i2c, mmc, pwm)" splitted
>>    into separate commits for gpio, i2c, mmc, pwm.
>>
>>   drivers/mmc/Kconfig            |   6 +
>>   drivers/mmc/Makefile           |   1 +
>>   drivers/mmc/nexell_dw_mmc_dm.c | 350 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 357 insertions(+)
>>   create mode 100644 drivers/mmc/nexell_dw_mmc_dm.c
>>
>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>> index 2f0eedc..bb8e7c0 100644
>> --- a/drivers/mmc/Kconfig
>> +++ b/drivers/mmc/Kconfig
>> @@ -253,6 +253,12 @@ config MMC_DW_SNPS
>>   	  This selects support for Synopsys DesignWare Memory Card Interface driver
>>   	  extensions used in various Synopsys ARC devboards.
>>   
>> +config NEXELL_DWMMC
>> +	bool "Nexell SD/MMC controller support"
>> +	depends on ARCH_NEXELL
>> +	depends on MMC_DW
>> +	default y
> 
> Not depends on DM_MMC?
> 
>> +
>>   config MMC_MESON_GX
>>   	bool "Meson GX EMMC controller support"
>>   	depends on DM_MMC && BLK && ARCH_MESON
>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>> index 9c1f8e5..a7b5a7b 100644
>> --- a/drivers/mmc/Makefile
>> +++ b/drivers/mmc/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_SH_MMCIF) += sh_mmcif.o
>>   obj-$(CONFIG_SH_SDHI) += sh_sdhi.o
>>   obj-$(CONFIG_STM32_SDMMC2) += stm32_sdmmc2.o
>>   obj-$(CONFIG_JZ47XX_MMC) += jz_mmc.o
>> +obj-$(CONFIG_NEXELL_DWMMC) += nexell_dw_mmc_dm.o
>>   
>>   # SDHCI
>>   obj-$(CONFIG_MMC_SDHCI)			+= sdhci.o
>> diff --git a/drivers/mmc/nexell_dw_mmc_dm.c b/drivers/mmc/nexell_dw_mmc_dm.c
>> new file mode 100644
>> index 0000000..b06b60d
>> --- /dev/null
>> +++ b/drivers/mmc/nexell_dw_mmc_dm.c
>> @@ -0,0 +1,350 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2016 Nexell
>> + * Youngbok, Park <park@nexell.co.kr>
>> + *
>> + * (C) Copyright 2019 Stefan Bosch <stefan_b@posteo.net>
>> + */
>> +
>> +#include <common.h>
>> +#include <clk.h>
>> +#include <dm.h>
>> +#include <dt-structs.h>
>> +#include <dwmmc.h>
>> +#include <syscon.h>
>> +#include <asm/gpio.h>
>> +#include <asm/arch/nx_gpio.h>
>> +#include <asm/arch/reset.h>
>> +
>> +#define DWMCI_CLKSEL			0x09C
>> +#define DWMCI_SHIFT_0			0x0
>> +#define DWMCI_SHIFT_1			0x1
>> +#define DWMCI_SHIFT_2			0x2
>> +#define DWMCI_SHIFT_3			0x3
>> +#define DWMCI_SET_SAMPLE_CLK(x)	(x)
>> +#define DWMCI_SET_DRV_CLK(x)	((x) << 16)
>> +#define DWMCI_SET_DIV_RATIO(x)	((x) << 24)
>> +#define DWMCI_CLKCTRL			0x114
>> +#define NX_MMC_CLK_DELAY(x, y, a, b)	((((x) & 0xFF) << 0) |\
>> +					(((y) & 0x03) << 16) |\
>> +					(((a) & 0xFF) << 8)  |\
>> +					(((b) & 0x03) << 24))
>> +
>> +struct nexell_mmc_plat {
>> +	struct mmc_config cfg;
>> +	struct mmc mmc;
>> +};
>> +
>> +struct nexell_dwmmc_priv {
>> +	struct clk *clk;
>> +	struct dwmci_host host;
>> +	int fifo_size;
>> +	bool fifo_mode;
>> +	int frequency;
>> +	u32 min_freq;
>> +	u32 max_freq;
>> +	int d_delay;
>> +	int d_shift;
>> +	int s_delay;
>> +	int s_shift;
>> +
>> +};
>> +
>> +struct clk *clk_get(const char *id);
>> +
>> +static void set_pin_stat(int index, int bit, int value)
>> +{
>> +#if !defined(CONFIG_SPL_BUILD)
>> +	nx_gpio_set_pad_function(index, bit, value);
>> +#else
>> +#if defined(CONFIG_ARCH_S5P4418) ||	\
>> +	defined(CONFIG_ARCH_S5P6818)
>> +
>> +	unsigned long base[5] = {
>> +		PHY_BASEADDR_GPIOA, PHY_BASEADDR_GPIOB,
>> +		PHY_BASEADDR_GPIOC, PHY_BASEADDR_GPIOD,
>> +		PHY_BASEADDR_GPIOE,
>> +	};
> 
> I don't understand why gpio pin is set in mmc driver?
> If nexell soc will change the gpio map and function value, does it needs to add other gpio control?
> 
>> +
>> +	dw_mmc_set_pin(base[index], bit, value);
>> +#endif
>> +#endif
>> +}
>> +
>> +static void nx_dw_mmc_set_pin(struct dwmci_host *host)
>> +{
>> +	debug("  %s(): dev_index == %d", __func__, host->dev_index);
>> +
>> +	switch (host->dev_index) {
>> +	case 0:
>> +		set_pin_stat(0, 29, 1);
>> +		set_pin_stat(0, 31, 1);
>> +		set_pin_stat(1, 1, 1);
>> +		set_pin_stat(1, 3, 1);
>> +		set_pin_stat(1, 5, 1);
>> +		set_pin_stat(1, 7, 1);
>> +		break;
>> +	case 1:
>> +		set_pin_stat(3, 22, 1);
>> +		set_pin_stat(3, 23, 1);
>> +		set_pin_stat(3, 24, 1);
>> +		set_pin_stat(3, 25, 1);
>> +		set_pin_stat(3, 26, 1);
>> +		set_pin_stat(3, 27, 1);
>> +		break;
>> +	case 2:
>> +		set_pin_stat(2, 18, 2);
>> +		set_pin_stat(2, 19, 2);
>> +		set_pin_stat(2, 20, 2);
>> +		set_pin_stat(2, 21, 2);
>> +		set_pin_stat(2, 22, 2);
>> +		set_pin_stat(2, 23, 2);
>> +		if (host->buswidth == 8) {
>> +			set_pin_stat(4, 21, 2);
>> +			set_pin_stat(4, 22, 2);
>> +			set_pin_stat(4, 23, 2);
>> +			set_pin_stat(4, 24, 2);
>> +		}
>> +		break;
>> +	default:
>> +		debug(" is invalid!");
> 
> Is invalid..then will not work fine?
> 
> i don't check your patch fully.
> Your patch doesn't control gpio with dt?
> Well, i don't agree this concept. it need to get opinion how to think about this.
> 
>> +	}
>> +	debug("\n");
>> +}
>> +
>> +static void nx_dw_mmc_clksel(struct dwmci_host *host)
>> +{
>> +	u32 val;
>> +
>> +#ifdef CONFIG_BOOST_MMC
> 
> What is BOOST_MMC?
> 
>> +	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
>> +	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(1);
>> +#else
>> +	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
>> +	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(3);
>> +#endif
>> +
>> +	dwmci_writel(host, DWMCI_CLKSEL, val);
>> +}
>> +
>> +static void nx_dw_mmc_reset(int ch)
>> +{
>> +	int rst_id = RESET_ID_SDMMC0 + ch;
> 
> RESET_ID_SDMMC0?
> 
>> +
>> +	nx_rstcon_setrst(rst_id, 0);
>> +	nx_rstcon_setrst(rst_id, 1);
>> +}
>> +
>> +static void nx_dw_mmc_clk_delay(struct udevice *dev)
>> +{
>> +	unsigned int delay;
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +	struct dwmci_host *host = &priv->host;
>> +
>> +	delay = NX_MMC_CLK_DELAY(priv->d_delay,
>> +				 priv->d_shift, priv->s_delay, priv->s_shift);
>> +
>> +	writel(delay, (host->ioaddr + DWMCI_CLKCTRL));
>> +	debug("%s(): Values set: d_delay==%d, d_shift==%d, s_delay==%d, "
>> +	      "s_shift==%d\n", __func__, priv->d_delay, priv->d_shift,
>> +	      priv->s_delay, priv->s_shift);
>> +}
>> +
>> +static unsigned int nx_dw_mmc_get_clk(struct dwmci_host *host, uint freq)
>> +{
>> +	struct clk *clk;
>> +	struct udevice *dev = host->priv;
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +
>> +	int index = host->dev_index;
>> +	char name[50] = { 0, };
>> +
>> +	clk = priv->clk;
>> +	if (!clk) {
>> +		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
> 
> DEV_NAME_SDHC ???
> 
>> +		clk = clk_get((const char *)name);
>> +		if (!clk)
>> +			return 0;
>> +		priv->clk = clk;
>> +	}
>> +
>> +	return clk_get_rate(clk) / 2;
>> +}
>> +
>> +static unsigned long nx_dw_mmc_set_clk(struct dwmci_host *host,
>> +				       unsigned int rate)
>> +{
>> +	struct clk *clk;
>> +	char name[50] = { 0, };
>> +	struct udevice *dev = host->priv;
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +
>> +	int index = host->dev_index;
>> +
>> +	clk = priv->clk;
>> +	if (!clk) {
>> +		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
>> +		clk = clk_get((const char *)name);
>> +		if (!clk)
>> +			return 0;
>> +		priv->clk = clk;
>> +	}
>> +
>> +	clk_disable(clk);
>> +	rate = clk_set_rate(clk, rate);
>> +	clk_enable(clk);
>> +
>> +	return rate;
>> +}
>> +
>> +static int nexell_dwmmc_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +	/* if (dev): *priv = dev->priv, else: *priv = NULL */
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +	struct dwmci_host *host = &priv->host;
>> +	int val = -1;
>> +
>> +	debug("%s()\n", __func__);
>> +
>> +	host->name = dev->name;
>> +	host->ioaddr = dev_read_addr_ptr(dev);
>> +
>> +	val = dev_read_u32_default(dev, "nexell,bus-width", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,bus-width' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	host->buswidth = val;
>> +	host->get_mmc_clk = nx_dw_mmc_get_clk;
>> +	host->clksel = nx_dw_mmc_clksel;
>> +	host->priv = dev;
>> +
>> +	val = dev_read_u32_default(dev, "index", -1);
>> +	if (val < 0) {
>> +		debug("  'index' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	host->dev_index = val;
>> +
>> +	val = dev_read_u32_default(dev, "fifo-size", 0x20);
>> +	if (val <= 0) {
>> +		debug("  'fifo-size' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->fifo_size = val;
>> +
>> +	priv->fifo_mode = dev_read_bool(dev, "fifo-mode");
>> +
>> +	val = dev_read_u32_default(dev, "frequency", -1);
>> +	if (val < 0) {
>> +		debug("  'frequency' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->frequency = val;
>> +
>> +	val = dev_read_u32_default(dev, "max-frequency", -1);
>> +	if (val < 0) {
>> +		debug("  'max-frequency' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->max_freq = val;
>> +	priv->min_freq = 400000;  /* 400 kHz */
>> +
>> +	val = dev_read_u32_default(dev, "nexell,drive_dly", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,drive_dly' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->d_delay = val;
>> +
>> +	val = dev_read_u32_default(dev, "nexell,drive_shift", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,drive_shift' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->d_shift = val;
>> +
>> +	val = dev_read_u32_default(dev, "nexell,sample_dly", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,sample_dly' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->s_delay = val;
>> +
>> +	val = dev_read_u32_default(dev, "nexell,sample_shift", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,sample_shift' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->s_shift = val;
>> +
>> +	debug("  index==%d, name==%s, ioaddr==0x%08x, buswidth==%d, "
>> +		  "fifo_size==%d, fifo_mode==%d, frequency==%d\n",
>> +		  host->dev_index, host->name, (u32)host->ioaddr,
>> +		  host->buswidth, priv->fifo_size, priv->fifo_mode,
>> +		  priv->frequency);
>> +	debug("  min_freq==%d, max_freq==%d, delay: "
>> +		  "0x%02x:0x%02x:0x%02x:0x%02x\n",
>> +		  priv->min_freq, priv->max_freq, priv->d_delay,
>> +		  priv->d_shift, priv->s_delay, priv->s_shift);
> 
> entirely not readable. not need to assign again with val.
> 
> priv->s_deley = dev_read_u32_default();
> 
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int nexell_dwmmc_probe(struct udevice *dev)
>> +{
>> +	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
>> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +	struct dwmci_host *host = &priv->host;
>> +	struct udevice *pwr_dev __maybe_unused;
>> +
>> +	debug("%s():\n", __func__);
> 
> Don't add unnecessary debug code. it's meaningless even if it's enabled.
> 
> Well, i didn't check other patches..but i think that your patches seems to based on older u-boot version.
> 
> Best Regards,
> Jaehoon Chung
> 
> 
>> +
>> +	host->fifoth_val = MSIZE(0x2) |
>> +		RX_WMARK(priv->fifo_size / 2 - 1) |
>> +		TX_WMARK(priv->fifo_size / 2);
>> +
>> +	host->fifo_mode = priv->fifo_mode;
>> +
>> +	dwmci_setup_cfg(&plat->cfg, host, priv->max_freq, priv->min_freq);
>> +	host->mmc = &plat->mmc;
>> +	host->mmc->priv = &priv->host;
>> +	host->mmc->dev = dev;
>> +	upriv->mmc = host->mmc;
>> +
>> +	nx_dw_mmc_set_pin(host);
>> +
>> +	debug("  nx_dw_mmc_set_clk(host, frequency * 4 == %d)\n",
>> +	      priv->frequency * 4);
>> +	nx_dw_mmc_set_clk(host, priv->frequency * 4);
>> +
>> +	nx_dw_mmc_reset(host->dev_index);
>> +	nx_dw_mmc_clk_delay(dev);
>> +
>> +	return dwmci_probe(dev);
>> +}
>> +
>> +static int nexell_dwmmc_bind(struct udevice *dev)
>> +{
>> +	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
>> +
>> +	return dwmci_bind(dev, &plat->mmc, &plat->cfg);
>> +}
>> +
>> +static const struct udevice_id nexell_dwmmc_ids[] = {
>> +	{ .compatible = "nexell,nexell-dwmmc" },
>> +	{ }
>> +};
>> +
>> +U_BOOT_DRIVER(nexell_dwmmc_drv) = {
>> +	.name		= "nexell_dwmmc",
>> +	.id		= UCLASS_MMC,
>> +	.of_match	= nexell_dwmmc_ids,
>> +	.ofdata_to_platdata = nexell_dwmmc_ofdata_to_platdata,
>> +	.ops		= &dm_dwmci_ops,
>> +	.bind		= nexell_dwmmc_bind,
>> +	.probe		= nexell_dwmmc_probe,
>> +	.priv_auto_alloc_size = sizeof(struct nexell_dwmmc_priv),
>> +	.platdata_auto_alloc_size = sizeof(struct nexell_mmc_plat),
>> +};
>>
>
Stefan Bosch April 9, 2020, 5:33 p.m. UTC | #3
Hi,

see below my answers to your questions.

Regards
Stefan Bosch


Hi,

thanks a lot for your reply. As you already guessed I have ported the 
outdated U-Boot from FriendlyARM, see:
https://github.com/friendlyarm/u-boot/tree/nanopi2-v2016.01

The original MMC-driver has been nexell_dw_mmc.c, so I renamed it to 
nexell_dw_mmc_dm.c after changing to DM.

I will have a closer look at your suggestions and give you feedback ASAP.


Regards
Stefan Bosch


Am 02.04.20 um 13:03 schrieb Jaehoon Chung:
> Hi,
> 
> On 3/28/20 6:43 PM, Stefan Bosch wrote:
>> Changes in relation to FriendlyARM's U-Boot nanopi2-v2016.01:
>> - mmc: nexell_dw_mmc.c changed to nexell_dw_mmc_dm.c (switched to DM).
> 
> It doesn't need to add postfix as _dm.
> 
Ok, I have renamed it to nexell_dw_mmc.c.

>>
>> Signed-off-by: Stefan Bosch <stefan_b@posteo.net>
>> ---
>>
>> Changes in v2:
>> - commit "i2c: mmc: add nexell driver (gpio, i2c, mmc, pwm)" splitted
>>    into separate commits for gpio, i2c, mmc, pwm.
>>
>>   drivers/mmc/Kconfig            |   6 +
>>   drivers/mmc/Makefile           |   1 +
>>   drivers/mmc/nexell_dw_mmc_dm.c | 350 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 357 insertions(+)
>>   create mode 100644 drivers/mmc/nexell_dw_mmc_dm.c
>>
>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>> index 2f0eedc..bb8e7c0 100644
>> --- a/drivers/mmc/Kconfig
>> +++ b/drivers/mmc/Kconfig
>> @@ -253,6 +253,12 @@ config MMC_DW_SNPS
>>   	  This selects support for Synopsys DesignWare Memory Card Interface driver
>>   	  extensions used in various Synopsys ARC devboards.
>>   
>> +config NEXELL_DWMMC
>> +	bool "Nexell SD/MMC controller support"
>> +	depends on ARCH_NEXELL
>> +	depends on MMC_DW
>> +	default y
> 
> Not depends on DM_MMC?
> 
You are right, I have inserted "depends on DM_MMC". I missed this when 
changing to DM.

>> +
>>   config MMC_MESON_GX
>>   	bool "Meson GX EMMC controller support"
>>   	depends on DM_MMC && BLK && ARCH_MESON
>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>> index 9c1f8e5..a7b5a7b 100644
>> --- a/drivers/mmc/Makefile
>> +++ b/drivers/mmc/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_SH_MMCIF) += sh_mmcif.o
>>   obj-$(CONFIG_SH_SDHI) += sh_sdhi.o
>>   obj-$(CONFIG_STM32_SDMMC2) += stm32_sdmmc2.o
>>   obj-$(CONFIG_JZ47XX_MMC) += jz_mmc.o
>> +obj-$(CONFIG_NEXELL_DWMMC) += nexell_dw_mmc_dm.o
>>   
>>   # SDHCI
>>   obj-$(CONFIG_MMC_SDHCI)			+= sdhci.o
>> diff --git a/drivers/mmc/nexell_dw_mmc_dm.c b/drivers/mmc/nexell_dw_mmc_dm.c
>> new file mode 100644
>> index 0000000..b06b60d
>> --- /dev/null
>> +++ b/drivers/mmc/nexell_dw_mmc_dm.c
>> @@ -0,0 +1,350 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2016 Nexell
>> + * Youngbok, Park <park@nexell.co.kr>
>> + *
>> + * (C) Copyright 2019 Stefan Bosch <stefan_b@posteo.net>
>> + */
>> +
>> +#include <common.h>
>> +#include <clk.h>
>> +#include <dm.h>
>> +#include <dt-structs.h>
>> +#include <dwmmc.h>
>> +#include <syscon.h>
>> +#include <asm/gpio.h>
>> +#include <asm/arch/nx_gpio.h>
>> +#include <asm/arch/reset.h>
>> +
>> +#define DWMCI_CLKSEL			0x09C
>> +#define DWMCI_SHIFT_0			0x0
>> +#define DWMCI_SHIFT_1			0x1
>> +#define DWMCI_SHIFT_2			0x2
>> +#define DWMCI_SHIFT_3			0x3
>> +#define DWMCI_SET_SAMPLE_CLK(x)	(x)
>> +#define DWMCI_SET_DRV_CLK(x)	((x) << 16)
>> +#define DWMCI_SET_DIV_RATIO(x)	((x) << 24)
>> +#define DWMCI_CLKCTRL			0x114
>> +#define NX_MMC_CLK_DELAY(x, y, a, b)	((((x) & 0xFF) << 0) |\
>> +					(((y) & 0x03) << 16) |\
>> +					(((a) & 0xFF) << 8)  |\
>> +					(((b) & 0x03) << 24))
>> +
>> +struct nexell_mmc_plat {
>> +	struct mmc_config cfg;
>> +	struct mmc mmc;
>> +};
>> +
>> +struct nexell_dwmmc_priv {
>> +	struct clk *clk;
>> +	struct dwmci_host host;
>> +	int fifo_size;
>> +	bool fifo_mode;
>> +	int frequency;
>> +	u32 min_freq;
>> +	u32 max_freq;
>> +	int d_delay;
>> +	int d_shift;
>> +	int s_delay;
>> +	int s_shift;
>> +
>> +};
>> +
>> +struct clk *clk_get(const char *id);
>> +
>> +static void set_pin_stat(int index, int bit, int value)
>> +{
>> +#if !defined(CONFIG_SPL_BUILD)
>> +	nx_gpio_set_pad_function(index, bit, value);
>> +#else
>> +#if defined(CONFIG_ARCH_S5P4418) ||	\
>> +	defined(CONFIG_ARCH_S5P6818)
>> +
>> +	unsigned long base[5] = {
>> +		PHY_BASEADDR_GPIOA, PHY_BASEADDR_GPIOB,
>> +		PHY_BASEADDR_GPIOC, PHY_BASEADDR_GPIOD,
>> +		PHY_BASEADDR_GPIOE,
>> +	};
> 
> I don't understand why gpio pin is set in mmc driver?
> If nexell soc will change the gpio map and function value, does it needs to add other gpio control?
> 
>> +
>> +	dw_mmc_set_pin(base[index], bit, value);
>> +#endif
>> +#endif
>> +}
>> +
>> +static void nx_dw_mmc_set_pin(struct dwmci_host *host)
>> +{
>> +	debug("  %s(): dev_index == %d", __func__, host->dev_index);
>> +
>> +	switch (host->dev_index) {
>> +	case 0:
>> +		set_pin_stat(0, 29, 1);
>> +		set_pin_stat(0, 31, 1);
>> +		set_pin_stat(1, 1, 1);
>> +		set_pin_stat(1, 3, 1);
>> +		set_pin_stat(1, 5, 1);
>> +		set_pin_stat(1, 7, 1);
>> +		break;
>> +	case 1:
>> +		set_pin_stat(3, 22, 1);
>> +		set_pin_stat(3, 23, 1);
>> +		set_pin_stat(3, 24, 1);
>> +		set_pin_stat(3, 25, 1);
>> +		set_pin_stat(3, 26, 1);
>> +		set_pin_stat(3, 27, 1);
>> +		break;
>> +	case 2:
>> +		set_pin_stat(2, 18, 2);
>> +		set_pin_stat(2, 19, 2);
>> +		set_pin_stat(2, 20, 2);
>> +		set_pin_stat(2, 21, 2);
>> +		set_pin_stat(2, 22, 2);
>> +		set_pin_stat(2, 23, 2);
>> +		if (host->buswidth == 8) {
>> +			set_pin_stat(4, 21, 2);
>> +			set_pin_stat(4, 22, 2);
>> +			set_pin_stat(4, 23, 2);
>> +			set_pin_stat(4, 24, 2);
>> +		}
>> +		break;
>> +	default:
>> +		debug(" is invalid!");
> 
> Is invalid..then will not work fine?
> 
> i don't check your patch fully.
> Your patch doesn't control gpio with dt?
> Well, i don't agree this concept. it need to get opinion how to think about this.
> 
Nexell has not implemented an pinctrl-driver, so the GPIOs are switched 
to MMC-function here.
I have removed the above "default", now I check for a valid dev_index in 
nexell_dwmmc_ofdata_to_platdata().

>> +	}
>> +	debug("\n");
>> +}
>> +
>> +static void nx_dw_mmc_clksel(struct dwmci_host *host)
>> +{
>> +	u32 val;
>> +
>> +#ifdef CONFIG_BOOST_MMC
> 
> What is BOOST_MMC?
> 
This influences what value is written to register offset 0x9C of the 
MMC-controller. Unfortunatelly, I can not give you more info because 
this register is indicated as "Reserved" in the S5P4418-datasheet I have 
access to (Revision 0.10 from October 2014).

>> +	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
>> +	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(1);
>> +#else
>> +	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
>> +	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(3);
>> +#endif
>> +
>> +	dwmci_writel(host, DWMCI_CLKSEL, val);
>> +}
>> +
>> +static void nx_dw_mmc_reset(int ch)
>> +{
>> +	int rst_id = RESET_ID_SDMMC0 + ch;
> 
> RESET_ID_SDMMC0?
> 
This is defined in arch/arm/include/asm/arch/nexell.h.

>> +
>> +	nx_rstcon_setrst(rst_id, 0);
>> +	nx_rstcon_setrst(rst_id, 1);
>> +}
>> +
>> +static void nx_dw_mmc_clk_delay(struct udevice *dev)
>> +{
>> +	unsigned int delay;
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +	struct dwmci_host *host = &priv->host;
>> +
>> +	delay = NX_MMC_CLK_DELAY(priv->d_delay,
>> +				 priv->d_shift, priv->s_delay, priv->s_shift);
>> +
>> +	writel(delay, (host->ioaddr + DWMCI_CLKCTRL));
>> +	debug("%s(): Values set: d_delay==%d, d_shift==%d, s_delay==%d, "
>> +	      "s_shift==%d\n", __func__, priv->d_delay, priv->d_shift,
>> +	      priv->s_delay, priv->s_shift);
>> +}
>> +
>> +static unsigned int nx_dw_mmc_get_clk(struct dwmci_host *host, uint freq)
>> +{
>> +	struct clk *clk;
>> +	struct udevice *dev = host->priv;
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +
>> +	int index = host->dev_index;
>> +	char name[50] = { 0, };
>> +
>> +	clk = priv->clk;
>> +	if (!clk) {
>> +		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
> 
> DEV_NAME_SDHC ???
> 
This is also defined in arch/arm/include/asm/arch/nexell.h: "nx-sdhc"

>> +		clk = clk_get((const char *)name);
>> +		if (!clk)
>> +			return 0;
>> +		priv->clk = clk;
>> +	}
>> +
>> +	return clk_get_rate(clk) / 2;
>> +}
>> +
>> +static unsigned long nx_dw_mmc_set_clk(struct dwmci_host *host,
>> +				       unsigned int rate)
>> +{
>> +	struct clk *clk;
>> +	char name[50] = { 0, };
>> +	struct udevice *dev = host->priv;
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +
>> +	int index = host->dev_index;
>> +
>> +	clk = priv->clk;
>> +	if (!clk) {
>> +		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
>> +		clk = clk_get((const char *)name);
>> +		if (!clk)
>> +			return 0;
>> +		priv->clk = clk;
>> +	}
>> +
>> +	clk_disable(clk);
>> +	rate = clk_set_rate(clk, rate);
>> +	clk_enable(clk);
>> +
>> +	return rate;
>> +}
>> +
>> +static int nexell_dwmmc_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +	/* if (dev): *priv = dev->priv, else: *priv = NULL */
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +	struct dwmci_host *host = &priv->host;
>> +	int val = -1;
>> +
>> +	debug("%s()\n", __func__);
>> +
>> +	host->name = dev->name;
>> +	host->ioaddr = dev_read_addr_ptr(dev);
>> +
>> +	val = dev_read_u32_default(dev, "nexell,bus-width", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,bus-width' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	host->buswidth = val;
>> +	host->get_mmc_clk = nx_dw_mmc_get_clk;
>> +	host->clksel = nx_dw_mmc_clksel;
>> +	host->priv = dev;
>> +
>> +	val = dev_read_u32_default(dev, "index", -1);
>> +	if (val < 0) {
>> +		debug("  'index' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	host->dev_index = val;
>> +
>> +	val = dev_read_u32_default(dev, "fifo-size", 0x20);
>> +	if (val <= 0) {
>> +		debug("  'fifo-size' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->fifo_size = val;
>> +
>> +	priv->fifo_mode = dev_read_bool(dev, "fifo-mode");
>> +
>> +	val = dev_read_u32_default(dev, "frequency", -1);
>> +	if (val < 0) {
>> +		debug("  'frequency' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->frequency = val;
>> +
>> +	val = dev_read_u32_default(dev, "max-frequency", -1);
>> +	if (val < 0) {
>> +		debug("  'max-frequency' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->max_freq = val;
>> +	priv->min_freq = 400000;  /* 400 kHz */
>> +
>> +	val = dev_read_u32_default(dev, "nexell,drive_dly", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,drive_dly' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->d_delay = val;
>> +
>> +	val = dev_read_u32_default(dev, "nexell,drive_shift", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,drive_shift' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->d_shift = val;
>> +
>> +	val = dev_read_u32_default(dev, "nexell,sample_dly", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,sample_dly' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->s_delay = val;
>> +
>> +	val = dev_read_u32_default(dev, "nexell,sample_shift", -1);
>> +	if (val < 0) {
>> +		debug("  'nexell,sample_shift' missing/invalid!\n");
>> +		return -EINVAL;
>> +	}
>> +	priv->s_shift = val;
>> +
>> +	debug("  index==%d, name==%s, ioaddr==0x%08x, buswidth==%d, "
>> +		  "fifo_size==%d, fifo_mode==%d, frequency==%d\n",
>> +		  host->dev_index, host->name, (u32)host->ioaddr,
>> +		  host->buswidth, priv->fifo_size, priv->fifo_mode,
>> +		  priv->frequency);
>> +	debug("  min_freq==%d, max_freq==%d, delay: "
>> +		  "0x%02x:0x%02x:0x%02x:0x%02x\n",
>> +		  priv->min_freq, priv->max_freq, priv->d_delay,
>> +		  priv->d_shift, priv->s_delay, priv->s_shift);
> 
> entirely not readable. not need to assign again with val.
> 
> priv->s_deley = dev_read_u32_default();
> 
> 
I have reworked nexell_dwmmc_ofdata_to_platdata(), i.e. valid default 
values are used now (where possible) and the appropriate if-blocks have 
been removed.

>> +
>> +	return 0;
>> +}
>> +
>> +static int nexell_dwmmc_probe(struct udevice *dev)
>> +{
>> +	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
>> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
>> +	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>> +	struct dwmci_host *host = &priv->host;
>> +	struct udevice *pwr_dev __maybe_unused;
>> +
>> +	debug("%s():\n", __func__);
> 
> Don't add unnecessary debug code. it's meaningless even if it's enabled.
> 
Ok, I have removed this debug code.

> Well, i didn't check other patches..but i think that your patches seems to based on older u-boot version.
> 
> Best Regards,
> Jaehoon Chung
> 
> 
>> +
>> +	host->fifoth_val = MSIZE(0x2) |
>> +		RX_WMARK(priv->fifo_size / 2 - 1) |
>> +		TX_WMARK(priv->fifo_size / 2);
>> +
>> +	host->fifo_mode = priv->fifo_mode;
>> +
>> +	dwmci_setup_cfg(&plat->cfg, host, priv->max_freq, priv->min_freq);
>> +	host->mmc = &plat->mmc;
>> +	host->mmc->priv = &priv->host;
>> +	host->mmc->dev = dev;
>> +	upriv->mmc = host->mmc;
>> +
>> +	nx_dw_mmc_set_pin(host);
>> +
>> +	debug("  nx_dw_mmc_set_clk(host, frequency * 4 == %d)\n",
>> +	      priv->frequency * 4);
>> +	nx_dw_mmc_set_clk(host, priv->frequency * 4);
>> +
>> +	nx_dw_mmc_reset(host->dev_index);
>> +	nx_dw_mmc_clk_delay(dev);
>> +
>> +	return dwmci_probe(dev);
>> +}
>> +
>> +static int nexell_dwmmc_bind(struct udevice *dev)
>> +{
>> +	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
>> +
>> +	return dwmci_bind(dev, &plat->mmc, &plat->cfg);
>> +}
>> +
>> +static const struct udevice_id nexell_dwmmc_ids[] = {
>> +	{ .compatible = "nexell,nexell-dwmmc" },
>> +	{ }
>> +};
>> +
>> +U_BOOT_DRIVER(nexell_dwmmc_drv) = {
>> +	.name		= "nexell_dwmmc",
>> +	.id		= UCLASS_MMC,
>> +	.of_match	= nexell_dwmmc_ids,
>> +	.ofdata_to_platdata = nexell_dwmmc_ofdata_to_platdata,
>> +	.ops		= &dm_dwmci_ops,
>> +	.bind		= nexell_dwmmc_bind,
>> +	.probe		= nexell_dwmmc_probe,
>> +	.priv_auto_alloc_size = sizeof(struct nexell_dwmmc_priv),
>> +	.platdata_auto_alloc_size = sizeof(struct nexell_mmc_plat),
>> +};
>>
>
Jaehoon Chung April 10, 2020, 12:25 a.m. UTC | #4
Hi,

On 4/10/20 2:33 AM, Stefan B. wrote:
> Hi,
> 
> see below my answers to your questions.
> 
> Regards
> Stefan Bosch
> 
> 
> Hi,
> 
> thanks a lot for your reply. As you already guessed I have ported the outdated U-Boot from FriendlyARM, see:
> https://protect2.fireeye.com/url?k=00de144b-5d1421fc-00df9f04-0cc47a3003e8-6dd6a1886465f918&q=1&u=https%3A%2F%2Fgithub.com%2Ffriendlyarm%2Fu-boot%2Ftree%2Fnanopi2-v2016.01
> 
> The original MMC-driver has been nexell_dw_mmc.c, so I renamed it to nexell_dw_mmc_dm.c after changing to DM.
> 
> I will have a closer look at your suggestions and give you feedback ASAP.

I don't know that you had received reviews about other patches.
If you want to apply new chip, then i think you need to implement drivers based on DM. 

> 
> 
> Regards
> Stefan Bosch
> 
> 
> Am 02.04.20 um 13:03 schrieb Jaehoon Chung:
>> Hi,
>>
>> On 3/28/20 6:43 PM, Stefan Bosch wrote:
>>> Changes in relation to FriendlyARM's U-Boot nanopi2-v2016.01:
>>> - mmc: nexell_dw_mmc.c changed to nexell_dw_mmc_dm.c (switched to DM).
>>
>> It doesn't need to add postfix as _dm.
>>
> Ok, I have renamed it to nexell_dw_mmc.c.
> 
>>>
>>> Signed-off-by: Stefan Bosch <stefan_b@posteo.net>
>>> ---
>>>
>>> Changes in v2:
>>> - commit "i2c: mmc: add nexell driver (gpio, i2c, mmc, pwm)" splitted
>>>    into separate commits for gpio, i2c, mmc, pwm.
>>>
>>>   drivers/mmc/Kconfig            |   6 +
>>>   drivers/mmc/Makefile           |   1 +
>>>   drivers/mmc/nexell_dw_mmc_dm.c | 350 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 357 insertions(+)
>>>   create mode 100644 drivers/mmc/nexell_dw_mmc_dm.c
>>>
>>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>>> index 2f0eedc..bb8e7c0 100644
>>> --- a/drivers/mmc/Kconfig
>>> +++ b/drivers/mmc/Kconfig
>>> @@ -253,6 +253,12 @@ config MMC_DW_SNPS
>>>         This selects support for Synopsys DesignWare Memory Card Interface driver
>>>         extensions used in various Synopsys ARC devboards.
>>>   +config NEXELL_DWMMC
>>> +    bool "Nexell SD/MMC controller support"
>>> +    depends on ARCH_NEXELL
>>> +    depends on MMC_DW
>>> +    default y
>>
>> Not depends on DM_MMC?
>>
> You are right, I have inserted "depends on DM_MMC". I missed this when changing to DM.
> 
>>> +
>>>   config MMC_MESON_GX
>>>       bool "Meson GX EMMC controller support"
>>>       depends on DM_MMC && BLK && ARCH_MESON
>>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>>> index 9c1f8e5..a7b5a7b 100644
>>> --- a/drivers/mmc/Makefile
>>> +++ b/drivers/mmc/Makefile
>>> @@ -43,6 +43,7 @@ obj-$(CONFIG_SH_MMCIF) += sh_mmcif.o
>>>   obj-$(CONFIG_SH_SDHI) += sh_sdhi.o
>>>   obj-$(CONFIG_STM32_SDMMC2) += stm32_sdmmc2.o
>>>   obj-$(CONFIG_JZ47XX_MMC) += jz_mmc.o
>>> +obj-$(CONFIG_NEXELL_DWMMC) += nexell_dw_mmc_dm.o
>>>     # SDHCI
>>>   obj-$(CONFIG_MMC_SDHCI)            += sdhci.o
>>> diff --git a/drivers/mmc/nexell_dw_mmc_dm.c b/drivers/mmc/nexell_dw_mmc_dm.c
>>> new file mode 100644
>>> index 0000000..b06b60d
>>> --- /dev/null
>>> +++ b/drivers/mmc/nexell_dw_mmc_dm.c
>>> @@ -0,0 +1,350 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * (C) Copyright 2016 Nexell
>>> + * Youngbok, Park <park@nexell.co.kr>
>>> + *
>>> + * (C) Copyright 2019 Stefan Bosch <stefan_b@posteo.net>
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <clk.h>
>>> +#include <dm.h>
>>> +#include <dt-structs.h>
>>> +#include <dwmmc.h>
>>> +#include <syscon.h>
>>> +#include <asm/gpio.h>
>>> +#include <asm/arch/nx_gpio.h>
>>> +#include <asm/arch/reset.h>
>>> +
>>> +#define DWMCI_CLKSEL            0x09C
>>> +#define DWMCI_SHIFT_0            0x0
>>> +#define DWMCI_SHIFT_1            0x1
>>> +#define DWMCI_SHIFT_2            0x2
>>> +#define DWMCI_SHIFT_3            0x3
>>> +#define DWMCI_SET_SAMPLE_CLK(x)    (x)
>>> +#define DWMCI_SET_DRV_CLK(x)    ((x) << 16)
>>> +#define DWMCI_SET_DIV_RATIO(x)    ((x) << 24)
>>> +#define DWMCI_CLKCTRL            0x114
>>> +#define NX_MMC_CLK_DELAY(x, y, a, b)    ((((x) & 0xFF) << 0) |\
>>> +                    (((y) & 0x03) << 16) |\
>>> +                    (((a) & 0xFF) << 8)  |\
>>> +                    (((b) & 0x03) << 24))
>>> +
>>> +struct nexell_mmc_plat {
>>> +    struct mmc_config cfg;
>>> +    struct mmc mmc;
>>> +};
>>> +
>>> +struct nexell_dwmmc_priv {
>>> +    struct clk *clk;
>>> +    struct dwmci_host host;
>>> +    int fifo_size;
>>> +    bool fifo_mode;
>>> +    int frequency;
>>> +    u32 min_freq;
>>> +    u32 max_freq;
>>> +    int d_delay;
>>> +    int d_shift;
>>> +    int s_delay;
>>> +    int s_shift;
>>> +
>>> +};
>>> +
>>> +struct clk *clk_get(const char *id);
>>> +
>>> +static void set_pin_stat(int index, int bit, int value)
>>> +{
>>> +#if !defined(CONFIG_SPL_BUILD)
>>> +    nx_gpio_set_pad_function(index, bit, value);
>>> +#else
>>> +#if defined(CONFIG_ARCH_S5P4418) ||    \
>>> +    defined(CONFIG_ARCH_S5P6818)
>>> +
>>> +    unsigned long base[5] = {
>>> +        PHY_BASEADDR_GPIOA, PHY_BASEADDR_GPIOB,
>>> +        PHY_BASEADDR_GPIOC, PHY_BASEADDR_GPIOD,
>>> +        PHY_BASEADDR_GPIOE,
>>> +    };
>>
>> I don't understand why gpio pin is set in mmc driver?
>> If nexell soc will change the gpio map and function value, does it needs to add other gpio control?
>>
>>> +
>>> +    dw_mmc_set_pin(base[index], bit, value);
>>> +#endif
>>> +#endif
>>> +}
>>> +
>>> +static void nx_dw_mmc_set_pin(struct dwmci_host *host)
>>> +{
>>> +    debug("  %s(): dev_index == %d", __func__, host->dev_index);
>>> +
>>> +    switch (host->dev_index) {
>>> +    case 0:
>>> +        set_pin_stat(0, 29, 1);
>>> +        set_pin_stat(0, 31, 1);
>>> +        set_pin_stat(1, 1, 1);
>>> +        set_pin_stat(1, 3, 1);
>>> +        set_pin_stat(1, 5, 1);
>>> +        set_pin_stat(1, 7, 1);
>>> +        break;
>>> +    case 1:
>>> +        set_pin_stat(3, 22, 1);
>>> +        set_pin_stat(3, 23, 1);
>>> +        set_pin_stat(3, 24, 1);
>>> +        set_pin_stat(3, 25, 1);
>>> +        set_pin_stat(3, 26, 1);
>>> +        set_pin_stat(3, 27, 1);
>>> +        break;
>>> +    case 2:
>>> +        set_pin_stat(2, 18, 2);
>>> +        set_pin_stat(2, 19, 2);
>>> +        set_pin_stat(2, 20, 2);
>>> +        set_pin_stat(2, 21, 2);
>>> +        set_pin_stat(2, 22, 2);
>>> +        set_pin_stat(2, 23, 2);
>>> +        if (host->buswidth == 8) {
>>> +            set_pin_stat(4, 21, 2);
>>> +            set_pin_stat(4, 22, 2);
>>> +            set_pin_stat(4, 23, 2);
>>> +            set_pin_stat(4, 24, 2);
>>> +        }
>>> +        break;
>>> +    default:
>>> +        debug(" is invalid!");
>>
>> Is invalid..then will not work fine?
>>
>> i don't check your patch fully.
>> Your patch doesn't control gpio with dt?
>> Well, i don't agree this concept. it need to get opinion how to think about this.
>>
> Nexell has not implemented an pinctrl-driver, so the GPIOs are switched to MMC-function here.
> I have removed the above "default", now I check for a valid dev_index in nexell_dwmmc_ofdata_to_platdata().

Then it needs to implement pinctrl driver.

> 
>>> +    }
>>> +    debug("\n");
>>> +}
>>> +
>>> +static void nx_dw_mmc_clksel(struct dwmci_host *host)
>>> +{
>>> +    u32 val;
>>> +
>>> +#ifdef CONFIG_BOOST_MMC
>>
>> What is BOOST_MMC?
>>
> This influences what value is written to register offset 0x9C of the MMC-controller. Unfortunatelly, I can not give you more info because this register is indicated as "Reserved" in the S5P4418-datasheet I have access to (Revision 0.10 from October 2014).

I didn't find where BOOST_MMC config is defined.
And it can be used with device-tree property. As i know it's relevant to clk phase and timing.

> 
>>> +    val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
>>> +        DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(1);
>>> +#else
>>> +    val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
>>> +        DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(3);
>>> +#endif
>>> +
>>> +    dwmci_writel(host, DWMCI_CLKSEL, val);
>>> +}
>>> +
>>> +static void nx_dw_mmc_reset(int ch)
>>> +{
>>> +    int rst_id = RESET_ID_SDMMC0 + ch;
>>
>> RESET_ID_SDMMC0?
>>
> This is defined in arch/arm/include/asm/arch/nexell.h.
> 
>>> +
>>> +    nx_rstcon_setrst(rst_id, 0);
>>> +    nx_rstcon_setrst(rst_id, 1);
>>> +}
>>> +
>>> +static void nx_dw_mmc_clk_delay(struct udevice *dev)
>>> +{
>>> +    unsigned int delay;
>>> +    struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>>> +    struct dwmci_host *host = &priv->host;
>>> +
>>> +    delay = NX_MMC_CLK_DELAY(priv->d_delay,
>>> +                 priv->d_shift, priv->s_delay, priv->s_shift);
>>> +
>>> +    writel(delay, (host->ioaddr + DWMCI_CLKCTRL));
>>> +    debug("%s(): Values set: d_delay==%d, d_shift==%d, s_delay==%d, "
>>> +          "s_shift==%d\n", __func__, priv->d_delay, priv->d_shift,
>>> +          priv->s_delay, priv->s_shift);
>>> +}
>>> +
>>> +static unsigned int nx_dw_mmc_get_clk(struct dwmci_host *host, uint freq)
>>> +{
>>> +    struct clk *clk;
>>> +    struct udevice *dev = host->priv;
>>> +    struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>>> +
>>> +    int index = host->dev_index;
>>> +    char name[50] = { 0, };
>>> +
>>> +    clk = priv->clk;
>>> +    if (!clk) {
>>> +        sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
>>
>> DEV_NAME_SDHC ???
>>
> This is also defined in arch/arm/include/asm/arch/nexell.h: "nx-sdhc"
> 
>>> +        clk = clk_get((const char *)name);
>>> +        if (!clk)
>>> +            return 0;
>>> +        priv->clk = clk;
>>> +    }
>>> +
>>> +    return clk_get_rate(clk) / 2;
>>> +}
>>> +
>>> +static unsigned long nx_dw_mmc_set_clk(struct dwmci_host *host,
>>> +                       unsigned int rate)
>>> +{
>>> +    struct clk *clk;
>>> +    char name[50] = { 0, };
>>> +    struct udevice *dev = host->priv;
>>> +    struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>>> +
>>> +    int index = host->dev_index;
>>> +
>>> +    clk = priv->clk;
>>> +    if (!clk) {
>>> +        sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
>>> +        clk = clk_get((const char *)name);
>>> +        if (!clk)
>>> +            return 0;
>>> +        priv->clk = clk;
>>> +    }
>>> +
>>> +    clk_disable(clk);
>>> +    rate = clk_set_rate(clk, rate);
>>> +    clk_enable(clk);
>>> +
>>> +    return rate;
>>> +}
>>> +
>>> +static int nexell_dwmmc_ofdata_to_platdata(struct udevice *dev)
>>> +{
>>> +    /* if (dev): *priv = dev->priv, else: *priv = NULL */
>>> +    struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>>> +    struct dwmci_host *host = &priv->host;
>>> +    int val = -1;
>>> +
>>> +    debug("%s()\n", __func__);
>>> +
>>> +    host->name = dev->name;
>>> +    host->ioaddr = dev_read_addr_ptr(dev);
>>> +
>>> +    val = dev_read_u32_default(dev, "nexell,bus-width", -1);
>>> +    if (val < 0) {
>>> +        debug("  'nexell,bus-width' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    host->buswidth = val;
>>> +    host->get_mmc_clk = nx_dw_mmc_get_clk;
>>> +    host->clksel = nx_dw_mmc_clksel;
>>> +    host->priv = dev;
>>> +
>>> +    val = dev_read_u32_default(dev, "index", -1);
>>> +    if (val < 0) {
>>> +        debug("  'index' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    host->dev_index = val;
>>> +
>>> +    val = dev_read_u32_default(dev, "fifo-size", 0x20);
>>> +    if (val <= 0) {
>>> +        debug("  'fifo-size' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv->fifo_size = val;
>>> +
>>> +    priv->fifo_mode = dev_read_bool(dev, "fifo-mode");
>>> +
>>> +    val = dev_read_u32_default(dev, "frequency", -1);
>>> +    if (val < 0) {
>>> +        debug("  'frequency' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv->frequency = val;
>>> +
>>> +    val = dev_read_u32_default(dev, "max-frequency", -1);
>>> +    if (val < 0) {
>>> +        debug("  'max-frequency' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv->max_freq = val;
>>> +    priv->min_freq = 400000;  /* 400 kHz */
>>> +
>>> +    val = dev_read_u32_default(dev, "nexell,drive_dly", -1);
>>> +    if (val < 0) {
>>> +        debug("  'nexell,drive_dly' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv->d_delay = val;
>>> +
>>> +    val = dev_read_u32_default(dev, "nexell,drive_shift", -1);
>>> +    if (val < 0) {
>>> +        debug("  'nexell,drive_shift' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv->d_shift = val;
>>> +
>>> +    val = dev_read_u32_default(dev, "nexell,sample_dly", -1);
>>> +    if (val < 0) {
>>> +        debug("  'nexell,sample_dly' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv->s_delay = val;
>>> +
>>> +    val = dev_read_u32_default(dev, "nexell,sample_shift", -1);
>>> +    if (val < 0) {
>>> +        debug("  'nexell,sample_shift' missing/invalid!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv->s_shift = val;
>>> +
>>> +    debug("  index==%d, name==%s, ioaddr==0x%08x, buswidth==%d, "
>>> +          "fifo_size==%d, fifo_mode==%d, frequency==%d\n",
>>> +          host->dev_index, host->name, (u32)host->ioaddr,
>>> +          host->buswidth, priv->fifo_size, priv->fifo_mode,
>>> +          priv->frequency);
>>> +    debug("  min_freq==%d, max_freq==%d, delay: "
>>> +          "0x%02x:0x%02x:0x%02x:0x%02x\n",
>>> +          priv->min_freq, priv->max_freq, priv->d_delay,
>>> +          priv->d_shift, priv->s_delay, priv->s_shift);
>>
>> entirely not readable. not need to assign again with val.
>>
>> priv->s_deley = dev_read_u32_default();
>>
>>
> I have reworked nexell_dwmmc_ofdata_to_platdata(), i.e. valid default values are used now (where possible) and the appropriate if-blocks have been removed.
> 
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int nexell_dwmmc_probe(struct udevice *dev)
>>> +{
>>> +    struct nexell_mmc_plat *plat = dev_get_platdata(dev);
>>> +    struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
>>> +    struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
>>> +    struct dwmci_host *host = &priv->host;
>>> +    struct udevice *pwr_dev __maybe_unused;
>>> +
>>> +    debug("%s():\n", __func__);
>>
>> Don't add unnecessary debug code. it's meaningless even if it's enabled.
>>
> Ok, I have removed this debug code.
> 
>> Well, i didn't check other patches..but i think that your patches seems to based on older u-boot version.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>
>>> +
>>> +    host->fifoth_val = MSIZE(0x2) |
>>> +        RX_WMARK(priv->fifo_size / 2 - 1) |
>>> +        TX_WMARK(priv->fifo_size / 2);
>>> +
>>> +    host->fifo_mode = priv->fifo_mode;
>>> +
>>> +    dwmci_setup_cfg(&plat->cfg, host, priv->max_freq, priv->min_freq);
>>> +    host->mmc = &plat->mmc;
>>> +    host->mmc->priv = &priv->host;
>>> +    host->mmc->dev = dev;
>>> +    upriv->mmc = host->mmc;
>>> +
>>> +    nx_dw_mmc_set_pin(host);
>>> +
>>> +    debug("  nx_dw_mmc_set_clk(host, frequency * 4 == %d)\n",
>>> +          priv->frequency * 4);
>>> +    nx_dw_mmc_set_clk(host, priv->frequency * 4);
>>> +
>>> +    nx_dw_mmc_reset(host->dev_index);
>>> +    nx_dw_mmc_clk_delay(dev);
>>> +
>>> +    return dwmci_probe(dev);
>>> +}
>>> +
>>> +static int nexell_dwmmc_bind(struct udevice *dev)
>>> +{
>>> +    struct nexell_mmc_plat *plat = dev_get_platdata(dev);
>>> +
>>> +    return dwmci_bind(dev, &plat->mmc, &plat->cfg);
>>> +}
>>> +
>>> +static const struct udevice_id nexell_dwmmc_ids[] = {
>>> +    { .compatible = "nexell,nexell-dwmmc" },
>>> +    { }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(nexell_dwmmc_drv) = {
>>> +    .name        = "nexell_dwmmc",
>>> +    .id        = UCLASS_MMC,
>>> +    .of_match    = nexell_dwmmc_ids,
>>> +    .ofdata_to_platdata = nexell_dwmmc_ofdata_to_platdata,
>>> +    .ops        = &dm_dwmci_ops,
>>> +    .bind        = nexell_dwmmc_bind,
>>> +    .probe        = nexell_dwmmc_probe,
>>> +    .priv_auto_alloc_size = sizeof(struct nexell_dwmmc_priv),
>>> +    .platdata_auto_alloc_size = sizeof(struct nexell_mmc_plat),
>>> +};
>>>
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 2f0eedc..bb8e7c0 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -253,6 +253,12 @@  config MMC_DW_SNPS
 	  This selects support for Synopsys DesignWare Memory Card Interface driver
 	  extensions used in various Synopsys ARC devboards.
 
+config NEXELL_DWMMC
+	bool "Nexell SD/MMC controller support"
+	depends on ARCH_NEXELL
+	depends on MMC_DW
+	default y
+
 config MMC_MESON_GX
 	bool "Meson GX EMMC controller support"
 	depends on DM_MMC && BLK && ARCH_MESON
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
index 9c1f8e5..a7b5a7b 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -43,6 +43,7 @@  obj-$(CONFIG_SH_MMCIF) += sh_mmcif.o
 obj-$(CONFIG_SH_SDHI) += sh_sdhi.o
 obj-$(CONFIG_STM32_SDMMC2) += stm32_sdmmc2.o
 obj-$(CONFIG_JZ47XX_MMC) += jz_mmc.o
+obj-$(CONFIG_NEXELL_DWMMC) += nexell_dw_mmc_dm.o
 
 # SDHCI
 obj-$(CONFIG_MMC_SDHCI)			+= sdhci.o
diff --git a/drivers/mmc/nexell_dw_mmc_dm.c b/drivers/mmc/nexell_dw_mmc_dm.c
new file mode 100644
index 0000000..b06b60d
--- /dev/null
+++ b/drivers/mmc/nexell_dw_mmc_dm.c
@@ -0,0 +1,350 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2016 Nexell
+ * Youngbok, Park <park@nexell.co.kr>
+ *
+ * (C) Copyright 2019 Stefan Bosch <stefan_b@posteo.net>
+ */
+
+#include <common.h>
+#include <clk.h>
+#include <dm.h>
+#include <dt-structs.h>
+#include <dwmmc.h>
+#include <syscon.h>
+#include <asm/gpio.h>
+#include <asm/arch/nx_gpio.h>
+#include <asm/arch/reset.h>
+
+#define DWMCI_CLKSEL			0x09C
+#define DWMCI_SHIFT_0			0x0
+#define DWMCI_SHIFT_1			0x1
+#define DWMCI_SHIFT_2			0x2
+#define DWMCI_SHIFT_3			0x3
+#define DWMCI_SET_SAMPLE_CLK(x)	(x)
+#define DWMCI_SET_DRV_CLK(x)	((x) << 16)
+#define DWMCI_SET_DIV_RATIO(x)	((x) << 24)
+#define DWMCI_CLKCTRL			0x114
+#define NX_MMC_CLK_DELAY(x, y, a, b)	((((x) & 0xFF) << 0) |\
+					(((y) & 0x03) << 16) |\
+					(((a) & 0xFF) << 8)  |\
+					(((b) & 0x03) << 24))
+
+struct nexell_mmc_plat {
+	struct mmc_config cfg;
+	struct mmc mmc;
+};
+
+struct nexell_dwmmc_priv {
+	struct clk *clk;
+	struct dwmci_host host;
+	int fifo_size;
+	bool fifo_mode;
+	int frequency;
+	u32 min_freq;
+	u32 max_freq;
+	int d_delay;
+	int d_shift;
+	int s_delay;
+	int s_shift;
+
+};
+
+struct clk *clk_get(const char *id);
+
+static void set_pin_stat(int index, int bit, int value)
+{
+#if !defined(CONFIG_SPL_BUILD)
+	nx_gpio_set_pad_function(index, bit, value);
+#else
+#if defined(CONFIG_ARCH_S5P4418) ||	\
+	defined(CONFIG_ARCH_S5P6818)
+
+	unsigned long base[5] = {
+		PHY_BASEADDR_GPIOA, PHY_BASEADDR_GPIOB,
+		PHY_BASEADDR_GPIOC, PHY_BASEADDR_GPIOD,
+		PHY_BASEADDR_GPIOE,
+	};
+
+	dw_mmc_set_pin(base[index], bit, value);
+#endif
+#endif
+}
+
+static void nx_dw_mmc_set_pin(struct dwmci_host *host)
+{
+	debug("  %s(): dev_index == %d", __func__, host->dev_index);
+
+	switch (host->dev_index) {
+	case 0:
+		set_pin_stat(0, 29, 1);
+		set_pin_stat(0, 31, 1);
+		set_pin_stat(1, 1, 1);
+		set_pin_stat(1, 3, 1);
+		set_pin_stat(1, 5, 1);
+		set_pin_stat(1, 7, 1);
+		break;
+	case 1:
+		set_pin_stat(3, 22, 1);
+		set_pin_stat(3, 23, 1);
+		set_pin_stat(3, 24, 1);
+		set_pin_stat(3, 25, 1);
+		set_pin_stat(3, 26, 1);
+		set_pin_stat(3, 27, 1);
+		break;
+	case 2:
+		set_pin_stat(2, 18, 2);
+		set_pin_stat(2, 19, 2);
+		set_pin_stat(2, 20, 2);
+		set_pin_stat(2, 21, 2);
+		set_pin_stat(2, 22, 2);
+		set_pin_stat(2, 23, 2);
+		if (host->buswidth == 8) {
+			set_pin_stat(4, 21, 2);
+			set_pin_stat(4, 22, 2);
+			set_pin_stat(4, 23, 2);
+			set_pin_stat(4, 24, 2);
+		}
+		break;
+	default:
+		debug(" is invalid!");
+	}
+	debug("\n");
+}
+
+static void nx_dw_mmc_clksel(struct dwmci_host *host)
+{
+	u32 val;
+
+#ifdef CONFIG_BOOST_MMC
+	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
+	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(1);
+#else
+	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
+	    DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(3);
+#endif
+
+	dwmci_writel(host, DWMCI_CLKSEL, val);
+}
+
+static void nx_dw_mmc_reset(int ch)
+{
+	int rst_id = RESET_ID_SDMMC0 + ch;
+
+	nx_rstcon_setrst(rst_id, 0);
+	nx_rstcon_setrst(rst_id, 1);
+}
+
+static void nx_dw_mmc_clk_delay(struct udevice *dev)
+{
+	unsigned int delay;
+	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
+	struct dwmci_host *host = &priv->host;
+
+	delay = NX_MMC_CLK_DELAY(priv->d_delay,
+				 priv->d_shift, priv->s_delay, priv->s_shift);
+
+	writel(delay, (host->ioaddr + DWMCI_CLKCTRL));
+	debug("%s(): Values set: d_delay==%d, d_shift==%d, s_delay==%d, "
+	      "s_shift==%d\n", __func__, priv->d_delay, priv->d_shift,
+	      priv->s_delay, priv->s_shift);
+}
+
+static unsigned int nx_dw_mmc_get_clk(struct dwmci_host *host, uint freq)
+{
+	struct clk *clk;
+	struct udevice *dev = host->priv;
+	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
+
+	int index = host->dev_index;
+	char name[50] = { 0, };
+
+	clk = priv->clk;
+	if (!clk) {
+		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
+		clk = clk_get((const char *)name);
+		if (!clk)
+			return 0;
+		priv->clk = clk;
+	}
+
+	return clk_get_rate(clk) / 2;
+}
+
+static unsigned long nx_dw_mmc_set_clk(struct dwmci_host *host,
+				       unsigned int rate)
+{
+	struct clk *clk;
+	char name[50] = { 0, };
+	struct udevice *dev = host->priv;
+	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
+
+	int index = host->dev_index;
+
+	clk = priv->clk;
+	if (!clk) {
+		sprintf(name, "%s.%d", DEV_NAME_SDHC, index);
+		clk = clk_get((const char *)name);
+		if (!clk)
+			return 0;
+		priv->clk = clk;
+	}
+
+	clk_disable(clk);
+	rate = clk_set_rate(clk, rate);
+	clk_enable(clk);
+
+	return rate;
+}
+
+static int nexell_dwmmc_ofdata_to_platdata(struct udevice *dev)
+{
+	/* if (dev): *priv = dev->priv, else: *priv = NULL */
+	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
+	struct dwmci_host *host = &priv->host;
+	int val = -1;
+
+	debug("%s()\n", __func__);
+
+	host->name = dev->name;
+	host->ioaddr = dev_read_addr_ptr(dev);
+
+	val = dev_read_u32_default(dev, "nexell,bus-width", -1);
+	if (val < 0) {
+		debug("  'nexell,bus-width' missing/invalid!\n");
+		return -EINVAL;
+	}
+	host->buswidth = val;
+	host->get_mmc_clk = nx_dw_mmc_get_clk;
+	host->clksel = nx_dw_mmc_clksel;
+	host->priv = dev;
+
+	val = dev_read_u32_default(dev, "index", -1);
+	if (val < 0) {
+		debug("  'index' missing/invalid!\n");
+		return -EINVAL;
+	}
+	host->dev_index = val;
+
+	val = dev_read_u32_default(dev, "fifo-size", 0x20);
+	if (val <= 0) {
+		debug("  'fifo-size' missing/invalid!\n");
+		return -EINVAL;
+	}
+	priv->fifo_size = val;
+
+	priv->fifo_mode = dev_read_bool(dev, "fifo-mode");
+
+	val = dev_read_u32_default(dev, "frequency", -1);
+	if (val < 0) {
+		debug("  'frequency' missing/invalid!\n");
+		return -EINVAL;
+	}
+	priv->frequency = val;
+
+	val = dev_read_u32_default(dev, "max-frequency", -1);
+	if (val < 0) {
+		debug("  'max-frequency' missing/invalid!\n");
+		return -EINVAL;
+	}
+	priv->max_freq = val;
+	priv->min_freq = 400000;  /* 400 kHz */
+
+	val = dev_read_u32_default(dev, "nexell,drive_dly", -1);
+	if (val < 0) {
+		debug("  'nexell,drive_dly' missing/invalid!\n");
+		return -EINVAL;
+	}
+	priv->d_delay = val;
+
+	val = dev_read_u32_default(dev, "nexell,drive_shift", -1);
+	if (val < 0) {
+		debug("  'nexell,drive_shift' missing/invalid!\n");
+		return -EINVAL;
+	}
+	priv->d_shift = val;
+
+	val = dev_read_u32_default(dev, "nexell,sample_dly", -1);
+	if (val < 0) {
+		debug("  'nexell,sample_dly' missing/invalid!\n");
+		return -EINVAL;
+	}
+	priv->s_delay = val;
+
+	val = dev_read_u32_default(dev, "nexell,sample_shift", -1);
+	if (val < 0) {
+		debug("  'nexell,sample_shift' missing/invalid!\n");
+		return -EINVAL;
+	}
+	priv->s_shift = val;
+
+	debug("  index==%d, name==%s, ioaddr==0x%08x, buswidth==%d, "
+		  "fifo_size==%d, fifo_mode==%d, frequency==%d\n",
+		  host->dev_index, host->name, (u32)host->ioaddr,
+		  host->buswidth, priv->fifo_size, priv->fifo_mode,
+		  priv->frequency);
+	debug("  min_freq==%d, max_freq==%d, delay: "
+		  "0x%02x:0x%02x:0x%02x:0x%02x\n",
+		  priv->min_freq, priv->max_freq, priv->d_delay,
+		  priv->d_shift, priv->s_delay, priv->s_shift);
+
+	return 0;
+}
+
+static int nexell_dwmmc_probe(struct udevice *dev)
+{
+	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
+	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
+	struct nexell_dwmmc_priv *priv = dev_get_priv(dev);
+	struct dwmci_host *host = &priv->host;
+	struct udevice *pwr_dev __maybe_unused;
+
+	debug("%s():\n", __func__);
+
+	host->fifoth_val = MSIZE(0x2) |
+		RX_WMARK(priv->fifo_size / 2 - 1) |
+		TX_WMARK(priv->fifo_size / 2);
+
+	host->fifo_mode = priv->fifo_mode;
+
+	dwmci_setup_cfg(&plat->cfg, host, priv->max_freq, priv->min_freq);
+	host->mmc = &plat->mmc;
+	host->mmc->priv = &priv->host;
+	host->mmc->dev = dev;
+	upriv->mmc = host->mmc;
+
+	nx_dw_mmc_set_pin(host);
+
+	debug("  nx_dw_mmc_set_clk(host, frequency * 4 == %d)\n",
+	      priv->frequency * 4);
+	nx_dw_mmc_set_clk(host, priv->frequency * 4);
+
+	nx_dw_mmc_reset(host->dev_index);
+	nx_dw_mmc_clk_delay(dev);
+
+	return dwmci_probe(dev);
+}
+
+static int nexell_dwmmc_bind(struct udevice *dev)
+{
+	struct nexell_mmc_plat *plat = dev_get_platdata(dev);
+
+	return dwmci_bind(dev, &plat->mmc, &plat->cfg);
+}
+
+static const struct udevice_id nexell_dwmmc_ids[] = {
+	{ .compatible = "nexell,nexell-dwmmc" },
+	{ }
+};
+
+U_BOOT_DRIVER(nexell_dwmmc_drv) = {
+	.name		= "nexell_dwmmc",
+	.id		= UCLASS_MMC,
+	.of_match	= nexell_dwmmc_ids,
+	.ofdata_to_platdata = nexell_dwmmc_ofdata_to_platdata,
+	.ops		= &dm_dwmci_ops,
+	.bind		= nexell_dwmmc_bind,
+	.probe		= nexell_dwmmc_probe,
+	.priv_auto_alloc_size = sizeof(struct nexell_dwmmc_priv),
+	.platdata_auto_alloc_size = sizeof(struct nexell_mmc_plat),
+};