diff mbox

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

Message ID 1446825570-30029-2-git-send-email-ludovic.desroches@atmel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ludovic Desroches Nov. 6, 2015, 3:59 p.m. UTC
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@atmel.com>
---
 drivers/mmc/host/sdhci.c | 7 -------
 1 file changed, 7 deletions(-)

Comments

Ludovic Desroches Nov. 9, 2015, 1:23 p.m. UTC | #1
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
>
Ulf Hansson Nov. 9, 2015, 2:12 p.m. UTC | #2
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?

> 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
Ludovic Desroches Nov. 9, 2015, 2:40 p.m. UTC | #3
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.


Regards

Ludovic
Ulf Hansson Nov. 9, 2015, 4 p.m. UTC | #4
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
Ludovic Desroches Nov. 9, 2015, 4:30 p.m. UTC | #5
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
Ludovic Desroches Nov. 24, 2015, 9:23 a.m. UTC | #6
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
Ulf Hansson Nov. 24, 2015, 11:01 a.m. UTC | #7
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
Ludovic Desroches Nov. 24, 2015, 1:12 p.m. UTC | #8
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
Ulf Hansson Nov. 24, 2015, 1:56 p.m. UTC | #9
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
Ludovic Desroches Nov. 24, 2015, 2:08 p.m. UTC | #10
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
Ulf Hansson Nov. 24, 2015, 3:17 p.m. UTC | #11
[...]

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

Patch

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