mmc: atmel: Silence debug output

After silencing the prints which were generated when reconfiguring the clock of the SD/MMC bus, surprisingly, the driver stopped working such that every attempt to use the SD/MMC bus caused the CPU to get totally stuck hard. It turns out that the prints generated a short delay, which was necessary for the CPU to reconfigure the clock without getting stuck. Thus, this patch adds a short delay after the clock configuration instead.

Signed-off-by Marek Vasut <[email protected]>

---
 drivers/mmc/gen_atmel_mci.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/mmc/gen_atmel_mci.c +2 -0

index 2815e57..8b05fcd 100644
@@ -113,6 +113,8 @@ static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen)
 	if (mmc->card_caps & mmc->cfg->host_caps & MMC_MODE_HS)
 		writel(MMCI_BIT(HSMODE), &mci->cfg);
 
+	udelay(50);
+
 	initialized = 1;
 }
 

After silencing the prints which were generated when reconfiguring the clock of the SD/MMC bus, surprisingly, the driver stopped working such that every attempt to use the SD/MMC bus caused the CPU to get totally stuck hard. It turns out that the prints generated a short delay, which was necessary for the CPU to reconfigure the clock without getting stuck. Thus, this patch adds a short delay after the clock configuration instead.

Signed-off-by Marek Vasut <[email protected]>

Reviewed-by Andreas Bie?mann <[email protected]>

---
drivers/mmc/gen_atmel_mci.c | 2 ++

1 file changed, 2 insertions(+)

Instead of passing just the register area as a private data, introduce a proper struct atmel_mci_priv structure instead. This will become useful in the subsequent patch, where we eliminate the global variable from this driver.

Signed-off-by Marek Vasut <[email protected]>

---
 drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

drivers/mmc/gen_atmel_mci.c +21 -11

index 8b05fcd..abc77cc 100644
@@ -32,6 +32,11 @@
 # define MCI_BUS 0
 #endif
 
+struct atmel_mci_priv {
+	struct mmc_config	cfg;
+	struct atmel_mci	*mci;
+};
+
 static int initialized = 0;
 
 /* Read Atmel MCI IP version */

@@ -55,7 +60,8 @@ static void dump_cmd(u32 cmdr, u32 arg, u32 status, const char* msg)
 /* Setup for MCI Clock and Block Size */
 static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen)
 {
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;
 	u32 bus_hz = get_mci_clk_rate();
 	u32 clkdiv = 255;
 	unsigned int version = atmel_mci_get_version(mci);

@@ -198,7 +204,8 @@ io_fail:
 static int
 mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 {
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;
 	u32 cmdr;
 	u32 error_flags = 0;
 	u32 status;

@@ -323,7 +330,8 @@ mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 /* Entered into mmc structure during driver init */
 static void mci_set_ios(struct mmc *mmc)
 {
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;
 	int bus_width = mmc->bus_width;
 	unsigned int version = atmel_mci_get_version(mci);
 	int busw;

@@ -359,7 +367,8 @@ static void mci_set_ios(struct mmc *mmc)
 /* Entered into mmc structure during driver init */
 static int mci_init(struct mmc *mmc)
 {
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;
 
 	/* Initialize controller */
 	writel(MMCI_BIT(SWRST), &mci->cr);	/* soft reset */

@@ -393,22 +402,23 @@ int atmel_mci_init(void *regs)
 {
 	struct mmc *mmc;
 	struct mmc_config *cfg;
-	struct atmel_mci *mci;
+	struct atmel_mci_priv *priv;
 	unsigned int version;
 
-	cfg = malloc(sizeof(*cfg));
-	if (cfg == NULL)
-		return -1;
-	memset(cfg, 0, sizeof(*cfg));
+	priv = calloc(1, sizeof(*priv));
+	if (!priv)
+		return -ENOMEM;
 
-	mci = (struct atmel_mci *)regs;
+	cfg = &priv->cfg;
 
 	cfg->name = "mci";
 	cfg->ops = &atmel_mci_ops;
 
+	priv->mci = (struct atmel_mci *)regs;
+
 	/* need to be able to pass these in on a board by board basis */
 	cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34;
-	version = atmel_mci_get_version(mci);
+	version = atmel_mci_get_version(priv->mci);
 	if ((version & 0xf00) >= 0x300) {
 		cfg->host_caps = MMC_MODE_8BIT;
 		cfg->host_caps |= MMC_MODE_HS | MMC_MODE_HS_52MHz;

Quoted 6 lines · Marek Vasut

Instead of passing just the register area as a private data, introduce a proper struct atmel_mci_priv structure instead. This will become useful in the subsequent patch, where we eliminate the global variable from this driver.

Signed-off-by Marek Vasut <[email protected]>

Reveiwed-by Andreas Bie?mann <[email protected]>

Quoted 127 lines

---

 drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

drivers/mmc/gen_atmel_mci.c +5 -0

index 8b05fcd..abc77cc 100644
@@ -32,6 +32,11 @@
 
 # define MCI_BUS 0
 #endif
 
+struct atmel_mci_priv {
+	struct mmc_config	cfg;
+	struct atmel_mci	*mci;
+};
+
 
 static int initialized = 0;

/* Read Atmel MCI IP version */

drivers/mmc/gen_atmel_mci.c +2 -1

@@ -55,7 +60,8 @@ static void dump_cmd(u32 cmdr, u32 arg, u32 status,
 const char* msg)
 
 /* Setup for MCI Clock and Block Size */
 static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen)
 {
 
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;

drivers/mmc/gen_atmel_mci.c +2 -1

 	u32 bus_hz = get_mci_clk_rate();
 	u32 clkdiv = 255;
 	unsigned int version = atmel_mci_get_version(mci);

@@ -198,7 +204,8 @@ io_fail:
 static int
 mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data
 *data) {
 
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;
 
 	u32 cmdr;

drivers/mmc/gen_atmel_mci.c +2 -1

 	u32 error_flags = 0;
 	u32 status;

@@ -323,7 +330,8 @@ mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 struct mmc_data *data)
 
 /* Entered into mmc structure during driver init */
 static void mci_set_ios(struct mmc *mmc)
 {
 
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;

drivers/mmc/gen_atmel_mci.c +2 -1

 	int bus_width = mmc->bus_width;
 	unsigned int version = atmel_mci_get_version(mci);
 	int busw;

@@ -359,7 +367,8 @@ static void mci_set_ios(struct mmc *mmc)
 
 /* Entered into mmc structure during driver init */
 static int mci_init(struct mmc *mmc)
 {
 
-	atmel_mci_t *mci = mmc->priv;
+	struct atmel_mci_priv *priv = mmc->priv;
+	atmel_mci_t *mci = priv->mci;
 

drivers/mmc/gen_atmel_mci.c +7 -6

 	/* Initialize controller */
 	writel(MMCI_BIT(SWRST), &mci->cr);	/* soft reset */

@@ -393,22 +402,23 @@ int atmel_mci_init(void *regs)
 
 {
 
 	struct mmc *mmc;
 	struct mmc_config *cfg;
 
-	struct atmel_mci *mci;
+	struct atmel_mci_priv *priv;
 
 	unsigned int version;
 
-	cfg = malloc(sizeof(*cfg));
-	if (cfg == NULL)
-		return -1;
-	memset(cfg, 0, sizeof(*cfg));
+	priv = calloc(1, sizeof(*priv));
+	if (!priv)
+		return -ENOMEM;
 
-	mci = (struct atmel_mci *)regs;
+	cfg = &priv->cfg;
 
 	cfg->name = "mci";
 	cfg->ops = &atmel_mci_ops;
 
+	priv->mci = (struct atmel_mci *)regs;
+
 
 	/* need to be able to pass these in on a board by board basis */

drivers/mmc/gen_atmel_mci.c +2 -2

 	cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34;
 
-	version = atmel_mci_get_version(mci);
+	version = atmel_mci_get_version(priv->mci);
 
 	if ((version & 0xf00) >= 0x300) {
 
 		cfg->host_caps = MMC_MODE_8BIT;
 		cfg->host_caps |= MMC_MODE_HS | MMC_MODE_HS_52MHz;

@@ -425,7 +435,7 @@ int atmel_mci_init(void *regs)
 
 	cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;
 
-	mmc = mmc_create(cfg, regs);
+	mmc = mmc_create(cfg, priv);
 

Quoted 3 lines

drivers/mmc/gen_atmel_mci.c +0 -0

 	if (mmc == NULL) {
 

free(cfg);

We shouldn't free cfg here but priv, the rest looks sane to me. Eventual return -ENODEV on !mmc and adopt the following comment, we may leak priv now as there is no de-init.

Quoted 29 lines · Andreas Bie?mann

Quoted 6 lines · Marek Vasut

Instead of passing just the register area as a private data, introduce a proper struct atmel_mci_priv structure instead. This will become useful in the subsequent patch, where we eliminate the global variable from this driver.

Signed-off-by Marek Vasut <[email protected]>

Reveiwed-by Andreas Bie?mann <[email protected]>

Quoted 19 lines

---

 drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

drivers/mmc/gen_atmel_mci.c +1 -1

index 8b05fcd..abc77cc 100644
@@ -425,7 +435,7 @@ int atmel_mci_init(void *regs)
 
 	cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;
 
-	mmc = mmc_create(cfg, regs);
+	mmc = mmc_create(cfg, priv);
 
 	if (mmc == NULL) {
 

Quoted 5 lines

Quoted 1 line

free(cfg);

We shouldn't free cfg here but priv, the rest looks sane to me. Eventual return -ENODEV on !mmc and adopt the following comment, we may leak priv now as there is no de-init.

Uh right, can you fix that while applying the patchset or do you want me to repost ?

Hi Marek,

Quoted 36 lines · Andreas Bie?mann

Quoted 34 lines · Marek Vasut

Quoted 6 lines

Instead of passing just the register area as a private data, introduce a proper struct atmel_mci_priv structure instead. This will become useful in the subsequent patch, where we eliminate the global variable from this driver.

Signed-off-by Marek Vasut <[email protected]>

Reveiwed-by Andreas Bie?mann <[email protected]>

Quoted 19 lines

---

 drivers/mmc/gen_atmel_mci.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

drivers/mmc/gen_atmel_mci.c +1 -1

index 8b05fcd..abc77cc 100644
@@ -425,7 +435,7 @@ int atmel_mci_init(void *regs)
 
 	cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;
 
-	mmc = mmc_create(cfg, regs);
+	mmc = mmc_create(cfg, priv);
 
 	if (mmc == NULL) {
 

Quoted 1 line

free(cfg);

We shouldn't free cfg here but priv, the rest looks sane to me. Eventual return -ENODEV on !mmc and adopt the following comment, we may leak priv now as there is no de-init.

Quoted 2 lines

Uh right, can you fix that while applying the patchset or do you want me to repost ?

I'll fix it.

Instead of passing just the register area as a private data, introduce a proper struct atmel_mci_priv structure instead. This will become useful in the subsequent patch, where we eliminate the global variable from this driver.

Signed-off-by Marek Vasut <[email protected]>

Reviewed-by Andreas Bie?mann <[email protected]>

[fix free()]

Signed-off-by Andreas Bie?mann <[email protected]>

---
drivers/mmc/gen_atmel_mci.c | 40 +++++++++++++++++++++++++---------------

1 file changed, 25 insertions(+), 15 deletions(-)

Global variables are bad. Get rid of this particular one, so we can correctly instantiate multiple atmel mci interfaces, without having them interfere with one another.

Signed-off-by Marek Vasut <[email protected]>

---
 drivers/mmc/gen_atmel_mci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/mmc/gen_atmel_mci.c +4 -4

index abc77cc..9a0332f 100644
@@ -35,10 +35,9 @@
 struct atmel_mci_priv {
 	struct mmc_config	cfg;
 	struct atmel_mci	*mci;
+	unsigned int		initialized:1;
 };
 
-static int initialized = 0;
-
 /* Read Atmel MCI IP version */
 static unsigned int atmel_mci_get_version(struct atmel_mci *mci)
 {

@@ -121,7 +120,7 @@ static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen)
 
 	udelay(50);
 
-	initialized = 1;
+	priv->initialized = 1;
 }
 
 /* Return the CMDR with flags for a given command and data packet */

@@ -210,7 +209,7 @@ mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 	u32 error_flags = 0;
 	u32 status;
 
-	if (!initialized) {
+	if (!priv->initialized) {
 		puts ("MCI not initialized!\n");
 		return COMM_ERR;
 	}

@@ -415,6 +414,7 @@ int atmel_mci_init(void *regs)
 	cfg->ops = &atmel_mci_ops;
 
 	priv->mci = (struct atmel_mci *)regs;
+	priv->initialized = 0;
 
 	/* need to be able to pass these in on a board by board basis */
 	cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34;

Global variables are bad. Get rid of this particular one, so we can correctly instantiate multiple atmel mci interfaces, without having them interfere with one another.

Signed-off-by Marek Vasut <[email protected]>

Reviewed-by Andreas Bie?mann <[email protected]>

---
drivers/mmc/gen_atmel_mci.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

This driver generates clearly debugging prints when changing clock speed, so silence those. Furthermore, the driver generates further prints in case a command fails to complete. The later case woud be useful, but for eMMC, command 8 can fail and it's not an error but a part of the specification. Thus, make this debug() as well.

Signed-off-by Marek Vasut <[email protected]>

---
 drivers/mmc/gen_atmel_mci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/mmc/gen_atmel_mci.c +4 -4

index 45bcffb..2815e57 100644
@@ -48,8 +48,8 @@ static unsigned int atmel_mci_get_version(struct atmel_mci *mci)
  */
 static void dump_cmd(u32 cmdr, u32 arg, u32 status, const char* msg)
 {
-	printf("gen_atmel_mci: CMDR %08x (%2u) ARGR %08x (SR: %08x) %s\n",
-		cmdr, cmdr&0x3F, arg, status, msg);
+	debug("gen_atmel_mci: CMDR %08x (%2u) ARGR %08x (SR: %08x) %s\n",
+	      cmdr, cmdr & 0x3F, arg, status, msg);
 }
 
 /* Setup for MCI Clock and Block Size */

@@ -73,7 +73,7 @@ static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen)
 			clkodd = clkdiv & 1;
 			clkdiv >>= 1;
 
-			printf("mci: setting clock %u Hz, block size %u\n",
+			debug("mci: setting clock %u Hz, block size %u\n",
 			       bus_hz / (clkdiv * 2 + clkodd + 2), blklen);
 		} else {
 			/* find clkdiv yielding a rate <= than requested */

@@ -81,7 +81,7 @@ static void mci_set_mode(struct mmc *mmc, u32 hz, u32 blklen)
 				if ((bus_hz / (clkdiv + 1) / 2) <= hz)
 					break;
 			}
-			printf("mci: setting clock %u Hz, block size %u\n",
+			debug("mci: setting clock %u Hz, block size %u\n",
 			       (bus_hz / (clkdiv + 1)) / 2, blklen);
 
 		}

This driver generates clearly debugging prints when changing clock speed, so silence those. Furthermore, the driver generates further prints in case a command fails to complete. The later case woud be useful, but for eMMC, command 8 can fail and it's not an error but a part of the specification. Thus, make this debug() as well.

Signed-off-by Marek Vasut <[email protected]>

Reviewed-by Andreas Bie?mann <[email protected]>

[fix checkpatch warnings]

Signed-off-by Andreas Bie?mann <[email protected]>

---
drivers/mmc/gen_atmel_mci.c | 12 ++++++------

1 file changed, 6 insertions(+), 6 deletions(-)