diff mbox

[v6,11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

Message ID 1350471893-29633-12-git-send-email-keyuan.liu@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Liu Oct. 17, 2012, 11:04 a.m. UTC 
From: Kevin Liu <kliu5@marvell.com>

Host controller must enable 1.8v signal for UHS modes. Otherwise
UHS modes won't take effect. But mmc core does NOT switch to 1.8v
for DDR50 mode. So enable the 1.8v signal for mmc DDR50 mode in host
driver.

In JEDEC spec, there are two emmc device types which support DDR50.
One type can work under signal 1.2v while the other type work under
signal 1.8v or 3v. So current code just keep 3v for the second device
type.

But in SD host spec, 1.8v signal must be enabled for all UHS modes
taking effect. So the fact is, if using SD host to work with emmc chip,
then 1.8v signal must be selected in order to enable DDR50.
Current code missed this.

Because DDR50 shall can work under both signal 1.8v and 3v according
to JEDEC spec,  So should not switch to 1.8v in mmc core code.
It's the host controller requirement that 1.8v signal must be enabled
in order to enable DDR50, which conflict with emmc spec. So add this
in host driver.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

Comments

Chris Ball Nov. 17, 2012, 10:35 p.m. UTC | #1 
Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c3e786d..522e501 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1529,8 +1529,15 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>  				ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
>  			else if (ios->timing == MMC_TIMING_UHS_SDR104)
>  				ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
> -			else if (ios->timing == MMC_TIMING_UHS_DDR50)
> +			else if (ios->timing == MMC_TIMING_UHS_DDR50) {
> +				struct mmc_card	*card;
> +
>  				ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
> +				card = container_of(&(host->mmc),
> +					struct mmc_card, host);
> +				if (mmc_card_mmc(card))
> +					ctrl_2 |= SDHCI_CTRL_VDD_180;
> +			}
>  			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>  		}
>  		if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&

I'm using DDR50 on an eMMC that's only powered by 3.3V (no 1.8V
available) with sdhci-pxav3, so it sounds like I don't want to merge
this patch?

Thanks,

- Chris.
Kevin Liu Nov. 19, 2012, 3:14 a.m. UTC | #2 
2012/11/18 Chris Ball <cjb@laptop.org>:
> Hi,
>
> On Wed, Oct 17 2012, Kevin Liu wrote:
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index c3e786d..522e501 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1529,8 +1529,15 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
>>                       else if (ios->timing == MMC_TIMING_UHS_SDR104)
>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
>> -                     else if (ios->timing == MMC_TIMING_UHS_DDR50)
>> +                     else if (ios->timing == MMC_TIMING_UHS_DDR50) {
>> +                             struct mmc_card *card;
>> +
>>                               ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>> +                             card = container_of(&(host->mmc),
>> +                                     struct mmc_card, host);
>> +                             if (mmc_card_mmc(card))
>> +                                     ctrl_2 |= SDHCI_CTRL_VDD_180;
>> +                     }
>>                       sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>>               }
>>               if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&
>
> I'm using DDR50 on an eMMC that's only powered by 3.3V (no 1.8V
> available) with sdhci-pxav3, so it sounds like I don't want to merge
> this patch?
>
This patch is NEEDED for both 3.3v and 1.8v signaling.
In the SD host spec for host control 2 register, 1.8v signaling enable
bit must be set in order for UHS-I mode taking effect. Otherwise, the
DDR50 mode won't take effect on host even it is selected.
It's the SD host requirement.

Thanks
Kevin
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Liu Nov. 19, 2012, 8:57 a.m. UTC | #3 
2012/11/19 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/11/18 Chris Ball <cjb@laptop.org>:
>> Hi,
>>
>> On Wed, Oct 17 2012, Kevin Liu wrote:
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index c3e786d..522e501 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1529,8 +1529,15 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
>>>                       else if (ios->timing == MMC_TIMING_UHS_SDR104)
>>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
>>> -                     else if (ios->timing == MMC_TIMING_UHS_DDR50)
>>> +                     else if (ios->timing == MMC_TIMING_UHS_DDR50) {
>>> +                             struct mmc_card *card;
>>> +
>>>                               ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>>> +                             card = container_of(&(host->mmc),
>>> +                                     struct mmc_card, host);
>>> +                             if (mmc_card_mmc(card))
>>> +                                     ctrl_2 |= SDHCI_CTRL_VDD_180;
>>> +                     }
>>>                       sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>>>               }
>>>               if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&
>>
>> I'm using DDR50 on an eMMC that's only powered by 3.3V (no 1.8V
>> available) with sdhci-pxav3, so it sounds like I don't want to merge
>> this patch?
>>
> This patch is NEEDED for both 3.3v and 1.8v signaling.
> In the SD host spec for host control 2 register, 1.8v signaling enable
> bit must be set in order for UHS-I mode taking effect. Otherwise, the
> DDR50 mode won't take effect on host even it is selected.
> It's the SD host requirement.

In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
with SD host.
You must enable 1.8v signaling on host for UHS-I modes, but you set
3.3v for emmc vccq. It's conflictable.
The only way for emmc DDR50 to work is to set 1.8v for vccq although
JEDEC spec said both 1.8v and 3.3v are ok.

Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Ball Dec. 7, 2012, 7:10 p.m. UTC | #4 
Hi,

On Mon, Nov 19 2012, Kevin Liu wrote:
> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
> with SD host.

I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
eMMC, so it looks like this isn't true.

- Chris.
Kevin Liu Dec. 8, 2012, 1:54 a.m. UTC | #5 
2012/12/8 Chris Ball <cjb@laptop.org>:
> Hi,
>
> On Mon, Nov 19 2012, Kevin Liu wrote:
>> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
>> with SD host.
>
> I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
> eMMC, so it looks like this isn't true.
>

Chris,

Thanks for the check.
If you are using sdhci-pxav3.c, 3.3v vccq can work since it enabled
the 1.8v signaling in the callback function set_uhs_signaling.
What I want to say is if you don't enable the 1.8v signaling bit while
using 3.3v vccq, DDR50 on emmc should NOT work. (Use default sdhci
code without implementing set_uhs_signaling as the other drivers.)
You can just remove the set_uhs_signaling in sdhci-pxav3.c and try again.
So we should do this in sdhci.c by default and remove the callback
function, which only aim to enabling the 1.8v signaling bit.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Liu Dec. 8, 2012, 3:14 a.m. UTC | #6 
2012/12/8 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/12/8 Chris Ball <cjb@laptop.org>:
>> Hi,
>>
>> On Mon, Nov 19 2012, Kevin Liu wrote:
>>> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
>>> with SD host.
>>
>> I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
>> eMMC, so it looks like this isn't true.
>>
>
> Chris,
>
> Thanks for the check.
> If you are using sdhci-pxav3.c, 3.3v vccq can work since it enabled
> the 1.8v signaling in the callback function set_uhs_signaling.
> What I want to say is if you don't enable the 1.8v signaling bit while
> using 3.3v vccq, DDR50 on emmc should NOT work. (Use default sdhci
> code without implementing set_uhs_signaling as the other drivers.)
> You can just remove the set_uhs_signaling in sdhci-pxav3.c and try again.
> So we should do this in sdhci.c by default and remove the callback
> function, which only aim to enabling the 1.8v signaling bit.
>

Chris,

I would like to say more about my undersanding of 1.8v signaling bit
and UHS mode select. (Host Control 2 register, offset 0x3E<3:0>)
According to spec, when 1.8v signaling bit is set, the signaling
voltage will be changed from 3.3v to 1.8v. But we still provide 3.3v
vccq in current code. So originally I think it's conflictable in logic
and we must change to 1.8v vccq. But the fact is both 1.8v and 3.3v
vccq can work with 1.8v signaling bit set. The reason is below:
The host controller can't provide and totally control the card power by itself.
In actual implementation, the card power is supplied by external
regulator on board (vmmc and vmmcq) rather than host controller. So
for the 1.8v signaling bit, it will NOT impact signal voltage as spec
said either. It's just a flag but MUST be checked when UHS-I mode
selected. That's why we can enable 1.8v signaling bit while using 3.3v
vccq regulator for emmc DDR50 working. I confirmed this with mmp3 sdh
owner.

So we just enable the 1.8v signaling bit in host while keep 1.8v or
3.3v vccq regulator since emmc support DDR50 with both 1.8v and 3.3v
vccq. In other words, we make use of the incomplete feature of 1.8v
signaling bit (not matched with sd host spec that this bit should
control the signal voltage) to let both 3.3v and 1.8v signaling work
(well matched with JEDEC spec that DDR50 can work under both 1.8v and
3.3v vccq). I think it's good, otherwise only 1.8v can work which make
things complex and add limitation for board hardware.

So there are below conslusions:
1. If card power is supplied by external regulator, then setting 1.8v
signaling bit with both 1.8v and 3.3v vccq can work for emmc DDR50.
2. If host controller provide and totally control the card power, then
setting 1.8v signaling bit with only 1.8v vccq can work.
3. no matter which one of above two, 1.8v signaling bit MUST be set
for DDR50 working.
My patch assure item 3 in sdhci.c by default rather than adding
callback function to do this.

How do you think?

Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Liu Dec. 11, 2012, 10:48 a.m. UTC | #7 
2012/12/7 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/12/8 Kevin Liu <keyuan.liu@gmail.com>:
>> 2012/12/8 Chris Ball <cjb@laptop.org>:
>>> Hi,
>>>
>>> On Mon, Nov 19 2012, Kevin Liu wrote:
>>>> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
>>>> with SD host.
>>>
>>> I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
>>> eMMC, so it looks like this isn't true.
>>>
>>
>> Chris,
>>
>> Thanks for the check.
>> If you are using sdhci-pxav3.c, 3.3v vccq can work since it enabled
>> the 1.8v signaling in the callback function set_uhs_signaling.
>> What I want to say is if you don't enable the 1.8v signaling bit while
>> using 3.3v vccq, DDR50 on emmc should NOT work. (Use default sdhci
>> code without implementing set_uhs_signaling as the other drivers.)
>> You can just remove the set_uhs_signaling in sdhci-pxav3.c and try again.
>> So we should do this in sdhci.c by default and remove the callback
>> function, which only aim to enabling the 1.8v signaling bit.
>>
>
> Chris,
>
> I would like to say more about my undersanding of 1.8v signaling bit
> and UHS mode select. (Host Control 2 register, offset 0x3E<3:0>)
> According to spec, when 1.8v signaling bit is set, the signaling
> voltage will be changed from 3.3v to 1.8v. But we still provide 3.3v
> vccq in current code. So originally I think it's conflictable in logic
> and we must change to 1.8v vccq. But the fact is both 1.8v and 3.3v
> vccq can work with 1.8v signaling bit set. The reason is below:
> The host controller can't provide and totally control the card power by itself.
> In actual implementation, the card power is supplied by external
> regulator on board (vmmc and vmmcq) rather than host controller. So
> for the 1.8v signaling bit, it will NOT impact signal voltage as spec
> said either. It's just a flag but MUST be checked when UHS-I mode
> selected. That's why we can enable 1.8v signaling bit while using 3.3v
> vccq regulator for emmc DDR50 working. I confirmed this with mmp3 sdh
> owner.
>
> So we just enable the 1.8v signaling bit in host while keep 1.8v or
> 3.3v vccq regulator since emmc support DDR50 with both 1.8v and 3.3v
> vccq. In other words, we make use of the incomplete feature of 1.8v
> signaling bit (not matched with sd host spec that this bit should
> control the signal voltage) to let both 3.3v and 1.8v signaling work
> (well matched with JEDEC spec that DDR50 can work under both 1.8v and
> 3.3v vccq). I think it's good, otherwise only 1.8v can work which make
> things complex and add limitation for board hardware.
>
> So there are below conslusions:
> 1. If card power is supplied by external regulator, then setting 1.8v
> signaling bit with both 1.8v and 3.3v vccq can work for emmc DDR50.
> 2. If host controller provide and totally control the card power, then
> setting 1.8v signaling bit with only 1.8v vccq can work.
> 3. no matter which one of above two, 1.8v signaling bit MUST be set
> for DDR50 working.
> My patch assure item 3 in sdhci.c by default rather than adding
> callback function to do this.
>
> How do you think?
>
Chris,

Hope I'm clear with the 1.8v signaling setting.
So any comments for this patch or the other patches in the same patchset?

Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c3e786d..522e501 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1529,8 +1529,15 @@  static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 				ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
 			else if (ios->timing == MMC_TIMING_UHS_SDR104)
 				ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
-			else if (ios->timing == MMC_TIMING_UHS_DDR50)
+			else if (ios->timing == MMC_TIMING_UHS_DDR50) {
+				struct mmc_card	*card;
+
 				ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
+				card = container_of(&(host->mmc),
+					struct mmc_card, host);
+				if (mmc_card_mmc(card))
+					ctrl_2 |= SDHCI_CTRL_VDD_180;
+			}
 			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
 		}
 		if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&