Re: [PATCH 0/2] musb_host: move DMA engine check from musb_tx_dma_set_mode_cppi_tusb() to its caller
* Bin Liu [160504 14:10]:
> Hi,
>
> On Wed, May 04, 2016 at 11:56:15PM +0300, Sergei Shtylyov wrote:
> > Hello.
> >
> > On 05/04/2016 11:46 PM, Bin Liu wrote:
> >
> > Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for
> > musb_host.c")
> > looks incomplete: the DMA engine checks are done outside the
> > Mentor/UX500
> > handler but inside the CPPI/TUSB handler. Move the checks out of the
> > CPPI/
> > TUSB handler into its caller, musb_tx_dma_program().
> >
> > Signed-off-by: Sergei Shtylyov
> >
> > ---
> > drivers/usb/musb/musb_host.c |7 +++
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > Index: usb/drivers/usb/musb/musb_host.c
> > ===
> > --- usb.orig/drivers/usb/musb/musb_host.c
> > +++ usb/drivers/usb/musb/musb_host.c
> > @@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus
> > {
> > struct dma_channel *channel = hw_ep->tx_channel;
> >
> > - if (!is_cppi_enabled(hw_ep->musb) &&
> > !tusb_dma_omap(hw_ep->musb))
> > - return -ENODEV;
> > -
> > channel->actual_len = 0;
> > >>
> > >>>Since this function has only two lines now, does it make sense to get rid
> > >>>of it completely?
> > >>
> > >>That would be the reverse to what Tony's patches did. I think gcc
> > >>will inline this function anyway.
> > >
> > >I believe the intention of Tony's patch is to get rid of #ifdefs. Any
> >
> >Right. But while doing that, he tried to avoid the code motion.
> >
> > >further patch could do whatever is right to improve the code. I
> > >personally don't rely on compiler's optimization. I don't have strong
> > >opinion here, you make the call.
> >
> >I'll leave the things as they are then.
> >
> > >>>Regards,
> > >>>-Bin.
> > >>>
> >
> > /*
> > @@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d
> > if (musb_dma_inventra(hw_ep->musb) ||
> > musb_dma_ux500(hw_ep->musb))
> > res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb,
> > offset, &length,
> > &mode);
> > - else
> > + else if (is_cppi_enabled(hw_ep->musb) ||
> > tusb_dma_omap(hw_ep->musb))
> > res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh,
> > urb,
> > offset, &length,
> > &mode);
> > + else
> > + return false;
> > >>
> > >>Well, using ret = -EINVAL; would have been more appropriate, I'd
> >
> >s/ret/res/.
> >
> > >>respin if you won't mind?
> > >
> > >I don't mind respin at all. But the function return type is bool, so
> > >flase is better, right?
> >
> >It'll just bail out on the *if* below:
>
> Ok, I now understand what you meant. I was thinking about the final
> code of your two patches together.
>
> No, please don't respin, I like your current version better.
Yeah looks fine to me. I intentionally tried to avoid any kind of code
changes to not break the driver while adding the multiarch support..
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] musb_host: move DMA engine check from musb_tx_dma_set_mode_cppi_tusb() to its caller
Hi,
On Wed, May 04, 2016 at 11:56:15PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 05/04/2016 11:46 PM, Bin Liu wrote:
>
> Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for
> musb_host.c")
> looks incomplete: the DMA engine checks are done outside the Mentor/UX500
> handler but inside the CPPI/TUSB handler. Move the checks out of the
> CPPI/
> TUSB handler into its caller, musb_tx_dma_program().
>
> Signed-off-by: Sergei Shtylyov
>
> ---
> drivers/usb/musb/musb_host.c |7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> Index: usb/drivers/usb/musb/musb_host.c
> ===
> --- usb.orig/drivers/usb/musb/musb_host.c
> +++ usb/drivers/usb/musb/musb_host.c
> @@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus
> {
> struct dma_channel *channel = hw_ep->tx_channel;
>
> - if (!is_cppi_enabled(hw_ep->musb) && !tusb_dma_omap(hw_ep->musb))
> - return -ENODEV;
> -
> channel->actual_len = 0;
> >>
> >>>Since this function has only two lines now, does it make sense to get rid
> >>>of it completely?
> >>
> >>That would be the reverse to what Tony's patches did. I think gcc
> >>will inline this function anyway.
> >
> >I believe the intention of Tony's patch is to get rid of #ifdefs. Any
>
>Right. But while doing that, he tried to avoid the code motion.
>
> >further patch could do whatever is right to improve the code. I
> >personally don't rely on compiler's optimization. I don't have strong
> >opinion here, you make the call.
>
>I'll leave the things as they are then.
>
> >>>Regards,
> >>>-Bin.
> >>>
>
> /*
> @@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d
> if (musb_dma_inventra(hw_ep->musb) ||
> musb_dma_ux500(hw_ep->musb))
> res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb,
> offset, &length,
> &mode);
> - else
> + else if (is_cppi_enabled(hw_ep->musb) || tusb_dma_omap(hw_ep->musb))
> res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh,
> urb,
> offset, &length,
> &mode);
> + else
> + return false;
> >>
> >>Well, using ret = -EINVAL; would have been more appropriate, I'd
>
>s/ret/res/.
>
> >>respin if you won't mind?
> >
> >I don't mind respin at all. But the function return type is bool, so
> >flase is better, right?
>
>It'll just bail out on the *if* below:
Ok, I now understand what you meant. I was thinking about the final
code of your two patches together.
No, please don't respin, I like your current version better.
>
> if (res)
> return false;
>
> >>
> >>MBR, Seregi
>
> >Regards,
> >-Bin.
>
> MBR, Sergei
Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] musb_host: move DMA engine check from musb_tx_dma_set_mode_cppi_tusb() to its caller
Hello.
On 05/04/2016 11:46 PM, Bin Liu wrote:
Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for musb_host.c")
looks incomplete: the DMA engine checks are done outside the Mentor/UX500
handler but inside the CPPI/TUSB handler. Move the checks out of the CPPI/
TUSB handler into its caller, musb_tx_dma_program().
Signed-off-by: Sergei Shtylyov
---
drivers/usb/musb/musb_host.c |7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
Index: usb/drivers/usb/musb/musb_host.c
===
--- usb.orig/drivers/usb/musb/musb_host.c
+++ usb/drivers/usb/musb/musb_host.c
@@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus
{
struct dma_channel *channel = hw_ep->tx_channel;
- if (!is_cppi_enabled(hw_ep->musb) && !tusb_dma_omap(hw_ep->musb))
- return -ENODEV;
-
channel->actual_len = 0;
Since this function has only two lines now, does it make sense to get rid
of it completely?
That would be the reverse to what Tony's patches did. I think gcc
will inline this function anyway.
I believe the intention of Tony's patch is to get rid of #ifdefs. Any
Right. But while doing that, he tried to avoid the code motion.
further patch could do whatever is right to improve the code. I
personally don't rely on compiler's optimization. I don't have strong
opinion here, you make the call.
I'll leave the things as they are then.
Regards,
-Bin.
/*
@@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d
if (musb_dma_inventra(hw_ep->musb) || musb_dma_ux500(hw_ep->musb))
res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb,
offset, &length, &mode);
- else
+ else if (is_cppi_enabled(hw_ep->musb) || tusb_dma_omap(hw_ep->musb))
res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh, urb,
offset, &length, &mode);
+ else
+ return false;
Well, using ret = -EINVAL; would have been more appropriate, I'd
s/ret/res/.
respin if you won't mind?
I don't mind respin at all. But the function return type is bool, so
flase is better, right?
It'll just bail out on the *if* below:
if (res)
return false;
MBR, Seregi
Regards,
-Bin.
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] musb_host: move DMA engine check from musb_tx_dma_set_mode_cppi_tusb() to its caller
Hi,
On Wed, May 04, 2016 at 11:06:58PM +0300, Sergei Shtylyov wrote:
> On 05/04/2016 11:01 PM, Bin Liu wrote:
>
> >>Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for musb_host.c")
> >>looks incomplete: the DMA engine checks are done outside the Mentor/UX500
> >>handler but inside the CPPI/TUSB handler. Move the checks out of the CPPI/
> >>TUSB handler into its caller, musb_tx_dma_program().
> >>
> >>Signed-off-by: Sergei Shtylyov
> >>
> >>---
> >> drivers/usb/musb/musb_host.c |7 +++
> >> 1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >>Index: usb/drivers/usb/musb/musb_host.c
> >>===
> >>--- usb.orig/drivers/usb/musb/musb_host.c
> >>+++ usb/drivers/usb/musb/musb_host.c
> >>@@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus
> >> {
> >>struct dma_channel *channel = hw_ep->tx_channel;
> >>
> >>- if (!is_cppi_enabled(hw_ep->musb) && !tusb_dma_omap(hw_ep->musb))
> >>- return -ENODEV;
> >>-
> >>channel->actual_len = 0;
>
> >Since this function has only two lines now, does it make sense to get rid
> >of it completely?
>
>That would be the reverse to what Tony's patches did. I think gcc
> will inline this function anyway.
I believe the intention of Tony's patch is to get rid of #ifdefs. Any
further patch could do whatever is right to improve the code. I
personally don't rely on compiler's optimization. I don't have strong
opinion here, you make the call.
>
> >Regards,
> >-Bin.
> >
> >>
> >>/*
> >>@@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d
> >>if (musb_dma_inventra(hw_ep->musb) || musb_dma_ux500(hw_ep->musb))
> >>res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb,
> >> offset, &length, &mode);
> >>- else
> >>+ else if (is_cppi_enabled(hw_ep->musb) || tusb_dma_omap(hw_ep->musb))
> >>res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh, urb,
> >> offset, &length, &mode);
> >>+ else
> >>+ return false;
>
>Well, using ret = -EINVAL; would have been more appropriate, I'd
> respin if you won't mind?
I don't mind respin at all. But the function return type is bool, so
flase is better, right?
>
> >>if (res)
> >>return false;
> >>
>
> MBR, Seregi
Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] musb_host: move DMA engine check from musb_tx_dma_set_mode_cppi_tusb() to its caller
On 05/04/2016 11:01 PM, Bin Liu wrote:
Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for musb_host.c")
looks incomplete: the DMA engine checks are done outside the Mentor/UX500
handler but inside the CPPI/TUSB handler. Move the checks out of the CPPI/
TUSB handler into its caller, musb_tx_dma_program().
Signed-off-by: Sergei Shtylyov
---
drivers/usb/musb/musb_host.c |7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
Index: usb/drivers/usb/musb/musb_host.c
===
--- usb.orig/drivers/usb/musb/musb_host.c
+++ usb/drivers/usb/musb/musb_host.c
@@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus
{
struct dma_channel *channel = hw_ep->tx_channel;
- if (!is_cppi_enabled(hw_ep->musb) && !tusb_dma_omap(hw_ep->musb))
- return -ENODEV;
-
channel->actual_len = 0;
Since this function has only two lines now, does it make sense to get rid
of it completely?
That would be the reverse to what Tony's patches did. I think gcc will
inline this function anyway.
Regards,
-Bin.
/*
@@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d
if (musb_dma_inventra(hw_ep->musb) || musb_dma_ux500(hw_ep->musb))
res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb,
offset, &length, &mode);
- else
+ else if (is_cppi_enabled(hw_ep->musb) || tusb_dma_omap(hw_ep->musb))
res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh, urb,
offset, &length, &mode);
+ else
+ return false;
Well, using ret = -EINVAL; would have been more appropriate, I'd respin if
you won't mind?
if (res)
return false;
MBR, Seregi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] musb_host: move DMA engine check from musb_tx_dma_set_mode_cppi_tusb() to its caller
Hi,
On Wed, May 04, 2016 at 01:51:58AM +0300, Sergei Shtylyov wrote:
> Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for musb_host.c")
> looks incomplete: the DMA engine checks are done outside the Mentor/UX500
> handler but inside the CPPI/TUSB handler. Move the checks out of the CPPI/
> TUSB handler into its caller, musb_tx_dma_program().
>
> Signed-off-by: Sergei Shtylyov
>
> ---
> drivers/usb/musb/musb_host.c |7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> Index: usb/drivers/usb/musb/musb_host.c
> ===
> --- usb.orig/drivers/usb/musb/musb_host.c
> +++ usb/drivers/usb/musb/musb_host.c
> @@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus
> {
> struct dma_channel *channel = hw_ep->tx_channel;
>
> - if (!is_cppi_enabled(hw_ep->musb) && !tusb_dma_omap(hw_ep->musb))
> - return -ENODEV;
> -
> channel->actual_len = 0;
Since this function has only two lines now, does it make sense to get rid
of it completely?
Regards,
-Bin.
>
> /*
> @@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d
> if (musb_dma_inventra(hw_ep->musb) || musb_dma_ux500(hw_ep->musb))
> res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb,
>offset, &length, &mode);
> - else
> + else if (is_cppi_enabled(hw_ep->musb) || tusb_dma_omap(hw_ep->musb))
> res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh, urb,
>offset, &length, &mode);
> + else
> + return false;
> if (res)
> return false;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] musb_host: move DMA engine check from musb_tx_dma_set_mode_cppi_tusb() to its caller
Commit 754fe4a92c07 ("usb: musb: Remove ifdefs for TX DMA for musb_host.c")
looks incomplete: the DMA engine checks are done outside the Mentor/UX500
handler but inside the CPPI/TUSB handler. Move the checks out of the CPPI/
TUSB handler into its caller, musb_tx_dma_program().
Signed-off-by: Sergei Shtylyov
---
drivers/usb/musb/musb_host.c |7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
Index: usb/drivers/usb/musb/musb_host.c
===
--- usb.orig/drivers/usb/musb/musb_host.c
+++ usb/drivers/usb/musb/musb_host.c
@@ -678,9 +678,6 @@ static int musb_tx_dma_set_mode_cppi_tus
{
struct dma_channel *channel = hw_ep->tx_channel;
- if (!is_cppi_enabled(hw_ep->musb) && !tusb_dma_omap(hw_ep->musb))
- return -ENODEV;
-
channel->actual_len = 0;
/*
@@ -704,9 +701,11 @@ static bool musb_tx_dma_program(struct d
if (musb_dma_inventra(hw_ep->musb) || musb_dma_ux500(hw_ep->musb))
res = musb_tx_dma_set_mode_mentor(dma, hw_ep, qh, urb,
offset, &length, &mode);
- else
+ else if (is_cppi_enabled(hw_ep->musb) || tusb_dma_omap(hw_ep->musb))
res = musb_tx_dma_set_mode_cppi_tusb(dma, hw_ep, qh, urb,
offset, &length, &mode);
+ else
+ return false;
if (res)
return false;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
