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

[Mike Leach]

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.
This however would necessitate a more comprehensive re-write.

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