HI,
On Fri, 8 May 2020 at 09:53, Sai Prakash Ranjan
<saiprakash.ranjan@xxxxxxxxxxxxxx> 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
