mmc: Board-specific MMC power initializations

PATCH Superseded Paul Kocialkowski 2 participants 9 messages Nov 1, 2014 – Nov 3, 2014 MMC, SD and SDIO

lore

Share & tools

lore

Paul Kocialkowski

author

2014-11-01 10:52 UTC #0 lore raw

v3 cover mmc: Board-specific MMC power initializations

Changelog:

v3: Changes to make v2 apply on master

v2: Implement board_mmc_power_init instead of define

Paul Kocialkowski

author

2014-11-01 10:52 UTC #1 lore raw

PATCH v3 1/2 mmc: Board-specific MMC power initializations

Some devices may use non-standard combinations of regulators to power MMC:
this allows these devices to provide a board-specific MMC power init function to set everything up in their own way.

Signed-off-by Paul Kocialkowski <[email protected]>

---
 drivers/mmc/mmc.c | 8 ++++++++
 include/mmc.h     | 1 +
 2 files changed, 9 insertions(+)

drivers/mmc/mmc.c +8 -0

index 44a4feb..125f347 100644
@@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev)
 }
 #endif
 
+/* board-specific MMC power initializations. */
+__weak int board_mmc_power_init(void)
+{
+	return -1;
+}
+
 int mmc_start_init(struct mmc *mmc)
 {
 	int err;

@@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
 	if (mmc->has_init)
 		return 0;
 
+	board_mmc_power_init();
+
 	/* made sure it's not NULL earlier */
 	err = mmc->cfg->ops->init(mmc);

include/mmc.h +1 -0

index d74a190..aaea644 100644
@@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
 int mmc_legacy_init(int verbose);
 #endif
 
+int board_mmc_power_init(void);
 int board_mmc_init(bd_t *bis);
 int cpu_mmc_init(bd_t *bis);
 int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);

Signature · 1 line

1.9.1

Igor Grinberg

2014-11-02 13:23 UTC #2 lore raw

PATCH v3 1/2 mmc: Board-specific MMC power initializations

Hi Paul,

Quoted 28 lines · Paul Kocialkowski

Some devices may use non-standard combinations of regulators to power MMC:
this allows these devices to provide a board-specific MMC power init function to set everything up in their own way.
Signed-off-by Paul Kocialkowski <[email protected]>
---
 drivers/mmc/mmc.c | 8 ++++++++
 include/mmc.h     | 1 +
 2 files changed, 9 insertions(+)
drivers/mmc/mmc.c +6 -0
index 44a4feb..125f347 100644
@@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev)
 }
 #endif
 
+/* board-specific MMC power initializations. */
+__weak int board_mmc_power_init(void)
+{
+	return -1;
+}
+
 int mmc_start_init(struct mmc *mmc)
 {
 	int err;

drivers/mmc/mmc.c int mmc_start_init(struct mmc *mmc) · line 1303

Quoted 5 lines

drivers/mmc/mmc.c +2 -0

@@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
 	if (mmc->has_init)
 		return 0;
 
+	board_mmc_power_init();
+

Shouldn't we have some error handling here?

Quoted 16 lines

drivers/mmc/mmc.c +0 -0

 	/* made sure it's not NULL earlier */
 	err = mmc->cfg->ops->init(mmc);

include/mmc.h +1 -0

index d74a190..aaea644 100644
@@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
 int mmc_legacy_init(int verbose);
 #endif
 
+int board_mmc_power_init(void);
 int board_mmc_init(bd_t *bis);
 int cpu_mmc_init(bd_t *bis);
 int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);

Signature · 2 lines

Regards,
Igor.

Paul Kocialkowski

author

2014-11-02 18:51 UTC #3 lore raw

PATCH v3 1/2 mmc: Board-specific MMC power initializations

Hi Igor,

Quoted 32 lines · Paul Kocialkowski

Quoted 31 lines
Some devices may use non-standard combinations of regulators to power MMC:
this allows these devices to provide a board-specific MMC power init function to set everything up in their own way.
Signed-off-by Paul Kocialkowski <[email protected]>
---
 drivers/mmc/mmc.c | 8 ++++++++
 include/mmc.h     | 1 +
 2 files changed, 9 insertions(+)
drivers/mmc/mmc.c +6 -0
index 44a4feb..125f347 100644
@@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev)
 }
 #endif
 
+/* board-specific MMC power initializations. */
+__weak int board_mmc_power_init(void)
+{
+	return -1;
+}
+
 int mmc_start_init(struct mmc *mmc)
 {
 	int err;
@@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
 	if (mmc->has_init)
 		return 0;
 

Quoted 4 lines

drivers/mmc/mmc.c int mmc_start_init(struct mmc *mmc) · line 1303
Quoted 2 lines
drivers/mmc/mmc.c +2 -0
+	board_mmc_power_init();
+
Shouldn't we have some error handling here?

I noticed how weak implementations tend to return -1 and not have their return code checked so much (see the other two such functions in mmc.c). I would be fine with checking the return code and returning 0 in the weak implementation.

If you think that's better, I'll make a new version with that.

Quoted 11 lines

drivers/mmc/mmc.c +0 -0

 	/* made sure it's not NULL earlier */
 	err = mmc->cfg->ops->init(mmc);

include/mmc.h +0 -0

index d74a190..aaea644 100644
@@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
 int mmc_legacy_init(int verbose);
 #endif

Quoted 6 lines

include/mmc.h struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode); · line 391
Quoted 5 lines
include/mmc.h +1 -0
+int board_mmc_power_init(void);
 int board_mmc_init(bd_t *bis);
 int cpu_mmc_init(bd_t *bis);
 int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141102/3aba81b0/attachment.pgp>

Igor Grinberg

2014-11-03 07:35 UTC #4 lore raw

PATCH v3 1/2 mmc: Board-specific MMC power initializations

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Paul,

Quoted 39 lines · Paul Kocialkowski

Hi Igor,
Quoted 32 lines · Paul Kocialkowski
Quoted 31 lines
Some devices may use non-standard combinations of regulators to power MMC:
this allows these devices to provide a board-specific MMC power init function to set everything up in their own way.
Signed-off-by Paul Kocialkowski <[email protected]>
---
 drivers/mmc/mmc.c | 8 ++++++++
 include/mmc.h     | 1 +
 2 files changed, 9 insertions(+)
drivers/mmc/mmc.c +6 -0
index 44a4feb..125f347 100644
@@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev)
 }
 #endif
 
+/* board-specific MMC power initializations. */
+__weak int board_mmc_power_init(void)
+{
+	return -1;
+}
+
 int mmc_start_init(struct mmc *mmc)
 {
 	int err;
@@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
 	if (mmc->has_init)
 		return 0;
 
Quoted 4 lines
drivers/mmc/mmc.c int mmc_start_init(struct mmc *mmc) · line 1303
Quoted 2 lines
drivers/mmc/mmc.c +2 -0
+	board_mmc_power_init();
+
Shouldn't we have some error handling here?

Quoted 6 lines

I noticed how weak implementations tend to return -1 and not have their return code checked so much (see the other two such functions in mmc.c). I would be fine with checking the return code and returning 0 in the weak implementation.
If you think that's better, I'll make a new version with that.

Well, otherwise it does not make sense to return int, does it? IMO it is a good practice to check the return value, and may be emit a warning or something (I'm not really sure if it should abort the init sequence).
Since I can't propose a good handling now, may be we should leave it for now and see if someone comes up with a good case for it. I'm fine with your decision.

Quoted 12 lines

drivers/mmc/mmc.c +0 -0

 	/* made sure it's not NULL earlier */
 	err = mmc->cfg->ops->init(mmc);

include/mmc.h +0 -0

index d74a190..aaea644 100644
@@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
 int mmc_legacy_init(int verbose);
 #endif

Quoted 7 lines

Quoted 6 lines
include/mmc.h struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode); · line 391
Quoted 5 lines
include/mmc.h +1 -0
+int board_mmc_power_init(void);
 int board_mmc_init(bd_t *bis);
 int cpu_mmc_init(bd_t *bis);
 int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);

- --
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUVzBHAAoJEBDE8YO64EfayhsP/jV2YAR6jH6gZJBNn/wGnHId a8U0YGG+f30RBPCdfCiraSRpL1C876wsf+InNDAHgo4HSaRdRwF4tUqWj78SpH9F QTo3qcA6S8S4GIRJ2+q+3BmtlEuYPWOZOi41erhaUF3kMLw6cMt+gL0Ggx4OXQAo 2bhSeQaaGynoFJwC0dIf0QhDjRkPCHbNh2IgROfRye0a7/0pF4tN/tvjLSxw8w54 3457N4TYreGEmMMrtNi/psekMYTjR6JFve3CwiMvOXiwOJGz/d6qj7vhw4Ba7q5i LE385Zj24Bi/+m7ls7hp5I522O0tkShGgPI3XvCiT4xrjXDBihsWG9RGsLSc5vBY EimiztfLVu3alc1cxclK9tq0p+FwyUNwXsUED+oR6t56/qr/BSWiSIDAMfN1zcfI paK9zXDoGvhg6T0pK+SsRvAE4D7vg+XT8WSU52l6/h8TRKB6r3cupSMXAyjmUpLl 1pEozVsg4qW6rJawQOiCjl0YU3ixeVwk0vmWWh+gBOMMLsuikQpFI5s6zUXteqnl IHFS6oiYW/SVppHrd95624suZQPTsrH6+4CvmBwmtBGbmErchsgfj8qjP/ilpn1a Y6Kv0mj2ndkgdFAc7zIEpwzF9yPsewVg8nc+vYLezyT7ePeTNYLpPWgQTL6qEdHH lcK7qYNEaYWIUMkbQM5P
=7kRq
-----END PGP SIGNATURE-----

Paul Kocialkowski

author

2014-11-01 10:52 UTC #5 lore raw

PATCH v3 2/2 omap_hsmmc: Board-specific TWL4030 MMC power initializations

In reply to Paul Kocialkowski

Boards using the TWL4030 regulator may not all use the LDOs the same way (e.g. MMC2 power can be controlled by another LDO than VMMC2). This delegates TWL4030 MMC power initializations to board-specific functions, that may still call twl4030_power_mmc_init for the default behavior.

Signed-off-by Paul Kocialkowski <[email protected]>

---
 board/comelit/dig297/dig297.c          |  9 +++++++++
 board/compulab/cm_t35/cm_t35.c         | 11 +++++++++++
 board/corscience/tricorder/tricorder.c | 11 +++++++++++
 board/isee/igep00x0/igep00x0.c         | 11 +++++++++++
 board/logicpd/omap3som/omap3logic.c    | 11 +++++++++++
 board/logicpd/zoom1/zoom1.c            |  9 +++++++++
 board/matrix_vision/mvblx/mvblx.c      |  9 +++++++++
 board/nokia/rx51/rx51.c                |  9 +++++++++
 board/overo/overo.c                    | 11 +++++++++++
 board/pandora/pandora.c                |  9 +++++++++
 board/technexion/tao3530/tao3530.c     | 11 +++++++++++
 board/ti/beagle/beagle.c               | 11 +++++++++++
 board/ti/evm/evm.c                     | 11 +++++++++++
 board/ti/sdp3430/sdp.c                 |  9 +++++++++
 board/timll/devkit8000/devkit8000.c    | 11 +++++++++++
 drivers/mmc/omap_hsmmc.c               |  7 +------
 16 files changed, 154 insertions(+), 6 deletions(-)

board/comelit/dig297/dig297.c +9 -0

index 2b826df..784483b 100644
@@ -133,6 +133,15 @@ int board_mmc_init(bd_t *bis)
 {
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
+
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_CMD_NET

board/compulab/cm_t35/cm_t35.c +11 -0

index d0b0930..2e59618 100644
@@ -462,6 +462,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif
+
 /*
  * Routine: setup_net_chip_gmpc
  * Description: Setting up the configuration GPMC registers specific to the

board/corscience/tricorder/tricorder.c +11 -0

index 9e81bf3..9ade210 100644
@@ -147,6 +147,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif
+
 /*
  * Routine: get_board_mem_timings
  * Description: If we use SPL then there is no x-loader nor config header

board/isee/igep00x0/igep00x0.c +11 -0

index 7b87cc2..70e7ca8 100644
@@ -150,6 +150,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif
+
 void set_fdt(void)
 {
 	switch (gd->bd->bi_arch_number) {

board/logicpd/omap3som/omap3logic.c +11 -0

index 1fd9f2c..1566a3b 100644
@@ -128,6 +128,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif
+
 #ifdef CONFIG_SMC911X
 /* GPMC CS1 settings for Logic SOM LV/Torpedo LAN92xx Ethernet chip */
 static const u32 gpmc_lan92xx_config[] = {

board/logicpd/zoom1/zoom1.c +9 -0

index 9ef0026..2556290 100644
@@ -109,6 +109,15 @@ int board_mmc_init(bd_t *bis)
 {
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
+
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_CMD_NET

board/matrix_vision/mvblx/mvblx.c +9 -0

index a69359f..1f4ca47 100644
@@ -94,6 +94,15 @@ int board_mmc_init(bd_t *bis)
 	omap_mmc_init(1, 0, 0, -1, -1);
 	return 0;
 }
+
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
 #endif
 
 #if defined(CONFIG_CMD_NET)

board/nokia/rx51/rx51.c +9 -0

index c2e07db..e313fc2 100644
@@ -659,3 +659,12 @@ int board_mmc_init(bd_t *bis)
 	omap_mmc_init(1, 0, 0, -1, -1);
 	return 0;
 }
+
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}

board/overo/overo.c +11 -0

index dfb8602..64ecc28 100644
@@ -493,6 +493,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+       twl4030_power_mmc_init();
+       mdelay(100);    /* ramp-up delay from Linux code */
+#endif
+       return 0;
+}
+#endif
+
 #if defined(CONFIG_USB_EHCI) &&  !defined(CONFIG_SPL_BUILD)
 static struct omap_usbhs_board_data usbhs_bdata = {
 	.port_mode[0] = OMAP_USBHS_PORT_MODE_UNUSED,

board/pandora/pandora.c +9 -0

index 146dcea..8fa0119 100644
@@ -126,4 +126,13 @@ int board_mmc_init(bd_t *bis)
 {
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
+
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
 #endif

board/technexion/tao3530/tao3530.c +11 -0

index 44a8240..64f2313 100644
@@ -188,6 +188,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif
+
 #if defined(CONFIG_USB_EHCI) && !defined(CONFIG_SPL_BUILD)
 /* Call usb_stop() before starting the kernel */
 void show_boot_progress(int val)

board/ti/beagle/beagle.c +11 -0

index 4c5e381..a5b7b3e 100644
@@ -534,6 +534,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif
+
 #if defined(CONFIG_USB_EHCI) && !defined(CONFIG_SPL_BUILD)
 /* Call usb_stop() before starting the kernel */
 void show_boot_progress(int val)

board/ti/evm/evm.c +11 -0

index 81dd081..c41a760 100644
@@ -264,3 +264,14 @@ int board_mmc_init(bd_t *bis)
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
 #endif
+
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif

board/ti/sdp3430/sdp.c +9 -0

index 957940d..44e0418 100644
@@ -195,4 +195,13 @@ int board_mmc_init(bd_t *bis)
 {
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
+
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
 #endif

board/timll/devkit8000/devkit8000.c +11 -0

index bcbee73..ef7fd0c 100644
@@ -124,6 +124,17 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_MMC)
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
+#endif
+	return 0;
+}
+#endif
+
 #if defined(CONFIG_DRIVER_DM9000) & !defined(CONFIG_SPL_BUILD)
 /*
  * Routine: board_eth_init

drivers/mmc/omap_hsmmc.c +1 -6

index ef2cbf9..6fb78b3 100644
@@ -135,12 +135,7 @@ static unsigned char mmc_board_init(struct mmc *mmc)
 	pbias_lite = readl(&t2_base->pbias_lite);
 	pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0);
 	writel(pbias_lite, &t2_base->pbias_lite);
-#endif
-#if defined(CONFIG_TWL4030_POWER)
-	twl4030_power_mmc_init();
-	mdelay(100);	/* ramp-up delay from Linux code */
-#endif
-#if defined(CONFIG_OMAP34XX)
+
 	writel(pbias_lite | PBIASLITEPWRDNZ1 |
 		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
 		&t2_base->pbias_lite);

Signature · 1 line

1.9.1

Igor Grinberg

2014-11-02 13:40 UTC #6 lore raw

PATCH v3 2/2 omap_hsmmc: Board-specific TWL4030 MMC power initializations

Hi Paul,

Quoted 6 lines · Paul Kocialkowski

Boards using the TWL4030 regulator may not all use the LDOs the same way (e.g. MMC2 power can be controlled by another LDO than VMMC2). This delegates TWL4030 MMC power initializations to board-specific functions, that may still call twl4030_power_mmc_init for the default behavior.
Signed-off-by Paul Kocialkowski <[email protected]>

This mostly looks good, several suggestions below though.

Quoted 24 lines

---
 board/comelit/dig297/dig297.c          |  9 +++++++++
 board/compulab/cm_t35/cm_t35.c         | 11 +++++++++++
 board/corscience/tricorder/tricorder.c | 11 +++++++++++
 board/isee/igep00x0/igep00x0.c         | 11 +++++++++++
 board/logicpd/omap3som/omap3logic.c    | 11 +++++++++++
 board/logicpd/zoom1/zoom1.c            |  9 +++++++++
 board/matrix_vision/mvblx/mvblx.c      |  9 +++++++++
 board/nokia/rx51/rx51.c                |  9 +++++++++
 board/overo/overo.c                    | 11 +++++++++++
 board/pandora/pandora.c                |  9 +++++++++
 board/technexion/tao3530/tao3530.c     | 11 +++++++++++
 board/ti/beagle/beagle.c               | 11 +++++++++++
 board/ti/evm/evm.c                     | 11 +++++++++++
 board/ti/sdp3430/sdp.c                 |  9 +++++++++
 board/timll/devkit8000/devkit8000.c    | 11 +++++++++++
 drivers/mmc/omap_hsmmc.c               |  7 +------
 16 files changed, 154 insertions(+), 6 deletions(-)

board/comelit/dig297/dig297.c +0 -0

index 2b826df..784483b 100644

board/comelit/dig297/dig297.c int board_mmc_init(bd_t *bis) · line 141

Quoted 9 lines

board/comelit/dig297/dig297.c +6 -0

@@ -133,6 +133,15 @@ int board_mmc_init(bd_t *bis)
 {
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
+
+int board_mmc_power_init(void)
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */

I guess all twl4030 based boards have to have this ramp up delay, right? If so, why don't we want to hide it inside the twl4030_power_mmc_init() function and not spread it across all boards?

board/comelit/dig297/dig297.c int board_mmc_init(bd_t *bis) · line 144

Quoted 3 lines

board/comelit/dig297/dig297.c +3 -0
+#endif
+	return 0;
+}

Also, what do you think of the below suggestion:
Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c (as it seems that many boards want it) as it currently is. This way you will not need to patch each board file, yet a board has the ability to control that call via the CONFIG_TWL4030_POWER and have a board specific callback should it need one.

I would also change envelope the call to twl4030_power_mmc_init() to something like CONFIG_TWL4030_MMC_POWER instead of CONFIG_TWL4030_POWER, so it will be more flexible, but that has little to do with this patch.

Quoted 3 lines

board/comelit/dig297/dig297.c +0 -0
 #endif
 
 #ifdef CONFIG_CMD_NET

[...]

Quoted 19 lines

drivers/mmc/omap_hsmmc.c +1 -6

index ef2cbf9..6fb78b3 100644
@@ -135,12 +135,7 @@ static unsigned char mmc_board_init(struct mmc *mmc)
 	pbias_lite = readl(&t2_base->pbias_lite);
 	pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0);
 	writel(pbias_lite, &t2_base->pbias_lite);
-#endif
-#if defined(CONFIG_TWL4030_POWER)
-	twl4030_power_mmc_init();
-	mdelay(100);	/* ramp-up delay from Linux code */
-#endif
-#if defined(CONFIG_OMAP34XX)
+
 	writel(pbias_lite | PBIASLITEPWRDNZ1 |
 		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
 		&t2_base->pbias_lite);

Signature · 2 lines

Regards,
Igor.

Paul Kocialkowski

author

2014-11-02 19:01 UTC #7 lore raw

PATCH v3 2/2 omap_hsmmc: Board-specific TWL4030 MMC power initializations

Hi Igor, thanks for the review.

Quoted 39 lines · Paul Kocialkowski

Quoted 6 lines

Boards using the TWL4030 regulator may not all use the LDOs the same way (e.g. MMC2 power can be controlled by another LDO than VMMC2). This delegates TWL4030 MMC power initializations to board-specific functions, that may still call twl4030_power_mmc_init for the default behavior.
Signed-off-by Paul Kocialkowski <[email protected]>

This mostly looks good, several suggestions below though.

Quoted 29 lines

---
 board/comelit/dig297/dig297.c          |  9 +++++++++
 board/compulab/cm_t35/cm_t35.c         | 11 +++++++++++
 board/corscience/tricorder/tricorder.c | 11 +++++++++++
 board/isee/igep00x0/igep00x0.c         | 11 +++++++++++
 board/logicpd/omap3som/omap3logic.c    | 11 +++++++++++
 board/logicpd/zoom1/zoom1.c            |  9 +++++++++
 board/matrix_vision/mvblx/mvblx.c      |  9 +++++++++
 board/nokia/rx51/rx51.c                |  9 +++++++++
 board/overo/overo.c                    | 11 +++++++++++
 board/pandora/pandora.c                |  9 +++++++++
 board/technexion/tao3530/tao3530.c     | 11 +++++++++++
 board/ti/beagle/beagle.c               | 11 +++++++++++
 board/ti/evm/evm.c                     | 11 +++++++++++
 board/ti/sdp3430/sdp.c                 |  9 +++++++++
 board/timll/devkit8000/devkit8000.c    | 11 +++++++++++
 drivers/mmc/omap_hsmmc.c               |  7 +------
 16 files changed, 154 insertions(+), 6 deletions(-)

board/comelit/dig297/dig297.c +2 -0

index 2b826df..784483b 100644
@@ -133,6 +133,15 @@ int board_mmc_init(bd_t *bis)
 {
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
+
+int board_mmc_power_init(void)

Quoted 8 lines

board/comelit/dig297/dig297.c int board_mmc_init(bd_t *bis) · line 141
Quoted 4 lines
board/comelit/dig297/dig297.c +4 -0
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
I guess all twl4030 based boards have to have this ramp up delay, right? If so, why don't we want to hide it inside the twl4030_power_mmc_init() function and not spread it across all boards?

That makes perfect sense, thanks for the suggestion. Will do in the next version.

Quoted 10 lines

board/comelit/dig297/dig297.c int board_mmc_init(bd_t *bis) · line 144
Quoted 3 lines
board/comelit/dig297/dig297.c +3 -0
+#endif
+	return 0;
+}
Also, what do you think of the below suggestion:
Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c (as it seems that many boards want it) as it currently is. This way you will not need to patch each board file, yet a board has the ability to control that call via the CONFIG_TWL4030_POWER and have a board specific callback should it need one.

One of the initial intents of this patch set was also to remove the implication of CONFIG_TWL4030_POWER on twl4030_power_mmc_init, because my device (LG Optimus Black P970) uses TWL4030 regulators in a way that doesn't match other boards or any reference design (so I need CONFIG_TWL4030_POWER but not twl4030_power_mmc_init).

Quoted 4 lines

I would also change envelope the call to twl4030_power_mmc_init() to something like CONFIG_TWL4030_MMC_POWER instead of CONFIG_TWL4030_POWER, so it will be more flexible, but that has little to do with this patch.

That would be acceptable for me, but it would mean having two places where the same thing happens: the new board_mmc_power_init function and internally on omap_hsmmc. That's the sort of lack of consistance I definitely do not like, but if you think it's the best way to do it, I don't see any further issue with that.

Patching all the boards isn't really that big a deal (identifying the boards is already done now anyway) and it makes similar sense to having multiple board_mmc_init that all look very much alike.

In addition, perhaps I should add a dev_index parameter to twl4030_power_mmc_init (in a separate patch) so that only the relevant LDO is enabled (see my previous patch enabling VMMC2 too, that was accepted): most devices only use one MMC controller, so there is no need to enable all the MMC LDOs.

That would give some legitimacy to having one twl4030_power_mmc_init per board, since it would reflect whether to enable one or both MMC LDOs.

What do you think?

Quoted 17 lines

Quoted 3 lines

board/comelit/dig297/dig297.c +0 -0
 #endif
 
 #ifdef CONFIG_CMD_NET

[...]

Quoted 11 lines

drivers/mmc/omap_hsmmc.c +0 -3

index ef2cbf9..6fb78b3 100644
@@ -135,12 +135,7 @@ static unsigned char mmc_board_init(struct mmc *mmc)
 	pbias_lite = readl(&t2_base->pbias_lite);
 	pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0);
 	writel(pbias_lite, &t2_base->pbias_lite);
-#endif
-#if defined(CONFIG_TWL4030_POWER)
-	twl4030_power_mmc_init();

Quoted 9 lines

drivers/mmc/omap_hsmmc.c static unsigned char mmc_board_init(struct mmc *mmc) · line 141
Quoted 8 lines
drivers/mmc/omap_hsmmc.c +1 -3
-	mdelay(100);	/* ramp-up delay from Linux code */
-#endif
-#if defined(CONFIG_OMAP34XX)
+
 	writel(pbias_lite | PBIASLITEPWRDNZ1 |
 		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
 		&t2_base->pbias_lite);

Igor Grinberg

2014-11-03 07:55 UTC #8 lore raw

PATCH v3 2/2 omap_hsmmc: Board-specific TWL4030 MMC power initializations

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 64 lines · Paul Kocialkowski

Hi Igor, thanks for the review.
Quoted 39 lines · Paul Kocialkowski
Quoted 6 lines
Boards using the TWL4030 regulator may not all use the LDOs the same way (e.g. MMC2 power can be controlled by another LDO than VMMC2). This delegates TWL4030 MMC power initializations to board-specific functions, that may still call twl4030_power_mmc_init for the default behavior.
Signed-off-by Paul Kocialkowski <[email protected]>
This mostly looks good, several suggestions below though.
Quoted 29 lines
---
 board/comelit/dig297/dig297.c          |  9 +++++++++
 board/compulab/cm_t35/cm_t35.c         | 11 +++++++++++
 board/corscience/tricorder/tricorder.c | 11 +++++++++++
 board/isee/igep00x0/igep00x0.c         | 11 +++++++++++
 board/logicpd/omap3som/omap3logic.c    | 11 +++++++++++
 board/logicpd/zoom1/zoom1.c            |  9 +++++++++
 board/matrix_vision/mvblx/mvblx.c      |  9 +++++++++
 board/nokia/rx51/rx51.c                |  9 +++++++++
 board/overo/overo.c                    | 11 +++++++++++
 board/pandora/pandora.c                |  9 +++++++++
 board/technexion/tao3530/tao3530.c     | 11 +++++++++++
 board/ti/beagle/beagle.c               | 11 +++++++++++
 board/ti/evm/evm.c                     | 11 +++++++++++
 board/ti/sdp3430/sdp.c                 |  9 +++++++++
 board/timll/devkit8000/devkit8000.c    | 11 +++++++++++
 drivers/mmc/omap_hsmmc.c               |  7 +------
 16 files changed, 154 insertions(+), 6 deletions(-)
board/comelit/dig297/dig297.c +2 -0
index 2b826df..784483b 100644
@@ -133,6 +133,15 @@ int board_mmc_init(bd_t *bis)
 {
 	return omap_mmc_init(0, 0, 0, -1, -1);
 }
+
+int board_mmc_power_init(void)
Quoted 8 lines
board/comelit/dig297/dig297.c int board_mmc_init(bd_t *bis) · line 141
Quoted 4 lines
board/comelit/dig297/dig297.c +4 -0
+{
+#if defined(CONFIG_TWL4030_POWER)
+	twl4030_power_mmc_init();
+	mdelay(100);	/* ramp-up delay from Linux code */
I guess all twl4030 based boards have to have this ramp up delay, right? If so, why don't we want to hide it inside the twl4030_power_mmc_init() function and not spread it across all boards?
That makes perfect sense, thanks for the suggestion. Will do in the next version.
Quoted 10 lines
board/comelit/dig297/dig297.c int board_mmc_init(bd_t *bis) · line 144
Quoted 3 lines
board/comelit/dig297/dig297.c +3 -0
+#endif
+	return 0;
+}
Also, what do you think of the below suggestion:
Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c (as it seems that many boards want it) as it currently is. This way you will not need to patch each board file, yet a board has the ability to control that call via the CONFIG_TWL4030_POWER and have a board specific callback should it need one.

Quoted 5 lines

One of the initial intents of this patch set was also to remove the implication of CONFIG_TWL4030_POWER on twl4030_power_mmc_init, because my device (LG Optimus Black P970) uses TWL4030 regulators in a way that doesn't match other boards or any reference design (so I need CONFIG_TWL4030_POWER but not twl4030_power_mmc_init).

In that case the below proposal suits you even better...

Quoted 11 lines

Quoted 5 lines
I would also change envelope the call to twl4030_power_mmc_init() to something like CONFIG_TWL4030_MMC_POWER instead of CONFIG_TWL4030_POWER, so it will be more flexible, but that has little to do with this patch.
That would be acceptable for me, but it would mean having two places where the same thing happens: the new board_mmc_power_init function and internally on omap_hsmmc. That's the sort of lack of consistance I definitely do not like, but if you think it's the best way to do it, I don't see any further issue with that.

Well, I fine with this.
Having the mmc power initialized in a "centralized" manner is good. I just wanted to spare patching multiple boards, but in light of the said below I think you are right.

Quoted 4 lines

Patching all the boards isn't really that big a deal (identifying the boards is already done now anyway) and it makes similar sense to having multiple board_mmc_init that all look very much alike.

Ok.

Quoted 6 lines

In addition, perhaps I should add a dev_index parameter to twl4030_power_mmc_init (in a separate patch) so that only the relevant LDO is enabled (see my previous patch enabling VMMC2 too, that was accepted): most devices only use one MMC controller, so there is no need to enable all the MMC LDOs.

That is a great idea.
This actually aroused in my mind when I first saw the VMMC2 patch.

Quoted 3 lines

That would give some legitimacy to having one twl4030_power_mmc_init per board, since it would reflect whether to enable one or both MMC LDOs.

Yep. I think, you should do this now, just before adding the multiple instances of twl4030_power_mmc_init() call and patching it later.

Quoted 2 lines

What do you think?

All said above makes a good sense. So let's do this.

Also, I think having a DM for MMC will make things much less "wild west" here, but that is a separate issue.

Quoted 19 lines

Quoted 4 lines

board/comelit/dig297/dig297.c +0 -0
 #endif
 
 #ifdef CONFIG_CMD_NET

[...]

Quoted 12 lines

drivers/mmc/omap_hsmmc.c +0 -4

index ef2cbf9..6fb78b3 100644
@@ -135,12 +135,7 @@ static unsigned char mmc_board_init(struct mmc *mmc)
 	pbias_lite = readl(&t2_base->pbias_lite);
 	pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0);
 	writel(pbias_lite, &t2_base->pbias_lite);
-#endif
-#if defined(CONFIG_TWL4030_POWER)
-	twl4030_power_mmc_init();
-	mdelay(100);	/* ramp-up delay from Linux code */

Quoted 9 lines

Quoted 8 lines
drivers/mmc/omap_hsmmc.c static unsigned char mmc_board_init(struct mmc *mmc) · line 141
Quoted 7 lines
drivers/mmc/omap_hsmmc.c +1 -2
-#endif
-#if defined(CONFIG_OMAP34XX)
+
 	writel(pbias_lite | PBIASLITEPWRDNZ1 |
 		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
 		&t2_base->pbias_lite);

- --
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUVzTpAAoJEBDE8YO64EfavjcP/A0K8waMwn2XsBFgVyTLB7Q+ MUhufFKVHr/7ddFMutIQXzpG3BMzN+Q3Iwi3GzNCoJetHLBNkLAST4nqzTxYYuhu uW16d+Jru40KsNX8JD1waoW2Igdp4AWtY30KT8Khh+OUNvXXbxExkedYPdDyoRz5 gmKigvGtMMyyWpclMqyXewLT6jJjmBEOkccmvbEWbu3xXf71Oxb2SSdM4fwZtdAU PJIPW8wYHDQuYKNWhPUSsfiu/NQ63xs5/mW8YePKpwE54YCutT97Gvme05G+xUE9 ck4ZI5C49haHSq28HzVDmhXu8oAcCZke9998abqKD+3oDb5w72Hnoy6apx9ZtWSR dRFJp/TMEG3sClftxI0zvZc85430On6TnzmSEb5LlO1HnmEF6QgKTEf6dwsjqCvD WU0V1b0p3FyYxnIc+GlAWpis1pG+dtXiXkyLIEbI4Xcvnn7fo48kUVsrHGiENDcc sCR2ewcnLxVtzQfbSs7AwTiThwxO9Nb5vsQh3Y+Z+1VLrZq5jOAnEB7EbgwqI/K8 snApLZyLFwiazOh/MVzxaonYUQRGYs0mLesjhXORyyQRcCvlUUSvS3AEMDV6EHP/ sRU8iXhZ1s3qlCVEL4pkSLiqQUJSZskumbt9/qrpavz4TPWDUSqc5/b5+7yqEPLm dBofkwkw/9oqiUyPD0ps
=FA1/
-----END PGP SIGNATURE-----