[PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ludovic Desroches]

Hi,

I would like to have some feedback for these two patches. I have two questions.

1.

To suppress the warnings telling me I have no vmmc and vqmmc supplies, I have
added them and discovered that adding a vmmc supply involve only writing 1 in
the Power Control Register involves enabling the SD Bus Power with a non valid
value for the SD Bus Voltage. If the host controllers strictly follows the
specification, it shall not enable SD Bus Power. So enough if it seems useless
to configure SD Bus Voltage because we have an external regulator, do it.

By the way, I am curious to understand what is really the SD Bus Voltage. I
mean talking about bus voltage makes me thinking more about vqmmc than vmmc.
Is it only bad naming or do I miss something?

>From the specification, there is this figure:

                    HOST
---------------------------------------------------
| ------------        3.3V                        |       VDD
| | Power SW |------------------------------------|----------
| ------------   |              |                 |
|       ---------|              |                 |
|       |                       |                 |
| --------------     ----------------------       |
| | Ref. Volt. |-----| Regulator/Selector |       |
| --------------     ----------------------       |
|                    3.3V/1.8V  |                 |
|                               |                 |
|                               |---------------  |
|                               |       |  |   |  |
|                               |       R  R   _  |
|                               |       |  |   _  |
|                               |       |  |   |  |
|                               |       |  |  /// |
|                            -------    |  |      |       CLK
|                            |     |----)--)------|----------
|  -----------------------   |Multi|    |  |      |       CMD
|  |Random Logic Circuits|---|Drive|----)---------|----------
|  -----------------------   |I/O  |    |         |       DAT
|                            |     |--------------|----------
|                            -------              |
---------------------------------------------------

In my mind vmmc is the 3.3V signal and vqmmc is the 3.3V/1.8V signal, so why
talking about bus voltage?


2.

Is the regulator-gpio usage the right thing to do for vqmmc? In my case it is
not really driven by a gpio but by a pio from the sdhci device. In the binding,
declaring the gpio is an option so I thought using this regulator fits my need.


P.S. patch 2 is based on another patch not yet in next:
http://www.spinics.net/lists/arm-kernel/msg453447.html

Thanks

Ludovic


Ludovic Desroches (2):
  mmc: sdhci: set bus voltage before enabling bus power
  ARM: at91/dt: sama5d2 Xplained: add regulators for sdhci devices

 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 14 ++++++++++++++
 drivers/mmc/host/sdhci.c                    |  7 -------
 2 files changed, 14 insertions(+), 7 deletions(-)

-- 
2.5.0

[PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ludovic Desroches]

When there is a vmmc regulator, only SD Bus Power is set to 1 in the
Power Control Register. It means SD Bus Voltage Select field is set to 0
that is a reserved value. The SD Host Controller specification says:
'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD
Bus Voltage Select.' and 'If the Host Driver selects an unsupported
voltage in the SD B?us Voltage Select field, the Host Controller may
ignore writes to SD Bus Power and keep its value at zero."

Having an external regulator means the SD Bus Voltage Select is useless
but if the Host Controller strictly follows the specification then we
need to set a valid value.

Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 drivers/mmc/host/sdhci.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ac97b46..0cfd7b2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1278,13 +1278,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 		spin_unlock_irq(&host->lock);
 		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
 		spin_lock_irq(&host->lock);
-
-		if (mode != MMC_POWER_OFF)
-			sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
-		else
-			sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
-
-		return;
 	}
 
 	if (mode != MMC_POWER_OFF) {
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ludovic Desroches]

On Fri, Nov 06, 2015 at 04:59:29PM +0100, Ludovic Desroches wrote:
> When there is a vmmc regulator, only SD Bus Power is set to 1 in the
> Power Control Register. It means SD Bus Voltage Select field is set to 0
> that is a reserved value. The SD Host Controller specification says:
> 'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD
> Bus Voltage Select.' and 'If the Host Driver selects an unsupported
> voltage in the SD B?us Voltage Select field, the Host Controller may
> ignore writes to SD Bus Power and keep its value at zero."
> 
> Having an external regulator means the SD Bus Voltage Select is useless
> but if the Host Controller strictly follows the specification then we
> need to set a valid value.

Ulf,

What is your opinion about this patch?

If the 'no regulator found' message is turned in debug message, I can get
rid of my vmmc regulator but I really think that writing only
SDHCI_POWER_ON is opposite to the sdhci spec. I would say that not
setting the bus voltage is a quirk!

Regards

Ludovic

> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>  drivers/mmc/host/sdhci.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ac97b46..0cfd7b2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1278,13 +1278,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>  		spin_unlock_irq(&host->lock);
>  		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
>  		spin_lock_irq(&host->lock);
> -
> -		if (mode != MMC_POWER_OFF)
> -			sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> -		else
> -			sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> -
> -		return;
>  	}
>  
>  	if (mode != MMC_POWER_OFF) {
> -- 
> 2.5.0
> 

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ulf Hansson]

On 9 November 2015 at 14:23, Ludovic Desroches
<ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Nov 06, 2015 at 04:59:29PM +0100, Ludovic Desroches wrote:
>> When there is a vmmc regulator, only SD Bus Power is set to 1 in the
>> Power Control Register. It means SD Bus Voltage Select field is set to 0
>> that is a reserved value. The SD Host Controller specification says:
>> 'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD
>> Bus Voltage Select.' and 'If the Host Driver selects an unsupported
>> voltage in the SD B?us Voltage Select field, the Host Controller may
>> ignore writes to SD Bus Power and keep its value at zero."
>>
>> Having an external regulator means the SD Bus Voltage Select is useless
>> but if the Host Controller strictly follows the specification then we
>> need to set a valid value.
>
> Ulf,
>
> What is your opinion about this patch?
>
> If the 'no regulator found' message is turned in debug message, I can get
> rid of my vmmc regulator but I really think that writing only

I expect you mean vqmmc?

> SDHCI_POWER_ON is opposite to the sdhci spec. I would say that not
> setting the bus voltage is a quirk!

I don't really follow.

I read the SDHCI spec and the section for the Power Control Register.
Bit 0 needs to be set when communicating with the card as it will for
example enables the clock. Before setting bit0 you must decide what
signal level to use, which is done by writing to bit 1->3.

If SDHCI monitors the power state (MMC_POWER_UP|ON|OFF) when its
->set_ios() callback are invoked and in combination of using the
->start_signal_voltage_switch() callback to change the signal voltage
level, this *should* work out nicely.

Now, looking at the related code in sdhci, I am kind of surprised that
it works. :-) Though, again I don't have the in-depth knowledge about
sdhci.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ludovic Desroches]

On Mon, Nov 09, 2015 at 03:12:46PM +0100, Ulf Hansson wrote:
> On 9 November 2015 at 14:23, Ludovic Desroches
> <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, Nov 06, 2015 at 04:59:29PM +0100, Ludovic Desroches wrote:
> >> When there is a vmmc regulator, only SD Bus Power is set to 1 in the
> >> Power Control Register. It means SD Bus Voltage Select field is set to 0
> >> that is a reserved value. The SD Host Controller specification says:
> >> 'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD
> >> Bus Voltage Select.' and 'If the Host Driver selects an unsupported
> >> voltage in the SD B?us Voltage Select field, the Host Controller may
> >> ignore writes to SD Bus Power and keep its value at zero."
> >>
> >> Having an external regulator means the SD Bus Voltage Select is useless
> >> but if the Host Controller strictly follows the specification then we
> >> need to set a valid value.
> >
> > Ulf,
> >
> > What is your opinion about this patch?
> >
> > If the 'no regulator found' message is turned in debug message, I can get
> > rid of my vmmc regulator but I really think that writing only
> 
> I expect you mean vqmmc?

I don't mean vmmc. In the sdhci_set_power function, we are using vmmc.
I feel not confortable with it because the power control register
contains 'SD Bus' fields so it should depend on vqmmc not vmmc.

> 
> > SDHCI_POWER_ON is opposite to the sdhci spec. I would say that not
> > setting the bus voltage is a quirk!
> 
> I don't really follow.
> 
> I read the SDHCI spec and the section for the Power Control Register.
> Bit 0 needs to be set when communicating with the card as it will for
> example enables the clock. Before setting bit0 you must decide what
> signal level to use, which is done by writing to bit 1->3.
> 

Right. But when having vmmc supply we do:
sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL) or
sdhci_writeb(host, 0, SDHCI_POWER_CONTROL) so we loose the signal level,
isn't it? 

> If SDHCI monitors the power state (MMC_POWER_UP|ON|OFF) when its
> ->set_ios() callback are invoked and in combination of using the
> ->start_signal_voltage_switch() callback to change the signal voltage
> level, this *should* work out nicely.
> 

It is my turn to not follow! We write into the Power Control Register
only in sdhci_set_power(). May I miss a callback or something else?
sdhci_do_start_signal_voltage_switch doesn't modify the Power Control
Register.

> Now, looking at the related code in sdhci, I am kind of surprised that
> it works. :-) Though, again I don't have the in-depth knowledge about
> sdhci.
> 

Me too, I am starting to dig into the sdhci spec and some points are
not crystal clear.


Regards

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ulf Hansson]

On 9 November 2015 at 15:40, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> On Mon, Nov 09, 2015 at 03:12:46PM +0100, Ulf Hansson wrote:
>> On 9 November 2015 at 14:23, Ludovic Desroches
>> <ludovic.desroches@atmel.com> wrote:
>> > On Fri, Nov 06, 2015 at 04:59:29PM +0100, Ludovic Desroches wrote:
>> >> When there is a vmmc regulator, only SD Bus Power is set to 1 in the
>> >> Power Control Register. It means SD Bus Voltage Select field is set to 0
>> >> that is a reserved value. The SD Host Controller specification says:
>> >> 'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD
>> >> Bus Voltage Select.' and 'If the Host Driver selects an unsupported
>> >> voltage in the SD B?us Voltage Select field, the Host Controller may
>> >> ignore writes to SD Bus Power and keep its value at zero."
>> >>
>> >> Having an external regulator means the SD Bus Voltage Select is useless
>> >> but if the Host Controller strictly follows the specification then we
>> >> need to set a valid value.
>> >
>> > Ulf,
>> >
>> > What is your opinion about this patch?
>> >
>> > If the 'no regulator found' message is turned in debug message, I can get
>> > rid of my vmmc regulator but I really think that writing only
>>
>> I expect you mean vqmmc?
>
> I don't mean vmmc. In the sdhci_set_power function, we are using vmmc.
> I feel not confortable with it because the power control register
> contains 'SD Bus' fields so it should depend on vqmmc not vmmc.
>
>>
>> > SDHCI_POWER_ON is opposite to the sdhci spec. I would say that not
>> > setting the bus voltage is a quirk!
>>
>> I don't really follow.
>>
>> I read the SDHCI spec and the section for the Power Control Register.
>> Bit 0 needs to be set when communicating with the card as it will for
>> example enables the clock. Before setting bit0 you must decide what
>> signal level to use, which is done by writing to bit 1->3.
>>
>
> Right. But when having vmmc supply we do:
> sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL) or
> sdhci_writeb(host, 0, SDHCI_POWER_CONTROL) so we loose the signal level,
> isn't it?
>
>> If SDHCI monitors the power state (MMC_POWER_UP|ON|OFF) when its
>> ->set_ios() callback are invoked and in combination of using the
>> ->start_signal_voltage_switch() callback to change the signal voltage
>> level, this *should* work out nicely.
>>
>
> It is my turn to not follow! We write into the Power Control Register
> only in sdhci_set_power(). May I miss a callback or something else?
> sdhci_do_start_signal_voltage_switch doesn't modify the Power Control
> Register.
>
>> Now, looking at the related code in sdhci, I am kind of surprised that
>> it works. :-) Though, again I don't have the in-depth knowledge about
>> sdhci.
>>
>
> Me too, I am starting to dig into the sdhci spec and some points are
> not crystal clear.
>

Okay, I am finally starting to understand some of your concern.

According to the spec, the Power Control Register should control the
signal voltage/bus voltage. As UHS mode was added to the spec, it
seems like the Power Control Register couldn't cover all new cases, as
why Host Control 2 register needed to be added. The Host Control 2
register, is what sdhci_do_start_signal_voltage_switch() uses to
change the signal voltage level, which all makes sense to me.

For sdhci_set_power(); it seems to use the Power Control Register to
control the power to the card (VDD/VMMC). Indeed this looks *really*
weird/wrong. I wonder if it's working because of luck, intentional
violation of the SDHCI spec or because of special variants.

Especially when looking into the case when you *don't* have a VMMC
regulator several strange quirks exists in sdhci_set_power().

In the case when you *have* a VMMC, I think just setting/clearing bit
0 (SDHCI_POWER_ON) and then bail out, is probably working with modern
HW because it's likely the only thing needed.

Now, this discussion was interesting, but I forgot what problem you
actually where trying to solve? :-)

Kind regards
Uffe

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ludovic Desroches]

On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote:
> On 9 November 2015 at 15:40, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > On Mon, Nov 09, 2015 at 03:12:46PM +0100, Ulf Hansson wrote:
> >> On 9 November 2015 at 14:23, Ludovic Desroches
> >> <ludovic.desroches@atmel.com> wrote:
> >> > On Fri, Nov 06, 2015 at 04:59:29PM +0100, Ludovic Desroches wrote:
> >> >> When there is a vmmc regulator, only SD Bus Power is set to 1 in the
> >> >> Power Control Register. It means SD Bus Voltage Select field is set to 0
> >> >> that is a reserved value. The SD Host Controller specification says:
> >> >> 'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD
> >> >> Bus Voltage Select.' and 'If the Host Driver selects an unsupported
> >> >> voltage in the SD B?us Voltage Select field, the Host Controller may
> >> >> ignore writes to SD Bus Power and keep its value at zero."
> >> >>
> >> >> Having an external regulator means the SD Bus Voltage Select is useless
> >> >> but if the Host Controller strictly follows the specification then we
> >> >> need to set a valid value.
> >> >
> >> > Ulf,
> >> >
> >> > What is your opinion about this patch?
> >> >
> >> > If the 'no regulator found' message is turned in debug message, I can get
> >> > rid of my vmmc regulator but I really think that writing only
> >>
> >> I expect you mean vqmmc?
> >
> > I don't mean vmmc. In the sdhci_set_power function, we are using vmmc.
> > I feel not confortable with it because the power control register
> > contains 'SD Bus' fields so it should depend on vqmmc not vmmc.
> >
> >>
> >> > SDHCI_POWER_ON is opposite to the sdhci spec. I would say that not
> >> > setting the bus voltage is a quirk!
> >>
> >> I don't really follow.
> >>
> >> I read the SDHCI spec and the section for the Power Control Register.
> >> Bit 0 needs to be set when communicating with the card as it will for
> >> example enables the clock. Before setting bit0 you must decide what
> >> signal level to use, which is done by writing to bit 1->3.
> >>
> >
> > Right. But when having vmmc supply we do:
> > sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL) or
> > sdhci_writeb(host, 0, SDHCI_POWER_CONTROL) so we loose the signal level,
> > isn't it?
> >
> >> If SDHCI monitors the power state (MMC_POWER_UP|ON|OFF) when its
> >> ->set_ios() callback are invoked and in combination of using the
> >> ->start_signal_voltage_switch() callback to change the signal voltage
> >> level, this *should* work out nicely.
> >>
> >
> > It is my turn to not follow! We write into the Power Control Register
> > only in sdhci_set_power(). May I miss a callback or something else?
> > sdhci_do_start_signal_voltage_switch doesn't modify the Power Control
> > Register.
> >
> >> Now, looking at the related code in sdhci, I am kind of surprised that
> >> it works. :-) Though, again I don't have the in-depth knowledge about
> >> sdhci.
> >>
> >
> > Me too, I am starting to dig into the sdhci spec and some points are
> > not crystal clear.
> >
> 
> Okay, I am finally starting to understand some of your concern.
> 
> According to the spec, the Power Control Register should control the
> signal voltage/bus voltage. As UHS mode was added to the spec, it
> seems like the Power Control Register couldn't cover all new cases, as
> why Host Control 2 register needed to be added. The Host Control 2
> register, is what sdhci_do_start_signal_voltage_switch() uses to
> change the signal voltage level, which all makes sense to me.
> 
> For sdhci_set_power(); it seems to use the Power Control Register to
> control the power to the card (VDD/VMMC). Indeed this looks *really*
> weird/wrong. I wonder if it's working because of luck, intentional
> violation of the SDHCI spec or because of special variants.
> 

Yes but I don't know if we are doing something weird/wrong or if it
is the naming of the 'SD Bus Voltage Select' which is weird/wrong.

> Especially when looking into the case when you *don't* have a VMMC
> regulator several strange quirks exists in sdhci_set_power().
> 
> In the case when you *have* a VMMC, I think just setting/clearing bit
> 0 (SDHCI_POWER_ON) and then bail out, is probably working with modern
> HW because it's likely the only thing needed.
> 
> Now, this discussion was interesting, but I forgot what problem you
> actually where trying to solve? :-)

There is this discussion because of two things:
- Fixing something I consider as a bug: when I have a VMMC, only
  setting/clearing bit 0. Our controller strictly obeys to the spec and
  check the 'SD Bus Voltage Select' field. Since we put a reserved value
  (000), the Power On is not performed.
- I was trying to get help to understand what is this 'SD Bus Voltage'.
  For our controller and sdhci_set_power(), it seems to stand for VMMC.
  For me, everything concerning bus voltage is related to VQMMC, so I was
  disappointed.


Regards

Ludovic

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ludovic Desroches]

Hi Ulf,

On Mon, Nov 09, 2015 at 05:30:26PM +0100, Ludovic Desroches wrote:
> On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote:

[...]

> > Now, this discussion was interesting, but I forgot what problem you
> > actually where trying to solve? :-)
> 
> There is this discussion because of two things:
> - Fixing something I consider as a bug: when I have a VMMC, only
>   setting/clearing bit 0. Our controller strictly obeys to the spec and
>   check the 'SD Bus Voltage Select' field. Since we put a reserved value
>   (000), the Power On is not performed.
> - I was trying to get help to understand what is this 'SD Bus Voltage'.
>   For our controller and sdhci_set_power(), it seems to stand for VMMC.
>   For me, everything concerning bus voltage is related to VQMMC, so I was
>   disappointed.

Do you plan to take the patch for VMMC? If yes, I will send a new patch
for the device tree (I'll only add vmmc, not vqmmc as discussed); if
not, forget these two patches.

Regards

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ulf Hansson]

On 24 November 2015 at 10:23, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> Hi Ulf,
>
> On Mon, Nov 09, 2015 at 05:30:26PM +0100, Ludovic Desroches wrote:
>> On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote:
>
> [...]
>
>> > Now, this discussion was interesting, but I forgot what problem you
>> > actually where trying to solve? :-)
>>
>> There is this discussion because of two things:
>> - Fixing something I consider as a bug: when I have a VMMC, only
>>   setting/clearing bit 0. Our controller strictly obeys to the spec and
>>   check the 'SD Bus Voltage Select' field. Since we put a reserved value
>>   (000), the Power On is not performed.
>> - I was trying to get help to understand what is this 'SD Bus Voltage'.
>>   For our controller and sdhci_set_power(), it seems to stand for VMMC.
>>   For me, everything concerning bus voltage is related to VQMMC, so I was
>>   disappointed.
>
> Do you plan to take the patch for VMMC? If yes, I will send a new patch
> for the device tree (I'll only add vmmc, not vqmmc as discussed); if
> not, forget these two patches.
>

Which patch do you refer to for "VMMC"?

Kind regards
Uffe

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ludovic Desroches]

On Tue, Nov 24, 2015 at 12:01:53PM +0100, Ulf Hansson wrote:
> On 24 November 2015 at 10:23, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > Hi Ulf,
> >
> > On Mon, Nov 09, 2015 at 05:30:26PM +0100, Ludovic Desroches wrote:
> >> On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote:
> >
> > [...]
> >
> >> > Now, this discussion was interesting, but I forgot what problem you
> >> > actually where trying to solve? :-)
> >>
> >> There is this discussion because of two things:
> >> - Fixing something I consider as a bug: when I have a VMMC, only
> >>   setting/clearing bit 0. Our controller strictly obeys to the spec and
> >>   check the 'SD Bus Voltage Select' field. Since we put a reserved value
> >>   (000), the Power On is not performed.
> >> - I was trying to get help to understand what is this 'SD Bus Voltage'.
> >>   For our controller and sdhci_set_power(), it seems to stand for VMMC.
> >>   For me, everything concerning bus voltage is related to VQMMC, so I was
> >>   disappointed.
> >
> > Do you plan to take the patch for VMMC? If yes, I will send a new patch
> > for the device tree (I'll only add vmmc, not vqmmc as discussed); if
> > not, forget these two patches.
> >
> 
> Which patch do you refer to for "VMMC"?
> 

This one to not write an invalid voltage in the power control register
even if we have an external regulator for vmmc.

Regards

Ludovic

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ulf Hansson]

On 24 November 2015 at 14:12, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> On Tue, Nov 24, 2015 at 12:01:53PM +0100, Ulf Hansson wrote:
>> On 24 November 2015 at 10:23, Ludovic Desroches
>> <ludovic.desroches@atmel.com> wrote:
>> > Hi Ulf,
>> >
>> > On Mon, Nov 09, 2015 at 05:30:26PM +0100, Ludovic Desroches wrote:
>> >> On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote:
>> >
>> > [...]
>> >
>> >> > Now, this discussion was interesting, but I forgot what problem you
>> >> > actually where trying to solve? :-)
>> >>
>> >> There is this discussion because of two things:
>> >> - Fixing something I consider as a bug: when I have a VMMC, only
>> >>   setting/clearing bit 0. Our controller strictly obeys to the spec and
>> >>   check the 'SD Bus Voltage Select' field. Since we put a reserved value
>> >>   (000), the Power On is not performed.
>> >> - I was trying to get help to understand what is this 'SD Bus Voltage'.
>> >>   For our controller and sdhci_set_power(), it seems to stand for VMMC.
>> >>   For me, everything concerning bus voltage is related to VQMMC, so I was
>> >>   disappointed.
>> >
>> > Do you plan to take the patch for VMMC? If yes, I will send a new patch
>> > for the device tree (I'll only add vmmc, not vqmmc as discussed); if
>> > not, forget these two patches.
>> >
>>
>> Which patch do you refer to for "VMMC"?
>>
>
> This one to not write an invalid voltage in the power control register
> even if we have an external regulator for vmmc.
>

As I stated earlier, according to the SDHCI spec in the section for
the Power Control Register. Bit 0 needs to be set when communicating
with the card as it will for
example enable the clock.

I suspect if I apply your patch several sdhci variants would break,
don't you think?

Kind regards
Uffe

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ludovic Desroches]

On Tue, Nov 24, 2015 at 02:56:21PM +0100, Ulf Hansson wrote:
> On 24 November 2015 at 14:12, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > On Tue, Nov 24, 2015 at 12:01:53PM +0100, Ulf Hansson wrote:
> >> On 24 November 2015 at 10:23, Ludovic Desroches
> >> <ludovic.desroches@atmel.com> wrote:
> >> > Hi Ulf,
> >> >
> >> > On Mon, Nov 09, 2015 at 05:30:26PM +0100, Ludovic Desroches wrote:
> >> >> On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote:
> >> >
> >> > [...]
> >> >
> >> >> > Now, this discussion was interesting, but I forgot what problem you
> >> >> > actually where trying to solve? :-)
> >> >>
> >> >> There is this discussion because of two things:
> >> >> - Fixing something I consider as a bug: when I have a VMMC, only
> >> >>   setting/clearing bit 0. Our controller strictly obeys to the spec and
> >> >>   check the 'SD Bus Voltage Select' field. Since we put a reserved value
> >> >>   (000), the Power On is not performed.
> >> >> - I was trying to get help to understand what is this 'SD Bus Voltage'.
> >> >>   For our controller and sdhci_set_power(), it seems to stand for VMMC.
> >> >>   For me, everything concerning bus voltage is related to VQMMC, so I was
> >> >>   disappointed.
> >> >
> >> > Do you plan to take the patch for VMMC? If yes, I will send a new patch
> >> > for the device tree (I'll only add vmmc, not vqmmc as discussed); if
> >> > not, forget these two patches.
> >> >
> >>
> >> Which patch do you refer to for "VMMC"?
> >>
> >
> > This one to not write an invalid voltage in the power control register
> > even if we have an external regulator for vmmc.
> >
> 
> As I stated earlier, according to the SDHCI spec in the section for
> the Power Control Register. Bit 0 needs to be set when communicating
> with the card as it will for
> example enable the clock.
> 

I am okay with bit 0. I don't want to change this part, it will be done
later in sdhci_set_power(). My concern is only about bit 3-1, I want to
go through the switch statement.

> I suspect if I apply your patch several sdhci variants would break,
> don't you think?

I wouldn't sign it with my blood but I don't think so. It seems they
don't care about the SD bus Voltage since they work with an unsupported
voltage.

Regards

Ludovic

Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power

[Ulf Hansson]

[...]

>> > This one to not write an invalid voltage in the power control register
>> > even if we have an external regulator for vmmc.
>> >
>>
>> As I stated earlier, according to the SDHCI spec in the section for
>> the Power Control Register. Bit 0 needs to be set when communicating
>> with the card as it will for
>> example enable the clock.
>>
>
> I am okay with bit 0. I don't want to change this part, it will be done
> later in sdhci_set_power(). My concern is only about bit 3-1, I want to
> go through the switch statement.

For those variants that have a VMMC and don't care about the other
bits (1->3), it means executing code that isn't needed.

Instead, as I have been telling people several times by now, let's
convert the "sdhci core" into a library, so each variant can pick and
do what suite them best.

>
>> I suspect if I apply your patch several sdhci variants would break,
>> don't you think?
>
> I wouldn't sign it with my blood but I don't think so. It seems they
> don't care about the SD bus Voltage since they work with an unsupported
> voltage.

You may very well be right, that it doesn't break anything. But in
this case I really don't want to take the risk.

As stated above, the proper solution would be that sdhci_set_power()
should be split up in smaller pieces, where each piece may become a
library function. Each host variant can then decide what to use.

Future wise, that would mean when changing a library function, it will
affect the subset of the sdhci variants that actually use it and not
*all* sdhci variants. Moreover it will lead to optimized code.

Kind regards
Uffe

Re: [PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ulf Hansson]

On 6 November 2015 at 16:59, Ludovic Desroches
<ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> Hi,
>
> I would like to have some feedback for these two patches. I have two questions.
>
> 1.
>
> To suppress the warnings telling me I have no vmmc and vqmmc supplies, I have
> added them and discovered that adding a vmmc supply involve only writing 1 in
> the Power Control Register involves enabling the SD Bus Power with a non valid
> value for the SD Bus Voltage. If the host controllers strictly follows the
> specification, it shall not enable SD Bus Power. So enough if it seems useless
> to configure SD Bus Voltage because we have an external regulator, do it.

I can't give you a detailed answer about the sdhci HW as I only have
limited knowledge.

What I can say is that people have been trying to fix all kind of
crazy corner cases by adding quirks and callbacks. This seems like yet
another one.

So, by turning sdhci into a set of library functions you could easier
pick and decide to what suites your particular variant. In this case
it seems like would have relied on using the external regulators to
control voltages, instead of some internal sdhci registers.

>
> By the way, I am curious to understand what is really the SD Bus Voltage. I
> mean talking about bus voltage makes me thinking more about vqmmc than vmmc.
> Is it only bad naming or do I miss something?
>
> From the specification, there is this figure:
>
>                     HOST
> ---------------------------------------------------
> | ------------        3.3V                        |       VDD
> | | Power SW |------------------------------------|----------
> | ------------   |              |                 |
> |       ---------|              |                 |
> |       |                       |                 |
> | --------------     ----------------------       |
> | | Ref. Volt. |-----| Regulator/Selector |       |
> | --------------     ----------------------       |
> |                    3.3V/1.8V  |                 |
> |                               |                 |
> |                               |---------------  |
> |                               |       |  |   |  |
> |                               |       R  R   _  |
> |                               |       |  |   _  |
> |                               |       |  |   |  |
> |                               |       |  |  /// |
> |                            -------    |  |      |       CLK
> |                            |     |----)--)------|----------
> |  -----------------------   |Multi|    |  |      |       CMD
> |  |Random Logic Circuits|---|Drive|----)---------|----------
> |  -----------------------   |I/O  |    |         |       DAT
> |                            |     |--------------|----------
> |                            -------              |
> ---------------------------------------------------
>
> In my mind vmmc is the 3.3V signal and vqmmc is the 3.3V/1.8V signal, so why
> talking about bus voltage?

"IO voltage", "bus voltage", "VQMMC", etc they all mean the same thing
to me. It's the voltage level of the signals going to the card.

>
>
> 2.
>
> Is the regulator-gpio usage the right thing to do for vqmmc? In my case it is
> not really driven by a gpio but by a pio from the sdhci device. In the binding,

What's a "pio"?

What do you mean by the it's driven from the sdhci device?

Is it the internal HW logic of the sdhci controller that manages the
IO voltage? And this logic can be controlled via certain register bits
in the SDHCI controller?

> declaring the gpio is an option so I thought using this regulator fits my need.

In quite many cases it makes sense to model this though a gpio
regulator. For example when you use a level shifter circuit. Those
normally have gpio pin routed to control the voltage level output for
the signals. For example switching between 1.8V and 2.9V.

[...]

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ludovic Desroches]

Hi Ulf,

Thanks for your answer.

On Fri, Nov 06, 2015 at 05:42:51PM +0100, Ulf Hansson wrote:
> On 6 November 2015 at 16:59, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > Hi,
> >
> > I would like to have some feedback for these two patches. I have two questions.
> >
> > 1.
> >
> > To suppress the warnings telling me I have no vmmc and vqmmc supplies, I have
> > added them and discovered that adding a vmmc supply involve only writing 1 in
> > the Power Control Register involves enabling the SD Bus Power with a non valid
> > value for the SD Bus Voltage. If the host controllers strictly follows the
> > specification, it shall not enable SD Bus Power. So enough if it seems useless
> > to configure SD Bus Voltage because we have an external regulator, do it.
> 
> I can't give you a detailed answer about the sdhci HW as I only have
> limited knowledge.
> 
> What I can say is that people have been trying to fix all kind of
> crazy corner cases by adding quirks and callbacks. This seems like yet
> another one.
> 

I know your point of view about it and I won't try to add another quirk
:).

IMO, it's not a quirk since it is mention in the specification that
the SD Host Driver shall set the SD Bus Voltage Select.

> So, by turning sdhci into a set of library functions you could easier
> pick and decide to what suites your particular variant. In this case
> it seems like would have relied on using the external regulators to
> control voltages, instead of some internal sdhci registers.
>

Yes, this is it. Even if you are using an extarnal regulator, you still
have to set SD Bus Power because it drives the SDCLK. For that reason you
need to set a valid Bus Voltage.
 
> >
> > By the way, I am curious to understand what is really the SD Bus Voltage. I
> > mean talking about bus voltage makes me thinking more about vqmmc than vmmc.
> > Is it only bad naming or do I miss something?
> >
> > From the specification, there is this figure:
> >
> >                     HOST
> > ---------------------------------------------------
> > | ------------        3.3V                        |       VDD
> > | | Power SW |------------------------------------|----------
> > | ------------   |              |                 |
> > |       ---------|              |                 |
> > |       |                       |                 |
> > | --------------     ----------------------       |
> > | | Ref. Volt. |-----| Regulator/Selector |       |
> > | --------------     ----------------------       |
> > |                    3.3V/1.8V  |                 |
> > |                               |                 |
> > |                               |---------------  |
> > |                               |       |  |   |  |
> > |                               |       R  R   _  |
> > |                               |       |  |   _  |
> > |                               |       |  |   |  |
> > |                               |       |  |  /// |
> > |                            -------    |  |      |       CLK
> > |                            |     |----)--)------|----------
> > |  -----------------------   |Multi|    |  |      |       CMD
> > |  |Random Logic Circuits|---|Drive|----)---------|----------
> > |  -----------------------   |I/O  |    |         |       DAT
> > |                            |     |--------------|----------
> > |                            -------              |
> > ---------------------------------------------------
> >
> > In my mind vmmc is the 3.3V signal and vqmmc is the 3.3V/1.8V signal, so why
> > talking about bus voltage?
> 
> "IO voltage", "bus voltage", "VQMMC", etc they all mean the same thing
> to me. It's the voltage level of the signals going to the card.
> 

I agree, I share your understanding so. I am disappointed about managing
VMMC instead of VQMMC in a function which should deal with the Bus
Voltage value.

> >
> >
> > 2.
> >
> > Is the regulator-gpio usage the right thing to do for vqmmc? In my case it is
> > not really driven by a gpio but by a pio from the sdhci device. In the binding,
> 
> What's a "pio"?
> 
> What do you mean by the it's driven from the sdhci device?
> 

Sorry I mean sdhci device from the SoC point of view, I should say
controller. So yes the signal is driven by the controller. 

> Is it the internal HW logic of the sdhci controller that manages the
> IO voltage? And this logic can be controlled via certain register bits
> in the SDHCI controller?
> 

Yes, it depends of the value of the '1.8V Signaling Enable' value in the
host control 2 register.

> > declaring the gpio is an option so I thought using this regulator fits my need.
> 
> In quite many cases it makes sense to model this though a gpio
> regulator. For example when you use a level shifter circuit. Those
> normally have gpio pin routed to control the voltage level output for
> the signals. For example switching between 1.8V and 2.9V.
> 

I agree, my concern is to know if I can consider it as a 'general' pio
since it is driven by the sdhci controller.


Regards

Ludovic

Re: [PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ulf Hansson]

[...]

>> > Is the regulator-gpio usage the right thing to do for vqmmc? In my case it is
>> > not really driven by a gpio but by a pio from the sdhci device. In the binding,
>>
>> What's a "pio"?
>>
>> What do you mean by the it's driven from the sdhci device?
>>
>
> Sorry I mean sdhci device from the SoC point of view, I should say
> controller. So yes the signal is driven by the controller.
>
>> Is it the internal HW logic of the sdhci controller that manages the
>> IO voltage? And this logic can be controlled via certain register bits
>> in the SDHCI controller?
>>
>
> Yes, it depends of the value of the '1.8V Signaling Enable' value in the
> host control 2 register.
>
>> > declaring the gpio is an option so I thought using this regulator fits my need.
>>
>> In quite many cases it makes sense to model this though a gpio
>> regulator. For example when you use a level shifter circuit. Those
>> normally have gpio pin routed to control the voltage level output for
>> the signals. For example switching between 1.8V and 2.9V.
>>
>
> I agree, my concern is to know if I can consider it as a 'general' pio
> since it is driven by the sdhci controller.

This doesn't seems like a case where a gpio regulator should be used
and I am not sure what problem it would solve. Beside to suppress the
log warnings (actually those aren't warnings but informations).

Isn't sdhci_do_start_signal_voltage_switch() doing what you need here?

Kind regards
Uffe

Re: [PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ludovic Desroches]

On Mon, Nov 09, 2015 at 10:38:03AM +0100, Ulf Hansson wrote:
> [...]
> 
> >> > Is the regulator-gpio usage the right thing to do for vqmmc? In my case it is
> >> > not really driven by a gpio but by a pio from the sdhci device. In the binding,
> >>
> >> What's a "pio"?
> >>
> >> What do you mean by the it's driven from the sdhci device?
> >>
> >
> > Sorry I mean sdhci device from the SoC point of view, I should say
> > controller. So yes the signal is driven by the controller.
> >
> >> Is it the internal HW logic of the sdhci controller that manages the
> >> IO voltage? And this logic can be controlled via certain register bits
> >> in the SDHCI controller?
> >>
> >
> > Yes, it depends of the value of the '1.8V Signaling Enable' value in the
> > host control 2 register.
> >
> >> > declaring the gpio is an option so I thought using this regulator fits my need.
> >>
> >> In quite many cases it makes sense to model this though a gpio
> >> regulator. For example when you use a level shifter circuit. Those
> >> normally have gpio pin routed to control the voltage level output for
> >> the signals. For example switching between 1.8V and 2.9V.
> >>
> >
> > I agree, my concern is to know if I can consider it as a 'general' pio
> > since it is driven by the sdhci controller.
> 
> This doesn't seems like a case where a gpio regulator should be used
> and I am not sure what problem it would solve. Beside to suppress the
> log warnings (actually those aren't warnings but informations).
> 
> Isn't sdhci_do_start_signal_voltage_switch() doing what you need here?
> 

It is. I am only wondering the best way to describe the hardware:
- No regulator but I have the 'no vqmmc regulator not found' message which
  is a bit annoying and which can be interpreted as an issue for someone
  who has no knowledge about this stuff.
- Describe the regulator since there is one on my board. But it is not a
  fixed regulator and even if it's close to a gpio one it is not.

Regards

Ludovic

Re: [PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ulf Hansson]

[...]

>>
>> This doesn't seems like a case where a gpio regulator should be used
>> and I am not sure what problem it would solve. Beside to suppress the
>> log warnings (actually those aren't warnings but informations).
>>
>> Isn't sdhci_do_start_signal_voltage_switch() doing what you need here?
>>
>
> It is. I am only wondering the best way to describe the hardware:
> - No regulator but I have the 'no vqmmc regulator not found' message which
>   is a bit annoying and which can be interpreted as an issue for someone
>   who has no knowledge about this stuff.

Hmm, should we turn them into debug messages? Both regulators are optional.

> - Describe the regulator since there is one on my board. But it is not a
>   fixed regulator and even if it's close to a gpio one it is not.

If it's driven by SDHCI internal logic, I would leave it to that.
There are no need to describe it at all.

Kind regards
Uffe

Re: [PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ludovic Desroches]

On Mon, Nov 09, 2015 at 11:50:50AM +0100, Ulf Hansson wrote:
> [...]
> 
> >>
> >> This doesn't seems like a case where a gpio regulator should be used
> >> and I am not sure what problem it would solve. Beside to suppress the
> >> log warnings (actually those aren't warnings but informations).
> >>
> >> Isn't sdhci_do_start_signal_voltage_switch() doing what you need here?
> >>
> >
> > It is. I am only wondering the best way to describe the hardware:
> > - No regulator but I have the 'no vqmmc regulator not found' message which
> >   is a bit annoying and which can be interpreted as an issue for someone
> >   who has no knowledge about this stuff.
> 
> Hmm, should we turn them into debug messages? Both regulators are optional.
> 

I think so. From my point of view, it is useless to indicate that
something optionnal is missing.

> > - Describe the regulator since there is one on my board. But it is not a
> >   fixed regulator and even if it's close to a gpio one it is not.
> 
> If it's driven by SDHCI internal logic, I would leave it to that.
> There are no need to describe it at all.

Ok thanks for your help.

> 
> Kind regards
> Uffe

Re: [PATCH 0/2] mmc: sdhci: potentially bad behavior when using vmmc supply

[Ulf Hansson]

On 9 November 2015 at 14:14, Ludovic Desroches
<ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, Nov 09, 2015 at 11:50:50AM +0100, Ulf Hansson wrote:
>> [...]
>>
>> >>
>> >> This doesn't seems like a case where a gpio regulator should be used
>> >> and I am not sure what problem it would solve. Beside to suppress the
>> >> log warnings (actually those aren't warnings but informations).
>> >>
>> >> Isn't sdhci_do_start_signal_voltage_switch() doing what you need here?
>> >>
>> >
>> > It is. I am only wondering the best way to describe the hardware:
>> > - No regulator but I have the 'no vqmmc regulator not found' message which
>> >   is a bit annoying and which can be interpreted as an issue for someone
>> >   who has no knowledge about this stuff.
>>
>> Hmm, should we turn them into debug messages? Both regulators are optional.
>>
>
> I think so. From my point of view, it is useless to indicate that
> something optionnal is missing.

Okay. Please go ahead and send a patch.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ARM: at91/dt: sama5d2 Xplained: add regulators for sdhci devices

[Ludovic Desroches]

Add vmmc and vqmmc regulators for sdhci devices.

The voltage for sdmmc0 vqmmc is selected by a signal from coming from
the sdhci device.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index 361cbf8..1ca4942 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -96,6 +96,8 @@
 			bus-width = <8>;
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_sdmmc0_default>;
+			vmmc-supply = <&vdd_3v3_reg>;
+			vqmmc-supply = <&vdd_sdhc_reg>;
 			non-removable;
 			mmc-ddr-1_8v;
 			status = "okay";
@@ -105,6 +107,8 @@
 			bus-width = <4>;
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_sdmmc1_default>;
+			vmmc-supply = <&vdd_3v3_reg>;
+			vqmmc-supply = <&vdd_3v3_reg>;
 			status = "okay"; /* conflict with qspi0 */
 		};
 
@@ -355,4 +359,14 @@
 			};
 		};
 	};
+
+	vdd_sdhc_reg: regulator@0 {
+		regulator-name = "VDD_SDHC";
+		vin_1v8-supply = <&vdd_sdhc_1v8_reg>;
+		vin_3v3-supply = <&vdd_3v3_reg>;
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <3300000>;
+		states = <1800000 0x1
+			  3300000 0x0>;
+	};
 };
-- 
2.5.0