Re: [PATCH] coresight: cti: Add power management to CTI driver

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Tue, 21 Apr 2020 at 04:34, Mike Leach <mike.leach@linaro.org> wrote:
>
> Hi Mathieu,
>
> On Mon, 20 Apr 2020 at 21:58, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > Good day Mike,
> >
> > On Wed, Apr 15, 2020 at 09:11:16PM +0100, Mike Leach wrote:
> > > Add in CPU hotplug and CPU idle power management handling to CTI driver.
> > >
> > > Only CPU bound CTIs will register with the PM routines, to enable PM of
> > > CTI alongside PM of ETM devices.
> > >
> > > Need separate HP PM event CPUHP_AP_ARM_CORESIGHT_CTI_STARTING as events
> > > can only be registered once.
> > >
> > > Applies to Linux 5.7-rc1, tested on platforms Juno-r1 and DB410c.
> > >
> > > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > > ---
> > >  drivers/hwtracing/coresight/coresight-cti.c | 163 +++++++++++++++++++-
> > >  include/linux/cpuhotplug.h                  |   1 +
> >
> > Please split this set in half, one for pm_runtime and another one for hotplug.
> >
>
> Will do.
>
> > >  2 files changed, 156 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
> > > index aa6e0249bd70..31996a20ce69 100644
> > > --- a/drivers/hwtracing/coresight/coresight-cti.c
> > > +++ b/drivers/hwtracing/coresight/coresight-cti.c
> > > @@ -5,6 +5,7 @@
> > >   */
> > >
> > >  #include <linux/property.h>
> > > +#include <linux/cpu_pm.h>
> > >  #include "coresight-cti.h"
> > >
> > >  /**
> > > @@ -27,6 +28,12 @@ static DEFINE_MUTEX(ect_mutex);
> > >  #define csdev_to_cti_drvdata(csdev)  \
> > >       dev_get_drvdata(csdev->dev.parent)
> > >
> > > +/* power management handling */
> > > +static int nr_cti_cpu;
> > > +
> > > +/* quick lookup list for CPU bound CTIs when power handling */
> > > +static struct cti_drvdata *cti_cpu_drvdata[NR_CPUS];
> > > +
> > >  /*
> > >   * CTI naming. CTI bound to cores will have the name cti_cpu<N> where
> > >   * N is the CPU ID. System CTIs will have the name cti_sys<I> where I
> > > @@ -36,7 +43,7 @@ static DEFINE_MUTEX(ect_mutex);
> > >   */
> > >  DEFINE_CORESIGHT_DEVLIST(cti_sys_devs, "cti_sys");
> > >
> > > -/* write set of regs to hardware - call with spinlock claimed */
> > > +/* write set of regs to hardware - call with spinlock claimed / cpu context */
> > >  void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
> > >  {
> > >       struct cti_config *config = &drvdata->config;
> > > @@ -73,7 +80,7 @@ static void cti_enable_hw_smp_call(void *info)
> > >  }
> > >
> > >  /* write regs to hardware and enable */
> > > -static int cti_enable_hw(struct cti_drvdata *drvdata)
> > > +static int cti_enable_hw(struct cti_drvdata *drvdata, bool hp_re_enable)
> >
> > s/hp_re_enable/hp_event
> >
> >
> > >  {
> > >       struct cti_config *config = &drvdata->config;
> > >       struct device *dev = &drvdata->csdev->dev;
> > > @e -82,8 +89,11 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
> > >       pm_runtime_get_sync(dev->parent);
> > >       spin_lock(&drvdata->spinlock);
> > >
> > > -     /* no need to do anything if enabled or unpowered*/
> > > -     if (config->hw_enabled || !config->hw_powered)
> > > +     /* no need to do anything if enabled or unpowered */
> > > +     if (!hp_re_enable) {
> > > +             if (config->hw_enabled || !config->hw_powered)
> > > +                     goto cti_state_unchanged;
> > > +     } else if (!atomic_read(&drvdata->config.enable_req_count))
> > >               goto cti_state_unchanged;
> > >
> > >       /* claim the device */
> > > @@ -91,7 +101,7 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
> > >       if (rc)
> > >               goto cti_err_not_enabled;
> > >
> > > -     if (drvdata->ctidev.cpu >= 0) {
> > > +     if (drvdata->ctidev.cpu >= 0 && !hp_re_enable) {
> > >               rc = smp_call_function_single(drvdata->ctidev.cpu,
> > >                                             cti_enable_hw_smp_call,
> > >                                             drvdata, 1);
> > > @@ -102,12 +112,15 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
> > >       }
> > >
> > >       config->hw_enabled = true;
> > > -     atomic_inc(&drvdata->config.enable_req_count);
> > > +     if (!hp_re_enable)
> > > +             atomic_inc(&drvdata->config.enable_req_count);
> > > +
> >
> > We are dealing with 2 different variables, i.e CPU CTI vs global CTI, hotplug
> > event vs framework event.  I wonder if we have just enough complexity to spin
> > off two flavours of cti_enable_hw()...
> >
>
> The complexity comes from the re-enable requirement not wanting to
> update any reference counting rather than the CPU / global CTI.
> On this basis I agree - perhaps cti_enable_hw() /
> cti_hp_re_enable_hw() would work - at the cost of some repeated code.

Right, there will indeed be some redundancy but I think it will be
well worth it.

>
>
> > >       spin_unlock(&drvdata->spinlock);
> > >       return rc;
> > >
> > >  cti_state_unchanged:
> > > -     atomic_inc(&drvdata->config.enable_req_count);
> > > +     if (!hp_re_enable)
> > > +             atomic_inc(&drvdata->config.enable_req_count);
> > >
> > >       /* cannot enable due to error */
> > >  cti_err_not_enabled:
> > > @@ -563,12 +576,127 @@ static void cti_remove_conn_xrefs(struct cti_drvdata *drvdata)
> > >       }
> > >  }
> > >
> > > +/** cti PM callbacks **/
> > > +static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> > > +                          void *v)
> > > +{
> > > +     struct cti_drvdata *drvdata;
> > > +     unsigned int cpu = smp_processor_id();
> > > +
> > > +     if (!cti_cpu_drvdata[cpu])
> > > +             return NOTIFY_OK;
> > > +
> > > +     drvdata = cti_cpu_drvdata[cpu];
> > > +
> > > +     if (WARN_ON_ONCE(drvdata->ctidev.cpu != cpu))
> > > +             return NOTIFY_BAD;
> > > +
> > > +     switch (cmd) {
> > > +     case CPU_PM_ENTER:
> > > +             /* CTI regs all static - we have a copy & nothing to save */
> > > +             drvdata->config.hw_powered = false;
> > > +             if (drvdata->config.hw_enabled)
> > > +                     coresight_disclaim_device(drvdata->base);
> > > +             break;
> > > +
> > > +     case CPU_PM_ENTER_FAILED:
> > > +             drvdata->config.hw_powered = true;
> > > +             if (drvdata->config.hw_enabled) {
> > > +                     if (coresight_claim_device(drvdata->base))
> > > +                             drvdata->config.hw_enabled = false;
> > > +             }
> > > +             break;
> > > +
> > > +     case CPU_PM_EXIT:
> > > +             /* write hardware registers to re-enable. */
> > > +             drvdata->config.hw_powered = true;
> > > +             drvdata->config.hw_enabled = false;
> > > +
> > > +             /* check enable reference count to enable HW */
> > > +             if (atomic_read(&drvdata->config.enable_req_count)) {
> > > +                     /* check we can claim the device as we re-power */
> > > +                     if (coresight_claim_device(drvdata->base))
> > > +                             return NOTIFY_OK;
> > > +
> > > +                     drvdata->config.hw_enabled = true;
> > > +                     cti_write_all_hw_regs(drvdata);
> > > +             }
> > > +             break;
> > > +     default:
> > > +             return NOTIFY_DONE;
> > > +     }
> >
> > For all of the above the config's hw_enabled and hw_powered are manipulated
> > without taking drvdata->spinlock.  Please use spin_trylock() and spin_unlock().
> >
>
> OK - good point.
>
> > > +
> > > +     return NOTIFY_OK;
> > > +}
> > > +
> > > +static struct notifier_block cti_cpu_pm_nb = {
> > > +     .notifier_call = cti_cpu_pm_notify,
> > > +};
> > > +
> > > +static int cti_cpu_pm_register(void)
> > > +{
> > > +     if (IS_ENABLED(CONFIG_CPU_PM))
> > > +             return cpu_pm_register_notifier(&cti_cpu_pm_nb);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void cti_cpu_pm_unregister(void)
> > > +{
> > > +     if (IS_ENABLED(CONFIG_CPU_PM))
> > > +             cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
> > > +}
> > > +
> > > +/* CPU HP handlers */
> > > +static int cti_starting_cpu(unsigned int cpu)
> > > +{
> > > +     struct cti_drvdata *drvdata = cti_cpu_drvdata[cpu];
> > > +
> > > +     if (!drvdata)
> > > +             return 0;
> > > +
> > > +     spin_lock(&drvdata->spinlock);
> > > +     drvdata->config.hw_powered = true;
> > > +     spin_unlock(&drvdata->spinlock);
> > > +
> > > +     cti_enable_hw(drvdata, true);
> > > +     return 0;
> > > +}
> > > +
> > > +static int cti_dying_cpu(unsigned int cpu)
> > > +{
> > > +     struct cti_drvdata *drvdata = cti_cpu_drvdata[cpu];
> > > +
> > > +     if (!drvdata)
> > > +             return 0;
> > > +
> > > +     spin_lock(&drvdata->spinlock);
> > > +     drvdata->config.hw_powered = false;
> > > +     coresight_disclaim_device(drvdata->base);
> > > +     spin_unlock(&drvdata->spinlock);
> > > +     return 0;
> > > +}
> > > +
> > > +/* release PM registrations */
> > > +static void cti_pm_release(struct cti_drvdata *drvdata)
> > > +{
> > > +     if (drvdata->ctidev.cpu >= 0) {
> > > +             if (--nr_cti_cpu == 0) {
> > > +                     cti_cpu_pm_unregister();
> > > +
> > > +                     cpuhp_remove_state_nocalls(
> > > +                             CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
> > > +             }
> > > +             cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
> > > +     }
> >
> >         if (drvdata->ctidev.cpu < 0)
> >                 return;
> >
> >         if (--nr_cti_cpu > 0)
> >                 return;
> >
> >         cti_cpu_pm_unregister();
> >         cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
> >         cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
> >
>
> Agreed.
>
> > > +
> > >  /** cti ect operations **/
> > >  int cti_enable(struct coresight_device *csdev)
> > >  {
> > >       struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
> > >
> > > -     return cti_enable_hw(drvdata);
> > > +     return cti_enable_hw(drvdata, false);
> > >  }
> > >
> > >  int cti_disable(struct coresight_device *csdev)
> > > @@ -598,6 +726,7 @@ static void cti_device_release(struct device *dev)
> > >
> > >       mutex_lock(&ect_mutex);
> > >       cti_remove_conn_xrefs(drvdata);
> > > +     cti_pm_release(drvdata);
> > >
> > >       /* remove from the list */
> > >       list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, node) {
> > > @@ -673,6 +802,23 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
> > >               goto err_out;
> > >       }
> > >
> > > +     /* setup CPU power management handling for CPU bound CTI devices. */
> > > +     if (drvdata->ctidev.cpu >= 0) {
> > > +             cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
> > > +             if (!nr_cti_cpu++) {
> > > +                     cpus_read_lock();
> > > +                     cpuhp_setup_state_nocalls_cpuslocked(
> > > +                             CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
> >
> > Any reasons CPUHP_AP_ARM_CORESIGHT_STARTING couldn't be reused?
> >
>
> Yes - any CPUHP constant can only be registered with the system once -
> a second attempt is rejected by the API. Last year I proposed moving
> the HP stuff out of the ETM driver into the generic coresight area,
> and developed a structure for any driver to use it - this approach was
> rejected by both you and Suzuki. We discussed @ the last connect and
> agreed that an additional CPUHP constant was the way to go.

What sparked this renewed interest is how CPUHP_AP_ONLINE_DYN is used
throughout the kernel without issues...  I wonder what is so special
about that one.

>
> Thanks for the feedback.
>
> Mike
>
> > Thanks,
> > Mathieu
> >
> > > +                             "arm/coresight_cti:starting",
> > > +                             cti_starting_cpu, cti_dying_cpu);
> > > +
> > > +                     ret = cti_cpu_pm_register();
> > > +                     cpus_read_unlock();
> > > +                     if (ret)
> > > +                             goto err_out;
> > > +             }
> > > +     }
> > > +
> > >       /* create dynamic attributes for connections */
> > >       ret = cti_create_cons_sysfs(dev, drvdata);
> > >       if (ret) {
> > > @@ -711,6 +857,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
> > >       return 0;
> > >
> > >  err_out:
> > > +     cti_pm_release(drvdata);
> > >       return ret;
> > >  }
> > >
> > > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > > index 77d70b633531..6dc7332307ca 100644
> > > --- a/include/linux/cpuhotplug.h
> > > +++ b/include/linux/cpuhotplug.h
> > > @@ -142,6 +142,7 @@ enum cpuhp_state {
> > >       CPUHP_AP_ARM_XEN_STARTING,
> > >       CPUHP_AP_ARM_KVMPV_STARTING,
> > >       CPUHP_AP_ARM_CORESIGHT_STARTING,
> > > +     CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
> > >       CPUHP_AP_ARM64_ISNDEP_STARTING,
> > >       CPUHP_AP_SMPCFD_DYING,
> > >       CPUHP_AP_X86_TBOOT_DYING,
> > > --
> > > 2.17.1
> > >
>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel