Message-ID: <364049a30dc9d242ec611bf27a16a6c9@codeaurora.org>
Date:   Tue, 12 May 2020 23:16:29 +0530
From:   Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
To:     Mike Leach <mike.leach@...aro.org>
Cc:     Suzuki K Poulose <suzuki.poulose@....com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH] coresight: dynamic-replicator: Fix handling of multiple
 connections

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

Powered by blists - more mailing lists