| Message ID | 1484640308-25976-5-git-send-email-raviteja.garimella@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: > This patch adds device tree bindings documentation for Synopsys > USB device controller platform driver. Bindings describe h/w, not drivers. > > Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> > --- > .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 ++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt > > diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt > new file mode 100644 > index 0000000..0c18327 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt > @@ -0,0 +1,27 @@ > +Synopsys USB Device controller. > + > +The device node is used for Synopsys Designware Cores AHB > +Subsystem Device Controller (UDC). > + > +This device node is used by UDCs integrated it Broadcom's > +Northstar2 and Cygnus SoC's. You need compatible strings for these in addition. > + > +Required properties: > + - compatible: should be "snps,dw-ahb-udc" This is a different IP than DWC2? > + - reg: Offset and length of UDC register set > + - interrupts: description of interrupt line > + - phys: phandle to phy node. > + - extcon: phandle to the extcon device. This is optional and > + not required for those that don't require extcon support. > + Extcon support will be required if the UDC is connected to > + a Dual Role Device Phy that supports both Host and Device > + mode based on the external cable. Drop this. It should be a part of the phy. Also, I don't care to see new users of extcon binding because it needs redoing. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, On 17-01-19 09:36 AM, Rob Herring wrote: > On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: >> This patch adds device tree bindings documentation for Synopsys >> USB device controller platform driver. > > Bindings describe h/w, not drivers. >> >> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> >> --- >> .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 ++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >> >> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >> new file mode 100644 >> index 0000000..0c18327 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >> @@ -0,0 +1,27 @@ >> +Synopsys USB Device controller. >> + >> +The device node is used for Synopsys Designware Cores AHB >> +Subsystem Device Controller (UDC). >> + >> +This device node is used by UDCs integrated it Broadcom's >> +Northstar2 and Cygnus SoC's. > > You need compatible strings for these in addition. > We don't need compatibility strings when an IP block is integrated into an SoC. Otherwise each time we add the IP block to a new SoC we would need to update ever linux driver that supports that SoC. That doesn't make sense? Cygnus and Northstar2 use existing drivers for such block as UARTs, SPI controllers, NAND controllers, etc, etc. We haven't added compatibility strings for those drivers and won't be. Perhaps comment above can be: This device node is used by UDCs integrated it such as Broadcom's Northstar2 and Cygnus SoC's. >> + >> +Required properties: >> + - compatible: should be "snps,dw-ahb-udc" > > This is a different IP than DWC2? > >> + - reg: Offset and length of UDC register set >> + - interrupts: description of interrupt line >> + - phys: phandle to phy node. >> + - extcon: phandle to the extcon device. This is optional and >> + not required for those that don't require extcon support. >> + Extcon support will be required if the UDC is connected to >> + a Dual Role Device Phy that supports both Host and Device >> + mode based on the external cable. > > Drop this. It should be a part of the phy. Also, I don't care to see new > users of extcon binding because it needs redoing. > We may need extcon support to support DRD Phy though. We have to work within the framework that exists in linux today. If modified in the future adapt to it as needed? Regards, Scott -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/19/2017 11:30 AM, Scott Branden wrote: > Hi Rob, > > On 17-01-19 09:36 AM, Rob Herring wrote: >> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: >>> This patch adds device tree bindings documentation for Synopsys >>> USB device controller platform driver. >> >> Bindings describe h/w, not drivers. >>> >>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> >>> --- >>> .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 >>> ++++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>> >>> diff --git >>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>> new file mode 100644 >>> index 0000000..0c18327 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>> @@ -0,0 +1,27 @@ >>> +Synopsys USB Device controller. >>> + >>> +The device node is used for Synopsys Designware Cores AHB >>> +Subsystem Device Controller (UDC). >>> + >>> +This device node is used by UDCs integrated it Broadcom's >>> +Northstar2 and Cygnus SoC's. >> >> You need compatible strings for these in addition. >> > We don't need compatibility strings when an IP block is integrated into > an SoC. Otherwise each time we add the IP block to a new SoC we would > need to update ever linux driver that supports that SoC. That doesn't > make sense? You probably do need such a thing, here is how the compatible strings for IP blocks integrated into SoCs could be used: - provide a compatible strings which describes exactly the integration of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for that is that you want to be able to capture the specific IP block integration into a specific SoC and all its quirks - if the block has its own revision scheme (and it can be relied on), provide it: brcm,udc-v1.2 and that is probably the most meaningful compatible string for a client program here - have a some kind of fallback/catchall compatible string that describes the block: brcm,udc which may also work just fine, although is not preferred Defining compatible strings is meant to avoid making (possibly incompatible) Device Tree binding changes in the future, and you always have the liberty as a client program (OS, bootloader) to match only the compatible strings you care about, from the most specific (which includes the exact SoC) to the least specific. The key thing is that, if the full set of compatible strings are present and available, you can retroactively fix your driver to be more specific, very much less so your Device Tree blob (although there is disagreement).
Hi Florian, On 17-01-19 11:40 AM, Florian Fainelli wrote: > On 01/19/2017 11:30 AM, Scott Branden wrote: >> Hi Rob, >> >> On 17-01-19 09:36 AM, Rob Herring wrote: >>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: >>>> This patch adds device tree bindings documentation for Synopsys >>>> USB device controller platform driver. >>> >>> Bindings describe h/w, not drivers. >>>> >>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> >>>> --- >>>> .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 >>>> ++++++++++++++++++++++ >>>> 1 file changed, 27 insertions(+) >>>> create mode 100644 >>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>> new file mode 100644 >>>> index 0000000..0c18327 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>> @@ -0,0 +1,27 @@ >>>> +Synopsys USB Device controller. >>>> + >>>> +The device node is used for Synopsys Designware Cores AHB >>>> +Subsystem Device Controller (UDC). >>>> + >>>> +This device node is used by UDCs integrated it Broadcom's >>>> +Northstar2 and Cygnus SoC's. >>> >>> You need compatible strings for these in addition. >>> >> We don't need compatibility strings when an IP block is integrated into >> an SoC. Otherwise each time we add the IP block to a new SoC we would >> need to update ever linux driver that supports that SoC. That doesn't >> make sense? > > You probably do need such a thing, here is how the compatible strings > for IP blocks integrated into SoCs could be used: > > - provide a compatible strings which describes exactly the integration > of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for > that is that you want to be able to capture the specific IP block > integration into a specific SoC and all its quirks > > - if the block has its own revision scheme (and it can be relied on), > provide it: brcm,udc-v1.2 and that is probably the most meaningful > compatible string for a client program here > > - have a some kind of fallback/catchall compatible string that describes > the block: brcm,udc which may also work just fine, although is not preferred > > Defining compatible strings is meant to avoid making (possibly > incompatible) Device Tree binding changes in the future, and you always > have the liberty as a client program (OS, bootloader) to match only the > compatible strings you care about, from the most specific (which > includes the exact SoC) to the least specific. > > The key thing is that, if the full set of compatible strings are present > and available, you can retroactively fix your driver to be more > specific, very much less so your Device Tree blob (although there is > disagreement). > The driver stands alone from the SoC and does not need compatibility strings per SoC. New SoCs will use the exact same block. We don't add compatibility strings to any other drivers when we add the same block to a new SoC. Yes, if the version of the IP changes then a version or feature compatibility string is added to the driver. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/19/2017 12:07 PM, Scott Branden wrote: > Hi Florian, > > On 17-01-19 11:40 AM, Florian Fainelli wrote: >> On 01/19/2017 11:30 AM, Scott Branden wrote: >>> Hi Rob, >>> >>> On 17-01-19 09:36 AM, Rob Herring wrote: >>>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: >>>>> This patch adds device tree bindings documentation for Synopsys >>>>> USB device controller platform driver. >>>> >>>> Bindings describe h/w, not drivers. >>>>> >>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> >>>>> --- >>>>> .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 >>>>> ++++++++++++++++++++++ >>>>> 1 file changed, 27 insertions(+) >>>>> create mode 100644 >>>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>> >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>> new file mode 100644 >>>>> index 0000000..0c18327 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>> @@ -0,0 +1,27 @@ >>>>> +Synopsys USB Device controller. >>>>> + >>>>> +The device node is used for Synopsys Designware Cores AHB >>>>> +Subsystem Device Controller (UDC). >>>>> + >>>>> +This device node is used by UDCs integrated it Broadcom's >>>>> +Northstar2 and Cygnus SoC's. >>>> >>>> You need compatible strings for these in addition. >>>> >>> We don't need compatibility strings when an IP block is integrated into >>> an SoC. Otherwise each time we add the IP block to a new SoC we would >>> need to update ever linux driver that supports that SoC. That doesn't >>> make sense? >> >> You probably do need such a thing, here is how the compatible strings >> for IP blocks integrated into SoCs could be used: >> >> - provide a compatible strings which describes exactly the integration >> of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for >> that is that you want to be able to capture the specific IP block >> integration into a specific SoC and all its quirks >> >> - if the block has its own revision scheme (and it can be relied on), >> provide it: brcm,udc-v1.2 and that is probably the most meaningful >> compatible string for a client program here >> >> - have a some kind of fallback/catchall compatible string that describes >> the block: brcm,udc which may also work just fine, although is not >> preferred >> >> Defining compatible strings is meant to avoid making (possibly >> incompatible) Device Tree binding changes in the future, and you always >> have the liberty as a client program (OS, bootloader) to match only the >> compatible strings you care about, from the most specific (which >> includes the exact SoC) to the least specific. >> >> The key thing is that, if the full set of compatible strings are present >> and available, you can retroactively fix your driver to be more >> specific, very much less so your Device Tree blob (although there is >> disagreement). >> > The driver stands alone from the SoC and does not need compatibility > strings per SoC. New SoCs will use the exact same block. Even if you take the exact same block and put it in a different SoC, that's still an integration work that 99% of the time goes just fine because the validation worked great, and the 1% of the time where you need to capture an integration bug, you are glad this SoC-specific compatible string exists such that you can work around it in the driver. One way to solve that is to use SoC specific compatible strings because that presents itself as a self-contained and standardized way, or you can have your driver call into a piece of code that reads the SoC type/revision, but AFAICT this seems to be frowned upon because it presents some kind of layering violation. > > We don't add compatibility strings to any other drivers when we add the > same block to a new SoC. Ideally we would define new compatible strings for each new SoC we tape out, yet don't necessarily match them in client programs, but just define them as a safeguard in case something went wrong at the integration stage that is discovered after the fact.
On 1/19/2017 12:17 PM, Florian Fainelli wrote: > On 01/19/2017 12:07 PM, Scott Branden wrote: >> Hi Florian, >> >> On 17-01-19 11:40 AM, Florian Fainelli wrote: >>> On 01/19/2017 11:30 AM, Scott Branden wrote: >>>> Hi Rob, >>>> >>>> On 17-01-19 09:36 AM, Rob Herring wrote: >>>>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: >>>>>> This patch adds device tree bindings documentation for Synopsys >>>>>> USB device controller platform driver. >>>>> >>>>> Bindings describe h/w, not drivers. >>>>>> >>>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> >>>>>> --- >>>>>> .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 >>>>>> ++++++++++++++++++++++ >>>>>> 1 file changed, 27 insertions(+) >>>>>> create mode 100644 >>>>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>> >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>> new file mode 100644 >>>>>> index 0000000..0c18327 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>> @@ -0,0 +1,27 @@ >>>>>> +Synopsys USB Device controller. >>>>>> + >>>>>> +The device node is used for Synopsys Designware Cores AHB >>>>>> +Subsystem Device Controller (UDC). >>>>>> + >>>>>> +This device node is used by UDCs integrated it Broadcom's >>>>>> +Northstar2 and Cygnus SoC's. >>>>> >>>>> You need compatible strings for these in addition. >>>>> >>>> We don't need compatibility strings when an IP block is integrated into >>>> an SoC. Otherwise each time we add the IP block to a new SoC we would >>>> need to update ever linux driver that supports that SoC. That doesn't >>>> make sense? >>> >>> You probably do need such a thing, here is how the compatible strings >>> for IP blocks integrated into SoCs could be used: >>> >>> - provide a compatible strings which describes exactly the integration >>> of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for >>> that is that you want to be able to capture the specific IP block >>> integration into a specific SoC and all its quirks >>> >>> - if the block has its own revision scheme (and it can be relied on), >>> provide it: brcm,udc-v1.2 and that is probably the most meaningful >>> compatible string for a client program here >>> >>> - have a some kind of fallback/catchall compatible string that describes >>> the block: brcm,udc which may also work just fine, although is not >>> preferred >>> >>> Defining compatible strings is meant to avoid making (possibly >>> incompatible) Device Tree binding changes in the future, and you always >>> have the liberty as a client program (OS, bootloader) to match only the >>> compatible strings you care about, from the most specific (which >>> includes the exact SoC) to the least specific. >>> >>> The key thing is that, if the full set of compatible strings are present >>> and available, you can retroactively fix your driver to be more >>> specific, very much less so your Device Tree blob (although there is >>> disagreement). >>> >> The driver stands alone from the SoC and does not need compatibility >> strings per SoC. New SoCs will use the exact same block. > > Even if you take the exact same block and put it in a different SoC, > that's still an integration work that 99% of the time goes just fine > because the validation worked great, and the 1% of the time where you > need to capture an integration bug, you are glad this SoC-specific > compatible string exists such that you can work around it in the driver. That's a very conservative estimate. Based on my experience, it's more like 50/50, i.e., roughly half of the time we found SoC integration specific quirks or workaround are needed. > > One way to solve that is to use SoC specific compatible strings because > that presents itself as a self-contained and standardized way, or you > can have your driver call into a piece of code that reads the SoC > type/revision, but AFAICT this seems to be frowned upon because it > presents some kind of layering violation. > >> >> We don't add compatibility strings to any other drivers when we add the >> same block to a new SoC. > > Ideally we would define new compatible strings for each new SoC we tape > out, yet don't necessarily match them in client programs, but just > define them as a safeguard in case something went wrong at the > integration stage that is discovered after the fact. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17-01-19 01:55 PM, Ray Jui wrote: > > > On 1/19/2017 12:17 PM, Florian Fainelli wrote: >> On 01/19/2017 12:07 PM, Scott Branden wrote: >>> Hi Florian, >>> >>> On 17-01-19 11:40 AM, Florian Fainelli wrote: >>>> On 01/19/2017 11:30 AM, Scott Branden wrote: >>>>> Hi Rob, >>>>> >>>>> On 17-01-19 09:36 AM, Rob Herring wrote: >>>>>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: >>>>>>> This patch adds device tree bindings documentation for Synopsys >>>>>>> USB device controller platform driver. >>>>>> >>>>>> Bindings describe h/w, not drivers. >>>>>>> >>>>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> >>>>>>> --- >>>>>>> .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 >>>>>>> ++++++++++++++++++++++ >>>>>>> 1 file changed, 27 insertions(+) >>>>>>> create mode 100644 >>>>>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>>> >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>>> new file mode 100644 >>>>>>> index 0000000..0c18327 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >>>>>>> @@ -0,0 +1,27 @@ >>>>>>> +Synopsys USB Device controller. >>>>>>> + >>>>>>> +The device node is used for Synopsys Designware Cores AHB >>>>>>> +Subsystem Device Controller (UDC). >>>>>>> + >>>>>>> +This device node is used by UDCs integrated it Broadcom's >>>>>>> +Northstar2 and Cygnus SoC's. >>>>>> >>>>>> You need compatible strings for these in addition. >>>>>> >>>>> We don't need compatibility strings when an IP block is integrated into >>>>> an SoC. Otherwise each time we add the IP block to a new SoC we would >>>>> need to update ever linux driver that supports that SoC. That doesn't >>>>> make sense? >>>> >>>> You probably do need such a thing, here is how the compatible strings >>>> for IP blocks integrated into SoCs could be used: >>>> >>>> - provide a compatible strings which describes exactly the integration >>>> of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for >>>> that is that you want to be able to capture the specific IP block >>>> integration into a specific SoC and all its quirks >>>> >>>> - if the block has its own revision scheme (and it can be relied on), >>>> provide it: brcm,udc-v1.2 and that is probably the most meaningful >>>> compatible string for a client program here >>>> >>>> - have a some kind of fallback/catchall compatible string that describes >>>> the block: brcm,udc which may also work just fine, although is not >>>> preferred >>>> >>>> Defining compatible strings is meant to avoid making (possibly >>>> incompatible) Device Tree binding changes in the future, and you always >>>> have the liberty as a client program (OS, bootloader) to match only the >>>> compatible strings you care about, from the most specific (which >>>> includes the exact SoC) to the least specific. >>>> >>>> The key thing is that, if the full set of compatible strings are present >>>> and available, you can retroactively fix your driver to be more >>>> specific, very much less so your Device Tree blob (although there is >>>> disagreement). >>>> >>> The driver stands alone from the SoC and does not need compatibility >>> strings per SoC. New SoCs will use the exact same block. >> >> Even if you take the exact same block and put it in a different SoC, >> that's still an integration work that 99% of the time goes just fine >> because the validation worked great, and the 1% of the time where you >> need to capture an integration bug, you are glad this SoC-specific >> compatible string exists such that you can work around it in the driver. > > That's a very conservative estimate. Based on my experience, it's more > like 50/50, i.e., roughly half of the time we found SoC integration > specific quirks or workaround are needed. > 50% is an exaggeration for sure. Maybe a driver you are has that issue but that is not the case with most drivers. We have many IP blocks in the SoC - both internal and externally sourced IP. We integrate SP805 timer driver into many SoCs and never specify a SoC specific compatibility string with it (nor should we). That being said - if your driver needs to know SoC specifics is should not need to have an SoC specific compatibility string added per driver. Why can your driver just not query that information from the upper level SoC specific info already present in device tree? Each SoC is already specified in device tree at the upper level already. Example: arch/arm/boot/dts/bcm7445.dtsi has this compatibility info already present in its device tree: compatible = "brcm,bcm7445", "brcm,brcmstb"; If needed, a driver should query this info rather than adding SoC specific compatibility strings to every single device tree entry. We should only add driver revision numbers as needed, not SoC specific names. That way drivers don't change when the (same revision) of the IP block is added to a new SoCs. And then if a SoC specific workaround is needed the upper level compatibility string can be queried should be utilized. It already exists today and is available for use to all drivers. >> >> One way to solve that is to use SoC specific compatible strings because >> that presents itself as a self-contained and standardized way, or you >> can have your driver call into a piece of code that reads the SoC >> type/revision, but AFAICT this seems to be frowned upon because it >> presents some kind of layering violation. >> >>> >>> We don't add compatibility strings to any other drivers when we add the >>> same block to a new SoC. >> >> Ideally we would define new compatible strings for each new SoC we tape >> out, yet don't necessarily match them in client programs, but just >> define them as a safeguard in case something went wrong at the >> integration stage that is discovered after the fact. >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/19/2017 02:36 PM, Scott Branden wrote: >>>> The driver stands alone from the SoC and does not need compatibility >>>> strings per SoC. New SoCs will use the exact same block. >>> >>> Even if you take the exact same block and put it in a different SoC, >>> that's still an integration work that 99% of the time goes just fine >>> because the validation worked great, and the 1% of the time where you >>> need to capture an integration bug, you are glad this SoC-specific >>> compatible string exists such that you can work around it in the driver. >> >> That's a very conservative estimate. Based on my experience, it's more >> like 50/50, i.e., roughly half of the time we found SoC integration >> specific quirks or workaround are needed. >> > 50% is an exaggeration for sure. Maybe a driver you are has that issue > but that is not the case with most drivers. We have many IP blocks in > the SoC - both internal and externally sourced IP. We integrate SP805 > timer driver into many SoCs and never specify a SoC specific > compatibility string with it (nor should we). Well, that's a good example where in premise, each SoC vendor integrating such a peripheral from a third party should actually have defined its own SoC/vendor compatible string to document the integration. And you can sometimes see some vendors having to workaround such essential peripherals and ending-up documenting compatible strings (or close enough in the example at [1]). [1]: http://www.spinics.net/lists/devicetree/msg159585.html It's a bad example though in that it's an IP that came from ARM, so the confidence level in getting the integration right is just higher (typically above level 9000), because ripping apart a third party is governed by strict architecture licensing agreements that usually prevents people suffering from the Not Invented Here syndrome from making damage. > > That being said - if your driver needs to know SoC specifics is should > not need to have an SoC specific compatibility string added per driver. > Why can your driver just not query that information from the upper level > SoC specific info already present in device tree? You could do that, but that just does not happen to be a common or recommended practice AFAICT, although I could be just wrong here of course. > > Each SoC is already specified in device tree at the upper level already. > Example: > arch/arm/boot/dts/bcm7445.dtsi has this compatibility info already > present in its device tree: > > compatible = "brcm,bcm7445", "brcm,brcmstb"; > > If needed, a driver should query this info rather than adding SoC > specific compatibility strings to every single device tree entry. Or you could just put it in the compatible string list for a given peripheral, and yes, this is a repetition of information that is already there at a higher level from that particular node, but, it has the advantage of making all this information self contained within that node's context, and that's a good design goal. > > We should only add driver revision numbers as needed, not SoC specific > names. That way drivers don't change when the (same revision) of the IP > block is added to a new SoCs. And then if a SoC specific workaround is > needed the upper level compatibility string can be queried should be > utilized. It already exists today and is available for use to all drivers. The point is to plan ahead for information that you *may*, but *wish* you did not need. Quite frankly, I don't think you are going to win any argument where you don't add a SoC compatible string to the binding, because there are tons of precedents and good practices that suggest doing it. You might as well just do it, it's documented, it's there, if you end up using it or not, that's totally up to the driver author.
Hi Rob, On Thu, Jan 19, 2017 at 11:06 PM, Rob Herring <robh@kernel.org> wrote: > On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote: >> This patch adds device tree bindings documentation for Synopsys >> USB device controller platform driver. > > Bindings describe h/w, not drivers. Will correct the commit message. >> >> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> >> --- >> .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 ++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >> >> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >> new file mode 100644 >> index 0000000..0c18327 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt >> @@ -0,0 +1,27 @@ >> +Synopsys USB Device controller. >> + >> +The device node is used for Synopsys Designware Cores AHB >> +Subsystem Device Controller (UDC). >> + >> +This device node is used by UDCs integrated it Broadcom's >> +Northstar2 and Cygnus SoC's. > > You need compatible strings for these in addition. Is it fine to have "brcm,iproc-udc"? iProc refers to a Broadcom family of processors that includes above mentioned SoCs. I see there are some compatible strings that are based on the IP, and some based on the SoCs. I chose to have the IP based string. Please let me know which one would be agreeable in this case. I will also correct the typo in the above notes -- it meant to be UDCs integrated into Broadcom's Northstar2 and Cygnus SoC's. > >> + >> +Required properties: >> + - compatible: should be "snps,dw-ahb-udc" > > This is a different IP than DWC2? Yes, this is different IP. DWC2 is HS OTG. > >> + - reg: Offset and length of UDC register set >> + - interrupts: description of interrupt line >> + - phys: phandle to phy node. >> + - extcon: phandle to the extcon device. This is optional and >> + not required for those that don't require extcon support. >> + Extcon support will be required if the UDC is connected to >> + a Dual Role Device Phy that supports both Host and Device >> + mode based on the external cable. > > Drop this. It should be a part of the phy. Also, I don't care to see new > users of extcon binding because it needs redoing. Currently we can't get the extcon node from Phy. "extcon_get_edev_by_phandle" requires "extcon" property, else would fail. As Scott said in one of the comments, we can drop this when we get that support in kernel. Is it fine? Thanks, Ravi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 19, 2017 at 4:56 PM, Florian Fainelli <florian.fainelli@broadcom.com> wrote: > On 01/19/2017 02:36 PM, Scott Branden wrote: >>>>> The driver stands alone from the SoC and does not need compatibility >>>>> strings per SoC. New SoCs will use the exact same block. >>>> >>>> Even if you take the exact same block and put it in a different SoC, >>>> that's still an integration work that 99% of the time goes just fine >>>> because the validation worked great, and the 1% of the time where you >>>> need to capture an integration bug, you are glad this SoC-specific >>>> compatible string exists such that you can work around it in the driver. >>> >>> That's a very conservative estimate. Based on my experience, it's more >>> like 50/50, i.e., roughly half of the time we found SoC integration >>> specific quirks or workaround are needed. >>> >> 50% is an exaggeration for sure. Maybe a driver you are has that issue >> but that is not the case with most drivers. We have many IP blocks in >> the SoC - both internal and externally sourced IP. We integrate SP805 >> timer driver into many SoCs and never specify a SoC specific >> compatibility string with it (nor should we). Even if it was only 10%, that's still reason to do it. > Well, that's a good example where in premise, each SoC vendor > integrating such a peripheral from a third party should actually have > defined its own SoC/vendor compatible string to document the > integration. And you can sometimes see some vendors having to workaround > such essential peripherals and ending-up documenting compatible strings > (or close enough in the example at [1]). ARM peripherals are a bit unique because they have ID registers and vendors tend to change them if they change the IP. And we can also set the ID in the DT. It's also a huge difference between a timer and a USB controller. There's very little in a timer that vendors can f*ck up as well as few revisions and config options. Experience has shown that USB always gets integrated in different ways. We can't even get the number of clocks right on licensed IP blocks. Like many things, there is a judgement call here. > [1]: http://www.spinics.net/lists/devicetree/msg159585.html > > It's a bad example though in that it's an IP that came from ARM, so the > confidence level in getting the integration right is just higher > (typically above level 9000), because ripping apart a third party is > governed by strict architecture licensing agreements that usually > prevents people suffering from the Not Invented Here syndrome from > making damage. > >> >> That being said - if your driver needs to know SoC specifics is should >> not need to have an SoC specific compatibility string added per driver. >> Why can your driver just not query that information from the upper level >> SoC specific info already present in device tree? > > You could do that, but that just does not happen to be a common or > recommended practice AFAICT, although I could be just wrong here of course. We did a lot of work to get rid machine_is_X(). Let's say you have 10 SoCs and 5 have a quirk in a device and 5 don't. You need 2 device compatible strings to match in that case. If you check at the top level you may have to check 5 strings because you can't claim all 5 SoCs to be compatible with each other (that only works for sub/supersets). You would also have to update the driver for new SoCs depending if they had the quirk or not. >> Each SoC is already specified in device tree at the upper level already. >> Example: >> arch/arm/boot/dts/bcm7445.dtsi has this compatibility info already >> present in its device tree: >> >> compatible = "brcm,bcm7445", "brcm,brcmstb"; >> >> If needed, a driver should query this info rather than adding SoC >> specific compatibility strings to every single device tree entry. > > Or you could just put it in the compatible string list for a given > peripheral, and yes, this is a repetition of information that is already > there at a higher level from that particular node, but, it has the > advantage of making all this information self contained within that > node's context, and that's a good design goal. > >> >> We should only add driver revision numbers as needed, not SoC specific >> names. That way drivers don't change when the (same revision) of the IP >> block is added to a new SoCs. And then if a SoC specific workaround is >> needed the upper level compatibility string can be queried should be >> utilized. It already exists today and is available for use to all drivers. > > The point is to plan ahead for information that you *may*, but *wish* > you did not need. > > Quite frankly, I don't think you are going to win any argument where you > don't add a SoC compatible string to the binding, because there are tons > of precedents and good practices that suggest doing it. You might as > well just do it, it's documented, it's there, if you end up using it or > not, that's totally up to the driver author. Right. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt new file mode 100644 index 0000000..0c18327 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt @@ -0,0 +1,27 @@ +Synopsys USB Device controller. + +The device node is used for Synopsys Designware Cores AHB +Subsystem Device Controller (UDC). + +This device node is used by UDCs integrated it Broadcom's +Northstar2 and Cygnus SoC's. + +Required properties: + - compatible: should be "snps,dw-ahb-udc" + - reg: Offset and length of UDC register set + - interrupts: description of interrupt line + - phys: phandle to phy node. + - extcon: phandle to the extcon device. This is optional and + not required for those that don't require extcon support. + Extcon support will be required if the UDC is connected to + a Dual Role Device Phy that supports both Host and Device + mode based on the external cable. + +Example: + udc_dwc: usb@664e0000 { + compatible = "snps,dw-ahb-udc"; + reg = <0x664e0000 0x2000>; + interrupts = <GIC_SPI 424 IRQ_TYPE_LEVEL_HIGH>; + phys = <&usbdrd_phy>; + extcon = <&usbdrd_phy>"; + };
This patch adds device tree bindings documentation for Synopsys USB device controller platform driver. Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com> --- .../devicetree/bindings/usb/snps,dw-ahb-udc.txt | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt