On Tue, 12 May 2020 at 05:49, Mike Leach <mike.leach@xxxxxxxxxx> wrote: > > HI, > > On Mon, 11 May 2020 at 15:41, Sai Prakash Ranjan > <saiprakash.ranjan@xxxxxxxxxxxxxx> wrote: > > > > Hi Suzuki, > > > > On 2020-05-11 20:00, Suzuki K Poulose wrote: > > > On 05/11/2020 03:16 PM, Sai Prakash Ranjan wrote: > > >> Hi Mike, > > >> > > >> On 2020-05-11 16:44, Mike Leach wrote: > > >> [...] > > >> > I have reviewed the replicator driver, and compared to all the other CS drivers. > This driver appears to be the only one that sets hardware values in > probe() and expects them to remain in place on enable, and uses that > state for programming decisions later, despite telling the PM > infrastructure that it is clear to suspend the device. > > Now we have a system where the replicator hardware is behaving > differently under the driver, but is it behaving unreasonably? > > >>>> > > >>>> I checked with the debug team and there is a limitation with > > >>>> the replicator(swao_replicator) in the AOSS group where it > > >>>> loses the idfilter register context when the clock is disabled. > > >>>> This is not just in SC7180 SoC but also reported on some latest > > >>>> upcoming QCOM SoCs as well and will need to be taken care in > > >>>> order to enable coresight on these chipsets. > > >>>> > > >>>> Here's what's happening - After the replicator is initialized, > > >>>> the clock is disabled in amba_pm_runtime_suspend() as a part of > > >>>> pm runtime workqueue with the assumption that there will be no > > >>>> loss of context after the replicator is initialized. But it doesn't > > >>>> hold good with the replicators with these unfortunate limitation > > >>>> and the idfilter register context is lost. > > >>>> > > >>>> [ 5.889406] amba_pm_runtime_suspend devname=6b06000.replicator > > >>>> ret=0 > > >>>> [ 5.914516] Workqueue: pm pm_runtime_work > > >>>> [ 5.918648] Call trace: > > >>>> [ 5.921185] dump_backtrace+0x0/0x1d0 > > >>>> [ 5.924958] show_stack+0x2c/0x38 > > >>>> [ 5.928382] dump_stack+0xc0/0x104 > > >>>> [ 5.931896] amba_pm_runtime_suspend+0xd8/0xe0 > > >>>> [ 5.936469] __rpm_callback+0xe0/0x140 > > >>>> [ 5.940332] rpm_callback+0x38/0x98 > > >>>> [ 5.943926] rpm_suspend+0xec/0x618 > > >>>> [ 5.947522] rpm_idle+0x5c/0x3f8 > > >>>> [ 5.950851] pm_runtime_work+0xa8/0xc0 > > >>>> [ 5.954718] process_one_work+0x1f8/0x4c0 > > >>>> [ 5.958848] worker_thread+0x50/0x468 > > >>>> [ 5.962623] kthread+0x12c/0x158 > > >>>> [ 5.965957] ret_from_fork+0x10/0x1c > > >>>> > > >>>> This is a platform/SoC specific replicator issue, so we can either > > >>>> introduce some DT property for replicators to identify which > > >>>> replicator > > >>>> has this limitation, check in replicator_enable() and reset the > > >>>> registers > > >>>> or have something like below diff to check the idfilter registers in > > >>>> replicator_enable() and then reset with clear comment specifying > > >>>> it’s > > >>>> the > > >>>> hardware limitation on some QCOM SoCs. Please let me know your > > >>>> thoughts > > >>>> on > > >>>> this? > > >>>> > > >> > > >> Sorry for hurrying up and sending the patch - > > >> https://lore.kernel.org/patchwork/patch/1239923/. > > >> I will send v2 based on further feedbacks here or there. > > >> > > >>> > > >>> 1) does this replicator part have a unique ID that differs from the > > >>> standard ARM designed replicators? > > >>> If so perhaps link the modification into this. (even if the part no > > >>> in > > >>> PIDR0/1 is the same the UCI should be different for a different > > >>> implementation) > > >>> > I have reviewed the replicator driver, and compared to all the other CS drivers. > This driver appears to be the only one that sets hardware values in > probe() and expects them to remain in place on enable, and uses that > state for programming decisions later, despite telling the PM > infrastructure that it is clear to suspend the device. > > Now we have a system where the replicator hardware is behaving > differently under the driver, but is it behaving unreasonably? > > >> > > >> pid=0x2bb909 for both replicators. So part number is same. > > >> UCI will be different for different implementation(QCOM maybe > > >> different from ARM), > > >> but will it be different for different replicators under the same > > >> impl(i.e., on QCOM). > > > > > > May be use PIDR4.DES_2 to match the Implementor and apply the work > > > around for all QCOM replicators ? > > > > > > To me that sounds the best option. > > > > > > > I agree, if it can be established that the register values that make > up UCI (pid0-4, devarch, devtype, PID:CLASS==0x9), can correctly > identify the parts then a flag can be set in the probe() function and > acted on during the enable() function. > > > Ok we can do this as well, but just for my understanding, why do we need > > to reset replicators > > in replicator_probe() and not in replicator_enable()? Are we accessing > > anything before > > we enable replicators? > > > > This was a design decision made by the original driver writer. A > normal AMBA device should not lose context due to clock removal (see > drivers/amba/bus.c), so resetting in probe means this operation is > done only once, rather than add overhead in the enable() function,and > later decisions can be made according to the state of the registers > set. > > As you have pointed out, for this replicator implementation the > context is unfortunately not retained when clocks are removed - so an > alternative method is required. > > perhaps something like:- > > probe() > ... > if (match_id_non_persistent_state_regs(ID)) > drvdata->check_filter_val_on_enable; > .... > > and a re-write of enable:- > > enable() > ... > CS_UNLOCK() > id0val = read(IDFILTER0); > id1val = read(IDFILTER1); > > /* some replicator designs lose context when AMBA clocks are removed - > check for this */ > if (drvdata->check_filter_val_on_enable && (id0val == id1val == 0x0)) > id0val = id1val = 0xff; > > if(id0xal == id1val == 0xff) > rc = claim_device() > > if (!rc) > switch (outport) > case 0: id0val = 0x0; break > case 1: id1va; = 0x0; break; > default: rc = -EINVAL; > > if (!rc) > write(id0val); > write(id1val); > CS_LOCK() > return rc; > .... > > Given that the access to the enable() function is predicated on a > reference count per active port, there is also a case for dropping the > check_filter_val_on_enable flag completely - once one port is active, > then the device will remain enabled until both ports are inactive. > This still allows for future development of selective filtering per > port. > > One other point here - there is a case as I mentioned above for moving > to a stored value model for the driver - as this is the only coresight > driver that appears to set state in the probe() function rather than > write all on enable. I favour that option. Looking at the funnel driver we may have the same issue with the port configuration, but we can have a look at that if/when it becomes an issue. > This however would necessitate a more comprehensive re-write. Maybe a little more effort but overall a better approach. Thanks, Mathieu > > Regards > > Mike > > > > Thanks, > > Sai > > -- > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a > > member > > of Code Aurora Forum, hosted by The Linux Foundation > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK
