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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110224172652.GC15234@riccoc20.at.omicron.at>
Date:	Thu, 24 Feb 2011 18:26:52 +0100
From:	Richard Cochran <richardcochran@...il.com>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	netdev@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	linux-arm-kernel@...ts.infradead.org,
	linuxppc-dev@...ts.ozlabs.org, Alan Cox <alan@...rguk.ukuu.org.uk>,
	Arnd Bergmann <arnd@...db.de>,
	Christoph Lameter <cl@...ux.com>,
	David Miller <davem@...emloft.net>,
	John Stultz <john.stultz@...aro.org>,
	Krzysztof Halasa <khc@...waw.pl>,
	Peter Zijlstra <peterz@...radead.org>,
	Rodolfo Giometti <giometti@...ux.it>,
	Thomas Gleixner <tglx@...utronix.de>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Mike Frysinger <vapier@...too.org>,
	Paul Mackerras <paulus@...ba.org>,
	Russell King <linux@....linux.org.uk>
Subject: Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found
 on the MPC85xx.

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.
> > +  - cksel        0= 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-netdev&m=127662247203659&w=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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html