Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758384AbbKSLVm (ORCPT ); Thu, 19 Nov 2015 06:21:42 -0500 Received: from pandora.arm.linux.org.uk ([78.32.30.218]:39849 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751165AbbKSLVl (ORCPT ); Thu, 19 Nov 2015 06:21:41 -0500 Date: Thu, 19 Nov 2015 11:21:30 +0000 From: Russell King - ARM Linux To: Marc Gonzalez Cc: Thomas Gleixner , Daniel Lezcano , LKML , Mans Rullgard , Viresh Kumar , Nicolas Pitre , Tony Lindgren , Sebastian Frias Subject: Re: [PATCH] clocksource: Store reg field within struct clocksource Message-ID: <20151119112130.GY8644@n2100.arm.linux.org.uk> References: <564C8086.80804@sigmadesigns.com> <20151118172114.GS8644@n2100.arm.linux.org.uk> <20151119103645.GV8644@n2100.arm.linux.org.uk> <564DAAB2.6040706@sigmadesigns.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <564DAAB2.6040706@sigmadesigns.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2606 Lines: 62 On Thu, Nov 19, 2015 at 11:55:46AM +0100, Marc Gonzalez wrote: > If you just object to the ifdef, then perhaps 'reg' can be included > unconditionally. > > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h > index 278dd279a7a8..50725fd23ab0 100644 > --- a/include/linux/clocksource.h > +++ b/include/linux/clocksource.h > @@ -69,6 +69,7 @@ struct clocksource { > * clocksource itself is cacheline aligned. > */ > cycle_t (*read)(struct clocksource *cs); > + void __iomem *reg; > cycle_t mask; > u32 mult; > u32 shift; > > > The basic cause of this problem is the ____cacheline_aligned annotation > > which effectively prevents wrapping struct clocksource to provide > > implementation specific data. > > True. But note that placing reg inside struct clocksource comes > for free on 32-bit platforms (it just replaces padding). Yes, I'd object less if it's just replacing padding. > > Maybe your idea is that struct clocksource should be bloated with all > > implementation specific data in the long term? > > reg is not implementation-specific data, right? That depends on how you look at what consitutes "implementation specific". The vast majority of clocksource drivers access some non-MMIO register, or some register that they need to store the __iomem pointer externally for other reasons. The original clocksource design did not have any iomem pointer. When I wrote the MMIO clocksource implementation, there was no ____cacheline_aligned on struct clocksource, and the arrangement I came to for the structure put the 'reg' and 'read' within the same cache line (note that the MMIO clocksource pre-dates Thomas' rearrangement of struct clocksource and the addition of the cache line alignment.) The original layout did not have any padding gaps. So, it was pointless to add a __iomem pointer to the main structure, which would have bloated the struct for every user, and it made no sense what so ever to do that. Things may have changed, and there may be padding, but things have changed again which actually mean that very little of struct clocksource will be in a cache line when the ->read function is called, so the whole idea of fitting things into one cache line here is totally irrelevant anymore. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/