[PATCH] ASoC: SOF: mediatek: mt8186 modify dram type as non-cache

[PATCH] ASoC: SOF: mediatek: mt8186 modify dram type as non-cache

[Trevor Wu]

To prevent incorrect access between the host and DSP sides, we need to
modify DRAM as a non-cache memory type. Additionally, we can retrieve
the size of shared DMA from the device tree.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
Reviewed-by: Kuan-Hsun Cheng <Allen-KH.Cheng@mediatek.com>
---
 sound/soc/sof/mediatek/mt8186/mt8186.c | 40 +++++++++++++++-----------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/sound/soc/sof/mediatek/mt8186/mt8186.c b/sound/soc/sof/mediatek/mt8186/mt8186.c
index 3e0ea0e109e2..f587edf9e0a7 100644
--- a/sound/soc/sof/mediatek/mt8186/mt8186.c
+++ b/sound/soc/sof/mediatek/mt8186/mt8186.c
@@ -111,6 +111,14 @@ static int platform_parse_resource(struct platform_device *pdev, void *data)
 
 	dev_dbg(dev, "DMA %pR\n", &res);
 
+	adsp->pa_shared_dram = (phys_addr_t)res.start;
+	adsp->shared_size = resource_size(&res);
+	if (adsp->pa_shared_dram & DRAM_REMAP_MASK) {
+		dev_err(dev, "adsp shared dma memory(%#x) is not 4K-aligned\n",
+			(u32)adsp->pa_shared_dram);
+		return -EINVAL;
+	}
+
 	ret = of_reserved_mem_device_init(dev);
 	if (ret) {
 		dev_err(dev, "of_reserved_mem_device_init failed\n");
@@ -244,23 +252,18 @@ static int adsp_shared_base_ioremap(struct platform_device *pdev, void *data)
 {
 	struct device *dev = &pdev->dev;
 	struct mtk_adsp_chip_info *adsp = data;
-	u32 shared_size;
 
 	/* remap shared-dram base to be non-cachable */
-	shared_size = TOTAL_SIZE_SHARED_DRAM_FROM_TAIL;
-	adsp->pa_shared_dram = adsp->pa_dram + adsp->dramsize - shared_size;
-	if (adsp->va_dram) {
-		adsp->shared_dram = adsp->va_dram + DSP_DRAM_SIZE - shared_size;
-	} else {
-		adsp->shared_dram = devm_ioremap(dev, adsp->pa_shared_dram,
-						 shared_size);
-		if (!adsp->shared_dram) {
-			dev_err(dev, "ioremap failed for shared DRAM\n");
-			return -ENOMEM;
-		}
+	adsp->shared_dram = devm_ioremap(dev, adsp->pa_shared_dram,
+					 adsp->shared_size);
+	if (!adsp->shared_dram) {
+		dev_err(dev, "failed to ioremap base %pa size %#x\n",
+			adsp->shared_dram, adsp->shared_size);
+		return -ENOMEM;
 	}
-	dev_dbg(dev, "shared-dram vbase=%p, phy addr :%pa, size=%#x\n",
-		adsp->shared_dram, &adsp->pa_shared_dram, shared_size);
+
+	dev_dbg(dev, "shared-dram vbase=%p, phy addr :%pa,  size=%#x\n",
+		adsp->shared_dram, &adsp->pa_shared_dram, adsp->shared_size);
 
 	return 0;
 }
@@ -307,9 +310,12 @@ static int mt8186_dsp_probe(struct snd_sof_dev *sdev)
 		return -ENOMEM;
 	}
 
-	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap_wc(sdev->dev,
-							  priv->adsp->pa_dram,
-							  priv->adsp->dramsize);
+	priv->adsp->va_sram = sdev->bar[SOF_FW_BLK_TYPE_IRAM];
+
+	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap(sdev->dev,
+						       priv->adsp->pa_dram,
+						       priv->adsp->dramsize);
+
 	if (!sdev->bar[SOF_FW_BLK_TYPE_SRAM]) {
 		dev_err(sdev->dev, "failed to ioremap base %pa size %#x\n",
 			&priv->adsp->pa_dram, priv->adsp->dramsize);
-- 
2.18.0

Re: [PATCH] ASoC: SOF: mediatek: mt8186 modify dram type as non-cache

[Eugen Hristev]

Hi Trevor,

On 8/3/23 10:50, Trevor Wu wrote:
> To prevent incorrect access between the host and DSP sides, we need to
> modify DRAM as a non-cache memory type. Additionally, we can retrieve
> the size of shared DMA from the device tree.
> 
> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
> Reviewed-by: Kuan-Hsun Cheng <Allen-KH.Cheng@mediatek.com>
> ---
>   sound/soc/sof/mediatek/mt8186/mt8186.c | 40 +++++++++++++++-----------
>   1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/sound/soc/sof/mediatek/mt8186/mt8186.c b/sound/soc/sof/mediatek/mt8186/mt8186.c
> index 3e0ea0e109e2..f587edf9e0a7 100644
> --- a/sound/soc/sof/mediatek/mt8186/mt8186.c
> +++ b/sound/soc/sof/mediatek/mt8186/mt8186.c
> @@ -111,6 +111,14 @@ static int platform_parse_resource(struct platform_device *pdev, void *data)
>   
>   	dev_dbg(dev, "DMA %pR\n", &res);
>   
> +	adsp->pa_shared_dram = (phys_addr_t)res.start;
> +	adsp->shared_size = resource_size(&res);
> +	if (adsp->pa_shared_dram & DRAM_REMAP_MASK) {
> +		dev_err(dev, "adsp shared dma memory(%#x) is not 4K-aligned\n",
> +			(u32)adsp->pa_shared_dram);
> +		return -EINVAL;
> +	}
> +

Would it be better to just realign to the next 4k boundary ?
Or, isn't it more usual to use dma_coerce_mask_and_coherent ?

>   	ret = of_reserved_mem_device_init(dev);
>   	if (ret) {
>   		dev_err(dev, "of_reserved_mem_device_init failed\n");
> @@ -244,23 +252,18 @@ static int adsp_shared_base_ioremap(struct platform_device *pdev, void *data)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct mtk_adsp_chip_info *adsp = data;
> -	u32 shared_size;
>   
>   	/* remap shared-dram base to be non-cachable */
> -	shared_size = TOTAL_SIZE_SHARED_DRAM_FROM_TAIL;
> -	adsp->pa_shared_dram = adsp->pa_dram + adsp->dramsize - shared_size;
> -	if (adsp->va_dram) {
> -		adsp->shared_dram = adsp->va_dram + DSP_DRAM_SIZE - shared_size;
> -	} else {
> -		adsp->shared_dram = devm_ioremap(dev, adsp->pa_shared_dram,
> -						 shared_size);
> -		if (!adsp->shared_dram) {
> -			dev_err(dev, "ioremap failed for shared DRAM\n");
> -			return -ENOMEM;
> -		}
> +	adsp->shared_dram = devm_ioremap(dev, adsp->pa_shared_dram,
> +					 adsp->shared_size);

You cannot use dma_alloc_coherent ? This should take care of all the 
cache maintainance for you.

> +	if (!adsp->shared_dram) {
> +		dev_err(dev, "failed to ioremap base %pa size %#x\n",
> +			adsp->shared_dram, adsp->shared_size);
> +		return -ENOMEM;
>   	}
> -	dev_dbg(dev, "shared-dram vbase=%p, phy addr :%pa, size=%#x\n",
> -		adsp->shared_dram, &adsp->pa_shared_dram, shared_size);
> +
> +	dev_dbg(dev, "shared-dram vbase=%p, phy addr :%pa,  size=%#x\n",
> +		adsp->shared_dram, &adsp->pa_shared_dram, adsp->shared_size);
>   
>   	return 0;
>   }
> @@ -307,9 +310,12 @@ static int mt8186_dsp_probe(struct snd_sof_dev *sdev)
>   		return -ENOMEM;
>   	}
>   
> -	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap_wc(sdev->dev,
> -							  priv->adsp->pa_dram,
> -							  priv->adsp->dramsize);
> +	priv->adsp->va_sram = sdev->bar[SOF_FW_BLK_TYPE_IRAM];
> +
> +	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap(sdev->dev,
> +						       priv->adsp->pa_dram,
> +						       priv->adsp->dramsize);
> +
Same here

>   	if (!sdev->bar[SOF_FW_BLK_TYPE_SRAM]) {
>   		dev_err(sdev->dev, "failed to ioremap base %pa size %#x\n",
>   			&priv->adsp->pa_dram, priv->adsp->dramsize);


Regards,
Eugen

Re: [PATCH] ASoC: SOF: mediatek: mt8186 modify dram type as non-cache

[Trevor Wu (吳文良)]

On Thu, 2023-08-03 at 11:04 +0300, Eugen Hristev wrote:
> Hi Trevor,
> 
> On 8/3/23 10:50, Trevor Wu wrote:
> > To prevent incorrect access between the host and DSP sides, we need
> > to
> > modify DRAM as a non-cache memory type. Additionally, we can
> > retrieve
> > the size of shared DMA from the device tree.
> > 
> > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> > Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
> > Reviewed-by: Kuan-Hsun Cheng <Allen-KH.Cheng@mediatek.com>
> > ---
> >   sound/soc/sof/mediatek/mt8186/mt8186.c | 40 +++++++++++++++----
> > -------
> >   1 file changed, 23 insertions(+), 17 deletions(-)
> > 
> > diff --git a/sound/soc/sof/mediatek/mt8186/mt8186.c
> > b/sound/soc/sof/mediatek/mt8186/mt8186.c
> > index 3e0ea0e109e2..f587edf9e0a7 100644
> > --- a/sound/soc/sof/mediatek/mt8186/mt8186.c
> > +++ b/sound/soc/sof/mediatek/mt8186/mt8186.c
> > @@ -111,6 +111,14 @@ static int platform_parse_resource(struct
> > platform_device *pdev, void *data)
> >   
> >   	dev_dbg(dev, "DMA %pR\n", &res);
> >   
> > +	adsp->pa_shared_dram = (phys_addr_t)res.start;
> > +	adsp->shared_size = resource_size(&res);
> > +	if (adsp->pa_shared_dram & DRAM_REMAP_MASK) {
> > +		dev_err(dev, "adsp shared dma memory(%#x) is not 4K-
> > aligned\n",
> > +			(u32)adsp->pa_shared_dram);
> > +		return -EINVAL;
> > +	}
> > +
> 
> Would it be better to just realign to the next 4k boundary ?
> Or, isn't it more usual to use dma_coerce_mask_and_coherent ?

Hi Eugen,
 
The 4k boundary checking serves as a sentinel to ensure that the memory
we assign on the AP side can be utilized on the DSP side. If we handle
it in the way you suggested, it would mean that some reserved memory
will never be used. 

> >   	ret = of_reserved_mem_device_init(dev);
> >   	if (ret) {
> >   		dev_err(dev, "of_reserved_mem_device_init failed\n");
> > @@ -244,23 +252,18 @@ static int adsp_shared_base_ioremap(struct
> > platform_device *pdev, void *data)
> >   {
> >   	struct device *dev = &pdev->dev;
> >   	struct mtk_adsp_chip_info *adsp = data;
> > -	u32 shared_size;
> >   
> >   	/* remap shared-dram base to be non-cachable */
> > -	shared_size = TOTAL_SIZE_SHARED_DRAM_FROM_TAIL;
> > -	adsp->pa_shared_dram = adsp->pa_dram + adsp->dramsize -
> > shared_size;
> > -	if (adsp->va_dram) {
> > -		adsp->shared_dram = adsp->va_dram + DSP_DRAM_SIZE -
> > shared_size;
> > -	} else {
> > -		adsp->shared_dram = devm_ioremap(dev, adsp-
> > >pa_shared_dram,
> > -						 shared_size);
> > -		if (!adsp->shared_dram) {
> > -			dev_err(dev, "ioremap failed for shared
> > DRAM\n");
> > -			return -ENOMEM;
> > -		}
> > +	adsp->shared_dram = devm_ioremap(dev, adsp->pa_shared_dram,
> > +					 adsp->shared_size);
> 
> You cannot use dma_alloc_coherent ? This should take care of all the 
> cache maintainance for you.

The purpose of this patch is to align the implementation on mt8195[1].
In fact, the usage of "adsp->shared_dram" has been discontinued. I will
create a follow-up patch to remove this part.

> 
> > +	if (!adsp->shared_dram) {
> > +		dev_err(dev, "failed to ioremap base %pa size %#x\n",
> > +			adsp->shared_dram, adsp->shared_size);
> > +		return -ENOMEM;
> >   	}
> > -	dev_dbg(dev, "shared-dram vbase=%p, phy addr :%pa, size=%#x\n",
> > -		adsp->shared_dram, &adsp->pa_shared_dram, shared_size);
> > +
> > +	dev_dbg(dev, "shared-dram vbase=%p, phy addr
> > :%pa,  size=%#x\n",
> > +		adsp->shared_dram, &adsp->pa_shared_dram, adsp-
> > >shared_size);
> >   
> >   	return 0;
> >   }
> > @@ -307,9 +310,12 @@ static int mt8186_dsp_probe(struct snd_sof_dev
> > *sdev)
> >   		return -ENOMEM;
> >   	}
> >   
> > -	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap_wc(sdev->dev,
> > -							  priv->adsp-
> > >pa_dram,
> > -							  priv->adsp-
> > >dramsize);
> > +	priv->adsp->va_sram = sdev->bar[SOF_FW_BLK_TYPE_IRAM];
> > +
> > +	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap(sdev->dev,
> > +						       priv->adsp-
> > >pa_dram,
> > +						       priv->adsp-
> > >dramsize);
> > +
> 
> Same here

There are two separate memories. However, the memory being referred to
here is not the one we set for of_reserved_mem_device_init().

[1]
https://lore.kernel.org/all/20220606210212.146626-5-pierre-louis.bossart@linux.intel.com/

Thanks,
Trevor

Re: [PATCH] ASoC: SOF: mediatek: mt8186 modify dram type as non-cache

[Mark Brown]

On Thu, 03 Aug 2023 15:50:28 +0800, Trevor Wu wrote:
> To prevent incorrect access between the host and DSP sides, we need to
> modify DRAM as a non-cache memory type. Additionally, we can retrieve
> the size of shared DMA from the device tree.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: SOF: mediatek: mt8186 modify dram type as non-cache
      commit: 1d54134df47684ee29d4d4bbe28174a4282389e0

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark