Message ID | 20210420165237.3523732-4-thierry.reding@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | memory: tegra: Driver unification | expand |
20.04.2021 19:52, Thierry Reding пишет: > -static int tegra_mc_suspend(struct device *dev) > +static int __maybe_unused tegra_mc_suspend(struct device *dev) > { > struct tegra_mc *mc = dev_get_drvdata(dev); > - int err; > > - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { > - err = tegra_gart_suspend(mc->gart); > - if (err) > - return err; > - } > + if (mc->soc->ops && mc->soc->ops->suspend) > + return mc->soc->ops->suspend(mc); > > return 0; > } > > -static int tegra_mc_resume(struct device *dev) > +static int __maybe_unused tegra_mc_resume(struct device *dev) > { > struct tegra_mc *mc = dev_get_drvdata(dev); > - int err; > > - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { > - err = tegra_gart_resume(mc->gart); > - if (err) > - return err; > - } > + if (mc->soc->ops && mc->soc->ops->resume) > + return mc->soc->ops->resume(mc); > > return 0; > } You added __maybe_unused, but haven't changed tegra_mc_pm_ops to use SET_SYSTEM_SLEEP_PM_OPS(), so the functions are always used.
On 20/04/2021 18:52, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Continuing the scheme of unification, push suspend/resume callbacks into > per-SoC driver so that they can be properly parameterized. > > While at it, also move the ->init() callback into the new tegra_mc_ops > structure to keep things clean. Please split this part. This is just moving pointer from one structure to another, quite small change. The rest of the patchset is quite different - you now call tegra_mc_suspend() from a per-SoC driver and move the code around. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/memory/tegra/mc.c | 24 ++++++++--------------- > drivers/memory/tegra/tegra186.c | 27 ++++++++++++++++++++++---- > drivers/memory/tegra/tegra20.c | 34 ++++++++++++++++++++++++++++++++- > include/soc/tegra/mc.h | 9 +++++++-- > 4 files changed, 71 insertions(+), 23 deletions(-) > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c > index b7e104bf6614..2b21131d779c 100644 > --- a/drivers/memory/tegra/mc.c > +++ b/drivers/memory/tegra/mc.c > @@ -829,8 +829,8 @@ static int tegra_mc_probe(struct platform_device *pdev) > > mc->debugfs.root = debugfs_create_dir("mc", NULL); > > - if (mc->soc->init) { > - err = mc->soc->init(mc); > + if (mc->soc->ops && mc->soc->ops->init) { > + err = mc->soc->ops->init(mc); > if (err < 0) > dev_err(&pdev->dev, "failed to initialize SoC driver: %d\n", > err); > @@ -867,30 +867,22 @@ static int tegra_mc_probe(struct platform_device *pdev) > return 0; > } > > -static int tegra_mc_suspend(struct device *dev) > +static int __maybe_unused tegra_mc_suspend(struct device *dev) > { > struct tegra_mc *mc = dev_get_drvdata(dev); > - int err; > > - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { > - err = tegra_gart_suspend(mc->gart); > - if (err) > - return err; > - } > + if (mc->soc->ops && mc->soc->ops->suspend) > + return mc->soc->ops->suspend(mc); > > return 0; > } > > -static int tegra_mc_resume(struct device *dev) > +static int __maybe_unused tegra_mc_resume(struct device *dev) > { > struct tegra_mc *mc = dev_get_drvdata(dev); > - int err; > > - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { > - err = tegra_gart_resume(mc->gart); > - if (err) > - return err; > - } > + if (mc->soc->ops && mc->soc->ops->resume) > + return mc->soc->ops->resume(mc); > > return 0; > } > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > index 8e77567d1378..9d3fdb609d55 100644 > --- a/drivers/memory/tegra/tegra186.c > +++ b/drivers/memory/tegra/tegra186.c > @@ -45,6 +45,17 @@ static void tegra186_mc_program_sid(struct tegra_mc *mc) > } > } > > +static int tegra186_mc_resume(struct tegra_mc *mc) > +{ > + tegra186_mc_program_sid(mc); > + > + return 0; > +} > + > +static const struct tegra_mc_ops tegra186_mc_ops = { > + .resume = tegra186_mc_resume, > +}; > + > #if defined(CONFIG_ARCH_TEGRA_186_SOC) > static const struct tegra_mc_client tegra186_mc_clients[] = { > { > @@ -701,6 +712,7 @@ static const struct tegra_mc_client tegra186_mc_clients[] = { > static const struct tegra_mc_soc tegra186_mc_soc = { > .num_clients = ARRAY_SIZE(tegra186_mc_clients), > .clients = tegra186_mc_clients, > + .ops = &tegra186_mc_ops, > }; > #endif > > @@ -1909,6 +1921,7 @@ static const struct tegra_mc_client tegra194_mc_clients[] = { > static const struct tegra_mc_soc tegra194_mc_soc = { > .num_clients = ARRAY_SIZE(tegra194_mc_clients), > .clients = tegra194_mc_clients, > + .ops = &tegra186_mc_ops, > }; > #endif > > @@ -1961,22 +1974,28 @@ static const struct of_device_id tegra186_mc_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, tegra186_mc_of_match); > > -static int __maybe_unused tegra186_mc_suspend(struct device *dev) > +static int __maybe_unused tegra_mc_suspend(struct device *dev) > { > + struct tegra_mc *mc = dev_get_drvdata(dev); > + > + if (mc->soc->ops && mc->soc->ops->suspend) > + return mc->soc->ops->suspend(mc); > + > return 0; > } > > -static int __maybe_unused tegra186_mc_resume(struct device *dev) > +static int __maybe_unused tegra_mc_resume(struct device *dev) > { > struct tegra_mc *mc = dev_get_drvdata(dev); > > - tegra186_mc_program_sid(mc); > + if (mc->soc->ops && mc->soc->ops->resume) > + return mc->soc->ops->resume(mc); > > return 0; > } > > static const struct dev_pm_ops tegra186_mc_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(tegra186_mc_suspend, tegra186_mc_resume) > + SET_SYSTEM_SLEEP_PM_OPS(tegra_mc_suspend, tegra_mc_resume) What's the benefit here? You basically define your own suspend-resume ops, on top of PM suspend-resume ops... Before it was quite obvious code - the Tegra186 MC driver had very simple suspend/resume which did simple job. Now it feels like trickier code to follow - Tegra186 driver calls it's resume (with the same name as others - another confusion) which is a simple wrapper calling somewhere else (need to jump to assinment of resume()). Best regards, Krzysztof
On Mon, Apr 26, 2021 at 10:47:11AM +0200, Krzysztof Kozlowski wrote: > On 20/04/2021 18:52, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Continuing the scheme of unification, push suspend/resume callbacks into > > per-SoC driver so that they can be properly parameterized. > > > > While at it, also move the ->init() callback into the new tegra_mc_ops > > structure to keep things clean. > > Please split this part. This is just moving pointer from one structure > to another, quite small change. > > The rest of the patchset is quite different - you now call > tegra_mc_suspend() from a per-SoC driver and move the code around. Okay, I think I can split that off into a separate patch. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/memory/tegra/mc.c | 24 ++++++++--------------- > > drivers/memory/tegra/tegra186.c | 27 ++++++++++++++++++++++---- > > drivers/memory/tegra/tegra20.c | 34 ++++++++++++++++++++++++++++++++- > > include/soc/tegra/mc.h | 9 +++++++-- > > 4 files changed, 71 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c > > index b7e104bf6614..2b21131d779c 100644 > > --- a/drivers/memory/tegra/mc.c > > +++ b/drivers/memory/tegra/mc.c > > @@ -829,8 +829,8 @@ static int tegra_mc_probe(struct platform_device *pdev) > > > > mc->debugfs.root = debugfs_create_dir("mc", NULL); > > > > - if (mc->soc->init) { > > - err = mc->soc->init(mc); > > + if (mc->soc->ops && mc->soc->ops->init) { > > + err = mc->soc->ops->init(mc); > > if (err < 0) > > dev_err(&pdev->dev, "failed to initialize SoC driver: %d\n", > > err); > > @@ -867,30 +867,22 @@ static int tegra_mc_probe(struct platform_device *pdev) > > return 0; > > } > > > > -static int tegra_mc_suspend(struct device *dev) > > +static int __maybe_unused tegra_mc_suspend(struct device *dev) > > { > > struct tegra_mc *mc = dev_get_drvdata(dev); > > - int err; > > > > - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { > > - err = tegra_gart_suspend(mc->gart); > > - if (err) > > - return err; > > - } > > + if (mc->soc->ops && mc->soc->ops->suspend) > > + return mc->soc->ops->suspend(mc); > > > > return 0; > > } > > > > -static int tegra_mc_resume(struct device *dev) > > +static int __maybe_unused tegra_mc_resume(struct device *dev) > > { > > struct tegra_mc *mc = dev_get_drvdata(dev); > > - int err; > > > > - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { > > - err = tegra_gart_resume(mc->gart); > > - if (err) > > - return err; > > - } > > + if (mc->soc->ops && mc->soc->ops->resume) > > + return mc->soc->ops->resume(mc); > > > > return 0; > > } > > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c > > index 8e77567d1378..9d3fdb609d55 100644 > > --- a/drivers/memory/tegra/tegra186.c > > +++ b/drivers/memory/tegra/tegra186.c > > @@ -45,6 +45,17 @@ static void tegra186_mc_program_sid(struct tegra_mc *mc) > > } > > } > > > > +static int tegra186_mc_resume(struct tegra_mc *mc) > > +{ > > + tegra186_mc_program_sid(mc); > > + > > + return 0; > > +} > > + > > +static const struct tegra_mc_ops tegra186_mc_ops = { > > + .resume = tegra186_mc_resume, > > +}; > > + > > #if defined(CONFIG_ARCH_TEGRA_186_SOC) > > static const struct tegra_mc_client tegra186_mc_clients[] = { > > { > > @@ -701,6 +712,7 @@ static const struct tegra_mc_client tegra186_mc_clients[] = { > > static const struct tegra_mc_soc tegra186_mc_soc = { > > .num_clients = ARRAY_SIZE(tegra186_mc_clients), > > .clients = tegra186_mc_clients, > > + .ops = &tegra186_mc_ops, > > }; > > #endif > > > > @@ -1909,6 +1921,7 @@ static const struct tegra_mc_client tegra194_mc_clients[] = { > > static const struct tegra_mc_soc tegra194_mc_soc = { > > .num_clients = ARRAY_SIZE(tegra194_mc_clients), > > .clients = tegra194_mc_clients, > > + .ops = &tegra186_mc_ops, > > }; > > #endif > > > > @@ -1961,22 +1974,28 @@ static const struct of_device_id tegra186_mc_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, tegra186_mc_of_match); > > > > -static int __maybe_unused tegra186_mc_suspend(struct device *dev) > > +static int __maybe_unused tegra_mc_suspend(struct device *dev) > > { > > + struct tegra_mc *mc = dev_get_drvdata(dev); > > + > > + if (mc->soc->ops && mc->soc->ops->suspend) > > + return mc->soc->ops->suspend(mc); > > + > > return 0; > > } > > > > -static int __maybe_unused tegra186_mc_resume(struct device *dev) > > +static int __maybe_unused tegra_mc_resume(struct device *dev) > > { > > struct tegra_mc *mc = dev_get_drvdata(dev); > > > > - tegra186_mc_program_sid(mc); > > + if (mc->soc->ops && mc->soc->ops->resume) > > + return mc->soc->ops->resume(mc); > > > > return 0; > > } > > > > static const struct dev_pm_ops tegra186_mc_pm_ops = { > > - SET_SYSTEM_SLEEP_PM_OPS(tegra186_mc_suspend, tegra186_mc_resume) > > + SET_SYSTEM_SLEEP_PM_OPS(tegra_mc_suspend, tegra_mc_resume) > > What's the benefit here? You basically define your own suspend-resume > ops, on top of PM suspend-resume ops... Before it was quite obvious code > - the Tegra186 MC driver had very simple suspend/resume which did simple > job. Now it feels like trickier code to follow - Tegra186 driver calls > it's resume (with the same name as others - another confusion) which is > a simple wrapper calling somewhere else (need to jump to assinment of > resume()). This confusion is merely temporary. The idea here is to make the Tegra186 driver the same as the "legacy" driver in this regard so that the unification patch becomes just a removal of this code. If I didn't first make this the same, I would've had to push this set of changes into the unification patch, making it more difficult to understand. Thierry
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c index b7e104bf6614..2b21131d779c 100644 --- a/drivers/memory/tegra/mc.c +++ b/drivers/memory/tegra/mc.c @@ -829,8 +829,8 @@ static int tegra_mc_probe(struct platform_device *pdev) mc->debugfs.root = debugfs_create_dir("mc", NULL); - if (mc->soc->init) { - err = mc->soc->init(mc); + if (mc->soc->ops && mc->soc->ops->init) { + err = mc->soc->ops->init(mc); if (err < 0) dev_err(&pdev->dev, "failed to initialize SoC driver: %d\n", err); @@ -867,30 +867,22 @@ static int tegra_mc_probe(struct platform_device *pdev) return 0; } -static int tegra_mc_suspend(struct device *dev) +static int __maybe_unused tegra_mc_suspend(struct device *dev) { struct tegra_mc *mc = dev_get_drvdata(dev); - int err; - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { - err = tegra_gart_suspend(mc->gart); - if (err) - return err; - } + if (mc->soc->ops && mc->soc->ops->suspend) + return mc->soc->ops->suspend(mc); return 0; } -static int tegra_mc_resume(struct device *dev) +static int __maybe_unused tegra_mc_resume(struct device *dev) { struct tegra_mc *mc = dev_get_drvdata(dev); - int err; - if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { - err = tegra_gart_resume(mc->gart); - if (err) - return err; - } + if (mc->soc->ops && mc->soc->ops->resume) + return mc->soc->ops->resume(mc); return 0; } diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c index 8e77567d1378..9d3fdb609d55 100644 --- a/drivers/memory/tegra/tegra186.c +++ b/drivers/memory/tegra/tegra186.c @@ -45,6 +45,17 @@ static void tegra186_mc_program_sid(struct tegra_mc *mc) } } +static int tegra186_mc_resume(struct tegra_mc *mc) +{ + tegra186_mc_program_sid(mc); + + return 0; +} + +static const struct tegra_mc_ops tegra186_mc_ops = { + .resume = tegra186_mc_resume, +}; + #if defined(CONFIG_ARCH_TEGRA_186_SOC) static const struct tegra_mc_client tegra186_mc_clients[] = { { @@ -701,6 +712,7 @@ static const struct tegra_mc_client tegra186_mc_clients[] = { static const struct tegra_mc_soc tegra186_mc_soc = { .num_clients = ARRAY_SIZE(tegra186_mc_clients), .clients = tegra186_mc_clients, + .ops = &tegra186_mc_ops, }; #endif @@ -1909,6 +1921,7 @@ static const struct tegra_mc_client tegra194_mc_clients[] = { static const struct tegra_mc_soc tegra194_mc_soc = { .num_clients = ARRAY_SIZE(tegra194_mc_clients), .clients = tegra194_mc_clients, + .ops = &tegra186_mc_ops, }; #endif @@ -1961,22 +1974,28 @@ static const struct of_device_id tegra186_mc_of_match[] = { }; MODULE_DEVICE_TABLE(of, tegra186_mc_of_match); -static int __maybe_unused tegra186_mc_suspend(struct device *dev) +static int __maybe_unused tegra_mc_suspend(struct device *dev) { + struct tegra_mc *mc = dev_get_drvdata(dev); + + if (mc->soc->ops && mc->soc->ops->suspend) + return mc->soc->ops->suspend(mc); + return 0; } -static int __maybe_unused tegra186_mc_resume(struct device *dev) +static int __maybe_unused tegra_mc_resume(struct device *dev) { struct tegra_mc *mc = dev_get_drvdata(dev); - tegra186_mc_program_sid(mc); + if (mc->soc->ops && mc->soc->ops->resume) + return mc->soc->ops->resume(mc); return 0; } static const struct dev_pm_ops tegra186_mc_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(tegra186_mc_suspend, tegra186_mc_resume) + SET_SYSTEM_SLEEP_PM_OPS(tegra_mc_suspend, tegra_mc_resume) }; static struct platform_driver tegra186_mc_driver = { diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c index 2db68a913b7a..a3335ad20f4d 100644 --- a/drivers/memory/tegra/tegra20.c +++ b/drivers/memory/tegra/tegra20.c @@ -687,6 +687,38 @@ static int tegra20_mc_init(struct tegra_mc *mc) return 0; } +static int tegra20_mc_suspend(struct tegra_mc *mc) +{ + int err; + + if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { + err = tegra_gart_suspend(mc->gart); + if (err < 0) + return err; + } + + return 0; +} + +static int tegra20_mc_resume(struct tegra_mc *mc) +{ + int err; + + if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart) { + err = tegra_gart_resume(mc->gart); + if (err < 0) + return err; + } + + return 0; +} + +static const struct tegra_mc_ops tegra20_mc_ops = { + .init = tegra20_mc_init, + .suspend = tegra20_mc_suspend, + .resume = tegra20_mc_resume, +}; + const struct tegra_mc_soc tegra20_mc_soc = { .clients = tegra20_mc_clients, .num_clients = ARRAY_SIZE(tegra20_mc_clients), @@ -698,5 +730,5 @@ const struct tegra_mc_soc tegra20_mc_soc = { .resets = tegra20_mc_resets, .num_resets = ARRAY_SIZE(tegra20_mc_resets), .icc_ops = &tegra20_mc_icc_ops, - .init = tegra20_mc_init, + .ops = &tegra20_mc_ops, }; diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index 9da4ef52ce30..7c49f75087c3 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -169,6 +169,12 @@ struct tegra_mc_icc_ops { void *data); }; +struct tegra_mc_ops { + int (*init)(struct tegra_mc *mc); + int (*suspend)(struct tegra_mc *mc); + int (*resume)(struct tegra_mc *mc); +}; + struct tegra_mc_soc { const struct tegra_mc_client *clients; unsigned int num_clients; @@ -190,8 +196,7 @@ struct tegra_mc_soc { unsigned int num_resets; const struct tegra_mc_icc_ops *icc_ops; - - int (*init)(struct tegra_mc *mc); + const struct tegra_mc_ops *ops; }; struct tegra_mc {