From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759295AbcIHLA5 (ORCPT ); Thu, 8 Sep 2016 07:00:57 -0400 Received: from mga04.intel.com ([192.55.52.120]:60240 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757564AbcIHLA4 (ORCPT ); Thu, 8 Sep 2016 07:00:56 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,300,1470726000"; d="asc'?scan'208";a="1026822855" From: Felipe Balbi To: Arnd Bergmann 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 In-Reply-To: <4934737.egJZVdaLZs@wuerfel> References: <2733202.7lpFC7RnDm@wuerfel> <87lgz2iv6d.fsf@linux.intel.com> <4934737.egJZVdaLZs@wuerfel> User-Agent: Notmuch/0.22.1+63~g994277e (https://notmuchmail.org) Emacs/25.1.3 (x86_64-pc-linux-gnu) Date: Thu, 08 Sep 2016 14:00:13 +0300 Message-ID: <87d1keirlu.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Arnd Bergmann writes: > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: >> >> > If we do that, we have to put child devices of the dwc3 devices into >> >> > the platform glue, and it also breaks those dwc3 devices that don't >> >> > have a parent driver. >> >>=20 >> >> Well, this is easy to fix: >> >>=20 >> >> if (dwc->dev->parent) { >> >> dwc->sysdev =3D dwc->dev->parent; >> >> } else { >> >> dev_info(dwc->dev, "Please provide a glue layer!\n"); >> >> dwc->sysdev =3D dwc->dev; >> >> } >> > >> > I don't understand. Do you mean we should have an extra level of >> > stacking and splitting "static struct platform_driver dwc3_driver" >> > in two so instead of >> > >> > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.d= ev) >> > >> > we do this? >> > >> > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "x= hci" (usb_bus.dev) >>=20 >> no=20 >>=20 >> If we have a parent device, use that as sysdev, otherwise use self as >> sysdev. > > But there is often a parent device in DT, as the xhci device is > attached to some internal bus that gets turned into a platform_device > as well, so checking whether there is a parent will get the wrong > device node. oh, that makes things more interesting :-s >> > 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=3D"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? >>=20 >> 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=3D<1> > and #size-cells=3D<0> in order to hold the child devices, for > example: > > / { > omap_dwc3_1: omap_dwc3_1@48880000 { > compatible =3D "ti,dwc3"; > #address-cells =3D <1>; > #size-cells =3D <1>; > ranges; > usb1: usb@48890000 { > compatible =3D "snps,dwc3"; > reg =3D <0x48890000 0x17000>; > #address-cells =3D <1>; > #size-cells =3D <0>; > interrupts =3D , > , > ; > interrupt-names =3D "peripheral", > "host", > "otg"; > phys =3D <&usb2_phy1>, <&usb3_phy1>; > phy-names =3D "usb2-phy", "usb3-phy"; > > hub@1 { > compatible =3D "usb5e3,608"; > reg =3D <1>; > #address-cells =3D <1>; > #size-cells =3D <0>; > > ethernet@1 { > compatible =3D "usb424,ec00"; > mac-address =3D [00 11 22 33 44 55]; > reg =3D <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 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? 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 =2D-- 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. */ =2D if (!dev->coherent_dma_mask) =2D dev->coherent_dma_mask =3D DMA_BIT_MASK(32); + if (!dev->coherent_dma_mask) { + if (!dev->parent->coherent_dma_mask) + dev->coherent_dma_mask =3D DMA_BIT_MASK(32); + else + dev->coherent_dma_mask =3D dev->parent->coherent_dm= a_mask; + } =20 /* * Set it to coherent_dma_mask by default if the architecture =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJX0US9AAoJEMy+uJnhGpkGMIAQANWyer9yQlvL+DBL489E1hrw 1d/ofHScz0CgwRUGyZaIEwul8KBXaGsIm6nJqRPhdJPU7t5TffjGUJAzc+kBVseq +5ij91xHSF49o/TEMDnrTcn4LUJAKLcp/eNkB2eMiKQSRvB2W0xbLSitNCnOfGAK 1cixbcaYxzgtCSDXIQLdcAbyih8FJ7LjOcVZ9eXOSZ5krmLsmnvW1phqehFF9Dq+ knPVfJbI2xrlOkobdvmt38ntV9aqUoZJH8Q0JaurZlDXV7VUQqQF3/b3tPFThSQI 35OXt0tX0K0uzGP3M/ec5xAqOvdc3mMG68UF5K5AfWBvHjmKXTjfZsFj7LVsJEvz L9bq/1zZ8rF1AEKH0gV1y+hNB2nOCyaTBJiJAXUt8Na2REGdJ5TO6tdfs/6PIxoj Fhq5Dhu4vHs00jR03FIP07tV75Q95vSvW4LOry4mdX/4Qev2dXgS8VM+oyaPTbbJ a+QbIGkIK+LAUjPYAKkwF/cjCxKayGm5BVU8MOPZhdch+HjUEVz/a/DJwgIaHAGK 0MkO6lVOlvmb5H38gTeSg0Q9oktEdDFDveAcf8XzquBSpCF6X4eKFz63M4ZxzHEZ yYoWStqzSloHdwCM9AbwKrT/ONgHiAzTr/xFP3NU9qfpaChVMwNpQJb24ScelscG MmQzii4WTk3qJ77iSint =JNN4 -----END PGP SIGNATURE----- --=-=-=-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: balbi@kernel.org (Felipe Balbi) Date: Thu, 08 Sep 2016 14:00:13 +0300 Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: <4934737.egJZVdaLZs@wuerfel> References: <2733202.7lpFC7RnDm@wuerfel> <87lgz2iv6d.fsf@linux.intel.com> <4934737.egJZVdaLZs@wuerfel> Message-ID: <87d1keirlu.fsf@linux.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, Arnd Bergmann writes: > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: >> >> > If we do that, we have to put child devices of the dwc3 devices into >> >> > the platform glue, and it also breaks those dwc3 devices that don't >> >> > have a parent driver. >> >> >> >> Well, this is easy to fix: >> >> >> >> if (dwc->dev->parent) { >> >> dwc->sysdev = dwc->dev->parent; >> >> } else { >> >> dev_info(dwc->dev, "Please provide a glue layer!\n"); >> >> dwc->sysdev = dwc->dev; >> >> } >> > >> > I don't understand. Do you mean we should have an extra level of >> > stacking and splitting "static struct platform_driver dwc3_driver" >> > in two so instead of >> > >> > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) >> > >> > we do this? >> > >> > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" (usb_bus.dev) >> >> no >> >> If we have a parent device, use that as sysdev, otherwise use self as >> sysdev. > > But there is often a parent device in DT, as the xhci device is > attached to some internal bus that gets turned into a platform_device > as well, so checking whether there is a parent will get the wrong > device node. oh, that makes things more interesting :-s >> > 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 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? 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; + } /* * Set it to coherent_dma_mask by default if the architecture -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 800 bytes Desc: not available URL: