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

From: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
To: Stephen Boyd <swboyd@chromium.org>
Cc: Mike Leach <mike.leach@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arm-msm@vger.kernel.org,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Russell King <linux@armlinux.org.uk>
Subject: Re: [PATCH] coresight: dynamic-replicator: Fix handling of multiple connections
Date: Wed, 13 May 2020 21:15:32 +0530	[thread overview]
Message-ID: <861f8ab0174d036cb1e49e34e4f81a92@codeaurora.org> (raw)
In-Reply-To: <158933457051.215346.13515171569230202840@swboyd.mtv.corp.google.com>

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