[PATCH 1/3 v4] usb: chipidea: Reallocate regmap only if lpm is detected

[PATCH 1/3 v4] usb: chipidea: Reallocate regmap only if lpm is detected

[Chris Ruehl]

usb: chipidea: Reallocate regmap only if lpm is detected

The regmap only needs to reallocate if the hw_read on the CAP register shows
lpm is used. Therefore the if() statement check the change.

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
Acked-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/core.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 5d8981c..9a5ef20 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -208,7 +208,8 @@ static int hw_device_init(struct ci_hdrc *ci, void __iomem *base)
 	reg = hw_read(ci, CAP_HCCPARAMS, HCCPARAMS_LEN) >>
 		__ffs(HCCPARAMS_LEN);
 	ci->hw_bank.lpm  = reg;
-	hw_alloc_regmap(ci, !!reg);
+	if (reg)
+		hw_alloc_regmap(ci, !!reg);
 	ci->hw_bank.size = ci->hw_bank.op - ci->hw_bank.abs;
 	ci->hw_bank.size += OP_LAST;
 	ci->hw_bank.size /= sizeof(u32);
@@ -642,6 +643,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 			: CI_ROLE_GADGET;
 	}
 
+	/* only update vbus status for peripheral */
+	if (ci->role == CI_ROLE_GADGET)
+		ci_handle_vbus_change(ci);
+
 	ret = ci_role_start(ci, ci->role);
 	if (ret) {
 		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
-- 
1.7.10.4

[PATCH 2/3 v4] Fix Internal error: : 808 [#1] ARM related to STS flag

[Chris Ruehl]

Fix Internal error: : 808 [#1] ARM related to STS flag

* init the sts flag to 0 (missed)
* fix write the real bit not sts value
* Set PORTCS_STS and DEVLC_STS only if sts = 1
 (prefered solution by Mr. Peter Chen, Maintainer of ChipIdea subsystem)

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---
 drivers/usb/chipidea/core.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 9a5ef20..2834801 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -243,7 +243,7 @@ static int hw_device_init(struct ci_hdrc *ci, void __iomem *base)
 
 static void hw_phymode_configure(struct ci_hdrc *ci)
 {
-	u32 portsc, lpm, sts;
+	u32 portsc, lpm, sts = 0;
 
 	switch (ci->platdata->phy_mode) {
 	case USBPHY_INTERFACE_MODE_UTMI:
@@ -273,10 +273,12 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
 
 	if (ci->hw_bank.lpm) {
 		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
-		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
+		if (sts)
+			hw_write(ci, OP_DEVLC, DEVLC_STS, DEVLC_STS);
 	} else {
 		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
-		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
+		if (sts)
+			hw_write(ci, OP_PORTSC, PORTSC_STS, PORTSC_STS);
 	}
 }
 
-- 
1.7.10.4

[PATCH 3/3 v4] usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init

[Chris Ruehl]

usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init
hw_phymode_configure configures the PORTSC registers and allow the
following phy_inits to operate on the right parameters. This fix a problem
where the UPLI (ISP1504) could not detected, because the Viewport was not
available and read the viewport return 0's only.

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
Acked-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 2834801..43897dd 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -564,6 +564,8 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	hw_phymode_configure(ci);
+
 	ret = ci_usb_phy_init(ci);
 	if (ret) {
 		dev_err(dev, "unable to init phy: %d\n", ret);
@@ -581,8 +583,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 
 	ci_get_otg_capable(ci);
 
-	hw_phymode_configure(ci);
-
 	dr_mode = ci->platdata->dr_mode;
 	/* initialize role(s) before the interrupt is requested */
 	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
-- 
1.7.10.4

Re: [PATCH 1/3 v4] usb: chipidea: Reallocate regmap only if lpm is detected

[Marc Kleine-Budde]

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

On 12/03/2013 09:01 AM, Chris Ruehl wrote:
> usb: chipidea: Reallocate regmap only if lpm is detected
> 
> The regmap only needs to reallocate if the hw_read on the CAP register shows
> lpm is used. Therefore the if() statement check the change.
> 
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> Acked-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/core.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 5d8981c..9a5ef20 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -208,7 +208,8 @@ static int hw_device_init(struct ci_hdrc *ci, void __iomem *base)
>  	reg = hw_read(ci, CAP_HCCPARAMS, HCCPARAMS_LEN) >>
>  		__ffs(HCCPARAMS_LEN);
>  	ci->hw_bank.lpm  = reg;
> -	hw_alloc_regmap(ci, !!reg);
> +	if (reg)
> +		hw_alloc_regmap(ci, !!reg);
>  	ci->hw_bank.size = ci->hw_bank.op - ci->hw_bank.abs;
>  	ci->hw_bank.size += OP_LAST;
>  	ci->hw_bank.size /= sizeof(u32);
> @@ -642,6 +643,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  			: CI_ROLE_GADGET;
>  	}
>  
> +	/* only update vbus status for peripheral */
> +	if (ci->role == CI_ROLE_GADGET)
> +		ci_handle_vbus_change(ci);
> +

This change seems unrelated to me.

Marc

>  	ret = ci_role_start(ci, ci->role);
>  	if (ret) {
>  		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
> 


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

RE: [PATCH 1/3 v4] usb: chipidea: Reallocate regmap only if lpm is detected

[Peter Chen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1794 bytes --]


 
> >
> > Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> > Acked-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  drivers/usb/chipidea/core.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 5d8981c..9a5ef20 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -208,7 +208,8 @@ static int hw_device_init(struct ci_hdrc *ci, void
> __iomem *base)
> >  	reg = hw_read(ci, CAP_HCCPARAMS, HCCPARAMS_LEN) >>
> >  		__ffs(HCCPARAMS_LEN);
> >  	ci->hw_bank.lpm  = reg;
> > -	hw_alloc_regmap(ci, !!reg);
> > +	if (reg)
> > +		hw_alloc_regmap(ci, !!reg);
> >  	ci->hw_bank.size = ci->hw_bank.op - ci->hw_bank.abs;
> >  	ci->hw_bank.size += OP_LAST;
> >  	ci->hw_bank.size /= sizeof(u32);
> > @@ -642,6 +643,10 @@ static int ci_hdrc_probe(struct platform_device
> *pdev)
> >  			: CI_ROLE_GADGET;
> >  	}
> >
> > +	/* only update vbus status for peripheral */
> > +	if (ci->role == CI_ROLE_GADGET)
> > +		ci_handle_vbus_change(ci);
> > +
> 
> This change seems unrelated to me.
> 
> Marc

Hi Chris. Why above change is there, it is not existed at your v3 patch.

Peter

> 
> >  	ret = ci_role_start(ci, ci->role);
> >  	if (ret) {
> >  		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
> >
> 
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 3/3 v4] usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init

[Peter Chen]

 
> 
> usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init

usb: chipidea: put hw_phymode_configure before ci_usb_phy_init
Besides, a blank line is needed between your subject and commit log.

> hw_phymode_configure configures the PORTSC registers and allow the
> following phy_inits to operate on the right parameters. This fix a
> problem
> where the UPLI (ISP1504) could not detected,
					    ^^be

> because the Viewport was not
> available and read the viewport return 0's only.
> 
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> Acked-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/core.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 2834801..43897dd 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -564,6 +564,8 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
> 
> +	hw_phymode_configure(ci);
> +
>  	ret = ci_usb_phy_init(ci);
>  	if (ret) {
>  		dev_err(dev, "unable to init phy: %d\n", ret);
> @@ -581,8 +583,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> 
>  	ci_get_otg_capable(ci);
> 
> -	hw_phymode_configure(ci);
> -
>  	dr_mode = ci->platdata->dr_mode;
>  	/* initialize role(s) before the interrupt is requested */
>  	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> --
> 1.7.10.4
> 

Re: [PATCH 1/3 v4] usb: chipidea: Reallocate regmap only if lpm is detected

[Chris Ruehl]

>> @@ -642,6 +643,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>>   			: CI_ROLE_GADGET;
>>   	}
>>
>> +	/* only update vbus status for peripheral */
>> +	if (ci->role == CI_ROLE_GADGET)
>> +		ci_handle_vbus_change(ci);
>> +
>
> This change seems unrelated to me.
>
> Marc

My fault I test a patch from Fabio on the wrong working branch. I must be
more careful.

Chris

Regression on next-20140116 [Was: [PATCH 3/3 v4] usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init]

[Uwe Kleine-König]

Hello,

On Tue, Dec 03, 2013 at 04:01:50PM +0800, Chris Ruehl wrote:
> usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init
> hw_phymode_configure configures the PORTSC registers and allow the
> following phy_inits to operate on the right parameters. This fix a problem
> where the UPLI (ISP1504) could not detected, because the Viewport was not
> available and read the viewport return 0's only.
This patch (or a later revision of it to be more exact) made it into
mainline as cd0b42c2a6d2.

On an i.MX27 based machine I'm hitting an oops (see below) on
next-20140116 + a few patches. (I didn't switch to 3.13+ yet, as I think
not everything I need has landed there.) The oops goes away (and still
better, lsusb reports my connected devices instead of "unable to
initialize libusb: -99") when I do at least one of the following:

 - set CONFIG_USB_CHIPIDEA=y instead of =m
 - revert commit
      cd0b42c2a6d2 (usb: chipidea: put hw_phymode_configure before ci_usb_phy_init)

The few patches I mentioned above are:
- device tree stuff to support my machine inclusive pinmuxing
- ulpi and cs support for drivers/usb/phy/phy-generic.c
  (I got these from Chris Ruehl by PM, don't know if they were sent
  out in public already.)

Unhandled fault: external abort on non-linefetch (0x808) at 0xf4424584
Internal error: : 808 [#1] PREEMPT ARM
Modules linked in: ci_hdrc_imx(+) ci_hdrc ehci_hcd usbcore usb_common usbmisc_imx
CPU: 0 PID: 354 Comm: systemd-udevd Not tainted 3.13.0-rc8-next-20140116-00005-gf6e6ae599f37 #45
task: c389e0e0 ti: c3ab0000 task.ti: c3ab0000
PC is at ci_hdrc_probe+0x320/0x794 [ci_hdrc]
LR is at ci_hdrc_probe+0x310/0x794 [ci_hdrc]
pc : [<bf08970c>]    lr : [<bf0896fc>]    psr: 60000013
sp : c3ab1c48  ip : c041f1fc  fp : c3ab0030
r10: c3b2b9c0  r9 : c3a97000  r8 : 40000000
r7 : 8c000400  r6 : 00000000  r5 : c3a97010  r4 : c3a14010
r3 : f4424584  r2 : 00000000  r1 : 00000004  r0 : 00000024
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0005317f  Table: a3ac8000  DAC: 00000015
Process systemd-udevd (pid: 354, stack limit = 0xc3ab01c0)
Stack: (0xc3ab1c48 to 0xc3ab2000)
1c40:                   d2000000 80000000 c3a97010 bf08b734 bf08b734 c389dc10
1c60: c0876958 00000000 c388d9c0 c01777b8 c01777a0 c3a97010 00000000 c0176440
1c80: 00000000 c3a97010 c0176588 c389dc10 00000000 c0174d74 c38214d8 c39c5454
1ca0: c3a97010 c3a97044 c3a97010 c017635c c3a97010 c036c638 c3a97010 c0175a60
1cc0: c3a97010 c3a97018 00000000 c01741ac 00000000 00000002 c3ab0030 c0248f10
1ce0: c035d1fc c3a97000 00000002 0000001c c3ab1d44 00000000 00000002 c0177628
1d00: 10024400 00000000 00000000 c3a97000 c3ab1d44 bf089c84 c389dc10 c3ad19b0
1d20: c389dc00 00000000 00000000 bf099610 00000000 bf0992e0 c3ab1d68 00000000
1d40: c00e6638 bf099614 00000100 00000000 c388b990 00000003 0000000a 00000001
1d60: 00000000 00000000 c3f79604 00000001 00000002 00000004 c3ad2dc0 c02e57da
1d80: c388f948 c3ad2d10 00000001 00000001 c389dc10 bf0997b8 bf0997b8 00000000
1da0: c0876958 00000001 00000000 c01777b8 c01777a0 c389dc10 00000000 c0176440
1dc0: c389dc10 c389dc44 bf0997b8 00000000 bf09b000 c017663c 00000000 bf0997b8
1de0: c01765cc c0174cd8 c38214a8 c3894750 bf0997b8 c39c5280 c036c638 c0175ca0
1e00: bf099778 00000350 00000000 bf0997b8 00000001 bf0997fc c3a386c0 c0176a30
1e20: 00000000 c3ab1f58 00000001 c0008910 c3ab0010 00000001 c0248a68 00000001
1e40: 00000001 c3a386f0 c086ad71 c0042c8c 60000013 c0361198 ffffffff bf0997fc
1e60: c086ad71 c0248a74 c03611b8 c0361194 00000000 c0037014 00000000 00000001
1e80: c3ab1f58 c3ab1f58 00000001 bf0997fc c3a386c0 bf099844 00000001 c3a386f0
1ea0: c086ad71 c0061764 bf099808 00007fff c005f2f4 bf000000 00000000 c005f480
1ec0: 00000000 bf099808 c3ab0028 bf09993c 00000000 c49a81a8 c3ab1eec b6eb68b4
1ee0: c49a7000 00001c74 007c1889 00000000 0000000c 00000000 00000000 00000000
1f00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1f20: 00000000 00000000 00000000 00000000 00000010 00000000 b6eb68b4 00000006
1f40: 0000017b c000dda4 c3ab0000 00000000 00587ca8 c0061e54 c49a7000 00001c74
1f60: c49a7cd0 c49a7b79 c49a8718 0000094c 00000aac 00000000 00000000 00000000
1f80: 0000001f 00000020 00000016 00000000 00000012 00000000 00020000 00000000
1fa0: 00000000 c000dbe0 00020000 00000000 00000006 b6eb68b4 00000000 00000000
1fc0: 00020000 00000000 00000000 0000017b 00000000 00000000 0002d208 00587ca8
1fe0: bebd4ad8 bebd4ac8 b6eaec0c b6e2ca40 60000010 00000006 00000a6f e5d30014
[<bf08970c>] (ci_hdrc_probe [ci_hdrc]) from [<c01777b8>] (platform_drv_probe+0x18/0x48)
[<c01777b8>] (platform_drv_probe) from [<c0176440>] (driver_probe_device+0xb0/0x1f8)
[<c0176440>] (driver_probe_device) from [<c0174d74>] (bus_for_each_drv+0x74/0x88)
[<c0174d74>] (bus_for_each_drv) from [<c017635c>] (device_attach+0x6c/0x84)
[<c017635c>] (device_attach) from [<c0175a60>] (bus_probe_device+0x28/0xa0)
[<c0175a60>] (bus_probe_device) from [<c01741ac>] (device_add+0x3f8/0x4d4)
[<c01741ac>] (device_add) from [<c0177628>] (platform_device_add+0x128/0x1bc)
[<c0177628>] (platform_device_add) from [<bf089c84>] (ci_hdrc_add_device+0x104/0x140 [ci_hdrc])
[<bf089c84>] (ci_hdrc_add_device [ci_hdrc]) from [<bf0992e0>] (ci_hdrc_imx_probe+0x2ac/0x360 [ci_hdrc_imx])
[<bf0992e0>] (ci_hdrc_imx_probe [ci_hdrc_imx]) from [<c01777b8>] (platform_drv_probe+0x18/0x48)
[<c01777b8>] (platform_drv_probe) from [<c0176440>] (driver_probe_device+0xb0/0x1f8)
[<c0176440>] (driver_probe_device) from [<c017663c>] (__driver_attach+0x70/0x94)
[<c017663c>] (__driver_attach) from [<c0174cd8>] (bus_for_each_dev+0x70/0x84)
[<c0174cd8>] (bus_for_each_dev) from [<c0175ca0>] (bus_add_driver+0xd8/0x1cc)
[<c0175ca0>] (bus_add_driver) from [<c0176a30>] (driver_register+0x9c/0xe0)
[<c0176a30>] (driver_register) from [<c0008910>] (do_one_initcall+0x94/0x140)
[<c0008910>] (do_one_initcall) from [<c0061764>] (load_module+0x13dc/0x1998)
[<c0061764>] (load_module) from [<c0061e54>] (SyS_finit_module+0x5c/0x6c)
[<c0061e54>] (SyS_finit_module) from [<c000dbe0>] (ret_fast_syscall+0x0/0x44)
Code: e5d42e52 e5943060 e3520000 1a000000 (e5837000)
---[ end trace f02dcc94b8467706 ]---

bf08970c is the str instruction of

	hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);

in hw_phymode_configure().

Any ideas to fix that apart from reverting the commit above (or changing
USB_CHIPIDEA to bool :-)?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: Regression on next-20140116 [Was: [PATCH 3/3 v4] usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init]

[Uwe Kleine-König]

Hello,

On Wed, Jan 22, 2014 at 10:49:51AM +0100, Uwe Kleine-König wrote:
> On Tue, Dec 03, 2013 at 04:01:50PM +0800, Chris Ruehl wrote:
> > usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init
> > hw_phymode_configure configures the PORTSC registers and allow the
> > following phy_inits to operate on the right parameters. This fix a problem
> > where the UPLI (ISP1504) could not detected, because the Viewport was not
> > available and read the viewport return 0's only.
> This patch (or a later revision of it to be more exact) made it into
> mainline as cd0b42c2a6d2.
> 
> On an i.MX27 based machine I'm hitting an oops (see below) on
> next-20140116 + a few patches. (I didn't switch to 3.13+ yet, as I think
> not everything I need has landed there.) The oops goes away (and still
> better, lsusb reports my connected devices instead of "unable to
> initialize libusb: -99") when I do at least one of the following:
> 
>  - set CONFIG_USB_CHIPIDEA=y instead of =m
>  - revert commit
>       cd0b42c2a6d2 (usb: chipidea: put hw_phymode_configure before ci_usb_phy_init)
I debugged that a bit further and the problem is that
hw_phymode_configure depends on the phy's clk being enabled (i.e.
usb_ipg_gate) and this is only enforced in ci_usb_phy_init (via
usb_phy_init -> usb_gen_phy_init). When CONFIG_USB_CHIPIDEA=y the init
call to disable all unused clocks wasn't run yet and so the clock is
still on as this is the boot default.

Considering that it's already late today and that I don't know the
chipidea driver I'm sure there are people who can come up with a better
patch with less effort than me. Any volunteers?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: Regression on next-20140116 [Was: [PATCH 3/3 v4] usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init]

[Peter Chen]

On Wed, Jan 22, 2014 at 10:41:33PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Jan 22, 2014 at 10:49:51AM +0100, Uwe Kleine-König wrote:
> > On Tue, Dec 03, 2013 at 04:01:50PM +0800, Chris Ruehl wrote:
> > > usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init
> > > hw_phymode_configure configures the PORTSC registers and allow the
> > > following phy_inits to operate on the right parameters. This fix a problem
> > > where the UPLI (ISP1504) could not detected, because the Viewport was not
> > > available and read the viewport return 0's only.
> > This patch (or a later revision of it to be more exact) made it into
> > mainline as cd0b42c2a6d2.
> > 
> > On an i.MX27 based machine I'm hitting an oops (see below) on
> > next-20140116 + a few patches. (I didn't switch to 3.13+ yet, as I think
> > not everything I need has landed there.) The oops goes away (and still
> > better, lsusb reports my connected devices instead of "unable to
> > initialize libusb: -99") when I do at least one of the following:
> > 
> >  - set CONFIG_USB_CHIPIDEA=y instead of =m
> >  - revert commit
> >       cd0b42c2a6d2 (usb: chipidea: put hw_phymode_configure before ci_usb_phy_init)
> I debugged that a bit further and the problem is that
> hw_phymode_configure depends on the phy's clk being enabled (i.e.
> usb_ipg_gate) and this is only enforced in ci_usb_phy_init (via
> usb_phy_init -> usb_gen_phy_init). When CONFIG_USB_CHIPIDEA=y the init
> call to disable all unused clocks wasn't run yet and so the clock is
> still on as this is the boot default.

Hi Uwe,
I am a little puzzled at your platform

- Which phy you have used? ulpi phy ,internal phy or other external phy?
- If you use ulpi phy, why you still need to use nop phy driver?
 Besides, according to chris patch, the ulpi can only be visited after
hw_phymode_configure?
- Do you have some hardware related operation at phy's probe? If it exists,
why not move it to phy->init?

Peter

> 
> Considering that it's already late today and that I don't know the
> chipidea driver I'm sure there are people who can come up with a better
> patch with less effort than me. Any volunteers?
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 
> 

-- 

Best Regards,
Peter Chen

Re: Regression on next-20140116 [Was: [PATCH 3/3 v4] usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init]

[Chris Ruehl]

On Thursday, January 23, 2014 09:22 AM, Peter Chen wrote:
> On Wed, Jan 22, 2014 at 10:41:33PM +0100, Uwe Kleine-König wrote:
>> Hello,
>>
>> On Wed, Jan 22, 2014 at 10:49:51AM +0100, Uwe Kleine-König wrote:
>>> On Tue, Dec 03, 2013 at 04:01:50PM +0800, Chris Ruehl wrote:
>>>> usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init
>>>> hw_phymode_configure configures the PORTSC registers and allow the
>>>> following phy_inits to operate on the right parameters. This fix a problem
>>>> where the UPLI (ISP1504) could not detected, because the Viewport was not
>>>> available and read the viewport return 0's only.
>>> This patch (or a later revision of it to be more exact) made it into
>>> mainline as cd0b42c2a6d2.
>>>
>>> On an i.MX27 based machine I'm hitting an oops (see below) on
>>> next-20140116 + a few patches. (I didn't switch to 3.13+ yet, as I think
>>> not everything I need has landed there.) The oops goes away (and still
>>> better, lsusb reports my connected devices instead of "unable to
>>> initialize libusb: -99") when I do at least one of the following:
>>>
>>>   - set CONFIG_USB_CHIPIDEA=y instead of =m
>>>   - revert commit
>>>        cd0b42c2a6d2 (usb: chipidea: put hw_phymode_configure before ci_usb_phy_init)
>> I debugged that a bit further and the problem is that
>> hw_phymode_configure depends on the phy's clk being enabled (i.e.
>> usb_ipg_gate) and this is only enforced in ci_usb_phy_init (via
>> usb_phy_init -> usb_gen_phy_init). When CONFIG_USB_CHIPIDEA=y the init
>> call to disable all unused clocks wasn't run yet and so the clock is
>> still on as this is the boot default.
> Hi Uwe,
> I am a little puzzled at your platform
>
> - Which phy you have used? ulpi phy ,internal phy or other external phy?
> - If you use ulpi phy, why you still need to use nop phy driver?
>   Besides, according to chris patch, the ulpi can only be visited after
> hw_phymode_configure?
> - Do you have some hardware related operation at phy's probe? If it exists,
> why not move it to phy->init?
>
> Peter
Peter,
I think thats my fault, I send Uwe my patches which call the phy-ulpi 
from the nop driver
in order to get the ISP1504 running with my board.

Its obversely wrong to call an other driver from the nop
see:  [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support and the 
concerns from
Heikki (mail-list linux-usb)

Uwe we may work together on this.

Chris

>> Considering that it's already late today and that I don't know the
>> chipidea driver I'm sure there are people who can come up with a better
>> patch with less effort than me. Any volunteers?
>>
>> Best regards
>> Uwe
>>
>> -- 
>> Pengutronix e.K.                           | Uwe Kleine-König            |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>>
>>

Re: Regression on next-20140116 [Was: [PATCH 3/3 v4] usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init]

[Uwe Kleine-König]

Hello Peter,

On Thu, Jan 23, 2014 at 09:22:41AM +0800, Peter Chen wrote:
> On Wed, Jan 22, 2014 at 10:41:33PM +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 22, 2014 at 10:49:51AM +0100, Uwe Kleine-König wrote:
> > > On Tue, Dec 03, 2013 at 04:01:50PM +0800, Chris Ruehl wrote:
> > > > usb: chipidea: hw_phymode_configure moved before ci_usb_phy_init
> > > > hw_phymode_configure configures the PORTSC registers and allow the
> > > > following phy_inits to operate on the right parameters. This fix a problem
> > > > where the UPLI (ISP1504) could not detected, because the Viewport was not
> > > > available and read the viewport return 0's only.
> > > This patch (or a later revision of it to be more exact) made it into
> > > mainline as cd0b42c2a6d2.
> > > 
> > > On an i.MX27 based machine I'm hitting an oops (see below) on
> > > next-20140116 + a few patches. (I didn't switch to 3.13+ yet, as I think
> > > not everything I need has landed there.) The oops goes away (and still
> > > better, lsusb reports my connected devices instead of "unable to
> > > initialize libusb: -99") when I do at least one of the following:
> > > 
> > >  - set CONFIG_USB_CHIPIDEA=y instead of =m
> > >  - revert commit
> > >       cd0b42c2a6d2 (usb: chipidea: put hw_phymode_configure before ci_usb_phy_init)
> > I debugged that a bit further and the problem is that
> > hw_phymode_configure depends on the phy's clk being enabled (i.e.
> > usb_ipg_gate) and this is only enforced in ci_usb_phy_init (via
> > usb_phy_init -> usb_gen_phy_init). When CONFIG_USB_CHIPIDEA=y the init
> > call to disable all unused clocks wasn't run yet and so the clock is
> > still on as this is the boot default.
> 
> I am a little puzzled at your platform
> 
> - Which phy you have used? ulpi phy ,internal phy or other external phy?
I'm using an ISP1504 (i.e. ULPI) phy.

> - If you use ulpi phy, why you still need to use nop phy driver?
I don't need to, that's just how I made it running. If you have a better
suggestion, don't hesitate to tell me. :-)

>  Besides, according to chris patch, the ulpi can only be visited after
> hw_phymode_configure?
It seems so, yes. For writing the PORTSC register the usb_ipg_gate clock
must be on. hw_phymode_configure writes the register and ci_usb_phy_init
enables the clock.

> - Do you have some hardware related operation at phy's probe? If it exists,
> why not move it to phy->init?
I guess that would work and the culprit is that I just took Chris' patch
because I don't have a clue about USB.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |