[U-Boot] [PATCH] mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

[U-Boot] [PATCH] mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

[clsee]

From: Chin Liang See <clsee@altera.com>

socfpga_dw_mmc driver will obtain the drvsel and
smplsel value from device tree instead of definition
in config header file.

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: Pavel Machek <pavel@denx.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
---
 arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
 drivers/mmc/socfpga_dw_mmc.c       | 24 ++++++++++++++++++++++--
 include/configs/socfpga_common.h   |  2 --
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi b/arch/arm/dts/socfpga_cyclone5.dtsi
index de36209..040b236 100644
--- a/arch/arm/dts/socfpga_cyclone5.dtsi
+++ b/arch/arm/dts/socfpga_cyclone5.dtsi
@@ -25,6 +25,8 @@
 			bus-width = <4>;
 			cap-mmc-highspeed;
 			cap-sd-highspeed;
+			drvsel = <3>;
+			smplsel = <0>;
 		};
 
 		sysmgr at ffd08000 {
diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c
index 8076761..2cd7a51 100644
--- a/drivers/mmc/socfpga_dw_mmc.c
+++ b/drivers/mmc/socfpga_dw_mmc.c
@@ -19,18 +19,25 @@ static const struct socfpga_clock_manager *clock_manager_base =
 static const struct socfpga_system_manager *system_manager_base =
 		(void *)SOCFPGA_SYSMGR_ADDRESS;
 
+/* socfpga implmentation specific drver private data */
+struct dwmci_socfpga_priv_data {
+	unsigned int drvsel;
+	unsigned int smplsel;
+};
+
 static void socfpga_dwmci_clksel(struct dwmci_host *host)
 {
 	unsigned int drvsel;
 	unsigned int smplsel;
+	struct dwmci_socfpga_priv_data *priv = host->priv;
 
 	/* Disable SDMMC clock. */
 	clrbits_le32(&clock_manager_base->per_pll.en,
 		CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK);
 
 	/* Configures drv_sel and smpl_sel */
-	drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL;
-	smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL;
+	drvsel = priv->drvsel;
+	smplsel = priv->smplsel;
 
 	debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel);
 	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),
@@ -50,8 +57,10 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx)
 	const unsigned long clk = cm_get_mmc_controller_clk_hz();
 
 	struct dwmci_host *host;
+	struct dwmci_socfpga_priv_data *priv;
 	fdt_addr_t reg_base;
 	int bus_width, fifo_depth;
+	unsigned int drvsel, smplsel;
 
 	if (clk == 0) {
 		printf("DWMMC%d: MMC clock is zero!", idx);
@@ -78,11 +87,19 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx)
 		return -EINVAL;
 	}
 
+	drvsel = fdtdec_get_uint(blob, node, "drvsel", 0);
+	smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
+
 	/* Allocate the host */
 	host = calloc(1, sizeof(*host));
 	if (!host)
 		return -ENOMEM;
 
+	/* Allocate the priv */
+	priv = calloc(1, sizeof(*priv));
+	if (!priv)
+		return -ENOMEM;
+
 	host->name = "SOCFPGA DWMMC";
 	host->ioaddr = (void *)reg_base;
 	host->buswidth = bus_width;
@@ -92,6 +109,9 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx)
 	host->bus_hz = clk;
 	host->fifoth_val = MSIZE(0x2) |
 		RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth / 2);
+	priv->drvsel = drvsel;
+	priv->smplsel = smplsel;
+	host->priv = priv;
 
 	return add_dwmci(host, host->bus_hz, 400000);
 }
diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
index f6808b5..b661cc2 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -153,8 +153,6 @@
 #define CONFIG_DWMMC
 #define CONFIG_SOCFPGA_DWMMC
 #define CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH	1024
-#define CONFIG_SOCFPGA_DWMMC_DRVSEL	3
-#define CONFIG_SOCFPGA_DWMMC_SMPSEL	0
 /* FIXME */
 /* using smaller max blk cnt to avoid flooding the limited stack we have */
 #define CONFIG_SYS_MMC_MAX_BLK_COUNT	256	/* FIXME -- SPL only? */
-- 
2.2.0

[U-Boot] [PATCH] mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

[Marek Vasut]

On Thursday, November 26, 2015 at 01:41:12 AM, clsee wrote:
> From: Chin Liang See <clsee@altera.com>
> 
> socfpga_dw_mmc driver will obtain the drvsel and
> smplsel value from device tree instead of definition
> in config header file.
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> ---
>  arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
>  drivers/mmc/socfpga_dw_mmc.c       | 24 ++++++++++++++++++++++--
>  include/configs/socfpga_common.h   |  2 --
>  3 files changed, 24 insertions(+), 4 deletions(-)

Hi!

[...]

> @@ -78,11 +87,19 @@ static int socfpga_dwmci_of_probe(const void *blob, int
> node, const int idx) return -EINVAL;
>  	}
> 
> +	drvsel = fdtdec_get_uint(blob, node, "drvsel", 0);

The default value here should be 3, otherwise this won't preserve the original
behavior of the driver in case the nodes in DT are missing.

> +	smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
> +
>  	/* Allocate the host */
>  	host = calloc(1, sizeof(*host));
>  	if (!host)
>  		return -ENOMEM;
> 
> +	/* Allocate the priv */
> +	priv = calloc(1, sizeof(*priv));
> +	if (!priv)
> +		return -ENOMEM;

If this call fails, you're leaking memory, since you calloc() some stuff before.

> +
>  	host->name = "SOCFPGA DWMMC";
>  	host->ioaddr = (void *)reg_base;
>  	host->buswidth = bus_width;
> @@ -92,6 +109,9 @@ static int socfpga_dwmci_of_probe(const void *blob, int
> node, const int idx) host->bus_hz = clk;
>  	host->fifoth_val = MSIZE(0x2) |
>  		RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth / 2);
> +	priv->drvsel = drvsel;
> +	priv->smplsel = smplsel;
> +	host->priv = priv;

You can move the fdtdec_get_uint() calls here directly, no need to introduce
ad-hoc variable and then just assign it into the private data.

>  	return add_dwmci(host, host->bus_hz, 400000);
>  }
> diff --git a/include/configs/socfpga_common.h
> b/include/configs/socfpga_common.h index f6808b5..b661cc2 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -153,8 +153,6 @@
>  #define CONFIG_DWMMC
>  #define CONFIG_SOCFPGA_DWMMC
>  #define CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH	1024
> -#define CONFIG_SOCFPGA_DWMMC_DRVSEL	3
> -#define CONFIG_SOCFPGA_DWMMC_SMPSEL	0
>  /* FIXME */
>  /* using smaller max blk cnt to avoid flooding the limited stack we have
> */ #define CONFIG_SYS_MMC_MAX_BLK_COUNT	256	/* FIXME -- SPL only? */

[U-Boot] [PATCH] mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

[Chin Liang See]

Hi Marek,

On Thu, 2015-11-26 at 01:50 +0100, Marek Vasut wrote:
> On Thursday, November 26, 2015 at 01:41:12 AM, clsee wrote:
> > From: Chin Liang See <clsee@altera.com>
> > 
> > socfpga_dw_mmc driver will obtain the drvsel and
> > smplsel value from device tree instead of definition
> > in config header file.
> > 
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Jaehoon Chung <jh80.chung@samsung.com>
> > ---
> >  arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
> >  drivers/mmc/socfpga_dw_mmc.c       | 24 ++++++++++++++++++++++--
> >  include/configs/socfpga_common.h   |  2 --
> >  3 files changed, 24 insertions(+), 4 deletions(-)
> 
> Hi!
> 
> [...]
> 
> > @@ -78,11 +87,19 @@ static int socfpga_dwmci_of_probe(const void
> > *blob, int
> > node, const int idx) return -EINVAL;
> >  	}
> > 
> > +	drvsel = fdtdec_get_uint(blob, node, "drvsel", 0);
> 
> The default value here should be 3, otherwise this won't preserve the
> original
> behavior of the driver in case the nodes in DT are missing.

Nice thinking, will update this.

> 
> > +	smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
> > +
> >  	/* Allocate the host */
> >  	host = calloc(1, sizeof(*host));
> >  	if (!host)
> >  		return -ENOMEM;
> > 
> > +	/* Allocate the priv */
> > +	priv = calloc(1, sizeof(*priv));
> > +	if (!priv)
> > +		return -ENOMEM;
> 
> If this call fails, you're leaking memory, since you calloc() some
> stuff before.
> 

Oops, yup, this need to be fixed

> > +
> >  	host->name = "SOCFPGA DWMMC";
> >  	host->ioaddr = (void *)reg_base;
> >  	host->buswidth = bus_width;
> > @@ -92,6 +109,9 @@ static int socfpga_dwmci_of_probe(const void
> > *blob, int
> > node, const int idx) host->bus_hz = clk;
> >  	host->fifoth_val = MSIZE(0x2) |
> >  		RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth
> > / 2);
> > +	priv->drvsel = drvsel;
> > +	priv->smplsel = smplsel;
> > +	host->priv = priv;
> 
> You can move the fdtdec_get_uint() calls here directly, no need to
> introduce
> ad-hoc variable and then just assign it into the private data.

Let me enhance this too

Thanks
Chin Liang

> 
> >  	return add_dwmci(host, host->bus_hz, 400000);
> >  }
> > diff --git a/include/configs/socfpga_common.h
> > b/include/configs/socfpga_common.h index f6808b5..b661cc2 100644
> > --- a/include/configs/socfpga_common.h
> > +++ b/include/configs/socfpga_common.h
> > @@ -153,8 +153,6 @@
> >  #define CONFIG_DWMMC
> >  #define CONFIG_SOCFPGA_DWMMC
> >  #define CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH	1024
> > -#define CONFIG_SOCFPGA_DWMMC_DRVSEL	3
> > -#define CONFIG_SOCFPGA_DWMMC_SMPSEL	0
> >  /* FIXME */
> >  /* using smaller max blk cnt to avoid flooding the limited stack
> > we have
> > */ #define CONFIG_SYS_MMC_MAX_BLK_COUNT	256	/* FIXME 
> > -- SPL only? */