* [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together @ 2015-01-27 17:23 Rhyland Klein 2015-01-28 6:06 ` Alexandre Courbot 2015-01-28 15:02 ` Ulf Hansson 0 siblings, 2 replies; 6+ messages in thread From: Rhyland Klein @ 2015-01-27 17:23 UTC (permalink / raw) To: ulf.hansson, chris Cc: swarren, thierry.reding, linux-tegra, linux-kernel, pkunapuli, gnurou, Rhyland Klein From: Pavan Kunapuli <pkunapuli@nvidia.com> If there is a gap between xfer mode and command register writes, tegra SDMMC controller can sometimes issue a spurious command before the CMD register is written. To avoid this, these two registers need to be written together in a single write operation. Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com> Signed-off-by: Rhyland Klein <rklein@nvidia.com> --- drivers/mmc/host/sdhci-tegra.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 59797106af93..3d34de47e57e 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -41,6 +41,7 @@ #define NVQUIRK_DISABLE_SDR50 BIT(3) #define NVQUIRK_DISABLE_SDR104 BIT(4) #define NVQUIRK_DISABLE_DDR50 BIT(5) +#define NVQUIRK_SHADOW_XFER_MODE_REG BIT(6) struct sdhci_tegra_soc_data { const struct sdhci_pltfm_data *pdata; @@ -67,6 +68,32 @@ static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) return readw(host->ioaddr + reg); } +static void tegra_sdhci_writew(struct sdhci_host *host, u16 val, int reg) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_tegra *tegra_host = pltfm_host->priv; + const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data; + + if (soc_data->nvquirks * NVQUIRK_SHADOW_XFER_MODE_REG) { + switch (reg) { + case SDHCI_TRANSFER_MODE: + /* + * Postpone this write, we must do it together with a + * command write that is down below. + */ + pltfm_host->xfer_mode_shadow = val; + return; + case SDHCI_COMMAND: + writel((val << 16) | pltfm_host->xfer_mode_shadow, + host->ioaddr + SDHCI_TRANSFER_MODE); + pltfm_host->xfer_mode_shadow = 0; + return; + } + } + + writew(val, host->ioaddr + reg); +} + static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -147,6 +174,7 @@ static void tegra_sdhci_set_bus_width(struct sdhci_host *host, int bus_width) static const struct sdhci_ops tegra_sdhci_ops = { .get_ro = tegra_sdhci_get_ro, .read_w = tegra_sdhci_readw, + .write_w = tegra_sdhci_writew, .write_l = tegra_sdhci_writel, .set_clock = sdhci_set_clock, .set_bus_width = tegra_sdhci_set_bus_width, @@ -201,7 +229,8 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = { .pdata = &sdhci_tegra114_pdata, .nvquirks = NVQUIRK_DISABLE_SDR50 | NVQUIRK_DISABLE_DDR50 | - NVQUIRK_DISABLE_SDR104, + NVQUIRK_DISABLE_SDR104 | + NVQUIRK_SHADOW_XFER_MODE_REG, }; static const struct of_device_id sdhci_tegra_dt_match[] = { -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together 2015-01-27 17:23 [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together Rhyland Klein @ 2015-01-28 6:06 ` Alexandre Courbot 2015-01-28 16:30 ` Rhyland Klein 2015-01-28 15:02 ` Ulf Hansson 1 sibling, 1 reply; 6+ messages in thread From: Alexandre Courbot @ 2015-01-28 6:06 UTC (permalink / raw) To: Rhyland Klein Cc: Ulf Hansson, Chris Ball, Stephen Warren, Thierry Reding, linux-tegra@vger.kernel.org, Linux Kernel Mailing List, pkunapuli On Wed, Jan 28, 2015 at 2:23 AM, Rhyland Klein <rklein@nvidia.com> wrote: > From: Pavan Kunapuli <pkunapuli@nvidia.com> > > If there is a gap between xfer mode and command register writes, > tegra SDMMC controller can sometimes issue a spurious command before > the CMD register is written. To avoid this, these two registers need > to be written together in a single write operation. > > Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com> > Signed-off-by: Rhyland Klein <rklein@nvidia.com> > --- > drivers/mmc/host/sdhci-tegra.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > index 59797106af93..3d34de47e57e 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -41,6 +41,7 @@ > #define NVQUIRK_DISABLE_SDR50 BIT(3) > #define NVQUIRK_DISABLE_SDR104 BIT(4) > #define NVQUIRK_DISABLE_DDR50 BIT(5) > +#define NVQUIRK_SHADOW_XFER_MODE_REG BIT(6) > > struct sdhci_tegra_soc_data { > const struct sdhci_pltfm_data *pdata; > @@ -67,6 +68,32 @@ static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) > return readw(host->ioaddr + reg); > } > > +static void tegra_sdhci_writew(struct sdhci_host *host, u16 val, int reg) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_tegra *tegra_host = pltfm_host->priv; > + const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data; > + > + if (soc_data->nvquirks * NVQUIRK_SHADOW_XFER_MODE_REG) { Isn't the '*' supposed to be a '&' here? > + switch (reg) { > + case SDHCI_TRANSFER_MODE: > + /* > + * Postpone this write, we must do it together with a > + * command write that is down below. > + */ > + pltfm_host->xfer_mode_shadow = val; > + return; > + case SDHCI_COMMAND: > + writel((val << 16) | pltfm_host->xfer_mode_shadow, > + host->ioaddr + SDHCI_TRANSFER_MODE); > + pltfm_host->xfer_mode_shadow = 0; That last line is probably not needed and could actually be harmful - if we try to write SDHCI_COMMAND twice in a raw without a write to SDHCI_TRANSFER_MODE in between, the zero will overwrite the previous value of SDHCI_TRANSFER_MODE. > + return; > + } > + } > + > + writew(val, host->ioaddr + reg); > +} > + > static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg) > { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -147,6 +174,7 @@ static void tegra_sdhci_set_bus_width(struct sdhci_host *host, int bus_width) > static const struct sdhci_ops tegra_sdhci_ops = { > .get_ro = tegra_sdhci_get_ro, > .read_w = tegra_sdhci_readw, > + .write_w = tegra_sdhci_writew, > .write_l = tegra_sdhci_writel, > .set_clock = sdhci_set_clock, > .set_bus_width = tegra_sdhci_set_bus_width, > @@ -201,7 +229,8 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = { > .pdata = &sdhci_tegra114_pdata, > .nvquirks = NVQUIRK_DISABLE_SDR50 | > NVQUIRK_DISABLE_DDR50 | > - NVQUIRK_DISABLE_SDR104, > + NVQUIRK_DISABLE_SDR104 | > + NVQUIRK_SHADOW_XFER_MODE_REG, > }; Since this only applies to Tegra114 (?), I wonder whether it would not be better to have a dedicated tegra114_sdhci_ops that implements tegra_sdhci_writew, and use it only in tegra_sdhci_writew. That way you could get rid of the NVQUIRK_SHADOW_XFER_MODE_REG and the test for it in tegra_sdhci_writew(), and chips prior to Tegra114 will not have to needlessly check for it every time they write a register. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together 2015-01-28 6:06 ` Alexandre Courbot @ 2015-01-28 16:30 ` Rhyland Klein 2015-02-09 6:22 ` Alexandre Courbot 0 siblings, 1 reply; 6+ messages in thread From: Rhyland Klein @ 2015-01-28 16:30 UTC (permalink / raw) To: Alexandre Courbot Cc: Ulf Hansson, Chris Ball, Stephen Warren, Thierry Reding, linux-tegra@vger.kernel.org, Linux Kernel Mailing List, pkunapuli On 1/28/2015 1:06 AM, Alexandre Courbot wrote: > On Wed, Jan 28, 2015 at 2:23 AM, Rhyland Klein <rklein@nvidia.com> wrote: >> From: Pavan Kunapuli <pkunapuli@nvidia.com> >> >> If there is a gap between xfer mode and command register writes, >> tegra SDMMC controller can sometimes issue a spurious command before >> the CMD register is written. To avoid this, these two registers need >> to be written together in a single write operation. >> >> Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com> >> Signed-off-by: Rhyland Klein <rklein@nvidia.com> >> --- >> drivers/mmc/host/sdhci-tegra.c | 31 ++++++++++++++++++++++++++++++- >> 1 file changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c >> index 59797106af93..3d34de47e57e 100644 >> --- a/drivers/mmc/host/sdhci-tegra.c >> +++ b/drivers/mmc/host/sdhci-tegra.c >> @@ -41,6 +41,7 @@ >> #define NVQUIRK_DISABLE_SDR50 BIT(3) >> #define NVQUIRK_DISABLE_SDR104 BIT(4) >> #define NVQUIRK_DISABLE_DDR50 BIT(5) >> +#define NVQUIRK_SHADOW_XFER_MODE_REG BIT(6) >> >> struct sdhci_tegra_soc_data { >> const struct sdhci_pltfm_data *pdata; >> @@ -67,6 +68,32 @@ static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) >> return readw(host->ioaddr + reg); >> } >> >> +static void tegra_sdhci_writew(struct sdhci_host *host, u16 val, int reg) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_tegra *tegra_host = pltfm_host->priv; >> + const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data; >> + >> + if (soc_data->nvquirks * NVQUIRK_SHADOW_XFER_MODE_REG) { > > Isn't the '*' supposed to be a '&' here? Yah .. not sure how that happened, but it should be '&' good catch. > >> + switch (reg) { >> + case SDHCI_TRANSFER_MODE: >> + /* >> + * Postpone this write, we must do it together with a >> + * command write that is down below. >> + */ >> + pltfm_host->xfer_mode_shadow = val; >> + return; >> + case SDHCI_COMMAND: >> + writel((val << 16) | pltfm_host->xfer_mode_shadow, >> + host->ioaddr + SDHCI_TRANSFER_MODE); >> + pltfm_host->xfer_mode_shadow = 0; > > That last line is probably not needed and could actually be harmful - > if we try to write SDHCI_COMMAND twice in a raw without a write to > SDHCI_TRANSFER_MODE in between, the zero will overwrite the previous > value of SDHCI_TRANSFER_MODE. True, will remove it. > >> + return; >> + } >> + } >> + >> + writew(val, host->ioaddr + reg); >> +} >> + >> static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg) >> { >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> @@ -147,6 +174,7 @@ static void tegra_sdhci_set_bus_width(struct sdhci_host *host, int bus_width) >> static const struct sdhci_ops tegra_sdhci_ops = { >> .get_ro = tegra_sdhci_get_ro, >> .read_w = tegra_sdhci_readw, >> + .write_w = tegra_sdhci_writew, >> .write_l = tegra_sdhci_writel, >> .set_clock = sdhci_set_clock, >> .set_bus_width = tegra_sdhci_set_bus_width, >> @@ -201,7 +229,8 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = { >> .pdata = &sdhci_tegra114_pdata, >> .nvquirks = NVQUIRK_DISABLE_SDR50 | >> NVQUIRK_DISABLE_DDR50 | >> - NVQUIRK_DISABLE_SDR104, >> + NVQUIRK_DISABLE_SDR104 | >> + NVQUIRK_SHADOW_XFER_MODE_REG, >> }; > > Since this only applies to Tegra114 (?), I wonder whether it would not > be better to have a dedicated tegra114_sdhci_ops that implements > tegra_sdhci_writew, and use it only in tegra_sdhci_writew. That way > you could get rid of the NVQUIRK_SHADOW_XFER_MODE_REG and the test for > it in tegra_sdhci_writew(), and chips prior to Tegra114 will not have > to needlessly check for it every time they write a register. The reason I did it this way, is that this doesn't explicitly just apply to T114. It actually applies to T114, T124 and T132. In that case, I think it makes sense to keep the QUIRK and I can update the commit description to reflect that. Thanks! -rhyland > -- nvpublic ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together 2015-01-28 16:30 ` Rhyland Klein @ 2015-02-09 6:22 ` Alexandre Courbot 2015-02-10 7:44 ` Ulf Hansson 0 siblings, 1 reply; 6+ messages in thread From: Alexandre Courbot @ 2015-02-09 6:22 UTC (permalink / raw) To: Rhyland Klein Cc: Ulf Hansson, Chris Ball, Stephen Warren, Thierry Reding, linux-tegra@vger.kernel.org, Linux Kernel Mailing List, pkunapuli On Thu, Jan 29, 2015 at 1:30 AM, Rhyland Klein <rklein@nvidia.com> wrote: > On 1/28/2015 1:06 AM, Alexandre Courbot wrote: >> On Wed, Jan 28, 2015 at 2:23 AM, Rhyland Klein <rklein@nvidia.com> wrote: >>> From: Pavan Kunapuli <pkunapuli@nvidia.com> >>> >>> If there is a gap between xfer mode and command register writes, >>> tegra SDMMC controller can sometimes issue a spurious command before >>> the CMD register is written. To avoid this, these two registers need >>> to be written together in a single write operation. >>> >>> Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com> >>> Signed-off-by: Rhyland Klein <rklein@nvidia.com> >>> --- >>> drivers/mmc/host/sdhci-tegra.c | 31 ++++++++++++++++++++++++++++++- >>> 1 file changed, 30 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c >>> index 59797106af93..3d34de47e57e 100644 >>> --- a/drivers/mmc/host/sdhci-tegra.c >>> +++ b/drivers/mmc/host/sdhci-tegra.c >>> @@ -41,6 +41,7 @@ >>> #define NVQUIRK_DISABLE_SDR50 BIT(3) >>> #define NVQUIRK_DISABLE_SDR104 BIT(4) >>> #define NVQUIRK_DISABLE_DDR50 BIT(5) >>> +#define NVQUIRK_SHADOW_XFER_MODE_REG BIT(6) >>> >>> struct sdhci_tegra_soc_data { >>> const struct sdhci_pltfm_data *pdata; >>> @@ -67,6 +68,32 @@ static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) >>> return readw(host->ioaddr + reg); >>> } >>> >>> +static void tegra_sdhci_writew(struct sdhci_host *host, u16 val, int reg) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct sdhci_tegra *tegra_host = pltfm_host->priv; >>> + const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data; >>> + >>> + if (soc_data->nvquirks * NVQUIRK_SHADOW_XFER_MODE_REG) { >> >> Isn't the '*' supposed to be a '&' here? > > Yah .. not sure how that happened, but it should be '&' good catch. > >> >>> + switch (reg) { >>> + case SDHCI_TRANSFER_MODE: >>> + /* >>> + * Postpone this write, we must do it together with a >>> + * command write that is down below. >>> + */ >>> + pltfm_host->xfer_mode_shadow = val; >>> + return; >>> + case SDHCI_COMMAND: >>> + writel((val << 16) | pltfm_host->xfer_mode_shadow, >>> + host->ioaddr + SDHCI_TRANSFER_MODE); >>> + pltfm_host->xfer_mode_shadow = 0; >> >> That last line is probably not needed and could actually be harmful - >> if we try to write SDHCI_COMMAND twice in a raw without a write to >> SDHCI_TRANSFER_MODE in between, the zero will overwrite the previous >> value of SDHCI_TRANSFER_MODE. > > True, will remove it. > >> >>> + return; >>> + } >>> + } >>> + >>> + writew(val, host->ioaddr + reg); >>> +} >>> + >>> static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg) >>> { >>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> @@ -147,6 +174,7 @@ static void tegra_sdhci_set_bus_width(struct sdhci_host *host, int bus_width) >>> static const struct sdhci_ops tegra_sdhci_ops = { >>> .get_ro = tegra_sdhci_get_ro, >>> .read_w = tegra_sdhci_readw, >>> + .write_w = tegra_sdhci_writew, >>> .write_l = tegra_sdhci_writel, >>> .set_clock = sdhci_set_clock, >>> .set_bus_width = tegra_sdhci_set_bus_width, >>> @@ -201,7 +229,8 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = { >>> .pdata = &sdhci_tegra114_pdata, >>> .nvquirks = NVQUIRK_DISABLE_SDR50 | >>> NVQUIRK_DISABLE_DDR50 | >>> - NVQUIRK_DISABLE_SDR104, >>> + NVQUIRK_DISABLE_SDR104 | >>> + NVQUIRK_SHADOW_XFER_MODE_REG, >>> }; >> >> Since this only applies to Tegra114 (?), I wonder whether it would not >> be better to have a dedicated tegra114_sdhci_ops that implements >> tegra_sdhci_writew, and use it only in tegra_sdhci_writew. That way >> you could get rid of the NVQUIRK_SHADOW_XFER_MODE_REG and the test for >> it in tegra_sdhci_writew(), and chips prior to Tegra114 will not have >> to needlessly check for it every time they write a register. > > The reason I did it this way, is that this doesn't explicitly just apply > to T114. It actually applies to T114, T124 and T132. In that case, I > think it makes sense to keep the QUIRK and I can update the commit > description to reflect that. All the same, I don't see what advantage we have checking for that condition for every single write while we could simply set the right function to use at probe time? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together 2015-02-09 6:22 ` Alexandre Courbot @ 2015-02-10 7:44 ` Ulf Hansson 0 siblings, 0 replies; 6+ messages in thread From: Ulf Hansson @ 2015-02-10 7:44 UTC (permalink / raw) To: Rhyland Klein Cc: Chris Ball, Alexandre Courbot, Stephen Warren, Thierry Reding, linux-tegra@vger.kernel.org, Linux Kernel Mailing List, pkunapuli On 9 February 2015 at 07:22, Alexandre Courbot <gnurou@gmail.com> wrote: > On Thu, Jan 29, 2015 at 1:30 AM, Rhyland Klein <rklein@nvidia.com> wrote: >> On 1/28/2015 1:06 AM, Alexandre Courbot wrote: >>> On Wed, Jan 28, 2015 at 2:23 AM, Rhyland Klein <rklein@nvidia.com> wrote: >>>> From: Pavan Kunapuli <pkunapuli@nvidia.com> >>>> >>>> If there is a gap between xfer mode and command register writes, >>>> tegra SDMMC controller can sometimes issue a spurious command before >>>> the CMD register is written. To avoid this, these two registers need >>>> to be written together in a single write operation. >>>> >>>> Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com> >>>> Signed-off-by: Rhyland Klein <rklein@nvidia.com> >>>> --- >>>> drivers/mmc/host/sdhci-tegra.c | 31 ++++++++++++++++++++++++++++++- >>>> 1 file changed, 30 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c >>>> index 59797106af93..3d34de47e57e 100644 >>>> --- a/drivers/mmc/host/sdhci-tegra.c >>>> +++ b/drivers/mmc/host/sdhci-tegra.c >>>> @@ -41,6 +41,7 @@ >>>> #define NVQUIRK_DISABLE_SDR50 BIT(3) >>>> #define NVQUIRK_DISABLE_SDR104 BIT(4) >>>> #define NVQUIRK_DISABLE_DDR50 BIT(5) >>>> +#define NVQUIRK_SHADOW_XFER_MODE_REG BIT(6) >>>> >>>> struct sdhci_tegra_soc_data { >>>> const struct sdhci_pltfm_data *pdata; >>>> @@ -67,6 +68,32 @@ static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) >>>> return readw(host->ioaddr + reg); >>>> } >>>> >>>> +static void tegra_sdhci_writew(struct sdhci_host *host, u16 val, int reg) >>>> +{ >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_tegra *tegra_host = pltfm_host->priv; >>>> + const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data; >>>> + >>>> + if (soc_data->nvquirks * NVQUIRK_SHADOW_XFER_MODE_REG) { >>> >>> Isn't the '*' supposed to be a '&' here? >> >> Yah .. not sure how that happened, but it should be '&' good catch. >> >>> >>>> + switch (reg) { >>>> + case SDHCI_TRANSFER_MODE: >>>> + /* >>>> + * Postpone this write, we must do it together with a >>>> + * command write that is down below. >>>> + */ >>>> + pltfm_host->xfer_mode_shadow = val; >>>> + return; >>>> + case SDHCI_COMMAND: >>>> + writel((val << 16) | pltfm_host->xfer_mode_shadow, >>>> + host->ioaddr + SDHCI_TRANSFER_MODE); >>>> + pltfm_host->xfer_mode_shadow = 0; >>> >>> That last line is probably not needed and could actually be harmful - >>> if we try to write SDHCI_COMMAND twice in a raw without a write to >>> SDHCI_TRANSFER_MODE in between, the zero will overwrite the previous >>> value of SDHCI_TRANSFER_MODE. >> >> True, will remove it. >> >>> >>>> + return; >>>> + } >>>> + } >>>> + >>>> + writew(val, host->ioaddr + reg); >>>> +} >>>> + >>>> static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg) >>>> { >>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> @@ -147,6 +174,7 @@ static void tegra_sdhci_set_bus_width(struct sdhci_host *host, int bus_width) >>>> static const struct sdhci_ops tegra_sdhci_ops = { >>>> .get_ro = tegra_sdhci_get_ro, >>>> .read_w = tegra_sdhci_readw, >>>> + .write_w = tegra_sdhci_writew, >>>> .write_l = tegra_sdhci_writel, >>>> .set_clock = sdhci_set_clock, >>>> .set_bus_width = tegra_sdhci_set_bus_width, >>>> @@ -201,7 +229,8 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = { >>>> .pdata = &sdhci_tegra114_pdata, >>>> .nvquirks = NVQUIRK_DISABLE_SDR50 | >>>> NVQUIRK_DISABLE_DDR50 | >>>> - NVQUIRK_DISABLE_SDR104, >>>> + NVQUIRK_DISABLE_SDR104 | >>>> + NVQUIRK_SHADOW_XFER_MODE_REG, >>>> }; >>> >>> Since this only applies to Tegra114 (?), I wonder whether it would not >>> be better to have a dedicated tegra114_sdhci_ops that implements >>> tegra_sdhci_writew, and use it only in tegra_sdhci_writew. That way >>> you could get rid of the NVQUIRK_SHADOW_XFER_MODE_REG and the test for >>> it in tegra_sdhci_writew(), and chips prior to Tegra114 will not have >>> to needlessly check for it every time they write a register. >> >> The reason I did it this way, is that this doesn't explicitly just apply >> to T114. It actually applies to T114, T124 and T132. In that case, I >> think it makes sense to keep the QUIRK and I can update the commit >> description to reflect that. > > All the same, I don't see what advantage we have checking for that > condition for every single write while we could simply set the right > function to use at probe time? The above seems like a reasonable optimization to do! Now, since $subject patch has been applied, I suggest you send a new patch addressing this issue. Kind regards Uffe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together 2015-01-27 17:23 [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together Rhyland Klein 2015-01-28 6:06 ` Alexandre Courbot @ 2015-01-28 15:02 ` Ulf Hansson 1 sibling, 0 replies; 6+ messages in thread From: Ulf Hansson @ 2015-01-28 15:02 UTC (permalink / raw) To: Rhyland Klein Cc: Chris Ball, Stephen Warren, Thierry Reding, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, pkunapuli, Alexandre Courbot On 27 January 2015 at 18:23, Rhyland Klein <rklein@nvidia.com> wrote: > From: Pavan Kunapuli <pkunapuli@nvidia.com> > > If there is a gap between xfer mode and command register writes, > tegra SDMMC controller can sometimes issue a spurious command before > the CMD register is written. To avoid this, these two registers need > to be written together in a single write operation.' Please repost to linux-mmc. Kind regards Uffe > > Signed-off-by: Pavan Kunapuli <pkunapuli@nvidia.com> > Signed-off-by: Rhyland Klein <rklein@nvidia.com> > --- > drivers/mmc/host/sdhci-tegra.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > index 59797106af93..3d34de47e57e 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -41,6 +41,7 @@ > #define NVQUIRK_DISABLE_SDR50 BIT(3) > #define NVQUIRK_DISABLE_SDR104 BIT(4) > #define NVQUIRK_DISABLE_DDR50 BIT(5) > +#define NVQUIRK_SHADOW_XFER_MODE_REG BIT(6) > > struct sdhci_tegra_soc_data { > const struct sdhci_pltfm_data *pdata; > @@ -67,6 +68,32 @@ static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) > return readw(host->ioaddr + reg); > } > > +static void tegra_sdhci_writew(struct sdhci_host *host, u16 val, int reg) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_tegra *tegra_host = pltfm_host->priv; > + const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data; > + > + if (soc_data->nvquirks * NVQUIRK_SHADOW_XFER_MODE_REG) { > + switch (reg) { > + case SDHCI_TRANSFER_MODE: > + /* > + * Postpone this write, we must do it together with a > + * command write that is down below. > + */ > + pltfm_host->xfer_mode_shadow = val; > + return; > + case SDHCI_COMMAND: > + writel((val << 16) | pltfm_host->xfer_mode_shadow, > + host->ioaddr + SDHCI_TRANSFER_MODE); > + pltfm_host->xfer_mode_shadow = 0; > + return; > + } > + } > + > + writew(val, host->ioaddr + reg); > +} > + > static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg) > { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -147,6 +174,7 @@ static void tegra_sdhci_set_bus_width(struct sdhci_host *host, int bus_width) > static const struct sdhci_ops tegra_sdhci_ops = { > .get_ro = tegra_sdhci_get_ro, > .read_w = tegra_sdhci_readw, > + .write_w = tegra_sdhci_writew, > .write_l = tegra_sdhci_writel, > .set_clock = sdhci_set_clock, > .set_bus_width = tegra_sdhci_set_bus_width, > @@ -201,7 +229,8 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = { > .pdata = &sdhci_tegra114_pdata, > .nvquirks = NVQUIRK_DISABLE_SDR50 | > NVQUIRK_DISABLE_DDR50 | > - NVQUIRK_DISABLE_SDR104, > + NVQUIRK_DISABLE_SDR104 | > + NVQUIRK_SHADOW_XFER_MODE_REG, > }; > > static const struct of_device_id sdhci_tegra_dt_match[] = { > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-10 7:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-27 17:23 [PATCH] mmc: tegra: Write xfer_mode, CMD regs in together Rhyland Klein 2015-01-28 6:06 ` Alexandre Courbot 2015-01-28 16:30 ` Rhyland Klein 2015-02-09 6:22 ` Alexandre Courbot 2015-02-10 7:44 ` Ulf Hansson 2015-01-28 15:02 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox