Re: [PATCH v3 3/3] mmc: xenon: Fix 2G limitation on AC5 SoC

[Adrian Hunter]

On 9/12/22 15:27, Vadym Kochan wrote:
> On Fri, 9 Dec 2022 14:13:06 +0200, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>> On 9/12/22 14:10, Vadym Kochan wrote:
>>> Hi Adrian,
>>>
>>> On Fri, 9 Dec 2022 13:53:58 +0200, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>>>> On 9/12/22 13:39, Vadym Kochan wrote:
>>>>> Hi Adrian,
>>>>>
>>>>> On Fri, 9 Dec 2022 09:23:05 +0200, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>>>>>> On 5/12/22 12:59, Vadym Kochan wrote:
>>>>>>> There is a limitation on AC5 SoC that mmc controller
>>>>>>> can't have DMA access over 2G memory, so use SDMA with
>>>>>>> a bounce buffer. Swiotlb can't help because on arm64 arch
>>>>>>> it reserves memblock's at the end of the memory.
>>>>>>>
>>>>>>> Additionally set mask to 34 bit since on AC5 SoC RAM starts
>>>>>>> at 0x2_00000000.
>>>>>>
>>>>>> Can you explain more about how a 34-bit DMA mask works when
>>>>>> SDMA only supports 32-bit addresses?
>>>>>>
>>>>>
>>>>> So, after I set
>>>>>
>>>>>>> +		host->flags &= ~SDHCI_USE_64_BIT_DMA;
>>>>>
>>>>> then sdhc core sets mask to 32 bit, but then dma_map fails to map
>>>>> bounce buffer because the base address is higher than 32bit - 0x2_00000000,
>>>>> and 34bit mask fixed it.
>>>>
>>>> What happens if the bounce buffer gets mapped in the range
>>>> 0x1_00000000 to 0x1_ffffffff ?
>>>>
>>>
>>> From my understanding, on the AC5 SoC RAM starts at 0x2_00000000 so the bounce
>>> buffer can be mapped in the range 0x2_00000000..0x2_ffffffff
>>
>> Right but I guess I meant what about 0x3_00000000..0x3_ffffffff ?
>> Isn't that also in DMA_BIT_MASK(34)
> 
> Yes, you are right.

So it would fail in that case?  Is it possible to use devicetree
reserved memory or some such, to set aside 64k for the bounce
buffer DMA mapping?

> 
>>
>>>
>>>>>
>>>>>>>
>>>>>>> Co-developed-by: Elad Nachman <enachman@xxxxxxxxxxx>
>>>>>>> Signed-off-by: Elad Nachman <enachman@xxxxxxxxxxx>
>>>>>>> Signed-off-by: Vadym Kochan <vadym.kochan@xxxxxxxxxxx>
>>>>>>> ---
>>>>>>>  drivers/mmc/host/sdhci-xenon.c | 38 ++++++++++++++++++++++++++++++++++
>>>>>>>  drivers/mmc/host/sdhci-xenon.h |  3 ++-
>>>>>>>  2 files changed, 40 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
>>>>>>> index 08e838400b52..5f3db0425674 100644
>>>>>>> --- a/drivers/mmc/host/sdhci-xenon.c
>>>>>>> +++ b/drivers/mmc/host/sdhci-xenon.c
>>>>>>> @@ -13,7 +13,9 @@
>>>>>>>  
>>>>>>>  #include <linux/acpi.h>
>>>>>>>  #include <linux/delay.h>
>>>>>>> +#include <linux/dma-mapping.h>
>>>>>>>  #include <linux/ktime.h>
>>>>>>> +#include <linux/mm.h>
>>>>>>>  #include <linux/module.h>
>>>>>>>  #include <linux/of.h>
>>>>>>>  #include <linux/pm.h>
>>>>>>> @@ -253,6 +255,22 @@ static unsigned int xenon_get_max_clock(struct sdhci_host *host)
>>>>>>>  		return pltfm_host->clock;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static int xenon_set_dma_mask(struct sdhci_host *host)
>>>>>>> +{
>>>>>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>> +	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>>>> +	struct mmc_host *mmc = host->mmc;
>>>>>>> +	struct device *dev = mmc_dev(mmc);
>>>>>>> +
>>>>>>> +	if (priv->hw_version == XENON_AC5) {
>>>>>>> +		host->flags &= ~SDHCI_USE_64_BIT_DMA;
>>>>>>> +
>>>>>>> +		return dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34));
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return sdhci_set_dma_mask(host);
>>>>>>> +}
>>>>>>> +
>>>>>>>  static const struct sdhci_ops sdhci_xenon_ops = {
>>>>>>>  	.voltage_switch		= xenon_voltage_switch,
>>>>>>>  	.set_clock		= sdhci_set_clock,
>>>>>>> @@ -261,6 +279,7 @@ static const struct sdhci_ops sdhci_xenon_ops = {
>>>>>>>  	.reset			= xenon_reset,
>>>>>>>  	.set_uhs_signaling	= xenon_set_uhs_signaling,
>>>>>>>  	.get_max_clock		= xenon_get_max_clock,
>>>>>>> +	.set_dma_mask		= xenon_set_dma_mask,
>>>>>>>  };
>>>>>>>  
>>>>>>>  static const struct sdhci_pltfm_data sdhci_xenon_pdata = {
>>>>>>> @@ -486,6 +505,18 @@ static void xenon_sdhc_unprepare(struct sdhci_host *host)
>>>>>>>  	xenon_disable_sdhc(host, sdhc_id);
>>>>>>>  }
>>>>>>>  
>>>>>>> +static int xenon_ac5_probe(struct sdhci_host *host)
>>>>>>> +{
>>>>>>> +	struct sysinfo si;
>>>>>>> +
>>>>>>> +	si_meminfo(&si);
>>>>>>> +
>>>>>>> +	if ((si.totalram * si.mem_unit) > SZ_2G)
>>>>>>> +		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static int xenon_probe(struct platform_device *pdev)
>>>>>>>  {
>>>>>>>  	struct sdhci_pltfm_host *pltfm_host;
>>>>>>> @@ -533,6 +564,12 @@ static int xenon_probe(struct platform_device *pdev)
>>>>>>>  		}
>>>>>>>  	}
>>>>>>>  
>>>>>>> +	if (priv->hw_version == XENON_AC5) {
>>>>>>> +		err = xenon_ac5_probe(host);
>>>>>>> +		if (err)
>>>>>>> +			goto err_clk_axi;
>>>>>>> +	}
>>>>>>> +
>>>>>>>  	err = mmc_of_parse(host->mmc);
>>>>>>>  	if (err)
>>>>>>>  		goto err_clk_axi;
>>>>>>> @@ -682,6 +719,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 {
>>>>>>
>>>>>
>>>>> Regards,
>>>>
>>