[04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original
diff mbox

Message ID e2c1bd6941e88f8988b1942c13c20cf436b3a963.1311936524.git.viresh.kumar@st.com
State New, archived
Headers show

Commit Message

Viresh KUMAR July 29, 2011, 10:49 a.m. UTC
Similar comment is present over routine also pl08x_choose_master_bus(). Keeping
one of them. Also rewrite that comment to convey message clearly.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

Comments

Vinod Koul July 29, 2011, 12:09 p.m. UTC | #1
On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> Similar comment is present over routine also pl08x_choose_master_bus(). Keeping
> one of them. Also rewrite that comment to convey message clearly.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/dma/amba-pl08x.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 1f7c510..4a64fdf 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -498,9 +498,9 @@ struct pl08x_lli_build_data {
>  };
>  
>  /*
> - * Autoselect a master bus to use for the transfer this prefers the
> - * destination bus if both available if fixed address on one bus the
> - * other will be chosen
> + * Autoselect a master bus to use for the transfer
> + * - prefers the destination bus if both available
> + * - if fixed address on one bus the other will be chosen
Not sure I get it, English is not my first language :)
>   */
>  static void pl08x_choose_master_bus(struct pl08x_lli_build_data *bd,
>  	struct pl08x_bus_data **mbus, struct pl08x_bus_data **sbus, u32 cctl)
> @@ -626,11 +626,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  	/* We need to count this down to zero */
>  	bd.remainder = txd->len;
>  
> -	/*
> -	 * Choose bus to align to
> -	 * - prefers destination bus if both available
> -	 * - if fixed address on one bus chooses other
> -	 */
>  	pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
>  
>  	dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu "
viresh kumar July 29, 2011, 3:47 p.m. UTC | #2
On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>>  /*
>> - * Autoselect a master bus to use for the transfer this prefers the
>> - * destination bus if both available if fixed address on one bus the
>> - * other will be chosen
>> + * Autoselect a master bus to use for the transfer
>> + * - prefers the destination bus if both available
>> + * - if fixed address on one bus the other will be chosen
> Not sure I get it, English is not my first language :)

Nor mine either. :)

Will give some more explanation.

But before that i wanted to know the exact purpose of this
master-slave concept here.
Probably it is for choosing the victim for reduction of width, in case
src and dest addresses
are misaligned to each other.
@Russell and Linus: Is this correct?

--
viresh
Russell King - ARM Linux July 29, 2011, 4:34 p.m. UTC | #3
On Fri, Jul 29, 2011 at 08:47:06AM -0700, viresh kumar wrote:
> On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> > On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> >>  /*
> >> - * Autoselect a master bus to use for the transfer this prefers the
> >> - * destination bus if both available if fixed address on one bus the
> >> - * other will be chosen
> >> + * Autoselect a master bus to use for the transfer
> >> + * - prefers the destination bus if both available
> >> + * - if fixed address on one bus the other will be chosen
> > Not sure I get it, English is not my first language :)
> 
> Nor mine either. :)
> 
> Will give some more explanation.
> 
> But before that i wanted to know the exact purpose of this
> master-slave concept here.
> Probably it is for choosing the victim for reduction of width, in case
> src and dest addresses
> are misaligned to each other.
> @Russell and Linus: Is this correct?

No idea - most of the LLI code that's in this driver came from within
ARM Ltd, and I've dared not change its behaviour in case the original
author knew something that isn't public.

In addition, as the ARM platforms I have with a PL080 on seem to have
their DMA support buggered beyond belief (that's putting it mildly),
I've not really been able to test anything more complex than single
byte DMA to a UART.  Linus W has one of the rare ARM platforms which
has a working implementation.

I'm not sure why the function is named in terms of master/slave because
those terms don't seem to have much to do with what its actually doing
(which is selecting either the source or destination to handle alignment
issues with.)  And it's not really about busses either.

If the LLI creation code can be greatly simplified and that function
killed off, that'd certainly be a great step forward.  In the mean time
I'd suggest leaving that comment as-is as long as the function exists.
Linus Walleij July 30, 2011, 10:32 p.m. UTC | #4
2011/7/29 viresh kumar <viresh.linux@gmail.com>:
> On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
>> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>>>  /*
>>> - * Autoselect a master bus to use for the transfer this prefers the
>>> - * destination bus if both available if fixed address on one bus the
>>> - * other will be chosen
>>> + * Autoselect a master bus to use for the transfer
>>> + * - prefers the destination bus if both available
>>> + * - if fixed address on one bus the other will be chosen
>> Not sure I get it, English is not my first language :)
>
> Nor mine either. :)
>
> Will give some more explanation.
>
> But before that i wanted to know the exact purpose of this
> master-slave concept here.

The PL08x has two bus mastering interfaces. They can be
connected to two separate busses and two adress spaces
even, usually that is not the case though.

> Probably it is for choosing the victim for reduction of width, in case
> src and dest addresses
> are misaligned to each other.
> @Russell and Linus: Is this correct?

You can see from the function that it is only about choosing the
master and slave. One of them is reading from some memory
and one of them is writing. These are called source and
destination respectively.

What it then does is:

- If one of the buses will be targeting a fixed
  address, choose the other one, so the one that increase address
  become master and the fixed address slave
.
- If both buses increas address (usually memory-to-memory) the
  third case is activated and deals with trying to set the widest
  bus as master.

Crystal clear?

Please copyedit/author something like the above...

Thanks,
Linus Walleij
Russell King - ARM Linux July 30, 2011, 10:57 p.m. UTC | #5
On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
> 2011/7/29 viresh kumar <viresh.linux@gmail.com>:
> > Nor mine either. :)
> >
> > Will give some more explanation.
> >
> > But before that i wanted to know the exact purpose of this
> > master-slave concept here.
> 
> The PL08x has two bus mastering interfaces. They can be
> connected to two separate busses and two adress spaces
> even, usually that is not the case though.

This is not what the function is about... that's done by my
pl08x_select_bus() function.

This function (pl08x_choose_master_bus) does something completely
different, and has to do with how the LLIs are laid out.  I think
it's to do with whether we use the source or destination for
alignment purposes.

It really has nothing to do with choosing busses.
Linus Walleij July 30, 2011, 11:37 p.m. UTC | #6
2011/7/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
>> The PL08x has two bus mastering interfaces. They can be
>> connected to two separate busses and two adress spaces
>> even, usually that is not the case though.
>
> This is not what the function is about... that's done by my
> pl08x_select_bus() function.

To be clear, I was explaining what I percieved as a
question from Viresh about what the comment in this hunk of
the patch was about:

@@ -498,9 +498,9 @@ struct pl08x_lli_build_data {
};
 /*
- * Autoselect a master bus to use for the transfer this prefers the
- * destination bus if both available if fixed address on one bus the
- * other will be chosen
+ * Autoselect a master bus to use for the transfer
+ * - prefers the destination bus if both available
+ * - if fixed address on one bus the other will be chosen
 */
 static void pl08x_choose_master_bus(struct pl08x_lli_build_data *bd,
       struct pl08x_bus_data **mbus, struct pl08x_bus_data **sbus, u32 cctl)

This is the comment just above pl08x_choose_master_bus(),
the pl08x_lli_build_data() just has a danglig semicolon in the hunk.

As for the LLI code, all of it's weird complexity comes from the fact
that the DMAC cannot let a single element pass across 1KB (0x400)
boundaries, which is one more than a little insane hardware
restriction, and I have no clue where that limitation actually comes
from, likely strange VHDL code.

Then for this hunk:

@@ -626,11 +626,6 @@ static int pl08x_fill_llis_for_desc(struct
pl08x_driver_data *pl08x,
       /* We need to count this down to zero */
       bd.remainder = txd->len;

-       /*
-        * Choose bus to align to
-        * - prefers destination bus if both available
-        * - if fixed address on one bus chooses other
-        */
       pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);

This is I think just redundant commenting of what the called function does,
so yes should be deleted.

Thanks,
Linus Walleij
viresh kumar July 31, 2011, 5:51 a.m. UTC | #7
On 7/30/11, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/7/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
>>> The PL08x has two bus mastering interfaces. They can be
>>> connected to two separate busses and two adress spaces
>>> even, usually that is not the case though.
>>
>> This is not what the function is about... that's done by my
>> pl08x_select_bus() function.
>
> To be clear, I was explaining what I percieved as a
> question from Viresh about what the comment in this hunk of
> the patch was about:
>

Just to clear my question again:
- It wasn't about the comment but concept behind Master/Slave terminology.
- Why are we referring src and dest as Master and Slave. As there is
nothing like that.
- As pointed by Russell and me earlier: Probably it is for choosing
the victim for
reduction of width, in case src and dest addresses are misaligned to each other.

If what i understood is correct, then we should get rid of this
routine or this master/slave
terminology, as this is causing confusion.

--
viresh
Russell King - ARM Linux July 31, 2011, 9:06 a.m. UTC | #8
On Sun, Jul 31, 2011 at 01:37:46AM +0200, Linus Walleij wrote:
> 2011/7/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
> >> The PL08x has two bus mastering interfaces. They can be
> >> connected to two separate busses and two adress spaces
> >> even, usually that is not the case though.
> >
> > This is not what the function is about... that's done by my
> > pl08x_select_bus() function.
> 
> To be clear, I was explaining what I percieved as a
> question from Viresh about what the comment in this hunk of
> the patch was about:

Yes, and the comment and function name are wrong for what it's doing.
I repeat: it has nothing to do with selecting an AHB bus.

> As for the LLI code, all of it's weird complexity comes from the fact
> that the DMAC cannot let a single element pass across 1KB (0x400)
> boundaries, which is one more than a little insane hardware
> restriction, and I have no clue where that limitation actually comes
> from, likely strange VHDL code.

Viresh's discussion with ARM Ltd indicates that this is not the case,
and that restriction comes from misunderstanding of the documentation.

Patch
diff mbox

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 1f7c510..4a64fdf 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -498,9 +498,9 @@  struct pl08x_lli_build_data {
 };
 
 /*
- * Autoselect a master bus to use for the transfer this prefers the
- * destination bus if both available if fixed address on one bus the
- * other will be chosen
+ * Autoselect a master bus to use for the transfer
+ * - prefers the destination bus if both available
+ * - if fixed address on one bus the other will be chosen
  */
 static void pl08x_choose_master_bus(struct pl08x_lli_build_data *bd,
 	struct pl08x_bus_data **mbus, struct pl08x_bus_data **sbus, u32 cctl)
@@ -626,11 +626,6 @@  static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	/* We need to count this down to zero */
 	bd.remainder = txd->len;
 
-	/*
-	 * Choose bus to align to
-	 * - prefers destination bus if both available
-	 * - if fixed address on one bus chooses other
-	 */
 	pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
 
 	dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu "