Message-ID: <4e086b59-fdca-729a-149f-85967604310f@broadcom.com>
Date:   Thu, 19 Jan 2017 14:36:58 -0800
From:   Scott Branden <scott.branden@...adcom.com>
To:     Ray Jui <ray.jui@...adcom.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh@...nel.org>,
        Raviteja Garimella <raviteja.garimella@...adcom.com>
Cc:     Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com, linux-usb@...r.kernel.org
Subject: Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform
 driver



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@...adcom.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.
>>

Powered by blists - more mailing lists