From patchwork Mon Dec 21 08:33:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Chen X-Patchwork-Id: 7894001 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id AE6CA9F349 for ; Mon, 21 Dec 2015 08:36:05 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B4C5720382 for ; Mon, 21 Dec 2015 08:36: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 B29A72028D for ; Mon, 21 Dec 2015 08:36: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 1aAvv1-0001m9-7P; Mon, 21 Dec 2015 08:34:11 +0000 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1aAvuy-0001iX-2u for linux-arm-kernel@lists.infradead.org; Mon, 21 Dec 2015 08:34:09 +0000 Received: by mail-lf0-x241.google.com with SMTP id y184so10689062lfc.0 for ; Mon, 21 Dec 2015 00:33:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=fdOkAAxaSgbUKKAaHybf39r+ula8o92lWSuqDx591r0=; b=SScrK5pTDSwjUYEYx+haOEFK7Bja5gGNqtuA9o5+1WRlX7ILywJrqZNpRjJI0Ip8oV xukwODP4EdW9eWiDvofKZ2Zfyee1xosC7iGkA0kd6+2Gejq/YZnYnNPFiWioCxUr9aIi +RdL4D5BbgytzLJSn7ofOiQW/HWwBR+V0L3Euh5r2TsXaHKbbv40eD8IVFBK6aEmiQf5 SqmyUS9rOCs3pSWfWaGgEH3C7E/wQ3jN6iTV9foaGeld7KunFHyeR8y9a+Lv+bdN7Rx6 JsH+NURZ4QmRZUqLS0QqEII5R2XfKS6eRdOM0wDqeiHLDasCOfBAVf3TNmh5MR/af6jc C/xQ== X-Received: by 10.25.147.209 with SMTP id v200mr5778040lfd.102.1450686825490; Mon, 21 Dec 2015 00:33:45 -0800 (PST) MIME-Version: 1.0 Received: by 10.112.181.65 with HTTP; Mon, 21 Dec 2015 00:33:25 -0800 (PST) In-Reply-To: References: From: Peter Chen Date: Mon, 21 Dec 2015 16:33:25 +0800 Message-ID: Subject: Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver To: Alan Stern X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20151221_003408_499483_D7E8869E X-CRM114-Status: GOOD ( 30.43 ) X-Spam-Score: -2.7 (--) 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: Mark Rutland , "devicetree@vger.kernel.org" , Pawel Moll , Arnd Bergmann , Greg Kroah-Hartman , Mathieu Poirier , Linux USB List , patryk@kowalczyk.ws, Felipe Balbi , Rob Herring , Peter Chen , "kernel@pengutronix.de" , Philipp Zabel , Fabio Estevam , Shawn Guo , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, 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 On Fri, Dec 18, 2015 at 11:38 PM, Alan Stern wrote: > On Fri, 18 Dec 2015, Peter Chen wrote: > >> On Fri, Dec 18, 2015 at 12:13 AM, Alan Stern wrote: > >> > It's not clear (to me, anyway) how this is meant to work. USB is a >> > completely discoverable bus: There is no way to represent devices >> > statically; they have to be discovered. But a device can't be >> > discovered unless it is functional, so if an onboard hub (or any other >> > sort of USB device) requires power, clocks, or something similar in >> > order to function, it won't be discovered. There won't be any device >> > structure to probe, and "forcing driver probe" won't accomplish >> > anything. >> > >> > It seems to me that the only way something like this could be made to >> > work is if the necessary actions are keyed off the host controller (and >> > in particular, _not_ the hub driver), presumably at the time the >> > controller is probed. >> >> The reason why I put the code at hub driver is the onboard USB device >> may be at 2nd level hub, at hub driver, we can know all devices under >> this level hub. > > Maybe. I'm not convinced. See below. > >> > With anything else, you run the risk that the >> > necessary resources won't get enabled before they are needed. >> > >> >> Sorry, I can't understand what you mean > > Suppose you have a platform driver to manage the device's resources, > and the platform driver is in loadable module. Suppose the device can > be detected even before the resources have been initialized, but it > can't work correctly. > > Then what happens when the platform driver's module doesn't get loaded > until _after_ the device has been detected and after the device failed > to initialize? > > You are right, so the platform driver is not a good choice for doing that. >> +static int hub_of_children_register(struct usb_device *hdev) >> +{ >> + struct device *dev; >> + >> + if (hdev->parent) >> + dev = &hdev->dev; >> + else >> + dev = bus_to_hcd(hdev->bus)->self.controller; >> + >> + if (!dev->of_node) >> + return 0; > > Suppose hdev->parent is not NULL (hdev isn't the root hub -- maybe it's > a 2nd-level hub). Then how did hdev->dev->of_node get set? > Yes, the USB device doesn't know device node. There are two problems which needs device tree to support, I have below solutions for them, please help to see if it is reasonable. 1. The USB device can't work without external clock, power, reset operation. At device tree, we have a node to describe external signals like clocks, gpios for all onboard devices under this controller. And this node is as phandle for host controller, see below: host controller's device node, and this API is called at usb_add_hcd, and opposite operation is called at usb_remove_hcd. This solution is almost the same with MMC power sequence solution. (see drivers/mmc/core/pwrseq.c) 2. There are MFD USB devices, which includes several interfaces under USB device, like i2c, gpios, etc. Due to lack of device tree support, USB class/device driver doesn't know which kinds of interfaces are needed for this board. This problem is hard to handle, I have a solution, but it can't cover two same devices under the same HUB ports. My solution is let USB know device node, the main idea is similar with PCIi's (see drivers/of/of_pci.c, drivers/pci/of.c), the most difficulty is find correct node for USB device after bus enumeration. Once the device is recognized, the USB core will create a USB device for it, since we know its parent device, and its parent device (eg, the host controller) has device node, and we can find all children nodes under this node, if the child's {vid, pid} is the same with {vid, pid} the device descriptor we read, we assign this node as usb device's node. At USB class/device driver, we can get the properties/phandles under this device node, and register them. At device tree, we need to describe the bus topology for this USB device. --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi @@ -108,6 +108,21 @@ default-brightness-level = <7>; status = "okay"; }; + + usbh1_pre_operation: usbh1_pre_operation { + clocks = <&clks IMX6QDL_CLK_1>, + <&clks IMX6QDL_CLK_2>, + <&clks IMX6QDL_CLK_3>, + <&clks IMX6QDL_CLK_4>, + <&clks IMX6QDL_CLK_5>, + <&clks IMX6QDL_CLK_6>; + reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>, + <&gpio4 7 GPIO_ACTIVE_LOW>, + <&gpio3 25 GPIO_ACTIVE_LOW>, + <&gpio3 27 GPIO_ACTIVE_LOW>, + <&gpio4 4 GPIO_ACTIVE_LOW>, + <&gpio4 6 GPIO_ACTIVE_LOW>; + }; }; &clks { @@ -590,6 +605,8 @@ &usbh1 { vbus-supply = <®_usb_h1_vbus>; status = "okay"; + + devices-pre-operation = <&usbh1_pre_operation> }; At code, we add one API of_usb_pre_operation to get clocks and gpios through