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

[Simon Glass]

Hi Mark,

On Thu, 14 Oct 2021 at 13:35, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Simon Glass <sjg at chromium.org>
> > Date: Mon, 11 Oct 2021 11:00:34 -0600
> >
> > Hi Mark,
> >
> > On Sun, 3 Oct 2021 at 12:31, Mark Kettenis <kettenis at 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 at 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?
>
> Not sure if I can describe what an IOMMU is in just a few lines, but
> I'll try.
>
> > > +
> > > +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 at 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.
>
> Sure.  That was one of the names I considered myself when writing the
> code.
>
> > > +{
> > > +       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 ?
>
> The binding is available in Documentation/bindings/iommu/iommu.txt in
> the Linux tree.  But I don't think copying it into the u-boot tree
> makes sense.  It will just get out of date.  Especially because this
> will hopefully be converted into a proper DT schema in the near
> future.

We do plan to sync these at some point, so don't worry about that. We
have bigger problems than the bindings.

But without this it is not discoverable in U-Boot so no one knows what
the driver is referring to.

Regards,
Simon