diff mbox series

[V4,4/8] drivers: mmc: am654_sdhci: Update OTAP/ITAP delay

Message ID 20230822184135.2328409-5-nm@ti.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series board: ti: Add support for BeaglePlay | expand

Commit Message

Nishanth Menon Aug. 22, 2023, 6:41 p.m. UTC 
From: Nitin Yadav <n-yadav@ti.com>

U-Boot is fail to boot class U1 UHS SD cards (such as microcenter)
due to incorrect OTAP and ITAP delay select values. Update OTAP and
ITAP delay select values based on recommeded RIOT values to fix boot
issue.

Signed-off-by: Nitin Yadav <n-yadav@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Picked up from TI u-boot
New patch in the series

 drivers/mmc/am654_sdhci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Mattijs Korpershoek Aug. 23, 2023, 8:06 a.m. UTC | #1 
On mar., août 22, 2023 at 13:41, Nishanth Menon <nm@ti.com> wrote:

> From: Nitin Yadav <n-yadav@ti.com>
>
> U-Boot is fail to boot class U1 UHS SD cards (such as microcenter)
> due to incorrect OTAP and ITAP delay select values. Update OTAP and
> ITAP delay select values based on recommeded RIOT values to fix boot
> issue.

nitpick: recommeded -> recommended

>
> Signed-off-by: Nitin Yadav <n-yadav@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Picked up from TI u-boot
> New patch in the series
>
>  drivers/mmc/am654_sdhci.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> index fd667aeafdaa..d0b51c0707d4 100644
> --- a/drivers/mmc/am654_sdhci.c
> +++ b/drivers/mmc/am654_sdhci.c
> @@ -442,12 +442,18 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>  {
>  	struct udevice *dev = host->mmc->dev;
>  	struct am654_sdhci_plat *plat = dev_get_plat(dev);
> -	u32 otap_del_sel, mask, val;
> +	u32 otap_del_sel, itap_del_sel, mask, val;
>  
>  	otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
> -	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> -	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
> +	itap_del_sel = plat->itap_del_sel[host->mmc->selected_mode];
> +	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK | ITAPDLYSEL_MASK |
> +		ITAPDLYENA_MASK;
> +	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT) |
> +		(1 << ITAPDLYENA_SHIFT) | (itap_del_sel << ITAPDLYSEL_SHIFT);
> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
> +			   1 << ITAPCHGWIN_SHIFT);
>  	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
>  
>  	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
>  			   plat->clkbuf_sel);
> @@ -501,7 +507,7 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>  	 * Remove the corresponding capability if an otap-del-sel
>  	 * value is not found
>  	 */
> -	for (i = MMC_HS; i <= MMC_HS_400; i++) {
> +	for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {

I'm not super familiar with mmc drivers in general, but it feels like
this bit does not really belong in this patch.

Is this another bugfix for removing otap-del-sel for MMC_LEGACY devices
or does U1 UHS SD cards are considered to be of MMC_LEGACY type ?

>  		ret = dev_read_u32(dev, td[i].otap_binding,
>  				   &plat->otap_del_sel[i]);
>  		if (ret) {
> -- 
> 2.40.0
Nitin Yadav Aug. 23, 2023, 10:15 a.m. UTC | #2 
Hi Mattijs,

On 23/08/23 13:36, Mattijs Korpershoek wrote:
> On mar., août 22, 2023 at 13:41, Nishanth Menon <nm@ti.com> wrote:
> 
>> From: Nitin Yadav <n-yadav@ti.com>
>>
>> U-Boot is fail to boot class U1 UHS SD cards (such as microcenter)
>> due to incorrect OTAP and ITAP delay select values. Update OTAP and
>> ITAP delay select values based on recommeded RIOT values to fix boot
>> issue.
> 
> nitpick: recommeded -> recommended
> 
>>
>> Signed-off-by: Nitin Yadav <n-yadav@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
>> Picked up from TI u-boot
>> New patch in the series
>>
>>   drivers/mmc/am654_sdhci.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
>> index fd667aeafdaa..d0b51c0707d4 100644
>> --- a/drivers/mmc/am654_sdhci.c
>> +++ b/drivers/mmc/am654_sdhci.c
>> @@ -442,12 +442,18 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>>   {
>>   	struct udevice *dev = host->mmc->dev;
>>   	struct am654_sdhci_plat *plat = dev_get_plat(dev);
>> -	u32 otap_del_sel, mask, val;
>> +	u32 otap_del_sel, itap_del_sel, mask, val;
>>   
>>   	otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
>> -	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
>> -	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
>> +	itap_del_sel = plat->itap_del_sel[host->mmc->selected_mode];
>> +	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK | ITAPDLYSEL_MASK |
>> +		ITAPDLYENA_MASK;
>> +	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT) |
>> +		(1 << ITAPDLYENA_SHIFT) | (itap_del_sel << ITAPDLYSEL_SHIFT);
>> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>> +			   1 << ITAPCHGWIN_SHIFT);
>>   	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
>> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
>>   
>>   	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
>>   			   plat->clkbuf_sel);
>> @@ -501,7 +507,7 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>>   	 * Remove the corresponding capability if an otap-del-sel
>>   	 * value is not found
>>   	 */
>> -	for (i = MMC_HS; i <= MMC_HS_400; i++) {
>> +	for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
> 
> I'm not super familiar with mmc drivers in general, but it feels like
> this bit does not really belong in this patch. >
> Is this another bugfix for removing otap-del-sel for MMC_LEGACY devices
> or does U1 UHS SD cards are considered to be of MMC_LEGACY type ?
yes, its two different fix. Internally I posted two patches for them. 
can be referred in below links:

https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=b5a2f7972fd142df03a15ba58829aa068c891abf

https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=527095165281cbc60a1b42995c1e55e6756cbe93

> 
>>   		ret = dev_read_u32(dev, td[i].otap_binding,
>>   				   &plat->otap_del_sel[i]);
>>   		if (ret) {
>> -- 
>> 2.40.0
Nitin Yadav Aug. 23, 2023, 10:18 a.m. UTC | #3 
On 23/08/23 15:45, Nitin Yadav wrote:
> Hi Mattijs,
> 
> On 23/08/23 13:36, Mattijs Korpershoek wrote:
>> On mar., août 22, 2023 at 13:41, Nishanth Menon <nm@ti.com> wrote:
>>
>>> From: Nitin Yadav <n-yadav@ti.com>
>>>
>>> U-Boot is fail to boot class U1 UHS SD cards (such as microcenter)
>>> due to incorrect OTAP and ITAP delay select values. Update OTAP and
>>> ITAP delay select values based on recommeded RIOT values to fix boot
>>> issue.
>>
>> nitpick: recommeded -> recommended
>>
>>>
>>> Signed-off-by: Nitin Yadav <n-yadav@ti.com>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> ---
>>> Picked up from TI u-boot
>>> New patch in the series
>>>
>>>   drivers/mmc/am654_sdhci.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
>>> index fd667aeafdaa..d0b51c0707d4 100644
>>> --- a/drivers/mmc/am654_sdhci.c
>>> +++ b/drivers/mmc/am654_sdhci.c
>>> @@ -442,12 +442,18 @@ static int j721e_4bit_sdhci_set_ios_post(struct 
>>> sdhci_host *host)
>>>   {
>>>       struct udevice *dev = host->mmc->dev;
>>>       struct am654_sdhci_plat *plat = dev_get_plat(dev);
>>> -    u32 otap_del_sel, mask, val;
>>> +    u32 otap_del_sel, itap_del_sel, mask, val;
>>>       otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
>>> -    mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
>>> -    val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
>>> +    itap_del_sel = plat->itap_del_sel[host->mmc->selected_mode];
>>> +    mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK | ITAPDLYSEL_MASK |
>>> +        ITAPDLYENA_MASK;
>>> +    val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << 
>>> OTAPDLYSEL_SHIFT) |
>>> +        (1 << ITAPDLYENA_SHIFT) | (itap_del_sel << ITAPDLYSEL_SHIFT);
>>> +    regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>>> +               1 << ITAPCHGWIN_SHIFT);
>>>       regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
>>> +    regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
>>>       regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
>>>                  plat->clkbuf_sel);
>>> @@ -501,7 +507,7 @@ static int sdhci_am654_get_otap_delay(struct 
>>> udevice *dev,
>>>        * Remove the corresponding capability if an otap-del-sel
>>>        * value is not found
>>>        */
>>> -    for (i = MMC_HS; i <= MMC_HS_400; i++) {
>>> +    for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
>>
>> I'm not super familiar with mmc drivers in general, but it feels like
>> this bit does not really belong in this patch. >
>> Is this another bugfix for removing otap-del-sel for MMC_LEGACY devices
>> or does U1 UHS SD cards are considered to be of MMC_LEGACY type ?
> yes, its two different fix. Internally I posted two patches for them. 
> can be referred in below links:
> 
> https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=b5a2f7972fd142df03a15ba58829aa068c891abf
> 
> https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=527095165281cbc60a1b42995c1e55e6756cbe93
> 
For uboot somehow it got merged as single patch as It was one of my 
first fix :) .
>>
>>>           ret = dev_read_u32(dev, td[i].otap_binding,
>>>                      &plat->otap_del_sel[i]);
>>>           if (ret) {
>>> -- 
>>> 2.40.0
>
Nishanth Menon Aug. 23, 2023, 11:38 a.m. UTC | #4 
On 15:48-20230823, Nitin Yadav wrote:
[...]

> > > > -    for (i = MMC_HS; i <= MMC_HS_400; i++) {
> > > > +    for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
> > > 
> > > I'm not super familiar with mmc drivers in general, but it feels like
> > > this bit does not really belong in this patch. >
> > > Is this another bugfix for removing otap-del-sel for MMC_LEGACY devices
> > > or does U1 UHS SD cards are considered to be of MMC_LEGACY type ?
> > yes, its two different fix. Internally I posted two patches for them.
> > can be referred in below links:
> > 
> > https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=b5a2f7972fd142df03a15ba58829aa068c891abf
> > 
> > https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=527095165281cbc60a1b42995c1e55e6756cbe93
> > 
> For uboot somehow it got merged as single patch as It was one of my first
> fix :) .

Thanks for the pointer. I will pick the latest up for V5.
Nishanth Menon Aug. 23, 2023, 2:21 p.m. UTC | #5 
On 06:38-20230823, Nishanth Menon wrote:
> On 15:48-20230823, Nitin Yadav wrote:
> [...]
> 
> > > > > -    for (i = MMC_HS; i <= MMC_HS_400; i++) {
> > > > > +    for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
> > > > 
> > > > I'm not super familiar with mmc drivers in general, but it feels like
> > > > this bit does not really belong in this patch. >
> > > > Is this another bugfix for removing otap-del-sel for MMC_LEGACY devices
> > > > or does U1 UHS SD cards are considered to be of MMC_LEGACY type ?
> > > yes, its two different fix. Internally I posted two patches for them.
> > > can be referred in below links:
> > > 
> > > https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=b5a2f7972fd142df03a15ba58829aa068c891abf
> > > 
> > > https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=527095165281cbc60a1b42995c1e55e6756cbe93
> > > 
> > For uboot somehow it got merged as single patch as It was one of my first
> > fix :) .
> 
> Thanks for the pointer. I will pick the latest up for V5.

After a bit of offline discussions with Nitin and other folks, looks
like sdhci fixes need to be it's own series. will drop that from V5.
diff mbox series

Patch

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index fd667aeafdaa..d0b51c0707d4 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -442,12 +442,18 @@  static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
 {
 	struct udevice *dev = host->mmc->dev;
 	struct am654_sdhci_plat *plat = dev_get_plat(dev);
-	u32 otap_del_sel, mask, val;
+	u32 otap_del_sel, itap_del_sel, mask, val;
 
 	otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
-	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
-	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
+	itap_del_sel = plat->itap_del_sel[host->mmc->selected_mode];
+	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK | ITAPDLYSEL_MASK |
+		ITAPDLYENA_MASK;
+	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT) |
+		(1 << ITAPDLYENA_SHIFT) | (itap_del_sel << ITAPDLYSEL_SHIFT);
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
+			   1 << ITAPCHGWIN_SHIFT);
 	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
 
 	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
 			   plat->clkbuf_sel);
@@ -501,7 +507,7 @@  static int sdhci_am654_get_otap_delay(struct udevice *dev,
 	 * Remove the corresponding capability if an otap-del-sel
 	 * value is not found
 	 */
-	for (i = MMC_HS; i <= MMC_HS_400; i++) {
+	for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
 		ret = dev_read_u32(dev, td[i].otap_binding,
 				   &plat->otap_del_sel[i]);
 		if (ret) {