* [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing
@ 2023-10-26 6:14 Nitin Yadav
2023-10-26 7:00 ` Adrian Hunter
2023-11-03 11:16 ` Ulf Hansson
0 siblings, 2 replies; 7+ messages in thread
From: Nitin Yadav @ 2023-10-26 6:14 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson; +Cc: linux-mmc, linux-kernel
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")
Signed-off-by: Nitin Yadav <n-yadav@ti.com>
---
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;
}
- 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]);
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing 2023-10-26 6:14 [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing Nitin Yadav @ 2023-10-26 7:00 ` Adrian Hunter 2023-10-26 7:03 ` Adrian Hunter 2023-11-03 11:16 ` Ulf Hansson 1 sibling, 1 reply; 7+ messages in thread From: Adrian Hunter @ 2023-10-26 7:00 UTC (permalink / raw) To: Nitin Yadav, ulf.hansson; +Cc: linux-mmc, linux-kernel 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> > --- > 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; > } > > - 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]); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing 2023-10-26 7:00 ` Adrian Hunter @ 2023-10-26 7:03 ` Adrian Hunter 2023-10-27 6:11 ` Nitin Yadav 0 siblings, 1 reply; 7+ messages in thread From: Adrian Hunter @ 2023-10-26 7:03 UTC (permalink / raw) To: Nitin Yadav, ulf.hansson; +Cc: linux-mmc, linux-kernel 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()? >> - 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]); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing 2023-10-26 7:03 ` Adrian Hunter @ 2023-10-27 6:11 ` Nitin Yadav 2023-10-30 8:07 ` Vignesh Raghavendra 0 siblings, 1 reply; 7+ messages in thread From: Nitin Yadav @ 2023-10-27 6:11 UTC (permalink / raw) To: Adrian Hunter, ulf.hansson; +Cc: linux-mmc, linux-kernel 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) > >>> - 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, Nitin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing 2023-10-27 6:11 ` Nitin Yadav @ 2023-10-30 8:07 ` Vignesh Raghavendra 2023-11-03 11:16 ` Ulf Hansson 0 siblings, 1 reply; 7+ messages in thread From: Vignesh Raghavendra @ 2023-10-30 8:07 UTC (permalink / raw) To: Nitin Yadav, Adrian Hunter, ulf.hansson; +Cc: linux-mmc, linux-kernel 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? 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing 2023-10-30 8:07 ` Vignesh Raghavendra @ 2023-11-03 11:16 ` Ulf Hansson 0 siblings, 0 replies; 7+ messages in thread From: Ulf Hansson @ 2023-11-03 11:16 UTC (permalink / raw) To: Vignesh Raghavendra; +Cc: Nitin Yadav, Adrian Hunter, linux-mmc, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing 2023-10-26 6:14 [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing Nitin Yadav 2023-10-26 7:00 ` Adrian Hunter @ 2023-11-03 11:16 ` Ulf Hansson 1 sibling, 0 replies; 7+ messages in thread From: Ulf Hansson @ 2023-11-03 11:16 UTC (permalink / raw) To: Nitin Yadav; +Cc: adrian.hunter, linux-mmc, linux-kernel On Thu, 26 Oct 2023 at 08:15, Nitin Yadav <n-yadav@ti.com> 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") > > Signed-off-by: Nitin Yadav <n-yadav@ti.com> Applied for fixes and by adding a stable tag, thanks! Kind regards Uffe > --- > 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; > } > > - 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]); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-03 11:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-26 6:14 [PATCH] mmc: sdhci_am654: fix start loop index for TAP value parsing Nitin Yadav 2023-10-26 7:00 ` Adrian Hunter 2023-10-26 7:03 ` Adrian Hunter 2023-10-27 6:11 ` Nitin Yadav 2023-10-30 8:07 ` Vignesh Raghavendra 2023-11-03 11:16 ` Ulf Hansson 2023-11-03 11:16 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox