Re: [PATCH v2 1/7] iommu: Add IOMMU uclass

[Simon Glass <sjg@chromium.org>]

Hi Mark,

On Sun, 3 Oct 2021 at 12:31, Mark Kettenis <kettenis@openbsd.org> wrote:
>
> This uclass is intended to manage IOMMUs on systems where the
> IOMMUs are not in bypass mode by default.  In that case U-Boot
> cannot ignore the IOMMUs if it wants to use devices that need
> to do DMA and sit behind such an IOMMU.
>
> This initial IOMMU uclass implementation does not implement and
> device ops and is intended for IOMMUs that have a bypass mode
> that does not require address translation.  Support for IOMMUs
> that do require address translation is planned and device ops
> will be defined when support for such IOMMUs will be added.
>
> Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> ---
>  drivers/Kconfig              |  2 ++
>  drivers/Makefile             |  1 +
>  drivers/core/device.c        |  8 +++++++
>  drivers/iommu/Kconfig        | 13 +++++++++++
>  drivers/iommu/Makefile       |  3 +++
>  drivers/iommu/iommu-uclass.c | 45 ++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h       |  1 +
>  include/iommu.h              | 16 +++++++++++++
>  8 files changed, 89 insertions(+)
>  create mode 100644 drivers/iommu/Kconfig
>  create mode 100644 drivers/iommu/Makefile
>  create mode 100644 drivers/iommu/iommu-uclass.c
>  create mode 100644 include/iommu.h
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 417d6f88c2..b26ca8cf70 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -50,6 +50,8 @@ source "drivers/i2c/Kconfig"
>
>  source "drivers/input/Kconfig"
>
> +source "drivers/iommu/Kconfig"
> +
>  source "drivers/led/Kconfig"
>
>  source "drivers/mailbox/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index fd218c9056..166aeb9817 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -109,6 +109,7 @@ obj-y += mtd/
>  obj-y += pwm/
>  obj-y += reset/
>  obj-y += input/
> +obj-y += iommu/
>  # SOC specific infrastructure drivers.
>  obj-y += smem/
>  obj-y += thermal/
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 29668f6fb3..5f480ad443 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -28,6 +28,7 @@
>  #include <dm/uclass.h>
>  #include <dm/uclass-internal.h>
>  #include <dm/util.h>
> +#include <iommu.h>
>  #include <linux/err.h>
>  #include <linux/list.h>
>  #include <power-domain.h>
> @@ -543,6 +544,13 @@ int device_probe(struct udevice *dev)
>                         goto fail;
>         }
>
> +       if (CONFIG_IS_ENABLED(IOMMU) && dev->parent &&
> +           (device_get_uclass_id(dev) != UCLASS_IOMMU)) {
> +               ret = dev_iommu_probe(dev);
> +               if (ret)
> +                       goto fail;
> +       }
> +
>         ret = device_get_dma_constraints(dev);
>         if (ret)
>                 goto fail;
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> new file mode 100644
> index 0000000000..8cb377560e
> --- /dev/null
> +++ b/drivers/iommu/Kconfig
> @@ -0,0 +1,13 @@
> +#
> +# IOMMU devices
> +#
> +
> +menu "IOMMU device drivers"
> +
> +config IOMMU
> +       bool "Enable Driver Model for IOMMU drivers"
> +       depends on DM
> +       help
> +         Enable driver model for IOMMU devices.

Need at least three lines. What is an IOMMU? How does it relate to
other devices?

> +
> +endmenu
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> new file mode 100644
> index 0000000000..f1ceb10150
> --- /dev/null
> +++ b/drivers/iommu/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +
> +obj-$(CONFIG_IOMMU) += iommu-uclass.o
> diff --git a/drivers/iommu/iommu-uclass.c b/drivers/iommu/iommu-uclass.c
> new file mode 100644
> index 0000000000..5c55df3066
> --- /dev/null
> +++ b/drivers/iommu/iommu-uclass.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Mark Kettenis <kettenis@openbsd.org>
> + */
> +
> +#define LOG_CATEGORY UCLASS_IOMMU
> +
> +#include <common.h>
> +#include <dm.h>
> +
> +#if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA))
> +int dev_iommu_probe(struct udevice *dev)

Can we rename to dev_iommus_enable()? I like to keep probe for use by
DM itself so it is confusing to talk about probing in a context other
than probing a single device.

> +{
> +       struct ofnode_phandle_args args;
> +       struct udevice *dev_iommu;
> +       int i, count, ret = 0;
> +
> +       count = dev_count_phandle_with_args(dev, "iommus",
> +                                           "#iommu-cells", 0);

Do you have a binding file you can add to docs/device-tree-bindings ?

> +       for (i = 0; i < count; i++) {
> +               ret = dev_read_phandle_with_args(dev, "iommus",
> +                                                "#iommu-cells", 0, i, &args);
> +               if (ret) {
> +                       debug("%s: dev_read_phandle_with_args failed: %d\n",
> +                             __func__, ret);
> +                       return ret;
> +               }
> +
> +               ret = uclass_get_device_by_ofnode(UCLASS_IOMMU, args.node,
> +                                                 &dev_iommu);
> +               if (ret) {
> +                       debug("%s: uclass_get_device_by_ofnode failed: %d\n",
> +                             __func__, ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
> +UCLASS_DRIVER(iommu) = {
> +       .id             = UCLASS_IOMMU,
> +       .name           = "iommu",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index e7edd409f3..56aa981613 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -61,6 +61,7 @@ enum uclass_id {
>         UCLASS_I2C_MUX,         /* I2C multiplexer */
>         UCLASS_I2S,             /* I2S bus */
>         UCLASS_IDE,             /* IDE device */
> +       UCLASS_IOMMU,           /* IOMMU */
>         UCLASS_IRQ,             /* Interrupt controller */
>         UCLASS_KEYBOARD,        /* Keyboard input device */
>         UCLASS_LED,             /* Light-emitting diode (LED) */
> diff --git a/include/iommu.h b/include/iommu.h
> new file mode 100644
> index 0000000000..d17a06a6f3
> --- /dev/null
> +++ b/include/iommu.h
> @@ -0,0 +1,16 @@
> +#ifndef _IOMMU_H
> +#define _IOMMU_H
> +
> +struct udevice;
> +
> +#if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) && \
> +       CONFIG_IS_ENABLED(IOMMU)
> +int dev_iommu_probe(struct udevice *dev);
> +#else
> +static inline int dev_iommu_probe(struct udevice *dev)
> +{
> +       return 0;
> +}
> +#endif
> +
> +#endif
> --
> 2.33.0
>

Regards,
Simon