[v3,07/18] dmaengine: dma-jz4780: Add support for the JZ4770 SoC
diff mbox

Message ID 20180721110643.19624-8-paul@crapouillou.net
State Changes Requested
Headers show

Commit Message

Paul Cercueil July 21, 2018, 11:06 a.m. UTC
The JZ4770 SoC has two DMA cores, each one featuring six DMA channels.
The major change is that each channel's clock can be enabled or disabled
through register writes.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
---
 drivers/dma/dma-jz4780.c | 46 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)

 v2: - Move transfer_ord_max variable to the new jz4780_dma_soc_data
       structure
     - The documentation update is now in patch 01/17

 v3: The Kconfig update was dropped thanks to patch 06/18

Comments

Vinod Koul July 24, 2018, 1:32 p.m. UTC | #1
On 21-07-18, 13:06, Paul Cercueil wrote:
> +static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
> +	unsigned int chn)

right justified and aligned with preceding please. While adding new
code to a existing driver it is a good idea to conform to existing style

> +{
> +	if (jzdma->version == ID_JZ4770)
> +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> +}
> +
> +static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
> +	unsigned int chn)
> +{
> +	if (jzdma->version == ID_JZ4770)
> +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));

so if another version has this feature we would do:
        if (jzdma->version == ID_JZ4770) ||
                if (jzdma->version == ID_JZXXXX))

and so on.. why not add a value, clk_enable in the description and use
that. For each controller it is set to true or false
Paul Cercueil July 24, 2018, 3:04 p.m. UTC | #2
Hi Vinod,

Le mar. 24 juil. 2018 à 15:32, Vinod <vkoul@kernel.org> a écrit :
> On 21-07-18, 13:06, Paul Cercueil wrote:
>>  +static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev 
>> *jzdma,
>>  +	unsigned int chn)
> 
> right justified and aligned with preceding please. While adding new
> code to a existing driver it is a good idea to conform to existing 
> style

OK.

>>  +{
>>  +	if (jzdma->version == ID_JZ4770)
>>  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
>>  +}
>>  +
>>  +static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev 
>> *jzdma,
>>  +	unsigned int chn)
>>  +{
>>  +	if (jzdma->version == ID_JZ4770)
>>  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
> 
> so if another version has this feature we would do:
>         if (jzdma->version == ID_JZ4770) ||
>                 if (jzdma->version == ID_JZXXXX))
> 
> and so on.. why not add a value, clk_enable in the description and use
> that. For each controller it is set to true or false

I agree with what you said in your other answers.
However here I still need to check the "version", because on JZ4725B 
and JZ4770+
the way to start/stop each DMA channel's clock is different, so I can't 
use a boolean.

> --
> ~Vinod

Thanks,
-Paul

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul July 24, 2018, 4:01 p.m. UTC | #3
On 24-07-18, 17:04, Paul Cercueil wrote:
> Hi Vinod,
> 
> Le mar. 24 juil. 2018 à 15:32, Vinod <vkoul@kernel.org> a écrit :
> > On 21-07-18, 13:06, Paul Cercueil wrote:
> > >  +static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev
> > > *jzdma,
> > >  +	unsigned int chn)
> > 
> > right justified and aligned with preceding please. While adding new
> > code to a existing driver it is a good idea to conform to existing style
> 
> OK.
> 
> > >  +{
> > >  +	if (jzdma->version == ID_JZ4770)
> > >  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> > >  +}
> > >  +
> > >  +static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev
> > > *jzdma,
> > >  +	unsigned int chn)
> > >  +{
> > >  +	if (jzdma->version == ID_JZ4770)
> > >  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
> > 
> > so if another version has this feature we would do:
> >         if (jzdma->version == ID_JZ4770) ||
> >                 if (jzdma->version == ID_JZXXXX))
> > 
> > and so on.. why not add a value, clk_enable in the description and use
> > that. For each controller it is set to true or false
> 
> I agree with what you said in your other answers.
> However here I still need to check the "version", because on JZ4725B and
> JZ4770+
> the way to start/stop each DMA channel's clock is different, so I can't use
> a boolean

sure describe the behavior and use that. Versions is not a very scalable
way..
Paul Cercueil Aug. 4, 2018, 9:02 a.m. UTC | #4
Hi Vinod,

Le mar. 24 juil. 2018 à 15:32, Vinod <vkoul@kernel.org> a écrit :
> On 21-07-18, 13:06, Paul Cercueil wrote:
>>  +static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev 
>> *jzdma,
>>  +	unsigned int chn)
> 
> right justified and aligned with preceding please. While adding new
> code to a existing driver it is a good idea to conform to existing 
> style

Well that's exactly what I did, this is the style used in the DMA 
driver,
so I tried to conform to it.

>>  +{
>>  +	if (jzdma->version == ID_JZ4770)
>>  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
>>  +}
>>  +
>>  +static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev 
>> *jzdma,
>>  +	unsigned int chn)
>>  +{
>>  +	if (jzdma->version == ID_JZ4770)
>>  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
> 
> so if another version has this feature we would do:
>         if (jzdma->version == ID_JZ4770) ||
>                 if (jzdma->version == ID_JZXXXX))
> 
> and so on.. why not add a value, clk_enable in the description and use
> that. For each controller it is set to true or false
> 
> --
> ~Vinod

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" 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/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 23e92d153919..a5f4a8d54516 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -29,6 +29,9 @@ 
 #define JZ_DMA_REG_DIRQP	0x04
 #define JZ_DMA_REG_DDR		0x08
 #define JZ_DMA_REG_DDRS		0x0c
+#define JZ_DMA_REG_DCKE		0x10
+#define JZ_DMA_REG_DCKES	0x14
+#define JZ_DMA_REG_DCKEC	0x18
 #define JZ_DMA_REG_DMACP	0x1c
 #define JZ_DMA_REG_DSIRQP	0x20
 #define JZ_DMA_REG_DSIRQM	0x24
@@ -132,11 +135,13 @@  struct jz4780_dma_chan {
 };
 
 enum jz_version {
+	ID_JZ4770,
 	ID_JZ4780,
 };
 
 struct jz4780_dma_soc_data {
 	unsigned int nb_channels;
+	unsigned int transfer_ord_max;
 };
 
 struct jz4780_dma_dev {
@@ -200,6 +205,20 @@  static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev *jzdma,
 	writel(val, jzdma->ctrl_base + reg);
 }
 
+static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
+	unsigned int chn)
+{
+	if (jzdma->version == ID_JZ4770)
+		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
+}
+
+static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
+	unsigned int chn)
+{
+	if (jzdma->version == ID_JZ4770)
+		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
+}
+
 static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
 	struct jz4780_dma_chan *jzchan, unsigned int count,
 	enum dma_transaction_type type)
@@ -234,8 +253,10 @@  static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
 	kfree(desc);
 }
 
-static uint32_t jz4780_dma_transfer_size(unsigned long val, uint32_t *shift)
+static uint32_t jz4780_dma_transfer_size(struct jz4780_dma_chan *jzchan,
+	unsigned long val, uint32_t *shift)
 {
+	struct jz4780_dma_dev *jzdma = jz4780_dma_chan_parent(jzchan);
 	int ord = ffs(val) - 1;
 
 	/*
@@ -247,8 +268,8 @@  static uint32_t jz4780_dma_transfer_size(unsigned long val, uint32_t *shift)
 	 */
 	if (ord == 3)
 		ord = 2;
-	else if (ord > 7)
-		ord = 7;
+	else if (ord > jzdma->soc_data->transfer_ord_max)
+		ord = jzdma->soc_data->transfer_ord_max;
 
 	*shift = ord;
 
@@ -300,7 +321,7 @@  static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan *jzchan,
 	 * divisible by the transfer size, and we must not use more than the
 	 * maximum burst specified by the user.
 	 */
-	tsz = jz4780_dma_transfer_size(addr | len | (width * maxburst),
+	tsz = jz4780_dma_transfer_size(jzchan, addr | len | (width * maxburst),
 				       &jzchan->transfer_shift);
 
 	switch (width) {
@@ -429,7 +450,7 @@  static struct dma_async_tx_descriptor *jz4780_dma_prep_dma_memcpy(
 	if (!desc)
 		return NULL;
 
-	tsz = jz4780_dma_transfer_size(dest | src | len,
+	tsz = jz4780_dma_transfer_size(jzchan, dest | src | len,
 				       &jzchan->transfer_shift);
 
 	jzchan->transfer_type = JZ_DMA_DRT_AUTO;
@@ -490,6 +511,9 @@  static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
 			(jzchan->curr_hwdesc + 1) % jzchan->desc->count;
 	}
 
+	/* Enable the channel's clock. */
+	jz4780_dma_chan_enable(jzdma, jzchan->id);
+
 	/* Use 4-word descriptors. */
 	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
 
@@ -537,6 +561,8 @@  static int jz4780_dma_terminate_all(struct dma_chan *chan)
 		jzchan->desc = NULL;
 	}
 
+	jz4780_dma_chan_disable(jzdma, jzchan->id);
+
 	vchan_get_all_descriptors(&jzchan->vchan, &head);
 
 	spin_unlock_irqrestore(&jzchan->vchan.lock, flags);
@@ -548,8 +574,10 @@  static int jz4780_dma_terminate_all(struct dma_chan *chan)
 static void jz4780_dma_synchronize(struct dma_chan *chan)
 {
 	struct jz4780_dma_chan *jzchan = to_jz4780_dma_chan(chan);
+	struct jz4780_dma_dev *jzdma = jz4780_dma_chan_parent(jzchan);
 
 	vchan_synchronize(&jzchan->vchan);
+	jz4780_dma_chan_disable(jzdma, jzchan->id);
 }
 
 static int jz4780_dma_config(struct dma_chan *chan,
@@ -775,10 +803,12 @@  static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
 }
 
 static const struct jz4780_dma_soc_data jz4780_dma_soc_data[] = {
-	[ID_JZ4780] = { .nb_channels = 32, },
+	[ID_JZ4770] = { .nb_channels = 6, .transfer_ord_max = 6, },
+	[ID_JZ4780] = { .nb_channels = 32, .transfer_ord_max = 7, },
 };
 
 static const struct of_device_id jz4780_dma_dt_match[] = {
+	{ .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
 	{ .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
 	{},
 };
@@ -901,7 +931,9 @@  static int jz4780_dma_probe(struct platform_device *pdev)
 	 */
 	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
 			  JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
-	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
+
+	if (jzdma->version == ID_JZ4780)
+		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
 
 	INIT_LIST_HEAD(&dd->channels);