| Message ID | 1409700683-25393-3-git-send-email-pab@pabigot.com |
|---|---|
| State | Accepted |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
On 09/02/2014 05:31 PM, Peter A. Bigot wrote: > The capacity and lba for an MMC device with part_num 0 reflects the > whole device. When mmc_switch_part() successfully switches to a > partition, the capacity is changed to that partition. As partition 0 > does not physically exist, attempts to switch back to the whole device > will indicate an error, but the capacity setting for the whole device > must still be restored to match the partition. > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > @@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int part_num) > ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, > (mmc->part_config & ~PART_ACCESS_MASK) > | (part_num & PART_ACCESS_MASK)); > - if (ret) > - return ret; > > - return mmc_set_capacity(mmc, part_num); > + /* > + * Set the capacity if the switch succeeded or was intended > + * to return to representing the raw device. > + */ > + if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0))) > + ret = mmc_set_capacity(mmc, part_num); > + > + return ret; > } I think this wouldn't be needed without patch 1/2, since without that patch, no partition switching should ever happen if HW partitions don't exist, and hence this patch shouldn't be required.
On 09/03/2014 10:48 AM, Stephen Warren wrote: > On 09/02/2014 05:31 PM, Peter A. Bigot wrote: >> The capacity and lba for an MMC device with part_num 0 reflects the >> whole device. When mmc_switch_part() successfully switches to a >> partition, the capacity is changed to that partition. As partition 0 >> does not physically exist, attempts to switch back to the whole device >> will indicate an error, but the capacity setting for the whole device >> must still be restored to match the partition. > >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > >> @@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int >> part_num) >> ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, >> (mmc->part_config & ~PART_ACCESS_MASK) >> | (part_num & PART_ACCESS_MASK)); >> - if (ret) >> - return ret; >> >> - return mmc_set_capacity(mmc, part_num); >> + /* >> + * Set the capacity if the switch succeeded or was intended >> + * to return to representing the raw device. >> + */ >> + if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0))) >> + ret = mmc_set_capacity(mmc, part_num); >> + >> + return ret; >> } > > I think this wouldn't be needed without patch 1/2, since without that > patch, no partition switching should ever happen if HW partitions > don't exist, and hence this patch shouldn't be required. Not so. In SPL mode, the mmc device passed in to the environment code is set up for partition 0. In the failure case, u-boot is configured to expect an environment in partition 2, and so invokes mmc_switch_part to go to partition 2 to see if it's a valid partition. In my case that fails because the partition size is zero, but regardless the mmc_switch_part back to mmc->part_num fails because the mmc_switch() call rejects the attempt with an error. Without this second patch, you end up with mmc->part_num left at zero but the capacity/lba fields configured for partition 2 which does not exist and has size zero, and SPL is unable to locate u-boot.img to continue. Please review the details in the meta-ti discussion. I'll respond to the comments on patch 1 separately. Peter
On 09/03/2014 09:59 AM, Peter A. Bigot wrote: > On 09/03/2014 10:48 AM, Stephen Warren wrote: >> On 09/02/2014 05:31 PM, Peter A. Bigot wrote: >>> The capacity and lba for an MMC device with part_num 0 reflects the >>> whole device. When mmc_switch_part() successfully switches to a >>> partition, the capacity is changed to that partition. As partition 0 >>> does not physically exist, attempts to switch back to the whole device >>> will indicate an error, but the capacity setting for the whole device >>> must still be restored to match the partition. >> >>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >> >>> @@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int >>> part_num) >>> ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, >>> (mmc->part_config & ~PART_ACCESS_MASK) >>> | (part_num & PART_ACCESS_MASK)); >>> - if (ret) >>> - return ret; >>> >>> - return mmc_set_capacity(mmc, part_num); >>> + /* >>> + * Set the capacity if the switch succeeded or was intended >>> + * to return to representing the raw device. >>> + */ >>> + if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0))) >>> + ret = mmc_set_capacity(mmc, part_num); >>> + >>> + return ret; >>> } >> >> I think this wouldn't be needed without patch 1/2, since without that >> patch, no partition switching should ever happen if HW partitions >> don't exist, and hence this patch shouldn't be required. > > Not so. > > In SPL mode, the mmc device passed in to the environment code is set up > for partition 0. In the failure case, u-boot is configured to expect an What failure case? > environment in partition 2, and so invokes mmc_switch_part to go to > partition 2 to see if it's a valid partition. In my case that fails > because the partition size is zero, but regardless the mmc_switch_part > back to mmc->part_num fails because the mmc_switch() call rejects the > attempt with an error. Isn't that where the bug should be fixed then; why doesn't mmc_switch() work as desired? If mmc_switch() isn't intended to work on devices without HW partitions, then why is it being called at all in any case (normal or failure case)? I also wonder why, if your board configuration is set up to assume an eMMC device with HW partitions, you're using a device without eMMC HW partitions; it seems like either the board configuration or your HW configuration is incorrect (or at least don't match), so if you have problems, it's not surprising, and not something that should be fixed. > Without this second patch, you end up with mmc->part_num left at zero > but the capacity/lba fields configured for partition 2 which does not > exist and has size zero, and SPL is unable to locate u-boot.img to > continue. > > Please review the details in the meta-ti discussion. I have no idea what that is.
On 09/03/2014 11:05 AM, Stephen Warren wrote: > On 09/03/2014 09:59 AM, Peter A. Bigot wrote: >> On 09/03/2014 10:48 AM, Stephen Warren wrote: >>> On 09/02/2014 05:31 PM, Peter A. Bigot wrote: >>>> The capacity and lba for an MMC device with part_num 0 reflects the >>>> whole device. When mmc_switch_part() successfully switches to a >>>> partition, the capacity is changed to that partition. As partition 0 >>>> does not physically exist, attempts to switch back to the whole device >>>> will indicate an error, but the capacity setting for the whole device >>>> must still be restored to match the partition. >>> >>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >>> >>>> @@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int >>>> part_num) >>>> ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, >>>> (mmc->part_config & ~PART_ACCESS_MASK) >>>> | (part_num & PART_ACCESS_MASK)); >>>> - if (ret) >>>> - return ret; >>>> >>>> - return mmc_set_capacity(mmc, part_num); >>>> + /* >>>> + * Set the capacity if the switch succeeded or was intended >>>> + * to return to representing the raw device. >>>> + */ >>>> + if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0))) >>>> + ret = mmc_set_capacity(mmc, part_num); >>>> + >>>> + return ret; >>>> } >>> >>> I think this wouldn't be needed without patch 1/2, since without that >>> patch, no partition switching should ever happen if HW partitions >>> don't exist, and hence this patch shouldn't be required. >> >> Not so. >> >> In SPL mode, the mmc device passed in to the environment code is set up >> for partition 0. In the failure case, u-boot is configured to expect an > > What failure case? > >> environment in partition 2, and so invokes mmc_switch_part to go to >> partition 2 to see if it's a valid partition. In my case that fails >> because the partition size is zero, but regardless the mmc_switch_part >> back to mmc->part_num fails because the mmc_switch() call rejects the >> attempt with an error. > > Isn't that where the bug should be fixed then; why doesn't > mmc_switch() work as desired? If mmc_switch() isn't intended to work > on devices without HW partitions, then why is it being called at all > in any case (normal or failure case)? I have no idea. > I also wonder why, if your board configuration is set up to assume an > eMMC device with HW partitions, you're using a device without eMMC HW > partitions; it seems like either the board configuration or your HW > configuration is incorrect (or at least don't match), so if you have > problems, it's not surprising, and not something that should be fixed. This is a beaglebone (black). It has an eMMC, and an SD card. It can boot from either, and will fall back to the SD card if the eMMC is uninitialized or under other magic conditions. If you believe the beaglebone u-boot configuration is wrong for how the beaglebone is intended to be used, I'll refer you to the TI folks to sort it out. > >> Without this second patch, you end up with mmc->part_num left at zero >> but the capacity/lba fields configured for partition 2 which does not >> exist and has size zero, and SPL is unable to locate u-boot.img to >> continue. >> >> Please review the details in the meta-ti discussion. > > I have no idea what that is. The link in the cover letter I provided with the patches, in hopes it would answer questions about why I was doing this. It all starts here: http://www.mail-archive.com/meta-ti@yoctoproject.org/msg04276.html Tom Rini: OK, so I've provided the patch upstream as you requested. I'm not going to continue to fight to get it incorporated. I believe you understand that there's a problem, and it's on hardware you're probably paid to support, unlike me. Y'all can figure out whether and how you want to resolve it. Peter
On Tue, Sep 02, 2014 at 06:31:23PM -0500, Peter A. Bigot wrote: > The capacity and lba for an MMC device with part_num 0 reflects the > whole device. When mmc_switch_part() successfully switches to a > partition, the capacity is changed to that partition. As partition 0 > does not physically exist, attempts to switch back to the whole device > will indicate an error, but the capacity setting for the whole device > must still be restored to match the partition. > > Signed-off-by: Peter A. Bigot <pab@pabigot.com> Tested-by: Tom Rini <trini@ti.com>
Hi Tom, On Sep 11, 2014, at 8:45 PM, Tom Rini <trini@ti.com> wrote: > On Tue, Sep 02, 2014 at 06:31:23PM -0500, Peter A. Bigot wrote: > >> The capacity and lba for an MMC device with part_num 0 reflects the >> whole device. When mmc_switch_part() successfully switches to a >> partition, the capacity is changed to that partition. As partition 0 >> does not physically exist, attempts to switch back to the whole device >> will indicate an error, but the capacity setting for the whole device >> must still be restored to match the partition. >> >> Signed-off-by: Peter A. Bigot <pab@pabigot.com> > > Tested-by: Tom Rini <trini@ti.com> > I’m going to take this in. It’s required to get the bone to boot from a standard MMC and I’d rather not introduce another boneblack variant to get it to boot from MMC. > -- > Tom Tested-by: Pantelis Antoniou <panto@antoniou-consulting.com> Acked-by: Pantelis Antoniou <panto@antoniou-consulting.com>
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index a26f3ce..fa04a3f 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int part_num) ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, (mmc->part_config & ~PART_ACCESS_MASK) | (part_num & PART_ACCESS_MASK)); - if (ret) - return ret; - return mmc_set_capacity(mmc, part_num); + /* + * Set the capacity if the switch succeeded or was intended + * to return to representing the raw device. + */ + if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0))) + ret = mmc_set_capacity(mmc, part_num); + + return ret; } int mmc_getcd(struct mmc *mmc)
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition. Signed-off-by: Peter A. Bigot <pab@pabigot.com> --- drivers/mmc/mmc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)