Re: [PATCH v1 1/2] dt-binding: ptp: add bindings document for dte based ptp clock

[Scott Branden]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > > > ---
> > > > > >     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