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

[Mathieu Poirier]

On Tue, 12 May 2020 at 05:49, Mike Leach <mike.leach@xxxxxxxxxx> wrote:
>
> HI,
>
> On Mon, 11 May 2020 at 15:41, Sai Prakash Ranjan
> <saiprakash.ranjan@xxxxxxxxxxxxxx> wrote:
> >
> > 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 have reviewed the replicator driver, and compared to all the other CS drivers.
> This driver appears to be the only one that sets hardware values in
> probe() and expects them to remain in place on enable, and uses that
> state for programming decisions later, despite telling the PM
> infrastructure that it is clear to suspend the device.
>
> Now we have a system where the replicator hardware is behaving
> differently under the driver, but is it behaving unreasonably?
> > >>>>
> > >>>> 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)
> > >>>
> I have reviewed the replicator driver, and compared to all the other CS drivers.
> This driver appears to be the only one that sets hardware values in
> probe() and expects them to remain in place on enable, and uses that
> state for programming decisions later, despite telling the PM
> infrastructure that it is clear to suspend the device.
>
> Now we have a system where the replicator hardware is behaving
> differently under the driver, but is it behaving unreasonably?
> > >>
> > >> 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.
> > >
> >
>
> I agree, if it can be established that the register values that make
> up UCI (pid0-4, devarch, devtype, PID:CLASS==0x9), can correctly
> identify the parts then a flag can be set in the probe() function and
> acted on during the enable() function.
>
> > 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?
> >
>
> This was a design decision made by the original driver writer. A
> normal AMBA device should not lose context due to clock removal (see
> drivers/amba/bus.c), so resetting in probe means this operation is
> done only once, rather than add overhead in the enable() function,and
> later decisions can be made according to the state of the registers
> set.
>
> As you have pointed out, for this replicator implementation  the
> context is unfortunately not retained when clocks are removed - so an
> alternative method is required.
>
> perhaps something like:-
>
> probe()
> ...
> if (match_id_non_persistent_state_regs(ID))
>     drvdata->check_filter_val_on_enable;
> ....
>
> and a re-write of enable:-
>
> enable()
> ...
> CS_UNLOCK()
> id0val = read(IDFILTER0);
> id1val = read(IDFILTER1);
>
> /* some replicator designs lose context when AMBA clocks are removed -
> check for this */
> if (drvdata->check_filter_val_on_enable && (id0val == id1val == 0x0))
>    id0val = id1val = 0xff;
>
> if(id0xal == id1val == 0xff)
>    rc =  claim_device()
>
> if (!rc)
>    switch (outport)
>       case 0: id0val  = 0x0; break
>       case 1: id1va; = 0x0; break;
>      default: rc = -EINVAL;
>
> if (!rc)
>    write(id0val);
>    write(id1val);
> CS_LOCK()
> return rc;
> ....
>
> Given that the access to the enable() function is predicated on a
> reference count per active port, there is also a case for dropping the
> check_filter_val_on_enable flag completely - once one port is active,
> then the device will remain enabled until both ports are inactive.
> This still allows for future development of selective filtering per
> port.
>
> One other point here - there is a case as I mentioned above for moving
> to a stored value model for the driver - as this is the only coresight
> driver that appears to set state in the probe() function rather than
> write all on enable.

I favour that option.  Looking at the funnel driver we may have the
same issue with the port configuration, but we can have a look at that
if/when it becomes an issue.

> This however would necessitate a more comprehensive re-write.

Maybe a little more effort but overall a better approach.

Thanks,
Mathieu

>
> Regards
>
> Mike
>
>
> > 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