[RFC PATCH 11/14] ARM: PMU: add device tree probing
Lorenzo Pieralisi
Lorenzo.Pieralisi at arm.com
Fri Aug 20 20:56:32 EST 2010
> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of Grant
> Likely
> Sent: 18 August 2010 22:37
> To: Lorenzo Pieralisi
> Cc: devicetree-discuss at lists.ozlabs.org; Philippe Robin; nico at fluxnic.net;
> linux at arm.linux.org.uk; Catalin Marinas; jeremy.kerr at canonical.com
> Subject: Re: [RFC PATCH 11/14] ARM: PMU: add device tree probing
>
> On Wed, Aug 18, 2010 at 12:59 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi at arm.com> wrote:
> > When OF is enabled, platform drivers are required to define a
> > match table in order to allow the kernel to find drivers suitable
> > for a given device. The device tree allows to retrieve resources
> > from device tree nodes dynamically.
> >
> > This patch adds device tree support to the ARM PMU driver. This
> > includes a match table and code to initialize the driver id from
> > the respective device tree node compatible property.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>
> > ---
> > arch/arm/include/asm/pmu.h | 6 ++++++
> > arch/arm/kernel/Makefile | 3 ++-
> > arch/arm/kernel/pmu-of.c | 30 ++++++++++++++++++++++++++++++
> > arch/arm/kernel/pmu.c | 18 ++++++++++++------
> > 4 files changed, 50 insertions(+), 7 deletions(-)
> > create mode 100644 arch/arm/kernel/pmu-of.c
> >
> > diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
> > index 8ccea01..d317b64 100644
> > --- a/arch/arm/include/asm/pmu.h
> > +++ b/arch/arm/include/asm/pmu.h
> > @@ -17,6 +17,12 @@ enum arm_pmu_type {
> > ARM_NUM_PMU_DEVICES,
> > };
> >
> > +#ifndef CONFIG_OF
> > +static inline int pmu_probe_dt(struct platform_device *pdev) { return -
> ENODEV; }
> > +#else
> > +extern int __devinit pmu_probe_dt(struct platform_device *pdev);
> > +#endif
> > +
> > #ifdef CONFIG_CPU_HAS_PMU
> >
> > /**
> > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > index 0ace897..b7d846f 100644
> > --- a/arch/arm/kernel/Makefile
> > +++ b/arch/arm/kernel/Makefile
> > @@ -48,7 +48,8 @@ obj-$(CONFIG_CPU_XSCALE) += xscale-cp0.o
> > obj-$(CONFIG_CPU_XSC3) += xscale-cp0.o
> > obj-$(CONFIG_CPU_MOHAWK) += xscale-cp0.o
> > obj-$(CONFIG_IWMMXT) += iwmmxt.o
> > -obj-$(CONFIG_CPU_HAS_PMU) += pmu.o
> > +pmu_of-$(CONFIG_OF) := pmu-of.o
> > +obj-$(CONFIG_CPU_HAS_PMU) += pmu.o $(pmu_of-y)
> > obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o
> > AFLAGS_iwmmxt.o := -Wa,-mcpu=iwmmxt
> >
> > diff --git a/arch/arm/kernel/pmu-of.c b/arch/arm/kernel/pmu-of.c
> > new file mode 100644
> > index 0000000..5c5ee94
> > --- /dev/null
> > +++ b/arch/arm/kernel/pmu-of.c
> > @@ -0,0 +1,30 @@
> > +/*
> > + * linux/arch/arm/kernel/pmu-of.c -- PMU DT probe function
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <asm/pmu.h>
> > +
> > +int __devinit pmu_probe_dt(struct platform_device *pdev)
> > +{
> > + int ret = -ENODEV;
> > + struct device_node *node = pdev->dev.of_node;
> > +
> > + if (of_device_is_compatible(node, "arm,arm-pmu")) {
> > + pdev->id = ARM_PMU_DEVICE_CPU;
> > + ret = 0;
> > + } else {
> > + pr_warning("Probing device not recognized "
> > + "device %s\n", node->full_name);
>
> Do you really want the warning on an OF-enabled kernel when booting on
> an non-OF board?
>
Good point. I would answer no. Valid in other contexts as well.
I will take that into account.
> > + }
> > +
> > + return ret;
> > +}
>
> How about:
> {
> if (!of_match_node(arm_pmu_matches, node))
> return -ENODEV;
>
> pdev->id = ARM_PMU_DEVICE_CPU;
> return 0;
> }
>
> Clearer and more concise, no?
>
Definitely.
> Also, (as you've brought up before) the pdev->id issue needs to be
> solved. I don't believe it is allowed to change the id field after
> the platform_device is registered. I'd rather see the id dynamically
> assigned, which generally works in the OF use-case because
> interconnections between devices are described in the tree and
> specific ids aren't really needed. However, it could be that the best
> thing to do is retrieve the system-wide unique id from the aliases
> node.
>
> ... and ditto on my comment that this will be simpler if it lives
> directly in pmu.c. :-)
When you say "retrieve the system-wide id from the aliases node" you mean
at platform_device register time in the OF layer (ie of_device_register),
right ?
>
> > diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> > index b8af96e..f070b3d 100644
> > --- a/arch/arm/kernel/pmu.c
> > +++ b/arch/arm/kernel/pmu.c
> > @@ -27,12 +27,12 @@ static struct platform_device
> *pmu_devices[ARM_NUM_PMU_DEVICES];
> >
> > static int __devinit pmu_device_probe(struct platform_device *pdev)
> > {
> > -
> > - if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
> > - pr_warning("received registration request for unknown "
> > - "device %d\n", pdev->id);
> > - return -EINVAL;
> > - }
> > + if (pmu_probe_dt(pdev) == -ENODEV)
> > + if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
> > + pr_warning("received registration request for
> unknown "
> > + "device %d\n", pdev->id);
> > + return -EINVAL;
> > + }
> >
> > if (pmu_devices[pdev->id])
> > pr_warning("registering new PMU device type %d overwrites
> "
> > @@ -45,6 +45,11 @@ static int __devinit pmu_device_probe(struct
> platform_device *pdev)
> > return 0;
> > }
> >
> > +static struct of_device_id arm_pmu_matches[] = {
> > + { .compatible = "arm,arm-pmu"},
> > + {},
> > +};
> > +
> > static struct platform_driver pmu_driver = {
> > .driver = {
> > .name = "arm-pmu",
> > @@ -54,6 +59,7 @@ static struct platform_driver pmu_driver = {
> >
> > static int __init register_pmu_driver(void)
> > {
> > + platform_init_match(&pmu_driver, arm_pmu_matches);
> > return platform_driver_register(&pmu_driver);
> > }
> > device_initcall(register_pmu_driver);
> > --
> > 1.6.3.3
> >
> >
>
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
More information about the devicetree-discuss
mailing list