| Message ID | 20240416162908.24180-5-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: dw: Fix src/dst addr width misconfig | expand |
On Tue, Apr 16, 2024 at 07:28:58PM +0300, Serge Semin wrote: > In order to have a more coherent DW AHB DMA slave configuration method > let's simplify the source and destination channel max-burst calculation > procedure: > > 1. Create the max-burst verification method as it has been just done for > the memory and peripheral address widths. Thus the DWC DMA slave config dwc_config() method ? > method will turn to a set of the verification methods execution. > > 2. Since both the generic DW AHB DMA and Intel DMA32 engines support the "i" in iDMA 32-bit stands for "integrated", so 'Intel iDMA 32-bit' > power-of-2 bursts only, then the specified by the client driver max-burst > values can be converted to being power-of-2 right in the max-burst > verification method. > > 3. Since max-burst encoded value is required on the CTL_LO fields > calculation stage, the encode_maxburst() callback can be easily dropped > from the dw_dma structure meanwhile the encoding procedure will be > executed right in the CTL_LO register value calculation. > > Thus the update will provide the next positive effects: the internal > DMA-slave config structure will contain only the real DMA-transfer config > value, which will be encoded to the DMA-controller register fields only > when it's required on the buffer mapping; the redundant encode_maxburst() > callback will be dropped simplifying the internal HW-abstraction API; > DWC-config method will look more readable executing the verification dwc_config() method ? > functions one-by-one. ... > +static void dwc_verify_maxburst(struct dma_chan *chan) It's inconsistent to the rest of _verify methods. It doesn't verify as it doesn't return anything. Make it int or rename the function. > +{ > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > + > + dwc->dma_sconfig.src_maxburst = > + clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst); > + dwc->dma_sconfig.dst_maxburst = > + clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst); > + > + dwc->dma_sconfig.src_maxburst = > + rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst); > + dwc->dma_sconfig.dst_maxburst = > + rounddown_pow_of_two(dwc->dma_sconfig.dst_maxburst); > +} ... > static int dwc_verify_p_buswidth(struct dma_chan *chan) > - reg_burst = rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst); > + reg_burst = dwc->dma_sconfig.src_maxburst; Seems you have a dependency, need a comment below that maxburst has to be "verified" [whatever] first. ... > +static inline u8 dw_dma_encode_maxburst(u32 maxburst) > +{ > + /* > + * Fix burst size according to dw_dmac. We need to convert them as: > + * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3. > + */ > + return maxburst > 1 ? fls(maxburst) - 2 : 0; > +} Split these moves to another preparatory patch.
On Tue, Apr 16, 2024 at 10:11:58PM +0300, Andy Shevchenko wrote: > On Tue, Apr 16, 2024 at 07:28:58PM +0300, Serge Semin wrote: > > In order to have a more coherent DW AHB DMA slave configuration method > > let's simplify the source and destination channel max-burst calculation > > procedure: > > > > 1. Create the max-burst verification method as it has been just done for > > the memory and peripheral address widths. Thus the DWC DMA slave config > > dwc_config() method > > ? Right. I'll just directly refer to the dwc_config() method here. > > > method will turn to a set of the verification methods execution. > > > > 2. Since both the generic DW AHB DMA and Intel DMA32 engines support the > > "i" in iDMA 32-bit stands for "integrated", so 'Intel iDMA 32-bit' Ok. Thanks for clarification. > > > power-of-2 bursts only, then the specified by the client driver max-burst > > values can be converted to being power-of-2 right in the max-burst > > verification method. > > > > 3. Since max-burst encoded value is required on the CTL_LO fields > > calculation stage, the encode_maxburst() callback can be easily dropped > > from the dw_dma structure meanwhile the encoding procedure will be > > executed right in the CTL_LO register value calculation. > > > > Thus the update will provide the next positive effects: the internal > > DMA-slave config structure will contain only the real DMA-transfer config > > value, which will be encoded to the DMA-controller register fields only > > when it's required on the buffer mapping; the redundant encode_maxburst() > > callback will be dropped simplifying the internal HW-abstraction API; > > DWC-config method will look more readable executing the verification > > dwc_config() method > > ? Ok. > > > functions one-by-one. > > ... > > > +static void dwc_verify_maxburst(struct dma_chan *chan) > > It's inconsistent to the rest of _verify methods. It doesn't verify as it > doesn't return anything. Make it int or rename the function. Making it int won't make much sense since currently the method doesn't imply returning an error status. IMO using "verify" was ok, but since you don't see it suitable please suggest a better alternative. mend, fix, align? > > > +{ > > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > > + > > + dwc->dma_sconfig.src_maxburst = > > + clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst); > > + dwc->dma_sconfig.dst_maxburst = > > + clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst); > > + > > + dwc->dma_sconfig.src_maxburst = > > + rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst); > > + dwc->dma_sconfig.dst_maxburst = > > + rounddown_pow_of_two(dwc->dma_sconfig.dst_maxburst); > > +} > > ... > > > static int dwc_verify_p_buswidth(struct dma_chan *chan) > > - reg_burst = rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst); > > + reg_burst = dwc->dma_sconfig.src_maxburst; > > Seems you have a dependency, need a comment below that maxburst has to be > "verified" [whatever] first. Ok. > > ... > > > +static inline u8 dw_dma_encode_maxburst(u32 maxburst) > > +{ > > + /* > > + * Fix burst size according to dw_dmac. We need to convert them as: > > + * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3. > > + */ > > + return maxburst > 1 ? fls(maxburst) - 2 : 0; > > +} > > Split these moves to another preparatory patch. Ok. -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Apr 17, 2024 at 11:35:39PM +0300, Serge Semin wrote: > On Tue, Apr 16, 2024 at 10:11:58PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 16, 2024 at 07:28:58PM +0300, Serge Semin wrote: ... > > > +static void dwc_verify_maxburst(struct dma_chan *chan) > > > It's inconsistent to the rest of _verify methods. It doesn't verify as it > > doesn't return anything. Make it int or rename the function. > > Making it int won't make much sense since currently the method doesn't > imply returning an error status. IMO using "verify" was ok, but since > you don't see it suitable please suggest a better alternative. mend, > fix, align? My suggestion is (and was) to have it return 0 for now.
On Thu, Apr 18, 2024 at 02:49:26PM +0300, Andy Shevchenko wrote: > On Wed, Apr 17, 2024 at 11:35:39PM +0300, Serge Semin wrote: > > On Tue, Apr 16, 2024 at 10:11:58PM +0300, Andy Shevchenko wrote: > > > On Tue, Apr 16, 2024 at 07:28:58PM +0300, Serge Semin wrote: > > ... > > > > > +static void dwc_verify_maxburst(struct dma_chan *chan) > > > > > It's inconsistent to the rest of _verify methods. It doesn't verify as it > > > doesn't return anything. Make it int or rename the function. > > > > Making it int won't make much sense since currently the method doesn't > > imply returning an error status. IMO using "verify" was ok, but since > > you don't see it suitable please suggest a better alternative. mend, > > fix, align? > > My suggestion is (and was) to have it return 0 for now. Ok. Let's have it returning zero then. -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 61e026310dd8..8b4ecd137ae2 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -779,6 +779,21 @@ bool dw_dma_filter(struct dma_chan *chan, void *param) } EXPORT_SYMBOL_GPL(dw_dma_filter); +static void dwc_verify_maxburst(struct dma_chan *chan) +{ + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); + + dwc->dma_sconfig.src_maxburst = + clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst); + dwc->dma_sconfig.dst_maxburst = + clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst); + + dwc->dma_sconfig.src_maxburst = + rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst); + dwc->dma_sconfig.dst_maxburst = + rounddown_pow_of_two(dwc->dma_sconfig.dst_maxburst); +} + static int dwc_verify_p_buswidth(struct dma_chan *chan) { struct dw_dma_chan *dwc = to_dw_dma_chan(chan); @@ -828,7 +843,7 @@ static int dwc_verify_m_buswidth(struct dma_chan *chan) dwc->dma_sconfig.src_addr_width = mem_width; } else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM) { reg_width = dwc->dma_sconfig.src_addr_width; - reg_burst = rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst); + reg_burst = dwc->dma_sconfig.src_maxburst; dwc->dma_sconfig.dst_addr_width = min(mem_width, reg_width * reg_burst); } @@ -839,15 +854,11 @@ static int dwc_verify_m_buswidth(struct dma_chan *chan) static int dwc_config(struct dma_chan *chan, struct dma_slave_config *sconfig) { struct dw_dma_chan *dwc = to_dw_dma_chan(chan); - struct dw_dma *dw = to_dw_dma(chan->device); int err; memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig)); - dwc->dma_sconfig.src_maxburst = - clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst); - dwc->dma_sconfig.dst_maxburst = - clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst); + dwc_verify_maxburst(chan); err = dwc_verify_p_buswidth(chan); if (err) @@ -857,9 +868,6 @@ static int dwc_config(struct dma_chan *chan, struct dma_slave_config *sconfig) if (err) return err; - dw->encode_maxburst(dwc, &dwc->dma_sconfig.src_maxburst); - dw->encode_maxburst(dwc, &dwc->dma_sconfig.dst_maxburst); - return 0; } diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c index c65438d1f1ff..c52333646edd 100644 --- a/drivers/dma/dw/dw.c +++ b/drivers/dma/dw/dw.c @@ -64,6 +64,15 @@ static size_t dw_dma_block2bytes(struct dw_dma_chan *dwc, u32 block, u32 width) return DWC_CTLH_BLOCK_TS(block) << width; } +static inline u8 dw_dma_encode_maxburst(u32 maxburst) +{ + /* + * Fix burst size according to dw_dmac. We need to convert them as: + * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3. + */ + return maxburst > 1 ? fls(maxburst) - 2 : 0; +} + static u32 dw_dma_prepare_ctllo(struct dw_dma_chan *dwc) { struct dma_slave_config *sconfig = &dwc->dma_sconfig; @@ -73,10 +82,10 @@ static u32 dw_dma_prepare_ctllo(struct dw_dma_chan *dwc) sms = dwc->dws.m_master; smsize = 0; dms = dwc->dws.p_master; - dmsize = sconfig->dst_maxburst; + dmsize = dw_dma_encode_maxburst(sconfig->dst_maxburst); } else if (dwc->direction == DMA_DEV_TO_MEM) { sms = dwc->dws.p_master; - smsize = sconfig->src_maxburst; + smsize = dw_dma_encode_maxburst(sconfig->src_maxburst); dms = dwc->dws.m_master; dmsize = 0; } else /* DMA_MEM_TO_MEM */ { @@ -91,15 +100,6 @@ static u32 dw_dma_prepare_ctllo(struct dw_dma_chan *dwc) DWC_CTLL_DMS(dms) | DWC_CTLL_SMS(sms); } -static void dw_dma_encode_maxburst(struct dw_dma_chan *dwc, u32 *maxburst) -{ - /* - * Fix burst size according to dw_dmac. We need to convert them as: - * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3. - */ - *maxburst = *maxburst > 1 ? fls(*maxburst) - 2 : 0; -} - static void dw_dma_set_device_name(struct dw_dma *dw, int id) { snprintf(dw->name, sizeof(dw->name), "dw:dmac%d", id); @@ -128,7 +128,6 @@ int dw_dma_probe(struct dw_dma_chip *chip) dw->suspend_chan = dw_dma_suspend_chan; dw->resume_chan = dw_dma_resume_chan; dw->prepare_ctllo = dw_dma_prepare_ctllo; - dw->encode_maxburst = dw_dma_encode_maxburst; dw->bytes2block = dw_dma_bytes2block; dw->block2bytes = dw_dma_block2bytes; diff --git a/drivers/dma/dw/idma32.c b/drivers/dma/dw/idma32.c index 3a1b12517655..428aba9fc2db 100644 --- a/drivers/dma/dw/idma32.c +++ b/drivers/dma/dw/idma32.c @@ -199,6 +199,11 @@ static size_t idma32_block2bytes(struct dw_dma_chan *dwc, u32 block, u32 width) return IDMA32C_CTLH_BLOCK_TS(block); } +static inline u32 idma32_encode_maxburst(u32 maxburst) +{ + return maxburst > 1 ? fls(maxburst) - 1 : 0; +} + static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc) { struct dma_slave_config *sconfig = &dwc->dma_sconfig; @@ -206,9 +211,9 @@ static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc) if (dwc->direction == DMA_MEM_TO_DEV) { smsize = 0; - dmsize = sconfig->dst_maxburst; + dmsize = idma32_encode_maxburst(sconfig->dst_maxburst); } else if (dwc->direction == DMA_DEV_TO_MEM) { - smsize = sconfig->src_maxburst; + smsize = idma32_encode_maxburst(sconfig->src_maxburst); dmsize = 0; } else /* DMA_MEM_TO_MEM */ { smsize = 0; @@ -219,11 +224,6 @@ static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc) DWC_CTLL_DST_MSIZE(dmsize) | DWC_CTLL_SRC_MSIZE(smsize); } -static void idma32_encode_maxburst(struct dw_dma_chan *dwc, u32 *maxburst) -{ - *maxburst = *maxburst > 1 ? fls(*maxburst) - 1 : 0; -} - static void idma32_set_device_name(struct dw_dma *dw, int id) { snprintf(dw->name, sizeof(dw->name), "idma32:dmac%d", id); @@ -280,7 +280,6 @@ int idma32_dma_probe(struct dw_dma_chip *chip) dw->suspend_chan = idma32_suspend_chan; dw->resume_chan = idma32_resume_chan; dw->prepare_ctllo = idma32_prepare_ctllo; - dw->encode_maxburst = idma32_encode_maxburst; dw->bytes2block = idma32_bytes2block; dw->block2bytes = idma32_block2bytes; diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h index 76654bd13c1a..5969d9cc8d7a 100644 --- a/drivers/dma/dw/regs.h +++ b/drivers/dma/dw/regs.h @@ -327,7 +327,6 @@ struct dw_dma { void (*suspend_chan)(struct dw_dma_chan *dwc, bool drain); void (*resume_chan)(struct dw_dma_chan *dwc, bool drain); u32 (*prepare_ctllo)(struct dw_dma_chan *dwc); - void (*encode_maxburst)(struct dw_dma_chan *dwc, u32 *maxburst); u32 (*bytes2block)(struct dw_dma_chan *dwc, size_t bytes, unsigned int width, size_t *len); size_t (*block2bytes)(struct dw_dma_chan *dwc, u32 block, u32 width);
In order to have a more coherent DW AHB DMA slave configuration method let's simplify the source and destination channel max-burst calculation procedure: 1. Create the max-burst verification method as it has been just done for the memory and peripheral address widths. Thus the DWC DMA slave config method will turn to a set of the verification methods execution. 2. Since both the generic DW AHB DMA and Intel DMA32 engines support the power-of-2 bursts only, then the specified by the client driver max-burst values can be converted to being power-of-2 right in the max-burst verification method. 3. Since max-burst encoded value is required on the CTL_LO fields calculation stage, the encode_maxburst() callback can be easily dropped from the dw_dma structure meanwhile the encoding procedure will be executed right in the CTL_LO register value calculation. Thus the update will provide the next positive effects: the internal DMA-slave config structure will contain only the real DMA-transfer config value, which will be encoded to the DMA-controller register fields only when it's required on the buffer mapping; the redundant encode_maxburst() callback will be dropped simplifying the internal HW-abstraction API; DWC-config method will look more readable executing the verification functions one-by-one. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/dma/dw/core.c | 26 +++++++++++++++++--------- drivers/dma/dw/dw.c | 23 +++++++++++------------ drivers/dma/dw/idma32.c | 15 +++++++-------- drivers/dma/dw/regs.h | 1 - 4 files changed, 35 insertions(+), 30 deletions(-)