[U-Boot] [RESEND 0/2] mmc:fix: MMC fixes for generic mmc driver running at GONI
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(-)
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)
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
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
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
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).
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
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
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.
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.
participants (3)
-
Andy Fleming -
Lei Wen -
Lukasz Majewski