---
Changes for v2
- Put default value for drvsel to 3 in case node in DT missing
- Remove unnecessary ad-hoc vairable
- Free up first calloc if second calloc failed

---
 arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
 drivers/mmc/socfpga_dw_mmc.c       | 22 ++++++++++++++++++++--
 include/configs/socfpga_common.h   |  2 --
 3 files changed, 22 insertions(+), 4 deletions(-)

arch/arm/dts/socfpga_cyclone5.dtsi +2 -0

index de36209..040b236 100644
@@ -25,6 +25,8 @@
 			bus-width = <4>;
 			cap-mmc-highspeed;
 			cap-sd-highspeed;
+			drvsel = <3>;
+			smplsel = <0>;
 		};
 
 		sysmgr at ffd08000 {

drivers/mmc/socfpga_dw_mmc.c +20 -2

index 8076761..38eb783 100644
@@ -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,6 +57,7 @@ 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;

@@ -83,6 +91,13 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx)
 	if (!host)
 		return -ENOMEM;
 
+	/* Allocate the priv */
+	priv = calloc(1, sizeof(*priv));
+	if (!priv) {
+		free(host);
+		return -ENOMEM;
+	}
+
 	host->name = "SOCFPGA DWMMC";
 	host->ioaddr = (void *)reg_base;
 	host->buswidth = bus_width;

@@ -92,6 +107,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 = fdtdec_get_uint(blob, node, "drvsel", 3);
+	priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
+	host->priv = priv;
 
 	return add_dwmci(host, host->bus_hz, 400000);
 }

include/configs/socfpga_common.h +0 -2

index f6808b5..b661cc2 100644
@@ -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? */

Signature · 1 line

2.2.0

Marek Vasut

2015-11-26 01:26 UTC #1 lore raw

PATCH v2 1/1 mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

Quoted 29 lines · Chin Liang See

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>
---
Changes for v2
- Put default value for drvsel to 3 in case node in DT missing
- Remove unnecessary ad-hoc vairable
- Free up first calloc if second calloc failed
---
 arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
 drivers/mmc/socfpga_dw_mmc.c       | 22 ++++++++++++++++++++--
 include/configs/socfpga_common.h   |  2 --
 3 files changed, 22 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/arch/arm/dts/socfpga_cyclone5.dtsi index de36209..040b236 100644

arch/arm/dts/socfpga_cyclone5.dtsi line 32

Quoted 8 lines

arch/arm/dts/socfpga_cyclone5.dtsi +2 -0

@@ -25,6 +25,8 @@
 			bus-width = <4>;
 			cap-mmc-highspeed;
 			cap-sd-highspeed;
+			drvsel = <3>;
+			smplsel = <0>;
 		};
 
 		sysmgr at ffd08000 {

I'm not very fond of bunding DT changes and driver changes together, these should be separate, so it'd be nice if you sent a V3 and split this into two patches.

Otherwise

Acked-by Marek Vasut <[email protected]>

Quoted 19 lines

drivers/mmc/socfpga_dw_mmc.c +7 -0

index 8076761..38eb783 100644
@@ -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;

drivers/mmc/socfpga_dw_mmc.c static const struct socfpga_clock_manager · line 40

Quoted 10 lines

drivers/mmc/socfpga_dw_mmc.c +2 -2

 
 	/* 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;

You can probably just use priv->xxx directly in the code and drop those vars.

Quoted 2 lines

drivers/mmc/socfpga_dw_mmc.c +0 -0

 	debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel);
 	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),

[...]

Chin Liang See

author

2015-11-26 01:30 UTC #2 lore raw

PATCH v2 1/1 mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

Quoted 45 lines · Marek Vasut

Quoted 29 lines · Chin Liang See
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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>
---
Changes for v2
- Put default value for drvsel to 3 in case node in DT missing
- Remove unnecessary ad-hoc vairable
- Free up first calloc if second calloc failed
---
 arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
 drivers/mmc/socfpga_dw_mmc.c       | 22 ++++++++++++++++++++--
 include/configs/socfpga_common.h   |  2 --
 3 files changed, 22 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/arch/arm/dts/socfpga_cyclone5.dtsi index de36209..040b236 100644
arch/arm/dts/socfpga_cyclone5.dtsi line 32
Quoted 8 lines
arch/arm/dts/socfpga_cyclone5.dtsi +2 -0
@@ -25,6 +25,8 @@
 			bus-width = <4>;
 			cap-mmc-highspeed;
 			cap-sd-highspeed;
+			drvsel = <3>;
+			smplsel = <0>;
 		};
 
 		sysmgr at ffd08000 {
I'm not very fond of bunding DT changes and driver changes together, these
should be separate, so it'd be nice if you sent a V3 and split this into
two patches.

Quoted 2 lines

Otherwise
Acked-by Marek Vasut <[email protected]>

Sure, I can split that as its easy to do that.

Quoted 26 lines

drivers/mmc/socfpga_dw_mmc.c +7 -0

index 8076761..38eb783 100644
@@ -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);

Quoted 8 lines

drivers/mmc/socfpga_dw_mmc.c static const struct socfpga_clock_manager *clock_manager_base = · line 40
Quoted 5 lines
drivers/mmc/socfpga_dw_mmc.c +2 -2
 	/* Configures drv_sel and smpl_sel */
-	drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL;
-	smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL;
+	drvsel = priv->drvsel;
+	smplsel = priv->smplsel;
You can probably just use priv->xxx directly in the code and drop those vars.

Sure, I can enhance that too

Thanks
Chin Liang

Quoted 5 lines

Quoted 4 lines

drivers/mmc/socfpga_dw_mmc.c +0 -0

 	debug("%s: drvsel %d smplsel %d\n", __func__, drvsel,
 smplsel);
 	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),

[...]

Jaehoon Chung

2015-11-26 01:33 UTC #3 lore raw

PATCH v2 1/1 mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

In reply to Chin Liang See

Quoted 56 lines · Chin Liang See

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>
---
Changes for v2
- Put default value for drvsel to 3 in case node in DT missing
- Remove unnecessary ad-hoc vairable
- Free up first calloc if second calloc failed
---
 arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
 drivers/mmc/socfpga_dw_mmc.c       | 22 ++++++++++++++++++++--
 include/configs/socfpga_common.h   |  2 --
 3 files changed, 22 insertions(+), 4 deletions(-)
arch/arm/dts/socfpga_cyclone5.dtsi +2 -0
index de36209..040b236 100644
@@ -25,6 +25,8 @@
 			bus-width = <4>;
 			cap-mmc-highspeed;
 			cap-sd-highspeed;
+			drvsel = <3>;
+			smplsel = <0>;
 		};
 
 		sysmgr at ffd08000 {
drivers/mmc/socfpga_dw_mmc.c +7 -0
index 8076761..38eb783 100644
@@ -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;

drivers/mmc/socfpga_dw_mmc.c static const struct socfpga_clock_manager *clock_manager_base = · line 40

Quoted 10 lines

drivers/mmc/socfpga_dw_mmc.c +2 -2

 
 	/* 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;

I found that drvsel and smplsel are used at only here. I think they should be removed. (priv->drvsel/smplse don't need to assign to drvsel/smplsel.)

how about?

Best Regards,
Jaehoon Chung

Quoted 49 lines

drivers/mmc/socfpga_dw_mmc.c +11 -0

 	debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel);
 	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),

@@ -50,6 +57,7 @@ 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;

@@ -83,6 +91,13 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx)
 	if (!host)
 		return -ENOMEM;
 
+	/* Allocate the priv */
+	priv = calloc(1, sizeof(*priv));
+	if (!priv) {
+		free(host);
+		return -ENOMEM;
+	}
+
 	host->name = "SOCFPGA DWMMC";
 	host->ioaddr = (void *)reg_base;
 	host->buswidth = bus_width;

@@ -92,6 +107,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 = fdtdec_get_uint(blob, node, "drvsel", 3);
+	priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
+	host->priv = priv;
 
 	return add_dwmci(host, host->bus_hz, 400000);
 }

include/configs/socfpga_common.h +0 -2

index f6808b5..b661cc2 100644
@@ -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? */

Chin Liang See

author

2015-11-26 01:41 UTC #4 lore raw

PATCH v2 1/1 mmc: socfpga_dw_mmc: Move drvsel and smplsel to dts

Quoted 75 lines · Jaehoon Chung

Quoted 59 lines · Chin Liang See
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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>
---
Changes for v2
- Put default value for drvsel to 3 in case node in DT missing
- Remove unnecessary ad-hoc vairable
- Free up first calloc if second calloc failed
---
 arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
 drivers/mmc/socfpga_dw_mmc.c       | 22 ++++++++++++++++++++--
 include/configs/socfpga_common.h   |  2 --
 3 files changed, 22 insertions(+), 4 deletions(-)
arch/arm/dts/socfpga_cyclone5.dtsi +2 -0
index de36209..040b236 100644
@@ -25,6 +25,8 @@
 			bus-width = <4>;
 			cap-mmc-highspeed;
 			cap-sd-highspeed;
+			drvsel = <3>;
+			smplsel = <0>;
 		};
 
 		sysmgr at ffd08000 {
drivers/mmc/socfpga_dw_mmc.c +7 -0
index 8076761..38eb783 100644
@@ -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;
drivers/mmc/socfpga_dw_mmc.c static const struct socfpga_clock_manager · line 41
Quoted 10 lines
drivers/mmc/socfpga_dw_mmc.c +2 -2
 
 	/* 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;
I found that drvsel and smplsel are used at only here. I think they should be removed. (priv->drvsel/smplse don't need to assign to drvsel/smplsel.)

Quoted 2 lines

how about?

Yup, Marek just note this to me too.
I will get rid the variable and use the priv directly

Thanks
Chin Liang

Quoted 61 lines

Best Regards,
Jaehoon Chung

Quoted 57 lines

drivers/mmc/socfpga_dw_mmc.c +0 -0

 	debug("%s: drvsel %d smplsel %d\n", __func__, drvsel,
 smplsel);

drivers/mmc/socfpga_dw_mmc.c +8 -0

 	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),

@@ -50,6 +57,7 @@ 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;

@@ -83,6 +91,13 @@ static int socfpga_dwmci_of_probe(const void
 *blob, int node, const int idx)
 	if (!host)
 		return -ENOMEM;
 
+	/* Allocate the priv */
+	priv = calloc(1, sizeof(*priv));
+	if (!priv) {
+		free(host);
+		return -ENOMEM;
+	}
+
 	host->name = "SOCFPGA DWMMC";
 	host->ioaddr = (void *)reg_base;

drivers/mmc/socfpga_dw_mmc.c +3 -0

 	host->buswidth = bus_width;

@@ -92,6 +107,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 = fdtdec_get_uint(blob, node, "drvsel", 3);
+	priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
+	host->priv = priv;

drivers/mmc/socfpga_dw_mmc.c +0 -0

 	return add_dwmci(host, host->bus_hz, 400000);
 }

include/configs/socfpga_common.h +0 -2

index f6808b5..b661cc2 100644
@@ -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? */