| Message ID | 1403110868-3924-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Wed, Jun 18, 2014 at 06:01:08PM +0100, Srinivas Kandagatla wrote: > Use case is when the phy is configured in host mode and a usb device is > attached to board before bootup. On bootup, with the existing code and > runtime pm enabled, the driver would decrement the pm usage count > without checking the current state of the phy. This pm usage count > decrement would trigger the runtime pm which than would abort the > usb enumeration which was in progress. In my case a usb stick gets > detected and then immediatly the driver goes to low power mode which is > not correct. > > log: > [ 1.631412] msm_hsusb_host 12520000.usb: EHCI Host Controller > [ 1.636556] msm_hsusb_host 12520000.usb: new USB bus registered, assigned bus number 1 > [ 1.642563] msm_hsusb_host 12520000.usb: irq 220, io mem 0x12520000 > [ 1.658197] msm_hsusb_host 12520000.usb: USB 2.0 started, EHCI 1.00 > [ 1.659473] hub 1-0:1.0: USB hub found > [ 1.663415] hub 1-0:1.0: 1 port detected > ... > [ 1.973352] usb 1-1: new high-speed USB device number 2 using msm_hsusb_host > [ 2.107707] usb-storage 1-1:1.0: USB Mass Storage device detected > [ 2.108993] scsi0 : usb-storage 1-1:1.0 > [ 2.678341] msm_otg 12520000.phy: USB in low power mode > [ 3.168977] usb 1-1: USB disconnect, device number 2 > > This issue was detected on IFC6410 board. > > This patch fixes the intial runtime pm trigger by checking the phy > state and decrementing the pm use count only when the phy state is IDLE. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/usb/phy/phy-msm-usb.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c > index 3bb559d..78cc870 100644 > --- a/drivers/usb/phy/phy-msm-usb.c > +++ b/drivers/usb/phy/phy-msm-usb.c > @@ -1229,7 +1229,9 @@ static void msm_otg_sm_work(struct work_struct *w) > motg->chg_state = USB_CHG_STATE_UNDEFINED; > motg->chg_type = USB_INVALID_CHARGER; > } > - pm_runtime_put_sync(otg->phy->dev); > + > + if (otg->phy->state == OTG_STATE_B_IDLE) > + pm_runtime_put_sync(otg->phy->dev); instead, you might as well just use autosuspend.
Hi Felipe, On 27/06/14 16:54, Felipe Balbi wrote: > Hi, > > On Wed, Jun 18, 2014 at 06:01:08PM +0100, Srinivas Kandagatla wrote: >> Use case is when the phy is configured in host mode and a usb device is >> attached to board before bootup. On bootup, with the existing code and >> runtime pm enabled, the driver would decrement the pm usage count >> without checking the current state of the phy. This pm usage count >> decrement would trigger the runtime pm which than would abort the >> usb enumeration which was in progress. In my case a usb stick gets >> detected and then immediatly the driver goes to low power mode which is >> not correct. >> >> log: >> [ 1.631412] msm_hsusb_host 12520000.usb: EHCI Host Controller >> [ 1.636556] msm_hsusb_host 12520000.usb: new USB bus registered, assigned bus number 1 >> [ 1.642563] msm_hsusb_host 12520000.usb: irq 220, io mem 0x12520000 >> [ 1.658197] msm_hsusb_host 12520000.usb: USB 2.0 started, EHCI 1.00 >> [ 1.659473] hub 1-0:1.0: USB hub found >> [ 1.663415] hub 1-0:1.0: 1 port detected >> ... >> [ 1.973352] usb 1-1: new high-speed USB device number 2 using msm_hsusb_host >> [ 2.107707] usb-storage 1-1:1.0: USB Mass Storage device detected >> [ 2.108993] scsi0 : usb-storage 1-1:1.0 >> [ 2.678341] msm_otg 12520000.phy: USB in low power mode >> [ 3.168977] usb 1-1: USB disconnect, device number 2 >> >> This issue was detected on IFC6410 board. >> >> This patch fixes the intial runtime pm trigger by checking the phy >> state and decrementing the pm use count only when the phy state is IDLE. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> drivers/usb/phy/phy-msm-usb.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c >> index 3bb559d..78cc870 100644 >> --- a/drivers/usb/phy/phy-msm-usb.c >> +++ b/drivers/usb/phy/phy-msm-usb.c >> @@ -1229,7 +1229,9 @@ static void msm_otg_sm_work(struct work_struct *w) >> motg->chg_state = USB_CHG_STATE_UNDEFINED; >> motg->chg_type = USB_INVALID_CHARGER; >> } >> - pm_runtime_put_sync(otg->phy->dev); >> + >> + if (otg->phy->state == OTG_STATE_B_IDLE) >> + pm_runtime_put_sync(otg->phy->dev); > > instead, you might as well just use autosuspend. autosuspend is a good idea and will provide a delay before rumtime suspend, however the bug which is fixed here still needs to be fixed anyway. runtime PM in this driver is not that great, the driver just increments the pm use count if the phy is configured in host or deivce mode. This patch fixes a bug in its state-machine which decrements pm use-count without checking the state. As this is a PHY driver, am not sure moving to autosuspend means we would end up just adding some static inactivity delay? Is it really going to add any value to this PHY driver? --srini > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 30, 2014 at 11:23:43AM +0100, Srinivas Kandagatla wrote: > Hi Felipe, > > On 27/06/14 16:54, Felipe Balbi wrote: > >Hi, > > > >On Wed, Jun 18, 2014 at 06:01:08PM +0100, Srinivas Kandagatla wrote: > >>Use case is when the phy is configured in host mode and a usb device is > >>attached to board before bootup. On bootup, with the existing code and > >>runtime pm enabled, the driver would decrement the pm usage count > >>without checking the current state of the phy. This pm usage count > >>decrement would trigger the runtime pm which than would abort the > >>usb enumeration which was in progress. In my case a usb stick gets > >>detected and then immediatly the driver goes to low power mode which is > >>not correct. > >> > >>log: > >>[ 1.631412] msm_hsusb_host 12520000.usb: EHCI Host Controller > >>[ 1.636556] msm_hsusb_host 12520000.usb: new USB bus registered, assigned bus number 1 > >>[ 1.642563] msm_hsusb_host 12520000.usb: irq 220, io mem 0x12520000 > >>[ 1.658197] msm_hsusb_host 12520000.usb: USB 2.0 started, EHCI 1.00 > >>[ 1.659473] hub 1-0:1.0: USB hub found > >>[ 1.663415] hub 1-0:1.0: 1 port detected > >>... > >>[ 1.973352] usb 1-1: new high-speed USB device number 2 using msm_hsusb_host > >>[ 2.107707] usb-storage 1-1:1.0: USB Mass Storage device detected > >>[ 2.108993] scsi0 : usb-storage 1-1:1.0 > >>[ 2.678341] msm_otg 12520000.phy: USB in low power mode > >>[ 3.168977] usb 1-1: USB disconnect, device number 2 > >> > >>This issue was detected on IFC6410 board. > >> > >>This patch fixes the intial runtime pm trigger by checking the phy > >>state and decrementing the pm use count only when the phy state is IDLE. > >> > >>Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > >>--- > >> drivers/usb/phy/phy-msm-usb.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c > >>index 3bb559d..78cc870 100644 > >>--- a/drivers/usb/phy/phy-msm-usb.c > >>+++ b/drivers/usb/phy/phy-msm-usb.c > >>@@ -1229,7 +1229,9 @@ static void msm_otg_sm_work(struct work_struct *w) > >> motg->chg_state = USB_CHG_STATE_UNDEFINED; > >> motg->chg_type = USB_INVALID_CHARGER; > >> } > >>- pm_runtime_put_sync(otg->phy->dev); > >>+ > >>+ if (otg->phy->state == OTG_STATE_B_IDLE) > >>+ pm_runtime_put_sync(otg->phy->dev); > > > >instead, you might as well just use autosuspend. > > autosuspend is a good idea and will provide a delay before rumtime suspend, > however the bug which is fixed here still needs to be fixed anyway. > > runtime PM in this driver is not that great, the driver just increments the > pm use count if the phy is configured in host or deivce mode. This patch > fixes a bug in its state-machine which decrements pm use-count without > checking the state. > > As this is a PHY driver, am not sure moving to autosuspend means we would > end up just adding some static inactivity delay? Is it really going to add > any value to this PHY driver? yeah, perhaps not... I guess we can apply this as a fix; you're right.
diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c index 3bb559d..78cc870 100644 --- a/drivers/usb/phy/phy-msm-usb.c +++ b/drivers/usb/phy/phy-msm-usb.c @@ -1229,7 +1229,9 @@ static void msm_otg_sm_work(struct work_struct *w) motg->chg_state = USB_CHG_STATE_UNDEFINED; motg->chg_type = USB_INVALID_CHARGER; } - pm_runtime_put_sync(otg->phy->dev); + + if (otg->phy->state == OTG_STATE_B_IDLE) + pm_runtime_put_sync(otg->phy->dev); break; case OTG_STATE_B_PERIPHERAL: dev_dbg(otg->phy->dev, "OTG_STATE_B_PERIPHERAL state\n");
Use case is when the phy is configured in host mode and a usb device is attached to board before bootup. On bootup, with the existing code and runtime pm enabled, the driver would decrement the pm usage count without checking the current state of the phy. This pm usage count decrement would trigger the runtime pm which than would abort the usb enumeration which was in progress. In my case a usb stick gets detected and then immediatly the driver goes to low power mode which is not correct. log: [ 1.631412] msm_hsusb_host 12520000.usb: EHCI Host Controller [ 1.636556] msm_hsusb_host 12520000.usb: new USB bus registered, assigned bus number 1 [ 1.642563] msm_hsusb_host 12520000.usb: irq 220, io mem 0x12520000 [ 1.658197] msm_hsusb_host 12520000.usb: USB 2.0 started, EHCI 1.00 [ 1.659473] hub 1-0:1.0: USB hub found [ 1.663415] hub 1-0:1.0: 1 port detected ... [ 1.973352] usb 1-1: new high-speed USB device number 2 using msm_hsusb_host [ 2.107707] usb-storage 1-1:1.0: USB Mass Storage device detected [ 2.108993] scsi0 : usb-storage 1-1:1.0 [ 2.678341] msm_otg 12520000.phy: USB in low power mode [ 3.168977] usb 1-1: USB disconnect, device number 2 This issue was detected on IFC6410 board. This patch fixes the intial runtime pm trigger by checking the phy state and decrementing the pm use count only when the phy state is IDLE. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/usb/phy/phy-msm-usb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)