dmaengine: at_xdmac: several improvements
Subsystems (2)
cover dmaengine: at_xdmac: several improvements
Hi,
This series is based on Vinod's next branch plus Maxime Ripard's series:
"[PATCH v5 00/61] dmaengine: Implement generic slave capabilities retrieval"
Cyrille Pitchen (1):
dmaengine: at_xdmac: wait for in-progress transaction to complete
after pausing a channel
Ludovic Desroches (2):
dmaengine: at_xdmac: simplify channel configuration stuff
dmaengine: at_xdmac: allow muliple dwidths when doing slave transfers
drivers/dma/at_xdmac.c | 51 ++++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 24 deletions(-)
Signature · 1 line
2.0.3
PATCH 1/3 dmaengine: at_xdmac: wait for in-progress transaction to complete after pausing a channel
From: Cyrille Pitchen <[email protected]>
--- drivers/dma/at_xdmac.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/dma/at_xdmac.c +6 -1
@@ -1136,9 +1136,14 @@ static int at_xdmac_device_pause(struct dma_chan *chan)dev_dbg (chan2dev (chan),"%s\n" , __func__); +if (test_and_set_bit (AT_XDMAC_CHAN_IS_PAUSED , &atchan->status)) +return 0 ; +spin_lock_bh (&atchan->lock);at_xdmac_write (atxdmac,AT_XDMAC_GRWS , atchan->mask); -set_bit (AT_XDMAC_CHAN_IS_PAUSED , &atchan->status); +while (at_xdmac_chan_read (atchan,AT_XDMAC_CC ) + & (AT_XDMAC_CC_WRIP |AT_XDMAC_CC_RDIP )) +cpu_relax ();spin_unlock_bh (&atchan->lock);return 0 ;
Signature · 1 line
2.0.3
PATCH 2/3 dmaengine: at_xdmac: simplify channel configuration stuff
In reply to Ludovic DesrochesUsing the cc field of the descriptor simplifies the management of the channel configuration.
--- drivers/dma/at_xdmac.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)
drivers/dma/at_xdmac.c +13 -20
@@ -200,6 +200,7 @@ struct at_xdmac_chan {u8 memif;/* Memory Interface */ u32 per_src_addr;u32 per_dst_addr; +u32 save_cc;u32 save_cim;u32 save_cnda;u32 save_cndc;
@@ -357,14 +358,7 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,*/ if (is_slave_direction (first->direction)) { reg =AT_XDMAC_CNDC_NDVIEW_NDV1 ; -if (first->direction ==DMA_MEM_TO_DEV ) - atchan->cfg[AT_XDMAC_CUR_CFG ] = - atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; -else - atchan->cfg[AT_XDMAC_CUR_CFG ] = - atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , - atchan->cfg[AT_XDMAC_CUR_CFG ]); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , first->lld.mbr_cfg); }else {/* * No need to write AT_XDMAC_CC reg, it will be done when the
@@ -568,7 +562,6 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,struct at_xdmac_desc *first =NULL , *prev =NULL ;struct scatterlist *sg;int i; -u32 cfg;unsigned int xfer_size =0 ;if (!sgl)
@@ -615,17 +608,17 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,if (direction ==DMA_DEV_TO_MEM ) { desc->lld.mbr_sa = atchan->per_src_addr; desc->lld.mbr_da = mem; - cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; }else { desc->lld.mbr_sa = mem; desc->lld.mbr_da = atchan->per_dst_addr; - cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; } - desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ - |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ - |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ - | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ - | len / (1 <<at_xdmac_get_dwidth (cfg));/* microblock length */ + desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ + |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ + |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ + | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ + | len / (1 <<at_xdmac_get_dwidth (desc->lld.mbr_cfg));/* microblock length */ dev_dbg (chan2dev (chan),"%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n" , __func__, &desc->lld.mbr_sa, &desc->lld.mbr_da, desc->lld.mbr_ubc);
@@ -889,7 +882,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,enum dma_status ret;int residue;u32 cur_nda, mask, value; -u8 dwidth =at_xdmac_get_dwidth (atchan->cfg[AT_XDMAC_CUR_CFG ]); +u8 dwidth =0 ; ret =dma_cookie_status (chan, cookie, txstate);if (ret ==DMA_COMPLETE )
@@ -933,6 +926,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,*/ descs_list = &desc->descs_list;list_for_each_entry_safe (desc, _desc, descs_list, desc_node) { + dwidth =at_xdmac_get_dwidth (desc->lld.mbr_cfg); residue -= (desc->lld.mbr_ubc &0xffffff ) << dwidth;if ((desc->lld.mbr_nda &0xfffffffc ) == cur_nda)break ;
@@ -1276,6 +1270,7 @@ static int atmel_xdmac_suspend(struct device *dev)list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) {struct at_xdmac_chan *atchan =to_at_xdmac_chan (chan); + atchan->save_cc =at_xdmac_chan_read (atchan,AT_XDMAC_CC );if (at_xdmac_chan_is_cyclic (atchan)) {if (!at_xdmac_chan_is_paused (atchan))at_xdmac_device_pause (chan);
@@ -1298,7 +1293,6 @@ static int atmel_xdmac_resume(struct device *dev)struct at_xdmac_chan *atchan;struct dma_chan *chan, *_chan;int i; -u32 cfg;clk_prepare_enable (atxdmac->clk);
@@ -1313,8 +1307,7 @@ static int atmel_xdmac_resume(struct device *dev)at_xdmac_write (atxdmac,AT_XDMAC_GE , atxdmac->save_gs);list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) { atchan =to_at_xdmac_chan (chan); - cfg = atchan->cfg[AT_XDMAC_CUR_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , cfg); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , atchan->save_cc);if (at_xdmac_chan_is_cyclic (atchan)) {at_xdmac_chan_write (atchan,AT_XDMAC_CNDA , atchan->save_cnda);at_xdmac_chan_write (atchan,AT_XDMAC_CNDC , atchan->save_cndc);
Signature · 1 line
2.0.3
PATCH 2/3 dmaengine: at_xdmac: simplify channel configuration stuff
Quoted 23 lines · Ludovic Desroches
Using the cc field of the descriptor simplifies the management of the channel configuration.
Signed-off-by Ludovic Desroches <[email protected]>--- drivers/dma/at_xdmac.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)drivers/dma/at_xdmac.c +1
-0@@ -200,6 +200,7 @@ struct at_xdmac_chan {u8 memif;/* Memory Interface */ u32 per_src_addr;u32 per_dst_addr; +u32 save_cc;u32 save_cim;u32 save_cnda;u32 save_cndc;@@ -357,14 +358,7 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,*/ if (is_slave_direction (first->direction)) {
drivers/dma/at_xdmac.c static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan, · line 361
Quoted 10 lines
drivers/dma/at_xdmac.c +1
-8reg =AT_XDMAC_CNDC_NDVIEW_NDV1 ; -if (first->direction ==DMA_MEM_TO_DEV ) - atchan->cfg[AT_XDMAC_CUR_CFG ] = - atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; -else - atchan->cfg[AT_XDMAC_CUR_CFG ] = - atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , - atchan->cfg[AT_XDMAC_CUR_CFG ]); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , first->lld.mbr_cfg);
how is this related to adding cc field?
Quoted 54 lines
drivers/dma/at_xdmac.c +9
-9}else {/* * No need to write AT_XDMAC_CC reg, it will be done when the @@ -568,7 +562,6 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,struct at_xdmac_desc *first =NULL , *prev =NULL ;struct scatterlist *sg;int i; -u32 cfg;unsigned int xfer_size =0 ;if (!sgl)@@ -615,17 +608,17 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,if (direction ==DMA_DEV_TO_MEM ) { desc->lld.mbr_sa = atchan->per_src_addr; desc->lld.mbr_da = mem; - cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; }else { desc->lld.mbr_sa = mem; desc->lld.mbr_da = atchan->per_dst_addr; - cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; } - desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ - |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ - |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ - | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ - | len / (1 <<at_xdmac_get_dwidth (cfg));/* microblock length */ + desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ + |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ + |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ + | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ + | len / (1 <<at_xdmac_get_dwidth (desc->lld.mbr_cfg));/* microblock length */ dev_dbg (chan2dev (chan),"%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n" , __func__, &desc->lld.mbr_sa, &desc->lld.mbr_da, desc->lld.mbr_ubc);@@ -889,7 +882,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,enum dma_status ret;int residue;u32 cur_nda, mask, value; -u8 dwidth =at_xdmac_get_dwidth (atchan->cfg[AT_XDMAC_CUR_CFG ]); +u8 dwidth =0 ; ret =dma_cookie_status (chan, cookie, txstate);if (ret ==DMA_COMPLETE )@@ -933,6 +926,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,*/ descs_list = &desc->descs_list;list_for_each_entry_safe (desc, _desc, descs_list, desc_node) { + dwidth =at_xdmac_get_dwidth (desc->lld.mbr_cfg); residue -= (desc->lld.mbr_ubc &0xffffff ) << dwidth;if ((desc->lld.mbr_nda &0xfffffffc ) == cur_nda)break ;
drivers/dma/at_xdmac.c static int atmel_xdmac_suspend(struct device *dev) · line 1273
Quoted 4 lines
drivers/dma/at_xdmac.c +1
-0@@ -1276,6 +1270,7 @@ static int atmel_xdmac_suspend(struct device *dev)list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) {struct at_xdmac_chan *atchan =to_at_xdmac_chan (chan); + atchan->save_cc =at_xdmac_chan_read (atchan,AT_XDMAC_CC );
okay finally we end us using the new field, the code before that seems to use existing lld.mbr_cfg, shouldnt this be another change explaining the changes...
Quoted 12 lines
drivers/dma/at_xdmac.c +0
-1if (at_xdmac_chan_is_cyclic (atchan)) {if (!at_xdmac_chan_is_paused (atchan))at_xdmac_device_pause (chan);@@ -1298,7 +1293,6 @@ static int atmel_xdmac_resume(struct device *dev)struct at_xdmac_chan *atchan;struct dma_chan *chan, *_chan;int i; -u32 cfg;clk_prepare_enable (atxdmac->clk);
drivers/dma/at_xdmac.c static int atmel_xdmac_resume(struct device *dev) · line 1310
Quoted 6 lines
drivers/dma/at_xdmac.c +1
-2@@ -1313,8 +1307,7 @@ static int atmel_xdmac_resume(struct device *dev)at_xdmac_write (atxdmac,AT_XDMAC_GE , atxdmac->save_gs);list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) { atchan =to_at_xdmac_chan (chan); - cfg = atchan->cfg[AT_XDMAC_CUR_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , cfg); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , atchan->save_cc);
only thing here wer are saving and restoring from save_cc. How does that impact rest of the code above?
Signature · 1 line
~Vinod
Quoted 6 lines
drivers/dma/at_xdmac.c +0
-0if (at_xdmac_chan_is_cyclic (atchan)) {at_xdmac_chan_write (atchan,AT_XDMAC_CNDA , atchan->save_cnda);at_xdmac_chan_write (atchan,AT_XDMAC_CNDC , atchan->save_cndc);Signature · 1 line
2.0.3
PATCH 2/3 dmaengine: at_xdmac: simplify channel configuration stuff
Quoted 27 lines · Vinod Koul
Quoted 26 lines · Ludovic Desroches
Using the cc field of the descriptor simplifies the management of the channel configuration.
Signed-off-by Ludovic Desroches <[email protected]>--- drivers/dma/at_xdmac.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)drivers/dma/at_xdmac.c +1
-2@@ -200,6 +200,7 @@ struct at_xdmac_chan {u8 memif;/* Memory Interface */ u32 per_src_addr;u32 per_dst_addr; +u32 save_cc;u32 save_cim;u32 save_cnda;u32 save_cndc;@@ -357,14 +358,7 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,*/ if (is_slave_direction (first->direction)) { reg =AT_XDMAC_CNDC_NDVIEW_NDV1 ; -if (first->direction ==DMA_MEM_TO_DEV ) - atchan->cfg[AT_XDMAC_CUR_CFG] =
Quoted 8 lines
drivers/dma/at_xdmac.c static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan, · line 361
Quoted 7 lines
drivers/dma/at_xdmac.c +1
-6- atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; - else - atchan->cfg[AT_XDMAC_CUR_CFG] = - atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , - atchan->cfg[AT_XDMAC_CUR_CFG ]); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , first->lld.mbr_cfg);how is this related to adding cc field?
It is not directly related to adding save_cc field but to use the lld cc field which is lld.mbr_cfg instead of cfg[AT_XDMAC_CUR_CFG].
Quoted 58 lines
Quoted 58 lines
drivers/dma/at_xdmac.c +9
-9}else {/* * No need to write AT_XDMAC_CC reg, it will be done when the @@ -568,7 +562,6 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,struct at_xdmac_desc *first =NULL , *prev =NULL ;struct scatterlist *sg;int i; -u32 cfg;unsigned int xfer_size =0 ;if (!sgl)@@ -615,17 +608,17 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,if (direction ==DMA_DEV_TO_MEM ) { desc->lld.mbr_sa = atchan->per_src_addr; desc->lld.mbr_da = mem; - cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; }else { desc->lld.mbr_sa = mem; desc->lld.mbr_da = atchan->per_dst_addr; - cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; } - desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ - |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ - |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ - | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ - | len / (1 <<at_xdmac_get_dwidth (cfg));/* microblock length */ + desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ + |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ + |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ + | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ + | len / (1 <<at_xdmac_get_dwidth (desc->lld.mbr_cfg));/* microblock length */ dev_dbg (chan2dev (chan),"%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n" , __func__, &desc->lld.mbr_sa, &desc->lld.mbr_da, desc->lld.mbr_ubc);@@ -889,7 +882,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,enum dma_status ret;int residue;u32 cur_nda, mask, value; -u8 dwidth =at_xdmac_get_dwidth (atchan->cfg[AT_XDMAC_CUR_CFG ]); +u8 dwidth =0 ; ret =dma_cookie_status (chan, cookie, txstate);if (ret ==DMA_COMPLETE )@@ -933,6 +926,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,*/ descs_list = &desc->descs_list;list_for_each_entry_safe (desc, _desc, descs_list, desc_node) { + dwidth =at_xdmac_get_dwidth (desc->lld.mbr_cfg); residue -= (desc->lld.mbr_ubc &0xffffff ) << dwidth;if ((desc->lld.mbr_nda &0xfffffffc ) == cur_nda)break ;@@ -1276,6 +1270,7 @@ static int atmel_xdmac_suspend(struct device *dev)list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) {struct at_xdmac_chan *atchan =to_at_xdmac_chan (chan);
Quoted 4 lines
drivers/dma/at_xdmac.c static int atmel_xdmac_suspend(struct device *dev) · line 1273
Quoted 1 line
drivers/dma/at_xdmac.c +1
-0+ atchan->save_cc =at_xdmac_chan_read (atchan,AT_XDMAC_CC );okay finally we end us using the new field, the code before that seems to use existing lld.mbr_cfg, shouldnt this be another change explaining the changes...
This patch is not only about adding save_cc field but trying to simplify the way cc register is managed. In my mind, both parts are tied, I have added the save_cc field because I thought it is easier and probably safer to do it in this way instead of relying on cfg[AT_XDMAC_CUR_CFG].
Quoted 12 lines
Quoted 12 lines
drivers/dma/at_xdmac.c +0
-1if (at_xdmac_chan_is_cyclic (atchan)) {if (!at_xdmac_chan_is_paused (atchan))at_xdmac_device_pause (chan);@@ -1298,7 +1293,6 @@ static int atmel_xdmac_resume(struct device *dev)struct at_xdmac_chan *atchan;struct dma_chan *chan, *_chan;int i; -u32 cfg;clk_prepare_enable (atxdmac->clk);
Quoted 10 lines
drivers/dma/at_xdmac.c static int atmel_xdmac_resume(struct device *dev) · line 1310
Quoted 7 lines
drivers/dma/at_xdmac.c +1
-2@@ -1313,8 +1307,7 @@ static int atmel_xdmac_resume(struct device *dev)at_xdmac_write (atxdmac,AT_XDMAC_GE , atxdmac->save_gs);list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) { atchan =to_at_xdmac_chan (chan); - cfg = atchan->cfg[AT_XDMAC_CUR_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , cfg); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , atchan->save_cc);only thing here wer are saving and restoring from
save_cc. How does that impact rest of the code above?
What is your concern about this part? I think it is safer to rely on the value of cc register when suspending instead of a "mirror" image of this register.
Quoted 11 lines
Signature · 1 line
~VinodQuoted 6 lines
drivers/dma/at_xdmac.c +0
-0if (at_xdmac_chan_is_cyclic (atchan)) {at_xdmac_chan_write (atchan,AT_XDMAC_CNDA , atchan->save_cnda);at_xdmac_chan_write (atchan,AT_XDMAC_CNDC , atchan->save_cndc);Signature · 1 line
2.0.3
Ludovic
PATCH 2/3 dmaengine: at_xdmac: simplify channel configuration stuff
Quoted 103 lines · Ludovic Desroches
Quoted 27 lines · Vinod Koul
Quoted 26 lines · Ludovic Desroches
Using the cc field of the descriptor simplifies the management of the channel configuration.
Signed-off-by Ludovic Desroches <[email protected]>--- drivers/dma/at_xdmac.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)drivers/dma/at_xdmac.c +1
-2@@ -200,6 +200,7 @@ struct at_xdmac_chan {u8 memif;/* Memory Interface */ u32 per_src_addr;u32 per_dst_addr; +u32 save_cc;u32 save_cim;u32 save_cnda;u32 save_cndc;@@ -357,14 +358,7 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,*/ if (is_slave_direction (first->direction)) { reg =AT_XDMAC_CNDC_NDVIEW_NDV1 ; -if (first->direction ==DMA_MEM_TO_DEV ) - atchan->cfg[AT_XDMAC_CUR_CFG] =Quoted 8 lines
drivers/dma/at_xdmac.c static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan, · line 361
Quoted 7 lines
drivers/dma/at_xdmac.c +1
-6- atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; - else - atchan->cfg[AT_XDMAC_CUR_CFG] = - atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , - atchan->cfg[AT_XDMAC_CUR_CFG ]); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , first->lld.mbr_cfg);how is this related to adding cc field?
It is not directly related to adding
save_ccfield but to use the lld cc field which is lld.mbr_cfg instead of cfg[AT_XDMAC_CUR_CFG].Quoted 58 lines
Quoted 58 lines
drivers/dma/at_xdmac.c +9
-9}else {/* * No need to write AT_XDMAC_CC reg, it will be done when the @@ -568,7 +562,6 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,struct at_xdmac_desc *first =NULL , *prev =NULL ;struct scatterlist *sg;int i; -u32 cfg;unsigned int xfer_size =0 ;if (!sgl)@@ -615,17 +608,17 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,if (direction ==DMA_DEV_TO_MEM ) { desc->lld.mbr_sa = atchan->per_src_addr; desc->lld.mbr_da = mem; - cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; }else { desc->lld.mbr_sa = mem; desc->lld.mbr_da = atchan->per_dst_addr; - cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; } - desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ - |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ - |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ - | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ - | len / (1 <<at_xdmac_get_dwidth (cfg));/* microblock length */ + desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ + |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ + |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ + | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ + | len / (1 <<at_xdmac_get_dwidth (desc->lld.mbr_cfg));/* microblock length */ dev_dbg (chan2dev (chan),"%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n" , __func__, &desc->lld.mbr_sa, &desc->lld.mbr_da, desc->lld.mbr_ubc);@@ -889,7 +882,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,enum dma_status ret;int residue;u32 cur_nda, mask, value; -u8 dwidth =at_xdmac_get_dwidth (atchan->cfg[AT_XDMAC_CUR_CFG ]); +u8 dwidth =0 ; ret =dma_cookie_status (chan, cookie, txstate);if (ret ==DMA_COMPLETE )@@ -933,6 +926,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,*/ descs_list = &desc->descs_list;list_for_each_entry_safe (desc, _desc, descs_list, desc_node) { + dwidth =at_xdmac_get_dwidth (desc->lld.mbr_cfg); residue -= (desc->lld.mbr_ubc &0xffffff ) << dwidth;if ((desc->lld.mbr_nda &0xfffffffc ) == cur_nda)break ;@@ -1276,6 +1270,7 @@ static int atmel_xdmac_suspend(struct device *dev)list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) {struct at_xdmac_chan *atchan =to_at_xdmac_chan (chan);Quoted 4 lines
drivers/dma/at_xdmac.c static int atmel_xdmac_suspend(struct device *dev) · line 1273
Quoted 1 line
drivers/dma/at_xdmac.c +1
-0+ atchan->save_cc =at_xdmac_chan_read (atchan,AT_XDMAC_CC );okay finally we end us using the new field, the code before that seems to use existing lld.mbr_cfg, shouldnt this be another change explaining the changes...
Quoted 26 lines
This patch is not only about adding
save_ccfield but trying to simplify the way cc register is managed. In my mind, both parts are tied, I have added thesave_ccfield because I thought it is easier and probably safer to do it in this way instead of relying on cfg[AT_XDMAC_CUR_CFG].Quoted 21 lines
Quoted 13 lines
drivers/dma/at_xdmac.c +0
-1if (at_xdmac_chan_is_cyclic (atchan)) {if (!at_xdmac_chan_is_paused (atchan))at_xdmac_device_pause (chan);@@ -1298,7 +1293,6 @@ static int atmel_xdmac_resume(struct device *dev)struct at_xdmac_chan *atchan;struct dma_chan *chan, *_chan;int i; -u32 cfg;clk_prepare_enable (atxdmac->clk);drivers/dma/at_xdmac.c static int atmel_xdmac_resume(struct device *dev) · line 1310
Quoted 6 lines
drivers/dma/at_xdmac.c +1
-2@@ -1313,8 +1307,7 @@ static int atmel_xdmac_resume(struct device *dev)at_xdmac_write (atxdmac,AT_XDMAC_GE , atxdmac->save_gs);list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) { atchan =to_at_xdmac_chan (chan); - cfg = atchan->cfg[AT_XDMAC_CUR_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , cfg); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , atchan->save_cc);only thing here wer are saving and restoring from
save_cc. How does that impact rest of the code above?
this part is the only one which is clear. I think right way would be to split these changes and explain in each one why we are doing them. Down the line when someone is working on this will know precisely why this change was made. Two years is a long time, even we will forget. So good changelog and splitting patches to do just one thing is very helpful
Signature · 1 line
~Vinod
PATCH 2/3 dmaengine: at_xdmac: simplify channel configuration stuff
Quoted 130 lines · Vinod Koul
Quoted 103 lines · Ludovic Desroches
Quoted 27 lines · Vinod Koul
Quoted 26 lines · Ludovic Desroches
Using the cc field of the descriptor simplifies the management of the channel configuration.
Signed-off-by Ludovic Desroches <[email protected]>--- drivers/dma/at_xdmac.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)drivers/dma/at_xdmac.c +1
-2@@ -200,6 +200,7 @@ struct at_xdmac_chan {u8 memif;/* Memory Interface */ u32 per_src_addr;u32 per_dst_addr; +u32 save_cc;u32 save_cim;u32 save_cnda;u32 save_cndc;@@ -357,14 +358,7 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,*/ if (is_slave_direction (first->direction)) { reg =AT_XDMAC_CNDC_NDVIEW_NDV1 ; -if (first->direction ==DMA_MEM_TO_DEV ) - atchan->cfg[AT_XDMAC_CUR_CFG] =Quoted 8 lines
drivers/dma/at_xdmac.c static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan, · line 361
Quoted 7 lines
drivers/dma/at_xdmac.c +1
-6- atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; - else - atchan->cfg[AT_XDMAC_CUR_CFG] = - atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , - atchan->cfg[AT_XDMAC_CUR_CFG ]); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , first->lld.mbr_cfg);how is this related to adding cc field?
It is not directly related to adding
save_ccfield but to use the lld cc field which is lld.mbr_cfg instead of cfg[AT_XDMAC_CUR_CFG].Quoted 58 lines
Quoted 58 lines
drivers/dma/at_xdmac.c +9
-9}else {/* * No need to write AT_XDMAC_CC reg, it will be done when the @@ -568,7 +562,6 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,struct at_xdmac_desc *first =NULL , *prev =NULL ;struct scatterlist *sg;int i; -u32 cfg;unsigned int xfer_size =0 ;if (!sgl)@@ -615,17 +608,17 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,if (direction ==DMA_DEV_TO_MEM ) { desc->lld.mbr_sa = atchan->per_src_addr; desc->lld.mbr_da = mem; - cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG ]; }else { desc->lld.mbr_sa = mem; desc->lld.mbr_da = atchan->per_dst_addr; - cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; + desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; } - desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ - |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ - |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ - | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ - | len / (1 <<at_xdmac_get_dwidth (cfg));/* microblock length */ + desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ + |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ + |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ + | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ + | len / (1 <<at_xdmac_get_dwidth (desc->lld.mbr_cfg));/* microblock length */ dev_dbg (chan2dev (chan),"%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n" , __func__, &desc->lld.mbr_sa, &desc->lld.mbr_da, desc->lld.mbr_ubc);@@ -889,7 +882,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,enum dma_status ret;int residue;u32 cur_nda, mask, value; -u8 dwidth =at_xdmac_get_dwidth (atchan->cfg[AT_XDMAC_CUR_CFG ]); +u8 dwidth =0 ; ret =dma_cookie_status (chan, cookie, txstate);if (ret ==DMA_COMPLETE )@@ -933,6 +926,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,*/ descs_list = &desc->descs_list;list_for_each_entry_safe (desc, _desc, descs_list, desc_node) { + dwidth =at_xdmac_get_dwidth (desc->lld.mbr_cfg); residue -= (desc->lld.mbr_ubc &0xffffff ) << dwidth;if ((desc->lld.mbr_nda &0xfffffffc ) == cur_nda)break ;@@ -1276,6 +1270,7 @@ static int atmel_xdmac_suspend(struct device *dev)list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) {struct at_xdmac_chan *atchan =to_at_xdmac_chan (chan);Quoted 4 lines
drivers/dma/at_xdmac.c static int atmel_xdmac_suspend(struct device *dev) · line 1273
Quoted 1 line
drivers/dma/at_xdmac.c +1
-0+ atchan->save_cc =at_xdmac_chan_read (atchan,AT_XDMAC_CC );okay finally we end us using the new field, the code before that seems to use existing lld.mbr_cfg, shouldnt this be another change explaining the changes...
Quoted 26 lines
This patch is not only about adding
save_ccfield but trying to simplify the way cc register is managed. In my mind, both parts are tied, I have added thesave_ccfield because I thought it is easier and probably safer to do it in this way instead of relying on cfg[AT_XDMAC_CUR_CFG].Quoted 21 lines
Quoted 13 lines
drivers/dma/at_xdmac.c +0
-1if (at_xdmac_chan_is_cyclic (atchan)) {if (!at_xdmac_chan_is_paused (atchan))at_xdmac_device_pause (chan);@@ -1298,7 +1293,6 @@ static int atmel_xdmac_resume(struct device *dev)struct at_xdmac_chan *atchan;struct dma_chan *chan, *_chan;int i; -u32 cfg;clk_prepare_enable (atxdmac->clk);drivers/dma/at_xdmac.c static int atmel_xdmac_resume(struct device *dev) · line 1310
Quoted 6 lines
drivers/dma/at_xdmac.c +1
-2@@ -1313,8 +1307,7 @@ static int atmel_xdmac_resume(struct device *dev)at_xdmac_write (atxdmac,AT_XDMAC_GE , atxdmac->save_gs);list_for_each_entry_safe (chan, _chan, &atxdmac->dma.channels, device_node) { atchan =to_at_xdmac_chan (chan); - cfg = atchan->cfg[AT_XDMAC_CUR_CFG ]; -at_xdmac_chan_write (atchan,AT_XDMAC_CC , cfg); +at_xdmac_chan_write (atchan,AT_XDMAC_CC , atchan->save_cc);only thing here wer are saving and restoring from
save_cc. How does that impact rest of the code above?
Quoted 5 lines
this part is the only one which is clear. I think right way would be to split these changes and explain in each one why we are doing them. Down the line when someone is working on this will know precisely why this change was made. Two years is a long time, even we will forget. So good changelog and splitting patches to do just one thing is very helpful
I don't see why this patch should be split. The goal is to remove the configuration register snapshot and to put it in the lli descriptor. Because I am removing this snapshot, I have to update suspend/resume and the way I have chosen is to directly get the value from the hardware register. If it is a blocking point I'll split it. I think you want me to put the addition of the save_cc field into another patches but I have no reason to do it before patch 2 since I have not encountered any bug yet (even if I think it is safer). And if I don't do it in patch 2, suspend/resume will be broken.
About the commit message I could elaborate a bit more:
This patch simplifies the channel configuration register management. Relying on a "software snapshot" of the configuration is not safe and too complex.
Multiple dwidths will be introduced for slave transfers. In this case, it becomes quite difficult to have an accurate snapshot of the channel configuration register in the way it is done. Using the channel configuration available in the lli descriptor simplifies this stuff.
Regards
Ludovic
PATCH 2/3 dmaengine: at_xdmac: simplify channel configuration stuff
Quoted 8 lines · Ludovic Desroches
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
only thing here wer are saving and restoring from
save_cc. How does that impact rest of the code above?this part is the only one which is clear. I think right way would be to split these changes and explain in each one why we are doing them. Down the line when someone is working on this will know precisely why this change was made. Two years is a long time, even we will forget. So good changelog and splitting patches to do just one thing is very helpful
Quoted 9 lines
I don't see why this patch should be split. The goal is to remove the configuration register snapshot and to put it in the lli descriptor. Because I am removing this snapshot, I have to update suspend/resume and the way I have chosen is to directly get the value from the hardware register. If it is a blocking point I'll split it. I think you want me to put the addition of the
save_ccfield into another patches but I have no reason to do it before patch 2 since I have not encountered any bug yet (even if I think it is safer). And if I don't do it in patch 2, suspend/resume will be broken.
cant the save cc bits come first and then rest of contents in this patch
Quoted 4 lines
About the commit message I could elaborate a bit more:
Quoted 8 lines
This patch simplifies the channel configuration register management. Relying on a "software snapshot" of the configuration is not safe and too complex.
Multiple dwidths will be introduced for slave transfers. In this case, it becomes quite difficult to have an accurate snapshot of the channel configuration register in the way it is done. Using the channel configuration available in the lli descriptor simplifies this stuff.
much better :)
Signature · 1 line
~Vinod
PATCH 3/3 dmaengine: at_xdmac: allow muliple dwidths when doing slave transfers
In reply to Ludovic DesrochesWhen using FIFO, we need to support differents data width in a single transfer. For example, serial device which usually uses 1-byte data width will use 4-bytes data width when using the FIFO. If the transfer size is not aligned on 4-bytes then the end of the transfer will be performed with 1-byte data-width. For that reason, at_xdmac_prep_slave_sg() now builds linked list descriptors using view 2 instead of view 1 so each of them can update the DWIDTH field into the Channel Configuration Register.
--- drivers/dma/at_xdmac.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/dma/at_xdmac.c +10 -5
@@ -25,6 +25,7 @@#include <linux/dmapool.h> #include <linux/interrupt.h> #include <linux/irq.h> +#include <linux/kernel.h> #include <linux/list.h> #include <linux/module.h> #include <linux/of_dma.h>
@@ -352,11 +353,11 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan,at_xdmac_chan_write (atchan,AT_XDMAC_CNDA , reg);/* -* When doing memory to memory transfer we need to use the next +* When doing non cyclic transfer we need to use the next * descriptor view 2 since some fields of the configuration register * depend on transfer size and src/dest addresses. */ -if (is_slave_direction (first->direction)) { +if (at_xdmac_chan_is_cyclic (atchan)) { reg =AT_XDMAC_CNDC_NDVIEW_NDV1 ;at_xdmac_chan_write (atchan,AT_XDMAC_CC , first->lld.mbr_cfg); }else {
@@ -583,7 +584,7 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,/* Prepare descriptors. */ for_each_sg (sgl, sg, sg_len, i) {struct at_xdmac_desc *desc =NULL ; -u32 len, mem; +u32 len, mem, dwidth, fixed_dwidth; len =sg_dma_len (sg); mem =sg_dma_address (sg);
@@ -614,11 +615,15 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, desc->lld.mbr_da = atchan->per_dst_addr; desc->lld.mbr_cfg = atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG ]; } - desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV1 /* next descriptor view */ + dwidth =at_xdmac_get_dwidth (desc->lld.mbr_cfg); + fixed_dwidth =IS_ALIGNED (len,1 << dwidth) + ?at_xdmac_get_dwidth (desc->lld.mbr_cfg) + :AT_XDMAC_CC_DWIDTH_BYTE ; + desc->lld.mbr_ubc =AT_XDMAC_MBR_UBC_NDV2 /* next descriptor view */ |AT_XDMAC_MBR_UBC_NDEN /* next descriptor dst parameter update */ |AT_XDMAC_MBR_UBC_NSEN /* next descriptor src parameter update */ | (i == sg_len -1 ?0 :AT_XDMAC_MBR_UBC_NDE )/* descriptor fetch */ - | len / (1 <<at_xdmac_get_dwidth (desc->lld.mbr_cfg));/* microblock length */ + | (len >> fixed_dwidth);/* microblock length */ dev_dbg (chan2dev (chan),"%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n" , __func__, &desc->lld.mbr_sa, &desc->lld.mbr_da, desc->lld.mbr_ubc);
Signature · 1 line
2.0.3