drivers: dma: intel-adsp-hda: Report total_copied bytes on ACE2/3 #77726
Conversation
zephyrbot
added
platform: Intel ADSP
zephyrbot
requested review from
andyross,
ceolin,
dcpleung,
jxstelter,
lgirdwood,
lyakh,
marc-hb,
marcinszkudlinski,
nashif,
softwarecki and
tmleman
drivers/dma/dma_intel_adsp_hda.c
Outdated
|
|
||
| llp_l = *DGLLLPL(cfg->base, cfg->regblock_size, channel); | ||
| llp_u = *DGLLLPU(cfg->base, cfg->regblock_size, channel); | ||
| stat->total_copied = ((uint64_t)llp_u << 32) | llp_l; |
There was a problem hiding this comment.
isn't there a potential problem with this code?
if llp_l is MAX_32, then llp_u will likely be increased by the time you read it, but the llp_l be close to zero. This sort of combination carries the risk of creating completely wrong counts. I think you need to re-read the llp_l and make sure its value did not become lower than before...
There was a problem hiding this comment.
@plbossart, it is copied directly from the existing intel_adsp_gpdma_llp_read() from
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/dma/dma_intel_adsp_gpdma.c#L133
I see, I will update, thanks for spotting it.
There was a problem hiding this comment.
wow, I earned by salary for the day. GPDMA is also wrong haha.
There was a problem hiding this comment.
probably need something along those lines
diff --git a/drivers/dma/dma_intel_adsp_gpdma.c b/drivers/dma/dma_intel_adsp_gpdma.c
index 5e3850244d9b..a3e24e994ad5 100644
--- a/drivers/dma/dma_intel_adsp_gpdma.c
+++ b/drivers/dma/dma_intel_adsp_gpdma.c
@@ -129,9 +129,15 @@ static inline void intel_adsp_gpdma_llp_read(const struct device *dev,
{
#ifdef CONFIG_DMA_INTEL_ADSP_GPDMA_HAS_LLP
const struct intel_adsp_gpdma_cfg *const dev_cfg = dev->config;
+ uint32_t llp_l1;
+ uint32_t llp_l2;
- *llp_l = dw_read(dev_cfg->shim, GPDMA_CHLLPL(channel));
+ llp_l1 = dw_read(dev_cfg->shim, GPDMA_CHLLPL(channel));
*llp_u = dw_read(dev_cfg->shim, GPDMA_CHLLPU(channel));
+ llp_l2 = dw_read(dev_cfg->shim, GPDMA_CHLLPL(channel));
+ if (llp_l2 < llp_l1)
+ *llp_u = dw_read(dev_cfg->shim, GPDMA_CHLLPU(channel));
+ *llp_l = llp_l2;
#endif
}
There was a problem hiding this comment.
Ah yes... the ole double word counter read, good catch!
There was a problem hiding this comment.
@ujfalusi do you want to send the same fix for GPDMA? same problem.
ujfalusi
force-pushed
the
peter/pr/hda-dma_ace2plus_llp_support_01
branch
from
c62bcc6
to
2c596ec
Compare
|
Changes since v1:
|
drivers/dma/dma_intel_adsp_hda.c
Outdated
| /* re-read the LLPU value, as LLPL just wrapped */ | ||
| llp_u = *DGLLLPU(cfg->base, cfg->regblock_size, channel); | ||
| } | ||
| stat->total_copied = ((uint64_t)llp_u << 32) | llp_l; |
There was a problem hiding this comment.
what would stat->total_copied be equal to on other platforms then? Not set at all? Should we set it to 0?
There was a problem hiding this comment.
Probably, but it is not subject of this PR, I can create a followup to reset the total_copied to 0 (set llp_l/u to 0) if it is not handled and at the same time do similar update to the gpdma.
There was a problem hiding this comment.
Just a sec, I will update this PR, the gpdma handles this already.
With ACE2/3 the HDA DMA includes registers to read the Linear Link Position. Previous platforms (CAVS, ACE1) was able to report the LLP for GPDMA. Since ACE2 all links are handled with HD-DMA, hence the new register has been added for the firmware to report the LLP to the host. Set the total_copied to 0 for older ACE1/CAVS platforms and in case of host DMA on ACE2/3 since the informatiojn is not available. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
ujfalusi
dismissed stale reviews from plbossart, serhiy-katsyuba-intel, dnikodem, and teburd
via
5da4b43
ujfalusi
force-pushed
the
peter/pr/hda-dma_ace2plus_llp_support_01
branch
from
2c596ec
to
5da4b43
Compare
|
Changes since v2:
|
ujfalusi
changed the title
With ACE2/3 the HDA DMA includes registers to read the Linear Link
Position.
Previous platforms (CAVS, ACE1) was able to report the LLP for GPDMA. Since
ACE2 all links are handled with HD-DMA, hence the new register has been
added for the firmware to report the LLP to the host.
Set the total_copied to 0 for older ACE1/CAVS platforms and in case of
host DMA on ACE2/3 since the informatiojn is not available.