Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754008AbcKOTI2 (ORCPT ); Tue, 15 Nov 2016 14:08:28 -0500 Received: from us01smtprelay-2.synopsys.com ([198.182.47.9]:60817 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751599AbcKOTI0 (ORCPT ); Tue, 15 Nov 2016 14:08:26 -0500 Subject: Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support To: Christian Lamparter , John Youn References: <1546251.ssrfyj8Loe@debian64> <3741942.Du5DzKuQeQ@debian64> From: 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 , Felipe Balbi Message-ID: <8235c9d5-891b-6f75-49a0-c8820c3b139f@synopsys.com> Date: Tue, 15 Nov 2016 11:08:23 -0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.9.139.49] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7821 Lines: 185 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 Canyonlands 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 favor >>>>> 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 >>>> "snps,dwc2" for INCR16 compatibility. >>> >>> 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? > > Yes, that's what I meant. I'll send a change for this shortly. > >> >> 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 dts >> to make them work with the changes I made: >> >> "You can't remove the old drivers as they are needed to work with >> 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. > > 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. > >> >> Of course, it would be great to hear from Rob Herring and/or Mark Rutland >> about this case. >> >> Regards, >> Christian >> >> [0] >> [1] >> [2] >> >> >>>> >>>> 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 Canyonlands 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/Documentation/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 Amlogic Meson8b SoCs; >>>> - "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic S905 SoCs; >>>> + - "amcc,dwc-otg": The DWC2 USB controller instance in AMCC Canyonlands 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[] = { >>>> { .compatible = "samsung,s3c6400-hsotg", .data = NULL}, >>>> { .compatible = "amlogic,meson8b-usb", .data = ¶ms_amlogic }, >>>> { .compatible = "amlogic,meson-gxbb-usb", .data = ¶ms_amlogic }, >>>> + { .compatible = "amcc,dwc-otg", .data = NULL }, >>>> {}, >>>> }; >>>> MODULE_DEVICE_TABLE(of, dwc2_of_match_table); >>>> > > For dwc2 part: > > Acked-by: John Youn > 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. Regards, John