Message-ID: <7bb41c9b-20d8-6003-8a0f-b09f0071ed71@broadcom.com>
Date:   Mon, 26 Jun 2017 22:10:57 -0700
From:   Scott Branden <scott.branden@...adcom.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh@...nel.org>
Cc:     Arun Parameswaran <arun.parameswaran@...adcom.com>,
        Richard Cochran <richardcochran@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        "bcm-kernel-feedback-list@...adcom.com" 
        <bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH v1 1/2] dt-binding: ptp: add bindings document for dte
 based ptp clock

Hi Rob/Florian,

Thanks for input but still don't see any need for SoC specific
compatible stings.  IP revision specific yes.

On 17-06-22 06:04 PM, Florian Fainelli wrote:
> On 06/22/2017 05:42 PM, Scott Branden wrote:
>>
>> On 17-06-21 08:19 PM, Rob Herring wrote:
>>> On Tue, Jun 20, 2017 at 3:48 PM, Scott Branden
>>> <scott.branden@...adcom.com> wrote:
>>>> Hi Rob,
>>>>
>>>>
>>>> On 17-06-18 07:04 AM, Rob Herring wrote:
>>>>> On Mon, Jun 12, 2017 at 01:26:00PM -0700, Arun Parameswaran wrote:
>>>>>> Add device tree binding documentation for the Broadcom DTE
>>>>>> PTP clock driver.
>>>>>>
>>>>>> Signed-off-by: Arun Parameswaran <arun.parameswaran@...adcom.com>
>>>>>> ---
>>>>>>     Documentation/devicetree/bindings/ptp/brcm,ptp-dte.txt | 13
>>>>>> +++++++++++++
>>>>>>     1 file changed, 13 insertions(+)
>>>>>>     create mode 100644
>>>>>> Documentation/devicetree/bindings/ptp/brcm,ptp-dte.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/ptp/brcm,ptp-dte.txt
>>>>>> b/Documentation/devicetree/bindings/ptp/brcm,ptp-dte.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..07590bc
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/ptp/brcm,ptp-dte.txt
>>>>>> @@ -0,0 +1,13 @@
>>>>>> +* Broadcom Digital Timing Engine(DTE) based PTP clock driver
>>>>> Bindings describe h/w, not drivers.
>>>>>
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: should be "brcm,ptp-dte"
>>>>> Looks too generic. You need SoC specific compatible strings.
>>>> Rob, could you please help me understand the use of adding SoC specific
>>>> compatible strings.
>>>> I still don't get it.
>>>>
>>>> It's my understanding that the SoC compatibility string is to future
>>>> proof
>>>> against bugs/incompatibilities
>>>> between different versions of the hardware block due to integration
>>>> issues
>>>> or any other reason.
>>>> You can then compare in your driver because the strings were already
>>>> used in
>>>> the dtb.
>>>>
>>>> That would make sense if you can't already differentiate what SoC you
>>>> are
>>>> running on.
>>>> But the SoC is already specified in the root of the device tree in the
>>>> compatible string?
>>>> Why can't you just use of_machine_is_compatible inside your driver when
>>>> needed?
>>> Use of of_machine_is_compatible in drivers will result in the same
>>> mess we had with machine_is_X defines pre-DT. It practically
>>> guarantees that you must update the driver for a new SoC (with
>>> fallback compatibles you don't). Plus the matching logic for
>>> of_machine_is_compatible is open coded logic in every driver which is
>>> worse IMO than having a standard match table.
>> I don't understand what you mean by fallback compatible then.
>>
>> Let's say I have 3 SoCs that each contain the same ip block.
>> You want us to add a fallback compatibility per SoC, is that correct?
> I think Rob meant a fallback compatibility for the particular block.
> E.g: brcm,iproc-ptp is the fallback compatible string, but in your
> SoC-specific DTS, you would have at least:
>
> compatible = "brcm,cygnus-ptp", "brcm,iproc-ptp";
>
> Where cygnus-ptp is more specific than iproc-ptp
>> Then, if there is a workaround discovered in a particular SoC the driver
>> can be updated in the future without changing the dtb.
>>
>> Then, the block gets added to a 4th SoC.
>> You want us to another new compatibility string for the new SoC?
>> If the new SoC has a bug then the driver has to be updated whether it is
>> in uses the fallback compatible or machine_is_compatible string.
>>
>> There is no difference in amount of code added to a driver when a new
>> SoC is introduced into the system that has bugs that need to be handled
>> by the driver.
>>
>> The difference is in your recommendation we need to go through all the
>> drivers used by the new SoC and add fallback compatibility strings.
> Not really, the fallback is what the driver should be matching by
> default (hence the name fallback) and if and only if you need to have a
> SoC-specific behavior in your driver (because of bugs, or slight
> differences) would you be matching this SoC-specific compatible string
> to capture that and key the driver behavior based off that.
>
>> Then, we have to modify all the devicetree documentation for all the
>> drivers.  Then, we have to ensure that all dts files populate this new
>> fallback string (even if it is unused).  We don't see the benefit in
>> doing any of that.  Using machine_is_compatible and having less
>> compatibility strings to deal appears much cleaner and more foolproof
>> for all situations.
> When you introduce a new SoC, you would update all the bindings for the
> devices (not drivers) that are susceptible to be used by this SoC. If
> all goes well, your SoC has little bugs, and your drivers don't even
> need to see an update because they are already matching the fallback
> compatible string.
>
> That's what I understand from the suggestion but I may be totally off
> rails here.
In this particular case, the IP is integrated within and outside a 
family of SoCs
(the exact same block is in Capri for example).
The only fallback compatablity string that makes sense is "brcm, 
dte-ptp-v1".
When we integrate a new version of this IP we would add "brcm, dte-ptp-v2".

Rob, I still see no need to define any other compatablity strings.

Everything else can be handled with machine_is_compatible
with the exact same amount of code change to the driver?
Adding the additional strings simply
adds a need to change documentation and changes to dts that are otherwise
completely unnecessary.  machine_is_compatible appears to be the cleaner
and simpler solution?

Regards,
  Scott

Powered by blists - more mailing lists