| Message ID | 20241216075819.2066772-8-larisa.grigore@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add eDMAv3 support for S32G2/S32G3 SoCs | expand |
On 16/12/2024 08:58, Larisa Grigore wrote: > Wait DMA hardware complete cleanup work by checking HRS bit before > disabling the channel to make sure trail data is already written to > memory. > > Fixes: 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate v3 support") Why Fixes are at the end of the patchset? They must be either separate patchset or first patches. Best regards, Krzysztof
On 12/17/2024 7:27 AM, Krzysztof Kozlowski wrote: > [You don't often get email from krzk@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On 16/12/2024 08:58, Larisa Grigore wrote: >> Wait DMA hardware complete cleanup work by checking HRS bit before >> disabling the channel to make sure trail data is already written to >> memory. >> >> Fixes: 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate v3 support") > > Why Fixes are at the end of the patchset? They must be either separate > patchset or first patches. > > Best regards, > Krzysztof Thank you for you review Krzysztof! Indeed, this commit should be moved right after "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" which introduces the eDMA V3 specific registers including HRS. I will move it in the next version. Best regards, Larisa
On 17/12/2024 15:19, Larisa Ileana Grigore wrote: > On 12/17/2024 7:27 AM, Krzysztof Kozlowski wrote: >> [You don't often get email from krzk@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >> >> On 16/12/2024 08:58, Larisa Grigore wrote: >>> Wait DMA hardware complete cleanup work by checking HRS bit before >>> disabling the channel to make sure trail data is already written to >>> memory. >>> >>> Fixes: 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate v3 support") >> >> Why Fixes are at the end of the patchset? They must be either separate >> patchset or first patches. >> >> Best regards, >> Krzysztof > > Thank you for you review Krzysztof! Indeed, this commit should be moved > right after "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" I don't understand this. Are you saying you introduce bug in one patch and fix in other? Why this cannot be separate patchset? Best regards, Krzysztof
On 12/17/2024 5:27 PM, Krzysztof Kozlowski wrote: > On 17/12/2024 15:19, Larisa Ileana Grigore wrote: >> On 12/17/2024 7:27 AM, Krzysztof Kozlowski wrote: >>> [You don't often get email from krzk@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >>> >>> On 16/12/2024 08:58, Larisa Grigore wrote: >>>> Wait DMA hardware complete cleanup work by checking HRS bit before >>>> disabling the channel to make sure trail data is already written to >>>> memory. >>>> >>>> Fixes: 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate v3 support") >>> >>> Why Fixes are at the end of the patchset? They must be either separate >>> patchset or first patches. >>> >>> Best regards, >>> Krzysztof >> >> Thank you for you review Krzysztof! Indeed, this commit should be moved >> right after "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" > > I don't understand this. Are you saying you introduce bug in one patch > and fix in other? Why this cannot be separate patchset? The bug was introduced by 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate v3 support"), commit which is already upstream. In the proposed fix, a channel is disabled after checking the HRS register which is a eDMAv3 specific register. In the upstream implementation, "struct edma_regs" is created based on the eDMAv2 register layout [1] which is different compared to the eDMAv3 register layout. The "hrs" field, which is used to access the HRS register, was introduced in one of the patches from this set [2]. So, this fix depends on two other commits: "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" [2] "dmaengine: fsl-edma: move eDMAv2 related registers to a new structure ’edma2_regs’" [3] "dmaengine: fsl-edma: add support for S32G based platforms" [4] depends also on [2] because it reads another eDMAv3 specific register "ES". This is the reason I've sent all these patches together. Please let me know your thoughts. [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/dma/fsl-edma-common.h#L123 [2] https://lore.kernel.org/all/20241216075819.2066772-5-larisa.grigore@oss.nxp.com/ [3] https://lore.kernel.org/all/20241216075819.2066772-4-larisa.grigore@oss.nxp.com/ [4] https://lore.kernel.org/all/20241216075819.2066772-7-larisa.grigore@oss.nxp.com/ > Best regards, > Krzysztof Best regards, Larisa
On 18/12/2024 14:24, Larisa Ileana Grigore wrote: >>> >>> Thank you for you review Krzysztof! Indeed, this commit should be moved >>> right after "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" >> >> I don't understand this. Are you saying you introduce bug in one patch >> and fix in other? Why this cannot be separate patchset? > > The bug was introduced by 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate > v3 support"), commit which is already upstream. > > In the proposed fix, a channel is disabled after checking the HRS > register which is a eDMAv3 specific register. > > In the upstream implementation, "struct edma_regs" is created based on > the eDMAv2 register layout [1] which is different compared to the eDMAv3 > register layout. > The "hrs" field, which is used to access the HRS register, was > introduced in one of the patches from this set [2]. > So, this fix depends on two other commits: > "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" [2] > "dmaengine: fsl-edma: move eDMAv2 related registers to a new structure > ’edma2_regs’" [3] OK, this explains the problem. Your fix cannot depend on other patches. Best regards, Krzysztof
On 12/18/2024 3:32 PM, Krzysztof Kozlowski wrote: > On 18/12/2024 14:24, Larisa Ileana Grigore wrote: >>>> >>>> Thank you for you review Krzysztof! Indeed, this commit should be moved >>>> right after "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" >>> >>> I don't understand this. Are you saying you introduce bug in one patch >>> and fix in other? Why this cannot be separate patchset? >> >> The bug was introduced by 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate >> v3 support"), commit which is already upstream. >> >> In the proposed fix, a channel is disabled after checking the HRS >> register which is a eDMAv3 specific register. >> >> In the upstream implementation, "struct edma_regs" is created based on >> the eDMAv2 register layout [1] which is different compared to the eDMAv3 >> register layout. >> The "hrs" field, which is used to access the HRS register, was >> introduced in one of the patches from this set [2]. >> So, this fix depends on two other commits: >> "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" [2] >> "dmaengine: fsl-edma: move eDMAv2 related registers to a new structure >> ’edma2_regs’" [3] > > OK, this explains the problem. Your fix cannot depend on other patches. Should I remove the "Fixes" tag in this case? > > Best regards, > Krzysztof Regards, Larisa
On 18/12/2024 14:38, Larisa Ileana Grigore wrote: > On 12/18/2024 3:32 PM, Krzysztof Kozlowski wrote: >> On 18/12/2024 14:24, Larisa Ileana Grigore wrote: >>>>> >>>>> Thank you for you review Krzysztof! Indeed, this commit should be moved >>>>> right after "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" >>>> >>>> I don't understand this. Are you saying you introduce bug in one patch >>>> and fix in other? Why this cannot be separate patchset? >>> >>> The bug was introduced by 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate >>> v3 support"), commit which is already upstream. >>> >>> In the proposed fix, a channel is disabled after checking the HRS >>> register which is a eDMAv3 specific register. >>> >>> In the upstream implementation, "struct edma_regs" is created based on >>> the eDMAv2 register layout [1] which is different compared to the eDMAv3 >>> register layout. >>> The "hrs" field, which is used to access the HRS register, was >>> introduced in one of the patches from this set [2]. >>> So, this fix depends on two other commits: >>> "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" [2] >>> "dmaengine: fsl-edma: move eDMAv2 related registers to a new structure >>> ’edma2_regs’" [3] >> >> OK, this explains the problem. Your fix cannot depend on other patches. > > Should I remove the "Fixes" tag in this case? No, you should rather rework the patches so the fix is independent and can be submitted separately. Best regards, Krzysztof
On Wed, Dec 18, 2024 at 03:38:38PM +0200, Larisa Ileana Grigore wrote: > On 12/18/2024 3:32 PM, Krzysztof Kozlowski wrote: > > On 18/12/2024 14:24, Larisa Ileana Grigore wrote: > > > > > > > > > > Thank you for you review Krzysztof! Indeed, this commit should be moved > > > > > right after "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" > > > > > > > > I don't understand this. Are you saying you introduce bug in one patch > > > > and fix in other? Why this cannot be separate patchset? > > > > > > The bug was introduced by 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate > > > v3 support"), commit which is already upstream. > > > > > > In the proposed fix, a channel is disabled after checking the HRS > > > register which is a eDMAv3 specific register. > > > > > > In the upstream implementation, "struct edma_regs" is created based on > > > the eDMAv2 register layout [1] which is different compared to the eDMAv3 > > > register layout. > > > The "hrs" field, which is used to access the HRS register, was > > > introduced in one of the patches from this set [2]. > > > So, this fix depends on two other commits: > > > "dmaengine: fsl-edma: add eDMAv3 registers to edma_regs" [2] > > > "dmaengine: fsl-edma: move eDMAv2 related registers to a new structure > > > ’edma2_regs’" [3] > > > > OK, this explains the problem. Your fix cannot depend on other patches. > > Should I remove the "Fixes" tag in this case? You should move these fixes patch to first patch in this serise! So greg can backport these fixes patches to old kernel easily. Frank > > > > > Best regards, > > Krzysztof > > Regards, > Larisa
diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c index 62d51b269e54..d364514f21be 100644 --- a/drivers/dma/fsl-edma-common.c +++ b/drivers/dma/fsl-edma-common.c @@ -9,6 +9,7 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/dma-mapping.h> +#include <linux/iopoll.h> #include <linux/pm_runtime.h> #include <linux/pm_domain.h> @@ -127,11 +128,19 @@ static void fsl_edma_enable_request(struct fsl_edma_chan *fsl_chan) static void fsl_edma3_disable_request(struct fsl_edma_chan *fsl_chan) { + struct fsl_edma_engine *fsl_edma = fsl_chan->edma; + struct edma_regs *regs = &fsl_chan->edma->regs; u32 val = edma_readl_chreg(fsl_chan, ch_csr); + u32 ch = fsl_chan->vchan.chan.chan_id; u32 flags; flags = fsl_edma_drvflags(fsl_chan); + /* Make sure there is no hardware request in progress. */ + read_poll_timeout(edma_readl, val, !(val & EDMA_V3_MP_HRS_CH(ch)), + EDMA_USEC_POLL, EDMA_USEC_TIMEOUT, false, fsl_edma, + regs->v3.hrs); + if (flags & FSL_EDMA_DRV_HAS_CHMUX) edma_writel(fsl_chan->edma, 0, fsl_chan->mux_addr); diff --git a/drivers/dma/fsl-edma-common.h b/drivers/dma/fsl-edma-common.h index 63e908fc3575..ed210bd71681 100644 --- a/drivers/dma/fsl-edma-common.h +++ b/drivers/dma/fsl-edma-common.h @@ -70,6 +70,10 @@ #define EDMA_V3_CH_CSR_ACTIVE BIT(31) #define EDMA_V3_CH_ES_ERR BIT(31) #define EDMA_V3_MP_ES_VLD BIT(31) +#define EDMA_V3_MP_HRS_CH(ch) BIT(ch) + +#define EDMA_USEC_POLL 10 +#define EDMA_USEC_TIMEOUT 10000 enum fsl_edma_pm_state { RUNNING = 0,
Wait DMA hardware complete cleanup work by checking HRS bit before disabling the channel to make sure trail data is already written to memory. Fixes: 72f5801a4e2b7 ("dmaengine: fsl-edma: integrate v3 support") Signed-off-by: Larisa Grigore <larisa.grigore@oss.nxp.com> --- drivers/dma/fsl-edma-common.c | 9 +++++++++ drivers/dma/fsl-edma-common.h | 4 ++++ 2 files changed, 13 insertions(+)