Date:	Wed, 04 Feb 2009 07:09:30 -0800
From:	Daniel Walker <dwalker@...o99.com>
To:	Patrick Ohly <patrick.ohly@...el.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	John Stultz <johnstul@...ibm.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH NET-NEXT 01/10] clocksource: allow usage independent of
	timekeeping.c

On Wed, 2009-02-04 at 15:46 +0100, Patrick Ohly wrote:
> Hi Daniel!
> 
> Thanks for looking at this. I think the previous discussion of this
> patch under the same subject on LKML already addressed your concerns,
> but I'll also reply in detail below.
> 
> On Wed, 2009-02-04 at 14:03 +0000, Daniel Walker wrote:
> > On Wed, 2009-02-04 at 14:01 +0100, Patrick Ohly wrote:
> > 
> > >  /**
> > > + * struct cyclecounter - hardware abstraction for a free running counter
> > > + *	Provides completely state-free accessors to the underlying hardware.
> > > + *	Depending on which hardware it reads, the cycle counter may wrap
> > > + *	around quickly. Locking rules (if necessary) have to be defined
> > > + *	by the implementor and user of specific instances of this API.
> > > + *
> > > + * @read:		returns the current cycle value
> > > + * @mask:		bitmask for two's complement
> > > + *			subtraction of non 64 bit counters,
> > > + *			see CLOCKSOURCE_MASK() helper macro
> > > + * @mult:		cycle to nanosecond multiplier
> > > + * @shift:		cycle to nanosecond divisor (power of two)
> > > + */
> > > +struct cyclecounter {
> > > +	cycle_t (*read)(const struct cyclecounter *cc);
> > > +	cycle_t mask;
> > > +	u32 mult;
> > > +	u32 shift;
> > > +};
> > 
> > Where are these defined? I don't see any in created in your code.
> 
> What do you mean with "these"? cycle_t? That type is defined in
> clocksource.h. This patch intentionally adds these definitions to the
> existing file because of the large conceptual overlap.

No, your creating a new structure here that wasn't declared. I was
referring to "struct cyclecounter". I did look up one of your prior
submission (Dec. 15) and reviewed that.

> In an earlier revision of the patch I had adapted clocksource itself so
> that it could be used outside of the time keeping code; John wanted me
> to use these smaller structs instead that you now find in the current
> patch.

Well, I think your original idea was better.. I don't think we need the
duplication of underlying clocksource mechanics.

> Eventually John wants to refactor clocksource so that it uses them and
> just adds additional elements in clocksource. Right now clocksource is a
> mixture of different concepts. Breaking out cyclecounter and timecounter
> is a first step towards that cleanup.

The problem I see is that your putting off the cleanup of struct
clocksource with duplication.. It should go in reverse , you should use
clocksources for your patch set. Which will motivate John to clean up
the clocksource structure.

There's a whole other issue which is that we have many architectures
already declaring struct clocksource for it's most basic usage. So I
think we want clocksource to be the base "cycle counter" structure (in
name and usage). Almost everything else in struct clocksource is
specific to generic timekeeping and could be factored out easily.

Daniel

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

Powered by blists - more mailing lists