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
