Re: [PATCH v3 3/3] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
On 05/18/2010 01:36 AM, Richard Cochran wrote: On Mon, May 17, 2010 at 01:05:54PM -0500, Scott Wood wrote: + - tmr_fiper1 Fixed interval period pulse generator. + - tmr_fiper2 Fixed interval period pulse generator. MPC8572 and P2020 have fiper3 as well. I doubt they really have a third fiper. First of all, this signal is not routed anywhere on the boards. OK, but that's a separate issue from whether it exists on the chip and could be used on a different board. Also, according to the documentation, it has no bit in the TMR_CTRL or the TMR_TEMASK registers. It does seem inconsistent -- but could just be bad docs. Unless there is a bit in TMR_TEMASK, you cannot get an interrupt from it. If you cannot use the signal externally (in the "real" world) and you cannot get an interrupt, what good is it to have such a periodic signal? Polling the bit in the TMR_TEVENT to see when a pulse occurs seems pointless. Scott, you have connections, right? Can you clarify this for me? I'll ask around. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/3] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
On Mon, May 17, 2010 at 01:05:54PM -0500, Scott Wood wrote: > >>> >+ - tmr_fiper1 Fixed interval period pulse generator. > >>> >+ - tmr_fiper2 Fixed interval period pulse generator. > >> > > MPC8572 and P2020 have fiper3 as well. I doubt they really have a third fiper. First of all, this signal is not routed anywhere on the boards. Also, according to the documentation, it has no bit in the TMR_CTRL or the TMR_TEMASK registers. Unless there is a bit in TMR_TEMASK, you cannot get an interrupt from it. If you cannot use the signal externally (in the "real" world) and you cannot get an interrupt, what good is it to have such a periodic signal? Polling the bit in the TMR_TEVENT to see when a pulse occurs seems pointless. Scott, you have connections, right? Can you clarify this for me? Thanks, Richard ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/3] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
On 05/17/2010 03:27 AM, Richard Cochran wrote: On Fri, May 14, 2010 at 12:46:57PM -0500, Scott Wood wrote: On 05/14/2010 11:46 AM, Richard Cochran wrote: diff --git a/Documentation/powerpc/dts-bindings/fsl/tsec.txt b/Documentation/powerpc/dts-bindings/fsl/tsec.txt Get rid of both device_type and model, and specify a compatible string instead (e.g. "fsl,etsec-ptp"). Okay, will do. I really am at a loss at understanding all the rules in the whole device tree world. I just tried to follow Documentation/powerpc and what is already present in the kernel. There's some stuff in there that isn't how we'd do it now, but is slow to change for compatibility reasons. Or perhaps this should just be some additional properties on the existing gianfar nodes, rather than presenting it as a separate device? How do you associate a given ptp block with the corresponding gianfar node? There only one PTP clock. Its registers repeat in each port's memory space, but you are only supposed to touch the first set of PTP registers. OK. I'm not too familiar with PTP itself, was looking more at the device tree and similar structural bits. There are no differences (that I know of) in how the PTP clocks work. I have in house the mpc8313, the mpc8572, and the p2020. The mpc8572 appears to lack some of the TMR_CTRL bits, but this is probably a documentation bug. I will check it. If there's any possibility of needing to make a distinction (which probably can't be ruled out with future chips), the chip name could be made part of the compatible string, with a secondary compatible showing a canonical part name for that version of the PTP block. E.g. p2020 might have: compatble = "fsl,p2020-etsec-ptp", "fsl,mpc8313-etsec-ptp"; The driver would bind only on the mpc8313 version. There are several examples of this, such as the Freescale i2c driver and binding (ignore the legacy "fsl-i2c"). > >+ - tmr_fiper1 Fixed interval period pulse generator. > >+ - tmr_fiper2 Fixed interval period pulse generator. MPC8572 and P2020 have fiper3 as well. They should probably have an "fsl,ptp-" prefix as well. Okay, but must I then change the following code in order to find them? Does adding the prefix just mean that I also add it to my search strings, or is it preprocessed (stripped) somehow? It is not stripped; you have to change the code as well. You've got two IRQs, with the same handler, and the same dev_id? From the manual it looks like there's one PTP interrupt per eTSEC (which would explain 3 interrupts on p2020). Will reduce to just one IRQ. The device tree should still contain all of the interrupts, in case they're needed later -- and put a comment in the driver saying why the first interrupt seems sufficient. +static struct of_device_id match_table[] = { + { .type = "ptp_clock" }, + {}, +}; This driver controls every possible PTP implementation? No, I only want to match with the eTSEC clock device. Given the compatible string above ("fsl,etsec-ptp"), what is the correct way to do this? (pointer to an existing driver to emulate would be enough) Put .compatible = "fsl,etsec-ptp" (or "fsl,mpc8313-etsec-ptp") where you have .type = "ptp_clock". -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/3] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
On 05/14/2010 06:46 PM, 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 and one alarm. > > Signed-off-by: Richard Cochran Tested-by: Wolfgang Grandegger on my Freescale MPC8313 setup with ptpd and ptpv2d. FYI: checkplatch.pl reports various errors for this patch series. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/3] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
On Fri, May 14, 2010 at 12:46:57PM -0500, Scott Wood wrote: > On 05/14/2010 11:46 AM, Richard Cochran wrote: > >diff --git a/Documentation/powerpc/dts-bindings/fsl/tsec.txt > >b/Documentation/powerpc/dts-bindings/fsl/tsec.txt > > Get rid of both device_type and model, and specify a compatible > string instead (e.g. "fsl,etsec-ptp"). Okay, will do. I really am at a loss at understanding all the rules in the whole device tree world. I just tried to follow Documentation/powerpc and what is already present in the kernel. > Or perhaps this should just be some additional properties on the > existing gianfar nodes, rather than presenting it as a separate > device? How do you associate a given ptp block with the > corresponding gianfar node? There only one PTP clock. Its registers repeat in each port's memory space, but you are only supposed to touch the first set of PTP registers. If you consider how PTP works, there can never be per port clocks, since this would make it impossible to make a boundary clock, for example. The whole idea of this PTP clock framework is to keep the clock drivers separate from the MAC drivers, even when they use the same hardware. The functionality is logically divided into two parts. The MAC provides time stamps, and the clock provides a way to control its offset and frequency. Up until this point, people have simply hacked new private ioctls into the driver for each MAC that supports PTP. That is not a good long term solution for PTP support in Linux. In general, I think it will not be hard to keep the MAC and the clock drivers from stepping on each other's toes. The eTSEC hardware is certainly able to be used in this way. > If there are differences in ptp implementation between different > versions of etsec, can the ptp driver see the etsec version > register? There are no differences (that I know of) in how the PTP clocks work. I have in house the mpc8313, the mpc8572, and the p2020. The mpc8572 appears to lack some of the TMR_CTRL bits, but this is probably a documentation bug. I will check it. > >+ - 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. > > Dashes are more typical in OF names than underscores, and it's > generally better to be a little more verbose -- these aren't local > loop iterators. The names come from the register mnemonics from the documentation. I prefer to use the same names as is found in the manuals. That way, a person working with docu in hand will have an easier job. > They should probably have an "fsl,ptp-" prefix as well. Okay, but must I then change the following code in order to find them? Does adding the prefix just mean that I also add it to my search strings, or is it preprocessed (stripped) somehow? static int get_of_u32(struct device_node *node, char *str, u32 *val) { int plen; const u32 *prop = of_get_property(node, str, &plen); if (!prop || plen != sizeof(*prop)) return -1; *val = *prop; return 0; } ... if (get_of_u32(node, "tclk_period",&etsects->tclk_period) || get_of_u32(node, "tmr_prsc",&etsects->tmr_prsc) || get_of_u32(node, "tmr_add",&etsects->tmr_add) || get_of_u32(node, "cksel",&etsects->cksel) || get_of_u32(node, "tmr_fiper1",&etsects->tmr_fiper1) || get_of_u32(node, "tmr_fiper2",&etsects->tmr_fiper2)) return -ENODEV; > >+ These properties set the operational parameters for the PTP > >+ clock. You must choose these carefully for the clock to work right. > > Do these values describe the way the hardware is, or how it's been > configured by firmware, or a set of values that are clearly optimal > for this particular board? If it's just configuration for the Linux > driver, that could reasonably differ based on what a given user or > OS will want, the device tree probably isn't the right place for it. The values are related to the board. One important parameter is the input clock, and the rest reflect some engineering decisions/tradeoffs related to the signals to and from the PTP clock. There is not just one "optimal" choice, so I wanted to let the designer set the values. In any case, the parameters are definitely related to the board (not to the cpu or to linux), so I think the device tree is the right place for them. > This one has 3 interrupts? The driver supports only two. The documentation does not specify the IRQ line that each event belongs to. After some trial and error, it appears that all of the ancillary clock interrupts arrive on the first interrupt. The other lines (one per
Re: [PATCH v3 3/3] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
On 05/14/2010 11:46 AM, Richard Cochran wrote: diff --git a/Documentation/powerpc/dts-bindings/fsl/tsec.txt b/Documentation/powerpc/dts-bindings/fsl/tsec.txt index edb7ae1..b09ba66 100644 --- a/Documentation/powerpc/dts-bindings/fsl/tsec.txt +++ b/Documentation/powerpc/dts-bindings/fsl/tsec.txt @@ -74,3 +74,59 @@ Example: interrupt-parent =<&mpic>; phy-handle =<&phy0> }; + +* Gianfar PTP clock nodes + +General Properties: + + - device_type Should be "ptp_clock" Device_type is deprecated in most contexts for flat device trees. + - modelModel of the device. Must be "eTSEC" Model, while abused by the current gianfar binding code, is not supposed to be something that is ordinarily used to bind on. It is supposed to be a freeform field for indicating the specific model of hardware, mainly for human consumption or as a last resort for working around problems. Get rid of both device_type and model, and specify a compatible string instead (e.g. "fsl,etsec-ptp"). Or perhaps this should just be some additional properties on the existing gianfar nodes, rather than presenting it as a separate device? How do you associate a given ptp block with the corresponding gianfar node? If there are differences in ptp implementation between different versions of etsec, can the ptp driver see the etsec version register? + - reg Offset and length of the register set for the device + - interrupts There should be at least two and 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. Dashes are more typical in OF names than underscores, and it's generally better to be a little more verbose -- these aren't local loop iterators. They should probably have an "fsl,ptp-" prefix as well. + These properties set the operational parameters for the PTP + clock. You must choose these carefully for the clock to work right. Do these values describe the way the hardware is, or how it's been configured by firmware, or a set of values that are clearly optimal for this particular board? If it's just configuration for the Linux driver, that could reasonably differ based on what a given user or OS will want, the device tree probably isn't the right place for it. diff --git a/arch/powerpc/boot/dts/p2020ds.dts b/arch/powerpc/boot/dts/p2020ds.dts index 1101914..f72353a 100644 --- a/arch/powerpc/boot/dts/p2020ds.dts +++ b/arch/powerpc/boot/dts/p2020ds.dts @@ -336,6 +336,20 @@ phy_type = "ulpi"; }; + ptp_cl...@24e00 { + device_type = "ptp_clock"; + model = "eTSEC"; + reg = <0x24E00 0xB0>; + interrupts = <68 2 69 2 70 2>; + interrupt-parent = < &mpic >; + tclk_period = <5>; + tmr_prsc = <200>; + tmr_add = <0xCCCD>; + cksel = <1>; + tmr_fiper1 = <0x3B9AC9FB>; + tmr_fiper2 = <0x0001869B>; + }; + This one has 3 interrupts? The driver supports only two. +/* Private globals */ +static struct ptp_clock *gianfar_clock; Do you not support more than one of these? +static struct etsects the_clock; "The" clock? As oppsed to the "other" clock one line above? :-) +static irqreturn_t isr(int irq, void *priv) +{ + struct etsects *etsects = priv; + struct ptp_clock_event event; + u64 ns; + u32 ack=0, lo, hi, mask, val; + + val = gfar_read(&etsects->regs->tmr_tevent); + + if (val& ETS1) { + ack |= ETS1; + hi = gfar_read(&etsects->regs->tmr_etts1_h); + lo = gfar_read(&etsects->regs->tmr_etts1_l); + event.type = PTP_CLOCK_EXTTS; + event.index = 0; + event.timestamp = ((u64) hi)<< 32; + event.timestamp |= lo; + ptp_clock_event(gianfar_clock,&event); + } + + if (val& ETS2) { + ack |= ETS2; + hi = gfar_read(&etsects->regs->tmr_etts2_h); + lo = gfar_read(&etsects->regs->tmr_etts2_l); + event.type = PTP_CLOCK_EXTTS; + event.index = 1; + event.timestamp = ((u64) hi)<< 32; + event.timestamp |= lo; + ptp_clock_event(gianfar_clock,&event); + } + + if (val& ALM2) { + ack |= ALM2; + if (etsects->alarm_