Hi Shawn,
On 09/02/2016 05:50 PM, Shawn Lin wrote:
> Hi Jaehoon,
>
> On 2016/8/31 18:48, Jaehoon Chung wrote:
>> On 08/31/2016 04:13 PM, Shawn Lin wrote:
>>> On 2016/8/31 13:51, Jaehoon Chung wrote:
>>>> HS400 mode doesn't need to do execute_tuning, because it's already
>>>> tuned in HS200 mode. And Tuning command is optional for UHS50 mode.
>>>>
>>>> In future, the general execute_tuning sequence can be included in this
>>>> function.
>>>
>>> Maybe this isn't what we want for mmc.
>>>
>>> I see both of sd and sdio call mmc_execute_tuning by checking
>>> the ios.timing, and explicitly we need to do tuning for
>>> MMC_TIMING_UHS_SDR50 as the mmc core asks we to do that.
>>>
>>> Moreover, why mmc didn't do something like:
>>>
>>> if (card->host->ios.timing == MMC_TIMING_XXXXX || ....)
>>> mmc_execute_tuning()
>>>
>>> So we don't need every host driver to add these check and I do
>>> see some host drivers check these timing for their execute_tuning
>>> callback..
>>>
>>> What is your opinion? :)
>>
>> Yep, your comment makes sense..So i'm checking SD specification.
>> I think it needs to know which UHS card is..(UHS50 or UHS104)..
>> But current kernel can't distinguish these..
>>
>> According to spec, we can distinguish UHS50 and UHS104 with TRAN_SPEED field.
>>
>> "UHS50 Card set TRANS_SPEED to 0Bh (100Mbit/sec), for both SDR50 and DDR50 modes.
>> UHS104 Card set TRAN_SPEED to 2Bh (200Mbit/sec)"
>>
>> I'm not sure it's helpful to me...but i will consider more...
>
> Ah, are we on the same page? :)
> I have some questions below.
>
>>
>> Thanks you for comment. :)
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>>>
>>>> Signed-off-by: Jaehoon Chung <jh80.chung@xxxxxxxxxxx>
>>>> ---
>>>> drivers/mmc/host/dw_mmc.c | 17 ++++++++++++++++-
>>>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index 22dacae..6571924 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -1568,10 +1568,25 @@ static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>>> struct dw_mci_slot *slot = mmc_priv(mmc);
>>>> struct dw_mci *host = slot->host;
>>>> const struct dw_mci_drv_data *drv_data = host->drv_data;
>>>> - int err = -EINVAL;
>>>> + int err = 0;
>>>> +
>>>> + switch (host->timing) {
>>>> + case MMC_TIMING_MMC_HS400:
>>>> + err = -EINVAL;
>
> What makes you think the code could be there? :)
> By looking the code, it's impossible, no?
>
>>>> + goto out;
>>>> + case MMC_TIMING_MMC_HS200:
>>>> + case MMC_TIMING_UHS_SDR104:
>>>> + case MMC_TIMING_UHS_DDR50:
>>>> + break;
>>>> + case MMC_TIMING_UHS_SDR50:
>>>> + /* Fall through */
>
> dw_mmc even doesn't try to do tuning for SDR50?
Yep, it's not correct.
>
>
>>>> + default:
>>>> + goto out;
>>>> + }
>>>>
>
> That confused me more or less. What makes us gain confidence
> that we could return 0 even if no any tuning process was done
> before using UHS-I.
I have dropped my RFC patch, because your previous comment is reasonable.
Dwmmc controller doesn't need to check which mode is used at tuning time.
(Already checked them on core side.)
It's differed to SDHCI controller. sdhci controller has the need_to_tuning_sdr50 bit at some register.
Best Regards,
Jaehoon Chung
>
>
>>>> if (drv_data && drv_data->execute_tuning)
>>>> err = drv_data->execute_tuning(slot, opcode);
>>>> +out:
>>>> return err;
>>>> }
>>>>
>>>>
>>>
>>>
>>
>>
>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
