| Message ID | 20200709224550.15539-6-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account | expand |
On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: > There are DMA devices (like ours version of Synopsys DW DMAC) which have > DMA capabilities non-uniformly redistributed between the device channels. > In order to provide a way of exposing the channel-specific parameters to > the DMA engine consumers, we introduce a new DMA-device callback. In case > if provided it gets called from the dma_get_slave_caps() method and is > able to override the generic DMA-device capabilities. In light of recent developments consider not to add 'slave' and a such words to the kernel.
On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: > > There are DMA devices (like ours version of Synopsys DW DMAC) which have > > DMA capabilities non-uniformly redistributed between the device channels. > > In order to provide a way of exposing the channel-specific parameters to > > the DMA engine consumers, we introduce a new DMA-device callback. In case > > if provided it gets called from the dma_get_slave_caps() method and is > > able to override the generic DMA-device capabilities. > > In light of recent developments consider not to add 'slave' and a such words to the kernel. As long as the 'slave' word is used in the name of the dma_slave_caps structure and in the rest of the DMA-engine subsystem, it will be ambiguous to use some else terminology. If renaming needs to be done, then it should be done synchronously for the whole subsystem. -Sergey > > > -- > With Best Regards, > Andy Shevchenko > >
On 10-07-20, 12:38, Serge Semin wrote: > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: > > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: > > > There are DMA devices (like ours version of Synopsys DW DMAC) which have > > > DMA capabilities non-uniformly redistributed between the device channels. > > > In order to provide a way of exposing the channel-specific parameters to > > > the DMA engine consumers, we introduce a new DMA-device callback. In case > > > if provided it gets called from the dma_get_slave_caps() method and is > > > able to override the generic DMA-device capabilities. > > > > > In light of recent developments consider not to add 'slave' and a such words to the kernel. > > As long as the 'slave' word is used in the name of the dma_slave_caps > structure and in the rest of the DMA-engine subsystem, it will be ambiguous > to use some else terminology. If renaming needs to be done, then it should be > done synchronously for the whole subsystem. Right, I have plans to tackle that during next merge window and have started changes. Thankfully slave_dma can be replaced by peripheral dma easily. But getting that in would be tricky as we need to change users too.
Hello Vinod, Could you please keep on this patchset review? Really, the patchset isn't that big and complicated to be working on it for such a long time. I've sent it out at the time of the kernel 5.6. I've considered all the Andy's comments since then. There is going to be 5.9 merge window soon, but the patchset still under review procedure, while I still have some work, which depends on the changes provided by this patchset. It would be great to at least submit it for review before the next merge window, and super-great have it merged in before that. There is a Peter Ujfalusi comment to the patch "[PATCH v7 04/11] dmaengine: Introduce max SG list entries capability", which needs your attention. Could you please take a look at that? So I could submit the next patchset revision if you agree with the Peter' suggestion. -Sergey On Mon, Jul 13, 2020 at 12:21:31PM +0530, Vinod Koul wrote: > On 10-07-20, 12:38, Serge Semin wrote: > > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: > > > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: > > > > There are DMA devices (like ours version of Synopsys DW DMAC) which have > > > > DMA capabilities non-uniformly redistributed between the device channels. > > > > In order to provide a way of exposing the channel-specific parameters to > > > > the DMA engine consumers, we introduce a new DMA-device callback. In case > > > > if provided it gets called from the dma_get_slave_caps() method and is > > > > able to override the generic DMA-device capabilities. > > > > > > > > In light of recent developments consider not to add 'slave' and a such words to the kernel. > > > > As long as the 'slave' word is used in the name of the dma_slave_caps > > structure and in the rest of the DMA-engine subsystem, it will be ambiguous > > to use some else terminology. If renaming needs to be done, then it should be > > done synchronously for the whole subsystem. > > Right, I have plans to tackle that during next merge window and have > started changes. Thankfully slave_dma can be replaced by peripheral dma > easily. But getting that in would be tricky as we need to change users > too. > > -- > ~Vinod
On 7/10/2020 2:38 AM, Serge Semin wrote: > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: >> On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: >>> There are DMA devices (like ours version of Synopsys DW DMAC) which have >>> DMA capabilities non-uniformly redistributed between the device channels. >>> In order to provide a way of exposing the channel-specific parameters to >>> the DMA engine consumers, we introduce a new DMA-device callback. In case >>> if provided it gets called from the dma_get_slave_caps() method and is >>> able to override the generic DMA-device capabilities. >> > >> In light of recent developments consider not to add 'slave' and a such words to the kernel. > > As long as the 'slave' word is used in the name of the dma_slave_caps > structure and in the rest of the DMA-engine subsystem, it will be ambiguous > to use some else terminology. If renaming needs to be done, then it should be > done synchronously for the whole subsystem. What about just calling it dma_device_caps? Consider this is a useful function not only slave DMA will utilize this. I can see this being useful for some of my future code with idxd driver. > > -Sergey > >> >> >> -- >> With Best Regards, >> Andy Shevchenko >> >>
On 13-07-20, 13:55, Dave Jiang wrote: > > > On 7/10/2020 2:38 AM, Serge Semin wrote: > > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: > > > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: > > > > There are DMA devices (like ours version of Synopsys DW DMAC) which have > > > > DMA capabilities non-uniformly redistributed between the device channels. > > > > In order to provide a way of exposing the channel-specific parameters to > > > > the DMA engine consumers, we introduce a new DMA-device callback. In case > > > > if provided it gets called from the dma_get_slave_caps() method and is > > > > able to override the generic DMA-device capabilities. > > > > > > > > In light of recent developments consider not to add 'slave' and a such words to the kernel. > > > > As long as the 'slave' word is used in the name of the dma_slave_caps > > structure and in the rest of the DMA-engine subsystem, it will be ambiguous > > to use some else terminology. If renaming needs to be done, then it should be > > done synchronously for the whole subsystem. > > What about just calling it dma_device_caps? Consider this is a useful > function not only slave DMA will utilize this. I can see this being useful > for some of my future code with idxd driver. Some of the caps may make sense to generic dmaengine but few of them do not :) While at it, am planning to make it dmaengine_periph_caps to denote that these are dmaengine peripheral capabilities.
On 7/14/2020 9:08 AM, Vinod Koul wrote: > On 13-07-20, 13:55, Dave Jiang wrote: >> >> >> On 7/10/2020 2:38 AM, Serge Semin wrote: >>> On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: >>>> On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: >>>>> There are DMA devices (like ours version of Synopsys DW DMAC) which have >>>>> DMA capabilities non-uniformly redistributed between the device channels. >>>>> In order to provide a way of exposing the channel-specific parameters to >>>>> the DMA engine consumers, we introduce a new DMA-device callback. In case >>>>> if provided it gets called from the dma_get_slave_caps() method and is >>>>> able to override the generic DMA-device capabilities. >>>> >>> >>>> In light of recent developments consider not to add 'slave' and a such words to the kernel. >>> >>> As long as the 'slave' word is used in the name of the dma_slave_caps >>> structure and in the rest of the DMA-engine subsystem, it will be ambiguous >>> to use some else terminology. If renaming needs to be done, then it should be >>> done synchronously for the whole subsystem. >> >> What about just calling it dma_device_caps? Consider this is a useful >> function not only slave DMA will utilize this. I can see this being useful >> for some of my future code with idxd driver. > > Some of the caps may make sense to generic dmaengine but few of them do > not :) While at it, am planning to make it dmaengine_periph_caps to > denote that these are dmaengine peripheral capabilities. > If the function only passes in periph_caps, how do we allow the non periph DMA utilize this function?
On Tue, Jul 14, 2020 at 09:18:16AM -0700, Dave Jiang wrote: > > > On 7/14/2020 9:08 AM, Vinod Koul wrote: > > On 13-07-20, 13:55, Dave Jiang wrote: > > > > > > > > > On 7/10/2020 2:38 AM, Serge Semin wrote: > > > > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: > > > > > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: > > > > > > There are DMA devices (like ours version of Synopsys DW DMAC) which have > > > > > > DMA capabilities non-uniformly redistributed between the device channels. > > > > > > In order to provide a way of exposing the channel-specific parameters to > > > > > > the DMA engine consumers, we introduce a new DMA-device callback. In case > > > > > > if provided it gets called from the dma_get_slave_caps() method and is > > > > > > able to override the generic DMA-device capabilities. > > > > > > > > > > > > > > In light of recent developments consider not to add 'slave' and a such words to the kernel. > > > > > > > > As long as the 'slave' word is used in the name of the dma_slave_caps > > > > structure and in the rest of the DMA-engine subsystem, it will be ambiguous > > > > to use some else terminology. If renaming needs to be done, then it should be > > > > done synchronously for the whole subsystem. > > > > > > What about just calling it dma_device_caps? Consider this is a useful > > > function not only slave DMA will utilize this. I can see this being useful > > > for some of my future code with idxd driver. > > > > Some of the caps may make sense to generic dmaengine but few of them do > > not :) While at it, am planning to make it dmaengine_periph_caps to > > denote that these are dmaengine peripheral capabilities. > > > > If the function only passes in periph_caps, how do we allow the non periph > DMA utilize this function? Hello Dave. That seems reasonable. "dma_device_caps" or even "dma_chan_caps" might be more suitable seeing after this patchset merged in the "dma_slave_caps" may really provide the DMA channel-specific configs. Moreover that structure is accessible only by means of the dma_chan descriptor: int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps); which makes those caps being the channel-specific even without this patchset. So as I see it "dma_chan_caps" might be the better choice. -Sergey
On 7/14/2020 9:29 AM, Serge Semin wrote: > On Tue, Jul 14, 2020 at 09:18:16AM -0700, Dave Jiang wrote: >> >> >> On 7/14/2020 9:08 AM, Vinod Koul wrote: >>> On 13-07-20, 13:55, Dave Jiang wrote: >>>> >>>> >>>> On 7/10/2020 2:38 AM, Serge Semin wrote: >>>>> On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote: >>>>>> On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote: >>>>>>> There are DMA devices (like ours version of Synopsys DW DMAC) which have >>>>>>> DMA capabilities non-uniformly redistributed between the device channels. >>>>>>> In order to provide a way of exposing the channel-specific parameters to >>>>>>> the DMA engine consumers, we introduce a new DMA-device callback. In case >>>>>>> if provided it gets called from the dma_get_slave_caps() method and is >>>>>>> able to override the generic DMA-device capabilities. >>>>>> >>>>> >>>>>> In light of recent developments consider not to add 'slave' and a such words to the kernel. >>>>> >>>>> As long as the 'slave' word is used in the name of the dma_slave_caps >>>>> structure and in the rest of the DMA-engine subsystem, it will be ambiguous >>>>> to use some else terminology. If renaming needs to be done, then it should be >>>>> done synchronously for the whole subsystem. >>>> >>>> What about just calling it dma_device_caps? Consider this is a useful >>>> function not only slave DMA will utilize this. I can see this being useful >>>> for some of my future code with idxd driver. >>> >>> Some of the caps may make sense to generic dmaengine but few of them do >>> not :) While at it, am planning to make it dmaengine_periph_caps to >>> denote that these are dmaengine peripheral capabilities. >>> >> > >> If the function only passes in periph_caps, how do we allow the non periph >> DMA utilize this function? > > Hello Dave. That seems reasonable. "dma_device_caps" or even "dma_chan_caps" > might be more suitable seeing after this patchset merged in the "dma_slave_caps" > may really provide the DMA channel-specific configs. Moreover that structure is > accessible only by means of the dma_chan descriptor: > > int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps); > > which makes those caps being the channel-specific even without this patchset. > > So as I see it "dma_chan_caps" might be the better choice. Hi Sergey. Yes I think that sounds pretty good. Especially seeing there are DMA engines that have channels with different/asymmetric capabilities now. > > -Sergey >
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index ad56ad58932c..ed1fd376f1a5 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -599,6 +599,16 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps) caps->cmd_resume = !!device->device_resume; caps->cmd_terminate = !!device->device_terminate_all; + /* + * DMA engine device might be configured with non-uniformly + * distributed slave capabilities per device channels. In this + * case the corresponding driver may provide the device_caps + * callback to override the generic capabilities with + * channel-specific ones. + */ + if (device->device_caps) + device->device_caps(chan, caps); + return 0; } EXPORT_SYMBOL_GPL(dma_get_slave_caps); diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index a7e4d8dfdd19..298b721c8b9f 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -799,6 +799,8 @@ struct dma_filter { * be called after period_len bytes have been transferred. * @device_prep_interleaved_dma: Transfer expression in a generic way. * @device_prep_dma_imm_data: DMA's 8 byte immediate data to the dst address + * @device_caps: May be used to override the generic DMA slave capabilities + * with per-channel specific ones * @device_config: Pushes a new configuration to a channel, return 0 or an error * code * @device_pause: Pauses any transfer happening on a channel. Returns @@ -899,6 +901,8 @@ struct dma_device { struct dma_chan *chan, dma_addr_t dst, u64 data, unsigned long flags); + void (*device_caps)(struct dma_chan *chan, + struct dma_slave_caps *caps); int (*device_config)(struct dma_chan *chan, struct dma_slave_config *config); int (*device_pause)(struct dma_chan *chan);