From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ulf Hansson Subject: Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning Date: Mon, 19 Jan 2015 10:27:57 +0100 Message-ID: References: <1417801271-15575-1-git-send-email-adrian.hunter@intel.com> <54B51C55.9060500@intel.com> <54B52D53.8080106@intel.com> <54B53592.9070303@broadcom.com> <54B54187.7080008@broadcom.com> <54B63D94.2050602@intel.com> <54B66015.1010507@intel.com> <54B793B2.8090100@intel.com> <54B7C9A7.1090803@broadcom.com> <54B7CBEB.9020404@broadcom.com> <54B7D5EB.4050508@broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-qg0-f52.google.com ([209.85.192.52]:47607 "EHLO mail-qg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751248AbbASJ16 (ORCPT ); Mon, 19 Jan 2015 04:27:58 -0500 Received: by mail-qg0-f52.google.com with SMTP id z107so2592521qgd.11 for ; Mon, 19 Jan 2015 01:27:57 -0800 (PST) In-Reply-To: <54B7D5EB.4050508@broadcom.com> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Arend van Spriel Cc: Adrian Hunter , Chris Ball , linux-mmc , Aaron Lu , Philip Rakity , Girish K S , Al Cooper , Arindam Nath , zhangfei.gao@marvell.com On 15 January 2015 at 15:59, Arend van Spriel wrote: > On 01/15/15 15:46, Ulf Hansson wrote: >> >> On 15 January 2015 at 15:17, Arend van Spriel 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 >>>>> 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