The following patches fix issues with phy configurations for
sdhci_am654 driver.
Faiz Abbas (2):
mmc: sdhci_am654: Fix minor phy configurations
mmc: sdhci_am654: Fix SLOTTYPE write
drivers/mmc/host/sdhci_am654.c | 37 ++++++++++++++++------------------
1 file changed, 17 insertions(+), 20 deletions(-)
--
2.19.2
In the call to regmap_update_bits() for SLOTTYPE, the mask and value
fields are exchanged. Fix this.
Signed-off-by: Faiz Abbas <[email protected]>
---
drivers/mmc/host/sdhci_am654.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 866a9082705f..613b151a73c5 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -205,8 +205,8 @@ static int sdhci_am654_init(struct sdhci_host *host)
if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
ctl_cfg_2 = SLOTTYPE_EMBEDDED;
- regmap_update_bits(sdhci_am654->base, CTL_CFG_2, ctl_cfg_2,
- SLOTTYPE_MASK);
+ regmap_update_bits(sdhci_am654->base, CTL_CFG_2, SLOTTYPE_MASK,
+ ctl_cfg_2);
return sdhci_add_host(host);
}
--
2.19.2
Fix the following minor things:
1. Line wrapping with the regmap_*() functions is way more conservative
than required by the 80 character rule. Expand the function calls out to
use less number of lines.
2. Add an error message if the DLL fails to lock.
Signed-off-by: Faiz Abbas <[email protected]>
---
drivers/mmc/host/sdhci_am654.c | 37 ++++++++++++++++------------------
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index eea183e90f1b..866a9082705f 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -88,8 +88,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
int ret;
if (sdhci_am654->dll_on) {
- regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
- ENDLL_MASK, 0);
+ regmap_update_bits(sdhci_am654->base, PHY_CTRL1, ENDLL_MASK, 0);
sdhci_am654->dll_on = false;
}
@@ -101,8 +100,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
val = (1 << OTAPDLYENA_SHIFT) |
(sdhci_am654->otap_del_sel << OTAPDLYSEL_SHIFT);
- regmap_update_bits(sdhci_am654->base, PHY_CTRL4,
- mask, val);
+ regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val);
switch (clock) {
case 200000000:
sel50 = 0;
@@ -120,8 +118,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
/* Configure PHY DLL frequency */
mask = SEL50_MASK | SEL100_MASK;
val = (sel50 << SEL50_SHIFT) | (sel100 << SEL100_SHIFT);
- regmap_update_bits(sdhci_am654->base, PHY_CTRL5,
- mask, val);
+ regmap_update_bits(sdhci_am654->base, PHY_CTRL5, mask, val);
/* Configure DLL TRIM */
mask = DLL_TRIM_ICP_MASK;
val = sdhci_am654->trm_icp << DLL_TRIM_ICP_SHIFT;
@@ -129,19 +126,21 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
/* Configure DLL driver strength */
mask |= DR_TY_MASK;
val |= sdhci_am654->drv_strength << DR_TY_SHIFT;
- regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
- mask, val);
+ regmap_update_bits(sdhci_am654->base, PHY_CTRL1, mask, val);
/* Enable DLL */
- regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
- ENDLL_MASK, 0x1 << ENDLL_SHIFT);
+ regmap_update_bits(sdhci_am654->base, PHY_CTRL1, ENDLL_MASK,
+ 0x1 << ENDLL_SHIFT);
/*
* Poll for DLL ready. Use a one second timeout.
* Works in all experiments done so far
*/
- ret = regmap_read_poll_timeout(sdhci_am654->base,
- PHY_STAT1, val,
- val & DLLRDY_MASK,
- 1000, 1000000);
+ ret = regmap_read_poll_timeout(sdhci_am654->base, PHY_STAT1,
+ val, val & DLLRDY_MASK, 1000,
+ 1000000);
+ if (ret) {
+ dev_err(mmc_dev(host->mmc), "DLL failed to relock\n");
+ return;
+ }
sdhci_am654->dll_on = true;
}
@@ -186,8 +185,7 @@ static int sdhci_am654_init(struct sdhci_host *host)
/* Reset OTAP to default value */
mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
- regmap_update_bits(sdhci_am654->base, PHY_CTRL4,
- mask, 0x0);
+ regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, 0x0);
regmap_read(sdhci_am654->base, PHY_STAT1, &val);
if (~val & CALDONE_MASK) {
@@ -201,15 +199,14 @@ static int sdhci_am654_init(struct sdhci_host *host)
}
/* Enable pins by setting IO mux to 0 */
- regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
- IOMUX_ENABLE_MASK, 0);
+ regmap_update_bits(sdhci_am654->base, PHY_CTRL1, IOMUX_ENABLE_MASK, 0);
/* Set slot type based on SD or eMMC */
if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
ctl_cfg_2 = SLOTTYPE_EMBEDDED;
- regmap_update_bits(sdhci_am654->base, CTL_CFG_2,
- ctl_cfg_2, SLOTTYPE_MASK);
+ regmap_update_bits(sdhci_am654->base, CTL_CFG_2, ctl_cfg_2,
+ SLOTTYPE_MASK);
return sdhci_add_host(host);
}
--
2.19.2
On 25/04/19 6:57 PM, Faiz Abbas wrote:
> In the call to regmap_update_bits() for SLOTTYPE, the mask and value
> fields are exchanged. Fix this.
Could you also comment on whether this has any known effect on the driver.
>
> Signed-off-by: Faiz Abbas <[email protected]>
> ---
> drivers/mmc/host/sdhci_am654.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 866a9082705f..613b151a73c5 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -205,8 +205,8 @@ static int sdhci_am654_init(struct sdhci_host *host)
> if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
> ctl_cfg_2 = SLOTTYPE_EMBEDDED;
>
> - regmap_update_bits(sdhci_am654->base, CTL_CFG_2, ctl_cfg_2,
> - SLOTTYPE_MASK);
> + regmap_update_bits(sdhci_am654->base, CTL_CFG_2, SLOTTYPE_MASK,
> + ctl_cfg_2);
>
> return sdhci_add_host(host);
> }
>
On 25/04/19 6:57 PM, Faiz Abbas wrote:
> Fix the following minor things:
>
> 1. Line wrapping with the regmap_*() functions is way more conservative
> than required by the 80 character rule. Expand the function calls out to
> use less number of lines.
>
> 2. Add an error message if the DLL fails to lock.
Please make the white space changes a separate patch.
Also I would prefer not to use "fix" in the subject unless the patch fixes
driver behaviour.
>
> Signed-off-by: Faiz Abbas <[email protected]>
> ---
> drivers/mmc/host/sdhci_am654.c | 37 ++++++++++++++++------------------
> 1 file changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index eea183e90f1b..866a9082705f 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -88,8 +88,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
> int ret;
>
> if (sdhci_am654->dll_on) {
> - regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> - ENDLL_MASK, 0);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1, ENDLL_MASK, 0);
>
> sdhci_am654->dll_on = false;
> }
> @@ -101,8 +100,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
> mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> val = (1 << OTAPDLYENA_SHIFT) |
> (sdhci_am654->otap_del_sel << OTAPDLYSEL_SHIFT);
> - regmap_update_bits(sdhci_am654->base, PHY_CTRL4,
> - mask, val);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val);
> switch (clock) {
> case 200000000:
> sel50 = 0;
> @@ -120,8 +118,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
> /* Configure PHY DLL frequency */
> mask = SEL50_MASK | SEL100_MASK;
> val = (sel50 << SEL50_SHIFT) | (sel100 << SEL100_SHIFT);
> - regmap_update_bits(sdhci_am654->base, PHY_CTRL5,
> - mask, val);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL5, mask, val);
> /* Configure DLL TRIM */
> mask = DLL_TRIM_ICP_MASK;
> val = sdhci_am654->trm_icp << DLL_TRIM_ICP_SHIFT;
> @@ -129,19 +126,21 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
> /* Configure DLL driver strength */
> mask |= DR_TY_MASK;
> val |= sdhci_am654->drv_strength << DR_TY_SHIFT;
> - regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> - mask, val);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1, mask, val);
> /* Enable DLL */
> - regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> - ENDLL_MASK, 0x1 << ENDLL_SHIFT);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1, ENDLL_MASK,
> + 0x1 << ENDLL_SHIFT);
> /*
> * Poll for DLL ready. Use a one second timeout.
> * Works in all experiments done so far
> */
> - ret = regmap_read_poll_timeout(sdhci_am654->base,
> - PHY_STAT1, val,
> - val & DLLRDY_MASK,
> - 1000, 1000000);
> + ret = regmap_read_poll_timeout(sdhci_am654->base, PHY_STAT1,
> + val, val & DLLRDY_MASK, 1000,
> + 1000000);
> + if (ret) {
> + dev_err(mmc_dev(host->mmc), "DLL failed to relock\n");
> + return;
> + }
>
> sdhci_am654->dll_on = true;
> }
> @@ -186,8 +185,7 @@ static int sdhci_am654_init(struct sdhci_host *host)
>
> /* Reset OTAP to default value */
> mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> - regmap_update_bits(sdhci_am654->base, PHY_CTRL4,
> - mask, 0x0);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, 0x0);
>
> regmap_read(sdhci_am654->base, PHY_STAT1, &val);
> if (~val & CALDONE_MASK) {
> @@ -201,15 +199,14 @@ static int sdhci_am654_init(struct sdhci_host *host)
> }
>
> /* Enable pins by setting IO mux to 0 */
> - regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
> - IOMUX_ENABLE_MASK, 0);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1, IOMUX_ENABLE_MASK, 0);
>
> /* Set slot type based on SD or eMMC */
> if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
> ctl_cfg_2 = SLOTTYPE_EMBEDDED;
>
> - regmap_update_bits(sdhci_am654->base, CTL_CFG_2,
> - ctl_cfg_2, SLOTTYPE_MASK);
> + regmap_update_bits(sdhci_am654->base, CTL_CFG_2, ctl_cfg_2,
> + SLOTTYPE_MASK);
>
> return sdhci_add_host(host);
> }
>
Hi Adrian,
On 26/04/19 11:30 AM, Adrian Hunter wrote:
> On 25/04/19 6:57 PM, Faiz Abbas wrote:
>> In the call to regmap_update_bits() for SLOTTYPE, the mask and value
>> fields are exchanged. Fix this.
>
> Could you also comment on whether this has any known effect on the driver.
>
This call was basically a NOP but it was the correct way around in
u-boot so it was just taking that value instead. No effect that was
known to me. Found this out just by inspection.
Thanks,
Faiz
Hi Adrian,
On 26/04/19 11:20 AM, Adrian Hunter wrote:
> On 25/04/19 6:57 PM, Faiz Abbas wrote:
>> Fix the following minor things:
>>
>> 1. Line wrapping with the regmap_*() functions is way more conservative
>> than required by the 80 character rule. Expand the function calls out to
>> use less number of lines.
>>
>> 2. Add an error message if the DLL fails to lock.
>
> Please make the white space changes a separate patch.
>
> Also I would prefer not to use "fix" in the subject unless the patch fixes
> driver behaviour.
>
Ok. Two different patches. No "fix" in the subject. Sending v2.
Thanks,
Faiz
On 7/05/19 7:27 PM, Faiz Abbas wrote:
> Hi Adrian,
>
> On 26/04/19 11:30 AM, Adrian Hunter wrote:
>> On 25/04/19 6:57 PM, Faiz Abbas wrote:
>>> In the call to regmap_update_bits() for SLOTTYPE, the mask and value
>>> fields are exchanged. Fix this.
>>
>> Could you also comment on whether this has any known effect on the driver.
>>
>
> This call was basically a NOP but it was the correct way around in
> u-boot so it was just taking that value instead. No effect that was
> known to me. Found this out just by inspection.
Please put that in the commit message.