[U-Boot] [PATCH v3] mmc-uclass: correct the device number
Not like the mmc-legacy which the devnum starts from 1, it starts from 0 in mmc-uclass, so the device number should be (devnum + 1) in get_mmc_num().
Signed-off-by: Kever Yang kever.yang@rock-chips.com ---
Changes in v3: - apply comments from Jaehoon Chung
Changes in v2: - add comment for get_mmc_num() in mmc.h - update mmc_get_next_devnum()
drivers/mmc/mmc-uclass.c | 4 ++-- include/mmc.h | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 38ced41..d0ca91b 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
int get_mmc_num(void) { - return max(blk_find_max_devnum(IF_TYPE_MMC), 0); + return max((blk_find_max_devnum(IF_TYPE_MMC) + 1), 0); }
int mmc_get_next_devnum(void) @@ -122,7 +122,7 @@ int mmc_get_next_devnum(void) if (ret < 0) return ret;
- return ret + 1; + return ret; }
struct blk_desc *mmc_get_blk_desc(struct mmc *mmc) diff --git a/include/mmc.h b/include/mmc.h index 8f309f1..dd47f34 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -503,6 +503,12 @@ void mmc_set_clock(struct mmc *mmc, uint clock); struct mmc *find_mmc_device(int dev_num); int mmc_set_dev(int dev_num); void print_mmc_devices(char separator); + +/** + * get_mmc_num() - get the total MMC device number + * + * @return 0 if there is no MMC device, else the number of devices + */ int get_mmc_num(void); int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf, enum mmc_hwpart_conf_mode mode);
Hi Kever,
On 07/19/2016 10:28 PM, Kever Yang wrote:
Not like the mmc-legacy which the devnum starts from 1, it starts from 0 in mmc-uclass, so the device number should be (devnum + 1) in get_mmc_num().
Looks good to me. I had already sent the similar patch for this. (http://patchwork.ozlabs.org/patch/643921/)
But I think this patch is better than mine.
Signed-off-by: Kever Yang kever.yang@rock-chips.com
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com Tested-by: Jaehoon Chung jh80.chung@samsung.com (On Exynos4 SoCs with eMMC/SD interface.)
Best Regards, Jaehoon Chung
Changes in v3:
- apply comments from Jaehoon Chung
Changes in v2:
- add comment for get_mmc_num() in mmc.h
- update mmc_get_next_devnum()
drivers/mmc/mmc-uclass.c | 4 ++-- include/mmc.h | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 38ced41..d0ca91b 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
int get_mmc_num(void) {
- return max(blk_find_max_devnum(IF_TYPE_MMC), 0);
- return max((blk_find_max_devnum(IF_TYPE_MMC) + 1), 0);
}
int mmc_get_next_devnum(void) @@ -122,7 +122,7 @@ int mmc_get_next_devnum(void) if (ret < 0) return ret;
- return ret + 1;
- return ret;
}
struct blk_desc *mmc_get_blk_desc(struct mmc *mmc) diff --git a/include/mmc.h b/include/mmc.h index 8f309f1..dd47f34 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -503,6 +503,12 @@ void mmc_set_clock(struct mmc *mmc, uint clock); struct mmc *find_mmc_device(int dev_num); int mmc_set_dev(int dev_num); void print_mmc_devices(char separator);
+/**
- get_mmc_num() - get the total MMC device number
- @return 0 if there is no MMC device, else the number of devices
- */
int get_mmc_num(void); int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf, enum mmc_hwpart_conf_mode mode);
Hi Kever,
On 19 July 2016 at 07:28, Kever Yang kever.yang@rock-chips.com wrote:
Not like the mmc-legacy which the devnum starts from 1, it starts from 0 in mmc-uclass, so the device number should be (devnum + 1) in get_mmc_num().
Signed-off-by: Kever Yang kever.yang@rock-chips.com
Changes in v3:
- apply comments from Jaehoon Chung
Changes in v2:
- add comment for get_mmc_num() in mmc.h
- update mmc_get_next_devnum()
drivers/mmc/mmc-uclass.c | 4 ++-- include/mmc.h | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 38ced41..d0ca91b 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
int get_mmc_num(void) {
return max(blk_find_max_devnum(IF_TYPE_MMC), 0);
return max((blk_find_max_devnum(IF_TYPE_MMC) + 1), 0);
Sorry to be pendantic, but the problem is that this blk_find_max_devnum() can return -ENODEV. You change it to 0 in this case, which is correct for get_mmc_num(), but not for mmc_get_next_devnum(). I think you should adjust the latter to call blk_find_max_devnum() directly, so it can return an error if there is one.
I realise that this may not matter in practice, but it is really confusing the way you have it.
}
int mmc_get_next_devnum(void) @@ -122,7 +122,7 @@ int mmc_get_next_devnum(void) if (ret < 0) return ret;
return ret + 1;
return ret;}
struct blk_desc *mmc_get_blk_desc(struct mmc *mmc) diff --git a/include/mmc.h b/include/mmc.h index 8f309f1..dd47f34 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -503,6 +503,12 @@ void mmc_set_clock(struct mmc *mmc, uint clock); struct mmc *find_mmc_device(int dev_num); int mmc_set_dev(int dev_num); void print_mmc_devices(char separator);
+/**
- get_mmc_num() - get the total MMC device number
- @return 0 if there is no MMC device, else the number of devices
- */
int get_mmc_num(void); int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf, enum mmc_hwpart_conf_mode mode); -- 1.9.1
Regards, Simon
Hi Simon,
On 07/22/2016 12:21 PM, Simon Glass wrote:
Hi Kever,
On 19 July 2016 at 07:28, Kever Yang kever.yang@rock-chips.com wrote:
Not like the mmc-legacy which the devnum starts from 1, it starts from 0 in mmc-uclass, so the device number should be (devnum + 1) in get_mmc_num().
Signed-off-by: Kever Yang kever.yang@rock-chips.com
Changes in v3:
- apply comments from Jaehoon Chung
Changes in v2:
- add comment for get_mmc_num() in mmc.h
- update mmc_get_next_devnum()
drivers/mmc/mmc-uclass.c | 4 ++-- include/mmc.h | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 38ced41..d0ca91b 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
int get_mmc_num(void) {
return max(blk_find_max_devnum(IF_TYPE_MMC), 0);
return max((blk_find_max_devnum(IF_TYPE_MMC) + 1), 0);Sorry to be pendantic, but the problem is that this blk_find_max_devnum() can return -ENODEV. You change it to 0 in this case, which is correct for get_mmc_num(), but not for mmc_get_next_devnum(). I think you should adjust the latter to call blk_find_max_devnum() directly, so it can return an error if there is one.
You're right, blk_find_max_devnum() can be return -ENODEV. But get_mmc_num() is returned max(-ENODEV, 0), then it should be always returned 0, if there is no device. 0 means no devices, doesn't? (get_mmc_num() never returned the error number.) Well, i didn't find that case until now..Is there case that return -ENODEV from mmc_get_num()?
And mmc_get_next_devnum() is called in mmc_legacy.c. I didn't find anywhere called mmc_get_next_devnum() in mmc_uclass.c
I realise that this may not matter in practice, but it is really confusing the way you have it.
Hmm, I'm confusing a lot for MMC DM. It seems that there are three cases..
1. Use the legacy. - It's just using the existing model.
2. Use DM_MMC and legacy. - I don't understand why use the combination of DM_MMC and legacy. - When i see the u-boot-dm repository,
ifdef CONFIG_DM_MMC obj-$(CONFIG_GENERIC_MMC) += mmc-uclass.o endif
ifndef CONFIG_BLK obj-$(CONFIG_GENERIC_MMC) += mmc_legacy.o endif
It should be conflicted too many things..
3. Use DM_MMC and BLK - I think this is our best way.
Right? It might be my misunderstanding. Even if i shouldn't misunderstand something, i want to help you on MMC and block side. So i will go ahead for fixing and cleaning.
Best Regards, Jaehoon Chung
}
int mmc_get_next_devnum(void) @@ -122,7 +122,7 @@ int mmc_get_next_devnum(void) if (ret < 0) return ret;
return ret + 1;
return ret;}
struct blk_desc *mmc_get_blk_desc(struct mmc *mmc) diff --git a/include/mmc.h b/include/mmc.h index 8f309f1..dd47f34 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -503,6 +503,12 @@ void mmc_set_clock(struct mmc *mmc, uint clock); struct mmc *find_mmc_device(int dev_num); int mmc_set_dev(int dev_num); void print_mmc_devices(char separator);
+/**
- get_mmc_num() - get the total MMC device number
- @return 0 if there is no MMC device, else the number of devices
- */
int get_mmc_num(void); int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf, enum mmc_hwpart_conf_mode mode); -- 1.9.1
Regards, Simon
Hi Jaehoon,
On 21 July 2016 at 22:14, Jaehoon Chung jh80.chung@samsung.com wrote:
Hi Simon,
On 07/22/2016 12:21 PM, Simon Glass wrote:
Hi Kever,
On 19 July 2016 at 07:28, Kever Yang kever.yang@rock-chips.com wrote:
Not like the mmc-legacy which the devnum starts from 1, it starts from 0 in mmc-uclass, so the device number should be (devnum + 1) in get_mmc_num().
Signed-off-by: Kever Yang kever.yang@rock-chips.com
Changes in v3:
- apply comments from Jaehoon Chung
Changes in v2:
- add comment for get_mmc_num() in mmc.h
- update mmc_get_next_devnum()
drivers/mmc/mmc-uclass.c | 4 ++-- include/mmc.h | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 38ced41..d0ca91b 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
int get_mmc_num(void) {
return max(blk_find_max_devnum(IF_TYPE_MMC), 0);
return max((blk_find_max_devnum(IF_TYPE_MMC) + 1), 0);Sorry to be pendantic, but the problem is that this blk_find_max_devnum() can return -ENODEV. You change it to 0 in this case, which is correct for get_mmc_num(), but not for mmc_get_next_devnum(). I think you should adjust the latter to call blk_find_max_devnum() directly, so it can return an error if there is one.
You're right, blk_find_max_devnum() can be return -ENODEV. But get_mmc_num() is returned max(-ENODEV, 0), then it should be always returned 0, if there is no device. 0 means no devices, doesn't? (get_mmc_num() never returned the error number.) Well, i didn't find that case until now..Is there case that return -ENODEV from mmc_get_num()?
And mmc_get_next_devnum() is called in mmc_legacy.c. I didn't find anywhere called mmc_get_next_devnum() in mmc_uclass.c
I realise that this may not matter in practice, but it is really confusing the way you have it.
Yes it is confusing, I agree. My point is that -ENODEV is actually an error. Still, returning 0 is OK. for get_mmc_num() I suppose. But we cannot just add 1. Adding 1 to -ENODEV does not yield 0.
Hmm, I'm confusing a lot for MMC DM. It seems that there are three cases..
- Use the legacy.
- It's just using the existing model.
- Use DM_MMC and legacy.
- I don't understand why use the combination of DM_MMC and legacy.
- When i see the u-boot-dm repository,
I allows the mmc driver to be probed from device tree. It was the first stage of the conversion.
ifdef CONFIG_DM_MMC obj-$(CONFIG_GENERIC_MMC) += mmc-uclass.o endif
ifndef CONFIG_BLK obj-$(CONFIG_GENERIC_MMC) += mmc_legacy.o endif
It should be conflicted too many things..
- Use DM_MMC and BLK
- I think this is our best way.
Right? It might be my misunderstanding. Even if i shouldn't misunderstand something, i want to help you on MMC and block side. So i will go ahead for fixing and cleaning.
Yes that is exactly right. I have been trying to push things towards case 3, as per my recently applied changes. But more work is needed.
And in fact there is a 4th case - DM_MMC_OPS. That is where we really need to end up. There is quite a bit of code to convert and I could use more help!
Best Regards, Jaehoon Chung
Regards, Simon
participants (3)
-
Jaehoon Chung -
Kever Yang -
Simon Glass