* [v2,1/4] dmaengine: xilinx_dma: Refactor axidma channel allocation
From: Radhey Shyam Pandey @ 2018-09-29 17:17 UTC (permalink / raw)
To: vkoul, dan.j.williams, michal.simek, appana.durga.rao,
radhey.shyam.pandey
Cc: dmaengine, linux-arm-kernel, linux-kernel
In axidma alloc_chan_resources merge BD and cyclic BD allocation.
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes for v2:
None
---
drivers/dma/xilinx/xilinx_dma.c | 36 ++++++++++++++++++------------------
1 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index c124423..06d1632 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -887,6 +887,24 @@ static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
chan->id);
return -ENOMEM;
}
+ /*
+ * For cyclic DMA mode we need to program the tail Descriptor
+ * register with a value which is not a part of the BD chain
+ * so allocating a desc segment during channel allocation for
+ * programming tail descriptor.
+ */
+ chan->cyclic_seg_v = dma_zalloc_coherent(chan->dev,
+ sizeof(*chan->cyclic_seg_v),
+ &chan->cyclic_seg_p, GFP_KERNEL);
+ if (!chan->cyclic_seg_v) {
+ dev_err(chan->dev,
+ "unable to allocate desc segment for cyclic DMA\n");
+ dma_free_coherent(chan->dev, sizeof(*chan->seg_v) *
+ XILINX_DMA_NUM_DESCS, chan->seg_v,
+ chan->seg_p);
+ return -ENOMEM;
+ }
+ chan->cyclic_seg_v->phys = chan->cyclic_seg_p;
for (i = 0; i < XILINX_DMA_NUM_DESCS; i++) {
chan->seg_v[i].hw.next_desc =
@@ -922,24 +940,6 @@ static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
return -ENOMEM;
}
- if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
- /*
- * For cyclic DMA mode we need to program the tail Descriptor
- * register with a value which is not a part of the BD chain
- * so allocating a desc segment during channel allocation for
- * programming tail descriptor.
- */
- chan->cyclic_seg_v = dma_zalloc_coherent(chan->dev,
- sizeof(*chan->cyclic_seg_v),
- &chan->cyclic_seg_p, GFP_KERNEL);
- if (!chan->cyclic_seg_v) {
- dev_err(chan->dev,
- "unable to allocate desc segment for cyclic DMA\n");
- return -ENOMEM;
- }
- chan->cyclic_seg_v->phys = chan->cyclic_seg_p;
- }
-
dma_cookie_init(dchan);
if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
^ permalink raw reply related
* [v2,2/4] dmaengine: xilinx_dma: Refactor axidma channel validation
From: Radhey Shyam Pandey @ 2018-09-29 17:17 UTC (permalink / raw)
To: vkoul, dan.j.williams, michal.simek, appana.durga.rao,
radhey.shyam.pandey
Cc: dmaengine, linux-arm-kernel, linux-kernel
In axidma start_transfer, prefer checking channel states before
other params i.e pending_list. No functional change.
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
Changes for v2:
Modified the commit message to mark it as non-functional change.
---
drivers/dma/xilinx/xilinx_dma.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 06d1632..a37871e 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1271,10 +1271,10 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
if (chan->err)
return;
- if (list_empty(&chan->pending_list))
+ if (!chan->idle)
return;
- if (!chan->idle)
+ if (list_empty(&chan->pending_list))
return;
head_desc = list_first_entry(&chan->pending_list,
^ permalink raw reply related
* [v2,3/4] dmaengine: xilinx_dma: Introduce helper macro for preparing dma address
From: Radhey Shyam Pandey @ 2018-09-29 17:17 UTC (permalink / raw)
To: vkoul, dan.j.williams, michal.simek, appana.durga.rao,
radhey.shyam.pandey
Cc: dmaengine, linux-arm-kernel, linux-kernel
This patch introduces the xilinx_prep_dma_addr_t macro which prepares
dma_addr_t from hardware buffer descriptor LSB and MSB fields. It will
be used in simple dma 64-bit programming sequence.
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
Changes for v2:
New patch- Preparatory change for 4/4 fix.
---
drivers/dma/xilinx/xilinx_dma.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index a37871e..c27ab64 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -190,6 +190,8 @@
/* AXI CDMA Specific Masks */
#define XILINX_CDMA_CR_SGMODE BIT(3)
+#define xilinx_prep_dma_addr_t(addr) \
+ ((dma_addr_t)((u64)addr##_##msb << 32 | (addr)))
/**
* struct xilinx_vdma_desc_hw - Hardware Descriptor
* @next_desc: Next Descriptor Pointer @0x00
^ permalink raw reply related
* [v2,4/4] dmaengine: xilinx_dma: Fix 64-bit simple CDMA transfer
From: Radhey Shyam Pandey @ 2018-09-29 17:18 UTC (permalink / raw)
To: vkoul, dan.j.williams, michal.simek, appana.durga.rao,
radhey.shyam.pandey
Cc: dmaengine, linux-arm-kernel, linux-kernel
In AXI CDMA simple mode also pass MSB bits of source and destination
address to xilinx_write function. This fixes simple CDMA operation
mode using 64-bit addressing.
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes for v2:
Use helper macro for preparing dma_addr_t.
---
drivers/dma/xilinx/xilinx_dma.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index c27ab64..d04ef85 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1247,8 +1247,10 @@ static void xilinx_cdma_start_transfer(struct xilinx_dma_chan *chan)
hw = &segment->hw;
- xilinx_write(chan, XILINX_CDMA_REG_SRCADDR, hw->src_addr);
- xilinx_write(chan, XILINX_CDMA_REG_DSTADDR, hw->dest_addr);
+ xilinx_write(chan, XILINX_CDMA_REG_SRCADDR,
+ xilinx_prep_dma_addr_t(hw->src_addr));
+ xilinx_write(chan, XILINX_CDMA_REG_DSTADDR,
+ xilinx_prep_dma_addr_t(hw->dest_addr));
/* Start the transfer */
dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
^ permalink raw reply related
* [v2,1/3] arm64: dts: actions: s900: Enable Tx DMA for UART5
From: kbuild test robot @ 2018-09-30 2:04 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: kbuild-all, vkoul, dan.j.williams, afaerber, robh+dt, gregkh,
jslaby, linux-serial, dmaengine, liuwei, 96boards, devicetree,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-kernel,
hzhang, bdong, manivannanece23, thomas.liau, jeff.chen, pn,
edgar.righi
Hi Manivannan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.19-rc5 next-20180928]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Manivannan-Sadhasivam/Add-slave-DMA-support-for-Actions-Semi-S900-SoC/20180929-155016
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm64
All errors (new ones prefixed by >>):
>> ERROR: Input tree has errors, aborting (use -f to force output)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* dt-bindings: dmaengine: usb-dmac: Add binding for r8a7744
From: Simon Horman @ 2018-10-01 13:00 UTC (permalink / raw)
To: Biju Das
Cc: Vinod Koul, Rob Herring, Mark Rutland, dmaengine, devicetree,
Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
linux-renesas-soc
On Thu, Sep 27, 2018 at 01:46:41PM +0100, Biju Das wrote:
> This patch adds binding for r8a7744 (RZ/G1N).
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* dmaengine: timb_dma: Use proper enum in td_prep_slave_sg
From: Vinod Koul @ 2018-10-02 14:41 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: Dan Williams, dmaengine, linux-kernel, Nick Desaulniers
On 11-09-18, 16:20, Nathan Chancellor wrote:
> Clang warns when implicitly converting from one enumerated type to
> another. Avoid this by using the equivalent value from the expected
> type.
>
> drivers/dma/timb_dma.c:548:27: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
> td_desc->desc_list_len, DMA_MEM_TO_DEV);
> ^~~~~~~~~~~~~~
> 1 warning generated.
Applied, thanks
^ permalink raw reply
* dmaengine: ep93xx: Return proper enum in ep93xx_dma_chan_direction
From: Vinod Koul @ 2018-10-02 14:42 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: Dan Williams, dmaengine, linux-kernel, Nick Desaulniers
On 11-09-18, 16:40, Nathan Chancellor wrote:
> Clang warns when implicitly converting from one enumerated type to
> another. Avoid this by using the equivalent value from the expected
> type.
>
> In file included from drivers/dma/ep93xx_dma.c:30:
> ./include/linux/platform_data/dma-ep93xx.h:88:10: warning: implicit
> conversion from enumeration type 'enum dma_data_direction' to different
> enumeration type 'enum dma_transfer_direction' [-Wenum-conversion]
> return DMA_NONE;
> ~~~~~~ ^~~~~~~~
> 1 warning generated.
Applied, thanks
^ permalink raw reply
* dt-bindings: rcar-dmac: Document r8a7744 support
From: Vinod Koul @ 2018-10-02 14:48 UTC (permalink / raw)
To: Biju Das
Cc: Rob Herring, Mark Rutland, dmaengine, devicetree, Simon Horman,
Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
linux-renesas-soc
On 17-09-18, 16:18, Biju Das wrote:
> Renesas RZ/G SoC also have the R-Car gen2/3 compatible DMA controllers.
> Document RZ/G1N (also known as R8A7744) SoC bindings.
Applied, thanks
^ permalink raw reply
* dt-bindings: dmaengine: usb-dmac: Add binding for r8a7744
From: Vinod Koul @ 2018-10-02 14:49 UTC (permalink / raw)
To: Biju Das
Cc: Rob Herring, Mark Rutland, dmaengine, devicetree, Simon Horman,
Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
linux-renesas-soc
On 27-09-18, 13:46, Biju Das wrote:
> This patch adds binding for r8a7744 (RZ/G1N).
Applied, thanks
^ permalink raw reply
* dmaengine: ioat: fix prototype of ioat_enumerate_channels
From: Vinod Koul @ 2018-10-02 14:52 UTC (permalink / raw)
To: Rami Rosen; +Cc: dmaengine, dave.jiang, dan.j.williams
On 27-09-18, 07:02, Rami Rosen wrote:
> This patch changes the return type of the static method
> ioat_init_channel() to be void. There is no need for
> this method to retrun any value, and the return
> value of this static method is not used anyhow.
> (Setting dma->chancnt is enough).
Sorry am not able to apply this, can you rebase on dmaengine-next and
resend
^ permalink raw reply
* [v5,4/7] dmaengine: xilinx_dma: program hardware supported buffer length
From: Vinod Koul @ 2018-10-02 14:56 UTC (permalink / raw)
To: Andrea Merello
Cc: dan.j.williams, michal.simek, appana.durga.rao, dmaengine,
linux-arm-kernel, linux-kernel, Rob Herring, Mark Rutland,
devicetree, Radhey Shyam Pandey
On 28-09-18, 08:53, Andrea Merello wrote:
> On Tue, Sep 18, 2018 at 6:25 PM Vinod <vkoul@kernel.org> wrote:
> > > @@ -964,7 +968,7 @@ static int xilinx_dma_calc_copysize(struct xilinx_dma_chan *chan,
> > > int size, int done)
> > > {
> > > size_t copy = min_t(size_t, size - done,
> > > - XILINX_DMA_MAX_TRANS_LEN);
> > > + chan->xdev->max_buffer_len);
> >
> > hmm why not add max_buffer_len in patch 1 again, and then use default
> > len as XILINX_DMA_MAX_TRANS_LEN and add multiple lengths here :)
>
> Sorry, I'm not getting your point. Could you please elaborate the "add
> multiple lengths here" thing ?
IIRC (sorry been travelling and vacation), add
chan->xdev->max_buffer_len in patch 1 and initialize it to
XILINX_DMA_MAX_TRANS_LEN. Then in subsequent patches update the length.
^ permalink raw reply
* [v5,2/7] dmaengine: xilinx_dma: in axidma slave_sg and dma_cyclic mode align split descriptors
From: Vinod Koul @ 2018-10-02 14:58 UTC (permalink / raw)
To: Andrea Merello
Cc: dan.j.williams, michal.simek, appana.durga.rao, dmaengine,
linux-arm-kernel, linux-kernel, Rob Herring, Mark Rutland,
devicetree, Radhey Shyam Pandey
On 28-09-18, 09:11, Andrea Merello wrote:
> On Tue, Sep 18, 2018 at 6:21 PM Vinod <vkoul@kernel.org> wrote:
> > > @@ -1804,7 +1817,7 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
> > > * Calculate the maximum number of bytes to transfer,
> > > * making sure it is less than the hw limit
> > > */
> > > - copy = xilinx_dma_calc_copysize(sg_dma_len(sg),
> > > + copy = xilinx_dma_calc_copysize(chan, sg_dma_len(sg),
> >
> > why not keep chan in patch 1 and add only handling in patch 2, seems
> > less churn to me..
>
> Indeed this was something I was unsure about.. I ended up in feeling
> better not to add introduce a function that takes an unused (yet)
> argument, but I can change this of course :)
IMO It is fine to add a user in subsequent patch in a series. Not fine to
add something and not use in "that" series :)
^ permalink raw reply
* [v1,1/1] dmaengine: stm32-dma: check whether length is aligned on FIFO threshold
From: Vinod Koul @ 2018-10-02 15:02 UTC (permalink / raw)
To: Pierre-Yves MORDRET
Cc: Dan Williams, Maxime Coquelin, Alexandre Torgue, dmaengine,
linux-arm-kernel, linux-kernel
On 11-09-18, 09:31, Pierre-Yves MORDRET wrote:
> When a period length is not multiple of FIFO some data may be stuck
> within FIFO.
>
> Burst/FIFO Threshold/Period or buffer length check has to be hardened
>
> In any case DMA will grant any request from client but will degraded
> any parameters whether awkward.
Applied, thanks
^ permalink raw reply
* XXX PCI/AER: remove unused variables
From: Arnd Bergmann @ 2018-10-02 21:02 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Arnd Bergmann, Oza Pawandeep, Jeff Kirsher, David S. Miller,
Solarflare linux maintainers, Edward Cree, Jacob Keller,
Amritha Nambiar, Alan Brady, Alexander Duyck, Shannon Nelson,
Jesper Dangaard Brouer, dmaengine, linux-kernel, intel-wired-lan,
netdev
A couple of files now contain a function with a return
code variable that is no longer used:
drivers/net/ethernet/sfc/efx.c: In function 'efx_io_slot_reset':
drivers/net/ethernet/sfc/efx.c:3824:6: error: unused variable 'rc' [-Werror=unused-variable]
int rc;
^~
drivers/net/ethernet/sfc/falcon/efx.c: In function 'ef4_io_slot_reset':
Oza Pawandeep <poza@codeaurora.org>
drivers/net/ethernet/sfc/falcon/efx.c:3163:6: error: unused variable 'rc' [-Werror=unused-variable]
int rc;
^~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c: In function 'ixgbe_io_slot_reset':
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:11143:6: error: unused variable 'err' [-Werror=unused-variable]
int err;
^~~
drivers/net/ethernet/intel/i40e/i40e_main.c: In function 'i40e_pci_error_slot_reset':
drivers/net/ethernet/intel/i40e/i40e_main.c:14555:6: error: unused variable 'err' [-Werror=unused-variable]
int err;
drivers/dma/ioat/init.c: In function 'ioat_pcie_error_slot_reset':
drivers/dma/ioat/init.c:1255:6: error: unused variable 'err' [-Werror=unused-variable]
int err;
This removes all the ones I found during randconfig build testing.
Fixes: 6dcde3e574b2 ("XXX PCI/AER: Remove pci_cleanup_aer_uncorrect_error_status() calls")
Cc: Oza Pawandeep <poza@codeaurora.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/dma/ioat/init.c | 1 -
drivers/net/ethernet/intel/i40e/i40e_main.c | 1 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 -
drivers/net/ethernet/sfc/efx.c | 1 -
drivers/net/ethernet/sfc/falcon/efx.c | 1 -
5 files changed, 5 deletions(-)
diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 80c475fb8ede..bd8db5c99597 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1252,7 +1252,6 @@ static pci_ers_result_t ioat_pcie_error_detected(struct pci_dev *pdev,
static pci_ers_result_t ioat_pcie_error_slot_reset(struct pci_dev *pdev)
{
pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
- int err;
dev_dbg(&pdev->dev, "%s post reset handling\n", DRV_NAME);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4654ac69dfb6..bc71a21c1dc2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -14552,7 +14552,6 @@ static pci_ers_result_t i40e_pci_error_slot_reset(struct pci_dev *pdev)
{
struct i40e_pf *pf = pci_get_drvdata(pdev);
pci_ers_result_t result;
- int err;
u32 reg;
dev_dbg(&pdev->dev, "%s\n", __func__);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 9bee51d900ac..0a2b9cad4b1c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -11140,7 +11140,6 @@ static pci_ers_result_t ixgbe_io_slot_reset(struct pci_dev *pdev)
{
struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
pci_ers_result_t result;
- int err;
if (pci_enable_device_mem(pdev)) {
e_err(probe, "Cannot re-enable PCI device after reset.\n");
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index ec941ca967b3..98fe7e762e17 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -3821,7 +3821,6 @@ static pci_ers_result_t efx_io_slot_reset(struct pci_dev *pdev)
{
struct efx_nic *efx = pci_get_drvdata(pdev);
pci_ers_result_t status = PCI_ERS_RESULT_RECOVERED;
- int rc;
if (pci_enable_device(pdev)) {
netif_err(efx, hw, efx->net_dev,
diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
index 9b728efd71a4..8b1f94d7a6c5 100644
--- a/drivers/net/ethernet/sfc/falcon/efx.c
+++ b/drivers/net/ethernet/sfc/falcon/efx.c
@@ -3160,7 +3160,6 @@ static pci_ers_result_t ef4_io_slot_reset(struct pci_dev *pdev)
{
struct ef4_nic *efx = pci_get_drvdata(pdev);
pci_ers_result_t status = PCI_ERS_RESULT_RECOVERED;
- int rc;
if (pci_enable_device(pdev)) {
netif_err(efx, hw, efx->net_dev,
^ permalink raw reply related
* XXX PCI/AER: remove unused variables
From: Bjorn Helgaas @ 2018-10-02 21:27 UTC (permalink / raw)
To: Arnd Bergmann
Cc: poza, Kirsher, Jeffrey T, David Miller, linux-net-drivers,
Edward Cree, Jacob Keller, amritha.nambiar, alan.brady,
Alex Duyck, shannon.nelson, brouer, dmaengine,
Linux Kernel Mailing List, intel-wired-lan, netdev
On Tue, Oct 2, 2018 at 4:04 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> A couple of files now contain a function with a return
> code variable that is no longer used:
These are fixed already, thanks.
> drivers/net/ethernet/sfc/efx.c: In function 'efx_io_slot_reset':
> drivers/net/ethernet/sfc/efx.c:3824:6: error: unused variable 'rc' [-Werror=unused-variable]
> int rc;
> ^~
> drivers/net/ethernet/sfc/falcon/efx.c: In function 'ef4_io_slot_reset':
> Oza Pawandeep <poza@codeaurora.org>
> drivers/net/ethernet/sfc/falcon/efx.c:3163:6: error: unused variable 'rc' [-Werror=unused-variable]
> int rc;
> ^~
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c: In function 'ixgbe_io_slot_reset':
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:11143:6: error: unused variable 'err' [-Werror=unused-variable]
> int err;
> ^~~
> drivers/net/ethernet/intel/i40e/i40e_main.c: In function 'i40e_pci_error_slot_reset':
> drivers/net/ethernet/intel/i40e/i40e_main.c:14555:6: error: unused variable 'err' [-Werror=unused-variable]
> int err;
>
> drivers/dma/ioat/init.c: In function 'ioat_pcie_error_slot_reset':
> drivers/dma/ioat/init.c:1255:6: error: unused variable 'err' [-Werror=unused-variable]
> int err;
>
> This removes all the ones I found during randconfig build testing.
>
> Fixes: 6dcde3e574b2 ("XXX PCI/AER: Remove pci_cleanup_aer_uncorrect_error_status() calls")
> Cc: Oza Pawandeep <poza@codeaurora.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/dma/ioat/init.c | 1 -
> drivers/net/ethernet/intel/i40e/i40e_main.c | 1 -
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 -
> drivers/net/ethernet/sfc/efx.c | 1 -
> drivers/net/ethernet/sfc/falcon/efx.c | 1 -
> 5 files changed, 5 deletions(-)
>
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 80c475fb8ede..bd8db5c99597 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -1252,7 +1252,6 @@ static pci_ers_result_t ioat_pcie_error_detected(struct pci_dev *pdev,
> static pci_ers_result_t ioat_pcie_error_slot_reset(struct pci_dev *pdev)
> {
> pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
> - int err;
>
> dev_dbg(&pdev->dev, "%s post reset handling\n", DRV_NAME);
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 4654ac69dfb6..bc71a21c1dc2 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -14552,7 +14552,6 @@ static pci_ers_result_t i40e_pci_error_slot_reset(struct pci_dev *pdev)
> {
> struct i40e_pf *pf = pci_get_drvdata(pdev);
> pci_ers_result_t result;
> - int err;
> u32 reg;
>
> dev_dbg(&pdev->dev, "%s\n", __func__);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 9bee51d900ac..0a2b9cad4b1c 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -11140,7 +11140,6 @@ static pci_ers_result_t ixgbe_io_slot_reset(struct pci_dev *pdev)
> {
> struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
> pci_ers_result_t result;
> - int err;
>
> if (pci_enable_device_mem(pdev)) {
> e_err(probe, "Cannot re-enable PCI device after reset.\n");
> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
> index ec941ca967b3..98fe7e762e17 100644
> --- a/drivers/net/ethernet/sfc/efx.c
> +++ b/drivers/net/ethernet/sfc/efx.c
> @@ -3821,7 +3821,6 @@ static pci_ers_result_t efx_io_slot_reset(struct pci_dev *pdev)
> {
> struct efx_nic *efx = pci_get_drvdata(pdev);
> pci_ers_result_t status = PCI_ERS_RESULT_RECOVERED;
> - int rc;
>
> if (pci_enable_device(pdev)) {
> netif_err(efx, hw, efx->net_dev,
> diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
> index 9b728efd71a4..8b1f94d7a6c5 100644
> --- a/drivers/net/ethernet/sfc/falcon/efx.c
> +++ b/drivers/net/ethernet/sfc/falcon/efx.c
> @@ -3160,7 +3160,6 @@ static pci_ers_result_t ef4_io_slot_reset(struct pci_dev *pdev)
> {
> struct ef4_nic *efx = pci_get_drvdata(pdev);
> pci_ers_result_t status = PCI_ERS_RESULT_RECOVERED;
> - int rc;
>
> if (pci_enable_device(pdev)) {
> netif_err(efx, hw, efx->net_dev,
> --
> 2.18.0
>
^ permalink raw reply
* dmaengine: ioat: fix prototype of ioat_enumerate_channels
From: Rami Rosen @ 2018-10-04 21:03 UTC (permalink / raw)
To: dmaengine; +Cc: vkoul, dave.jiang, dan.j.williams, Rami Rosen
Signed-off-by: Rami Rosen <ramirose@gmail.com>
---
drivers/dma/ioat/init.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 21a5708985bc..0fec3c554fe3 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -129,7 +129,7 @@ static void
ioat_init_channel(struct ioatdma_device *ioat_dma,
struct ioatdma_chan *ioat_chan, int idx);
static void ioat_intr_quirk(struct ioatdma_device *ioat_dma);
-static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma);
+static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma);
static int ioat3_dma_self_test(struct ioatdma_device *ioat_dma);
static int ioat_dca_enabled = 1;
@@ -575,7 +575,7 @@ static void ioat_dma_remove(struct ioatdma_device *ioat_dma)
* ioat_enumerate_channels - find and initialize the device's channels
* @ioat_dma: the ioat dma device to be enumerated
*/
-static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
+static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
{
struct ioatdma_chan *ioat_chan;
struct device *dev = &ioat_dma->pdev->dev;
@@ -594,7 +594,7 @@ static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
xfercap_log = readb(ioat_dma->reg_base + IOAT_XFERCAP_OFFSET);
xfercap_log &= 0x1f; /* bits [4:0] valid */
if (xfercap_log == 0)
- return 0;
+ return;
dev_dbg(dev, "%s: xfercap = %d\n", __func__, 1 << xfercap_log);
for (i = 0; i < dma->chancnt; i++) {
@@ -611,7 +611,6 @@ static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
}
}
dma->chancnt = i;
- return i;
}
/**
^ permalink raw reply related
* dmaengine: ioat: fix prototype of ioat_enumerate_channels
From: Vinod Koul @ 2018-10-05 14:24 UTC (permalink / raw)
To: Rami Rosen; +Cc: dmaengine, dave.jiang, dan.j.williams
On 05-10-18, 00:03, Rami Rosen wrote:
> Signed-off-by: Rami Rosen <ramirose@gmail.com>
> ---
> drivers/dma/ioat/init.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
Applied, thanks
>
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 21a5708985bc..0fec3c554fe3 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -129,7 +129,7 @@ static void
> ioat_init_channel(struct ioatdma_device *ioat_dma,
> struct ioatdma_chan *ioat_chan, int idx);
> static void ioat_intr_quirk(struct ioatdma_device *ioat_dma);
> -static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma);
> +static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma);
> static int ioat3_dma_self_test(struct ioatdma_device *ioat_dma);
>
> static int ioat_dca_enabled = 1;
> @@ -575,7 +575,7 @@ static void ioat_dma_remove(struct ioatdma_device *ioat_dma)
> * ioat_enumerate_channels - find and initialize the device's channels
> * @ioat_dma: the ioat dma device to be enumerated
> */
> -static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
> +static void ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
> {
> struct ioatdma_chan *ioat_chan;
> struct device *dev = &ioat_dma->pdev->dev;
> @@ -594,7 +594,7 @@ static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
> xfercap_log = readb(ioat_dma->reg_base + IOAT_XFERCAP_OFFSET);
> xfercap_log &= 0x1f; /* bits [4:0] valid */
> if (xfercap_log == 0)
> - return 0;
> + return;
> dev_dbg(dev, "%s: xfercap = %d\n", __func__, 1 << xfercap_log);
>
> for (i = 0; i < dma->chancnt; i++) {
> @@ -611,7 +611,6 @@ static int ioat_enumerate_channels(struct ioatdma_device *ioat_dma)
> }
> }
> dma->chancnt = i;
> - return i;
> }
>
> /**
> --
> 2.17.1
^ permalink raw reply
* XXX PCI/AER: remove unused variables
From: Vinod Koul @ 2018-10-05 14:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Bjorn Helgaas, Oza Pawandeep, Jeff Kirsher, David S. Miller,
Solarflare linux maintainers, Edward Cree, Jacob Keller,
Amritha Nambiar, Alan Brady, Alexander Duyck, Shannon Nelson,
Jesper Dangaard Brouer, dmaengine, linux-kernel, intel-wired-lan,
netdev
On 02-10-18, 23:02, Arnd Bergmann wrote:
> A couple of files now contain a function with a return
> code variable that is no longer used:
>
> drivers/net/ethernet/sfc/efx.c: In function 'efx_io_slot_reset':
> drivers/net/ethernet/sfc/efx.c:3824:6: error: unused variable 'rc' [-Werror=unused-variable]
> int rc;
> ^~
> drivers/net/ethernet/sfc/falcon/efx.c: In function 'ef4_io_slot_reset':
> Oza Pawandeep <poza@codeaurora.org>
> drivers/net/ethernet/sfc/falcon/efx.c:3163:6: error: unused variable 'rc' [-Werror=unused-variable]
> int rc;
> ^~
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c: In function 'ixgbe_io_slot_reset':
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:11143:6: error: unused variable 'err' [-Werror=unused-variable]
> int err;
> ^~~
> drivers/net/ethernet/intel/i40e/i40e_main.c: In function 'i40e_pci_error_slot_reset':
> drivers/net/ethernet/intel/i40e/i40e_main.c:14555:6: error: unused variable 'err' [-Werror=unused-variable]
> int err;
>
> drivers/dma/ioat/init.c: In function 'ioat_pcie_error_slot_reset':
> drivers/dma/ioat/init.c:1255:6: error: unused variable 'err' [-Werror=unused-variable]
> int err;
>
> This removes all the ones I found during randconfig build testing.
>
> Fixes: 6dcde3e574b2 ("XXX PCI/AER: Remove pci_cleanup_aer_uncorrect_error_status() calls")
> Cc: Oza Pawandeep <poza@codeaurora.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/dma/ioat/init.c | 1 -
For this:
Acked-by: Vinod Koul <vkoul@kernel.org>
^ permalink raw reply
* [v3,2/2] dmaengine: uniphier-mdmac: add UniPhier MIO DMAC driver
From: Vinod Koul @ 2018-10-05 14:57 UTC (permalink / raw)
To: Masahiro Yamada
Cc: dmaengine, Masami Hiramatsu, Jassi Brar, linux-kernel,
Dan Williams, linux-arm-kernel
On 13-09-18, 09:51, Masahiro Yamada wrote:
> +#define UNIPHIER_MDMAC_CH_IRQ_STAT 0x010 // current hw status (RO)
> +#define UNIPHIER_MDMAC_CH_IRQ_REQ 0x014 // latched STAT (WOC)
> +#define UNIPHIER_MDMAC_CH_IRQ_EN 0x018 // IRQ enable mask
> +#define UNIPHIER_MDMAC_CH_IRQ_DET 0x01c // REQ & EN (RO)
> +#define UNIPHIER_MDMAC_CH_IRQ__ABORT BIT(13)
> +#define UNIPHIER_MDMAC_CH_IRQ__DONE BIT(1)
why notation if UNIPHIER_MDMAC_CH_IRQ__FOO ?
> +#define UNIPHIER_MDMAC_CH_SRC_MODE 0x020 // mode of source
> +#define UNIPHIER_MDMAC_CH_DEST_MODE 0x024 // mode of destination
> +#define UNIPHIER_MDMAC_CH_MODE__ADDR_INC (0 << 4)
> +#define UNIPHIER_MDMAC_CH_MODE__ADDR_DEC (1 << 4)
> +#define UNIPHIER_MDMAC_CH_MODE__ADDR_FIXED (2 << 4)
> +#define UNIPHIER_MDMAC_CH_SRC_ADDR 0x028 // source address
> +#define UNIPHIER_MDMAC_CH_DEST_ADDR 0x02c // destination address
> +#define UNIPHIER_MDMAC_CH_SIZE 0x030 // transfer bytes
Please use /* comment */ style for these
> +/* mc->vc.lock must be held by caller */
> +static struct uniphier_mdmac_desc *__uniphier_mdmac_next_desc(
> + struct uniphier_mdmac_chan *mc)
this can be made to look better by:
static struct uniphier_mdmac_desc *
__uniphier_mdmac_next_desc(struct uniphier_mdmac_chan *mc)
Btw why leading __ for function name here and other places?
> +{
> + struct virt_dma_desc *vd;
> +
> + vd = vchan_next_desc(&mc->vc);
> + if (!vd) {
> + mc->md = NULL;
> + return NULL;
> + }
> +
> + list_del(&vd->node);
> +
> + mc->md = to_uniphier_mdmac_desc(vd);
> +
> + return mc->md;
> +}
> +
> +/* mc->vc.lock must be held by caller */
> +static void __uniphier_mdmac_handle(struct uniphier_mdmac_chan *mc,
> + struct uniphier_mdmac_desc *md)
please align this to previous line opening brace (hint checkpatch with
--strict option will give you the warning)
> +static irqreturn_t uniphier_mdmac_interrupt(int irq, void *dev_id)
> +{
> + struct uniphier_mdmac_chan *mc = dev_id;
> + struct uniphier_mdmac_desc *md;
> + irqreturn_t ret = IRQ_HANDLED;
> + u32 irq_stat;
> +
> + spin_lock(&mc->vc.lock);
> +
> + irq_stat = readl(mc->reg_ch_base + UNIPHIER_MDMAC_CH_IRQ_DET);
> +
> + /*
> + * Some channels share a single interrupt line. If the IRQ status is 0,
> + * this is probably triggered by a different channel.
> + */
> + if (!irq_stat) {
> + ret = IRQ_NONE;
> + goto out;
> + }
> +
> + /* write 1 to clear */
> + writel(irq_stat, mc->reg_ch_base + UNIPHIER_MDMAC_CH_IRQ_REQ);
> +
> + /*
> + * UNIPHIER_MDMAC_CH_IRQ__DONE interrupt is asserted even when the DMA
> + * is aborted. To distinguish the normal completion and the abort,
^^^^
double space..
> +static int uniphier_mdmac_config(struct dma_chan *chan,
> + struct dma_slave_config *config)
> +{
> + /* Nothing in struct dma_slave_config is configurable. */
> + return 0;
> +}
I dont think config callback is mandatory, so we can drop this
> +static enum dma_status uniphier_mdmac_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct virt_dma_chan *vc;
> + struct virt_dma_desc *vd;
> + struct uniphier_mdmac_chan *mc;
> + struct uniphier_mdmac_desc *md = NULL;
> + enum dma_status stat;
> + unsigned long flags;
> +
> + stat = dma_cookie_status(chan, cookie, txstate);
> + /* Return immediately if we do not need to compute the residue. */
> + if (stat == DMA_COMPLETE || !txstate)
> + return stat;
> +
> + vc = to_virt_chan(chan);
> +
> + spin_lock_irqsave(&vc->lock, flags);
> +
> + mc = to_uniphier_mdmac_chan(vc);
> +
> + if (mc->md && mc->md->vd.tx.cookie == cookie)
> + md = mc->md;
> +
> + if (!md) {
> + vd = vchan_find_desc(vc, cookie);
> + if (vd)
> + md = to_uniphier_mdmac_desc(vd);
> + }
in both of these cases you are calling __uniphier_mdmac_get_residue()
which reads the register and updates. But I think you should read
register only in the first case when descriptor is submitted and not in
latter case when descriptor is queued
> +static int uniphier_mdmac_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct uniphier_mdmac_device *mdev;
> + struct dma_device *ddev;
> + struct resource *res;
> + int nr_chans, ret, i;
> +
> + nr_chans = platform_irq_count(pdev);
> + if (nr_chans < 0)
> + return nr_chans;
> +
> + ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> + if (ret)
> + return ret;
> +
> + mdev = devm_kzalloc(dev, struct_size(mdev, channels, nr_chans),
> + GFP_KERNEL);
> + if (!mdev)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + mdev->reg_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(mdev->reg_base))
> + return PTR_ERR(mdev->reg_base);
> +
> + mdev->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(mdev->clk)) {
> + dev_err(dev, "failed to get clock\n");
> + return PTR_ERR(mdev->clk);
> + }
> +
> + ret = clk_prepare_enable(mdev->clk);
> + if (ret)
> + return ret;
> +
> + ddev = &mdev->ddev;
> + ddev->dev = dev;
> + dma_cap_set(DMA_PRIVATE, ddev->cap_mask);
> + ddev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
> + ddev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
undefined?
> +static int uniphier_mdmac_remove(struct platform_device *pdev)
> +{
> + struct uniphier_mdmac_device *mdev = platform_get_drvdata(pdev);
> +
> + of_dma_controller_free(pdev->dev.of_node);
> + dma_async_device_unregister(&mdev->ddev);
> + clk_disable_unprepare(mdev->clk);
at this point your irq is registered and can be fired, the tasklets are
not killed :(
^ permalink raw reply
* [v2,2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Vinod Koul @ 2018-10-05 15:01 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: dan.j.williams, afaerber, robh+dt, gregkh, jslaby, linux-serial,
dmaengine, liuwei, 96boards, devicetree, daniel.thompson,
amit.kucheria, linux-arm-kernel, linux-kernel, hzhang, bdong,
manivannanece23, thomas.liau, jeff.chen, pn, edgar.righi
On 29-09-18, 13:16, Manivannan Sadhasivam wrote:
> Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC. The slave
> mode supports bus width of 4 bytes common for all peripherals and 1 byte
> specific for UART.
>
> The cyclic mode supports only block mode transfer.
Applied after adding driver tag, thanks
^ permalink raw reply
* [v3,2/2] dmaengine: uniphier-mdmac: add UniPhier MIO DMAC driver
From: Masahiro Yamada @ 2018-10-05 19:11 UTC (permalink / raw)
To: Vinod Koul
Cc: open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Masami Hiramatsu,
Jassi Brar, Linux Kernel Mailing List, Dan Williams,
linux-arm-kernel
Hi Vinod,
Thanks for looking at this closely.
On Fri, Oct 5, 2018 at 11:57 PM Vinod <vkoul@kernel.org> wrote:
>
> On 13-09-18, 09:51, Masahiro Yamada wrote:
>
> > +#define UNIPHIER_MDMAC_CH_IRQ_STAT 0x010 // current hw status (RO)
> > +#define UNIPHIER_MDMAC_CH_IRQ_REQ 0x014 // latched STAT (WOC)
> > +#define UNIPHIER_MDMAC_CH_IRQ_EN 0x018 // IRQ enable mask
> > +#define UNIPHIER_MDMAC_CH_IRQ_DET 0x01c // REQ & EN (RO)
> > +#define UNIPHIER_MDMAC_CH_IRQ__ABORT BIT(13)
> > +#define UNIPHIER_MDMAC_CH_IRQ__DONE BIT(1)
>
> why notation if UNIPHIER_MDMAC_CH_IRQ__FOO ?
Macros without double-underscore are just register offsets.
Macros with double-underscore are bit flags in
the corresponding register.
I often use this notation,
and I also see somebody else did so.
For example, see
drivers/mtd/nand/raw/denali.h
Please let me know if you do not like it.
I can adjust myself to your preference.
> > +#define UNIPHIER_MDMAC_CH_SRC_MODE 0x020 // mode of source
> > +#define UNIPHIER_MDMAC_CH_DEST_MODE 0x024 // mode of destination
> > +#define UNIPHIER_MDMAC_CH_MODE__ADDR_INC (0 << 4)
> > +#define UNIPHIER_MDMAC_CH_MODE__ADDR_DEC (1 << 4)
> > +#define UNIPHIER_MDMAC_CH_MODE__ADDR_FIXED (2 << 4)
> > +#define UNIPHIER_MDMAC_CH_SRC_ADDR 0x028 // source address
> > +#define UNIPHIER_MDMAC_CH_DEST_ADDR 0x02c // destination address
> > +#define UNIPHIER_MDMAC_CH_SIZE 0x030 // transfer bytes
>
> Please use /* comment */ style for these
I just thought people are getting tolerant of C++ comments.
Linus is so:
https://lkml.org/lkml/2017/11/25/133
However, C++ is not officially allowed in the
Linux coding style.
Will fix (without odd closing */ alignment)
> > +/* mc->vc.lock must be held by caller */
> > +static struct uniphier_mdmac_desc *__uniphier_mdmac_next_desc(
> > + struct uniphier_mdmac_chan *mc)
>
> this can be made to look better by:
>
> static struct uniphier_mdmac_desc *
> __uniphier_mdmac_next_desc(struct uniphier_mdmac_chan *mc)
OK.
This is not mentioned in the coding style doc at least,
but common enough.
Will fix.
> Btw why leading __ for function name here and other places?
Just a reminder of "mc->vc.lock must be held by caller".
It is common to use double-underscore prefixing
for functions that should be used with care.
However, I am happy to adjust myself to the maintainer.
Please let me know if you do not like it, then I will remove them out.
> > +{
> > + struct virt_dma_desc *vd;
> > +
> > + vd = vchan_next_desc(&mc->vc);
> > + if (!vd) {
> > + mc->md = NULL;
> > + return NULL;
> > + }
> > +
> > + list_del(&vd->node);
> > +
> > + mc->md = to_uniphier_mdmac_desc(vd);
> > +
> > + return mc->md;
> > +}
> > +
> > +/* mc->vc.lock must be held by caller */
> > +static void __uniphier_mdmac_handle(struct uniphier_mdmac_chan *mc,
> > + struct uniphier_mdmac_desc *md)
>
> please align this to previous line opening brace (hint checkpatch with
> --strict option will give you the warning)
This is already aligned.
Perhaps, due to your mailer.
> > +static irqreturn_t uniphier_mdmac_interrupt(int irq, void *dev_id)
> > +{
> > + struct uniphier_mdmac_chan *mc = dev_id;
> > + struct uniphier_mdmac_desc *md;
> > + irqreturn_t ret = IRQ_HANDLED;
> > + u32 irq_stat;
> > +
> > + spin_lock(&mc->vc.lock);
> > +
> > + irq_stat = readl(mc->reg_ch_base + UNIPHIER_MDMAC_CH_IRQ_DET);
> > +
> > + /*
> > + * Some channels share a single interrupt line. If the IRQ status is 0,
> > + * this is probably triggered by a different channel.
> > + */
> > + if (!irq_stat) {
> > + ret = IRQ_NONE;
> > + goto out;
> > + }
> > +
> > + /* write 1 to clear */
> > + writel(irq_stat, mc->reg_ch_base + UNIPHIER_MDMAC_CH_IRQ_REQ);
> > +
> > + /*
> > + * UNIPHIER_MDMAC_CH_IRQ__DONE interrupt is asserted even when the DMA
> > + * is aborted. To distinguish the normal completion and the abort,
> ^^^^
> double space..
OK, will fix.
> > +static int uniphier_mdmac_config(struct dma_chan *chan,
> > + struct dma_slave_config *config)
> > +{
> > + /* Nothing in struct dma_slave_config is configurable. */
> > + return 0;
> > +}
>
> I dont think config callback is mandatory, so we can drop this
Will remove.
> > +static enum dma_status uniphier_mdmac_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct virt_dma_chan *vc;
> > + struct virt_dma_desc *vd;
> > + struct uniphier_mdmac_chan *mc;
> > + struct uniphier_mdmac_desc *md = NULL;
> > + enum dma_status stat;
> > + unsigned long flags;
> > +
> > + stat = dma_cookie_status(chan, cookie, txstate);
> > + /* Return immediately if we do not need to compute the residue. */
> > + if (stat == DMA_COMPLETE || !txstate)
> > + return stat;
> > +
> > + vc = to_virt_chan(chan);
> > +
> > + spin_lock_irqsave(&vc->lock, flags);
> > +
> > + mc = to_uniphier_mdmac_chan(vc);
> > +
> > + if (mc->md && mc->md->vd.tx.cookie == cookie)
> > + md = mc->md;
> > +
> > + if (!md) {
> > + vd = vchan_find_desc(vc, cookie);
> > + if (vd)
> > + md = to_uniphier_mdmac_desc(vd);
> > + }
>
> in both of these cases you are calling __uniphier_mdmac_get_residue()
> which reads the register and updates. But I think you should read
> register only in the first case when descriptor is submitted and not in
> latter case when descriptor is queued
Good catch!
Will fix.
> > +static int uniphier_mdmac_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct uniphier_mdmac_device *mdev;
> > + struct dma_device *ddev;
> > + struct resource *res;
> > + int nr_chans, ret, i;
> > +
> > + nr_chans = platform_irq_count(pdev);
> > + if (nr_chans < 0)
> > + return nr_chans;
> > +
> > + ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> > + if (ret)
> > + return ret;
> > +
> > + mdev = devm_kzalloc(dev, struct_size(mdev, channels, nr_chans),
> > + GFP_KERNEL);
> > + if (!mdev)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + mdev->reg_base = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(mdev->reg_base))
> > + return PTR_ERR(mdev->reg_base);
> > +
> > + mdev->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(mdev->clk)) {
> > + dev_err(dev, "failed to get clock\n");
> > + return PTR_ERR(mdev->clk);
> > + }
> > +
> > + ret = clk_prepare_enable(mdev->clk);
> > + if (ret)
> > + return ret;
> > +
> > + ddev = &mdev->ddev;
> > + ddev->dev = dev;
> > + dma_cap_set(DMA_PRIVATE, ddev->cap_mask);
> > + ddev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
> > + ddev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
>
> undefined?
Precisely, I do not know the *_addr_widths.
As far as I read dmaengine/provider.rst
this represents the data bytes that are read/written at a time.
Really I do not know (care about) the transfer width.
As I commented in v2, the connection of the device side is hard-wired.
The transfer width cannot be observed from SW view.
What should I do?
> > +static int uniphier_mdmac_remove(struct platform_device *pdev)
> > +{
> > + struct uniphier_mdmac_device *mdev = platform_get_drvdata(pdev);
> > +
> > + of_dma_controller_free(pdev->dev.of_node);
> > + dma_async_device_unregister(&mdev->ddev);
> > + clk_disable_unprepare(mdev->clk);
>
> at this point your irq is registered and can be fired, the tasklets are
> not killed :(
Please let me clarify the concerns here.
Before the .remove hook is called, all the consumers should
have already put the dma channels.
So, no new descriptor is coming in.
However,
Some already-issued descriptors might be remaining, and being processed.
[1] This DMA engine might be still running
when clk_disable_unprepare() is being called.
The register access with its clock disabled
would cause the system crash.
[2] vchan_cookie_complete() might being called at this point
and schedule the tasklet.
It might call uniphier_mdmac_desc_free() after
the reference disapperrs.
Is this correct?
Do you have recommendation
for module removal guideline?
---
Best Regards
Masahiro Yamada
^ permalink raw reply
* [RFC,2/2] dmaengine: xilinx_dma: Add Xilinx AXI MCDMA Engine driver support
From: Radhey Shyam Pandey @ 2018-10-06 7:09 UTC (permalink / raw)
To: Vinod
Cc: robh+dt@kernel.org, mark.rutland@arm.com, Michal Simek,
dan.j.williams@intel.com, Appana Durga Kedareswara Rao,
lars@metafoo.de, dmaengine@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Thanks for the review.
> On 31-07-18, 23:16, Radhey Shyam Pandey wrote:
> > struct xilinx_dma_config {
> > @@ -402,6 +470,7 @@ struct xilinx_dma_config {
> > int (*clk_init)(struct platform_device *pdev, struct clk **axi_clk,
> > struct clk **tx_clk, struct clk **txs_clk,
> > struct clk **rx_clk, struct clk **rxs_clk);
> > + irqreturn_t (*irq_handler)(int irq, void *data);
>
> this sounds like a preparatory change?
Yes, I will split it in a separate patch.
>
> > + } else if (chan->xdev->dma_config->dmatype ==
> XDMA_TYPE_AXIMCDMA) {
> > + /* Allocate the buffer descriptors. */
> > + chan->seg_mv = dma_zalloc_coherent(chan->dev,
> > + sizeof(*chan->seg_mv) *
> > + XILINX_DMA_NUM_DESCS,
> > + &chan->seg_p,
> GFP_KERNEL);
> > + if (!chan->seg_mv) {
> > + dev_err(chan->dev,
> > + "unable to allocate channel %d descriptors\n",
> > + chan->id);
> > + return -ENOMEM;
> > + }
> > + for (i = 0; i < XILINX_DMA_NUM_DESCS; i++) {
> > + chan->seg_mv[i].hw.next_desc =
> > + lower_32_bits(chan->seg_p + sizeof(*chan->seg_mv) *
> > + ((i + 1) % XILINX_DMA_NUM_DESCS));
> > + chan->seg_mv[i].hw.next_desc_msb =
> > + upper_32_bits(chan->seg_p + sizeof(*chan->seg_mv) *
> > + ((i + 1) % XILINX_DMA_NUM_DESCS));
> > + chan->seg_mv[i].phys = chan->seg_p +
> > + sizeof(*chan->seg_v) * i;
> > + list_add_tail(&chan->seg_mv[i].node,
> > + &chan->free_seg_list);
> > + }
>
> only change with this and previous one seems to be use of seg_mv instead
> of seg_v right? if so, can you try to modularise this..
Agree. I will modularise it.
>
> > /**
> > + * xilinx_mcdma_start_transfer - Starts MCDMA transfer
> > + * @chan: Driver specific channel struct pointer
> > + */
> > +static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan)
> > +{
> > + struct xilinx_dma_tx_descriptor *head_desc, *tail_desc;
> > + struct xilinx_axidma_tx_segment *tail_segment;
> > + u32 reg;
> > +
> > + if (chan->err)
> > + return;
> > +
> > + if (!chan->idle)
> > + return;
> > +
> > + if (list_empty(&chan->pending_list))
> > + return;
>
> okay i was thinking that we need lock here, but then this is called with
> lock held, worth mentioning in the comment though..
Ok. Will add.
>
> > +static irqreturn_t xilinx_mcdma_irq_handler(int irq, void *data)
> > +{
> > + struct xilinx_dma_chan *chan = data;
> > + u32 status, ser_offset, chan_sermask, chan_offset = 0, chan_id;
> > +
> > + if (chan->direction == DMA_DEV_TO_MEM)
> > + ser_offset = XILINX_MCDMA_RXINT_SER_OFFSET;
> > + else
> > + ser_offset = XILINX_MCDMA_TXINT_SER_OFFSET;
> > +
> > + /* Read the channel id raising the interrupt*/
> > + chan_sermask = dma_ctrl_read(chan, ser_offset);
> > + chan_id = ffs(chan_sermask);
> > +
> > + if (!chan_id)
> > + return IRQ_NONE;
> > +
> > + if (chan->direction == DMA_DEV_TO_MEM)
> > + chan_offset = XILINX_DMA_MAX_CHANS_PER_DEVICE / 2;
> > +
> > + chan_offset = chan_offset + (chan_id - 1);
> > + chan = chan->xdev->chan[chan_offset];
> > + /* Read the status and ack the interrupts. */
> > + status = dma_ctrl_read(chan,
> XILINX_MCDMA_CHAN_SR_OFFSET(chan->tdest));
> > + if (!(status & XILINX_MCDMA_IRQ_ALL_MASK))
> > + return IRQ_NONE;
> > +
> > + dma_ctrl_write(chan, XILINX_MCDMA_CHAN_SR_OFFSET(chan-
> >tdest),
> > + status & XILINX_MCDMA_IRQ_ALL_MASK);
> > +
> > + if (status & XILINX_MCDMA_IRQ_ERR_MASK) {
> > + dev_err(chan->dev, "Channel %p has errors %x cdr %x tdr
> %x\n",
> > + chan, dma_ctrl_read(chan,
> > + XILINX_MCDMA_CH_ERR_OFFSET),
> dma_ctrl_read(chan,
> > + XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest)),
> > + dma_ctrl_read(chan,
> > + XILINX_MCDMA_CHAN_TDESC_OFFSET
> > + (chan->tdest)));
>
> this looks very hard to read, please start each dma_ctrl_read() from a
> new line to make it better
Ok . will change.
>
> > + chan->err = true;
> > + }
> > +
> > + if (status & XILINX_MCDMA_IRQ_DELAY_MASK) {
> > + /*
> > + * Device takes too long to do the transfer when user requires
> > + * responsiveness.
> > + */
> > + dev_dbg(chan->dev, "Inter-packet latency too long\n");
>
> so we just log it..?
For now yes. It will be used later when MCDMA is used by the network client.
>
> > + }
> > +
> > + if (status & XILINX_MCDMA_IRQ_IOC_MASK) {
> > + spin_lock(&chan->lock);
> > + xilinx_dma_complete_descriptor(chan);
> > + chan->idle = true;
> > + chan->start_transfer(chan);
> > + spin_unlock(&chan->lock);
> > + }
> > +
> > + tasklet_schedule(&chan->tasklet);
> > + return IRQ_HANDLED;
> > +
>
> bogus empty line...
Will fix.
>
> > +static struct dma_async_tx_descriptor *xilinx_mcdma_prep_slave_sg(
> > + struct dma_chan *dchan, struct scatterlist *sgl, unsigned int sg_len,
> > + enum dma_transfer_direction direction, unsigned long flags,
> > + void *context)
>
> indent is pretty bad here too :(
Will fix.
>
> > +{
> > + struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
> > + struct xilinx_dma_tx_descriptor *desc;
> > + struct xilinx_aximcdma_tx_segment *segment = NULL;
> > + u32 *app_w = (u32 *)context;
> > + struct scatterlist *sg;
> > + size_t copy;
> > + size_t sg_used;
> > + unsigned int i;
> > +
> > + if (!is_slave_direction(direction))
> > + return NULL;
> > +
> > + /* Allocate a transaction descriptor. */
> > + desc = xilinx_dma_alloc_tx_descriptor(chan);
> > + if (!desc)
> > + return NULL;
> > +
> > + dma_async_tx_descriptor_init(&desc->async_tx, &chan->common);
> > + desc->async_tx.tx_submit = xilinx_dma_tx_submit;
> > +
> > + /* Build transactions using information in the scatter gather list */
> > + for_each_sg(sgl, sg, sg_len, i) {
> > + sg_used = 0;
> > +
> > + /* Loop until the entire scatterlist entry is used */
> > + while (sg_used < sg_dma_len(sg)) {
> > + struct xilinx_aximcdma_desc_hw *hw;
> > +
> > + /* Get a free segment */
> > + segment = xilinx_aximcdma_alloc_tx_segment(chan);
> > + if (!segment)
> > + goto error;
> > +
> > + /*
> > + * Calculate the maximum number of bytes to transfer,
> > + * making sure it is less than the hw limit
> > + */
> > + copy = min_t(size_t, sg_dma_len(sg) - sg_used,
> > + XILINX_DMA_MAX_TRANS_LEN);
> > + hw = &segment->hw;
> > +
> > + /* Fill in the descriptor */
> > + xilinx_aximcdma_buf(chan, hw, sg_dma_address(sg),
> > + sg_used);
> > + hw->control = copy;
> > +
> > + if (chan->direction == DMA_MEM_TO_DEV) {
> > + if (app_w)
>
> why not make condition as: chan->direction == DMA_MEM_TO_DEV &&
> app_w
Ok . will change.
>
> > + memcpy(hw->app, app_w, sizeof(u32)
> *
> > + XILINX_DMA_NUM_APP_WORDS);
> > + }
> > +
> > + sg_used += copy;
> > + /*
> > + * Insert the segment into the descriptor segments
> > + * list.
> > + */
> > + list_add_tail(&segment->node, &desc->segments);
> > + }
> > + }
> > +
> > + segment = list_first_entry(&desc->segments,
> > + struct xilinx_aximcdma_tx_segment, node);
> > + desc->async_tx.phys = segment->phys;
> > +
> > + /* For the last DMA_MEM_TO_DEV transfer, set EOP */
> > + if (chan->direction == DMA_MEM_TO_DEV) {
> > + segment->hw.control |= XILINX_MCDMA_BD_SOP;
> > + segment = list_last_entry(&desc->segments,
> > + struct xilinx_aximcdma_tx_segment,
> > + node);
> > + segment->hw.control |= XILINX_MCDMA_BD_EOP;
> > + }
> > +
> > + return &desc->async_tx;
> > +
> > +error:
> > + xilinx_dma_free_tx_descriptor(chan, desc);
>
> will it free the ones allocated here or all descriptors?
It will free single descriptor and all its segments.
>
> > /**
> > * xilinx_dma_prep_slave_sg - prepare descriptors for a DMA_SLAVE
> transaction
> > @@ -2422,12 +2827,16 @@ static int xilinx_dma_chan_probe(struct
> xilinx_dma_device *xdev,
> >
> > if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel") ||
> > of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel") ||
> > - of_device_is_compatible(node, "xlnx,axi-cdma-channel")) {
> > + of_device_is_compatible(node, "xlnx,axi-cdma-channel") ||
> > + of_device_is_compatible(node, "xlnx,axi-mcdma-mm2s-channel")) {
>
> this is not scaling, maybe you should use data with each
> compatible to check for specific things..
Yes, we can reuse axidma s2mm and mm2 channel nodes. MCDMA stuff can
be selectively programmed based on xilinx_dma_config.dmatype.
>
> > +static const struct xilinx_dma_config aximcdma_config = {
> > + .dmatype = XDMA_TYPE_AXIMCDMA,
> > + .clk_init = axidma_clk_init,
> > + .irq_handler = xilinx_mcdma_irq_handler,
> > +};
> > static const struct xilinx_dma_config axicdma_config = {
> > .dmatype = XDMA_TYPE_CDMA,
> > .clk_init = axicdma_clk_init,
> > + .irq_handler = xilinx_dma_irq_handler,
>
> this should be in preparatory patch
Yes.
> --
> ~Vinod
^ permalink raw reply
* [v3,2/2] dmaengine: uniphier-mdmac: add UniPhier MIO DMAC driver
From: Vinod Koul @ 2018-10-06 16:22 UTC (permalink / raw)
To: Masahiro Yamada
Cc: open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Masami Hiramatsu,
Jassi Brar, Linux Kernel Mailing List, Dan Williams,
linux-arm-kernel
On 06-10-18, 04:11, Masahiro Yamada wrote:
> On Fri, Oct 5, 2018 at 11:57 PM Vinod <vkoul@kernel.org> wrote:
> >
> > On 13-09-18, 09:51, Masahiro Yamada wrote:
> >
> > > +#define UNIPHIER_MDMAC_CH_IRQ_STAT 0x010 // current hw status (RO)
> > > +#define UNIPHIER_MDMAC_CH_IRQ_REQ 0x014 // latched STAT (WOC)
> > > +#define UNIPHIER_MDMAC_CH_IRQ_EN 0x018 // IRQ enable mask
> > > +#define UNIPHIER_MDMAC_CH_IRQ_DET 0x01c // REQ & EN (RO)
> > > +#define UNIPHIER_MDMAC_CH_IRQ__ABORT BIT(13)
> > > +#define UNIPHIER_MDMAC_CH_IRQ__DONE BIT(1)
> >
> > why notation if UNIPHIER_MDMAC_CH_IRQ__FOO ?
>
>
> Macros without double-underscore are just register offsets.
>
> Macros with double-underscore are bit flags in
> the corresponding register.
>
>
> I often use this notation,
> and I also see somebody else did so.
>
> For example, see
> drivers/mtd/nand/raw/denali.h
>
> Please let me know if you do not like it.
> I can adjust myself to your preference.
Hmm I dont have a strong preference either way, though might be
worthwhile to document this style so that future updates can be
consistent
>
>
>
> > > +#define UNIPHIER_MDMAC_CH_SRC_MODE 0x020 // mode of source
> > > +#define UNIPHIER_MDMAC_CH_DEST_MODE 0x024 // mode of destination
> > > +#define UNIPHIER_MDMAC_CH_MODE__ADDR_INC (0 << 4)
> > > +#define UNIPHIER_MDMAC_CH_MODE__ADDR_DEC (1 << 4)
> > > +#define UNIPHIER_MDMAC_CH_MODE__ADDR_FIXED (2 << 4)
> > > +#define UNIPHIER_MDMAC_CH_SRC_ADDR 0x028 // source address
> > > +#define UNIPHIER_MDMAC_CH_DEST_ADDR 0x02c // destination address
> > > +#define UNIPHIER_MDMAC_CH_SIZE 0x030 // transfer bytes
> >
> > Please use /* comment */ style for these
>
> I just thought people are getting tolerant of C++ comments.
>
> Linus is so:
> https://lkml.org/lkml/2017/11/25/133
>
> However, C++ is not officially allowed in the
> Linux coding style.
>
> Will fix (without odd closing */ alignment)
Lets be conistent with the subsystem and use one style unless we decide
to move..
> > > +/* mc->vc.lock must be held by caller */
> > > +static struct uniphier_mdmac_desc *__uniphier_mdmac_next_desc(
> > > + struct uniphier_mdmac_chan *mc)
> >
> > this can be made to look better by:
> >
> > static struct uniphier_mdmac_desc *
> > __uniphier_mdmac_next_desc(struct uniphier_mdmac_chan *mc)
>
> OK.
> This is not mentioned in the coding style doc at least,
> but common enough.
> Will fix.
Coding style tells you guideline, it is upto you to make code look and
read better :)
> > Btw why leading __ for function name here and other places?
>
> Just a reminder of "mc->vc.lock must be held by caller".
A comment is just fine..
> It is common to use double-underscore prefixing
> for functions that should be used with care.
>
> However, I am happy to adjust myself to the maintainer.
> Please let me know if you do not like it, then I will remove them out.
I would like these to be removed
> > > +{
> > > + struct virt_dma_desc *vd;
> > > +
> > > + vd = vchan_next_desc(&mc->vc);
> > > + if (!vd) {
> > > + mc->md = NULL;
> > > + return NULL;
> > > + }
> > > +
> > > + list_del(&vd->node);
> > > +
> > > + mc->md = to_uniphier_mdmac_desc(vd);
> > > +
> > > + return mc->md;
> > > +}
> > > +
> > > +/* mc->vc.lock must be held by caller */
> > > +static void __uniphier_mdmac_handle(struct uniphier_mdmac_chan *mc,
> > > + struct uniphier_mdmac_desc *md)
> >
> > please align this to previous line opening brace (hint checkpatch with
> > --strict option will give you the warning)
>
> This is already aligned.
> Perhaps, due to your mailer.
As I said please check checkpatch --strict
> > > +static int uniphier_mdmac_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct uniphier_mdmac_device *mdev;
> > > + struct dma_device *ddev;
> > > + struct resource *res;
> > > + int nr_chans, ret, i;
> > > +
> > > + nr_chans = platform_irq_count(pdev);
> > > + if (nr_chans < 0)
> > > + return nr_chans;
> > > +
> > > + ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mdev = devm_kzalloc(dev, struct_size(mdev, channels, nr_chans),
> > > + GFP_KERNEL);
> > > + if (!mdev)
> > > + return -ENOMEM;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + mdev->reg_base = devm_ioremap_resource(dev, res);
> > > + if (IS_ERR(mdev->reg_base))
> > > + return PTR_ERR(mdev->reg_base);
> > > +
> > > + mdev->clk = devm_clk_get(dev, NULL);
> > > + if (IS_ERR(mdev->clk)) {
> > > + dev_err(dev, "failed to get clock\n");
> > > + return PTR_ERR(mdev->clk);
> > > + }
> > > +
> > > + ret = clk_prepare_enable(mdev->clk);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ddev = &mdev->ddev;
> > > + ddev->dev = dev;
> > > + dma_cap_set(DMA_PRIVATE, ddev->cap_mask);
> > > + ddev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
> > > + ddev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
> >
> > undefined?
>
> Precisely, I do not know the *_addr_widths.
This is "your" controller, you know the capability!
>
> As far as I read dmaengine/provider.rst
> this represents the data bytes that are read/written at a time.
>
> Really I do not know (care about) the transfer width.
>
> As I commented in v2, the connection of the device side is hard-wired.
> The transfer width cannot be observed from SW view.
>
> What should I do?
Add the widths that are supported by the controller
> > > +static int uniphier_mdmac_remove(struct platform_device *pdev)
> > > +{
> > > + struct uniphier_mdmac_device *mdev = platform_get_drvdata(pdev);
> > > +
> > > + of_dma_controller_free(pdev->dev.of_node);
> > > + dma_async_device_unregister(&mdev->ddev);
> > > + clk_disable_unprepare(mdev->clk);
> >
> > at this point your irq is registered and can be fired, the tasklets are
> > not killed :(
>
>
> Please let me clarify the concerns here.
>
> Before the .remove hook is called, all the consumers should
> have already put the dma channels.
> So, no new descriptor is coming in.
>
> However,
>
> Some already-issued descriptors might be remaining, and being processed.
>
> [1] This DMA engine might be still running
> when clk_disable_unprepare() is being called.
> The register access with its clock disabled
> would cause the system crash.
Yes and dmaengine may fire a spurious irq..
>
> [2] vchan_cookie_complete() might being called at this point
> and schedule the tasklet.
> It might call uniphier_mdmac_desc_free() after
> the reference disapperrs.
>
> Is this correct?
Correct :)
> Do you have recommendation
> for module removal guideline?
Yes please free up or disable irq explictly, ensure pending irqs have
completed and then ensure all the tasklets are killed and in this order
for obvious reasons
^ permalink raw reply
* [v2] dmaengine: fsl-edma: remove dma_slave_config direction usage
From: Vinod Koul @ 2018-10-07 14:14 UTC (permalink / raw)
To: dmaengine; +Cc: Vinod Koul, Angelo Dureghello, Krzysztof Kozlowski
dma_slave_config direction was marked as deprecated quite some
time back, remove the usage from this driver so that the field
can be removed
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
CC: Angelo Dureghello <angelo@sysam.it>
CC: Krzysztof Kozlowski <krzk@kernel.org>
Angelo, Krzysztof,
I have rebased this against the latest fsl-edma changes, can you
please verify this and let me know, thnx
drivers/dma/fsl-edma-common.c | 74 +++++++++++++++++++++++++------------------
drivers/dma/fsl-edma-common.h | 12 ++-----
include/linux/dmaengine.h | 1 -
3 files changed, 46 insertions(+), 41 deletions(-)
diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
index 8ba80f4b6f55..8876c4c1bb2c 100644
--- a/drivers/dma/fsl-edma-common.c
+++ b/drivers/dma/fsl-edma-common.c
@@ -178,19 +178,7 @@ int fsl_edma_slave_config(struct dma_chan *chan,
{
struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
- fsl_chan->fsc.dir = cfg->direction;
- if (cfg->direction == DMA_DEV_TO_MEM) {
- fsl_chan->fsc.dev_addr = cfg->src_addr;
- fsl_chan->fsc.addr_width = cfg->src_addr_width;
- fsl_chan->fsc.burst = cfg->src_maxburst;
- fsl_chan->fsc.attr = fsl_edma_get_tcd_attr(cfg->src_addr_width);
- } else if (cfg->direction == DMA_MEM_TO_DEV) {
- fsl_chan->fsc.dev_addr = cfg->dst_addr;
- fsl_chan->fsc.addr_width = cfg->dst_addr_width;
- fsl_chan->fsc.burst = cfg->dst_maxburst;
- fsl_chan->fsc.attr = fsl_edma_get_tcd_attr(cfg->dst_addr_width);
- } else
- return -EINVAL;
+ memcpy(&fsl_chan->cfg, cfg, sizeof(*cfg));
return 0;
}
@@ -202,7 +190,7 @@ static size_t fsl_edma_desc_residue(struct fsl_edma_chan *fsl_chan,
struct fsl_edma_desc *edesc = fsl_chan->edesc;
struct edma_regs *regs = &fsl_chan->edma->regs;
u32 ch = fsl_chan->vchan.chan.chan_id;
- enum dma_transfer_direction dir = fsl_chan->fsc.dir;
+ enum dma_transfer_direction dir = edesc->dirn;
dma_addr_t cur_addr, dma_addr;
size_t len, size;
int i;
@@ -387,7 +375,7 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
u32 src_addr, dst_addr, last_sg, nbytes;
u16 soff, doff, iter;
- if (!is_slave_direction(fsl_chan->fsc.dir))
+ if (!is_slave_direction(direction))
return NULL;
sg_len = buf_len / period_len;
@@ -395,9 +383,21 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
if (!fsl_desc)
return NULL;
fsl_desc->iscyclic = true;
+ fsl_desc->dirn = direction;
dma_buf_next = dma_addr;
- nbytes = fsl_chan->fsc.addr_width * fsl_chan->fsc.burst;
+ if (direction == DMA_MEM_TO_DEV) {
+ fsl_chan->attr =
+ fsl_edma_get_tcd_attr(fsl_chan->cfg.dst_addr_width);
+ nbytes = fsl_chan->cfg.dst_addr_width *
+ fsl_chan->cfg.dst_maxburst;
+ } else {
+ fsl_chan->attr =
+ fsl_edma_get_tcd_attr(fsl_chan->cfg.src_addr_width);
+ nbytes = fsl_chan->cfg.src_addr_width *
+ fsl_chan->cfg.src_maxburst;
+ }
+
iter = period_len / nbytes;
for (i = 0; i < sg_len; i++) {
@@ -407,20 +407,20 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
/* get next sg's physical address */
last_sg = fsl_desc->tcd[(i + 1) % sg_len].ptcd;
- if (fsl_chan->fsc.dir == DMA_MEM_TO_DEV) {
+ if (direction == DMA_MEM_TO_DEV) {
src_addr = dma_buf_next;
- dst_addr = fsl_chan->fsc.dev_addr;
- soff = fsl_chan->fsc.addr_width;
+ dst_addr = fsl_chan->cfg.dst_addr;
+ soff = fsl_chan->cfg.dst_addr_width;
doff = 0;
} else {
- src_addr = fsl_chan->fsc.dev_addr;
+ src_addr = fsl_chan->cfg.src_addr;
dst_addr = dma_buf_next;
soff = 0;
- doff = fsl_chan->fsc.addr_width;
+ doff = fsl_chan->cfg.src_addr_width;
}
fsl_edma_fill_tcd(fsl_desc->tcd[i].vtcd, src_addr, dst_addr,
- fsl_chan->fsc.attr, soff, nbytes, 0, iter,
+ fsl_chan->attr, soff, nbytes, 0, iter,
iter, doff, last_sg, true, false, true);
dma_buf_next += period_len;
}
@@ -441,42 +441,54 @@ struct dma_async_tx_descriptor *fsl_edma_prep_slave_sg(
u16 soff, doff, iter;
int i;
- if (!is_slave_direction(fsl_chan->fsc.dir))
+ if (!is_slave_direction(direction))
return NULL;
fsl_desc = fsl_edma_alloc_desc(fsl_chan, sg_len);
if (!fsl_desc)
return NULL;
fsl_desc->iscyclic = false;
+ fsl_desc->dirn = direction;
+
+ if (direction == DMA_MEM_TO_DEV) {
+ fsl_chan->attr =
+ fsl_edma_get_tcd_attr(fsl_chan->cfg.dst_addr_width);
+ nbytes = fsl_chan->cfg.dst_addr_width *
+ fsl_chan->cfg.dst_maxburst;
+ } else {
+ fsl_chan->attr =
+ fsl_edma_get_tcd_attr(fsl_chan->cfg.src_addr_width);
+ nbytes = fsl_chan->cfg.src_addr_width *
+ fsl_chan->cfg.src_maxburst;
+ }
- nbytes = fsl_chan->fsc.addr_width * fsl_chan->fsc.burst;
for_each_sg(sgl, sg, sg_len, i) {
/* get next sg's physical address */
last_sg = fsl_desc->tcd[(i + 1) % sg_len].ptcd;
- if (fsl_chan->fsc.dir == DMA_MEM_TO_DEV) {
+ if (direction == DMA_MEM_TO_DEV) {
src_addr = sg_dma_address(sg);
- dst_addr = fsl_chan->fsc.dev_addr;
- soff = fsl_chan->fsc.addr_width;
+ dst_addr = fsl_chan->cfg.dst_addr;
+ soff = fsl_chan->cfg.dst_addr_width;
doff = 0;
} else {
- src_addr = fsl_chan->fsc.dev_addr;
+ src_addr = fsl_chan->cfg.src_addr;
dst_addr = sg_dma_address(sg);
soff = 0;
- doff = fsl_chan->fsc.addr_width;
+ doff = fsl_chan->cfg.src_addr_width;
}
iter = sg_dma_len(sg) / nbytes;
if (i < sg_len - 1) {
last_sg = fsl_desc->tcd[(i + 1)].ptcd;
fsl_edma_fill_tcd(fsl_desc->tcd[i].vtcd, src_addr,
- dst_addr, fsl_chan->fsc.attr, soff,
+ dst_addr, fsl_chan->attr, soff,
nbytes, 0, iter, iter, doff, last_sg,
false, false, true);
} else {
last_sg = 0;
fsl_edma_fill_tcd(fsl_desc->tcd[i].vtcd, src_addr,
- dst_addr, fsl_chan->fsc.attr, soff,
+ dst_addr, fsl_chan->attr, soff,
nbytes, 0, iter, iter, doff, last_sg,
true, true, false);
}
diff --git a/drivers/dma/fsl-edma-common.h b/drivers/dma/fsl-edma-common.h
index a6f5b99ee95f..8917e8865959 100644
--- a/drivers/dma/fsl-edma-common.h
+++ b/drivers/dma/fsl-edma-common.h
@@ -109,14 +109,6 @@ struct fsl_edma_sw_tcd {
struct fsl_edma_hw_tcd *vtcd;
};
-struct fsl_edma_slave_config {
- enum dma_transfer_direction dir;
- enum dma_slave_buswidth addr_width;
- u32 dev_addr;
- u32 burst;
- u32 attr;
-};
-
struct fsl_edma_chan {
struct virt_dma_chan vchan;
enum dma_status status;
@@ -125,7 +117,8 @@ struct fsl_edma_chan {
u32 slave_id;
struct fsl_edma_engine *edma;
struct fsl_edma_desc *edesc;
- struct fsl_edma_slave_config fsc;
+ struct dma_slave_config cfg;
+ u32 attr;
struct dma_pool *tcd_pool;
};
@@ -133,6 +126,7 @@ struct fsl_edma_desc {
struct virt_dma_desc vdesc;
struct fsl_edma_chan *echan;
bool iscyclic;
+ enum dma_transfer_direction dirn;
unsigned int n_tcds;
struct fsl_edma_sw_tcd tcd[];
};
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index d49ec5c31944..f158eaae0ef6 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -365,7 +365,6 @@ enum dma_slave_buswidth {
* data, then prefer to do that.
*/
struct dma_slave_config {
- enum dma_transfer_direction direction;
phys_addr_t src_addr;
phys_addr_t dst_addr;
enum dma_slave_buswidth src_addr_width;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox