mmc-uclass: correct the device number

PATCH Superseded Kever Yang 3 participants 5 messages Jul 19, 2016 – Aug 1, 2016 MMC, SD and SDIO

Share & tools

Kever Yang

author

2016-07-19 13:28 UTC #0 lore raw

PATCH v3 1/1 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 <[email protected]>

---

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(-)

drivers/mmc/mmc-uclass.c +2 -2

index 38ced41..d0ca91b 100644
@@ -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)

include/mmc.h +6 -0

index 8f309f1..dd47f34 100644
@@ -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);

Signature · 1 line

1.9.1

Jaehoon Chung

2016-07-21 02:04 UTC #1 lore raw

PATCH v3 1/1 mmc-uclass: correct the device number

Hi Kever,

Quoted 2 lines · Kever Yang

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.

Quoted 2 lines

Signed-off-by Kever Yang <[email protected]>

Reviewed-by Jaehoon Chung <[email protected]>

Tested-by Jaehoon Chung <[email protected]>

(On Exynos4 SoCs with eMMC/SD interface.)

Best Regards,
Jaehoon Chung

Quoted 53 lines

---

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(-)

drivers/mmc/mmc-uclass.c +2 -2

index 38ced41..d0ca91b 100644
@@ -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)

include/mmc.h +6 -0

index 8f309f1..dd47f34 100644
@@ -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);

Simon Glass

2016-07-22 03:21 UTC #2 lore raw

PATCH v3 1/1 mmc-uclass: correct the device number

In reply to Kever Yang

Hi Kever,

Quoted 22 lines · Kever Yang

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 <[email protected]>
---
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(-)
drivers/mmc/mmc-uclass.c +0 -0
index 38ced41..d0ca91b 100644

drivers/mmc/mmc-uclass.c struct mmc *find_mmc_device(int dev_num) · line 114

Quoted 5 lines

drivers/mmc/mmc-uclass.c +1 -1

@@ -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.

Quoted 30 lines

drivers/mmc/mmc-uclass.c +1 -1

 }
 
 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)

include/mmc.h +6 -1

index 8f309f1..dd47f34 100644
@@ -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);
--

Quoted 3 lines

1.9.1

Regards,
Simon

Jaehoon Chung

2016-07-22 04:14 UTC #3 lore raw

PATCH v3 1/1 mmc-uclass: correct the device number

Hi Simon,

Quoted 26 lines · Simon Glass

Hi Kever,
Quoted 23 lines · Kever Yang
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 <[email protected]>
---
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(-)
drivers/mmc/mmc-uclass.c +0 -0
index 38ced41..d0ca91b 100644
@@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
 

Quoted 11 lines

drivers/mmc/mmc-uclass.c struct mmc *find_mmc_device(int dev_num) · line 114
Quoted 4 lines
drivers/mmc/mmc-uclass.c +1 -1
 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

Quoted 3 lines

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

Quoted 40 lines

Quoted 31 lines

drivers/mmc/mmc-uclass.c +1 -1

 }
 
 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)

include/mmc.h +6 -1

index 8f309f1..dd47f34 100644
@@ -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);
--

Quoted 3 lines

1.9.1

Regards,
Simon

Simon Glass

2016-08-01 02:21 UTC #4 lore raw

PATCH v3 1/1 mmc-uclass: correct the device number

Hi Jaehoon,

Quoted 41 lines · Jaehoon Chung

Hi Simon,
Quoted 26 lines · Simon Glass
Hi Kever,
Quoted 23 lines · Kever Yang
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 <[email protected]>
---
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(-)
drivers/mmc/mmc-uclass.c +0 -0
index 38ced41..d0ca91b 100644
@@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
 
Quoted 11 lines
drivers/mmc/mmc-uclass.c struct mmc *find_mmc_device(int dev_num) · line 114
Quoted 4 lines
drivers/mmc/mmc-uclass.c +1 -1
 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.

Quoted 12 lines

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
Quoted 3 lines
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.

Quoted 10 lines

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,

I allows the mmc driver to be probed from device tree. It was the first stage of the conversion.

Quoted 9 lines

ifdef CONFIG_DM_MMC
obj-$(CONFIG_GENERIC_MMC) += mmc-uclass.o
endif
ifndef CONFIG_BLK
obj-$(CONFIG_GENERIC_MMC) += mmc_legacy.o
endif

Quoted 8 lines

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.

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!

Quoted 5 lines

Best Regards,
Jaehoon Chung
Quoted 1 line

Regards,
Simon