Re: [PATCH v6 13/21] iommu/tegra: gart: Integrate with Memory Controller driver

From: Thierry Reding
Date: Wed Dec 12 2018 - 05:14:16 EST

Next message: Catalin Marinas: "Re: [PATCH V2] kmemleak: Add config to select auto scan"
Previous message: Alexandre Belloni: "Re: [PATCH] ARM: at91: add support in soc driver for LPDDR2 SiP"
In reply to: Dmitry Osipenko: "[PATCH v6 13/21] iommu/tegra: gart: Integrate with Memory Controller driver"
Next in thread: Dmitry Osipenko: "Re: [PATCH v6 13/21] iommu/tegra: gart: Integrate with Memory Controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Dec 09, 2018 at 11:29:42PM +0300, Dmitry Osipenko wrote:
> The device-tree binding has been changed. There is no separate GART device
> anymore, it is squashed into the Memory Controller. Integrate GART module
> with the MC in a way it is done for the SMMU of Tegra30+.
>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
> drivers/iommu/Kconfig | 1 +
> drivers/iommu/tegra-gart.c | 77 ++++++++++++--------------------------
> drivers/memory/tegra/mc.c | 41 ++++++++++++++++++++
> include/soc/tegra/mc.h | 27 +++++++++++++
> 4 files changed, 93 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index d9a25715650e..83c099bb7288 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -282,6 +282,7 @@ config ROCKCHIP_IOMMU
> config TEGRA_IOMMU_GART
> bool "Tegra GART IOMMU Support"
> depends on ARCH_TEGRA_2x_SOC
> + depends on TEGRA_MC
> select IOMMU_API
> help
> Enables support for remapping discontiguous physical memory
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index 835fea461c59..0a72b6afa842 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -19,16 +19,17 @@
> * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> */
>
> -#include <linux/init.h>
> #include <linux/io.h>
> #include <linux/iommu.h>
> #include <linux/list.h>
> #include <linux/moduleparam.h>
> -#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/vmalloc.h>
>
> +#include <soc/tegra/mc.h>
> +
> /* bitmap of the page sizes currently supported */
> #define GART_IOMMU_PGSIZES (SZ_4K)
>
> @@ -397,9 +398,8 @@ static const struct iommu_ops gart_iommu_ops = {
> .iotlb_sync = gart_iommu_sync,
> };
>
> -static int tegra_gart_suspend(struct device *dev)
> +int tegra_gart_suspend(struct gart_device *gart)
> {
> - struct gart_device *gart = dev_get_drvdata(dev);
> unsigned long iova;
> u32 *data = gart->savedata;
> unsigned long flags;
> @@ -411,9 +411,8 @@ static int tegra_gart_suspend(struct device *dev)
> return 0;
> }
>
> -static int tegra_gart_resume(struct device *dev)
> +int tegra_gart_resume(struct gart_device *gart)
> {
> - struct gart_device *gart = dev_get_drvdata(dev);
> unsigned long flags;
>
> spin_lock_irqsave(&gart->pte_lock, flags);
> @@ -422,41 +421,39 @@ static int tegra_gart_resume(struct device *dev)
> return 0;
> }
>
> -static int tegra_gart_probe(struct platform_device *pdev)
> +struct gart_device *tegra_gart_probe(struct device *dev,
> + const struct tegra_smmu_soc *soc,
> + struct tegra_mc *mc)
> {
> struct gart_device *gart;
> - struct resource *res, *res_remap;
> + struct resource *res_remap;
> void __iomem *gart_regs;
> - struct device *dev = &pdev->dev;
> int ret;
>
> BUILD_BUG_ON(PAGE_SHIFT != GART_PAGE_SHIFT);
>
> + /* Tegra30+ has an SMMU and no GART */
> + if (soc)
> + return NULL;

This looks weird. Why do we even call tegra_gart_probe() on anything but
Tegra20?

> +
> /* the GART memory aperture is required */
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - res_remap = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - if (!res || !res_remap) {
> + res_remap = platform_get_resource(to_platform_device(dev),
> + IORESOURCE_MEM, 1);
> + if (!res_remap) {
> dev_err(dev, "GART memory aperture expected\n");
> - return -ENXIO;
> + return ERR_PTR(-ENXIO);
> }
>
> gart = devm_kzalloc(dev, sizeof(*gart), GFP_KERNEL);
> if (!gart) {
> dev_err(dev, "failed to allocate gart_device\n");
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
> }
>
> - gart_regs = devm_ioremap(dev, res->start, resource_size(res));
> - if (!gart_regs) {
> - dev_err(dev, "failed to remap GART registers\n");
> - return -ENXIO;
> - }
> -
> - ret = iommu_device_sysfs_add(&gart->iommu, &pdev->dev, NULL,
> - dev_name(&pdev->dev));
> + ret = iommu_device_sysfs_add(&gart->iommu, dev, NULL, "gart");
> if (ret) {
> dev_err(dev, "Failed to register IOMMU in sysfs\n");
> - return ret;
> + return ERR_PTR(ret);
> }
>
> iommu_device_set_ops(&gart->iommu, &gart_iommu_ops);
> @@ -468,7 +465,8 @@ static int tegra_gart_probe(struct platform_device *pdev)
> goto remove_sysfs;
> }
>
> - gart->dev = &pdev->dev;
> + gart->dev = dev;
> + gart_regs = mc->regs + GART_REG_BASE;
> spin_lock_init(&gart->pte_lock);
> spin_lock_init(&gart->client_lock);
> INIT_LIST_HEAD(&gart->client);
> @@ -483,46 +481,19 @@ static int tegra_gart_probe(struct platform_device *pdev)
> goto unregister_iommu;
> }
>
> - platform_set_drvdata(pdev, gart);
> do_gart_setup(gart, NULL);
>
> gart_handle = gart;
>
> - return 0;
> + return gart;
>
> unregister_iommu:
> iommu_device_unregister(&gart->iommu);
> remove_sysfs:
> iommu_device_sysfs_remove(&gart->iommu);
>
> - return ret;
> -}
> -
> -static const struct dev_pm_ops tegra_gart_pm_ops = {
> - .suspend = tegra_gart_suspend,
> - .resume = tegra_gart_resume,
> -};
> -
> -static const struct of_device_id tegra_gart_of_match[] = {
> - { .compatible = "nvidia,tegra20-gart", },
> - { },
> -};
> -
> -static struct platform_driver tegra_gart_driver = {
> - .probe = tegra_gart_probe,
> - .driver = {
> - .name = "tegra-gart",
> - .pm = &tegra_gart_pm_ops,
> - .of_match_table = tegra_gart_of_match,
> - .suppress_bind_attrs = true,
> - },
> -};
> -
> -static int __init tegra_gart_init(void)
> -{
> - return platform_driver_register(&tegra_gart_driver);
> + return ERR_PTR(ret);
> }
> -subsys_initcall(tegra_gart_init);
>
> module_param(gart_debug, bool, 0644);
> MODULE_PARM_DESC(gart_debug, "Enable GART debugging");
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index 55ecfb2d8cfd..4cae1c3a853b 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -702,13 +702,54 @@ static int tegra_mc_probe(struct platform_device *pdev)
> PTR_ERR(mc->smmu));
> }
>
> + if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART)) {
> + mc->gart = tegra_gart_probe(&pdev->dev, mc->soc->smmu, mc);
> + if (IS_ERR(mc->gart))
> + dev_err(&pdev->dev, "failed to probe GART: %ld\n",
> + PTR_ERR(mc->gart));
> + }

Perhaps if we add a check for for !mc->soc->smmu here we can avoid
passing this data structure to tegra_gart_probe() and remove the
corresponding check from tegra_gart_probe(). That seems more like a
more logical sequence than attempting to probe GART on device that may
not have one and return.

Also, since there's no error return here, do we want to set mc->gart to
NULL on error to avoid crashing later on...

> +
> + return 0;
> +}
> +
> +static int tegra_mc_suspend(struct device *dev)
> +{
> + struct tegra_mc *mc = dev_get_drvdata(dev);
> + int err;
> +
> + if (mc->gart) {

... like here?

> + err = tegra_gart_suspend(mc->gart);
> + if (err)
> + return err;
> + }
> +
> return 0;
> }
>
> +static int tegra_mc_resume(struct device *dev)
> +{
> + struct tegra_mc *mc = dev_get_drvdata(dev);
> + int err;
> +
> + if (mc->gart) {

And here?

> + err = tegra_gart_resume(mc->gart);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops tegra_mc_pm_ops = {
> + .suspend = tegra_mc_suspend,
> + .resume = tegra_mc_resume,
> +};
> +
> static struct platform_driver tegra_mc_driver = {
> .driver = {
> .name = "tegra-mc",
> .of_match_table = tegra_mc_of_match,
> + .pm = &tegra_mc_pm_ops,
> .suppress_bind_attrs = true,
> },
> .prevent_deferred_probe = true,
> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> index db5bfdf589b4..5da42e3fb801 100644
> --- a/include/soc/tegra/mc.h
> +++ b/include/soc/tegra/mc.h
> @@ -77,6 +77,7 @@ struct tegra_smmu_soc {
>
> struct tegra_mc;
> struct tegra_smmu;
> +struct gart_device;
>
> #ifdef CONFIG_TEGRA_IOMMU_SMMU
> struct tegra_smmu *tegra_smmu_probe(struct device *dev,
> @@ -96,6 +97,31 @@ static inline void tegra_smmu_remove(struct tegra_smmu *smmu)
> }
> #endif
>
> +#ifdef CONFIG_TEGRA_IOMMU_GART
> +struct gart_device *tegra_gart_probe(struct device *dev,
> + const struct tegra_smmu_soc *soc,
> + struct tegra_mc *mc);
> +int tegra_gart_suspend(struct gart_device *gart);
> +int tegra_gart_resume(struct gart_device *gart);
> +#else
> +static inline struct gart_device *
> +tegra_gart_probe(struct device *dev, const struct tegra_smmu_soc *soc,
> + struct tegra_mc *mc)
> +{
> + return NULL;
> +}
> +
> +static inline int tegra_gart_suspend(struct gart_device *gart)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int tegra_gart_resume(struct gart_device *gart)
> +{
> + return -ENODEV;
> +}
> +#endif

That doesn't look right. If we don't enable GART, then the dummy
implementations will be used, but they return error codes, so
suspend/resume of MC will also fail, causing the whole system to
not be able to suspend if GART is disabled.

I think it'd be better to avoid the dummy functions and instead add
extra checks for IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) where these are
called. That way references to these functions should be discarded
at translation time.

To be specific, tegra_mc_suspend() and tegra_mc_resume() would change
like this:

if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART) && mc->gart)

Thierry

Attachment: signature.asc
Description: PGP signature

Next message: Catalin Marinas: "Re: [PATCH V2] kmemleak: Add config to select auto scan"
Previous message: Alexandre Belloni: "Re: [PATCH] ARM: at91: add support in soc driver for LPDDR2 SiP"
In reply to: Dmitry Osipenko: "[PATCH v6 13/21] iommu/tegra: gart: Integrate with Memory Controller driver"
Next in thread: Dmitry Osipenko: "Re: [PATCH v6 13/21] iommu/tegra: gart: Integrate with Memory Controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]