| Message ID | 1307456541-11026-2-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, > static int __devinit pmu_device_probe(struct platform_device *pdev) > { > + enum arm_pmu_type type = pdev->id; > > - if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) { > + if (pdev->dev.of_node) > + 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[pdev->id]) > + if (pmu_devices[type]) > pr_warning("registering new PMU device type %d overwrites " > - "previous registration!\n", pdev->id); > + "previous registration!\n", type); > else > pr_info("registered new PMU device of type %d\n", > - pdev->id); > + type); > > - pmu_devices[pdev->id] = pdev; > + pmu_devices[type] = pdev; > return 0; > } I don't think this is the best way to handle the type when we've got an FDT description: * release_pmu hasn't been updated to match the type logic here, so it might do anything when handed a platform_device initialised by FDT code. * the warning message for an invalid registration still uses pdev->id rather than type. This can't currently be reached when the PMU was handed to us via FDT, but it may confuse refactoring later on. * If we want to add a new PMU type, we'll have to add more logic to pmu_device_probe. Given that work is going on to add support for system PMUs, this doesn't seem particularly brilliant. > +static struct of_device_id pmu_device_ids[] = { > + { .compatible = "arm,cortex-a9-pmu" }, > + { .compatible = "arm,cortex-a8-pmu" }, > + { .compatible = "arm,arm1136-pmu" }, > + { .compatible = "arm,arm1176-pmu" }, > + {}, > +}; > + > static struct platform_driver pmu_driver = { > .driver = { > .name = "arm-pmu", > + .of_match_table = pmu_device_ids, > }, > .probe = pmu_device_probe, > }; This all seems fine for handling CPU PMUs. I think that a better strategy would be to separate the type logic from the registration. I have a patch for this: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052455.html With it, you won't need to change pmu_device_probe, and adding FDT support should just be a matter of adding the of_match_table. Mark
Mark, On 06/08/2011 10:54 AM, Mark Rutland wrote: > Hi, > >> static int __devinit pmu_device_probe(struct platform_device *pdev) >> { >> + enum arm_pmu_type type = pdev->id; >> >> - if (pdev->id< 0 || pdev->id>= ARM_NUM_PMU_DEVICES) { >> + if (pdev->dev.of_node) >> + 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[pdev->id]) >> + if (pmu_devices[type]) >> pr_warning("registering new PMU device type %d overwrites " >> - "previous registration!\n", pdev->id); >> + "previous registration!\n", type); >> else >> pr_info("registered new PMU device of type %d\n", >> - pdev->id); >> + type); >> >> - pmu_devices[pdev->id] = pdev; >> + pmu_devices[type] = pdev; >> return 0; >> } > > I don't think this is the best way to handle the type when we've got an FDT > description: > > * release_pmu hasn't been updated to match the type logic here, so it might do > anything when handed a platform_device initialised by FDT code. > > * the warning message for an invalid registration still uses pdev->id rather > than type. This can't currently be reached when the PMU was handed to us via > FDT, but it may confuse refactoring later on. > > * If we want to add a new PMU type, we'll have to add more logic to > pmu_device_probe. Given that work is going on to add support for system PMUs, > this doesn't seem particularly brilliant. > >> +static struct of_device_id pmu_device_ids[] = { >> + { .compatible = "arm,cortex-a9-pmu" }, >> + { .compatible = "arm,cortex-a8-pmu" }, >> + { .compatible = "arm,arm1136-pmu" }, >> + { .compatible = "arm,arm1176-pmu" }, >> + {}, >> +}; >> + >> static struct platform_driver pmu_driver = { >> .driver = { >> .name = "arm-pmu", >> + .of_match_table = pmu_device_ids, >> }, >> .probe = pmu_device_probe, >> }; > > This all seems fine for handling CPU PMUs. > > I think that a better strategy would be to separate the type logic from the > registration. I have a patch for this: > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052455.html > > With it, you won't need to change pmu_device_probe, and adding FDT support > should just be a matter of adding the of_match_table. > Okay. I'll rebase mine on top of your changes. Rob
Hi Rob,
I've Addded Jamie on Cc here as he was interested in the possibility of
adding platform_device_id tables. Hopefully it'll be easy to discuss
{of,platform}_id_tables in one thread.
As Jamie pointed out to me the existence of platform_device_id tables,
I took a look around and noticed that of_device_id tables also seem to
provide support for driver-specific parameters (I saw an example of
usage in arch/sparc/kernel/pci_schizo.c).
I've had a go at getting {of,platform}_device_id tables to provide the
PMU type, so they can be used similarly (the macros make entries look
identical apart from the {plat,of} prefix).
I don't have any entries currently for the platform_device_id table,
but it'd be useful for system pmus, if we were to add an L2 Cache
controller PMU driver, it might have a binding like:
> PLAT_MATCH("arm,pl310-pmu", ARM_PMU_TYPE_L2CC),
How does the following series look to you? The first 2 patches are the
ones I mentioned before, unchanged (apart from additional acks).
Mark.
Mark Rutland (4):
ARM: pmu: refactor reservation
ARM: pmu: reject duplicate PMU registrations
ARM: pmu: add OF probing support
ARM: pmu: add platform_device_id table support
Documentation/devicetree/bindings/arm/pmu.txt | 22 ++++++
arch/arm/include/asm/pmu.h | 2 +-
arch/arm/kernel/perf_event.c | 4 +-
arch/arm/kernel/pmu.c | 91 ++++++++++++++++++++-----
4 files changed, 99 insertions(+), 20 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/pmu.txt
On Wed, Jun 08, 2011 at 11:40:01AM -0500, Rob Herring wrote: > Mark, > > On 06/08/2011 10:54 AM, Mark Rutland wrote: > >Hi, > > > >> static int __devinit pmu_device_probe(struct platform_device *pdev) > >> { > >>+ enum arm_pmu_type type = pdev->id; > >> > >>- if (pdev->id< 0 || pdev->id>= ARM_NUM_PMU_DEVICES) { > >>+ if (pdev->dev.of_node) > >>+ 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[pdev->id]) > >>+ if (pmu_devices[type]) > >> pr_warning("registering new PMU device type %d overwrites " > >>- "previous registration!\n", pdev->id); > >>+ "previous registration!\n", type); > >> else > >> pr_info("registered new PMU device of type %d\n", > >>- pdev->id); > >>+ type); > >> > >>- pmu_devices[pdev->id] = pdev; > >>+ pmu_devices[type] = pdev; > >> return 0; > >> } > > > >I don't think this is the best way to handle the type when we've got an FDT > >description: > > > >* release_pmu hasn't been updated to match the type logic here, so it might do > > anything when handed a platform_device initialised by FDT code. > > > >* the warning message for an invalid registration still uses pdev->id rather > > than type. This can't currently be reached when the PMU was handed to us via > > FDT, but it may confuse refactoring later on. > > > >* If we want to add a new PMU type, we'll have to add more logic to > > pmu_device_probe. Given that work is going on to add support for system PMUs, > > this doesn't seem particularly brilliant. > > > >>+static struct of_device_id pmu_device_ids[] = { > >>+ { .compatible = "arm,cortex-a9-pmu" }, > >>+ { .compatible = "arm,cortex-a8-pmu" }, > >>+ { .compatible = "arm,arm1136-pmu" }, > >>+ { .compatible = "arm,arm1176-pmu" }, > >>+ {}, > >>+}; > >>+ > >> static struct platform_driver pmu_driver = { > >> .driver = { > >> .name = "arm-pmu", > >>+ .of_match_table = pmu_device_ids, > >> }, > >> .probe = pmu_device_probe, > >> }; > > > >This all seems fine for handling CPU PMUs. > > > >I think that a better strategy would be to separate the type logic from the > >registration. I have a patch for this: > >http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052455.html > > > >With it, you won't need to change pmu_device_probe, and adding FDT support > >should just be a matter of adding the of_match_table. > > > > Okay. I'll rebase mine on top of your changes. The DT binding looks good to me though. g.
diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt new file mode 100644 index 0000000..739d00c --- /dev/null +++ b/Documentation/devicetree/bindings/arm/pmu.txt @@ -0,0 +1,22 @@ +* ARM Performance Monitor Units + +ARM cores often have a PMU for counting cpu and cache events like cache misses +and hits. The interface to the PMU is part of the ARM ARM. The ARM PMU +representation in the device tree should be done as under:- + +Required properties: + +- compatible : should be one of + "arm,cortex-a9-pmu" + "arm,cortex-a8-pmu" + "arm,arm1176-pmu" + "arm,arm1136-pmu" +- interrupts : 1 combined interrupt or 1 per core. + +Example: + +pmu { + compatible = "arm,cortex-a9-pmu"; + interrupts = <100 101>; +}; + diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c index 2c79eec..41bc972 100644 --- a/arch/arm/kernel/pmu.c +++ b/arch/arm/kernel/pmu.c @@ -27,27 +27,40 @@ static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES]; static int __devinit pmu_device_probe(struct platform_device *pdev) { + enum arm_pmu_type type = pdev->id; - if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) { + if (pdev->dev.of_node) + 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[pdev->id]) + if (pmu_devices[type]) pr_warning("registering new PMU device type %d overwrites " - "previous registration!\n", pdev->id); + "previous registration!\n", type); else pr_info("registered new PMU device of type %d\n", - pdev->id); + type); - pmu_devices[pdev->id] = pdev; + pmu_devices[type] = pdev; return 0; } +static struct of_device_id pmu_device_ids[] = { + { .compatible = "arm,cortex-a9-pmu" }, + { .compatible = "arm,cortex-a8-pmu" }, + { .compatible = "arm,arm1136-pmu" }, + { .compatible = "arm,arm1176-pmu" }, + {}, +}; + static struct platform_driver pmu_driver = { .driver = { .name = "arm-pmu", + .of_match_table = pmu_device_ids, }, .probe = pmu_device_probe, };