Re: [PATCH 2/11] memory: tegra: add mc flush support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5040 bytes --]

On Wed, Jan 07, 2015 at 12:08:05PM +0200, Peter De Schrijver wrote:
> On Tue, Jan 06, 2015 at 03:18:22PM +0100, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Dec 23, 2014 at 06:39:55PM +0800, Vince Hsu wrote:
> > > The flush operation of memory clients is needed for various IP blocks in
> > > the Tegra SoCs to perform a clean reset.
> > > 
> > > Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > > ---
> > >  drivers/memory/tegra/mc.c | 21 +++++++++++++++++++++
> > >  include/soc/tegra/mc.h    | 23 ++++++++++++++++++++++-
> > >  2 files changed, 43 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > > index fe3c44e7e1d1..a2928b4b26fe 100644
> > > --- a/drivers/memory/tegra/mc.c
> > > +++ b/drivers/memory/tegra/mc.c
> > > @@ -62,6 +62,27 @@ static const struct of_device_id tegra_mc_of_match[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
> > >  
> > > +int tegra_mc_flush(struct tegra_mc *mc, unsigned int swgroup, bool enable)
> > > +{
> > > +	int i;
> > > +	const struct tegra_mc_hr *client;
> > > +
> > > +	if (!mc || !mc->soc->hr_clients ||
> > > +			!mc->soc->ops || !mc->soc->ops->flush)
> > > +		return -EINVAL;;
> > > +
> > > +	client = mc->soc->hr_clients;
> > > +
> > > +	for (i = 0; i < mc->soc->num_hr_clients; i++, client++) {
> > > +		if (swgroup == client->swgroup) {
> > > +			return mc->soc->ops->flush(mc, client, enable);
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL(tegra_mc_flush);
> > 
> > Like Lucas already mentioned in response to another patch, having a
> > boolean "enable" argument is suboptimal here. Now according to
> > documentation the proper reset sequence for clients is something like
> > this:
> > 
> > 	1) set the FLUSH_ENABLE bit for the client
> > 	2) poll the FLUSH_DONE bit for the client
> > 	3) assert reset to the client using the CAR
> > 	4) deassert reset to the client using the CAR
> > 	5) clear the FLUSH_ENABLE bit for the client
> > 
> 
> Do we ever need to do this outside a powergating or railgating sequence?

I don't think so. I worry a little that we may encounter situations
where the driver itself wants to reset the hardware module via the CAR,
though I think if we made sure that any driver resets would only happen
with a runtime PM reference held it should be safe.

Furthermore there's the issue of keeping backwards-compatibility. There
are some drivers that actually do this kind of reset today, so they need
to be carefully audited before conversion.

And we need to conditionalize the manual powergate sequences that are
currently used by the various drivers so that they don't happen when
power domains are initialized.

> > This is really inconvenient because we can't flush the client using a
> > single operation. So I think we'll need two functions here, something
> > like: tegra_mc_flush_enable/disable(), or tegra_mc_flush_{,de}assert().
> > Or maybe even: tegra_mc_reset_{,de}assert() to mirror the reset
> > controller API. I suppose we could even export it using the reset
> > controller framework.
> > 
> > Doing so would allow us to have power domain DT nodes like this:
> > 
> > 	pmc@0,7000e400 {
> > 		power-domains {
> > 			...
> > 
> > 			gpu {
> > 				resets = <&tegra_car 184>,
> > 					 <&mc TEGRA_SWGROUP_GPU>;
> > 				reset-names = "module", "client";
> > 			};
> > 
> > 			...
> > 		};
> > 	};
> > 
> > The PMC driver could then grab the "module" and "client" resets and do
> > something like this:
> > 
> > 	reset_control_assert(powergate->rst_client);
> > 	reset_control_assert(powergate->rst_module);
> > 	reset_control_deassert(powergate->rst_module);
> > 	reset_control_deassert(powergate->rst_client);
> > 
> > Optionally the above could be extended with a reset_control_status()-
> > loop. Alternatively reset_control_assert() would block until the
> > FLUSH_DONE bit is set.
> > 
> 
> I think the reset_control_assert should wait for the FLUSH_DONE bit to be set
> because only then all outstanding memory transactions for the client are
> completed so you can't realistically claim the reset has been asserted before
> the bit is set. Then you could also expose a single reset which handles both
> the memory client and the module reset in CAR? Ie:
> 
> reset_control_assert(powergate->rst_module); would set FLUSH_ENABLE for the
> memory client, wait for the FLUSH_DONE bit and then assert the CAR reset.
> reset_control_deassert(powergate->rst_module); would deassert the CAR reset
> and then clear the FLUSH_ENABLE bit. Or is there a usecase to control them
> individually?

I suppose that would be possible, but it'd mean that we need to access
registers across driver boundaries (the CAR driver would need to write
the memory controller's registers or vice versa). Doing this with
separate reset controls allows the drivers to be nicely separated.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]