[PATCH v1 0/2] Add support for Broadcom DTE based PTP clock

[PATCH v1 0/2] Add support for Broadcom DTE based PTP clock

[Arun Parameswaran]

Hi,

This patchset adds support for the DTE based PTP clock for Broadcom SoCs.

The DTE nco based PTP clock can be used in both wired and wireless networks
for precision time-stmaping purposes.

Arun Parameswaran (2):
  dt-binding: ptp: add bindings document for dte based ptp clock
  ptp: Add a ptp clock driver for Broadcom DTE

 .../devicetree/bindings/ptp/brcm,ptp-dte.txt       |  13 +
 drivers/ptp/Kconfig                                |  16 +
 drivers/ptp/Makefile                               |   1 +
 drivers/ptp/ptp_dte.c                              | 353 +++++++++++++++++++++
 4 files changed, 383 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ptp/brcm,ptp-dte.txt
 create mode 100644 drivers/ptp/ptp_dte.c

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arun Parameswaran]

Add device tree binding documentation for the Broadcom DTE
PTP clock driver.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.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
+
+Required properties:
+- compatible: should be "brcm,ptp-dte"
+- reg: address and length of the DTE block's NCO registers
+
+Example:
+
+ptp_dte: ptp_dte@180af650 {
+	compatible = "brcm,ptp-dte";
+	reg = <0x180af650 0x10>;
+	status = "okay";
+};
-- 
1.9.1

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

[Richard Cochran]

On Mon, Jun 12, 2017 at 01:26:00PM -0700, Arun Parameswaran wrote:
> +Example:
> +
> +ptp_dte: ptp_dte@180af650 {
> +	compatible = "brcm,ptp-dte";
> +	reg = <0x180af650 0x10>;
> +	status = "okay";
> +};

This patch set looks okay, as far as it goes, but how does one
actually use the clock?

How does one connect this node to an Ethernet MAC, for example?

Thanks,
Richard

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

[Arun Parameswaran]

Hi Richard,

On 17-06-12 10:09 PM, Richard Cochran wrote:
> On Mon, Jun 12, 2017 at 01:26:00PM -0700, Arun Parameswaran wrote:
>> +Example:
>> +
>> +ptp_dte: ptp_dte@180af650 {
>> +	compatible = "brcm,ptp-dte";
>> +	reg = <0x180af650 0x10>;
>> +	status = "okay";
>> +};
> This patch set looks okay, as far as it goes, but how does one
> actually use the clock?
>
> How does one connect this node to an Ethernet MAC, for example?
In some of the SoC's, the DTE's NCO is synced in hardware with the Ethernet's
1588 Shim NCO (used for time-stamping the packets). We are working on getting
all the future SoC's synced this way (for Ethernet).

In other scenarios, we use an user-space app to perform the sync based on
time-stamps from external hardware sources like the Wi-Fi.
> Thanks,
> Richard

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ray Jui]

On 6/13/17 10:46 AM, Arun Parameswaran wrote:
> 
> Hi Richard,
> 
> On 17-06-12 10:09 PM, Richard Cochran wrote:
>> On Mon, Jun 12, 2017 at 01:26:00PM -0700, Arun Parameswaran wrote:
>>> +Example:
>>> +
>>> +ptp_dte: ptp_dte@180af650 {
>>> +	compatible = "brcm,ptp-dte";
>>> +	reg = <0x180af650 0x10>;
>>> +	status = "okay";
>>> +};
>> This patch set looks okay, as far as it goes, but how does one
>> actually use the clock?
>>
>> How does one connect this node to an Ethernet MAC, for example?
> In some of the SoC's, the DTE's NCO is synced in hardware with the Ethernet's
> 1588 Shim NCO (used for time-stamping the packets). We are working on getting
> all the future SoC's synced this way (for Ethernet).
> 

So apparently you do not need any phandle based DT binding defined for
the connection between the Ethernet device node and the PTP device node?

> In other scenarios, we use an user-space app to perform the sync based on
> time-stamps from external hardware sources like the Wi-Fi.
>> Thanks,
>> Richard
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arun Parameswaran]

On 17-06-14 11:18 AM, Ray Jui wrote:
> 
> 
> On 6/13/17 10:46 AM, Arun Parameswaran wrote:
>>
>> Hi Richard,
>>
>> On 17-06-12 10:09 PM, Richard Cochran wrote:
>>> On Mon, Jun 12, 2017 at 01:26:00PM -0700, Arun Parameswaran wrote:
>>>> +Example:
>>>> +
>>>> +ptp_dte: ptp_dte@180af650 {
>>>> +	compatible = "brcm,ptp-dte";
>>>> +	reg = <0x180af650 0x10>;
>>>> +	status = "okay";
>>>> +};
>>> This patch set looks okay, as far as it goes, but how does one
>>> actually use the clock?
>>>
>>> How does one connect this node to an Ethernet MAC, for example?
>> In some of the SoC's, the DTE's NCO is synced in hardware with the Ethernet's
>> 1588 Shim NCO (used for time-stamping the packets). We are working on getting
>> all the future SoC's synced this way (for Ethernet).
>>
> 
> So apparently you do not need any phandle based DT binding defined for
> the connection between the Ethernet device node and the PTP device node?
> 
Correct. This driver doesn't need any phandle.

The Ethernet/Switch driver, in the SoC's where the hardware link is not
available, might need the phandle to this driver.

>> In other scenarios, we use an user-space app to perform the sync based on
>> time-stamps from external hardware sources like the Wi-Fi.
>>> Thanks,
>>> Richard
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arun Parameswaran]

On 17-06-14 11:18 AM, Ray Jui wrote:
> 
> 
> On 6/13/17 10:46 AM, Arun Parameswaran wrote:
>>
>> Hi Richard,
>>
>> On 17-06-12 10:09 PM, Richard Cochran wrote:
>>> On Mon, Jun 12, 2017 at 01:26:00PM -0700, Arun Parameswaran wrote:
>>>> +Example:
>>>> +
>>>> +ptp_dte: ptp_dte@180af650 {
>>>> +	compatible = "brcm,ptp-dte";
>>>> +	reg = <0x180af650 0x10>;
>>>> +	status = "okay";
>>>> +};
>>> This patch set looks okay, as far as it goes, but how does one
>>> actually use the clock?
>>>
>>> How does one connect this node to an Ethernet MAC, for example?
>> In some of the SoC's, the DTE's NCO is synced in hardware with the Ethernet's
>> 1588 Shim NCO (used for time-stamping the packets). We are working on getting
>> all the future SoC's synced this way (for Ethernet).
>>
> 
> So apparently you do not need any phandle based DT binding defined for
> the connection between the Ethernet device node and the PTP device node?
> 
Correct. This driver doesn't need any phandle.

The Ethernet driver, in SoC's where the hardware link is not available,
might need the phandle to this driver.

>> In other scenarios, we use an user-space app to perform the sync based on
>> time-stamps from external hardware sources like the Wi-Fi.
>>> Thanks,
>>> Richard
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
>  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.

> +- reg: address and length of the DTE block's NCO registers
> +
> +Example:
> +
> +ptp_dte: ptp_dte@180af650 {

Don't use '_' in node names.

> +	compatible = "brcm,ptp-dte";
> +	reg = <0x180af650 0x10>;
> +	status = "okay";
> +};
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arun Parameswaran]

Hi David, Rob,
I will address all of Rob's comments below.

Since a part of the patch was applied to 'net-next', would you like me to
send a new patch (based on the applied one), or a 'V2' of this patch ?

Thanks
Arun

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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> ---
>>  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.
> 
>> +- reg: address and length of the DTE block's NCO registers
>> +
>> +Example:
>> +
>> +ptp_dte: ptp_dte@180af650 {
> 
> Don't use '_' in node names.
> 
>> +	compatible = "brcm,ptp-dte";
>> +	reg = <0x180af650 0x10>;
>> +	status = "okay";
>> +};
>> -- 
>> 1.9.1
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Scott Branden]

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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> ---
>>   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?

Please explain what I'm missing.  I see other drivers already following 
the of_machine_is_compatible
approach and it makes more sense to me than adding SoC specific 
compatible strings into every
driver.

Regards,
  Scott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Tue, Jun 20, 2017 at 3:48 PM, Scott Branden
<scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> ---
>>>   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.

>
> Please explain what I'm missing.  I see other drivers already following the
> of_machine_is_compatible
> approach and it makes more sense to me than adding SoC specific compatible
> strings into every
> driver.

If the IP blocks are all the same, you don't have to add them to any
drivers, just add fallback compatibles.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Scott Branden]

On 17-06-21 08:19 PM, Rob Herring wrote:
> On Tue, Jun 20, 2017 at 3:48 PM, Scott Branden
> <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>    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?

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.
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.
>> Please explain what I'm missing.  I see other drivers already following the
>> of_machine_is_compatible
>> approach and it makes more sense to me than adding SoC specific compatible
>> strings into every
>> driver.
> If the IP blocks are all the same, you don't have to add them to any
> drivers, just add fallback compatibles.

> Rob
Regards,
Scott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Fainelli]

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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>> ---
>>>>>    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.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

[Rob Herring]

On Tue, Jun 27, 2017 at 12:10 AM, Scott Branden
<scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>> ---
>>>>>>>     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".

Unless you have strong version control of IP versions, version numbers
aren't generally reliable. Also, in my experience doing time at Si
companies, h/w designers can't be trusted regarding what changed or
not.

There are some exceptions. We accept version numbers mainly for FPGA
IP as in that case, the FPGA vendor is making releases of IP with end
users picking versions and the version numbering is well defined. v1
and v2 does not look well defined to me. What happens when you have an
ECO fix for example?

> 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?

How is a static table with driver data per compatible the same as coding:

if (of_machine_is_compatible("soc1"))
  ...
else if (of_machine_is_compatible("soc2"))
  ...
else if (of_machine_is_compatible("soc3") || of_machine_is_compatible("soc4"))
 ...
and so on...

As I mentioned previously, we don't want this mess in drivers.

The static table matching guarantees that you don't have to update the
driver for a new SoC when a block is "exactly the same". Just have the
existing block compatible string as a fallback compatible. If you have
something like above, then you may have to update the driver depending
on how the logic is written. For example, if you add soc5 which has
the same IP block as soc1, then you have to update the driver unless
soc5 can claim to be compatible with soc1 at the top-level (which only
works when the new SoC is a pure superset).

> 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?

The documentation changes should be oneliners.

Why do you need dts changes? The dts for a new SoC should be correct
from the start with the new compatibles and appropriate fallback
compatibles.

The bottom line is this is how DT was designed to work and is what
everyone else follows. Broadcom is not special.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Scott Branden]

Hi Rob,


On 17-06-29 03:40 PM, Rob Herring wrote:
> On Tue, Jun 27, 2017 at 12:10 AM, Scott Branden
> <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>>> ---
>>>>>>>>      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".
> Unless you have strong version control of IP versions, version numbers
> aren't generally reliable. Also, in my experience doing time at Si
> companies, h/w designers can't be trusted regarding what changed or
> not.
>
> There are some exceptions. We accept version numbers mainly for FPGA
> IP as in that case, the FPGA vendor is making releases of IP with end
> users picking versions and the version numbering is well defined. v1
> and v2 does not look well defined to me. What happens when you have an
> ECO fix for example?
When you have an ECO fix that would be a new version.  So v1.1, 1.2, etc.
Appropriate compatibility string would be:
"brcm, dte-ptp-v1.1", "brcm, dte-ptp-v1"

What else would you like us to do in this case?
Do you want us to add Soc specific revision compatibility strings?
>> 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?
> How is a static table with driver data per compatible the same as coding:
The static table with driver data is used with the compatibility string
"brcm, dte-ptp-v1", etc.  It is only the odd case where machine specific
is used to handle a bug (due to silicon designers lying to us that the 
version was the same
or they integrated something differently somehow).
> if (of_machine_is_compatible("soc1"))
>    ...
> else if (of_machine_is_compatible("soc2"))
>    ...
> else if (of_machine_is_compatible("soc3") || of_machine_is_compatible("soc4"))
>   ...
> and so on...
>
> As I mentioned previously, we don't want this mess in drivers.
Investigating drivers:
I see the use of of_machine_is_compatible to handle SoC specific cases.
I see the use of of_device_is_compatible to handle some SoC specific 
cases as well.
If there is a bug discovered that needs to be handle in a driver for an 
SoC specific case then code
needs to be added that would use either of those 2 checks to handle it.

Based on that - I don't see the benefit of adding the soc specific 
compatibility strings in the dts
files.

if (of_machine_is_compatible("brcm,soc1")
seems much cleaner than:
if (of_device_is_compatible("brcm,soc1-ptp-dte")

I see no mess in that and I see both cases of if statements in use today 
in various drivers.

If you really have a problem with the existing drivers calling either of 
the above 2 functions,
then adding the machine into the lookup of your data driven table would 
be a cleaner solution than
adding all these soc specific compatibility strings?  This would also solve
all the inconsistencies present today.
> The static table matching guarantees that you don't have to update the
> driver for a new SoC when a block is "exactly the same". Just have the
> existing block compatible string as a fallback compatible. If you have
> something like above, then you may have to update the driver depending
> on how the logic is written. For example, if you add soc5 which has
> the same IP block as soc1, then you have to update the driver unless
> soc5 can claim to be compatible with soc1 at the top-level (which only
> works when the new SoC is a pure superset).
>
>> 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?
> The documentation changes should be oneliners.
>
> Why do you need dts changes? The dts for a new SoC should be correct
> from the start with the new compatibles and appropriate fallback
> compatibles.
> The bottom line is this is how DT was designed to work and is what
> everyone else follows. Broadcom is not special.
Could you please point me at the design your are referring to?
I have been searching for details on the use of the fallback string and 
have yet to find it documented.
There really is no consistency.  That is what I'm trying to understand 
here.  Broadcom is not special.

I would like to have the proper understanding as devicetree is a loose 
mess in general.

Also, I find many places where dts files
are using the same compatibility strings for different SoCs because they 
use the same IP block.
People are not create fallback compatibility per SoC everywhere.
> Rob
Regards,
   Scott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/2] ptp: Add a ptp clock driver for Broadcom DTE

[Arun Parameswaran]

This patch adds a ptp clock driver for the Broadcom SoCs using
the Digital timing Engine (DTE) nco.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/ptp/Kconfig   |  16 +++
 drivers/ptp/Makefile  |   1 +
 drivers/ptp/ptp_dte.c | 353 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 370 insertions(+)
 create mode 100644 drivers/ptp/ptp_dte.c

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 384f661..a21ad10 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -25,6 +25,22 @@ config PTP_1588_CLOCK
 	  To compile this driver as a module, choose M here: the module
 	  will be called ptp.
 
+config PTP_1588_CLOCK_DTE
+	tristate "Broadcom DTE as PTP clock"
+	depends on PTP_1588_CLOCK
+	depends on NET && HAS_IOMEM
+	depends on ARCH_BCM_MOBILE || (ARCH_BCM_IPROC && !(ARCH_BCM_NSP || ARCH_BCM_5301X)) || COMPILE_TEST
+	default y
+	help
+	  This driver adds support for using the Digital timing engine
+	  (DTE) in the Broadcom SoC's as a PTP clock.
+
+	  The clock can be used in both wired and wireless networks
+	  for PTP purposes.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ptp_dte.
+
 config PTP_1588_CLOCK_GIANFAR
 	tristate "Freescale eTSEC as PTP clock"
 	depends on GIANFAR
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 5307361..d1f2fb1 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -4,6 +4,7 @@
 
 ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o
 obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
+obj-$(CONFIG_PTP_1588_CLOCK_DTE)	+= ptp_dte.o
 obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
 obj-$(CONFIG_PTP_1588_CLOCK_PCH)	+= ptp_pch.o
 obj-$(CONFIG_PTP_1588_CLOCK_KVM)	+= ptp_kvm.o
diff --git a/drivers/ptp/ptp_dte.c b/drivers/ptp/ptp_dte.c
new file mode 100644
index 0000000..00145a3
--- /dev/null
+++ b/drivers/ptp/ptp_dte.c
@@ -0,0 +1,353 @@
+/*
+ * Copyright 2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/types.h>
+
+#define DTE_NCO_LOW_TIME_REG	0x00
+#define DTE_NCO_TIME_REG	0x04
+#define DTE_NCO_OVERFLOW_REG	0x08
+#define DTE_NCO_INC_REG		0x0c
+
+#define DTE_NCO_SUM2_MASK	0xffffffff
+#define DTE_NCO_SUM2_SHIFT	4ULL
+
+#define DTE_NCO_SUM3_MASK	0xff
+#define DTE_NCO_SUM3_SHIFT	36ULL
+#define DTE_NCO_SUM3_WR_SHIFT	8
+
+#define DTE_NCO_TS_WRAP_MASK	0xfff
+#define DTE_NCO_TS_WRAP_LSHIFT	32
+
+#define DTE_NCO_INC_DEFAULT	0x80000000
+#define DTE_NUM_REGS_TO_RESTORE	4
+
+/* Full wrap around is 44bits in ns (~4.887 hrs) */
+#define DTE_WRAP_AROUND_NSEC_SHIFT 44
+
+/* 44 bits NCO */
+#define DTE_NCO_MAX_NS	0xFFFFFFFFFFF
+
+/* 125MHz with 3.29 reg cfg */
+#define DTE_PPB_ADJ(ppb) (u32)(div64_u64((((u64)abs(ppb) * BIT(28)) +\
+				      62500000ULL), 125000000ULL))
+
+/* ptp dte priv structure */
+struct ptp_dte {
+	void __iomem *regs;
+	struct ptp_clock *ptp_clk;
+	struct ptp_clock_info caps;
+	struct device *dev;
+	u32 ts_ovf_last;
+	u32 ts_wrap_cnt;
+	spinlock_t lock;
+	u32 reg_val[DTE_NUM_REGS_TO_RESTORE];
+};
+
+static void dte_write_nco(void __iomem *regs, s64 ns)
+{
+	u32 sum2, sum3;
+
+	sum2 = (u32)((ns >> DTE_NCO_SUM2_SHIFT) & DTE_NCO_SUM2_MASK);
+	/* compensate for ignoring sum1 */
+	if (sum2 != DTE_NCO_SUM2_MASK)
+		sum2++;
+
+	/* to write sum3, bits [15:8] needs to be written */
+	sum3 = (u32)(((ns >> DTE_NCO_SUM3_SHIFT) & DTE_NCO_SUM3_MASK) <<
+		     DTE_NCO_SUM3_WR_SHIFT);
+
+	writel(0, (regs + DTE_NCO_LOW_TIME_REG));
+	writel(sum2, (regs + DTE_NCO_TIME_REG));
+	writel(sum3, (regs + DTE_NCO_OVERFLOW_REG));
+}
+
+static s64 dte_read_nco(void __iomem *regs)
+{
+	u32 sum2, sum3;
+	s64 ns;
+
+	/*
+	 * ignoring sum1 (4 bits) gives a 16ns resolution, which
+	 * works due to the async register read.
+	 */
+	sum3 = readl(regs + DTE_NCO_OVERFLOW_REG) & DTE_NCO_SUM3_MASK;
+	sum2 = readl(regs + DTE_NCO_TIME_REG);
+	ns = ((s64)sum3 << DTE_NCO_SUM3_SHIFT) |
+		 ((s64)sum2 << DTE_NCO_SUM2_SHIFT);
+
+	return ns;
+}
+
+static void dte_write_nco_delta(struct ptp_dte *ptp_dte, s64 delta)
+{
+	s64 ns;
+
+	ns = dte_read_nco(ptp_dte->regs);
+
+	/* handle wraparound conditions */
+	if ((delta < 0) && (abs(delta) > ns)) {
+		if (ptp_dte->ts_wrap_cnt) {
+			ns += DTE_NCO_MAX_NS + delta;
+			ptp_dte->ts_wrap_cnt--;
+		} else {
+			ns = 0;
+		}
+	} else {
+		ns += delta;
+		if (ns > DTE_NCO_MAX_NS) {
+			ptp_dte->ts_wrap_cnt++;
+			ns -= DTE_NCO_MAX_NS;
+		}
+	}
+
+	dte_write_nco(ptp_dte->regs, ns);
+
+	ptp_dte->ts_ovf_last = (ns >> DTE_NCO_TS_WRAP_LSHIFT) &
+			DTE_NCO_TS_WRAP_MASK;
+}
+
+static s64 dte_read_nco_with_ovf(struct ptp_dte *ptp_dte)
+{
+	u32 ts_ovf;
+	s64 ns = 0;
+
+	ns = dte_read_nco(ptp_dte->regs);
+
+	/*Timestamp overflow: 8 LSB bits of sum3, 4 MSB bits of sum2 */
+	ts_ovf = (ns >> DTE_NCO_TS_WRAP_LSHIFT) & DTE_NCO_TS_WRAP_MASK;
+
+	/* Check for wrap around */
+	if (ts_ovf < ptp_dte->ts_ovf_last)
+		ptp_dte->ts_wrap_cnt++;
+
+	ptp_dte->ts_ovf_last = ts_ovf;
+
+	/* adjust for wraparounds */
+	ns += (s64)(BIT_ULL(DTE_WRAP_AROUND_NSEC_SHIFT) * ptp_dte->ts_wrap_cnt);
+
+	return ns;
+}
+
+static int ptp_dte_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	u32 nco_incr;
+	unsigned long flags;
+	struct ptp_dte *ptp_dte = container_of(ptp, struct ptp_dte, caps);
+
+	if (abs(ppb) > ptp_dte->caps.max_adj) {
+		dev_err(ptp_dte->dev, "ppb adj too big\n");
+		return -EINVAL;
+	}
+
+	if (ppb < 0)
+		nco_incr = DTE_NCO_INC_DEFAULT - DTE_PPB_ADJ(ppb);
+	else
+		nco_incr = DTE_NCO_INC_DEFAULT + DTE_PPB_ADJ(ppb);
+
+	spin_lock_irqsave(&ptp_dte->lock, flags);
+	writel(nco_incr, ptp_dte->regs + DTE_NCO_INC_REG);
+	spin_unlock_irqrestore(&ptp_dte->lock, flags);
+
+	return 0;
+}
+
+static int ptp_dte_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	unsigned long flags;
+	struct ptp_dte *ptp_dte = container_of(ptp, struct ptp_dte, caps);
+
+	spin_lock_irqsave(&ptp_dte->lock, flags);
+	dte_write_nco_delta(ptp_dte, delta);
+	spin_unlock_irqrestore(&ptp_dte->lock, flags);
+
+	return 0;
+}
+
+static int ptp_dte_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+	unsigned long flags;
+	struct ptp_dte *ptp_dte = container_of(ptp, struct ptp_dte, caps);
+
+	spin_lock_irqsave(&ptp_dte->lock, flags);
+	*ts = ns_to_timespec64(dte_read_nco_with_ovf(ptp_dte));
+	spin_unlock_irqrestore(&ptp_dte->lock, flags);
+
+	return 0;
+}
+
+static int ptp_dte_settime(struct ptp_clock_info *ptp,
+			     const struct timespec64 *ts)
+{
+	unsigned long flags;
+	struct ptp_dte *ptp_dte = container_of(ptp, struct ptp_dte, caps);
+
+	spin_lock_irqsave(&ptp_dte->lock, flags);
+
+	/* Disable nco increment */
+	writel(0, ptp_dte->regs + DTE_NCO_INC_REG);
+
+	dte_write_nco(ptp_dte->regs, timespec64_to_ns(ts));
+
+	/* reset overflow and wrap counter */
+	ptp_dte->ts_ovf_last = 0;
+	ptp_dte->ts_wrap_cnt = 0;
+
+	/* Enable nco increment */
+	writel(DTE_NCO_INC_DEFAULT, ptp_dte->regs + DTE_NCO_INC_REG);
+
+	spin_unlock_irqrestore(&ptp_dte->lock, flags);
+
+	return 0;
+}
+
+static int ptp_dte_enable(struct ptp_clock_info *ptp,
+			    struct ptp_clock_request *rq, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static struct ptp_clock_info ptp_dte_caps = {
+	.owner		= THIS_MODULE,
+	.name		= "DTE PTP timer",
+	.max_adj	= 50000000,
+	.n_ext_ts	= 0,
+	.n_pins		= 0,
+	.pps		= 0,
+	.adjfreq	= ptp_dte_adjfreq,
+	.adjtime	= ptp_dte_adjtime,
+	.gettime64	= ptp_dte_gettime,
+	.settime64	= ptp_dte_settime,
+	.enable		= ptp_dte_enable,
+};
+
+static int ptp_dte_probe(struct platform_device *pdev)
+{
+	struct ptp_dte *ptp_dte;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+
+	ptp_dte = devm_kzalloc(dev, sizeof(struct ptp_dte), GFP_KERNEL);
+	if (!ptp_dte)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ptp_dte->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ptp_dte->regs)) {
+		dev_err(dev,
+			"%s: io remap failed\n", __func__);
+		return PTR_ERR(ptp_dte->regs);
+	}
+
+	spin_lock_init(&ptp_dte->lock);
+
+	ptp_dte->dev = dev;
+	ptp_dte->caps = ptp_dte_caps;
+	ptp_dte->ptp_clk = ptp_clock_register(&ptp_dte->caps, &pdev->dev);
+	if (IS_ERR(ptp_dte->ptp_clk)) {
+		dev_err(dev,
+			"%s: Failed to register ptp clock\n", __func__);
+		return PTR_ERR(ptp_dte->ptp_clk);
+	}
+
+	platform_set_drvdata(pdev, ptp_dte);
+
+	dev_info(dev, "ptp clk probe done\n");
+
+	return 0;
+}
+
+static int ptp_dte_remove(struct platform_device *pdev)
+{
+	struct ptp_dte *ptp_dte = platform_get_drvdata(pdev);
+	u8 i;
+
+	ptp_clock_unregister(ptp_dte->ptp_clk);
+
+	for (i = 0; i < DTE_NUM_REGS_TO_RESTORE; i++)
+		writel(0, ptp_dte->regs + (i * sizeof(u32)));
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ptp_dte_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ptp_dte *ptp_dte = platform_get_drvdata(pdev);
+	u8 i;
+
+	for (i = 0; i < DTE_NUM_REGS_TO_RESTORE; i++) {
+		ptp_dte->reg_val[i] =
+			readl(ptp_dte->regs + (i * sizeof(u32)));
+	}
+
+	/* disable the nco */
+	writel(0, ptp_dte->regs + DTE_NCO_INC_REG);
+
+	return 0;
+}
+
+static int ptp_dte_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ptp_dte *ptp_dte = platform_get_drvdata(pdev);
+	u8 i;
+
+	for (i = 0; i < DTE_NUM_REGS_TO_RESTORE; i++) {
+		if ((i * sizeof(u32)) != DTE_NCO_OVERFLOW_REG)
+			writel(ptp_dte->reg_val[i],
+				(ptp_dte->regs + (i * sizeof(u32))));
+		else
+			writel(((ptp_dte->reg_val[i] &
+				DTE_NCO_SUM3_MASK) << DTE_NCO_SUM3_WR_SHIFT),
+				(ptp_dte->regs + (i * sizeof(u32))));
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops ptp_dte_pm_ops = {
+	.suspend = ptp_dte_suspend,
+	.resume = ptp_dte_resume
+};
+
+#define PTP_DTE_PM_OPS	(&ptp_dte_pm_ops)
+#else
+#define PTP_DTE_PM_OPS	NULL
+#endif
+
+static const struct of_device_id ptp_dte_of_match[] = {
+	{ .compatible = "brcm,ptp-dte", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ptp_dte_of_match);
+
+static struct platform_driver ptp_dte_driver = {
+	.driver = {
+		.name = "ptp-dte",
+		.pm = PTP_DTE_PM_OPS,
+		.of_match_table = ptp_dte_of_match,
+	},
+	.probe    = ptp_dte_probe,
+	.remove   = ptp_dte_remove,
+};
+module_platform_driver(ptp_dte_driver);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Broadcom DTE PTP Clock driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v1 0/2] Add support for Broadcom DTE based PTP clock

[David Miller]

From: Arun Parameswaran <arun.parameswaran-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Date: Mon, 12 Jun 2017 13:25:59 -0700

> This patchset adds support for the DTE based PTP clock for Broadcom SoCs.
> 
> The DTE nco based PTP clock can be used in both wired and wireless networks
> for precision time-stmaping purposes.

Series applied to net-next, thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html