Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752192AbdI0R5t (ORCPT ); Wed, 27 Sep 2017 13:57:49 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:59396 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751408AbdI0R5r (ORCPT ); Wed, 27 Sep 2017 13:57:47 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 884496044E Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=jackp@codeaurora.org Date: Wed, 27 Sep 2017 10:57:41 -0700 From: Jack Pham To: Manu Gautam Cc: Kishon Vijay Abraham I , Felipe Balbi , linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, Vivek Gautam , Viresh Kumar , Krzysztof Kozlowski , "open list:GENERIC PHY FRAMEWORK" Subject: Re: [PATCH v2 14/17] phy: qcom-qusb2: Set vbus sw-override signal in device mode Message-ID: <20170927175741.GA12812@usblab-sd-06.qualcomm.com> References: <1506502753-27408-1-git-send-email-mgautam@codeaurora.org> <1506502753-27408-15-git-send-email-mgautam@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1506502753-27408-15-git-send-email-mgautam@codeaurora.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2129 Lines: 51 Hi Manu, On Wed, Sep 27, 2017 at 02:29:10PM +0530, Manu Gautam wrote: > VBUS signal coming from PHY must be asserted in device for > controller to start operation or assert pull-up. For some > platforms where VBUS line is not connected to PHY there is > HS_PHY_CTRL register in QSCRATCH wrapper that can be used > by software to override VBUS signal going to controller. > > Signed-off-by: Manu Gautam > --- > > +static int qusb2_phy_set_mode(struct phy *phy, enum phy_mode mode) > +{ > + struct qusb2_phy *qphy = phy_get_drvdata(phy); > + > + qphy->mode = mode; > + > + /* Update VBUS override in qscratch register */ > + if (qphy->qscratch_base) { > + if (mode == PHY_MODE_USB_DEVICE) > + qusb2_setbits(qphy->qscratch_base, QSCRATCH_HS_PHY_CTRL, > + UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL); > + else > + qusb2_clrbits(qphy->qscratch_base, QSCRATCH_HS_PHY_CTRL, > + UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL); Wouldn't this be better off handled in the controller glue driver? Two reasons I think this patch is unattractive: - qscratch_base is part of the controller's register space. Your later patch 16/17 ("phy: qcom-qmp: Override lane0_power_present signal in device mode") does a similar thing and hence both drivers have to ioremap() the same register resource while at the same time avoiding request_mem_region() (called by devm_ioremap_resource) to allow it to be mapped in both places. - VBUS override bit becomes asserted simply because the mode is changed to device mode but this is irrespective of the actual VBUS state. This could break some test setups which perform a logical disconnect by switching off/on VBUS while leaving data lines connected. Controller would go merrily along thinking it is still attached to the host. Instead maybe this could be tied to EXTCON_USB handling in the glue driver; though it would need to be an additional notifier on top of dwc3/drd.c which already handles extcon for host/device mode. Jack -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project