Re: [PATCH 4/4] mmc: xenon: Add ac5 support via bounce buffer

[Adrian Hunter <adrian.hunter@intel.com>]

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 {