[U-Boot] [RESEND 0/2] mmc:fix: MMC fixes for generic mmc driver running at GONI

[U-Boot] [RESEND 0/2] mmc:fix: MMC fixes for generic mmc driver running at GONI

[Lukasz Majewski]

Those two patches have been combined together for easier review.

Both fixes corner cases caught on the Samsung's GONI target.
Namely:
- lack of call to mmc_init with fat handling
- Not setting MMC capabilities according to host capabilities


Lukasz Majewski (2):
  mmc:fix: Set mmc width according to MMC host capabilities
  mmc:fix Call mmc_init() when executing mmc_get_dev()

 drivers/mmc/mmc.c |    9 +++++++--
 include/mmc.h     |    3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

-- 
1.7.2.3

[U-Boot] [RESEND 1/2] mmc:fix: Set mmc width according to MMC host capabilities

[Lukasz Majewski]

This patch sets the MMC width according to the MMC host capabilities.
It turned out, that there are some targets (e.g. GONI), which are able
to read data from SPI only at 4 bit mode.
This patch restricts the width number according to the MMC host.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Andy Fleming <afleming@gmail.com>
---
 drivers/mmc/mmc.c |    4 +++-
 include/mmc.h     |    3 +++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index e70fa9f..618960e 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1199,7 +1199,9 @@ int mmc_startup(struct mmc *mmc)
 		else
 			mmc_set_clock(mmc, 25000000);
 	} else {
-		for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {
+		width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
+			 MMC_MODE_WIDTH_BITS_SHIFT);
+		for (; width >= 0; width--) {
 			/* Set the card to use 4 bit*/
 			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
 					EXT_CSD_BUS_WIDTH, width);
diff --git a/include/mmc.h b/include/mmc.h
index f52df70..ee16349 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -47,6 +47,9 @@
 #define MMC_MODE_SPI		0x400
 #define MMC_MODE_HC		0x800
 
+#define MMC_MODE_MASK_WIDTH_BITS (MMC_MODE_4BIT | MMC_MODE_8BIT)
+#define MMC_MODE_WIDTH_BITS_SHIFT 8
+
 #define SD_DATA_4BIT	0x00040000
 
 #define IS_SD(x) (x->version & SD_VERSION_SD)
-- 
1.7.2.3

[U-Boot] [RESEND 2/2] mmc:fix Call mmc_init() when executing mmc_get_dev()

[Lukasz Majewski]

This code adds call to mmc_init(), for partition related commands (e.g.
fatls, fatinfo etc.).

It is safe to call mmc_init() multiple times since mmc->has_init flag
prevents from multiple initialization.

The FAT related code calls get_dev high level method and then uses
elements from mmc->block_dev, which is uninitialized until the mmc_init
(and thereof mmc_startup) is called.

This problem appears on boards, which don't use mmc as the default
place for envs

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Andy Fleming <afleming@gmail.com>
---
 drivers/mmc/mmc.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 618960e..84eb4e0 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1305,8 +1305,11 @@ int mmc_register(struct mmc *mmc)
 block_dev_desc_t *mmc_get_dev(int dev)
 {
 	struct mmc *mmc = find_mmc_device(dev);
+	if (!mmc)
+		return NULL;
 
-	return mmc ? &mmc->block_dev : NULL;
+	mmc_init(mmc);
+	return &mmc->block_dev;
 }
 #endif
 
-- 
1.7.2.3

[U-Boot] [RESEND 2/2] mmc:fix Call mmc_init() when executing mmc_get_dev()

[Lei Wen]

Hi Lukasz,

On Thu, Apr 19, 2012 at 8:39 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> This code adds call to mmc_init(), for partition related commands (e.g.
> fatls, fatinfo etc.).
>
> It is safe to call mmc_init() multiple times since mmc->has_init flag
> prevents from multiple initialization.
>
> The FAT related code calls get_dev high level method and then uses
> elements from mmc->block_dev, which is uninitialized until the mmc_init
> (and thereof mmc_startup) is called.
>
> This problem appears on boards, which don't use mmc as the default
> place for envs
>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Andy Fleming <afleming@gmail.com>
> ---
> ?drivers/mmc/mmc.c | ? ?5 ++++-
> ?1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 618960e..84eb4e0 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1305,8 +1305,11 @@ int mmc_register(struct mmc *mmc)
> ?block_dev_desc_t *mmc_get_dev(int dev)
> ?{
> ? ? ? ?struct mmc *mmc = find_mmc_device(dev);
> + ? ? ? if (!mmc)
> + ? ? ? ? ? ? ? return NULL;
>
> - ? ? ? return mmc ? &mmc->block_dev : NULL;
> + ? ? ? mmc_init(mmc);

I'm concerning with this adding init here.
Since not every platform mount with emmc as boot device, and what they
need is booting fast. If you order them to initialize all mmc/sd at mmc register
stage, this adding booting time may not be the one they want to see.

For FAT command, I think you could abstract a init method, in which mmc
could call its mmc_init(). I previously make a patch for this, don't whether it
could fit your need:

diff --git a/disk/part.c b/disk/part.c
index e4e7997..3d00670 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -77,7 +77,7 @@ DECLARE_GLOBAL_DATA_PTR;
 block_dev_desc_t *get_dev(char* ifname, int dev)
 {
 	const struct block_drvr *drvr = block_drvr;
-	block_dev_desc_t* (*reloc_get_dev)(int dev);
+	block_dev_desc_t* (*reloc_get_dev)(int dev), *dev_desc;
 	char *name;

 	name = drvr->name;
@@ -91,8 +91,13 @@ block_dev_desc_t *get_dev(char* ifname, int dev)
 		name += gd->reloc_off;
 		reloc_get_dev += gd->reloc_off;
 #endif
-		if (strncmp(ifname, name, strlen(name)) == 0)
-			return reloc_get_dev(dev);
+		if (strncmp(ifname, name, strlen(name)) == 0) {
+			dev_desc = reloc_get_dev(dev);
+			if (dev_desc && dev_desc->dev_init(dev_desc->dev))
+				dev_desc = NULL;
+
+			return dev_desc;
+		}
 		drvr++;
 	}
 	return NULL;
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 1622417..c4c48e7 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1150,6 +1150,7 @@ int mmc_send_if_cond(struct mmc *mmc)
 	return 0;
 }

+static int mmc_dev_init(int dev_num);
 int mmc_register(struct mmc *mmc)
 {
 	/* Setup the universal parts of the block interface just once */
@@ -1159,6 +1160,7 @@ int mmc_register(struct mmc *mmc)
 	mmc->block_dev.block_read = mmc_bread;
 	mmc->block_dev.block_write = mmc_bwrite;
 	mmc->block_dev.block_erase = mmc_berase;
+	mmc->block_dev.dev_init = mmc_dev_init;
 	if (!mmc->b_max)
 		mmc->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;

@@ -1226,6 +1228,15 @@ int mmc_init(struct mmc *mmc)
 	return err;
 }

+static int mmc_dev_init(int dev_num)
+{
+	struct mmc *mmc = find_mmc_device(dev_num);
+	if (!mmc)
+		return -1;
+
+	return mmc_init(mmc);
+}
+
 /*
  * CPU and board-specific MMC initializations.  Aliased function
  * signals caller to move on
diff --git a/include/part.h b/include/part.h
index 2864adb..dac2bdd 100644
--- a/include/part.h
+++ b/include/part.h
@@ -41,6 +41,7 @@ typedef struct block_dev_desc {
 	char		vendor [40+1];	/* IDE model, SCSI Vendor */
 	char		product[20+1];	/* IDE Serial no, SCSI product */
 	char		revision[8+1];	/* firmware revision */
+	int		(*dev_init)(int dev);
 	unsigned long	(*block_read)(int dev,
 				      unsigned long start,
 				      lbaint_t blkcnt,


Thanks,
Lei

Thanks,
Lei

[U-Boot] [RESEND 1/2] mmc:fix: Set mmc width according to MMC host capabilities

[Lei Wen]

On Thu, Apr 19, 2012 at 8:39 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> This patch sets the MMC width according to the MMC host capabilities.
> It turned out, that there are some targets (e.g. GONI), which are able
> to read data from SPI only at 4 bit mode.
> This patch restricts the width number according to the MMC host.
>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Andy Fleming <afleming@gmail.com>
> ---
> ?drivers/mmc/mmc.c | ? ?4 +++-
> ?include/mmc.h ? ? | ? ?3 +++
> ?2 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index e70fa9f..618960e 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1199,7 +1199,9 @@ int mmc_startup(struct mmc *mmc)
> ? ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ? ? ? ? ?mmc_set_clock(mmc, 25000000);
> ? ? ? ?} else {
> - ? ? ? ? ? ? ? for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {
> + ? ? ? ? ? ? ? width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
> + ? ? ? ? ? ? ? ? ? ? ? ?MMC_MODE_WIDTH_BITS_SHIFT);
> + ? ? ? ? ? ? ? for (; width >= 0; width--) {
> ? ? ? ? ? ? ? ? ? ? ? ?/* Set the card to use 4 bit*/
> ? ? ? ? ? ? ? ? ? ? ? ?err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?EXT_CSD_BUS_WIDTH, width);
> diff --git a/include/mmc.h b/include/mmc.h
> index f52df70..ee16349 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -47,6 +47,9 @@
> ?#define MMC_MODE_SPI ? ? ? ? ? 0x400
> ?#define MMC_MODE_HC ? ? ? ? ? ?0x800
>
> +#define MMC_MODE_MASK_WIDTH_BITS (MMC_MODE_4BIT | MMC_MODE_8BIT)
> +#define MMC_MODE_WIDTH_BITS_SHIFT 8
> +
> ?#define SD_DATA_4BIT ? 0x00040000
>
> ?#define IS_SD(x) (x->version & SD_VERSION_SD)
> --
> 1.7.2.3
>
Acked-by: Lei Wen <leiwen@marvell.com>

Thanks,
Lei

[U-Boot] [RESEND 2/2] mmc:fix Call mmc_init() when executing mmc_get_dev()

[Lukasz Majewski]

Hi, Lei

> I'm concerning with this adding init here.
> Since not every platform mount with emmc as boot device, and what they
> need is booting fast. 

If I remember correctly, u-boot policy is to not initialize the mmc
until it is needed (i.e. command is executed).
So the extra init won't be executed until fatls or mmc is executed.

> If you order them to initialize all mmc/sd at
> mmc register stage, this adding booting time may not be the one they
> want to see.

I think that booting time will not increase, because in the mmc_init()
there is a check:

	if (mmc->has_init)
		return 0;

To prevent multiple register level initialization.

The execution time increase is boiled down to executing a
few instructions (when mmc->has_init is set).


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center
Platform Group

[U-Boot] [RESEND 2/2] mmc:fix Call mmc_init() when executing mmc_get_dev()

[Lei Wen]

Hi Lukasz,

On Fri, Apr 20, 2012 at 3:09 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi, Lei
>
>> I'm concerning with this adding init here.
>> Since not every platform mount with emmc as boot device, and what they
>> need is booting fast.
>
> If I remember correctly, u-boot policy is to not initialize the mmc
> until it is needed (i.e. command is executed).
> So the extra init won't be executed until fatls or mmc is executed.
>
>> If you order them to initialize all mmc/sd at
>> mmc register stage, this adding booting time may not be the one they
>> want to see.
>
> I think that booting time will not increase, because in the mmc_init()
> there is a check:
>
> ? ? ? ?if (mmc->has_init)
> ? ? ? ? ? ? ? ?return 0;
>
> To prevent multiple register level initialization.
>
> The execution time increase is boiled down to executing a
> few instructions (when mmc->has_init is set).
>
>
I misunderstood your original patch...
I was thinking you want to init the mmc device directly in the mmc register
process which is a bad idea. But since you add the init only in the get_dev,
I think this approach is ok to me.

Thanks,
Lei

[U-Boot] [RESEND 0/2] mmc:fix: MMC fixes for generic mmc driver running at GONI

[Lukasz Majewski]

Hi Andy,

> Those two patches have been combined together for easier review.
> 
> Both fixes corner cases caught on the Samsung's GONI target.
> Namely:
> - lack of call to mmc_init with fat handling
> - Not setting MMC capabilities according to host capabilities
> 
> 
> Lukasz Majewski (2):
>   mmc:fix: Set mmc width according to MMC host capabilities
>   mmc:fix Call mmc_init() when executing mmc_get_dev()
> 
>  drivers/mmc/mmc.c |    9 +++++++--
>  include/mmc.h     |    3 +++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 

Hi Andy,

Are those fixes acceptable for the mmc subsystem?

Both have spent quite some time on the mailing list.


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [RESEND 2/2] mmc:fix Call mmc_init() when executing mmc_get_dev()

[Andy Fleming]

On Fri, Apr 20, 2012 at 2:09 AM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi, Lei
>
>> I'm concerning with this adding init here.
>> Since not every platform mount with emmc as boot device, and what they
>> need is booting fast.
>
> If I remember correctly, u-boot policy is to not initialize the mmc
> until it is needed (i.e. command is executed).
> So the extra init won't be executed until fatls or mmc is executed.
>
>> If you order them to initialize all mmc/sd at
>> mmc register stage, this adding booting time may not be the one they
>> want to see.
>
> I think that booting time will not increase, because in the mmc_init()
> there is a check:
>
> ? ? ? ?if (mmc->has_init)
> ? ? ? ? ? ? ? ?return 0;

This right here is something we need to discuss as a community. On the
one hand, I can see the desire to not do unnecessary initialization
every time we issue a command. On the other hand, we need some way of
dealing with the possibility that the cards that were in the slot when
we booted are no longer there (or that empty slots have now been
filled).

I'm now taking comments...

Andy

[U-Boot] [RESEND 2/2] mmc:fix Call mmc_init() when executing mmc_get_dev()

[Lukasz Majewski]

Hi Andy,

> On Fri, Apr 20, 2012 at 2:09 AM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> > Hi, Lei
> >
> >> I'm concerning with this adding init here.
> >> Since not every platform mount with emmc as boot device, and what
> >> they need is booting fast.
> >
> > If I remember correctly, u-boot policy is to not initialize the mmc
> > until it is needed (i.e. command is executed).
> > So the extra init won't be executed until fatls or mmc is executed.
> >
> >> If you order them to initialize all mmc/sd at
> >> mmc register stage, this adding booting time may not be the one
> >> they want to see.
> >
> > I think that booting time will not increase, because in the
> > mmc_init() there is a check:
> >
> > ? ? ? ?if (mmc->has_init)
> > ? ? ? ? ? ? ? ?return 0;
> 
> This right here is something we need to discuss as a community. On the
> one hand, I can see the desire to not do unnecessary initialization
> every time we issue a command.

I cannot agree on that. I believe that subsystems (like MMC) shall be
explicitly enabled when needed. 

Moreover we cannot guarantee, that MMC subsystem will be enabled only at
one place. For example mmc group of commands can explicitly enable it,
but also any other command which needs access to eMMC storage will
enable it.

Therefore, I think that:
?if (mmc->has_init)
? ? ? ? ? ?return 0;

check is needed. 

Additionally we cannot check all possibilities of calling mmc_init()
and due to that performing two times initialization from scratch can be
catastrophic. 

One more solution came to my mind. We can break u-boot policy, define a
flag (e.g. CONFIG_MMC_BOOT_ON), which will always issue the mmc_init()
during boot time. Then we can remove (or not execute)
mmc_init() call from other commands/use cases.


> On the other hand, we need some way of
> dealing with the possibility that the cards that were in the slot when
> we booted are no longer there (or that empty slots have now been
> filled).

One good idea would be to distinct SD cards (removable) with eMMC
persistent devices.

Creating a "mmc device class" would solve the problem. Then one
can assume, that eMMC is always "inserted" and no above problem appears.

With SD card one can probe (in some cards - card detect or more
reliably by issuing command) when trying to send CMD to it. Response
with 0xFFFF shall indicate that card has been removed.

I must check if above ideas can be implemented with new device model
for u-boot.

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group