| Message ID | 1531239793-11781-4-git-send-email-yibin.gong@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 11-07-18, 00:23, Robin Gong wrote: > dmatest(memcpy) will never call dmaengine_slave_config before prep, and that should have been a hint to you that you should not expect that > so jobs in dmaengine_slave_config need to be moved into somewhere > before device_prep_dma_memcpy. Besides, dmatest never setup chan > ->private as other common case like uart/audio/spi will always setup > chan->private. Here check it to judge if it's dmatest case and do > jobs in slave_config. and you should not do anything for dmatest. Supporting it means memcpy implementation is not correct :) > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > --- > drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index ed2267d..48f3749 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan) > { > struct sdma_channel *sdmac = to_sdma_chan(chan); > struct imx_dma_data *data = chan->private; > + struct imx_dma_data default_data; > int prio, ret; > > - if (!data) > - return -EINVAL; > + ret = clk_enable(sdmac->sdma->clk_ipg); > + if (ret) > + return ret; > + ret = clk_enable(sdmac->sdma->clk_ahb); > + if (ret) > + goto disable_clk_ipg; > + /* > + * dmatest(memcpy) will never call dmaengine_slave_config before prep, > + * so jobs in dmaengine_slave_config need to be moved into somewhere > + * before device_prep_dma_memcpy. Besides, dmatest never setup chan > + * ->private as other common cases like uart/audio/spi will setup > + * chan->private always. Here check it to judge if it's dmatest case > + * and do jobs in slave_config. > + */ > + if (!data) { > + dev_warn(sdmac->sdma->dev, "dmatest is running?\n"); why is that a warning! > + sdmac->word_size = sdmac->sdma->dma_device.copy_align; > + default_data.priority = 2; > + default_data.peripheral_type = IMX_DMATYPE_MEMORY; > + default_data.dma_request = 0; > + default_data.dma_request2 = 0; > + data = &default_data; > + > + sdma_config_ownership(sdmac, false, true, false); > + sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY); > + sdma_load_context(sdmac); > + } this needs to be default for memcpy
> -----Original Message----- > From: Vinod [mailto:vkoul@kernel.org] > Sent: 2018年7月10日 23:33 > To: Robin Gong <yibin.gong@nxp.com> > Cc: dan.j.williams@intel.com; shawnguo@kernel.org; > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>; > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org; > kernel@pengutronix.de; dmaengine@vger.kernel.org; > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest > > On 11-07-18, 00:23, Robin Gong wrote: > > dmatest(memcpy) will never call dmaengine_slave_config before prep, > > and that should have been a hint to you that you should not expect that > > > so jobs in dmaengine_slave_config need to be moved into somewhere > > before device_prep_dma_memcpy. Besides, dmatest never setup chan > > ->private as other common case like uart/audio/spi will always setup > > chan->private. Here check it to judge if it's dmatest case and do > > jobs in slave_config. > > and you should not do anything for dmatest. Supporting it means memcpy > implementation is not correct :) Okay, I will any word about dmatest here since memcpy assume no calling slave_config. > > > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > > --- > > drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++--------- > > 1 file changed, 28 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index > > ed2267d..48f3749 100644 > > --- a/drivers/dma/imx-sdma.c > > +++ b/drivers/dma/imx-sdma.c > > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct > > dma_chan *chan) { > > struct sdma_channel *sdmac = to_sdma_chan(chan); > > struct imx_dma_data *data = chan->private; > > + struct imx_dma_data default_data; > > int prio, ret; > > > > - if (!data) > > - return -EINVAL; > > + ret = clk_enable(sdmac->sdma->clk_ipg); > > + if (ret) > > + return ret; > > + ret = clk_enable(sdmac->sdma->clk_ahb); > > + if (ret) > > + goto disable_clk_ipg; > > + /* > > + * dmatest(memcpy) will never call dmaengine_slave_config before prep, > > + * so jobs in dmaengine_slave_config need to be moved into somewhere > > + * before device_prep_dma_memcpy. Besides, dmatest never setup chan > > + * ->private as other common cases like uart/audio/spi will setup > > + * chan->private always. Here check it to judge if it's dmatest case > > + * and do jobs in slave_config. > > + */ > > + if (!data) { > > + dev_warn(sdmac->sdma->dev, "dmatest is running?\n"); > > why is that a warning! Current SDMA driver assume filter function to set chan->private with specific data (struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c): static bool filter(struct dma_chan *chan, void *param) { if (!imx_dma_is_general_purpose(chan)) return false; chan->private = param; return true; } But in memcpy case, at lease dmatest case, no chan->private set in its filter function. So here take dmatest a special case and do some prepare jobs for memcpy. But if the Upper device driver call dma_request_channel() with their specific filter without 'chan->private' setting in the future. The warning message is a useful hint to them to Add 'chan->private' in filter function. Or doc it somewhere? > > > + sdmac->word_size = sdmac->sdma->dma_device.copy_align; > > + default_data.priority = 2; > > + default_data.peripheral_type = IMX_DMATYPE_MEMORY; > > + default_data.dma_request = 0; > > + default_data.dma_request2 = 0; > > + data = &default_data; > > + > > + sdma_config_ownership(sdmac, false, true, false); > > + sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY); > > + sdma_load_context(sdmac); > > + } > > this needs to be default for memcpy > > -- > ~Vinod
On Wed, Jul 11, 2018 at 06:37:02AM +0000, Robin Gong wrote: > > > -----Original Message----- > > From: Vinod [mailto:vkoul@kernel.org] > > Sent: 2018年7月10日 23:33 > > To: Robin Gong <yibin.gong@nxp.com> > > Cc: dan.j.williams@intel.com; shawnguo@kernel.org; > > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>; > > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org; > > kernel@pengutronix.de; dmaengine@vger.kernel.org; > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest > > > > On 11-07-18, 00:23, Robin Gong wrote: > > > dmatest(memcpy) will never call dmaengine_slave_config before prep, > > > > and that should have been a hint to you that you should not expect that > > > > > so jobs in dmaengine_slave_config need to be moved into somewhere > > > before device_prep_dma_memcpy. Besides, dmatest never setup chan > > > ->private as other common case like uart/audio/spi will always setup > > > chan->private. Here check it to judge if it's dmatest case and do > > > jobs in slave_config. > > > > and you should not do anything for dmatest. Supporting it means memcpy > > implementation is not correct :) > Okay, I will any word about dmatest here since memcpy assume no calling > slave_config. > > > > > > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > > > --- > > > drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++--------- > > > 1 file changed, 28 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index > > > ed2267d..48f3749 100644 > > > --- a/drivers/dma/imx-sdma.c > > > +++ b/drivers/dma/imx-sdma.c > > > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct > > > dma_chan *chan) { > > > struct sdma_channel *sdmac = to_sdma_chan(chan); > > > struct imx_dma_data *data = chan->private; > > > + struct imx_dma_data default_data; > > > int prio, ret; > > > > > > - if (!data) > > > - return -EINVAL; > > > + ret = clk_enable(sdmac->sdma->clk_ipg); > > > + if (ret) > > > + return ret; > > > + ret = clk_enable(sdmac->sdma->clk_ahb); > > > + if (ret) > > > + goto disable_clk_ipg; > > > + /* > > > + * dmatest(memcpy) will never call dmaengine_slave_config before prep, > > > + * so jobs in dmaengine_slave_config need to be moved into somewhere > > > + * before device_prep_dma_memcpy. Besides, dmatest never setup chan > > > + * ->private as other common cases like uart/audio/spi will setup > > > + * chan->private always. Here check it to judge if it's dmatest case > > > + * and do jobs in slave_config. > > > + */ > > > + if (!data) { > > > + dev_warn(sdmac->sdma->dev, "dmatest is running?\n"); > > > > why is that a warning! > Current SDMA driver assume filter function to set chan->private with specific data > (struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c): > static bool filter(struct dma_chan *chan, void *param) > { > if (!imx_dma_is_general_purpose(chan)) > return false; > chan->private = param; > return true; > } > > But in memcpy case, at lease dmatest case, no chan->private set in its filter function. > So here take dmatest a special case and do some prepare jobs for memcpy. But if the > Upper device driver call dma_request_channel() with their specific filter without > 'chan->private' setting in the future. The warning message is a useful hint to them to > Add 'chan->private' in filter function. Or doc it somewhere? Instead of doing heuristics to guess whether we are doing memcpy you could instead make memcpy the default when slave_config is not called, i.e. drop the if (!data) check completely. > > > > > + sdmac->word_size = sdmac->sdma->dma_device.copy_align; > > > + default_data.priority = 2; > > > + default_data.peripheral_type = IMX_DMATYPE_MEMORY; > > > + default_data.dma_request = 0; > > > + default_data.dma_request2 = 0; > > > + data = &default_data; > > > + > > > + sdma_config_ownership(sdmac, false, true, false); > > > + sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY); > > > + sdma_load_context(sdmac); > > > + } > > > > this needs to be default for memcpy The problem seems to be that we do not know whether we are doing memcpy or not. Normally we get the information how a channel is to be configured in dma_device->device_config, but this function is not called in the memcpy case. An alternative might also be to do the setup in dma_device->device_prep_dma_memcpy. Sascha
DQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IHMuaGF1ZXJAcGVuZ3V0cm9u aXguZGUgW21haWx0bzpzLmhhdWVyQHBlbmd1dHJvbml4LmRlXQ0KPiBTZW50OiAyMDE45bm0N+ac iDEx5pelIDE0OjU0DQo+IFRvOiBSb2JpbiBHb25nIDx5aWJpbi5nb25nQG54cC5jb20+DQo+IENj OiBWaW5vZCA8dmtvdWxAa2VybmVsLm9yZz47IGRhbi5qLndpbGxpYW1zQGludGVsLmNvbTsNCj4g c2hhd25ndW9Aa2VybmVsLm9yZzsgRmFiaW8gRXN0ZXZhbSA8ZmFiaW8uZXN0ZXZhbUBueHAuY29t PjsNCj4gbGludXhAYXJtbGludXgub3JnLnVrOyBsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJh ZGVhZC5vcmc7DQo+IGtlcm5lbEBwZW5ndXRyb25peC5kZTsgZG1hZW5naW5lQHZnZXIua2VybmVs Lm9yZzsNCj4gbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgZGwtbGludXgtaW14IDxsaW51 eC1pbXhAbnhwLmNvbT4NCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2MSAzLzRdIGRtYWVuZ2luZTog aW14LXNkbWE6IHN1cHBvcnQgZG1hdGVzdA0KPiANCj4gT24gV2VkLCBKdWwgMTEsIDIwMTggYXQg MDY6Mzc6MDJBTSArMDAwMCwgUm9iaW4gR29uZyB3cm90ZToNCj4gPg0KPiA+ID4gLS0tLS1Pcmln aW5hbCBNZXNzYWdlLS0tLS0NCj4gPiA+IEZyb206IFZpbm9kIFttYWlsdG86dmtvdWxAa2VybmVs Lm9yZ10NCj4gPiA+IFNlbnQ6IDIwMTjlubQ35pyIMTDml6UgMjM6MzMNCj4gPiA+IFRvOiBSb2Jp biBHb25nIDx5aWJpbi5nb25nQG54cC5jb20+DQo+ID4gPiBDYzogZGFuLmoud2lsbGlhbXNAaW50 ZWwuY29tOyBzaGF3bmd1b0BrZXJuZWwub3JnOw0KPiA+ID4gcy5oYXVlckBwZW5ndXRyb25peC5k ZTsgRmFiaW8gRXN0ZXZhbSA8ZmFiaW8uZXN0ZXZhbUBueHAuY29tPjsNCj4gPiA+IGxpbnV4QGFy bWxpbnV4Lm9yZy51azsgbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOw0KPiA+ ID4ga2VybmVsQHBlbmd1dHJvbml4LmRlOyBkbWFlbmdpbmVAdmdlci5rZXJuZWwub3JnOw0KPiA+ ID4gbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgZGwtbGludXgtaW14IDxsaW51eC1pbXhA bnhwLmNvbT4NCj4gPiA+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjEgMy80XSBkbWFlbmdpbmU6IGlt eC1zZG1hOiBzdXBwb3J0IGRtYXRlc3QNCj4gPiA+DQo+ID4gPiBPbiAxMS0wNy0xOCwgMDA6MjMs IFJvYmluIEdvbmcgd3JvdGU6DQo+ID4gPiA+IGRtYXRlc3QobWVtY3B5KSB3aWxsIG5ldmVyIGNh bGwgZG1hZW5naW5lX3NsYXZlX2NvbmZpZyBiZWZvcmUNCj4gPiA+ID4gcHJlcCwNCj4gPiA+DQo+ ID4gPiBhbmQgdGhhdCBzaG91bGQgaGF2ZSBiZWVuIGEgaGludCB0byB5b3UgdGhhdCB5b3Ugc2hv dWxkIG5vdCBleHBlY3QNCj4gPiA+IHRoYXQNCj4gPiA+DQo+ID4gPiA+IHNvIGpvYnMgaW4gZG1h ZW5naW5lX3NsYXZlX2NvbmZpZyBuZWVkIHRvIGJlIG1vdmVkIGludG8gc29tZXdoZXJlDQo+ID4g PiA+IGJlZm9yZSBkZXZpY2VfcHJlcF9kbWFfbWVtY3B5LiBCZXNpZGVzLCBkbWF0ZXN0IG5ldmVy IHNldHVwIGNoYW4NCj4gPiA+ID4gLT5wcml2YXRlIGFzIG90aGVyIGNvbW1vbiBjYXNlIGxpa2Ug dWFydC9hdWRpby9zcGkgd2lsbCBhbHdheXMNCj4gPiA+ID4gLT5zZXR1cA0KPiA+ID4gPiBjaGFu LT5wcml2YXRlLiBIZXJlIGNoZWNrIGl0IHRvIGp1ZGdlIGlmIGl0J3MgZG1hdGVzdCBjYXNlIGFu ZCBkbw0KPiA+ID4gPiBqb2JzIGluIHNsYXZlX2NvbmZpZy4NCj4gPiA+DQo+ID4gPiBhbmQgeW91 IHNob3VsZCBub3QgZG8gYW55dGhpbmcgZm9yIGRtYXRlc3QuIFN1cHBvcnRpbmcgaXQgbWVhbnMN Cj4gPiA+IG1lbWNweSBpbXBsZW1lbnRhdGlvbiBpcyBub3QgY29ycmVjdCA6KQ0KPiA+IE9rYXks IEkgd2lsbCBhbnkgd29yZCBhYm91dCBkbWF0ZXN0IGhlcmUgc2luY2UgbWVtY3B5IGFzc3VtZSBu bw0KPiA+IGNhbGxpbmcgc2xhdmVfY29uZmlnLg0KPiA+ID4NCj4gPiA+ID4NCj4gPiA+ID4gU2ln bmVkLW9mZi1ieTogUm9iaW4gR29uZyA8eWliaW4uZ29uZ0BueHAuY29tPg0KPiA+ID4gPiAtLS0N Cj4gPiA+ID4gIGRyaXZlcnMvZG1hL2lteC1zZG1hLmMgfCAzNyArKysrKysrKysrKysrKysrKysr KysrKysrKysrLS0tLS0tLS0tDQo+ID4gPiA+ICAxIGZpbGUgY2hhbmdlZCwgMjggaW5zZXJ0aW9u cygrKSwgOSBkZWxldGlvbnMoLSkNCj4gPiA+ID4NCj4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZl cnMvZG1hL2lteC1zZG1hLmMgYi9kcml2ZXJzL2RtYS9pbXgtc2RtYS5jIGluZGV4DQo+ID4gPiA+ IGVkMjI2N2QuLjQ4ZjM3NDkgMTAwNjQ0DQo+ID4gPiA+IC0tLSBhL2RyaXZlcnMvZG1hL2lteC1z ZG1hLmMNCj4gPiA+ID4gKysrIGIvZHJpdmVycy9kbWEvaW14LXNkbWEuYw0KPiA+ID4gPiBAQCAt MTIyMiwxMCArMTIyMiwzNiBAQCBzdGF0aWMgaW50DQo+ID4gPiA+IHNkbWFfYWxsb2NfY2hhbl9y ZXNvdXJjZXMoc3RydWN0IGRtYV9jaGFuICpjaGFuKSAgew0KPiA+ID4gPiAgCXN0cnVjdCBzZG1h X2NoYW5uZWwgKnNkbWFjID0gdG9fc2RtYV9jaGFuKGNoYW4pOw0KPiA+ID4gPiAgCXN0cnVjdCBp bXhfZG1hX2RhdGEgKmRhdGEgPSBjaGFuLT5wcml2YXRlOw0KPiA+ID4gPiArCXN0cnVjdCBpbXhf ZG1hX2RhdGEgZGVmYXVsdF9kYXRhOw0KPiA+ID4gPiAgCWludCBwcmlvLCByZXQ7DQo+ID4gPiA+ DQo+ID4gPiA+IC0JaWYgKCFkYXRhKQ0KPiA+ID4gPiAtCQlyZXR1cm4gLUVJTlZBTDsNCj4gPiA+ ID4gKwlyZXQgPSBjbGtfZW5hYmxlKHNkbWFjLT5zZG1hLT5jbGtfaXBnKTsNCj4gPiA+ID4gKwlp ZiAocmV0KQ0KPiA+ID4gPiArCQlyZXR1cm4gcmV0Ow0KPiA+ID4gPiArCXJldCA9IGNsa19lbmFi bGUoc2RtYWMtPnNkbWEtPmNsa19haGIpOw0KPiA+ID4gPiArCWlmIChyZXQpDQo+ID4gPiA+ICsJ CWdvdG8gZGlzYWJsZV9jbGtfaXBnOw0KPiA+ID4gPiArCS8qDQo+ID4gPiA+ICsJICogZG1hdGVz dChtZW1jcHkpIHdpbGwgbmV2ZXIgY2FsbCBkbWFlbmdpbmVfc2xhdmVfY29uZmlnIGJlZm9yZQ0K PiBwcmVwLA0KPiA+ID4gPiArCSAqIHNvIGpvYnMgaW4gZG1hZW5naW5lX3NsYXZlX2NvbmZpZyBu ZWVkIHRvIGJlIG1vdmVkIGludG8NCj4gc29tZXdoZXJlDQo+ID4gPiA+ICsJICogYmVmb3JlIGRl dmljZV9wcmVwX2RtYV9tZW1jcHkuIEJlc2lkZXMsIGRtYXRlc3QgbmV2ZXIgc2V0dXANCj4gY2hh bg0KPiA+ID4gPiArCSAqIC0+cHJpdmF0ZSBhcyBvdGhlciBjb21tb24gY2FzZXMgbGlrZSB1YXJ0 L2F1ZGlvL3NwaSB3aWxsIHNldHVwDQo+ID4gPiA+ICsJICogY2hhbi0+cHJpdmF0ZSBhbHdheXMu IEhlcmUgY2hlY2sgaXQgdG8ganVkZ2UgaWYgaXQncyBkbWF0ZXN0IGNhc2UNCj4gPiA+ID4gKwkg KiBhbmQgZG8gam9icyBpbiBzbGF2ZV9jb25maWcuDQo+ID4gPiA+ICsJICovDQo+ID4gPiA+ICsJ aWYgKCFkYXRhKSB7DQo+ID4gPiA+ICsJCWRldl93YXJuKHNkbWFjLT5zZG1hLT5kZXYsICJkbWF0 ZXN0IGlzIHJ1bm5pbmc/XG4iKTsNCj4gPiA+DQo+ID4gPiB3aHkgaXMgdGhhdCBhIHdhcm5pbmch DQo+ID4gQ3VycmVudCBTRE1BIGRyaXZlciBhc3N1bWUgZmlsdGVyIGZ1bmN0aW9uIHRvIHNldCBj aGFuLT5wcml2YXRlIHdpdGgNCj4gPiBzcGVjaWZpYyBkYXRhIChzdHJ1Y3QgaW14X2RtYV9kYXRh IGRtYV9kYXRhKWxpa2UgYmVsb3cNCj4gKHNvdW5kL3NvYy9mc2wvZnNsX2FzcmNfZG1hLmMpOg0K PiA+IHN0YXRpYyBib29sIGZpbHRlcihzdHJ1Y3QgZG1hX2NoYW4gKmNoYW4sIHZvaWQgKnBhcmFt KSB7DQo+ID4gICAgICAgICBpZiAoIWlteF9kbWFfaXNfZ2VuZXJhbF9wdXJwb3NlKGNoYW4pKQ0K PiA+ICAgICAgICAgICAgICAgICByZXR1cm4gZmFsc2U7DQo+ID4gICAgICAgICBjaGFuLT5wcml2 YXRlID0gcGFyYW07DQo+ID4gICAgICAgICByZXR1cm4gdHJ1ZTsNCj4gPiB9DQo+ID4NCj4gPiBC dXQgaW4gbWVtY3B5IGNhc2UsIGF0IGxlYXNlIGRtYXRlc3QgY2FzZSwgbm8gY2hhbi0+cHJpdmF0 ZSBzZXQgaW4gaXRzIGZpbHRlcg0KPiBmdW5jdGlvbi4NCj4gPiBTbyBoZXJlIHRha2UgZG1hdGVz dCBhIHNwZWNpYWwgY2FzZSBhbmQgZG8gc29tZSBwcmVwYXJlIGpvYnMgZm9yDQo+ID4gbWVtY3B5 LiBCdXQgaWYgdGhlIFVwcGVyIGRldmljZSBkcml2ZXIgY2FsbCBkbWFfcmVxdWVzdF9jaGFubmVs KCkgd2l0aA0KPiA+IHRoZWlyIHNwZWNpZmljIGZpbHRlciB3aXRob3V0ICdjaGFuLT5wcml2YXRl JyBzZXR0aW5nIGluIHRoZSBmdXR1cmUuDQo+ID4gVGhlIHdhcm5pbmcgbWVzc2FnZSBpcyBhIHVz ZWZ1bCBoaW50IHRvIHRoZW0gdG8gQWRkICdjaGFuLT5wcml2YXRlJyBpbiBmaWx0ZXINCj4gZnVu Y3Rpb24uICBPciBkb2MgaXQgc29tZXdoZXJlPw0KPiANCj4gSW5zdGVhZCBvZiBkb2luZyBoZXVy aXN0aWNzIHRvIGd1ZXNzIHdoZXRoZXIgd2UgYXJlIGRvaW5nIG1lbWNweSB5b3UgY291bGQNCj4g aW5zdGVhZCBtYWtlIG1lbWNweSB0aGUgZGVmYXVsdCB3aGVuIHNsYXZlX2NvbmZpZyBpcyBub3Qg Y2FsbGVkLCBpLmUuIGRyb3AgdGhlDQo+IGlmICghZGF0YSkgY2hlY2sgY29tcGxldGVseS4NClll cywgZm9yIG1lbWNweSBjYXNlLCB0aGF0J3MgYSBnb29kIHdheSwgYnV0IGhvdyB0byB3YXJuaW5n IHRoZSBmdXR1cmUgY2FzZQ0KV2l0aG91dCBzZXR1cCAnY2hhbi0+cHJpdmF0ZScuLi4NCj4gDQo+ ID4gPg0KPiA+ID4gPiArCQlzZG1hYy0+d29yZF9zaXplICA9DQo+IHNkbWFjLT5zZG1hLT5kbWFf ZGV2aWNlLmNvcHlfYWxpZ247DQo+ID4gPiA+ICsJCWRlZmF1bHRfZGF0YS5wcmlvcml0eSA9IDI7 DQo+ID4gPiA+ICsJCWRlZmF1bHRfZGF0YS5wZXJpcGhlcmFsX3R5cGUgPSBJTVhfRE1BVFlQRV9N RU1PUlk7DQo+ID4gPiA+ICsJCWRlZmF1bHRfZGF0YS5kbWFfcmVxdWVzdCA9IDA7DQo+ID4gPiA+ ICsJCWRlZmF1bHRfZGF0YS5kbWFfcmVxdWVzdDIgPSAwOw0KPiA+ID4gPiArCQlkYXRhID0gJmRl ZmF1bHRfZGF0YTsNCj4gPiA+ID4gKw0KPiA+ID4gPiArCQlzZG1hX2NvbmZpZ19vd25lcnNoaXAo c2RtYWMsIGZhbHNlLCB0cnVlLCBmYWxzZSk7DQo+ID4gPiA+ICsJCXNkbWFfZ2V0X3BjKHNkbWFj LCBJTVhfRE1BVFlQRV9NRU1PUlkpOw0KPiA+ID4gPiArCQlzZG1hX2xvYWRfY29udGV4dChzZG1h Yyk7DQo+ID4gPiA+ICsJfQ0KPiA+ID4NCj4gPiA+IHRoaXMgbmVlZHMgdG8gYmUgZGVmYXVsdCBm b3IgbWVtY3B5DQo+IA0KPiBUaGUgcHJvYmxlbSBzZWVtcyB0byBiZSB0aGF0IHdlIGRvIG5vdCBr bm93IHdoZXRoZXIgd2UgYXJlIGRvaW5nIG1lbWNweQ0KPiBvciBub3QuIE5vcm1hbGx5IHdlIGdl dCB0aGUgaW5mb3JtYXRpb24gaG93IGEgY2hhbm5lbCBpcyB0byBiZSBjb25maWd1cmVkIGluDQo+ IGRtYV9kZXZpY2UtPmRldmljZV9jb25maWcsIGJ1dCB0aGlzIGZ1bmN0aW9uIGlzIG5vdCBjYWxs ZWQgaW4gdGhlIG1lbWNweSBjYXNlLg0KPiANCj4gQW4gYWx0ZXJuYXRpdmUgbWlnaHQgYWxzbyBi ZSB0byBkbyB0aGUgc2V0dXAgaW4NCj4gZG1hX2RldmljZS0+ZGV2aWNlX3ByZXBfZG1hX21lbWNw eS4NClllcywgSSd2ZSB0aGluayBhYm91dCBpdCBiZWZvcmUsIGJ1dCBzdWNoIHByZXBhcmUgc3Rl cHMgb25seSBuZWVkZWQgaW4gb25jZSB0aW1lLi4uDQo+IA0KPiBTYXNjaGENCj4gDQo+IC0tDQo+ IFBlbmd1dHJvbml4IGUuSy4gICAgICAgICAgICAgICAgICAgICAgICAgICB8DQo+IHwNCj4gSW5k dXN0cmlhbCBMaW51eCBTb2x1dGlvbnMgICAgICAgICAgICAgICAgIHwNCj4gaHR0cHM6Ly9lbWVh MDEuc2FmZWxpbmtzLnByb3RlY3Rpb24ub3V0bG9vay5jb20vP3VybD1odHRwJTNBJTJGJTJGd3d3 Lg0KPiBwZW5ndXRyb25peC5kZSUyRiZhbXA7ZGF0YT0wMiU3QzAxJTdDeWliaW4uZ29uZyU0MG54 cC5jb20lN0MzZmNmMDMNCj4gZGIxMmY0NDEzOThmYmIwOGQ1ZTZmYjE0MmIlN0M2ODZlYTFkM2Jj MmI0YzZmYTkyY2Q5OWM1YzMwMTYzNSU3QzANCj4gJTdDMCU3QzYzNjY2ODg4ODQxNjMyODk2MCZh bXA7c2RhdGE9RTFEVDFCVzRiNVExVldna01OWnFBMjgNCj4gb0slMkZWVlF2aUM4cUYyJTJCcUcw RmVvJTNEJmFtcDtyZXNlcnZlZD0wICB8DQo+IFBlaW5lciBTdHIuIDYtOCwgMzExMzcgSGlsZGVz aGVpbSwgR2VybWFueSB8IFBob25lOiArNDktNTEyMS0yMDY5MTctMCAgICB8DQo+IEFtdHNnZXJp Y2h0IEhpbGRlc2hlaW0sIEhSQSAyNjg2ICAgICAgICAgICB8IEZheDoNCj4gKzQ5LTUxMjEtMjA2 OTE3LTU1NTUgfA0K -- 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 11-07-18, 08:53, s.hauer@pengutronix.de wrote: > On Wed, Jul 11, 2018 at 06:37:02AM +0000, Robin Gong wrote: > > > > > -----Original Message----- > > > From: Vinod [mailto:vkoul@kernel.org] > > > Sent: 2018年7月10日 23:33 > > > To: Robin Gong <yibin.gong@nxp.com> > > > Cc: dan.j.williams@intel.com; shawnguo@kernel.org; > > > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>; > > > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org; > > > kernel@pengutronix.de; dmaengine@vger.kernel.org; > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest > > > > > > On 11-07-18, 00:23, Robin Gong wrote: > > > > dmatest(memcpy) will never call dmaengine_slave_config before prep, > > > > > > and that should have been a hint to you that you should not expect that > > > > > > > so jobs in dmaengine_slave_config need to be moved into somewhere > > > > before device_prep_dma_memcpy. Besides, dmatest never setup chan > > > > ->private as other common case like uart/audio/spi will always setup > > > > chan->private. Here check it to judge if it's dmatest case and do > > > > jobs in slave_config. > > > > > > and you should not do anything for dmatest. Supporting it means memcpy > > > implementation is not correct :) > > Okay, I will any word about dmatest here since memcpy assume no calling > > slave_config. > > > > > > > > > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > > > > --- > > > > drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++--------- > > > > 1 file changed, 28 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index > > > > ed2267d..48f3749 100644 > > > > --- a/drivers/dma/imx-sdma.c > > > > +++ b/drivers/dma/imx-sdma.c > > > > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct > > > > dma_chan *chan) { > > > > struct sdma_channel *sdmac = to_sdma_chan(chan); > > > > struct imx_dma_data *data = chan->private; > > > > + struct imx_dma_data default_data; > > > > int prio, ret; > > > > > > > > - if (!data) > > > > - return -EINVAL; > > > > + ret = clk_enable(sdmac->sdma->clk_ipg); > > > > + if (ret) > > > > + return ret; > > > > + ret = clk_enable(sdmac->sdma->clk_ahb); > > > > + if (ret) > > > > + goto disable_clk_ipg; > > > > + /* > > > > + * dmatest(memcpy) will never call dmaengine_slave_config before prep, > > > > + * so jobs in dmaengine_slave_config need to be moved into somewhere > > > > + * before device_prep_dma_memcpy. Besides, dmatest never setup chan > > > > + * ->private as other common cases like uart/audio/spi will setup > > > > + * chan->private always. Here check it to judge if it's dmatest case > > > > + * and do jobs in slave_config. > > > > + */ > > > > + if (!data) { > > > > + dev_warn(sdmac->sdma->dev, "dmatest is running?\n"); > > > > > > why is that a warning! > > Current SDMA driver assume filter function to set chan->private with specific data > > (struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c): > > static bool filter(struct dma_chan *chan, void *param) > > { > > if (!imx_dma_is_general_purpose(chan)) > > return false; > > chan->private = param; > > return true; > > } > > > > But in memcpy case, at lease dmatest case, no chan->private set in its filter function. > > So here take dmatest a special case and do some prepare jobs for memcpy. But if the > > Upper device driver call dma_request_channel() with their specific filter without > > 'chan->private' setting in the future. The warning message is a useful hint to them to > > Add 'chan->private' in filter function. Or doc it somewhere? > > Instead of doing heuristics to guess whether we are doing memcpy you > could instead make memcpy the default when slave_config is not called, > i.e. drop the if (!data) check completely. > > > > > > > > + sdmac->word_size = sdmac->sdma->dma_device.copy_align; > > > > + default_data.priority = 2; > > > > + default_data.peripheral_type = IMX_DMATYPE_MEMORY; > > > > + default_data.dma_request = 0; > > > > + default_data.dma_request2 = 0; > > > > + data = &default_data; > > > > + > > > > + sdma_config_ownership(sdmac, false, true, false); > > > > + sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY); > > > > + sdma_load_context(sdmac); > > > > + } > > > > > > this needs to be default for memcpy > > The problem seems to be that we do not know whether we are doing memcpy > or not. Normally we get the information how a channel is to be > configured in dma_device->device_config, but this function is not called > in the memcpy case. Not really true, device_config only provides parameters to be configured for next slave transaction > An alternative might also be to do the setup in dma_device->device_prep_dma_memcpy. Precisely, see how other drivers do this Let's roll back a bit and foresee why is this required. In case of memcpy, you need to tell DMA to do transfer from src to dstn and size. Additional parameters like buswidth etc should be derived for maximum throughput (after all we are dma, people want it to be done fastest) Now for slave, you are interfacing with a peripheral and don't know how that is setup. So you need to match the parameters, otherwise you get overflow or underflow and hence need for device_config Please do not derive additional notions from these, please do not assume anything else, unless provided in documentation :) In doubt, just ask! HTH
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVmlub2QgW21haWx0bzp2 a291bEBrZXJuZWwub3JnXQ0KPiBTZW50OiAyMDE45bm0N+aciDEx5pelIDE1OjE5DQo+IFRvOiBz LmhhdWVyQHBlbmd1dHJvbml4LmRlDQo+IENjOiBSb2JpbiBHb25nIDx5aWJpbi5nb25nQG54cC5j b20+OyBkYW4uai53aWxsaWFtc0BpbnRlbC5jb207DQo+IHNoYXduZ3VvQGtlcm5lbC5vcmc7IEZh YmlvIEVzdGV2YW0gPGZhYmlvLmVzdGV2YW1AbnhwLmNvbT47DQo+IGxpbnV4QGFybWxpbnV4Lm9y Zy51azsgbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOw0KPiBrZXJuZWxAcGVu Z3V0cm9uaXguZGU7IGRtYWVuZ2luZUB2Z2VyLmtlcm5lbC5vcmc7DQo+IGxpbnV4LWtlcm5lbEB2 Z2VyLmtlcm5lbC5vcmc7IGRsLWxpbnV4LWlteCA8bGludXgtaW14QG54cC5jb20+DQo+IFN1Ympl Y3Q6IFJlOiBbUEFUQ0ggdjEgMy80XSBkbWFlbmdpbmU6IGlteC1zZG1hOiBzdXBwb3J0IGRtYXRl c3QNCj4gDQo+IE9uIDExLTA3LTE4LCAwODo1Mywgcy5oYXVlckBwZW5ndXRyb25peC5kZSB3cm90 ZToNCj4gPiBPbiBXZWQsIEp1bCAxMSwgMjAxOCBhdCAwNjozNzowMkFNICswMDAwLCBSb2JpbiBH b25nIHdyb3RlOg0KPiA+ID4NCj4gPiA+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4g PiA+ID4gRnJvbTogVmlub2QgW21haWx0bzp2a291bEBrZXJuZWwub3JnXQ0KPiA+ID4gPiBTZW50 OiAyMDE45bm0N+aciDEw5pelIDIzOjMzDQo+ID4gPiA+IFRvOiBSb2JpbiBHb25nIDx5aWJpbi5n b25nQG54cC5jb20+DQo+ID4gPiA+IENjOiBkYW4uai53aWxsaWFtc0BpbnRlbC5jb207IHNoYXdu Z3VvQGtlcm5lbC5vcmc7DQo+ID4gPiA+IHMuaGF1ZXJAcGVuZ3V0cm9uaXguZGU7IEZhYmlvIEVz dGV2YW0gPGZhYmlvLmVzdGV2YW1AbnhwLmNvbT47DQo+ID4gPiA+IGxpbnV4QGFybWxpbnV4Lm9y Zy51azsgbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOw0KPiA+ID4gPiBrZXJu ZWxAcGVuZ3V0cm9uaXguZGU7IGRtYWVuZ2luZUB2Z2VyLmtlcm5lbC5vcmc7DQo+ID4gPiA+IGxp bnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7IGRsLWxpbnV4LWlteCA8bGludXgtaW14QG54cC5j b20+DQo+ID4gPiA+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjEgMy80XSBkbWFlbmdpbmU6IGlteC1z ZG1hOiBzdXBwb3J0IGRtYXRlc3QNCj4gPiA+ID4NCj4gPiA+ID4gT24gMTEtMDctMTgsIDAwOjIz LCBSb2JpbiBHb25nIHdyb3RlOg0KPiA+ID4gPiA+IGRtYXRlc3QobWVtY3B5KSB3aWxsIG5ldmVy IGNhbGwgZG1hZW5naW5lX3NsYXZlX2NvbmZpZyBiZWZvcmUNCj4gPiA+ID4gPiBwcmVwLA0KPiA+ ID4gPg0KPiA+ID4gPiBhbmQgdGhhdCBzaG91bGQgaGF2ZSBiZWVuIGEgaGludCB0byB5b3UgdGhh dCB5b3Ugc2hvdWxkIG5vdCBleHBlY3QNCj4gPiA+ID4gdGhhdA0KPiA+ID4gPg0KPiA+ID4gPiA+ IHNvIGpvYnMgaW4gZG1hZW5naW5lX3NsYXZlX2NvbmZpZyBuZWVkIHRvIGJlIG1vdmVkIGludG8N Cj4gPiA+ID4gPiBzb21ld2hlcmUgYmVmb3JlIGRldmljZV9wcmVwX2RtYV9tZW1jcHkuIEJlc2lk ZXMsIGRtYXRlc3QgbmV2ZXINCj4gPiA+ID4gPiBzZXR1cCBjaGFuDQo+ID4gPiA+ID4gLT5wcml2 YXRlIGFzIG90aGVyIGNvbW1vbiBjYXNlIGxpa2UgdWFydC9hdWRpby9zcGkgd2lsbCBhbHdheXMN Cj4gPiA+ID4gPiAtPnNldHVwDQo+ID4gPiA+ID4gY2hhbi0+cHJpdmF0ZS4gSGVyZSBjaGVjayBp dCB0byBqdWRnZSBpZiBpdCdzIGRtYXRlc3QgY2FzZSBhbmQNCj4gPiA+ID4gPiBjaGFuLT5kbw0K PiA+ID4gPiA+IGpvYnMgaW4gc2xhdmVfY29uZmlnLg0KPiA+ID4gPg0KPiA+ID4gPiBhbmQgeW91 IHNob3VsZCBub3QgZG8gYW55dGhpbmcgZm9yIGRtYXRlc3QuIFN1cHBvcnRpbmcgaXQgbWVhbnMN Cj4gPiA+ID4gbWVtY3B5IGltcGxlbWVudGF0aW9uIGlzIG5vdCBjb3JyZWN0IDopDQo+ID4gPiBP a2F5LCBJIHdpbGwgYW55IHdvcmQgYWJvdXQgZG1hdGVzdCBoZXJlIHNpbmNlIG1lbWNweSBhc3N1 bWUgbm8NCj4gPiA+IGNhbGxpbmcgc2xhdmVfY29uZmlnLg0KPiA+ID4gPg0KPiA+ID4gPiA+DQo+ ID4gPiA+ID4gU2lnbmVkLW9mZi1ieTogUm9iaW4gR29uZyA8eWliaW4uZ29uZ0BueHAuY29tPg0K PiA+ID4gPiA+IC0tLQ0KPiA+ID4gPiA+ICBkcml2ZXJzL2RtYS9pbXgtc2RtYS5jIHwgMzcNCj4g PiA+ID4gPiArKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tDQo+ID4gPiA+ID4g IDEgZmlsZSBjaGFuZ2VkLCAyOCBpbnNlcnRpb25zKCspLCA5IGRlbGV0aW9ucygtKQ0KPiA+ID4g PiA+DQo+ID4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZG1hL2lteC1zZG1hLmMgYi9kcml2 ZXJzL2RtYS9pbXgtc2RtYS5jDQo+ID4gPiA+ID4gaW5kZXgNCj4gPiA+ID4gPiBlZDIyNjdkLi40 OGYzNzQ5IDEwMDY0NA0KPiA+ID4gPiA+IC0tLSBhL2RyaXZlcnMvZG1hL2lteC1zZG1hLmMNCj4g PiA+ID4gPiArKysgYi9kcml2ZXJzL2RtYS9pbXgtc2RtYS5jDQo+ID4gPiA+ID4gQEAgLTEyMjIs MTAgKzEyMjIsMzYgQEAgc3RhdGljIGludA0KPiA+ID4gPiA+IHNkbWFfYWxsb2NfY2hhbl9yZXNv dXJjZXMoc3RydWN0IGRtYV9jaGFuICpjaGFuKSAgew0KPiA+ID4gPiA+ICAJc3RydWN0IHNkbWFf Y2hhbm5lbCAqc2RtYWMgPSB0b19zZG1hX2NoYW4oY2hhbik7DQo+ID4gPiA+ID4gIAlzdHJ1Y3Qg aW14X2RtYV9kYXRhICpkYXRhID0gY2hhbi0+cHJpdmF0ZTsNCj4gPiA+ID4gPiArCXN0cnVjdCBp bXhfZG1hX2RhdGEgZGVmYXVsdF9kYXRhOw0KPiA+ID4gPiA+ICAJaW50IHByaW8sIHJldDsNCj4g PiA+ID4gPg0KPiA+ID4gPiA+IC0JaWYgKCFkYXRhKQ0KPiA+ID4gPiA+IC0JCXJldHVybiAtRUlO VkFMOw0KPiA+ID4gPiA+ICsJcmV0ID0gY2xrX2VuYWJsZShzZG1hYy0+c2RtYS0+Y2xrX2lwZyk7 DQo+ID4gPiA+ID4gKwlpZiAocmV0KQ0KPiA+ID4gPiA+ICsJCXJldHVybiByZXQ7DQo+ID4gPiA+ ID4gKwlyZXQgPSBjbGtfZW5hYmxlKHNkbWFjLT5zZG1hLT5jbGtfYWhiKTsNCj4gPiA+ID4gPiAr CWlmIChyZXQpDQo+ID4gPiA+ID4gKwkJZ290byBkaXNhYmxlX2Nsa19pcGc7DQo+ID4gPiA+ID4g KwkvKg0KPiA+ID4gPiA+ICsJICogZG1hdGVzdChtZW1jcHkpIHdpbGwgbmV2ZXIgY2FsbCBkbWFl bmdpbmVfc2xhdmVfY29uZmlnIGJlZm9yZQ0KPiBwcmVwLA0KPiA+ID4gPiA+ICsJICogc28gam9i cyBpbiBkbWFlbmdpbmVfc2xhdmVfY29uZmlnIG5lZWQgdG8gYmUgbW92ZWQgaW50bw0KPiBzb21l d2hlcmUNCj4gPiA+ID4gPiArCSAqIGJlZm9yZSBkZXZpY2VfcHJlcF9kbWFfbWVtY3B5LiBCZXNp ZGVzLCBkbWF0ZXN0IG5ldmVyIHNldHVwDQo+IGNoYW4NCj4gPiA+ID4gPiArCSAqIC0+cHJpdmF0 ZSBhcyBvdGhlciBjb21tb24gY2FzZXMgbGlrZSB1YXJ0L2F1ZGlvL3NwaSB3aWxsIHNldHVwDQo+ ID4gPiA+ID4gKwkgKiBjaGFuLT5wcml2YXRlIGFsd2F5cy4gSGVyZSBjaGVjayBpdCB0byBqdWRn ZSBpZiBpdCdzIGRtYXRlc3QgY2FzZQ0KPiA+ID4gPiA+ICsJICogYW5kIGRvIGpvYnMgaW4gc2xh dmVfY29uZmlnLg0KPiA+ID4gPiA+ICsJICovDQo+ID4gPiA+ID4gKwlpZiAoIWRhdGEpIHsNCj4g PiA+ID4gPiArCQlkZXZfd2FybihzZG1hYy0+c2RtYS0+ZGV2LCAiZG1hdGVzdCBpcyBydW5uaW5n P1xuIik7DQo+ID4gPiA+DQo+ID4gPiA+IHdoeSBpcyB0aGF0IGEgd2FybmluZyENCj4gPiA+IEN1 cnJlbnQgU0RNQSBkcml2ZXIgYXNzdW1lIGZpbHRlciBmdW5jdGlvbiB0byBzZXQgY2hhbi0+cHJp dmF0ZSB3aXRoDQo+ID4gPiBzcGVjaWZpYyBkYXRhIChzdHJ1Y3QgaW14X2RtYV9kYXRhIGRtYV9k YXRhKWxpa2UgYmVsb3cNCj4gKHNvdW5kL3NvYy9mc2wvZnNsX2FzcmNfZG1hLmMpOg0KPiA+ID4g c3RhdGljIGJvb2wgZmlsdGVyKHN0cnVjdCBkbWFfY2hhbiAqY2hhbiwgdm9pZCAqcGFyYW0pIHsN Cj4gPiA+ICAgICAgICAgaWYgKCFpbXhfZG1hX2lzX2dlbmVyYWxfcHVycG9zZShjaGFuKSkNCj4g PiA+ICAgICAgICAgICAgICAgICByZXR1cm4gZmFsc2U7DQo+ID4gPiAgICAgICAgIGNoYW4tPnBy aXZhdGUgPSBwYXJhbTsNCj4gPiA+ICAgICAgICAgcmV0dXJuIHRydWU7DQo+ID4gPiB9DQo+ID4g Pg0KPiA+ID4gQnV0IGluIG1lbWNweSBjYXNlLCBhdCBsZWFzZSBkbWF0ZXN0IGNhc2UsIG5vIGNo YW4tPnByaXZhdGUgc2V0IGluIGl0cyBmaWx0ZXINCj4gZnVuY3Rpb24uDQo+ID4gPiBTbyBoZXJl IHRha2UgZG1hdGVzdCBhIHNwZWNpYWwgY2FzZSBhbmQgZG8gc29tZSBwcmVwYXJlIGpvYnMgZm9y DQo+ID4gPiBtZW1jcHkuIEJ1dCBpZiB0aGUgVXBwZXIgZGV2aWNlIGRyaXZlciBjYWxsIGRtYV9y ZXF1ZXN0X2NoYW5uZWwoKQ0KPiA+ID4gd2l0aCB0aGVpciBzcGVjaWZpYyBmaWx0ZXIgd2l0aG91 dCAnY2hhbi0+cHJpdmF0ZScgc2V0dGluZyBpbiB0aGUNCj4gPiA+IGZ1dHVyZS4gVGhlIHdhcm5p bmcgbWVzc2FnZSBpcyBhIHVzZWZ1bCBoaW50IHRvIHRoZW0gdG8gQWRkICdjaGFuLT5wcml2YXRl Jw0KPiBpbiBmaWx0ZXIgZnVuY3Rpb24uICBPciBkb2MgaXQgc29tZXdoZXJlPw0KPiA+DQo+ID4g SW5zdGVhZCBvZiBkb2luZyBoZXVyaXN0aWNzIHRvIGd1ZXNzIHdoZXRoZXIgd2UgYXJlIGRvaW5n IG1lbWNweSB5b3UNCj4gPiBjb3VsZCBpbnN0ZWFkIG1ha2UgbWVtY3B5IHRoZSBkZWZhdWx0IHdo ZW4gc2xhdmVfY29uZmlnIGlzIG5vdCBjYWxsZWQsDQo+ID4gaS5lLiBkcm9wIHRoZSBpZiAoIWRh dGEpIGNoZWNrIGNvbXBsZXRlbHkuDQo+ID4NCj4gPiA+ID4NCj4gPiA+ID4gPiArCQlzZG1hYy0+ d29yZF9zaXplICA9DQo+IHNkbWFjLT5zZG1hLT5kbWFfZGV2aWNlLmNvcHlfYWxpZ247DQo+ID4g PiA+ID4gKwkJZGVmYXVsdF9kYXRhLnByaW9yaXR5ID0gMjsNCj4gPiA+ID4gPiArCQlkZWZhdWx0 X2RhdGEucGVyaXBoZXJhbF90eXBlID0gSU1YX0RNQVRZUEVfTUVNT1JZOw0KPiA+ID4gPiA+ICsJ CWRlZmF1bHRfZGF0YS5kbWFfcmVxdWVzdCA9IDA7DQo+ID4gPiA+ID4gKwkJZGVmYXVsdF9kYXRh LmRtYV9yZXF1ZXN0MiA9IDA7DQo+ID4gPiA+ID4gKwkJZGF0YSA9ICZkZWZhdWx0X2RhdGE7DQo+ ID4gPiA+ID4gKw0KPiA+ID4gPiA+ICsJCXNkbWFfY29uZmlnX293bmVyc2hpcChzZG1hYywgZmFs c2UsIHRydWUsIGZhbHNlKTsNCj4gPiA+ID4gPiArCQlzZG1hX2dldF9wYyhzZG1hYywgSU1YX0RN QVRZUEVfTUVNT1JZKTsNCj4gPiA+ID4gPiArCQlzZG1hX2xvYWRfY29udGV4dChzZG1hYyk7DQo+ ID4gPiA+ID4gKwl9DQo+ID4gPiA+DQo+ID4gPiA+IHRoaXMgbmVlZHMgdG8gYmUgZGVmYXVsdCBm b3IgbWVtY3B5DQo+ID4NCj4gPiBUaGUgcHJvYmxlbSBzZWVtcyB0byBiZSB0aGF0IHdlIGRvIG5v dCBrbm93IHdoZXRoZXIgd2UgYXJlIGRvaW5nDQo+ID4gbWVtY3B5IG9yIG5vdC4gTm9ybWFsbHkg d2UgZ2V0IHRoZSBpbmZvcm1hdGlvbiBob3cgYSBjaGFubmVsIGlzIHRvIGJlDQo+ID4gY29uZmln dXJlZCBpbiBkbWFfZGV2aWNlLT5kZXZpY2VfY29uZmlnLCBidXQgdGhpcyBmdW5jdGlvbiBpcyBu b3QNCj4gPiBjYWxsZWQgaW4gdGhlIG1lbWNweSBjYXNlLg0KPiANCj4gTm90IHJlYWxseSB0cnVl LCBkZXZpY2VfY29uZmlnIG9ubHkgcHJvdmlkZXMgcGFyYW1ldGVycyB0byBiZSBjb25maWd1cmVk IGZvcg0KPiBuZXh0IHNsYXZlIHRyYW5zYWN0aW9uDQo+IA0KPiA+IEFuIGFsdGVybmF0aXZlIG1p Z2h0IGFsc28gYmUgdG8gZG8gdGhlIHNldHVwIGluDQo+IGRtYV9kZXZpY2UtPmRldmljZV9wcmVw X2RtYV9tZW1jcHkuDQo+IA0KPiBQcmVjaXNlbHksIHNlZSBob3cgb3RoZXIgZHJpdmVycyBkbyB0 aGlzDQo+IA0KPiBMZXQncyByb2xsIGJhY2sgYSBiaXQgYW5kIGZvcmVzZWUgd2h5IGlzIHRoaXMg cmVxdWlyZWQuDQo+IA0KPiBJbiBjYXNlIG9mIG1lbWNweSwgeW91IG5lZWQgdG8gdGVsbCBETUEg dG8gZG8gdHJhbnNmZXIgZnJvbSBzcmMgdG8gZHN0biBhbmQNCj4gc2l6ZS4gQWRkaXRpb25hbCBw YXJhbWV0ZXJzIGxpa2UgYnVzd2lkdGggZXRjIHNob3VsZCBiZSBkZXJpdmVkIGZvciBtYXhpbXVt DQo+IHRocm91Z2hwdXQgKGFmdGVyIGFsbCB3ZSBhcmUgZG1hLCBwZW9wbGUgd2FudCBpdCB0byBi ZSBkb25lDQo+IGZhc3Rlc3QpDQo+IA0KPiBOb3cgZm9yIHNsYXZlLCB5b3UgYXJlIGludGVyZmFj aW5nIHdpdGggYSBwZXJpcGhlcmFsIGFuZCBkb24ndCBrbm93IGhvdyB0aGF0IGlzDQo+IHNldHVw LiBTbyB5b3UgbmVlZCB0byBtYXRjaCB0aGUgcGFyYW1ldGVycywgb3RoZXJ3aXNlIHlvdSBnZXQg b3ZlcmZsb3cgb3INCj4gdW5kZXJmbG93IGFuZCBoZW5jZSBuZWVkIGZvciBkZXZpY2VfY29uZmln DQo+IA0KPiBQbGVhc2UgZG8gbm90IGRlcml2ZSBhZGRpdGlvbmFsIG5vdGlvbnMgZnJvbSB0aGVz ZSwgcGxlYXNlIGRvIG5vdCBhc3N1bWUNCj4gYW55dGhpbmcgZWxzZSwgdW5sZXNzIHByb3ZpZGVk IGluIGRvY3VtZW50YXRpb24gOikNCkkgd2lsbCBtb3ZlIHN1Y2ggcHJlcGFyZSBqb2JzIGZyb20g c2xhdmVfY29uZmlnIHRvIGRldmljZV9wcmVwX2RtYV9tZW1jcHkNCkluc3RlYWQgb2YgZGV2aWNl X2FsbG9jX2NoYW5fcmVzb3VyY2VzIGFzIEkgZGlkIGluIHYxLCB0aHVzIHdlIGhhdmUgbm8gJ2No YW4tPnByaXZhdGUnDQppc3N1ZSwganVzdCBsaWtlIGRyaXZlcnMvZG1hL3N0bTMyLW1kbWEuYy4g VGhlIG9ubHkgbGltaXRhdGlvbiBpcyB0aG9zZSBwcmVwYXJlIGpvYnMNCihzb21lIHJlZ2lzdGVy IHNldHRpbmcpIHdpbGwgYmUgZG9uZSBldmVyeSB0aW1lIG1lbWNweSBpbnN0ZWFkIG9mIG9ubHkg b25lIHRpbWUgaW4gc2xhdmVfY29uZmlnDQpvciB2MSBjYXNlLiBJcyB0aGF0IG9rPw0KPiANCj4g SW4gZG91YnQsIGp1c3QgYXNrIQ0KPiANCj4gSFRIDQo+IC0tDQo+IH5WaW5vZA0K -- 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 11-07-18, 08:16, Robin Gong wrote: > > > The problem seems to be that we do not know whether we are doing > > > memcpy or not. Normally we get the information how a channel is to be > > > configured in dma_device->device_config, but this function is not > > > called in the memcpy case. > > > > Not really true, device_config only provides parameters to be configured for > > next slave transaction > > > > > An alternative might also be to do the setup in > > dma_device->device_prep_dma_memcpy. > > > > Precisely, see how other drivers do this > > > > Let's roll back a bit and foresee why is this required. > > > > In case of memcpy, you need to tell DMA to do transfer from src to dstn and > > size. Additional parameters like buswidth etc should be derived for maximum > > throughput (after all we are dma, people want it to be done > > fastest) > > > > Now for slave, you are interfacing with a peripheral and don't know how that is > > setup. So you need to match the parameters, otherwise you get overflow or > > underflow and hence need for device_config > > > > Please do not derive additional notions from these, please do not assume > > anything else, unless provided in documentation :) > I will move such prepare jobs from slave_config to device_prep_dma_memcpy > Instead of device_alloc_chan_resources as I did in v1, thus we have no 'chan->private' > issue, just like drivers/dma/stm32-mdma.c. The only limitation is those prepare jobs > (some register setting) will be done every time memcpy instead of only one time in slave_config > or v1 case. Is that ok? sounds fine to me
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index ed2267d..48f3749 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan) { struct sdma_channel *sdmac = to_sdma_chan(chan); struct imx_dma_data *data = chan->private; + struct imx_dma_data default_data; int prio, ret; - if (!data) - return -EINVAL; + ret = clk_enable(sdmac->sdma->clk_ipg); + if (ret) + return ret; + ret = clk_enable(sdmac->sdma->clk_ahb); + if (ret) + goto disable_clk_ipg; + /* + * dmatest(memcpy) will never call dmaengine_slave_config before prep, + * so jobs in dmaengine_slave_config need to be moved into somewhere + * before device_prep_dma_memcpy. Besides, dmatest never setup chan + * ->private as other common cases like uart/audio/spi will setup + * chan->private always. Here check it to judge if it's dmatest case + * and do jobs in slave_config. + */ + if (!data) { + dev_warn(sdmac->sdma->dev, "dmatest is running?\n"); + sdmac->word_size = sdmac->sdma->dma_device.copy_align; + default_data.priority = 2; + default_data.peripheral_type = IMX_DMATYPE_MEMORY; + default_data.dma_request = 0; + default_data.dma_request2 = 0; + data = &default_data; + + sdma_config_ownership(sdmac, false, true, false); + sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY); + sdma_load_context(sdmac); + } switch (data->priority) { case DMA_PRIO_HIGH: @@ -1244,13 +1270,6 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan) sdmac->event_id0 = data->dma_request; sdmac->event_id1 = data->dma_request2; - ret = clk_enable(sdmac->sdma->clk_ipg); - if (ret) - return ret; - ret = clk_enable(sdmac->sdma->clk_ahb); - if (ret) - goto disable_clk_ipg; - ret = sdma_set_channel_priority(sdmac, prio); if (ret) goto disable_clk_ahb;
dmatest(memcpy) will never call dmaengine_slave_config before prep, so jobs in dmaengine_slave_config need to be moved into somewhere before device_prep_dma_memcpy. Besides, dmatest never setup chan ->private as other common case like uart/audio/spi will always setup chan->private. Here check it to judge if it's dmatest case and do jobs in slave_config. Signed-off-by: Robin Gong <yibin.gong@nxp.com> --- drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-)