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

From: Stephen Boyd <swboyd@chromium.org>
To: Mike Leach <mike.leach@linaro.org>,
	Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Cc: 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: Tue, 12 May 2020 18:49:30 -0700	[thread overview]
Message-ID: <158933457051.215346.13515171569230202840@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAJ9a7VjAoUmMG9pLEzE_rMSpOjwVOi-ZCinF87n9H0JgfMDsiQ@mail.gmail.com>

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.

> 
> The AMBA driver only /drivers/amba/bus.c  gives permission to
> remove/restore the clocks from the devices (pm_suspend pm_resume
> callbacks) - this reduces the power consumption of these devices if
> the clock is not running, but state must be retained.
> 

Ideally the drivers can have enough knowledge about this situation to do
the proper save/restore steps so that if the coresight hardware isn't
being used we don't keep it powered forever and so that across system
wide suspend/resume we can properly power it off.