Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231026061458.1116276-1-n-yadav@ti.com> <8b7f948d-316c-4135-875a-de455ff4849c@intel.com>
 <8148dae9-e3fc-4589-ba57-a3f7a3e63b80@intel.com> <7054b3bb-de99-3fb0-5f17-78249f31c53f@ti.com>
 <42f1b9a6-2dad-42ca-a41c-3a57f87323cc@ti.com>
In-Reply-To: <42f1b9a6-2dad-42ca-a41c-3a57f87323cc@ti.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri, 3 Nov 2023 12:16:12 +0100
Message-ID: <CAPDyKFrCSTW3G6H7qS89d+UQ6RJcAYcKSPULVT8J7XKsUDpHdw@mail.gmail.com>
Subject: Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing
To:     Vignesh Raghavendra <vigneshr@ti.com>
Cc:     Nitin Yadav <n-yadav@ti.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 30 Oct 2023 at 09:07, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
> Hi Nitin, Adrian
>
> On 27/10/23 11:41, Nitin Yadav wrote:
> > Hi Adrian,
> >
> > On 26/10/23 12:33, Adrian Hunter wrote:
> >> On 26/10/23 10:00, Adrian Hunter wrote:
> >>> On 26/10/23 09:14, Nitin Yadav wrote:
> >>>> ti,otap-del-sel-legacy/ti,itap-del-sel-legacy passed from DT
> >>>> are currently ignored for all SD/MMC and eMMC modes. Fix this
> >>>> by making start loop index to MMC_TIMING_LEGACY.
> >>>>
> >>>> Fixes: 8ee5fc0e0b3be ("mmc: sdhci_am654: Update OTAPDLY writes")
> >>>>
> >>>
> >>> There isn't usually a blank line here
> >>>
> >>> Perhaps a Cc: stable@vger.kernel.org tag?
> >>>
> >>>> Signed-off-by: Nitin Yadav <n-yadav@ti.com>
> >>>
> >>> Nevertheless:
> >>>
> >>> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> >>
> >> Sorry, sent that prematurely - see comment below
> >>
> >>>
> >>>
> >>>> ---
> >>>>  drivers/mmc/host/sdhci_am654.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> >>>> index 544aaaf5cb0f..aae9d255c6a1 100644
> >>>> --- a/drivers/mmc/host/sdhci_am654.c
> >>>> +++ b/drivers/mmc/host/sdhci_am654.c
> >>>> @@ -606,7 +606,7 @@ static int sdhci_am654_get_otap_delay(struct sdhci_host *host,
> >>>>            return 0;
> >>>>    }
> >>>>
> >>
> >> Isn't the MMC_TIMING_LEGACY information read at the top of
> >> sdhci_am654_get_otap_delay()?
> > Loop also take care of ITAP. Looks like at some point single property
> > ti,otap-del-sel was used for all modes and then we moved to one property
> > per mode:
> > https://lore.kernel.org/r/20200108150920.14547-3-faiz_abbas@ti.com
> > (since v5.7)
>
> Looks like ti,otap-del-sel is deprecated for a while now (since v5.7+).
> I think that's sufficient enough time to drop it now (don't see any in
> kernel DT use this property). Lets drop the above code which handles
> MMC_TIMING_LEGACY separately, so that below for() loop can handle the
> whole set of bindings efficiently.
>
> Since this patch is marked for stable, can we get rid of the check for
> deprecated property in a follow up patch?

This seems reasonable to me, however, let's also get the DT
maintainers view on this.

I have queued up $subject patch as a fix and tagged it for stable
kernels. Feel free to post the patches to remove the support for the
deprecated binding on top.

Kind regards
Uffe


>
> Something like below? (completely untested):
>
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index c125485ba80e..50c8d3051096 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -577,32 +577,17 @@ static int sdhci_am654_get_otap_delay(struct sdhci_host *host,
>         int i;
>         int ret;
>
> -       ret = device_property_read_u32(dev, td[MMC_TIMING_LEGACY].otap_binding,
> -                                &sdhci_am654->otap_del_sel[MMC_TIMING_LEGACY]);
> -       if (ret) {
> -               /*
> -                * ti,otap-del-sel-legacy is mandatory, look for old binding
> -                * if not found.
> -                */
> -               ret = device_property_read_u32(dev, "ti,otap-del-sel",
> -                                              &sdhci_am654->otap_del_sel[0]);
> -               if (ret) {
> -                       dev_err(dev, "Couldn't find otap-del-sel\n");
> -
> -                       return ret;
> -               }
> -
> -               dev_info(dev, "Using legacy binding ti,otap-del-sel\n");
> -               sdhci_am654->legacy_otapdly = true;
> -
> -               return 0;
> -       }
> -
> -       for (i = MMC_TIMING_MMC_HS; i <= MMC_TIMING_MMC_HS400; i++) {
> +       for (i = MMC_TIMING_LEGACY; i <= MMC_TIMING_MMC_HS400; i++) {
>
>                 ret = device_property_read_u32(dev, td[i].otap_binding,
>                                                &sdhci_am654->otap_del_sel[i]);
>                 if (ret) {
> +                       if (i == MMC_TIMING_LEGACY) {
> +                               dev_err(dev, "ti,otap-del-sel-legacy is mandatory");
> +                               return ret;
> +                       }
> +
>                         dev_dbg(dev, "Couldn't find %s\n",
>                                 td[i].otap_binding);
>                         /*
>
>
>
> >>
> >>>> -  for (i = MMC_TIMING_MMC_HS; i <= MMC_TIMING_MMC_HS400; i++) {
> >>>> +  for (i = MMC_TIMING_LEGACY; i <= MMC_TIMING_MMC_HS400; i++) {
> >>>>
> >>>>            ret = device_property_read_u32(dev, td[i].otap_binding,
> >>>>                                           &sdhci_am654->otap_del_sel[i]);
> >>>
> >>
> >
>
> --
> Regards
> Vignesh