| Message ID | 1506502753-27408-15-git-send-email-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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@codeaurora.org> > --- > > +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
On Wed, Sep 27, 2017 at 10:57:41AM -0700, Jack Pham wrote: > 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@codeaurora.org> > > --- > > > > +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. That is to say, we'd probably need to split out dwc3-qcom from dwc3-of-simple.c into its own driver (again) in order to add this. Jack
Hi Jack, On 9/28/2017 12:46 AM, Jack Pham wrote: > On Wed, Sep 27, 2017 at 10:57:41AM -0700, Jack Pham wrote: >> 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@codeaurora.org> >>> --- >>> >>> +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. Right. There is one more reason why qusb2 driver needs qscratch: - During runtime suspend, it has to check linestate to set correct polarity for dp/dm wakeup interrupt in order to detect disconnect/resume ion LS and FS/HS modes. >> - 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. Yes, dwc3/drd.c currently deals with only EXTCON_USB_HOST. So, for platforms where role swap happens using only Vbus or single GPIO this should take care of. > That is to say, we'd probably need to split out dwc3-qcom from > dwc3-of-simple.c into its own driver (again) in order to add this. > > Jack However, I agree that more appropriate place for lane0-pwr-present and vbus override update is dwc3 glue driver. Since we don't have one right now, IMO once we have dwc3-qcom driver in place, this handling can be moved from PHY to glue driver. Until then we can use this approach to get USB device mode working on qcom platforms which are using dwc3-of-simple.c e.g. sdm820 dragonboard.
Hi Manu, On Thu, Sep 28, 2017 at 09:30:38AM +0530, Manu Gautam wrote: > On 9/28/2017 12:46 AM, Jack Pham wrote: > > On Wed, Sep 27, 2017 at 10:57:41AM -0700, Jack Pham wrote: > >> 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@codeaurora.org> > >>> --- > >>> > >>> +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. > > Right. There is one more reason why qusb2 driver needs qscratch: > - During runtime suspend, it has to check linestate to set correct polarity for dp/dm > wakeup interrupt in order to detect disconnect/resume ion LS and FS/HS modes. Ugh, oh yeah. The way I understand we did it in our downstream driver is still to have the controller driver read the linestate but then pass the information via additional set_mode() flags which the PHY driver could use to correctly arm the interrupt trigger polarity. An alternative would be to access a couple of the debug QUSB2PHY registers that also provide a reading of the current UTMI linestate. The HPG mentions them vaguely, and I can't remember if we tested that interface or not. Assuming it works, would that be preferable to reading a non-PHY register here? > >> - 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. > > Yes, dwc3/drd.c currently deals with only EXTCON_USB_HOST. So, for platforms > where role swap happens using only Vbus or single GPIO this should take care of. > > > > That is to say, we'd probably need to split out dwc3-qcom from > > dwc3-of-simple.c into its own driver (again) in order to add this. > > > > Jack > > However, I agree that more appropriate place for lane0-pwr-present and > vbus override update is dwc3 glue driver. Since we don't have one right now, > > IMO once we have dwc3-qcom driver in place, this handling can be moved from > PHY to glue driver. Until then we can use this approach to get USB device mode > working on qcom platforms which are using dwc3-of-simple.c e.g. sdm820 > dragonboard. Could that be done in this series too? IMO better to get it right in one shot. Is this aimed for 4.15? Jack
diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c index bda1f4c..0e9d88b 100644 --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c @@ -68,6 +68,11 @@ #define QUSB2PHY_IMP_CTRL2 0x224 #define QUSB2PHY_CHG_CTRL2 0x23c +/* QSCRATCH register bits */ +#define QSCRATCH_HS_PHY_CTRL 0x10 +#define UTMI_OTG_VBUS_VALID BIT(20) +#define SW_SESSVLD_SEL BIT(28) + struct qusb2_phy_init_tbl { unsigned int offset; unsigned int val; @@ -211,6 +216,7 @@ struct qusb2_phy_cfg { * * @phy: generic phy * @base: iomapped memory space for qubs2 phy + * @qscratch_base: iomapped memory space for qscratch region * * @cfg_ahb_clk: AHB2PHY interface clock * @ref_clk: phy reference clock @@ -223,10 +229,12 @@ struct qusb2_phy_cfg { * * @cfg: phy config data * @has_se_clk_scheme: indicate if PHY has single-ended ref clock scheme + * @mode: indicate current PHY mode of operation e.g. HOST or DEVICE */ struct qusb2_phy { struct phy *phy; void __iomem *base; + void __iomem *qscratch_base; struct clk *cfg_ahb_clk; struct clk *ref_clk; @@ -239,6 +247,7 @@ struct qusb2_phy { const struct qusb2_phy_cfg *cfg; bool has_se_clk_scheme; + enum phy_mode mode; }; static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val) @@ -307,6 +316,25 @@ static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy) qusb2_setbits(qphy->base, QUSB2PHY_PORT_TUNE2, val[0] << 0x4); } +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); + } + + return 0; +} + static int qusb2_phy_init(struct phy *phy) { struct qusb2_phy *qphy = phy_get_drvdata(phy); @@ -473,6 +501,7 @@ static int qusb2_phy_exit(struct phy *phy) static const struct phy_ops qusb2_phy_gen_ops = { .init = qusb2_phy_init, .exit = qusb2_phy_exit, + .set_mode = qusb2_phy_set_mode, .owner = THIS_MODULE, }; @@ -507,6 +536,16 @@ static int qusb2_phy_probe(struct platform_device *pdev) if (IS_ERR(qphy->base)) return PTR_ERR(qphy->base); + /* Check if platform uses qscratch wrapper */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qscratch"); + if (res) { + /* Can't request region as used by other phy and glue drivers */ + qphy->qscratch_base = devm_ioremap(dev, res->start, + resource_size(res)); + if (IS_ERR(qphy->qscratch_base)) + return PTR_ERR(qphy->qscratch_base); + } + qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb"); if (IS_ERR(qphy->cfg_ahb_clk)) { ret = PTR_ERR(qphy->cfg_ahb_clk);
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@codeaurora.org> --- drivers/phy/qualcomm/phy-qcom-qusb2.c | 39 +++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)