Patchwork [U-Boot,MPC837x,v2] Make it work again with USB.

login
register
mail settings
Submitter Kim Phillips
Date Dec. 13, 2010, 10:52 p.m.
Message ID <20101213165238.0b496e57.kim.phillips@freescale.com>
Download mbox | patch
Permalink /patch/75433/
State Changes Requested
Delegated to: Kim Phillips
Headers show

Comments

Kim Phillips - Dec. 13, 2010, 10:52 p.m.
On Mon, 29 Nov 2010 08:59:21 +0100
"Schwarz,Andre" <andre.schwarz@matrix-vision.de> wrote:

> Kim,
> 
> 
> > On Tue, 16 Nov 2010 13:37:50 +0100
> > Andre Schwarz <andre.schwarz@matrix-vision.de> wrote:
> >
> > > USB register range IMMR+0x00-0xff is reserved and hangs the CPU.
> > >
> > > Signed-off-by: Andre Schwarz <andre.schwarz@matrix-vision.de>
> > > ---
> >
> > nack, 837x has a usb controller at IMMR+0x23000. 
> yes - but offset 0x00-0xff is explicitly reserved regarding to the manual.
> Don't know whether it is a "should not" or "must not be touched".
>  
> All I can see is a CPU hang with arbiter event register reporting a timeout on
> 0xe0023000.
>  
> 
>   Check to see whether there is an invalid USB clock setting in the SCCR? 
> All clocks are turned on except SEC and 2nd TSEC.
>  
> After all USB is running fine with this patch, i.e. there can hardly be a
> missing clock.
> 
> 
> Please re-think you NAK.

afaik, 834x and 837x don't have any special USB settings in common, so,
this patch, at least in it's current form, is not on.

0xe0023500 should be the address of the config register being accessed
here; please check the code isn't accessing 0xe0023000, as you mention
above.

If that's correct, try something like the following so we can determine
what setting the USB controller didn't agree with:


Thanks,

Kim
Andre Schwarz - Feb. 28, 2011, 4:18 p.m.
Kim,

finally found some time to re-work my 8377 board code for submission.

sorry to bother you again, but I again stumbled over the discussed USB 
init issue :
>>> nack, 837x has a usb controller at IMMR+0x23000.
>> yes - but offset 0x00-0xff is explicitly reserved regarding to the manual.
>> Don't know whether it is a "should not" or "must not be touched".
>>   
>> All I can see is a CPU hang with arbiter event register reporting a timeout on
>> 0xe0023000.
>>   
>>
>>    Check to see whether there is an invalid USB clock setting in the SCCR? 
>> All clocks are turned on except SEC and 2nd TSEC.
>>   
>> After all USB is running fine with this patch, i.e. there can hardly be a
>> missing clock.
>>
>>
>> Please re-think you NAK.
> afaik, 834x and 837x don't have any special USB settings in common, so,
> this patch, at least in it's current form, is not on.
>
> 0xe0023500 should be the address of the config register being accessed
> here; please check the code isn't accessing 0xe0023000, as you mention
> above.

ok - this was some kind of misunderstanding.
ehci regs are based at immr + 0x23000 with the "config" pointing to 
offset 0x500 inside ehci.
This looks sane to me.

> If that's correct, try something like the following so we can determine
> what setting the USB controller didn't agree with:
>
> diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c
> index 7a1cae7..cbc4157 100644
> --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c
> +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c
> @@ -332,7 +332,7 @@ void cpu_init_f (volatile immap_t * im)
>   	struct usb_ehci *ehci = (struct usb_ehci *)CONFIG_SYS_FSL_USB_ADDR;
>
>   	/* Configure interface. */
> -	setbits_be32(&ehci->control, REFSEL_16MHZ | UTMI_PHY_EN);
MPC837x has only 2 working bits inside the control register :

Bit29: USB_EN -> should be set to 1 before USB can be used.
Bit31: ULPI_INT_EN -> enables an ULPI wake-up irq.

Both "REFSEL_16MHZ" and "UTMI_PHY_EN" are completely out of scope for 
MPC837x.

> +	setbits_be32(&ehci->control, 0);
>
>   	/* Wait for clock to stabilize */
This loop never returns on MPC837x because "PHY_CLK_VALID" isn't valid.
>   	do {    temp = __raw_readl(&ehci->control);
>                  udelay(1000);
>          } while (!(temp&  PHY_CLK_VALID));
>
I still wonder how there can be a single working MPC837x board with 
CONFIG_USB_EHCI_FSL set.

Some pending patches on your side ?
What kind of patch might get an ACK from your side ?
Kim Phillips - March 31, 2011, 1:14 a.m.
On Mon, 28 Feb 2011 17:18:38 +0100
Andre Schwarz <andre.schwarz@matrix-vision.de> wrote:

> sorry to bother you again, but I again stumbled over the discussed USB 
> init issue :
> >>> nack, 837x has a usb controller at IMMR+0x23000.
> >> yes - but offset 0x00-0xff is explicitly reserved regarding to the manual.
> >> Don't know whether it is a "should not" or "must not be touched".
> >>   
> >> All I can see is a CPU hang with arbiter event register reporting a timeout on
> >> 0xe0023000.
> >>   
> >>
> >>    Check to see whether there is an invalid USB clock setting in the SCCR? 
> >> All clocks are turned on except SEC and 2nd TSEC.
> >>   
> >> After all USB is running fine with this patch, i.e. there can hardly be a
> >> missing clock.
> >>
> >>
> >> Please re-think you NAK.
> > afaik, 834x and 837x don't have any special USB settings in common, so,
> > this patch, at least in it's current form, is not on.
> >
> > 0xe0023500 should be the address of the config register being accessed
> > here; please check the code isn't accessing 0xe0023000, as you mention
> > above.
> 
> ok - this was some kind of misunderstanding.
> ehci regs are based at immr + 0x23000 with the "config" pointing to 
> offset 0x500 inside ehci.
> This looks sane to me.

ok, as long as it's confirmed.

> > If that's correct, try something like the following so we can determine
> > what setting the USB controller didn't agree with:
> >
> > diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c
> > index 7a1cae7..cbc4157 100644
> > --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c
> > +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c
> > @@ -332,7 +332,7 @@ void cpu_init_f (volatile immap_t * im)
> >   	struct usb_ehci *ehci = (struct usb_ehci *)CONFIG_SYS_FSL_USB_ADDR;
> >
> >   	/* Configure interface. */
> > -	setbits_be32(&ehci->control, REFSEL_16MHZ | UTMI_PHY_EN);
> MPC837x has only 2 working bits inside the control register :
> 
> Bit29: USB_EN -> should be set to 1 before USB can be used.
> Bit31: ULPI_INT_EN -> enables an ULPI wake-up irq.
> 
> Both "REFSEL_16MHZ" and "UTMI_PHY_EN" are completely out of scope for 
> MPC837x.

that's why I'm suggesting we confirm that touching the REFSEL_16MHZ and
UTMI_PHY_EN bits aren't sending the 837x controller into oblivion - did
you test the patch?

> > +	setbits_be32(&ehci->control, 0);
> >
> >   	/* Wait for clock to stabilize */
> This loop never returns on MPC837x because "PHY_CLK_VALID" isn't valid.

right, we need to narrow down the reason for this.

> >   	do {    temp = __raw_readl(&ehci->control);
> >                  udelay(1000);
> >          } while (!(temp&  PHY_CLK_VALID));
> >
> I still wonder how there can be a single working MPC837x board with 
> CONFIG_USB_EHCI_FSL set.
> 
> Some pending patches on your side ?
> What kind of patch might get an ACK from your side ?

nothing that suggests 834x and 837x have any special USB settings in
common - because it's not true and therefore misleading.

Kim
Andre Schwarz - March 31, 2011, 7:52 a.m.
Kim,

> On Mon, 28 Feb 2011 17:18:38 +0100
> Andre Schwarz<andre.schwarz@matrix-vision.de>  wrote:
>
>> sorry to bother you again, but I again stumbled over the discussed USB
>> init issue :
>>>>> nack, 837x has a usb controller at IMMR+0x23000.
>>>> yes - but offset 0x00-0xff is explicitly reserved regarding to the manual.
>>>> Don't know whether it is a "should not" or "must not be touched".
>>>>
>>>> All I can see is a CPU hang with arbiter event register reporting a timeout on
>>>> 0xe0023000.
>>>>
>>>>
>>>>     Check to see whether there is an invalid USB clock setting in the SCCR?
>>>> All clocks are turned on except SEC and 2nd TSEC.
>>>>
>>>> After all USB is running fine with this patch, i.e. there can hardly be a
>>>> missing clock.
>>>>
>>>>
>>>> Please re-think you NAK.
>>> afaik, 834x and 837x don't have any special USB settings in common, so,
>>> this patch, at least in it's current form, is not on.
>>>
>>> 0xe0023500 should be the address of the config register being accessed
>>> here; please check the code isn't accessing 0xe0023000, as you mention
>>> above.
>> ok - this was some kind of misunderstanding.
>> ehci regs are based at immr + 0x23000 with the "config" pointing to
>> offset 0x500 inside ehci.
>> This looks sane to me.
> ok, as long as it's confirmed.
>
>>> If that's correct, try something like the following so we can determine
>>> what setting the USB controller didn't agree with:
>>>
>>> diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c
>>> index 7a1cae7..cbc4157 100644
>>> --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c
>>> +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c
>>> @@ -332,7 +332,7 @@ void cpu_init_f (volatile immap_t * im)
>>>    	struct usb_ehci *ehci = (struct usb_ehci *)CONFIG_SYS_FSL_USB_ADDR;
>>>
>>>    	/* Configure interface. */
>>> -	setbits_be32(&ehci->control, REFSEL_16MHZ | UTMI_PHY_EN);
>> MPC837x has only 2 working bits inside the control register :
>>
>> Bit29: USB_EN ->  should be set to 1 before USB can be used.
>> Bit31: ULPI_INT_EN ->  enables an ULPI wake-up irq.
>>
>> Both "REFSEL_16MHZ" and "UTMI_PHY_EN" are completely out of scope for
>> MPC837x.
> that's why I'm suggesting we confirm that touching the REFSEL_16MHZ and
> UTMI_PHY_EN bits aren't sending the 837x controller into oblivion - did
> you test the patch?

yes - writing those bits or not makes no difference ...

>>> +	setbits_be32(&ehci->control, 0);
>>>
>>>    	/* Wait for clock to stabilize */
>> This loop never returns on MPC837x because "PHY_CLK_VALID" isn't valid.
> right, we need to narrow down the reason for this.
>
... it is this loop that *can not* return since 
ehci->control[PHY_CLK_VALID] is always 0 on 837x.
This can be seen by having a look at the reference manual (Rev. 1 page 
20-46 / Chapter 20.3.2.28).

>>>    	do {    temp = __raw_readl(&ehci->control);
>>>                   udelay(1000);
>>>           } while (!(temp&   PHY_CLK_VALID));
>>>
>> I still wonder how there can be a single working MPC837x board with
>> CONFIG_USB_EHCI_FSL set.
>>
>> Some pending patches on your side ?
>> What kind of patch might get an ACK from your side ?
> nothing that suggests 834x and 837x have any special USB settings in
> common - because it's not true and therefore misleading.

I never ever said that 834x and 837x have anything in common regarding USB.
All I say is that they both must not run into this loop.

If you see any problems or'ing 837x into the #ifndef I suggest you come 
up with a positive
#ifdef being valid for only those chips that need it. Honestly I don't 
know which SoC's will need it.

All I want is to skip this loop on 837x.


Regards,
André


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Patch

diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c
index 7a1cae7..cbc4157 100644
--- a/arch/powerpc/cpu/mpc83xx/cpu_init.c
+++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c
@@ -332,7 +332,7 @@  void cpu_init_f (volatile immap_t * im)
 	struct usb_ehci *ehci = (struct usb_ehci *)CONFIG_SYS_FSL_USB_ADDR;
 
 	/* Configure interface. */
-	setbits_be32(&ehci->control, REFSEL_16MHZ | UTMI_PHY_EN);
+	setbits_be32(&ehci->control, 0);
 
 	/* Wait for clock to stabilize */
 	do {