| Message ID | 1419331204-26679-3-git-send-email-vinceh@nvidia.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
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 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. Stephen, Peter, any thoughts on the above? Thierry
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? > 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? Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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); + static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc) { unsigned long long tick; diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index 63deb8d9f82a..4894aec7d2a0 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -39,6 +39,21 @@ struct tegra_mc_client { struct tegra_mc_la la; }; +/* hot reset */ +struct tegra_mc_hr { + unsigned int swgroup; + unsigned int ctrl; + unsigned int status; + unsigned int bit; +}; + +struct tegra_mc; + +struct tegra_mc_ops { + int (*flush)(struct tegra_mc *mc, const struct tegra_mc_hr *hr_client, + bool enable); +}; + struct tegra_smmu_swgroup { unsigned int swgroup; unsigned int reg; @@ -64,7 +79,6 @@ struct tegra_smmu_soc { const struct tegra_smmu_ops *ops; }; -struct tegra_mc; struct tegra_smmu; #ifdef CONFIG_TEGRA_IOMMU_SMMU @@ -84,6 +98,11 @@ struct tegra_mc_soc { const struct tegra_mc_client *clients; unsigned int num_clients; + const struct tegra_mc_hr *hr_clients; + unsigned int num_hr_clients; + + const struct tegra_mc_ops *ops; + const unsigned int *emem_regs; unsigned int num_emem_regs; @@ -104,4 +123,6 @@ struct tegra_mc { unsigned long tick; }; +int tegra_mc_flush(struct tegra_mc *mc, unsigned int swgroup, bool enable); + #endif /* __SOC_TEGRA_MC_H__ */
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(-)