[v2,6/9] arm: twr-k70f120m: extend Freescale eDMA driver with the ability to support Kinetis SoC
diff mbox

Message ID 1435667250-28299-7-git-send-email-pawelo@king.net.pl
State New
Headers show

Commit Message

Paul Osmialowski June 30, 2015, 12:27 p.m. UTC
Surprisingly small amount of work was required in order to extend already
existing eDMA driver with the support for Kinetis SoC architecture.

Signed-off-by: Paul Osmialowski <pawelo@king.net.pl>
---
 Documentation/devicetree/bindings/dma/fsl-edma.txt |  38 ++++++-
 drivers/dma/fsl-edma.c                             | 114 ++++++++++++++++-----
 2 files changed, 125 insertions(+), 27 deletions(-)

Comments

Vinod Koul July 5, 2015, 6:45 a.m. UTC | #1
On Tue, Jun 30, 2015 at 02:27:27PM +0200, Paul Osmialowski wrote:
The patch title is not per subsystem semantics, pls fix that

> Surprisingly small amount of work was required in order to extend already
> existing eDMA driver with the support for Kinetis SoC architecture.
And this doesn't tell me the stuff you added/removed/fixed in current driver
to extend to Kinetis. Our changelog should tell what changed and why

Btw is this patch dependent upon the rest of the series?

>  
>  static int
> -fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma)
> +fsl_edma_irq_init(struct platform_device *pdev,
> +		  struct fsl_edma_engine *fsl_edma)
please keep style changes in a separate patch

>  {
> +	struct device_node *np = pdev->dev.of_node;
> +	int irq, errirq;
>  	int ret;
>  
> -	fsl_edma->txirq = platform_get_irq_byname(pdev, "edma-tx");
> -	if (fsl_edma->txirq < 0) {
> -		dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
> -		return fsl_edma->txirq;
> -	}
> -
> -	fsl_edma->errirq = platform_get_irq_byname(pdev, "edma-err");
> -	if (fsl_edma->errirq < 0) {
> +	errirq = platform_get_irq_byname(pdev, "edma-err");
> +	if (errirq < 0) {
>  		dev_err(&pdev->dev, "Can't get edma-err irq.\n");
> -		return fsl_edma->errirq;
> +		return irq;
shouldn't this be errirq

>  	}
>  
> -	if (fsl_edma->txirq == fsl_edma->errirq) {
> -		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
> -				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
> -		if (ret) {
> -			dev_err(&pdev->dev, "Can't register eDMA IRQ.\n");
> -			 return  ret;
> +	if (fsl_edma->kinetis) {
> +		int i;
> +		int irqs = of_irq_count(np);
> +
> +		if (irqs <= 1) {
> +			dev_err(&pdev->dev, "Wrong eDMA irq count %d\n", irqs);
> +			return -EINVAL;
why not return irqs?

>  		}
> -	} else {
> -		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
> +
> +		for (i = 0; i < (irqs - 1); i++) {
> +			char irq_name[32];
> +
> +			sprintf(irq_name, "edma-tx-%d,%d", i, 16 + i);
> +			irq = platform_get_irq_byname(pdev, irq_name);
> +			if (irq < 0) {
> +				dev_err(&pdev->dev, "Can't get %s irq.\n",
> +							irq_name);
> +				return irq;
> +			}
> +
> +			ret = devm_request_irq(&pdev->dev, irq,
>  				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
> -		if (ret) {
> -			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
> -			return  ret;
> +			if (ret) {
> +				dev_err(&pdev->dev, "Can't register %s IRQ.\n",
> +							irq_name);
> +				return  ret;
> +			}
>  		}
>  
> -		ret = devm_request_irq(&pdev->dev, fsl_edma->errirq,
> -				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
> +		ret = devm_request_irq(&pdev->dev, errirq,
> +			fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
>  		if (ret) {
>  			dev_err(&pdev->dev, "Can't register eDMA err IRQ.\n");
> -			return  ret;
> +		return  ret;
> +		}
> +	} else {
> +		irq = platform_get_irq_byname(pdev, "edma-tx");
> +		if (irq < 0) {
> +			dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
> +			return irq;
> +		}
> +
> +		if (irq == errirq) {
> +			ret = devm_request_irq(&pdev->dev, irq,
> +				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +						"Can't register eDMA IRQ.\n");
> +				return  ret;
> +			}
> +		} else {
> +			ret = devm_request_irq(&pdev->dev, irq,
> +				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +					    "Can't register eDMA tx IRQ.\n");
> +				return  ret;
> +			}
> +
> +			ret = devm_request_irq(&pdev->dev, errirq,
> +				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +					    "Can't register eDMA err IRQ.\n");
> +				return  ret;
> +			}
>  		}
>  	}
Okay here we have a bunch of changes without explanation on why these are
required and what are they supposed to do. So it is kind of very difficult to
review!

So please split these up in sequence of patches where each patch does only
ONE thing with right changelog messages

Thanks
Paul Osmialowski July 5, 2015, 9:45 a.m. UTC | #2
Hi Vinod,

You're right, this should be separate patchset. Having patchset too big
leads to huge list of maintainers and lack of focusing on details 
(on-error return values are total mess now!).

I plan to split it into three: 1) initial support (so the proof that 
platform is living could be read only on JTAG), 2) DMA support and 3) UART 
support.

Also in my view extending this DMA driver requires acceptance of initial
platform support first - there's no point in extending this driver without
having support for a platform for which we are extending it.

Thanks,
Paul

On Sun, 5 Jul 2015, Vinod Koul wrote:

> On Tue, Jun 30, 2015 at 02:27:27PM +0200, Paul Osmialowski wrote:
> The patch title is not per subsystem semantics, pls fix that
>
>> Surprisingly small amount of work was required in order to extend already
>> existing eDMA driver with the support for Kinetis SoC architecture.
> And this doesn't tell me the stuff you added/removed/fixed in current driver
> to extend to Kinetis. Our changelog should tell what changed and why
>
> Btw is this patch dependent upon the rest of the series?
>
>>
>>  static int
>> -fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma)
>> +fsl_edma_irq_init(struct platform_device *pdev,
>> +		  struct fsl_edma_engine *fsl_edma)
> please keep style changes in a separate patch
>
>>  {
>> +	struct device_node *np = pdev->dev.of_node;
>> +	int irq, errirq;
>>  	int ret;
>>
>> -	fsl_edma->txirq = platform_get_irq_byname(pdev, "edma-tx");
>> -	if (fsl_edma->txirq < 0) {
>> -		dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
>> -		return fsl_edma->txirq;
>> -	}
>> -
>> -	fsl_edma->errirq = platform_get_irq_byname(pdev, "edma-err");
>> -	if (fsl_edma->errirq < 0) {
>> +	errirq = platform_get_irq_byname(pdev, "edma-err");
>> +	if (errirq < 0) {
>>  		dev_err(&pdev->dev, "Can't get edma-err irq.\n");
>> -		return fsl_edma->errirq;
>> +		return irq;
> shouldn't this be errirq
>
>>  	}
>>
>> -	if (fsl_edma->txirq == fsl_edma->errirq) {
>> -		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
>> -				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
>> -		if (ret) {
>> -			dev_err(&pdev->dev, "Can't register eDMA IRQ.\n");
>> -			 return  ret;
>> +	if (fsl_edma->kinetis) {
>> +		int i;
>> +		int irqs = of_irq_count(np);
>> +
>> +		if (irqs <= 1) {
>> +			dev_err(&pdev->dev, "Wrong eDMA irq count %d\n", irqs);
>> +			return -EINVAL;
> why not return irqs?
>
>>  		}
>> -	} else {
>> -		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
>> +
>> +		for (i = 0; i < (irqs - 1); i++) {
>> +			char irq_name[32];
>> +
>> +			sprintf(irq_name, "edma-tx-%d,%d", i, 16 + i);
>> +			irq = platform_get_irq_byname(pdev, irq_name);
>> +			if (irq < 0) {
>> +				dev_err(&pdev->dev, "Can't get %s irq.\n",
>> +							irq_name);
>> +				return irq;
>> +			}
>> +
>> +			ret = devm_request_irq(&pdev->dev, irq,
>>  				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
>> -		if (ret) {
>> -			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
>> -			return  ret;
>> +			if (ret) {
>> +				dev_err(&pdev->dev, "Can't register %s IRQ.\n",
>> +							irq_name);
>> +				return  ret;
>> +			}
>>  		}
>>
>> -		ret = devm_request_irq(&pdev->dev, fsl_edma->errirq,
>> -				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
>> +		ret = devm_request_irq(&pdev->dev, errirq,
>> +			fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
>>  		if (ret) {
>>  			dev_err(&pdev->dev, "Can't register eDMA err IRQ.\n");
>> -			return  ret;
>> +		return  ret;
>> +		}
>> +	} else {
>> +		irq = platform_get_irq_byname(pdev, "edma-tx");
>> +		if (irq < 0) {
>> +			dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
>> +			return irq;
>> +		}
>> +
>> +		if (irq == errirq) {
>> +			ret = devm_request_irq(&pdev->dev, irq,
>> +				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
>> +			if (ret) {
>> +				dev_err(&pdev->dev,
>> +						"Can't register eDMA IRQ.\n");
>> +				return  ret;
>> +			}
>> +		} else {
>> +			ret = devm_request_irq(&pdev->dev, irq,
>> +				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
>> +			if (ret) {
>> +				dev_err(&pdev->dev,
>> +					    "Can't register eDMA tx IRQ.\n");
>> +				return  ret;
>> +			}
>> +
>> +			ret = devm_request_irq(&pdev->dev, errirq,
>> +				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
>> +			if (ret) {
>> +				dev_err(&pdev->dev,
>> +					    "Can't register eDMA err IRQ.\n");
>> +				return  ret;
>> +			}
>>  		}
>>  	}
> Okay here we have a bunch of changes without explanation on why these are
> required and what are they supposed to do. So it is kind of very difficult to
> review!
>
> So please split these up in sequence of patches where each patch does only
> ONE thing with right changelog messages
>
> Thanks
>
> -- 
> ~Vinod
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/dma/fsl-edma.txt b/Documentation/devicetree/bindings/dma/fsl-edma.txt
index 191d7bd..88c3e20 100644
--- a/Documentation/devicetree/bindings/dma/fsl-edma.txt
+++ b/Documentation/devicetree/bindings/dma/fsl-edma.txt
@@ -9,6 +9,7 @@  group, DMAMUX0 or DMAMUX1, but not both.
 Required properties:
 - compatible :
 	- "fsl,vf610-edma" for eDMA used similar to that on Vybrid vf610 SoC
+	- "fsl,kinetis-edma" for eDMA used similar to that on Kinetis SoC
 - reg : Specifies base physical address(s) and size of the eDMA registers.
 	The 1st region is eDMA control register's address and size.
 	The 2nd and the 3rd regions are programmable channel multiplexing
@@ -16,7 +17,8 @@  Required properties:
 - interrupts : A list of interrupt-specifiers, one for each entry in
 	interrupt-names.
 - interrupt-names : Should contain:
-	"edma-tx" - the transmission interrupt
+	"edma-tx" - the transmission interrupt (Vybrid)
+	"edma-tx-n,m" - interrupt for channels n (0-15) and m (16-31) (Kinetis)
 	"edma-err" - the error interrupt
 - #dma-cells : Must be <2>.
 	The 1st cell specifies the DMAMUX(0 for DMAMUX0 and 1 for DMAMUX1).
@@ -28,6 +30,7 @@  Required properties:
 - clock-names : A list of channel group clock names. Should contain:
 	"dmamux0" - clock name of mux0 group
 	"dmamux1" - clock name of mux1 group
+	"edma"    - clock gate for whole controller (Kinetis only)
 - clocks : A list of phandle and clock-specifier pairs, one for each entry in
 	clock-names.
 
@@ -54,6 +57,39 @@  edma0: dma-controller@40018000 {
 		<&clks VF610_CLK_DMAMUX1>;
 };
 
+edma: dma-controller@40008000 {
+	compatible = "fsl,kinetis-edma";
+	reg = <0x40008000 0x2000>, /* DMAC */
+		<0x40021000 0x1000>, /* DMAMUX0 */
+		<0x40022000 0x1000>; /* DMAMUX1 */
+	#dma-cells = <2>;
+	dma-channels = <32>;
+	interrupts =	 <0>,  <1>,  <2>,  <3>,
+			 <4>,  <5>,  <6>,  <7>,
+			 <8>,  <9>, <10>, <11>,
+			<12>, <13>, <14>, <15>,
+			<16>;
+	interrupt-names = "edma-tx-0,16",
+			  "edma-tx-1,17",
+			  "edma-tx-2,18",
+			  "edma-tx-3,19",
+			  "edma-tx-4,20",
+			  "edma-tx-5,21",
+			  "edma-tx-6,22",
+			  "edma-tx-7,23",
+			  "edma-tx-8,24",
+			  "edma-tx-9,25",
+			  "edma-tx-10,26",
+			  "edma-tx-11,27",
+			  "edma-tx-12,28",
+			  "edma-tx-13,29",
+			  "edma-tx-14,30",
+			  "edma-tx-15,31",
+			  "edma-err";
+	clocks = <&mcg_cclk_gate 6 1>,
+		 <&mcg_pclk_gate 5 1>, <&mcg_pclk_gate 5 2>;
+	clock-names = "edma", "dmamux0", "dmamux1";
+};
 
 * DMA clients
 DMA client drivers that uses the DMA function must use the format described
diff --git a/drivers/dma/fsl-edma.c b/drivers/dma/fsl-edma.c
index 915eec3..e41a3b9 100644
--- a/drivers/dma/fsl-edma.c
+++ b/drivers/dma/fsl-edma.c
@@ -164,13 +164,13 @@  struct fsl_edma_desc {
 struct fsl_edma_engine {
 	struct dma_device	dma_dev;
 	void __iomem		*membase;
+	struct clk		*clk;
 	void __iomem		*muxbase[DMAMUX_NR];
 	struct clk		*muxclk[DMAMUX_NR];
 	struct mutex		fsl_edma_mutex;
 	u32			n_chans;
-	int			txirq;
-	int			errirq;
 	bool			big_endian;
+	bool			kinetis;
 	struct fsl_edma_chan	chans[];
 };
 
@@ -788,42 +788,85 @@  static void fsl_edma_free_chan_resources(struct dma_chan *chan)
 }
 
 static int
-fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma)
+fsl_edma_irq_init(struct platform_device *pdev,
+		  struct fsl_edma_engine *fsl_edma)
 {
+	struct device_node *np = pdev->dev.of_node;
+	int irq, errirq;
 	int ret;
 
-	fsl_edma->txirq = platform_get_irq_byname(pdev, "edma-tx");
-	if (fsl_edma->txirq < 0) {
-		dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
-		return fsl_edma->txirq;
-	}
-
-	fsl_edma->errirq = platform_get_irq_byname(pdev, "edma-err");
-	if (fsl_edma->errirq < 0) {
+	errirq = platform_get_irq_byname(pdev, "edma-err");
+	if (errirq < 0) {
 		dev_err(&pdev->dev, "Can't get edma-err irq.\n");
-		return fsl_edma->errirq;
+		return irq;
 	}
 
-	if (fsl_edma->txirq == fsl_edma->errirq) {
-		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
-				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
-		if (ret) {
-			dev_err(&pdev->dev, "Can't register eDMA IRQ.\n");
-			 return  ret;
+	if (fsl_edma->kinetis) {
+		int i;
+		int irqs = of_irq_count(np);
+
+		if (irqs <= 1) {
+			dev_err(&pdev->dev, "Wrong eDMA irq count %d\n", irqs);
+			return -EINVAL;
 		}
-	} else {
-		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
+
+		for (i = 0; i < (irqs - 1); i++) {
+			char irq_name[32];
+
+			sprintf(irq_name, "edma-tx-%d,%d", i, 16 + i);
+			irq = platform_get_irq_byname(pdev, irq_name);
+			if (irq < 0) {
+				dev_err(&pdev->dev, "Can't get %s irq.\n",
+							irq_name);
+				return irq;
+			}
+
+			ret = devm_request_irq(&pdev->dev, irq,
 				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
-		if (ret) {
-			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
-			return  ret;
+			if (ret) {
+				dev_err(&pdev->dev, "Can't register %s IRQ.\n",
+							irq_name);
+				return  ret;
+			}
 		}
 
-		ret = devm_request_irq(&pdev->dev, fsl_edma->errirq,
-				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
+		ret = devm_request_irq(&pdev->dev, errirq,
+			fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
 		if (ret) {
 			dev_err(&pdev->dev, "Can't register eDMA err IRQ.\n");
-			return  ret;
+		return  ret;
+		}
+	} else {
+		irq = platform_get_irq_byname(pdev, "edma-tx");
+		if (irq < 0) {
+			dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
+			return irq;
+		}
+
+		if (irq == errirq) {
+			ret = devm_request_irq(&pdev->dev, irq,
+				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+						"Can't register eDMA IRQ.\n");
+				return  ret;
+			}
+		} else {
+			ret = devm_request_irq(&pdev->dev, irq,
+				fsl_edma_tx_handler, 0, "eDMA tx", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+					    "Can't register eDMA tx IRQ.\n");
+				return  ret;
+			}
+
+			ret = devm_request_irq(&pdev->dev, errirq,
+				fsl_edma_err_handler, 0, "eDMA err", fsl_edma);
+			if (ret) {
+				dev_err(&pdev->dev,
+					    "Can't register eDMA err IRQ.\n");
+				return  ret;
+			}
 		}
 	}
 
@@ -851,6 +894,7 @@  static int fsl_edma_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	fsl_edma->n_chans = chans;
+	fsl_edma->kinetis = of_device_is_compatible(np, "fsl,kinetis-edma");
 	mutex_init(&fsl_edma->fsl_edma_mutex);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -858,6 +902,20 @@  static int fsl_edma_probe(struct platform_device *pdev)
 	if (IS_ERR(fsl_edma->membase))
 		return PTR_ERR(fsl_edma->membase);
 
+	if (fsl_edma->kinetis) {
+		fsl_edma->clk = devm_clk_get(&pdev->dev, "edma");
+		if (IS_ERR(fsl_edma->clk)) {
+			dev_err(&pdev->dev, "Missing EDMA clock.\n");
+			return PTR_ERR(fsl_edma->clk);
+		}
+
+		ret = clk_prepare_enable(fsl_edma->clk);
+		if (ret) {
+			dev_err(&pdev->dev, "EDMA clk failed.\n");
+			return ret;
+		}
+	}
+
 	for (i = 0; i < DMAMUX_NR; i++) {
 		char clkname[32];
 
@@ -956,10 +1014,14 @@  static int fsl_edma_remove(struct platform_device *pdev)
 	for (i = 0; i < DMAMUX_NR; i++)
 		clk_disable_unprepare(fsl_edma->muxclk[i]);
 
+	if (fsl_edma->kinetis)
+		clk_disable_unprepare(fsl_edma->clk);
+
 	return 0;
 }
 
 static const struct of_device_id fsl_edma_dt_ids[] = {
+	{ .compatible = "fsl,kinetis-edma", },
 	{ .compatible = "fsl,vf610-edma", },
 	{ /* sentinel */ }
 };