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

[Sai Prakash Ranjan]

Hi Suzuki,

On 2020-05-11 20:00, Suzuki K Poulose wrote:
> On 05/11/2020 03:16 PM, Sai Prakash Ranjan wrote:
> > Hi Mike,
> >
> > On 2020-05-11 16:44, Mike Leach wrote:
> > [...]
> >
> > > > 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?
> >
> > Sorry for hurrying up and sending the patch - 
> > https://lore.kernel.org/patchwork/patch/1239923/.
> > I will send v2 based on further feedbacks here or there.
> >
> > > 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)
> >
> > pid=0x2bb909 for both replicators. So part number is same.
> > UCI will be different for different implementation(QCOM maybe 
> > different from ARM),
> > but will it be different for different replicators under the same 
> > impl(i.e., on QCOM).
>
> May be use PIDR4.DES_2 to match the Implementor and apply the work
> around for all QCOM replicators ?
>
> To me that sounds the best option.

Ok we can do this as well, but just for my understanding, why do we need 
to reset replicators
in replicator_probe() and not in replicator_enable()? Are we accessing 
anything before
we enable replicators?

Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation