| Message ID | 1350771032-11527-3-git-send-email-linux@prisktech.co.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Le dimanche 21 octobre 2012 00:10:32, Tony Prisk a écrit : > Add a binding document for ehci-platform driver. > > Signed-off-by: Tony Prisk <linux@prisktech.co.nz> > --- > .../devicetree/bindings/usb/ehci-platform.txt | 27 > ++++++++++++++++++++ 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt > > diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt > b/Documentation/devicetree/bindings/usb/ehci-platform.txt new file mode > 100644 > index 0000000..930b19e > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt > @@ -0,0 +1,27 @@ > +Generic Platform EHCI Controller > +----------------------------------------------------- > + > +Required properties: > +- compatible : "linux,ehci-platform" Here you changed to linux,ehci-platform, which is good, but in the example you forgot to update it. Otherwise, this is ok with me. > +- reg : Should contain 1 register ranges(address and length) > +- interrupts : EHCI controller interrupt > + > +Optional properties: > +- caps-offset : offset to the capabilities register (default = 0) > +- has-tt : controller has transaction translator(s). > +- has-synopsys-hc-bug : controller has the synopsys hc bug > +- no-io-watchdog : controller does not need io watchdog > + > +- big-endian : descriptors and registers are both big endian. This > + is the equivalent of specifying big-endian-desc and big-endian-regs. > +OR > +- big-endian-desc : descriptors are in big-endian format > +- big-endian-regs : mmio is in big-endian format > + > +Example: > + ehci@d8007c00 { > + compatible = "ehci-platform"; > + reg = <0xd8007c00 0x200>; > + interrupts = <43>; > + has-tt; > + };
On 10/20/2012 04:10 PM, Tony Prisk wrote: > Add a binding document for ehci-platform driver. > > Signed-off-by: Tony Prisk <linux@prisktech.co.nz> > --- > .../devicetree/bindings/usb/ehci-platform.txt | 27 ++++++++++++++++++++ > 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt > > diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt b/Documentation/devicetree/bindings/usb/ehci-platform.txt > new file mode 100644 > index 0000000..930b19e > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt > @@ -0,0 +1,27 @@ > +Generic Platform EHCI Controller > +----------------------------------------------------- > + > +Required properties: > +- compatible : "linux,ehci-platform" That compatible value doesn't look right. The HW isn't defined by Linux. The binding is supposed to represent HW, not any single OS's use of that HW or the way its driver works. Something like "usb,ehci" might be more appropriate. Certainly, the value should not be "linux,", nor derived from Linux's driver name. > +Optional properties: > +- caps-offset : offset to the capabilities register (default = 0) > +- has-tt : controller has transaction translator(s). > +- has-synopsys-hc-bug : controller has the synopsys hc bug That would normally be determined by the driver based on the particular compatible value that is in device tree. > +- no-io-watchdog : controller does not need io watchdog > + > +- big-endian : descriptors and registers are both big endian. This > + is the equivalent of specifying big-endian-desc and big-endian-regs. > +OR > +- big-endian-desc : descriptors are in big-endian format > +- big-endian-regs : mmio is in big-endian format Hmmm. That looks odd. Presumably if those properties aren't specified, the default is little-endian? Shouldn't this be a tri-state: big, little, native, with default native? I don't know what the EHCI specification mandates here (and if it does mandate something, the default should match the specification). Isn't this something that readl/writel would take care of, or are there cases where the register endianness of just this one HW block mismatches all other HW blocks? > +Example: > + ehci@d8007c00 { > + compatible = "ehci-platform"; > + reg = <0xd8007c00 0x200>; > + interrupts = <43>; > + has-tt; > + }; >
On Mon, 22 Oct 2012, Stephen Warren wrote: > On 10/20/2012 04:10 PM, Tony Prisk wrote: > > Add a binding document for ehci-platform driver. > > +Optional properties: > > +- caps-offset : offset to the capabilities register (default = 0) > > +- has-tt : controller has transaction translator(s). > > +- has-synopsys-hc-bug : controller has the synopsys hc bug > > That would normally be determined by the driver based on the particular > compatible value that is in device tree. I don't understand this comment. Isn't "has-synopsys-hc-bug" the compatible value in question? > > +- big-endian : descriptors and registers are both big endian. This > > + is the equivalent of specifying big-endian-desc and big-endian-regs. > > +OR > > +- big-endian-desc : descriptors are in big-endian format > > +- big-endian-regs : mmio is in big-endian format > > Hmmm. That looks odd. Presumably if those properties aren't specified, > the default is little-endian? Shouldn't this be a tri-state: big, > little, native, with default native? I don't know what the EHCI > specification mandates here (and if it does mandate something, the > default should match the specification). Isn't this something that > readl/writel would take care of, or are there cases where the register > endianness of just this one HW block mismatches all other HW blocks? The EHCI spec assumes a PCI implementation; it doesn't consider other sorts. And it doesn't say anything about the endianness of multi-byte descriptors in memory. Yes, there are cases where one HW block has an endianness that doesn't match other HW blocks. Or to be more accurate, it doesn't match what the other HW blocks expect. For example, on ARM readl and writel expect to do byte-swapping but some particular EHCI blocks don't need it. Alan Stern
On 10/22/2012 11:34 AM, Alan Stern wrote: > On Mon, 22 Oct 2012, Stephen Warren wrote: > >> On 10/20/2012 04:10 PM, Tony Prisk wrote: >>> Add a binding document for ehci-platform driver. > >>> +Optional properties: >>> +- caps-offset : offset to the capabilities register (default = 0) >>> +- has-tt : controller has transaction translator(s). >>> +- has-synopsys-hc-bug : controller has the synopsys hc bug >> >> That would normally be determined by the driver based on the particular >> compatible value that is in device tree. > > I don't understand this comment. Isn't "has-synopsys-hc-bug" the > compatible value in question? "compatible value" in this context means that value of the property named "compatible". >>> +- big-endian : descriptors and registers are both big endian. This >>> + is the equivalent of specifying big-endian-desc and big-endian-regs. >>> +OR >>> +- big-endian-desc : descriptors are in big-endian format >>> +- big-endian-regs : mmio is in big-endian format >> >> Hmmm. That looks odd. Presumably if those properties aren't specified, >> the default is little-endian? Shouldn't this be a tri-state: big, >> little, native, with default native? I don't know what the EHCI >> specification mandates here (and if it does mandate something, the >> default should match the specification). Isn't this something that >> readl/writel would take care of, or are there cases where the register >> endianness of just this one HW block mismatches all other HW blocks? > > The EHCI spec assumes a PCI implementation; it doesn't consider other > sorts. And it doesn't say anything about the endianness of multi-byte > descriptors in memory. OK, so does this binding default to assuming little-endian (which I assume matches PCI), unless the big-endian properties are given? Is the case of little-endian EHCI registers on a big-endian CPU a common enough thing that adding a third state native-endian wouldn't be useful?
On Mon, 22 Oct 2012, Stephen Warren wrote: > >>> +- has-tt : controller has transaction translator(s). > >>> +- has-synopsys-hc-bug : controller has the synopsys hc bug > >> > >> That would normally be determined by the driver based on the particular > >> compatible value that is in device tree. > > > > I don't understand this comment. Isn't "has-synopsys-hc-bug" the > > compatible value in question? > > "compatible value" in this context means that value of the property > named "compatible". I see. But why would it be done this way instead having a separate property? And doesn't the same reasoning apply to has-tt? Doesn't that mean the driver would have to match four different hardware types? What happens if a third characteristic like these comes around; would the driver then have to check against eight different types? > >>> +- big-endian : descriptors and registers are both big endian. This > >>> + is the equivalent of specifying big-endian-desc and big-endian-regs. > >>> +OR > >>> +- big-endian-desc : descriptors are in big-endian format > >>> +- big-endian-regs : mmio is in big-endian format > >> > >> Hmmm. That looks odd. Presumably if those properties aren't specified, > >> the default is little-endian? Shouldn't this be a tri-state: big, > >> little, native, with default native? I don't know what the EHCI > >> specification mandates here (and if it does mandate something, the > >> default should match the specification). Isn't this something that > >> readl/writel would take care of, or are there cases where the register > >> endianness of just this one HW block mismatches all other HW blocks? > > > > The EHCI spec assumes a PCI implementation; it doesn't consider other > > sorts. And it doesn't say anything about the endianness of multi-byte > > descriptors in memory. > > OK, so does this binding default to assuming little-endian (which I > assume matches PCI), unless the big-endian properties are given? Yes, that is the intention. The ehci-hcd driver works the same way; it assumes little-endian unless USB_EHCI_BIG_ENDIAN_DESC or USB_EHCI_BIG_ENDIAN_MMIO is set. (That will have to change in the future, though.) > Is the > case of little-endian EHCI registers on a big-endian CPU a common enough > thing that adding a third state native-endian wouldn't be useful? I'm not sure how to answer. Little-endian EHCI registers are very common, even among big-endian CPUs (they are probably the majority, in fact). I don't see how adding native-endian would help; the readl() function always assumes the values it reads are little-endian, so in that sense little-endian _is_ the native state. Also, as far as I know there aren't any examples of big-endian EHCI registers on systems with little-endian CPUs. Alan Stern
On 10/22/2012 01:00 PM, Alan Stern wrote: > On Mon, 22 Oct 2012, Stephen Warren wrote: > >>>>> +- has-tt : controller has transaction translator(s). >>>>> +- has-synopsys-hc-bug : controller has the synopsys hc bug >>>> >>>> That would normally be determined by the driver based on the particular >>>> compatible value that is in device tree. >>> >>> I don't understand this comment. Isn't "has-synopsys-hc-bug" the >>> compatible value in question? >> >> "compatible value" in this context means that value of the property >> named "compatible". > > I see. But why would it be done this way instead having a separate > property? Well, I did say normally:-) I can certainly see an argument for representing these differences using custom properties, rather than deriving the information from the compatible value. It's probably be OK to do so for something generic like this; it's just perhaps not always the default choice. Do note that even though this binding document dictates a particular value for the compatible property, every device tree should additionally add a separate value alongside it to indicate the specific HW model that's actually present, so that if some device-specific bug-fix or workaround needs to be applied, the model can be identified anyway. So, rather than: compatible = "usb-ehci"; You should always have e.g.: compatible = "nvidia,tegra20-ehci", "usb-ehci"; Given that, there is then always enough information in the device tree for the driver to be able to derive the other values from the compatible value. Whether you want to derive the information, or whether you want to explicitly represent it via properties, is a decision to make based on the trade-offs. Oh, and I note that quite a few device trees already use compatible value "usb-ehci" in their device-trees. Care needs to be taken not to usurp that value from any existing device drivers if that was to be picked as the compatible value required by this binding. > And doesn't the same reasoning apply to has-tt? Doesn't that mean the > driver would have to match four different hardware types? What happens > if a third characteristic like these comes around; would the driver > then have to check against eight different types? No, the compatible value represents the model, so you'd have a table like: compatible -> bugX has_tt nvidia,tegra20-ehci -> 0 1 vendor1,foo-ehci -> 0 1 vendor2,bar-ehci -> 1 1 vendor3,baz-ehci -> 0 1 vendor4,qux-ehci -> 0 1 ... So the table size isn't related to the number of options. The table size is probably bigger than subset of options combinations that make sense.
On Mon, 22 Oct 2012, Stephen Warren wrote: > > I see. But why would it be done this way instead having a separate > > property? > > Well, I did say normally:-) > > I can certainly see an argument for representing these differences using > custom properties, rather than deriving the information from the > compatible value. It's probably be OK to do so for something generic > like this; it's just perhaps not always the default choice. > > Do note that even though this binding document dictates a particular > value for the compatible property, every device tree should additionally > add a separate value alongside it to indicate the specific HW model > that's actually present, so that if some device-specific bug-fix or > workaround needs to be applied, the model can be identified anyway. > > So, rather than: > > compatible = "usb-ehci"; > > You should always have e.g.: > > compatible = "nvidia,tegra20-ehci", "usb-ehci"; > > Given that, there is then always enough information in the device tree > for the driver to be able to derive the other values from the compatible > value. Yes, I get it. > Whether you want to derive the information, or whether you want to > explicitly represent it via properties, is a decision to make based on > the trade-offs. > > Oh, and I note that quite a few device trees already use compatible > value "usb-ehci" in their device-trees. Care needs to be taken not to > usurp that value from any existing device drivers if that was to be > picked as the compatible value required by this binding. Right. I think Tony's new binding should use compatible value "usb-ehci-platform". It will essentially be a superset of "usb-ehci". > > And doesn't the same reasoning apply to has-tt? Doesn't that mean the > > driver would have to match four different hardware types? What happens > > if a third characteristic like these comes around; would the driver > > then have to check against eight different types? > > No, the compatible value represents the model, so you'd have a table like: > > compatible -> bugX has_tt > nvidia,tegra20-ehci -> 0 1 > vendor1,foo-ehci -> 0 1 > vendor2,bar-ehci -> 1 1 > vendor3,baz-ehci -> 0 1 > vendor4,qux-ehci -> 0 1 > ... > > So the table size isn't related to the number of options. The table size > is probably bigger than subset of options combinations that make sense. Under the circumstances, and considering that usb-ehci-platform will be rather generic, IMO it will be better to make has-tt, has-synopsys-hc-bug, and no-io-watchdog separate properties, each naturally defaulting to "not present". That will avoid the need to update the driver's table each time a new board comes along. Alan Stern
On 10/23/2012 08:10 AM, Alan Stern wrote: > On Mon, 22 Oct 2012, Stephen Warren wrote: > >>> I see. But why would it be done this way instead having a separate >>> property? >> >> Well, I did say normally:-) >> >> I can certainly see an argument for representing these differences using >> custom properties, rather than deriving the information from the >> compatible value. It's probably be OK to do so for something generic >> like this; it's just perhaps not always the default choice. >> >> Do note that even though this binding document dictates a particular >> value for the compatible property, every device tree should additionally >> add a separate value alongside it to indicate the specific HW model >> that's actually present, so that if some device-specific bug-fix or >> workaround needs to be applied, the model can be identified anyway. >> >> So, rather than: >> >> compatible = "usb-ehci"; >> >> You should always have e.g.: >> >> compatible = "nvidia,tegra20-ehci", "usb-ehci"; >> >> Given that, there is then always enough information in the device tree >> for the driver to be able to derive the other values from the compatible >> value. > > Yes, I get it. > >> Whether you want to derive the information, or whether you want to >> explicitly represent it via properties, is a decision to make based on >> the trade-offs. >> >> Oh, and I note that quite a few device trees already use compatible >> value "usb-ehci" in their device-trees. Care needs to be taken not to >> usurp that value from any existing device drivers if that was to be >> picked as the compatible value required by this binding. > > Right. I think Tony's new binding should use compatible value > "usb-ehci-platform". It will essentially be a superset of "usb-ehci". I know this is bike-shedding a little bit, but... The word "platform" isn't really about describing the HW, but rather is derived from the Linux SW used to program that HW. DT should be purely about describing the HW. Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better?
On Tue, 23 Oct 2012, Stephen Warren wrote: > >> So, rather than: > >> > >> compatible = "usb-ehci"; > >> > >> You should always have e.g.: > >> > >> compatible = "nvidia,tegra20-ehci", "usb-ehci"; > >> > >> Given that, there is then always enough information in the device tree > >> for the driver to be able to derive the other values from the compatible > >> value. > > > > Yes, I get it. > > > >> Whether you want to derive the information, or whether you want to > >> explicitly represent it via properties, is a decision to make based on > >> the trade-offs. > >> > >> Oh, and I note that quite a few device trees already use compatible > >> value "usb-ehci" in their device-trees. Care needs to be taken not to > >> usurp that value from any existing device drivers if that was to be > >> picked as the compatible value required by this binding. > > > > Right. I think Tony's new binding should use compatible value > > "usb-ehci-platform". It will essentially be a superset of "usb-ehci". > > I know this is bike-shedding a little bit, but... > > The word "platform" isn't really about describing the HW, but rather is > derived from the Linux SW used to program that HW. DT should be purely > about describing the HW. > > Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better? Neither of those really describes the hardware either. Which name gets used seems pretty arbitrary. Nothing intrinsically distinguishes this class of hardware. The only thing these devices have in common is that they can be managed by Linux's ehci-platform driver. For that purpose, "usb-ehci-platform" makes more sense than "usb-ehci-generic" or "usb-ehci-simple". (It also allows the class to enlarge over time as the driver becomes more capable -- is that a bad thing? Are DT board descriptions written for a later kernel supposed to be unusable with an earlier kernel that doesn't support the same features?) In essense, we're trying to say that the HW is compatible with a particular driver rather than with some other type of HW. Maybe DT wasn't intended to provide this kind of information, but it seems like a logical thing to do. Unless DT already offers some other way to accomplish the same thing? Alan Stern
On 10/23/2012 11:59 AM, Alan Stern wrote: > On Tue, 23 Oct 2012, Stephen Warren wrote: > >>>> So, rather than: >>>> >>>> compatible = "usb-ehci"; >>>> >>>> You should always have e.g.: >>>> >>>> compatible = "nvidia,tegra20-ehci", "usb-ehci"; >>>> >>>> Given that, there is then always enough information in the device tree >>>> for the driver to be able to derive the other values from the compatible >>>> value. >>> >>> Yes, I get it. >>> >>>> Whether you want to derive the information, or whether you want to >>>> explicitly represent it via properties, is a decision to make based on >>>> the trade-offs. >>>> >>>> Oh, and I note that quite a few device trees already use compatible >>>> value "usb-ehci" in their device-trees. Care needs to be taken not to >>>> usurp that value from any existing device drivers if that was to be >>>> picked as the compatible value required by this binding. >>> >>> Right. I think Tony's new binding should use compatible value >>> "usb-ehci-platform". It will essentially be a superset of "usb-ehci". >> >> I know this is bike-shedding a little bit, but... >> >> The word "platform" isn't really about describing the HW, but rather is >> derived from the Linux SW used to program that HW. DT should be purely >> about describing the HW. >> >> Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better? > > Neither of those really describes the hardware either. Which name gets > used seems pretty arbitrary. > > Nothing intrinsically distinguishes this class of hardware. The only > thing these devices have in common is that they can be managed by > Linux's ehci-platform driver. I don't agree. They're all EHCI USB controllers (or all EHCI USB controllers that don't require custom drivers due to quirks), which is a much more general statement than anything related to which driver Linux uses for them. > For that purpose, "usb-ehci-platform" > makes more sense than "usb-ehci-generic" or "usb-ehci-simple". > > (It also allows the class to enlarge over time as the driver becomes > more capable -- is that a bad thing? Are DT board descriptions written > for a later kernel supposed to be unusable with an earlier kernel that > doesn't support the same features?) > > In essense, we're trying to say that the HW is compatible with a > particular driver rather than with some other type of HW. Using a compatible value in order to pick a specific driver in Linux is explicitly against the device tree design principles; DT is supposed to represent just the hardware. > Maybe DT > wasn't intended to provide this kind of information, but it seems like > a logical thing to do. Unless DT already offers some other way to > accomplish the same thing? The typical way is for the Linux driver to declare that is supports a variety of different HW models. Admittedly this is annoying when there are many HW models that otherwise wouldn't need any driver changes, hence the desire to (additionally) include some generic value in the compatible field, but again, what that generic value is should be driven by the HW itself, not the SW that's using it.
On Tue, 23 Oct 2012, Stephen Warren wrote: > > Nothing intrinsically distinguishes this class of hardware. The only > > thing these devices have in common is that they can be managed by > > Linux's ehci-platform driver. > > I don't agree. They're all EHCI USB controllers (or all EHCI USB > controllers that don't require custom drivers due to quirks), which is a > much more general statement than anything related to which driver Linux > uses for them. That's true, but it doesn't distinguish these devices. There are other EHCI controllers with no quirks that nevertheless can't be handled by ehci-platform because they have requirements (_not_ quirks) that ehci-platform is unable to deal with currently -- clocks or power supplies or special register settings or PHYs or etc. > > In essense, we're trying to say that the HW is compatible with a > > particular driver rather than with some other type of HW. > > Using a compatible value in order to pick a specific driver in Linux is > explicitly against the device tree design principles; DT is supposed to > represent just the hardware. > > > Maybe DT > > wasn't intended to provide this kind of information, but it seems like > > a logical thing to do. Unless DT already offers some other way to > > accomplish the same thing? > > The typical way is for the Linux driver to declare that is supports a > variety of different HW models. > > Admittedly this is annoying when there are many HW models that otherwise > wouldn't need any driver changes, hence the desire to (additionally) > include some generic value in the compatible field, but again, what that > generic value is should be driven by the HW itself, not the SW that's > using it. Okay, fine. But then what should the binding documentation specify for the compatible value? A list of all the HW models that the driver currently supports? Alan Stern
On 10/23/2012 02:33 PM, Alan Stern wrote: > On Tue, 23 Oct 2012, Stephen Warren wrote: > >>> Nothing intrinsically distinguishes this class of hardware. The only >>> thing these devices have in common is that they can be managed by >>> Linux's ehci-platform driver. >> >> I don't agree. They're all EHCI USB controllers (or all EHCI USB >> controllers that don't require custom drivers due to quirks), which is a >> much more general statement than anything related to which driver Linux >> uses for them. > > That's true, but it doesn't distinguish these devices. There are other > EHCI controllers with no quirks that nevertheless can't be handled by > ehci-platform because they have requirements (_not_ quirks) that > ehci-platform is unable to deal with currently -- clocks or power > supplies or special register settings or PHYs or etc. But what if the h/w has quirks you haven't yet discovered? You need the compatible property to be specific now, so if there are any future driver changes needed the DT does not have to change (as it may be in firmware which you can't change). >>> In essense, we're trying to say that the HW is compatible with a >>> particular driver rather than with some other type of HW. >> >> Using a compatible value in order to pick a specific driver in Linux is >> explicitly against the device tree design principles; DT is supposed to >> represent just the hardware. >> >>> Maybe DT >>> wasn't intended to provide this kind of information, but it seems like >>> a logical thing to do. Unless DT already offers some other way to >>> accomplish the same thing? >> >> The typical way is for the Linux driver to declare that is supports a >> variety of different HW models. >> >> Admittedly this is annoying when there are many HW models that otherwise >> wouldn't need any driver changes, hence the desire to (additionally) >> include some generic value in the compatible field, but again, what that >> generic value is should be driven by the HW itself, not the SW that's >> using it. > > Okay, fine. But then what should the binding documentation specify for > the compatible value? A list of all the HW models that the driver > currently supports? The binding docs should be independent of the driver. There may be fields the driver ignores. So you need to document all defined compatible strings. It is fine if the driver only has "usb-ehci", but it is not fine for the DT to only have that. Rob
On Tue, 23 Oct 2012, Rob Herring wrote: > On 10/23/2012 02:33 PM, Alan Stern wrote: > > On Tue, 23 Oct 2012, Stephen Warren wrote: > > > >>> Nothing intrinsically distinguishes this class of hardware. The only > >>> thing these devices have in common is that they can be managed by > >>> Linux's ehci-platform driver. > >> > >> I don't agree. They're all EHCI USB controllers (or all EHCI USB > >> controllers that don't require custom drivers due to quirks), which is a > >> much more general statement than anything related to which driver Linux > >> uses for them. > > > > That's true, but it doesn't distinguish these devices. There are other > > EHCI controllers with no quirks that nevertheless can't be handled by > > ehci-platform because they have requirements (_not_ quirks) that > > ehci-platform is unable to deal with currently -- clocks or power > > supplies or special register settings or PHYs or etc. > > But what if the h/w has quirks you haven't yet discovered? You need the > compatible property to be specific now, so if there are any future > driver changes needed the DT does not have to change (as it may be in > firmware which you can't change). That argument applies always, doesn't it? There's always a chance that you might discover a new quirk in a device for which the DT board file has already been written and committed to firmware. The board file could declare that the device is compatible with something older, but the new quirk might ruin that compatibility. > > Okay, fine. But then what should the binding documentation specify for > > the compatible value? A list of all the HW models that the driver > > currently supports? > > The binding docs should be independent of the driver. There may be > fields the driver ignores. So you need to document all defined > compatible strings. It is fine if the driver only has "usb-ehci", but it > is not fine for the DT to only have that. Under the circumstances, do we really need a new binding document for the ehci-platform driver? 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 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? The one that matters most and is most clearly a property of the HW is "has-tt". IMO it should be added right away, even though there may already be DT board files that could specify it but don't. Right now, for example, the ehci-xilinx-of driver checks instead for the "support-usb-fs" property, as defined in Documentation/devicetree/bindings/xilinx.txt. Alan Stern
On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote: > Under the circumstances, do we really need a new binding document for > the ehci-platform driver? 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 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. 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. 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. > 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? I would add it only if required / has users. > Alan Stern Sebastian
On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote: > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote: >> 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. >> 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. > 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. I agree that having separate compatible values like usb-ehci and usb-ehci-with-tt probably doesn't make sense here. > 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. > 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. > >> 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. > I would add it only if required / has users.
On 10/24/2012 08:57 AM, Alan Stern wrote: > On Tue, 23 Oct 2012, Rob Herring wrote: > >> On 10/23/2012 02:33 PM, Alan Stern wrote: >>> On Tue, 23 Oct 2012, Stephen Warren wrote: >>> >>>>> Nothing intrinsically distinguishes this class of hardware. The only >>>>> thing these devices have in common is that they can be managed by >>>>> Linux's ehci-platform driver. >>>> >>>> I don't agree. They're all EHCI USB controllers (or all EHCI USB >>>> controllers that don't require custom drivers due to quirks), which is a >>>> much more general statement than anything related to which driver Linux >>>> uses for them. >>> >>> That's true, but it doesn't distinguish these devices. There are other >>> EHCI controllers with no quirks that nevertheless can't be handled by >>> ehci-platform because they have requirements (_not_ quirks) that >>> ehci-platform is unable to deal with currently -- clocks or power >>> supplies or special register settings or PHYs or etc. >> >> But what if the h/w has quirks you haven't yet discovered? You need the >> compatible property to be specific now, so if there are any future >> driver changes needed the DT does not have to change (as it may be in >> firmware which you can't change). > > That argument applies always, doesn't it? There's always a chance that > you might discover a new quirk in a device for which the DT board file > has already been written and committed to firmware. The board file > could declare that the device is compatible with something older, but > the new quirk might ruin that compatibility. If the board file /only/ declares that the device is compatible with the older/most-generic thing, that would be a bug in the .dts file. As a general rule, the device tree should be fixed, although there may be reasons to work around some buggy device trees in the kernel we should avoid it if possible. Device tree files always need a completely specific value in the compatible property, even when less-specific/more-generic values are also present. So for example, the Linux driver might only care about the following existing: compatible = "usb-ehci". However, any actual EHCI controller is always some specific model, so the compatible value must define which specific model it is, e.g.: compatible = "nvidia,tegra20-ehci", "usb-ehci". Now lets say the Tegra30 EHCI controller is identical to the Tegra20 controller. That needs to be explicitly represented too: compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci". In that case, the driver might still only declare support for "nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit about the actual HW model. This doesn't continue forever though; you typically only list the specific HW model, the base specific model it's compatible with, and any generic value needed. So, a hypothetical Tegra40 EHCI controller would be: compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci". Given that, there is always something to key any newly required quirk off, so even if the need for a quirk is found later, the device tree already contains the information needed to trigger it. This is why quirks should always be keyed off compatible values, since they're guaranteed to be there in a correctly written device tree. Generic HW features (such as has-tt) don't need to be derived from the compatible value (although they could be), since they are something that's known up-front and hence should be present in any non-buggy device tree from day one. That's why the binding needs to thought out ahead of time, or if necessary (e.g. if expanded to support both EHCI and XHCI and XYZHCI which might require extra standard properties) evolved in a backwards-compatible fashion.
On Wednesday 24 October 2012 10:16:31 Stephen Warren wrote: > On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote: > > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote: > >> 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. > > >> 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. > > > 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. > > > 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 :) > > > 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. > > > >> 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. > > > I would add it only if required / has users.
On Wed, 24 Oct 2012, Stephen Warren wrote: > On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote: > > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote: > >> 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. > > >> 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) Yes, it does. > 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. Which might be difficult since the inner loop would be controlled by the outer code in the driver core. How do we determine which existing drivers claim to support usb-ehci? A quick search under arch/ and drivers/ turns up nothing but drivers/usb/host/ehci-ppc-of.c. Changing it to a more HW-specific match should be easy enough, and then "usb-ehci" would be safe to use in ehci-platform.c. Alan Stern
On Wednesday 24 October 2012 12:38:42 Alan Stern wrote: > On Wed, 24 Oct 2012, Stephen Warren wrote: > > > On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote: > > > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote: > > >> 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. > > > > >> 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) > > Yes, it does. > > > 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. > > Which might be difficult since the inner loop would be controlled by > the outer code in the driver core. > > How do we determine which existing drivers claim to support usb-ehci? > A quick search under arch/ and drivers/ turns up nothing but > drivers/usb/host/ehci-ppc-of.c. Changing it to a more HW-specific > match should be easy enough, and then "usb-ehci" would be safe to use > in ehci-platform.c. As long as no one enables both ehci-platform and ehci-ppc-of at the same time there is no problem. ehci-ppc-of should be removed in favor of ehci-platform and make sure that the specific quirk in ehci-ppc-of also gets ported, other that I see no issue using "usb-ehci" as the least detailed compatible property name. -- Florian
On Wed, 24 Oct 2012, Stephen Warren wrote: > 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" is definitely a description of the HW. "has-synopsys-hc-bug" is too, although determining whether or not it should apply to a particular controller might be difficult. I'm inclined not to include it among the properties. "no-io-watchdog" is not the greatest name. It describes to controllers that always do generate IRQs for I/O events when they are supposed to (and hence the driver doesn't need to set up a watchdog timer to detect I/O completions that didn't generate an IRQ). So while the concept is HW-specific, the name refers to a driver implementation issue. A better name might be something like "reliable-IRQs". Again, it's not such an easy thing to test for. Almost all the existing drivers leave it unset. Alan Stern
On 10/24/2012 10:38 AM, Alan Stern wrote: > On Wed, 24 Oct 2012, Stephen Warren wrote: > >> On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote: >>> On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote: >>>> 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. >> >>>> 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) > > Yes, it does. > >> 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. > > Which might be difficult since the inner loop would be controlled by > the outer code in the driver core. > > How do we determine which existing drivers claim to support usb-ehci? > A quick search under arch/ and drivers/ turns up nothing but > drivers/usb/host/ehci-ppc-of.c. Changing it to a more HW-specific > match should be easy enough, and then "usb-ehci" would be safe to use > in ehci-platform.c. It's not drivers that claim to support usb-ehci, but device tree files that contain nodes that include "usb-ehci" in their compatible value, yet need to be handled by a driver that isn't the generic USB EHCI driver, and that driver binds to the other more specific compatible value: > $ grep -HrnI usb-ehci arch/*/boot/dts > arch/arm/boot/dts/spear3xx.dtsi:76: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/at91sam9x5.dtsi:399: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; > arch/arm/boot/dts/spear13xx.dtsi:145: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/spear13xx.dtsi:152: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/tegra20.dtsip:215: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > arch/arm/boot/dts/tegra20.dtsip:224: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > arch/arm/boot/dts/tegra20.dtsip:232: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > arch/arm/boot/dts/spear600.dtsi:88: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/spear600.dtsi:96: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/at91sam9g45.dtsi:392: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; > arch/powerpc/boot/dts/sequoia.dts:156: compatible = "ibm,usb-ehci-440epx", "usb-ehci"; > arch/powerpc/boot/dts/wii.dts:120: compatible = "nintendo,hollywood-usb-ehci", > arch/powerpc/boot/dts/wii.dts:121: "usb-ehci"; > arch/powerpc/boot/dts/canyonlands.dts:167: compatible = "ibm,usb-ehci-460ex", "usb-ehci"; and then search for all those other compatible values in drivers. The ARM instances certainly all have this issue (although perhaps it's worth investigating if a generic could work for them instead). The PPC instances need more investigation; the values don't show up in of match tables but rather in code.
On 10/24/2012 10:44 AM, Alan Stern wrote: > On Wed, 24 Oct 2012, Stephen Warren wrote: > >> 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" is definitely a description of the HW. OK. > "has-synopsys-hc-bug" is too, although determining whether or not it > should apply to a particular controller might be difficult. I'm > inclined not to include it among the properties. > > "no-io-watchdog" is not the greatest name. It describes to controllers > that always do generate IRQs for I/O events when they are supposed to > (and hence the driver doesn't need to set up a watchdog timer to detect > I/O completions that didn't generate an IRQ). So while the concept is > HW-specific, the name refers to a driver implementation issue. A > better name might be something like "reliable-IRQs". Again, it's not > such an easy thing to test for. Almost all the existing drivers leave > it unset. OK, I'd be inclined to drive those last two by quirks then, since they aren't architectural features of EHCI but rather implementation issues. And indeed have the quirk table have a "reliable IRQs" field instead of "no IO watchdog", to minimize the table size.
On Wed, 24 Oct 2012, Stephen Warren wrote: > Device tree files always need a completely specific value in the > compatible property, even when less-specific/more-generic values are > also present. So for example, the Linux driver might only care about the > following existing: > > compatible = "usb-ehci". > > However, any actual EHCI controller is always some specific model, so > the compatible value must define which specific model it is, e.g.: > > compatible = "nvidia,tegra20-ehci", "usb-ehci". > > Now lets say the Tegra30 EHCI controller is identical to the Tegra20 > controller. That needs to be explicitly represented too: > > compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci". > > In that case, the driver might still only declare support for > "nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit > about the actual HW model. > > This doesn't continue forever though; you typically only list the > specific HW model, the base specific model it's compatible with, and any > generic value needed. So, a hypothetical Tegra40 EHCI controller would be: > > compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci". > > Given that, there is always something to key any newly required quirk > off, so even if the need for a quirk is found later, the device tree > already contains the information needed to trigger it. Okay. It appears that quite a few .dts/.dtsi files mention "usb-ehci", for no apparent reason (given that the drivers don't list it): [stern@iolanthe usb-3.6]$ find arch -name '*.dts*' | xargs grep usb-ehci arch/mips/cavium-octeon/octeon_3xxx.dts: compatible = "cavium,octeon-6335-ehci","usb-ehci"; arch/mips/cavium-octeon/octeon_68xx.dts: compatible = "cavium,octeon-6335-ehci","usb-ehci"; arch/arm/boot/dts/tegra20.dtsi: compatible = "nvidia,tegra20-ehci", "usb-ehci"; arch/arm/boot/dts/tegra20.dtsi: compatible = "nvidia,tegra20-ehci", "usb-ehci"; arch/arm/boot/dts/tegra20.dtsi: compatible = "nvidia,tegra20-ehci", "usb-ehci"; arch/arm/boot/dts/at91sam9x5.dtsi: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; arch/arm/boot/dts/spear13xx.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; arch/arm/boot/dts/spear13xx.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; arch/arm/boot/dts/spear3xx.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; arch/arm/boot/dts/spear600.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; arch/arm/boot/dts/spear600.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; arch/arm/boot/dts/at91sam9g45.dtsi: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; arch/powerpc/boot/dts/wii.dts: compatible = "nintendo,hollywood-usb-ehci", arch/powerpc/boot/dts/wii.dts: "usb-ehci"; arch/powerpc/boot/dts/sequoia.dts: compatible = "ibm,usb-ehci-440epx", "usb-ehci"; arch/powerpc/boot/dts/canyonlands.dts: compatible = "ibm,usb-ehci-460ex", "usb-ehci"; Is there a real reason that I'm not aware of? Or can all these entries safely be removed? Alan Stern
On Wednesday 24 October 2012 12:54:05 Alan Stern wrote: > On Wed, 24 Oct 2012, Stephen Warren wrote: > > > Device tree files always need a completely specific value in the > > compatible property, even when less-specific/more-generic values are > > also present. So for example, the Linux driver might only care about the > > following existing: > > > > compatible = "usb-ehci". > > > > However, any actual EHCI controller is always some specific model, so > > the compatible value must define which specific model it is, e.g.: > > > > compatible = "nvidia,tegra20-ehci", "usb-ehci". > > > > Now lets say the Tegra30 EHCI controller is identical to the Tegra20 > > controller. That needs to be explicitly represented too: > > > > compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci". > > > > In that case, the driver might still only declare support for > > "nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit > > about the actual HW model. > > > > This doesn't continue forever though; you typically only list the > > specific HW model, the base specific model it's compatible with, and any > > generic value needed. So, a hypothetical Tegra40 EHCI controller would be: > > > > compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci". > > > > Given that, there is always something to key any newly required quirk > > off, so even if the need for a quirk is found later, the device tree > > already contains the information needed to trigger it. > > Okay. It appears that quite a few .dts/.dtsi files mention "usb-ehci", > for no apparent reason (given that the drivers don't list it): > > [stern@iolanthe usb-3.6]$ find arch -name '*.dts*' | xargs grep usb-ehci > arch/mips/cavium-octeon/octeon_3xxx.dts: compatible = "cavium,octeon-6335-ehci","usb-ehci"; > arch/mips/cavium-octeon/octeon_68xx.dts: compatible = "cavium,octeon-6335-ehci","usb-ehci"; > arch/arm/boot/dts/tegra20.dtsi: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > arch/arm/boot/dts/tegra20.dtsi: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > arch/arm/boot/dts/tegra20.dtsi: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > arch/arm/boot/dts/at91sam9x5.dtsi: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; > arch/arm/boot/dts/spear13xx.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/spear13xx.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/spear3xx.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/spear600.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/spear600.dtsi: compatible = "st,spear600-ehci", "usb-ehci"; > arch/arm/boot/dts/at91sam9g45.dtsi: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; > arch/powerpc/boot/dts/wii.dts: compatible = "nintendo,hollywood-usb-ehci", > arch/powerpc/boot/dts/wii.dts: "usb-ehci"; > arch/powerpc/boot/dts/sequoia.dts: compatible = "ibm,usb-ehci-440epx", "usb-ehci"; > arch/powerpc/boot/dts/canyonlands.dts: compatible = "ibm,usb-ehci-460ex", "usb-ehci"; > > Is there a real reason that I'm not aware of? Or can all these entries > safely be removed? Apart from the powerpc entries for which a real driver exists, the others were probably added in the hope that sooner or later, someone will come up with a matching driver, and the corresponding dts file would not even have to be updated to benefit from this. -- Florian
On 10/24/2012 11:44 AM, Alan Stern wrote: > On Wed, 24 Oct 2012, Stephen Warren wrote: > >> 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" is definitely a description of the HW. Can you spell out tt. > "has-synopsys-hc-bug" is too, although determining whether or not it > should apply to a particular controller might be difficult. I'm > inclined not to include it among the properties. What happens when there are 2 synopsys hc bugs? Something more specific about what the bug is would be better. > "no-io-watchdog" is not the greatest name. It describes to controllers > that always do generate IRQs for I/O events when they are supposed to > (and hence the driver doesn't need to set up a watchdog timer to detect > I/O completions that didn't generate an IRQ). So while the concept is > HW-specific, the name refers to a driver implementation issue. A > better name might be something like "reliable-IRQs". Again, it's not > such an easy thing to test for. Almost all the existing drivers leave > it unset. Shouldn't the default be reliable irqs? What about "unreliable-irqs"? Rob
On Wed, 24 Oct 2012, Stephen Warren wrote: > > How do we determine which existing drivers claim to support usb-ehci? > > A quick search under arch/ and drivers/ turns up nothing but > > drivers/usb/host/ehci-ppc-of.c. Changing it to a more HW-specific > > match should be easy enough, and then "usb-ehci" would be safe to use > > in ehci-platform.c. > > It's not drivers that claim to support usb-ehci, but device tree files > that contain nodes that include "usb-ehci" in their compatible value, > yet need to be handled by a driver that isn't the generic USB EHCI > driver, and that driver binds to the other more specific compatible value: > > > $ grep -HrnI usb-ehci arch/*/boot/dts > > arch/arm/boot/dts/spear3xx.dtsi:76: compatible = "st,spear600-ehci", "usb-ehci"; > > arch/arm/boot/dts/at91sam9x5.dtsi:399: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; > > arch/arm/boot/dts/spear13xx.dtsi:145: compatible = "st,spear600-ehci", "usb-ehci"; > > arch/arm/boot/dts/spear13xx.dtsi:152: compatible = "st,spear600-ehci", "usb-ehci"; > > arch/arm/boot/dts/tegra20.dtsip:215: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > > arch/arm/boot/dts/tegra20.dtsip:224: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > > arch/arm/boot/dts/tegra20.dtsip:232: compatible = "nvidia,tegra20-ehci", "usb-ehci"; > > arch/arm/boot/dts/spear600.dtsi:88: compatible = "st,spear600-ehci", "usb-ehci"; > > arch/arm/boot/dts/spear600.dtsi:96: compatible = "st,spear600-ehci", "usb-ehci"; > > arch/arm/boot/dts/at91sam9g45.dtsi:392: compatible = "atmel,at91sam9g45-ehci", "usb-ehci"; > > arch/powerpc/boot/dts/sequoia.dts:156: compatible = "ibm,usb-ehci-440epx", "usb-ehci"; > > arch/powerpc/boot/dts/wii.dts:120: compatible = "nintendo,hollywood-usb-ehci", > > arch/powerpc/boot/dts/wii.dts:121: "usb-ehci"; > > arch/powerpc/boot/dts/canyonlands.dts:167: compatible = "ibm,usb-ehci-460ex", "usb-ehci"; > > and then search for all those other compatible values in drivers. The > ARM instances certainly all have this issue (although perhaps it's worth > investigating if a generic could work for them instead). The PPC > instances need more investigation; the values don't show up in of match > tables but rather in code. Ah, now I'm starting to get the picture. So by listing "usb-ehci" in its device ID table, a driver would essentially be saying that it can handle _all_ USB EHCI controllers. (Which means that the entry in ehci-ppc-of.c is questionable; perhaps the intention is that this driver really does handle all EHCI controllers on any PPC-based OpenFirmware platform bus.) > This doesn't continue forever though; you typically only list the > specific HW model, the base specific model it's compatible with, and any > generic value needed. So, a hypothetical Tegra40 EHCI controller would be: > > compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci". What's the reason for listing the generic value if drivers can't key off it? Does it contain any information that's not already present in the specific values? It sounds like the ehci-platform driver should simply list all the specific HW device types (or base HW device types) that it handles, and it shouldn't include "usb-ehci" in the list. As more DT board files are created or as ehci-platform becomes capable to taking over from other drivers, its device list would grow. And it sounds like the only property we really need to add to the usb-ehci binding at the moment is "has-tt". Alan Stern
On Wed, 24 Oct 2012, Rob Herring wrote: > On 10/24/2012 11:44 AM, Alan Stern wrote: > > On Wed, 24 Oct 2012, Stephen Warren wrote: > > > >> 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" is definitely a description of the HW. > > Can you spell out tt. It stands for Transaction Translator. The acronym is a standard one, used in the USB specs. > > "has-synopsys-hc-bug" is too, although determining whether or not it > > should apply to a particular controller might be difficult. I'm > > inclined not to include it among the properties. > > What happens when there are 2 synopsys hc bugs? Something more specific > about what the bug is would be better. We will leave it out. > > "no-io-watchdog" is not the greatest name. It describes to controllers > > that always do generate IRQs for I/O events when they are supposed to > > (and hence the driver doesn't need to set up a watchdog timer to detect > > I/O completions that didn't generate an IRQ). So while the concept is > > HW-specific, the name refers to a driver implementation issue. A > > better name might be something like "reliable-IRQs". Again, it's not > > such an easy thing to test for. Almost all the existing drivers leave > > it unset. > > Shouldn't the default be reliable irqs? What about "unreliable-irqs"? I don't know why the default was set the way it was. That was before my time as EHCI maintainer. Right now only a few EHCI drivers claim to have reliable IRQs. Avoiding the watchdog timer is a fairly minor optimization in any case. It fires only once every 100 ms, and only while I/O is in progress. Alan Stern
On Wed, 24 Oct 2012, Florian Fainelli wrote: > As long as no one enables both ehci-platform and ehci-ppc-of at the same time > there is no problem. ehci-ppc-of should be removed in favor of ehci-platform > and make sure that the specific quirk in ehci-ppc-of also gets ported, other > that I see no issue using "usb-ehci" as the least detailed compatible property > name. Suppose a DT board file is created for a oontroller which ehci-platform can't handle. Then by your proposal, the board file shouldn't have "usb-ehci" in its compatible property. Now later on, suppose ehci-platform is enhanced so that it can manage that controller. It's too late to update the board file because the information has already been written to various firmwares. The enhanced ehci-platform would have to include a special entry to match the controller. Since this reasoning applies every time ehci-platform is updated, it seems reasonable to use the same approach right from the beginning. Alan Stern
On 10/24/2012 11:46 AM, Alan Stern wrote: > On Wed, 24 Oct 2012, Stephen Warren wrote: > >>> How do we determine which existing drivers claim to support usb-ehci? >>> A quick search under arch/ and drivers/ turns up nothing but >>> drivers/usb/host/ehci-ppc-of.c. Changing it to a more HW-specific >>> match should be easy enough, and then "usb-ehci" would be safe to use >>> in ehci-platform.c. >> >> It's not drivers that claim to support usb-ehci, but device tree files >> that contain nodes that include "usb-ehci" in their compatible value, >> yet need to be handled by a driver that isn't the generic USB EHCI >> driver, and that driver binds to the other more specific compatible value: >> >>> $ grep -HrnI usb-ehci arch/*/boot/dts ... >> and then search for all those other compatible values in drivers. The >> ARM instances certainly all have this issue (although perhaps it's worth >> investigating if a generic could work for them instead). The PPC >> instances need more investigation; the values don't show up in of match >> tables but rather in code. > > Ah, now I'm starting to get the picture. > > So by listing "usb-ehci" in its device ID table, a driver would > essentially be saying that it can handle _all_ USB EHCI controllers. Yes, exactly. > (Which means that the entry in ehci-ppc-of.c is questionable; perhaps > the intention is that this driver really does handle all EHCI > controllers on any PPC-based OpenFirmware platform bus.) Yes, that seems questionable, although perhaps within the context of only enabling the driver on PPC specifically, it may be reasonable. >> This doesn't continue forever though; you typically only list the >> specific HW model, the base specific model it's compatible with, and any >> generic value needed. So, a hypothetical Tegra40 EHCI controller would be: >> >> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci". > > What's the reason for listing the generic value if drivers can't key > off it? Does it contain any information that's not already present in > the specific values? This may or may not be a mistake. The idea is that usb-ehci is included in the device node's compatible list iff the HW is 100% compatible with a "usb-ehci" HW device, and hence a pure usb-ehci driver can handle the hardware without any additional knowledge. (That's true in general for any compatible value). It's quite possible that this often gets translated to the subtly different "the HW is an EHCI controller, so it gets compatible="usb-ehci"" when writing .dts files. So yes we probably should remove compatible="usb-ehci" from any device node for HW which really isn't a pure EHCI device. That would presumably require looking at the existing custom drivers and/or HW specs to determine what, if anything, is different about the HW. Related, at least on Tegra, the PHY handling is IIRC pretty tightly coupled into the EHCI driver. We probably should have separate nodes for, and drivers for, the PHYs instead. I don't know if that would remove all the non-standard attributes of the Tegra EHCI HW and hence allow Tegra's EHCI to be handled by the generic driver. From memory, there are certainly some HW workarounds in the Tegra EHCI driver that would need to be ported though. > It sounds like the ehci-platform driver should simply list all the > specific HW device types (or base HW device types) that it handles, and > it shouldn't include "usb-ehci" in the list. As more DT board files > are created or as ehci-platform becomes capable to taking over from > other drivers, its device list would grow. That's certainly a reasonable way to go too. I think the only downside with that approach is that every new device needs a kernel update to add it to the table, whereas using a generic compatible="usb-ehci" wouldn't, assuming no quirks were required for the new device.
On Wednesday 24 October 2012 14:04:12 Alan Stern wrote: > On Wed, 24 Oct 2012, Florian Fainelli wrote: > > > As long as no one enables both ehci-platform and ehci-ppc-of at the same time > > there is no problem. ehci-ppc-of should be removed in favor of ehci-platform > > and make sure that the specific quirk in ehci-ppc-of also gets ported, other > > that I see no issue using "usb-ehci" as the least detailed compatible property > > name. > > Suppose a DT board file is created for a oontroller which ehci-platform > can't handle. Then by your proposal, the board file shouldn't have > "usb-ehci" in its compatible property. > > Now later on, suppose ehci-platform is enhanced so that it can manage > that controller. It's too late to update the board file because the > information has already been written to various firmwares. The > enhanced ehci-platform would have to include a special entry to match > the controller. In any case you are supposed to use a compatible property which describes as much as possible your hardware, and this one should have the precedence if a special treatment is required, so I see no problem with this approach. > > Since this reasoning applies every time ehci-platform is updated, it > seems reasonable to use the same approach right from the beginning. > > Alan Stern >
On 10/24/2012 8:09 AM, Stephen Warren wrote: > On 10/24/2012 11:46 AM, Alan Stern wrote: >> On Wed, 24 Oct 2012, Stephen Warren wrote: >> >>>> How do we determine which existing drivers claim to support usb-ehci? >>>> A quick search under arch/ and drivers/ turns up nothing but >>>> drivers/usb/host/ehci-ppc-of.c. Changing it to a more HW-specific >>>> match should be easy enough, and then "usb-ehci" would be safe to use >>>> in ehci-platform.c. >>> >>> It's not drivers that claim to support usb-ehci, but device tree files >>> that contain nodes that include "usb-ehci" in their compatible value, >>> yet need to be handled by a driver that isn't the generic USB EHCI >>> driver, and that driver binds to the other more specific compatible value: >>> >>>> $ grep -HrnI usb-ehci arch/*/boot/dts > ... >>> and then search for all those other compatible values in drivers. The >>> ARM instances certainly all have this issue (although perhaps it's worth >>> investigating if a generic could work for them instead). The PPC >>> instances need more investigation; the values don't show up in of match >>> tables but rather in code. >> >> Ah, now I'm starting to get the picture. >> >> So by listing "usb-ehci" in its device ID table, a driver would >> essentially be saying that it can handle _all_ USB EHCI controllers. Actually, it means that the driver can handle at least USB EHCI controllers that are 100% compatible with the EHCI spec (requiring nothing extra). The driver might be able to handle almost-compatible controllers, possibly with the help of additional properties. If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that a given version of that driver will work, but it's worth a try in the event that no more-specific driver is matched. > > Yes, exactly. > >> (Which means that the entry in ehci-ppc-of.c is questionable; perhaps >> the intention is that this driver really does handle all EHCI >> controllers on any PPC-based OpenFirmware platform bus.) > > Yes, that seems questionable, although perhaps within the context of > only enabling the driver on PPC specifically, it may be reasonable. > >>> This doesn't continue forever though; you typically only list the >>> specific HW model, the base specific model it's compatible with, and any >>> generic value needed. So, a hypothetical Tegra40 EHCI controller would be: >>> >>> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci". >> >> What's the reason for listing the generic value if drivers can't key >> off it? Does it contain any information that's not already present in >> the specific values? > > This may or may not be a mistake. > > The idea is that usb-ehci is included in the device node's compatible > list iff the HW is 100% compatible with a "usb-ehci" HW device, and > hence a pure usb-ehci driver can handle the hardware without any > additional knowledge. (That's true in general for any compatible value). > > It's quite possible that this often gets translated to the subtly > different "the HW is an EHCI controller, so it gets > compatible="usb-ehci"" when writing .dts files. > > So yes we probably should remove compatible="usb-ehci" from any device > node for HW which really isn't a pure EHCI device. That would presumably > require looking at the existing custom drivers and/or HW specs to > determine what, if anything, is different about the HW. > > Related, at least on Tegra, the PHY handling is IIRC pretty tightly > coupled into the EHCI driver. We probably should have separate nodes > for, and drivers for, the PHYs instead. I don't know if that would > remove all the non-standard attributes of the Tegra EHCI HW and hence > allow Tegra's EHCI to be handled by the generic driver. From memory, > there are certainly some HW workarounds in the Tegra EHCI driver that > would need to be ported though. > >> It sounds like the ehci-platform driver should simply list all the >> specific HW device types (or base HW device types) that it handles, and >> it shouldn't include "usb-ehci" in the list. As more DT board files >> are created or as ehci-platform becomes capable to taking over from >> other drivers, its device list would grow. > > That's certainly a reasonable way to go too. I think the only downside > with that approach is that every new device needs a kernel update to add > it to the table, whereas using a generic compatible="usb-ehci" wouldn't, > assuming no quirks were required for the new device. > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss >
On Wed, 24 Oct 2012, Mitch Bradley wrote: > >> Ah, now I'm starting to get the picture. > >> > >> So by listing "usb-ehci" in its device ID table, a driver would > >> essentially be saying that it can handle _all_ USB EHCI controllers. > > > Actually, it means that the driver can handle at least USB EHCI > controllers that are 100% compatible with the EHCI spec (requiring > nothing extra). The driver might be able to handle almost-compatible > controllers, possibly with the help of additional properties. Unfortunately that's not saying very much. The EHCI spec describes only PCI-based controllers. Every other sort does require something extra. > If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that > a given version of that driver will work, but it's worth a try in the > event that no more-specific driver is matched. While I haven't checked in detail, it seems quite likely that this would not work with some of the devices that have "usb-ehci" listed in their compatible values. Alan Stern
On Wed, 24 Oct 2012, Stephen Warren wrote: > > What's the reason for listing the generic value if drivers can't key > > off it? Does it contain any information that's not already present in > > the specific values? > > This may or may not be a mistake. > > The idea is that usb-ehci is included in the device node's compatible > list iff the HW is 100% compatible with a "usb-ehci" HW device, and But there is no such thing as a "usb-ehci" HW device. That's part of the reason this whole discussion got started. > hence a pure usb-ehci driver can handle the hardware without any > additional knowledge. (That's true in general for any compatible value). To as great an extent as is reasonable, ehci-platform is supposed to be that "pure usb-ehci driver". > It's quite possible that this often gets translated to the subtly > different "the HW is an EHCI controller, so it gets > compatible="usb-ehci"" when writing .dts files. > > So yes we probably should remove compatible="usb-ehci" from any device > node for HW which really isn't a pure EHCI device. That would presumably > require looking at the existing custom drivers and/or HW specs to > determine what, if anything, is different about the HW. Of course, removing it from the board files in the new kernel won't have any effect on old board files embedded in firmware. Those old firmwares then might not work with newer kernels. > Related, at least on Tegra, the PHY handling is IIRC pretty tightly > coupled into the EHCI driver. We probably should have separate nodes > for, and drivers for, the PHYs instead. I don't know if that would > remove all the non-standard attributes of the Tegra EHCI HW and hence > allow Tegra's EHCI to be handled by the generic driver. From memory, > there are certainly some HW workarounds in the Tegra EHCI driver that > would need to be ported though. In fact, ehci-tegra is probably the _most_ non-standard of the EHCI drivers. > > It sounds like the ehci-platform driver should simply list all the > > specific HW device types (or base HW device types) that it handles, and > > it shouldn't include "usb-ehci" in the list. As more DT board files > > are created or as ehci-platform becomes capable to taking over from > > other drivers, its device list would grow. > > That's certainly a reasonable way to go too. I think the only downside > with that approach is that every new device needs a kernel update to add > it to the table, whereas using a generic compatible="usb-ehci" wouldn't, > assuming no quirks were required for the new device. New devices could list an older device they are upwardly compatible with. Then no kernel update would be needed. Now, I agree that a generic entry would be more logical, but there's no generic "standard" hardware for it to describe. Alan Stern
On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote: > >> So by listing "usb-ehci" in its device ID table, a driver would > >> essentially be saying that it can handle _all_ USB EHCI controllers. > > > Actually, it means that the driver can handle at least USB EHCI > controllers that are 100% compatible with the EHCI spec (requiring > nothing extra). The driver might be able to handle almost-compatible > controllers, possibly with the help of additional properties. > > If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that > a given version of that driver will work, but it's worth a try in the > event that no more-specific driver is matched. Not sure fallback is a good term here. The of parses the compatible from left to right. If the device specific entry is not found (in the driver) then end up with usb-ehci. If we need a quirk later on we add the device specific entry to the driver (which will match before usb-ehci is found) and we could use this entry to apply the quirk. That way you can apply quirks without touch the firmware / device tree. Sebastian
On Thu, 25 Oct 2012, Sebastian Andrzej Siewior wrote: > On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote: > > >> So by listing "usb-ehci" in its device ID table, a driver would > > >> essentially be saying that it can handle _all_ USB EHCI controllers. > > > > > > Actually, it means that the driver can handle at least USB EHCI > > controllers that are 100% compatible with the EHCI spec (requiring > > nothing extra). The driver might be able to handle almost-compatible > > controllers, possibly with the help of additional properties. > > > > If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that > > a given version of that driver will work, but it's worth a try in the > > event that no more-specific driver is matched. > > Not sure fallback is a good term here. The of parses the compatible from left > to right. If the device specific entry is not found (in the driver) then end > up with usb-ehci. If we need a quirk later on we add the device specific entry > to the driver (which will match before usb-ehci is found) and we could use > this entry to apply the quirk. That way you can apply quirks without touch the > firmware / device tree. What happens if the drivers get probed in the wrong order? That is, if ehci-platform gets probed before ehci-spear (or whatever)? Alan Stern
On 10/25/2012 04:23 AM, Sebastian Andrzej Siewior wrote: > On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote: >>>> So by listing "usb-ehci" in its device ID table, a driver would >>>> essentially be saying that it can handle _all_ USB EHCI controllers. >> >> >> Actually, it means that the driver can handle at least USB EHCI >> controllers that are 100% compatible with the EHCI spec (requiring >> nothing extra). The driver might be able to handle almost-compatible >> controllers, possibly with the help of additional properties. >> >> If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that >> a given version of that driver will work, but it's worth a try in the >> event that no more-specific driver is matched. > > Not sure fallback is a good term here. The of parses the compatible from left > to right. If the device specific entry is not found (in the driver) then end > up with usb-ehci. If we need a quirk later on we add the device specific entry > to the driver (which will match before usb-ehci is found) and we could use > this entry to apply the quirk. That way you can apply quirks without touch the > firmware / device tree. That's the theory on how the compatible values are used, but in practice, each driver is tested against all the values in a node before moving on to the next driver, so the (current kernel's implementation of the) matching order is not what's needed to make that work.
On Thu, Oct 25, 2012 at 10:36:14AM -0400, Alan Stern wrote: > What happens if the drivers get probed in the wrong order? That is, if > ehci-platform gets probed before ehci-spear (or whatever)? The "wrong" driver may get loaded. Right now, you should have them all in one driver. For instance the crypto node in mpc8315erdb.dts: | crypto@30000 { | compatible = "fsl,sec3.3", "fsl,sec3.1", "fsl,sec3.0", | "fsl,sec2.4", "fsl,sec2.2", "fsl,sec2.1", | "fsl,sec2.0"; … The higher the version, the more features are available by the hardware. The driver [0] probes only for "fsl,sec2.0" but it uses of_device_is_compatible() to check for the other entries. You could also add all 7 compatible entries to the driver and distinguish them by the driver_data field. This is an implementation detail. However, having two drivers, one for "fsl,sec3.3" and one for "fsl,sec2.0", would "randomly" load one of the two driver depending on link order and so on. [0] drivers/crypto/talitos.c > > Alan Stern > Sebastian
On Fri, 26 Oct 2012, Sebastian Andrzej Siewior wrote: > On Thu, Oct 25, 2012 at 10:36:14AM -0400, Alan Stern wrote: > > What happens if the drivers get probed in the wrong order? That is, if > > ehci-platform gets probed before ehci-spear (or whatever)? > > The "wrong" driver may get loaded. That's my point. That's why ehci-platform.c should not claim to match all devices listing "usb-ehci" in their compatible property. Alan Stern
diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt b/Documentation/devicetree/bindings/usb/ehci-platform.txt new file mode 100644 index 0000000..930b19e --- /dev/null +++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt @@ -0,0 +1,27 @@ +Generic Platform EHCI Controller +----------------------------------------------------- + +Required properties: +- compatible : "linux,ehci-platform" +- reg : Should contain 1 register ranges(address and length) +- interrupts : EHCI controller interrupt + +Optional properties: +- caps-offset : offset to the capabilities register (default = 0) +- has-tt : controller has transaction translator(s). +- has-synopsys-hc-bug : controller has the synopsys hc bug +- no-io-watchdog : controller does not need io watchdog + +- big-endian : descriptors and registers are both big endian. This + is the equivalent of specifying big-endian-desc and big-endian-regs. +OR +- big-endian-desc : descriptors are in big-endian format +- big-endian-regs : mmio is in big-endian format + +Example: + ehci@d8007c00 { + compatible = "ehci-platform"; + reg = <0xd8007c00 0x200>; + interrupts = <43>; + has-tt; + };
Add a binding document for ehci-platform driver. Signed-off-by: Tony Prisk <linux@prisktech.co.nz> --- .../devicetree/bindings/usb/ehci-platform.txt | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt