[PATCH v2 0/2] Correct Tegra20 FUSE driver DMA usage

[PATCH v2 0/2] Correct Tegra20 FUSE driver DMA usage

[Dmitry Osipenko]

Change log:

v2:
	- Dropped DT patch as turned out it was incorrect and made things
	  work by luck.

	- Now FUSE driver requests DMA channel from the APB DMA driver
	  utilizing DMA channel filter.

	- This patchset now depends on the APB DMA driver patch that allows
	  DMA client to issue a non-flow controlled transfers. I haven't
	  included that patch to this patchset since DMA usage is broken
	  anyway right now. It will work once this patchset and APBDMA
	  patch get applied.

Dmitry Osipenko (2):
  soc/tegra: fuse: Fix reading registers using DMA on Tegra20
  soc/tegra: fuse: Explicitly request DMA channel from APB DMA driver

 drivers/soc/tegra/fuse/fuse-tegra.c   |  1 +
 drivers/soc/tegra/fuse/fuse-tegra20.c | 15 +++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.14.1

[PATCH v2 1/2] soc/tegra: fuse: Fix reading registers using DMA on Tegra20

[Dmitry Osipenko]

DMA config is incorrect, because of it DMA transfer is never issued and
tegra20_fuse_read() always returns 0x0.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/soc/tegra/fuse/fuse-tegra.c   | 1 +
 drivers/soc/tegra/fuse/fuse-tegra20.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index b7c552e3133c..73a3a2c74021 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -132,6 +132,7 @@ static int tegra_fuse_probe(struct platform_device *pdev)
 
 	/* take over the memory region from the early initialization */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	fuse->phys = res->start;
 	fuse->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(fuse->base))
 		return PTR_ERR(fuse->base);
diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c b/drivers/soc/tegra/fuse/fuse-tegra20.c
index 294413a969a0..a33f48c06771 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra20.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
@@ -59,7 +59,7 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse, unsigned int offset)
 
 	mutex_lock(&fuse->apbdma.lock);
 
-	fuse->apbdma.config.src_addr = fuse->apbdma.phys + FUSE_BEGIN + offset;
+	fuse->apbdma.config.src_addr = fuse->phys + FUSE_BEGIN + offset;
 
 	err = dmaengine_slave_config(fuse->apbdma.chan, &fuse->apbdma.config);
 	if (err)
@@ -119,6 +119,7 @@ static int tegra20_fuse_probe(struct tegra_fuse *fuse)
 	fuse->apbdma.config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	fuse->apbdma.config.src_maxburst = 1;
 	fuse->apbdma.config.dst_maxburst = 1;
+	fuse->apbdma.config.direction = DMA_DEV_TO_MEM;
 
 	init_completion(&fuse->apbdma.wait);
 	mutex_init(&fuse->apbdma.lock);
-- 
2.14.1

Re: [PATCH v2 1/2] soc/tegra: fuse: Fix reading registers using DMA on Tegra20

[Jon Hunter]

On 03/10/17 23:35, Dmitry Osipenko wrote:
> DMA config is incorrect, because of it DMA transfer is never issued and
> tegra20_fuse_read() always returns 0x0.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/soc/tegra/fuse/fuse-tegra.c   | 1 +
>  drivers/soc/tegra/fuse/fuse-tegra20.c | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
> index b7c552e3133c..73a3a2c74021 100644
> --- a/drivers/soc/tegra/fuse/fuse-tegra.c
> +++ b/drivers/soc/tegra/fuse/fuse-tegra.c
> @@ -132,6 +132,7 @@ static int tegra_fuse_probe(struct platform_device *pdev)
>  
>  	/* take over the memory region from the early initialization */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	fuse->phys = res->start;
>  	fuse->base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(fuse->base))
>  		return PTR_ERR(fuse->base);
> diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c b/drivers/soc/tegra/fuse/fuse-tegra20.c
> index 294413a969a0..a33f48c06771 100644
> --- a/drivers/soc/tegra/fuse/fuse-tegra20.c
> +++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
> @@ -59,7 +59,7 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse, unsigned int offset)
>  
>  	mutex_lock(&fuse->apbdma.lock);
>  
> -	fuse->apbdma.config.src_addr = fuse->apbdma.phys + FUSE_BEGIN + offset;
> +	fuse->apbdma.config.src_addr = fuse->phys + FUSE_BEGIN + offset;
>  
>  	err = dmaengine_slave_config(fuse->apbdma.chan, &fuse->apbdma.config);
>  	if (err)
> @@ -119,6 +119,7 @@ static int tegra20_fuse_probe(struct tegra_fuse *fuse)
>  	fuse->apbdma.config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>  	fuse->apbdma.config.src_maxburst = 1;
>  	fuse->apbdma.config.dst_maxburst = 1;
> +	fuse->apbdma.config.direction = DMA_DEV_TO_MEM;
>  
>  	init_completion(&fuse->apbdma.wait);
>  	mutex_init(&fuse->apbdma.lock);

Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

Re: [PATCH v2 1/2] soc/tegra: fuse: Fix reading registers using DMA on Tegra20

[Jon Hunter]

On 04/10/17 11:47, Jon Hunter wrote:
> 
> On 03/10/17 23:35, Dmitry Osipenko wrote:
>> DMA config is incorrect, because of it DMA transfer is never issued and
>> tegra20_fuse_read() always returns 0x0.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/soc/tegra/fuse/fuse-tegra.c   | 1 +
>>  drivers/soc/tegra/fuse/fuse-tegra20.c | 3 ++-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
>> index b7c552e3133c..73a3a2c74021 100644
>> --- a/drivers/soc/tegra/fuse/fuse-tegra.c
>> +++ b/drivers/soc/tegra/fuse/fuse-tegra.c
>> @@ -132,6 +132,7 @@ static int tegra_fuse_probe(struct platform_device *pdev)
>>  
>>  	/* take over the memory region from the early initialization */
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	fuse->phys = res->start;
>>  	fuse->base = devm_ioremap_resource(&pdev->dev, res);
>>  	if (IS_ERR(fuse->base))
>>  		return PTR_ERR(fuse->base);
>> diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c b/drivers/soc/tegra/fuse/fuse-tegra20.c
>> index 294413a969a0..a33f48c06771 100644
>> --- a/drivers/soc/tegra/fuse/fuse-tegra20.c
>> +++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
>> @@ -59,7 +59,7 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse, unsigned int offset)
>>  
>>  	mutex_lock(&fuse->apbdma.lock);
>>  
>> -	fuse->apbdma.config.src_addr = fuse->apbdma.phys + FUSE_BEGIN + offset;
>> +	fuse->apbdma.config.src_addr = fuse->phys + FUSE_BEGIN + offset;
>>  
>>  	err = dmaengine_slave_config(fuse->apbdma.chan, &fuse->apbdma.config);
>>  	if (err)
>> @@ -119,6 +119,7 @@ static int tegra20_fuse_probe(struct tegra_fuse *fuse)
>>  	fuse->apbdma.config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>>  	fuse->apbdma.config.src_maxburst = 1;
>>  	fuse->apbdma.config.dst_maxburst = 1;
>> +	fuse->apbdma.config.direction = DMA_DEV_TO_MEM;
>>  
>>  	init_completion(&fuse->apbdma.wait);
>>  	mutex_init(&fuse->apbdma.lock);
> 
> Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

BTW, if you do rev this series, then maybe it is worth explicitly
setting 'fuse->apbdma.config.device_fc = false'.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v2 1/2] soc/tegra: fuse: Fix reading registers using DMA on Tegra20

[Dmitry Osipenko]

On 05.10.2017 15:22, Jon Hunter wrote:
> 
> On 04/10/17 11:47, Jon Hunter wrote:
>>
>> On 03/10/17 23:35, Dmitry Osipenko wrote:
>>> DMA config is incorrect, because of it DMA transfer is never issued and
>>> tegra20_fuse_read() always returns 0x0.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  drivers/soc/tegra/fuse/fuse-tegra.c   | 1 +
>>>  drivers/soc/tegra/fuse/fuse-tegra20.c | 3 ++-
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
>>> index b7c552e3133c..73a3a2c74021 100644
>>> --- a/drivers/soc/tegra/fuse/fuse-tegra.c
>>> +++ b/drivers/soc/tegra/fuse/fuse-tegra.c
>>> @@ -132,6 +132,7 @@ static int tegra_fuse_probe(struct platform_device *pdev)
>>>  
>>>  	/* take over the memory region from the early initialization */
>>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	fuse->phys = res->start;
>>>  	fuse->base = devm_ioremap_resource(&pdev->dev, res);
>>>  	if (IS_ERR(fuse->base))
>>>  		return PTR_ERR(fuse->base);
>>> diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c b/drivers/soc/tegra/fuse/fuse-tegra20.c
>>> index 294413a969a0..a33f48c06771 100644
>>> --- a/drivers/soc/tegra/fuse/fuse-tegra20.c
>>> +++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
>>> @@ -59,7 +59,7 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse, unsigned int offset)
>>>  
>>>  	mutex_lock(&fuse->apbdma.lock);
>>>  
>>> -	fuse->apbdma.config.src_addr = fuse->apbdma.phys + FUSE_BEGIN + offset;
>>> +	fuse->apbdma.config.src_addr = fuse->phys + FUSE_BEGIN + offset;
>>>  
>>>  	err = dmaengine_slave_config(fuse->apbdma.chan, &fuse->apbdma.config);
>>>  	if (err)
>>> @@ -119,6 +119,7 @@ static int tegra20_fuse_probe(struct tegra_fuse *fuse)
>>>  	fuse->apbdma.config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>>>  	fuse->apbdma.config.src_maxburst = 1;
>>>  	fuse->apbdma.config.dst_maxburst = 1;
>>> +	fuse->apbdma.config.direction = DMA_DEV_TO_MEM;
>>>  
>>>  	init_completion(&fuse->apbdma.wait);
>>>  	mutex_init(&fuse->apbdma.lock);
>>
>> Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> BTW, if you do rev this series, then maybe it is worth explicitly
> setting 'fuse->apbdma.config.device_fc = false'.
> 

I'll make v3 for sure. Setting device_fc isn't strictly needed, but it is good
to have for expressiveness. I'll add it to v3, thanks.

[PATCH v2 2/2] soc/tegra: fuse: Explicitly request DMA channel from APB DMA driver

[Dmitry Osipenko]

Currently fuse driver requests DMA channel from an arbitrary DMA device,
it is not a problem since there is only one DMA provider for Tegra20 yet,
but it will become troublesome once another provider will be added.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/soc/tegra/fuse/fuse-tegra20.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c b/drivers/soc/tegra/fuse/fuse-tegra20.c
index a33f48c06771..e9e127a24c70 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra20.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
@@ -37,6 +37,11 @@
 #define FUSE_UID_LOW	0x08
 #define FUSE_UID_HIGH	0x0c
 
+static const struct of_device_id apbdma_of_match[] = {
+	{ .compatible = "nvidia,tegra20-apbdma" },
+	{ },
+};
+
 static u32 tegra20_fuse_read_early(struct tegra_fuse *fuse, unsigned int offset)
 {
 	return readl_relaxed(fuse->base + FUSE_BEGIN + offset);
@@ -96,6 +101,11 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse, unsigned int offset)
 	return value;
 }
 
+static bool dma_filter(struct dma_chan *chan, void *filter_param)
+{
+	return of_match_device(apbdma_of_match, chan->device->dev) != NULL;
+}
+
 static int tegra20_fuse_probe(struct tegra_fuse *fuse)
 {
 	dma_cap_mask_t mask;
@@ -103,7 +113,7 @@ static int tegra20_fuse_probe(struct tegra_fuse *fuse)
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_SLAVE, mask);
 
-	fuse->apbdma.chan = dma_request_channel(mask, NULL, NULL);
+	fuse->apbdma.chan = __dma_request_channel(&mask, dma_filter, NULL);
 	if (!fuse->apbdma.chan)
 		return -EPROBE_DEFER;
 
-- 
2.14.1

Re: [PATCH v2 2/2] soc/tegra: fuse: Explicitly request DMA channel from APB DMA driver

[Jon Hunter]

On 03/10/17 23:35, Dmitry Osipenko wrote:
> Currently fuse driver requests DMA channel from an arbitrary DMA device,
> it is not a problem since there is only one DMA provider for Tegra20 yet,
> but it will become troublesome once another provider will be added.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/soc/tegra/fuse/fuse-tegra20.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c b/drivers/soc/tegra/fuse/fuse-tegra20.c
> index a33f48c06771..e9e127a24c70 100644
> --- a/drivers/soc/tegra/fuse/fuse-tegra20.c
> +++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
> @@ -37,6 +37,11 @@
>  #define FUSE_UID_LOW	0x08
>  #define FUSE_UID_HIGH	0x0c
>  
> +static const struct of_device_id apbdma_of_match[] = {
> +	{ .compatible = "nvidia,tegra20-apbdma" },
> +	{ },
> +};
> +
>  static u32 tegra20_fuse_read_early(struct tegra_fuse *fuse, unsigned int offset)
>  {
>  	return readl_relaxed(fuse->base + FUSE_BEGIN + offset);
> @@ -96,6 +101,11 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse, unsigned int offset)
>  	return value;
>  }
>  
> +static bool dma_filter(struct dma_chan *chan, void *filter_param)
> +{
> +	return of_match_device(apbdma_of_match, chan->device->dev) != NULL;

Is it simpler just to use of_device_is_compatible() here?

Cheers
Jon

-- 
nvpublic

Re: [PATCH v2 2/2] soc/tegra: fuse: Explicitly request DMA channel from APB DMA driver

[Dmitry Osipenko]

On 04.10.2017 13:48, Jon Hunter wrote:
> 
> 
> On 03/10/17 23:35, Dmitry Osipenko wrote:
>> Currently fuse driver requests DMA channel from an arbitrary DMA device,
>> it is not a problem since there is only one DMA provider for Tegra20 yet,
>> but it will become troublesome once another provider will be added.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/soc/tegra/fuse/fuse-tegra20.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/tegra/fuse/fuse-tegra20.c b/drivers/soc/tegra/fuse/fuse-tegra20.c
>> index a33f48c06771..e9e127a24c70 100644
>> --- a/drivers/soc/tegra/fuse/fuse-tegra20.c
>> +++ b/drivers/soc/tegra/fuse/fuse-tegra20.c
>> @@ -37,6 +37,11 @@
>>  #define FUSE_UID_LOW	0x08
>>  #define FUSE_UID_HIGH	0x0c
>>  
>> +static const struct of_device_id apbdma_of_match[] = {
>> +	{ .compatible = "nvidia,tegra20-apbdma" },
>> +	{ },
>> +};
>> +
>>  static u32 tegra20_fuse_read_early(struct tegra_fuse *fuse, unsigned int offset)
>>  {
>>  	return readl_relaxed(fuse->base + FUSE_BEGIN + offset);
>> @@ -96,6 +101,11 @@ static u32 tegra20_fuse_read(struct tegra_fuse *fuse, unsigned int offset)
>>  	return value;
>>  }
>>  
>> +static bool dma_filter(struct dma_chan *chan, void *filter_param)
>> +{
>> +	return of_match_device(apbdma_of_match, chan->device->dev) != NULL;
> 
> Is it simpler just to use of_device_is_compatible() here?
> 

Good point! I haven't noticed that function in the OF API.