Re: [PATCH] coresight: dynamic-replicator: Fix handling of multiple connections

[Mike Leach <mike.leach@linaro.org>]

HI,

On Fri, 8 May 2020 at 09:53, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi Suzuki, Mike,
>
> On 2020-05-06 13:05, Sai Prakash Ranjan wrote:
> [...]
>
> >>>>
> >>> OK - sorry I read your statement saying that replicator1 was 0 after
> >>> the reset in probe(), rather than look at the logs.
> >>>
> >>> From the logs it is working at the time probe() occurs, but by the
> >>> time we come to enable the replicator later, something has reset
> >>> these
> >>> registers / hardware outside the control of the replicator driver.
> >>>
> >>
> >> Yes, I will try to get some more information from the firmware side if
> >> there is anything messing up.
> >>
> >
> > This turned out to be a clock/pm issue. To confirm, I just marked clk
> > as critical
> > so that it won't be gated and I saw the replicator1(swao_replicator)
> > registers
> > intact after probe. Also alternatively, I tried to comment out
> > disabling pclk
> > to check if there is something wrong in amba pm and this keeps the
> > registers
> > intact as well.
> >
> > @@ -288,7 +295,7 @@ static int amba_probe(struct device *dev)
> >                 pm_runtime_set_suspended(dev);
> >                 pm_runtime_put_noidle(dev);
> >
> > -               amba_put_disable_pclk(pcdev);
> > +               //amba_put_disable_pclk(pcdev);
> >                 dev_pm_domain_detach(dev, true);
> >         } while (0);
> >
>
> I checked with the debug team and there is a limitation with
> the replicator(swao_replicator) in the AOSS group where it
> loses the idfilter register context when the clock is disabled.
> This is not just in SC7180 SoC but also reported on some latest
> upcoming QCOM SoCs as well and will need to be taken care in
> order to enable coresight on these chipsets.
>
> Here's what's happening -  After the replicator is initialized,
> the clock is disabled in amba_pm_runtime_suspend() as a part of
> pm runtime workqueue with the assumption that there will be no
> loss of context after the replicator is initialized. But it doesn't
> hold good with the replicators with these unfortunate limitation
> and the idfilter register context is lost.
>
> [    5.889406] amba_pm_runtime_suspend devname=6b06000.replicator ret=0
> [    5.914516] Workqueue: pm pm_runtime_work
> [    5.918648] Call trace:
> [    5.921185]  dump_backtrace+0x0/0x1d0
> [    5.924958]  show_stack+0x2c/0x38
> [    5.928382]  dump_stack+0xc0/0x104
> [    5.931896]  amba_pm_runtime_suspend+0xd8/0xe0
> [    5.936469]  __rpm_callback+0xe0/0x140
> [    5.940332]  rpm_callback+0x38/0x98
> [    5.943926]  rpm_suspend+0xec/0x618
> [    5.947522]  rpm_idle+0x5c/0x3f8
> [    5.950851]  pm_runtime_work+0xa8/0xc0
> [    5.954718]  process_one_work+0x1f8/0x4c0
> [    5.958848]  worker_thread+0x50/0x468
> [    5.962623]  kthread+0x12c/0x158
> [    5.965957]  ret_from_fork+0x10/0x1c
>
> This is a platform/SoC specific replicator issue, so we can either
> introduce some DT property for replicators to identify which replicator
> has this limitation, check in replicator_enable() and reset the
> registers
> or have something like below diff to check the idfilter registers in
> replicator_enable() and then reset with clear comment specifying it’s
> the
> hardware limitation on some QCOM SoCs. Please let me know your thoughts
> on
> this?
>

1) does this replicator part have a unique ID that differs from the
standard ARM designed replicators?
If so perhaps link the modification into this. (even if the part no in
PIDR0/1 is the same the UCI should be different for a different
implementation)

2) We have used DT properties in the past - (e.g. scatter gather in
TMC) where hardware compatibility issues have impacted on the
operation of a coresight component. This is further complicated by the
fact that an ACPI property would be needed as well.

3) The sysfs access to FILTERID0/1 on this replicator is going to show
different values than on a standard replicator (0x00 instead of 0xFF).
Does this need to be addressed?

4 ) An alternative approach could be to model the driver on the ETM /
CTI drivers where the register values are held in the driver structure
and only applied on enable / disable.

Thoughts?

Regards

Mike



> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c
> b/drivers/hwtracing/coresight/coresight-replicator.c
> index e7dc1c31d20d..a9c039c944eb 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -68,6 +68,17 @@ static int dynamic_replicator_enable(struct
> replicator_drvdata *drvdata,
>          int rc = 0;
>          u32 reg;
>
> +       /*
> +        * On some QCOM SoCs with replicators in Always-On domain,
> disabling
> +        * clock will result in replicator losing its context. Currently
> +        * as a part of pm_runtime workqueue, amba_pm_runtime_suspend
> disables
> +        * clock assuming the context is not lost which is not true for
> cases
> +        * with hardware limitations as the above.
> +        */
> +       if ((readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0) == 0)
> &&
> +           (readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1) == 0))
> +               dynamic_replicator_reset(drvdata);
> +
>          switch (outport) {
>          case 0:
>                  reg = REPLICATOR_IDFILTER0;
>
>
>
> Thanks,
> Sai
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member
> of Code Aurora Forum, hosted by The Linux Foundation



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK