Re: [PATCH v2 14/17] phy: qcom-qusb2: Set vbus sw-override signal in device mode

[Jack Pham]

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 <mgautam@xxxxxxxxxxxxxx>
> ---
>  
> +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