From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941452AbcIHLLP (ORCPT ); Thu, 8 Sep 2016 07:11:15 -0400 Received: from mout.kundenserver.de ([212.227.17.24]:56198 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751786AbcIHLLN (ORCPT ); Thu, 8 Sep 2016 07:11:13 -0400 From: Arnd Bergmann To: Felipe Balbi Cc: Peter Chen , Leo Li , Grygorii Strashko , Russell King - ARM Linux , Catalin Marinas , Yoshihiro Shimoda , "linux-usb@vger.kernel.org" , Sekhar Nori , lkml , Stuart Yoder , Scott Wood , David Fisher , "Thang Q. Nguyen" , Alan Stern , Greg Kroah-Hartman , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Date: Thu, 08 Sep 2016 13:11:01 +0200 Message-ID: <3757738.rjzYG3yq8e@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <87d1keirlu.fsf@linux.intel.com> References: <4934737.egJZVdaLZs@wuerfel> <87d1keirlu.fsf@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:LA3Q7B1YMcfO8VUrvaY7/DlfRrNEgXb7LSeNeT6u36oKWUHGSiq 0gki8o3nQj+UBoXhtP5FhloNtHREs9LYC8VAkbpe6XsrSoxBnOUjqooLuqaLBqriU8rA9RU w59+T1IEEkHhukflzS+wUGQzcY/ppS+DXOKT6zJMdRT9Hke6s7dZhGVXooXth/V2Q9jtdMd 2aOPg3kI9nvrfzERsRiQQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:gWyLlCQ6NP8=:u5cmyzPPlWiTtQChYq30nr uD2aKGKrVWbTMnnVGjiMnjUt0BFAVeTWfH/6QlsOnZl2HZ262Gvy2wYKlIA4PD2oleaEDKwKt kMpZX1WKyyVKYWTEYQ7S4tKLYWG40fPbhJMLI5++SnaCbl/4GliAQyGLeo5utBTp/7xdRAgvE +h/u3uKBJcUKTz4DbENx383t2IeqBPSsC1X/DkEhn/cmiVebjOULdX/7HLDLiZXvFdHQYpB8J /npDCpIz1TMQXVfa3TyotoHLSwNefkAFqPzxyh40rbO+X6vVJ8xIyLwYFPhtKo/FnPDul63ly qEHxaADqgcZ89gFhP2i3dYz4f0YBG/LgTdLOqXWyeixDZkMW3Wto0trXufwKV20R+n26QFAyo 6t5DzKic3zMq6SvfmJh6CZ9Q4IHAs3eQ5+fVll1CONIMhjHU19G1Dskl4Lgl3ur1EbC+hAZvj PA1fmKU8rXzyYAr1jx8/c2tUVek1lP5B+ydEfynXgsMsDKiG4973bEmzf6psmLrvg2xanKDB5 Z4Jq2Zjz3pCb/Tn1r5HzkqAERASErFfOOh8x34un3aoRyH0NitIENe8Qh/Kr6f+s/0EdQSuE1 BfF31JrA5p9hDk/lURK1bsa/NbubU2TLA1OgXBZ0laqa8xAnjY3SVYbban5uHetyUKrYCMR6V d5oWi88lEeRUD9jQfx/CXjs6azztpb0sDUUC2wwbQ6/oDzTJFPk+9P4JGDyfrXfE1AapkVJAX 3W8NhDdshPOVl4qi0qtMeJCyjCRLHDY9OlBtq4djRv/nzrP3UYOFP2W9noZdR36Qo983Ow2l0 RRQwI3zkBVlJf+RYHPHlEuLNG4r8sAhnIKjbi8GvEK+xkQqnlmlAjlEwdZmrXuHjLJUoWWRmM +GpTJvyVxhdkM7+4GHkGko+kKEFpTg7fr6InK1S1o= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > >> Arnd Bergmann writes: > >> > That sounds a bit clumsy for the sake of consistency with PCI. > >> > The advantage is that xhci can always use the grandparent device > >> > as sysdev whenever it isn't probed through PCI or firmware > >> > itself, but the purpose of the dwc3-glue is otherwise questionable. > >> > > >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 > >> > device when that is created from the PCI driver and checking for that > >> > with the device property interface instead? If it's "snps,dwc3" > >> > we use the device itself while for "snps,dwc3-pci", we use the parent? > >> > >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? > > > > That would be incompatible with the USB binding, as the sysdev > > is assumed to be a USB host controller with #address-cells=<1> > > and #size-cells=<0> in order to hold the child devices, for > > example: > > > > / { > > omap_dwc3_1: omap_dwc3_1@48880000 { > > compatible = "ti,dwc3"; > > #address-cells = <1>; > > #size-cells = <1>; > > ranges; > > usb1: usb@48890000 { > > compatible = "snps,dwc3"; > > reg = <0x48890000 0x17000>; > > #address-cells = <1>; > > #size-cells = <0>; > > interrupts = , > > , > > ; > > interrupt-names = "peripheral", > > "host", > > "otg"; > > phys = <&usb2_phy1>, <&usb3_phy1>; > > phy-names = "usb2-phy", "usb3-phy"; > > > > hub@1 { > > compatible = "usb5e3,608"; > > reg = <1>; > > #address-cells = <1>; > > #size-cells = <0>; > > > > ethernet@1 { > > compatible = "usb424,ec00"; > > mac-address = [00 11 22 33 44 55]; > > reg = <1>; > > }; > > }; > > }; > > }; > > > > It's also the node that contains the "phys" properties and > > presumably other properties like "otg-rev", "maximum-speed" > > etc. > > > > If we make the sysdev point to the parent, then we can no longer > > look up those properties and child devices from the USB core code > > by looking at "sysdev->of_node". > > this also makes things more interesting. I can't of anything other than > having some type of flag passed via e.g. device_properties by dwc3-pci.c > :-s Ok. > It's quite a hack, though. I still think that inheriting DMA (or > manually initializing a child with parent's DMA bits and pieces) is the > best way to go. So we're back to of_dma_configure() and > acpi_dma_configure(), right? That won't solve the problems with the DT properties or the dma configuration for PCI devices though. > But this needs to be done before dwc3_probe() executes. For dwc3-pci > that's easy, but for DT devices, seems like it should be in of > core. Below is, clearly, not enough but should show the idea: > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index fd5cfad7c403..a54610198946 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node *np) > * 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); > + if (!dev->coherent_dma_mask) { > + if (!dev->parent->coherent_dma_mask) > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > + else > + dev->coherent_dma_mask = dev->parent->coherent_dma_mask; > + } > As the comment above that code says, the default 32-bit mask is intentional, and you need the driver to ask for the mask it wants using dma_set_mask_and_coherent(), while the platform code should be able to use dev->of_node to figure out whether that mask is supported. Just setting the initial mask to something else based on what the parent supports will not do the right thing elsewhere. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Thu, 08 Sep 2016 13:11:01 +0200 Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: <87d1keirlu.fsf@linux.intel.com> References: <4934737.egJZVdaLZs@wuerfel> <87d1keirlu.fsf@linux.intel.com> Message-ID: <3757738.rjzYG3yq8e@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > >> Arnd Bergmann writes: > >> > That sounds a bit clumsy for the sake of consistency with PCI. > >> > The advantage is that xhci can always use the grandparent device > >> > as sysdev whenever it isn't probed through PCI or firmware > >> > itself, but the purpose of the dwc3-glue is otherwise questionable. > >> > > >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 > >> > device when that is created from the PCI driver and checking for that > >> > with the device property interface instead? If it's "snps,dwc3" > >> > we use the device itself while for "snps,dwc3-pci", we use the parent? > >> > >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? > > > > That would be incompatible with the USB binding, as the sysdev > > is assumed to be a USB host controller with #address-cells=<1> > > and #size-cells=<0> in order to hold the child devices, for > > example: > > > > / { > > omap_dwc3_1: omap_dwc3_1 at 48880000 { > > compatible = "ti,dwc3"; > > #address-cells = <1>; > > #size-cells = <1>; > > ranges; > > usb1: usb at 48890000 { > > compatible = "snps,dwc3"; > > reg = <0x48890000 0x17000>; > > #address-cells = <1>; > > #size-cells = <0>; > > interrupts = , > > , > > ; > > interrupt-names = "peripheral", > > "host", > > "otg"; > > phys = <&usb2_phy1>, <&usb3_phy1>; > > phy-names = "usb2-phy", "usb3-phy"; > > > > hub at 1 { > > compatible = "usb5e3,608"; > > reg = <1>; > > #address-cells = <1>; > > #size-cells = <0>; > > > > ethernet at 1 { > > compatible = "usb424,ec00"; > > mac-address = [00 11 22 33 44 55]; > > reg = <1>; > > }; > > }; > > }; > > }; > > > > It's also the node that contains the "phys" properties and > > presumably other properties like "otg-rev", "maximum-speed" > > etc. > > > > If we make the sysdev point to the parent, then we can no longer > > look up those properties and child devices from the USB core code > > by looking at "sysdev->of_node". > > this also makes things more interesting. I can't of anything other than > having some type of flag passed via e.g. device_properties by dwc3-pci.c > :-s Ok. > It's quite a hack, though. I still think that inheriting DMA (or > manually initializing a child with parent's DMA bits and pieces) is the > best way to go. So we're back to of_dma_configure() and > acpi_dma_configure(), right? That won't solve the problems with the DT properties or the dma configuration for PCI devices though. > But this needs to be done before dwc3_probe() executes. For dwc3-pci > that's easy, but for DT devices, seems like it should be in of > core. Below is, clearly, not enough but should show the idea: > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index fd5cfad7c403..a54610198946 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node *np) > * 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); > + if (!dev->coherent_dma_mask) { > + if (!dev->parent->coherent_dma_mask) > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > + else > + dev->coherent_dma_mask = dev->parent->coherent_dma_mask; > + } > As the comment above that code says, the default 32-bit mask is intentional, and you need the driver to ask for the mask it wants using dma_set_mask_and_coherent(), while the platform code should be able to use dev->of_node to figure out whether that mask is supported. Just setting the initial mask to something else based on what the parent supports will not do the right thing elsewhere. Arnd