From patchwork Fri Mar 16 14:32:29 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roger Quadros X-Patchwork-Id: 10287603 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4F6AB60386 for ; Fri, 16 Mar 2018 14:33:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3D1AF28E68 for ; Fri, 16 Mar 2018 14:33:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 315EE28E92; Fri, 16 Mar 2018 14:33:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id F25F528E68 for ; Fri, 16 Mar 2018 14:33:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=YmkxrUviLdkGiMLRosKTha7kmxLtmgZ7nfrzOyj04pM=; b=cXCDEaGDoaXMI3 ze1JxbxhapaC1MabMs0oUGNjGmLFcgZKN83HjKaZI5Sff4MuvFQcgc/dNIe+DRiRb3KID6ohGb1k3 dRmUVlF6gYJHMoSeF0AqxGDvfobUzOKM6VeQ4Yw4QmaDcs/KrSDU3CcnXBZJ+2dwRtHHuEmyTOgYl Awow4KJtoSZL5ELCru2XnwGmZFDpHgkp2Xugn6Ihgo5nbw85M82S4xskTf4LHV+WpLYHVXQqVhdvN 3uT7vekXORvv8pyIFXn3kfYOuTOi6zbD0zZNNLcF6gaSw7QoL00CGvGc3suj9LxtuCt30yR9/Mmpw IVM0RDLV1wcpWzFJQhtw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1ewqQK-0001m1-4L; Fri, 16 Mar 2018 14:33:36 +0000 Received: from lelnx194.ext.ti.com ([198.47.27.80]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1ewqPp-0001Ph-NG; Fri, 16 Mar 2018 14:33:09 +0000 Received: from dlelxv90.itg.ti.com ([172.17.2.17]) by lelnx194.ext.ti.com (8.15.1/8.15.1) with ESMTP id w2GEWasl029121; Fri, 16 Mar 2018 09:32:36 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ti.com; s=ti-com-17Q1; t=1521210756; bh=ZJiodkS0nm2NAO62LZsNU+tY9O8tpnHY+EYmiqVYrfk=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=FOO6EwVez0L7sBvF0QzwfCeJSwWs+cvf+bXIFkf56nb7+mrIhjUm4LGQZpTi/OYtk mEP7pIDd8tRfnOG9JTKWhiqRTorIQkwTPwDYPfXPKFsqoJe9Q8bJvdbeonHGANA7Qw lBtjHAcWNrdXahcIQ3hT//sPyMhk0jftEjkKLbLI= Received: from DLEE104.ent.ti.com (dlee104.ent.ti.com [157.170.170.34]) by dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id w2GEWa6e032460; Fri, 16 Mar 2018 09:32:36 -0500 Received: from DLEE113.ent.ti.com (157.170.170.24) by DLEE104.ent.ti.com (157.170.170.34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1261.35; Fri, 16 Mar 2018 09:32:35 -0500 Received: from dlep33.itg.ti.com (157.170.170.75) by DLEE113.ent.ti.com (157.170.170.24) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_RSA_WITH_AES_256_CBC_SHA) id 15.1.1261.35 via Frontend Transport; Fri, 16 Mar 2018 09:32:35 -0500 Received: from [192.168.2.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep33.itg.ti.com (8.14.3/8.13.8) with ESMTP id w2GEWTNv003438; Fri, 16 Mar 2018 09:32:30 -0500 Subject: Re: [PATCH usb-next v10 3/8] usb: core: add a wrapper for the USB PHYs on the HCD To: Martin Blumenstingl , , , , , References: <20180218184504.3331-1-martin.blumenstingl@googlemail.com> <20180218184504.3331-4-martin.blumenstingl@googlemail.com> From: Roger Quadros Message-ID: <7f511a6f-f3a2-2002-f601-5ce1742f4539@ti.com> Date: Fri, 16 Mar 2018 16:32:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180218184504.3331-4-martin.blumenstingl@googlemail.com> Content-Language: en-GB X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20180316_073306_019739_84445A60 X-CRM114-Status: GOOD ( 37.80 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, Chunfeng Yun , "Gerlach, Dave" , Peter.Chen@nxp.com, Keerthy , narmstrong@baylibre.com, yixun.lan@amlogic.com, robh+dt@kernel.org, jonathanh@nvidia.com, linux@prisktech.co.nz, matthias.bgg@gmail.com, thierry.reding@gmail.com, linux-mediatek@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-tegra@vger.kernel.org, stern@rowland.harvard.edu, KISHON VIJAY ABRAHAM , linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+patchwork-linux-mediatek=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP +some TI folks Hi Martin, On 18/02/18 20:44, Martin Blumenstingl wrote: > Many SoC platforms have separate devices for the USB PHY which are > registered through the generic PHY framework. These PHYs have to be > enabled to make the USB controller actually work. They also have to be > disabled again on shutdown/suspend. > > Currently (at least) the following HCI platform drivers are using custom > code to obtain all PHYs via devicetree for the roothub/controller and > disable/enable them when required: > - ehci-platform.c has ehci_platform_power_{on,off} > - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off} > - ohci-platform.c has ohci_platform_power_{on,off} > > With this new wrapper the USB PHYs can be specified directly in the > USB controller's devicetree node (just like on the drivers listed > above). This allows SoCs like the Amlogic Meson GXL family to operate > correctly once this is wired up correctly. These SoCs use a dwc3 > controller and require all USB PHYs to be initialized (if one of the USB > PHYs it not initialized then none of USB port works at all). > > Signed-off-by: Martin Blumenstingl > Tested-by: Yixun Lan > Cc: Neil Armstrong > Cc: Chunfeng Yun This patch is breaking low power cases on TI SoCs when USB is in host mode. I'll explain why below. > --- > drivers/usb/core/Makefile | 2 +- > drivers/usb/core/phy.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++ > drivers/usb/core/phy.h | 7 ++ > 3 files changed, 166 insertions(+), 1 deletion(-) > create mode 100644 drivers/usb/core/phy.c > create mode 100644 drivers/usb/core/phy.h > > diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile > index 92c9cefb4317..18e874b0441e 100644 > --- a/drivers/usb/core/Makefile > +++ b/drivers/usb/core/Makefile > @@ -6,7 +6,7 @@ > usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o > usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o > usbcore-y += devio.o notify.o generic.o quirks.o devices.o > -usbcore-y += port.o > +usbcore-y += phy.o port.o > > usbcore-$(CONFIG_OF) += of.o > usbcore-$(CONFIG_USB_PCI) += hcd-pci.o > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c > new file mode 100644 > index 000000000000..09b7c43c0ea4 > --- /dev/null > +++ b/drivers/usb/core/phy.c > @@ -0,0 +1,158 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * A wrapper for multiple PHYs which passes all phy_* function calls to > + * multiple (actual) PHY devices. This is comes handy when initializing > + * all PHYs on a HCD and to keep them all in the same state. > + * > + * Copyright (C) 2018 Martin Blumenstingl > + */ > + > +#include > +#include > +#include > +#include > + > +#include "phy.h" > + > +struct usb_phy_roothub { > + struct phy *phy; > + struct list_head list; > +}; > + > +static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev) > +{ > + struct usb_phy_roothub *roothub_entry; > + > + roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL); > + if (!roothub_entry) > + return ERR_PTR(-ENOMEM); > + > + INIT_LIST_HEAD(&roothub_entry->list); > + > + return roothub_entry; > +} > + > +static int usb_phy_roothub_add_phy(struct device *dev, int index, > + struct list_head *list) > +{ > + struct usb_phy_roothub *roothub_entry; > + struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > + > + if (IS_ERR_OR_NULL(phy)) { > + if (!phy || PTR_ERR(phy) == -ENODEV) > + return 0; > + else > + return PTR_ERR(phy); > + } > + > + roothub_entry = usb_phy_roothub_alloc(dev); > + if (IS_ERR(roothub_entry)) > + return PTR_ERR(roothub_entry); > + > + roothub_entry->phy = phy; > + > + list_add_tail(&roothub_entry->list, list); > + > + return 0; > +} > + > +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev) > +{ > + struct usb_phy_roothub *phy_roothub; > + struct usb_phy_roothub *roothub_entry; > + struct list_head *head; > + int i, num_phys, err; > + > + num_phys = of_count_phandle_with_args(dev->of_node, "phys", > + "#phy-cells"); > + if (num_phys <= 0) > + return NULL; > + > + phy_roothub = usb_phy_roothub_alloc(dev); > + if (IS_ERR(phy_roothub)) > + return phy_roothub; > + > + for (i = 0; i < num_phys; i++) { > + err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list); > + if (err) > + goto err_out; > + } > + > + head = &phy_roothub->list; > + > + list_for_each_entry(roothub_entry, head, list) { > + err = phy_init(roothub_entry->phy); The phy_init() function actually enables the PHY clocks. It should be moved to the usb_phy_roothub_exit() routine just before calling phy_power_on(). > + if (err) > + goto err_exit_phys; > + } > + > + return phy_roothub; > + > +err_exit_phys: > + list_for_each_entry_continue_reverse(roothub_entry, head, list) > + phy_exit(roothub_entry->phy); > + > +err_out: > + return ERR_PTR(err); > +} > +EXPORT_SYMBOL_GPL(usb_phy_roothub_init); > + > +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub) > +{ > + struct usb_phy_roothub *roothub_entry; > + struct list_head *head; > + int err, ret = 0; > + > + if (!phy_roothub) > + return 0; > + > + head = &phy_roothub->list; > + > + list_for_each_entry(roothub_entry, head, list) { > + err = phy_exit(roothub_entry->phy); > + if (err) > + ret = ret; > + } phy_exit() should be moved to usb_phy_roothub_poweroff() just after calling phy_power_off(). With that there is nothing else being done here. Shouldn't we be doing the equivalent of usb_phy_roothub_del_phy() and usb_phy_roothub_free() here? > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(usb_phy_roothub_exit); > + > +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub) > +{ > + struct usb_phy_roothub *roothub_entry; > + struct list_head *head; > + int err; > + > + if (!phy_roothub) > + return 0; > + > + head = &phy_roothub->list; > + > + list_for_each_entry(roothub_entry, head, list) { > + err = phy_power_on(roothub_entry->phy); > + if (err) > + goto err_out; > + } > + > + return 0; > + > +err_out: > + list_for_each_entry_continue_reverse(roothub_entry, head, list) > + phy_power_off(roothub_entry->phy); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(usb_phy_roothub_power_on); > + > +void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub) > +{ > + struct usb_phy_roothub *roothub_entry; > + > + if (!phy_roothub) > + return; > + > + list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list) > + phy_power_off(roothub_entry->phy); Not doing the phy_exit() here leaves the clocks enabled on our SoC and we're no longer able to reach low power states on system suspend. > +} > +EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off); > diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h > new file mode 100644 > index 000000000000..6fde59bfbff8 > --- /dev/null > +++ b/drivers/usb/core/phy.h > @@ -0,0 +1,7 @@ > +struct usb_phy_roothub; > + > +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev); > +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub); > + > +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub); > +void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub); > The following patch fixes the issue for me. Let me know what you think and I can post it officially. diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c index 09b7c43..23232d3 100644 --- a/drivers/usb/core/phy.c +++ b/drivers/usb/core/phy.c @@ -59,8 +59,6 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index, struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev) { struct usb_phy_roothub *phy_roothub; - struct usb_phy_roothub *roothub_entry; - struct list_head *head; int i, num_phys, err; num_phys = of_count_phandle_with_args(dev->of_node, "phys", @@ -75,25 +73,10 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev) for (i = 0; i < num_phys; i++) { err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list); if (err) - goto err_out; - } - - head = &phy_roothub->list; - - list_for_each_entry(roothub_entry, head, list) { - err = phy_init(roothub_entry->phy); - if (err) - goto err_exit_phys; + return ERR_PTR(err); } return phy_roothub; - -err_exit_phys: - list_for_each_entry_continue_reverse(roothub_entry, head, list) - phy_exit(roothub_entry->phy); - -err_out: - return ERR_PTR(err); } EXPORT_SYMBOL_GPL(usb_phy_roothub_init); @@ -106,13 +89,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub) if (!phy_roothub) return 0; - head = &phy_roothub->list; - - list_for_each_entry(roothub_entry, head, list) { - err = phy_exit(roothub_entry->phy); - if (err) - ret = ret; - } + /* TODO: usb_phy_roothub_del_phy */ + /* TODO: usb_phy_roothub_free */ return ret; } @@ -130,16 +108,23 @@ int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub) head = &phy_roothub->list; list_for_each_entry(roothub_entry, head, list) { - err = phy_power_on(roothub_entry->phy); + err = phy_init(roothub_entry->phy); if (err) goto err_out; + err = phy_power_on(roothub_entry->phy); + if (err) { + phy_exit(roothub_entry->phy); + goto err_out; + } } return 0; err_out: - list_for_each_entry_continue_reverse(roothub_entry, head, list) + list_for_each_entry_continue_reverse(roothub_entry, head, list) { phy_power_off(roothub_entry->phy); + phy_exit(roothub_entry->phy); + } return err; } @@ -152,7 +137,9 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub) if (!phy_roothub) return; - list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list) + list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list) { phy_power_off(roothub_entry->phy); + phy_exit(roothub_entry->phy); + } } EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);