[RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Fabrizio Castro]

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

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

Re: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Shawn Lin]

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


-- 
Best Regards
Shawn Lin

Re: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Fabrizio Castro]

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.

Re: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Fabrizio Castro]

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.

Re: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Ulf Hansson]

[...]

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

RE: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Fabrizio Castro]

> [...]
>
> >>
> >> 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.

Re: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 239 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] mmc: tmio: check mmc_regulator_get_supply return value

[Ulf Hansson]

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
>