| Message ID | 1387386987-3581-1-git-send-email-clsee@altera.com |
|---|---|
| State | Superseded |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
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; > +} > + >
Dear Chin Liang See, PLease fix your address list. There is no such address as "Andy@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
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; > > +} > > + > > > >
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@denx.de". Oh... I presume you are referring to Andy Fleming. I am getting delivery failure to afleming@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 >
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; +} +
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