[U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver support for SOCFPGA

[U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver support for SOCFPGA

[Chin Liang See]

To add the DesignWare MMC driver support for Altera SOCFPGA. It
required information such as clocks and bus width from platform
specific files (SOCFPGA handoff files)

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Rajeshwari Shinde <rajeshwari.s@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Andy Fleming <afleming@freescale.com>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
---
Changes for v2
- Adding u-boot-mmc maintainer
---
 arch/arm/include/asm/arch-socfpga/dwmmc.h |   12 +++++
 drivers/mmc/Makefile                      |    1 +
 drivers/mmc/socfpga_dw_mmc.c              |   72 +++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100755 arch/arm/include/asm/arch-socfpga/dwmmc.h
 create mode 100755 drivers/mmc/socfpga_dw_mmc.c

diff --git a/arch/arm/include/asm/arch-socfpga/dwmmc.h b/arch/arm/include/asm/arch-socfpga/dwmmc.h
new file mode 100755
index 0000000..945eb64
--- /dev/null
+++ b/arch/arm/include/asm/arch-socfpga/dwmmc.h
@@ -0,0 +1,12 @@
+/*
+ * (C) Copyright 2013 Altera Corporation <www.altera.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef	_SOCFPGA_DWMMC_H_
+#define	_SOCFPGA_DWMMC_H_
+
+extern int socfpga_dwmmc_init(u32 regbase, int bus_width, int index);
+
+#endif /* _SOCFPGA_SDMMC_H_ */
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
index 1ed26ca..e793ed9 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_TEGRA_MMC) += tegra_mmc.o
 obj-$(CONFIG_DWMMC) += dw_mmc.o
 obj-$(CONFIG_EXYNOS_DWMMC) += exynos_dw_mmc.o
 obj-$(CONFIG_ZYNQ_SDHCI) += zynq_sdhci.o
+obj-$(CONFIG_SOCFPGA_DWMMC) += socfpga_dw_mmc.o
 ifdef CONFIG_SPL_BUILD
 obj-$(CONFIG_SPL_MMC_BOOT) += fsl_esdhc_spl.o
 else
diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c
new file mode 100755
index 0000000..554f51b
--- /dev/null
+++ b/drivers/mmc/socfpga_dw_mmc.c
@@ -0,0 +1,72 @@
+/*
+ * (C) Copyright 2013 Altera Corporation <www.altera.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <dwmmc.h>
+#include <asm/arch/dwmmc.h>
+
+#define CLKMGR_PERPLLGRP_EN_REG		(SOCFPGA_CLKMGR_ADDRESS + 0xA0)
+#define CLKMGR_SDMMC_CLK_ENABLE		(1 << 8)
+#define SYSMGR_SDMMCGRP_CTRL_REG	(SOCFPGA_SYSMGR_ADDRESS + 0x108)
+#define SYSMGR_SDMMC_CTRL_GET_DRVSEL(x)	(((x) >> 0) & 0x7)
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)	\
+	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
+
+static char *SOCFPGA_NAME = "SOCFPGA DWMMC";
+
+static void socfpga_dwmci_clksel(struct dwmci_host *host)
+{
+	unsigned int en;
+	unsigned int drvsel;
+	unsigned int smplsel;
+
+	/* Disable SDMMC clock. */
+	en = readl(CLKMGR_PERPLLGRP_EN_REG);
+	en &= ~CLKMGR_SDMMC_CLK_ENABLE;
+	writel(en, CLKMGR_PERPLLGRP_EN_REG);
+
+	/* Configures drv_sel and smpl_sel */
+	drvsel = 3;
+	smplsel = 0;
+
+	debug("%s: drvsel %d smplsel %d\n", __FUNCTION__, drvsel, smplsel);
+	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),
+		SYSMGR_SDMMCGRP_CTRL_REG);
+
+	debug("%s: SYSMGR_SDMMCGRP_CTRL_REG = 0x%x\n", __FUNCTION__,
+		readl(SYSMGR_SDMMCGRP_CTRL_REG));
+	/* Enable SDMMC clock */
+	en = readl(CLKMGR_PERPLLGRP_EN_REG);
+	en |= CLKMGR_SDMMC_CLK_ENABLE;
+	writel(en, CLKMGR_PERPLLGRP_EN_REG);
+}
+
+int socfpga_dwmmc_init(u32 regbase, int bus_width, int index)
+{
+	struct dwmci_host *host = NULL;
+	host = calloc(sizeof(struct dwmci_host), 1);
+	if (!host) {
+		printf("dwmci_host calloc fail!\n");
+		return 1;
+	}
+
+	host->name = SOCFPGA_NAME;
+	host->ioaddr = (void *)regbase;
+	host->buswidth = bus_width;
+	host->clksel = socfpga_dwmci_clksel;
+	host->dev_index = index;
+	/* fixed clock divide by 4 which due to the SDMMC wrapper */
+	host->bus_hz = CONFIG_DWMMC_BUS_HZ;
+	host->fifoth_val = MSIZE(0x2) |
+		RX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2 - 1) |
+		TX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2);
+
+	add_dwmci(host, host->bus_hz, 400000);
+
+	return 0;
+}
+
-- 
1.7.9.5

[U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver support for SOCFPGA

[Jaehoon Chung]

Hi, Chin.

On 12/19/2013 02:16 AM, Chin Liang See wrote:
> To add the DesignWare MMC driver support for Altera SOCFPGA. It
> required information such as clocks and bus width from platform
> specific files (SOCFPGA handoff files)
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Rajeshwari Shinde <rajeshwari.s@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Andy Fleming <afleming@freescale.com>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
> Changes for v2
> - Adding u-boot-mmc maintainer
> ---
>  arch/arm/include/asm/arch-socfpga/dwmmc.h |   12 +++++
>  drivers/mmc/Makefile                      |    1 +
>  drivers/mmc/socfpga_dw_mmc.c              |   72 +++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
>  create mode 100755 arch/arm/include/asm/arch-socfpga/dwmmc.h
>  create mode 100755 drivers/mmc/socfpga_dw_mmc.c
> 
> diff --git a/arch/arm/include/asm/arch-socfpga/dwmmc.h b/arch/arm/include/asm/arch-socfpga/dwmmc.h
> new file mode 100755
> index 0000000..945eb64
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-socfpga/dwmmc.h
> @@ -0,0 +1,12 @@
> +/*
> + * (C) Copyright 2013 Altera Corporation <www.altera.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef	_SOCFPGA_DWMMC_H_
> +#define	_SOCFPGA_DWMMC_H_
> +
> +extern int socfpga_dwmmc_init(u32 regbase, int bus_width, int index);
> +
> +#endif /* _SOCFPGA_SDMMC_H_ */
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index 1ed26ca..e793ed9 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_TEGRA_MMC) += tegra_mmc.o
>  obj-$(CONFIG_DWMMC) += dw_mmc.o
>  obj-$(CONFIG_EXYNOS_DWMMC) += exynos_dw_mmc.o
>  obj-$(CONFIG_ZYNQ_SDHCI) += zynq_sdhci.o
> +obj-$(CONFIG_SOCFPGA_DWMMC) += socfpga_dw_mmc.o
>  ifdef CONFIG_SPL_BUILD
>  obj-$(CONFIG_SPL_MMC_BOOT) += fsl_esdhc_spl.o
>  else
> diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c
> new file mode 100755
> index 0000000..554f51b
> --- /dev/null
> +++ b/drivers/mmc/socfpga_dw_mmc.c
> @@ -0,0 +1,72 @@
> +/*
> + * (C) Copyright 2013 Altera Corporation <www.altera.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <dwmmc.h>
> +#include <asm/arch/dwmmc.h>
> +
> +#define CLKMGR_PERPLLGRP_EN_REG		(SOCFPGA_CLKMGR_ADDRESS + 0xA0)
> +#define CLKMGR_SDMMC_CLK_ENABLE		(1 << 8)
> +#define SYSMGR_SDMMCGRP_CTRL_REG	(SOCFPGA_SYSMGR_ADDRESS + 0x108)
Where is SOCFPGA_CLKMGR_ADDRESS defined?

> +#define SYSMGR_SDMMC_CTRL_GET_DRVSEL(x)	(((x) >> 0) & 0x7)
((x) & 0x7) is more readable?
> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)	\
> +	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
> +
> +static char *SOCFPGA_NAME = "SOCFPGA DWMMC";
> +
> +static void socfpga_dwmci_clksel(struct dwmci_host *host)
> +{
> +	unsigned int en;
> +	unsigned int drvsel;
> +	unsigned int smplsel;
> +
> +	/* Disable SDMMC clock. */
> +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> +	en &= ~CLKMGR_SDMMC_CLK_ENABLE;
> +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> +
> +	/* Configures drv_sel and smpl_sel */
> +	drvsel = 3;
> +	smplsel = 0;
Is this value static? then why is value assigned drvsel and smpsel at here?
I didn't know that SOCFPGA is only used with drvsel = 3, smplsel = 0.
But if you need to change this value for other SoC version in future, I think that hard coding is not good.

> +
> +	debug("%s: drvsel %d smplsel %d\n", __FUNCTION__, drvsel, smplsel);
> +	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),
> +		SYSMGR_SDMMCGRP_CTRL_REG);
> +
> +	debug("%s: SYSMGR_SDMMCGRP_CTRL_REG = 0x%x\n", __FUNCTION__,
> +		readl(SYSMGR_SDMMCGRP_CTRL_REG));
> +	/* Enable SDMMC clock */
> +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> +	en |= CLKMGR_SDMMC_CLK_ENABLE;
> +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> +}
> +
> +int socfpga_dwmmc_init(u32 regbase, int bus_width, int index)
> +{
> +	struct dwmci_host *host = NULL;
> +	host = calloc(sizeof(struct dwmci_host), 1);
> +	if (!host) {
> +		printf("dwmci_host calloc fail!\n");
> +		return 1;
> +	}
> +
> +	host->name = SOCFPGA_NAME;
> +	host->ioaddr = (void *)regbase;
> +	host->buswidth = bus_width;
> +	host->clksel = socfpga_dwmci_clksel;
> +	host->dev_index = index;
> +	/* fixed clock divide by 4 which due to the SDMMC wrapper */
> +	host->bus_hz = CONFIG_DWMMC_BUS_HZ;
I didn't want to use the CONFIG_DWMMC_BUS_HZ.

> +	host->fifoth_val = MSIZE(0x2) |
> +		RX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2 - 1) |
> +		TX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2);
> +
> +	add_dwmci(host, host->bus_hz, 400000);
add_dwmci() has the return value.

Best Regards,
Jaehoon Chung
> +
> +	return 0;
> +}
> +
> 

[U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver support for SOCFPGA

[Chin Liang See]

Dear Jaehoon,

On Thu, 2013-12-26 at 14:05 +0900, Jaehoon Chung wrote:
> Hi, Chin.
> 
> On 12/19/2013 02:16 AM, Chin Liang See wrote:
> > 
> > +
> > +#define CLKMGR_PERPLLGRP_EN_REG		(SOCFPGA_CLKMGR_ADDRESS + 0xA0)
> > +#define CLKMGR_SDMMC_CLK_ENABLE		(1 << 8)
> > +#define SYSMGR_SDMMCGRP_CTRL_REG	(SOCFPGA_SYSMGR_ADDRESS + 0x108)
> Where is SOCFPGA_CLKMGR_ADDRESS defined?

This is located at platform specific declaration file. 

> 
> > +#define SYSMGR_SDMMC_CTRL_GET_DRVSEL(x)	(((x) >> 0) & 0x7)
> ((x) & 0x7) is more readable?
> > +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)	\
> > +	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
> > +
> > +static char *SOCFPGA_NAME = "SOCFPGA DWMMC";
> > +
> > +static void socfpga_dwmci_clksel(struct dwmci_host *host)
> > +{
> > +	unsigned int en;
> > +	unsigned int drvsel;
> > +	unsigned int smplsel;
> > +
> > +	/* Disable SDMMC clock. */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en &= ~CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> > +
> > +	/* Configures drv_sel and smpl_sel */
> > +	drvsel = 3;
> > +	smplsel = 0;
> Is this value static? then why is value assigned drvsel and smpsel at here?
> I didn't know that SOCFPGA is only used with drvsel = 3, smplsel = 0.
> But if you need to change this value for other SoC version in future, I think that hard coding is not good.

Good suggestion as I put them as macro now for v3

> 
> > +
> > +	debug("%s: drvsel %d smplsel %d\n", __FUNCTION__, drvsel, smplsel);
> > +	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),
> > +		SYSMGR_SDMMCGRP_CTRL_REG);
> > +
> > +	debug("%s: SYSMGR_SDMMCGRP_CTRL_REG = 0x%x\n", __FUNCTION__,
> > +		readl(SYSMGR_SDMMCGRP_CTRL_REG));
> > +	/* Enable SDMMC clock */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en |= CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> > +}
> > +
> > +int socfpga_dwmmc_init(u32 regbase, int bus_width, int index)
> > +{
> > +	struct dwmci_host *host = NULL;
> > +	host = calloc(sizeof(struct dwmci_host), 1);
> > +	if (!host) {
> > +		printf("dwmci_host calloc fail!\n");
> > +		return 1;
> > +	}
> > +
> > +	host->name = SOCFPGA_NAME;
> > +	host->ioaddr = (void *)regbase;
> > +	host->buswidth = bus_width;
> > +	host->clksel = socfpga_dwmci_clksel;
> > +	host->dev_index = index;
> > +	/* fixed clock divide by 4 which due to the SDMMC wrapper */
> > +	host->bus_hz = CONFIG_DWMMC_BUS_HZ;
> I didn't want to use the CONFIG_DWMMC_BUS_HZ.

Yup, this is SOCFPGA specific and I am using CONFIG_SOCFPGA_DWMMC_BUS_HZ
for v3


Thanks

Chin Liang

> 
> > +	host->fifoth_val = MSIZE(0x2) |
> > +		RX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2 - 1) |
> > +		TX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2);
> > +
> > +	add_dwmci(host, host->bus_hz, 400000);
> add_dwmci() has the return value.
> 
> Best Regards,
> Jaehoon Chung
> > +
> > +	return 0;
> > +}
> > +
> > 
> 
> 

[U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver support for SOCFPGA

[Wolfgang Denk]

Dear Chin Liang See,

PLease fix your address list.  There is no such address as
"Andy at denx.de".

In message <1387386987-3581-1-git-send-email-clsee@altera.com> you wrote:
> To add the DesignWare MMC driver support for Altera SOCFPGA. It
> required information such as clocks and bus width from platform
> specific files (SOCFPGA handoff files)
...
> +#define CLKMGR_PERPLLGRP_EN_REG		(SOCFPGA_CLKMGR_ADDRESS + 0xA0)
...
> +#define SYSMGR_SDMMCGRP_CTRL_REG	(SOCFPGA_SYSMGR_ADDRESS + 0x108)

This looks as if you were trying to access device rtegisters through a
base address plus offset notation?

We do not allow this in U-Boot, as the compiler then has no chance to
check if you are using the correct data types, i. e. it cannot warn
you for example when you access a 32 bit register for a 16 bit data
type.

Please use C structs and proper I/O accessors instead.

> +	/* Disable SDMMC clock. */
> +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> +	en &= ~CLKMGR_SDMMC_CLK_ENABLE;
> +	writel(en, CLKMGR_PERPLLGRP_EN_REG);

Please fix such code.  Use proper I/O accessors.  This could (and
should) be written as:

	clrbits_le32(clkmgr->perpllgrp_en, CLKMGR_SDMMC_CLK_ENABLE);

[or similar struct member name].

> +	/* Enable SDMMC clock */
> +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> +	en |= CLKMGR_SDMMC_CLK_ENABLE;
> +	writel(en, CLKMGR_PERPLLGRP_EN_REG);

Ditto here, etc.

Please check all your device accesses.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
That was the thing about deserts. They had their  own  gravity.  They
sucked you into the centre.           - Terry Pratchett, _Small Gods_

[U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver support for SOCFPGA

[Chin Liang See]

Dear Wolfgang,

On Thu, 2013-12-26 at 09:38 +0100, ZY - wd wrote:
> Dear Chin Liang See,
> 
> PLease fix your address list.  There is no such address as
> "Andy at denx.de".

Oh... I presume you are referring to Andy Fleming. I am getting delivery
failure to afleming at freescale.com and removing it from CC list.

> 
> In message <1387386987-3581-1-git-send-email-clsee@altera.com> you wrote:
> > To add the DesignWare MMC driver support for Altera SOCFPGA. It
> > required information such as clocks and bus width from platform
> > specific files (SOCFPGA handoff files)
> ...
> > +#define CLKMGR_PERPLLGRP_EN_REG		(SOCFPGA_CLKMGR_ADDRESS + 0xA0)
> ...
> > +#define SYSMGR_SDMMCGRP_CTRL_REG	(SOCFPGA_SYSMGR_ADDRESS + 0x108)
> 
> This looks as if you were trying to access device rtegisters through a
> base address plus offset notation?
> 
> We do not allow this in U-Boot, as the compiler then has no chance to
> check if you are using the correct data types, i. e. it cannot warn
> you for example when you access a 32 bit register for a 16 bit data
> type.
> 

Noted and I believe I miss out this. I will be using structure for v3

> Please use C structs and proper I/O accessors instead.
> 
> > +	/* Disable SDMMC clock. */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en &= ~CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> 
> Please fix such code.  Use proper I/O accessors.  This could (and
> should) be written as:
> 
> 	clrbits_le32(clkmgr->perpllgrp_en, CLKMGR_SDMMC_CLK_ENABLE);
> 
> [or similar struct member name].
> 
> > +	/* Enable SDMMC clock */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en |= CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> 
> Ditto here, etc.

Noted.

Thanks

Chin Liang

> 
> Please check all your device accesses.
> 
> Best regards,
> 
> Wolfgang Denk
>