| Message ID | 1506079337-12882-1-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017/9/22 19:22, Fabrizio Castro wrote: > mmc_regulator_get_supply returns -EPROBE_DEFER if either vmmc or > vqmmc regulators had their probing deferred. > vqmmc regulator is needed by UHS to work properly, therefore this > patch checks the value returned by mmc_regulator_get_supply to > make sure we have a reference to both vmmc and vqmmc (if found in > the DT). > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > --- > > I believe this patch is in order for UHS to work properly on the RZ/G1 platforms > as the signal voltage needs to be switched from 3.3V to 1.8V, and therefore we > need to make sure we hold a reference to vqmmc. Without this patch there is a > chance that the regulator driver gets probed later on and therefore we end up > without control over the power regulator. I have seen an RZ/G1E based platform > failing the transition to UHS because of this. It makes sense. > However, there are side effects to it as the mmc devices will be enumerated > in a different order (due to -EPROBE_DEFER), this means this patch would likely > break existing platforms. However this doesn't. The user daemon should never relies on the sequency of probing the mmc block part. We neither support to provide alias for the device id, nor guarantee the first probed controller get the first block id. Please use PARTUUID for rootfs, etc. So this shouldn't be a big deal. > Please, let me know your comments. > > drivers/mmc/host/tmio_mmc_core.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 88a9435..3df0413 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1153,8 +1153,11 @@ static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > { > struct tmio_mmc_data *pdata = host->pdata; > struct mmc_host *mmc = host->mmc; > + int err; > > - mmc_regulator_get_supply(mmc); > + err = mmc_regulator_get_supply(mmc); > + if (err) > + return err; > > /* use ocr_mask if no regulator */ > if (!mmc->ocr_avail) >
> > However, there are side effects to it as the mmc devices will be enumerated > > in a different order (due to -EPROBE_DEFER), this means this patch would likely > > break existing platforms. > > However this doesn't. The user daemon should never relies on the > sequency of probing the mmc block part. We neither support to provide > alias for the device id, nor guarantee the first probed controller get > the first block id. Please use PARTUUID for rootfs, etc. So this > shouldn't be a big deal. That's exactly what I thought as well... Thanks for the patch! Will test it the next days, but it looks good already.
Thank you guys for your comments! > > > However, there are side effects to it as the mmc devices will be enumerated > > > in a different order (due to -EPROBE_DEFER), this means this patch would likely > > > break existing platforms. > > > > However this doesn't. The user daemon should never relies on the > > sequency of probing the mmc block part. We neither support to provide > > alias for the device id, nor guarantee the first probed controller get > > the first block id. Please use PARTUUID for rootfs, etc. So this > > shouldn't be a big deal. > > That's exactly what I thought as well... > > Thanks for the patch! Will test it the next days, but it looks good > already. How is the testing going? Any problem with the patch? Thanks, Fabrizio Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Fabrizio, On Wed, Oct 04, 2017 at 10:50:54AM +0000, Fabrizio Castro wrote: > Thank you guys for your comments! > > > > > However, there are side effects to it as the mmc devices will be enumerated > > > > in a different order (due to -EPROBE_DEFER), this means this patch would likely > > > > break existing platforms. > > > > > > However this doesn't. The user daemon should never relies on the > > > sequency of probing the mmc block part. We neither support to provide > > > alias for the device id, nor guarantee the first probed controller get > > > the first block id. Please use PARTUUID for rootfs, etc. So this > > > shouldn't be a big deal. > > > > That's exactly what I thought as well... > > > > Thanks for the patch! Will test it the next days, but it looks good > > already. > > How is the testing going? Any problem with the patch? Sorry for the delay, there were some other things in the queue... Well, the good news is that it didn't cause a regression for me on R-Car E2 (Alt board) and H3 (Salvator X board). Sadly, it doesn't also fix the SDR104 problems on Alt. I had a little hope this could be the culprit, but sadly it is not. But this is not a problem with this patch. It still fixes a valid issue. Looking how other drivers deal with the issue, though, I noticed they only bail out on -EPROBE_DEFER and would continue on other errors (if mmc_regulator_get_supply would throw them, but it doesn't). So some digging why other drivers do it that way seems to be apropriate to me. Or asking Ulf :) Kind regards, Wolfram
Hi Wolfram, Ulf > Hi Fabrizio, > > On Wed, Oct 04, 2017 at 10:50:54AM +0000, Fabrizio Castro wrote: > > Thank you guys for your comments! > > > > > > > However, there are side effects to it as the mmc devices will be enumerated > > > > > in a different order (due to -EPROBE_DEFER), this means this patch would likely > > > > > break existing platforms. > > > > > > > > However this doesn't. The user daemon should never relies on the > > > > sequency of probing the mmc block part. We neither support to provide > > > > alias for the device id, nor guarantee the first probed controller get > > > > the first block id. Please use PARTUUID for rootfs, etc. So this > > > > shouldn't be a big deal. > > > > > > That's exactly what I thought as well... > > > > > > Thanks for the patch! Will test it the next days, but it looks good > > > already. > > > > How is the testing going? Any problem with the patch? > > Sorry for the delay, there were some other things in the queue... No worries at all! And thank you for getting back to me. > > Well, the good news is that it didn't cause a regression for me on R-Car > E2 (Alt board) and H3 (Salvator X board). > > Sadly, it doesn't also fix the SDR104 problems on Alt. I had a little > hope this could be the culprit, but sadly it is not. But this is not a > problem with this patch. It still fixes a valid issue. > > Looking how other drivers deal with the issue, though, I noticed they > only bail out on -EPROBE_DEFER and would continue on other errors (if > mmc_regulator_get_supply would throw them, but it doesn't). So some > digging why other drivers do it that way seems to be apropriate to me. Good point! Maybe if this patch explicitly tested for -EPROBE_DEFER the maintenance of mmc_regulator_get_supply would be easier as all of the drivers behaved similarly. On the other hand, both "vmmc" and "vqmmc" are considered optional by mmc_regulator_get_supply, therefore their absence is tolerated (0 is returned), and as you said the only other possible value returned by mmc_regulator_get_supply is -EPROBE_DEFER. If in the future the logic of mmc_regulator_get_supply changes, I guess even the driver needs to adapt as there may be something wrong with the regulators, therefore I personally rather the driver to bail out whatever the error returned by mmc_regulator_get_supply. Looking at the history of the other drivers I couldn't spot any meaningful comment connected to the return value of mmc_regulator_get_supply. > Or asking Ulf :) I guess you are right! ;) Ulf, what's the best option here? Kind regards, Fabrizio > > Kind regards, > > Wolfram Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> >> Well, the good news is that it didn't cause a regression for me on R-Car >> E2 (Alt board) and H3 (Salvator X board). >> >> Sadly, it doesn't also fix the SDR104 problems on Alt. I had a little >> hope this could be the culprit, but sadly it is not. But this is not a >> problem with this patch. It still fixes a valid issue. >> >> Looking how other drivers deal with the issue, though, I noticed they >> only bail out on -EPROBE_DEFER and would continue on other errors (if >> mmc_regulator_get_supply would throw them, but it doesn't). So some >> digging why other drivers do it that way seems to be apropriate to me. > > Good point! > Maybe if this patch explicitly tested for -EPROBE_DEFER the maintenance > of mmc_regulator_get_supply would be easier as all of the drivers behaved > similarly. > On the other hand, both "vmmc" and "vqmmc" are considered optional by > mmc_regulator_get_supply, therefore their absence is tolerated (0 is > returned), and as you said the only other possible value returned by > mmc_regulator_get_supply is -EPROBE_DEFER. > If in the future the logic of mmc_regulator_get_supply changes, I guess > even the driver needs to adapt as there may be something wrong with the > regulators, therefore I personally rather the driver to bail out whatever > the error returned by mmc_regulator_get_supply. Looking at the history > of the other drivers I couldn't spot any meaningful comment connected > to the return value of mmc_regulator_get_supply. It has been a bit tricky to deal with these regulators in *one* common way. For some platforms a regulator are optional, while for other it isn't. In the end we decided to pick the "optional" way for both vmmc and vqmmc, and then leave the details in the error path to be sorted out by each an every mmc host driver. > >> Or asking Ulf :) > > I guess you are right! ;) > > Ulf, what's the best option here? I think it's perfectly okay to return an error if mmc_regulator_get_supply() propagates that. That's likely what all host driver should be doing, instead of explicitly check for -EPROBE_DEFER. Of course, even when mmc_regulator_get_supply() returns 0, that's not a guarantee that the regulator(s) was actually hooked up. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> [...] > > >> > >> Well, the good news is that it didn't cause a regression for me on R-Car > >> E2 (Alt board) and H3 (Salvator X board). > >> > >> Sadly, it doesn't also fix the SDR104 problems on Alt. I had a little > >> hope this could be the culprit, but sadly it is not. But this is not a > >> problem with this patch. It still fixes a valid issue. > >> > >> Looking how other drivers deal with the issue, though, I noticed they > >> only bail out on -EPROBE_DEFER and would continue on other errors (if > >> mmc_regulator_get_supply would throw them, but it doesn't). So some > >> digging why other drivers do it that way seems to be apropriate to me. > > > > Good point! > > Maybe if this patch explicitly tested for -EPROBE_DEFER the maintenance > > of mmc_regulator_get_supply would be easier as all of the drivers behaved > > similarly. > > On the other hand, both "vmmc" and "vqmmc" are considered optional by > > mmc_regulator_get_supply, therefore their absence is tolerated (0 is > > returned), and as you said the only other possible value returned by > > mmc_regulator_get_supply is -EPROBE_DEFER. > > If in the future the logic of mmc_regulator_get_supply changes, I guess > > even the driver needs to adapt as there may be something wrong with the > > regulators, therefore I personally rather the driver to bail out whatever > > the error returned by mmc_regulator_get_supply. Looking at the history > > of the other drivers I couldn't spot any meaningful comment connected > > to the return value of mmc_regulator_get_supply. > > It has been a bit tricky to deal with these regulators in *one* common > way. For some platforms a regulator are optional, while for other it > isn't. > > In the end we decided to pick the "optional" way for both vmmc and > vqmmc, and then leave the details in the error path to be sorted out > by each an every mmc host driver. > > > > >> Or asking Ulf :) > > > > I guess you are right! ;) > > > > Ulf, what's the best option here? > > I think it's perfectly okay to return an error if > mmc_regulator_get_supply() propagates that. That's likely what all > host driver should be doing, instead of explicitly check for > -EPROBE_DEFER. Thank you Uffe. I guess the patch is alright then. Wolfram, do you want to add a Tested-by? Kind regards, Fabrizio > > Of course, even when mmc_regulator_get_supply() returns 0, that's not > a guarantee that the regulator(s) was actually hooked up. > > Kind regards > Uffe Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> Thank you Uffe. I guess the patch is alright then. > > Wolfram, do you want to add a Tested-by? Sure: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On 22 September 2017 at 13:22, Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > mmc_regulator_get_supply returns -EPROBE_DEFER if either vmmc or > vqmmc regulators had their probing deferred. > vqmmc regulator is needed by UHS to work properly, therefore this > patch checks the value returned by mmc_regulator_get_supply to > make sure we have a reference to both vmmc and vqmmc (if found in > the DT). > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> Thanks, applied for next! Kind regards Uffe > --- > > I believe this patch is in order for UHS to work properly on the RZ/G1 platforms > as the signal voltage needs to be switched from 3.3V to 1.8V, and therefore we > need to make sure we hold a reference to vqmmc. Without this patch there is a > chance that the regulator driver gets probed later on and therefore we end up > without control over the power regulator. I have seen an RZ/G1E based platform > failing the transition to UHS because of this. > However, there are side effects to it as the mmc devices will be enumerated > in a different order (due to -EPROBE_DEFER), this means this patch would likely > break existing platforms. > Please, let me know your comments. > > drivers/mmc/host/tmio_mmc_core.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 88a9435..3df0413 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1153,8 +1153,11 @@ static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > { > struct tmio_mmc_data *pdata = host->pdata; > struct mmc_host *mmc = host->mmc; > + int err; > > - mmc_regulator_get_supply(mmc); > + err = mmc_regulator_get_supply(mmc); > + if (err) > + return err; > > /* use ocr_mask if no regulator */ > if (!mmc->ocr_avail) > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 88a9435..3df0413 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1153,8 +1153,11 @@ static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) { struct tmio_mmc_data *pdata = host->pdata; struct mmc_host *mmc = host->mmc; + int err; - mmc_regulator_get_supply(mmc); + err = mmc_regulator_get_supply(mmc); + if (err) + return err; /* use ocr_mask if no regulator */ if (!mmc->ocr_avail)
mmc_regulator_get_supply returns -EPROBE_DEFER if either vmmc or vqmmc regulators had their probing deferred. vqmmc regulator is needed by UHS to work properly, therefore this patch checks the value returned by mmc_regulator_get_supply to make sure we have a reference to both vmmc and vqmmc (if found in the DT). Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- I believe this patch is in order for UHS to work properly on the RZ/G1 platforms as the signal voltage needs to be switched from 3.3V to 1.8V, and therefore we need to make sure we hold a reference to vqmmc. Without this patch there is a chance that the regulator driver gets probed later on and therefore we end up without control over the power regulator. I have seen an RZ/G1E based platform failing the transition to UHS because of this. However, there are side effects to it as the mmc devices will be enumerated in a different order (due to -EPROBE_DEFER), this means this patch would likely break existing platforms. Please, let me know your comments. drivers/mmc/host/tmio_mmc_core.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)