[PATCH v2 1/4] mmc: sdhci-tegra: Separate T19x and T23x SoC data

[PATCH v2 1/4] mmc: sdhci-tegra: Separate T19x and T23x SoC data

[Prathamesh Shete]

Create new SoC data structure for T23x platforms.

StreamID programming is one of the additional feature
added in Tegra234 and later chips

Signed-off-by: Aniruddha Tvs Rao <anrao@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 2d2d8260c681..a6c5bbae77b4 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -1556,7 +1556,21 @@ static const struct sdhci_tegra_soc_data soc_data_tegra194 = {
 	.max_tap_delay = 139,
 };
 
+static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
+	.pdata = &sdhci_tegra186_pdata,
+	.dma_mask = DMA_BIT_MASK(39),
+	.nvquirks = NVQUIRK_NEEDS_PAD_CONTROL |
+		    NVQUIRK_HAS_PADCALIB |
+		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
+		    NVQUIRK_ENABLE_SDR50 |
+		    NVQUIRK_ENABLE_SDR104 |
+		    NVQUIRK_HAS_TMCLK,
+	.min_tap_delay = 95,
+	.max_tap_delay = 111,
+};
+
 static const struct of_device_id sdhci_tegra_dt_match[] = {
+	{ .compatible = "nvidia,tegra234-sdhci", .data = &soc_data_tegra234 },
 	{ .compatible = "nvidia,tegra194-sdhci", .data = &soc_data_tegra194 },
 	{ .compatible = "nvidia,tegra186-sdhci", .data = &soc_data_tegra186 },
 	{ .compatible = "nvidia,tegra210-sdhci", .data = &soc_data_tegra210 },
-- 
2.17.1

[PATCH v2 2/4] mmc: sdhci-tegra: Add support to program MC streamID

[Prathamesh Shete]

As per T23x MSS IAS SMMU clients are supposed
to program streamid from their respective address spaces instead of MC
override
Define NVQUIRK_PROGRAM_MC_STREAMID and use it to program SMMU streamid
from the SDMMC client address space

Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index a6c5bbae77b4..b66b0cc51497 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -25,6 +25,7 @@
 #include <linux/mmc/slot-gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/ktime.h>
+#include <linux/iommu.h>
 
 #include <soc/tegra/common.h>
 
@@ -94,6 +95,8 @@
 #define SDHCI_TEGRA_AUTO_CAL_STATUS			0x1ec
 #define SDHCI_TEGRA_AUTO_CAL_ACTIVE			BIT(31)
 
+#define SDHCI_TEGRA_CIF2AXI_CTRL_0			0x1fc
+
 #define NVQUIRK_FORCE_SDHCI_SPEC_200			BIT(0)
 #define NVQUIRK_ENABLE_BLOCK_GAP_DET			BIT(1)
 #define NVQUIRK_ENABLE_SDHCI_SPEC_300			BIT(2)
@@ -121,6 +124,7 @@
 #define NVQUIRK_HAS_TMCLK				BIT(10)
 
 #define NVQUIRK_HAS_ANDROID_GPT_SECTOR			BIT(11)
+#define NVQUIRK_PROGRAM_MC_STREAMID			BIT(17)
 
 /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
 #define SDHCI_TEGRA_CQE_BASE_ADDR			0xF000
@@ -177,6 +181,7 @@ struct sdhci_tegra {
 	bool enable_hwcq;
 	unsigned long curr_clk_rate;
 	u8 tuned_tap_delay;
+	u32 streamid;
 };
 
 static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
@@ -1564,6 +1569,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
 		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
 		    NVQUIRK_ENABLE_SDR50 |
 		    NVQUIRK_ENABLE_SDR104 |
+		    NVQUIRK_PROGRAM_MC_STREAMID |
 		    NVQUIRK_HAS_TMCLK,
 	.min_tap_delay = 95,
 	.max_tap_delay = 111,
@@ -1637,6 +1643,7 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_tegra *tegra_host;
 	struct clk *clk;
+	struct iommu_fwspec *fwspec;
 	int rc;
 
 	soc_data = of_device_get_match_data(&pdev->dev);
@@ -1775,6 +1782,23 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 	if (rc)
 		goto err_add_host;
 
+	/* Program MC streamID for DMA transfers */
+	if (soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) {
+		fwspec = dev_iommu_fwspec_get(&pdev->dev);
+		if (fwspec == NULL) {
+			rc = -ENODEV;
+			dev_err(mmc_dev(host->mmc),
+				"failed to get MC streamid: %d\n",
+				rc);
+			goto err_rst_get;
+		} else {
+			tegra_host->streamid = fwspec->ids[0] & 0xffff;
+			tegra_sdhci_writel(host, tegra_host->streamid |
+						(tegra_host->streamid << 8),
+						SDHCI_TEGRA_CIF2AXI_CTRL_0);
+		}
+	}
+
 	return 0;
 
 err_add_host:
@@ -1861,6 +1885,8 @@ static int sdhci_tegra_suspend(struct device *dev)
 static int sdhci_tegra_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
 	ret = mmc_gpio_set_cd_wake(host->mmc, false);
@@ -1871,6 +1897,13 @@ static int sdhci_tegra_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* Re-program MC streamID for DMA transfers */
+	if (tegra_host->soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) {
+		tegra_sdhci_writel(host, tegra_host->streamid |
+					(tegra_host->streamid << 8),
+					SDHCI_TEGRA_CIF2AXI_CTRL_0);
+	}
+
 	ret = sdhci_resume_host(host);
 	if (ret)
 		goto disable_clk;
-- 
2.17.1

[PATCH v2 3/4] mmc: sdhci-tegra: Issue CMD and DAT resets together

[Prathamesh Shete]

In case of error condition to avoid system crash
Tegra SDMMC controller requires CMD and DAT resets
issued together.

This is applicable to Tegra186 and later chips.

Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c |  3 ++-
 drivers/mmc/host/sdhci.c       | 11 ++++++++---
 drivers/mmc/host/sdhci.h       |  2 ++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index b66b0cc51497..7d16dc41fe91 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -1530,7 +1530,8 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
 		  SDHCI_QUIRK_NO_HISPD_BIT |
 		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
 		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+		   SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER,
 	.ops  = &tegra186_sdhci_ops,
 };
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7689ffec5ad1..289fa8ae4866 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3063,9 +3063,14 @@ static bool sdhci_request_done(struct sdhci_host *host)
 		 * Spec says we should do both at the same time, but Ricoh
 		 * controllers do not like that.
 		 */
-		sdhci_do_reset(host, SDHCI_RESET_CMD);
-		sdhci_do_reset(host, SDHCI_RESET_DATA);
-
+		if (host->quirks2 &
+			SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER) {
+			sdhci_do_reset(host, SDHCI_RESET_CMD |
+					SDHCI_RESET_DATA);
+		} else {
+			sdhci_do_reset(host, SDHCI_RESET_CMD);
+			sdhci_do_reset(host, SDHCI_RESET_DATA);
+		}
 		host->pending_reset = false;
 	}
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 95a08f09df30..8045308f7859 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -480,6 +480,8 @@ struct sdhci_host {
  * block count.
  */
 #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
+/* Issue CMD and DATA reset together */
+#define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER      (1<<19)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.17.1

[PATCH v2 4/4] mmc: sdhci-tegra: Use actual clock rate for SW tuning correction

[Prathamesh Shete]

Ensure tegra_host member "curr_clk_rate" holds the actual clock rate
instead of requested clock rate for proper use during tuning correction
algorithm.

Fixes: ea8fc5953e8b ("mmc: tegra: update hw tuning process")

Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 7d16dc41fe91..42b018d4ebc3 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -778,7 +778,7 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 		dev_err(dev, "failed to set clk rate to %luHz: %d\n",
 			host_clk, err);
 
-	tegra_host->curr_clk_rate = host_clk;
+	tegra_host->curr_clk_rate = clk_get_rate(pltfm_host->clk);
 	if (tegra_host->ddr_signaling)
 		host->max_clk = host_clk;
 	else
-- 
2.17.1

Re: [PATCH v2 1/4] mmc: sdhci-tegra: Separate T19x and T23x SoC data

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

On Wed, Sep 14, 2022 at 03:26:25PM +0530, Prathamesh Shete wrote:
> Create new SoC data structure for T23x platforms.

Can you please use consistent spelling of Tegra194 and Tegra234 here and
in the subject? That makes it a bit easier to search for things.

> StreamID programming is one of the additional feature
> added in Tegra234 and later chips

That's a bit confusing because this patch doesn't do anything with
stream ID programming. Perhaps defer that comment to when it becomes
relevant?

Thierry

> 
> Signed-off-by: Aniruddha Tvs Rao <anrao@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 2d2d8260c681..a6c5bbae77b4 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -1556,7 +1556,21 @@ static const struct sdhci_tegra_soc_data soc_data_tegra194 = {
>  	.max_tap_delay = 139,
>  };
>  
> +static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
> +	.pdata = &sdhci_tegra186_pdata,
> +	.dma_mask = DMA_BIT_MASK(39),
> +	.nvquirks = NVQUIRK_NEEDS_PAD_CONTROL |
> +		    NVQUIRK_HAS_PADCALIB |
> +		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
> +		    NVQUIRK_ENABLE_SDR50 |
> +		    NVQUIRK_ENABLE_SDR104 |
> +		    NVQUIRK_HAS_TMCLK,
> +	.min_tap_delay = 95,
> +	.max_tap_delay = 111,
> +};
> +
>  static const struct of_device_id sdhci_tegra_dt_match[] = {
> +	{ .compatible = "nvidia,tegra234-sdhci", .data = &soc_data_tegra234 },
>  	{ .compatible = "nvidia,tegra194-sdhci", .data = &soc_data_tegra194 },
>  	{ .compatible = "nvidia,tegra186-sdhci", .data = &soc_data_tegra186 },
>  	{ .compatible = "nvidia,tegra210-sdhci", .data = &soc_data_tegra210 },
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/4] mmc: sdhci-tegra: Add support to program MC streamID

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5983 bytes --]

On Wed, Sep 14, 2022 at 03:26:26PM +0530, Prathamesh Shete wrote:
> As per T23x MSS IAS SMMU clients are supposed

This reference isn't useful because this document is not available
publicly. If this information exists in the TRM, then make a reference
to that, otherwise just leave out the reference and keep the rest of the
comment.

> to program streamid from their respective address spaces instead of MC

s/streamid/stream ID/ to match the ARM SMMU spelling.

> override
> Define NVQUIRK_PROGRAM_MC_STREAMID and use it to program SMMU streamid
> from the SDMMC client address space

Maybe make this all look more like one big paragraph. Right now it looks
fragments of sentences thrown together and is difficult to read.

> 
> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index a6c5bbae77b4..b66b0cc51497 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -25,6 +25,7 @@
>  #include <linux/mmc/slot-gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/ktime.h>
> +#include <linux/iommu.h>
>  
>  #include <soc/tegra/common.h>
>  
> @@ -94,6 +95,8 @@
>  #define SDHCI_TEGRA_AUTO_CAL_STATUS			0x1ec
>  #define SDHCI_TEGRA_AUTO_CAL_ACTIVE			BIT(31)
>  
> +#define SDHCI_TEGRA_CIF2AXI_CTRL_0			0x1fc
> +
>  #define NVQUIRK_FORCE_SDHCI_SPEC_200			BIT(0)
>  #define NVQUIRK_ENABLE_BLOCK_GAP_DET			BIT(1)
>  #define NVQUIRK_ENABLE_SDHCI_SPEC_300			BIT(2)
> @@ -121,6 +124,7 @@
>  #define NVQUIRK_HAS_TMCLK				BIT(10)
>  
>  #define NVQUIRK_HAS_ANDROID_GPT_SECTOR			BIT(11)
> +#define NVQUIRK_PROGRAM_MC_STREAMID			BIT(17)

Why is this called "program MC stream ID"? What's programmed is the SMMU
stream ID, right? Perhaps just leave out that MC_ prefix altogether
since there's no ambiguity with any other quirk to begin with.

Also, why skip from bit 11 (of the GPT sector quirk) to bit 17? Can we
not use bit 12 as the next one?

>  
>  /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
>  #define SDHCI_TEGRA_CQE_BASE_ADDR			0xF000
> @@ -177,6 +181,7 @@ struct sdhci_tegra {
>  	bool enable_hwcq;
>  	unsigned long curr_clk_rate;
>  	u8 tuned_tap_delay;
> +	u32 streamid;
>  };
>  
>  static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
> @@ -1564,6 +1569,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
>  		    NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
>  		    NVQUIRK_ENABLE_SDR50 |
>  		    NVQUIRK_ENABLE_SDR104 |
> +		    NVQUIRK_PROGRAM_MC_STREAMID |
>  		    NVQUIRK_HAS_TMCLK,
>  	.min_tap_delay = 95,
>  	.max_tap_delay = 111,
> @@ -1637,6 +1643,7 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>  	struct sdhci_pltfm_host *pltfm_host;
>  	struct sdhci_tegra *tegra_host;
>  	struct clk *clk;
> +	struct iommu_fwspec *fwspec;

The above is largely sorted in reverse christmas tree order, so this one
sticks out a bit. Maybe put it before the clk declaration. Not usually a
big deal, really, but since you're going to touch this anyway, may as
well touch that up.

>  	int rc;
>  
>  	soc_data = of_device_get_match_data(&pdev->dev);
> @@ -1775,6 +1782,23 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>  	if (rc)
>  		goto err_add_host;
>  
> +	/* Program MC streamID for DMA transfers */
> +	if (soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) {
> +		fwspec = dev_iommu_fwspec_get(&pdev->dev);
> +		if (fwspec == NULL) {
> +			rc = -ENODEV;
> +			dev_err(mmc_dev(host->mmc),
> +				"failed to get MC streamid: %d\n",
> +				rc);
> +			goto err_rst_get;

Do we really want to make this fatal? What if somebody really wants to
not put the SD/MMC controllers behind an IOMMU? It's quite unlikely to
happen, but it's technically possible.

Also, there was a brief time where the DTS files didn't have any iommus
properties, so if somebody were to use a DTS from that era and a kernel
with this patch applied, they'd end up with non-functional SD/MMC.
Again, that's very unlikely to happen, but it could, if for example this
patch ends up being back-ported or something like that.

I think it's safe (and easier) to ignore this case. Perhaps if you
really want people to use SD/MMC you may want to add a warning here
instead. But even that shouldn't be necessary. If the stream ID is not
programmed as required, the SMMU should fault and give people the hint
that they need to fix this.

> +		} else {
> +			tegra_host->streamid = fwspec->ids[0] & 0xffff;
> +			tegra_sdhci_writel(host, tegra_host->streamid |
> +						(tegra_host->streamid << 8),
> +						SDHCI_TEGRA_CIF2AXI_CTRL_0);

Perhaps define macros for the read/write stream ID fields in this
register? Otherwise it might be confusing why you're writing the value
twice.

Thierry

> +		}
> +	}
> +
>  	return 0;
>  
>  err_add_host:
> @@ -1861,6 +1885,8 @@ static int sdhci_tegra_suspend(struct device *dev)
>  static int sdhci_tegra_resume(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
>  	int ret;
>  
>  	ret = mmc_gpio_set_cd_wake(host->mmc, false);
> @@ -1871,6 +1897,13 @@ static int sdhci_tegra_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Re-program MC streamID for DMA transfers */
> +	if (tegra_host->soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) {
> +		tegra_sdhci_writel(host, tegra_host->streamid |
> +					(tegra_host->streamid << 8),
> +					SDHCI_TEGRA_CIF2AXI_CTRL_0);
> +	}
> +
>  	ret = sdhci_resume_host(host);
>  	if (ret)
>  		goto disable_clk;
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] mmc: sdhci-tegra: Issue CMD and DAT resets together

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

On Wed, Sep 14, 2022 at 03:26:27PM +0530, Prathamesh Shete wrote:
> In case of error condition to avoid system crash
> Tegra SDMMC controller requires CMD and DAT resets
> issued together.

It might be worth specifying exactly what "system crash" means. Does
this always happen (i.e. do we have a problem right now?) or are there
specific circumstances that cause the crash.

> This is applicable to Tegra186 and later chips.
> 
> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c |  3 ++-
>  drivers/mmc/host/sdhci.c       | 11 ++++++++---
>  drivers/mmc/host/sdhci.h       |  2 ++
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index b66b0cc51497..7d16dc41fe91 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -1530,7 +1530,8 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
>  		  SDHCI_QUIRK_NO_HISPD_BIT |
>  		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
>  		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> -	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> +		   SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER,
>  	.ops  = &tegra186_sdhci_ops,
>  };
>  
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 7689ffec5ad1..289fa8ae4866 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3063,9 +3063,14 @@ static bool sdhci_request_done(struct sdhci_host *host)
>  		 * Spec says we should do both at the same time, but Ricoh
>  		 * controllers do not like that.
>  		 */

The comment above seems to indicate that the current behavior (i.e.
splitting the CMD and DATA resets) is actually the quirk, so I wonder if
this perhaps should be reversed? I suppose it could be difficult to
track down the exact controllers that need the separate resets, but this
might be worth doing. It's possible that other controllers might run
into the same issue that we are if they work strictly to the spec.

Adrian, any ideas on how much of this is just cargo-culted? Do we play
it safe and do the "double workaround" or do we want to attempt to
rectify this by adding a Ricoh-specific quirk?

Thierry

> -		sdhci_do_reset(host, SDHCI_RESET_CMD);
> -		sdhci_do_reset(host, SDHCI_RESET_DATA);
> -
> +		if (host->quirks2 &
> +			SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER) {
> +			sdhci_do_reset(host, SDHCI_RESET_CMD |
> +					SDHCI_RESET_DATA);
> +		} else {
> +			sdhci_do_reset(host, SDHCI_RESET_CMD);
> +			sdhci_do_reset(host, SDHCI_RESET_DATA);
> +		}
>  		host->pending_reset = false;
>  	}
>  
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 95a08f09df30..8045308f7859 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -480,6 +480,8 @@ struct sdhci_host {
>   * block count.
>   */
>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
> +/* Issue CMD and DATA reset together */
> +#define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER      (1<<19)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/4] mmc: sdhci-tegra: Use actual clock rate for SW tuning correction

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

On Wed, Sep 14, 2022 at 03:26:28PM +0530, Prathamesh Shete wrote:
> Ensure tegra_host member "curr_clk_rate" holds the actual clock rate
> instead of requested clock rate for proper use during tuning correction
> algorithm.

Ideally there shouldn't be a deviation between host_clk and the actual
clock rate that was set. Perhaps it'd be good to provide a bit more
information on what the deviation can be and when that happens, as well
as what the consequences are. That would make it a bit more obvious why
this fix is needed.

Thierry

> 
> Fixes: ea8fc5953e8b ("mmc: tegra: update hw tuning process")
> 
> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 7d16dc41fe91..42b018d4ebc3 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -778,7 +778,7 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>  		dev_err(dev, "failed to set clk rate to %luHz: %d\n",
>  			host_clk, err);
>  
> -	tegra_host->curr_clk_rate = host_clk;
> +	tegra_host->curr_clk_rate = clk_get_rate(pltfm_host->clk);
>  	if (tegra_host->ddr_signaling)
>  		host->max_clk = host_clk;
>  	else
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] mmc: sdhci-tegra: Issue CMD and DAT resets together

[Adrian Hunter]

On 14/09/22 15:20, Thierry Reding wrote:
> On Wed, Sep 14, 2022 at 03:26:27PM +0530, Prathamesh Shete wrote:
>> In case of error condition to avoid system crash
>> Tegra SDMMC controller requires CMD and DAT resets
>> issued together.
> 
> It might be worth specifying exactly what "system crash" means. Does
> this always happen (i.e. do we have a problem right now?) or are there
> specific circumstances that cause the crash.
> 
>> This is applicable to Tegra186 and later chips.
>>
>> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
>> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
>> ---
>>  drivers/mmc/host/sdhci-tegra.c |  3 ++-
>>  drivers/mmc/host/sdhci.c       | 11 ++++++++---
>>  drivers/mmc/host/sdhci.h       |  2 ++
>>  3 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
>> index b66b0cc51497..7d16dc41fe91 100644
>> --- a/drivers/mmc/host/sdhci-tegra.c
>> +++ b/drivers/mmc/host/sdhci-tegra.c
>> @@ -1530,7 +1530,8 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
>>  		  SDHCI_QUIRK_NO_HISPD_BIT |
>>  		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
>>  		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>> -	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
>> +		   SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER,
>>  	.ops  = &tegra186_sdhci_ops,
>>  };
>>  
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 7689ffec5ad1..289fa8ae4866 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -3063,9 +3063,14 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>  		 * Spec says we should do both at the same time, but Ricoh
>>  		 * controllers do not like that.
>>  		 */
> 
> The comment above seems to indicate that the current behavior (i.e.
> splitting the CMD and DATA resets) is actually the quirk, so I wonder if
> this perhaps should be reversed? I suppose it could be difficult to
> track down the exact controllers that need the separate resets, but this
> might be worth doing. It's possible that other controllers might run
> into the same issue that we are if they work strictly to the spec.
> 
> Adrian, any ideas on how much of this is just cargo-culted? Do we play
> it safe and do the "double workaround" or do we want to attempt to
> rectify this by adding a Ricoh-specific quirk?

It is a good question, but it has been that way for a very long time,
and the spec tends to document them separately anyway, so it doesn't
seem there is much reason to change.

> 
> Thierry
> 
>> -		sdhci_do_reset(host, SDHCI_RESET_CMD);
>> -		sdhci_do_reset(host, SDHCI_RESET_DATA);
>> -
>> +		if (host->quirks2 &
>> +			SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER) {
>> +			sdhci_do_reset(host, SDHCI_RESET_CMD |
>> +					SDHCI_RESET_DATA);
>> +		} else {
>> +			sdhci_do_reset(host, SDHCI_RESET_CMD);
>> +			sdhci_do_reset(host, SDHCI_RESET_DATA);
>> +		}
>>  		host->pending_reset = false;
>>  	}
>>  
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 95a08f09df30..8045308f7859 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -480,6 +480,8 @@ struct sdhci_host {
>>   * block count.
>>   */
>>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
>> +/* Issue CMD and DATA reset together */
>> +#define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER      (1<<19)
>>  
>>  	int irq;		/* Device IRQ */
>>  	void __iomem *ioaddr;	/* Mapped address */
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 3/4] mmc: sdhci-tegra: Issue CMD and DAT resets together

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2983 bytes --]

On Wed, Sep 14, 2022 at 09:21:07PM +0300, Adrian Hunter wrote:
> On 14/09/22 15:20, Thierry Reding wrote:
> > On Wed, Sep 14, 2022 at 03:26:27PM +0530, Prathamesh Shete wrote:
> >> In case of error condition to avoid system crash
> >> Tegra SDMMC controller requires CMD and DAT resets
> >> issued together.
> > 
> > It might be worth specifying exactly what "system crash" means. Does
> > this always happen (i.e. do we have a problem right now?) or are there
> > specific circumstances that cause the crash.
> > 
> >> This is applicable to Tegra186 and later chips.
> >>
> >> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> >> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> >> ---
> >>  drivers/mmc/host/sdhci-tegra.c |  3 ++-
> >>  drivers/mmc/host/sdhci.c       | 11 ++++++++---
> >>  drivers/mmc/host/sdhci.h       |  2 ++
> >>  3 files changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> >> index b66b0cc51497..7d16dc41fe91 100644
> >> --- a/drivers/mmc/host/sdhci-tegra.c
> >> +++ b/drivers/mmc/host/sdhci-tegra.c
> >> @@ -1530,7 +1530,8 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
> >>  		  SDHCI_QUIRK_NO_HISPD_BIT |
> >>  		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
> >>  		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> >> -	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> >> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> >> +		   SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER,
> >>  	.ops  = &tegra186_sdhci_ops,
> >>  };
> >>  
> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> index 7689ffec5ad1..289fa8ae4866 100644
> >> --- a/drivers/mmc/host/sdhci.c
> >> +++ b/drivers/mmc/host/sdhci.c
> >> @@ -3063,9 +3063,14 @@ static bool sdhci_request_done(struct sdhci_host *host)
> >>  		 * Spec says we should do both at the same time, but Ricoh
> >>  		 * controllers do not like that.
> >>  		 */
> > 
> > The comment above seems to indicate that the current behavior (i.e.
> > splitting the CMD and DATA resets) is actually the quirk, so I wonder if
> > this perhaps should be reversed? I suppose it could be difficult to
> > track down the exact controllers that need the separate resets, but this
> > might be worth doing. It's possible that other controllers might run
> > into the same issue that we are if they work strictly to the spec.
> > 
> > Adrian, any ideas on how much of this is just cargo-culted? Do we play
> > it safe and do the "double workaround" or do we want to attempt to
> > rectify this by adding a Ricoh-specific quirk?
> 
> It is a good question, but it has been that way for a very long time,
> and the spec tends to document them separately anyway, so it doesn't
> seem there is much reason to change.

Fair enough. Prathamesh, perhaps revise the comment above as part of
this patch because with the change below it now sounds a bit confusing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/4] mmc: sdhci-tegra: Add support to program MC streamID

[kernel test robot]

Hi Prathamesh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on linus/master v6.0-rc5 next-20220915]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Prathamesh-Shete/mmc-sdhci-tegra-Separate-T19x-and-T23x-SoC-data/20220914-175832
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: hexagon-randconfig-r045-20220915 (https://download.01.org/0day-ci/archive/20220916/202209160420.bcAdUgK2-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/9817941f43bffe4de367526928377b9723068fb5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Prathamesh-Shete/mmc-sdhci-tegra-Separate-T19x-and-T23x-SoC-data/20220914-175832
        git checkout 9817941f43bffe4de367526928377b9723068fb5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/mmc/host/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/mmc/host/sdhci-tegra.c:1795:35: error: no member named 'ids' in 'struct iommu_fwspec'
                           tegra_host->streamid = fwspec->ids[0] & 0xffff;
                                                  ~~~~~~  ^
   1 error generated.


vim +1795 drivers/mmc/host/sdhci-tegra.c

  1638	
  1639	static int sdhci_tegra_probe(struct platform_device *pdev)
  1640	{
  1641		const struct sdhci_tegra_soc_data *soc_data;
  1642		struct sdhci_host *host;
  1643		struct sdhci_pltfm_host *pltfm_host;
  1644		struct sdhci_tegra *tegra_host;
  1645		struct clk *clk;
  1646		struct iommu_fwspec *fwspec;
  1647		int rc;
  1648	
  1649		soc_data = of_device_get_match_data(&pdev->dev);
  1650		if (!soc_data)
  1651			return -EINVAL;
  1652	
  1653		host = sdhci_pltfm_init(pdev, soc_data->pdata, sizeof(*tegra_host));
  1654		if (IS_ERR(host))
  1655			return PTR_ERR(host);
  1656		pltfm_host = sdhci_priv(host);
  1657	
  1658		tegra_host = sdhci_pltfm_priv(pltfm_host);
  1659		tegra_host->ddr_signaling = false;
  1660		tegra_host->pad_calib_required = false;
  1661		tegra_host->pad_control_available = false;
  1662		tegra_host->soc_data = soc_data;
  1663	
  1664		if (soc_data->nvquirks & NVQUIRK_HAS_ANDROID_GPT_SECTOR)
  1665			host->mmc->caps2 |= MMC_CAP2_ALT_GPT_TEGRA;
  1666	
  1667		if (soc_data->nvquirks & NVQUIRK_NEEDS_PAD_CONTROL) {
  1668			rc = tegra_sdhci_init_pinctrl_info(&pdev->dev, tegra_host);
  1669			if (rc == 0)
  1670				host->mmc_host_ops.start_signal_voltage_switch =
  1671					sdhci_tegra_start_signal_voltage_switch;
  1672		}
  1673	
  1674		/* Hook to periodically rerun pad calibration */
  1675		if (soc_data->nvquirks & NVQUIRK_HAS_PADCALIB)
  1676			host->mmc_host_ops.request = tegra_sdhci_request;
  1677	
  1678		host->mmc_host_ops.hs400_enhanced_strobe =
  1679				tegra_sdhci_hs400_enhanced_strobe;
  1680	
  1681		if (!host->ops->platform_execute_tuning)
  1682			host->mmc_host_ops.execute_tuning =
  1683					tegra_sdhci_execute_hw_tuning;
  1684	
  1685		rc = mmc_of_parse(host->mmc);
  1686		if (rc)
  1687			goto err_parse_dt;
  1688	
  1689		if (tegra_host->soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
  1690			host->mmc->caps |= MMC_CAP_1_8V_DDR;
  1691	
  1692		/* HW busy detection is supported, but R1B responses are required. */
  1693		host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY;
  1694	
  1695		/* GPIO CD can be set as a wakeup source */
  1696		host->mmc->caps |= MMC_CAP_CD_WAKE;
  1697	
  1698		tegra_sdhci_parse_dt(host);
  1699	
  1700		tegra_host->power_gpio = devm_gpiod_get_optional(&pdev->dev, "power",
  1701								 GPIOD_OUT_HIGH);
  1702		if (IS_ERR(tegra_host->power_gpio)) {
  1703			rc = PTR_ERR(tegra_host->power_gpio);
  1704			goto err_power_req;
  1705		}
  1706	
  1707		/*
  1708		 * Tegra210 has a separate SDMMC_LEGACY_TM clock used for host
  1709		 * timeout clock and SW can choose TMCLK or SDCLK for hardware
  1710		 * data timeout through the bit USE_TMCLK_FOR_DATA_TIMEOUT of
  1711		 * the register SDHCI_TEGRA_VENDOR_SYS_SW_CTRL.
  1712		 *
  1713		 * USE_TMCLK_FOR_DATA_TIMEOUT bit default is set to 1 and SDMMC uses
  1714		 * 12Mhz TMCLK which is advertised in host capability register.
  1715		 * With TMCLK of 12Mhz provides maximum data timeout period that can
  1716		 * be achieved is 11s better than using SDCLK for data timeout.
  1717		 *
  1718		 * So, TMCLK is set to 12Mhz and kept enabled all the time on SoC's
  1719		 * supporting separate TMCLK.
  1720		 */
  1721	
  1722		if (soc_data->nvquirks & NVQUIRK_HAS_TMCLK) {
  1723			clk = devm_clk_get(&pdev->dev, "tmclk");
  1724			if (IS_ERR(clk)) {
  1725				rc = PTR_ERR(clk);
  1726				if (rc == -EPROBE_DEFER)
  1727					goto err_power_req;
  1728	
  1729				dev_warn(&pdev->dev, "failed to get tmclk: %d\n", rc);
  1730				clk = NULL;
  1731			}
  1732	
  1733			clk_set_rate(clk, 12000000);
  1734			rc = clk_prepare_enable(clk);
  1735			if (rc) {
  1736				dev_err(&pdev->dev,
  1737					"failed to enable tmclk: %d\n", rc);
  1738				goto err_power_req;
  1739			}
  1740	
  1741			tegra_host->tmclk = clk;
  1742		}
  1743	
  1744		clk = devm_clk_get(mmc_dev(host->mmc), NULL);
  1745		if (IS_ERR(clk)) {
  1746			rc = dev_err_probe(&pdev->dev, PTR_ERR(clk),
  1747					   "failed to get clock\n");
  1748			goto err_clk_get;
  1749		}
  1750		pltfm_host->clk = clk;
  1751	
  1752		tegra_host->rst = devm_reset_control_get_exclusive(&pdev->dev,
  1753								   "sdhci");
  1754		if (IS_ERR(tegra_host->rst)) {
  1755			rc = PTR_ERR(tegra_host->rst);
  1756			dev_err(&pdev->dev, "failed to get reset control: %d\n", rc);
  1757			goto err_rst_get;
  1758		}
  1759	
  1760		rc = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
  1761		if (rc)
  1762			goto err_rst_get;
  1763	
  1764		pm_runtime_enable(&pdev->dev);
  1765		rc = pm_runtime_resume_and_get(&pdev->dev);
  1766		if (rc)
  1767			goto err_pm_get;
  1768	
  1769		rc = reset_control_assert(tegra_host->rst);
  1770		if (rc)
  1771			goto err_rst_assert;
  1772	
  1773		usleep_range(2000, 4000);
  1774	
  1775		rc = reset_control_deassert(tegra_host->rst);
  1776		if (rc)
  1777			goto err_rst_assert;
  1778	
  1779		usleep_range(2000, 4000);
  1780	
  1781		rc = sdhci_tegra_add_host(host);
  1782		if (rc)
  1783			goto err_add_host;
  1784	
  1785		/* Program MC streamID for DMA transfers */
  1786		if (soc_data->nvquirks & NVQUIRK_PROGRAM_MC_STREAMID) {
  1787			fwspec = dev_iommu_fwspec_get(&pdev->dev);
  1788			if (fwspec == NULL) {
  1789				rc = -ENODEV;
  1790				dev_err(mmc_dev(host->mmc),
  1791					"failed to get MC streamid: %d\n",
  1792					rc);
  1793				goto err_rst_get;
  1794			} else {
> 1795				tegra_host->streamid = fwspec->ids[0] & 0xffff;
  1796				tegra_sdhci_writel(host, tegra_host->streamid |
  1797							(tegra_host->streamid << 8),
  1798							SDHCI_TEGRA_CIF2AXI_CTRL_0);
  1799			}
  1800		}
  1801	
  1802		return 0;
  1803	
  1804	err_add_host:
  1805		reset_control_assert(tegra_host->rst);
  1806	err_rst_assert:
  1807		pm_runtime_put_sync_suspend(&pdev->dev);
  1808	err_pm_get:
  1809		pm_runtime_disable(&pdev->dev);
  1810	err_rst_get:
  1811	err_clk_get:
  1812		clk_disable_unprepare(tegra_host->tmclk);
  1813	err_power_req:
  1814	err_parse_dt:
  1815		sdhci_pltfm_free(pdev);
  1816		return rc;
  1817	}
  1818	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp