Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

On Wednesday 24 October 2012 10:16:31 Stephen Warren wrote:

On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:

quoted

On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:

quoted

Under the circumstances, do we really need a new binding document for 
the ehci-platform driver?

It seems reasonable to add the new properties to usb-ehci.txt, since
they do describe the HW.

quoted

quoted

We should be able to use the existing 
usb-ehci binding, perhaps with some new properties added:

	has-synopsys-hc-bug
	no-io-watchdog
	has-tt

That sounds fine to me.

However, there is an implementation issue here. I believe the way Linux
searches for a driver for a particular node is:

for every driver that's registered
    if the driver's supported compatible list matches the device
        use the driver

(See drivers/base/platform.c:platform_match() which implements the if
statement above, and I assume the driver core implements the outer for
loop above)

That means that if the generic driver supports compatible="usb-ehci", it
may "steal" device nodes that have
compatible="something-custom","usb-ehci", even if there's a driver
specifically for "something-custom". We would need to re-arrange the
driver matching code to:

for each compatible value in the node:
    for each driver that's registered:
        if the driver supports the compatible value:
            use the driver.

quoted

On the PCI side we have VID, PID which is used for quirks. Sometimes we have a
revision ID which can be used to figure out if "this quirk" is still required.
The PCI driver probes by class so the ehci driver does not have a large table
of VID/PID for each controller out there. And the USB controller in two
different Intel boards has a different PID so a quirk, if required, could be
applied only to the specific mainboard.

Based on this we need atleast two compatible entries one "HW-Specific"
followed by a generic one (similar to PCI class).
Doing it the PCI way we would have to have table and figure out which
HW-specific compatible entry sets the TT flag and which one does the
no-io-watchdog. Having has-tt in compatible isn't right.

Yes, the driver could easily bind to anything compatible with
"usb-ehci", then use the HW-specific compatible value to index into a
quirk table in the same way that specific PCI IDs index into a quirk table.

Sounds good.

I agree that having separate compatible values like usb-ehci and
usb-ehci-with-tt probably doesn't make sense here.

quoted

I'm all with Alan here, to make a shortcut and allow Linux specific properties
which describe a specific quirk in less code lines. Other OS can just ignore
those and build their table based on the compatible entry if they want to.

We should absolutely avoid Linux-specific properties where possible.

That said, what Linux-specific properties are you talking about? The
properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
are all purely a description of HW, aren't they.

has-tt and has-synopsys-hc-bug are certainly hardware properties, while
no-io-watchdog is a Linux driver software workaround for a hardware issue,
so that's kind of in a grey zone to decide whether this describes hardware or
not. Let's just assume that this is a hardware issue :)

quoted

So usb-ehci should be fine. It is a generic USB-EHCI controller after all.
Quirks or no quirks, the register layout is the same, the functionality is the
same. If you can't map memory >4GiB then you need a quirk for this but you may
discover it way too late.
If your platform driver requires extra code for the PHY then it is still an
EHCI controller. The PHY wasn't setup by the bootloader / bios so Linux has to
deal with it.

quoted

We probably can omit has-synopsys-hc-bug, as that is specific to one
type of MIPS ATH79 controller.  The driver can check for it explicitly,
if necessary.

In fact, it's not clear that these new properties need to be added now.  
They can be added over time as needed, as existing drivers are
converted over to DT.  Or is it preferable to document these things
now, preemptively as it were?

It's best to define the binding up-front so it doesn't churn, where
possible. This will also ensure that multiple people don't try to update
the binding document to add the same feature in different ways.

Agreed, we do support these properties in the non-DT case, so I see no reason
why we should not document them in the binding too.

quoted

I would add it only if required / has users.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html