[PATCH 0/5] musb/dsps: suspend related patches for 3.14 (rebased)

[PATCH 0/5] musb/dsps: suspend related patches for 3.14 (rebased)

[Daniel Mack]

Hi Felipe,

These five patches for musb/dsps didn't make it into 3.13, which is no
big deal, at least as far as I'm concerned :)

I rebased the missing bits on top of 3.13-rc1. There are no functional
changes in comparison to what was posted earlier on this list.

Successfully tested on a AM33xx board.


Thanks,
Daniel


Daniel Mack (5):
  usb: musb: conditionally save and restore the context on suspend
  usb: musb: call musb_port_suspend from musb_bus_suspend
  usb: musb: dsps: add {tx,rx}_mode to wrapper
  usb: musb: dsps: add support for suspend and resume
  usb: musb: dsps: indentation and whitespace fixes

 drivers/usb/musb/musb_core.c    | 17 +++++++-
 drivers/usb/musb/musb_core.h    |  1 +
 drivers/usb/musb/musb_dsps.c    | 86 +++++++++++++++++++++++++++++++++++------
 drivers/usb/musb/musb_host.c    |  2 +
 drivers/usb/musb/musb_host.h    |  4 ++
 drivers/usb/musb/musb_virthub.c |  4 +-
 include/linux/usb/musb.h        |  3 ++
 7 files changed, 102 insertions(+), 15 deletions(-)

-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] usb: musb: conditionally save and restore the context on suspend

[Daniel Mack]

It appears not all platforms featuring a musb core need to save the musb
core registers at suspend time and restore them on resume.

The dsps platform does, however. So add a bit in struct
musb_hdrc_platform_data to let platforms specify their need of such
action being taken.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/usb/musb/musb_core.c | 17 ++++++++++++++++-
 include/linux/usb/musb.h     |  3 +++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 0a43329..a8ded57 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev)
 {
 	struct musb	*musb = dev_to_musb(dev);
 	unsigned long	flags;
+	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
 
 	spin_lock_irqsave(&musb->lock, flags);
 
@@ -2215,16 +2216,30 @@ static int musb_suspend(struct device *dev)
 		 */
 	}
 
+	if (plat->restore_after_suspend)
+		musb_save_context(musb);
+
 	spin_unlock_irqrestore(&musb->lock, flags);
 	return 0;
 }
 
 static int musb_resume_noirq(struct device *dev)
 {
-	/* for static cmos like DaVinci, register values were preserved
+	struct musb	*musb = dev_to_musb(dev);
+	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
+
+	/*
+	 * For static cmos like DaVinci, register values were preserved
 	 * unless for some reason the whole soc powered down or the USB
 	 * module got reset through the PSC (vs just being disabled).
+	 *
+	 * The plaform data tells us about the necessity of saving and
+	 * restoring the context across a suspend cycle.
 	 */
+
+	if (plat->restore_after_suspend)
+		musb_restore_context(musb);
+
 	return 0;
 }
 
diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h
index eb50525..e5a581c 100644
--- a/include/linux/usb/musb.h
+++ b/include/linux/usb/musb.h
@@ -99,6 +99,9 @@ struct musb_hdrc_platform_data {
 	/* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */
 	u8		mode;
 
+	/* should the musb core restore registers after suspend? */
+	u8		restore_after_suspend:1;
+
 	/* for clk_get() */
 	const char	*clock;
 
-- 
1.8.4.2

[PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Daniel Mack]

Make musb_port_suspend() externally available, and call it when to host
goes into suspend. This allows the core to go into suspend while a
device is connected.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/usb/musb/musb_host.c    | 2 ++
 drivers/usb/musb/musb_host.h    | 2 ++
 drivers/usb/musb/musb_virthub.c | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 6582a20..81caf9f 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd)
 	struct musb	*musb = hcd_to_musb(hcd);
 	u8		devctl;
 
+	musb_port_suspend(musb, true);
+
 	if (!is_host_active(musb))
 		return 0;
 
diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
index 960d735..e660af9 100644
--- a/drivers/usb/musb/musb_host.h
+++ b/drivers/usb/musb/musb_host.h
@@ -92,6 +92,7 @@ extern void musb_host_rx(struct musb *, u8);
 extern void musb_root_disconnect(struct musb *musb);
 extern void musb_host_resume_root_hub(struct musb *musb);
 extern void musb_host_poke_root_hub(struct musb *musb);
+extern void musb_port_suspend(struct musb *musb, bool do_suspend);
 #else
 static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
 {
@@ -121,6 +122,7 @@ static inline void musb_root_disconnect(struct musb *musb)	{}
 static inline void musb_host_resume_root_hub(struct musb *musb)	{}
 static inline void musb_host_poll_rh_status(struct musb *musb)	{}
 static inline void musb_host_poke_root_hub(struct musb *musb)	{}
+static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
 #endif
 
 struct usb_hcd;
diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
index 9af6bba..e977441 100644
--- a/drivers/usb/musb/musb_virthub.c
+++ b/drivers/usb/musb/musb_virthub.c
@@ -44,7 +44,7 @@
 
 #include "musb_core.h"
 
-static void musb_port_suspend(struct musb *musb, bool do_suspend)
+void musb_port_suspend(struct musb *musb, bool do_suspend)
 {
 	struct usb_otg	*otg = musb->xceiv->otg;
 	u8		power;
-- 
1.8.4.2

[PATCH 3/5] usb: musb: dsps: add {tx,rx}_mode to wrapper

[Daniel Mack]

rx_mode and tx_mode need to be read at suspend time and restored on
resume for dsps platforms. So add it to the wrapper struct first, and
initialize the values.

Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/musb/musb_dsps.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 1901f6f..e57d712 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -83,6 +83,8 @@ struct dsps_musb_wrapper {
 	u16	coreintr_status;
 	u16	phy_utmi;
 	u16	mode;
+	u16	tx_mode;
+	u16	rx_mode;
 
 	/* bit positions for control */
 	unsigned	reset:5;
@@ -605,6 +607,8 @@ static const struct dsps_musb_wrapper am33xx_driver_data = {
 	.coreintr_status	= 0x34,
 	.phy_utmi		= 0xe0,
 	.mode			= 0xe8,
+	.tx_mode		= 0x70,
+	.rx_mode		= 0x74,
 	.reset			= 0,
 	.otg_disable		= 21,
 	.iddig			= 8,
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Daniel Mack]

The dsps platform needs to save save some registers at suspend time and
restore them after resume. This patch adds a struct for these registers,
and also lets the musb core know that the core registers need to be
saved as well.

We also have to call musb_port_reset() for this platform upon resume, so
this function has to be made non-static.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/usb/musb/musb_core.h    |  1 +
 drivers/usb/musb/musb_dsps.c    | 59 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/musb/musb_host.h    |  2 ++
 drivers/usb/musb/musb_virthub.c |  2 +-
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 29f7cd7..a423037 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -295,6 +295,7 @@ struct musb {
 
 	irqreturn_t		(*isr)(int, void *);
 	struct work_struct	irq_work;
+
 	u16			hwvers;
 
 	u16			intrrxe;
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index e57d712..361ddf8 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -112,6 +112,19 @@ struct dsps_musb_wrapper {
 	u8		poll_seconds;
 };
 
+/*
+ * register shadow for suspend
+ */
+struct dsps_context {
+	u32 control;
+	u32 epintr;
+	u32 coreintr;
+	u32 phy_utmi;
+	u32 mode;
+	u32 tx_mode;
+	u32 rx_mode;
+};
+
 /**
  * DSPS glue structure.
  */
@@ -121,6 +134,8 @@ struct dsps_glue {
 	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
 	struct timer_list timer;	/* otg_workaround timer */
 	unsigned long last_timer;    /* last timer data for each instance */
+
+	struct dsps_context context;
 };
 
 static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
@@ -506,6 +521,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
 	}
 	pdata.config = config;
 	pdata.platform_ops = &dsps_ops;
+	pdata.restore_after_suspend = 1;
 
 	config->num_eps = get_int_prop(dn, "mentor,num-eps");
 	config->ram_bits = get_int_prop(dn, "mentor,ram-bits");
@@ -632,11 +648,54 @@ static const struct of_device_id musb_dsps_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, musb_dsps_of_match);
 
+#ifdef CONFIG_PM
+static int dsps_suspend(struct device *dev)
+{
+	struct dsps_glue *glue = dev_get_drvdata(dev);
+	const struct dsps_musb_wrapper *wrp = glue->wrp;
+	struct musb *musb = platform_get_drvdata(glue->musb);
+	void __iomem *mbase = musb->ctrl_base;
+
+	glue->context.control = dsps_readl(mbase, wrp->control);
+	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
+	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
+	glue->context.phy_utmi = dsps_readl(mbase, wrp->phy_utmi);
+	glue->context.mode = dsps_readl(mbase, wrp->mode);
+	glue->context.tx_mode = dsps_readl(mbase, wrp->tx_mode);
+	glue->context.rx_mode = dsps_readl(mbase, wrp->rx_mode);
+
+	return 0;
+}
+
+static int dsps_resume(struct device *dev)
+{
+	struct dsps_glue *glue = dev_get_drvdata(dev);
+	const struct dsps_musb_wrapper *wrp = glue->wrp;
+	struct musb *musb = platform_get_drvdata(glue->musb);
+	void __iomem *mbase = musb->ctrl_base;
+
+	dsps_writel(mbase, wrp->control, glue->context.control);
+	dsps_writel(mbase, wrp->epintr_set, glue->context.epintr);
+	dsps_writel(mbase, wrp->coreintr_set, glue->context.coreintr);
+	dsps_writel(mbase, wrp->phy_utmi, glue->context.phy_utmi);
+	dsps_writel(mbase, wrp->mode, glue->context.mode);
+	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
+	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
+
+	musb_port_reset(musb, false);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
+
 static struct platform_driver dsps_usbss_driver = {
 	.probe		= dsps_probe,
 	.remove         = dsps_remove,
 	.driver         = {
 		.name   = "musb-dsps",
+		.pm	= &dsps_pm_ops,
 		.of_match_table	= musb_dsps_of_match,
 	},
 };
diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
index e660af9..7436c24 100644
--- a/drivers/usb/musb/musb_host.h
+++ b/drivers/usb/musb/musb_host.h
@@ -93,6 +93,7 @@ extern void musb_root_disconnect(struct musb *musb);
 extern void musb_host_resume_root_hub(struct musb *musb);
 extern void musb_host_poke_root_hub(struct musb *musb);
 extern void musb_port_suspend(struct musb *musb, bool do_suspend);
+extern void musb_port_reset(struct musb *musb, bool do_reset);
 #else
 static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
 {
@@ -123,6 +124,7 @@ static inline void musb_host_resume_root_hub(struct musb *musb)	{}
 static inline void musb_host_poll_rh_status(struct musb *musb)	{}
 static inline void musb_host_poke_root_hub(struct musb *musb)	{}
 static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
+static inline void musb_port_reset(struct musb *musb)		{}
 #endif
 
 struct usb_hcd;
diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
index e977441..24e46c0 100644
--- a/drivers/usb/musb/musb_virthub.c
+++ b/drivers/usb/musb/musb_virthub.c
@@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
 	}
 }
 
-static void musb_port_reset(struct musb *musb, bool do_reset)
+void musb_port_reset(struct musb *musb, bool do_reset)
 {
 	u8		power;
 	void __iomem	*mbase = musb->mregs;
-- 
1.8.4.2

[PATCH 5/5] usb: musb: dsps: indentation and whitespace fixes

[Daniel Mack]

Just a cosmetic thing, no functional change.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/usb/musb/musb_dsps.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 361ddf8..b27c072 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -131,9 +131,9 @@ struct dsps_context {
 struct dsps_glue {
 	struct device *dev;
 	struct platform_device *musb;	/* child musb pdev */
-	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
+	const struct dsps_musb_wrapper *wrp;	/* wrapper register offsets */
 	struct timer_list timer;	/* otg_workaround timer */
-	unsigned long last_timer;    /* last timer data for each instance */
+	unsigned long last_timer;	/* last timer data for each instance */
 
 	struct dsps_context context;
 };
@@ -213,7 +213,7 @@ static void dsps_musb_disable(struct musb *musb)
 
 	dsps_writel(reg_base, wrp->coreintr_clear, wrp->usb_bitmap);
 	dsps_writel(reg_base, wrp->epintr_clear,
-			 wrp->txep_bitmap | wrp->rxep_bitmap);
+		    wrp->txep_bitmap | wrp->rxep_bitmap);
 	dsps_writeb(musb->mregs, MUSB_DEVCTL, 0);
 }
 
@@ -234,7 +234,7 @@ static void otg_timer(unsigned long _musb)
 	 */
 	devctl = dsps_readb(mregs, MUSB_DEVCTL);
 	dev_dbg(musb->controller, "Poll devctl %02x (%s)\n", devctl,
-				usb_otg_state_string(musb->xceiv->state));
+		usb_otg_state_string(musb->xceiv->state));
 
 	spin_lock_irqsave(&musb->lock, flags);
 	switch (musb->xceiv->state) {
@@ -298,7 +298,7 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
 		dsps_writel(reg_base, wrp->coreintr_status, usbintr);
 
 	dev_dbg(musb->controller, "usbintr (%x) epintr(%x)\n",
-			usbintr, epintr);
+		usbintr, epintr);
 	/*
 	 * DRVVBUS IRQs are the only proxy we have (a very poor one!) for
 	 * DSPS IP's missing ID change IRQ.  We need an ID change IRQ to
@@ -348,10 +348,9 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
 
 		/* NOTE: this must complete power-on within 100 ms. */
 		dev_dbg(musb->controller, "VBUS %s (%s)%s, devctl %02x\n",
-				drvvbus ? "on" : "off",
-				usb_otg_state_string(musb->xceiv->state),
-				err ? " ERROR" : "",
-				devctl);
+			drvvbus ? "on" : "off",
+			usb_otg_state_string(musb->xceiv->state),
+			err ? " ERROR" : "", devctl);
 		ret = IRQ_HANDLED;
 	}
 
@@ -692,9 +691,9 @@ static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
 
 static struct platform_driver dsps_usbss_driver = {
 	.probe		= dsps_probe,
-	.remove         = dsps_remove,
-	.driver         = {
-		.name   = "musb-dsps",
+	.remove		= dsps_remove,
+	.driver		= {
+		.name	= "musb-dsps",
 		.pm	= &dsps_pm_ops,
 		.of_match_table	= musb_dsps_of_match,
 	},
-- 
1.8.4.2

Re: [PATCH 1/5] usb: musb: conditionally save and restore the context on suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Hi,

On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote:
> It appears not all platforms featuring a musb core need to save the musb
> core registers at suspend time and restore them on resume.
> 
> The dsps platform does, however. So add a bit in struct
> musb_hdrc_platform_data to let platforms specify their need of such
> action being taken.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  drivers/usb/musb/musb_core.c | 17 ++++++++++++++++-
>  include/linux/usb/musb.h     |  3 +++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 0a43329..a8ded57 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev)
>  {
>  	struct musb	*musb = dev_to_musb(dev);
>  	unsigned long	flags;
> +	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);

we don't want to have platform_data on DT-based boot. It's best to just
save those registers unconditionally as it doesn't hurt.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote:
> Make musb_port_suspend() externally available, and call it when to host
> goes into suspend. This allows the core to go into suspend while a
> device is connected.
> 
> Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/musb/musb_host.c    | 2 ++
>  drivers/usb/musb/musb_host.h    | 2 ++
>  drivers/usb/musb/musb_virthub.c | 2 +-
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 6582a20..81caf9f 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd)
>  	struct musb	*musb = hcd_to_musb(hcd);
>  	u8		devctl;
>  
> +	musb_port_suspend(musb, true);

have you considered the fact that when musb looses context it'll cause a
disconnect on the bus because soft_connect bit is lost ?

What if you have a mounted file system on a pendrive ? Should we allow
suspend in that case ?

Alan, Greg, any hints ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6003 bytes --]

On Mon, Nov 25, 2013 at 08:39:52PM +0100, Daniel Mack wrote:
> The dsps platform needs to save save some registers at suspend time and
> restore them after resume. This patch adds a struct for these registers,
> and also lets the musb core know that the core registers need to be
> saved as well.
> 
> We also have to call musb_port_reset() for this platform upon resume, so
> this function has to be made non-static.
> 
> Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/musb/musb_core.h    |  1 +
>  drivers/usb/musb/musb_dsps.c    | 59 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/musb/musb_host.h    |  2 ++
>  drivers/usb/musb/musb_virthub.c |  2 +-
>  4 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index 29f7cd7..a423037 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -295,6 +295,7 @@ struct musb {
>  
>  	irqreturn_t		(*isr)(int, void *);
>  	struct work_struct	irq_work;
> +
>  	u16			hwvers;
>  
>  	u16			intrrxe;
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index e57d712..361ddf8 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -112,6 +112,19 @@ struct dsps_musb_wrapper {
>  	u8		poll_seconds;
>  };
>  
> +/*
> + * register shadow for suspend
> + */
> +struct dsps_context {
> +	u32 control;
> +	u32 epintr;
> +	u32 coreintr;
> +	u32 phy_utmi;
> +	u32 mode;
> +	u32 tx_mode;
> +	u32 rx_mode;
> +};
> +
>  /**
>   * DSPS glue structure.
>   */
> @@ -121,6 +134,8 @@ struct dsps_glue {
>  	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
>  	struct timer_list timer;	/* otg_workaround timer */
>  	unsigned long last_timer;    /* last timer data for each instance */
> +
> +	struct dsps_context context;
>  };
>  
>  static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
> @@ -506,6 +521,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
>  	}
>  	pdata.config = config;
>  	pdata.platform_ops = &dsps_ops;
> +	pdata.restore_after_suspend = 1;
>  
>  	config->num_eps = get_int_prop(dn, "mentor,num-eps");
>  	config->ram_bits = get_int_prop(dn, "mentor,ram-bits");
> @@ -632,11 +648,54 @@ static const struct of_device_id musb_dsps_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, musb_dsps_of_match);
>  
> +#ifdef CONFIG_PM
> +static int dsps_suspend(struct device *dev)
> +{
> +	struct dsps_glue *glue = dev_get_drvdata(dev);
> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
> +	struct musb *musb = platform_get_drvdata(glue->musb);
> +	void __iomem *mbase = musb->ctrl_base;
> +
> +	glue->context.control = dsps_readl(mbase, wrp->control);
> +	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
> +	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
> +	glue->context.phy_utmi = dsps_readl(mbase, wrp->phy_utmi);
> +	glue->context.mode = dsps_readl(mbase, wrp->mode);
> +	glue->context.tx_mode = dsps_readl(mbase, wrp->tx_mode);
> +	glue->context.rx_mode = dsps_readl(mbase, wrp->rx_mode);
> +
> +	return 0;
> +}
> +
> +static int dsps_resume(struct device *dev)
> +{
> +	struct dsps_glue *glue = dev_get_drvdata(dev);
> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
> +	struct musb *musb = platform_get_drvdata(glue->musb);
> +	void __iomem *mbase = musb->ctrl_base;
> +
> +	dsps_writel(mbase, wrp->control, glue->context.control);
> +	dsps_writel(mbase, wrp->epintr_set, glue->context.epintr);
> +	dsps_writel(mbase, wrp->coreintr_set, glue->context.coreintr);
> +	dsps_writel(mbase, wrp->phy_utmi, glue->context.phy_utmi);
> +	dsps_writel(mbase, wrp->mode, glue->context.mode);
> +	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
> +	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
> +
> +	musb_port_reset(musb, false);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
> +
>  static struct platform_driver dsps_usbss_driver = {
>  	.probe		= dsps_probe,
>  	.remove         = dsps_remove,
>  	.driver         = {
>  		.name   = "musb-dsps",
> +		.pm	= &dsps_pm_ops,
>  		.of_match_table	= musb_dsps_of_match,
>  	},
>  };
> diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
> index e660af9..7436c24 100644
> --- a/drivers/usb/musb/musb_host.h
> +++ b/drivers/usb/musb/musb_host.h
> @@ -93,6 +93,7 @@ extern void musb_root_disconnect(struct musb *musb);
>  extern void musb_host_resume_root_hub(struct musb *musb);
>  extern void musb_host_poke_root_hub(struct musb *musb);
>  extern void musb_port_suspend(struct musb *musb, bool do_suspend);
> +extern void musb_port_reset(struct musb *musb, bool do_reset);
>  #else
>  static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
>  {
> @@ -123,6 +124,7 @@ static inline void musb_host_resume_root_hub(struct musb *musb)	{}
>  static inline void musb_host_poll_rh_status(struct musb *musb)	{}
>  static inline void musb_host_poke_root_hub(struct musb *musb)	{}
>  static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
> +static inline void musb_port_reset(struct musb *musb)		{}
>  #endif
>  
>  struct usb_hcd;
> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> index e977441..24e46c0 100644
> --- a/drivers/usb/musb/musb_virthub.c
> +++ b/drivers/usb/musb/musb_virthub.c
> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>  	}
>  }
>  
> -static void musb_port_reset(struct musb *musb, bool do_reset)
> +void musb_port_reset(struct musb *musb, bool do_reset)

NAK, this should not be called from the glue layers at all. If anything
call from musb_core's resume callback. That will only be called after
parent's resume has been called anyway.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] usb: musb: dsps: indentation and whitespace fixes

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

On Mon, Nov 25, 2013 at 08:39:53PM +0100, Daniel Mack wrote:
> Just a cosmetic thing, no functional change.
> 
> Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/musb/musb_dsps.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 361ddf8..b27c072 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -131,9 +131,9 @@ struct dsps_context {
>  struct dsps_glue {
>  	struct device *dev;
>  	struct platform_device *musb;	/* child musb pdev */
> -	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
> +	const struct dsps_musb_wrapper *wrp;	/* wrapper register offsets */
>  	struct timer_list timer;	/* otg_workaround timer */
> -	unsigned long last_timer;    /* last timer data for each instance */
> +	unsigned long last_timer;	/* last timer data for each instance */
>  
>  	struct dsps_context context;
>  };
> @@ -213,7 +213,7 @@ static void dsps_musb_disable(struct musb *musb)
>  
>  	dsps_writel(reg_base, wrp->coreintr_clear, wrp->usb_bitmap);
>  	dsps_writel(reg_base, wrp->epintr_clear,
> -			 wrp->txep_bitmap | wrp->rxep_bitmap);
> +		    wrp->txep_bitmap | wrp->rxep_bitmap);

quite frankly, I don't what checkpatch.pl is trying to force here. I
much prefer a tab-only indentation, matching with ( characters forces us
to add some spaces. I would rather not take this patch.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Daniel Mack]

On 11/25/2013 08:46 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote:
>> Make musb_port_suspend() externally available, and call it when to host
>> goes into suspend. This allows the core to go into suspend while a
>> device is connected.
>>
>> Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/usb/musb/musb_host.c    | 2 ++
>>  drivers/usb/musb/musb_host.h    | 2 ++
>>  drivers/usb/musb/musb_virthub.c | 2 +-
>>  3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>> index 6582a20..81caf9f 100644
>> --- a/drivers/usb/musb/musb_host.c
>> +++ b/drivers/usb/musb/musb_host.c
>> @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd)
>>  	struct musb	*musb = hcd_to_musb(hcd);
>>  	u8		devctl;
>>  
>> +	musb_port_suspend(musb, true);
> 
> have you considered the fact that when musb looses context it'll cause a
> disconnect on the bus because soft_connect bit is lost ?
>
> What if you have a mounted file system on a pendrive ? Should we allow
> suspend in that case ?

Well, I would have expected that, but in fact, the opposite is true.
With 3.12, mounting a filesystem on a USB media and accessing it after
resume was exactly my test case, and it worked just fine with that patch.

In 3.13, something about the parition table reading seems to be broken
currently, I'll have a closer look.


Thanks,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: musb: dsps: indentation and whitespace fixes

[Daniel Mack]

On 11/25/2013 08:49 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 08:39:53PM +0100, Daniel Mack wrote:
>> Just a cosmetic thing, no functional change.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  drivers/usb/musb/musb_dsps.c | 23 +++++++++++------------
>>  1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
>> index 361ddf8..b27c072 100644
>> --- a/drivers/usb/musb/musb_dsps.c
>> +++ b/drivers/usb/musb/musb_dsps.c
>> @@ -131,9 +131,9 @@ struct dsps_context {
>>  struct dsps_glue {
>>  	struct device *dev;
>>  	struct platform_device *musb;	/* child musb pdev */
>> -	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
>> +	const struct dsps_musb_wrapper *wrp;	/* wrapper register offsets */
>>  	struct timer_list timer;	/* otg_workaround timer */
>> -	unsigned long last_timer;    /* last timer data for each instance */
>> +	unsigned long last_timer;	/* last timer data for each instance */
>>  
>>  	struct dsps_context context;
>>  };
>> @@ -213,7 +213,7 @@ static void dsps_musb_disable(struct musb *musb)
>>  
>>  	dsps_writel(reg_base, wrp->coreintr_clear, wrp->usb_bitmap);
>>  	dsps_writel(reg_base, wrp->epintr_clear,
>> -			 wrp->txep_bitmap | wrp->rxep_bitmap);
>> +		    wrp->txep_bitmap | wrp->rxep_bitmap);
> 
> quite frankly, I don't what checkpatch.pl is trying to force here. I
> much prefer a tab-only indentation, matching with ( characters forces us
> to add some spaces. I would rather not take this patch.
> 

Alright, I really don't care much. I'll drop the patch when resending.

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

Hi,

On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote:
> On 11/25/2013 08:46 PM, Felipe Balbi wrote:
> > On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote:
> >> Make musb_port_suspend() externally available, and call it when to host
> >> goes into suspend. This allows the core to go into suspend while a
> >> device is connected.
> >>
> >> Signed-off-by: Daniel Mack <zonque@gmail.com>
> >> ---
> >>  drivers/usb/musb/musb_host.c    | 2 ++
> >>  drivers/usb/musb/musb_host.h    | 2 ++
> >>  drivers/usb/musb/musb_virthub.c | 2 +-
> >>  3 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> >> index 6582a20..81caf9f 100644
> >> --- a/drivers/usb/musb/musb_host.c
> >> +++ b/drivers/usb/musb/musb_host.c
> >> @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd)
> >>  	struct musb	*musb = hcd_to_musb(hcd);
> >>  	u8		devctl;
> >>  
> >> +	musb_port_suspend(musb, true);
> > 
> > have you considered the fact that when musb looses context it'll cause a
> > disconnect on the bus because soft_connect bit is lost ?
> >
> > What if you have a mounted file system on a pendrive ? Should we allow
> > suspend in that case ?
> 
> Well, I would have expected that, but in fact, the opposite is true.
> With 3.12, mounting a filesystem on a USB media and accessing it after
> resume was exactly my test case, and it worked just fine with that patch.

so you had a mounted file system, suspend, resume, it was still mounted?
or did it reenumerate and you remounted it ?

Try to do the same with transfers in flight, it's likely to corrupt your
file system.

> In 3.13, something about the parition table reading seems to be broken
> currently, I'll have a closer look.

cool

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Daniel Mack]

On 11/25/2013 08:48 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 08:39:52PM +0100, Daniel Mack wrote:
>> The dsps platform needs to save save some registers at suspend time and
>> restore them after resume. This patch adds a struct for these registers,
>> and also lets the musb core know that the core registers need to be
>> saved as well.
>>
>> We also have to call musb_port_reset() for this platform upon resume, so
>> this function has to be made non-static.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  drivers/usb/musb/musb_core.h    |  1 +
>>  drivers/usb/musb/musb_dsps.c    | 59 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/musb/musb_host.h    |  2 ++
>>  drivers/usb/musb/musb_virthub.c |  2 +-
>>  4 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
>> index 29f7cd7..a423037 100644
>> --- a/drivers/usb/musb/musb_core.h
>> +++ b/drivers/usb/musb/musb_core.h
>> @@ -295,6 +295,7 @@ struct musb {
>>  
>>  	irqreturn_t		(*isr)(int, void *);
>>  	struct work_struct	irq_work;
>> +
>>  	u16			hwvers;
>>  
>>  	u16			intrrxe;
>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
>> index e57d712..361ddf8 100644
>> --- a/drivers/usb/musb/musb_dsps.c
>> +++ b/drivers/usb/musb/musb_dsps.c
>> @@ -112,6 +112,19 @@ struct dsps_musb_wrapper {
>>  	u8		poll_seconds;
>>  };
>>  
>> +/*
>> + * register shadow for suspend
>> + */
>> +struct dsps_context {
>> +	u32 control;
>> +	u32 epintr;
>> +	u32 coreintr;
>> +	u32 phy_utmi;
>> +	u32 mode;
>> +	u32 tx_mode;
>> +	u32 rx_mode;
>> +};
>> +
>>  /**
>>   * DSPS glue structure.
>>   */
>> @@ -121,6 +134,8 @@ struct dsps_glue {
>>  	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
>>  	struct timer_list timer;	/* otg_workaround timer */
>>  	unsigned long last_timer;    /* last timer data for each instance */
>> +
>> +	struct dsps_context context;
>>  };
>>  
>>  static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
>> @@ -506,6 +521,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
>>  	}
>>  	pdata.config = config;
>>  	pdata.platform_ops = &dsps_ops;
>> +	pdata.restore_after_suspend = 1;
>>  
>>  	config->num_eps = get_int_prop(dn, "mentor,num-eps");
>>  	config->ram_bits = get_int_prop(dn, "mentor,ram-bits");
>> @@ -632,11 +648,54 @@ static const struct of_device_id musb_dsps_of_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, musb_dsps_of_match);
>>  
>> +#ifdef CONFIG_PM
>> +static int dsps_suspend(struct device *dev)
>> +{
>> +	struct dsps_glue *glue = dev_get_drvdata(dev);
>> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
>> +	struct musb *musb = platform_get_drvdata(glue->musb);
>> +	void __iomem *mbase = musb->ctrl_base;
>> +
>> +	glue->context.control = dsps_readl(mbase, wrp->control);
>> +	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
>> +	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
>> +	glue->context.phy_utmi = dsps_readl(mbase, wrp->phy_utmi);
>> +	glue->context.mode = dsps_readl(mbase, wrp->mode);
>> +	glue->context.tx_mode = dsps_readl(mbase, wrp->tx_mode);
>> +	glue->context.rx_mode = dsps_readl(mbase, wrp->rx_mode);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dsps_resume(struct device *dev)
>> +{
>> +	struct dsps_glue *glue = dev_get_drvdata(dev);
>> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
>> +	struct musb *musb = platform_get_drvdata(glue->musb);
>> +	void __iomem *mbase = musb->ctrl_base;
>> +
>> +	dsps_writel(mbase, wrp->control, glue->context.control);
>> +	dsps_writel(mbase, wrp->epintr_set, glue->context.epintr);
>> +	dsps_writel(mbase, wrp->coreintr_set, glue->context.coreintr);
>> +	dsps_writel(mbase, wrp->phy_utmi, glue->context.phy_utmi);
>> +	dsps_writel(mbase, wrp->mode, glue->context.mode);
>> +	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
>> +	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
>> +
>> +	musb_port_reset(musb, false);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
>> +
>>  static struct platform_driver dsps_usbss_driver = {
>>  	.probe		= dsps_probe,
>>  	.remove         = dsps_remove,
>>  	.driver         = {
>>  		.name   = "musb-dsps",
>> +		.pm	= &dsps_pm_ops,
>>  		.of_match_table	= musb_dsps_of_match,
>>  	},
>>  };
>> diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
>> index e660af9..7436c24 100644
>> --- a/drivers/usb/musb/musb_host.h
>> +++ b/drivers/usb/musb/musb_host.h
>> @@ -93,6 +93,7 @@ extern void musb_root_disconnect(struct musb *musb);
>>  extern void musb_host_resume_root_hub(struct musb *musb);
>>  extern void musb_host_poke_root_hub(struct musb *musb);
>>  extern void musb_port_suspend(struct musb *musb, bool do_suspend);
>> +extern void musb_port_reset(struct musb *musb, bool do_reset);
>>  #else
>>  static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
>>  {
>> @@ -123,6 +124,7 @@ static inline void musb_host_resume_root_hub(struct musb *musb)	{}
>>  static inline void musb_host_poll_rh_status(struct musb *musb)	{}
>>  static inline void musb_host_poke_root_hub(struct musb *musb)	{}
>>  static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
>> +static inline void musb_port_reset(struct musb *musb)		{}
>>  #endif
>>  
>>  struct usb_hcd;
>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
>> index e977441..24e46c0 100644
>> --- a/drivers/usb/musb/musb_virthub.c
>> +++ b/drivers/usb/musb/musb_virthub.c
>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>>  	}
>>  }
>>  
>> -static void musb_port_reset(struct musb *musb, bool do_reset)
>> +void musb_port_reset(struct musb *musb, bool do_reset)
> 
> NAK, this should not be called from the glue layers at all. If anything
> call from musb_core's resume callback. That will only be called after
> parent's resume has been called anyway.

Given that this driver is successfully used with suspend/resume on other
platforms without that reset, but it's clearly needed for dsps, I'm
fairly sure this should be a glue-layer specific thing. Hence the change.

I think it'll break things on other platforms if we unconditionally
reset the port after resume, but I can't test it. Anyone?


Thanks,
Daniel

Re: [PATCH 1/5] usb: musb: conditionally save and restore the context on suspend

[Daniel Mack]

On 11/25/2013 08:44 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote:
>> It appears not all platforms featuring a musb core need to save the musb
>> core registers at suspend time and restore them on resume.
>>
>> The dsps platform does, however. So add a bit in struct
>> musb_hdrc_platform_data to let platforms specify their need of such
>> action being taken.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  drivers/usb/musb/musb_core.c | 17 ++++++++++++++++-
>>  include/linux/usb/musb.h     |  3 +++
>>  2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> index 0a43329..a8ded57 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
>> @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev)
>>  {
>>  	struct musb	*musb = dev_to_musb(dev);
>>  	unsigned long	flags;
>> +	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
> 
> we don't want to have platform_data on DT-based boot. It's best to just
> save those registers unconditionally as it doesn't hurt.
> 

My concern about doing it unconditionally from the core is simply that I
fear regressions for other platforms. I can of course drop it if you're
certain that that's correct.

I can only test this on a dsps glue layer, and I have no documentation
for the musb core. All I'm left with here is fishing in muddy waters :/


Daniel

Re: [PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

Hi,

On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
> >> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> >> index e977441..24e46c0 100644
> >> --- a/drivers/usb/musb/musb_virthub.c
> >> +++ b/drivers/usb/musb/musb_virthub.c
> >> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
> >>  	}
> >>  }
> >>  
> >> -static void musb_port_reset(struct musb *musb, bool do_reset)
> >> +void musb_port_reset(struct musb *musb, bool do_reset)
> > 
> > NAK, this should not be called from the glue layers at all. If anything
> > call from musb_core's resume callback. That will only be called after
> > parent's resume has been called anyway.
> 
> Given that this driver is successfully used with suspend/resume on other
> platforms without that reset, but it's clearly needed for dsps, I'm
> fairly sure this should be a glue-layer specific thing. Hence the change.
> 
> I think it'll break things on other platforms if we unconditionally
> reset the port after resume, but I can't test it. Anyone?

your original commit log only says "we need to issue port reset" but it
never explains why. It could very well be you just found a device which
needs to be reset when coming out of suspend and it misses a quirk flag ?

In any case, those functions should never be called by the glue, if
reset needs to be called, it must be called by musb-hdrc.ko, if you need
a flag, pass a flag but I need a really good explanation in your commit
log of why that's necessary.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/5] usb: musb: conditionally save and restore the context on suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]

Hi,

On Mon, Nov 25, 2013 at 09:08:51PM +0100, Daniel Mack wrote:
> On 11/25/2013 08:44 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote:
> >> It appears not all platforms featuring a musb core need to save the musb
> >> core registers at suspend time and restore them on resume.
> >>
> >> The dsps platform does, however. So add a bit in struct
> >> musb_hdrc_platform_data to let platforms specify their need of such
> >> action being taken.
> >>
> >> Signed-off-by: Daniel Mack <zonque@gmail.com>
> >> ---
> >>  drivers/usb/musb/musb_core.c | 17 ++++++++++++++++-
> >>  include/linux/usb/musb.h     |  3 +++
> >>  2 files changed, 19 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> >> index 0a43329..a8ded57 100644
> >> --- a/drivers/usb/musb/musb_core.c
> >> +++ b/drivers/usb/musb/musb_core.c
> >> @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev)
> >>  {
> >>  	struct musb	*musb = dev_to_musb(dev);
> >>  	unsigned long	flags;
> >> +	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
> > 
> > we don't want to have platform_data on DT-based boot. It's best to just
> > save those registers unconditionally as it doesn't hurt.
> > 
> 
> My concern about doing it unconditionally from the core is simply that I
> fear regressions for other platforms. I can of course drop it if you're
> certain that that's correct.
> 
> I can only test this on a dsps glue layer, and I have no documentation
> for the musb core. All I'm left with here is fishing in muddy waters :/

I really don't think it should cause any issues with any other platform.

We can make to leave this soaking in linux-next for a long time and hope
people test it.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Daniel Mack]

On 11/25/2013 09:01 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote:
>> On 11/25/2013 08:46 PM, Felipe Balbi wrote:
>>> On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote:
>>>> Make musb_port_suspend() externally available, and call it when to host
>>>> goes into suspend. This allows the core to go into suspend while a
>>>> device is connected.
>>>>
>>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>>> ---
>>>>  drivers/usb/musb/musb_host.c    | 2 ++
>>>>  drivers/usb/musb/musb_host.h    | 2 ++
>>>>  drivers/usb/musb/musb_virthub.c | 2 +-
>>>>  3 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>>>> index 6582a20..81caf9f 100644
>>>> --- a/drivers/usb/musb/musb_host.c
>>>> +++ b/drivers/usb/musb/musb_host.c
>>>> @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd)
>>>>  	struct musb	*musb = hcd_to_musb(hcd);
>>>>  	u8		devctl;
>>>>  
>>>> +	musb_port_suspend(musb, true);
>>>
>>> have you considered the fact that when musb looses context it'll cause a
>>> disconnect on the bus because soft_connect bit is lost ?
>>>
>>> What if you have a mounted file system on a pendrive ? Should we allow
>>> suspend in that case ?
>>
>> Well, I would have expected that, but in fact, the opposite is true.
>> With 3.12, mounting a filesystem on a USB media and accessing it after
>> resume was exactly my test case, and it worked just fine with that patch.
> 
> so you had a mounted file system, suspend, resume, it was still mounted?

Yes, exactly.

> or did it reenumerate and you remounted it ?

No, it did not reenumerate, but the core did a reset on it. See the
following log.

# mount /dev/sda /mnt
# md5sum /mnt/sine_left.wav
bcd90548c1ebf293289072f1a6161f0f  /mnt/sine_left.wav
#
#
# echo mem > /sys/power/state
[   31.553425] PM: Syncing filesystems ... done.
[   31.575462] Freezing user space processes ... (elapsed 0.002 seconds)
done.
[   31.585275] Freezing remaining freezable tasks ... (elapsed 0.002
seconds) done.
[   31.595953] PM: Sending message for entering DeepSleep mode
[   31.628887] PM: suspend of devices complete after 21.097 msecs
[   31.641082] PM: late suspend of devices complete after 5.906 msecs
[   31.653834] PM: noirq suspend of devices complete after 6.135 msecs
[   31.660558] PM: GFX domain entered low power state
[   31.660558] PM: Successfully put all powerdomains to target state
[   31.660558] PM: Wakeup source GPIO0
[   31.679130] PM: noirq resume of devices complete after 17.941 msecs
[   31.693371] PM: early resume of devices complete after 4.388 msecs
[   31.702750] net eth0: initializing cpsw version 1.12 (0)
[   31.712084] net eth0: phy found : id is : 0x4dd076
[   31.717317] libphy: PHY  not found
[   31.722448] net eth0: phy  not found on slave 1
[   32.030792] usb 1-1: reset high-speed USB device number 2 using
musb-hdrc <<<< !
[   32.395192] PM: resume of devices complete after 695.279 msecs
[   32.407816] PM: Sending message for resetting M3 state machine
[   32.414683] Restarting tasks ... done.
[   36.701568] libphy: 4a101000.mdio:04 - Link is Up - 100/Full
#
#
# md5sum /mnt/sine_left.wav
bcd90548c1ebf293289072f1a6161f0f  /mnt/sine_left.wav

> Try to do the same with transfers in flight, it's likely to corrupt your
> file system.

I did that as well, and it appeared stable to me. After all, the USB
device was properly put into suspend before VBUS went away, and the usb
storage driver caught up after the core's reset.

In my tests, this was the only approach that would work after resume.



Daniel

Re: [PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Daniel Mack]

On 11/25/2013 09:08 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
>>>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
>>>> index e977441..24e46c0 100644
>>>> --- a/drivers/usb/musb/musb_virthub.c
>>>> +++ b/drivers/usb/musb/musb_virthub.c
>>>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>>>>  	}
>>>>  }
>>>>  
>>>> -static void musb_port_reset(struct musb *musb, bool do_reset)
>>>> +void musb_port_reset(struct musb *musb, bool do_reset)
>>>
>>> NAK, this should not be called from the glue layers at all. If anything
>>> call from musb_core's resume callback. That will only be called after
>>> parent's resume has been called anyway.
>>
>> Given that this driver is successfully used with suspend/resume on other
>> platforms without that reset, but it's clearly needed for dsps, I'm
>> fairly sure this should be a glue-layer specific thing. Hence the change.
>>
>> I think it'll break things on other platforms if we unconditionally
>> reset the port after resume, but I can't test it. Anyone?
> 
> your original commit log only says "we need to issue port reset" but it
> never explains why. It could very well be you just found a device which
> needs to be reset when coming out of suspend and it misses a quirk flag ?

I think I can really rule that out, as I tested with multiple USB sticks
and also tested the same sticks on other embedded platforms.

> In any case, those functions should never be called by the glue, if
> reset needs to be called, it must be called by musb-hdrc.ko, if you need
> a flag, pass a flag but I need a really good explanation in your commit
> log of why that's necessary.

Well, if I'd only knew exactly why. All these patches are really the
result of many days of trial and error, with multiple drivers (musb,
cppi41, ...) involved. And as I said - I have no documentation of the
musb core itself.

So yes, I can do it the other way around and pass a flag, but what I
can't provide is a good explanantion why the dsps glue behaves
differently here than others. I'm curious myself, and all I know is that
with this reset in place, things work as expected on AM33xx.


Thanks for your feedback,
Daniel

Re: [PATCH 1/5] usb: musb: conditionally save and restore the context on suspend

[Daniel Mack]

On 11/25/2013 09:13 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 25, 2013 at 09:08:51PM +0100, Daniel Mack wrote:
>> On 11/25/2013 08:44 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote:
>>>> It appears not all platforms featuring a musb core need to save the musb
>>>> core registers at suspend time and restore them on resume.
>>>>
>>>> The dsps platform does, however. So add a bit in struct
>>>> musb_hdrc_platform_data to let platforms specify their need of such
>>>> action being taken.
>>>>
>>>> Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>  drivers/usb/musb/musb_core.c | 17 ++++++++++++++++-
>>>>  include/linux/usb/musb.h     |  3 +++
>>>>  2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>>>> index 0a43329..a8ded57 100644
>>>> --- a/drivers/usb/musb/musb_core.c
>>>> +++ b/drivers/usb/musb/musb_core.c
>>>> @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev)
>>>>  {
>>>>  	struct musb	*musb = dev_to_musb(dev);
>>>>  	unsigned long	flags;
>>>> +	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
>>>
>>> we don't want to have platform_data on DT-based boot. It's best to just
>>> save those registers unconditionally as it doesn't hurt.
>>>
>>
>> My concern about doing it unconditionally from the core is simply that I
>> fear regressions for other platforms. I can of course drop it if you're
>> certain that that's correct.
>>
>> I can only test this on a dsps glue layer, and I have no documentation
>> for the musb core. All I'm left with here is fishing in muddy waters :/
> 
> I really don't think it should cause any issues with any other platform.
> 
> We can make to leave this soaking in linux-next for a long time and hope
> people test it.
> 

Ok, sounds good. I'll do it and repost, along with the other changes.


Many thanks,
Daniel


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Alan Stern]

On Mon, 25 Nov 2013, Felipe Balbi wrote:

> On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote:
> > Make musb_port_suspend() externally available, and call it when to host
> > goes into suspend. This allows the core to go into suspend while a
> > device is connected.

> have you considered the fact that when musb looses context it'll cause a
> disconnect on the bus because soft_connect bit is lost ?
> 
> What if you have a mounted file system on a pendrive ? Should we allow
> suspend in that case ?
> 
> Alan, Greg, any hints ?

An HCD should strive to avoid losing power sessions over a
suspend/resume.  But if this is unavoidable, the USB Persist mechanism
should be able to cope by issuing a reset-resume.  (Unless userspace
has turned Persist off, of course -- I believe some distributions do
this.  Chrome?)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Alan Stern]

On Mon, 25 Nov 2013, Daniel Mack wrote:

> >>> What if you have a mounted file system on a pendrive ? Should we allow
> >>> suspend in that case ?
> >>
> >> Well, I would have expected that, but in fact, the opposite is true.
> >> With 3.12, mounting a filesystem on a USB media and accessing it after
> >> resume was exactly my test case, and it worked just fine with that patch.
> > 
> > so you had a mounted file system, suspend, resume, it was still mounted?
> 
> Yes, exactly.
> 
> > or did it reenumerate and you remounted it ?
> 
> No, it did not reenumerate, but the core did a reset on it. See the
> following log.

...
> [   32.030792] usb 1-1: reset high-speed USB device number 2 using
> musb-hdrc <<<< !

Like I said, you get a reset-resume.

> > Try to do the same with transfers in flight, it's likely to corrupt your
> > file system.

It's not so easy to suspend a system with USB mass-storage transfers in 
flight.  For one thing, userspace gets frozen before the suspend 
starts, so there's nothing to initiate new I/O requests (except perhaps 
for dirty-block writebacks).

Also, the child SCSI device gets suspended before the USB device, so
its I/O queue stops running.  In addition, the usb-storage suspend
routine and the main I/O thread are mutually exclusive (they both grab
the private mutex), so a suspend can't occur while a SCSI command is
underway.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Daniel Mack]

On 11/25/2013 09:41 PM, Alan Stern wrote:
> On Mon, 25 Nov 2013, Daniel Mack wrote:
> 
>>>>> What if you have a mounted file system on a pendrive ? Should we allow
>>>>> suspend in that case ?
>>>>
>>>> Well, I would have expected that, but in fact, the opposite is true.
>>>> With 3.12, mounting a filesystem on a USB media and accessing it after
>>>> resume was exactly my test case, and it worked just fine with that patch.
>>>
>>> so you had a mounted file system, suspend, resume, it was still mounted?
>>
>> Yes, exactly.
>>
>>> or did it reenumerate and you remounted it ?
>>
>> No, it did not reenumerate, but the core did a reset on it. See the
>> following log.
> 
> ...
>> [   32.030792] usb 1-1: reset high-speed USB device number 2 using
>> musb-hdrc <<<< !
> 
> Like I said, you get a reset-resume.

Yup. We can't avoid losing VBUS on this platform, and even if we could,
we wouldn't want that, in order to save as much power as possible.

>>> Try to do the same with transfers in flight, it's likely to corrupt your
>>> file system.
> 
> It's not so easy to suspend a system with USB mass-storage transfers in 
> flight.  For one thing, userspace gets frozen before the suspend 
> starts, so there's nothing to initiate new I/O requests (except perhaps 
> for dirty-block writebacks).
> 
> Also, the child SCSI device gets suspended before the USB device, so
> its I/O queue stops running.  In addition, the usb-storage suspend
> routine and the main I/O thread are mutually exclusive (they both grab
> the private mutex), so a suspend can't occur while a SCSI command is
> underway.

Thanks Alan for the explanation. That was my understanding as well.


Best regards,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

Hi,

On Mon, Nov 25, 2013 at 03:41:14PM -0500, Alan Stern wrote:
> On Mon, 25 Nov 2013, Daniel Mack wrote:
> 
> > >>> What if you have a mounted file system on a pendrive ? Should we allow
> > >>> suspend in that case ?
> > >>
> > >> Well, I would have expected that, but in fact, the opposite is true.
> > >> With 3.12, mounting a filesystem on a USB media and accessing it after
> > >> resume was exactly my test case, and it worked just fine with that patch.
> > > 
> > > so you had a mounted file system, suspend, resume, it was still mounted?
> > 
> > Yes, exactly.
> > 
> > > or did it reenumerate and you remounted it ?
> > 
> > No, it did not reenumerate, but the core did a reset on it. See the
> > following log.
> 
> ...
> > [   32.030792] usb 1-1: reset high-speed USB device number 2 using
> > musb-hdrc <<<< !
> 
> Like I said, you get a reset-resume.
> 
> > > Try to do the same with transfers in flight, it's likely to corrupt your
> > > file system.
> 
> It's not so easy to suspend a system with USB mass-storage transfers in 
> flight.  For one thing, userspace gets frozen before the suspend 
> starts, so there's nothing to initiate new I/O requests (except perhaps 
> for dirty-block writebacks).
> 
> Also, the child SCSI device gets suspended before the USB device, so
> its I/O queue stops running.  In addition, the usb-storage suspend
> routine and the main I/O thread are mutually exclusive (they both grab
> the private mutex), so a suspend can't occur while a SCSI command is
> underway.

awesome, thanks for the explanation Alan :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

Hi,

On Mon, Nov 25, 2013 at 09:26:55PM +0100, Daniel Mack wrote:
> On 11/25/2013 09:08 PM, Felipe Balbi wrote:
> > On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
> >>>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> >>>> index e977441..24e46c0 100644
> >>>> --- a/drivers/usb/musb/musb_virthub.c
> >>>> +++ b/drivers/usb/musb/musb_virthub.c
> >>>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -static void musb_port_reset(struct musb *musb, bool do_reset)
> >>>> +void musb_port_reset(struct musb *musb, bool do_reset)
> >>>
> >>> NAK, this should not be called from the glue layers at all. If anything
> >>> call from musb_core's resume callback. That will only be called after
> >>> parent's resume has been called anyway.
> >>
> >> Given that this driver is successfully used with suspend/resume on other
> >> platforms without that reset, but it's clearly needed for dsps, I'm
> >> fairly sure this should be a glue-layer specific thing. Hence the change.
> >>
> >> I think it'll break things on other platforms if we unconditionally
> >> reset the port after resume, but I can't test it. Anyone?
> > 
> > your original commit log only says "we need to issue port reset" but it
> > never explains why. It could very well be you just found a device which
> > needs to be reset when coming out of suspend and it misses a quirk flag ?
> 
> I think I can really rule that out, as I tested with multiple USB sticks
> and also tested the same sticks on other embedded platforms.
> 
> > In any case, those functions should never be called by the glue, if
> > reset needs to be called, it must be called by musb-hdrc.ko, if you need
> > a flag, pass a flag but I need a really good explanation in your commit
> > log of why that's necessary.
> 
> Well, if I'd only knew exactly why. All these patches are really the
> result of many days of trial and error, with multiple drivers (musb,
> cppi41, ...) involved. And as I said - I have no documentation of the
> musb core itself.
> 
> So yes, I can do it the other way around and pass a flag, but what I
> can't provide is a good explanantion why the dsps glue behaves
> differently here than others. I'm curious myself, and all I know is that
> with this reset in place, things work as expected on AM33xx.

ok, then let's pass a flag. Meanwhile I'll try to figure out internally
why we need that reset. As Alan said, usb-persist should already for a
bus-reset when resuming, right ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Daniel Mack]

On 11/25/2013 09:01 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote:

>> In 3.13, something about the parition table reading seems to be broken
>> currently, I'll have a closer look.
> 
> cool

Ah, and forget about this one. I didn't look close enough. There's no
such issue in 3.13. Sorry for the noise.




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On Mon, Nov 25, 2013 at 09:47:16PM +0100, Daniel Mack wrote:
> On 11/25/2013 09:01 PM, Felipe Balbi wrote:
> > On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote:
> 
> >> In 3.13, something about the parition table reading seems to be broken
> >> currently, I'll have a closer look.
> > 
> > cool
> 
> Ah, and forget about this one. I didn't look close enough. There's no
> such issue in 3.13. Sorry for the noise.

no problem :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Daniel Mack]

On 11/25/2013 09:46 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 25, 2013 at 09:26:55PM +0100, Daniel Mack wrote:
>> On 11/25/2013 09:08 PM, Felipe Balbi wrote:
>>> On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
>>>>>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
>>>>>> index e977441..24e46c0 100644
>>>>>> --- a/drivers/usb/musb/musb_virthub.c
>>>>>> +++ b/drivers/usb/musb/musb_virthub.c
>>>>>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>>>>>>  	}
>>>>>>  }
>>>>>>  
>>>>>> -static void musb_port_reset(struct musb *musb, bool do_reset)
>>>>>> +void musb_port_reset(struct musb *musb, bool do_reset)
>>>>>
>>>>> NAK, this should not be called from the glue layers at all. If anything
>>>>> call from musb_core's resume callback. That will only be called after
>>>>> parent's resume has been called anyway.
>>>>
>>>> Given that this driver is successfully used with suspend/resume on other
>>>> platforms without that reset, but it's clearly needed for dsps, I'm
>>>> fairly sure this should be a glue-layer specific thing. Hence the change.
>>>>
>>>> I think it'll break things on other platforms if we unconditionally
>>>> reset the port after resume, but I can't test it. Anyone?
>>>
>>> your original commit log only says "we need to issue port reset" but it
>>> never explains why. It could very well be you just found a device which
>>> needs to be reset when coming out of suspend and it misses a quirk flag ?
>>
>> I think I can really rule that out, as I tested with multiple USB sticks
>> and also tested the same sticks on other embedded platforms.
>>
>>> In any case, those functions should never be called by the glue, if
>>> reset needs to be called, it must be called by musb-hdrc.ko, if you need
>>> a flag, pass a flag but I need a really good explanation in your commit
>>> log of why that's necessary.
>>
>> Well, if I'd only knew exactly why. All these patches are really the
>> result of many days of trial and error, with multiple drivers (musb,
>> cppi41, ...) involved. And as I said - I have no documentation of the
>> musb core itself.
>>
>> So yes, I can do it the other way around and pass a flag, but what I
>> can't provide is a good explanantion why the dsps glue behaves
>> differently here than others. I'm curious myself, and all I know is that
>> with this reset in place, things work as expected on AM33xx.
> 
> ok, then let's pass a flag. Meanwhile I'll try to figure out internally
> why we need that reset.

Well, we don't need a reset, I wasn't quite precise here. What we need
is to manually *de*assert the reset when we resume. Note the 'false'
flag that is passed to musb_port_reset().

> As Alan said, usb-persist should already for a
> bus-reset when resuming, right ?

The explicit un-reset is really needed, otherwise the port will
reenumerate the device, and a previously mounted filesystem will become
invalid of course. The log looks like this in that case:

[   17.045641] PM: resume of devices complete after 166.084 msecs
[   17.056461] PM: Sending message for resetting M3 state machine
[   17.063451] Restarting tasks ... [   17.071767] usb 1-1: USB
disconnect, device number 2
done.
[   17.403402] usb 1-1: new high-speed USB device number 3 using musb-hdrc
[   17.766959] usb 1-1: New USB device found, idVendor=058f, idProduct=6387
[   17.774849] usb 1-1: New USB device strings: Mfr=1, Product=2,
SerialNumber=3



Thanks,
Daniel

Re: [PATCH 4/5] usb: musb: dsps: add support for suspend and resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

Hi,

On Mon, Nov 25, 2013 at 09:54:56PM +0100, Daniel Mack wrote:
> > As Alan said, usb-persist should already for a
> > bus-reset when resuming, right ?
> 
> The explicit un-reset is really needed, otherwise the port will
> reenumerate the device, and a previously mounted filesystem will become
> invalid of course. The log looks like this in that case:
> 
> [   17.045641] PM: resume of devices complete after 166.084 msecs
> [   17.056461] PM: Sending message for resetting M3 state machine
> [   17.063451] Restarting tasks ... [   17.071767] usb 1-1: USB
> disconnect, device number 2
> done.
> [   17.403402] usb 1-1: new high-speed USB device number 3 using musb-hdrc
> [   17.766959] usb 1-1: New USB device found, idVendor=058f, idProduct=6387
> [   17.774849] usb 1-1: New USB device strings: Mfr=1, Product=2,
> SerialNumber=3

got it now ;-) Thanks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/5] usb: musb: conditionally save and restore the context on suspend

[Sergei Shtylyov]

Hello.

On 25-11-2013 23:39, Daniel Mack wrote:

> It appears not all platforms featuring a musb core need to save the musb
> core registers at suspend time and restore them on resume.

> The dsps platform does, however. So add a bit in struct
> musb_hdrc_platform_data to let platforms specify their need of such
> action being taken.

> Signed-off-by: Daniel Mack <zonque@gmail.com>
[...]

> diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h
> index eb50525..e5a581c 100644
> --- a/include/linux/usb/musb.h
> +++ b/include/linux/usb/musb.h
> @@ -99,6 +99,9 @@ struct musb_hdrc_platform_data {
>   	/* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */
>   	u8		mode;
>
> +	/* should the musb core restore registers after suspend? */
> +	u8		restore_after_suspend:1;
> +

    Better placement seems to be with 'extvbus' field which is also 1-bit -- 
that would save space in the structure.

WBR, Sergei

Re: [PATCH 1/5] usb: musb: conditionally save and restore the context on suspend

[Daniel Mack]

On 11/26/2013 11:50 AM, Sergei Shtylyov wrote:
> On 25-11-2013 23:39, Daniel Mack wrote:
> 
>> It appears not all platforms featuring a musb core need to save the musb
>> core registers at suspend time and restore them on resume.
> 
>> The dsps platform does, however. So add a bit in struct
>> musb_hdrc_platform_data to let platforms specify their need of such
>> action being taken.
> 
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
> [...]
> 
>> diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h
>> index eb50525..e5a581c 100644
>> --- a/include/linux/usb/musb.h
>> +++ b/include/linux/usb/musb.h
>> @@ -99,6 +99,9 @@ struct musb_hdrc_platform_data {
>>   	/* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */
>>   	u8		mode;
>>
>> +	/* should the musb core restore registers after suspend? */
>> +	u8		restore_after_suspend:1;
>> +
> 
>     Better placement seems to be with 'extvbus' field which is also 1-bit -- 
> that would save space in the structure.

That patch is deprecated, as things are now done unconditionally,
without consulting this flag.


Daniel