[RFC PATCH] ARM: pmu: add OF match support

[RFC PATCH] ARM: pmu: add OF match support

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Add OF match table to enable OF style driver binding. The dts entry is like
this:

pmu {
	compatible = "arm,pmu";
	interrupts = <100 101>;
};

The use of pdev->id as an index breaks with OF device binding. Change to use
a counter instead. If more than 1 pmu device is ever really supported, a
better solution to match users with particular pmu is probably needed.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/kernel/pmu.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
index b8af96e..7084057 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -24,30 +24,37 @@
 static volatile long pmu_lock;
 
 static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
+static int pmu_device_count;
 
 static int __devinit pmu_device_probe(struct platform_device *pdev)
 {
 
-	if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
+	if (pmu_device_count >= ARM_NUM_PMU_DEVICES) {
 		pr_warning("received registration request for unknown "
 				"device %d\n", pdev->id);
 		return -EINVAL;
 	}
 
-	if (pmu_devices[pdev->id])
+	if (pmu_devices[pmu_device_count])
 		pr_warning("registering new PMU device type %d overwrites "
 				"previous registration!\n", pdev->id);
 	else
 		pr_info("registered new PMU device of type %d\n",
 				pdev->id);
 
-	pmu_devices[pdev->id] = pdev;
+	pmu_devices[pmu_device_count++] = pdev;
 	return 0;
 }
 
+static struct of_device_id pmu_device_ids[] = {
+	{ .compatible = "arm,pmu" },
+	{},
+};
+
 static struct platform_driver pmu_driver = {
 	.driver		= {
 		.name	= "arm-pmu",
+		.of_match_table = pmu_device_ids,
 	},
 	.probe		= pmu_device_probe,
 };
-- 
1.7.1

RE: [RFC PATCH] ARM: pmu: add OF match support

[Will Deacon]

Hi Rob,

> Add OF match table to enable OF style driver binding. The dts entry is like
> this:
> 
> pmu {
> 	compatible = "arm,pmu";
> 	interrupts = <100 101>;
> };
> 
> The use of pdev->id as an index breaks with OF device binding. Change to use
> a counter instead. If more than 1 pmu device is ever really supported, a
> better solution to match users with particular pmu is probably needed.

We will want to support multiple PMU devices in the near future but
this is currently blocked on userspace issues. One such device would
be the event counters on the PL310, which could be used to complement
the CPU PMU on the Cortex-A9.

I don't like the idea of a counter as it forces platforms to register
their devices in a specific order. Is it possible to separate different
types of PMU in the device tree and then have pmu.c work out what to do
with them?

Will

Re: [RFC PATCH] ARM: pmu: add OF match support

[Grant Likely]

On Tue, Feb 8, 2011 at 8:53 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>
> Add OF match table to enable OF style driver binding. The dts entry is like
> this:
>
> pmu {
>        compatible = "arm,pmu";
>        interrupts = <100 101>;
> };
>
> The use of pdev->id as an index breaks with OF device binding. Change to use
> a counter instead. If more than 1 pmu device is ever really supported, a
> better solution to match users with particular pmu is probably needed.
>
> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm/kernel/pmu.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> index b8af96e..7084057 100644
> --- a/arch/arm/kernel/pmu.c
> +++ b/arch/arm/kernel/pmu.c
> @@ -24,30 +24,37 @@
>  static volatile long pmu_lock;
>
>  static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
> +static int pmu_device_count;
>
>  static int __devinit pmu_device_probe(struct platform_device *pdev)
>  {
>
> -       if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
> +       if (pmu_device_count >= ARM_NUM_PMU_DEVICES) {
>                pr_warning("received registration request for unknown "
>                                "device %d\n", pdev->id);
>                return -EINVAL;
>        }
>
> -       if (pmu_devices[pdev->id])
> +       if (pmu_devices[pmu_device_count])
>                pr_warning("registering new PMU device type %d overwrites "
>                                "previous registration!\n", pdev->id);
>        else
>                pr_info("registered new PMU device of type %d\n",
>                                pdev->id);
>
> -       pmu_devices[pdev->id] = pdev;
> +       pmu_devices[pmu_device_count++] = pdev;
>        return 0;
>  }
>
> +static struct of_device_id pmu_device_ids[] = {
> +       { .compatible = "arm,pmu" },

This is a pretty generic compatible string and it doesn't account for
the possibility that the implementation behaviour will change with
newer devices.  Is there any form of versioning on the pmu that would
be appropriate to encode here?

g.

RE: [RFC PATCH] ARM: pmu: add OF match support

[Will Deacon]

Hi Grant, Rob,

> On Tue, Feb 8, 2011 at 8:53 PM, Rob Herring <robherring2@gmail.com> wrote:
> > From: Rob Herring <rob.herring@calxeda.com>
> >
> > Add OF match table to enable OF style driver binding. The dts entry is like
> > this:
> >
> > pmu {
> >        compatible = "arm,pmu";
> >        interrupts = <100 101>;
> > };
> >
> > The use of pdev->id as an index breaks with OF device binding. Change to use
> > a counter instead. If more than 1 pmu device is ever really supported, a
> > better solution to match users with particular pmu is probably needed.
> >
> > Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> > ---
> >  arch/arm/kernel/pmu.c |   13 ++++++++++---
> >  1 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> > index b8af96e..7084057 100644
> > --- a/arch/arm/kernel/pmu.c
> > +++ b/arch/arm/kernel/pmu.c
> > @@ -24,30 +24,37 @@
> >  static volatile long pmu_lock;
> >
> >  static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
> > +static int pmu_device_count;
> >
> >  static int __devinit pmu_device_probe(struct platform_device *pdev)
> >  {
> >
> > -       if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
> > +       if (pmu_device_count >= ARM_NUM_PMU_DEVICES) {
> >                pr_warning("received registration request for unknown "
> >                                "device %d\n", pdev->id);
> >                return -EINVAL;
> >        }
> >
> > -       if (pmu_devices[pdev->id])
> > +       if (pmu_devices[pmu_device_count])
> >                pr_warning("registering new PMU device type %d overwrites "
> >                                "previous registration!\n", pdev->id);
> >        else
> >                pr_info("registered new PMU device of type %d\n",
> >                                pdev->id);
> >
> > -       pmu_devices[pdev->id] = pdev;
> > +       pmu_devices[pmu_device_count++] = pdev;
> >        return 0;
> >  }
> >
> > +static struct of_device_id pmu_device_ids[] = {
> > +       { .compatible = "arm,pmu" },
> 
> This is a pretty generic compatible string and it doesn't account for
> the possibility that the implementation behaviour will change with
> newer devices.  Is there any form of versioning on the pmu that would
> be appropriate to encode here?

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?

As for versioning, the PMU detection is done dynamically at runtime,
so knowing that we're poking a CPU is enough.

Cheers,

Will

RE: [RFC PATCH] ARM: pmu: add OF match support

[Will Deacon]

Hi Grant, Rob,

> On Tue, Feb 8, 2011 at 8:53 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >
> > Add OF match table to enable OF style driver binding. The dts entry is like
> > this:
> >
> > pmu {
> >        compatible = "arm,pmu";
> >        interrupts = <100 101>;
> > };
> >
> > The use of pdev->id as an index breaks with OF device binding. Change to use
> > a counter instead. If more than 1 pmu device is ever really supported, a
> > better solution to match users with particular pmu is probably needed.
> >
> > Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  arch/arm/kernel/pmu.c |   13 ++++++++++---
> >  1 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> > index b8af96e..7084057 100644
> > --- a/arch/arm/kernel/pmu.c
> > +++ b/arch/arm/kernel/pmu.c
> > @@ -24,30 +24,37 @@
> >  static volatile long pmu_lock;
> >
> >  static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
> > +static int pmu_device_count;
> >
> >  static int __devinit pmu_device_probe(struct platform_device *pdev)
> >  {
> >
> > -       if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
> > +       if (pmu_device_count >= ARM_NUM_PMU_DEVICES) {
> >                pr_warning("received registration request for unknown "
> >                                "device %d\n", pdev->id);
> >                return -EINVAL;
> >        }
> >
> > -       if (pmu_devices[pdev->id])
> > +       if (pmu_devices[pmu_device_count])
> >                pr_warning("registering new PMU device type %d overwrites "
> >                                "previous registration!\n", pdev->id);
> >        else
> >                pr_info("registered new PMU device of type %d\n",
> >                                pdev->id);
> >
> > -       pmu_devices[pdev->id] = pdev;
> > +       pmu_devices[pmu_device_count++] = pdev;
> >        return 0;
> >  }
> >
> > +static struct of_device_id pmu_device_ids[] = {
> > +       { .compatible = "arm,pmu" },
> 
> This is a pretty generic compatible string and it doesn't account for
> the possibility that the implementation behaviour will change with
> newer devices.  Is there any form of versioning on the pmu that would
> be appropriate to encode here?

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?

As for versioning, the PMU detection is done dynamically at runtime,
so knowing that we're poking a CPU is enough.

Cheers,

Will

Re: [RFC PATCH] ARM: pmu: add OF match support

[Grant Likely]

On Wed, Feb 9, 2011 at 10:17 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Grant, Rob,
>
>> On Tue, Feb 8, 2011 at 8:53 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> >
>> > Add OF match table to enable OF style driver binding. The dts entry is like
>> > this:
>> >
>> > pmu {
>> >        compatible = "arm,pmu";
>> >        interrupts = <100 101>;
>> > };
>> >
>> > The use of pdev->id as an index breaks with OF device binding. Change to use
>> > a counter instead. If more than 1 pmu device is ever really supported, a
>> > better solution to match users with particular pmu is probably needed.
>> >
>> > Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> > ---
>> >  arch/arm/kernel/pmu.c |   13 ++++++++++---
>> >  1 files changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
>> > index b8af96e..7084057 100644
>> > --- a/arch/arm/kernel/pmu.c
>> > +++ b/arch/arm/kernel/pmu.c
>> > @@ -24,30 +24,37 @@
>> >  static volatile long pmu_lock;
>> >
>> >  static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
>> > +static int pmu_device_count;
>> >
>> >  static int __devinit pmu_device_probe(struct platform_device *pdev)
>> >  {
>> >
>> > -       if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
>> > +       if (pmu_device_count >= ARM_NUM_PMU_DEVICES) {
>> >                pr_warning("received registration request for unknown "
>> >                                "device %d\n", pdev->id);
>> >                return -EINVAL;
>> >        }
>> >
>> > -       if (pmu_devices[pdev->id])
>> > +       if (pmu_devices[pmu_device_count])
>> >                pr_warning("registering new PMU device type %d overwrites "
>> >                                "previous registration!\n", pdev->id);
>> >        else
>> >                pr_info("registered new PMU device of type %d\n",
>> >                                pdev->id);
>> >
>> > -       pmu_devices[pdev->id] = pdev;
>> > +       pmu_devices[pmu_device_count++] = pdev;
>> >        return 0;
>> >  }
>> >
>> > +static struct of_device_id pmu_device_ids[] = {
>> > +       { .compatible = "arm,pmu" },
>>
>> This is a pretty generic compatible string and it doesn't account for
>> the possibility that the implementation behaviour will change with
>> newer devices.  Is there any form of versioning on the pmu that would
>> be appropriate to encode here?
>
> 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"}...

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

g.

RE: [RFC PATCH] ARM: pmu: add OF match support

[Will Deacon]

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

Cheers,

Will

RE: [RFC PATCH] ARM: pmu: add OF match support

[Will Deacon]

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

Cheers,

Will

Re: [RFC PATCH] ARM: pmu: add OF match support

[Grant Likely]

On Wed, Feb 9, 2011 at 11:53 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> 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.

g.

Re: [RFC PATCH] ARM: pmu: add OF match support

[Lorenzo Pieralisi]

On Wed, 2011-02-09 at 12:03 -0700, Grant Likely wrote:

<snip>

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

Just to mention that this is ok as long as the platform device id 
is not used when we probe from DT (eg release_pmu ?).
If it is used for any reason it has to be initialized in the DT layer
since, as Grant brought up before, it cannot be changed after device
registration (ie @ dt_probe() ).

Lorenzo

Re: [RFC PATCH] ARM: pmu: add OF match support

[Rob Herring]

On 02/09/2011 01:03 PM, Grant Likely wrote:
> On Wed, Feb 9, 2011 at 11:53 AM, Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>  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 "<vendor>,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

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

Rob

Re: [RFC PATCH] ARM: pmu: add OF match support

[Grant Likely]

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<will.deacon-5wv7dgnIgG8@public.gmane.org>  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 "<vendor>,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.

RE: [RFC PATCH] ARM: pmu: add OF match support

[Will Deacon]

Hi Rob,

> Add OF match table to enable OF style driver binding. The dts entry is like
> this:
> 
> pmu {
> 	compatible = "arm,pmu";
> 	interrupts = <100 101>;
> };
> 
> The use of pdev->id as an index breaks with OF device binding. Change to use
> a counter instead. If more than 1 pmu device is ever really supported, a
> better solution to match users with particular pmu is probably needed.

We will want to support multiple PMU devices in the near future but
this is currently blocked on userspace issues. One such device would
be the event counters on the PL310, which could be used to complement
the CPU PMU on the Cortex-A9.

I don't like the idea of a counter as it forces platforms to register
their devices in a specific order. Is it possible to separate different
types of PMU in the device tree and then have pmu.c work out what to do
with them?

Will

Re: [RFC PATCH] ARM: pmu: add OF match support

[Rob Herring]

Will,

On 02/09/2011 03:51 AM, Will Deacon wrote:
> Hi Rob,
>
>> Add OF match table to enable OF style driver binding. The dts entry is like
>> this:
>>
>> pmu {
>> 	compatible = "arm,pmu";
>> 	interrupts =<100 101>;
>> };
>>
>> The use of pdev->id as an index breaks with OF device binding. Change to use
>> a counter instead. If more than 1 pmu device is ever really supported, a
>> better solution to match users with particular pmu is probably needed.
>
> We will want to support multiple PMU devices in the near future but
> this is currently blocked on userspace issues. One such device would
> be the event counters on the PL310, which could be used to complement
> the CPU PMU on the Cortex-A9.
>
> I don't like the idea of a counter as it forces platforms to register
> their devices in a specific order. Is it possible to separate different
> types of PMU in the device tree and then have pmu.c work out what to do
> with them?

Yes, they are easily distinguished by the compatible strings. Adding a 
pmu_get function which returns the device based on a string or 
capabilities may be a better solution. If you had 2 pmu's of the same 
type, the current code would break.

In any case, how about this:

static int __devinit pmu_device_probe(struct platform_device *pdev)
{
	enum arm_pmu_type type = pdev->id;

	if (of_device_is_compatible(pdev->dev.of_node, "arm,pmu"))
		type = ARM_PMU_DEVICE_CPU;

	if (type < 0 || type >= ARM_NUM_PMU_DEVICES) {
		pr_warning("received registration request for unknown "
				"device %d\n", pdev->id);
		return -EINVAL;
	}

	if (pmu_devices[type])
		pr_warning("registering new PMU device type %d overwrites "
				"previous registration!\n", type);
	else
		pr_info("registered new PMU device of type %d\n",
				type);

	pmu_devices[type] = pdev;
	return 0;
}

Rob

Re: [RFC PATCH] ARM: pmu: add OF match support

[Rob Herring]

Grant,

On 02/09/2011 08:02 AM, Rob Herring wrote:
> Will,
>
> On 02/09/2011 03:51 AM, Will Deacon wrote:
>> Hi Rob,
>>
>>> Add OF match table to enable OF style driver binding. The dts entry
>>> is like
>>> this:
>>>
>>> pmu {
>>> compatible = "arm,pmu";
>>> interrupts =<100 101>;
>>> };
>>>
>>> The use of pdev->id as an index breaks with OF device binding. Change
>>> to use
>>> a counter instead. If more than 1 pmu device is ever really supported, a
>>> better solution to match users with particular pmu is probably needed.
>>
>> We will want to support multiple PMU devices in the near future but
>> this is currently blocked on userspace issues. One such device would
>> be the event counters on the PL310, which could be used to complement
>> the CPU PMU on the Cortex-A9.
>>
>> I don't like the idea of a counter as it forces platforms to register
>> their devices in a specific order. Is it possible to separate different
>> types of PMU in the device tree and then have pmu.c work out what to do
>> with them?
>
> Yes, they are easily distinguished by the compatible strings. Adding a
> pmu_get function which returns the device based on a string or
> capabilities may be a better solution. If you had 2 pmu's of the same
> type, the current code would break.
>
> In any case, how about this:
>
> static int __devinit pmu_device_probe(struct platform_device *pdev)
> {
> enum arm_pmu_type type = pdev->id;
>
> if (of_device_is_compatible(pdev->dev.of_node, "arm,pmu"))
> type = ARM_PMU_DEVICE_CPU;

Actually, this needs to be:

#ifdef CONFIG_OF
	if (of_device_is_compatible(pdev->dev.of_node, "arm,pmu"))
		type = ARM_PMU_DEVICE_CPU;
#endif

Seems fixing the struct device conditional is just the tip of the 
iceberg. Should we do empty functions for all driver accessed OF functions?

Rob

Re: [RFC PATCH] ARM: pmu: add OF match support

[Grant Likely]

On Wed, Feb 9, 2011 at 9:55 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Grant,
>
> On 02/09/2011 08:02 AM, Rob Herring wrote:
>>
>> Will,
>>
>> On 02/09/2011 03:51 AM, Will Deacon wrote:
>>>
>>> Hi Rob,
>>>
>>>> Add OF match table to enable OF style driver binding. The dts entry
>>>> is like
>>>> this:
>>>>
>>>> pmu {
>>>> compatible = "arm,pmu";
>>>> interrupts =<100 101>;
>>>> };
>>>>
>>>> The use of pdev->id as an index breaks with OF device binding. Change
>>>> to use
>>>> a counter instead. If more than 1 pmu device is ever really supported, a
>>>> better solution to match users with particular pmu is probably needed.
>>>
>>> We will want to support multiple PMU devices in the near future but
>>> this is currently blocked on userspace issues. One such device would
>>> be the event counters on the PL310, which could be used to complement
>>> the CPU PMU on the Cortex-A9.
>>>
>>> I don't like the idea of a counter as it forces platforms to register
>>> their devices in a specific order. Is it possible to separate different
>>> types of PMU in the device tree and then have pmu.c work out what to do
>>> with them?
>>
>> Yes, they are easily distinguished by the compatible strings. Adding a
>> pmu_get function which returns the device based on a string or
>> capabilities may be a better solution. If you had 2 pmu's of the same
>> type, the current code would break.
>>
>> In any case, how about this:
>>
>> static int __devinit pmu_device_probe(struct platform_device *pdev)
>> {
>> enum arm_pmu_type type = pdev->id;
>>
>> if (of_device_is_compatible(pdev->dev.of_node, "arm,pmu"))
>> type = ARM_PMU_DEVICE_CPU;
>
> Actually, this needs to be:
>
> #ifdef CONFIG_OF
>        if (of_device_is_compatible(pdev->dev.of_node, "arm,pmu"))
>                type = ARM_PMU_DEVICE_CPU;
> #endif
>
> Seems fixing the struct device conditional is just the tip of the iceberg.
> Should we do empty functions for all driver accessed OF functions?

Usually all of the dt parsing code can be shuffled off into another
function that can be #ifdef/#else/#endif away, but there is always a
few functions that need to have empty implementations when CONFIG_OF
is disabled.  I prefer to take it on a case-by-case basis.

The #ifdef around dev->of_node is going away in 2.6.39

g.