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

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

[Sai Prakash Ranjan]

Since commit 30af4fb619e5 ("coresight: dynamic-replicator:
Handle multiple connections"), we do not make sure that
the other port is disabled when the dynamic replicator is
enabled. This is seen to cause the CPU hardlockup atleast
on SC7180 SoC with the following topology when enabling ETM
with ETR as the sink via sysfs. Since there is no trace id
logic in coresight yet to make use of multiple sinks in
parallel for different trace sessions, disable the other
port when one port is turned on.

       etm0_out
	  |
   apss_funnel_in0
          |
  apss_merge_funnel_in
          |
      funnel1_in4
	  |
   merge_funnel_in1
	  |
    swao_funnel_in
          |
        etf_in
	  |
  swao_replicator_in
          |
   replicator_in
	  |
        etr_in

  Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
  CPU: 7 PID: 0 Comm: swapper/7 Tainted: G S  B             5.4.25 #100
  Hardware name: Qualcomm Technologies, Inc. SC7180 IDP (DT)
  Call trace:
   dump_backtrace+0x0/0x188
   show_stack+0x20/0x2c
   dump_stack+0xdc/0x144
   panic+0x168/0x370
   arch_seccomp_spec_mitigate+0x0/0x14
   watchdog_timer_fn+0x68/0x290
   __hrtimer_run_queues+0x264/0x498
   hrtimer_interrupt+0xf0/0x22c
   arch_timer_handler_phys+0x40/0x50
   handle_percpu_devid_irq+0x8c/0x158
   __handle_domain_irq+0x84/0xc4
   gic_handle_irq+0x100/0x1c4
   el1_irq+0xbc/0x180
   arch_cpu_idle+0x3c/0x5c
   default_idle_call+0x1c/0x38
   do_idle+0x100/0x280
   cpu_startup_entry+0x24/0x28
   secondary_start_kernel+0x15c/0x170
  SMP: stopping secondary CPUs

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Tested-by: Stephen Boyd <swboyd@chromium.org>
---
Changes since RFC:
 * Reworded commit text and included the topology on SC7180.
---
 .../hwtracing/coresight/coresight-replicator.c    | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index e7dc1c31d20d..f4eaa38f8f43 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -66,14 +66,16 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
 				     int inport, int outport)
 {
 	int rc = 0;
-	u32 reg;
+	u32 reg0, reg1;
 
 	switch (outport) {
 	case 0:
-		reg = REPLICATOR_IDFILTER0;
+		reg0 = REPLICATOR_IDFILTER0;
+		reg1 = REPLICATOR_IDFILTER1;
 		break;
 	case 1:
-		reg = REPLICATOR_IDFILTER1;
+		reg0 = REPLICATOR_IDFILTER1;
+		reg1 = REPLICATOR_IDFILTER0;
 		break;
 	default:
 		WARN_ON(1);
@@ -87,8 +89,11 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
 		rc = coresight_claim_device_unlocked(drvdata->base);
 
 	/* Ensure that the outport is enabled. */
-	if (!rc)
-		writel_relaxed(0x00, drvdata->base + reg);
+	if (!rc) {
+		writel_relaxed(0x00, drvdata->base + reg0);
+		writel_relaxed(0xff, drvdata->base + reg1);
+	}
+
 	CS_LOCK(drvdata->base);
 
 	return rc;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Suzuki K Poulose]

On 04/26/2020 03:37 PM, Sai Prakash Ranjan wrote:
> Since commit 30af4fb619e5 ("coresight: dynamic-replicator:
> Handle multiple connections"), we do not make sure that
> the other port is disabled when the dynamic replicator is
> enabled. This is seen to cause the CPU hardlockup atleast
> on SC7180 SoC with the following topology when enabling ETM
> with ETR as the sink via sysfs. Since there is no trace id
> logic in coresight yet to make use of multiple sinks in
> parallel for different trace sessions, disable the other
> port when one port is turned on.
> 
>         etm0_out
> 	  |
>     apss_funnel_in0
>            |
>    apss_merge_funnel_in
>            |
>        funnel1_in4
> 	  |
>     merge_funnel_in1
> 	  |
>      swao_funnel_in
>            |
>          etf_in
> 	  |
>    swao_replicator_in
>            |
>     replicator_in
> 	  |
>          etr_in
> 
>    Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
>    CPU: 7 PID: 0 Comm: swapper/7 Tainted: G S  B             5.4.25 #100
>    Hardware name: Qualcomm Technologies, Inc. SC7180 IDP (DT)
>    Call trace:
>     dump_backtrace+0x0/0x188
>     show_stack+0x20/0x2c
>     dump_stack+0xdc/0x144
>     panic+0x168/0x370
>     arch_seccomp_spec_mitigate+0x0/0x14
>     watchdog_timer_fn+0x68/0x290
>     __hrtimer_run_queues+0x264/0x498
>     hrtimer_interrupt+0xf0/0x22c
>     arch_timer_handler_phys+0x40/0x50
>     handle_percpu_devid_irq+0x8c/0x158
>     __handle_domain_irq+0x84/0xc4
>     gic_handle_irq+0x100/0x1c4
>     el1_irq+0xbc/0x180
>     arch_cpu_idle+0x3c/0x5c
>     default_idle_call+0x1c/0x38
>     do_idle+0x100/0x280
>     cpu_startup_entry+0x24/0x28
>     secondary_start_kernel+0x15c/0x170
>    SMP: stopping secondary CPUs
> 
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> Tested-by: Stephen Boyd <swboyd@chromium.org>
> ---
> Changes since RFC:
>   * Reworded commit text and included the topology on SC7180.


> ---
>   .../hwtracing/coresight/coresight-replicator.c    | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index e7dc1c31d20d..f4eaa38f8f43 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -66,14 +66,16 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
>   				     int inport, int outport)
>   {
>   	int rc = 0;
> -	u32 reg;
> +	u32 reg0, reg1;
>   
>   	switch (outport) {
>   	case 0:
> -		reg = REPLICATOR_IDFILTER0;
> +		reg0 = REPLICATOR_IDFILTER0;
> +		reg1 = REPLICATOR_IDFILTER1;
>   		break;
>   	case 1:
> -		reg = REPLICATOR_IDFILTER1;
> +		reg0 = REPLICATOR_IDFILTER1;
> +		reg1 = REPLICATOR_IDFILTER0;
>   		break;
>   	default:
>   		WARN_ON(1);
> @@ -87,8 +89,11 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
>   		rc = coresight_claim_device_unlocked(drvdata->base);
>   
>   	/* Ensure that the outport is enabled. */
> -	if (!rc)
> -		writel_relaxed(0x00, drvdata->base + reg);
> +	if (!rc) {
> +		writel_relaxed(0x00, drvdata->base + reg0);
> +		writel_relaxed(0xff, drvdata->base + reg1);
> +	}
> +

This is not sufficient. You must prevent another session trying to
enable the other port of the replicator as this could silently fail
the "on-going" session. Not ideal. Fail the attempt to enable a port
if the other port is active. You could track this in software and
fail early.

Suzuki

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

[Mike Leach]

HI,

On Mon, 27 Apr 2020 at 10:15, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 04/26/2020 03:37 PM, Sai Prakash Ranjan wrote:
> > Since commit 30af4fb619e5 ("coresight: dynamic-replicator:
> > Handle multiple connections"), we do not make sure that
> > the other port is disabled when the dynamic replicator is
> > enabled. This is seen to cause the CPU hardlockup atleast
> > on SC7180 SoC with the following topology when enabling ETM
> > with ETR as the sink via sysfs. Since there is no trace id
> > logic in coresight yet to make use of multiple sinks in
> > parallel for different trace sessions, disable the other
> > port when one port is turned on.
> >
> >         etm0_out
> >         |
> >     apss_funnel_in0
> >            |
> >    apss_merge_funnel_in
> >            |
> >        funnel1_in4
> >         |
> >     merge_funnel_in1
> >         |
> >      swao_funnel_in
> >            |
> >          etf_in
> >         |
> >    swao_replicator_in
> >            |
> >     replicator_in
> >         |
> >          etr_in
> >
> >    Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
> >    CPU: 7 PID: 0 Comm: swapper/7 Tainted: G S  B             5.4.25 #100
> >    Hardware name: Qualcomm Technologies, Inc. SC7180 IDP (DT)
> >    Call trace:
> >     dump_backtrace+0x0/0x188
> >     show_stack+0x20/0x2c
> >     dump_stack+0xdc/0x144
> >     panic+0x168/0x370
> >     arch_seccomp_spec_mitigate+0x0/0x14
> >     watchdog_timer_fn+0x68/0x290
> >     __hrtimer_run_queues+0x264/0x498
> >     hrtimer_interrupt+0xf0/0x22c
> >     arch_timer_handler_phys+0x40/0x50
> >     handle_percpu_devid_irq+0x8c/0x158
> >     __handle_domain_irq+0x84/0xc4
> >     gic_handle_irq+0x100/0x1c4
> >     el1_irq+0xbc/0x180
> >     arch_cpu_idle+0x3c/0x5c
> >     default_idle_call+0x1c/0x38
> >     do_idle+0x100/0x280
> >     cpu_startup_entry+0x24/0x28
> >     secondary_start_kernel+0x15c/0x170
> >    SMP: stopping secondary CPUs
> >
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > Tested-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> > Changes since RFC:
> >   * Reworded commit text and included the topology on SC7180.
>
>
> > ---
> >   .../hwtracing/coresight/coresight-replicator.c    | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> > index e7dc1c31d20d..f4eaa38f8f43 100644
> > --- a/drivers/hwtracing/coresight/coresight-replicator.c
> > +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> > @@ -66,14 +66,16 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
> >                                    int inport, int outport)
> >   {
> >       int rc = 0;
> > -     u32 reg;
> > +     u32 reg0, reg1;
> >
> >       switch (outport) {
> >       case 0:
> > -             reg = REPLICATOR_IDFILTER0;
> > +             reg0 = REPLICATOR_IDFILTER0;
> > +             reg1 = REPLICATOR_IDFILTER1;
> >               break;
> >       case 1:
> > -             reg = REPLICATOR_IDFILTER1;
> > +             reg0 = REPLICATOR_IDFILTER1;
> > +             reg1 = REPLICATOR_IDFILTER0;
> >               break;
> >       default:
> >               WARN_ON(1);
> > @@ -87,8 +89,11 @@ static int dynamic_replicator_enable(struct replicator_drvdata *drvdata,
> >               rc = coresight_claim_device_unlocked(drvdata->base);
> >
> >       /* Ensure that the outport is enabled. */
> > -     if (!rc)
> > -             writel_relaxed(0x00, drvdata->base + reg);
> > +     if (!rc) {
> > +             writel_relaxed(0x00, drvdata->base + reg0);
> > +             writel_relaxed(0xff, drvdata->base + reg1);
> > +     }
> > +
>
> This is not sufficient. You must prevent another session trying to
> enable the other port of the replicator as this could silently fail
> the "on-going" session. Not ideal. Fail the attempt to enable a port
> if the other port is active. You could track this in software and
> fail early.
>
> Suzuki

While I have no issue in principle with not enabling a path to a sink
that is not in use - indeed in some cases attaching to unused sinks
can cause back-pressure that slows throughput (cf TPIU) - I am
concerned that this modification is masking an underlying issue with
the platform in question.

Should we decide to enable the diversion of different IDs to different
sinks or allow different sessions go to different sinks, then this has
potential to fail on the SC7180 SoC - and it will be difficult in
future to associate a problem with this discussion.

Regards

Mike




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

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

[Suzuki K Poulose]

On 04/27/2020 10:45 AM, Mike Leach wrote:
> HI,
> 
> On Mon, 27 Apr 2020 at 10:15, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> On 04/26/2020 03:37 PM, Sai Prakash Ranjan wrote:
>>> Since commit 30af4fb619e5 ("coresight: dynamic-replicator:
>>> Handle multiple connections"), we do not make sure that
>>> the other port is disabled when the dynamic replicator is
>>> enabled. This is seen to cause the CPU hardlockup atleast
>>> on SC7180 SoC with the following topology when enabling ETM
>>> with ETR as the sink via sysfs. Since there is no trace id
>>> logic in coresight yet to make use of multiple sinks in
>>> parallel for different trace sessions, disable the other
>>> port when one port is turned on.
>>>
>>>          etm0_out
>>>          |
>>>      apss_funnel_in0
>>>             |
>>>     apss_merge_funnel_in
>>>             |
>>>         funnel1_in4
>>>          |
>>>      merge_funnel_in1
>>>          |
>>>       swao_funnel_in
>>>             |
>>>           etf_in
>>>          |
>>>     swao_replicator_in
>>>             |
>>>      replicator_in
>>>          |
>>>           etr_in
>>>
>>>     Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
>>>     CPU: 7 PID: 0 Comm: swapper/7 Tainted: G S  B             5.4.25 #100
>>>     Hardware name: Qualcomm Technologies, Inc. SC7180 IDP (DT)
>>>     Call trace:
>>>      dump_backtrace+0x0/0x188
>>>      show_stack+0x20/0x2c
>>>      dump_stack+0xdc/0x144
>>>      panic+0x168/0x370
>>>      arch_seccomp_spec_mitigate+0x0/0x14
>>>      watchdog_timer_fn+0x68/0x290
>>>      __hrtimer_run_queues+0x264/0x498
>>>      hrtimer_interrupt+0xf0/0x22c
>>>      arch_timer_handler_phys+0x40/0x50
>>>      handle_percpu_devid_irq+0x8c/0x158
>>>      __handle_domain_irq+0x84/0xc4
>>>      gic_handle_irq+0x100/0x1c4
>>>      el1_irq+0xbc/0x180
>>>      arch_cpu_idle+0x3c/0x5c
>>>      default_idle_call+0x1c/0x38
>>>      do_idle+0x100/0x280
>>>      cpu_startup_entry+0x24/0x28
>>>      secondary_start_kernel+0x15c/0x170
>>>     SMP: stopping secondary CPUs
>>>
>>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>>> Tested-by: Stephen Boyd <swboyd@chromium.org>


>>
>> This is not sufficient. You must prevent another session trying to
>> enable the other port of the replicator as this could silently fail
>> the "on-going" session. Not ideal. Fail the attempt to enable a port
>> if the other port is active. You could track this in software and
>> fail early.
>>
>> Suzuki
> 
> While I have no issue in principle with not enabling a path to a sink
> that is not in use - indeed in some cases attaching to unused sinks
> can cause back-pressure that slows throughput (cf TPIU) - I am
> concerned that this modification is masking an underlying issue with
> the platform in question.
> 
> Should we decide to enable the diversion of different IDs to different
> sinks or allow different sessions go to different sinks, then this has
> potential to fail on the SC7180 SoC - and it will be difficult in
> future to associate a problem with this discussion.

Mike,

I think thats a good point.
Sai, please could we narrow down this to the real problem and may be
work around it for the "device" ? Do we know which sink is causing the
back pressure ? We could then push the "work around" to the replicator
it is connected to.

Suzuki

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

[Sai Prakash Ranjan]

On 2020-04-27 19:23, Suzuki K Poulose wrote:
> On 04/27/2020 10:45 AM, Mike Leach wrote:
[...]
>>> 
>>> This is not sufficient. You must prevent another session trying to
>>> enable the other port of the replicator as this could silently fail
>>> the "on-going" session. Not ideal. Fail the attempt to enable a port
>>> if the other port is active. You could track this in software and
>>> fail early.
>>> 
>>> Suzuki
>> 
>> While I have no issue in principle with not enabling a path to a sink
>> that is not in use - indeed in some cases attaching to unused sinks
>> can cause back-pressure that slows throughput (cf TPIU) - I am
>> concerned that this modification is masking an underlying issue with
>> the platform in question.
>> 
>> Should we decide to enable the diversion of different IDs to different
>> sinks or allow different sessions go to different sinks, then this has
>> potential to fail on the SC7180 SoC - and it will be difficult in
>> future to associate a problem with this discussion.
> 
> Mike,
> 
> I think thats a good point.
> Sai, please could we narrow down this to the real problem and may be
> work around it for the "device" ? Do we know which sink is causing the
> back pressure ? We could then push the "work around" to the replicator
> it is connected to.
> 
> Suzuki

Hi Suzuki, Mike,

To add some more to the information provided earlier, 
swao_replicator(6b06000) and etf are
in AOSS (Always-On-SubSystem) group. Also TPIU(connected to 
qdss_replicator) and EUD(connected
to swao_replicator) sinks are unused.

Please ignore the id filter values provided earlier.
Here are ID filter values after boot and before enabling replicator. As 
per
these idfilter values, we should not try to enable replicator if its 
already
enabled (in this case for swao_replicator) right?

localhost ~ # cat 
/sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter0
0x0
localhost ~ # cat 
/sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter1
0x0

localhost ~ # cat 
/sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter0
0xff
localhost ~ # cat 
/sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter1
0xff

I think the unused sink EUD(also in AOSS group) probably is causing the 
backpressure here.

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

On 2020-04-28 17:53, Sai Prakash Ranjan wrote:
> On 2020-04-27 19:23, Suzuki K Poulose wrote:
>> On 04/27/2020 10:45 AM, Mike Leach wrote:
> [...]
>>>> 
>>>> This is not sufficient. You must prevent another session trying to
>>>> enable the other port of the replicator as this could silently fail
>>>> the "on-going" session. Not ideal. Fail the attempt to enable a port
>>>> if the other port is active. You could track this in software and
>>>> fail early.
>>>> 
>>>> Suzuki
>>> 
>>> While I have no issue in principle with not enabling a path to a sink
>>> that is not in use - indeed in some cases attaching to unused sinks
>>> can cause back-pressure that slows throughput (cf TPIU) - I am
>>> concerned that this modification is masking an underlying issue with
>>> the platform in question.
>>> 
>>> Should we decide to enable the diversion of different IDs to 
>>> different
>>> sinks or allow different sessions go to different sinks, then this 
>>> has
>>> potential to fail on the SC7180 SoC - and it will be difficult in
>>> future to associate a problem with this discussion.
>> 
>> Mike,
>> 
>> I think thats a good point.
>> Sai, please could we narrow down this to the real problem and may be
>> work around it for the "device" ? Do we know which sink is causing the
>> back pressure ? We could then push the "work around" to the replicator
>> it is connected to.
>> 
>> Suzuki
> 
> Hi Suzuki, Mike,
> 
> To add some more to the information provided earlier,
> swao_replicator(6b06000) and etf are
> in AOSS (Always-On-SubSystem) group. Also TPIU(connected to
> qdss_replicator) and EUD(connected
> to swao_replicator) sinks are unused.
> 
> Please ignore the id filter values provided earlier.
> Here are ID filter values after boot and before enabling replicator. As 
> per
> these idfilter values, we should not try to enable replicator if its 
> already
> enabled (in this case for swao_replicator) right?
> 
> localhost ~ # cat
> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter0
> 0x0
> localhost ~ # cat
> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter1
> 0x0
> 
> localhost ~ # cat
> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter0
> 0xff
> localhost ~ # cat
> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter1
> 0xff
> 

Looking more into replicator1(swao_replicator) values as 0x0 even after 
replicator_reset()
in replicator probe, I added dynamic_replicator_reset in 
dynamic_replicator_enable()
and am not seeing any hardlockup. Also I added some prints to check the 
idfilter
values before and after reset and found that its not set to 0xff even 
after replicator_reset()
in replicator probe, I don't see any other path setting it to 0x0.

After probe:

[    8.477669] func replicator_probe before reset replicator replicator1 
REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
[    8.489470] func replicator_probe after reset replicator replicator1 
REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
[    8.502738] func replicator_probe before reset replicator replicator0 
REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
[    8.515214] func replicator_probe after reset replicator replicator0 
REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
localhost ~ #
localhost ~ #
localhost ~ # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
localhost ~ #
localhost ~ # echo 1 > /sys/bus/coresight/devices/etm0/enable_source
[   58.490485] func dynamic_replicator_enable before reset replicator 
replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
[   58.503246] func dynamic_replicator_enable after reset replicator 
replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
[   58.520902] func dynamic_replicator_enable before reset replicator 
replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
[   58.533500] func dynamic_replicator_enable after reset replicator 
replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
localhost ~ #

Can we have a replicator_reset in dynamic_replicator_enable?

diff --git a/drivers/hwtracing/coresight/coresight-replicator.c 
b/drivers/hwtracing/coresight/coresight-replicator.c
index e7dc1c31d20d..794f8e4c049f 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -68,6 +68,8 @@ static int dynamic_replicator_enable(struct 
replicator_drvdata *drvdata,
         int rc = 0;
         u32 reg;

+       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

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

[Suzuki K Poulose]

On 04/29/2020 12:47 PM, Sai Prakash Ranjan wrote:
> On 2020-04-28 17:53, Sai Prakash Ranjan wrote:
>> On 2020-04-27 19:23, Suzuki K Poulose wrote:
>>> On 04/27/2020 10:45 AM, Mike Leach wrote:
>> [...]
>>>>>
>>>>> This is not sufficient. You must prevent another session trying to
>>>>> enable the other port of the replicator as this could silently fail
>>>>> the "on-going" session. Not ideal. Fail the attempt to enable a port
>>>>> if the other port is active. You could track this in software and
>>>>> fail early.
>>>>>
>>>>> Suzuki
>>>>
>>>> While I have no issue in principle with not enabling a path to a sink
>>>> that is not in use - indeed in some cases attaching to unused sinks
>>>> can cause back-pressure that slows throughput (cf TPIU) - I am
>>>> concerned that this modification is masking an underlying issue with
>>>> the platform in question.
>>>>
>>>> Should we decide to enable the diversion of different IDs to different
>>>> sinks or allow different sessions go to different sinks, then this has
>>>> potential to fail on the SC7180 SoC - and it will be difficult in
>>>> future to associate a problem with this discussion.
>>>
>>> Mike,
>>>
>>> I think thats a good point.
>>> Sai, please could we narrow down this to the real problem and may be
>>> work around it for the "device" ? Do we know which sink is causing the
>>> back pressure ? We could then push the "work around" to the replicator
>>> it is connected to.
>>>
>>> Suzuki
>>
>> Hi Suzuki, Mike,
>>
>> To add some more to the information provided earlier,
>> swao_replicator(6b06000) and etf are
>> in AOSS (Always-On-SubSystem) group. Also TPIU(connected to
>> qdss_replicator) and EUD(connected
>> to swao_replicator) sinks are unused.
>>
>> Please ignore the id filter values provided earlier.
>> Here are ID filter values after boot and before enabling replicator. 
>> As per
>> these idfilter values, we should not try to enable replicator if its 
>> already
>> enabled (in this case for swao_replicator) right?
>>
>> localhost ~ # cat
>> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter0
>> 0x0
>> localhost ~ # cat
>> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter1
>> 0x0
>>
>> localhost ~ # cat
>> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter0
>> 0xff
>> localhost ~ # cat
>> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter1
>> 0xff
>>
> 
> Looking more into replicator1(swao_replicator) values as 0x0 even after 
> replicator_reset()
> in replicator probe, I added dynamic_replicator_reset in 
> dynamic_replicator_enable()
> and am not seeing any hardlockup. Also I added some prints to check the 
> idfilter
> values before and after reset and found that its not set to 0xff even 
> after replicator_reset()
> in replicator probe, I don't see any other path setting it to 0x0.
> 
> After probe:
> 
> [    8.477669] func replicator_probe before reset replicator replicator1 
> REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> [    8.489470] func replicator_probe after reset replicator replicator1 
> REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff

AFAICS, after the reset both of them are set to 0xff.

> [    8.502738] func replicator_probe before reset replicator replicator0 
> REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> [    8.515214] func replicator_probe after reset replicator replicator0 
> REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff



> localhost ~ #
> localhost ~ #
> localhost ~ # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
> localhost ~ #
> localhost ~ # echo 1 > /sys/bus/coresight/devices/etm0/enable_source
> [   58.490485] func dynamic_replicator_enable before reset replicator 
> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> [   58.503246] func dynamic_replicator_enable after reset replicator 
> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> [   58.520902] func dynamic_replicator_enable before reset replicator 
> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0

You need to find what is resetting the IDFILTERs to 0 for replicator1.

> [   58.533500] func dynamic_replicator_enable after reset replicator 
> replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> localhost ~ #
> 
> Can we have a replicator_reset in dynamic_replicator_enable?
> 
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c 
> b/drivers/hwtracing/coresight/coresight-replicator.c
> index e7dc1c31d20d..794f8e4c049f 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -68,6 +68,8 @@ static int dynamic_replicator_enable(struct 
> replicator_drvdata *drvdata,
>          int rc = 0;
>          u32 reg;
> 
> +       dynamic_replicator_reset(drvdata);
> +

Again you are trying to mask an issue with this. Is the firmware
using the replicator for anything ? If so, this needs to be claimed
to prevent us from using it.

Suzuki

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

[Sai Prakash Ranjan]

On 2020-04-29 19:19, Suzuki K Poulose wrote:
> On 04/29/2020 12:47 PM, Sai Prakash Ranjan wrote:
>> On 2020-04-28 17:53, Sai Prakash Ranjan wrote:
>>> On 2020-04-27 19:23, Suzuki K Poulose wrote:
>>>> On 04/27/2020 10:45 AM, Mike Leach wrote:
>>> [...]
>>>>>> 
>>>>>> This is not sufficient. You must prevent another session trying to
>>>>>> enable the other port of the replicator as this could silently 
>>>>>> fail
>>>>>> the "on-going" session. Not ideal. Fail the attempt to enable a 
>>>>>> port
>>>>>> if the other port is active. You could track this in software and
>>>>>> fail early.
>>>>>> 
>>>>>> Suzuki
>>>>> 
>>>>> While I have no issue in principle with not enabling a path to a 
>>>>> sink
>>>>> that is not in use - indeed in some cases attaching to unused sinks
>>>>> can cause back-pressure that slows throughput (cf TPIU) - I am
>>>>> concerned that this modification is masking an underlying issue 
>>>>> with
>>>>> the platform in question.
>>>>> 
>>>>> Should we decide to enable the diversion of different IDs to 
>>>>> different
>>>>> sinks or allow different sessions go to different sinks, then this 
>>>>> has
>>>>> potential to fail on the SC7180 SoC - and it will be difficult in
>>>>> future to associate a problem with this discussion.
>>>> 
>>>> Mike,
>>>> 
>>>> I think thats a good point.
>>>> Sai, please could we narrow down this to the real problem and may be
>>>> work around it for the "device" ? Do we know which sink is causing 
>>>> the
>>>> back pressure ? We could then push the "work around" to the 
>>>> replicator
>>>> it is connected to.
>>>> 
>>>> Suzuki
>>> 
>>> Hi Suzuki, Mike,
>>> 
>>> To add some more to the information provided earlier,
>>> swao_replicator(6b06000) and etf are
>>> in AOSS (Always-On-SubSystem) group. Also TPIU(connected to
>>> qdss_replicator) and EUD(connected
>>> to swao_replicator) sinks are unused.
>>> 
>>> Please ignore the id filter values provided earlier.
>>> Here are ID filter values after boot and before enabling replicator. 
>>> As per
>>> these idfilter values, we should not try to enable replicator if its 
>>> already
>>> enabled (in this case for swao_replicator) right?
>>> 
>>> localhost ~ # cat
>>> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter0
>>> 0x0
>>> localhost ~ # cat
>>> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter1
>>> 0x0
>>> 
>>> localhost ~ # cat
>>> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter0
>>> 0xff
>>> localhost ~ # cat
>>> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter1
>>> 0xff
>>> 
>> 
>> Looking more into replicator1(swao_replicator) values as 0x0 even 
>> after replicator_reset()
>> in replicator probe, I added dynamic_replicator_reset in 
>> dynamic_replicator_enable()
>> and am not seeing any hardlockup. Also I added some prints to check 
>> the idfilter
>> values before and after reset and found that its not set to 0xff even 
>> after replicator_reset()
>> in replicator probe, I don't see any other path setting it to 0x0.
>> 
>> After probe:
>> 
>> [    8.477669] func replicator_probe before reset replicator 
>> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
>> [    8.489470] func replicator_probe after reset replicator 
>> replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> 
> AFAICS, after the reset both of them are set to 0xff.

Yes I see this too as we call replicator_reset() in probe. What I wanted 
to highlight was the below part where it is set to 0x0 before enabling 
dynamic replicator.

> 
>> [    8.502738] func replicator_probe before reset replicator 
>> replicator0 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
>> [    8.515214] func replicator_probe after reset replicator 
>> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> 
> 
> 
>> localhost ~ #
>> localhost ~ #
>> localhost ~ # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
>> localhost ~ #
>> localhost ~ # echo 1 > /sys/bus/coresight/devices/etm0/enable_source
>> [   58.490485] func dynamic_replicator_enable before reset replicator 
>> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> [   58.503246] func dynamic_replicator_enable after reset replicator 
>> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> [   58.520902] func dynamic_replicator_enable before reset replicator 
>> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> 
> You need to find what is resetting the IDFILTERs to 0 for replicator1.
> 

That is right.

>> [   58.533500] func dynamic_replicator_enable after reset replicator 
>> replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> localhost ~ #
>> 
>> Can we have a replicator_reset in dynamic_replicator_enable?
>> 
>> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c 
>> b/drivers/hwtracing/coresight/coresight-replicator.c
>> index e7dc1c31d20d..794f8e4c049f 100644
>> --- a/drivers/hwtracing/coresight/coresight-replicator.c
>> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
>> @@ -68,6 +68,8 @@ static int dynamic_replicator_enable(struct 
>> replicator_drvdata *drvdata,
>>          int rc = 0;
>>          u32 reg;
>> 
>> +       dynamic_replicator_reset(drvdata);
>> +
> 
> Again you are trying to mask an issue with this. Is the firmware
> using the replicator for anything ? If so, this needs to be claimed
> to prevent us from using it.
> 

I was trying to narrow down further as you suggested. There are other 
ETMs like AOP ETM which use this replicator, will need to check with the 
firmware team for details.

Thanks,
Sai

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

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

[Mike Leach]

Hi,

On Wed, 29 Apr 2020 at 14:59, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> On 2020-04-29 19:19, Suzuki K Poulose wrote:
> > On 04/29/2020 12:47 PM, Sai Prakash Ranjan wrote:
> >> On 2020-04-28 17:53, Sai Prakash Ranjan wrote:
> >>> On 2020-04-27 19:23, Suzuki K Poulose wrote:
> >>>> On 04/27/2020 10:45 AM, Mike Leach wrote:
> >>> [...]
> >>>>>>
> >>>>>> This is not sufficient. You must prevent another session trying to
> >>>>>> enable the other port of the replicator as this could silently
> >>>>>> fail
> >>>>>> the "on-going" session. Not ideal. Fail the attempt to enable a
> >>>>>> port
> >>>>>> if the other port is active. You could track this in software and
> >>>>>> fail early.
> >>>>>>
> >>>>>> Suzuki
> >>>>>
> >>>>> While I have no issue in principle with not enabling a path to a
> >>>>> sink
> >>>>> that is not in use - indeed in some cases attaching to unused sinks
> >>>>> can cause back-pressure that slows throughput (cf TPIU) - I am
> >>>>> concerned that this modification is masking an underlying issue
> >>>>> with
> >>>>> the platform in question.
> >>>>>
> >>>>> Should we decide to enable the diversion of different IDs to
> >>>>> different
> >>>>> sinks or allow different sessions go to different sinks, then this
> >>>>> has
> >>>>> potential to fail on the SC7180 SoC - and it will be difficult in
> >>>>> future to associate a problem with this discussion.
> >>>>
> >>>> Mike,
> >>>>
> >>>> I think thats a good point.
> >>>> Sai, please could we narrow down this to the real problem and may be
> >>>> work around it for the "device" ? Do we know which sink is causing
> >>>> the
> >>>> back pressure ? We could then push the "work around" to the
> >>>> replicator
> >>>> it is connected to.
> >>>>
> >>>> Suzuki
> >>>
> >>> Hi Suzuki, Mike,
> >>>
> >>> To add some more to the information provided earlier,
> >>> swao_replicator(6b06000) and etf are
> >>> in AOSS (Always-On-SubSystem) group. Also TPIU(connected to
> >>> qdss_replicator) and EUD(connected
> >>> to swao_replicator) sinks are unused.
> >>>
> >>> Please ignore the id filter values provided earlier.
> >>> Here are ID filter values after boot and before enabling replicator.
> >>> As per
> >>> these idfilter values, we should not try to enable replicator if its
> >>> already
> >>> enabled (in this case for swao_replicator) right?
> >>>
> >>> localhost ~ # cat
> >>> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter0
> >>> 0x0
> >>> localhost ~ # cat
> >>> /sys/bus/amba/devices/6b06000.replicator/replicator1/mgmt/idfilter1
> >>> 0x0
> >>>
> >>> localhost ~ # cat
> >>> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter0
> >>> 0xff
> >>> localhost ~ # cat
> >>> /sys/bus/amba/devices/6046000.replicator/replicator0/mgmt/idfilter1
> >>> 0xff
> >>>
> >>
> >> Looking more into replicator1(swao_replicator) values as 0x0 even
> >> after replicator_reset()
> >> in replicator probe, I added dynamic_replicator_reset in
> >> dynamic_replicator_enable()
> >> and am not seeing any hardlockup. Also I added some prints to check
> >> the idfilter
> >> values before and after reset and found that its not set to 0xff even
> >> after replicator_reset()
> >> in replicator probe, I don't see any other path setting it to 0x0.
> >>
> >> After probe:
> >>
> >> [    8.477669] func replicator_probe before reset replicator
> >> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> >> [    8.489470] func replicator_probe after reset replicator
> >> replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >
> > AFAICS, after the reset both of them are set to 0xff.
>
> Yes I see this too as we call replicator_reset() in probe. What I wanted
> to highlight was the below part where it is set to 0x0 before enabling
> dynamic replicator.
>
> >
> >> [    8.502738] func replicator_probe before reset replicator
> >> replicator0 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> >> [    8.515214] func replicator_probe after reset replicator
> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >
> >
> >
> >> localhost ~ #
> >> localhost ~ #
> >> localhost ~ # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
> >> localhost ~ #
> >> localhost ~ # echo 1 > /sys/bus/coresight/devices/etm0/enable_source
> >> [   58.490485] func dynamic_replicator_enable before reset replicator
> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >> [   58.503246] func dynamic_replicator_enable after reset replicator
> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >> [   58.520902] func dynamic_replicator_enable before reset replicator
> >> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> >
> > You need to find what is resetting the IDFILTERs to 0 for replicator1.
> >
>
> That is right.
>

By default all replicators have the IDFILTER registers set to 0 out of
hardware reset. This ensures that programmable replicators behave in
the same way as non-programmable replicators out of reset.

The  dynamic_replicator_reset() is of course a driver state reset -
which filters out all trace on the output ports. The trace is then
enabled when we set the trace path from source to sink.

It seems to me that you have 2 problems that need solving here:

1) Why does the reset_replicator() called from probe() _not_ work
correctly on replicator 1? It seems to work later if you introduce a
reset after more of the system has powered and booted. This is
startiing to look a little like a PM / clocking issue.

This failure is causing the state when we are trying to set an output
port that both branches of this replicator are enabled for output.
In effect for this replicator, setting the output port has no effect
as it is already enabled.

2) Why does having both ports of this repilicator enabled cause a hard
lockup? This is a separate hardware  / system issue.

The worst that should happen if both branches of a replicator are
enabled is that you get undesirable back pressure. (e.g. there is a
system we have seen - I think it is Juno - where there is a static
replicator feeding the TPIU and ETR - we need to disable the TPIU to
prevent undesired back pressure).

Regards

Mike


> >> [   58.533500] func dynamic_replicator_enable after reset replicator
> >> replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >> localhost ~ #
> >>
> >> Can we have a replicator_reset in dynamic_replicator_enable?
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c
> >> b/drivers/hwtracing/coresight/coresight-replicator.c
> >> index e7dc1c31d20d..794f8e4c049f 100644
> >> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> >> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> >> @@ -68,6 +68,8 @@ static int dynamic_replicator_enable(struct
> >> replicator_drvdata *drvdata,
> >>          int rc = 0;
> >>          u32 reg;
> >>
> >> +       dynamic_replicator_reset(drvdata);
> >> +
> >
> > Again you are trying to mask an issue with this. Is the firmware
> > using the replicator for anything ? If so, this needs to be claimed
> > to prevent us from using it.
> >
>
> I was trying to narrow down further as you suggested. There are other
> ETMs like AOP ETM which use this replicator, will need to check with the
> firmware team for details.
>
> 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

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

[Sai Prakash Ranjan]

Hi Mike,

On 2020-04-29 19:57, Mike Leach wrote:
> Hi,
> 

[...]

>> >> Looking more into replicator1(swao_replicator) values as 0x0 even
>> >> after replicator_reset()
>> >> in replicator probe, I added dynamic_replicator_reset in
>> >> dynamic_replicator_enable()
>> >> and am not seeing any hardlockup. Also I added some prints to check
>> >> the idfilter
>> >> values before and after reset and found that its not set to 0xff even
>> >> after replicator_reset()
>> >> in replicator probe, I don't see any other path setting it to 0x0.
>> >>
>> >> After probe:
>> >>
>> >> [    8.477669] func replicator_probe before reset replicator
>> >> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
>> >> [    8.489470] func replicator_probe after reset replicator
>> >> replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> >
>> > AFAICS, after the reset both of them are set to 0xff.
>> 
>> Yes I see this too as we call replicator_reset() in probe. What I 
>> wanted
>> to highlight was the below part where it is set to 0x0 before enabling
>> dynamic replicator.
>> 
>> >
>> >> [    8.502738] func replicator_probe before reset replicator
>> >> replicator0 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
>> >> [    8.515214] func replicator_probe after reset replicator
>> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> >
>> >
>> >
>> >> localhost ~ #
>> >> localhost ~ #
>> >> localhost ~ # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
>> >> localhost ~ #
>> >> localhost ~ # echo 1 > /sys/bus/coresight/devices/etm0/enable_source
>> >> [   58.490485] func dynamic_replicator_enable before reset replicator
>> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> >> [   58.503246] func dynamic_replicator_enable after reset replicator
>> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> >> [   58.520902] func dynamic_replicator_enable before reset replicator
>> >> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
>> >
>> > You need to find what is resetting the IDFILTERs to 0 for replicator1.
>> >
>> 
>> That is right.
>> 
> 
> By default all replicators have the IDFILTER registers set to 0 out of
> hardware reset. This ensures that programmable replicators behave in
> the same way as non-programmable replicators out of reset.
> 
> The  dynamic_replicator_reset() is of course a driver state reset -
> which filters out all trace on the output ports. The trace is then
> enabled when we set the trace path from source to sink.
> 

Thanks for these explanations.

> It seems to me that you have 2 problems that need solving here:
> 
> 1) Why does the reset_replicator() called from probe() _not_ work
> correctly on replicator 1? It seems to work later if you introduce a
> reset after more of the system has powered and booted. This is
> startiing to look a little like a PM / clocking issue.

reset_replicator() does work in probe correctly for both replicators, 
below logs is collected before and after reset in probe. It is later 
that it's set back to 0x0 and hence the suggestion to look at firmware 
using this replicator1.

[    8.477669] func replicator_probe before reset replicator replicator1 
REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
[    8.489470] func replicator_probe after reset replicator replicator1 
REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff

> 
> This failure is causing the state when we are trying to set an output
> port that both branches of this replicator are enabled for output.
> In effect for this replicator, setting the output port has no effect
> as it is already enabled.
> 
> 2) Why does having both ports of this repilicator enabled cause a hard
> lockup? This is a separate hardware  / system issue.
> 
> The worst that should happen if both branches of a replicator are
> enabled is that you get undesirable back pressure. (e.g. there is a
> system we have seen - I think it is Juno - where there is a static
> replicator feeding the TPIU and ETR - we need to disable the TPIU to
> prevent undesired back pressure).
> 

Ok so hardlockup is not expected because of this backpressure.

Thanks,
Sai

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

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

[Mike Leach]

Hi,

On Wed, 29 Apr 2020 at 15:48, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi Mike,
>
> On 2020-04-29 19:57, Mike Leach wrote:
> > Hi,
> >
>
> [...]
>
> >> >> Looking more into replicator1(swao_replicator) values as 0x0 even
> >> >> after replicator_reset()
> >> >> in replicator probe, I added dynamic_replicator_reset in
> >> >> dynamic_replicator_enable()
> >> >> and am not seeing any hardlockup. Also I added some prints to check
> >> >> the idfilter
> >> >> values before and after reset and found that its not set to 0xff even
> >> >> after replicator_reset()
> >> >> in replicator probe, I don't see any other path setting it to 0x0.
> >> >>
> >> >> After probe:
> >> >>
> >> >> [    8.477669] func replicator_probe before reset replicator
> >> >> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> >> >> [    8.489470] func replicator_probe after reset replicator
> >> >> replicator1 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >> >
> >> > AFAICS, after the reset both of them are set to 0xff.
> >>
> >> Yes I see this too as we call replicator_reset() in probe. What I
> >> wanted
> >> to highlight was the below part where it is set to 0x0 before enabling
> >> dynamic replicator.
> >>
> >> >
> >> >> [    8.502738] func replicator_probe before reset replicator
> >> >> replicator0 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> >> >> [    8.515214] func replicator_probe after reset replicator
> >> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >> >
> >> >
> >> >
> >> >> localhost ~ #
> >> >> localhost ~ #
> >> >> localhost ~ # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
> >> >> localhost ~ #
> >> >> localhost ~ # echo 1 > /sys/bus/coresight/devices/etm0/enable_source
> >> >> [   58.490485] func dynamic_replicator_enable before reset replicator
> >> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >> >> [   58.503246] func dynamic_replicator_enable after reset replicator
> >> >> replicator0 REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
> >> >> [   58.520902] func dynamic_replicator_enable before reset replicator
> >> >> replicator1 REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> >> >
> >> > You need to find what is resetting the IDFILTERs to 0 for replicator1.
> >> >
> >>
> >> That is right.
> >>
> >
> > By default all replicators have the IDFILTER registers set to 0 out of
> > hardware reset. This ensures that programmable replicators behave in
> > the same way as non-programmable replicators out of reset.
> >
> > The  dynamic_replicator_reset() is of course a driver state reset -
> > which filters out all trace on the output ports. The trace is then
> > enabled when we set the trace path from source to sink.
> >
>
> Thanks for these explanations.
>
> > It seems to me that you have 2 problems that need solving here:
> >
> > 1) Why does the reset_replicator() called from probe() _not_ work
> > correctly on replicator 1? It seems to work later if you introduce a
> > reset after more of the system has powered and booted. This is
> > startiing to look a little like a PM / clocking issue.
>
> reset_replicator() does work in probe correctly for both replicators,
> below logs is collected before and after reset in probe. It is later
> that it's set back to 0x0 and hence the suggestion to look at firmware
> using this replicator1.
>
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.


> [    8.477669] func replicator_probe before reset replicator replicator1
> REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
> [    8.489470] func replicator_probe after reset replicator replicator1
> REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>
> >
> > This failure is causing the state when we are trying to set an output
> > port that both branches of this replicator are enabled for output.
> > In effect for this replicator, setting the output port has no effect
> > as it is already enabled.
> >
> > 2) Why does having both ports of this repilicator enabled cause a hard
> > lockup? This is a separate hardware  / system issue.
> >
> > The worst that should happen if both branches of a replicator are
> > enabled is that you get undesirable back pressure. (e.g. there is a
> > system we have seen - I think it is Juno - where there is a static
> > replicator feeding the TPIU and ETR - we need to disable the TPIU to
> > prevent undesired back pressure).
> >
>
> Ok so hardlockup is not expected because of this backpressure.
>

Hardlockup is not expected, but this is not related to any possible
backpressure.

Ordinarily having both legs of a replicator enabled should not cause
system failure.

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

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

[Sai Prakash Ranjan]

Hi Mike,

On 2020-04-29 22:28, Mike Leach wrote:
> Hi,
> 

[...]

>> >> > You need to find what is resetting the IDFILTERs to 0 for replicator1.
>> >> >
>> >>
>> >> That is right.
>> >>
>> >
>> > By default all replicators have the IDFILTER registers set to 0 out of
>> > hardware reset. This ensures that programmable replicators behave in
>> > the same way as non-programmable replicators out of reset.
>> >
>> > The  dynamic_replicator_reset() is of course a driver state reset -
>> > which filters out all trace on the output ports. The trace is then
>> > enabled when we set the trace path from source to sink.
>> >
>> 
>> Thanks for these explanations.
>> 
>> > It seems to me that you have 2 problems that need solving here:
>> >
>> > 1) Why does the reset_replicator() called from probe() _not_ work
>> > correctly on replicator 1? It seems to work later if you introduce a
>> > reset after more of the system has powered and booted. This is
>> > startiing to look a little like a PM / clocking issue.
>> 
>> reset_replicator() does work in probe correctly for both replicators,
>> below logs is collected before and after reset in probe. It is later
>> that it's set back to 0x0 and hence the suggestion to look at firmware
>> using this replicator1.
>> 
> 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.

> 
>> [    8.477669] func replicator_probe before reset replicator 
>> replicator1
>> REPLICATOR_IDFILTER0=0x0 REPLICATOR_IDFILTER1=0x0
>> [    8.489470] func replicator_probe after reset replicator 
>> replicator1
>> REPLICATOR_IDFILTER0=0xff REPLICATOR_IDFILTER1=0xff
>> 
>> >
>> > This failure is causing the state when we are trying to set an output
>> > port that both branches of this replicator are enabled for output.
>> > In effect for this replicator, setting the output port has no effect
>> > as it is already enabled.
>> >
>> > 2) Why does having both ports of this repilicator enabled cause a hard
>> > lockup? This is a separate hardware  / system issue.
>> >
>> > The worst that should happen if both branches of a replicator are
>> > enabled is that you get undesirable back pressure. (e.g. there is a
>> > system we have seen - I think it is Juno - where there is a static
>> > replicator feeding the TPIU and ETR - we need to disable the TPIU to
>> > prevent undesired back pressure).
>> >
>> 
>> Ok so hardlockup is not expected because of this backpressure.
>> 
> 
> Hardlockup is not expected, but this is not related to any possible
> backpressure.
> 
> Ordinarily having both legs of a replicator enabled should not cause
> system failure.
> 

Ok got it, thanks.

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

Hi Suzuki, Mike,

On 2020-04-29 22:41, Sai Prakash Ranjan wrote:
> Hi Mike,
> 
> On 2020-04-29 22:28, Mike Leach wrote:
>> Hi,
>> 
> 
> [...]
> 
>>> >> > You need to find what is resetting the IDFILTERs to 0 for replicator1.
>>> >> >
>>> >>
>>> >> That is right.
>>> >>
>>> >
>>> > By default all replicators have the IDFILTER registers set to 0 out of
>>> > hardware reset. This ensures that programmable replicators behave in
>>> > the same way as non-programmable replicators out of reset.
>>> >
>>> > The  dynamic_replicator_reset() is of course a driver state reset -
>>> > which filters out all trace on the output ports. The trace is then
>>> > enabled when we set the trace path from source to sink.
>>> >
>>> 
>>> Thanks for these explanations.
>>> 
>>> > It seems to me that you have 2 problems that need solving here:
>>> >
>>> > 1) Why does the reset_replicator() called from probe() _not_ work
>>> > correctly on replicator 1? It seems to work later if you introduce a
>>> > reset after more of the system has powered and booted. This is
>>> > startiing to look a little like a PM / clocking issue.
>>> 
>>> reset_replicator() does work in probe correctly for both replicators,
>>> below logs is collected before and after reset in probe. It is later
>>> that it's set back to 0x0 and hence the suggestion to look at 
>>> firmware
>>> using this replicator1.
>>> 
>> 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);

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

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?

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

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

[Mike Leach]

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

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

[Sai Prakash Ranjan]

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).

> 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.
> 

Yes, this was also one option which I had mentioned. But as you said we 
need
to have an ACPI property as well and these systems with limitations are 
DT based.

> 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?
> 

I don't think we need to change this because its actually showing the 
right
values for the replicator.

> 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.
> 

This looks good to me since we really don't need to reset replicator in 
probe,
we need to reset it only in replicator_enable() and that ensures clocks 
are enabled
without having to assume things(from amba) about context being lost or 
not when
clocks are disabled since that is implementation defined anyways.

But, why can't we just move replicator_reset() from probe to 
replicator_enable()?
Anything wrong with it? Seems like right thing to do because we will be 
having
clocks enabled when we touch the replicator registers and only in the 
enable
path.

Thanks,
Sai

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

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

[Suzuki K Poulose]

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.

Suzuki

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

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@codeaurora.org> 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

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

[Mathieu Poirier]

On Tue, 12 May 2020 at 05:49, Mike Leach <mike.leach@linaro.org> wrote:
>
> HI,
>
> On Mon, 11 May 2020 at 15:41, Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> 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

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

[Sai Prakash Ranjan]

Hi Mike,

On 2020-05-12 17:19, Mike Leach wrote:
[...]

>> >>
>> >> 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?

Thanks for taking your time to review this. For new replicator behaving
unreasonably, I think the assumption that the context is not lost on
disabling clock is flawed since its implementation defined. Is such
assumption documented in any TRM?

>> >>
>> >> 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.
> 

So here I have a doubt as to why we need to use UCI because PID = 
0x2bb909
and CID = 0xb105900d are same for both replicators, so UCI won't 
identify the
different replicators(in same implementation i.e., on QCOM) here.
Am I missing something?

Thats why I think Suzuki suggested to use PIDR4_DES2 and check for QCOM 
impl
and add a workaround for all replicators, something like below: (will 
need cleaning)

#define PIDR4_DES2	0xFD0

if (FIELD_GET(GENMASK(3, 0), readl_relaxed(drvdata->base + PIDR4_DES2)) 
== 0x4)
	id0val = id1val = 0xff;

... and the rest as you suggested.

> 
> 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;
> ....
> 

Thanks for this detailed idea for workaround. I will add this once we
know whether we need to use UCI or PIDR4_DES2.

> 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.
> 

I would defer this to experts as you or suzuki will have more idea
regarding this than me.

Thanks,
Sai

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

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

[Mike Leach]

HI Sai,

On Tue, 12 May 2020 at 18:46, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi Mike,
>
> On 2020-05-12 17:19, Mike Leach wrote:
> [...]
>
> >> >>
> >> >> 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?
>
> Thanks for taking your time to review this. For new replicator behaving
> unreasonably, I think the assumption that the context is not lost on
> disabling clock is flawed since its implementation defined. Is such
> assumption documented in any TRM?
>

Looking at the AMBA driver there is a comment there that AMBA does not
lose state when clocks are removed. This is consistent with the AMBA
protocol spec which states that AMBA slaves can only be accessed /
read / write on various strobe signals,  or state reset on PRESET
signal, all timed by the rising edge of the bus clock. state changes
are not permitted on clock events alone. Given this static nature of
AMBA slaves then removing the clock should not have any effect.

The AMBA driver only /drivers/amba/bus.c  gives permission to
remove/restore the clocks from the devices (pm_suspend pm_resume
callbacks) - this reduces the power consumption of these devices if
the clock is not running, but state must be retained.

> >> >>
> >> >> 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.
> >
>
> So here I have a doubt as to why we need to use UCI because PID =
> 0x2bb909
> and CID = 0xb105900d are same for both replicators, so UCI won't
> identify the
> different replicators(in same implementation i.e., on QCOM) here.
> Am I missing something?
>
> Thats why I think Suzuki suggested to use PIDR4_DES2 and check for QCOM
> impl
> and add a workaround for all replicators, something like below: (will
> need cleaning)
>
> #define PIDR4_DES2      0xFD0
>
> if (FIELD_GET(GENMASK(3, 0), readl_relaxed(drvdata->base + PIDR4_DES2))
> == 0x4)
>         id0val = id1val = 0xff;
>

Please look at the CoreSight components specification 3.0 (ARM IHI
0029E) Section B2.1.2 which describes the Unique Component Identifier
(UCI).
As mentioned above this consists of a combination of bits from
multiple registers, including PIDR4.

> ... and the rest as you suggested.
>
> >
> > 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;
> > ....
> >
>
> Thanks for this detailed idea for workaround. I will add this once we
> know whether we need to use UCI or PIDR4_DES2.
>
> > 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.
> >
>
> I would defer this to experts as you or suzuki will have more idea
> regarding this than me.
>
> Thanks,
> Sai
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member
> of Code Aurora Forum, hosted by The Linux Foundation

Thanks

Mike

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

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

[Stephen Boyd]

Quoting Mike Leach (2020-05-12 14:52:33)
> HI Sai,
> 
> On Tue, 12 May 2020 at 18:46, Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
> >
> > Hi Mike,
> >
> > On 2020-05-12 17:19, Mike Leach wrote:
> > [...]
> >
> > >> >>
> > >> >> 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?
> >
> > Thanks for taking your time to review this. For new replicator behaving
> > unreasonably, I think the assumption that the context is not lost on
> > disabling clock is flawed since its implementation defined. Is such
> > assumption documented in any TRM?
> >
> 
> Looking at the AMBA driver there is a comment there that AMBA does not
> lose state when clocks are removed. This is consistent with the AMBA
> protocol spec which states that AMBA slaves can only be accessed /
> read / write on various strobe signals,  or state reset on PRESET
> signal, all timed by the rising edge of the bus clock. state changes
> are not permitted on clock events alone. Given this static nature of
> AMBA slaves then removing the clock should not have any effect.

I believe the "clock" that is being used here is actually a software
message to the power manager hardware that the debug subsystem isn't
being used anymore. When nothing is requesting that it be enabled the
power manager turns off the power to the debug subsystem and then the
register context is lost. It shouldn't be a clock in the clk subsystem.
It should be a power domain and be attached to the amba devices in the
usual ways. Then the normal runtime PM semantics would follow. If amba
devices require a clk then we'll have to provide a dummy one that
doesn't do anything on this platform.

> 
> The AMBA driver only /drivers/amba/bus.c  gives permission to
> remove/restore the clocks from the devices (pm_suspend pm_resume
> callbacks) - this reduces the power consumption of these devices if
> the clock is not running, but state must be retained.
> 

Ideally the drivers can have enough knowledge about this situation to do
the proper save/restore steps so that if the coresight hardware isn't
being used we don't keep it powered forever and so that across system
wide suspend/resume we can properly power it off.

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

[Sai Prakash Ranjan]

On 2020-05-13 07:19, Stephen Boyd wrote:
> Quoting Mike Leach (2020-05-12 14:52:33)
>> HI Sai,
>> 
>> On Tue, 12 May 2020 at 18:46, Sai Prakash Ranjan
>> <saiprakash.ranjan@codeaurora.org> wrote:
>> >
>> > Hi Mike,
>> >
>> > On 2020-05-12 17:19, Mike Leach wrote:
>> > [...]
>> >
>> > >> >>
>> > >> >> 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?
>> >
>> > Thanks for taking your time to review this. For new replicator behaving
>> > unreasonably, I think the assumption that the context is not lost on
>> > disabling clock is flawed since its implementation defined. Is such
>> > assumption documented in any TRM?
>> >
>> 
>> Looking at the AMBA driver there is a comment there that AMBA does not
>> lose state when clocks are removed. This is consistent with the AMBA
>> protocol spec which states that AMBA slaves can only be accessed /
>> read / write on various strobe signals,  or state reset on PRESET
>> signal, all timed by the rising edge of the bus clock. state changes
>> are not permitted on clock events alone. Given this static nature of
>> AMBA slaves then removing the clock should not have any effect.
> 
> I believe the "clock" that is being used here is actually a software
> message to the power manager hardware that the debug subsystem isn't
> being used anymore. When nothing is requesting that it be enabled the
> power manager turns off the power to the debug subsystem and then the
> register context is lost. It shouldn't be a clock in the clk subsystem.
> It should be a power domain and be attached to the amba devices in the
> usual ways. Then the normal runtime PM semantics would follow. If amba
> devices require a clk then we'll have to provide a dummy one that
> doesn't do anything on this platform.
> 

Note that there are 2 dynamic replicators and the behaviour is different 
only on
one of the replicators(swao_replicator) which is in AOSS domain. I don't 
see
how runtime PM would help us differentiate between them and handle PM 
differently
for different replicators.

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

Hi Mike,

On 2020-05-13 03:22, Mike Leach wrote:

[...]

> 
> Looking at the AMBA driver there is a comment there that AMBA does not
> lose state when clocks are removed. This is consistent with the AMBA
> protocol spec which states that AMBA slaves can only be accessed /
> read / write on various strobe signals,  or state reset on PRESET
> signal, all timed by the rising edge of the bus clock. state changes
> are not permitted on clock events alone. Given this static nature of
> AMBA slaves then removing the clock should not have any effect.
> 
> The AMBA driver only /drivers/amba/bus.c  gives permission to
> remove/restore the clocks from the devices (pm_suspend pm_resume
> callbacks) - this reduces the power consumption of these devices if
> the clock is not running, but state must be retained.
> 

Thanks for the clarification.

>> >> >>
>> >> >> 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.
>> >
>> 
>> So here I have a doubt as to why we need to use UCI because PID =
>> 0x2bb909
>> and CID = 0xb105900d are same for both replicators, so UCI won't
>> identify the
>> different replicators(in same implementation i.e., on QCOM) here.
>> Am I missing something?
>> 
>> Thats why I think Suzuki suggested to use PIDR4_DES2 and check for 
>> QCOM
>> impl
>> and add a workaround for all replicators, something like below: (will
>> need cleaning)
>> 
>> #define PIDR4_DES2      0xFD0
>> 
>> if (FIELD_GET(GENMASK(3, 0), readl_relaxed(drvdata->base + 
>> PIDR4_DES2))
>> == 0x4)
>>         id0val = id1val = 0xff;
>> 
> 
> Please look at the CoreSight components specification 3.0 (ARM IHI
> 0029E) Section B2.1.2 which describes the Unique Component Identifier
> (UCI).
> As mentioned above this consists of a combination of bits from
> multiple registers, including PIDR4.
> 

Ok got it now, thanks for clearing the doubt. I will go ahead with
this method to identify QCOM impl and post a patch.

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

Hi Mike, Suzuki

[...]

>> 
>> Please look at the CoreSight components specification 3.0 (ARM IHI
>> 0029E) Section B2.1.2 which describes the Unique Component Identifier
>> (UCI).
>> As mentioned above this consists of a combination of bits from
>> multiple registers, including PIDR4.
>> 
> 
> Ok got it now, thanks for clearing the doubt. I will go ahead with
> this method to identify QCOM impl and post a patch.
> 

Looking some more into this, since we have this limitation only on
specific replicator on very few QCOM SoCs, rather than having a blanket
workaround for all QCOM, we were thinking it would be better to have
this workaround based on a firmware property something like
"qcom,replicator-loses-context" for those replicators with this
limitation and then set the drvdata->check_idfilter_val based on
this property.

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

Hi Mike, Suzuki,

On 2020-05-16 15:34, Sai Prakash Ranjan wrote:
> Hi Mike, Suzuki
> 
> [...]
> 
>>> 
>>> Please look at the CoreSight components specification 3.0 (ARM IHI
>>> 0029E) Section B2.1.2 which describes the Unique Component Identifier
>>> (UCI).
>>> As mentioned above this consists of a combination of bits from
>>> multiple registers, including PIDR4.
>>> 
>> 
>> Ok got it now, thanks for clearing the doubt. I will go ahead with
>> this method to identify QCOM impl and post a patch.
>> 
> 
> Looking some more into this, since we have this limitation only on
> specific replicator on very few QCOM SoCs, rather than having a blanket
> workaround for all QCOM, we were thinking it would be better to have
> this workaround based on a firmware property something like
> "qcom,replicator-loses-context" for those replicators with this
> limitation and then set the drvdata->check_idfilter_val based on
> this property.
> 

Sorry for going back and forth on this one, but I think having a 
firmware
property will clearly help us identify the issue on specific SoCs rather 
than
wholesale workaround for all QCOM SoCs. For now, I will post a patch 
based on
the property "qcom,replicator-loses-context", please feel free to yell 
at me
if this is completely wrong and we can discuss it further in that patch.

Thanks,
Sai

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

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

[Sai Prakash Ranjan]

On 2020-05-11 19:46, 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).
> 

Here is the cid=0xb105900d for both replicators.

Thanks,
Sai

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