[PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[tmarri]

From: Tirumala Marri <tmarri@apm.com>

v15:
 1. Fixed some checkpatch errors.
 2. Added spin lock check during read-modify-write of interrupt registers
    out side intr handlers.
 3. Removed Kconfig var which is not used and not defined.

v14:
 1. Modifying dwc_reg_read/write functions to accepts offset as arg.
 2. Adding spin_lock to common interrupt function.
 3. Adding start/stop, vbus_power functions to gadget_opst struct.
 4. Removed some unnecessary comments and prints.
 5. op_state_str() replace with common function.
 6. Removed some unnecessary checks from apmppc.c file.

Tirumala Marri (10):
  USB/ppc4xx: Add Synopsys DesignWare HS USB OTG Register definitions
  USB/ppc4xx: Add Synopsys DesignWare HS USB OTG driver framework
  USB/ppc4xx: Add Synopsys DWC OTG Core Interface Layer (CIL)
  USB/ppc4xx: Add Synopsys DWC OTG HCD function
  USB/ppc4xx: Add Synopsys DWC OTG HCD interrupt function
  USB/ppc4xx: Add Synopsys DWC OTG HCD queue function
  USB/ppc4xx: Add Synopsys DWC OTG PCD function
  USB ppc4xx: Add Synopsys DWC OTG PCD interrupt function
  USB ppc4xx: Add Synopsys DWC OTG driver kernel configuration and
    Makefile
  USB/ppc4xx:Synopsys DWC OTG driver enable gadget support

 drivers/usb/Kconfig               |    2 +
 drivers/usb/Makefile              |    1 +
 drivers/usb/dwc/Kconfig           |   84 ++
 drivers/usb/dwc/Makefile          |   19 +
 drivers/usb/dwc/apmppc.c          |  355 ++++++
 drivers/usb/dwc/cil.c             |  890 +++++++++++++
 drivers/usb/dwc/cil.h             | 1174 ++++++++++++++++++
 drivers/usb/dwc/cil_intr.c        |  616 +++++++++
 drivers/usb/dwc/driver.h          |   76 ++
 drivers/usb/dwc/hcd.c             | 2471 +++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc/hcd.h             |  416 +++++++
 drivers/usb/dwc/hcd_intr.c        | 1477 ++++++++++++++++++++++
 drivers/usb/dwc/hcd_queue.c       |  696 +++++++++++
 drivers/usb/dwc/param.c           |  180 +++
 drivers/usb/dwc/pcd.c             | 1791 +++++++++++++++++++++++++++
 drivers/usb/dwc/pcd.h             |  139 +++
 drivers/usb/dwc/pcd_intr.c        | 2312 ++++++++++++++++++++++++++++++++++
 drivers/usb/dwc/regs.h            | 1326 ++++++++++++++++++++
 drivers/usb/gadget/Kconfig        |   10 +
 drivers/usb/gadget/gadget_chips.h |    3 +
 20 files changed, 14038 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/dwc/Kconfig
 create mode 100644 drivers/usb/dwc/Makefile
 create mode 100644 drivers/usb/dwc/apmppc.c
 create mode 100644 drivers/usb/dwc/cil.c
 create mode 100644 drivers/usb/dwc/cil.h
 create mode 100644 drivers/usb/dwc/cil_intr.c
 create mode 100644 drivers/usb/dwc/driver.h
 create mode 100644 drivers/usb/dwc/hcd.c
 create mode 100644 drivers/usb/dwc/hcd.h
 create mode 100644 drivers/usb/dwc/hcd_intr.c
 create mode 100644 drivers/usb/dwc/hcd_queue.c
 create mode 100644 drivers/usb/dwc/param.c
 create mode 100644 drivers/usb/dwc/pcd.c
 create mode 100644 drivers/usb/dwc/pcd.h
 create mode 100644 drivers/usb/dwc/pcd_intr.c
 create mode 100644 drivers/usb/dwc/regs.h

Re: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Pratyush Anand]

Hi Tirumala,

I have reviwed all the patches.
Thanks for taking my most of the diffs over your v13.
There are few thing, which are really very important like,
AHB disable check, EP Disable/Stall (have commented about them in
respective patches). I am doubtful, that how does all of usbcv test cases p=
asses
without those modifications.
So, please have a look on those comments.
You may  add my signed-off.

Regards
Pratyush


On Sat, Oct 15, 2011 at 3:38 AM,  <tmarri@apm.com> wrote:
> From: Tirumala Marri <tmarri@apm.com>
>
> v15:
> =A01. Fixed some checkpatch errors.
> =A02. Added spin lock check during read-modify-write of interrupt registe=
rs
> =A0 =A0out side intr handlers.
> =A03. Removed Kconfig var which is not used and not defined.
>
> v14:
> =A01. Modifying dwc_reg_read/write functions to accepts offset as arg.
> =A02. Adding spin_lock to common interrupt function.
> =A03. Adding start/stop, vbus_power functions to gadget_opst struct.
> =A04. Removed some unnecessary comments and prints.
> =A05. op_state_str() replace with common function.
> =A06. Removed some unnecessary checks from apmppc.c file.
>
> Tirumala Marri (10):
> =A0USB/ppc4xx: Add Synopsys DesignWare HS USB OTG Register definitions
> =A0USB/ppc4xx: Add Synopsys DesignWare HS USB OTG driver framework
> =A0USB/ppc4xx: Add Synopsys DWC OTG Core Interface Layer (CIL)
> =A0USB/ppc4xx: Add Synopsys DWC OTG HCD function
> =A0USB/ppc4xx: Add Synopsys DWC OTG HCD interrupt function
> =A0USB/ppc4xx: Add Synopsys DWC OTG HCD queue function
> =A0USB/ppc4xx: Add Synopsys DWC OTG PCD function
> =A0USB ppc4xx: Add Synopsys DWC OTG PCD interrupt function
> =A0USB ppc4xx: Add Synopsys DWC OTG driver kernel configuration and
> =A0 =A0Makefile
> =A0USB/ppc4xx:Synopsys DWC OTG driver enable gadget support
>
> =A0drivers/usb/Kconfig =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A02 +
> =A0drivers/usb/Makefile =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A01 +
> =A0drivers/usb/dwc/Kconfig =A0 =A0 =A0 =A0 =A0 | =A0 84 ++
> =A0drivers/usb/dwc/Makefile =A0 =A0 =A0 =A0 =A0| =A0 19 +
> =A0drivers/usb/dwc/apmppc.c =A0 =A0 =A0 =A0 =A0| =A0355 ++++++
> =A0drivers/usb/dwc/cil.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0890 +++++++++++++
> =A0drivers/usb/dwc/cil.h =A0 =A0 =A0 =A0 =A0 =A0 | 1174 +++++++++++++++++=
+
> =A0drivers/usb/dwc/cil_intr.c =A0 =A0 =A0 =A0| =A0616 +++++++++
> =A0drivers/usb/dwc/driver.h =A0 =A0 =A0 =A0 =A0| =A0 76 ++
> =A0drivers/usb/dwc/hcd.c =A0 =A0 =A0 =A0 =A0 =A0 | 2471 +++++++++++++++++=
++++++++++++++++++++
> =A0drivers/usb/dwc/hcd.h =A0 =A0 =A0 =A0 =A0 =A0 | =A0416 +++++++
> =A0drivers/usb/dwc/hcd_intr.c =A0 =A0 =A0 =A0| 1477 +++++++++++++++++++++=
+
> =A0drivers/usb/dwc/hcd_queue.c =A0 =A0 =A0 | =A0696 +++++++++++
> =A0drivers/usb/dwc/param.c =A0 =A0 =A0 =A0 =A0 | =A0180 +++
> =A0drivers/usb/dwc/pcd.c =A0 =A0 =A0 =A0 =A0 =A0 | 1791 +++++++++++++++++=
++++++++++
> =A0drivers/usb/dwc/pcd.h =A0 =A0 =A0 =A0 =A0 =A0 | =A0139 +++
> =A0drivers/usb/dwc/pcd_intr.c =A0 =A0 =A0 =A0| 2312 +++++++++++++++++++++=
+++++++++++++
> =A0drivers/usb/dwc/regs.h =A0 =A0 =A0 =A0 =A0 =A0| 1326 +++++++++++++++++=
+++
> =A0drivers/usb/gadget/Kconfig =A0 =A0 =A0 =A0| =A0 10 +
> =A0drivers/usb/gadget/gadget_chips.h | =A0 =A03 +
> =A020 files changed, 14038 insertions(+), 0 deletions(-)
> =A0create mode 100644 drivers/usb/dwc/Kconfig
> =A0create mode 100644 drivers/usb/dwc/Makefile
> =A0create mode 100644 drivers/usb/dwc/apmppc.c
> =A0create mode 100644 drivers/usb/dwc/cil.c
> =A0create mode 100644 drivers/usb/dwc/cil.h
> =A0create mode 100644 drivers/usb/dwc/cil_intr.c
> =A0create mode 100644 drivers/usb/dwc/driver.h
> =A0create mode 100644 drivers/usb/dwc/hcd.c
> =A0create mode 100644 drivers/usb/dwc/hcd.h
> =A0create mode 100644 drivers/usb/dwc/hcd_intr.c
> =A0create mode 100644 drivers/usb/dwc/hcd_queue.c
> =A0create mode 100644 drivers/usb/dwc/param.c
> =A0create mode 100644 drivers/usb/dwc/pcd.c
> =A0create mode 100644 drivers/usb/dwc/pcd.h
> =A0create mode 100644 drivers/usb/dwc/pcd_intr.c
> =A0create mode 100644 drivers/usb/dwc/regs.h
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>

RE: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Tirumala Marri]

Hi Pratyush,
<
<Hi Tirumala,
<
<I have reviwed all the patches.
<Thanks for taking my most of the diffs over your v13.
<There are few thing, which are really very important like,
<AHB disable check, EP Disable/Stall (have commented about them in
<respective patches). I am doubtful, that how does all of usbcv test
<cases passes
<without those modifications.
<So, please have a look on those comments.
<You may  add my signed-off.
<
[Tirumala Marri]  I don't do all the unit tests before submission.
I only do few device tests like Ethernet and file backed storage.
For host mode I do some basic IO test. I will try embed your suggestions.
Thx,
Marri

Re: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Olof Johansson]

On Fri, Oct 14, 2011 at 03:08:34PM -0700, tmarri@apm.com wrote:
> From: Tirumala Marri <tmarri@apm.com>

Overall this driver seems to be based on the IP vendor driver? It
looks like a completely flexible driver that implements all possible
combinations of everything.

And as a result, it's huge, and it's got a lot of extra code in there that I'm
willing to bet that you have never even executed on your platform.

Please, pare it down to the portions that you have used, and know works
and can support. If others need the extra functionality in the future,
they can and will expand and bring in what is needed.

Compare this to the dwc3 driver, which is much much cleaner.

Overall other comments:

* Register definitions are crazy long. It means you have to do lots of line
  wraps to keep the 80-character limit, which makes it hard to read the code.
* The header files seem to have been autogenerated and have unnneeded
  shift/mask operations.

* It doesn't build on non-powerpc platforms since it uses out_{b,l}e accessors.


-Olof

Re: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Olof Johansson]

Hi,

On Fri, Oct 21, 2011 at 10:33 AM, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Oct 14, 2011 at 03:08:34PM -0700, tmarri@apm.com wrote:
>> From: Tirumala Marri <tmarri@apm.com>
>
> Overall this driver seems to be based on the IP vendor driver? It
> looks like a completely flexible driver that implements all possible
> combinations of everything.

After looking a little closer at the patch set, it seems that number
one thing you need to do is simplify the driver. A lot.

I suggest starting from scratch with a clean implementation, only
bringing in the pieces you need. Over time, if other vendors are using
other aspects of the same IP, they get to add those parts of the
driver when they need it.  Look at the dwc3 driver for comparison on
cleanliness, it's much cleaner than what you've posted.


-Olof

Re: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Pratyush Anand]

On Sun, Oct 23, 2011 at 2:00 AM, Olof Johansson <olof@lixom.net> wrote:
>
> Hi,
>
> On Fri, Oct 21, 2011 at 10:33 AM, Olof Johansson <olof@lixom.net> wrote:
> > On Fri, Oct 14, 2011 at 03:08:34PM -0700, tmarri@apm.com wrote:
> >> From: Tirumala Marri <tmarri@apm.com>
> >
> > Overall this driver seems to be based on the IP vendor driver? It
> > looks like a completely flexible driver that implements all possible
> > combinations of everything.
>
> After looking a little closer at the patch set, it seems that number
> one thing you need to do is simplify the driver. A lot.
>
> I suggest starting from scratch with a clean implementation, only
> bringing in the pieces you need. Over time, if other vendors are using

Writing from scratch might take lot of time. I have used this driver
on a ARM based platform,
and found it stable after just  a bit modification , which I have
suggested in my earlier mails.
I have run all the test cases of standard testusb (except for
iscochronous), and have found
them working after the suggested modifications. So, I think that
driver is stable in a sense, and
can be included into mainline.

Regards
Pratyush


> other aspects of the same IP, they get to add those parts of the
> driver when they need it. =A0Look at the dwc3 driver for comparison on
> cleanliness, it's much cleaner than what you've posted.
>
>
> -Olof
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

RE: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Tirumala Marri]

<
<Overall this driver seems to be based on the IP vendor driver? It
<looks like a completely flexible driver that implements all possible
<combinations of everything.
<
[Tirumala Marri] Some what true that it was based on skeletal driver
Provided from IP vendor.
<And as a result, it's huge, and it's got a lot of extra code in there
<that I'm
<willing to bet that you have never even executed on your platform.
<
<Please, pare it down to the portions that you have used, and know works
<and can support. If others need the extra functionality in the future,
<they can and will expand and bring in what is needed.
<
[Tirumala Marri] I can sure review and find if there are any dead
functions.
It may not be 100% free of dead code as some of the code paths may execute
Asynchronously.
<Compare this to the dwc3 driver, which is much much cleaner.
<
[Tirumala Marri] I will check.
<Overall other comments:
<
<* Register definitions are crazy long. It means you have to do lots of
<line
<  wraps to keep the 80-character limit, which makes it hard to read the
<code.
[Tirumala Marri] This was suggestion from the review to use bit shifting.
I welcome any suggestions.
<* The header files seem to have been autogenerated and have unnneeded
<  shift/mask operations.
<
<* It doesn't build on non-powerpc platforms since it uses out_{b,l}e
<accessors.
<
[Tirumala Marri] You have to select Little Endian mode for LE platform
>From make menuconfig.

Thx,
Marri

Re: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Olof Johansson]

On Tue, Oct 25, 2011 at 2:54 PM, Tirumala Marri <tmarri@apm.com> wrote:
> <
> <Overall this driver seems to be based on the IP vendor driver? It
> <looks like a completely flexible driver that implements all possible
> <combinations of everything.
> <
> [Tirumala Marri] Some what true that it was based on skeletal driver
> Provided from IP vendor.

Please use a real mail program instead of this [name] comment style.
It's hard to read.

> <And as a result, it's huge, and it's got a lot of extra code in there
> <that I'm
> <willing to bet that you have never even executed on your platform.
> <
> <Please, pare it down to the portions that you have used, and know works
> <and can support. If others need the extra functionality in the future,
> <they can and will expand and bring in what is needed.
> <
> [Tirumala Marri] I can sure review and find if there are any dead
> functions.
> It may not be 100% free of dead code as some of the code paths may execut=
e
> Asynchronously.

No, just start over from scratch. Just leave the crap driver behind,
use it for reference but write the new one.

It's obvious given that you are already at iteration v15 and it's
still looking this bad that this is not realistic to get reviewed and
accepted as-is. I don't think staging is a good target either -- what
the driver really needs is _functional_ cut-down to only cover the use
cases that your product uses, and staging cleanups are mostly around
style and refactoring, not changing, fixing or removing functionality.

> <Compare this to the dwc3 driver, which is much much cleaner.
> <
> [Tirumala Marri] I will check.
> <Overall other comments:
> <
> <* Register definitions are crazy long. It means you have to do lots of
> <line
> < =A0wraps to keep the 80-character limit, which makes it hard to read th=
e
> <code.
> [Tirumala Marri] This was suggestion from the review to use bit shifting.
> I welcome any suggestions.

I'd be surprised if anyone asked to you that kind of crazy shifting.

> <* The header files seem to have been autogenerated and have unnneeded
> < =A0shift/mask operations.
> <
> <* It doesn't build on non-powerpc platforms since it uses out_{b,l}e
> <accessors.
> <
> [Tirumala Marri] You have to select Little Endian mode for LE platform
> From make menuconfig.

I don't think you understood what I meant. Try building an ARM config
with this driver enabled, for example, and you'll see that it breaks
the build.


-Olof

RE: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Tirumala Marri]

<
<No, just start over from scratch. Just leave the crap driver behind,
<use it for reference but write the new one.
<
<It's obvious given that you are already at iteration v15 and it's
<still looking this bad that this is not realistic to get reviewed and
<accepted as-is. I don't think staging is a good target either -- what
<the driver really needs is _functional_ cut-down to only cover the use
<cases that your product uses, and staging cleanups are mostly around
<style and refactoring, not changing, fixing or removing functionality.
<
[Tirumala Marri] I like to hear What maintainer GregKH thinks. We already
spent lot
Of time on it and passed multiple hands.
--marri

RE: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Geoff Levand]

Hi Marri,

On Wed, 2011-10-26 at 09:06 -0700, Tirumala Marri wrote:
> <
> <No, just start over from scratch. Just leave the crap driver behind,
> <use it for reference but write the new one.
> <
> <It's obvious given that you are already at iteration v15 and it's
> <still looking this bad that this is not realistic to get reviewed and
> <accepted as-is. I don't think staging is a good target either -- what
> <the driver really needs is _functional_ cut-down to only cover the use
> <cases that your product uses, and staging cleanups are mostly around
> <style and refactoring, not changing, fixing or removing functionality.
> <
> [Tirumala Marri] I like to hear What maintainer GregKH thinks. We already
> spent lot
> Of time on it and passed multiple hands.

I don't think the amount of time spent should be given much
consideration.  What matters is whether or not the code is acceptable.
Maybe you can do some coding on a whiteboard to get it right -- hahaha.

-Geoff

RE: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Tirumala Marri]

Greg,

<
<No, just start over from scratch. Just leave the crap driver behind,
<use it for reference but write the new one.
<
<It's obvious given that you are already at iteration v15 and it's
<still looking this bad that this is not realistic to get reviewed and
<accepted as-is. I don't think staging is a good target either -- what
<the driver really needs is _functional_ cut-down to only cover the use
<cases that your product uses, and staging cleanups are mostly around
<style and refactoring, not changing, fixing or removing functionality.
<
<> <Compare this to the dwc3 driver, which is much much cleaner.
<> <
Could you please comment on this. This has been reviewed so many times
And we did the major changes. I think it is un-acceptable to say to
re-write
The driver now after 15 reviews? This should have happened in first 5
reviews.


Thx,
Marri

Re: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Greg KH]

On Wed, Oct 26, 2011 at 10:17:19AM -0700, Tirumala Marri wrote:
> Greg,
> 
> <
> <No, just start over from scratch. Just leave the crap driver behind,
> <use it for reference but write the new one.

What email client are you using, I have never seen this type of
formatting, and it makes it impossible to read your replies.  Please
resolve this.

> <It's obvious given that you are already at iteration v15 and it's
> <still looking this bad that this is not realistic to get reviewed and
> <accepted as-is. I don't think staging is a good target either -- what
> <the driver really needs is _functional_ cut-down to only cover the use
> <cases that your product uses, and staging cleanups are mostly around
> <style and refactoring, not changing, fixing or removing functionality.
> <
> <> <Compare this to the dwc3 driver, which is much much cleaner.
> <> <
> Could you please comment on this. This has been reviewed so many times
> And we did the major changes. I think it is un-acceptable to say to
> re-write
> The driver now after 15 reviews? This should have happened in first 5
> reviews.

Sometimes people miss things on previous reviews, this is not a perfict
system, only the best that we currently know how to do, don't you agree?

If this driver contains lots of things that are not necessary for it to
work at this point in time, why wouldn't that be removed now?

Anyway, I think it would probably be best to get this into staging for
now, which will help you get a way to clean it up in an easier manner,
and still have users have access to it.  Is that acceptable?

thanks,

greg k-h

RE: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

[Tirumala Marri]

>
>What email client are you using, I have never seen this type of
>formatting, and it makes it impossible to read your replies.  Please
>resolve this.
Wrong prefix setting. Should be fixed now.

>Sometimes people miss things on previous reviews, this is not a perfict
>system, only the best that we currently know how to do, don't you agree?
>
>If this driver contains lots of things that are not necessary for it to
>work at this point in time, why wouldn't that be removed now?
>
>Anyway, I think it would probably be best to get this into staging for
>now, which will help you get a way to clean it up in an easier manner,
>and still have users have access to it.  Is that acceptable?
>
Yes it would be great if it can make it to staging. I will try remove
The un used code and re-send the patch.

Thx,
Marri