From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DAA9AC433EF for ; Mon, 20 Sep 2021 08:33:59 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F0A3461077 for ; Mon, 20 Sep 2021 08:33:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org F0A3461077 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5B7088290A; Mon, 20 Sep 2021 10:33:56 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 57FF381280; Mon, 20 Sep 2021 10:33:53 +0200 (CEST) Received: from sibelius.xs4all.nl (sibelius.xs4all.nl [83.163.83.176]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 7C53281280 for ; Mon, 20 Sep 2021 10:33:49 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=mark.kettenis@xs4all.nl Received: from localhost (bloch.sibelius.xs4all.nl [local]) by bloch.sibelius.xs4all.nl (OpenSMTPD) with ESMTPA id f6b24431; Mon, 20 Sep 2021 10:33:47 +0200 (CEST) Date: Mon, 20 Sep 2021 10:33:47 +0200 (CEST) From: Mark Kettenis To: Simon Glass Cc: kettenis@openbsd.org, u-boot@lists.denx.de, oliver.graute@kococonnector.com, stephan@gerhold.net, masami.hiramatsu@linaro.org, priyanka.jain@nxp.com, ycliang@andestech.com, padmarao.begari@microchip.com, michael@walle.cc, kishon@ti.com, xypron.glpk@gmx.de, bin.meng@windriver.com, ashe@kivikakk.ee, michal.simek@xilinx.com, wasim.khan@nxp.com, hs@denx.de, igor.opaniuk@foundries.io, ye.li@nxp.com, sr@denx.de, takahiro.akashi@linaro.org, weijie.gao@mediatek.com, vabhav.sharma@nxp.com, andriy.shevchenko@linux.intel.com, p.yadav@ti.com In-Reply-To: (message from Simon Glass on Sun, 19 Sep 2021 21:16:00 -0600) Subject: Re: [PATCH 3/5] misc: Add Apple DART driver References: <20210918135437.36667-1-kettenis@openbsd.org> <20210918135437.36667-4-kettenis@openbsd.org> Message-ID: <56146d936d16f96c@bloch.sibelius.xs4all.nl> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean > From: Simon Glass > Date: Sun, 19 Sep 2021 21:16:00 -0600 > > Hi Mark, > > On Sat, 18 Sept 2021 at 07:55, Mark Kettenis 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 > > --- > > 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 > > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index 997b713221..d70b060e74 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -38,6 +38,13 @@ config ALTERA_SYSID > > Select this to enable a sysid for Altera devices. Please find > > details on the "Embedded Peripherals IP User Guide" of Altera. > > > > +config APPLE_DART > > + bool "Apple DART support" > > + depends on MISC && ARCH_APPLE > > + default y > > + help > > + Enable support for the DART on Apple SoCs. > > Should have at least 3 lines. E.g. what does DART stand for, what does > it do and what does the driver support? Sure. The DART is what we usually call an IOMMU. > > + > > config ATSHA204A > > bool "Support for Atmel ATSHA204A module" > > depends on MISC > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > index b64cd2a4de..f666cd392d 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -29,6 +29,7 @@ endif > > endif > > obj-$(CONFIG_ALI152X) += ali512x.o > > obj-$(CONFIG_ALTERA_SYSID) += altera_sysid.o > > +obj-$(CONFIG_APPLE_DART) += apple_dart.o > > obj-$(CONFIG_ATSHA204A) += atsha204a-i2c.o > > obj-$(CONFIG_CBMEM_CONSOLE) += cbmem_console.o > > obj-$(CONFIG_DS4510) += ds4510.o > > diff --git a/drivers/misc/apple_dart.c b/drivers/misc/apple_dart.c > > new file mode 100644 > > index 0000000000..f619a624d0 > > --- /dev/null > > +++ b/drivers/misc/apple_dart.c > > @@ -0,0 +1,171 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 2021 Mark Kettenis > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define DART_PARAMS2 0x0004 > > +#define DART_PARAMS2_BYPASS_SUPPORT BIT(0) > > +#define DART_TLB_OP 0x0020 > > +#define DART_TLB_OP_OPMASK (0xfff << 20) > > +#define DART_TLB_OP_FLUSH (0x001 << 20) > > +#define DART_TLB_OP_BUSY BIT(2) > > +#define DART_TLB_OP_SIDMASK 0x0034 > > +#define DART_ERROR_STATUS 0x0040 > > +#define DART_TCR(sid) (0x0100 + 4 * (sid)) > > +#define DART_TCR_TRANSLATE_ENABLE BIT(7) > > +#define DART_TCR_BYPASS_DART BIT(8) > > +#define DART_TCR_BYPASS_DAPF BIT(12) > > +#define DART_TTBR(sid, idx) (0x0200 + 16 * (sid) + 4 * (idx)) > > +#define DART_TTBR_VALID BIT(31) > > +#define DART_TTBR_SHIFT 12 > > + > > +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. > > + 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. > > + > > +static void apple_dart_flush_tlb(struct apple_dart_priv *priv) > > comments on these functions > > > +{ > > + u32 status; > > + > > + writel(0xffffffff, priv->base + DART_TLB_OP_SIDMASK); > > + writel(DART_TLB_OP_FLUSH, priv->base + DART_TLB_OP); > > + > > + for (;;) { > > + status = readl(priv->base + DART_TLB_OP); > > + if ((status & DART_TLB_OP_OPMASK) == 0) > > + break; > > + if ((status & DART_TLB_OP_BUSY) == 0) > > + break; > > + } > > +} > > + > > +static int apple_dart_clk_init(struct udevice *dev, > > + struct apple_dart_priv *priv) > > +{ > > + int ret; > > + > > + ret = clk_get_bulk(dev, &priv->clks); > > + if (ret == -ENOSYS || ret == -ENOENT) > > Does -ENOSYS not indicate an error? If it doesn't, I think a comment > would help here. So we just learned that what we previously considered to be clocks are really better modelled as power domains. So this code will go away. > > + return 0; > > + if (ret) > > + return ret; > > + > > + ret = clk_enable_bulk(&priv->clks); > > + if (ret) { > > + clk_release_bulk(&priv->clks); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +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? > > + > > + return 0; > > +} > > + > > +static int apple_dart_probe(struct udevice *dev) > > +{ > > + struct apple_dart_priv *priv = dev_get_priv(dev); > > + phys_addr_t phys; > > + u64 *l1, *l2; > > + int sid, i, j; > > + int ret; > > + > > + apple_dart_phys_start = gd->ram_top - apple_dart_size; > > + > > + priv->base = dev_read_addr_ptr(dev); > > + if (!priv->base) > > + return -EINVAL; > > + > > + ret = apple_dart_clk_init(dev, priv); > > + if (ret) > > + return ret; > > + > > + l1 = memalign(SZ_64K, SZ_64K); > > + memset(l1, 0, SZ_64K); > > + > > + i = 0; > > + phys = apple_dart_phys_start; > > + while (phys < apple_dart_phys_start + apple_dart_size) { > > + l2 = memalign(SZ_16K, SZ_16K); > > check for error > > > + 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. > > + } > > + > > + flush_dcache_range((unsigned long)l1, (unsigned long)l1 + SZ_64K); > > + > > + for (sid = 0; sid < 16; sid++) { > > comment,...what is this doing? > > > + for (i = 0; i < 4; i++) > > + writel(0, priv->base + DART_TTBR(sid, i)); > > + } > > + > > + apple_dart_flush_tlb(priv); > > + > > + for (sid = 0; sid < 16; sid++) { > > + phys = (phys_addr_t)l1; > > + for (i = 0; i < 4; i++) { > > + writel((phys >> DART_TTBR_SHIFT) | DART_TTBR_VALID, > > + priv->base + DART_TTBR(sid, i)); > > + phys += SZ_16K; > > + } > > + } > > + > > + apple_dart_flush_tlb(priv); > > + > > + for (sid = 0; sid < 16; sid++) > > + writel(DART_TCR_TRANSLATE_ENABLE, priv->base + DART_TCR(sid)); > > + > > + return 0; > > +} > > + > > +static const struct udevice_id apple_dart_ids[] = { > > + { .compatible = "apple,t8103-dart" }, > > + { /* sentinel */ } > > +}; > > + > > +U_BOOT_DRIVER(apple_dart) = { > > + .name = "apple_dart", > > + .id = UCLASS_MISC, > > + .of_match = apple_dart_ids, > > + .priv_auto = sizeof(struct apple_dart_priv), > > + .bind = apple_dart_bind, > > + .probe = apple_dart_probe > > +}; > > -- > > 2.33.0 > > > > Regards, > Simon >