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
