On 15 January 2015 at 15:59, Arend van Spriel <ar...@broadcom.com> wrote: > On 01/15/15 15:46, Ulf Hansson wrote: >> >> On 15 January 2015 at 15:17, Arend van Spriel<ar...@broadcom.com> wrote: >>> >>> On 01/15/15 15:07, Arend van Spriel wrote: >>>> >>>> >>>> On 01/15/15 14:39, Ulf Hansson wrote: >>>>> >>>>> >>>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hun...@intel.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> On 14/01/15 14:59, Ulf Hansson wrote: >>>>>>> >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>>> >>>>>>>>> The value from the register is also just randomly selected, only >>>>>>>>> difference is that it's the HW that has randomly set it. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Presumably the value is chosen based on the maximum rate of >>>>>>>> temperature >>>>>>>> change and the corresponding effect that has on the signal. >>>>>>>> >>>>>>>>> >>>>>>>>> Even if the above commit was merged, I don't think it was the >>>>>>>>> correct >>>>>>>>> way of dealing with re-tuning. >>>>>>>>> >>>>>>>>> First of all, re-tuning this is a mmc protocol specific thing >>>>>>>>> should >>>>>>>>> be managed from the mmc core, like the approach you have taken in >>>>>>>>> your >>>>>>>>> $subject patchset. Second I question whether the timer is useful at >>>>>>>>> all. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> The SD Host Controller Specification does not document another way >>>>>>>> to do >>>>>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never >>>>>>>> done. >>>>>>>> >>>>>>>> In the patches I sent, the driver must call mmc_retune_needed() to >>>>>>>> set >>>>>>>> host->need_retune = 1 otherwise mmc_retune() does nothing. >>>>>>>> >>>>>>>> I would like to extend the model to include transparently re-tuning >>>>>>>> and >>>>>>>> re-trying when there is a CRC error, but that is a separate issue, >>>>>>>> not >>>>>>>> documented in the spec but recommended by others. >>>>>>>> >>>>>>> >>>>>>> That perfect and in line from what I heard as recommendations from >>>>>>> memory vendors as well. >>>>>> >>>>>> >>>>>> >>>>>> How would that work for SDIO? How do you know it is OK to retry SDIO >>>>>> operations? >>>>> >>>>> >>>>> >>>>> Retries or error handling, needs to be handled from SDIO func drivers >>>>> or upper level code. They certainly also need it for other errors, >>>>> which are not caused by the lack of a re-tune. I believe they exist >>>>> already. >>>>> >>>>> For mmc core point of view, we need to act on SDIO data transfers >>>>> errors and perform re-tuning for cases when it makes sense. >>>>> >>>>> More importantly, using a timer won't make SDIO data transfers error >>>>> free, since we can still end up needing a re-tune at any point. >>>>> >>>>> Still, you do have point for SDIO. Minimizing the number of errors for >>>>> SDIO could be important, due to that an SDIO func driver may not be >>>>> able to recover from data errors as smoothly as the mmc block layer >>>>> can. Thus, a timer could help to improve the situation, but I think it >>>>> only makes sense in the SDIO case. >>>>> >>>>> BTW, what's your experience around SDIO cards supporting SDR104. I >>>>> have never used such, have you? >>>> >>>> >>>> >>>> My primary focus in all this discussing is about SDIO cards. The main >>>> reason being that our 11ac wifi SDIO cards do support SDR104. So the >>>> brcmfmac driver support SDIO and has retry mechanisms in place. However, >>>> it may also end-up doing an abort under certain conditions. >>>> >>>> You also mentioned using runtime-pm, but how do you deal with func >>>> drivers not supporting runtime-pm. That is already an issue aka. bug >>>> right now. Our driver does not support runtime-pm (yet) and we have >>>> reported issues that host controller does runtime-pm basically killing >>>> communication between device and func driver. >>> >>> >> >> Runtime PM is implemented a bit differently between SDIO vs MMC/SD. >> Your are right. >> >> For MMC/SD the mmc block device handles pm_runtime_get|put() in >> principle per request basis and makes use of the >> pm_runtime_autosuspend feature. While in the SDIO case, it's entirely >> up the SDIO func driver to deal with pm_runtime_get|put(). >> >> So it seems like we can use runtime PM for MMC/SD but not for SDIO. At >> least not using the SDIO func device. >> >>> >>> Could leave it to the function driver to call mmc_retune_needed(). >> >> >> Hmm, the positive side from such approach would be that the SDIO func >> driver can decide when it's convenient to do a re-tune. > > > I would say "appropriate" instead of "convenient". > >> The negative side is that all SDIO func driver would need to care >> about this. I am not sure we want that. > > > The whole retry handling also seems deferred to the SDIO func driver and the > same for runtime-pm. As the "retune needed" question would pops up during > the retry handling it seems not a bad option.
Okay, your argument seems reasonable, let's got for this approach. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
