| Message ID | 20240416162908.24180-4-fancer.lancer@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: dw: Fix src/dst addr width misconfig | expand |
On Wed, Apr 17, 2024 at 11:11:46PM +0300, Serge Semin wrote: > On Tue, Apr 16, 2024 at 10:04:42PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote: ... > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > + sms = dwc->dws.m_master; > > > + smsize = 0; > > > + dms = dwc->dws.p_master; > > > + dmsize = sconfig->dst_maxburst; > > > > > I would group it differently, i.e. > > > > sms = dwc->dws.m_master; > > dms = dwc->dws.p_master; > > smsize = 0; > > dmsize = sconfig->dst_maxburst; > > Could you please clarify, why? From my point of view it was better to > group the source master ID and the source master burst size inits > together. Sure. The point here is that when you look at the DMA channel configuration usually you operate with the semantically tied fields for source and destination. At least this is my experience, I always check both sides of the transfer for the same field, e.g., master setting, hence I want to have them coupled. > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > + sms = dwc->dws.p_master; > > > + smsize = sconfig->src_maxburst; > > > + dms = dwc->dws.m_master; > > > + dmsize = 0; > > > + } else /* DMA_MEM_TO_MEM */ { > > > + sms = dwc->dws.m_master; > > > + smsize = 0; > > > + dms = dwc->dws.m_master; > > > + dmsize = 0; > > > + } > > > > Ditto for two above cases. ... > > > static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc) > > > { > > > struct dma_slave_config *sconfig = &dwc->dma_sconfig; > > > - u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0; > > > - u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0; > > > + u8 smsize, dmsize; > > > + > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > + smsize = 0; > > > + dmsize = sconfig->dst_maxburst; > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > + smsize = sconfig->src_maxburst; > > > + dmsize = 0; > > > + } else /* DMA_MEM_TO_MEM */ { > > > + smsize = 0; > > > + dmsize = 0; > > > + } > > > > u8 smsize = 0, dmsize = 0; > > > > if (dwc->direction == DMA_MEM_TO_DEV) > > dmsize = sconfig->dst_maxburst; > > else if (dwc->direction == DMA_DEV_TO_MEM) > > smsize = sconfig->src_maxburst; > > > > ? > > > > Something similar also can be done in the Synopsys case above, no? > > As in case of the patch #1 the if-else statement here was designed > like that intentionally: to signify that the else clause implies the > DMA_MEM_TO_MEM transfer. Any other one (like DMA_DEV_TO_DEV) would > need to have the statement alteration. My version as I read it: - for M2D the dmsize is important - for D2M the smsize is important - for anything else use defaults (which are 0) > Moreover even though your > version looks smaller, but it causes one redundant store operation. Most likely not. Any assembler here? I can check on x86_64, but I believe it simply assigns 0 for both u8 at once using xor r16,r16 or so. Maybe ARM or MIPS (what do you use?) sucks? :-) > Do you think it still would be better to use your version despite of > my reasoning?
On Thu, Apr 18, 2024 at 02:47:18PM +0300, Andy Shevchenko wrote: > On Wed, Apr 17, 2024 at 11:11:46PM +0300, Serge Semin wrote: > > On Tue, Apr 16, 2024 at 10:04:42PM +0300, Andy Shevchenko wrote: > > > On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote: > > ... > > > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > > + sms = dwc->dws.m_master; > > > > + smsize = 0; > > > > + dms = dwc->dws.p_master; > > > > + dmsize = sconfig->dst_maxburst; > > > > > > > > I would group it differently, i.e. > > > > > > sms = dwc->dws.m_master; > > > dms = dwc->dws.p_master; > > > smsize = 0; > > > dmsize = sconfig->dst_maxburst; > > > > Could you please clarify, why? From my point of view it was better to > > group the source master ID and the source master burst size inits > > together. > > Sure. The point here is that when you look at the DMA channel configuration > usually you operate with the semantically tied fields for source and > destination. At least this is my experience, I always check both sides > of the transfer for the same field, e.g., master setting, hence I want to > have them coupled. Ok. I see. Thanks for clarification. I normally do that in another order: group the functionally related fields together - all source-related configs first, then all destination-related configs. Honestly I don't have strong opinion about this part, it's just my personal preference. Am I right to think that from your experience in kernel it's normally done in the order you described? > > > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > > + sms = dwc->dws.p_master; > > > > + smsize = sconfig->src_maxburst; > > > > + dms = dwc->dws.m_master; > > > > + dmsize = 0; > > > > + } else /* DMA_MEM_TO_MEM */ { > > > > + sms = dwc->dws.m_master; > > > > + smsize = 0; > > > > + dms = dwc->dws.m_master; > > > > + dmsize = 0; > > > > + } > > > > > > Ditto for two above cases. > > ... > > > > > static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc) > > > > { > > > > struct dma_slave_config *sconfig = &dwc->dma_sconfig; > > > > - u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0; > > > > - u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0; > > > > > + u8 smsize, dmsize; > > > > + > > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > > + smsize = 0; > > > > + dmsize = sconfig->dst_maxburst; > > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > > + smsize = sconfig->src_maxburst; > > > > + dmsize = 0; > > > > + } else /* DMA_MEM_TO_MEM */ { > > > > + smsize = 0; > > > > + dmsize = 0; > > > > + } > > > > > > u8 smsize = 0, dmsize = 0; > > > > > > if (dwc->direction == DMA_MEM_TO_DEV) > > > dmsize = sconfig->dst_maxburst; > > > else if (dwc->direction == DMA_DEV_TO_MEM) > > > smsize = sconfig->src_maxburst; > > > > > > ? > > > > > > Something similar also can be done in the Synopsys case above, no? > > > > As in case of the patch #1 the if-else statement here was designed > > like that intentionally: to signify that the else clause implies the > > DMA_MEM_TO_MEM transfer. Any other one (like DMA_DEV_TO_DEV) would > > need to have the statement alteration. > > My version as I read it: > - for M2D the dmsize is important > - for D2M the smsize is important > - for anything else use defaults (which are 0) > Ok. Let's follow your way in this case. After your how-to-read-it comment your version no longer look less readable than what is implemented by me. Thanks for clarification. > > Moreover even though your > > version looks smaller, but it causes one redundant store operation. > > Most likely not. Any assembler here? I can check on x86_64, but I believe it > simply assigns 0 for both u8 at once using xor r16,r16 or so. > > Maybe ARM or MIPS (what do you use?) sucks? :-) Interestingly, but asm-code in both cases match.) So the redundant store operation in your C-code gets to be optimized away. -Serge(y) > > > Do you think it still would be better to use your version despite of > > my reasoning? > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Apr 18, 2024 at 10:00:02PM +0300, Serge Semin wrote: > On Thu, Apr 18, 2024 at 02:47:18PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 17, 2024 at 11:11:46PM +0300, Serge Semin wrote: > > > On Tue, Apr 16, 2024 at 10:04:42PM +0300, Andy Shevchenko wrote: > > > > On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote: ... > > > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > > > + sms = dwc->dws.m_master; > > > > > + smsize = 0; > > > > > + dms = dwc->dws.p_master; > > > > > + dmsize = sconfig->dst_maxburst; > > > > > > > I would group it differently, i.e. > > > > > > > > sms = dwc->dws.m_master; > > > > dms = dwc->dws.p_master; > > > > smsize = 0; > > > > dmsize = sconfig->dst_maxburst; > > > > > > Could you please clarify, why? From my point of view it was better to > > > group the source master ID and the source master burst size inits > > > together. > > > Sure. The point here is that when you look at the DMA channel configuration > > usually you operate with the semantically tied fields for source and > > destination. At least this is my experience, I always check both sides > > of the transfer for the same field, e.g., master setting, hence I want to > > have them coupled. > > Ok. I see. Thanks for clarification. I normally do that in another > order: group the functionally related fields together - all > source-related configs first, then all destination-related configs. > Honestly I don't have strong opinion about this part, it's just my > personal preference. Am I right to think that from your experience in > kernel it's normally done in the order you described? In this driver I believe I have followed that one, yes. > > > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > > > + sms = dwc->dws.p_master; > > > > > + smsize = sconfig->src_maxburst; > > > > > + dms = dwc->dws.m_master; > > > > > + dmsize = 0; > > > > > + } else /* DMA_MEM_TO_MEM */ { > > > > > + sms = dwc->dws.m_master; > > > > > + smsize = 0; > > > > > + dms = dwc->dws.m_master; > > > > > + dmsize = 0; > > > > > + } > > > > > > > > Ditto for two above cases.
On Fri, Apr 19, 2024 at 12:00:40AM +0300, Andy Shevchenko wrote: > On Thu, Apr 18, 2024 at 10:00:02PM +0300, Serge Semin wrote: > > On Thu, Apr 18, 2024 at 02:47:18PM +0300, Andy Shevchenko wrote: > > > On Wed, Apr 17, 2024 at 11:11:46PM +0300, Serge Semin wrote: > > > > On Tue, Apr 16, 2024 at 10:04:42PM +0300, Andy Shevchenko wrote: > > > > > On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote: > > ... > > > > > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > > > > + sms = dwc->dws.m_master; > > > > > > + smsize = 0; > > > > > > + dms = dwc->dws.p_master; > > > > > > + dmsize = sconfig->dst_maxburst; > > > > > > > > > I would group it differently, i.e. > > > > > > > > > > sms = dwc->dws.m_master; > > > > > dms = dwc->dws.p_master; > > > > > smsize = 0; > > > > > dmsize = sconfig->dst_maxburst; > > > > > > > > Could you please clarify, why? From my point of view it was better to > > > > group the source master ID and the source master burst size inits > > > > together. > > > > > Sure. The point here is that when you look at the DMA channel configuration > > > usually you operate with the semantically tied fields for source and > > > destination. At least this is my experience, I always check both sides > > > of the transfer for the same field, e.g., master setting, hence I want to > > > have them coupled. > > > > Ok. I see. Thanks for clarification. I normally do that in another > > order: group the functionally related fields together - all > > source-related configs first, then all destination-related configs. > > Honestly I don't have strong opinion about this part, it's just my > > personal preference. Am I right to think that from your experience in > > kernel it's normally done in the order you described? > > In this driver I believe I have followed that one, yes. Agreed then. I'll change the order to the way you ask. -Serge(y) > > > > > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > > > > + sms = dwc->dws.p_master; > > > > > > + smsize = sconfig->src_maxburst; > > > > > > + dms = dwc->dws.m_master; > > > > > > + dmsize = 0; > > > > > > + } else /* DMA_MEM_TO_MEM */ { > > > > > > + sms = dwc->dws.m_master; > > > > > > + smsize = 0; > > > > > > + dms = dwc->dws.m_master; > > > > > > + dmsize = 0; > > > > > > + } > > > > > > > > > > Ditto for two above cases. > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c index a4862263ff14..c65438d1f1ff 100644 --- a/drivers/dma/dw/dw.c +++ b/drivers/dma/dw/dw.c @@ -67,12 +67,24 @@ static size_t dw_dma_block2bytes(struct dw_dma_chan *dwc, u32 block, u32 width) static u32 dw_dma_prepare_ctllo(struct dw_dma_chan *dwc) { struct dma_slave_config *sconfig = &dwc->dma_sconfig; - u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0; - u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0; - u8 p_master = dwc->dws.p_master; - u8 m_master = dwc->dws.m_master; - u8 dms = (dwc->direction == DMA_MEM_TO_DEV) ? p_master : m_master; - u8 sms = (dwc->direction == DMA_DEV_TO_MEM) ? p_master : m_master; + u8 sms, smsize, dms, dmsize; + + if (dwc->direction == DMA_MEM_TO_DEV) { + sms = dwc->dws.m_master; + smsize = 0; + dms = dwc->dws.p_master; + dmsize = sconfig->dst_maxburst; + } else if (dwc->direction == DMA_DEV_TO_MEM) { + sms = dwc->dws.p_master; + smsize = sconfig->src_maxburst; + dms = dwc->dws.m_master; + dmsize = 0; + } else /* DMA_MEM_TO_MEM */ { + sms = dwc->dws.m_master; + smsize = 0; + dms = dwc->dws.m_master; + dmsize = 0; + } return DWC_CTLL_LLP_D_EN | DWC_CTLL_LLP_S_EN | DWC_CTLL_DST_MSIZE(dmsize) | DWC_CTLL_SRC_MSIZE(smsize) | diff --git a/drivers/dma/dw/idma32.c b/drivers/dma/dw/idma32.c index 58f4078d83fe..3a1b12517655 100644 --- a/drivers/dma/dw/idma32.c +++ b/drivers/dma/dw/idma32.c @@ -202,8 +202,18 @@ static size_t idma32_block2bytes(struct dw_dma_chan *dwc, u32 block, u32 width) static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc) { struct dma_slave_config *sconfig = &dwc->dma_sconfig; - u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0; - u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0; + u8 smsize, dmsize; + + if (dwc->direction == DMA_MEM_TO_DEV) { + smsize = 0; + dmsize = sconfig->dst_maxburst; + } else if (dwc->direction == DMA_DEV_TO_MEM) { + smsize = sconfig->src_maxburst; + dmsize = 0; + } else /* DMA_MEM_TO_MEM */ { + smsize = 0; + dmsize = 0; + } return DWC_CTLL_LLP_D_EN | DWC_CTLL_LLP_S_EN | DWC_CTLL_DST_MSIZE(dmsize) | DWC_CTLL_SRC_MSIZE(smsize);
Currently the CTL LO fields are calculated on the platform-specific basis. It's implemented by means of the prepare_ctllo() callbacks using the ternary operator within the local variables init block at the beginning of the block scope. The functions code currently is relatively hard to comprehend and isn't that optimal since implies four conditional statements executed and two additional local variables defined. Let's simplify the DW AHB DMA prepare_ctllo() method by unrolling the ternary operators into the normal if-else statement, dropping redundant master-interface ID variables and initializing the local variables based on the singly evaluated DMA-transfer direction check. Thus the method will look much more readable since now the fields content can be easily inferred right from the if-else branch. Provide the same update in the Intel DMA32 core driver for sake of the driver code unification. Note besides of the effects described above this update is basically a preparation before dropping the max burst encoding callback. It will require calling the burst fields calculation methods right in the prepare_ctllo() callbacks, which would have made the later function code even more complex. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/dma/dw/dw.c | 24 ++++++++++++++++++------ drivers/dma/dw/idma32.c | 14 ++++++++++++-- 2 files changed, 30 insertions(+), 8 deletions(-)