[v5,1/3] dmaengine: dw-axi-dmac: Support hardware quirks

Commit Message

Jia Jie Ho May 30, 2024, 3:11 a.m. UTC

Adds separate dma hardware descriptor setup for JH8100 hardware quirks.
JH8100 engine uses AXI1 master for data transfer but current dma driver
is hardcoded to use AXI0 only. The FIFO offset needs to be incremented due
to hardware limitations.

Signed-off-by: Jia Jie Ho <[e-mail]>
---
 .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    | 32 ++++++++++++++++---
 drivers/dma/dw-axi-dmac/dw-axi-dmac.h         |  2 ++
 include/linux/dma/dw_axi.h                    | 11 +++++++
 3 files changed, 40 insertions(+), 5 deletions(-)
 create mode 100644 include/linux/dma/dw_axi.h

Comments

Vinod Koul June 11, 2024, 5:50 p.m. UTC | #1

On 30-05-24, 11:11, Jia Jie Ho wrote:

> +
> +struct dw_axi_peripheral_config {
> +#define DWAXIDMAC_STARFIVE_SM_ALGO	BIT(0)

what does this quirk mean?

> +	u32 quirks;

Can you explain why you need this to be exposed. I would prefer we use
existing interfaces and not define a new one...

Jia Jie Ho June 12, 2024, 10:13 a.m. UTC | #2

> On 30-05-24, 11:11, Jia Jie Ho wrote:
> 
> > +
> > +struct dw_axi_peripheral_config {
> > +#define DWAXIDMAC_STARFIVE_SM_ALGO	BIT(0)
> 
> what does this quirk mean?
> 
> > +	u32 quirks;
> 
> Can you explain why you need this to be exposed. I would prefer we use
> existing interfaces and not define a new one...
> 

Hi Vinod,
Thanks for reviewing this.
This is a dedicated dma controller for the crypto engine.
I am adding this quirk to:
1. Select the src and dest AXI master for transfers between mem and dev. 
    Driver currently only uses AXI0 for both.
2. Workaround a hardware limitation on the crypto engine to
     transfer data > 256B by incrementing the peripheral FIFO offset.

What is the recommended way to handle such cases besides using 
peripheral_config in dma_slave_config?

Best regards,
Jia Jie

Vinod Koul June 28, 2024, 7:39 a.m. UTC | #3

Hi JiaJie

On 12-06-24, 10:13, JiaJie Ho wrote:
> > On 30-05-24, 11:11, Jia Jie Ho wrote:
> > 
> > > +
> > > +struct dw_axi_peripheral_config {
> > > +#define DWAXIDMAC_STARFIVE_SM_ALGO	BIT(0)
> > 
> > what does this quirk mean?
> > 
> > > +	u32 quirks;
> > 
> > Can you explain why you need this to be exposed. I would prefer we use
> > existing interfaces and not define a new one...
> > 
> 
> Hi Vinod,
> Thanks for reviewing this.
> This is a dedicated dma controller for the crypto engine.
> I am adding this quirk to:
> 1. Select the src and dest AXI master for transfers between mem and dev. 
>     Driver currently only uses AXI0 for both.

why cant this information be passed thru DT, that is typically done by
most controllers?

> 2. Workaround a hardware limitation on the crypto engine to
>      transfer data > 256B by incrementing the peripheral FIFO offset.

Dont set the transfer data > 256B, you should ideally use maxburst for
configuring the FIFO...

> 
> What is the recommended way to handle such cases besides using 
> peripheral_config in dma_slave_config?

First use dma_slave_config() fields, if they are not appropriate, lets
discuss why before we add custom methods or use peripheral_config

Patch

diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
index 18ce7d64958b..1a8f2e140848 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -648,6 +648,7 @@  static void set_desc_dest_master(struct axi_dma_hw_desc *hw_desc,
 
 static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 				  struct axi_dma_hw_desc *hw_desc,
+				  struct axi_dma_desc *desc,
 				  dma_addr_t mem_addr, size_t len)
 {
 	unsigned int data_width = BIT(chan->chip->dw->hdata->m_data_width);
@@ -656,6 +657,8 @@  static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 	dma_addr_t device_addr;
 	size_t axi_block_ts;
 	size_t block_ts;
+	bool hw_quirks = chan->quirks & DWAXIDMAC_STARFIVE_SM_ALGO;
+	u32 val;
 	u32 ctllo, ctlhi;
 	u32 burst_len, src_burst_trans_len, dst_burst_trans_len;
 
@@ -676,7 +679,8 @@  static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 		device_addr = chan->config.dst_addr;
 		ctllo = reg_width << CH_CTL_L_DST_WIDTH_POS |
 			mem_width << CH_CTL_L_SRC_WIDTH_POS |
-			DWAXIDMAC_CH_CTL_L_NOINC << CH_CTL_L_DST_INC_POS |
+			(hw_quirks ? DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_DST_INC_POS :
+				     DWAXIDMAC_CH_CTL_L_NOINC << CH_CTL_L_DST_INC_POS) |
 			DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_SRC_INC_POS;
 		block_ts = len >> mem_width;
 		break;
@@ -686,7 +690,8 @@  static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 		ctllo = reg_width << CH_CTL_L_SRC_WIDTH_POS |
 			mem_width << CH_CTL_L_DST_WIDTH_POS |
 			DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_DST_INC_POS |
-			DWAXIDMAC_CH_CTL_L_NOINC << CH_CTL_L_SRC_INC_POS;
+			(hw_quirks ? DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_SRC_INC_POS :
+				     DWAXIDMAC_CH_CTL_L_NOINC << CH_CTL_L_SRC_INC_POS);
 		block_ts = len >> reg_width;
 		break;
 	default:
@@ -739,6 +744,17 @@  static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 
 	set_desc_src_master(hw_desc);
 
+	if (hw_quirks) {
+		if (chan->direction == DMA_MEM_TO_DEV) {
+			set_desc_dest_master(hw_desc, desc);
+		} else {
+			/* Select AXI1 for src master */
+			val = le32_to_cpu(hw_desc->lli->ctl_lo);
+			val |= CH_CTL_L_SRC_MAST;
+			hw_desc->lli->ctl_lo = cpu_to_le32(val);
+		}
+	}
+
 	hw_desc->len = len;
 	return 0;
 }
@@ -815,8 +831,8 @@  dw_axi_dma_chan_prep_cyclic(struct dma_chan *dchan, dma_addr_t dma_addr,
 	for (i = 0; i < total_segments; i++) {
 		hw_desc = &desc->hw_desc[i];
 
-		status = dw_axi_dma_set_hw_desc(chan, hw_desc, src_addr,
-						segment_len);
+		status = dw_axi_dma_set_hw_desc(chan, hw_desc, NULL,
+						src_addr, segment_len);
 		if (status < 0)
 			goto err_desc_get;
 
@@ -898,7 +914,8 @@  dw_axi_dma_chan_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
 
 		do {
 			hw_desc = &desc->hw_desc[loop++];
-			status = dw_axi_dma_set_hw_desc(chan, hw_desc, mem, segment_len);
+			status = dw_axi_dma_set_hw_desc(chan, hw_desc, desc,
+							mem, segment_len);
 			if (status < 0)
 				goto err_desc_get;
 
@@ -1036,8 +1053,13 @@  static int dw_axi_dma_chan_slave_config(struct dma_chan *dchan,
 					struct dma_slave_config *config)
 {
 	struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
+	struct dw_axi_peripheral_config *periph = config->peripheral_config;
 
 	memcpy(&chan->config, config, sizeof(*config));
+	if (config->peripheral_size == sizeof(*periph))
+		chan->quirks = periph->quirks;
+	else
+		chan->quirks = 0;
 
 	return 0;
 }
diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
index d76d3d88ceb6..6a68c40c1bf3 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
@@ -14,6 +14,7 @@ 
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/dmaengine.h>
+#include <linux/dma/dw_axi.h>
 #include <linux/types.h>
 
 #include "../virt-dma.h"
@@ -50,6 +51,7 @@  struct axi_dma_chan {
 	struct dma_slave_config		config;
 	enum dma_transfer_direction	direction;
 	bool				cyclic;
+	u32				quirks;
 	/* these other elements are all protected by vc.lock */
 	bool				is_paused;
 };
diff --git a/include/linux/dma/dw_axi.h b/include/linux/dma/dw_axi.h
new file mode 100644
index 000000000000..fd49152869a4
--- /dev/null
+++ b/include/linux/dma/dw_axi.h
@@ -0,0 +1,11 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_DMA_DW_AXI_H
+#define __LINUX_DMA_DW_AXI_H
+
+#include <linux/types.h>
+
+struct dw_axi_peripheral_config {
+#define DWAXIDMAC_STARFIVE_SM_ALGO	BIT(0)
+	u32 quirks;
+};
+#endif /* __LINUX_DMA_DW_AXI_H */