| Message ID | 20180721110643.19624-8-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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..
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
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);