Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-03-17 Thread Grant Likely 
On Fri, Feb 25, 2011 at 08:53:20AM +0100, Richard Cochran wrote:
 On Thu, Feb 24, 2011 at 11:27:31AM -0600, Scott Wood wrote:
 
  My vote, if it goes in a separate node at all, is fsl,etsec-ptp,
 
 So, that is what the patch does.
 
  and let the driver use SVR.
 
 What is SVR?

An on chip register that provides the exact version of the SoC.  (As
opposed to PVR which is the version of the cpu core).

g.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-24 Thread Richard Cochran 
On Wed, Feb 23, 2011 at 10:54:59AM -0700, Grant Likely wrote:
 On Wed, Feb 23, 2011 at 11:26:12AM -0600, Scott Wood wrote:

  The eTSEC revision is probeable as well, but due the way PTP is described as
  a separate node, the driver doesn't have straightforward access to those
  registers.
 
 Ignorant question: Should the ptp be described as a separate node?

Well, the PTP Hardware Clock function is logically separate from the
MAC function. PHCs can be implemented in the MAC, in the PHY, or in
between in an FPGA on MII bus.

If the PHC is in the MAC, then it might be wise to implement one
driver that offers both the MAC and the PHC.

In the case of gianfar, it is not really necessary to combine the PHC
into the gianfar driver, since the registers are pretty well
separated. Also, given the size and complexity (and churn over time)
of the gianfar driver, I decided to keep the PHC separate.

Right now, the driver correctly handles all the clock revisions in the
boards that I have (mpc8313, mpc8572, p2020ds, p2020rdb).

If checking the revision becomes important, then we can always export
a function from gianfar to provide this.

Thanks,

Richard
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-24 Thread Richard Cochran 
On Wed, Feb 23, 2011 at 01:24:44PM -0600, Scott Wood wrote:
 Whatever string is used should be written into a binding document.
 
 fsl,etsec-v1.6-ptp seems like it would be just as good for that purpose.
 
 Even just fsl,etsec-ptp will identify the binding, though it's lacking in
 identifying the hardware (in the absence of access to the eTSEC ID
 registers).

I read the conversation, and I don't mind admitting that I do not
understand what you both are arguing/discussing about.

How should I set the strings?  Like this?

arch/powerpc/boot/dts/mpc8313erdb.dts:
ptp_clock@24E00 {
compatible = fsl,mpc8313-etsec-ptp;
}
arch/powerpc/boot/dts/mpc8572ds.dts:
ptp_clock@24E00 {
compatible = fsl,mpc8572-etsec-ptp;
} 
arch/powerpc/boot/dts/p2020ds.dts:
ptp_clock@24E00 {
compatible = fsl,p2020ds-etsec-ptp;
} 
arch/powerpc/boot/dts/p2020rdb.dts:
ptp_clock@24E00 {
compatible = fsl,p2020rdb-etsec-ptp;
} 

drivers/net/gianfar_ptp.c:

static struct of_device_id match_table[] = {
{ .compatible = fsl,mpc8313-etsec-ptp },
{ .compatible = fsl,mpc8572-etsec-ptp },
{ .compatible = fsl,p2020ds-etsec-ptp },
{ .compatible = fsl,p2020rdb-etsec-ptp },
{},
};

Please let me know if this is what you meant.

Thanks,
Richard
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-24 Thread Scott Wood 
On Thu, 24 Feb 2011 17:39:44 +0100
Richard Cochran richardcoch...@gmail.com wrote:

 On Wed, Feb 23, 2011 at 10:54:59AM -0700, Grant Likely wrote:
  On Wed, Feb 23, 2011 at 11:26:12AM -0600, Scott Wood wrote:
 
   The eTSEC revision is probeable as well, but due the way PTP is described 
   as
   a separate node, the driver doesn't have straightforward access to those
   registers.
  
  Ignorant question: Should the ptp be described as a separate node?
 
 Well, the PTP Hardware Clock function is logically separate from the
 MAC function.

The eTSEC node doesn't describe the MAC function, it describes the whole
device (or at least it should... we make an exception for MDIO, which
should probably have been a subnode instead).

 PHCs can be implemented in the MAC, in the PHY, or in
 between in an FPGA on MII bus.
 
 If the PHC is in the MAC, then it might be wise to implement one
 driver that offers both the MAC and the PHC.
 
 In the case of gianfar, it is not really necessary to combine the PHC
 into the gianfar driver, since the registers are pretty well
 separated.

How the drivers are structured in Linux is a separate concern from how the
devices are described in the device tree.  The tree is supposed to be an
OS-independent representation of hardware.

If Linux has multiple drivers that correspond to portions of one node, a
toplevel driver can register platform devices for the components, adding
in any additional information like versioning that it gets from the
toplevel registers.

-Scott

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-24 Thread Richard Cochran 
On Wed, Feb 23, 2011 at 09:50:58AM -0700, Grant Likely wrote:
 On Wed, Feb 23, 2011 at 11:38:17AM +0100, Richard Cochran wrote:
  +Clock Properties:
  +
  +  - tclk-period  Timer reference clock period in nanoseconds.
  +  - tmr-prsc Prescaler, divides the output clock.
  +  - tmr-add  Frequency compensation value.
  +  - cksel0= external clock, 1= eTSEC system clock, 3= RTC clock 
  input.
  + Currently the driver only supports choice 1.
 
 I'd be hesitant about defining something that isn't actually
 implemented yet.  You may find the binding to be insufficient at a
 later date.

Okay, I'll remove it.
We never got the external VCO working anyhow.

  +  - tmr-fiper1   Fixed interval period pulse generator.
  +  - tmr-fiper2   Fixed interval period pulse generator.
  +  - max-adj  Maximum frequency adjustment in parts per billion.
 
 These are all custom properties (not part of any shared binding) so
 they should probably be prefixed with 'fsl,'.

Okay, fine.

  +  The calculation for tmr_fiper2 is the same as for tmr_fiper1. The
  +  driver expects that tmr_fiper1 will be correctly set to produce a 1
  +  Pulse Per Second (PPS) signal, since this will be offered to the PPS
  +  subsystem to synchronize the Linux clock.
 
 Good documentation, thanks.  Question though, how many of these values
 will the end user (or board builder) be likely to want to change.  It
 is risky encoding the calculation results into the device tree when
 they aren't the actually parameters that will be manipulated, or at
 least very user-unfriendly.

The whole thing is pretty opaque, and my explanation is (IMHO) way
better that Freescale's documentation of how the fipers work.

The board designer / system designer will want to set these carefully,
but never change them. Basically, for a given input clock, there is
only one optimal setting.

I think the device tree is the right place for that kind of setting.

The fiper1 signal should always be a 1 PPS.  We could make fiper2 run
time programmable via PHC ioctls, but I think this can wait.


  +   etsects-irq = irq_of_parse_and_map(node, 0);
 
 Use platform_get_irq().

Okay.

  +   etsects-regs = of_iomap(node, 0);
 
 Use platform_get_resource(), and don't forget to request the
 resources.

Okay, but didn't you tell me before to do this way?

   http://marc.info/?l=linux-netdevm=127662247203659w=4

  +static struct of_platform_driver gianfar_ptp_driver = {
 
 Use a platform_driver instead.  of_platform_driver is deprecated and
 being removed.

Ja, should have noticed that myself, sorry.

  +++ b/drivers/net/gianfar_ptp_reg.h
 
 This data is only used by gianfar_ptp.c, so there is no need for a
 separate include file.  Move the contents of gianfar_ptp_reg.h into
 gianfar_ptp.c

You are right, of course, since private #defines and declarations
should simply stay in their .c files. Some people think that all
#defines and declarations must go into a header file.

I am not one of those people, but in this case, I generated the file
from a little tool I wrote and so kept it separate.

Still, it is no trouble to combine the header into the driver .c file.

Thanks for your review,

Richard
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-24 Thread Scott Wood 
On Thu, 24 Feb 2011 17:50:04 +0100
Richard Cochran richardcoch...@gmail.com wrote:

 On Wed, Feb 23, 2011 at 01:24:44PM -0600, Scott Wood wrote:
  Whatever string is used should be written into a binding document.
  
  fsl,etsec-v1.6-ptp seems like it would be just as good for that purpose.
  
  Even just fsl,etsec-ptp will identify the binding, though it's lacking in
  identifying the hardware (in the absence of access to the eTSEC ID
  registers).
 
 I read the conversation, and I don't mind admitting that I do not
 understand what you both are arguing/discussing about.
 
 How should I set the strings?  Like this?
 
 arch/powerpc/boot/dts/mpc8313erdb.dts:
   ptp_clock@24E00 {
   compatible = fsl,mpc8313-etsec-ptp;
   }
 arch/powerpc/boot/dts/mpc8572ds.dts:
   ptp_clock@24E00 {
   compatible = fsl,mpc8572-etsec-ptp;
   } 
 arch/powerpc/boot/dts/p2020ds.dts:
   ptp_clock@24E00 {
   compatible = fsl,p2020ds-etsec-ptp;
   } 
 arch/powerpc/boot/dts/p2020rdb.dts:
   ptp_clock@24E00 {
   compatible = fsl,p2020rdb-etsec-ptp;
   } 
 
 drivers/net/gianfar_ptp.c:
 
 static struct of_device_id match_table[] = {
   { .compatible = fsl,mpc8313-etsec-ptp },
   { .compatible = fsl,mpc8572-etsec-ptp },
   { .compatible = fsl,p2020ds-etsec-ptp },
   { .compatible = fsl,p2020rdb-etsec-ptp },
   {},
 };

Those last two are boards, not chips.  I don't think even Grant is asking
to take things that far.

My vote, if it goes in a separate node at all, is fsl,etsec-ptp, and let
the driver use SVR.  Even encoding an etsec version in the compatible
string would be difficult, unless fixed up by u-boot, as it appears to
differ based on chip revision (and the chip manuals seem to often not match
the hardware regarding the advertised eTSEC revision) and we don't normally
have separate dts files for different revisions of the same chip.  Plus,
our docs (at least the public ones) don't seem to be very helpful in
determining what version of eTSEC implies what.

If you want to use chip-based compatibles instead, then use the actual name
of the chip.  You'll need to verify 100% compatibility if you want to claim
compatibility with another chip; it's probably easier/safer to just list
every single Freescale chip that has this type of PTP in a huge compatible
table, like PCI drivers do.

-Scott

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-24 Thread Richard Cochran 
On Thu, Feb 24, 2011 at 11:27:31AM -0600, Scott Wood wrote:

 My vote, if it goes in a separate node at all, is fsl,etsec-ptp,

So, that is what the patch does.

 and let the driver use SVR.

What is SVR?

Thanks,
Richard
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-23 Thread Richard Cochran 
The eTSEC includes a PTP clock with quite a few features. This patch adds
support for the basic clock adjustment functions, plus two external time
stamps, one alarm, and the PPS callback.

Signed-off-by: Richard Cochran richard.coch...@omicron.at
Acked-by: John Stultz johns...@us.ibm.com
---
 .../devicetree/bindings/net/fsl-tsec-phy.txt   |   57 +++
 arch/powerpc/boot/dts/mpc8313erdb.dts  |   14 +
 arch/powerpc/boot/dts/mpc8572ds.dts|   14 +
 arch/powerpc/boot/dts/p2020ds.dts  |   14 +
 arch/powerpc/boot/dts/p2020rdb.dts |   14 +
 drivers/net/Makefile   |1 +
 drivers/net/gianfar_ptp.c  |  448 
 drivers/net/gianfar_ptp_reg.h  |  113 +
 drivers/ptp/Kconfig|   13 +
 9 files changed, 688 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/gianfar_ptp.c
 create mode 100644 drivers/net/gianfar_ptp_reg.h

diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt 
b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
index edb7ae1..f6edbb8 100644
--- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
+++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
@@ -74,3 +74,60 @@ Example:
interrupt-parent = mpic;
phy-handle = phy0
};
+
+* Gianfar PTP clock nodes
+
+General Properties:
+
+  - compatible   Should be fsl,etsec-ptp
+  - reg  Offset and length of the register set for the device
+  - interrupts   There should be at least two interrupts. Some devices
+ have as many as four PTP related interrupts.
+
+Clock Properties:
+
+  - tclk-period  Timer reference clock period in nanoseconds.
+  - tmr-prsc Prescaler, divides the output clock.
+  - tmr-add  Frequency compensation value.
+  - cksel0= external clock, 1= eTSEC system clock, 3= RTC clock input.
+ Currently the driver only supports choice 1.
+  - tmr-fiper1   Fixed interval period pulse generator.
+  - tmr-fiper2   Fixed interval period pulse generator.
+  - max-adj  Maximum frequency adjustment in parts per billion.
+
+  These properties set the operational parameters for the PTP
+  clock. You must choose these carefully for the clock to work right.
+  Here is how to figure good values:
+
+  TimerOsc = system clock   MHz
+  tclk_period  = desired clock period   nanoseconds
+  NominalFreq  = 1000 / tclk_period MHz
+  FreqDivRatio = TimerOsc / NominalFreq (must be greater that 1.0)
+  tmr_add  = ceil(2^32 / FreqDivRatio)
+  OutputClock  = NominalFreq / tmr_prsc MHz
+  PulseWidth   = 1 / OutputClockmicroseconds
+  FiperFreq1   = desired frequency in Hz
+  FiperDiv1= 100 * OutputClock / FiperFreq1
+  tmr_fiper1   = tmr_prsc * tclk_period * FiperDiv1 - tclk_period
+  max_adj  = 10 * (FreqDivRatio - 1.0) - 1
+
+  The calculation for tmr_fiper2 is the same as for tmr_fiper1. The
+  driver expects that tmr_fiper1 will be correctly set to produce a 1
+  Pulse Per Second (PPS) signal, since this will be offered to the PPS
+  subsystem to synchronize the Linux clock.
+
+Example:
+
+   ptp_clock@24E00 {
+   compatible = fsl,etsec-ptp;
+   reg = 0x24E00 0xB0;
+   interrupts = 12 0x8 13 0x8;
+   interrupt-parent =  ipic ;
+   tclk-period = 10;
+   tmr-prsc= 100;
+   tmr-add = 0x99A4;
+   cksel   = 0x1;
+   tmr-fiper1  = 0x3B9AC9F6;
+   tmr-fiper2  = 0x00018696;
+   max-adj = 65998;
+   };
diff --git a/arch/powerpc/boot/dts/mpc8313erdb.dts 
b/arch/powerpc/boot/dts/mpc8313erdb.dts
index 183f2aa..85a7eaa 100644
--- a/arch/powerpc/boot/dts/mpc8313erdb.dts
+++ b/arch/powerpc/boot/dts/mpc8313erdb.dts
@@ -208,6 +208,20 @@
sleep = pmc 0x0030;
};
 
+   ptp_clock@24E00 {
+   compatible = fsl,etsec-ptp;
+   reg = 0x24E00 0xB0;
+   interrupts = 12 0x8 13 0x8;
+   interrupt-parent =  ipic ;
+   tclk-period = 10;
+   tmr-prsc= 100;
+   tmr-add = 0x99A4;
+   cksel   = 0x1;
+   tmr-fiper1  = 0x3B9AC9F6;
+   tmr-fiper2  = 0x00018696;
+   max-adj = 65998;
+   };
+
enet0: ethernet@24000 {
#address-cells = 1;
#size-cells = 1;
diff --git a/arch/powerpc/boot/dts/mpc8572ds.dts 
b/arch/powerpc/boot/dts/mpc8572ds.dts
index cafc128..74208cd 100644
--- a/arch/powerpc/boot/dts/mpc8572ds.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds.dts
@@ -324,6 +324,20 @@

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-23 Thread Grant Likely 
On Wed, Feb 23, 2011 at 11:38:17AM +0100, Richard Cochran wrote:
 The eTSEC includes a PTP clock with quite a few features. This patch adds
 support for the basic clock adjustment functions, plus two external time
 stamps, one alarm, and the PPS callback.
 
 Signed-off-by: Richard Cochran richard.coch...@omicron.at
 Acked-by: John Stultz johns...@us.ibm.com
 ---
  .../devicetree/bindings/net/fsl-tsec-phy.txt   |   57 +++
  arch/powerpc/boot/dts/mpc8313erdb.dts  |   14 +
  arch/powerpc/boot/dts/mpc8572ds.dts|   14 +
  arch/powerpc/boot/dts/p2020ds.dts  |   14 +
  arch/powerpc/boot/dts/p2020rdb.dts |   14 +
  drivers/net/Makefile   |1 +
  drivers/net/gianfar_ptp.c  |  448 
 
  drivers/net/gianfar_ptp_reg.h  |  113 +
  drivers/ptp/Kconfig|   13 +
  9 files changed, 688 insertions(+), 0 deletions(-)
  create mode 100644 drivers/net/gianfar_ptp.c
  create mode 100644 drivers/net/gianfar_ptp_reg.h
 
 diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt 
 b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
 index edb7ae1..f6edbb8 100644
 --- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
 +++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
 @@ -74,3 +74,60 @@ Example:
   interrupt-parent = mpic;
   phy-handle = phy0
   };
 +
 +* Gianfar PTP clock nodes
 +
 +General Properties:
 +
 +  - compatible   Should be fsl,etsec-ptp

Should specify an *exact* part; ie: fsl,mpc8313-etsec-ptp instead of
trying to define a generic catchall.  The reason is that the same
marketing name can end up getting applied to a wide range of parts.

Instead, choose one specific device to stand in as the 'common'
implementation and get all parts with the same core to claim
compatibility with it.  ie: a p2020 might have:

compatible = fsl,mpc2020-etsec-ptp, fsl,mpc8313-etsec-ptp;

 +  - reg  Offset and length of the register set for the device
 +  - interrupts   There should be at least two interrupts. Some devices
 + have as many as four PTP related interrupts.
 +
 +Clock Properties:
 +
 +  - tclk-period  Timer reference clock period in nanoseconds.
 +  - tmr-prsc Prescaler, divides the output clock.
 +  - tmr-add  Frequency compensation value.
 +  - cksel0= external clock, 1= eTSEC system clock, 3= RTC clock 
 input.
 + Currently the driver only supports choice 1.

I'd be hesitant about defining something that isn't actually
implemented yet.  You may find the binding to be insufficient at a
later date.

 +  - tmr-fiper1   Fixed interval period pulse generator.
 +  - tmr-fiper2   Fixed interval period pulse generator.
 +  - max-adj  Maximum frequency adjustment in parts per billion.

These are all custom properties (not part of any shared binding) so
they should probably be prefixed with 'fsl,'.

 +
 +  These properties set the operational parameters for the PTP
 +  clock. You must choose these carefully for the clock to work right.
 +  Here is how to figure good values:
 +
 +  TimerOsc = system clock   MHz
 +  tclk_period  = desired clock period   nanoseconds
 +  NominalFreq  = 1000 / tclk_period MHz
 +  FreqDivRatio = TimerOsc / NominalFreq (must be greater that 1.0)
 +  tmr_add  = ceil(2^32 / FreqDivRatio)
 +  OutputClock  = NominalFreq / tmr_prsc MHz
 +  PulseWidth   = 1 / OutputClockmicroseconds
 +  FiperFreq1   = desired frequency in Hz
 +  FiperDiv1= 100 * OutputClock / FiperFreq1
 +  tmr_fiper1   = tmr_prsc * tclk_period * FiperDiv1 - tclk_period
 +  max_adj  = 10 * (FreqDivRatio - 1.0) - 1
 +
 +  The calculation for tmr_fiper2 is the same as for tmr_fiper1. The
 +  driver expects that tmr_fiper1 will be correctly set to produce a 1
 +  Pulse Per Second (PPS) signal, since this will be offered to the PPS
 +  subsystem to synchronize the Linux clock.

Good documentation, thanks.  Question though, how many of these values
will the end user (or board builder) be likely to want to change.  It
is risky encoding the calculation results into the device tree when
they aren't the actually parameters that will be manipulated, or at
least very user-unfriendly.

 +
 +Example:
 +
 + ptp_clock@24E00 {
 + compatible = fsl,etsec-ptp;
 + reg = 0x24E00 0xB0;
 + interrupts = 12 0x8 13 0x8;
 + interrupt-parent =  ipic ;
 + tclk-period = 10;
 + tmr-prsc= 100;
 + tmr-add = 0x99A4;
 + cksel   = 0x1;
 + tmr-fiper1  = 0x3B9AC9F6;
 + tmr-fiper2  = 0x00018696;
 + max-adj = 65998;
 + };
 diff --git a/arch/powerpc/boot/dts/mpc8313erdb.dts 
 b/arch/powerpc/boot/dts/mpc8313erdb.dts
 index 183f2aa..85a7eaa

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-23 Thread Scott Wood 
On Wed, 23 Feb 2011 09:50:58 -0700
Grant Likely grant.lik...@secretlab.ca wrote:

 On Wed, Feb 23, 2011 at 11:38:17AM +0100, Richard Cochran wrote:
  +
  +* Gianfar PTP clock nodes
  +
  +General Properties:
  +
  +  - compatible   Should be fsl,etsec-ptp
 
 Should specify an *exact* part; ie: fsl,mpc8313-etsec-ptp instead of
 trying to define a generic catchall.  The reason is that the same
 marketing name can end up getting applied to a wide range of parts.
 
 Instead, choose one specific device to stand in as the 'common'
 implementation and get all parts with the same core to claim
 compatibility with it.  ie: a p2020 might have:
 
   compatible = fsl,mpc2020-etsec-ptp, fsl,mpc8313-etsec-ptp;

eTSEC is versioned, that's more reliable than the chip name since chips
have revisions (rev 2.1 of mpc8313 has eTSEC 1.6, not sure about previous
revs of mpc8313).  Logic blocks can be and have been uprevved between one
revision of a chip to the next.  I think fsl,mpc8313rev2.1-etsec-ptp
would be taking things a bit too far (and there could be board-level bugs
too...).

If you really need to know the exact SoC you're on, look in SVR (which
will provide revision info as well).  Isn't the device tree for things that
can't be probed?

The eTSEC revision is probeable as well, but due the way PTP is described as
a separate node, the driver doesn't have straightforward access to those
registers.

Insisting on an explicit chip also encourages people to claim compatibility
with that chip without ensuring that it really is fully compatible.

-Scott

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.

2011-02-23 Thread Scott Wood 
On Wed, 23 Feb 2011 10:54:59 -0700
Grant Likely grant.lik...@secretlab.ca wrote:

 On Wed, Feb 23, 2011 at 11:26:12AM -0600, Scott Wood wrote:
  eTSEC is versioned, that's more reliable than the chip name since chips
  have revisions (rev 2.1 of mpc8313 has eTSEC 1.6, not sure about previous
  revs of mpc8313).  Logic blocks can be and have been uprevved between one
  revision of a chip to the next.  I think fsl,mpc8313rev2.1-etsec-ptp
  would be taking things a bit too far (and there could be board-level bugs
  too...).
  
  If you really need to know the exact SoC you're on, look in SVR (which
  will provide revision info as well).  Isn't the device tree for things that
  can't be probed?
 
 This is far more about the binding than it is about the chip revision.
 When documenting a binding it makes far more sense to anchor it to a
 specific implementation than to try and come up with a 'generic'
 catchall.

Whatever string is used should be written into a binding document.

fsl,etsec-v1.6-ptp seems like it would be just as good for that purpose.

Even just fsl,etsec-ptp will identify the binding, though it's lacking in
identifying the hardware (in the absence of access to the eTSEC ID
registers).

If somehow Freescale makes something completely different in the future
called etsec-ptp, then we'll just have to pick a different name for
*that* compatible (after we smack whoever was responsible for reusing the
name).  The point of the vendor namespacing is to constrain this problem to
a manageable scope.

  The eTSEC revision is probeable as well, but due the way PTP is described as
  a separate node, the driver doesn't have straightforward access to those
  registers.
 
 Ignorant question: Should the ptp be described as a separate node?

Probably not.

  Insisting on an explicit chip also encourages people to claim compatibility
  with that chip without ensuring that it really is fully compatible.
 
 In practise, I've not seen this to be an issue.

I see it often enough in our BSPs (though the BSP device trees tend to be
problematic in a variety of ways), especially on things like guts and
pmc.

It's a question of how strong a statement people are asked to make -- in a
place where fixing errors is somewhat painful, and we don't really need that
strong statement of compatibility to be made, as long as there's another
way to fully identify the hardware (e.g. SVR, top-level board compatible) if
some strange workaround needs to be made[1].

To turn things around, in practice, I've not seen compatibles that don't
encode a specific chip name to be a problem, as long as it's well
documented what it means.

-Scott

[1] IIRC, this was the original reason for citing a specific chip, but it
doesn't hold up if that chip gets cited by other chips as compatible.  If
compatibliity includes having all the same bugs, then very little will be
able to claim compatibility, and we'll be back to long lists of device
IDs in the driver.  Not to mention errata that are discovered after a
device tree claiming compatibility is released...

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev