[patch v1 2/3] arm: pmu: support pmu irq routed from CTI

Ming Lei tom.leiming at gmail.com
Wed Mar 2 05:10:28 EST 2011


Hi Will,

2011/3/2 Will Deacon <will.deacon at arm.com>:
> Hi Ming Lei,
>
> It's good to see somebody looking at OMAP4 since I've been hearing from
> people having trouble getting perf going on the PandaBoard.
>
>
> On Tue, 2011-03-01 at 16:58 +0000, tom.leiming at gmail.com wrote:
>> From: Ming Lei <tom.leiming at gmail.com>
>>
>> This patch introduces pmu_platform_data struct to
>> support pmu irq routed from CTI, such as implemented
>> on OMAP4.
>>
>> Generally speaking, clearing cti irq should be done in
>> irq handler, also enabling cti module after calling
>> request_irq and disabling cti module before calling
>> free_irq.
>>
>> Signed-off-by: Ming Lei <tom.leiming at gmail.com>
>> ---
>>  arch/arm/include/asm/pmu.h   |   12 +++++++++
>>  arch/arm/kernel/perf_event.c |   57 ++++++++++++++++++++++++++++++-----------
>>  2 files changed, 53 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
>> index 7544ce6..6162aaf 100644
>> --- a/arch/arm/include/asm/pmu.h
>> +++ b/arch/arm/include/asm/pmu.h
>> @@ -13,20 +13,32 @@
>>  #define __ARM_PMU_H__
>>
>>  #include <linux/interrupt.h>
>> +#include <asm/cti.h>
>>
>>  enum arm_pmu_type {
>>         ARM_PMU_DEVICE_CPU      = 0,
>>         ARM_NUM_PMU_DEVICES,
>>  };
>>
>> +#define MAX_CTI_NUM  4
>
> Why is this in the PMU header file rather than the CTI one?
>
>>  /*
>>   * struct arm_pmu_platdata - ARM PMU platform data
>>   *
>> + * @use_cti_irq: pmu irq is routed from cti
>> + * @cti_cnt: cti counts used for pmu irq routing
>> + * @cti: cti instances used to help access cti registers
>>   * @handle_irq: an optional handler which will be called from the interrupt and
>>   * passed the address of the low level handler, and can be used to implement
>>   * any platform specific handling before or after calling it.
>> + *
>> + * If pmu irq is routed from CTI, @use_cti_irq, @cti_cnt and
>> + * @cti must be initialized and passed to pmu driver.
>>   */
>>  struct arm_pmu_platdata {
>> +       int use_cti_irq;
>> +       int cti_cnt;
>> +       struct cti cti[MAX_CTI_NUM];
>> +
>>         irqreturn_t (*handle_irq)(int irq, void *dev,
>>                                   irq_handler_t pmu_handler);
>>  };
>> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
>> index 22e194eb..9027a8f 100644
>> --- a/arch/arm/kernel/perf_event.c
>> +++ b/arch/arm/kernel/perf_event.c
>> @@ -377,18 +377,39 @@ validate_group(struct perf_event *event)
>>         return 0;
>>  }
>>
>> -static irqreturn_t armpmu_platform_irq(int irq, void *dev)
>> +static inline int cti_irq(struct arm_pmu_platdata *data)
>>  {
>> -       struct arm_pmu_platdata *plat = dev_get_platdata(&pmu_device->dev);
>> +       return data && data->use_cti_irq;
>> +}
>> +
>> +static inline struct cti *irq_to_cti(struct arm_pmu_platdata *data,
>> +       int irq)
>> +{
>> +       int idx;
>>
>> -       return plat->handle_irq(irq, dev, armpmu->handle_irq);
>> +       for(idx = 0; idx < data->cti_cnt; idx++)
>> +               if (data->cti[idx].irq == irq)
>> +                       return &data->cti[idx];
>> +       return NULL;
>> +}
>
> Hmm, since OMAP is the only platform using this, I'm not especially
> happy sticking this directly in the PMU code.
>
>> +static inline irqreturn_t armpmu_handle_irq(int irq_num, void *dev)
>> +{
>> +       struct arm_pmu_platdata *plat = dev;
>> +
>> +       if (cti_irq(plat))
>> +               cti_irq_ack(irq_to_cti(plat, irq_num));
>> +
>> +       if (plat && plat->handle_irq)
>> +               return plat->handle_irq(irq_num, dev, armpmu->handle_irq);
>> +       else
>> +               return armpmu->handle_irq(irq_num, NULL);
>>  }
>>
> You can use the new arm_pmu_platdata.handle_irq function to register
> your own IRQ callback. I don't think we should define the handler here.
>
>>  static int
>>  armpmu_reserve_hardware(void)
>>  {
>>         struct arm_pmu_platdata *plat;
>> -       irq_handler_t handle_irq;
>>         int i, err = -ENODEV, irq;
>>
>>         pmu_device = reserve_pmu(ARM_PMU_DEVICE_CPU);
>> @@ -399,11 +420,7 @@ armpmu_reserve_hardware(void)
>>
>>         init_pmu(ARM_PMU_DEVICE_CPU);
>>
>> -       plat = dev_get_platdata(&pmu_device->dev);
>> -       if (plat && plat->handle_irq)
>> -               handle_irq = armpmu_platform_irq;
>> -       else
>> -               handle_irq = armpmu->handle_irq;
>> +       plat = platform_get_drvdata(pmu_device);
>>
>>         if (pmu_device->num_resources < 1) {
>>                 pr_err("no irqs for PMUs defined\n");
>> @@ -415,21 +432,25 @@ armpmu_reserve_hardware(void)
>>                 if (irq < 0)
>>                         continue;
>>
>> -               err = request_irq(irq, handle_irq,
>> +               err = request_irq(irq, armpmu_handle_irq,
>>                                   IRQF_DISABLED | IRQF_NOBALANCING,
>> -                                 "armpmu", NULL);
>> +                                 "armpmu", plat);
>>                 if (err) {
>>                         pr_warning("unable to request IRQ%d for ARM perf "
>>                                 "counters\n", irq);
>>                         break;
>> -               }
>> +               } else if (cti_irq(plat))
>> +                       cti_enable(irq_to_cti(plat, irq));
>>         }
>>
>>         if (err) {
>>                 for (i = i - 1; i >= 0; --i) {
>>                         irq = platform_get_irq(pmu_device, i);
>> -                       if (irq >= 0)
>> -                               free_irq(irq, NULL);
>> +                       if (irq >= 0) {
>> +                               if (cti_irq(plat))
>> +                                       cti_disable(irq_to_cti(plat, irq));
>> +                               free_irq(irq, plat);
>> +                       }
>>                 }
>>                 release_pmu(pmu_device);
>>                 pmu_device = NULL;
>> @@ -442,11 +463,15 @@ static void
>>  armpmu_release_hardware(void)
>>  {
>>         int i, irq;
>> +       struct arm_pmu_platdata *plat = platform_get_drvdata(pmu_device);
>>
>>         for (i = pmu_device->num_resources - 1; i >= 0; --i) {
>>                 irq = platform_get_irq(pmu_device, i);
>> -               if (irq >= 0)
>> -                       free_irq(irq, NULL);
>> +               if (irq >= 0) {
>> +                       if (cti_irq(plat))
>> +                               cti_disable(irq_to_cti(plat, irq));
>> +                       free_irq(irq, plat);
>> +               }
>>         }
>>         armpmu->stop();
>>
> Right, so I think a better way to do this is to put all the CTI stuff in
> its own file. Whether this lives in the OMAP BSP is up to you (perhaps
> the best place for it at the moment, until other platforms use it too).
>
> Then we can add ->enable and ->disable hooks in the platdata which are
> not CTI-specific and may also be useful for other platforms and PMU
> implementations.
>
> Does that work for you?

Good, I will move cti code out of perf_event.c and introduce
.enable_irq and .disable_irq into platdata, which makes perf_event.c
cleaner.

thanks,
-- 
Lei Ming



More information about the linux-arm-kernel mailing list