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@xxxxxxxxxxxxxx> 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