| Message ID | 20211129111047.5299-1-pshete@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mmc: sdhci-tegra: Add support to enumerate in HS400ES mode | expand |
On 29/11/2021 13:10, Prathamesh Shete wrote: > When CMD13 is sent after switching to HS400 mode, the bus > is operating at either MMC_HIGH_26_MAX_DTR or MMC_HIGH_52_MAX_DTR. > To meet Tegra SDHCI requirement at HS400 mode, force SDHCI > interface clock to MMC_HS200_MAX_DTR (200 MHz) so that host > controller CAR clock and the interface clock are rate matched. This sounds like an issue with or without enhanced strobe? Also it seems more like a fix than "adding support". i.e. is this really "mmc: sdhci-tegra: Fix switch to HS400ES mode" ? > > Signed-off-by: Prathamesh Shete <pshete@nvidia.com> > --- > drivers/mmc/host/sdhci-tegra.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > index 387ce9cdbd7c..d800396d1112 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -123,6 +123,8 @@ > SDHCI_TRNS_BLK_CNT_EN | \ > SDHCI_TRNS_DMA) > > +static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock); Please move one of the functions so that the forward declaration is not necessary. > + > struct sdhci_tegra_soc_data { > const struct sdhci_pltfm_data *pdata; > u64 dma_mask; > @@ -369,6 +371,16 @@ static void tegra_sdhci_hs400_enhanced_strobe(struct mmc_host *mmc, > > sdhci_writel(host, val, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL); > > + /* > + * When CMD13 is sent after switching to HS400 mode, the bus > + * is operating at either MMC_HIGH_26_MAX_DTR or > + * MMC_HIGH_52_MAX_DTR. > + * To meet Tegra SDHCI requirement at HS400 mode, force SDHCI > + * interface clock to MMC_HS200_MAX_DTR (200 MHz) so that host > + * controller CAR clock and the interface clock are rate matched. > + */ > + tegra_sdhci_set_clock(host, MMC_HS200_MAX_DTR); This being done irrespective of ios->enhanced_strobe, which doesn't seem right based on the comment? > + Please remove the blank line. > } > > static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask) >
Thanks for review This issue is specific to HS400ES mode. Updated and pushed version v2. Updated commit message and comments to clarify on HS400ES mode. Also moved the function as suggested. Answers to some questions inlined. Thanks Prathamesh. -----Original Message----- From: Adrian Hunter <adrian.hunter@intel.com> Sent: Thursday, December 2, 2021 12:10 PM To: Prathamesh Shete <pshete@nvidia.com>; ulf.hansson@linaro.org; thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>; p.zabel@pengutronix.de; linux-mmc@vger.kernel.org; linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org Cc: Aniruddha Tvs Rao <anrao@nvidia.com> Subject: Re: [PATCH] mmc: sdhci-tegra: Add support to enumerate in HS400ES mode External email: Use caution opening links or attachments On 29/11/2021 13:10, Prathamesh Shete wrote: > When CMD13 is sent after switching to HS400 mode, the bus is operating > at either MMC_HIGH_26_MAX_DTR or MMC_HIGH_52_MAX_DTR. > To meet Tegra SDHCI requirement at HS400 mode, force SDHCI interface > clock to MMC_HS200_MAX_DTR (200 MHz) so that host controller CAR clock > and the interface clock are rate matched. This sounds like an issue with or without enhanced strobe? [P.S]issue is specific to HS400ES mode updated commit message in v2. Also it seems more like a fix than "adding support". i.e. is this really "mmc: sdhci-tegra: Fix switch to HS400ES mode" ? [P.S] yes this is fix ... updated commit subject in version v2. > > Signed-off-by: Prathamesh Shete <pshete@nvidia.com> > --- > drivers/mmc/host/sdhci-tegra.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-tegra.c > b/drivers/mmc/host/sdhci-tegra.c index 387ce9cdbd7c..d800396d1112 > 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -123,6 +123,8 @@ > SDHCI_TRNS_BLK_CNT_EN | \ > SDHCI_TRNS_DMA) > > +static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned > +int clock); Please move one of the functions so that the forward declaration is not necessary. [P.S]Done. > + > struct sdhci_tegra_soc_data { > const struct sdhci_pltfm_data *pdata; > u64 dma_mask; > @@ -369,6 +371,16 @@ static void > tegra_sdhci_hs400_enhanced_strobe(struct mmc_host *mmc, > > sdhci_writel(host, val, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL); > > + /* > + * When CMD13 is sent after switching to HS400 mode, the bus > + * is operating at either MMC_HIGH_26_MAX_DTR or > + * MMC_HIGH_52_MAX_DTR. > + * To meet Tegra SDHCI requirement at HS400 mode, force SDHCI > + * interface clock to MMC_HS200_MAX_DTR (200 MHz) so that host > + * controller CAR clock and the interface clock are rate matched. > + */ > + tegra_sdhci_set_clock(host, MMC_HS200_MAX_DTR); This being done irrespective of ios->enhanced_strobe, which doesn't seem right based on the comment? [P.S]Updated comments. > + Please remove the blank line. [P.S]done > } > > static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask) >
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 387ce9cdbd7c..d800396d1112 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -123,6 +123,8 @@ SDHCI_TRNS_BLK_CNT_EN | \ SDHCI_TRNS_DMA) +static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock); + struct sdhci_tegra_soc_data { const struct sdhci_pltfm_data *pdata; u64 dma_mask; @@ -369,6 +371,16 @@ static void tegra_sdhci_hs400_enhanced_strobe(struct mmc_host *mmc, sdhci_writel(host, val, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL); + /* + * When CMD13 is sent after switching to HS400 mode, the bus + * is operating at either MMC_HIGH_26_MAX_DTR or + * MMC_HIGH_52_MAX_DTR. + * To meet Tegra SDHCI requirement at HS400 mode, force SDHCI + * interface clock to MMC_HS200_MAX_DTR (200 MHz) so that host + * controller CAR clock and the interface clock are rate matched. + */ + tegra_sdhci_set_clock(host, MMC_HS200_MAX_DTR); + } static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
When CMD13 is sent after switching to HS400 mode, the bus is operating at either MMC_HIGH_26_MAX_DTR or MMC_HIGH_52_MAX_DTR. To meet Tegra SDHCI requirement at HS400 mode, force SDHCI interface clock to MMC_HS200_MAX_DTR (200 MHz) so that host controller CAR clock and the interface clock are rate matched. Signed-off-by: Prathamesh Shete <pshete@nvidia.com> --- drivers/mmc/host/sdhci-tegra.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)