Re: [PATCH 3/5] misc: Add Apple DART driver

21 Sep 2021

      Hi Mark,
On Mon, 20 Sept 2021 at 02:33, Mark Kettenis mark.kettenis@xs4all.nl wrote:
...
...
From: Simon Glass sjg@chromium.org
Date: Sun, 19 Sep 2021 21:16:00 -0600
Hi Mark,
On Sat, 18 Sept 2021 at 07:55, Mark Kettenis kettenis@openbsd.org wrote:
...
The DART is an IOMMU that is used on Apple's M1 SoC.  This driver
supports the DART in bypass mode as well as in a mode where it
creates a 1:1 mapping of a subset of RAM as not all DARTs support
bypass mode.  The USB3 ports integrated on the SoC use a DART
that supports bypass mode.  The 1:1 mapping will be used in the
future to support other devices such as the PCIe host bridge
of the M1 SoC.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
drivers/misc/Kconfig      |   7 ++
 drivers/misc/Makefile     |   1 +
 drivers/misc/apple_dart.c | 171 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 179 insertions(+)
 create mode 100644 drivers/misc/apple_dart.c
[..]
...
...
...

+struct apple_dart_priv {
How about s/apple_dart/dart/ ?
It makes the code easier to read.
I think using apple_dart_ consistently as a prefix makes more sense.
OK I don't mind too much. I just prefer shorter code and sometimes
people use globally unique things in drivers when it really doesn't
help anything.
...
...
...

  struct clk_bulk clks;

  void *base;

+};

+dma_addr_t apple_dart_bus_start;
+phys_addr_t apple_dart_phys_start;
+phys_size_t apple_dart_size = SZ_512M;
Try to avoid variables in drivers. Can these go in a priv struct?
Not really since the intent is that these variables specify a global
"window" that is mapped 1:1 into all the DARTs.
So there are multiple DART devices? In that can you could store this
info in a priv struct attached to the uclass.
[..]
...
...
...
+static int apple_dart_bind(struct udevice *dev)
+{

  void *base;

  int sid, i;

  base = dev_read_addr_ptr(dev);

  if (!base)

          return -EINVAL;

  u32 params2 = readl(base + DART_PARAMS2);

  if (params2 & DART_PARAMS2_BYPASS_SUPPORT) {

          for (sid = 0; sid < 16; sid++) {

                  writel(DART_TCR_BYPASS_DART | DART_TCR_BYPASS_DAPF,

                         base + DART_TCR(sid));

                  for (i = 0; i < 4; i++)

                          writel(0, base + DART_TTBR(sid, i));

          }

  }

Not allowed hardware access in bind(). Can this more to probe() ?
Well, I need to make sure that this happens before other drivers get
probed (in particular the xhci-dwc3 driver).  Is there a better
mechanism to achieve that?
If those drivers have something in the DT indicating that they need
this, then you can add a uclass_get_device_by_phandle() in those
drivers.
If not, then you can probe all the DART devices with uclass_probe_all().
Having said that, I see you are using UCLASS_MISC. I suspect this
should have its own UCLASS_IOMMU.
[..]
...
...
...

          memset(l2, 0, SZ_16K);

          for (j = 0; j < 2048; j++) {

                  l2[j] = phys | 0x3;

                  phys += SZ_16K;

          }

          flush_dcache_range((unsigned long)l2,

                             (unsigned long)l2 + SZ_16K);

          l1[i++] = (phys_addr_t)l2 | 0x8 | 0x3;

Do you need the cast? What are the magic numbers here? Can you use an
enum/#define ?
Not sure if we know the exact meaning of those bits yet.  But there is
a Linux driver now, so maybe I need to look at it again.
Oh well, a comment that we don't know is fine, too. People can fix it
later if it becomes known.
[..]
Regards,
Simon