From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grant Likely Subject: Re: [RFC PATCH] ARM: pmu: add OF match support Date: Sat, 12 Feb 2011 23:23:25 -0700 Message-ID: <20110213062325.GE17375@angua.secretlab.ca> References: <1297223617-19173-1-git-send-email-robherring2@gmail.com> <3399156206431206254@unknownmsgid> <-4919269306841091192@unknownmsgid> <4D531CA0.8050402@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <4D531CA0.8050402-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org To: Rob Herring Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Will Deacon , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org On Wed, Feb 09, 2011 at 05:00:48PM -0600, Rob Herring wrote: > On 02/09/2011 01:03 PM, Grant Likely wrote: > >On Wed, Feb 9, 2011 at 11:53 AM, Will Deacon wrote: > >>Grant, > >> > >>>>Following on from Rob's update, it would be nice if you could specify that > >>>>the PMU is a CPU PMU (as opposed to L2-cache, bus, gpu etc) in the string. > >>>>That way adding different PMUs in the future seems more natural and it accounts > >>>>for your concerns above. Is that ok, or does the compatible string have to > >>>>match that used by the platform bus? > >>> > >>>It does make sense to encode the specific implementation into the > >>>compatible string. A single device driver can bind against multiple > >>>compatible strings. ie. the match table could include {.compatible = > >>>"arm,cortex-a9-pmu"},{.compatible = "arm,cortex-a9-l2cache-pmu"}... > >> > >>Ok - that's great! Specifying the CPU is probably a little verbose, but > >>something like "arm,armv7pmu" would be really helpful when it comes to > >>multiple devices. > >> > >>>>As for versioning, the PMU detection is done dynamically at runtime, > >>>>so knowing that we're poking a CPU is enough. > >>> > >>>Fair enough. It is still good practice in the compatible list to > >>>encode the specific PMU implementation (maybe arm,cortex-a9-pmu?) > >>>instead of trying to define a 'generic' or wildcard compatible value. > >>>Newer implementations can always claim compatibility with an older > >>>implementation so that the kernel doesn't have to be modified to find > >>>the new devices. "arm,pmu" is probably too generic. > >> > >>"arm,pmu" is definitely too generic. If it's good practice to specify > >>as much as possible, then go for it - I just feel a little uneasy having > >>to add lots of redundant kernel code but it sounds like you'll hopefully > >>prove me wrong on that :) > > > >Shouldn't be any redundant kernel code. It's just a list of match > >values, and the driver code already knows how to handle it. The > >advantage of being specific is that if the driver ever does need > >information about the specific implementation (say, because the > >hardware reported value is buggy), then the data is available. > > By that argument, we should use ",cortex-a9-rXpX-pmu." As a > particular rev of the A9 PMU or vendor's SOC could be buggy. That > would quickly grow into a long list. Just with cpu models, we would > have: > > cortex-a9 > cortex-a8 > cortex-a15 > cortex-a5 > arm1136 > arm1176 > l210 > l220 > pl310 > various xscale flavors > Qualcomm SMP This is not even remotely an unreasonable list. Also remember that newer revisions can (and should!) claim compatibility with an older version if it is indeed compatible. Typically what happens is one particular implementation ends up becoming the defacto standard implementation. So instead of each implementation claiming compatibility with a loosely defined "arm,pmu" (a definition that can end up changing over time), each of the above implementation may end up claiming "arm,cortex-a8-pmu" compatibility. In essence the string "arm,cortex-a8-pmu" *becomes* the generic version without the ambiguity of what to do if/when arm invents an incompatible PMU. > Keep in mind that the whole point of this platform device is just to > specify the interrupt connection which is the only part that varies > by SOC (and is not probe-able). That's tangential to the argument. There is very good reason to anchor compatible values to specific implementations. Doing it this way costs nothing. Not doing it causes ambiguity down the road that is hard to recover from. g.