Re: [PATCH 16/21] usb: chipidea: msm: Restore wrapper settings after reset

Peter Chen
2016-06-30T09:01:43Z

On Wed, Jun 29, 2016 at 12:13:45PM -0700, Stephen Boyd wrote:
> Quoting Peter Chen (2016-06-29 01:26:48)
> > On Sun, Jun 26, 2016 at 12:28:33AM -0700, Stephen Boyd wrote:
> > > When the RESET bit is set in the USBCMD register it resets quite
> > > @@ -21,11 +23,22 @@
> > > #define HS_PHY_SEC_CTRL 0x0278
> > > # define HS_PHY_DIG_CLAMP_N BIT(16)
> > >
> > > +#define HS_PHY_GENCONFIG 0x009c
> > > +# define HS_PHY_TXFIFO_IDLE_FORCE_DIS BIT(4)
> > > +
> > > +#define HS_PHY_GENCONFIG_2 0x00a0
> > > +# define HS_PHY_SESS_VLD_CTRL_EN BIT(7)
> > > +# define HS_PHY_ULPI_TX_PKT_EN_CLR_FIX BIT(19)
> > > +
> > > +#define HSPHY_SESS_VLD_CTRL BIT(25)
> > > +
> >
> > Keep alignment please.
>
> I take it this means it should look like:
>
> #define HS_PHY_GENCONFIG
> #define HS_PHY_TXFIFO_IDLE_FORCE_DIS
>
> ?
>

Yes

> > > @@ -141,6 +172,13 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
> > > if (!base)
> > > return -ENOMEM;
> > >
> > > + ci->vbus_edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
> > > + if (IS_ERR(ci->vbus_edev)) {
> > > + if (PTR_ERR(ci->vbus_edev) != -ENODEV)
> > > + return PTR_ERR(ci->vbus_edev);
> > > + ci->vbus_edev = NULL;
> > > + }
> > > +
> >
> > Why not using ci->platdata->vbus_extcon directly?
>
> Because ci->platdata->vbus_extcon is assigned after the child platform
> driver probes, and we have no idea when that will happen from the
> ci_hdrc_msm driver probe. If we try after ci_hdrc_add_device() we'll
> race with the driver probe and only get the pointer sometimes.

ci->platdata->vbus_extcon->edev is assigned at ci_get_platdata which is
called before ci core device is created.

>
> >
> > > reset_control_assert(reset);
> > > usleep_range(10000, 12000);
> > > reset_control_deassert(reset);
> > > @@ -157,6 +195,14 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
> > > if (ret)
> > > goto err_mux;
> > >
> > > + ulpi_node = of_find_node_by_name(pdev->dev.of_node, "ulpi");
> > > + if (ulpi_node) {
> > > + phy_node = of_get_next_available_child(ulpi_node, NULL);
> > > + ci->hsic = of_device_is_compatible(phy_node, "qcom,usb-hsic-phy");
> > > + of_node_put(phy_node);
> > > + }
> > > + of_node_put(ulpi_node);
> > > +
> >
> > Just confirm with you that ci->platdata->phy_mode is not enough?
>
> Right. The phy_mode is never set to HSIC. It's always ULPI.

Ok

--

Best Regards,
Peter Chen