Hi Mark,
On Thu, 14 Oct 2021 at 15:11, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 14 Oct 2021 14:55:19 -0600
Hi Mark,
On Thu, 14 Oct 2021 at 14:51, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 14 Oct 2021 14:20:21 -0600
Hi Mark,
On Thu, 14 Oct 2021 at 13:35, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Mon, 11 Oct 2021 11:00:34 -0600
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?
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@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.
Well, at this point they're so much out of date that I don't look there unless I'm looking for someting that I know to be u-boot specific.
But without this it is not discoverable in U-Boot so no one knows what the driver is referring to.
Really? The canonical place for device tree bindings that aren't part of the device tree specification itself *is* the Linux kernel tree.
Yes please, these should be in the U-Boot tree at this point. If they move out of the kernel into some other repo then we can revisit it. I don't really understand your reluctance. I hope that what you say will be true at some point, but as you probably know there are some U-Boot bindings which are not in that repo.
Should I still add my Signed-off-by on files copied verbatim from the Linux kernel sources?
Yes that's my understanding..."The Signed-off-by: tag indicates that the signer was involved in the development of the patch, or that he/she was in the patch's delivery path"
That's why I sometimes add a tag when applying a patch, e.g. if I make a minor tweak.
Regards, Simon