[PATCH][0/4] usb: sm501 ohci controller patches

[Magnus Damm]

usb: sm501 ohci controller patches

[1/4] usb: dma bounce buffer support using HCD_BOUNCE flag
[2/4] usb: only set URB_NO_TRANSFER_DMA_MAP when dma is enabled
[3/4] usb: ohci-sm501 driver
[4/4] usb: use single ohci_start()

The sm501 usb host controller is a regular ohci implementation but with the
exception that it can only do dma to a certain area of memory. This memory is
sm501 specific and the mfd driver for sm501 currently divides this memory
between the framebuffer and the usb host controller.

To support the sm501 usb host controller we need to allocate all usb data from
this sm501 memory. The function dma_declare_coherent_memory() helps us with
that - all dma_alloc_coherent() calls will end up allocating such memory.

The USB core does however sometimes seem to allocate data using kmalloc, so to
fully support that behaviour we need to add bounce buffer support. These bounce
buffers can be requested using the flag HCD_BOUNCE.

It is of course possible to implement these bounce buffers using dma map
functions such as dma_map_single(), but such a solution will be architecture
specific. This seems to be more of a chip specific problem to me, so I prefer
to have bounce buffer support in the usb core.

So the two main patches provide HCD_BOUNCE support and the ohci-sm501 driver,
and the two other ones remove duplicated code and fix a dma flag issue.

Signed-off-by: Magnus Damm <***@igel.co.jp>
---

drivers/usb/core/buffer.c | 7
drivers/usb/core/hcd.c | 115 ++++++++++++++
drivers/usb/core/hcd.h | 1
drivers/usb/core/message.c | 3
drivers/usb/host/ohci-au1xxx.c | 24 ---
drivers/usb/host/ohci-ep93xx.c | 19 --
drivers/usb/host/ohci-hcd.c | 23 ++
drivers/usb/host/ohci-lh7a404.c | 22 --
drivers/usb/host/ohci-pnx4008.c | 18 --
drivers/usb/host/ohci-pnx8550.c | 24 ---
drivers/usb/host/ohci-ppc-of.c | 21 --
drivers/usb/host/ohci-ppc-soc.c | 20 --
drivers/usb/host/ohci-s3c2410.c | 21 --
drivers/usb/host/ohci-sa1111.c | 21 --
drivers/usb/host/ohci-sm501.c | 301 ++++++++++++++++++++++++++++++++++++---
15 files changed, 433 insertions(+), 207 deletions(-)

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

usb: dma bounce buffer support using HCD_BOUNCE flag

This patch adds dma bounce buffer support to the usb core. These buffers
can be enabled with the HCD_BOUNCE flag, and they make sure that all data
passed to the host controller is allocated using dma_alloc_coherent().

Signed-off-by: Magnus Damm <***@igel.co.jp>
---

drivers/usb/core/buffer.c | 7 +-
drivers/usb/core/hcd.c | 115 +++++++++++++++++++++++++++++++++++++++++++--
drivers/usb/core/hcd.h | 1
3 files changed, 116 insertions(+), 7 deletions(-)

--- 0001/drivers/usb/core/buffer.c
+++ work/drivers/usb/core/buffer.c 2008-01-13 18:52:08.000000000 +0900
@@ -53,7 +53,8 @@ int hcd_buffer_create(struct usb_hcd *hc
char name[16];
int i, size;

- if (!hcd->self.controller->dma_mask)
+ if (!hcd->self.controller->dma_mask &&
+ !(hcd->driver->flags & HCD_BOUNCE))
return 0;

for (i = 0; i < HCD_BUFFER_POOLS; i++) {
@@ -107,7 +108,7 @@ void *hcd_buffer_alloc(
int i;

/* some USB hosts just use PIO */
- if (!bus->controller->dma_mask) {
+ if (!bus->controller->dma_mask && !(hcd->driver->flags & HCD_BOUNCE)) {
*dma = ~(dma_addr_t) 0;
return kmalloc(size, mem_flags);
}
@@ -132,7 +133,7 @@ void hcd_buffer_free(
if (!addr)
return;

- if (!bus->controller->dma_mask) {
+ if (!bus->controller->dma_mask && !(hcd->driver->flags & HCD_BOUNCE)) {
kfree(addr);
return;
}
--- 0001/drivers/usb/core/hcd.c
+++ work/drivers/usb/core/hcd.c 2008-01-13 18:52:08.000000000 +0900
@@ -35,6 +35,7 @@
#include <linux/mutex.h>
#include <asm/irq.h>
#include <asm/byteorder.h>
+#include <asm/unaligned.h>
#include <linux/platform_device.h>
#include <linux/workqueue.h>

@@ -1112,13 +1113,88 @@ void usb_hcd_unlink_urb_from_ep(struct u
}
EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);

-static void map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+static int hcd_alloc_coherent(struct usb_bus *bus,
+ gfp_t mem_flags, dma_addr_t *dma_handle,
+ void **vaddr_handle, size_t size,
+ enum dma_data_direction dir)
{
+ unsigned char *vaddr;
+
+ vaddr = hcd_buffer_alloc(bus, size + sizeof(vaddr),
+ mem_flags, dma_handle);
+ if (!vaddr)
+ return -ENOMEM;
+
+ put_unaligned((unsigned long)*vaddr_handle,
+ (unsigned long *)(vaddr + size));
+
+ if (dir == DMA_TO_DEVICE)
+ memcpy(vaddr, *vaddr_handle, size);
+
+ *vaddr_handle = vaddr;
+ return 0;
+}
+
+static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle,
+ void **vaddr_handle, size_t size,
+ enum dma_data_direction dir)
+{
+ unsigned char *vaddr = *vaddr_handle;
+
+ vaddr = (void *)get_unaligned((unsigned long *)(vaddr + size));
+
+ if (dir == DMA_FROM_DEVICE)
+ memcpy(vaddr, *vaddr_handle, size);
+
+ hcd_buffer_free(bus, size + sizeof(vaddr), *vaddr_handle, *dma_handle);
+
+ *vaddr_handle = vaddr;
+ *dma_handle = 0;
+}
+
+static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+ gfp_t mem_flags)
+{
+
+ int status = 0;
+
/* Map the URB's buffers for DMA access.
* Lower level HCD code should use *_dma exclusively,
* unless it uses pio or talks to another transport.
*/
- if (hcd->self.uses_dma && !is_root_hub(urb->dev)) {
+ if (is_root_hub(urb->dev))
+ return 0;
+
+ if (hcd->driver->flags & HCD_BOUNCE) {
+ if (usb_endpoint_xfer_control(&urb->ep->desc)
+ && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
+ status = hcd_alloc_coherent(urb->dev->bus, mem_flags,
+ &urb->setup_dma,
+ (void **)&urb->setup_packet,
+ sizeof (struct usb_ctrlrequest),
+ DMA_TO_DEVICE);
+
+ if (status == 0 && urb->transfer_buffer_length != 0
+ && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
+ status = hcd_alloc_coherent(urb->dev->bus, mem_flags,
+ &urb->transfer_dma,
+ &urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ usb_urb_dir_in(urb)
+ ? DMA_FROM_DEVICE
+ : DMA_TO_DEVICE);
+ if (status && usb_endpoint_xfer_control(&urb->ep->desc)
+ && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
+ hcd_free_coherent(urb->dev->bus,
+ &urb->setup_dma,
+ (void **)&urb->setup_packet,
+ sizeof (struct usb_ctrlrequest),
+ DMA_TO_DEVICE);
+ }
+ return status;
+ }
+
+ if (hcd->self.uses_dma) {
if (usb_endpoint_xfer_control(&urb->ep->desc)
&& !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
urb->setup_dma = dma_map_single (
@@ -1136,11 +1212,34 @@ static void map_urb_for_dma(struct usb_h
? DMA_FROM_DEVICE
: DMA_TO_DEVICE);
}
+ return 0;
}

static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
{
- if (hcd->self.uses_dma && !is_root_hub(urb->dev)) {
+ if (is_root_hub(urb->dev))
+ return;
+
+ if (hcd->driver->flags & HCD_BOUNCE) {
+ if (usb_endpoint_xfer_control(&urb->ep->desc)
+ && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
+ hcd_free_coherent(urb->dev->bus, &urb->setup_dma,
+ (void **)&urb->setup_packet,
+ sizeof (struct usb_ctrlrequest),
+ DMA_TO_DEVICE);
+ if (urb->transfer_buffer_length != 0
+ && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
+ hcd_free_coherent(urb->dev->bus,
+ &urb->transfer_dma,
+ &urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ usb_urb_dir_in(urb)
+ ? DMA_FROM_DEVICE
+ : DMA_TO_DEVICE);
+ return;
+ }
+
+ if (hcd->self.uses_dma) {
if (usb_endpoint_xfer_control(&urb->ep->desc)
&& !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
dma_unmap_single(hcd->self.controller, urb->setup_dma,
@@ -1185,7 +1284,12 @@ int usb_hcd_submit_urb (struct urb *urb,
* URBs must be submitted in process context with interrupts
* enabled.
*/
- map_urb_for_dma(hcd, urb);
+ status = map_urb_for_dma(hcd, urb, mem_flags);
+ if (unlikely(status)) {
+ usbmon_urb_submit_error(&hcd->self, urb, status);
+ goto error;
+ }
+
if (is_root_hub(urb->dev))
status = rh_urb_enqueue(hcd, urb);
else
@@ -1194,6 +1298,9 @@ int usb_hcd_submit_urb (struct urb *urb,
if (unlikely(status)) {
usbmon_urb_submit_error(&hcd->self, urb, status);
unmap_urb_for_dma(hcd, urb);
+ }
+ error:
+ if (unlikely(status)) {
urb->hcpriv = NULL;
INIT_LIST_HEAD(&urb->urb_list);
atomic_dec(&urb->use_count);
--- 0001/drivers/usb/core/hcd.h
+++ work/drivers/usb/core/hcd.h 2008-01-13 19:54:10.000000000 +0900
@@ -165,6 +165,7 @@ struct hc_driver {

int flags;
#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */
+#define HCD_BOUNCE 0x0002 /* HC needs bounce buffers */
#define HCD_USB11 0x0010 /* USB 1.1 */
#define HCD_USB2 0x0020 /* USB 2.0 */


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

For what it's worth, the SA1111 has a somewhat similar requirement
coming from a DMA erratum that makes every other megabyte invalid
in terms of DMA addressing. So it does bounce buffering ... but
it's done transparently to usbcore, in its DMA mapping operations.

Could you talk a bit more about why that approach isn't appropriate
here? My first guess would be that it just costs too much, and
this is used on systems without manny spare resources. Including
for example ones where the memcpy overheads would hurt battery
lifetimes, even if there were spare CPU cycles.

ISTR this specific issue (needing memory from a particular area) came

Post by Magnus Damm
--- 0001/drivers/usb/core/hcd.h
+++ work/drivers/usb/core/hcd.h 2008-01-13 19:54:10.000000000 +0900
@@ -165,6 +165,7 @@ struct hc_driver {
int flags;
#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */
+#define HCD_BOUNCE 0x0002 /* HC needs bounce buffers */

I think this makes "bounce buffers" become a too-overloaded term.

These are most certainly bounce buffers ... but the significance
isn't that detail, but a different one: that they're HC-local
memory. That is, normal bounce buffers couldn't do the job.

If the normal bounce buffer logic is inappropriate, then I'd suggest
coming up with a less generic term and symbol. Maybe HCD_LOCAL_MEM
would capture all the relevant details.

- Dave

Post by Magnus Damm
#define HCD_USB11 0x0010 /* USB 1.1 */
#define HCD_USB2 0x0020 /* USB 2.0 */

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

Post by David Brownell
For what it's worth, the SA1111 has a somewhat similar requirement
coming from a DMA erratum that makes every other megabyte invalid
in terms of DMA addressing. So it does bounce buffering ... but
it's done transparently to usbcore, in its DMA mapping operations.

Yeah, that sounds like a similar requirement. I suppose we're talking
CONFIG_DMABOUNCE and arch/arm/common/dmabounce.c?

My first working version did hook into the DMA mapping operations for
the sh architecture. It used declared coherent memory as buffers which
wasn't efficient but seemed to work pretty well.

Post by David Brownell
Could you talk a bit more about why that approach isn't appropriate
here? My first guess would be that it just costs too much, and
this is used on systems without manny spare resources. Including
for example ones where the memcpy overheads would hurt battery
lifetimes, even if there were spare CPU cycles.

The sm501 chip can be hooked up to several different architectures,
and it can be attached to the system in multiple ways. I have some pci
cards here that are equipped with sm501/sm502 using pci. I use them on
x86. Then I have some embedded sh boards from renesas where the sm501
is hooked up using a local bus (just a CS bank I think). The sm501 is
equipped with local memory in both cases, and the device is unable to
do dma to other parts of the system memory - at least that's true when
it's hooked up on the local bus. I think that applies for the pci
configuration as well, but I there may be some magic pci bus mastering
mode that i'm unaware of.

So using architecture specific DMA mapping operations is of course
always an option, but since the chip can be hooked up to all sorts of
architectures i think it makes more sense to make it architecture
independent. The only requirement for the architecture would be to
have dma_declare_coherent_memory() support, and that interface seems
to fit perfectly for our needs in this case.

I also suspect that other usb host controllers exist with similar
requirements, so having host controller and architecture independent
support may be a good idea for other devices as well.

Post by David Brownell
ISTR this specific issue (needing memory from a particular area) came

Post by Magnus Damm
--- 0001/drivers/usb/core/hcd.h
+++ work/drivers/usb/core/hcd.h 2008-01-13 19:54:10.000000000 +0900
@@ -165,6 +165,7 @@ struct hc_driver {
int flags;
#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */
+#define HCD_BOUNCE 0x0002 /* HC needs bounce buffers */

I think this makes "bounce buffers" become a too-overloaded term.
These are most certainly bounce buffers ... but the significance
isn't that detail, but a different one: that they're HC-local
memory. That is, normal bounce buffers couldn't do the job.
If the normal bounce buffer logic is inappropriate, then I'd suggest
coming up with a less generic term and symbol. Maybe HCD_LOCAL_MEM
would capture all the relevant details.

Sure, I totally agree with you. I wasn't sure if a flag was the
correct way to go either, for a while I was thinking of adding two
callbacks for map and unmap and let them handle things for us. I do
think a flag is cleaner though.

Any comments? Can I just change the name of the flag to HCD_LOCAL_MEM
and repost?

Thanks!

/ magnus

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

Post by Magnus Damm

Post by David Brownell
For what it's worth, the SA1111 has a somewhat similar requirement
coming from a DMA erratum that makes every other megabyte invalid
in terms of DMA addressing. So it does bounce buffering ... but
it's done transparently to usbcore, in its DMA mapping operations.

Yeah, that sounds like a similar requirement. I suppose we're talking
CONFIG_DMABOUNCE and arch/arm/common/dmabounce.c?

Yes. That could be done in a more generic manner, except for the
belief that making that be more reusable would be a Bad Thing though
and accordingly that "we" don't want to encourage such ugliness.

Post by Magnus Damm
My first working version did hook into the DMA mapping operations for
the sh architecture. It used declared coherent memory as buffers which
wasn't efficient but seemed to work pretty well.

Post by David Brownell
Could you talk a bit more about why that approach isn't appropriate
here? My first guess would be that it just costs too much, and
this is used on systems without manny spare resources. Including
for example ones where the memcpy overheads would hurt battery
lifetimes, even if there were spare CPU cycles.

The sm501 chip can be hooked up to several different architectures,
and it can be attached to the system in multiple ways.

... so it needs something not specific to the ARM or SH support.

Post by Magnus Damm
I have some pci
cards here that are equipped with sm501/sm502 using pci. I use them on
x86. Then I have some embedded sh boards from renesas where the sm501
is hooked up using a local bus (just a CS bank I think). The sm501 is
equipped with local memory in both cases,

That is, a private bank of DRAM rather than a block of SRAM
integrated onto that chip? That's a slightly different model
than I've usually seen with USB, but not uncommon otherwise.

And there are advantages to having a DRAM controller and using
external RAM, instead of what's usually a way-too-small block
of expensive SRAM. (Unless board real estate is very tight,
or the system is designed for _very_ low power operation.)

Post by Magnus Damm
and the device is unable to
do dma to other parts of the system memory - at least that's true when
it's hooked up on the local bus.

If it's just hooked up as a slave CS bank, rather than through some
symmetric interface supporting bus mastering, that'd make sense.

Post by Magnus Damm
I think that applies for the pci
configuration as well, but I there may be some magic pci bus mastering
mode that i'm unaware of.

The SM502 seems to have a DMA controller that can be a PCI bus master,
or use local memory (DRAM possibly shared with the framebuffer). I'd
expect it to just use standard PCI bus mastering ... though you might
need special PCI bus glue if its OHCI isn't presented the normal way.

I'm not surprised they didn't _also_ do the work needed to sync up with
some external DMA engine. The sort where a peripheral raises its DMA
request line to load/unload its data, and then gives Linux fits because
the drivers/dma/* code doesn't (yet) believe such engines exist. ;)

Post by Magnus Damm
So using architecture specific DMA mapping operations is of course
always an option, but since the chip can be hooked up to all sorts of
architectures i think it makes more sense to make it architecture
independent. The only requirement for the architecture would be to
have dma_declare_coherent_memory() support, and that interface seems
to fit perfectly for our needs in this case.

Although I don't think there's necessarily a requirement that such
memory be given a dma-coherent mapping in kernel address space.
It's really no different than any other chunk of DRAM. (Right?
The SM50x chip acts as a bridge to that local memory bank.)

The relevant points here being that there is current infrastructure
to declare device-local memory that's suitable for DMA ... and that
it may be the only memory suitable for such DMA, depending on the
board architecture. So to use it, we have to go through the dma
coherent allocators.

Post by Magnus Damm
I also suspect that other usb host controllers exist with similar
requirements, so having host controller and architecture independent
support may be a good idea for other devices as well.

The example I gave was the NXP/Philips ISP chips targetted at
embedded systems ... which seem to require all DMA to be from
their own (tiny) SRAMs. There are other embedded HCs I've seen
on discrete add-on chips which adopt that same model. When the
HC is better integrated (part of an SOC) it tends to get rid
of such restrictions and have access to the same address space
everything else does. (IOMMUs there are uncommon.)

Post by Magnus Damm

Post by David Brownell
ISTR this specific issue (needing memory from a particular area) came

Post by Magnus Damm
--- 0001/drivers/usb/core/hcd.h
+++ work/drivers/usb/core/hcd.h 2008-01-13 19:54:10.000000000 +0900
@@ -165,6 +165,7 @@ struct hc_driver {
int flags;
#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */
+#define HCD_BOUNCE 0x0002 /* HC needs bounce buffers */

I think this makes "bounce buffers" become a too-overloaded term.
These are most certainly bounce buffers ... but the significance
isn't that detail, but a different one: that they're HC-local
memory. That is, normal bounce buffers couldn't do the job.
If the normal bounce buffer logic is inappropriate, then I'd suggest
coming up with a less generic term and symbol. Maybe HCD_LOCAL_MEM
would capture all the relevant details.

Sure, I totally agree with you. I wasn't sure if a flag was the
correct way to go either, for a while I was thinking of adding two
callbacks for map and unmap and let them handle things for us. I do
think a flag is cleaner though.
Any comments? Can I just change the name of the flag to HCD_LOCAL_MEM
and repost?

OK by me. Maybe a few other folk have opinions though ... Alan?

- Dave

Post by Magnus Damm
Thanks!
/ magnus

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

Post by David Brownell

Post by Magnus Damm

Post by David Brownell
Could you talk a bit more about why that approach isn't appropriate
here? My first guess would be that it just costs too much, and
this is used on systems without manny spare resources. Including
for example ones where the memcpy overheads would hurt battery
lifetimes, even if there were spare CPU cycles.

The sm501 chip can be hooked up to several different architectures,
and it can be attached to the system in multiple ways.

... so it needs something not specific to the ARM or SH support.

Right. I think that's the right way forward at least. =)

Post by David Brownell

Post by Magnus Damm
I have some pci
cards here that are equipped with sm501/sm502 using pci. I use them on
x86. Then I have some embedded sh boards from renesas where the sm501
is hooked up using a local bus (just a CS bank I think). The sm501 is
equipped with local memory in both cases,

That is, a private bank of DRAM rather than a block of SRAM
integrated onto that chip? That's a slightly different model
than I've usually seen with USB, but not uncommon otherwise.

Actually, I'm not sure if it's on-chip or not. I've been trying to
locate the ram chips on my pci board, but I can't find any.

According to some text written on the pcb it should be possible to
have 16-256 MB of external SDRAM. WIthout finding any ram chips I
still seem to have 8 MB memory for frame buffer and usb host data. On
top of that there should be 16k of on-chip SRAM attached to some 8051
microcontroller block.

Post by David Brownell
And there are advantages to having a DRAM controller and using
external RAM, instead of what's usually a way-too-small block
of expensive SRAM. (Unless board real estate is very tight,
or the system is designed for _very_ low power operation.)

Post by Magnus Damm
and the device is unable to
do dma to other parts of the system memory - at least that's true when
it's hooked up on the local bus.

If it's just hooked up as a slave CS bank, rather than through some
symmetric interface supporting bus mastering, that'd make sense.

Right. One of the ways to hook up the sm501 chip seems to be "slave
mode". Good luck trying to do any dma there. =)

Post by David Brownell

Post by Magnus Damm
I think that applies for the pci
configuration as well, but I there may be some magic pci bus mastering
mode that i'm unaware of.

The SM502 seems to have a DMA controller that can be a PCI bus master,
or use local memory (DRAM possibly shared with the framebuffer). I'd
expect it to just use standard PCI bus mastering ... though you might
need special PCI bus glue if its OHCI isn't presented the normal way.

I've tested the ohci-sm501 driver using both pci interface on x86 and
in "slave mode" on sh4. Both work just fine. Maybe it's possible to
optimize by copying data using the sm501 dma controller in pci mode,
but I wonder if standard pci bus mastering will work - the sm501 ohci
controller seems to expect a local memory offset instead of physical
address for dma. This regardless of bus type. I have not tried playing
around with bus mastering and pci though.

Post by David Brownell
I'm not surprised they didn't _also_ do the work needed to sync up with
some external DMA engine. The sort where a peripheral raises its DMA
request line to load/unload its data, and then gives Linux fits because
the drivers/dma/* code doesn't (yet) believe such engines exist. ;)

Post by Magnus Damm
So using architecture specific DMA mapping operations is of course
always an option, but since the chip can be hooked up to all sorts of
architectures i think it makes more sense to make it architecture
independent. The only requirement for the architecture would be to
have dma_declare_coherent_memory() support, and that interface seems
to fit perfectly for our needs in this case.

Although I don't think there's necessarily a requirement that such
memory be given a dma-coherent mapping in kernel address space.
It's really no different than any other chunk of DRAM. (Right?
The SM50x chip acts as a bridge to that local memory bank.)

I may read Documentation/DMA-mapping.txt wrong, but I think this
coherent mapping means that the sm501 sees the same data as the cpu
and no cache flush is required.

Post by David Brownell
The relevant points here being that there is current infrastructure
to declare device-local memory that's suitable for DMA ... and that
it may be the only memory suitable for such DMA, depending on the
board architecture. So to use it, we have to go through the dma
coherent allocators.

Right. The term dma is a bit fuzzy in my mind since it is used to
describe both pci bus mastering dma and good old "manual" dma with dma
channels and various alignment, location and size restrictions for the
memory. The DMA-mapping.txt document says that all physically
contiguous buffers that are allocated using kmalloc() or
__get_free_page() are ok for DMA. That may be true for the PCI type of
bus mastering dma (which the document describes), but it's certainly
not true for the sm501 in slave mode. To use the usb host in sm501 we
need dma, and all data needs to be allocated from the local memory for
that dma to work. Basically, the usb core code seems to be designed
with pci bus mastering dma in mind.

Post by David Brownell

Post by Magnus Damm
I also suspect that other usb host controllers exist with similar
requirements, so having host controller and architecture independent
support may be a good idea for other devices as well.

The example I gave was the NXP/Philips ISP chips targetted at
embedded systems ... which seem to require all DMA to be from
their own (tiny) SRAMs. There are other embedded HCs I've seen
on discrete add-on chips which adopt that same model. When the
HC is better integrated (part of an SOC) it tends to get rid
of such restrictions and have access to the same address space
everything else does. (IOMMUs there are uncommon.)

Exactly.

Post by David Brownell

Post by Magnus Damm
Any comments? Can I just change the name of the flag to HCD_LOCAL_MEM
and repost?

OK by me. Maybe a few other folk have opinions though ... Alan?

Great, thanks. I'll repost later this week unless there are any objections.

/ magnus

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Alan Stern]

Post by David Brownell

Post by Magnus Damm
Any comments? Can I just change the name of the flag to HCD_LOCAL_MEM
and repost?

OK by me. Maybe a few other folk have opinions though ... Alan?

The core changes look okay, except for some questions of style
preference.

In map_urb_for_dma() and unmap_urb_for_dma(), I think it would look
cleaner if the tests were rearranged along these lines:

if (is_root_hub(urb->dev))
return 0;

if (usb_endpoint_xfer_control(&urb->ep->desc)
&& !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP)) {
if (hcd->self.uses_dma)
urb->setup_dma = dma_map_single...
else if (hcd->driver->flags & HCD_LOCAL_MEM)
status = hcd_alloc_coherent...
}

if (urb->transfer_buffer_length != 0
&& !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
...

Post by David Brownell
if (unlikely(status)) {
usbmon_urb_submit_error(&hcd->self, urb, status);
unmap_urb_for_dma(hcd, urb);
+ }
+ if (unlikely(status)) {
urb->hcpriv = NULL;
INIT_LIST_HEAD(&urb->urb_list);
atomic_dec(&urb->use_count);

There's no reason to close the block and start another "if" statement.
Just put the statement label inside the original block.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

usb: only set URB_NO_TRANSFER_DMA_MAP when dma is enabled

usb_sg_init() currently sets URB_NO_TRANSFER_DMA_MAP even if dma isn't
enabled. This patch changes the code so URB_NO_TRANSFER_DMA_MAP gets
set only when dma is enabled and transfer_dma is valid.

Signed-off-by: Magnus Damm <***@igel.co.jp>
---

drivers/usb/core/message.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

--- 0001/drivers/usb/core/message.c
+++ work/drivers/usb/core/message.c 2008-01-13 18:53:13.000000000 +0900
@@ -394,7 +394,7 @@ int usb_sg_init (
if (!io->urbs)
goto nomem;

- urb_flags = URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT;
+ urb_flags = URB_NO_INTERRUPT;
if (usb_pipein (pipe))
urb_flags |= URB_SHORT_NOT_OK;

@@ -432,6 +432,7 @@ int usb_sg_init (
* to prevent stale pointers and to help spot bugs.
*/
if (dma) {
+ io->urbs[i]->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
io->urbs [i]->transfer_dma = sg_dma_address (sg + i);
len = sg_dma_len (sg + i);
#if defined(CONFIG_HIGHMEM) || defined(CONFIG_GART_IOMMU)

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

Hi guys,

Thanks for all your help so far.

Post by Magnus Damm
usb: only set URB_NO_TRANSFER_DMA_MAP when dma is enabled
usb_sg_init() currently sets URB_NO_TRANSFER_DMA_MAP even if dma isn't
enabled. This patch changes the code so URB_NO_TRANSFER_DMA_MAP gets
set only when dma is enabled and transfer_dma is valid.

What about this patch? Any thoughts?

This patch is needed for usb storage to work properly with ohci-sm501.
Should URB_NO_TRANSFER_DMA_MAP really be set even if dma isn't used? I
say not.

Thanks,

/ magnus

Post by Magnus Damm
--- 0001/drivers/usb/core/message.c
+++ work/drivers/usb/core/message.c 2008-01-13 18:53:13.000000000 +0900
@@ -394,7 +394,7 @@ int usb_sg_init (
if (!io->urbs)
goto nomem;
- urb_flags = URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT;
+ urb_flags = URB_NO_INTERRUPT;
if (usb_pipein (pipe))
urb_flags |= URB_SHORT_NOT_OK;
@@ -432,6 +432,7 @@ int usb_sg_init (
* to prevent stale pointers and to help spot bugs.
*/
if (dma) {
+ io->urbs[i]->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
io->urbs [i]->transfer_dma = sg_dma_address (sg + i);
len = sg_dma_len (sg + i);
#if defined(CONFIG_HIGHMEM) || defined(CONFIG_GART_IOMMU)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

Post by Magnus Damm
Hi guys,
Thanks for all your help so far.

Post by Magnus Damm
usb: only set URB_NO_TRANSFER_DMA_MAP when dma is enabled
usb_sg_init() currently sets URB_NO_TRANSFER_DMA_MAP even if dma isn't
enabled. This patch changes the code so URB_NO_TRANSFER_DMA_MAP gets
set only when dma is enabled and transfer_dma is valid.

What about this patch? Any thoughts?
This patch is needed for usb storage to work properly with ohci-sm501.
Should URB_NO_TRANSFER_DMA_MAP really be set even if dma isn't used? I
say not.

Makes sense.

Post by Magnus Damm
Thanks,
/ magnus

Post by Magnus Damm
--- 0001/drivers/usb/core/message.c
+++ work/drivers/usb/core/message.c 2008-01-13 18:53:13.000000000 +0900
@@ -394,7 +394,7 @@ int usb_sg_init (
if (!io->urbs)
goto nomem;
- urb_flags = URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT;
+ urb_flags = URB_NO_INTERRUPT;
if (usb_pipein (pipe))
urb_flags |= URB_SHORT_NOT_OK;
@@ -432,6 +432,7 @@ int usb_sg_init (
* to prevent stale pointers and to help spot bugs.
*/
if (dma) {
+ io->urbs[i]->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
io->urbs [i]->transfer_dma = sg_dma_address (sg + i);
len = sg_dma_len (sg + i);
#if defined(CONFIG_HIGHMEM) || defined(CONFIG_GART_IOMMU)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Alan Stern]

Post by Magnus Damm
Hi guys,
Thanks for all your help so far.

Post by Magnus Damm
usb: only set URB_NO_TRANSFER_DMA_MAP when dma is enabled
usb_sg_init() currently sets URB_NO_TRANSFER_DMA_MAP even if dma isn't
enabled. This patch changes the code so URB_NO_TRANSFER_DMA_MAP gets
set only when dma is enabled and transfer_dma is valid.

What about this patch? Any thoughts?
This patch is needed for usb storage to work properly with ohci-sm501.
Should URB_NO_TRANSFER_DMA_MAP really be set even if dma isn't used? I
say not.

The URB_NO_TRANSFER_DMA_MAP setting is moot when DMA isn't used; the
flag shouldn't even be looked at. But if you need this patch for the
ohci-sm501 then go right ahead; there's nothing wrong with it.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

Post by Alan Stern

Post by Magnus Damm
Hi guys,
Thanks for all your help so far.

Post by Magnus Damm
usb: only set URB_NO_TRANSFER_DMA_MAP when dma is enabled
usb_sg_init() currently sets URB_NO_TRANSFER_DMA_MAP even if dma isn't
enabled. This patch changes the code so URB_NO_TRANSFER_DMA_MAP gets
set only when dma is enabled and transfer_dma is valid.

What about this patch? Any thoughts?
This patch is needed for usb storage to work properly with ohci-sm501.
Should URB_NO_TRANSFER_DMA_MAP really be set even if dma isn't used? I
say not.

The URB_NO_TRANSFER_DMA_MAP setting is moot when DMA isn't used; the
flag shouldn't even be looked at. But if you need this patch for the
ohci-sm501 then go right ahead; there's nothing wrong with it.

Alan and David, thanks for your comments.

The HCD_LOCAL_MEM code only copies data into the bounce buffers if
URB_NO_TRANSFER_DMA_MAP is unset. If that flag is set then the code
expects the buffer to have been allocated using dma_alloc_coherent()
and therefore there is no point in copying it. Or mapping it, as the
original dma map code did. Anyway, so having that flag set even though
dma is disabled and the dma handle invalid (data is not at all in
buffers allocated using dma_alloc_coherent) breaks usb storage for
ohci-sm501.

/ magnus

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Alan Stern]

Post by Magnus Damm
Alan and David, thanks for your comments.
The HCD_LOCAL_MEM code only copies data into the bounce buffers if
URB_NO_TRANSFER_DMA_MAP is unset. If that flag is set then the code
expects the buffer to have been allocated using dma_alloc_coherent()
and therefore there is no point in copying it. Or mapping it, as the
original dma map code did. Anyway, so having that flag set even though
dma is disabled and the dma handle invalid (data is not at all in
buffers allocated using dma_alloc_coherent) breaks usb storage for
ohci-sm501.

If usb-storage breaks then won't other drivers like ub break as well?
The documentation for URB_NO_TRANSFER_DMA_MAP and URB_NO_SETUP_DMA_MAP
says only that the DMA handle must be valid; it doesn't say that the
buffer had to allocated by usb_buffer_alloc(), hcd_buffer_alloc(), or
dma_alloc_coherent().

Now, there's nothing wrong with your patch. It avoids setting
URB_NO_TRANSFER_DMA_MAP when the DMA handle is invalid. But the way
you are overloading the meaning of that flag could easily lead to
problems with other drivers.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

usb: ohci-sm501 driver

This patch adds sm501 ohci support. It's all very straightforward with the
exception of dma_declare_coherent_memory() and HCD_BOUNCE. Together they
are used to ensure that usb data is allocated using dma_alloc_coherent(),
and that only valid dma memory is used to allocate from. This driver is
a platform device, and the mfd driver sm501.c is already creating one
usb host controller instance per sm501.

Signed-off-by: Magnus Damm <***@igel.co.jp>
---

drivers/usb/host/ohci-hcd.c | 5
drivers/usb/host/ohci-sm501.c | 282 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 287 insertions(+)

--- 0001/drivers/usb/host/ohci-hcd.c
+++ work/drivers/usb/host/ohci-hcd.c 2008-01-13 17:46:20.000000000 +0900
@@ -1048,6 +1048,11 @@ MODULE_LICENSE ("GPL");
#define SSB_OHCI_DRIVER ssb_ohci_driver
#endif

+#ifdef CONFIG_MFD_SM501
+#include "ohci-sm501.c"
+#define PLATFORM_DRIVER ohci_hcd_sm501_driver
+#endif
+
#if !defined(PCI_DRIVER) && \
!defined(PLATFORM_DRIVER) && \
!defined(OF_PLATFORM_DRIVER) && \
--- /dev/null
+++ work/drivers/usb/host/ohci-sm501.c 2008-01-13 17:58:22.000000000 +0900
@@ -0,0 +1,282 @@
+/*
+ * OHCI HCD (Host Controller Driver) for USB.
+ *
+ * (C) Copyright 1999 Roman Weissgaerber <***@vienna.at>
+ * (C) Copyright 2000-2005 David Brownell
+ * (C) Copyright 2002 Hewlett-Packard Company
+ * (C) Copyright 2008 Magnus Damm
+ *
+ * SM501 Bus Glue - based on ohci-omap.c
+ * Modified for OMAP by Tony Lindgren <***@atomide.com>
+ * Based on the 2.4 OMAP OHCI driver originally done by MontaVista Software Inc.
+ * and on ohci-sa1111.c by Christopher Hoover <***@hpl.hp.com>
+ *
+ * This file is licenced under the GPL.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/sm501.h>
+#include <linux/sm501-regs.h>
+
+static int ohci_sm501_start (struct usb_hcd *hcd)
+{
+ struct device *dev = hcd->self.controller;
+ struct ohci_hcd *ohci = hcd_to_ohci (hcd);
+ int ret;
+
+ if ((ret = ohci_init(ohci)) < 0)
+ return ret;
+
+ if ((ret = ohci_run (ohci)) < 0) {
+ dev_err (dev, "can't start %s", hcd->self.bus_name);
+ ohci_stop (hcd);
+ return ret;
+ }
+ return 0;
+}
+
+static int usb_hcd_sm501_probe (const struct hc_driver *driver,
+ struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res, *mem;
+ int retval, irq;
+ struct usb_hcd *hcd = 0;
+ struct ohci_hcd *ohci;
+ static u64 dummy_mask = 1 << 0;
+
+ irq = retval = platform_get_irq(pdev, 0);
+ if (retval < 0) {
+ goto err0;
+ }
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (mem == NULL) {
+ dev_err(dev, "no resource definition for memory\n");
+ retval = -ENOENT;
+ goto err0;
+ }
+
+ if (!request_mem_region(mem->start, mem->end - mem->start + 1,
+ pdev->name)) {
+ dev_err(dev, "request_mem_region failed\n");
+ retval = -EBUSY;
+ goto err0;
+ }
+
+ /* The sm501 chip is equipped with local memory that may be used
+ * by on-chip devices such as the video controller and the usb host.
+ * This driver uses dma_declare_coherent_memory() to make sure
+ * usb allocations with dma_alloc_coherent() allocate from
+ * this local memory. The dma_handle returned by dma_alloc_coherent()
+ * will be an offset starting from 0 for the first local memory byte.
+ *
+ * So as long as data is allocated using dma_alloc_coherent() all is
+ * fine. This is however not always the case - buffers may be allocated
+ * using kmalloc() - so the usb core needs to be told that it must copy
+ * data into our local memory if the buffers happen to be placed in
+ * regular memory. The HCD_BOUNCE flag does just that.
+ */
+
+ if (!dma_declare_coherent_memory(dev, mem->start,
+ mem->start - mem->parent->start,
+ (mem->end - mem->start) + 1,
+ DMA_MEMORY_MAP |
+ DMA_MEMORY_EXCLUSIVE)) {
+ dev_err(dev, "cannot declare coherent memory\n");
+ retval = -ENXIO;
+ goto err1;
+ }
+
+ /* allocate, reserve and remap resources for registers */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
+ dev_err(dev, "no resource definition for registers\n");
+ retval = -ENOENT;
+ goto err2;
+ }
+
+ hcd = usb_create_hcd (driver, &pdev->dev, pdev->dev.bus_id);
+ if (!hcd) {
+ retval = -ENOMEM;
+ goto err2;
+ }
+
+ hcd->rsrc_start = res->start;
+ hcd->rsrc_len = res->end - res->start + 1;
+
+ if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, pdev->name)) {
+ dev_err(dev, "request_mem_region failed\n");
+ retval = -EBUSY;
+ goto err3;
+ }
+
+ hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
+ if (hcd->regs == NULL) {
+ dev_err(dev, "cannot remap registers\n");
+ retval = -ENXIO;
+ goto err4;
+ }
+
+ ohci = hcd_to_ohci(hcd);
+ ohci_hcd_init(ohci);
+
+ retval = usb_add_hcd(hcd, irq, IRQF_DISABLED | IRQF_SHARED);
+ if (retval)
+ goto err4;
+
+ /* enable power and unmask interrupts */
+
+ sm501_unit_power(dev->parent, SM501_GATE_USB_HOST, 1);
+ sm501_modify_reg(dev->parent, SM501_IRQ_MASK, 1 << 6, 0);
+
+ return 0;
+err4:
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+err3:
+ usb_put_hcd(hcd);
+err2:
+ dma_release_declared_memory(dev);
+err1:
+ release_mem_region(mem->start, mem->end - mem->start + 1);
+err0:
+ return retval;
+}
+
+static inline void usb_hcd_sm501_remove (struct usb_hcd *hcd,
+ struct platform_device *pdev)
+{
+ struct resource *mem;
+
+ usb_remove_hcd(hcd);
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+ usb_put_hcd(hcd);
+ dma_release_declared_memory(&pdev->dev);
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ release_mem_region(mem->start, mem->end - mem->start + 1);
+
+ /* mask interrupts and disable power */
+
+ sm501_modify_reg(pdev->dev.parent, SM501_IRQ_MASK, 0, 1 << 6);
+ sm501_unit_power(pdev->dev.parent, SM501_GATE_USB_HOST, 0);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static const struct hc_driver ohci_sm501_hc_driver = {
+ .description = hcd_name,
+ .product_desc = "SM501 OHCI",
+ .hcd_priv_size = sizeof(struct ohci_hcd),
+
+ /*
+ * generic hardware linkage
+ */
+ .irq = ohci_irq,
+ .flags = HCD_USB11 | HCD_MEMORY | HCD_BOUNCE,
+
+ /*
+ * basic lifecycle operations
+ */
+ .start = ohci_sm501_start,
+ .stop = ohci_stop,
+ .shutdown = ohci_shutdown,
+
+ /*
+ * managing i/o requests and associated device resources
+ */
+ .urb_enqueue = ohci_urb_enqueue,
+ .urb_dequeue = ohci_urb_dequeue,
+ .endpoint_disable = ohci_endpoint_disable,
+
+ /*
+ * scheduling support
+ */
+ .get_frame_number = ohci_get_frame,
+
+ /*
+ * root hub support
+ */
+ .hub_status_data = ohci_hub_status_data,
+ .hub_control = ohci_hub_control,
+ .hub_irq_enable = ohci_rhsc_enable,
+#ifdef CONFIG_PM
+ .bus_suspend = ohci_bus_suspend,
+ .bus_resume = ohci_bus_resume,
+#endif
+ .start_port_reset = ohci_start_port_reset,
+};
+
+/*-------------------------------------------------------------------------*/
+
+static int ohci_hcd_sm501_drv_probe(struct platform_device *dev)
+{
+ return usb_hcd_sm501_probe(&ohci_sm501_hc_driver, dev);
+}
+
+static int ohci_hcd_sm501_drv_remove(struct platform_device *dev)
+{
+ struct usb_hcd *hcd = platform_get_drvdata(dev);
+
+ usb_hcd_sm501_remove(hcd, dev);
+ platform_set_drvdata(dev, NULL);
+
+ return 0;
+}
+
+/*-------------------------------------------------------------------------*/
+
+#ifdef CONFIG_PM
+
+static int ohci_sm501_suspend(struct platform_device *pdev, pm_message_t message)
+{
+ struct device *dev = &pdev->dev;
+ struct ohci_hcd *ohci = hcd_to_ohci(platform_get_drvdata(pdev));
+
+ if (time_before(jiffies, ohci->next_statechange))
+ msleep(5);
+ ohci->next_statechange = jiffies;
+
+ sm501_unit_power(dev->parent, SM501_GATE_USB_HOST, 0);
+ ohci_to_hcd(ohci)->state = HC_STATE_SUSPENDED;
+ dev->power.power_state = PMSG_SUSPEND;
+ return 0;
+}
+
+static int ohci_sm501_resume(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ohci_hcd *ohci = hcd_to_ohci(platform_get_drvdata(pdev));
+
+ if (time_before(jiffies, ohci->next_statechange))
+ msleep(5);
+ ohci->next_statechange = jiffies;
+
+ sm501_unit_power(dev->parent, SM501_GATE_USB_HOST, 1);
+ dev->power.power_state = PMSG_ON;
+ usb_hcd_resume_root_hub(platform_get_drvdata(pdev));
+ return 0;
+}
+
+#endif
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * Driver definition to register with the SM501 bus
+ */
+static struct platform_driver ohci_hcd_sm501_driver = {
+ .probe = ohci_hcd_sm501_drv_probe,
+ .remove = ohci_hcd_sm501_drv_remove,
+ .shutdown = usb_hcd_platform_shutdown,
+#ifdef CONFIG_PM
+ .suspend = ohci_sm501_suspend,
+ .resume = ohci_sm501_resume,
+#endif
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "sm501-usb",
+ },
+};

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

Post by Magnus Damm
+static int ohci_hcd_sm501_drv_probe(struct platform_device *dev)
+{
+ return usb_hcd_sm501_probe(&ohci_sm501_hc_driver, dev);
+}
+
+static int ohci_hcd_sm501_drv_remove(struct platform_device *dev)
+{
+ struct usb_hcd *hcd = platform_get_drvdata(dev);
+
+ usb_hcd_sm501_remove(hcd, dev);
+ platform_set_drvdata(dev, NULL);
+
+ return 0;
+}

I'll repeat the feedback I've given to some other OHCI drivers
recently.

That "two probe routines" idiom needs to die. Fix it so there
is only one probe() routine, and one remove() routine.

- Dave


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

Post by Magnus Damm
usb: ohci-sm501 driver
This patch adds sm501 ohci support.

Just sm501, not the sm502 you had mentioned?

Post by Magnus Damm
It's all very straightforward with the
exception of dma_declare_coherent_memory() and HCD_BOUNCE.

If I skimmed those docs right, sm502 won't always need that.

For example, if the sm502 is hooked up through PCI, it doesn't
necessarily even need to have local memory ... or use it for
OHCI, if it's present. (Certainly there's a DMA controller
on the chip that can do PCI bus mastering. I'm assuming that
the OHCI silicon is hooked up to that controller...)

If that's so, that stuff should be a function of how the
platform device was configured. For example, an sm502 on
a PCI card might set up a bunch of platform devices for
its various functions (assuming they don't show up as normal
functions) ... but without memory resource #1, thereby
signaling that it doesn't need to use local memory or
do the dma_alloc_coherent() dance.

Post by Magnus Damm
Together they
are used to ensure that usb data is allocated using dma_alloc_coherent(),
and that only valid dma memory is used to allocate from. This driver is
a platform device, and the mfd driver sm501.c is already creating one
usb host controller instance per sm501.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

Post by David Brownell

Post by Magnus Damm
usb: ohci-sm501 driver
This patch adds sm501 ohci support.

Just sm501, not the sm502 you had mentioned?

The driver works with sm502 as well. sm501 and sm502 seem very
similar. The sm502 device is using the same pci device id as sm501.
The sm501 mfd driver works well with sm502 devices - at least for my
little setup.

Post by David Brownell

Post by Magnus Damm
It's all very straightforward with the
exception of dma_declare_coherent_memory() and HCD_BOUNCE.

If I skimmed those docs right, sm502 won't always need that.
For example, if the sm502 is hooked up through PCI, it doesn't
necessarily even need to have local memory ... or use it for
OHCI, if it's present. (Certainly there's a DMA controller
on the chip that can do PCI bus mastering. I'm assuming that
the OHCI silicon is hooked up to that controller...)

You may be right. I'm not sure if it can do PCI bus mastering and if
the ohci block is hooked up to the dma controller. I have not played
around so much with the PCI interface.

My main goal is to add support for usb to some embedded boards made by
Renesas. They are hooked up with "slave mode" so they need the bounce
buffers. And the bounce buffers _do_ work for pci as well - I just
double checked using my sm502 pci card.

Post by David Brownell
If that's so, that stuff should be a function of how the
platform device was configured. For example, an sm502 on
a PCI card might set up a bunch of platform devices for
its various functions (assuming they don't show up as normal
functions) ... but without memory resource #1, thereby
signaling that it doesn't need to use local memory or
do the dma_alloc_coherent() dance.

I totally agree. I can probably allocate find time to hack up pci bus
mastering support - if it works properly that is. Given that, would it
be possible to accept this implementation as a first step? It does
work for both "slave mode" and pci, even if pci may be suboptimal. I
prefer to see pci bus mastering as an enhancement.

/ magnus

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

Post by Magnus Damm

Post by David Brownell
If that's so, that stuff should be a function of how the
platform device was configured.  For example, an sm502 on
a PCI card might set up a bunch of platform devices for
its various functions (assuming they don't show up as normal
functions) ... but without memory resource #1, thereby
signaling that it doesn't need to use local memory or
do the dma_alloc_coherent() dance.

I totally agree. I can probably allocate find time to hack up pci bus
mastering support - if it works properly that is.

If it works normally, all you should need to do is get rid
of the flag saying to use local memory, and not set it up...

Post by Magnus Damm
Given that, would it
be possible to accept this implementation as a first step? It does
work for both "slave mode" and pci, even if pci may be suboptimal. I
prefer to see pci bus mastering as an enhancement.

Sure.

- Dave

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

usb: use single ohci_start()

This patch adds adds an ohci_start() function that easily can be shared
between many ohci drivers. This patch only changes drivers to ohci_start()
when the driver specific implementation exactly match ohci_start(). It also
seems popular to sometimes do ohci_init() in .reset instead of .start, and if
those drivers can be rewritten then it may be possible to use ohci_start()
in even more drivers.

Signed-off-by: Magnus Damm <***@igel.co.jp>
---

drivers/usb/host/ohci-au1xxx.c | 24 +-----------------------
drivers/usb/host/ohci-ep93xx.c | 19 +------------------
drivers/usb/host/ohci-hcd.c | 18 ++++++++++++++++++
drivers/usb/host/ohci-lh7a404.c | 22 +---------------------
drivers/usb/host/ohci-pnx4008.c | 18 +-----------------
drivers/usb/host/ohci-pnx8550.c | 24 +-----------------------
drivers/usb/host/ohci-ppc-of.c | 21 +--------------------
drivers/usb/host/ohci-ppc-soc.c | 20 +-------------------
drivers/usb/host/ohci-s3c2410.c | 21 +--------------------
drivers/usb/host/ohci-sa1111.c | 21 +--------------------
drivers/usb/host/ohci-sm501.c | 19 +------------------
11 files changed, 28 insertions(+), 199 deletions(-)

--- 0001/drivers/usb/host/ohci-au1xxx.c
+++ work/drivers/usb/host/ohci-au1xxx.c 2008-01-13 19:41:47.000000000 +0900
@@ -231,28 +231,6 @@ static void usb_ohci_au1xxx_remove(struc

/*-------------------------------------------------------------------------*/

-static int __devinit
-ohci_au1xxx_start (struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci (hcd);
- int ret;
-
- ohci_dbg (ohci, "ohci_au1xxx_start, ohci:%p", ohci);
-
- if ((ret = ohci_init (ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run (ohci)) < 0) {
- err ("can't start %s", hcd->self.bus_name);
- ohci_stop (hcd);
- return ret;
- }
-
- return 0;
-}
-
-/*-------------------------------------------------------------------------*/
-
static const struct hc_driver ohci_au1xxx_hc_driver = {
.description = hcd_name,
.product_desc = "Au1xxx OHCI",
@@ -267,7 +245,7 @@ static const struct hc_driver ohci_au1xx
/*
* basic lifecycle operations
*/
- .start = ohci_au1xxx_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,

--- 0001/drivers/usb/host/ohci-ep93xx.c
+++ work/drivers/usb/host/ohci-ep93xx.c 2008-01-13 19:41:22.000000000 +0900
@@ -103,30 +103,13 @@ static void usb_hcd_ep93xx_remove(struct
usb_put_hcd(hcd);
}

-static int __devinit ohci_ep93xx_start(struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci(hcd);
- int ret;
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run(ohci)) < 0) {
- err("can't start %s", hcd->self.bus_name);
- ohci_stop(hcd);
- return ret;
- }
-
- return 0;
-}
-
static struct hc_driver ohci_ep93xx_hc_driver = {
.description = hcd_name,
.product_desc = "EP93xx OHCI",
.hcd_priv_size = sizeof(struct ohci_hcd),
.irq = ohci_irq,
.flags = HCD_USB11 | HCD_MEMORY,
- .start = ohci_ep93xx_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,
.urb_enqueue = ohci_urb_enqueue,
--- 0011/drivers/usb/host/ohci-hcd.c
+++ work/drivers/usb/host/ohci-hcd.c 2008-01-13 19:31:56.000000000 +0900
@@ -79,6 +79,7 @@ static const char hcd_name [] = "ohci_hc

static void ohci_dump (struct ohci_hcd *ohci, int verbose);
static int ohci_init (struct ohci_hcd *ohci);
+static int ohci_start (struct usb_hcd *hcd);
static void ohci_stop (struct usb_hcd *hcd);

#if defined(CONFIG_PM) || defined(CONFIG_PCI)
@@ -870,6 +871,23 @@ static irqreturn_t ohci_irq (struct usb_

/*-------------------------------------------------------------------------*/

+static int ohci_start (struct usb_hcd *hcd)
+{
+ struct device *dev = hcd->self.controller;
+ struct ohci_hcd *ohci = hcd_to_ohci (hcd);
+ int ret;
+
+ if ((ret = ohci_init(ohci)) < 0)
+ return ret;
+
+ if ((ret = ohci_run (ohci)) < 0) {
+ dev_err (dev, "can't start %s", hcd->self.bus_name);
+ ohci_stop (hcd);
+ return ret;
+ }
+ return 0;
+}
+
static void ohci_stop (struct usb_hcd *hcd)
{
struct ohci_hcd *ohci = hcd_to_ohci (hcd);
--- 0001/drivers/usb/host/ohci-lh7a404.c
+++ work/drivers/usb/host/ohci-lh7a404.c 2008-01-13 19:40:59.000000000 +0900
@@ -138,26 +138,6 @@ void usb_hcd_lh7a404_remove (struct usb_

/*-------------------------------------------------------------------------*/

-static int __devinit
-ohci_lh7a404_start (struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci (hcd);
- int ret;
-
- ohci_dbg (ohci, "ohci_lh7a404_start, ohci:%p", ohci);
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run (ohci)) < 0) {
- err ("can't start %s", hcd->self.bus_name);
- ohci_stop (hcd);
- return ret;
- }
- return 0;
-}
-
-/*-------------------------------------------------------------------------*/
-
static const struct hc_driver ohci_lh7a404_hc_driver = {
.description = hcd_name,
.product_desc = "LH7A404 OHCI",
@@ -172,7 +152,7 @@ static const struct hc_driver ohci_lh7a4
/*
* basic lifecycle operations
*/
- .start = ohci_lh7a404_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,

--- 0001/drivers/usb/host/ohci-pnx4008.c
+++ work/drivers/usb/host/ohci-pnx4008.c 2008-01-13 19:40:25.000000000 +0900
@@ -230,22 +230,6 @@ static void pnx4008_stop_hc(void)
__raw_writel(tmp, USB_OTG_STAT_CONTROL);
}

-static int __devinit ohci_pnx4008_start(struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci(hcd);
- int ret;
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run(ohci)) < 0) {
- dev_err(hcd->self.controller, "can't start\n");
- ohci_stop(hcd);
- return ret;
- }
- return 0;
-}
-
static const struct hc_driver ohci_pnx4008_hc_driver = {
.description = hcd_name,
.product_desc = "pnx4008 OHCI",
@@ -260,7 +244,7 @@ static const struct hc_driver ohci_pnx40
/*
* basic lifecycle operations
*/
- .start = ohci_pnx4008_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,

--- 0001/drivers/usb/host/ohci-pnx8550.c
+++ work/drivers/usb/host/ohci-pnx8550.c 2008-01-13 19:40:03.000000000 +0900
@@ -145,28 +145,6 @@ void usb_hcd_pnx8550_remove (struct usb_

/*-------------------------------------------------------------------------*/

-static int __devinit
-ohci_pnx8550_start (struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci (hcd);
- int ret;
-
- ohci_dbg (ohci, "ohci_pnx8550_start, ohci:%p", ohci);
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run (ohci)) < 0) {
- err ("can't start %s", hcd->self.bus_name);
- ohci_stop (hcd);
- return ret;
- }
-
- return 0;
-}
-
-/*-------------------------------------------------------------------------*/
-
static const struct hc_driver ohci_pnx8550_hc_driver = {
.description = hcd_name,
.product_desc = "PNX8550 OHCI",
@@ -181,7 +159,7 @@ static const struct hc_driver ohci_pnx85
/*
* basic lifecycle operations
*/
- .start = ohci_pnx8550_start,
+ .start = ohci_start,
.stop = ohci_stop,

/*
--- 0001/drivers/usb/host/ohci-ppc-of.c
+++ work/drivers/usb/host/ohci-ppc-of.c 2008-01-13 19:39:14.000000000 +0900
@@ -18,25 +18,6 @@
#include <asm/of_platform.h>
#include <asm/prom.h>

-
-static int __devinit
-ohci_ppc_of_start(struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci(hcd);
- int ret;
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run(ohci)) < 0) {
- err("can't start %s", ohci_to_hcd(ohci)->self.bus_name);
- ohci_stop(hcd);
- return ret;
- }
-
- return 0;
-}
-
static const struct hc_driver ohci_ppc_of_hc_driver = {
.description = hcd_name,
.product_desc = "OF OHCI",
@@ -51,7 +32,7 @@ static const struct hc_driver ohci_ppc_o
/*
* basic lifecycle operations
*/
- .start = ohci_ppc_of_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,

--- 0001/drivers/usb/host/ohci-ppc-soc.c
+++ work/drivers/usb/host/ohci-ppc-soc.c 2008-01-13 19:38:53.000000000 +0900
@@ -120,24 +120,6 @@ static void usb_hcd_ppc_soc_remove(struc
usb_put_hcd(hcd);
}

-static int __devinit
-ohci_ppc_soc_start(struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci(hcd);
- int ret;
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run(ohci)) < 0) {
- err("can't start %s", ohci_to_hcd(ohci)->self.bus_name);
- ohci_stop(hcd);
- return ret;
- }
-
- return 0;
-}
-
static const struct hc_driver ohci_ppc_soc_hc_driver = {
.description = hcd_name,
.hcd_priv_size = sizeof(struct ohci_hcd),
@@ -151,7 +133,7 @@ static const struct hc_driver ohci_ppc_s
/*
* basic lifecycle operations
*/
- .start = ohci_ppc_soc_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,

--- 0001/drivers/usb/host/ohci-s3c2410.c
+++ work/drivers/usb/host/ohci-s3c2410.c 2008-01-13 19:37:52.000000000 +0900
@@ -412,25 +412,6 @@ static int usb_hcd_s3c2410_probe (const

/*-------------------------------------------------------------------------*/

-static int
-ohci_s3c2410_start (struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci (hcd);
- int ret;
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run (ohci)) < 0) {
- err ("can't start %s", hcd->self.bus_name);
- ohci_stop (hcd);
- return ret;
- }
-
- return 0;
-}
-
-
static const struct hc_driver ohci_s3c2410_hc_driver = {
.description = hcd_name,
.product_desc = "S3C24XX OHCI",
@@ -445,7 +426,7 @@ static const struct hc_driver ohci_s3c24
/*
* basic lifecycle operations
*/
- .start = ohci_s3c2410_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,

--- 0001/drivers/usb/host/ohci-sa1111.c
+++ work/drivers/usb/host/ohci-sa1111.c 2008-01-13 19:37:24.000000000 +0900
@@ -178,25 +178,6 @@ void usb_hcd_sa1111_remove (struct usb_h

/*-------------------------------------------------------------------------*/

-static int __devinit
-ohci_sa1111_start (struct usb_hcd *hcd)
-{
- struct ohci_hcd *ohci = hcd_to_ohci (hcd);
- int ret;
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run (ohci)) < 0) {
- err ("can't start %s", hcd->self.bus_name);
- ohci_stop (hcd);
- return ret;
- }
- return 0;
-}
-
-/*-------------------------------------------------------------------------*/
-
static const struct hc_driver ohci_sa1111_hc_driver = {
.description = hcd_name,
.product_desc = "SA-1111 OHCI",
@@ -211,7 +192,7 @@ static const struct hc_driver ohci_sa111
/*
* basic lifecycle operations
*/
- .start = ohci_sa1111_start,
+ .start = ohci_start,
.stop = ohci_stop,

/*
--- 0011/drivers/usb/host/ohci-sm501.c
+++ work/drivers/usb/host/ohci-sm501.c 2008-01-13 19:32:03.000000000 +0900
@@ -21,23 +21,6 @@
#include <linux/sm501.h>
#include <linux/sm501-regs.h>

-static int ohci_sm501_start (struct usb_hcd *hcd)
-{
- struct device *dev = hcd->self.controller;
- struct ohci_hcd *ohci = hcd_to_ohci (hcd);
- int ret;
-
- if ((ret = ohci_init(ohci)) < 0)
- return ret;
-
- if ((ret = ohci_run (ohci)) < 0) {
- dev_err (dev, "can't start %s", hcd->self.bus_name);
- ohci_stop (hcd);
- return ret;
- }
- return 0;
-}
-
static int usb_hcd_sm501_probe (const struct hc_driver *driver,
struct platform_device *pdev)
{
@@ -180,7 +163,7 @@ static const struct hc_driver ohci_sm501
/*
* basic lifecycle operations
*/
- .start = ohci_sm501_start,
+ .start = ohci_start,
.stop = ohci_stop,
.shutdown = ohci_shutdown,


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

Post by Magnus Damm
usb: use single ohci_start()
This patch adds adds an ohci_start() function that easily can be shared
between many ohci drivers. This patch only changes drivers to ohci_start()
when the driver specific implementation exactly match ohci_start(). It also
seems popular to sometimes do ohci_init() in .reset instead of .start, and if
those drivers can be rewritten then it may be possible to use ohci_start()
in even more drivers.

Actually, the drivers that you made use ohci_start() are the problem ones...
you might get a hint of that because the PCI glue isn't structured that way,
yet that code gets bugfixed quicker than anything else.

Thing is, the reset() entry point is where all the one-time init *should*
happen, and that includes calling ohci_init(). Its name is a historical
goof; it doesn't just reset the hardware any more, that's a side effect.

And the start() entry point is what should be moving from the initialized
but inactive state into the operational state -- what ohci_run() does,
and maybe a bit more.

Whereas, ohci_start() idioms are calling ohci_init() *AND* ohci_run(), but
they should really only be doing the latter as start() entry points.

The situation got bad because of lots of copy'n'paste that was mostly
harmless on embedded hardware that often didn't suffer from the stranger
nuances of HCD lifecycle. And because some ancient 2.2/2.4 init sequence
code didn't get overhauled for a very long time. (In fact it's still not
exactly done...)

So, NAK on this one, sorry. The idea of code reuse is great, but this
is the wrong code to re-use.

- Dave

Post by Magnus Damm
 drivers/usb/host/ohci-au1xxx.c  |   24 +-----------------------
 drivers/usb/host/ohci-ep93xx.c  |   19 +------------------
 drivers/usb/host/ohci-hcd.c     |   18 ++++++++++++++++++
 drivers/usb/host/ohci-lh7a404.c |   22 +---------------------
 drivers/usb/host/ohci-pnx4008.c |   18 +-----------------
 drivers/usb/host/ohci-pnx8550.c |   24 +-----------------------
 drivers/usb/host/ohci-ppc-of.c  |   21 +--------------------
 drivers/usb/host/ohci-ppc-soc.c |   20 +-------------------
 drivers/usb/host/ohci-s3c2410.c |   21 +--------------------
 drivers/usb/host/ohci-sa1111.c  |   21 +--------------------
 drivers/usb/host/ohci-sm501.c   |   19 +------------------

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Magnus Damm]

Post by David Brownell

Post by Magnus Damm
usb: use single ohci_start()
This patch adds adds an ohci_start() function that easily can be shared
between many ohci drivers. This patch only changes drivers to ohci_start()
when the driver specific implementation exactly match ohci_start(). It also
seems popular to sometimes do ohci_init() in .reset instead of .start, and if
those drivers can be rewritten then it may be possible to use ohci_start()
in even more drivers.

Actually, the drivers that you made use ohci_start() are the problem ones...
you might get a hint of that because the PCI glue isn't structured that way,
yet that code gets bugfixed quicker than anything else.
Thing is, the reset() entry point is where all the one-time init *should*
happen, and that includes calling ohci_init(). Its name is a historical
goof; it doesn't just reset the hardware any more, that's a side effect.
And the start() entry point is what should be moving from the initialized
but inactive state into the operational state -- what ohci_run() does,
and maybe a bit more.
Whereas, ohci_start() idioms are calling ohci_init() *AND* ohci_run(), but
they should really only be doing the latter as start() entry points.
The situation got bad because of lots of copy'n'paste that was mostly
harmless on embedded hardware that often didn't suffer from the stranger
nuances of HCD lifecycle. And because some ancient 2.2/2.4 init sequence
code didn't get overhauled for a very long time. (In fact it's still not
exactly done...)
So, NAK on this one, sorry. The idea of code reuse is great, but this
is the wrong code to re-use.

No worries, I understand your point. So what about the following:

Move ohci_hcd_init() into ohci_init():
ohci_hcd_init() seems to be called at probe time today - with the
exception of ohci-pci.c, ohci-ps3.c and ohci-ssb.c. (They are the ones
that do the right thing, right?) It looks like calling ohci_hcd_init()
from ohci_init() should be ok.

Connect ohci_init() to .reset:
Just change the argument to struct usb_hcd * and connect it to the
driver callbacks.

Connect ohci_start() to .start:
Rename ohci_run() to ohci_start() and change the argument. Connect it
to the driver callbacks.

I'd be happy to hack up the above code if you think it's a good idea...

Thanks for the feedback!

/ magnus

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel