| Message ID | 20240207011520.3128382-3-jm@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add tuning algorithm for delay chain | expand |
On 2/6/24 7:15 PM, Judith Mendez wrote: > For DDR52 timing, DLL is enabled but tuning is not carried > out, therefore the ITAPDLY value in PHY CTRL 4 register is > not correct. Fix this by writing ITAPDLY after enabling DLL. > > Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes") > Signed-off-by: Judith Mendez <jm@ti.com> > --- > Changelog: > v1->v2: > - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock() > instead of sdhci_am654_setup_dll() > --- > drivers/mmc/host/sdhci_am654.c | 1 + > 1 file changed, 1 insertion(+) See how much easier this patch is this way :) Reviewed-by: Andrew Davis <afd@ti.com> > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index 2c66a965c225..b50db5d4a452 100644 > --- a/drivers/mmc/host/sdhci_am654.c > +++ b/drivers/mmc/host/sdhci_am654.c > @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) > > if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { > sdhci_am654_setup_dll(host, clock); > + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]); > sdhci_am654->dll_enable = true; > } else { > sdhci_am654_setup_delay_chain(sdhci_am654, timing);
Hi Andrew, On 2/12/24 11:13 AM, Andrew Davis wrote: > On 2/6/24 7:15 PM, Judith Mendez wrote: >> For DDR52 timing, DLL is enabled but tuning is not carried >> out, therefore the ITAPDLY value in PHY CTRL 4 register is >> not correct. Fix this by writing ITAPDLY after enabling DLL. >> >> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed >> modes") >> Signed-off-by: Judith Mendez <jm@ti.com> >> --- >> Changelog: >> v1->v2: >> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock() >> instead of sdhci_am654_setup_dll() >> --- >> drivers/mmc/host/sdhci_am654.c | 1 + >> 1 file changed, 1 insertion(+) > > See how much easier this patch is this way :) Thanks for your review. :D It does look simpler. > > Reviewed-by: Andrew Davis <afd@ti.com> > >> >> diff --git a/drivers/mmc/host/sdhci_am654.c >> b/drivers/mmc/host/sdhci_am654.c >> index 2c66a965c225..b50db5d4a452 100644 >> --- a/drivers/mmc/host/sdhci_am654.c >> +++ b/drivers/mmc/host/sdhci_am654.c >> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct >> sdhci_host *host, unsigned int clock) >> if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { >> sdhci_am654_setup_dll(host, clock); >> + sdhci_am654_write_itapdly(sdhci_am654, >> sdhci_am654->itap_del_sel[timing]); >> sdhci_am654->dll_enable = true; >> } else { >> sdhci_am654_setup_delay_chain(sdhci_am654, timing);
On 7/02/24 03:15, Judith Mendez wrote: > For DDR52 timing, DLL is enabled but tuning is not carried > out, therefore the ITAPDLY value in PHY CTRL 4 register is > not correct. Fix this by writing ITAPDLY after enabling DLL. > > Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes") Note that the Fixes tags make a different ordering than the patch order i.e. Patch Fixes in version 1 13ebeae68ac9 v5.10-rc1 2 a161c45f2979 v5.7-rc1 3 8ee5fc0e0b3b v5.7-rc1 4 8ee5fc0e0b3b v5.7-rc1 4 a0a62497f6aa v5.10-rc1 5 fe52e2fbc6ef v5.9-rc1 6 1accbced1c32 v5.3-rc1 7 a161c45f2979 v5.7-rc1 That might make backporting these patches more challenging. > Signed-off-by: Judith Mendez <jm@ti.com> > --- > Changelog: > v1->v2: > - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock() > instead of sdhci_am654_setup_dll() > --- > drivers/mmc/host/sdhci_am654.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index 2c66a965c225..b50db5d4a452 100644 > --- a/drivers/mmc/host/sdhci_am654.c > +++ b/drivers/mmc/host/sdhci_am654.c > @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) > > if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { > sdhci_am654_setup_dll(host, clock); > + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]); > sdhci_am654->dll_enable = true; > } else { > sdhci_am654_setup_delay_chain(sdhci_am654, timing);
Hi Adrian, On 2/16/24 11:09 AM, Adrian Hunter wrote: > On 7/02/24 03:15, Judith Mendez wrote: >> For DDR52 timing, DLL is enabled but tuning is not carried >> out, therefore the ITAPDLY value in PHY CTRL 4 register is >> not correct. Fix this by writing ITAPDLY after enabling DLL. >> >> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes") > > Note that the Fixes tags make a different ordering > than the patch order i.e. > > Patch Fixes in version > 1 13ebeae68ac9 v5.10-rc1 > 2 a161c45f2979 v5.7-rc1 > 3 8ee5fc0e0b3b v5.7-rc1 > 4 8ee5fc0e0b3b v5.7-rc1 > 4 a0a62497f6aa v5.10-rc1 > 5 fe52e2fbc6ef v5.9-rc1 > 6 1accbced1c32 v5.3-rc1 > 7 a161c45f2979 v5.7-rc1 > > That might make backporting these patches more challenging. Are you suggesting to remove the fixes tag here? > >> Signed-off-by: Judith Mendez <jm@ti.com> >> --- >> Changelog: >> v1->v2: >> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock() >> instead of sdhci_am654_setup_dll() >> --- >> drivers/mmc/host/sdhci_am654.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >> index 2c66a965c225..b50db5d4a452 100644 >> --- a/drivers/mmc/host/sdhci_am654.c >> +++ b/drivers/mmc/host/sdhci_am654.c >> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) >> >> if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { >> sdhci_am654_setup_dll(host, clock); >> + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]); >> sdhci_am654->dll_enable = true; >> } else { >> sdhci_am654_setup_delay_chain(sdhci_am654, timing); > ~ Judith
On 20/02/24 23:05, Judith Mendez wrote: > Hi Adrian, > > On 2/16/24 11:09 AM, Adrian Hunter wrote: >> On 7/02/24 03:15, Judith Mendez wrote: >>> For DDR52 timing, DLL is enabled but tuning is not carried >>> out, therefore the ITAPDLY value in PHY CTRL 4 register is >>> not correct. Fix this by writing ITAPDLY after enabling DLL. >>> >>> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes") >> >> Note that the Fixes tags make a different ordering >> than the patch order i.e. >> >> Patch Fixes in version >> 1 13ebeae68ac9 v5.10-rc1 >> 2 a161c45f2979 v5.7-rc1 >> 3 8ee5fc0e0b3b v5.7-rc1 >> 4 8ee5fc0e0b3b v5.7-rc1 >> 4 a0a62497f6aa v5.10-rc1 >> 5 fe52e2fbc6ef v5.9-rc1 >> 6 1accbced1c32 v5.3-rc1 >> 7 a161c45f2979 v5.7-rc1 >> >> That might make backporting these patches more challenging. > > Are you suggesting to remove the fixes tag here? No, it is just something to think about if you intend to backport these patches to older kernels. > >> >>> Signed-off-by: Judith Mendez <jm@ti.com> >>> --- >>> Changelog: >>> v1->v2: >>> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock() >>> instead of sdhci_am654_setup_dll() >>> --- >>> drivers/mmc/host/sdhci_am654.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>> index 2c66a965c225..b50db5d4a452 100644 >>> --- a/drivers/mmc/host/sdhci_am654.c >>> +++ b/drivers/mmc/host/sdhci_am654.c >>> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) >>> if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { >>> sdhci_am654_setup_dll(host, clock); >>> + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]); >>> sdhci_am654->dll_enable = true; >>> } else { >>> sdhci_am654_setup_delay_chain(sdhci_am654, timing); >> > > ~ Judith
Hi Adrian, On 2/28/24 7:21 AM, Adrian Hunter wrote: > On 20/02/24 23:05, Judith Mendez wrote: >> Hi Adrian, >> >> On 2/16/24 11:09 AM, Adrian Hunter wrote: >>> On 7/02/24 03:15, Judith Mendez wrote: >>>> For DDR52 timing, DLL is enabled but tuning is not carried >>>> out, therefore the ITAPDLY value in PHY CTRL 4 register is >>>> not correct. Fix this by writing ITAPDLY after enabling DLL. >>>> >>>> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes") >>> >>> Note that the Fixes tags make a different ordering >>> than the patch order i.e. >>> >>> Patch Fixes in version >>> 1 13ebeae68ac9 v5.10-rc1 >>> 2 a161c45f2979 v5.7-rc1 >>> 3 8ee5fc0e0b3b v5.7-rc1 >>> 4 8ee5fc0e0b3b v5.7-rc1 >>> 4 a0a62497f6aa v5.10-rc1 >>> 5 fe52e2fbc6ef v5.9-rc1 >>> 6 1accbced1c32 v5.3-rc1 >>> 7 a161c45f2979 v5.7-rc1 >>> >>> That might make backporting these patches more challenging. >> >> Are you suggesting to remove the fixes tag here? > > No, it is just something to think about if you intend to > backport these patches to older kernels. Thanks, Ill keep this in mind for v3. > >> >>> >>>> Signed-off-by: Judith Mendez <jm@ti.com> >>>> --- >>>> Changelog: >>>> v1->v2: >>>> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock() >>>> instead of sdhci_am654_setup_dll() >>>> --- >>>> drivers/mmc/host/sdhci_am654.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>> index 2c66a965c225..b50db5d4a452 100644 >>>> --- a/drivers/mmc/host/sdhci_am654.c >>>> +++ b/drivers/mmc/host/sdhci_am654.c >>>> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) >>>> if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { >>>> sdhci_am654_setup_dll(host, clock); >>>> + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]); >>>> sdhci_am654->dll_enable = true; >>>> } else { >>>> sdhci_am654_setup_delay_chain(sdhci_am654, timing); >>> >> >> ~ Judith >
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c index 2c66a965c225..b50db5d4a452 100644 --- a/drivers/mmc/host/sdhci_am654.c +++ b/drivers/mmc/host/sdhci_am654.c @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { sdhci_am654_setup_dll(host, clock); + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]); sdhci_am654->dll_enable = true; } else { sdhci_am654_setup_delay_chain(sdhci_am654, timing);
For DDR52 timing, DLL is enabled but tuning is not carried out, therefore the ITAPDLY value in PHY CTRL 4 register is not correct. Fix this by writing ITAPDLY after enabling DLL. Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes") Signed-off-by: Judith Mendez <jm@ti.com> --- Changelog: v1->v2: - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock() instead of sdhci_am654_setup_dll() --- drivers/mmc/host/sdhci_am654.c | 1 + 1 file changed, 1 insertion(+)