Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing

[Ulf Hansson]

On Mon, 30 Oct 2023 at 09:07, Vignesh Raghavendra <vigneshr@xxxxxx> 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@xxxxxxxxxxxxxxx tag?
> >>>
> >>>> Signed-off-by: Nitin Yadav <n-yadav@xxxxxx>
> >>>
> >>> Nevertheless:
> >>>
> >>> Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> >>
> >> 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@xxxxxx
> > (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