From patchwork Wed Apr 27 16:53:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Felipe Balbi X-Patchwork-Id: 8960481 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id A63C7BF29F for ; Wed, 27 Apr 2016 16:56:05 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 856E52022D for ; Wed, 27 Apr 2016 16:56:04 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 69F0C20109 for ; Wed, 27 Apr 2016 16:56:03 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1avSji-0006Ta-0W; Wed, 27 Apr 2016 16:54:50 +0000 Received: from casper.infradead.org ([2001:770:15f::2]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1avSjF-0005sl-Jz for linux-arm-kernel@bombadil.infradead.org; Wed, 27 Apr 2016 16:54:21 +0000 Received: from mail-lf0-x244.google.com ([2a00:1450:4010:c07::244]) by casper.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1avSjC-0002au-Md for linux-arm-kernel@lists.infradead.org; Wed, 27 Apr 2016 16:54:20 +0000 Received: by mail-lf0-x244.google.com with SMTP id m101so8550404lfi.1 for ; Wed, 27 Apr 2016 09:53:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:in-reply-to:references:user-agent:date :message-id:mime-version; bh=T6NJ322erJZmnj8YOhZzdkjtm67rvuKsU08Id8ezM1Q=; b=JKmug66mh9A3NfgzHujTb1KwyYlq2PBopyjbhX4p4ajqW2W3YMe4CbdanPeYzy/JaC vMXHUynFw3Il82dTj+MGZI1FlnyQVjcvnsq9rP25svEHTxjwi5vlfKxQY8gDT888bhpC S+KUmEI6kl4sBfHOSaK9UBWK3CuxGZ2xEN3RGMLYIAKApw9+kqtS7yWUbxrLVVuwhDGR 3hjmu1ZCc6rxH5nsupxifqTfFSGvD8iUwmNRTtDJy0TJ7tJP5oYygWazJUZAmeWwm3Oq 4JUh1gl9p0LhJwBvKCEtrt2gQA2dE7UQzdj+TGMHeVWTbmR9+RGGa19h7q6CCIX8sNlv O9DQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:in-reply-to:references :user-agent:date:message-id:mime-version; bh=T6NJ322erJZmnj8YOhZzdkjtm67rvuKsU08Id8ezM1Q=; b=W51SAcIH9Lf8yeQ2hGUbU4LFhdCtZVKRiFp8x1lEeThPPgp11mQz2CNhtp1Hr/kIcY MiVKWqyerfJwvTlg0EKNLf5cWj8nHeUQmrWHr8So5PonYdHCsPA0FvuZ1b/JgM03misS 9x0QJHFEsjxdihvWc8f/VnmTPFFc1NF5f1TsgNvqvKNfKztFlAicQ8EK6X75udh+aRcH lnsoeGZPpGmkPc0jxR34EUlJd1Spv8OeyEi6ntp85tpLnU5B0saeukSry9ItnBJoTaw4 fRJP3bF2cmUHJs9kchuMuFjjhH2NfNemwXHQNAh604FsH7K4fdy3u0/5ml/lXwkbw4Ed Ar/g== X-Gm-Message-State: AOPr4FX9nH6DcKss+z/TCGR2M/loYCZMpVQ/WCerZuwpkPjwWDzdxHmVJWpH/tC+1Pmviw== X-Received: by 10.25.79.12 with SMTP id d12mr4167940lfb.68.1461776036567; Wed, 27 Apr 2016 09:53:56 -0700 (PDT) Received: from localhost (drbcxd1v9s7--9m1lrp-4.rev.dnainternet.fi. [2001:14bb:70:4531:d63d:7eff:feaa:a973]) by smtp.gmail.com with ESMTPSA id jw6sm924535lbc.13.2016.04.27.09.53.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Apr 2016 09:53:54 -0700 (PDT) From: Felipe Balbi To: Arnd Bergmann , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: <4985498.GT33RtioOA@wuerfel> References: <1461612094-30939-1-git-send-email-grygorii.strashko@ti.com> <9452408.uEOOA4q1YW@wuerfel> <20160427155019.GE20646@e104818-lin.cambridge.arm.com> <4985498.GT33RtioOA@wuerfel> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/25.0.92.2 (x86_64-pc-linux-gnu) Date: Wed, 27 Apr 2016 19:53:51 +0300 Message-ID: <877ffjro0w.fsf@ti.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160427_175418_872850_06977C66 X-CRM114-Status: GOOD ( 41.59 ) X-Spam-Score: -2.3 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Grygorii Strashko , Catalin Marinas , Yoshihiro Shimoda , linux-usb@vger.kernel.org, Sekhar Nori , linux-kernel@vger.kernel.org, David Fisher , "Thang Q. Nguyen" , Greg Kroah-Hartman Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, Arnd Bergmann writes: > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote: >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: >> > > >> > > I would be in favour of a dma_inherit() function as well. We could hack >> > > something up in the arch code (like below) but I would rather prefer an >> > > explicit dma_inherit() call by drivers creating such devices. >> > > >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h >> > > index ba437f090a74..ea6fb9b0e8fa 100644 >> > > --- a/arch/arm64/include/asm/dma-mapping.h >> > > +++ b/arch/arm64/include/asm/dma-mapping.h >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; >> > > >> > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) >> > > { >> > > - if (dev && dev->archdata.dma_ops) >> > > - return dev->archdata.dma_ops; >> > > + while (dev) { >> > > + if (dev->archdata.dma_ops) >> > > + return dev->archdata.dma_ops; >> > > + dev = dev->parent; >> > > + } >> > >> > I think this would be a very bad idea: we don't want to have random >> > devices be able to perform DMA just because their parent devices >> > have been set up that way. >> >> I agree, it's a big hack. It would be nice to have a simpler way to do >> this in driver code rather than explicitly calling >> of_dma_configure/arch_setup_dma_ops as per the original patch in this >> thread. > > I haven't followed the entire discussion, but what's wrong with passing > around a pointer to a 'struct device *hwdev' that represents the physical > device that does the DMA? that will likely create duplicated solutions in several drivers and it'll be a pain to maintain. There's another complication, dwc3 can be integrated in many different ways. See the device child-parent tree representations below: a) with a parent PCI device: pci_bus_type - dwc3-pci - dwc3 - xhci-plat b) with a parent platform_device (OF): platform_bus_type - dwc3-${omap,st,of-simple,exynos,keystone} - dwc3 - xhci-plat c) without a parent at all (thanks Grygorii): platform_bus_type - dwc3 - xhci-plat (a) and (b) above are the common cases. The DMA-capable device is clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only having proper DMA configuration in OF platforms (because of the unconditional of_dma_configure() during OF device creation) and xhci-plat not knowing about DMA at all and hardcoding some crappy defaults. (c) is the uncommon case which creates some problems. In this case, dwc3 itself is the DMA-capable device and dwc3->dev->parent is the platform_bus_type itself. Now consider the problem this creates: i. the patch that I wrote [1] becomes invalid for (c), thanks to Grygorii for pointing this out before it was too late. ii. xhci-plat can also be described directly in DT (and is in some cases). This means that assuming xhci-plat's parent's parent to be the DMA-capable device is also an invalid assumption. iii. one might argue that for DT-based platforms *with* a glue layer ((b) above), OF already "copies" some sensible DMA defaults during device creation. PCI-based systems just don't have the luxury of creating random PCI devices like that :-) I say it copies because I can pass *any* struct device_node pointer and it'll just copy that to the struct device argument. Here's of_dma_configure() to make your life easier: void of_dma_configure(struct device *dev, struct device_node *np) { u64 dma_addr, paddr, size; int ret; bool coherent; unsigned long offset; struct iommu_ops *iommu; /* * Set default coherent_dma_mask to 32 bit. Drivers are expected to * setup the correct supported mask. */ if (!dev->coherent_dma_mask) dev->coherent_dma_mask = DMA_BIT_MASK(32); /* * Set it to coherent_dma_mask by default if the architecture * code has not set it. */ if (!dev->dma_mask) dev->dma_mask = &dev->coherent_dma_mask; ret = of_dma_get_range(np, &dma_addr, &paddr, &size); if (ret < 0) { dma_addr = offset = 0; size = dev->coherent_dma_mask + 1; } else { offset = PFN_DOWN(paddr - dma_addr); /* * Add a work around to treat the size as mask + 1 in case * it is defined in DT as a mask. */ if (size & 1) { dev_warn(dev, "Invalid size 0x%llx for dma-range\n", size); size = size + 1; } if (!size) { dev_err(dev, "Adjusted size 0x%llx invalid\n", size); return; } dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); } dev->dma_pfn_offset = offset; /* * Limit coherent and dma mask based on size and default mask * set by the driver. */ dev->coherent_dma_mask = min(dev->coherent_dma_mask, DMA_BIT_MASK(ilog2(dma_addr + size))); *dev->dma_mask = min((*dev->dma_mask), DMA_BIT_MASK(ilog2(dma_addr + size))); coherent = of_dma_is_coherent(np); dev_dbg(dev, "device is%sdma coherent\n", coherent ? " " : " not "); iommu = of_iommu_configure(dev, np); dev_dbg(dev, "device is%sbehind an iommu\n", iommu ? " " : " not "); arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); } The only clean way to fix this up is with a dma_inherit()-like API which would allow dwc3's glue layers ((a) and (b) above) to initialize child's (dwc3) DMA configuration during child's creation. Something like below: that's all I'm asking for :-) dma_inherit() should, probably, be arch-specific to handle details like IOMMU (which today sits under archdata). without something like this, we're gonna have to resort to tons of checks trying to find the correct DMA configuration to use in all possible usage scenarios of dwc3. Note, however, that I'm using dwc3 as an example, but anywhere you see manual platform_device creation, there's a potential problem WRT DMA and/or IOMMU. [1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=2725d6f974c4c268ae5fb746f8e3b33b76135aa8 diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c index adc1e8a624cb..74b599269e2c 100644 --- a/drivers/usb/dwc3/dwc3-pci.c +++ b/drivers/usb/dwc3/dwc3-pci.c @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci, return -ENOMEM; } + dma_inherit(&dwc->dev, dev); + memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res)); res[0].start = pci_resource_start(pci, 0);