Hi,
On Fri, Aug 26, 2011 at 10:59:48AM -0400, Alan Stern wrote:
> On Fri, 26 Aug 2011, Felipe Balbi wrote:
>
> > > to see how it looks when somebody handles a bunch of platforms in a
> > > single source file. For example, is it possible to merge ehci-fsl.c,
> > > ehci-xilinx-of.c, ehci-sh.c, and ehci-omap.c into a single file without
> > > getting something that looks like Frankenstein's monster?
> >
> > I find it very unlikely, but what's the point ? That's really not what
> > we want.
>
> I thought that was exactly what you wanted. Your scheme is to register
> a platform device and make the ?hci-hcd module that device's driver,
> right? Which means a single source file will have to contain the bus
> glue for fsl, xilinx, sh, omap, and all the other EHCI platform
> implementations.
of course not. You completely misunderstood. Have you not looked into
dwc3 driver as I suggested ? Just spend some time reading dwc3-pci.c,
dwc3-omap.c and core/gadget.c. It's just pointless for me to keep on
trying to make you understand what I mean when, clearly I can't - maybe
my english is failing me -, and there is a real working implementation
of my "scheme".
> Unless you register multiple platform drivers within the same module.
> Was that your plan? I thought you intended to have just a single
no. Of course not ;-)
> platform driver that would work with everything. If not -- if you're
I want the common part (the EHCI compliant driver) to have its own
platform_device/platform_driver pair. Then platform-specific part only
has to platform_device_alloc("ehci", -1); platform_device_add(ehci);
hc_driver moves to ehci-hcd.c, all those symbols are made static, PM
is implemented so that ehci-hcd.c will only do whatever is EHCI specific
(cleaning up the ring, putting bus to suspend, blablabla) and
platform-specific part will handle platform-specific things (disable
clocks, enable wakeup sources, etc etc).
> going to have multiple drivers in the same module -- then what's the
> reason for adding a "glue" platform device below a PCI controller?
You really need to read those files I suggested you did.
> > What's common should definitely go to a common file - like when
> > I moved the hc_driver from xhci-pci.c to xhci.c -, on the other hand, if
> > it's platform-specific code, why do we want to combine them ?
> >
> > When I talk about common code, I mean the duplicated hc_driver on
> > ehci-<arch>.c files. That could all go into a common source
> > (ehci-hcd.c?) and the arch files become smaller and easier to maintain
> > (less code == less trouble).
>
> But the hc_driver structure _does_ contain some platform-specific
> fields: .product_desc and .reset for sure (although maybe the reset
that can be passed in as part of driver_data field or via
devices's platform_data.
> routines can be factored better), and several other fields in a few of
> the drivers.
platform_data ? Let's take ->reset as example:
ehci.h:
struct ehci_platform_data {
int (*reset)(struct device *child);
}
ehci.c:
static int ehci_reset(struct usb_hcd *hcd)
{
struct ehci *ehci = hcd_to_ehci(hcd);
if (ehci->ops->reset)
return ehci->ops->reset(ehci->dev);
return ehci_default_reset(ehci);
}
> > > The end result is that if you plug in a standard mass-storage device,
> > > the main usb-storage module gets loaded and acts as the driver. If you
> > > plug in one of the non-standard devices then the appropriate submodule
> > > acts as a driver, and the main module is also loaded in order to
> > > satisfy the export dependencies.
> >
> > Generally I like to avoid exported symbols where possible. That's why
> > using the device hierarchy from driver core came to mind.
>
> The purpose of the device hierarchy isn't to allow programmers to avoid
> exporting symbols. Its purpose is to provide a software model for the
Why not ? If we can split things up by telling the kernel exactly what
the HW is composed of, what's the problem with it ? In fact, avoiding
exported symbols will just make things better, easier to track and more
"localized" (meaning that ehci-omap will only know about all things
OMAP, ehci-pci will only know about all things PCI and ehci-hcd will
only know about all things EHCI).
> actual hardware.
And I have been telling you that on "actual HW" you will see an EHCI IP
with a PCI bridge, or an EHCI IP with an OMAP-integration
context/bridge/wrapper.
> > > xhci-hcd could be made to work similarly, except that there would be no
> > > "standard" driver -- there would just be a submodule driver for PCI
> > > plus one or more for various platform devices. The main module would
> > > contain only the xHCI core routines.
> > >
> > > I realize this involves an extra layer of software compared to what you
> > > want, but it might end up being cleaner -- especially if other vendors
> > > design their own USB-3 IP blocks that are not sufficiently compatible
> > > with your DesignWare code. We could handle them simply by adding
> > > another IP-specific platform-driver submodule, whereas your scheme
> > > really couldn't handle them without great difficulty.
> >
> > I didn't get this. DesignWare is one IP. Any licensee from Synopsys will
> > get that IP, configure the way they want wrt FIFO sizes, number of
> > endpoints, number of interrupters, number of RAMs, etc etc etc. But the
> > IP itself (and the programming model) remains the same.
>
> What will happen when some other company, not Synopsys, creates their
> own XHCI IP?
DWC3 != XHCI
XHCI == Host side
DWC3 == DRD Device
DWC3's Host side is compliant to XHCI. Why are you confusing things
here? If other company creates their own XHCI IP, they should be able to
re-use XHCI stack on Linux without any issues. I'm trying to make that
simpler for a wider audience. As of today, USB Host stack on linux is
very PCI-centric.
> > So if SoC Vendor XYZ decides to license the IP from Synopsys, all they
> > have to do is provide the parent device and allocate the core
> > platform_device. If they need something a bit more complex, they can
> > pass in platform_data or use the driver_data depending on the case.
>
> And what if vendor XYZ decides to license the non-Synopsys IP?
Then they'll need their own driver. Or, if it's a XHCI IP, they can
re-use the XHCI stack. I really don't understand what you mean here.
> > Now, with regards to xhci, this will become very similar. No matter
> > where xhci is being used, other than Silicon bugs/quirks and compliance
> > to XHCI 0.96 or 1.0, the programming model is the same. Meaning that
> > struct hc_driver for xhci will be the same thing throughout. We can
> > already see what I'm talking about with ehci-<arch>.c files. Take a look
> > at their struct hc_driver structure and you'll see that they are just
> > using ehci_* functions exported by ehci-core code.
>
> They are not. At least, not all of them. Look at ehci-tegra.c for
> example.
for map/unmap_urb, that can be passed via e.g. platform_data. For
->reset(), that's basically some clock handling with a call to the
default ehci_reset code. Can be better factored by adding the
platform_device to ehci-hcd.c
> > What I'm trying to say is that no matter where [ouex]hci is being used,
> > the standard is the same, correct ? So ehci_urb_enqueue, ehci_irq,
> > ehci_urb_dequeue and all their friends really don't need to be exported.
> > If [ouex]hci have their own platform_device/platform_driver, those could
> > be made static and [ouex]hci-<arch>.c can, at least, remove the
> > hc_driver definition, since that's just a big pile of duplicated code.
> >
> > Now, I apologize if I didn't get your point and I ask you to describe
> > further what you meant ;-)
>
> Maybe you weren't aware of all the little special-purpose changes lying
> hidden in the various <arch>.c files. They could become a substantial
> hindrance to your plan.
Show me that, I still didn't see. We can easily use a platform_data to
pass the differences as function pointers, but they still don't need to
be exported. Also, if an architecture does not need a special purpose
e.g. ->reset callback, it should not be required to pass it, we just
need to be careful with null pointers.
if (!ehci->ops->reset)
ehci_default_reset();
ehci->ops->reset();
--
balbi
Attachment:
signature.asc
Description: Digital signature
