* [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry
2017-09-25 23:22 [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver Dmitry Osipenko
@ 2017-09-25 23:22 ` Dmitry Osipenko
2017-09-26 9:56 ` Peter De Schrijver
2017-09-25 23:22 ` [PATCH v1 2/5] clk: tegra: Bump SCLK clock rate to 216MHz on Tegra20 Dmitry Osipenko
` (3 subsequent siblings)
4 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-25 23:22 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
AHB DMA presents on Tegra20/30. Add missing entries, so that driver
for AHB DMA could be implemented.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/clk/tegra/clk-id.h | 1 +
drivers/clk/tegra/clk-tegra-periph.c | 1 +
drivers/clk/tegra/clk-tegra20.c | 6 ++++++
drivers/clk/tegra/clk-tegra30.c | 2 ++
4 files changed, 10 insertions(+)
diff --git a/drivers/clk/tegra/clk-id.h b/drivers/clk/tegra/clk-id.h
index 689f344377a7..c1661b47bbda 100644
--- a/drivers/clk/tegra/clk-id.h
+++ b/drivers/clk/tegra/clk-id.h
@@ -12,6 +12,7 @@ enum clk_id {
tegra_clk_amx,
tegra_clk_amx1,
tegra_clk_apb2ape,
+ tegra_clk_ahbdma,
tegra_clk_apbdma,
tegra_clk_apbif,
tegra_clk_ape,
diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
index 848255cc0209..95a3d8c95f06 100644
--- a/drivers/clk/tegra/clk-tegra-periph.c
+++ b/drivers/clk/tegra/clk-tegra-periph.c
@@ -823,6 +823,7 @@ static struct tegra_periph_init_data gate_clks[] = {
GATE("timer", "clk_m", 5, 0, tegra_clk_timer, CLK_IS_CRITICAL),
GATE("isp", "clk_m", 23, 0, tegra_clk_isp, 0),
GATE("vcp", "clk_m", 29, 0, tegra_clk_vcp, 0),
+ GATE("ahbdma", "clk_m", 33, 0, tegra_clk_ahbdma, 0),
GATE("apbdma", "clk_m", 34, 0, tegra_clk_apbdma, 0),
GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 837e5cbd60e9..e76c0d292ca7 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -449,6 +449,7 @@ static struct tegra_devclk devclks[] __initdata = {
{ .con_id = "audio", .dt_id = TEGRA20_CLK_AUDIO },
{ .con_id = "audio_2x", .dt_id = TEGRA20_CLK_AUDIO_2X },
{ .dev_id = "tegra20-ac97", .dt_id = TEGRA20_CLK_AC97 },
+ { .dev_id = "tegra-ahbdma", .dt_id = TEGRA20_CLK_AHBDMA },
{ .dev_id = "tegra-apbdma", .dt_id = TEGRA20_CLK_APBDMA },
{ .dev_id = "rtc-tegra", .dt_id = TEGRA20_CLK_RTC },
{ .dev_id = "timer", .dt_id = TEGRA20_CLK_TIMER },
@@ -806,6 +807,11 @@ static void __init tegra20_periph_clk_init(void)
clk_base, 0, 3, periph_clk_enb_refcnt);
clks[TEGRA20_CLK_AC97] = clk;
+ /* ahbdma */
+ clk = tegra_clk_register_periph_gate("ahbdma", "hclk", 0, clk_base,
+ 0, 33, periph_clk_enb_refcnt);
+ clks[TEGRA20_CLK_AHBDMA] = clk;
+
/* apbdma */
clk = tegra_clk_register_periph_gate("apbdma", "pclk", 0, clk_base,
0, 34, periph_clk_enb_refcnt);
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index a2d163f759b4..e99701557f29 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -612,6 +612,7 @@ static struct tegra_devclk devclks[] __initdata = {
{ .con_id = "fuse_burn", .dev_id = "fuse-tegra", .dt_id = TEGRA30_CLK_FUSE_BURN },
{ .con_id = "apbif", .dev_id = "tegra30-ahub", .dt_id = TEGRA30_CLK_APBIF },
{ .con_id = "hda2hdmi", .dev_id = "tegra30-hda", .dt_id = TEGRA30_CLK_HDA2HDMI },
+ { .dev_id = "tegra-ahbdma", .dt_id = TEGRA30_CLK_AHBDMA },
{ .dev_id = "tegra-apbdma", .dt_id = TEGRA30_CLK_APBDMA },
{ .dev_id = "rtc-tegra", .dt_id = TEGRA30_CLK_RTC },
{ .dev_id = "timer", .dt_id = TEGRA30_CLK_TIMER },
@@ -788,6 +789,7 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = {
[tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3, .present = true },
[tegra_clk_disp1] = { .dt_id = TEGRA30_CLK_DISP1, .present = true },
[tegra_clk_disp2] = { .dt_id = TEGRA30_CLK_DISP2, .present = true },
+ [tegra_clk_ahbdma] = { .dt_id = TEGRA30_CLK_AHBDMA, .present = true },
[tegra_clk_apbdma] = { .dt_id = TEGRA30_CLK_APBDMA, .present = true },
[tegra_clk_rtc] = { .dt_id = TEGRA30_CLK_RTC, .present = true },
[tegra_clk_timer] = { .dt_id = TEGRA30_CLK_TIMER, .present = true },
--
2.14.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry
2017-09-25 23:22 ` [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry Dmitry Osipenko
@ 2017-09-26 9:56 ` Peter De Schrijver
2017-09-26 14:46 ` Dmitry Osipenko
0 siblings, 1 reply; 45+ messages in thread
From: Peter De Schrijver @ 2017-09-26 9:56 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul, linux-tegra, devicetree, dmaengine, linux-clk,
linux-kernel
On Tue, Sep 26, 2017 at 02:22:02AM +0300, Dmitry Osipenko wrote:
> AHB DMA presents on Tegra20/30. Add missing entries, so that driver
> for AHB DMA could be implemented.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/clk/tegra/clk-id.h | 1 +
> drivers/clk/tegra/clk-tegra-periph.c | 1 +
> drivers/clk/tegra/clk-tegra20.c | 6 ++++++
> drivers/clk/tegra/clk-tegra30.c | 2 ++
> 4 files changed, 10 insertions(+)
>
> diff --git a/drivers/clk/tegra/clk-id.h b/drivers/clk/tegra/clk-id.h
> index 689f344377a7..c1661b47bbda 100644
> --- a/drivers/clk/tegra/clk-id.h
> +++ b/drivers/clk/tegra/clk-id.h
> @@ -12,6 +12,7 @@ enum clk_id {
> tegra_clk_amx,
> tegra_clk_amx1,
> tegra_clk_apb2ape,
> + tegra_clk_ahbdma,
> tegra_clk_apbdma,
> tegra_clk_apbif,
> tegra_clk_ape,
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
> index 848255cc0209..95a3d8c95f06 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -823,6 +823,7 @@ static struct tegra_periph_init_data gate_clks[] = {
> GATE("timer", "clk_m", 5, 0, tegra_clk_timer, CLK_IS_CRITICAL),
> GATE("isp", "clk_m", 23, 0, tegra_clk_isp, 0),
> GATE("vcp", "clk_m", 29, 0, tegra_clk_vcp, 0),
> + GATE("ahbdma", "clk_m", 33, 0, tegra_clk_ahbdma, 0),
Parent for this should be hclk on Tegra30 and later chips as well..
> GATE("apbdma", "clk_m", 34, 0, tegra_clk_apbdma, 0),
> GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
> GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 837e5cbd60e9..e76c0d292ca7 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -449,6 +449,7 @@ static struct tegra_devclk devclks[] __initdata = {
> { .con_id = "audio", .dt_id = TEGRA20_CLK_AUDIO },
> { .con_id = "audio_2x", .dt_id = TEGRA20_CLK_AUDIO_2X },
> { .dev_id = "tegra20-ac97", .dt_id = TEGRA20_CLK_AC97 },
> + { .dev_id = "tegra-ahbdma", .dt_id = TEGRA20_CLK_AHBDMA },
This isn't needed if you use DT bindings to get the clock handle.
> { .dev_id = "tegra-apbdma", .dt_id = TEGRA20_CLK_APBDMA },
> { .dev_id = "rtc-tegra", .dt_id = TEGRA20_CLK_RTC },
> { .dev_id = "timer", .dt_id = TEGRA20_CLK_TIMER },
> @@ -806,6 +807,11 @@ static void __init tegra20_periph_clk_init(void)
> clk_base, 0, 3, periph_clk_enb_refcnt);
> clks[TEGRA20_CLK_AC97] = clk;
>
> + /* ahbdma */
> + clk = tegra_clk_register_periph_gate("ahbdma", "hclk", 0, clk_base,
> + 0, 33, periph_clk_enb_refcnt);
> + clks[TEGRA20_CLK_AHBDMA] = clk;
> +
You can use the generic definition here if you correct the entry above.
> /* apbdma */
> clk = tegra_clk_register_periph_gate("apbdma", "pclk", 0, clk_base,
> 0, 34, periph_clk_enb_refcnt);
> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
> index a2d163f759b4..e99701557f29 100644
> --- a/drivers/clk/tegra/clk-tegra30.c
> +++ b/drivers/clk/tegra/clk-tegra30.c
> @@ -612,6 +612,7 @@ static struct tegra_devclk devclks[] __initdata = {
> { .con_id = "fuse_burn", .dev_id = "fuse-tegra", .dt_id = TEGRA30_CLK_FUSE_BURN },
> { .con_id = "apbif", .dev_id = "tegra30-ahub", .dt_id = TEGRA30_CLK_APBIF },
> { .con_id = "hda2hdmi", .dev_id = "tegra30-hda", .dt_id = TEGRA30_CLK_HDA2HDMI },
> + { .dev_id = "tegra-ahbdma", .dt_id = TEGRA30_CLK_AHBDMA },
Same as for Tegra20.
> { .dev_id = "tegra-apbdma", .dt_id = TEGRA30_CLK_APBDMA },
> { .dev_id = "rtc-tegra", .dt_id = TEGRA30_CLK_RTC },
> { .dev_id = "timer", .dt_id = TEGRA30_CLK_TIMER },
> @@ -788,6 +789,7 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = {
> [tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3, .present = true },
> [tegra_clk_disp1] = { .dt_id = TEGRA30_CLK_DISP1, .present = true },
> [tegra_clk_disp2] = { .dt_id = TEGRA30_CLK_DISP2, .present = true },
> + [tegra_clk_ahbdma] = { .dt_id = TEGRA30_CLK_AHBDMA, .present = true },
> [tegra_clk_apbdma] = { .dt_id = TEGRA30_CLK_APBDMA, .present = true },
> [tegra_clk_rtc] = { .dt_id = TEGRA30_CLK_RTC, .present = true },
> [tegra_clk_timer] = { .dt_id = TEGRA30_CLK_TIMER, .present = true },
Cheers,
Peter.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry
2017-09-26 9:56 ` Peter De Schrijver
@ 2017-09-26 14:46 ` Dmitry Osipenko
2017-09-27 8:36 ` Peter De Schrijver
0 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-26 14:46 UTC (permalink / raw)
To: Peter De Schrijver
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul, linux-tegra, devicetree, dmaengine, linux-clk,
linux-kernel
On 26.09.2017 12:56, Peter De Schrijver wrote:
> On Tue, Sep 26, 2017 at 02:22:02AM +0300, Dmitry Osipenko wrote:
>> AHB DMA presents on Tegra20/30. Add missing entries, so that driver
>> for AHB DMA could be implemented.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> drivers/clk/tegra/clk-id.h | 1 +
>> drivers/clk/tegra/clk-tegra-periph.c | 1 +
>> drivers/clk/tegra/clk-tegra20.c | 6 ++++++
>> drivers/clk/tegra/clk-tegra30.c | 2 ++
>> 4 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-id.h b/drivers/clk/tegra/clk-id.h
>> index 689f344377a7..c1661b47bbda 100644
>> --- a/drivers/clk/tegra/clk-id.h
>> +++ b/drivers/clk/tegra/clk-id.h
>> @@ -12,6 +12,7 @@ enum clk_id {
>> tegra_clk_amx,
>> tegra_clk_amx1,
>> tegra_clk_apb2ape,
>> + tegra_clk_ahbdma,
>> tegra_clk_apbdma,
>> tegra_clk_apbif,
>> tegra_clk_ape,
>> diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
>> index 848255cc0209..95a3d8c95f06 100644
>> --- a/drivers/clk/tegra/clk-tegra-periph.c
>> +++ b/drivers/clk/tegra/clk-tegra-periph.c
>> @@ -823,6 +823,7 @@ static struct tegra_periph_init_data gate_clks[] = {
>> GATE("timer", "clk_m", 5, 0, tegra_clk_timer, CLK_IS_CRITICAL),
>> GATE("isp", "clk_m", 23, 0, tegra_clk_isp, 0),
>> GATE("vcp", "clk_m", 29, 0, tegra_clk_vcp, 0),
>> + GATE("ahbdma", "clk_m", 33, 0, tegra_clk_ahbdma, 0),
>
> Parent for this should be hclk on Tegra30 and later chips as well..
>
It looks like other clocks have a wrong parent too here, aren't they? Like for
example "apbdma" should have "pclk" as a parent, isn't it?
>> GATE("apbdma", "clk_m", 34, 0, tegra_clk_apbdma, 0),
>> GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
>> GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
>> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
>> index 837e5cbd60e9..e76c0d292ca7 100644
>> --- a/drivers/clk/tegra/clk-tegra20.c
>> +++ b/drivers/clk/tegra/clk-tegra20.c
>> @@ -449,6 +449,7 @@ static struct tegra_devclk devclks[] __initdata = {
>> { .con_id = "audio", .dt_id = TEGRA20_CLK_AUDIO },
>> { .con_id = "audio_2x", .dt_id = TEGRA20_CLK_AUDIO_2X },
>> { .dev_id = "tegra20-ac97", .dt_id = TEGRA20_CLK_AC97 },
>> + { .dev_id = "tegra-ahbdma", .dt_id = TEGRA20_CLK_AHBDMA },
>
> This isn't needed if you use DT bindings to get the clock handle.
>
Yes, I added it for consistency. Shouldn't we get rid of that all legacy stuff
already?
>> { .dev_id = "tegra-apbdma", .dt_id = TEGRA20_CLK_APBDMA },
>> { .dev_id = "rtc-tegra", .dt_id = TEGRA20_CLK_RTC },
>> { .dev_id = "timer", .dt_id = TEGRA20_CLK_TIMER },
>> @@ -806,6 +807,11 @@ static void __init tegra20_periph_clk_init(void)
>> clk_base, 0, 3, periph_clk_enb_refcnt);
>> clks[TEGRA20_CLK_AC97] = clk;
>>
>> + /* ahbdma */
>> + clk = tegra_clk_register_periph_gate("ahbdma", "hclk", 0, clk_base,
>> + 0, 33, periph_clk_enb_refcnt);
>> + clks[TEGRA20_CLK_AHBDMA] = clk;
>> +
>
> You can use the generic definition here if you correct the entry above.
>
Good point, same applies to "apbdma". Thank you for the suggestion.
>> /* apbdma */
>> clk = tegra_clk_register_periph_gate("apbdma", "pclk", 0, clk_base,
>> 0, 34, periph_clk_enb_refcnt);
>> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
>> index a2d163f759b4..e99701557f29 100644
>> --- a/drivers/clk/tegra/clk-tegra30.c
>> +++ b/drivers/clk/tegra/clk-tegra30.c
>> @@ -612,6 +612,7 @@ static struct tegra_devclk devclks[] __initdata = {
>> { .con_id = "fuse_burn", .dev_id = "fuse-tegra", .dt_id = TEGRA30_CLK_FUSE_BURN },
>> { .con_id = "apbif", .dev_id = "tegra30-ahub", .dt_id = TEGRA30_CLK_APBIF },
>> { .con_id = "hda2hdmi", .dev_id = "tegra30-hda", .dt_id = TEGRA30_CLK_HDA2HDMI },
>> + { .dev_id = "tegra-ahbdma", .dt_id = TEGRA30_CLK_AHBDMA },
>
> Same as for Tegra20.
>
>> { .dev_id = "tegra-apbdma", .dt_id = TEGRA30_CLK_APBDMA },
>> { .dev_id = "rtc-tegra", .dt_id = TEGRA30_CLK_RTC },
>> { .dev_id = "timer", .dt_id = TEGRA30_CLK_TIMER },
>> @@ -788,6 +789,7 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = {
>> [tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3, .present = true },
>> [tegra_clk_disp1] = { .dt_id = TEGRA30_CLK_DISP1, .present = true },
>> [tegra_clk_disp2] = { .dt_id = TEGRA30_CLK_DISP2, .present = true },
>> + [tegra_clk_ahbdma] = { .dt_id = TEGRA30_CLK_AHBDMA, .present = true },
>> [tegra_clk_apbdma] = { .dt_id = TEGRA30_CLK_APBDMA, .present = true },
>> [tegra_clk_rtc] = { .dt_id = TEGRA30_CLK_RTC, .present = true },
>> [tegra_clk_timer] = { .dt_id = TEGRA30_CLK_TIMER, .present = true },
>
> Cheers,
>
> Peter.
>
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry
2017-09-26 14:46 ` Dmitry Osipenko
@ 2017-09-27 8:36 ` Peter De Schrijver
2017-09-27 9:41 ` Dmitry Osipenko
0 siblings, 1 reply; 45+ messages in thread
From: Peter De Schrijver @ 2017-09-27 8:36 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul, linux-tegra, devicetree, dmaengine, linux-clk,
linux-kernel
On Tue, Sep 26, 2017 at 05:46:01PM +0300, Dmitry Osipenko wrote:
> On 26.09.2017 12:56, Peter De Schrijver wrote:
> > On Tue, Sep 26, 2017 at 02:22:02AM +0300, Dmitry Osipenko wrote:
> >> AHB DMA presents on Tegra20/30. Add missing entries, so that driver
> >> for AHB DMA could be implemented.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >> drivers/clk/tegra/clk-id.h | 1 +
> >> drivers/clk/tegra/clk-tegra-periph.c | 1 +
> >> drivers/clk/tegra/clk-tegra20.c | 6 ++++++
> >> drivers/clk/tegra/clk-tegra30.c | 2 ++
> >> 4 files changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/clk/tegra/clk-id.h b/drivers/clk/tegra/clk-id.h
> >> index 689f344377a7..c1661b47bbda 100644
> >> --- a/drivers/clk/tegra/clk-id.h
> >> +++ b/drivers/clk/tegra/clk-id.h
> >> @@ -12,6 +12,7 @@ enum clk_id {
> >> tegra_clk_amx,
> >> tegra_clk_amx1,
> >> tegra_clk_apb2ape,
> >> + tegra_clk_ahbdma,
> >> tegra_clk_apbdma,
> >> tegra_clk_apbif,
> >> tegra_clk_ape,
> >> diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
> >> index 848255cc0209..95a3d8c95f06 100644
> >> --- a/drivers/clk/tegra/clk-tegra-periph.c
> >> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> >> @@ -823,6 +823,7 @@ static struct tegra_periph_init_data gate_clks[] = {
> >> GATE("timer", "clk_m", 5, 0, tegra_clk_timer, CLK_IS_CRITICAL),
> >> GATE("isp", "clk_m", 23, 0, tegra_clk_isp, 0),
> >> GATE("vcp", "clk_m", 29, 0, tegra_clk_vcp, 0),
> >> + GATE("ahbdma", "clk_m", 33, 0, tegra_clk_ahbdma, 0),
> >
> > Parent for this should be hclk on Tegra30 and later chips as well..
> >
>
> It looks like other clocks have a wrong parent too here, aren't they? Like for
> example "apbdma" should have "pclk" as a parent, isn't it?
>
Yes. That is correct.
> >> GATE("apbdma", "clk_m", 34, 0, tegra_clk_apbdma, 0),
> >> GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
> >> GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
> >> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> >> index 837e5cbd60e9..e76c0d292ca7 100644
> >> --- a/drivers/clk/tegra/clk-tegra20.c
> >> +++ b/drivers/clk/tegra/clk-tegra20.c
> >> @@ -449,6 +449,7 @@ static struct tegra_devclk devclks[] __initdata = {
> >> { .con_id = "audio", .dt_id = TEGRA20_CLK_AUDIO },
> >> { .con_id = "audio_2x", .dt_id = TEGRA20_CLK_AUDIO_2X },
> >> { .dev_id = "tegra20-ac97", .dt_id = TEGRA20_CLK_AC97 },
> >> + { .dev_id = "tegra-ahbdma", .dt_id = TEGRA20_CLK_AHBDMA },
> >
> > This isn't needed if you use DT bindings to get the clock handle.
> >
>
> Yes, I added it for consistency. Shouldn't we get rid of that all legacy stuff
> already?
>
We probably should, but we can start by not adding more :)
> >> { .dev_id = "tegra-apbdma", .dt_id = TEGRA20_CLK_APBDMA },
> >> { .dev_id = "rtc-tegra", .dt_id = TEGRA20_CLK_RTC },
> >> { .dev_id = "timer", .dt_id = TEGRA20_CLK_TIMER },
> >> @@ -806,6 +807,11 @@ static void __init tegra20_periph_clk_init(void)
> >> clk_base, 0, 3, periph_clk_enb_refcnt);
> >> clks[TEGRA20_CLK_AC97] = clk;
> >>
> >> + /* ahbdma */
> >> + clk = tegra_clk_register_periph_gate("ahbdma", "hclk", 0, clk_base,
> >> + 0, 33, periph_clk_enb_refcnt);
> >> + clks[TEGRA20_CLK_AHBDMA] = clk;
> >> +
> >
> > You can use the generic definition here if you correct the entry above.
> >
>
> Good point, same applies to "apbdma". Thank you for the suggestion.
>
Indeed.
Peter.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry
2017-09-27 8:36 ` Peter De Schrijver
@ 2017-09-27 9:41 ` Dmitry Osipenko
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-27 9:41 UTC (permalink / raw)
To: Peter De Schrijver
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul, linux-tegra, devicetree, dmaengine, linux-clk,
linux-kernel
On 27.09.2017 11:36, Peter De Schrijver wrote:
> On Tue, Sep 26, 2017 at 05:46:01PM +0300, Dmitry Osipenko wrote:
>> On 26.09.2017 12:56, Peter De Schrijver wrote:
>>> On Tue, Sep 26, 2017 at 02:22:02AM +0300, Dmitry Osipenko wrote:
>>>> AHB DMA presents on Tegra20/30. Add missing entries, so that driver
>>>> for AHB DMA could be implemented.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>> drivers/clk/tegra/clk-id.h | 1 +
>>>> drivers/clk/tegra/clk-tegra-periph.c | 1 +
>>>> drivers/clk/tegra/clk-tegra20.c | 6 ++++++
>>>> drivers/clk/tegra/clk-tegra30.c | 2 ++
>>>> 4 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-id.h b/drivers/clk/tegra/clk-id.h
>>>> index 689f344377a7..c1661b47bbda 100644
>>>> --- a/drivers/clk/tegra/clk-id.h
>>>> +++ b/drivers/clk/tegra/clk-id.h
>>>> @@ -12,6 +12,7 @@ enum clk_id {
>>>> tegra_clk_amx,
>>>> tegra_clk_amx1,
>>>> tegra_clk_apb2ape,
>>>> + tegra_clk_ahbdma,
>>>> tegra_clk_apbdma,
>>>> tegra_clk_apbif,
>>>> tegra_clk_ape,
>>>> diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
>>>> index 848255cc0209..95a3d8c95f06 100644
>>>> --- a/drivers/clk/tegra/clk-tegra-periph.c
>>>> +++ b/drivers/clk/tegra/clk-tegra-periph.c
>>>> @@ -823,6 +823,7 @@ static struct tegra_periph_init_data gate_clks[] = {
>>>> GATE("timer", "clk_m", 5, 0, tegra_clk_timer, CLK_IS_CRITICAL),
>>>> GATE("isp", "clk_m", 23, 0, tegra_clk_isp, 0),
>>>> GATE("vcp", "clk_m", 29, 0, tegra_clk_vcp, 0),
>>>> + GATE("ahbdma", "clk_m", 33, 0, tegra_clk_ahbdma, 0),
>>>
>>> Parent for this should be hclk on Tegra30 and later chips as well..
>>>
>>
>> It looks like other clocks have a wrong parent too here, aren't they? Like for
>> example "apbdma" should have "pclk" as a parent, isn't it?
>>
>
> Yes. That is correct.
>
Okay, I'll fix it in V2.
>>>> GATE("apbdma", "clk_m", 34, 0, tegra_clk_apbdma, 0),
>>>> GATE("kbc", "clk_32k", 36, TEGRA_PERIPH_ON_APB | TEGRA_PERIPH_NO_RESET, tegra_clk_kbc, 0),
>>>> GATE("fuse", "clk_m", 39, TEGRA_PERIPH_ON_APB, tegra_clk_fuse, 0),
>>>> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
>>>> index 837e5cbd60e9..e76c0d292ca7 100644
>>>> --- a/drivers/clk/tegra/clk-tegra20.c
>>>> +++ b/drivers/clk/tegra/clk-tegra20.c
>>>> @@ -449,6 +449,7 @@ static struct tegra_devclk devclks[] __initdata = {
>>>> { .con_id = "audio", .dt_id = TEGRA20_CLK_AUDIO },
>>>> { .con_id = "audio_2x", .dt_id = TEGRA20_CLK_AUDIO_2X },
>>>> { .dev_id = "tegra20-ac97", .dt_id = TEGRA20_CLK_AC97 },
>>>> + { .dev_id = "tegra-ahbdma", .dt_id = TEGRA20_CLK_AHBDMA },
>>>
>>> This isn't needed if you use DT bindings to get the clock handle.
>>>
>>
>> Yes, I added it for consistency. Shouldn't we get rid of that all legacy stuff
>> already?
>>
>
> We probably should, but we can start by not adding more :)
>
Sure ;)
>>>> { .dev_id = "tegra-apbdma", .dt_id = TEGRA20_CLK_APBDMA },
>>>> { .dev_id = "rtc-tegra", .dt_id = TEGRA20_CLK_RTC },
>>>> { .dev_id = "timer", .dt_id = TEGRA20_CLK_TIMER },
>>>> @@ -806,6 +807,11 @@ static void __init tegra20_periph_clk_init(void)
>>>> clk_base, 0, 3, periph_clk_enb_refcnt);
>>>> clks[TEGRA20_CLK_AC97] = clk;
>>>>
>>>> + /* ahbdma */
>>>> + clk = tegra_clk_register_periph_gate("ahbdma", "hclk", 0, clk_base,
>>>> + 0, 33, periph_clk_enb_refcnt);
>>>> + clks[TEGRA20_CLK_AHBDMA] = clk;
>>>> +
>>>
>>> You can use the generic definition here if you correct the entry above.
>>>
>>
>> Good point, same applies to "apbdma". Thank you for the suggestion.
>>
>
> Indeed.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v1 2/5] clk: tegra: Bump SCLK clock rate to 216MHz on Tegra20
2017-09-25 23:22 [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver Dmitry Osipenko
2017-09-25 23:22 ` [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry Dmitry Osipenko
@ 2017-09-25 23:22 ` Dmitry Osipenko
2017-09-26 10:01 ` Peter De Schrijver
2017-09-25 23:22 ` [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller Dmitry Osipenko
` (2 subsequent siblings)
4 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-25 23:22 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
AHB DMA is a running on 1/2 of SCLK rate, APB on 1/4. Increasing SCLK rate
results in an increased DMA transfer rate.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/clk/tegra/clk-tegra20.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index e76c0d292ca7..c511716093e2 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1031,7 +1031,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
{ TEGRA20_CLK_PLL_P_OUT3, TEGRA20_CLK_CLK_MAX, 72000000, 1 },
{ TEGRA20_CLK_PLL_P_OUT4, TEGRA20_CLK_CLK_MAX, 24000000, 1 },
{ TEGRA20_CLK_PLL_C, TEGRA20_CLK_CLK_MAX, 600000000, 1 },
- { TEGRA20_CLK_PLL_C_OUT1, TEGRA20_CLK_CLK_MAX, 120000000, 1 },
+ { TEGRA20_CLK_PLL_C_OUT1, TEGRA20_CLK_CLK_MAX, 216000000, 1 },
{ TEGRA20_CLK_SCLK, TEGRA20_CLK_PLL_C_OUT1, 0, 1 },
{ TEGRA20_CLK_HCLK, TEGRA20_CLK_CLK_MAX, 0, 1 },
{ TEGRA20_CLK_PCLK, TEGRA20_CLK_CLK_MAX, 60000000, 1 },
--
2.14.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v1 2/5] clk: tegra: Bump SCLK clock rate to 216MHz on Tegra20
2017-09-25 23:22 ` [PATCH v1 2/5] clk: tegra: Bump SCLK clock rate to 216MHz on Tegra20 Dmitry Osipenko
@ 2017-09-26 10:01 ` Peter De Schrijver
0 siblings, 0 replies; 45+ messages in thread
From: Peter De Schrijver @ 2017-09-26 10:01 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul, linux-tegra, devicetree, dmaengine, linux-clk,
linux-kernel
On Tue, Sep 26, 2017 at 02:22:03AM +0300, Dmitry Osipenko wrote:
> AHB DMA is a running on 1/2 of SCLK rate, APB on 1/4. Increasing SCLK rate
> results in an increased DMA transfer rate.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Acked-By: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
> drivers/clk/tegra/clk-tegra20.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index e76c0d292ca7..c511716093e2 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1031,7 +1031,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
> { TEGRA20_CLK_PLL_P_OUT3, TEGRA20_CLK_CLK_MAX, 72000000, 1 },
> { TEGRA20_CLK_PLL_P_OUT4, TEGRA20_CLK_CLK_MAX, 24000000, 1 },
> { TEGRA20_CLK_PLL_C, TEGRA20_CLK_CLK_MAX, 600000000, 1 },
> - { TEGRA20_CLK_PLL_C_OUT1, TEGRA20_CLK_CLK_MAX, 120000000, 1 },
> + { TEGRA20_CLK_PLL_C_OUT1, TEGRA20_CLK_CLK_MAX, 216000000, 1 },
> { TEGRA20_CLK_SCLK, TEGRA20_CLK_PLL_C_OUT1, 0, 1 },
> { TEGRA20_CLK_HCLK, TEGRA20_CLK_CLK_MAX, 0, 1 },
> { TEGRA20_CLK_PCLK, TEGRA20_CLK_CLK_MAX, 60000000, 1 },
> --
> 2.14.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
2017-09-25 23:22 [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver Dmitry Osipenko
2017-09-25 23:22 ` [PATCH v1 1/5] clk: tegra: Add AHB DMA clock entry Dmitry Osipenko
2017-09-25 23:22 ` [PATCH v1 2/5] clk: tegra: Bump SCLK clock rate to 216MHz on Tegra20 Dmitry Osipenko
@ 2017-09-25 23:22 ` Dmitry Osipenko
2017-09-26 14:45 ` Jon Hunter
2017-09-28 9:29 ` Vinod Koul
[not found] ` <cover.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-09-28 9:31 ` [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver Vinod Koul
4 siblings, 2 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-25 23:22 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers
memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver
doesn't yet implement transfers larger than 64K and scatter-gather
transfers that have NENT > 1, HW doesn't have native support for these
cases.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/dma/Kconfig | 9 +
drivers/dma/Makefile | 1 +
drivers/dma/tegra20-ahb-dma.c | 679 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 689 insertions(+)
create mode 100644 drivers/dma/tegra20-ahb-dma.c
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index fadc4d8783bd..937c110a715b 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -503,6 +503,15 @@ config TXX9_DMAC
Support the TXx9 SoC internal DMA controller. This can be
integrated in chips such as the Toshiba TX4927/38/39.
+config TEGRA20_AHB_DMA
+ tristate "NVIDIA Tegra20 AHB DMA support"
+ depends on ARCH_TEGRA
+ select DMA_ENGINE
+ help
+ Enable support for the NVIDIA Tegra20 AHB DMA controller driver.
+ This DMA controller transfers data from memory to AHB peripherals
+ or vice versa, it supports memory to memory data transfer as well.
+
config TEGRA20_APB_DMA
bool "NVIDIA Tegra20 APB DMA support"
depends on ARCH_TEGRA
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index f08f8de1b567..82d5d36b0379 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
obj-$(CONFIG_STM32_DMA) += stm32-dma.o
obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
+obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o
obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o
obj-$(CONFIG_TIMB_DMA) += timb_dma.o
diff --git a/drivers/dma/tegra20-ahb-dma.c b/drivers/dma/tegra20-ahb-dma.c
new file mode 100644
index 000000000000..8316d64e35e1
--- /dev/null
+++ b/drivers/dma/tegra20-ahb-dma.c
@@ -0,0 +1,679 @@
+/*
+ * Copyright 2017 Dmitry Osipenko <digetx@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "dmaengine.h"
+
+#define TEGRA_AHBDMA_CMD 0x0
+#define TEGRA_AHBDMA_CMD_ENABLE BIT(31)
+
+#define TEGRA_AHBDMA_IRQ_ENB_MASK 0x20
+#define TEGRA_AHBDMA_IRQ_ENB_CH(ch) BIT(ch)
+
+#define TEGRA_AHBDMA_CHANNEL_BASE(ch) (0x1000 + (ch) * 0x20)
+
+#define TEGRA_AHBDMA_CHANNEL_CSR 0x0
+#define TEGRA_AHBDMA_CHANNEL_ADDR_WRAP BIT(18)
+#define TEGRA_AHBDMA_CHANNEL_FLOW BIT(24)
+#define TEGRA_AHBDMA_CHANNEL_ONCE BIT(26)
+#define TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB BIT(27)
+#define TEGRA_AHBDMA_CHANNEL_IE_EOC BIT(30)
+#define TEGRA_AHBDMA_CHANNEL_ENABLE BIT(31)
+#define TEGRA_AHBDMA_CHANNEL_REQ_SEL_SHIFT 16
+#define TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK 0xFFFC
+
+#define TEGRA_AHBDMA_CHANNEL_STA 0x4
+#define TEGRA_AHBDMA_CHANNEL_IS_EOC BIT(30)
+
+#define TEGRA_AHBDMA_CHANNEL_AHB_PTR 0x10
+
+#define TEGRA_AHBDMA_CHANNEL_AHB_SEQ 0x14
+#define TEGRA_AHBDMA_CHANNEL_INTR_ENB BIT(31)
+#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT 24
+#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_1 2
+#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_4 3
+#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_8 4
+
+#define TEGRA_AHBDMA_CHANNEL_XMB_PTR 0x18
+
+#define TEGRA_AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)
+
+#define TEGRA_AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \
+ BIT(DMA_MEM_TO_DEV)
+
+struct tegra_ahbdma_tx_desc {
+ struct dma_async_tx_descriptor desc;
+ struct tasklet_struct tasklet;
+ struct list_head node;
+ enum dma_transfer_direction dir;
+ dma_addr_t mem_paddr;
+ unsigned long flags;
+ size_t size;
+ bool in_fly;
+ bool cyclic;
+};
+
+struct tegra_ahbdma_chan {
+ struct dma_chan dma_chan;
+ struct list_head active_list;
+ struct list_head pending_list;
+ struct completion idling;
+ void __iomem *regs;
+ spinlock_t lock;
+ unsigned int id;
+};
+
+struct tegra_ahbdma {
+ struct tegra_ahbdma_chan channels[4];
+ struct dma_device dma_dev;
+ struct reset_control *rst;
+ struct clk *clk;
+ void __iomem *regs;
+};
+
+static inline struct tegra_ahbdma *to_ahbdma(struct dma_device *dev)
+{
+ return container_of(dev, struct tegra_ahbdma, dma_dev);
+}
+
+static inline struct tegra_ahbdma_chan *to_ahbdma_chan(struct dma_chan *chan)
+{
+ return container_of(chan, struct tegra_ahbdma_chan, dma_chan);
+}
+
+static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc(
+ struct dma_async_tx_descriptor *tx)
+{
+ return container_of(tx, struct tegra_ahbdma_tx_desc, desc);
+}
+
+static void tegra_ahbdma_submit_tx(struct tegra_ahbdma_chan *chan,
+ struct tegra_ahbdma_tx_desc *tx)
+{
+ u32 csr;
+
+ writel_relaxed(tx->mem_paddr,
+ chan->regs + TEGRA_AHBDMA_CHANNEL_XMB_PTR);
+
+ csr = readl_relaxed(chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
+
+ csr &= ~TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK;
+ csr &= ~TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB;
+ csr |= TEGRA_AHBDMA_CHANNEL_ENABLE;
+ csr |= TEGRA_AHBDMA_CHANNEL_IE_EOC;
+ csr |= tx->size - sizeof(u32);
+
+ if (tx->dir == DMA_DEV_TO_MEM)
+ csr |= TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB;
+
+ if (!tx->cyclic)
+ csr |= TEGRA_AHBDMA_CHANNEL_ONCE;
+
+ writel_relaxed(csr, chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
+
+ tx->in_fly = true;
+}
+
+static void tegra_ahbdma_tasklet(unsigned long data)
+{
+ struct tegra_ahbdma_tx_desc *tx = (struct tegra_ahbdma_tx_desc *)data;
+ struct dma_async_tx_descriptor *desc = &tx->desc;
+
+ dmaengine_desc_get_callback_invoke(desc, NULL);
+
+ if (!tx->cyclic && !dmaengine_desc_test_reuse(desc))
+ kfree(tx);
+}
+
+static bool tegra_ahbdma_tx_completed(struct tegra_ahbdma_chan *chan,
+ struct tegra_ahbdma_tx_desc *tx)
+{
+ struct dma_async_tx_descriptor *desc = &tx->desc;
+ bool reuse = dmaengine_desc_test_reuse(desc);
+ bool interrupt = tx->flags & DMA_PREP_INTERRUPT;
+ bool completed = !tx->cyclic;
+
+ if (completed)
+ dma_cookie_complete(desc);
+
+ if (interrupt)
+ tasklet_schedule(&tx->tasklet);
+
+ if (completed) {
+ list_del(&tx->node);
+
+ if (reuse)
+ tx->in_fly = false;
+
+ if (!interrupt && !reuse)
+ kfree(tx);
+ }
+
+ return completed;
+}
+
+static bool tegra_ahbdma_next_tx_issued(struct tegra_ahbdma_chan *chan)
+{
+ struct tegra_ahbdma_tx_desc *tx;
+
+ tx = list_first_entry_or_null(&chan->active_list,
+ struct tegra_ahbdma_tx_desc,
+ node);
+ if (tx)
+ tegra_ahbdma_submit_tx(chan, tx);
+
+ return !!tx;
+}
+
+static void tegra_ahbdma_handle_channel(struct tegra_ahbdma_chan *chan)
+{
+ struct tegra_ahbdma_tx_desc *tx;
+ unsigned long flags;
+ u32 status;
+
+ status = readl_relaxed(chan->regs + TEGRA_AHBDMA_CHANNEL_STA);
+ if (!(status & TEGRA_AHBDMA_CHANNEL_IS_EOC))
+ return;
+
+ writel_relaxed(TEGRA_AHBDMA_CHANNEL_IS_EOC,
+ chan->regs + TEGRA_AHBDMA_CHANNEL_STA);
+
+ spin_lock_irqsave(&chan->lock, flags);
+
+ if (!completion_done(&chan->idling)) {
+ tx = list_first_entry(&chan->active_list,
+ struct tegra_ahbdma_tx_desc,
+ node);
+
+ if (tegra_ahbdma_tx_completed(chan, tx) &&
+ !tegra_ahbdma_next_tx_issued(chan))
+ complete_all(&chan->idling);
+ }
+
+ spin_unlock_irqrestore(&chan->lock, flags);
+}
+
+static irqreturn_t tegra_ahbdma_isr(int irq, void *dev_id)
+{
+ struct tegra_ahbdma *tdma = dev_id;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(tdma->channels); i++)
+ tegra_ahbdma_handle_channel(&tdma->channels[i]);
+
+ return IRQ_HANDLED;
+}
+
+static dma_cookie_t tegra_ahbdma_tx_submit(struct dma_async_tx_descriptor *desc)
+{
+ struct tegra_ahbdma_tx_desc *tx = to_ahbdma_tx_desc(desc);
+ struct tegra_ahbdma_chan *chan = to_ahbdma_chan(desc->chan);
+ dma_cookie_t cookie;
+
+ cookie = dma_cookie_assign(desc);
+
+ spin_lock_irq(&chan->lock);
+ list_add_tail(&tx->node, &chan->pending_list);
+ spin_unlock_irq(&chan->lock);
+
+ return cookie;
+}
+
+static int tegra_ahbdma_tx_desc_free(struct dma_async_tx_descriptor *desc)
+{
+ kfree(to_ahbdma_tx_desc(desc));
+
+ return 0;
+}
+
+static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg(
+ struct dma_chan *chan,
+ struct scatterlist *sgl,
+ unsigned int sg_len,
+ enum dma_transfer_direction dir,
+ unsigned long flags,
+ void *context)
+{
+ struct tegra_ahbdma_tx_desc *tx;
+
+ /* unimplemented */
+ if (sg_len != 1 || sg_dma_len(sgl) > SZ_64K)
+ return NULL;
+
+ tx = kzalloc(sizeof(*tx), GFP_KERNEL);
+ if (!tx)
+ return NULL;
+
+ dma_async_tx_descriptor_init(&tx->desc, chan);
+
+ tx->desc.tx_submit = tegra_ahbdma_tx_submit;
+ tx->desc.desc_free = tegra_ahbdma_tx_desc_free;
+ tx->mem_paddr = sg_dma_address(sgl);
+ tx->size = sg_dma_len(sgl);
+ tx->flags = flags;
+ tx->dir = dir;
+
+ tasklet_init(&tx->tasklet, tegra_ahbdma_tasklet, (unsigned long)tx);
+
+ return &tx->desc;
+}
+
+static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic(
+ struct dma_chan *chan,
+ dma_addr_t buf_addr,
+ size_t buf_len,
+ size_t period_len,
+ enum dma_transfer_direction dir,
+ unsigned long flags)
+{
+ struct tegra_ahbdma_tx_desc *tx;
+
+ /* unimplemented */
+ if (buf_len != period_len || buf_len > SZ_64K)
+ return NULL;
+
+ tx = kzalloc(sizeof(*tx), GFP_KERNEL);
+ if (!tx)
+ return NULL;
+
+ dma_async_tx_descriptor_init(&tx->desc, chan);
+
+ tx->desc.tx_submit = tegra_ahbdma_tx_submit;
+ tx->mem_paddr = buf_addr;
+ tx->size = buf_len;
+ tx->flags = flags;
+ tx->cyclic = true;
+ tx->dir = dir;
+
+ tasklet_init(&tx->tasklet, tegra_ahbdma_tasklet, (unsigned long)tx);
+
+ return &tx->desc;
+}
+
+static void tegra_ahbdma_issue_pending(struct dma_chan *chan)
+{
+ struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
+ struct tegra_ahbdma_tx_desc *tx;
+ struct list_head *entry, *tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ahbdma_chan->lock, flags);
+
+ list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list)
+ list_move_tail(entry, &ahbdma_chan->active_list);
+
+ if (completion_done(&ahbdma_chan->idling)) {
+ tx = list_first_entry_or_null(&ahbdma_chan->active_list,
+ struct tegra_ahbdma_tx_desc,
+ node);
+ if (tx) {
+ tegra_ahbdma_submit_tx(ahbdma_chan, tx);
+ reinit_completion(&ahbdma_chan->idling);
+ }
+ }
+
+ spin_unlock_irqrestore(&ahbdma_chan->lock, flags);
+}
+
+static enum dma_status tegra_ahbdma_tx_status(struct dma_chan *chan,
+ dma_cookie_t cookie,
+ struct dma_tx_state *state)
+{
+ struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
+ struct tegra_ahbdma_tx_desc *tx;
+ enum dma_status cookie_status;
+ unsigned long flags;
+ size_t residual;
+ u32 status;
+
+ spin_lock_irqsave(&ahbdma_chan->lock, flags);
+
+ cookie_status = dma_cookie_status(chan, cookie, state);
+ if (cookie_status != DMA_COMPLETE) {
+ list_for_each_entry(tx, &ahbdma_chan->active_list, node) {
+ if (tx->desc.cookie == cookie)
+ goto found;
+ }
+ }
+
+ goto unlock;
+
+found:
+ if (tx->in_fly) {
+ status = readl_relaxed(
+ ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_STA);
+ status &= TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK;
+
+ residual = status;
+ } else
+ residual = tx->size;
+
+ dma_set_residue(state, residual);
+
+unlock:
+ spin_unlock_irqrestore(&ahbdma_chan->lock, flags);
+
+ return cookie_status;
+}
+
+static int tegra_ahbdma_terminate_all(struct dma_chan *chan)
+{
+ struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
+ struct tegra_ahbdma_tx_desc *tx;
+ struct list_head *entry, *tmp;
+ u32 csr;
+
+ spin_lock_irq(&ahbdma_chan->lock);
+
+ csr = readl_relaxed(ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
+ csr &= ~TEGRA_AHBDMA_CHANNEL_ENABLE;
+
+ writel_relaxed(csr, ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
+
+ list_for_each_safe(entry, tmp, &ahbdma_chan->active_list) {
+ tx = list_entry(entry, struct tegra_ahbdma_tx_desc, node);
+ list_del(entry);
+ kfree(tx);
+ }
+
+ list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list) {
+ tx = list_entry(entry, struct tegra_ahbdma_tx_desc, node);
+ list_del(entry);
+ kfree(tx);
+ }
+
+ complete_all(&ahbdma_chan->idling);
+
+ spin_unlock_irq(&ahbdma_chan->lock);
+
+ return 0;
+}
+
+static int tegra_ahbdma_config(struct dma_chan *chan,
+ struct dma_slave_config *sconfig)
+{
+ struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
+ enum dma_transfer_direction dir = sconfig->direction;
+ u32 burst, ahb_seq, ahb_addr;
+
+ if (sconfig->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
+ sconfig->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
+ return -EINVAL;
+
+ if (dir == DMA_DEV_TO_MEM) {
+ burst = sconfig->src_maxburst;
+ ahb_addr = sconfig->src_addr;
+ } else {
+ burst = sconfig->dst_maxburst;
+ ahb_addr = sconfig->dst_addr;
+ }
+
+ switch (burst) {
+ case 1: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_1; break;
+ case 4: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_4; break;
+ case 8: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_8; break;
+ default:
+ return -EINVAL;
+ }
+
+ ahb_seq = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
+ ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
+ ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
+
+ writel_relaxed(ahb_seq,
+ ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
+
+ writel_relaxed(ahb_addr,
+ ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
+
+ return 0;
+}
+
+static void tegra_ahbdma_synchronize(struct dma_chan *chan)
+{
+ wait_for_completion(&to_ahbdma_chan(chan)->idling);
+}
+
+static void tegra_ahbdma_free_chan_resources(struct dma_chan *chan)
+{
+ struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
+ struct tegra_ahbdma_tx_desc *tx;
+ struct list_head *entry, *tmp;
+
+ list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list) {
+ tx = list_entry(entry, struct tegra_ahbdma_tx_desc, node);
+ list_del(entry);
+ kfree(tx);
+ }
+}
+
+static void tegra_ahbdma_init_channel(struct tegra_ahbdma *tdma,
+ unsigned int chan_id)
+{
+ struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id];
+ struct dma_chan *dma_chan = &ahbdma_chan->dma_chan;
+ struct dma_device *dma_dev = &tdma->dma_dev;
+
+ INIT_LIST_HEAD(&ahbdma_chan->active_list);
+ INIT_LIST_HEAD(&ahbdma_chan->pending_list);
+ init_completion(&ahbdma_chan->idling);
+ spin_lock_init(&ahbdma_chan->lock);
+ complete(&ahbdma_chan->idling);
+
+ ahbdma_chan->regs = tdma->regs + TEGRA_AHBDMA_CHANNEL_BASE(chan_id);
+ ahbdma_chan->id = chan_id;
+
+ dma_cookie_init(dma_chan);
+ dma_chan->device = dma_dev;
+
+ list_add_tail(&dma_chan->device_node, &dma_dev->channels);
+}
+
+static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct tegra_ahbdma *tdma = ofdma->of_dma_data;
+ struct dma_chan *chan;
+ u32 csr;
+
+ chan = dma_get_any_slave_channel(&tdma->dma_dev);
+ if (!chan)
+ return NULL;
+
+ /* enable channels flow control */
+ if (dma_spec->args_count == 1) {
+ csr = TEGRA_AHBDMA_CHANNEL_FLOW;
+ csr |= dma_spec->args[0] << TEGRA_AHBDMA_CHANNEL_REQ_SEL_SHIFT;
+
+ writel_relaxed(csr,
+ to_ahbdma_chan(chan)->regs + TEGRA_AHBDMA_CHANNEL_CSR);
+ }
+
+ return chan;
+}
+
+static int tegra_ahbdma_init_hw(struct tegra_ahbdma *tdma, struct device *dev)
+{
+ int err;
+
+ err = reset_control_assert(tdma->rst);
+ if (err) {
+ dev_err(dev, "Failed to assert reset: %d\n", err);
+ return err;
+ }
+
+ err = clk_prepare_enable(tdma->clk);
+ if (err) {
+ dev_err(dev, "Failed to enable clock: %d\n", err);
+ return err;
+ }
+
+ usleep_range(1000, 2000);
+
+ err = reset_control_deassert(tdma->rst);
+ if (err) {
+ dev_err(dev, "Failed to deassert reset: %d\n", err);
+ return err;
+ }
+
+ writel_relaxed(TEGRA_AHBDMA_CMD_ENABLE, tdma->regs + TEGRA_AHBDMA_CMD);
+
+ writel_relaxed(TEGRA_AHBDMA_IRQ_ENB_CH(0) |
+ TEGRA_AHBDMA_IRQ_ENB_CH(1) |
+ TEGRA_AHBDMA_IRQ_ENB_CH(2) |
+ TEGRA_AHBDMA_IRQ_ENB_CH(3),
+ tdma->regs + TEGRA_AHBDMA_IRQ_ENB_MASK);
+
+ return 0;
+}
+
+static int tegra_ahbdma_probe(struct platform_device *pdev)
+{
+ struct dma_device *dma_dev;
+ struct tegra_ahbdma *tdma;
+ struct resource *res_regs;
+ unsigned int i;
+ int irq;
+ int err;
+
+ tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma), GFP_KERNEL);
+ if (!tdma)
+ return -ENOMEM;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "Failed to get IRQ\n");
+ return irq;
+ }
+
+ err = devm_request_irq(&pdev->dev, irq, tegra_ahbdma_isr, 0,
+ dev_name(&pdev->dev), tdma);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to request IRQ\n");
+ return -ENODEV;
+ }
+
+ res_regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res_regs)
+ return -ENODEV;
+
+ tdma->regs = devm_ioremap_resource(&pdev->dev, res_regs);
+ if (IS_ERR(tdma->regs))
+ return PTR_ERR(tdma->regs);
+
+ tdma->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(tdma->clk)) {
+ dev_err(&pdev->dev, "Failed to get AHB-DMA clock\n");
+ return PTR_ERR(tdma->clk);
+ }
+
+ tdma->rst = devm_reset_control_get(&pdev->dev, NULL);
+ if (IS_ERR(tdma->rst)) {
+ dev_err(&pdev->dev, "Failed to get AHB-DMA reset\n");
+ return PTR_ERR(tdma->rst);
+ }
+
+ err = tegra_ahbdma_init_hw(tdma, &pdev->dev);
+ if (err)
+ return err;
+
+ dma_dev = &tdma->dma_dev;
+
+ INIT_LIST_HEAD(&dma_dev->channels);
+
+ for (i = 0; i < ARRAY_SIZE(tdma->channels); i++)
+ tegra_ahbdma_init_channel(tdma, i);
+
+ dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
+ dma_cap_set(DMA_CYCLIC, dma_dev->cap_mask);
+ dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
+
+ dma_dev->max_burst = 8;
+ dma_dev->directions = TEGRA_AHBDMA_DIRECTIONS;
+ dma_dev->src_addr_widths = TEGRA_AHBDMA_BUS_WIDTH;
+ dma_dev->dst_addr_widths = TEGRA_AHBDMA_BUS_WIDTH;
+ dma_dev->descriptor_reuse = true;
+ dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
+ dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources;
+ dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg;
+ dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic;
+ dma_dev->device_terminate_all = tegra_ahbdma_terminate_all;
+ dma_dev->device_issue_pending = tegra_ahbdma_issue_pending;
+ dma_dev->device_tx_status = tegra_ahbdma_tx_status;
+ dma_dev->device_config = tegra_ahbdma_config;
+ dma_dev->device_synchronize = tegra_ahbdma_synchronize;
+ dma_dev->dev = &pdev->dev;
+
+ err = dma_async_device_register(dma_dev);
+ if (err) {
+ dev_err(&pdev->dev, "Device registration failed %d\n", err);
+ return err;
+ }
+
+ err = of_dma_controller_register(pdev->dev.of_node,
+ tegra_ahbdma_of_xlate, tdma);
+ if (err) {
+ dev_err(&pdev->dev, "OF registration failed %d\n", err);
+ dma_async_device_unregister(dma_dev);
+ return err;
+ }
+
+ platform_set_drvdata(pdev, tdma);
+
+ return 0;
+}
+
+static int tegra_ahbdma_remove(struct platform_device *pdev)
+{
+ struct tegra_ahbdma *tdma = platform_get_drvdata(pdev);
+
+ of_dma_controller_free(pdev->dev.of_node);
+ dma_async_device_unregister(&tdma->dma_dev);
+ clk_disable_unprepare(tdma->clk);
+
+ return 0;
+}
+
+static const struct of_device_id tegra_ahbdma_of_match[] = {
+ { .compatible = "nvidia,tegra20-ahbdma" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, tegra_ahbdma_of_match);
+
+static struct platform_driver tegra_ahbdma_driver = {
+ .driver = {
+ .name = "tegra-ahbdma",
+ .of_match_table = tegra_ahbdma_of_match,
+ },
+ .probe = tegra_ahbdma_probe,
+ .remove = tegra_ahbdma_remove,
+};
+module_platform_driver(tegra_ahbdma_driver);
+
+MODULE_DESCRIPTION("NVIDIA Tegra AHB DMA Controller driver");
+MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
+MODULE_LICENSE("GPL");
--
2.14.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
2017-09-25 23:22 ` [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller Dmitry Osipenko
@ 2017-09-26 14:45 ` Jon Hunter
[not found] ` <481add20-9cea-a91a-e72c-45a824362e64-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2017-09-28 9:29 ` Vinod Koul
1 sibling, 1 reply; 45+ messages in thread
From: Jon Hunter @ 2017-09-26 14:45 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
Hi Dmitry,
On 26/09/17 00:22, Dmitry Osipenko wrote:
> AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers
> memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver
> doesn't yet implement transfers larger than 64K and scatter-gather
> transfers that have NENT > 1, HW doesn't have native support for these
> cases.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/dma/Kconfig | 9 +
> drivers/dma/Makefile | 1 +
> drivers/dma/tegra20-ahb-dma.c | 679 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 689 insertions(+)
> create mode 100644 drivers/dma/tegra20-ahb-dma.c
...
> diff --git a/drivers/dma/tegra20-ahb-dma.c b/drivers/dma/tegra20-ahb-dma.c
> new file mode 100644
> index 000000000000..8316d64e35e1
> --- /dev/null
> +++ b/drivers/dma/tegra20-ahb-dma.c
> @@ -0,0 +1,679 @@
> +/*
> + * Copyright 2017 Dmitry Osipenko <digetx@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "dmaengine.h"
> +
> +#define TEGRA_AHBDMA_CMD 0x0
> +#define TEGRA_AHBDMA_CMD_ENABLE BIT(31)
> +
> +#define TEGRA_AHBDMA_IRQ_ENB_MASK 0x20
> +#define TEGRA_AHBDMA_IRQ_ENB_CH(ch) BIT(ch)
> +
> +#define TEGRA_AHBDMA_CHANNEL_BASE(ch) (0x1000 + (ch) * 0x20)
> +
> +#define TEGRA_AHBDMA_CHANNEL_CSR 0x0
> +#define TEGRA_AHBDMA_CHANNEL_ADDR_WRAP BIT(18)
> +#define TEGRA_AHBDMA_CHANNEL_FLOW BIT(24)
> +#define TEGRA_AHBDMA_CHANNEL_ONCE BIT(26)
> +#define TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB BIT(27)
> +#define TEGRA_AHBDMA_CHANNEL_IE_EOC BIT(30)
> +#define TEGRA_AHBDMA_CHANNEL_ENABLE BIT(31)
> +#define TEGRA_AHBDMA_CHANNEL_REQ_SEL_SHIFT 16
> +#define TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK 0xFFFC
> +
> +#define TEGRA_AHBDMA_CHANNEL_STA 0x4
> +#define TEGRA_AHBDMA_CHANNEL_IS_EOC BIT(30)
> +
> +#define TEGRA_AHBDMA_CHANNEL_AHB_PTR 0x10
> +
> +#define TEGRA_AHBDMA_CHANNEL_AHB_SEQ 0x14
> +#define TEGRA_AHBDMA_CHANNEL_INTR_ENB BIT(31)
> +#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT 24
> +#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_1 2
> +#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_4 3
> +#define TEGRA_AHBDMA_CHANNEL_AHB_BURST_8 4
> +
> +#define TEGRA_AHBDMA_CHANNEL_XMB_PTR 0x18
> +
> +#define TEGRA_AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)
> +
> +#define TEGRA_AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \
> + BIT(DMA_MEM_TO_DEV)
> +
> +struct tegra_ahbdma_tx_desc {
> + struct dma_async_tx_descriptor desc;
> + struct tasklet_struct tasklet;
> + struct list_head node;
Any reason why we cannot use the virt-dma framework for this driver? I
would hope it would simplify the driver a bit.
> + enum dma_transfer_direction dir;
> + dma_addr_t mem_paddr;
> + unsigned long flags;
> + size_t size;
> + bool in_fly;
> + bool cyclic;
> +};
> +
> +struct tegra_ahbdma_chan {
> + struct dma_chan dma_chan;
> + struct list_head active_list;
> + struct list_head pending_list;
> + struct completion idling;
> + void __iomem *regs;
> + spinlock_t lock;
> + unsigned int id;
> +};
> +
> +struct tegra_ahbdma {
> + struct tegra_ahbdma_chan channels[4];
> + struct dma_device dma_dev;
> + struct reset_control *rst;
> + struct clk *clk;
> + void __iomem *regs;
> +};
> +
> +static inline struct tegra_ahbdma *to_ahbdma(struct dma_device *dev)
> +{
> + return container_of(dev, struct tegra_ahbdma, dma_dev);
> +}
> +
> +static inline struct tegra_ahbdma_chan *to_ahbdma_chan(struct dma_chan *chan)
> +{
> + return container_of(chan, struct tegra_ahbdma_chan, dma_chan);
> +}
> +
> +static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc(
> + struct dma_async_tx_descriptor *tx)
> +{
> + return container_of(tx, struct tegra_ahbdma_tx_desc, desc);
> +}
> +
> +static void tegra_ahbdma_submit_tx(struct tegra_ahbdma_chan *chan,
> + struct tegra_ahbdma_tx_desc *tx)
> +{
> + u32 csr;
> +
> + writel_relaxed(tx->mem_paddr,
> + chan->regs + TEGRA_AHBDMA_CHANNEL_XMB_PTR);
> +
> + csr = readl_relaxed(chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
> +
> + csr &= ~TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK;
> + csr &= ~TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB;
> + csr |= TEGRA_AHBDMA_CHANNEL_ENABLE;
> + csr |= TEGRA_AHBDMA_CHANNEL_IE_EOC;
> + csr |= tx->size - sizeof(u32);
> +
> + if (tx->dir == DMA_DEV_TO_MEM)
> + csr |= TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB;
> +
> + if (!tx->cyclic)
> + csr |= TEGRA_AHBDMA_CHANNEL_ONCE;
> +
> + writel_relaxed(csr, chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
> +
> + tx->in_fly = true;
> +}
> +
> +static void tegra_ahbdma_tasklet(unsigned long data)
> +{
> + struct tegra_ahbdma_tx_desc *tx = (struct tegra_ahbdma_tx_desc *)data;
> + struct dma_async_tx_descriptor *desc = &tx->desc;
> +
> + dmaengine_desc_get_callback_invoke(desc, NULL);
> +
> + if (!tx->cyclic && !dmaengine_desc_test_reuse(desc))
> + kfree(tx);
> +}
> +
> +static bool tegra_ahbdma_tx_completed(struct tegra_ahbdma_chan *chan,
> + struct tegra_ahbdma_tx_desc *tx)
> +{
> + struct dma_async_tx_descriptor *desc = &tx->desc;
> + bool reuse = dmaengine_desc_test_reuse(desc);
> + bool interrupt = tx->flags & DMA_PREP_INTERRUPT;
> + bool completed = !tx->cyclic;
> +
> + if (completed)
> + dma_cookie_complete(desc);
> +
> + if (interrupt)
> + tasklet_schedule(&tx->tasklet);
> +
> + if (completed) {
> + list_del(&tx->node);
> +
> + if (reuse)
> + tx->in_fly = false;
> +
> + if (!interrupt && !reuse)
> + kfree(tx);
> + }
> +
> + return completed;
> +}
> +
> +static bool tegra_ahbdma_next_tx_issued(struct tegra_ahbdma_chan *chan)
> +{
> + struct tegra_ahbdma_tx_desc *tx;
> +
> + tx = list_first_entry_or_null(&chan->active_list,
> + struct tegra_ahbdma_tx_desc,
> + node);
> + if (tx)
> + tegra_ahbdma_submit_tx(chan, tx);
> +
> + return !!tx;
> +}
> +
> +static void tegra_ahbdma_handle_channel(struct tegra_ahbdma_chan *chan)
> +{
> + struct tegra_ahbdma_tx_desc *tx;
> + unsigned long flags;
> + u32 status;
> +
> + status = readl_relaxed(chan->regs + TEGRA_AHBDMA_CHANNEL_STA);
> + if (!(status & TEGRA_AHBDMA_CHANNEL_IS_EOC))
> + return;
> +
> + writel_relaxed(TEGRA_AHBDMA_CHANNEL_IS_EOC,
> + chan->regs + TEGRA_AHBDMA_CHANNEL_STA);
> +
> + spin_lock_irqsave(&chan->lock, flags);
> +
> + if (!completion_done(&chan->idling)) {
> + tx = list_first_entry(&chan->active_list,
> + struct tegra_ahbdma_tx_desc,
> + node);
> +
> + if (tegra_ahbdma_tx_completed(chan, tx) &&
> + !tegra_ahbdma_next_tx_issued(chan))
> + complete_all(&chan->idling);
> + }
> +
> + spin_unlock_irqrestore(&chan->lock, flags);
> +}
> +
> +static irqreturn_t tegra_ahbdma_isr(int irq, void *dev_id)
> +{
> + struct tegra_ahbdma *tdma = dev_id;
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(tdma->channels); i++)
> + tegra_ahbdma_handle_channel(&tdma->channels[i]);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static dma_cookie_t tegra_ahbdma_tx_submit(struct dma_async_tx_descriptor *desc)
> +{
> + struct tegra_ahbdma_tx_desc *tx = to_ahbdma_tx_desc(desc);
> + struct tegra_ahbdma_chan *chan = to_ahbdma_chan(desc->chan);
> + dma_cookie_t cookie;
> +
> + cookie = dma_cookie_assign(desc);
> +
> + spin_lock_irq(&chan->lock);
> + list_add_tail(&tx->node, &chan->pending_list);
> + spin_unlock_irq(&chan->lock);
> +
> + return cookie;
> +}
> +
> +static int tegra_ahbdma_tx_desc_free(struct dma_async_tx_descriptor *desc)
> +{
> + kfree(to_ahbdma_tx_desc(desc));
> +
> + return 0;
> +}
> +
> +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg(
> + struct dma_chan *chan,
> + struct scatterlist *sgl,
> + unsigned int sg_len,
> + enum dma_transfer_direction dir,
> + unsigned long flags,
> + void *context)
> +{
> + struct tegra_ahbdma_tx_desc *tx;
> +
> + /* unimplemented */
> + if (sg_len != 1 || sg_dma_len(sgl) > SZ_64K)
> + return NULL;
> +
> + tx = kzalloc(sizeof(*tx), GFP_KERNEL);
> + if (!tx)
> + return NULL;
> +
> + dma_async_tx_descriptor_init(&tx->desc, chan);
> +
> + tx->desc.tx_submit = tegra_ahbdma_tx_submit;
> + tx->desc.desc_free = tegra_ahbdma_tx_desc_free;
> + tx->mem_paddr = sg_dma_address(sgl);
> + tx->size = sg_dma_len(sgl);
> + tx->flags = flags;
> + tx->dir = dir;
> +
> + tasklet_init(&tx->tasklet, tegra_ahbdma_tasklet, (unsigned long)tx);
> +
> + return &tx->desc;
> +}
> +
> +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic(
> + struct dma_chan *chan,
> + dma_addr_t buf_addr,
> + size_t buf_len,
> + size_t period_len,
> + enum dma_transfer_direction dir,
> + unsigned long flags)
> +{
> + struct tegra_ahbdma_tx_desc *tx;
> +
> + /* unimplemented */
> + if (buf_len != period_len || buf_len > SZ_64K)
> + return NULL;
> +
> + tx = kzalloc(sizeof(*tx), GFP_KERNEL);
> + if (!tx)
> + return NULL;
> +
> + dma_async_tx_descriptor_init(&tx->desc, chan);
> +
> + tx->desc.tx_submit = tegra_ahbdma_tx_submit;
> + tx->mem_paddr = buf_addr;
> + tx->size = buf_len;
> + tx->flags = flags;
> + tx->cyclic = true;
> + tx->dir = dir;
> +
> + tasklet_init(&tx->tasklet, tegra_ahbdma_tasklet, (unsigned long)tx);
> +
> + return &tx->desc;
> +}
> +
> +static void tegra_ahbdma_issue_pending(struct dma_chan *chan)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + struct tegra_ahbdma_tx_desc *tx;
> + struct list_head *entry, *tmp;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ahbdma_chan->lock, flags);
> +
> + list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list)
> + list_move_tail(entry, &ahbdma_chan->active_list);
> +
> + if (completion_done(&ahbdma_chan->idling)) {
> + tx = list_first_entry_or_null(&ahbdma_chan->active_list,
> + struct tegra_ahbdma_tx_desc,
> + node);
> + if (tx) {
> + tegra_ahbdma_submit_tx(ahbdma_chan, tx);
> + reinit_completion(&ahbdma_chan->idling);
> + }
> + }
> +
> + spin_unlock_irqrestore(&ahbdma_chan->lock, flags);
> +}
> +
> +static enum dma_status tegra_ahbdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *state)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + struct tegra_ahbdma_tx_desc *tx;
> + enum dma_status cookie_status;
> + unsigned long flags;
> + size_t residual;
> + u32 status;
> +
> + spin_lock_irqsave(&ahbdma_chan->lock, flags);
> +
> + cookie_status = dma_cookie_status(chan, cookie, state);
> + if (cookie_status != DMA_COMPLETE) {
> + list_for_each_entry(tx, &ahbdma_chan->active_list, node) {
> + if (tx->desc.cookie == cookie)
> + goto found;
> + }
> + }
> +
> + goto unlock;
> +
> +found:
> + if (tx->in_fly) {
> + status = readl_relaxed(
> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_STA);
> + status &= TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK;
> +
> + residual = status;
> + } else
> + residual = tx->size;
> +
> + dma_set_residue(state, residual);
> +
> +unlock:
> + spin_unlock_irqrestore(&ahbdma_chan->lock, flags);
> +
> + return cookie_status;
> +}
> +
> +static int tegra_ahbdma_terminate_all(struct dma_chan *chan)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + struct tegra_ahbdma_tx_desc *tx;
> + struct list_head *entry, *tmp;
> + u32 csr;
> +
> + spin_lock_irq(&ahbdma_chan->lock);
> +
> + csr = readl_relaxed(ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
> + csr &= ~TEGRA_AHBDMA_CHANNEL_ENABLE;
> +
> + writel_relaxed(csr, ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_CSR);
> +
> + list_for_each_safe(entry, tmp, &ahbdma_chan->active_list) {
> + tx = list_entry(entry, struct tegra_ahbdma_tx_desc, node);
> + list_del(entry);
> + kfree(tx);
> + }
> +
> + list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list) {
> + tx = list_entry(entry, struct tegra_ahbdma_tx_desc, node);
> + list_del(entry);
> + kfree(tx);
> + }
> +
> + complete_all(&ahbdma_chan->idling);
> +
> + spin_unlock_irq(&ahbdma_chan->lock);
> +
> + return 0;
> +}
> +
> +static int tegra_ahbdma_config(struct dma_chan *chan,
> + struct dma_slave_config *sconfig)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + enum dma_transfer_direction dir = sconfig->direction;
> + u32 burst, ahb_seq, ahb_addr;
> +
> + if (sconfig->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
> + sconfig->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + return -EINVAL;
> +
> + if (dir == DMA_DEV_TO_MEM) {
> + burst = sconfig->src_maxburst;
> + ahb_addr = sconfig->src_addr;
> + } else {
> + burst = sconfig->dst_maxburst;
> + ahb_addr = sconfig->dst_addr;
> + }
> +
> + switch (burst) {
> + case 1: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_1; break;
> + case 4: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_4; break;
> + case 8: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_8; break;
> + default:
> + return -EINVAL;
> + }
> +
> + ahb_seq = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
> +
> + writel_relaxed(ahb_seq,
> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
> +
> + writel_relaxed(ahb_addr,
> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
> +
> + return 0;
> +}
> +
> +static void tegra_ahbdma_synchronize(struct dma_chan *chan)
> +{
> + wait_for_completion(&to_ahbdma_chan(chan)->idling);
> +}
> +
> +static void tegra_ahbdma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + struct tegra_ahbdma_tx_desc *tx;
> + struct list_head *entry, *tmp;
> +
> + list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list) {
> + tx = list_entry(entry, struct tegra_ahbdma_tx_desc, node);
> + list_del(entry);
> + kfree(tx);
> + }
> +}
> +
> +static void tegra_ahbdma_init_channel(struct tegra_ahbdma *tdma,
> + unsigned int chan_id)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id];
> + struct dma_chan *dma_chan = &ahbdma_chan->dma_chan;
> + struct dma_device *dma_dev = &tdma->dma_dev;
> +
> + INIT_LIST_HEAD(&ahbdma_chan->active_list);
> + INIT_LIST_HEAD(&ahbdma_chan->pending_list);
> + init_completion(&ahbdma_chan->idling);
> + spin_lock_init(&ahbdma_chan->lock);
> + complete(&ahbdma_chan->idling);
> +
> + ahbdma_chan->regs = tdma->regs + TEGRA_AHBDMA_CHANNEL_BASE(chan_id);
> + ahbdma_chan->id = chan_id;
> +
> + dma_cookie_init(dma_chan);
> + dma_chan->device = dma_dev;
> +
> + list_add_tail(&dma_chan->device_node, &dma_dev->channels);
> +}
> +
> +static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args *dma_spec,
> + struct of_dma *ofdma)
> +{
> + struct tegra_ahbdma *tdma = ofdma->of_dma_data;
> + struct dma_chan *chan;
> + u32 csr;
> +
> + chan = dma_get_any_slave_channel(&tdma->dma_dev);
> + if (!chan)
> + return NULL;
> +
> + /* enable channels flow control */
> + if (dma_spec->args_count == 1) {
The DT doc says #dma-cells should be '1' and so if not equal 1, is this
not an error?
> + csr = TEGRA_AHBDMA_CHANNEL_FLOW;
> + csr |= dma_spec->args[0] << TEGRA_AHBDMA_CHANNEL_REQ_SEL_SHIFT;
What about the TRIG_REQ field?
> +
> + writel_relaxed(csr,
> + to_ahbdma_chan(chan)->regs + TEGRA_AHBDMA_CHANNEL_CSR);
> + }
> +
> + return chan;
> +}
> +
> +static int tegra_ahbdma_init_hw(struct tegra_ahbdma *tdma, struct device *dev)
> +{
> + int err;
> +
> + err = reset_control_assert(tdma->rst);
> + if (err) {
> + dev_err(dev, "Failed to assert reset: %d\n", err);
> + return err;
> + }
> +
> + err = clk_prepare_enable(tdma->clk);
> + if (err) {
> + dev_err(dev, "Failed to enable clock: %d\n", err);
> + return err;
> + }
> +
> + usleep_range(1000, 2000);
> +
> + err = reset_control_deassert(tdma->rst);
> + if (err) {
> + dev_err(dev, "Failed to deassert reset: %d\n", err);
> + return err;
> + }
> +
> + writel_relaxed(TEGRA_AHBDMA_CMD_ENABLE, tdma->regs + TEGRA_AHBDMA_CMD);
> +
> + writel_relaxed(TEGRA_AHBDMA_IRQ_ENB_CH(0) |
> + TEGRA_AHBDMA_IRQ_ENB_CH(1) |
> + TEGRA_AHBDMA_IRQ_ENB_CH(2) |
> + TEGRA_AHBDMA_IRQ_ENB_CH(3),
> + tdma->regs + TEGRA_AHBDMA_IRQ_ENB_MASK);
> +
> + return 0;
> +}
Personally I would use the pm_runtime callbacks for this sort of thing
and ...
> +static int tegra_ahbdma_probe(struct platform_device *pdev)
> +{
> + struct dma_device *dma_dev;
> + struct tegra_ahbdma *tdma;
> + struct resource *res_regs;
> + unsigned int i;
> + int irq;
> + int err;
> +
> + tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma), GFP_KERNEL);
> + if (!tdma)
> + return -ENOMEM;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "Failed to get IRQ\n");
> + return irq;
> + }
> +
> + err = devm_request_irq(&pdev->dev, irq, tegra_ahbdma_isr, 0,
> + dev_name(&pdev->dev), tdma);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to request IRQ\n");
> + return -ENODEV;
> + }
> +
> + res_regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res_regs)
> + return -ENODEV;
> +
> + tdma->regs = devm_ioremap_resource(&pdev->dev, res_regs);
> + if (IS_ERR(tdma->regs))
> + return PTR_ERR(tdma->regs);
> +
> + tdma->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(tdma->clk)) {
> + dev_err(&pdev->dev, "Failed to get AHB-DMA clock\n");
> + return PTR_ERR(tdma->clk);
> + }
> +
> + tdma->rst = devm_reset_control_get(&pdev->dev, NULL);
> + if (IS_ERR(tdma->rst)) {
> + dev_err(&pdev->dev, "Failed to get AHB-DMA reset\n");
> + return PTR_ERR(tdma->rst);
> + }
> +
> + err = tegra_ahbdma_init_hw(tdma, &pdev->dev);
> + if (err)
> + return err;
... here is looks like we turn the clocks on and leave them on. I would
rather that we turn them on when the DMA channel is requested and turn
them off again when freed. Again would be good to use pm_runtime APIs
for this.
> + dma_dev = &tdma->dma_dev;
> +
> + INIT_LIST_HEAD(&dma_dev->channels);
> +
> + for (i = 0; i < ARRAY_SIZE(tdma->channels); i++)
> + tegra_ahbdma_init_channel(tdma, i);
> +
> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
> + dma_cap_set(DMA_CYCLIC, dma_dev->cap_mask);
> + dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
> +
> + dma_dev->max_burst = 8;
> + dma_dev->directions = TEGRA_AHBDMA_DIRECTIONS;
> + dma_dev->src_addr_widths = TEGRA_AHBDMA_BUS_WIDTH;
> + dma_dev->dst_addr_widths = TEGRA_AHBDMA_BUS_WIDTH;
> + dma_dev->descriptor_reuse = true;
> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> + dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources;
> + dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg;
> + dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic;
> + dma_dev->device_terminate_all = tegra_ahbdma_terminate_all;
> + dma_dev->device_issue_pending = tegra_ahbdma_issue_pending;
> + dma_dev->device_tx_status = tegra_ahbdma_tx_status;
> + dma_dev->device_config = tegra_ahbdma_config;
> + dma_dev->device_synchronize = tegra_ahbdma_synchronize;
> + dma_dev->dev = &pdev->dev;
> +
> + err = dma_async_device_register(dma_dev);
> + if (err) {
> + dev_err(&pdev->dev, "Device registration failed %d\n", err);
> + return err;
> + }
> +
> + err = of_dma_controller_register(pdev->dev.of_node,
> + tegra_ahbdma_of_xlate, tdma);
> + if (err) {
> + dev_err(&pdev->dev, "OF registration failed %d\n", err);
> + dma_async_device_unregister(dma_dev);
> + return err;
> + }
> +
> + platform_set_drvdata(pdev, tdma);
> +
> + return 0;
> +}
> +
> +static int tegra_ahbdma_remove(struct platform_device *pdev)
> +{
> + struct tegra_ahbdma *tdma = platform_get_drvdata(pdev);
> +
> + of_dma_controller_free(pdev->dev.of_node);
> + dma_async_device_unregister(&tdma->dma_dev);
> + clk_disable_unprepare(tdma->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id tegra_ahbdma_of_match[] = {
> + { .compatible = "nvidia,tegra20-ahbdma" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, tegra_ahbdma_of_match);
> +
> +static struct platform_driver tegra_ahbdma_driver = {
> + .driver = {
> + .name = "tegra-ahbdma",
> + .of_match_table = tegra_ahbdma_of_match,
It would be nice to have suspend/resume handler too. We could do a
similar thing to the APB dma driver.
> + },
> + .probe = tegra_ahbdma_probe,
> + .remove = tegra_ahbdma_remove,
> +};
> +module_platform_driver(tegra_ahbdma_driver);
> +
> +MODULE_DESCRIPTION("NVIDIA Tegra AHB DMA Controller driver");
> +MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
> +MODULE_LICENSE("GPL");
Cheers
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
2017-09-25 23:22 ` [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller Dmitry Osipenko
2017-09-26 14:45 ` Jon Hunter
@ 2017-09-28 9:29 ` Vinod Koul
2017-09-28 12:17 ` Dmitry Osipenko
2017-09-28 14:06 ` Dmitry Osipenko
1 sibling, 2 replies; 45+ messages in thread
From: Vinod Koul @ 2017-09-28 9:29 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel
On Tue, Sep 26, 2017 at 02:22:05AM +0300, Dmitry Osipenko wrote:
> +config TEGRA20_AHB_DMA
> + tristate "NVIDIA Tegra20 AHB DMA support"
> + depends on ARCH_TEGRA
Can we add COMPILE_TEST, helps me compile drivers
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
no vchan.h, so i presume we are not using that here, any reason why?
> +
> +#include "dmaengine.h"
> +
> +#define TEGRA_AHBDMA_CMD 0x0
> +#define TEGRA_AHBDMA_CMD_ENABLE BIT(31)
> +
> +#define TEGRA_AHBDMA_IRQ_ENB_MASK 0x20
> +#define TEGRA_AHBDMA_IRQ_ENB_CH(ch) BIT(ch)
> +
> +#define TEGRA_AHBDMA_CHANNEL_BASE(ch) (0x1000 + (ch) * 0x20)
> +
> +#define TEGRA_AHBDMA_CHANNEL_CSR 0x0
> +#define TEGRA_AHBDMA_CHANNEL_ADDR_WRAP BIT(18)
> +#define TEGRA_AHBDMA_CHANNEL_FLOW BIT(24)
> +#define TEGRA_AHBDMA_CHANNEL_ONCE BIT(26)
> +#define TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB BIT(27)
> +#define TEGRA_AHBDMA_CHANNEL_IE_EOC BIT(30)
> +#define TEGRA_AHBDMA_CHANNEL_ENABLE BIT(31)
> +#define TEGRA_AHBDMA_CHANNEL_REQ_SEL_SHIFT 16
> +#define TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK 0xFFFC
GENMASK() ?
> +static void tegra_ahbdma_tasklet(unsigned long data)
> +{
> + struct tegra_ahbdma_tx_desc *tx = (struct tegra_ahbdma_tx_desc *)data;
> + struct dma_async_tx_descriptor *desc = &tx->desc;
> +
> + dmaengine_desc_get_callback_invoke(desc, NULL);
> +
> + if (!tx->cyclic && !dmaengine_desc_test_reuse(desc))
> + kfree(tx);
lot of code here can be reduced if we use vchan
> +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic(
> + struct dma_chan *chan,
> + dma_addr_t buf_addr,
> + size_t buf_len,
> + size_t period_len,
> + enum dma_transfer_direction dir,
> + unsigned long flags)
> +{
> + struct tegra_ahbdma_tx_desc *tx;
> +
> + /* unimplemented */
> + if (buf_len != period_len || buf_len > SZ_64K)
> + return NULL;
> +
> + tx = kzalloc(sizeof(*tx), GFP_KERNEL);
> + if (!tx)
> + return NULL;
> +
> + dma_async_tx_descriptor_init(&tx->desc, chan);
> +
> + tx->desc.tx_submit = tegra_ahbdma_tx_submit;
> + tx->mem_paddr = buf_addr;
> + tx->size = buf_len;
> + tx->flags = flags;
> + tx->cyclic = true;
> + tx->dir = dir;
> +
> + tasklet_init(&tx->tasklet, tegra_ahbdma_tasklet, (unsigned long)tx);
why not precalulcate the register settings here. While submitting you are in
hot path keeping dmaengine idle so faster you can submit, better the perf
> +static void tegra_ahbdma_issue_pending(struct dma_chan *chan)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + struct tegra_ahbdma_tx_desc *tx;
> + struct list_head *entry, *tmp;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ahbdma_chan->lock, flags);
> +
> + list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list)
> + list_move_tail(entry, &ahbdma_chan->active_list);
> +
> + if (completion_done(&ahbdma_chan->idling)) {
> + tx = list_first_entry_or_null(&ahbdma_chan->active_list,
> + struct tegra_ahbdma_tx_desc,
> + node);
> + if (tx) {
> + tegra_ahbdma_submit_tx(ahbdma_chan, tx);
what is chan is already running?
> + reinit_completion(&ahbdma_chan->idling);
> + }
> + }
> +
> + spin_unlock_irqrestore(&ahbdma_chan->lock, flags);
> +}
> +
> +static enum dma_status tegra_ahbdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *state)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + struct tegra_ahbdma_tx_desc *tx;
> + enum dma_status cookie_status;
> + unsigned long flags;
> + size_t residual;
> + u32 status;
> +
> + spin_lock_irqsave(&ahbdma_chan->lock, flags);
> +
> + cookie_status = dma_cookie_status(chan, cookie, state);
> + if (cookie_status != DMA_COMPLETE) {
residue can be NULL so check it before proceeding ahead
> +static int tegra_ahbdma_config(struct dma_chan *chan,
> + struct dma_slave_config *sconfig)
> +{
> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
> + enum dma_transfer_direction dir = sconfig->direction;
> + u32 burst, ahb_seq, ahb_addr;
> +
> + if (sconfig->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
> + sconfig->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + return -EINVAL;
> +
> + if (dir == DMA_DEV_TO_MEM) {
> + burst = sconfig->src_maxburst;
> + ahb_addr = sconfig->src_addr;
> + } else {
> + burst = sconfig->dst_maxburst;
> + ahb_addr = sconfig->dst_addr;
> + }
> +
> + switch (burst) {
> + case 1: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_1; break;
> + case 4: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_4; break;
> + case 8: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_8; break;
pls make this statement and break on subsequent lines, readablity matters
> + default:
> + return -EINVAL;
> + }
> +
> + ahb_seq = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
> +
> + writel_relaxed(ahb_seq,
> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
> +
> + writel_relaxed(ahb_addr,
> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
oh no, you don't write to HW here. This can be called anytime when you have
txn running! You should save these and use them in prep_ calls.
> +static int tegra_ahbdma_remove(struct platform_device *pdev)
> +{
> + struct tegra_ahbdma *tdma = platform_get_drvdata(pdev);
> +
> + of_dma_controller_free(pdev->dev.of_node);
> + dma_async_device_unregister(&tdma->dma_dev);
> + clk_disable_unprepare(tdma->clk);
not ensuring tasklets are killed and irq is freed so no more tasklets can
run? I think that needs to be done...
> +MODULE_DESCRIPTION("NVIDIA Tegra AHB DMA Controller driver");
> +MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
> +MODULE_LICENSE("GPL");
MODULE_ALIAS?
--
~Vinod
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
2017-09-28 9:29 ` Vinod Koul
@ 2017-09-28 12:17 ` Dmitry Osipenko
2017-09-28 14:06 ` Dmitry Osipenko
1 sibling, 0 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-28 12:17 UTC (permalink / raw)
To: Vinod Koul
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 28.09.2017 12:29, Vinod Koul wrote:
> On Tue, Sep 26, 2017 at 02:22:05AM +0300, Dmitry Osipenko wrote:
>
>> +config TEGRA20_AHB_DMA
>> + tristate "NVIDIA Tegra20 AHB DMA support"
>> + depends on ARCH_TEGRA
>
> Can we add COMPILE_TEST, helps me compile drivers
>
Good point.
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_dma.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>
> no vchan.h, so i presume we are not using that here, any reason why?
>
Jon Hunter asked the same question, I already reworked driver to use the
virt-dma. Turned out it is a really neat helper, -100 lines of driver code.
>> +
>> +#include "dmaengine.h"
>> +
>> +#define TEGRA_AHBDMA_CMD 0x0
>> +#define TEGRA_AHBDMA_CMD_ENABLE BIT(31)
>> +
>> +#define TEGRA_AHBDMA_IRQ_ENB_MASK 0x20
>> +#define TEGRA_AHBDMA_IRQ_ENB_CH(ch) BIT(ch)
>> +
>> +#define TEGRA_AHBDMA_CHANNEL_BASE(ch) (0x1000 + (ch) * 0x20)
>> +
>> +#define TEGRA_AHBDMA_CHANNEL_CSR 0x0
>> +#define TEGRA_AHBDMA_CHANNEL_ADDR_WRAP BIT(18)
>> +#define TEGRA_AHBDMA_CHANNEL_FLOW BIT(24)
>> +#define TEGRA_AHBDMA_CHANNEL_ONCE BIT(26)
>> +#define TEGRA_AHBDMA_CHANNEL_DIR_TO_XMB BIT(27)
>> +#define TEGRA_AHBDMA_CHANNEL_IE_EOC BIT(30)
>> +#define TEGRA_AHBDMA_CHANNEL_ENABLE BIT(31)
>> +#define TEGRA_AHBDMA_CHANNEL_REQ_SEL_SHIFT 16
>> +#define TEGRA_AHBDMA_CHANNEL_WCOUNT_MASK 0xFFFC
>
> GENMASK() ?
>
Okay.
>> +static void tegra_ahbdma_tasklet(unsigned long data)
>> +{
>> + struct tegra_ahbdma_tx_desc *tx = (struct tegra_ahbdma_tx_desc *)data;
>> + struct dma_async_tx_descriptor *desc = &tx->desc;
>> +
>> + dmaengine_desc_get_callback_invoke(desc, NULL);
>> +
>> + if (!tx->cyclic && !dmaengine_desc_test_reuse(desc))
>> + kfree(tx);
>
> lot of code here can be reduced if we use vchan
>
+1
>> +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic(
>> + struct dma_chan *chan,
>> + dma_addr_t buf_addr,
>> + size_t buf_len,
>> + size_t period_len,
>> + enum dma_transfer_direction dir,
>> + unsigned long flags)
>> +{
>> + struct tegra_ahbdma_tx_desc *tx;
>> +
>> + /* unimplemented */
>> + if (buf_len != period_len || buf_len > SZ_64K)
>> + return NULL;
>> +
>> + tx = kzalloc(sizeof(*tx), GFP_KERNEL);
>> + if (!tx)
>> + return NULL;
>> +
>> + dma_async_tx_descriptor_init(&tx->desc, chan);
>> +
>> + tx->desc.tx_submit = tegra_ahbdma_tx_submit;
>> + tx->mem_paddr = buf_addr;
>> + tx->size = buf_len;
>> + tx->flags = flags;
>> + tx->cyclic = true;
>> + tx->dir = dir;
>> +
>> + tasklet_init(&tx->tasklet, tegra_ahbdma_tasklet, (unsigned long)tx);
>
> why not precalulcate the register settings here. While submitting you are in
> hot path keeping dmaengine idle so faster you can submit, better the perf
>
I may argue that the perf impact isn't measurable, but I agree that
precalculated register value would be a bit cleaner. Thanks for the suggestion.
>> +static void tegra_ahbdma_issue_pending(struct dma_chan *chan)
>> +{
>> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
>> + struct tegra_ahbdma_tx_desc *tx;
>> + struct list_head *entry, *tmp;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&ahbdma_chan->lock, flags);
>> +
>> + list_for_each_safe(entry, tmp, &ahbdma_chan->pending_list)
>> + list_move_tail(entry, &ahbdma_chan->active_list);
>> +
>> + if (completion_done(&ahbdma_chan->idling)) {
>> + tx = list_first_entry_or_null(&ahbdma_chan->active_list,
>> + struct tegra_ahbdma_tx_desc,
>> + node);
>> + if (tx) {
>> + tegra_ahbdma_submit_tx(ahbdma_chan, tx);
>
> what is chan is already running?
>
It can't run here, we just checked whether it is idling. That would be a HW bug.
>> + reinit_completion(&ahbdma_chan->idling);
>> + }
>> + }
>> +
>> + spin_unlock_irqrestore(&ahbdma_chan->lock, flags);
>> +}
>> +
>> +static enum dma_status tegra_ahbdma_tx_status(struct dma_chan *chan,
>> + dma_cookie_t cookie,
>> + struct dma_tx_state *state)
>> +{
>> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
>> + struct tegra_ahbdma_tx_desc *tx;
>> + enum dma_status cookie_status;
>> + unsigned long flags;
>> + size_t residual;
>> + u32 status;
>> +
>> + spin_lock_irqsave(&ahbdma_chan->lock, flags);
>> +
>> + cookie_status = dma_cookie_status(chan, cookie, state);
>> + if (cookie_status != DMA_COMPLETE) {
>
> residue can be NULL so check it before proceeding ahead
>
Yeah, I noticed it too and fixed it in the upcoming V2 yesterday.
>> +static int tegra_ahbdma_config(struct dma_chan *chan,
>> + struct dma_slave_config *sconfig)
>> +{
>> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
>> + enum dma_transfer_direction dir = sconfig->direction;
>> + u32 burst, ahb_seq, ahb_addr;
>> +
>> + if (sconfig->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
>> + sconfig->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
>> + return -EINVAL;
>> +
>> + if (dir == DMA_DEV_TO_MEM) {
>> + burst = sconfig->src_maxburst;
>> + ahb_addr = sconfig->src_addr;
>> + } else {
>> + burst = sconfig->dst_maxburst;
>> + ahb_addr = sconfig->dst_addr;
>> + }
>> +
>> + switch (burst) {
>> + case 1: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_1; break;
>> + case 4: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_4; break;
>> + case 8: burst = TEGRA_AHBDMA_CHANNEL_AHB_BURST_8; break;
>
> pls make this statement and break on subsequent lines, readablity matters
>
Okay.
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + ahb_seq = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
>> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
>> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
>> +
>> + writel_relaxed(ahb_seq,
>> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
>> +
>> + writel_relaxed(ahb_addr,
>> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
>
> oh no, you don't write to HW here. This can be called anytime when you have
> txn running! You should save these and use them in prep_ calls.
>
Okay.
>> +static int tegra_ahbdma_remove(struct platform_device *pdev)
>> +{
>> + struct tegra_ahbdma *tdma = platform_get_drvdata(pdev);
>> +
>> + of_dma_controller_free(pdev->dev.of_node);
>> + dma_async_device_unregister(&tdma->dma_dev);
>> + clk_disable_unprepare(tdma->clk);
>
> not ensuring tasklets are killed and irq is freed so no more tasklets can
> run? I think that needs to be done...
>
Already fixed in V2 by using vchan_synchronize() that kills tasklet in
tegra_ahbdma_synchronize(). DMA core invokes synchronization upon channels
resource freeing.
>> +MODULE_DESCRIPTION("NVIDIA Tegra AHB DMA Controller driver");
>> +MODULE_AUTHOR("Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
>> +MODULE_LICENSE("GPL");
>
> MODULE_ALIAS?
>
Not needed, driver is "OF-only". It's default alias is "tegra20-ahb-dma".
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
2017-09-28 9:29 ` Vinod Koul
2017-09-28 12:17 ` Dmitry Osipenko
@ 2017-09-28 14:06 ` Dmitry Osipenko
2017-09-28 14:35 ` Dmitry Osipenko
2017-09-28 16:21 ` Vinod Koul
1 sibling, 2 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-28 14:06 UTC (permalink / raw)
To: Vinod Koul
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 28.09.2017 12:29, Vinod Koul wrote:
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + ahb_seq = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
>> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
>> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
>> +
>> + writel_relaxed(ahb_seq,
>> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
>> +
>> + writel_relaxed(ahb_addr,
>> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
>
> oh no, you don't write to HW here. This can be called anytime when you have
> txn running! You should save these and use them in prep_ calls.
>
BTW, some of the DMA drivers have exactly the same problem. I now see that it is
actually documented explicitly in provider.txt, but that's inconsistent across
the actual drivers.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
2017-09-28 14:06 ` Dmitry Osipenko
@ 2017-09-28 14:35 ` Dmitry Osipenko
[not found] ` <260fa409-0d07-ec9e-9e3b-fb08255026d8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-09-28 16:21 ` Vinod Koul
1 sibling, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-28 14:35 UTC (permalink / raw)
To: Vinod Koul
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel
On 28.09.2017 17:06, Dmitry Osipenko wrote:
> On 28.09.2017 12:29, Vinod Koul wrote:
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ahb_seq = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
>>> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
>>> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
>>> +
>>> + writel_relaxed(ahb_seq,
>>> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
>>> +
>>> + writel_relaxed(ahb_addr,
>>> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
>>
>> oh no, you don't write to HW here. This can be called anytime when you have
>> txn running! You should save these and use them in prep_ calls.
>>
>
> BTW, some of the DMA drivers have exactly the same problem. I now see that it is
> actually documented explicitly in provider.txt, but that's inconsistent across
> the actual drivers.
>
Also, shouldn't prep_ and dma_slave_config be protected with locking? I don't
see DMA core doing any locking and seems none of the drivers too.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
2017-09-28 14:06 ` Dmitry Osipenko
2017-09-28 14:35 ` Dmitry Osipenko
@ 2017-09-28 16:21 ` Vinod Koul
1 sibling, 0 replies; 45+ messages in thread
From: Vinod Koul @ 2017-09-28 16:21 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel
On Thu, Sep 28, 2017 at 05:06:03PM +0300, Dmitry Osipenko wrote:
> On 28.09.2017 12:29, Vinod Koul wrote:
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ahb_seq = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
> >> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
> >> + ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
> >> +
> >> + writel_relaxed(ahb_seq,
> >> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
> >> +
> >> + writel_relaxed(ahb_addr,
> >> + ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
> >
> > oh no, you don't write to HW here. This can be called anytime when you have
> > txn running! You should save these and use them in prep_ calls.
> >
>
> BTW, some of the DMA drivers have exactly the same problem. I now see that it is
> actually documented explicitly in provider.txt, but that's inconsistent across
> the actual drivers.
yeah they need to be fixed!
--
~Vinod
^ permalink raw reply [flat|nested] 45+ messages in thread
[parent not found: <cover.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
[not found] ` <cover.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-09-25 23:22 ` Dmitry Osipenko
2017-09-26 14:50 ` Jon Hunter
[not found] ` <604d92036e0936443290e68a2226f935fb348113.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-09-25 23:22 ` [PATCH v1 5/5] ARM: dts: tegra: Add AHB DMA controller nodes Dmitry Osipenko
1 sibling, 2 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-25 23:22 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
on Tegra20/30 SoC's.
Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
.../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
new file mode 100644
index 000000000000..2af9aa76ae11
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
@@ -0,0 +1,23 @@
+* NVIDIA Tegra AHB DMA controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra20-ahbdma"
+- reg: Should contain registers base address and length.
+- interrupts: Should contain one entry, DMA controller interrupt.
+- clocks: Should contain one entry, DMA controller clock.
+- resets : Should contain one entry, DMA controller reset.
+- #dma-cells: Should be <1>. The cell represents DMA request select value
+ for the peripheral. For more details consult the Tegra TRM's
+ documentation, in particular AHB DMA channel control register
+ REQ_SEL field.
+
+Example:
+
+ahbdma: ahbdma@60008000 {
+ compatible = "nvidia,tegra20-ahbdma";
+ reg = <0x60008000 0x2000>;
+ interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&tegra_car TEGRA20_CLK_AHBDMA>;
+ resets = <&tegra_car 33>;
+ #dma-cells = <1>;
+};
--
2.14.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-25 23:22 ` [PATCH v1 3/5] dt-bindings: Add DT bindings " Dmitry Osipenko
@ 2017-09-26 14:50 ` Jon Hunter
2017-09-26 15:16 ` Dmitry Osipenko
[not found] ` <bee2a524-0891-01e1-4e03-f6cf6a89e6b1-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[not found] ` <604d92036e0936443290e68a2226f935fb348113.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
1 sibling, 2 replies; 45+ messages in thread
From: Jon Hunter @ 2017-09-26 14:50 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 26/09/17 00:22, Dmitry Osipenko wrote:
> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
> on Tegra20/30 SoC's.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>
> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
> new file mode 100644
> index 000000000000..2af9aa76ae11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
> @@ -0,0 +1,23 @@
> +* NVIDIA Tegra AHB DMA controller
> +
> +Required properties:
> +- compatible: Must be "nvidia,tegra20-ahbdma"
> +- reg: Should contain registers base address and length.
> +- interrupts: Should contain one entry, DMA controller interrupt.
> +- clocks: Should contain one entry, DMA controller clock.
> +- resets : Should contain one entry, DMA controller reset.
> +- #dma-cells: Should be <1>. The cell represents DMA request select value
> + for the peripheral. For more details consult the Tegra TRM's
> + documentation, in particular AHB DMA channel control register
> + REQ_SEL field.
What about the TRIG_SEL field? Do we need to handle this here as well?
Cheers
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-26 14:50 ` Jon Hunter
@ 2017-09-26 15:16 ` Dmitry Osipenko
[not found] ` <bee2a524-0891-01e1-4e03-f6cf6a89e6b1-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
1 sibling, 0 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-26 15:16 UTC (permalink / raw)
To: Jon Hunter, Thierry Reding, Laxman Dewangan, Peter De Schrijver,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 26.09.2017 17:50, Jon Hunter wrote:
>
> On 26/09/17 00:22, Dmitry Osipenko wrote:
>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>> on Tegra20/30 SoC's.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>
>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>> new file mode 100644
>> index 000000000000..2af9aa76ae11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>> @@ -0,0 +1,23 @@
>> +* NVIDIA Tegra AHB DMA controller
>> +
>> +Required properties:
>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>> +- reg: Should contain registers base address and length.
>> +- interrupts: Should contain one entry, DMA controller interrupt.
>> +- clocks: Should contain one entry, DMA controller clock.
>> +- resets : Should contain one entry, DMA controller reset.
>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>> + for the peripheral. For more details consult the Tegra TRM's
>> + documentation, in particular AHB DMA channel control register
>> + REQ_SEL field.
>
> What about the TRIG_SEL field? Do we need to handle this here as well?
>
I've followed APB DMA here, that HW also has TRIG_SEL but ignores it for some
reason. I think technically it should be present in the binding, yeah.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread[parent not found: <bee2a524-0891-01e1-4e03-f6cf6a89e6b1-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
[not found] ` <bee2a524-0891-01e1-4e03-f6cf6a89e6b1-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2017-09-27 1:57 ` Dmitry Osipenko
2017-09-27 8:34 ` Jon Hunter
0 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-27 1:57 UTC (permalink / raw)
To: Jon Hunter, Thierry Reding, Laxman Dewangan, Peter De Schrijver,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 26.09.2017 17:50, Jon Hunter wrote:
>
> On 26/09/17 00:22, Dmitry Osipenko wrote:
>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>> on Tegra20/30 SoC's.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>
>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>> new file mode 100644
>> index 000000000000..2af9aa76ae11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>> @@ -0,0 +1,23 @@
>> +* NVIDIA Tegra AHB DMA controller
>> +
>> +Required properties:
>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>> +- reg: Should contain registers base address and length.
>> +- interrupts: Should contain one entry, DMA controller interrupt.
>> +- clocks: Should contain one entry, DMA controller clock.
>> +- resets : Should contain one entry, DMA controller reset.
>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>> + for the peripheral. For more details consult the Tegra TRM's
>> + documentation, in particular AHB DMA channel control register
>> + REQ_SEL field.
>
> What about the TRIG_SEL field? Do we need to handle this here as well?
>
Actually, DMA transfer trigger isn't related a hardware description. It's up to
software to decide what trigger to select. So it shouldn't be in the binding.
And I think the same applies to requester... any objections?
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-27 1:57 ` Dmitry Osipenko
@ 2017-09-27 8:34 ` Jon Hunter
2017-09-27 12:12 ` Dmitry Osipenko
` (2 more replies)
0 siblings, 3 replies; 45+ messages in thread
From: Jon Hunter @ 2017-09-27 8:34 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 27/09/17 02:57, Dmitry Osipenko wrote:
> On 26.09.2017 17:50, Jon Hunter wrote:
>>
>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>> on Tegra20/30 SoC's.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
>>> 1 file changed, 23 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>> new file mode 100644
>>> index 000000000000..2af9aa76ae11
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>> @@ -0,0 +1,23 @@
>>> +* NVIDIA Tegra AHB DMA controller
>>> +
>>> +Required properties:
>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>> +- reg: Should contain registers base address and length.
>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>> +- clocks: Should contain one entry, DMA controller clock.
>>> +- resets : Should contain one entry, DMA controller reset.
>>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>>> + for the peripheral. For more details consult the Tegra TRM's
>>> + documentation, in particular AHB DMA channel control register
>>> + REQ_SEL field.
>>
>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>
>
> Actually, DMA transfer trigger isn't related a hardware description. It's up to
> software to decide what trigger to select. So it shouldn't be in the binding.
I think it could be, if say a board wanted a GPIO to trigger a transfer.
> And I think the same applies to requester... any objections?
Well, the REQ_SEL should definitely be in the binding.
Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
like we never bothered with it for the APB DMA and so maybe no ones uses
this.
Cheers
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-27 8:34 ` Jon Hunter
@ 2017-09-27 12:12 ` Dmitry Osipenko
[not found] ` <69ea8dec-db7a-fcfa-6fa7-ea70de4c9ef4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-09-27 23:32 ` Stephen Boyd
[not found] ` <0fd316e9-3584-e9bd-2a8b-e73eaa6a9a48-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-27 12:12 UTC (permalink / raw)
To: Jon Hunter, Thierry Reding, Laxman Dewangan, Peter De Schrijver,
Prashant Gaikwad, Michael Turquette, Stephen Boyd, Rob Herring,
Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 27.09.2017 11:34, Jon Hunter wrote:
>
> On 27/09/17 02:57, Dmitry Osipenko wrote:
>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>
>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>> on Tegra20/30 SoC's.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
>>>> 1 file changed, 23 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>> new file mode 100644
>>>> index 000000000000..2af9aa76ae11
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>> @@ -0,0 +1,23 @@
>>>> +* NVIDIA Tegra AHB DMA controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>> +- reg: Should contain registers base address and length.
>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>> +- resets : Should contain one entry, DMA controller reset.
>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>> + documentation, in particular AHB DMA channel control register
>>>> + REQ_SEL field.
>>>
>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>
>>
>> Actually, DMA transfer trigger isn't related a hardware description. It's up to
>> software to decide what trigger to select. So it shouldn't be in the binding.
>
> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>
GPIO isn't a very good example, there is no "GPIO" trigger. To me all triggers
are software-defined, so that software could create transfer chains.
>> And I think the same applies to requester... any objections?
>
> Well, the REQ_SEL should definitely be in the binding.
>
Okay.
> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
> like we never bothered with it for the APB DMA and so maybe no ones uses
> this.
>
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-27 8:34 ` Jon Hunter
2017-09-27 12:12 ` Dmitry Osipenko
@ 2017-09-27 23:32 ` Stephen Boyd
2017-09-28 8:33 ` Jon Hunter
[not found] ` <0fd316e9-3584-e9bd-2a8b-e73eaa6a9a48-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2 siblings, 1 reply; 45+ messages in thread
From: Stephen Boyd @ 2017-09-27 23:32 UTC (permalink / raw)
To: Jon Hunter
Cc: Dmitry Osipenko, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Rob Herring, Vinod Koul, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel
On 09/27, Jon Hunter wrote:
>
>
> Well, the REQ_SEL should definitely be in the binding.
>
> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
Wrong Stephen?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-27 23:32 ` Stephen Boyd
@ 2017-09-28 8:33 ` Jon Hunter
0 siblings, 0 replies; 45+ messages in thread
From: Jon Hunter @ 2017-09-28 8:33 UTC (permalink / raw)
To: Stephen Boyd
Cc: Dmitry Osipenko, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Rob Herring, Vinod Koul, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel, Stephen Warren
On 28/09/17 00:32, Stephen Boyd wrote:
> On 09/27, Jon Hunter wrote:
>>
>>
>> Well, the REQ_SEL should definitely be in the binding.
>>
>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>
> Wrong Stephen?
Indeed! With all the folks in copy, I assumed we had the right one :-)
Cheers
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 45+ messages in thread
[parent not found: <0fd316e9-3584-e9bd-2a8b-e73eaa6a9a48-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
[not found] ` <0fd316e9-3584-e9bd-2a8b-e73eaa6a9a48-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2017-09-29 19:30 ` Stephen Warren
2017-09-30 3:11 ` Dmitry Osipenko
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Warren @ 2017-09-29 19:30 UTC (permalink / raw)
To: Jon Hunter, Dmitry Osipenko, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 09/27/2017 02:34 AM, Jon Hunter wrote:
>
> On 27/09/17 02:57, Dmitry Osipenko wrote:
>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>
>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>> on Tegra20/30 SoC's.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
>>>> 1 file changed, 23 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>> new file mode 100644
>>>> index 000000000000..2af9aa76ae11
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>> @@ -0,0 +1,23 @@
>>>> +* NVIDIA Tegra AHB DMA controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>> +- reg: Should contain registers base address and length.
>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>> +- resets : Should contain one entry, DMA controller reset.
>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>> + documentation, in particular AHB DMA channel control register
>>>> + REQ_SEL field.
>>>
>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>
>>
>> Actually, DMA transfer trigger isn't related a hardware description. It's up to
>> software to decide what trigger to select. So it shouldn't be in the binding.
>
> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>
>> And I think the same applies to requester... any objections?
>
> Well, the REQ_SEL should definitely be in the binding.
>
> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
> like we never bothered with it for the APB DMA and so maybe no ones uses
> this.
I don't think TRIG_SEL should be in the binding, at least at present.
While TRIG_SEL certainly is something used to configure the transfer, I
believe the semantics of the current DMA binding only cover DMA
transfers that are initiated when SW desires, rather than being a
combination of after SW programs the transfer plus some other HW event.
So, we always use a default/hard-coded TRIG_SEL value. As such, there's
no need for a TRIG_SEL value in DT. There's certainly no known use-case
that requires a non-default TRIG_SEL value at present. We could add an
extra #dma-cells value later if we find a use for it, and the semantics
of that use-case make sense to add it to the DMA specifier, rather than
some other separate higher-level property/driver/...
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-29 19:30 ` Stephen Warren
@ 2017-09-30 3:11 ` Dmitry Osipenko
2017-10-02 17:05 ` Stephen Warren
0 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-30 3:11 UTC (permalink / raw)
To: Stephen Warren, Jon Hunter, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 29.09.2017 22:30, Stephen Warren wrote:
> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>
>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>
>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>> on Tegra20/30 SoC's.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>> ++++++++++++++++++++++
>>>>> 1 file changed, 23 insertions(+)
>>>>> create mode 100644
>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>> new file mode 100644
>>>>> index 000000000000..2af9aa76ae11
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>> @@ -0,0 +1,23 @@
>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>> +- reg: Should contain registers base address and length.
>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>> + documentation, in particular AHB DMA channel control register
>>>>> + REQ_SEL field.
>>>>
>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>
>>>
>>> Actually, DMA transfer trigger isn't related a hardware description. It's up to
>>> software to decide what trigger to select. So it shouldn't be in the binding.
>>
>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>
>>> And I think the same applies to requester... any objections?
>>
>> Well, the REQ_SEL should definitely be in the binding.
>>
>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>> like we never bothered with it for the APB DMA and so maybe no ones uses
>> this.
>
> I don't think TRIG_SEL should be in the binding, at least at present. While
> TRIG_SEL certainly is something used to configure the transfer, I believe the
> semantics of the current DMA binding only cover DMA transfers that are initiated
> when SW desires, rather than being a combination of after SW programs the
> transfer plus some other HW event. So, we always use a default/hard-coded
> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
> certainly no known use-case that requires a non-default TRIG_SEL value at
> present. We could add an extra #dma-cells value later if we find a use for it,
> and the semantics of that use-case make sense to add it to the DMA specifier,
> rather than some other separate higher-level property/driver/...
Thank you for the comment. If we'd want to extend the binding further with the
trigger, how to differentiate trigger from the requester in a case of a single
#data-cell?
Of course realistically a chance that the further extension would be needed is
very-very low, so we may defer the efforts to solve that question and for now
make driver aware of the potential #dma-cells extension.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-09-30 3:11 ` Dmitry Osipenko
@ 2017-10-02 17:05 ` Stephen Warren
2017-10-02 23:02 ` Dmitry Osipenko
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Warren @ 2017-10-02 17:05 UTC (permalink / raw)
To: Dmitry Osipenko, Jon Hunter, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 09/29/2017 09:11 PM, Dmitry Osipenko wrote:
> On 29.09.2017 22:30, Stephen Warren wrote:
>> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>>
>>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>>
>>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>>> on Tegra20/30 SoC's.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>>> ++++++++++++++++++++++
>>>>>> 1 file changed, 23 insertions(+)
>>>>>> create mode 100644
>>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..2af9aa76ae11
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>> @@ -0,0 +1,23 @@
>>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>>> +- reg: Should contain registers base address and length.
>>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>>> + documentation, in particular AHB DMA channel control register
>>>>>> + REQ_SEL field.
>>>>>
>>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>>
>>>>
>>>> Actually, DMA transfer trigger isn't related a hardware description. It's up to
>>>> software to decide what trigger to select. So it shouldn't be in the binding.
>>>
>>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>>
>>>> And I think the same applies to requester... any objections?
>>>
>>> Well, the REQ_SEL should definitely be in the binding.
>>>
>>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>>> like we never bothered with it for the APB DMA and so maybe no ones uses
>>> this.
>>
>> I don't think TRIG_SEL should be in the binding, at least at present. While
>> TRIG_SEL certainly is something used to configure the transfer, I believe the
>> semantics of the current DMA binding only cover DMA transfers that are initiated
>> when SW desires, rather than being a combination of after SW programs the
>> transfer plus some other HW event. So, we always use a default/hard-coded
>> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
>> certainly no known use-case that requires a non-default TRIG_SEL value at
>> present. We could add an extra #dma-cells value later if we find a use for it,
>> and the semantics of that use-case make sense to add it to the DMA specifier,
>> rather than some other separate higher-level property/driver/...
>
> Thank you for the comment. If we'd want to extend the binding further with the
> trigger, how to differentiate trigger from the requester in a case of a single
> #data-cell?
>
> Of course realistically a chance that the further extension would be needed is
> very-very low, so we may defer the efforts to solve that question and for now
> make driver aware of the potential #dma-cells extension.
The request selector cell isn't optional, so is always present. If we
later add an optional trig_sel cell, we'll either have:
#dma-cells=<1>: req_sel
or:
#dma-cells=<2>: req_sel, trig_sel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-10-02 17:05 ` Stephen Warren
@ 2017-10-02 23:02 ` Dmitry Osipenko
2017-10-03 10:32 ` Jon Hunter
0 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-10-02 23:02 UTC (permalink / raw)
To: Stephen Warren, Jon Hunter, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 02.10.2017 20:05, Stephen Warren wrote:
> On 09/29/2017 09:11 PM, Dmitry Osipenko wrote:
>> On 29.09.2017 22:30, Stephen Warren wrote:
>>> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>>>
>>>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>>>
>>>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>>>> on Tegra20/30 SoC's.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> ---
>>>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>>>> ++++++++++++++++++++++
>>>>>>> 1 file changed, 23 insertions(+)
>>>>>>> create mode 100644
>>>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..2af9aa76ae11
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>> @@ -0,0 +1,23 @@
>>>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>>>> +- reg: Should contain registers base address and length.
>>>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select
>>>>>>> value
>>>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>>>> + documentation, in particular AHB DMA channel control register
>>>>>>> + REQ_SEL field.
>>>>>>
>>>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>>>
>>>>>
>>>>> Actually, DMA transfer trigger isn't related a hardware description. It's
>>>>> up to
>>>>> software to decide what trigger to select. So it shouldn't be in the binding.
>>>>
>>>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>>>
>>>>> And I think the same applies to requester... any objections?
>>>>
>>>> Well, the REQ_SEL should definitely be in the binding.
>>>>
>>>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>>>> like we never bothered with it for the APB DMA and so maybe no ones uses
>>>> this.
>>>
>>> I don't think TRIG_SEL should be in the binding, at least at present. While
>>> TRIG_SEL certainly is something used to configure the transfer, I believe the
>>> semantics of the current DMA binding only cover DMA transfers that are initiated
>>> when SW desires, rather than being a combination of after SW programs the
>>> transfer plus some other HW event. So, we always use a default/hard-coded
>>> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
>>> certainly no known use-case that requires a non-default TRIG_SEL value at
>>> present. We could add an extra #dma-cells value later if we find a use for it,
>>> and the semantics of that use-case make sense to add it to the DMA specifier,
>>> rather than some other separate higher-level property/driver/...
>>
>> Thank you for the comment. If we'd want to extend the binding further with the
>> trigger, how to differentiate trigger from the requester in a case of a single
>> #data-cell?
>>
>> Of course realistically a chance that the further extension would be needed is
>> very-very low, so we may defer the efforts to solve that question and for now
>> make driver aware of the potential #dma-cells extension.
>
> The request selector cell isn't optional, so is always present. If we later add
> an optional trig_sel cell, we'll either have:
>
> #dma-cells=<1>: req_sel
>
> or:
>
> #dma-cells=<2>: req_sel, trig_sel
Why request sel. couldn't be optional? Could you please elaborate a bit more?
I think possible options are:
#dma-cells=<1>: req_sel
#dma-cells=<1>: trig_sel
#dma-cells=<2>: req_sel, trig_sel
The only difference between request and trigger is that trigger issues the whole
transfer, while request only a single burst. Isn't it possible to have a case in
HW for the "trigger-only" option? If not or it's a rareness, then I agree that
REQ_SEL must be mandatory.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-10-02 23:02 ` Dmitry Osipenko
@ 2017-10-03 10:32 ` Jon Hunter
[not found] ` <4443a8fb-7a4d-922b-2dd3-53236d39a050-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2017-10-03 15:38 ` Stephen Warren
0 siblings, 2 replies; 45+ messages in thread
From: Jon Hunter @ 2017-10-03 10:32 UTC (permalink / raw)
To: Dmitry Osipenko, Stephen Warren, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 03/10/17 00:02, Dmitry Osipenko wrote:
> On 02.10.2017 20:05, Stephen Warren wrote:
>> On 09/29/2017 09:11 PM, Dmitry Osipenko wrote:
>>> On 29.09.2017 22:30, Stephen Warren wrote:
>>>> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>>>>
>>>>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>>>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>>>>> on Tegra20/30 SoC's.
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>> ---
>>>>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>>>>> ++++++++++++++++++++++
>>>>>>>> 1 file changed, 23 insertions(+)
>>>>>>>> create mode 100644
>>>>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..2af9aa76ae11
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>> @@ -0,0 +1,23 @@
>>>>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>>>>> +- reg: Should contain registers base address and length.
>>>>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select
>>>>>>>> value
>>>>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>>>>> + documentation, in particular AHB DMA channel control register
>>>>>>>> + REQ_SEL field.
>>>>>>>
>>>>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>>>>
>>>>>>
>>>>>> Actually, DMA transfer trigger isn't related a hardware description. It's
>>>>>> up to
>>>>>> software to decide what trigger to select. So it shouldn't be in the binding.
>>>>>
>>>>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>>>>
>>>>>> And I think the same applies to requester... any objections?
>>>>>
>>>>> Well, the REQ_SEL should definitely be in the binding.
>>>>>
>>>>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>>>>> like we never bothered with it for the APB DMA and so maybe no ones uses
>>>>> this.
>>>>
>>>> I don't think TRIG_SEL should be in the binding, at least at present. While
>>>> TRIG_SEL certainly is something used to configure the transfer, I believe the
>>>> semantics of the current DMA binding only cover DMA transfers that are initiated
>>>> when SW desires, rather than being a combination of after SW programs the
>>>> transfer plus some other HW event. So, we always use a default/hard-coded
>>>> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
>>>> certainly no known use-case that requires a non-default TRIG_SEL value at
>>>> present. We could add an extra #dma-cells value later if we find a use for it,
>>>> and the semantics of that use-case make sense to add it to the DMA specifier,
>>>> rather than some other separate higher-level property/driver/...
>>>
>>> Thank you for the comment. If we'd want to extend the binding further with the
>>> trigger, how to differentiate trigger from the requester in a case of a single
>>> #data-cell?
>>>
>>> Of course realistically a chance that the further extension would be needed is
>>> very-very low, so we may defer the efforts to solve that question and for now
>>> make driver aware of the potential #dma-cells extension.
>>
>> The request selector cell isn't optional, so is always present. If we later add
>> an optional trig_sel cell, we'll either have:
>>
>> #dma-cells=<1>: req_sel
>>
>> or:
>>
>> #dma-cells=<2>: req_sel, trig_sel
>
> Why request sel. couldn't be optional? Could you please elaborate a bit more?
>
> I think possible options are:
>
> #dma-cells=<1>: req_sel
> #dma-cells=<1>: trig_sel
With the above, how would you know that it is the req_sel or trig_sel
that is specified?
> #dma-cells=<2>: req_sel, trig_sel
>
> The only difference between request and trigger is that trigger issues the whole
> transfer, while request only a single burst. Isn't it possible to have a case in
> HW for the "trigger-only" option? If not or it's a rareness, then I agree that
> REQ_SEL must be mandatory.
I think that what Stephen is proposing is that for now we go with
'#dma-cells=<1>' and if we ever need to support the trigger cell we
could add support for '#dma-cells=<2>'. So with this proposal the
'req_sel' would always be required for both '#dma-cells=<1>' and
'#dma-cells=<2>'. Even if the req_sel is not actually used but the
'trig_sel' is, the user would have to set 'req_sel' to some pre-defined
value (eg. -1) where we know to ignore it.
Cheers
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 45+ messages in thread[parent not found: <4443a8fb-7a4d-922b-2dd3-53236d39a050-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
[not found] ` <4443a8fb-7a4d-922b-2dd3-53236d39a050-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2017-10-03 12:07 ` Dmitry Osipenko
2017-10-03 12:19 ` Jon Hunter
0 siblings, 1 reply; 45+ messages in thread
From: Dmitry Osipenko @ 2017-10-03 12:07 UTC (permalink / raw)
To: Jon Hunter, Stephen Warren, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 03.10.2017 13:32, Jon Hunter wrote:
>
>
> On 03/10/17 00:02, Dmitry Osipenko wrote:
>> On 02.10.2017 20:05, Stephen Warren wrote:
>>> On 09/29/2017 09:11 PM, Dmitry Osipenko wrote:
>>>> On 29.09.2017 22:30, Stephen Warren wrote:
>>>>> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>>>>>
>>>>>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>>>>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>>>>>
>>>>>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>>>>>> on Tegra20/30 SoC's.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>>>> ---
>>>>>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>> 1 file changed, 23 insertions(+)
>>>>>>>>> create mode 100644
>>>>>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..2af9aa76ae11
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>> @@ -0,0 +1,23 @@
>>>>>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>>>>>> +- reg: Should contain registers base address and length.
>>>>>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select
>>>>>>>>> value
>>>>>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>>>>>> + documentation, in particular AHB DMA channel control register
>>>>>>>>> + REQ_SEL field.
>>>>>>>>
>>>>>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>>>>>
>>>>>>>
>>>>>>> Actually, DMA transfer trigger isn't related a hardware description. It's
>>>>>>> up to
>>>>>>> software to decide what trigger to select. So it shouldn't be in the binding.
>>>>>>
>>>>>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>>>>>
>>>>>>> And I think the same applies to requester... any objections?
>>>>>>
>>>>>> Well, the REQ_SEL should definitely be in the binding.
>>>>>>
>>>>>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>>>>>> like we never bothered with it for the APB DMA and so maybe no ones uses
>>>>>> this.
>>>>>
>>>>> I don't think TRIG_SEL should be in the binding, at least at present. While
>>>>> TRIG_SEL certainly is something used to configure the transfer, I believe the
>>>>> semantics of the current DMA binding only cover DMA transfers that are initiated
>>>>> when SW desires, rather than being a combination of after SW programs the
>>>>> transfer plus some other HW event. So, we always use a default/hard-coded
>>>>> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
>>>>> certainly no known use-case that requires a non-default TRIG_SEL value at
>>>>> present. We could add an extra #dma-cells value later if we find a use for it,
>>>>> and the semantics of that use-case make sense to add it to the DMA specifier,
>>>>> rather than some other separate higher-level property/driver/...
>>>>
>>>> Thank you for the comment. If we'd want to extend the binding further with the
>>>> trigger, how to differentiate trigger from the requester in a case of a single
>>>> #data-cell?
>>>>
>>>> Of course realistically a chance that the further extension would be needed is
>>>> very-very low, so we may defer the efforts to solve that question and for now
>>>> make driver aware of the potential #dma-cells extension.
>>>
>>> The request selector cell isn't optional, so is always present. If we later add
>>> an optional trig_sel cell, we'll either have:
>>>
>>> #dma-cells=<1>: req_sel
>>>
>>> or:
>>>
>>> #dma-cells=<2>: req_sel, trig_sel
>>
>> Why request sel. couldn't be optional? Could you please elaborate a bit more?
>>
>> I think possible options are:
>>
>> #dma-cells=<1>: req_sel
>> #dma-cells=<1>: trig_sel
>
> With the above, how would you know that it is the req_sel or trig_sel
> that is specified?
>
>> #dma-cells=<2>: req_sel, trig_sel
>>
>> The only difference between request and trigger is that trigger issues the whole
>> transfer, while request only a single burst. Isn't it possible to have a case in
>> HW for the "trigger-only" option? If not or it's a rareness, then I agree that
>> REQ_SEL must be mandatory.
>
> I think that what Stephen is proposing is that for now we go with
> '#dma-cells=<1>' and if we ever need to support the trigger cell we
> could add support for '#dma-cells=<2>'. So with this proposal the
> 'req_sel' would always be required for both '#dma-cells=<1>' and
> '#dma-cells=<2>'. Even if the req_sel is not actually used but the
> 'trig_sel' is, the user would have to set 'req_sel' to some pre-defined
> value (eg. -1) where we know to ignore it.
>
Okay, I see now. Thank you for the clarification, but then we should have that
pre-defined value declared in the binding?
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-10-03 12:07 ` Dmitry Osipenko
@ 2017-10-03 12:19 ` Jon Hunter
0 siblings, 0 replies; 45+ messages in thread
From: Jon Hunter @ 2017-10-03 12:19 UTC (permalink / raw)
To: Dmitry Osipenko, Stephen Warren, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 03/10/17 13:07, Dmitry Osipenko wrote:
> On 03.10.2017 13:32, Jon Hunter wrote:
>>
>>
>> On 03/10/17 00:02, Dmitry Osipenko wrote:
>>> On 02.10.2017 20:05, Stephen Warren wrote:
>>>> On 09/29/2017 09:11 PM, Dmitry Osipenko wrote:
>>>>> On 29.09.2017 22:30, Stephen Warren wrote:
>>>>>> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>>>>>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>>>>>>
>>>>>>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>>>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>>>>>>> on Tegra20/30 SoC's.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>>> 1 file changed, 23 insertions(+)
>>>>>>>>>> create mode 100644
>>>>>>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..2af9aa76ae11
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>> @@ -0,0 +1,23 @@
>>>>>>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>>>>>>> +
>>>>>>>>>> +Required properties:
>>>>>>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>>>>>>> +- reg: Should contain registers base address and length.
>>>>>>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>>>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>>>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>>>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select
>>>>>>>>>> value
>>>>>>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>>>>>>> + documentation, in particular AHB DMA channel control register
>>>>>>>>>> + REQ_SEL field.
>>>>>>>>>
>>>>>>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Actually, DMA transfer trigger isn't related a hardware description. It's
>>>>>>>> up to
>>>>>>>> software to decide what trigger to select. So it shouldn't be in the binding.
>>>>>>>
>>>>>>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>>>>>>
>>>>>>>> And I think the same applies to requester... any objections?
>>>>>>>
>>>>>>> Well, the REQ_SEL should definitely be in the binding.
>>>>>>>
>>>>>>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>>>>>>> like we never bothered with it for the APB DMA and so maybe no ones uses
>>>>>>> this.
>>>>>>
>>>>>> I don't think TRIG_SEL should be in the binding, at least at present. While
>>>>>> TRIG_SEL certainly is something used to configure the transfer, I believe the
>>>>>> semantics of the current DMA binding only cover DMA transfers that are initiated
>>>>>> when SW desires, rather than being a combination of after SW programs the
>>>>>> transfer plus some other HW event. So, we always use a default/hard-coded
>>>>>> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
>>>>>> certainly no known use-case that requires a non-default TRIG_SEL value at
>>>>>> present. We could add an extra #dma-cells value later if we find a use for it,
>>>>>> and the semantics of that use-case make sense to add it to the DMA specifier,
>>>>>> rather than some other separate higher-level property/driver/...
>>>>>
>>>>> Thank you for the comment. If we'd want to extend the binding further with the
>>>>> trigger, how to differentiate trigger from the requester in a case of a single
>>>>> #data-cell?
>>>>>
>>>>> Of course realistically a chance that the further extension would be needed is
>>>>> very-very low, so we may defer the efforts to solve that question and for now
>>>>> make driver aware of the potential #dma-cells extension.
>>>>
>>>> The request selector cell isn't optional, so is always present. If we later add
>>>> an optional trig_sel cell, we'll either have:
>>>>
>>>> #dma-cells=<1>: req_sel
>>>>
>>>> or:
>>>>
>>>> #dma-cells=<2>: req_sel, trig_sel
>>>
>>> Why request sel. couldn't be optional? Could you please elaborate a bit more?
>>>
>>> I think possible options are:
>>>
>>> #dma-cells=<1>: req_sel
>>> #dma-cells=<1>: trig_sel
>>
>> With the above, how would you know that it is the req_sel or trig_sel
>> that is specified?
>>
>>> #dma-cells=<2>: req_sel, trig_sel
>>>
>>> The only difference between request and trigger is that trigger issues the whole
>>> transfer, while request only a single burst. Isn't it possible to have a case in
>>> HW for the "trigger-only" option? If not or it's a rareness, then I agree that
>>> REQ_SEL must be mandatory.
>>
>> I think that what Stephen is proposing is that for now we go with
>> '#dma-cells=<1>' and if we ever need to support the trigger cell we
>> could add support for '#dma-cells=<2>'. So with this proposal the
>> 'req_sel' would always be required for both '#dma-cells=<1>' and
>> '#dma-cells=<2>'. Even if the req_sel is not actually used but the
>> 'trig_sel' is, the user would have to set 'req_sel' to some pre-defined
>> value (eg. -1) where we know to ignore it.
>>
>
> Okay, I see now. Thank you for the clarification, but then we should have that
> pre-defined value declared in the binding?
I would have thought it should be in the dt-binding header.
Cheers
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-10-03 10:32 ` Jon Hunter
[not found] ` <4443a8fb-7a4d-922b-2dd3-53236d39a050-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2017-10-03 15:38 ` Stephen Warren
2017-10-03 17:04 ` Dmitry Osipenko
1 sibling, 1 reply; 45+ messages in thread
From: Stephen Warren @ 2017-10-03 15:38 UTC (permalink / raw)
To: Jon Hunter, Dmitry Osipenko, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 10/03/2017 04:32 AM, Jon Hunter wrote:
>
>
> On 03/10/17 00:02, Dmitry Osipenko wrote:
>> On 02.10.2017 20:05, Stephen Warren wrote:
>>> On 09/29/2017 09:11 PM, Dmitry Osipenko wrote:
>>>> On 29.09.2017 22:30, Stephen Warren wrote:
>>>>> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>>>>>
>>>>>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>>>>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>>>>>
>>>>>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>>>>>> on Tegra20/30 SoC's.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> ---
>>>>>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>> 1 file changed, 23 insertions(+)
>>>>>>>>> create mode 100644
>>>>>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..2af9aa76ae11
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>> @@ -0,0 +1,23 @@
>>>>>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>>>>>> +- reg: Should contain registers base address and length.
>>>>>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select
>>>>>>>>> value
>>>>>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>>>>>> + documentation, in particular AHB DMA channel control register
>>>>>>>>> + REQ_SEL field.
>>>>>>>>
>>>>>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>>>>>
>>>>>>>
>>>>>>> Actually, DMA transfer trigger isn't related a hardware description. It's
>>>>>>> up to
>>>>>>> software to decide what trigger to select. So it shouldn't be in the binding.
>>>>>>
>>>>>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>>>>>
>>>>>>> And I think the same applies to requester... any objections?
>>>>>>
>>>>>> Well, the REQ_SEL should definitely be in the binding.
>>>>>>
>>>>>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>>>>>> like we never bothered with it for the APB DMA and so maybe no ones uses
>>>>>> this.
>>>>>
>>>>> I don't think TRIG_SEL should be in the binding, at least at present. While
>>>>> TRIG_SEL certainly is something used to configure the transfer, I believe the
>>>>> semantics of the current DMA binding only cover DMA transfers that are initiated
>>>>> when SW desires, rather than being a combination of after SW programs the
>>>>> transfer plus some other HW event. So, we always use a default/hard-coded
>>>>> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
>>>>> certainly no known use-case that requires a non-default TRIG_SEL value at
>>>>> present. We could add an extra #dma-cells value later if we find a use for it,
>>>>> and the semantics of that use-case make sense to add it to the DMA specifier,
>>>>> rather than some other separate higher-level property/driver/...
>>>>
>>>> Thank you for the comment. If we'd want to extend the binding further with the
>>>> trigger, how to differentiate trigger from the requester in a case of a single
>>>> #data-cell?
>>>>
>>>> Of course realistically a chance that the further extension would be needed is
>>>> very-very low, so we may defer the efforts to solve that question and for now
>>>> make driver aware of the potential #dma-cells extension.
>>>
>>> The request selector cell isn't optional, so is always present. If we later add
>>> an optional trig_sel cell, we'll either have:
>>>
>>> #dma-cells=<1>: req_sel
>>>
>>> or:
>>>
>>> #dma-cells=<2>: req_sel, trig_sel
>>
>> Why request sel. couldn't be optional? Could you please elaborate a bit more?
The documentation currently says it's mandatory, and DT bindings must be
evolved in a backwards-compatible fashion.
>> I think possible options are:
>>
>> #dma-cells=<1>: req_sel
>> #dma-cells=<1>: trig_sel
>
> With the above, how would you know that it is the req_sel or trig_sel
> that is specified?
Also, if req_sel were optional, then it'd be impossible to distinguish
between those cases, so we can't design a binding like that. In general,
when adding extra optional cells to an #xxx-cells style binding, then
whenever cell N is present, all cells before cell N must be present even
if optional.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-10-03 15:38 ` Stephen Warren
@ 2017-10-03 17:04 ` Dmitry Osipenko
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-10-03 17:04 UTC (permalink / raw)
To: Stephen Warren, Jon Hunter, Thierry Reding, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra, devicetree, dmaengine, linux-clk, linux-kernel
On 03.10.2017 18:38, Stephen Warren wrote:
> On 10/03/2017 04:32 AM, Jon Hunter wrote:
>>
>>
>> On 03/10/17 00:02, Dmitry Osipenko wrote:
>>> On 02.10.2017 20:05, Stephen Warren wrote:
>>>> On 09/29/2017 09:11 PM, Dmitry Osipenko wrote:
>>>>> On 29.09.2017 22:30, Stephen Warren wrote:
>>>>>> On 09/27/2017 02:34 AM, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 27/09/17 02:57, Dmitry Osipenko wrote:
>>>>>>>> On 26.09.2017 17:50, Jon Hunter wrote:
>>>>>>>>>
>>>>>>>>> On 26/09/17 00:22, Dmitry Osipenko wrote:
>>>>>>>>>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>>>>>>>>>> on Tegra20/30 SoC's.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23
>>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>>> 1 file changed, 23 insertions(+)
>>>>>>>>>> create mode 100644
>>>>>>>>>> Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>> b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..2af9aa76ae11
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>>>>>>>>> @@ -0,0 +1,23 @@
>>>>>>>>>> +* NVIDIA Tegra AHB DMA controller
>>>>>>>>>> +
>>>>>>>>>> +Required properties:
>>>>>>>>>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>>>>>>>>>> +- reg: Should contain registers base address and length.
>>>>>>>>>> +- interrupts: Should contain one entry, DMA controller interrupt.
>>>>>>>>>> +- clocks: Should contain one entry, DMA controller clock.
>>>>>>>>>> +- resets : Should contain one entry, DMA controller reset.
>>>>>>>>>> +- #dma-cells: Should be <1>. The cell represents DMA request select
>>>>>>>>>> value
>>>>>>>>>> + for the peripheral. For more details consult the Tegra TRM's
>>>>>>>>>> + documentation, in particular AHB DMA channel control register
>>>>>>>>>> + REQ_SEL field.
>>>>>>>>>
>>>>>>>>> What about the TRIG_SEL field? Do we need to handle this here as well?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Actually, DMA transfer trigger isn't related a hardware description. It's
>>>>>>>> up to
>>>>>>>> software to decide what trigger to select. So it shouldn't be in the
>>>>>>>> binding.
>>>>>>>
>>>>>>> I think it could be, if say a board wanted a GPIO to trigger a transfer.
>>>>>>>
>>>>>>>> And I think the same applies to requester... any objections?
>>>>>>>
>>>>>>> Well, the REQ_SEL should definitely be in the binding.
>>>>>>>
>>>>>>> Laxman, Stephen, what are your thoughts on the TRIG_SEL field? Looks
>>>>>>> like we never bothered with it for the APB DMA and so maybe no ones uses
>>>>>>> this.
>>>>>>
>>>>>> I don't think TRIG_SEL should be in the binding, at least at present. While
>>>>>> TRIG_SEL certainly is something used to configure the transfer, I believe the
>>>>>> semantics of the current DMA binding only cover DMA transfers that are
>>>>>> initiated
>>>>>> when SW desires, rather than being a combination of after SW programs the
>>>>>> transfer plus some other HW event. So, we always use a default/hard-coded
>>>>>> TRIG_SEL value. As such, there's no need for a TRIG_SEL value in DT. There's
>>>>>> certainly no known use-case that requires a non-default TRIG_SEL value at
>>>>>> present. We could add an extra #dma-cells value later if we find a use for
>>>>>> it,
>>>>>> and the semantics of that use-case make sense to add it to the DMA specifier,
>>>>>> rather than some other separate higher-level property/driver/...
>>>>>
>>>>> Thank you for the comment. If we'd want to extend the binding further with the
>>>>> trigger, how to differentiate trigger from the requester in a case of a single
>>>>> #data-cell?
>>>>>
>>>>> Of course realistically a chance that the further extension would be needed is
>>>>> very-very low, so we may defer the efforts to solve that question and for now
>>>>> make driver aware of the potential #dma-cells extension.
>>>>
>>>> The request selector cell isn't optional, so is always present. If we later add
>>>> an optional trig_sel cell, we'll either have:
>>>>
>>>> #dma-cells=<1>: req_sel
>>>>
>>>> or:
>>>>
>>>> #dma-cells=<2>: req_sel, trig_sel
>>>
>>> Why request sel. couldn't be optional? Could you please elaborate a bit more?
>
> The documentation currently says it's mandatory, and DT bindings must be evolved
> in a backwards-compatible fashion.
>
>>> I think possible options are:
>>>
>>> #dma-cells=<1>: req_sel
>>> #dma-cells=<1>: trig_sel
>>
>> With the above, how would you know that it is the req_sel or trig_sel
>> that is specified?
>
> Also, if req_sel were optional, then it'd be impossible to distinguish between
> those cases, so we can't design a binding like that. In general, when adding
> extra optional cells to an #xxx-cells style binding, then whenever cell N is
> present, all cells before cell N must be present even if optional.
I've checked how extending of #dma-cells looks like and it is indeed a good
variant since it preserves backward compatibility. Thank you and Jon for the
comments and suggestions, I'll send out v2 soon.
^ permalink raw reply [flat|nested] 45+ messages in thread
[parent not found: <604d92036e0936443290e68a2226f935fb348113.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
[not found] ` <604d92036e0936443290e68a2226f935fb348113.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-10-05 20:33 ` Rob Herring
2017-10-05 21:30 ` Dmitry Osipenko
0 siblings, 1 reply; 45+ messages in thread
From: Rob Herring @ 2017-10-05 20:33 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Vinod Koul, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Tue, Sep 26, 2017 at 02:22:04AM +0300, Dmitry Osipenko wrote:
> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
> on Tegra20/30 SoC's.
>
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>
> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
> new file mode 100644
> index 000000000000..2af9aa76ae11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
> @@ -0,0 +1,23 @@
> +* NVIDIA Tegra AHB DMA controller
> +
> +Required properties:
> +- compatible: Must be "nvidia,tegra20-ahbdma"
> +- reg: Should contain registers base address and length.
> +- interrupts: Should contain one entry, DMA controller interrupt.
> +- clocks: Should contain one entry, DMA controller clock.
> +- resets : Should contain one entry, DMA controller reset.
> +- #dma-cells: Should be <1>. The cell represents DMA request select value
> + for the peripheral. For more details consult the Tegra TRM's
> + documentation, in particular AHB DMA channel control register
> + REQ_SEL field.
> +
> +Example:
> +
> +ahbdma: ahbdma@60008000 {
Use standard node names. dma-controller in this case.
> + compatible = "nvidia,tegra20-ahbdma";
> + reg = <0x60008000 0x2000>;
> + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&tegra_car TEGRA20_CLK_AHBDMA>;
> + resets = <&tegra_car 33>;
> + #dma-cells = <1>;
> +};
> --
> 2.14.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 3/5] dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
2017-10-05 20:33 ` Rob Herring
@ 2017-10-05 21:30 ` Dmitry Osipenko
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-10-05 21:30 UTC (permalink / raw)
To: Rob Herring
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Vinod Koul, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel
On 05.10.2017 23:33, Rob Herring wrote:
> On Tue, Sep 26, 2017 at 02:22:04AM +0300, Dmitry Osipenko wrote:
>> Document DT bindings for NVIDIA Tegra AHB DMA controller that presents
>> on Tegra20/30 SoC's.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 ++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>>
>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>> new file mode 100644
>> index 000000000000..2af9aa76ae11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt
>> @@ -0,0 +1,23 @@
>> +* NVIDIA Tegra AHB DMA controller
>> +
>> +Required properties:
>> +- compatible: Must be "nvidia,tegra20-ahbdma"
>> +- reg: Should contain registers base address and length.
>> +- interrupts: Should contain one entry, DMA controller interrupt.
>> +- clocks: Should contain one entry, DMA controller clock.
>> +- resets : Should contain one entry, DMA controller reset.
>> +- #dma-cells: Should be <1>. The cell represents DMA request select value
>> + for the peripheral. For more details consult the Tegra TRM's
>> + documentation, in particular AHB DMA channel control register
>> + REQ_SEL field.
>> +
>> +Example:
>> +
>> +ahbdma: ahbdma@60008000 {
>
> Use standard node names. dma-controller in this case.
>
Okay, I'll change it in v3. Thank you for the comment.
>> + compatible = "nvidia,tegra20-ahbdma";
>> + reg = <0x60008000 0x2000>;
>> + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&tegra_car TEGRA20_CLK_AHBDMA>;
>> + resets = <&tegra_car 33>;
>> + #dma-cells = <1>;
>> +};
>> --
>> 2.14.1
>>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v1 5/5] ARM: dts: tegra: Add AHB DMA controller nodes
[not found] ` <cover.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-09-25 23:22 ` [PATCH v1 3/5] dt-bindings: Add DT bindings " Dmitry Osipenko
@ 2017-09-25 23:22 ` Dmitry Osipenko
1 sibling, 0 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-25 23:22 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, Vinod Koul
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Add AHB DMA controller nodes to Tegra20/30 DT's.
Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
arch/arm/boot/dts/tegra20.dtsi | 9 +++++++++
arch/arm/boot/dts/tegra30.dtsi | 9 +++++++++
2 files changed, 18 insertions(+)
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index f1579c9a7ef4..d0c0c26427f7 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -200,6 +200,15 @@
reg = <0x60007000 0x1000>;
};
+ ahbdma: ahbdma@60008000 {
+ compatible = "nvidia,tegra20-ahbdma";
+ reg = <0x60008000 0x2000>;
+ interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&tegra_car TEGRA20_CLK_AHBDMA>;
+ resets = <&tegra_car 33>;
+ #dma-cells = <1>;
+ };
+
apbdma: dma@6000a000 {
compatible = "nvidia,tegra20-apbdma";
reg = <0x6000a000 0x1200>;
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 13960fda7471..0800d9a8c546 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -291,6 +291,15 @@
reg = <0x60007000 0x1000>;
};
+ ahbdma: ahbdma@60008000 {
+ compatible = "nvidia,tegra20-ahbdma";
+ reg = <0x60008000 0x2000>;
+ interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&tegra_car TEGRA30_CLK_AHBDMA>;
+ resets = <&tegra_car 33>;
+ #dma-cells = <1>;
+ };
+
apbdma: dma@6000a000 {
compatible = "nvidia,tegra30-apbdma", "nvidia,tegra20-apbdma";
reg = <0x6000a000 0x1400>;
--
2.14.1
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver
2017-09-25 23:22 [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver Dmitry Osipenko
` (3 preceding siblings ...)
[not found] ` <cover.1506380746.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-09-28 9:31 ` Vinod Koul
2017-09-28 12:24 ` Dmitry Osipenko
4 siblings, 1 reply; 45+ messages in thread
From: Vinod Koul @ 2017-09-28 9:31 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel
On Tue, Sep 26, 2017 at 02:22:01AM +0300, Dmitry Osipenko wrote:
> NVIDIA Tegra20/30 SoC's have AHB DMA controller. It has 4 DMA channels,
> supports AHB <-> Memory and Memory <-> Memory transfers, slave / master
> modes. This driver is primarily supposed to be used by gpu/host1x in a
> master mode, performing 3D HW context stores.
>
> Dmitry Osipenko (5):
> clk: tegra: Add AHB DMA clock entry
> clk: tegra: Bump SCLK clock rate to 216MHz on Tegra20
> dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
> dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
> ARM: dts: tegra: Add AHB DMA controller nodes
I don't think they are dependent, so consider sending them separately
--
~Vinod
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver
2017-09-28 9:31 ` [PATCH v1 0/5] NVIDIA Tegra AHB DMA controller driver Vinod Koul
@ 2017-09-28 12:24 ` Dmitry Osipenko
0 siblings, 0 replies; 45+ messages in thread
From: Dmitry Osipenko @ 2017-09-28 12:24 UTC (permalink / raw)
To: Vinod Koul
Cc: Thierry Reding, Jonathan Hunter, Laxman Dewangan,
Peter De Schrijver, Prashant Gaikwad, Michael Turquette,
Stephen Boyd, Rob Herring, linux-tegra, devicetree, dmaengine,
linux-clk, linux-kernel
On 28.09.2017 12:31, Vinod Koul wrote:
> On Tue, Sep 26, 2017 at 02:22:01AM +0300, Dmitry Osipenko wrote:
>> NVIDIA Tegra20/30 SoC's have AHB DMA controller. It has 4 DMA channels,
>> supports AHB <-> Memory and Memory <-> Memory transfers, slave / master
>> modes. This driver is primarily supposed to be used by gpu/host1x in a
>> master mode, performing 3D HW context stores.
>>
>> Dmitry Osipenko (5):
>> clk: tegra: Add AHB DMA clock entry
>> clk: tegra: Bump SCLK clock rate to 216MHz on Tegra20
>> dt-bindings: Add DT bindings for NVIDIA Tegra AHB DMA controller
>> dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
>> ARM: dts: tegra: Add AHB DMA controller nodes
>
> I don't think they are dependent, so consider sending them separately
>
Well, they are dependent in a sense of making driver usable. Only the "SCLK rate
bump" patch isn't strictly needed.
Splitting this series won't cause building failures, but all pieces should be in
place for the working driver. So I suppose it is okay if clk patches would get
in earlier than the others, I'll split the series.
Thank you for the review.
--
Dmitry
^ permalink raw reply [flat|nested] 45+ messages in thread