Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965004AbcKPJZp (ORCPT ); Wed, 16 Nov 2016 04:25:45 -0500 Received: from mga06.intel.com ([134.134.136.31]:31600 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753140AbcKPJZg (ORCPT ); Wed, 16 Nov 2016 04:25:36 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,647,1473145200"; d="asc'?scan'208";a="4928502" From: Felipe Balbi To: John Youn , Christian Lamparter , John Youn Cc: "linux-kernel\@vger.kernel.org" , "devicetree\@vger.kernel.org" , "linux-usb\@vger.kernel.org" , "linuxppc-dev\@lists.ozlabs.org" , Mark Rutland , Rob Herring , Greg Kroah-Hartman Subject: Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support In-Reply-To: <8235c9d5-891b-6f75-49a0-c8820c3b139f@synopsys.com> References: <1546251.ssrfyj8Loe@debian64> <3741942.Du5DzKuQeQ@debian64> <8235c9d5-891b-6f75-49a0-c8820c3b139f@synopsys.com> Date: Wed, 16 Nov 2016 11:24:54 +0200 Message-ID: <8737irvkt5.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-Length: 9132 Lines: 231 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi,=20 John Youn writes: > On 11/14/2016 3:00 PM, John Youn wrote: >> On 11/11/2016 3:12 PM, Christian Lamparter wrote: >>> On Friday, November 11, 2016 2:20:42 PM CET John Youn wrote: >>>> On 11/11/2016 2:05 PM, Christian Lamparter wrote: >>>>> On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote: >>>>>> On 11/11/2016 12:59 PM, Christian Lamparter wrote: >>>>>>> This patch adds support for the "amcc,usb-otg" device >>>>>>> which is found in the PowerPC Canyonlands' dts. >>>>>>> >>>>>>> The device definition was added by: >>>>>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonl= ands board")' >>>>>>> but without any driver support as the dwc2 driver wasn't >>>>>>> available at that time. >>>>>>> >>>>>>> Note: The system can't use the generic "snps,dwc2" compatible >>>>>>> because of the special ahbcfg configuration. The default >>>>>>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang >>>>>>> when the USB and SATA is used concurrently. >>>>>> >>>>>> I don't want to add any more of these param structures to the driver >>>>>> unless really necessary. We're trying to remove usage of them in fav= or >>>>>> of using auto-detected defaults and device properties to override >>>>>> them. >>>>> Ok, thanks. I think that would work. I've attached an updated patch. >>>>> Can it be applied/queued now? Or do you want me to resent it later? >>>>> >>>>>> The AHB Burst is actually one of the ones we were going to do next >>>>>> because our platform also doesn't work well with INCR4. In fact I'm >>>>>> thinking of making the default INCR. >>>>> Is that actually possible to change the default still? This would >>>>> require to re-evaluate all existing archs/platforms that use=20 >>>>> "snps,dwc2" for INCR16 compatibility.=20 >>>> >>>> INCR, not INCR16, but you're right, so we may not change it even >>>> though though INCR is usually the right choice over INCR4. >>> What about making a device-tree property? >>=20 >> Yes, that's what I meant. I'll send a change for this shortly. >>=20 >>> >>> Recommended properties: >>> - g-ahb-bursts : specifies the ahb bursts length, should be one of >>> "single", "INCRx", "INCR4", "INCR8", or "INCR16". If not specified >>> the safer but inefficient "INCR4" is used. The optimal setting is >>> "INCRx". >>> >>> Would this work? If so, I can make a patch over the weekend. >>>> Anyways, with the binding, can't you just set the compatible string to >>>> snps,dwc2? >>> >>> Ah, let me explain. I had a discussion with Mark Rutland and Rob Herring >>> a while back about device-tree bindings. >>> >>> They made it very clear to me, that they don't want any generic "catch = all >>> compatible" strings: >>> >>> "Bindings should be for hardware (either specific device models, or for >>> classes), and not for Linux drivers. The latter is subject to arbitrary >>> changes while the former is not, as old hardware continues to exist and >>> does not change while drivers get completely reworked." [0] >>> >>> Furthermore, this is an existing binding in kernel's canyonlands.dts [1] >>> and this binding can't be easily changed. Rob Herring explained this in >>> the context of the "basic-mmio-gpio" patch [2] when I was editing the d= ts >>> to make them work with the changes I made: >>> >>> "You can't remove the old drivers as they are needed to work with=20 >>> old dtbs, so there is no gain. >>> >>> You would need to match on existing compatibles such as >>> moxa,moxart-gpio and provide a match data struct that has all the info >>> you are adding here (e.g. data register offset). Then additionally you >>> could add "basic-mmio-gpio" (I would drop "basic" part) and the >>> additional data associated with it. But it has to be new properties, >>> not changing properties. Changing the reg values doesn't work." >>> >>> So, for this to work with the existing canyonlands.dts, I need to have >>> the "amcc,dwc-otg" compatible string. >>=20 >> Ok, if that's the case. But still a bit confused as to what driver was >> working with it before since the binding was not defined for dwc2. >>=20 >>> >>> Of course, it would be great to hear from Rob Herring and/or Mark Rutla= nd >>> about this case. >>> >>> Regards, >>> Christian >>> >>> [0] >>> [1] >>> [2] >>> >>>=20=20 >>>>> >>>>> From what I can tell based would be: >>>>> bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx, >>>>> stratix10, meson-gxbb, rt3050 and some Altera FPGAs. >>>>> >>>>>> If that's all you need then a devicetree binding should be enough >>>>>> right? >>>>> Yes. The device is working fine so far. >>>>> >>>>> Regards, >>>>> Christian >>>>> >>>>> --- >>>>> From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001 >>>>> From: Christian Lamparter >>>>> Date: Sun, 6 Nov 2016 00:39:24 +0100 >>>>> Subject: [PATCH] usb: dwc2: add amcc,dwc-otg support >>>>> >>>>> This patch adds support for the "amcc,usb-otg" device >>>>> which is found in the PowerPC Canyonlands' dts. >>>>> >>>>> The device definition was added by: >>>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlan= ds board")' >>>>> but without any driver support as the dwc2 driver wasn't >>>>> available at that time. >>>>> >>>>> Note: The system can't use the generic "snps,dwc2" compatible >>>>> because of the special ahbcfg configuration. The default >>>>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang >>>>> when the USB and SATA is used concurrently. >>>>> >>>>> Cc: Felipe Balbi >>>>> Cc: John Youn >>>>> Signed-off-by: Christian Lamparter >>>>> --- >>>>> v1->v2: >>>>> - moved definitons to params.c >>>>> - removed dma_enable / host_dma parameter >>>>> - added dma_desc_fs_enable parameter >>>>> v2->v3: >>>>> - removed parameters >>>>> >>>>> Please queue this patch until GAHBCFG_HBSTLEN_INCR16 is the default >>>>> for ahbcfg. >>>>> --- >>>>> Documentation/devicetree/bindings/usb/dwc2.txt | 1 + >>>>> drivers/usb/dwc2/params.c | 1 + >>>>> 2 files changed, 2 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documen= tation/devicetree/bindings/usb/dwc2.txt >>>>> index 10a2a4b..6ccfe85 100644 >>>>> --- a/Documentation/devicetree/bindings/usb/dwc2.txt >>>>> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt >>>>> @@ -12,6 +12,7 @@ Required properties: >>>>> - "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq = XRX SoCs; >>>>> - "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlog= ic Meson8b SoCs; >>>>> - "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Am= logic S905 SoCs; >>>>> + - "amcc,dwc-otg": The DWC2 USB controller instance in AMCC Canyonl= ands 460EX SoCs; >>>>> - snps,dwc2: A generic DWC2 USB controller with default parameters. >>>>> - reg : Should contain 1 register range (address and length) >>>>> - interrupts : Should contain 1 interrupt >>>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c >>>>> index 64d5c66..9506ab0 100644 >>>>> --- a/drivers/usb/dwc2/params.c >>>>> +++ b/drivers/usb/dwc2/params.c >>>>> @@ -239,6 +239,7 @@ const struct of_device_id dwc2_of_match_table[] = =3D { >>>>> { .compatible =3D "samsung,s3c6400-hsotg", .data =3D NULL}, >>>>> { .compatible =3D "amlogic,meson8b-usb", .data =3D ¶ms_amlogic = }, >>>>> { .compatible =3D "amlogic,meson-gxbb-usb", .data =3D ¶ms_amlog= ic }, >>>>> + { .compatible =3D "amcc,dwc-otg", .data =3D NULL }, >>>>> {}, >>>>> }; >>>>> MODULE_DEVICE_TABLE(of, dwc2_of_match_table); >>>>> >>=20 >> For dwc2 part: >>=20 >> Acked-by: John Youn >>=20 > > Hi Felipe, > > Can you drop this from your testing/next? > > I meant for the 2nd version to be applied, without the params > structure. > > I can send you a clean version to apply later today. done =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlgsJeYACgkQzL64meEa mQavAQ//VAVPIuKVeRtFEVyKsF3gfM5Ar+67Qbk7HXYH9LBWNXkKx91/lIlISWMV MHs7J+xyVeiQ4ioPXTwdzIaX+w2yO3WjrGRHmHDAwHgA3HDCvtnUlLYaOyRAv4PD XwPrUnodM/k+07BtAYYAssqnJJL8VXxD3GJYMKbHmf4LAHu0q+TtyrjJyMAdhFKE PLqjQUx3rk5J8ubuzxbWGjVUaFqiPqcYTaql4ydIac1nQcrSmR2+xM+1RE105Vn3 FPhbGJKkld46seMn7kiKCU1s8Zz4r+BttJPO7LBR7Qomk6GaKV81/FcCphfY3Wpf iZj1el556aMejkGlWpFUczY0dBvFavhZeLa7oLu5SQDlBOv/81EGiA77fZ/Z3qxG 0DwupQnkTmE9B491/qe8vUTFV2Kdip+tRzL7TiXgbih97yHns5SIClMFmI1+qo6R CQBqcYm75+2qEZMut9liPlOHHwwZsW8j3lNjkWx8jBQL1nGB1f/pEJD1VwOgbdDW QFBZiRZQoRZXI1cJwtOb7Sye3Pbgk/UjqYoyiNe2UgBXpq4UFr4CMnI2h/pvU+ba 7kV7XJQ+jXuDe9xRNB8uJW5/LeaRF/72IkotcZFlTiBGZR8T/+3q2v/TbFGZ4zrA sEZCaXQN6yE+CBRRsrDh0e87YF/RmVsIY4WWE5dBGLmw5W80wz4= =ecvz -----END PGP SIGNATURE----- --=-=-=--