ARM: pmu: add OF match support
RFC PATCH 1/1 ARM: pmu: add OF match support
From: Rob Herring <[email protected]>
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.
--- arch/arm/kernel/pmu.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
arch/arm/kernel/pmu.c +10 -3
@@ -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, };
Signature · 1 line
1.7.1
RFC PATCH 1/1 ARM: pmu: add OF match support
Hi Rob,
Quoted 11 lines
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->idas 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
RFC PATCH 1/1 ARM: pmu: add OF match support
Will,
Quoted 19 lines · Will Deacon
Hi Rob,
Quoted 11 lines
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->idas 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.
Quoted 4 lines
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.cwork 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
RFC PATCH 1/1 ARM: pmu: add OF match support
Grant,
Quoted 34 lines · Rob Herring
Will,
Quoted 21 lines · Will Deacon
Hi Rob,
Quoted 9 lines
Add OF match table to enable OF style driver binding. The dts entry is like
this:pmu {
compatible = "arm,pmu";
interrupts =<100 101>;
};Quoted 4 lines
The use of
pdev->idas 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.
Quoted 4 lines
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.cwork out what to do with them?Yes, they are easily distinguished by the compatible strings. Adding a
pmu_getfunction 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.
Quoted 8 lines
In any case, how about this:
static int
__devinitpmu_device_probe(struct platform_device *pdev){
enumarm_pmu_typetype =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
RFC PATCH 1/1 ARM: pmu: add OF match support
Quoted 50 lines · Rob Herring
Grant,
Quoted 36 lines · Rob Herring
Will,
Quoted 22 lines · Will Deacon
Hi Rob,
Quoted 9 lines
Add OF match table to enable OF style driver binding. The dts entry is like
this:pmu {
compatible = "arm,pmu";
interrupts =<100 101>;
};Quoted 4 lines
The use of
pdev->idas 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.
Quoted 4 lines
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.cwork out what to do with them?Yes, they are easily distinguished by the compatible strings. Adding a
pmu_getfunction 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.Quoted 8 lines
In any case, how about this:
static int
__devinitpmu_device_probe(struct platform_device *pdev){
enumarm_pmu_typetype =pdev->id;if (
of_device_is_compatible(pdev->dev.of_node, "arm,pmu")) type =ARM_PMU_DEVICE_CPU;Actually, this needs to be:
Quoted 7 lines
#ifdef
CONFIG_OF
? ? ? ?if (of_device_is_compatible(pdev->dev.of_node, "arm,pmu")) ? ? ? ? ? ? ? ?type =ARM_PMU_DEVICE_CPU;
#endifSeems 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.
A message that is not in the archive (sent off-list or not yet received): 1297223617-19173-1-git-send-email-robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Hi Rob,
Quoted 11 lines
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->idas 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
Hi Rob,
Quoted 11 lines
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->idas 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
RFC PATCH 1/1 ARM: pmu: add OF match support
In reply to Rob HerringQuoted 47 lines · Rob Herring
From: Rob Herring <[email protected]>
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->idas 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 <[email protected]>---
?arch/arm/kernel/pmu.c | ? 13 ++++++++++--- ?1 files changed, 10 insertions(+), 3 deletions(-)arch/arm/kernel/pmu.c +2
-1@@ -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);
Quoted 7 lines
- ? ? ? pmu_devices[pdev->id] = pdev;
+ ? ? ? pmu_devices[pmu_device_count++] = pdev;
? ? ? ?return 0;
?}+static struct
of_device_idpmu_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.
RFC PATCH 1/1 ARM: pmu: add OF match support
Hi Grant, Rob,
Quoted 53 lines · Rob Herring
Quoted 52 lines
From: Rob Herring <[email protected]>
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->idas 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 <[email protected]>--- arch/arm/kernel/pmu.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)arch/arm/kernel/pmu.c +4
-3@@ -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 ; }
Quoted 7 lines
arch/arm/kernel/pmu.c line 50
Quoted 2 lines
arch/arm/kernel/pmu.c +2
-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
Hi Grant, Rob,
Quoted 53 lines · Rob Herring
Quoted 52 lines
From: Rob Herring <[email protected]>
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->idas 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 <[email protected]>--- arch/arm/kernel/pmu.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)arch/arm/kernel/pmu.c +4
-3@@ -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 ; }
Quoted 7 lines
arch/arm/kernel/pmu.c line 50
Quoted 2 lines
arch/arm/kernel/pmu.c +2
-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
A message that is not in the archive (sent off-list or not yet received): AANLkTi=7azYw8+sYrbDsLOfF1aZynBDo+ub37O9j+k49-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Hi Grant, Rob,
Quoted 53 lines · Rob Herring
Quoted 52 lines
From: Rob Herring <rob.herring-bsGFqQB8/[email protected]>
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->idas 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/[email protected]>--- arch/arm/kernel/pmu.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)arch/arm/kernel/pmu.c +4
-3@@ -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 ; }
Quoted 7 lines
arch/arm/kernel/pmu.c line 50
Quoted 2 lines
arch/arm/kernel/pmu.c +2
-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
A message that is not in the archive (sent off-list or not yet received): 3399156206431206254@unknownmsgid
RFC PATCH 1/1 ARM: pmu: add OF match support
Quoted 63 lines · Will Deacon
Hi Grant, Rob,
Quoted 53 lines · Rob Herring
Quoted 52 lines
From: Rob Herring <[email protected]>
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->idas 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 <[email protected]>---
?arch/arm/kernel/pmu.c | ? 13 ++++++++++--- ?1 files changed, 10 insertions(+), 3 deletions(-)arch/arm/kernel/pmu.c +2
-1@@ -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;
?}Quoted 7 lines
Quoted 2 lines
+static struct
of_device_idpmu_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?
Quoted 5 lines
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"}...
Quoted 2 lines
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.
RFC PATCH 1/1 ARM: pmu: add OF match support
Grant,
Quoted 10 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 2 lines
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
Grant,
Quoted 10 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 2 lines
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
A message that is not in the archive (sent off-list or not yet received): AANLkTin5aA0tq-PYBmRKh6tLv6bHRaK5ymjhsj9T4-Ah-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Grant,
Quoted 10 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 2 lines
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
A message that is not in the archive (sent off-list or not yet received): -4919269306841091192@unknownmsgid
RFC PATCH 1/1 ARM: pmu: add OF match support
Quoted 27 lines · Will Deacon
Grant,
Quoted 10 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 4 lines
"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.
RFC PATCH 1/1 ARM: pmu: add OF match support
On Wed, 2011-02-09 at 12:03 -0700, Grant Likely wrote:
<snip>
Quoted 24 lines
Quoted 19 lines
Quoted 4 lines
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 4 lines
"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 :)
Quoted 6 lines
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
RFC PATCH 1/1 ARM: pmu: add OF match support
In reply to Grant LikelyQuoted 33 lines · Grant Likely
Quoted 27 lines · Will Deacon
Grant,
Quoted 10 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 4 lines
"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 :)
Quoted 5 lines
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
RFC PATCH 1/1 ARM: pmu: add OF match support
Quoted 45 lines · Rob Herring
Quoted 33 lines · Grant Likely
Quoted 27 lines · Will Deacon
Grant,
Quoted 10 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 4 lines
"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 :)
Quoted 5 lines
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:
Quoted 11 lines
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.
Quoted 3 lines
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.