| Message ID | 1472622687-16389-1-git-send-email-jh80.chung@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? :) > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > --- > 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; > + 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 */ > + default: > + goto out; > + } > > if (drv_data && drv_data->execute_tuning) > err = drv_data->execute_tuning(slot, opcode); > +out: > return err; > } > >
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... Thanks you for comment. :) Best Regards, Jaehoon Chung > >> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >> --- >> 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; >> + 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 */ >> + default: >> + goto out; >> + } >> >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31 August 2016 at 12:48, Jaehoon Chung <jh80.chung@samsung.com> 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.. What about MMC_TIMING_UHS_SDR50 and MMC_TIMING_UHS_SDR104? Are you saying we need some more additional timing type(s)? > > 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)" > [...] Kind regards Uffe -- 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
On 08/31/2016 08:10 PM, Ulf Hansson wrote: > On 31 August 2016 at 12:48, Jaehoon Chung <jh80.chung@samsung.com> 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.. > > What about MMC_TIMING_UHS_SDR50 and MMC_TIMING_UHS_SDR104? Are you > saying we need some more additional timing type(s)? Not timing types,,SD card types.. SD card are UHS50 and UHS104. I'm not sure yet..i'm reading SD Physical Layer Specification in more detail.. But until now, it looks like my misunderstanding.. UHS104 card might be checked with Access mode field from switch Mode 1 operation. (in current kernel.) So I'm checking whether it's helpful or not for distinguishing card types..maybe not.. Best Regards, Jaehoon Chung > >> >> 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)" >> > > [...] > > Kind regards > Uffe > -- > 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 > > > -- 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
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@samsung.com> >>> --- >>> 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? >>> + 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. >>> if (drv_data && drv_data->execute_tuning) >>> err = drv_data->execute_tuning(slot, opcode); >>> +out: >>> return err; >>> } >>> >>> >> >> > > > >
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@samsung.com> >>>> --- >>>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + 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 */ + default: + goto out; + } if (drv_data && drv_data->execute_tuning) err = drv_data->execute_tuning(slot, opcode); +out: return err; }
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. Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> --- drivers/mmc/host/dw_mmc.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)