Received-SPF: pass (google.com: domain of linux-kernel+bounces-16539-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <d3208fee-9784-4dee-994d-ac1f7c526e74@intel.com>
Date: Thu, 4 Jan 2024 12:50:34 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/4] mmc: xenon: Add ac5 support via bounce buffer
Content-Language: en-US
To: Elad Nachman <enachman@marvell.com>, robh+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, andrew@lunn.ch,
 gregory.clement@bootlin.com, sebastian.hesselbarth@gmail.com,
 huziji@marvell.com, ulf.hansson@linaro.org, catalin.marinas@arm.com,
 will@kernel.org, thunder.leizhen@huawei.com, bhe@redhat.com,
 akpm@linux-foundation.org, yajun.deng@linux.dev, chris.zjh@huawei.com,
 linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Cc: cyuval@marvell.com
References: <20231227123257.1170590-1-enachman@marvell.com>
 <20231227123257.1170590-5-enachman@marvell.com>
From: Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <20231227123257.1170590-5-enachman@marvell.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 27/12/23 14:32, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> AC5/X/IM SOCs has a variant of the Xenon eMMC controller,
> in which only 31-bit of addressing pass from the controller
> on the AXI bus.
> Since we cannot guarantee that only buffers from the first 2GB
> of memory will reach the driver, the driver is configured for
> SDMA mode, without 64-bit mode, overriding the DMA mask to 34-bit
> to support the DDR memory mapping, which starts at offset 8GB.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>

One minor comment below otherwise:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-xenon.c | 33 ++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci-xenon.h |  3 ++-
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
> index 25ba7aecc3be..4d6df1815da1 100644
> --- a/drivers/mmc/host/sdhci-xenon.c
> +++ b/drivers/mmc/host/sdhci-xenon.c
> @@ -18,6 +18,8 @@
>  #include <linux/of.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/mm.h>
> +#include <linux/dma-mapping.h>
>  
>  #include "sdhci-pltfm.h"
>  #include "sdhci-xenon.h"
> @@ -422,6 +424,7 @@ static int xenon_probe_params(struct platform_device *pdev)
>  	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>  	u32 sdhc_id, nr_sdhc;
>  	u32 tuning_count;
> +	struct sysinfo si;
>  
>  	/* Disable HS200 on Armada AP806 */
>  	if (priv->hw_version == XENON_AP806)
> @@ -450,6 +453,23 @@ static int xenon_probe_params(struct platform_device *pdev)
>  	}
>  	priv->tuning_count = tuning_count;
>  
> +	/*
> +	 * AC5/X/IM HW has only 31-bits passed in the crossbar switch.
> +	 * If we have more than 2GB of memory, this means we might pass
> +	 * memory pointers which are above 2GB and which cannot be properly
> +	 * represented. In this case, disable ADMA, 64-bit DMA and allow only SDMA.
> +	 * This effectively will enable bounce buffer quirk in the
> +	 * generic SDHCI driver, which will make sure DMA is only done
> +	 * from supported memory regions:
> +	 */
> +	if (priv->hw_version == XENON_AC5) {
> +		si_meminfo(&si);
> +		if (si.totalram * si.mem_unit > SZ_2G) {
> +			host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
> +			host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
> +		}
> +	}
> +
>  	return xenon_phy_parse_params(dev, host);
>  }
>  
> @@ -562,7 +582,17 @@ static int xenon_probe(struct platform_device *pdev)
>  		goto remove_sdhc;
>  
>  	pm_runtime_put_autosuspend(&pdev->dev);
> -
> +	/*
> +	 * If we previously detected AC5 with over 2GB of memory,
> +	 * then we disable ADMA and 64-bit DMA.
> +	 * This means generic SDHCI driver has set the DMA mask to
> +	 * 32-bit. Since DDR starts at 0x2_0000_0000, we must use
> +	 * 34-bit DMA mask to access this DDR memory:
> +	 */
> +	if (priv->hw_version == XENON_AC5) {
> +		if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)

Kernel style is to avoid nested if-statements i.e.

	if (priv->hw_version == XENON_AC5 &&
	    host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
		dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(34));

> +			dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(34));
> +	}
>  	return 0;
>  
>  remove_sdhc:
> @@ -680,6 +710,7 @@ static const struct of_device_id sdhci_xenon_dt_ids[] = {
>  	{ .compatible = "marvell,armada-ap807-sdhci", .data = (void *)XENON_AP807},
>  	{ .compatible = "marvell,armada-cp110-sdhci", .data =  (void *)XENON_CP110},
>  	{ .compatible = "marvell,armada-3700-sdhci", .data =  (void *)XENON_A3700},
> +	{ .compatible = "marvell,ac5-sdhci",	     .data =  (void *)XENON_AC5},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
> index 3e9c6c908a79..0460d97aad26 100644
> --- a/drivers/mmc/host/sdhci-xenon.h
> +++ b/drivers/mmc/host/sdhci-xenon.h
> @@ -57,7 +57,8 @@ enum xenon_variant {
>  	XENON_A3700,
>  	XENON_AP806,
>  	XENON_AP807,
> -	XENON_CP110
> +	XENON_CP110,
> +	XENON_AC5
>  };
>  
>  struct xenon_priv {