[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Jagan Teki]

During usb shutdown or 'usb reset' all the necessary clocks
on the specific controller will disable. Usually this shutdown
happen during U-Boot proper handoff to Linux.

There is an issue in Allwinner A64, is during OHCI1 shutdown
the controller is unable to access the register space
so the Linux boot hangs at this place.

This particular condition occur when we enable both the
controllers, so this patch will disable OHCI1 and EHCI1 for
bananapi-m64 and nanopi-a64 boards. It will re-enable the same
once the issue got fixed.

Log:
=> usb reset
resetting USB...

PHY0: mask = 0x101, usb_clk_cfg = 0x30202
sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
EHCI failed to shut down host controller.
ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202

PHY1: mask = 0x202, usb_clk_cfg = 0x0
ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540

<< hang here >>

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Note:
Reason for this patch, since we have week to release.
Hopefully this will fix in coming releases.

debugging:
=========
Allwinner A64 share common PHY between OTG & Host controller,
so it's job of OTG to shutdown the controller. But unfortunately
there is no shutdown call from command(usb reset) or handoff code
for OTG controller in gadget mode, UCLASS_USB_DEV_GENERIC.

So, we added glue code to shutdown musb, and it's shutdown
propely.

Then we found an issue of disabling OHCI1 gate clock
during OHCI0 shutdown. ohci driver is trying to clear BIT(16)
for OHCI0, but it also clears BIT(17), which is for OHCI1.
fortunately this is resolved when we clear usb_clk_cfg after
reset0_cfg and ahb_gate0, but OHCI1 still hang.

so, we still need to figure it out. Any help appreciated.

Above log reproduced on
https://github.com/amarula/u-boot-amarula/tree/sun-dev
repo since it has some fixes and improvements wrt mainline code.

 arch/arm/dts/sun50i-a64-bananapi-m64.dts | 8 --------
 arch/arm/dts/sun50i-a64-nanopi-a64.dts   | 8 --------
 2 files changed, 16 deletions(-)

diff --git a/arch/arm/dts/sun50i-a64-bananapi-m64.dts b/arch/arm/dts/sun50i-a64-bananapi-m64.dts
index dcde4a4881..32e3402998 100644
--- a/arch/arm/dts/sun50i-a64-bananapi-m64.dts
+++ b/arch/arm/dts/sun50i-a64-bananapi-m64.dts
@@ -72,10 +72,6 @@
 	status = "okay";
 };
 
-&ehci1 {
-	status = "okay";
-};
-
 &i2c1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c1_pins>;
@@ -120,10 +116,6 @@
 	status = "okay";
 };
 
-&ohci1 {
-	status = "okay";
-};
-
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
diff --git a/arch/arm/dts/sun50i-a64-nanopi-a64.dts b/arch/arm/dts/sun50i-a64-nanopi-a64.dts
index 778636c73a..ba36944caa 100644
--- a/arch/arm/dts/sun50i-a64-nanopi-a64.dts
+++ b/arch/arm/dts/sun50i-a64-nanopi-a64.dts
@@ -70,10 +70,6 @@
 	status = "okay";
 };
 
-&ehci1 {
-	status = "okay";
-};
-
 /* i2c1 connected with gpio headers like pine64, bananapi */
 &i2c1 {
 	pinctrl-names = "default";
@@ -100,10 +96,6 @@
 	status = "okay";
 };
 
-&ohci1 {
-	status = "okay";
-};
-
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
-- 
2.17.1

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Marek Vasut]

On 07/02/2018 10:53 PM, Jagan Teki wrote:
> During usb shutdown or 'usb reset' all the necessary clocks
> on the specific controller will disable. Usually this shutdown
> happen during U-Boot proper handoff to Linux.

No, 'usb reset' can be triggered by the user any time.

> There is an issue in Allwinner A64, is during OHCI1 shutdown
> the controller is unable to access the register space
> so the Linux boot hangs at this place.

This doesn't make any sense, Linux should enable the necessary clock
before accessing any controller registers, unless there is a bug in Linux.

> This particular condition occur when we enable both the
> controllers, so this patch will disable OHCI1 and EHCI1 for
> bananapi-m64 and nanopi-a64 boards. It will re-enable the same
> once the issue got fixed.
> 
> Log:
> => usb reset
> resetting USB...
> 
> PHY0: mask = 0x101, usb_clk_cfg = 0x30202
> sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
> EHCI failed to shut down host controller.
> ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
> ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
> ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
> 
> PHY1: mask = 0x202, usb_clk_cfg = 0x0
> ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540

Why is this usb reset so noisy ?

> << hang here >>

Please fix this properly, this patch is pure attempt to hide a bug.
NAK from me.

> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Note:
> Reason for this patch, since we have week to release.
> Hopefully this will fix in coming releases.
> 
> debugging:
> =========
> Allwinner A64 share common PHY between OTG & Host controller,
> so it's job of OTG to shutdown the controller. But unfortunately
> there is no shutdown call from command(usb reset) or handoff code
> for OTG controller in gadget mode, UCLASS_USB_DEV_GENERIC.
> 
> So, we added glue code to shutdown musb, and it's shutdown
> propely.
> 
> Then we found an issue of disabling OHCI1 gate clock
> during OHCI0 shutdown. ohci driver is trying to clear BIT(16)
> for OHCI0, but it also clears BIT(17), which is for OHCI1.
> fortunately this is resolved when we clear usb_clk_cfg after
> reset0_cfg and ahb_gate0, but OHCI1 still hang.
> 
> so, we still need to figure it out. Any help appreciated.
> 
> Above log reproduced on
> https://github.com/amarula/u-boot-amarula/tree/sun-dev
> repo since it has some fixes and improvements wrt mainline code.
> 
>  arch/arm/dts/sun50i-a64-bananapi-m64.dts | 8 --------
>  arch/arm/dts/sun50i-a64-nanopi-a64.dts   | 8 --------
>  2 files changed, 16 deletions(-)
> 
> diff --git a/arch/arm/dts/sun50i-a64-bananapi-m64.dts b/arch/arm/dts/sun50i-a64-bananapi-m64.dts
> index dcde4a4881..32e3402998 100644
> --- a/arch/arm/dts/sun50i-a64-bananapi-m64.dts
> +++ b/arch/arm/dts/sun50i-a64-bananapi-m64.dts
> @@ -72,10 +72,6 @@
>  	status = "okay";
>  };
>  
> -&ehci1 {
> -	status = "okay";
> -};
> -
>  &i2c1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&i2c1_pins>;
> @@ -120,10 +116,6 @@
>  	status = "okay";
>  };
>  
> -&ohci1 {
> -	status = "okay";
> -};
> -
>  &uart0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&uart0_pins_a>;
> diff --git a/arch/arm/dts/sun50i-a64-nanopi-a64.dts b/arch/arm/dts/sun50i-a64-nanopi-a64.dts
> index 778636c73a..ba36944caa 100644
> --- a/arch/arm/dts/sun50i-a64-nanopi-a64.dts
> +++ b/arch/arm/dts/sun50i-a64-nanopi-a64.dts
> @@ -70,10 +70,6 @@
>  	status = "okay";
>  };
>  
> -&ehci1 {
> -	status = "okay";
> -};
> -
>  /* i2c1 connected with gpio headers like pine64, bananapi */
>  &i2c1 {
>  	pinctrl-names = "default";
> @@ -100,10 +96,6 @@
>  	status = "okay";
>  };
>  
> -&ohci1 {
> -	status = "okay";
> -};
> -
>  &uart0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&uart0_pins_a>;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Tom Rini]

On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
> On 07/02/2018 10:53 PM, Jagan Teki wrote:
> > During usb shutdown or 'usb reset' all the necessary clocks
> > on the specific controller will disable. Usually this shutdown
> > happen during U-Boot proper handoff to Linux.
> 
> No, 'usb reset' can be triggered by the user any time.

Yes, and it's also triggered as part of the hand-off, which is the use
case in question.

> > There is an issue in Allwinner A64, is during OHCI1 shutdown
> > the controller is unable to access the register space
> > so the Linux boot hangs at this place.
> 
> This doesn't make any sense, Linux should enable the necessary clock
> before accessing any controller registers, unless there is a bug in Linux.

Should but doesn't always?  So yes, there's possibly / hopefully a bug
in the dts files.

> > This particular condition occur when we enable both the
> > controllers, so this patch will disable OHCI1 and EHCI1 for
> > bananapi-m64 and nanopi-a64 boards. It will re-enable the same
> > once the issue got fixed.
> > 
> > Log:
> > => usb reset
> > resetting USB...
> > 
> > PHY0: mask = 0x101, usb_clk_cfg = 0x30202
> > sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
> > EHCI failed to shut down host controller.
> > ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
> > ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
> > ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
> > 
> > PHY1: mask = 0x202, usb_clk_cfg = 0x0
> > ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
> 
> Why is this usb reset so noisy ?

... I would assume additional debug messages.

> > << hang here >>
> 
> Please fix this properly, this patch is pure attempt to hide a bug.
> NAK from me.

Well, the point of this patch as Jagan says is to hide the bug for the
release so that Linux can boot, which is an important case.

Jagan, can you bisect down to when this started happening?  I assume
it's a recent'ish thing.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180702/de99cdd4/attachment.sig>

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Marek Vasut]

On 07/02/2018 11:40 PM, Tom Rini wrote:
> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>> During usb shutdown or 'usb reset' all the necessary clocks
>>> on the specific controller will disable. Usually this shutdown
>>> happen during U-Boot proper handoff to Linux.
>>
>> No, 'usb reset' can be triggered by the user any time.
> 
> Yes, and it's also triggered as part of the hand-off, which is the use
> case in question.

No, that's not true. The USB controllers are torn down when starting the
OS, which is a different thing from usb reset, which brings them back up
and rescans the bus too.

>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>> the controller is unable to access the register space
>>> so the Linux boot hangs at this place.
>>
>> This doesn't make any sense, Linux should enable the necessary clock
>> before accessing any controller registers, unless there is a bug in Linux.
> 
> Should but doesn't always?  So yes, there's possibly / hopefully a bug
> in the dts files.

How did you reach that conclusion about the DTS files ? There is a bug
in Linux, but it's likely in the driver which doesn't enable the clock
before accessing the registers.

But maybe the description here is completely confusing, since the output
down below would indicate this hang is still in U-Boot.

>>> This particular condition occur when we enable both the
>>> controllers, so this patch will disable OHCI1 and EHCI1 for
>>> bananapi-m64 and nanopi-a64 boards. It will re-enable the same
>>> once the issue got fixed.
>>>
>>> Log:
>>> => usb reset
>>> resetting USB...
>>>
>>> PHY0: mask = 0x101, usb_clk_cfg = 0x30202
>>> sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
>>> EHCI failed to shut down host controller.
>>> ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
>>> ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
>>> ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
>>>
>>> PHY1: mask = 0x202, usb_clk_cfg = 0x0
>>> ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
>>
>> Why is this usb reset so noisy ?
> 
> ... I would assume additional debug messages.
> 
>>> << hang here >>
>>
>> Please fix this properly, this patch is pure attempt to hide a bug.
>> NAK from me.
> 
> Well, the point of this patch as Jagan says is to hide the bug for the
> release so that Linux can boot, which is an important case.

But the above implies that Linux can boot and the hang happens while
still in U-Boot due to some ordering bug between clock and register
access in the .remove function of some driver (I guess). That is what
needs to be debugged and fixed.

> Jagan, can you bisect down to when this started happening?  I assume
> it's a recent'ish thing.  Thanks!
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Maxime Ripard]

On Mon, Jul 02, 2018 at 11:57:45PM +0200, Marek Vasut wrote:
> On 07/02/2018 11:40 PM, Tom Rini wrote:
> > On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
> >> On 07/02/2018 10:53 PM, Jagan Teki wrote:
> >>> During usb shutdown or 'usb reset' all the necessary clocks
> >>> on the specific controller will disable. Usually this shutdown
> >>> happen during U-Boot proper handoff to Linux.
> >>
> >> No, 'usb reset' can be triggered by the user any time.
> > 
> > Yes, and it's also triggered as part of the hand-off, which is the use
> > case in question.
> 
> No, that's not true. The USB controllers are torn down when starting the
> OS, which is a different thing from usb reset, which brings them back up
> and rescans the bus too.
> 
> >>> There is an issue in Allwinner A64, is during OHCI1 shutdown
> >>> the controller is unable to access the register space
> >>> so the Linux boot hangs at this place.
> >>
> >> This doesn't make any sense, Linux should enable the necessary clock
> >> before accessing any controller registers, unless there is a bug in Linux.
> > 
> > Should but doesn't always?  So yes, there's possibly / hopefully a bug
> > in the dts files.
> 
> How did you reach that conclusion about the DTS files ? There is a bug
> in Linux, but it's likely in the driver which doesn't enable the clock
> before accessing the registers.
> 
> But maybe the description here is completely confusing, since the output
> down below would indicate this hang is still in U-Boot.
> 
> >>> This particular condition occur when we enable both the
> >>> controllers, so this patch will disable OHCI1 and EHCI1 for
> >>> bananapi-m64 and nanopi-a64 boards. It will re-enable the same
> >>> once the issue got fixed.
> >>>
> >>> Log:
> >>> => usb reset
> >>> resetting USB...
> >>>
> >>> PHY0: mask = 0x101, usb_clk_cfg = 0x30202
> >>> sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
> >>> EHCI failed to shut down host controller.
> >>> ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
> >>> ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
> >>> ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
> >>>
> >>> PHY1: mask = 0x202, usb_clk_cfg = 0x0
> >>> ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
> >>
> >> Why is this usb reset so noisy ?
> > 
> > ... I would assume additional debug messages.
> > 
> >>> << hang here >>
> >>
> >> Please fix this properly, this patch is pure attempt to hide a bug.
> >> NAK from me.
> > 
> > Well, the point of this patch as Jagan says is to hide the bug for the
> > release so that Linux can boot, which is an important case.
> 
> But the above implies that Linux can boot and the hang happens while
> still in U-Boot due to some ordering bug between clock and register
> access in the .remove function of some driver (I guess). That is what
> needs to be debugged and fixed.

Yeah, I agree. If we have a bug in U-Boot or in Linux, we should fix
whatever it is. Papering over it will just keep it, with no one
interested in fixing it anymore.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180703/fa590659/attachment.sig>

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Jagan Teki]

On Tue, Jul 3, 2018 at 3:10 AM, Tom Rini <trini@konsulko.com> wrote:
> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>> > During usb shutdown or 'usb reset' all the necessary clocks
>> > on the specific controller will disable. Usually this shutdown
>> > happen during U-Boot proper handoff to Linux.
>>
>> No, 'usb reset' can be triggered by the user any time.
>
> Yes, and it's also triggered as part of the hand-off, which is the use
> case in question.
>
>> > There is an issue in Allwinner A64, is during OHCI1 shutdown
>> > the controller is unable to access the register space
>> > so the Linux boot hangs at this place.
>>
>> This doesn't make any sense, Linux should enable the necessary clock
>> before accessing any controller registers, unless there is a bug in Linux.
>
> Should but doesn't always?  So yes, there's possibly / hopefully a bug
> in the dts files.
>
>> > This particular condition occur when we enable both the
>> > controllers, so this patch will disable OHCI1 and EHCI1 for
>> > bananapi-m64 and nanopi-a64 boards. It will re-enable the same
>> > once the issue got fixed.
>> >
>> > Log:
>> > => usb reset
>> > resetting USB...
>> >
>> > PHY0: mask = 0x101, usb_clk_cfg = 0x30202
>> > sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
>> > EHCI failed to shut down host controller.
>> > ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
>> > ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
>> > ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
>> >
>> > PHY1: mask = 0x202, usb_clk_cfg = 0x0
>> > ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
>>
>> Why is this usb reset so noisy ?
>
> ... I would assume additional debug messages.
>
>> > << hang here >>
>>
>> Please fix this properly, this patch is pure attempt to hide a bug.
>> NAK from me.
>
> Well, the point of this patch as Jagan says is to hide the bug for the
> release so that Linux can boot, which is an important case.
>
> Jagan, can you bisect down to when this started happening?  I assume
> it's a recent'ish thing.  Thanks!

commit b62cdbddedc3 ("sunxi: clock: Fix EHCI and OHCI clocks on A64")
is effecting this change, but functionally this commit is proper but
handling clock directly in drivers look not effective particularly in
the case of A64 where sharing of clocks and gates between OHCI and
EHCI.

Here are the possible changes to look at it.

1)  with this patch, disabling [oe]hci1 controllers on two boards (bpi
and nanopi):
      this change specific to two boards, rest of A64 boards are OK.

2)  turn-off gate and clocks for H3/H5/A64 from Andre [1]
      this would change all H3/H5/A64 shutdown behavior not to disable
gates and clocks during .remove

3)  turn-off clock only for A64
     this would fix, two board problems in 1) and also specific to A64.

4)  add counter mechanism to disable all clock at once, suggested by Marek
     either we can add counter mechanism to A64 or for all other SOC.
this need to test in all Allwinner boards if the counter is globally
managed in ohci-sunxi.c

Again, there is an ongoing work for syncing all DT changes from Linux,
by Andre. 1) change will update later in the release. rest of 2), 3)
and 4) will make changes in driver [eo]hci-sunxi.c and this drivers
indeed remove once we have dm clock which would also planned for
upcoming releases.

[1] https://patchwork.ozlabs.org/patch/938279/

Jagan.

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Tom Rini]

On Tue, Jul 03, 2018 at 06:06:37PM +0530, Jagan Teki wrote:
> On Tue, Jul 3, 2018 at 3:10 AM, Tom Rini <trini@konsulko.com> wrote:
> > On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
> >> On 07/02/2018 10:53 PM, Jagan Teki wrote:
> >> > During usb shutdown or 'usb reset' all the necessary clocks
> >> > on the specific controller will disable. Usually this shutdown
> >> > happen during U-Boot proper handoff to Linux.
> >>
> >> No, 'usb reset' can be triggered by the user any time.
> >
> > Yes, and it's also triggered as part of the hand-off, which is the use
> > case in question.
> >
> >> > There is an issue in Allwinner A64, is during OHCI1 shutdown
> >> > the controller is unable to access the register space
> >> > so the Linux boot hangs at this place.
> >>
> >> This doesn't make any sense, Linux should enable the necessary clock
> >> before accessing any controller registers, unless there is a bug in Linux.
> >
> > Should but doesn't always?  So yes, there's possibly / hopefully a bug
> > in the dts files.
> >
> >> > This particular condition occur when we enable both the
> >> > controllers, so this patch will disable OHCI1 and EHCI1 for
> >> > bananapi-m64 and nanopi-a64 boards. It will re-enable the same
> >> > once the issue got fixed.
> >> >
> >> > Log:
> >> > => usb reset
> >> > resetting USB...
> >> >
> >> > PHY0: mask = 0x101, usb_clk_cfg = 0x30202
> >> > sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
> >> > EHCI failed to shut down host controller.
> >> > ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
> >> > ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
> >> > ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
> >> >
> >> > PHY1: mask = 0x202, usb_clk_cfg = 0x0
> >> > ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
> >>
> >> Why is this usb reset so noisy ?
> >
> > ... I would assume additional debug messages.
> >
> >> > << hang here >>
> >>
> >> Please fix this properly, this patch is pure attempt to hide a bug.
> >> NAK from me.
> >
> > Well, the point of this patch as Jagan says is to hide the bug for the
> > release so that Linux can boot, which is an important case.
> >
> > Jagan, can you bisect down to when this started happening?  I assume
> > it's a recent'ish thing.  Thanks!
> 
> commit b62cdbddedc3 ("sunxi: clock: Fix EHCI and OHCI clocks on A64")
> is effecting this change, but functionally this commit is proper but
> handling clock directly in drivers look not effective particularly in
> the case of A64 where sharing of clocks and gates between OHCI and
> EHCI.
> 
> Here are the possible changes to look at it.
> 
> 1)  with this patch, disabling [oe]hci1 controllers on two boards (bpi
> and nanopi):
>       this change specific to two boards, rest of A64 boards are OK.
> 
> 2)  turn-off gate and clocks for H3/H5/A64 from Andre [1]
>       this would change all H3/H5/A64 shutdown behavior not to disable
> gates and clocks during .remove
> 
> 3)  turn-off clock only for A64
>      this would fix, two board problems in 1) and also specific to A64.
> 
> 4)  add counter mechanism to disable all clock at once, suggested by Marek
>      either we can add counter mechanism to A64 or for all other SOC.
> this need to test in all Allwinner boards if the counter is globally
> managed in ohci-sunxi.c
> 
> Again, there is an ongoing work for syncing all DT changes from Linux,
> by Andre. 1) change will update later in the release. rest of 2), 3)
> and 4) will make changes in driver [eo]hci-sunxi.c and this drivers
> indeed remove once we have dm clock which would also planned for
> upcoming releases.
> 
> [1] https://patchwork.ozlabs.org/patch/938279/

Adding a few more people to the list.  It looks like b62cdbddedc3 was in
response to fef73766d9ad.  So, this close to the release, what do we
need to do to get things back to the state things were in for v2018.05?
And then what are the combinations that aren't working and need to be
addressed again in v2018.09 so that we can make forward progress?
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180703/6093c7a3/attachment.sig>

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Andre Przywara]

Hi,

On 02/07/18 22:57, Marek Vasut wrote:
> On 07/02/2018 11:40 PM, Tom Rini wrote:
>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>> on the specific controller will disable. Usually this shutdown
>>>> happen during U-Boot proper handoff to Linux.
>>>
>>> No, 'usb reset' can be triggered by the user any time.
>>
>> Yes, and it's also triggered as part of the hand-off, which is the use
>> case in question.
> 
> No, that's not true. The USB controllers are torn down when starting the
> OS, which is a different thing from usb reset, which brings them back up
> and rescans the bus too.
> 
>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>> the controller is unable to access the register space
>>>> so the Linux boot hangs at this place.
>>>
>>> This doesn't make any sense, Linux should enable the necessary clock
>>> before accessing any controller registers, unless there is a bug in Linux.
>>
>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>> in the dts files.
> 
> How did you reach that conclusion about the DTS files ? There is a bug
> in Linux, but it's likely in the driver which doesn't enable the clock
> before accessing the registers.
> 
> But maybe the description here is completely confusing, since the output
> down below would indicate this hang is still in U-Boot.

Yes, it is. There is no bug in Linux.

U-Boot trips over its own feet when bringing down the USB controllers:
It shutdowns one part (EHCI or OHCI) on the register level, then turns
off the clocks and reset gates. But because they are shared between
controllers, this turns off the other controller as well. Then it tries
to bring down the the second part (OHCI or EHCI, respectively) on the
USB register level, which hangs, because the AHB clock is already off.
As this just happens quite late, it looks like U-Boot already said
goodbye, but it actually hasn't completely finished.
So Linux is completely fine and the bug is entirely in U-Boot.
My patch [1] tries to paper^Wsolve this in a different way, though it
isn't perfect either. I think there is a bit more to it than I assumed
yesterday, so I need to go back to the code later tonight to see what's
really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
about EHCI and OHCI).

Cheers,
Andre.

[1] https://lists.denx.de/pipermail/u-boot/2018-July/333533.html

>>>> This particular condition occur when we enable both the
>>>> controllers, so this patch will disable OHCI1 and EHCI1 for
>>>> bananapi-m64 and nanopi-a64 boards. It will re-enable the same
>>>> once the issue got fixed.
>>>>
>>>> Log:
>>>> => usb reset
>>>> resetting USB...
>>>>
>>>> PHY0: mask = 0x101, usb_clk_cfg = 0x30202
>>>> sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
>>>> EHCI failed to shut down host controller.
>>>> ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
>>>> ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
>>>> ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
>>>>
>>>> PHY1: mask = 0x202, usb_clk_cfg = 0x0
>>>> ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
>>>
>>> Why is this usb reset so noisy ?
>>
>> ... I would assume additional debug messages.
>>
>>>> << hang here >>
>>>
>>> Please fix this properly, this patch is pure attempt to hide a bug.
>>> NAK from me.
>>
>> Well, the point of this patch as Jagan says is to hide the bug for the
>> release so that Linux can boot, which is an important case.
> 
> But the above implies that Linux can boot and the hang happens while
> still in U-Boot due to some ordering bug between clock and register
> access in the .remove function of some driver (I guess). That is what
> needs to be debugged and fixed.
> 
>> Jagan, can you bisect down to when this started happening?  I assume
>> it's a recent'ish thing.  Thanks!
>>
> 
> 

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Jagan Teki]

On Tue, Jul 3, 2018 at 8:52 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 02/07/18 22:57, Marek Vasut wrote:
>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>> on the specific controller will disable. Usually this shutdown
>>>>> happen during U-Boot proper handoff to Linux.
>>>>
>>>> No, 'usb reset' can be triggered by the user any time.
>>>
>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>> case in question.
>>
>> No, that's not true. The USB controllers are torn down when starting the
>> OS, which is a different thing from usb reset, which brings them back up
>> and rescans the bus too.
>>
>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>> the controller is unable to access the register space
>>>>> so the Linux boot hangs at this place.
>>>>
>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>
>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>> in the dts files.
>>
>> How did you reach that conclusion about the DTS files ? There is a bug
>> in Linux, but it's likely in the driver which doesn't enable the clock
>> before accessing the registers.
>>
>> But maybe the description here is completely confusing, since the output
>> down below would indicate this hang is still in U-Boot.
>
> Yes, it is. There is no bug in Linux.
>
> U-Boot trips over its own feet when bringing down the USB controllers:
> It shutdowns one part (EHCI or OHCI) on the register level, then turns
> off the clocks and reset gates. But because they are shared between
> controllers, this turns off the other controller as well. Then it tries
> to bring down the the second part (OHCI or EHCI, respectively) on the
> USB register level, which hangs, because the AHB clock is already off.
> As this just happens quite late, it looks like U-Boot already said
> goodbye, but it actually hasn't completely finished.
> So Linux is completely fine and the bug is entirely in U-Boot.
> My patch [1] tries to paper^Wsolve this in a different way, though it
> isn't perfect either. I think there is a bit more to it than I assumed
> yesterday, so I need to go back to the code later tonight to see what's
> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
> about EHCI and OHCI).
>
> Cheers,
> Andre.
>
> [1] https://lists.denx.de/pipermail/u-boot/2018-July/333533.html

Do we really need turn-off ahb and reset gates? these are gracefully
disabled during shutdown.

Jagan.

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Marek Vasut]

On 07/03/2018 05:22 PM, Andre Przywara wrote:
> Hi,
> 
> On 02/07/18 22:57, Marek Vasut wrote:
>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>> on the specific controller will disable. Usually this shutdown
>>>>> happen during U-Boot proper handoff to Linux.
>>>>
>>>> No, 'usb reset' can be triggered by the user any time.
>>>
>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>> case in question.
>>
>> No, that's not true. The USB controllers are torn down when starting the
>> OS, which is a different thing from usb reset, which brings them back up
>> and rescans the bus too.
>>
>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>> the controller is unable to access the register space
>>>>> so the Linux boot hangs at this place.
>>>>
>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>
>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>> in the dts files.
>>
>> How did you reach that conclusion about the DTS files ? There is a bug
>> in Linux, but it's likely in the driver which doesn't enable the clock
>> before accessing the registers.
>>
>> But maybe the description here is completely confusing, since the output
>> down below would indicate this hang is still in U-Boot.
> 
> Yes, it is. There is no bug in Linux.
> 
> U-Boot trips over its own feet when bringing down the USB controllers:
> It shutdowns one part (EHCI or OHCI) on the register level, then turns
> off the clocks and reset gates. But because they are shared between
> controllers, this turns off the other controller as well. Then it tries
> to bring down the the second part (OHCI or EHCI, respectively) on the
> USB register level, which hangs, because the AHB clock is already off.
> As this just happens quite late, it looks like U-Boot already said
> goodbye, but it actually hasn't completely finished.
> So Linux is completely fine and the bug is entirely in U-Boot.
> My patch [1] tries to paper^Wsolve this in a different way, though it
> isn't perfect either. I think there is a bit more to it than I assumed
> yesterday, so I need to go back to the code later tonight to see what's
> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
> about EHCI and OHCI).

Well, please keep poking.

Maybe a dumb idea, but what about enabling the clock at the beginning of
remove function to guarantee they are ON and then disabling the clock at
the end of the function. Would that work maybe ? ie.

.remove() {
	clk_enable(..);
	readl()/writel()/...
	clk_disable(..);
}

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Marek Vasut]

On 07/03/2018 06:25 PM, Jagan Teki wrote:
> On Tue, Jul 3, 2018 at 8:52 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> Hi,
>>
>> On 02/07/18 22:57, Marek Vasut wrote:
>>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>>> on the specific controller will disable. Usually this shutdown
>>>>>> happen during U-Boot proper handoff to Linux.
>>>>>
>>>>> No, 'usb reset' can be triggered by the user any time.
>>>>
>>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>>> case in question.
>>>
>>> No, that's not true. The USB controllers are torn down when starting the
>>> OS, which is a different thing from usb reset, which brings them back up
>>> and rescans the bus too.
>>>
>>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>>> the controller is unable to access the register space
>>>>>> so the Linux boot hangs at this place.
>>>>>
>>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>>
>>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>>> in the dts files.
>>>
>>> How did you reach that conclusion about the DTS files ? There is a bug
>>> in Linux, but it's likely in the driver which doesn't enable the clock
>>> before accessing the registers.
>>>
>>> But maybe the description here is completely confusing, since the output
>>> down below would indicate this hang is still in U-Boot.
>>
>> Yes, it is. There is no bug in Linux.
>>
>> U-Boot trips over its own feet when bringing down the USB controllers:
>> It shutdowns one part (EHCI or OHCI) on the register level, then turns
>> off the clocks and reset gates. But because they are shared between
>> controllers, this turns off the other controller as well. Then it tries
>> to bring down the the second part (OHCI or EHCI, respectively) on the
>> USB register level, which hangs, because the AHB clock is already off.
>> As this just happens quite late, it looks like U-Boot already said
>> goodbye, but it actually hasn't completely finished.
>> So Linux is completely fine and the bug is entirely in U-Boot.
>> My patch [1] tries to paper^Wsolve this in a different way, though it
>> isn't perfect either. I think there is a bit more to it than I assumed
>> yesterday, so I need to go back to the code later tonight to see what's
>> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
>> about EHCI and OHCI).
>>
>> Cheers,
>> Andre.
>>
>> [1] https://lists.denx.de/pipermail/u-boot/2018-July/333533.html
> 
> Do we really need turn-off ahb and reset gates? these are gracefully
> disabled during shutdown.

Given that this SoC is mostly operated from battery OR in constrained
environments where useless high thermal dissipation might cause trouble,
yes, turning off as much as possible is desired.

Besides, it's a good practice to keep things in order.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Jagan Teki]

On Tue, Jul 3, 2018 at 10:14 PM, Marek Vasut <marex@denx.de> wrote:
> On 07/03/2018 05:22 PM, Andre Przywara wrote:
>> Hi,
>>
>> On 02/07/18 22:57, Marek Vasut wrote:
>>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>>> on the specific controller will disable. Usually this shutdown
>>>>>> happen during U-Boot proper handoff to Linux.
>>>>>
>>>>> No, 'usb reset' can be triggered by the user any time.
>>>>
>>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>>> case in question.
>>>
>>> No, that's not true. The USB controllers are torn down when starting the
>>> OS, which is a different thing from usb reset, which brings them back up
>>> and rescans the bus too.
>>>
>>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>>> the controller is unable to access the register space
>>>>>> so the Linux boot hangs at this place.
>>>>>
>>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>>
>>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>>> in the dts files.
>>>
>>> How did you reach that conclusion about the DTS files ? There is a bug
>>> in Linux, but it's likely in the driver which doesn't enable the clock
>>> before accessing the registers.
>>>
>>> But maybe the description here is completely confusing, since the output
>>> down below would indicate this hang is still in U-Boot.
>>
>> Yes, it is. There is no bug in Linux.
>>
>> U-Boot trips over its own feet when bringing down the USB controllers:
>> It shutdowns one part (EHCI or OHCI) on the register level, then turns
>> off the clocks and reset gates. But because they are shared between
>> controllers, this turns off the other controller as well. Then it tries
>> to bring down the the second part (OHCI or EHCI, respectively) on the
>> USB register level, which hangs, because the AHB clock is already off.
>> As this just happens quite late, it looks like U-Boot already said
>> goodbye, but it actually hasn't completely finished.
>> So Linux is completely fine and the bug is entirely in U-Boot.
>> My patch [1] tries to paper^Wsolve this in a different way, though it
>> isn't perfect either. I think there is a bit more to it than I assumed
>> yesterday, so I need to go back to the code later tonight to see what's
>> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
>> about EHCI and OHCI).
>
> Well, please keep poking.
>
> Maybe a dumb idea, but what about enabling the clock at the beginning of
> remove function to guarantee they are ON and then disabling the clock at
> the end of the function. Would that work maybe ? ie.
>
> .remove() {
>         clk_enable(..);
>         readl()/writel()/...
>         clk_disable(..);
> }

I've verified clock bits before disabling, and bits are enabled as
usual. and even verified your idea of enabling before disable[2]

=> usb reset
resetting USB...
ohci_usb_remove: input mask = 0x10000, input usb_clk_cfg = 0x30303
ohci_usb_remove: usb_clk_cfg = 0x20303
EHCI failed to shut down host controller.

[2] https://paste.ubuntu.com/p/V9KKxMx6Cj/

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Marek Vasut]

On 07/03/2018 07:09 PM, Jagan Teki wrote:
> On Tue, Jul 3, 2018 at 10:14 PM, Marek Vasut <marex@denx.de> wrote:
>> On 07/03/2018 05:22 PM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 02/07/18 22:57, Marek Vasut wrote:
>>>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>>>> on the specific controller will disable. Usually this shutdown
>>>>>>> happen during U-Boot proper handoff to Linux.
>>>>>>
>>>>>> No, 'usb reset' can be triggered by the user any time.
>>>>>
>>>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>>>> case in question.
>>>>
>>>> No, that's not true. The USB controllers are torn down when starting the
>>>> OS, which is a different thing from usb reset, which brings them back up
>>>> and rescans the bus too.
>>>>
>>>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>>>> the controller is unable to access the register space
>>>>>>> so the Linux boot hangs at this place.
>>>>>>
>>>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>>>
>>>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>>>> in the dts files.
>>>>
>>>> How did you reach that conclusion about the DTS files ? There is a bug
>>>> in Linux, but it's likely in the driver which doesn't enable the clock
>>>> before accessing the registers.
>>>>
>>>> But maybe the description here is completely confusing, since the output
>>>> down below would indicate this hang is still in U-Boot.
>>>
>>> Yes, it is. There is no bug in Linux.
>>>
>>> U-Boot trips over its own feet when bringing down the USB controllers:
>>> It shutdowns one part (EHCI or OHCI) on the register level, then turns
>>> off the clocks and reset gates. But because they are shared between
>>> controllers, this turns off the other controller as well. Then it tries
>>> to bring down the the second part (OHCI or EHCI, respectively) on the
>>> USB register level, which hangs, because the AHB clock is already off.
>>> As this just happens quite late, it looks like U-Boot already said
>>> goodbye, but it actually hasn't completely finished.
>>> So Linux is completely fine and the bug is entirely in U-Boot.
>>> My patch [1] tries to paper^Wsolve this in a different way, though it
>>> isn't perfect either. I think there is a bit more to it than I assumed
>>> yesterday, so I need to go back to the code later tonight to see what's
>>> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
>>> about EHCI and OHCI).
>>
>> Well, please keep poking.
>>
>> Maybe a dumb idea, but what about enabling the clock at the beginning of
>> remove function to guarantee they are ON and then disabling the clock at
>> the end of the function. Would that work maybe ? ie.
>>
>> .remove() {
>>         clk_enable(..);
>>         readl()/writel()/...
>>         clk_disable(..);
>> }
> 
> I've verified clock bits before disabling, and bits are enabled as
> usual. and even verified your idea of enabling before disable[2]

Verified ... with what result ?

> => usb reset
> resetting USB...
> ohci_usb_remove: input mask = 0x10000, input usb_clk_cfg = 0x30303
> ohci_usb_remove: usb_clk_cfg = 0x20303
> EHCI failed to shut down host controller.
> 
> [2] https://paste.ubuntu.com/p/V9KKxMx6Cj/

(no need to use pastebin to share 20 line patch, in fact it doesn't help
the ML archives at all)

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Jagan Teki]

On Tue, Jul 3, 2018 at 11:26 PM, Marek Vasut <marex@denx.de> wrote:
> On 07/03/2018 07:09 PM, Jagan Teki wrote:
>> On Tue, Jul 3, 2018 at 10:14 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 07/03/2018 05:22 PM, Andre Przywara wrote:
>>>> Hi,
>>>>
>>>> On 02/07/18 22:57, Marek Vasut wrote:
>>>>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>>>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>>>>> on the specific controller will disable. Usually this shutdown
>>>>>>>> happen during U-Boot proper handoff to Linux.
>>>>>>>
>>>>>>> No, 'usb reset' can be triggered by the user any time.
>>>>>>
>>>>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>>>>> case in question.
>>>>>
>>>>> No, that's not true. The USB controllers are torn down when starting the
>>>>> OS, which is a different thing from usb reset, which brings them back up
>>>>> and rescans the bus too.
>>>>>
>>>>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>>>>> the controller is unable to access the register space
>>>>>>>> so the Linux boot hangs at this place.
>>>>>>>
>>>>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>>>>
>>>>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>>>>> in the dts files.
>>>>>
>>>>> How did you reach that conclusion about the DTS files ? There is a bug
>>>>> in Linux, but it's likely in the driver which doesn't enable the clock
>>>>> before accessing the registers.
>>>>>
>>>>> But maybe the description here is completely confusing, since the output
>>>>> down below would indicate this hang is still in U-Boot.
>>>>
>>>> Yes, it is. There is no bug in Linux.
>>>>
>>>> U-Boot trips over its own feet when bringing down the USB controllers:
>>>> It shutdowns one part (EHCI or OHCI) on the register level, then turns
>>>> off the clocks and reset gates. But because they are shared between
>>>> controllers, this turns off the other controller as well. Then it tries
>>>> to bring down the the second part (OHCI or EHCI, respectively) on the
>>>> USB register level, which hangs, because the AHB clock is already off.
>>>> As this just happens quite late, it looks like U-Boot already said
>>>> goodbye, but it actually hasn't completely finished.
>>>> So Linux is completely fine and the bug is entirely in U-Boot.
>>>> My patch [1] tries to paper^Wsolve this in a different way, though it
>>>> isn't perfect either. I think there is a bit more to it than I assumed
>>>> yesterday, so I need to go back to the code later tonight to see what's
>>>> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
>>>> about EHCI and OHCI).
>>>
>>> Well, please keep poking.
>>>
>>> Maybe a dumb idea, but what about enabling the clock at the beginning of
>>> remove function to guarantee they are ON and then disabling the clock at
>>> the end of the function. Would that work maybe ? ie.
>>>
>>> .remove() {
>>>         clk_enable(..);
>>>         readl()/writel()/...
>>>         clk_disable(..);
>>> }
>>
>> I've verified clock bits before disabling, and bits are enabled as
>> usual. and even verified your idea of enabling before disable[2]
>
> Verified ... with what result ?

Same hang, with properly disabling clock during OHCI0 shutdown.

>
>> => usb reset
>> resetting USB...
>> ohci_usb_remove: input mask = 0x10000, input usb_clk_cfg = 0x30303
>> ohci_usb_remove: usb_clk_cfg = 0x20303
>> EHCI failed to shut down host controller.

<< hang here >>

>>
>> [2] https://paste.ubuntu.com/p/V9KKxMx6Cj/
>
> (no need to use pastebin to share 20 line patch, in fact it doesn't help
> the ML archives at all)

Sorry, copying same here.

--- a/drivers/usb/host/ohci-sunxi.c
+++ b/drivers/usb/host/ohci-sunxi.c
@@ -128,10 +128,14 @@ static int ohci_usb_remove(struct udevice *dev)
        if (ret)
                return ret;

+       printf("%s: mask = 0x%x, usb_clk_cfg = 0x%x\n", __func__,
+               priv->usb_gate_mask, readl(&priv->ccm->usb_clk_cfg));
+       setbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
        if (priv->cfg->has_reset)
                clrbits_le32(priv->reset0_cfg, priv->ahb_gate_mask);
-       clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
        clrbits_le32(&priv->ccm->ahb_gate0, priv->ahb_gate_mask);
+       clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
+       printf("%s: usb_clk_cfg = 0x%x\n", __func__,
readl(&priv->ccm->usb_clk_cfg));

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Marek Vasut]

On 07/03/2018 08:02 PM, Jagan Teki wrote:
> On Tue, Jul 3, 2018 at 11:26 PM, Marek Vasut <marex@denx.de> wrote:
>> On 07/03/2018 07:09 PM, Jagan Teki wrote:
>>> On Tue, Jul 3, 2018 at 10:14 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 07/03/2018 05:22 PM, Andre Przywara wrote:
>>>>> Hi,
>>>>>
>>>>> On 02/07/18 22:57, Marek Vasut wrote:
>>>>>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>>>>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>>>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>>>>>> on the specific controller will disable. Usually this shutdown
>>>>>>>>> happen during U-Boot proper handoff to Linux.
>>>>>>>>
>>>>>>>> No, 'usb reset' can be triggered by the user any time.
>>>>>>>
>>>>>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>>>>>> case in question.
>>>>>>
>>>>>> No, that's not true. The USB controllers are torn down when starting the
>>>>>> OS, which is a different thing from usb reset, which brings them back up
>>>>>> and rescans the bus too.
>>>>>>
>>>>>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>>>>>> the controller is unable to access the register space
>>>>>>>>> so the Linux boot hangs at this place.
>>>>>>>>
>>>>>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>>>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>>>>>
>>>>>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>>>>>> in the dts files.
>>>>>>
>>>>>> How did you reach that conclusion about the DTS files ? There is a bug
>>>>>> in Linux, but it's likely in the driver which doesn't enable the clock
>>>>>> before accessing the registers.
>>>>>>
>>>>>> But maybe the description here is completely confusing, since the output
>>>>>> down below would indicate this hang is still in U-Boot.
>>>>>
>>>>> Yes, it is. There is no bug in Linux.
>>>>>
>>>>> U-Boot trips over its own feet when bringing down the USB controllers:
>>>>> It shutdowns one part (EHCI or OHCI) on the register level, then turns
>>>>> off the clocks and reset gates. But because they are shared between
>>>>> controllers, this turns off the other controller as well. Then it tries
>>>>> to bring down the the second part (OHCI or EHCI, respectively) on the
>>>>> USB register level, which hangs, because the AHB clock is already off.
>>>>> As this just happens quite late, it looks like U-Boot already said
>>>>> goodbye, but it actually hasn't completely finished.
>>>>> So Linux is completely fine and the bug is entirely in U-Boot.
>>>>> My patch [1] tries to paper^Wsolve this in a different way, though it
>>>>> isn't perfect either. I think there is a bit more to it than I assumed
>>>>> yesterday, so I need to go back to the code later tonight to see what's
>>>>> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
>>>>> about EHCI and OHCI).
>>>>
>>>> Well, please keep poking.
>>>>
>>>> Maybe a dumb idea, but what about enabling the clock at the beginning of
>>>> remove function to guarantee they are ON and then disabling the clock at
>>>> the end of the function. Would that work maybe ? ie.
>>>>
>>>> .remove() {
>>>>         clk_enable(..);
>>>>         readl()/writel()/...
>>>>         clk_disable(..);
>>>> }
>>>
>>> I've verified clock bits before disabling, and bits are enabled as
>>> usual. and even verified your idea of enabling before disable[2]
>>
>> Verified ... with what result ?
> 
> Same hang, with properly disabling clock during OHCI0 shutdown.

So the clock were enabled and yet there was a hang ? Why ?

I was under the impression that disabling clock was the problem, maybe
that is not the case ?

Are you sure you enabled all the clock ?

>>> => usb reset
>>> resetting USB...
>>> ohci_usb_remove: input mask = 0x10000, input usb_clk_cfg = 0x30303
>>> ohci_usb_remove: usb_clk_cfg = 0x20303
>>> EHCI failed to shut down host controller.
> 
> << hang here >>
> 
>>>
>>> [2] https://paste.ubuntu.com/p/V9KKxMx6Cj/
>>
>> (no need to use pastebin to share 20 line patch, in fact it doesn't help
>> the ML archives at all)
> 
> Sorry, copying same here.
> 
> --- a/drivers/usb/host/ohci-sunxi.c
> +++ b/drivers/usb/host/ohci-sunxi.c
> @@ -128,10 +128,14 @@ static int ohci_usb_remove(struct udevice *dev)
>         if (ret)
>                 return ret;
> 
> +       printf("%s: mask = 0x%x, usb_clk_cfg = 0x%x\n", __func__,
> +               priv->usb_gate_mask, readl(&priv->ccm->usb_clk_cfg));
> +       setbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
>         if (priv->cfg->has_reset)
>                 clrbits_le32(priv->reset0_cfg, priv->ahb_gate_mask);
> -       clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
>         clrbits_le32(&priv->ccm->ahb_gate0, priv->ahb_gate_mask);
> +       clrbits_le32(&priv->ccm->usb_clk_cfg, priv->usb_gate_mask);
> +       printf("%s: usb_clk_cfg = 0x%x\n", __func__,
> readl(&priv->ccm->usb_clk_cfg));

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Jagan Teki]

On Tue, Jul 3, 2018 at 11:46 PM, Marek Vasut <marex@denx.de> wrote:
> On 07/03/2018 08:02 PM, Jagan Teki wrote:
>> On Tue, Jul 3, 2018 at 11:26 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 07/03/2018 07:09 PM, Jagan Teki wrote:
>>>> On Tue, Jul 3, 2018 at 10:14 PM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 07/03/2018 05:22 PM, Andre Przywara wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 02/07/18 22:57, Marek Vasut wrote:
>>>>>>> On 07/02/2018 11:40 PM, Tom Rini wrote:
>>>>>>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>>>>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>>>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>>>>>>> on the specific controller will disable. Usually this shutdown
>>>>>>>>>> happen during U-Boot proper handoff to Linux.
>>>>>>>>>
>>>>>>>>> No, 'usb reset' can be triggered by the user any time.
>>>>>>>>
>>>>>>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>>>>>>> case in question.
>>>>>>>
>>>>>>> No, that's not true. The USB controllers are torn down when starting the
>>>>>>> OS, which is a different thing from usb reset, which brings them back up
>>>>>>> and rescans the bus too.
>>>>>>>
>>>>>>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>>>>>>> the controller is unable to access the register space
>>>>>>>>>> so the Linux boot hangs at this place.
>>>>>>>>>
>>>>>>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>>>>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>>>>>>
>>>>>>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>>>>>>> in the dts files.
>>>>>>>
>>>>>>> How did you reach that conclusion about the DTS files ? There is a bug
>>>>>>> in Linux, but it's likely in the driver which doesn't enable the clock
>>>>>>> before accessing the registers.
>>>>>>>
>>>>>>> But maybe the description here is completely confusing, since the output
>>>>>>> down below would indicate this hang is still in U-Boot.
>>>>>>
>>>>>> Yes, it is. There is no bug in Linux.
>>>>>>
>>>>>> U-Boot trips over its own feet when bringing down the USB controllers:
>>>>>> It shutdowns one part (EHCI or OHCI) on the register level, then turns
>>>>>> off the clocks and reset gates. But because they are shared between
>>>>>> controllers, this turns off the other controller as well. Then it tries
>>>>>> to bring down the the second part (OHCI or EHCI, respectively) on the
>>>>>> USB register level, which hangs, because the AHB clock is already off.
>>>>>> As this just happens quite late, it looks like U-Boot already said
>>>>>> goodbye, but it actually hasn't completely finished.
>>>>>> So Linux is completely fine and the bug is entirely in U-Boot.
>>>>>> My patch [1] tries to paper^Wsolve this in a different way, though it
>>>>>> isn't perfect either. I think there is a bit more to it than I assumed
>>>>>> yesterday, so I need to go back to the code later tonight to see what's
>>>>>> really going on (I suspect it's about OHCI 0 and 1 sharing a clock, not
>>>>>> about EHCI and OHCI).
>>>>>
>>>>> Well, please keep poking.
>>>>>
>>>>> Maybe a dumb idea, but what about enabling the clock at the beginning of
>>>>> remove function to guarantee they are ON and then disabling the clock at
>>>>> the end of the function. Would that work maybe ? ie.
>>>>>
>>>>> .remove() {
>>>>>         clk_enable(..);
>>>>>         readl()/writel()/...
>>>>>         clk_disable(..);
>>>>> }
>>>>
>>>> I've verified clock bits before disabling, and bits are enabled as
>>>> usual. and even verified your idea of enabling before disable[2]
>>>
>>> Verified ... with what result ?
>>
>> Same hang, with properly disabling clock during OHCI0 shutdown.
>
> So the clock were enabled and yet there was a hang ? Why ?

ie what I really not understand.

>
> I was under the impression that disabling clock was the problem, maybe
> that is not the case ?
>
> Are you sure you enabled all the clock ?

Yes I'm sure about it. usb_clk_cfg = 0x30303 is final out of clock
init. It iterated twice during initialization sequence.
- 1st iteration bit 0, 8 were enabled by PHY driver and bit 16 enabled by OHCI0
- 2nd iteration bit 1, 9 were enabled by PHY driver and bit 17 enabled by OHCI1

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[André Przywara]

On 07/03/2018 03:52 PM, Tom Rini wrote:
> On Tue, Jul 03, 2018 at 06:06:37PM +0530, Jagan Teki wrote:
>> On Tue, Jul 3, 2018 at 3:10 AM, Tom Rini <trini@konsulko.com> wrote:
>>> On Mon, Jul 02, 2018 at 11:27:58PM +0200, Marek Vasut wrote:
>>>> On 07/02/2018 10:53 PM, Jagan Teki wrote:
>>>>> During usb shutdown or 'usb reset' all the necessary clocks
>>>>> on the specific controller will disable. Usually this shutdown
>>>>> happen during U-Boot proper handoff to Linux.
>>>>
>>>> No, 'usb reset' can be triggered by the user any time.
>>>
>>> Yes, and it's also triggered as part of the hand-off, which is the use
>>> case in question.
>>>
>>>>> There is an issue in Allwinner A64, is during OHCI1 shutdown
>>>>> the controller is unable to access the register space
>>>>> so the Linux boot hangs at this place.
>>>>
>>>> This doesn't make any sense, Linux should enable the necessary clock
>>>> before accessing any controller registers, unless there is a bug in Linux.
>>>
>>> Should but doesn't always?  So yes, there's possibly / hopefully a bug
>>> in the dts files.
>>>
>>>>> This particular condition occur when we enable both the
>>>>> controllers, so this patch will disable OHCI1 and EHCI1 for
>>>>> bananapi-m64 and nanopi-a64 boards. It will re-enable the same
>>>>> once the issue got fixed.
>>>>>
>>>>> Log:
>>>>> => usb reset
>>>>> resetting USB...
>>>>>
>>>>> PHY0: mask = 0x101, usb_clk_cfg = 0x30202
>>>>> sunxi_musb_exit: ahb_gate0  = 0x33004540, reset0_cfg = 0x33004540
>>>>> EHCI failed to shut down host controller.
>>>>> ehci_usb_remove: ahb_gate0  = 0x32004540, reset0_cfg = 0x32004540
>>>>> ohci_usb_remove: ahb_gate0  = 0x22004540, reset0_cfg = 0x22004540
>>>>> ohci_usb_remove: mask = 0x10000, usb_clk_cfg = 0x20202
>>>>>
>>>>> PHY1: mask = 0x202, usb_clk_cfg = 0x0
>>>>> ehci_usb_remove: ahb_gate0  = 0x20004540, reset0_cfg = 0x20004540
>>>>
>>>> Why is this usb reset so noisy ?
>>>
>>> ... I would assume additional debug messages.
>>>
>>>>> << hang here >>
>>>>
>>>> Please fix this properly, this patch is pure attempt to hide a bug.
>>>> NAK from me.
>>>
>>> Well, the point of this patch as Jagan says is to hide the bug for the
>>> release so that Linux can boot, which is an important case.
>>>
>>> Jagan, can you bisect down to when this started happening?  I assume
>>> it's a recent'ish thing.  Thanks!
>>
>> commit b62cdbddedc3 ("sunxi: clock: Fix EHCI and OHCI clocks on A64")
>> is effecting this change, but functionally this commit is proper but
>> handling clock directly in drivers look not effective particularly in
>> the case of A64 where sharing of clocks and gates between OHCI and
>> EHCI.
>>
>> Here are the possible changes to look at it.
>>
>> 1)  with this patch, disabling [oe]hci1 controllers on two boards (bpi
>> and nanopi):
>>       this change specific to two boards, rest of A64 boards are OK.
>>
>> 2)  turn-off gate and clocks for H3/H5/A64 from Andre [1]
>>       this would change all H3/H5/A64 shutdown behavior not to disable
>> gates and clocks during .remove
>>
>> 3)  turn-off clock only for A64
>>      this would fix, two board problems in 1) and also specific to A64.
>>
>> 4)  add counter mechanism to disable all clock at once, suggested by Marek
>>      either we can add counter mechanism to A64 or for all other SOC.
>> this need to test in all Allwinner boards if the counter is globally
>> managed in ohci-sunxi.c
>>
>> Again, there is an ongoing work for syncing all DT changes from Linux,
>> by Andre. 1) change will update later in the release. rest of 2), 3)
>> and 4) will make changes in driver [eo]hci-sunxi.c and this drivers
>> indeed remove once we have dm clock which would also planned for
>> upcoming releases.
>>
>> [1] https://patchwork.ozlabs.org/patch/938279/
> 
> Adding a few more people to the list.  It looks like b62cdbddedc3 was in
> response to fef73766d9ad.  So, this close to the release, what do we
> need to do to get things back to the state things were in for v2018.05?
> And then what are the combinations that aren't working and need to be
> addressed again in v2018.09 so that we can make forward progress?

Without going into much detail here, the clock dependency for two
companion controllers on those A64 is weird, and we hack it somehow
since we don't have a DM_CLK driver for sunxi (yet, see below). That
works for init, since we just set already set bits. For shutdown, we
*happen* to take down the AHB gate clocks and resets correctly, because
the order of shutdown matches the dependency (EHCI first, the OHCI). Not
sure if this is intentional, though. It's fragile, but works.

What we don't (and can't easily) model is another oddity: the USB clock
for [OE]HCI0 is actually the parent of [OE]HCI1. So if we need to bring
down *two* controllers and do it in the natural order, the second one is
dead before it can be disabled properly.

This was somewhat hidden before, since we had only one controller in
operation. b62cdbddedc3 somewhat fixed that, but the DT for the Pine64
(which was the test vehicle) has the controller still disabled. Enabling
this (what I did) or using other boards (BananaPi and NanoPi from Jagan)
triggered this bug though.
AFAICS this parent relation only affects the A64 with its two
controllers, so:
- We could just fix it for now by *not* disabling the USB clocks (and
only those, gates and reset still go down) at all. This is basically one
part of my patch from yesterday (the second part is not needed).
- We do more effort and skip disabling for OHCI0, but disable both
clocks for OHCI1. This still relies on the order, but would probably
shut down the controllers. A bit hackish to implement, though.
I will try the second solution now and let you decide on the list.

Long term:
The proper solution is a DT based DM_CLK driver, which models it like
Linux does. Work is underway[1], but this somewhat opens a can of worms
(needs DM support for UART, MMC, a DM_RESET driver, pinctrl ...), so it
takes a bit of time.
Fixing the current ad-hoc solution somewhat properly with ref-counting
is not easy (two driver files) and not really worthwhile, I believe, but
we can make it work like described above until the proper solution comes
into play.

Cheers,
Andre.

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Vasily Khoruzhick]

On Tue, Jul 3, 2018 at 2:22 PM, André Przywara <andre.przywara@arm.com> wrote:

>> Adding a few more people to the list.  It looks like b62cdbddedc3 was in
>> response to fef73766d9ad.  So, this close to the release, what do we
>> need to do to get things back to the state things were in for v2018.05?
>> And then what are the combinations that aren't working and need to be
>> addressed again in v2018.09 so that we can make forward progress?
>
> Without going into much detail here, the clock dependency for two
> companion controllers on those A64 is weird, and we hack it somehow
> since we don't have a DM_CLK driver for sunxi (yet, see below). That
> works for init, since we just set already set bits. For shutdown, we
> *happen* to take down the AHB gate clocks and resets correctly, because
> the order of shutdown matches the dependency (EHCI first, the OHCI). Not
> sure if this is intentional, though. It's fragile, but works.
>
> What we don't (and can't easily) model is another oddity: the USB clock
> for [OE]HCI0 is actually the parent of [OE]HCI1. So if we need to bring
> down *two* controllers and do it in the natural order, the second one is
> dead before it can be disabled properly.
>
> This was somewhat hidden before, since we had only one controller in
> operation. b62cdbddedc3 somewhat fixed that, but the DT for the Pine64
> (which was the test vehicle) has the controller still disabled. Enabling
> this (what I did) or using other boards (BananaPi and NanoPi from Jagan)
> triggered this bug though.
> AFAICS this parent relation only affects the A64 with its two
> controllers, so:
> - We could just fix it for now by *not* disabling the USB clocks (and
> only those, gates and reset still go down) at all. This is basically one
> part of my patch from yesterday (the second part is not needed).
> - We do more effort and skip disabling for OHCI0, but disable both
> clocks for OHCI1. This still relies on the order, but would probably
> shut down the controllers. A bit hackish to implement, though.
> I will try the second solution now and let you decide on the list.
>
> Long term:
> The proper solution is a DT based DM_CLK driver, which models it like
> Linux does. Work is underway[1], but this somewhat opens a can of worms
> (needs DM support for UART, MMC, a DM_RESET driver, pinctrl ...), so it
> takes a bit of time.

I tried enabling DM for MMC on A64 recently and unfortunately it results in
SPL exceeding 32kb size limit.
So I guess we'll have to find a workaround for this issue as well...

> Fixing the current ad-hoc solution somewhat properly with ref-counting
> is not easy (two driver files) and not really worthwhile, I believe, but
> we can make it work like described above until the proper solution comes
> into play.
>
> Cheers,
> Andre.

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[André Przywara]

On 07/04/2018 01:25 AM, Vasily Khoruzhick wrote:
> On Tue, Jul 3, 2018 at 2:22 PM, André Przywara <andre.przywara@arm.com> wrote:
> 
>>> Adding a few more people to the list.  It looks like b62cdbddedc3 was in
>>> response to fef73766d9ad.  So, this close to the release, what do we
>>> need to do to get things back to the state things were in for v2018.05?
>>> And then what are the combinations that aren't working and need to be
>>> addressed again in v2018.09 so that we can make forward progress?
>>
>> Without going into much detail here, the clock dependency for two
>> companion controllers on those A64 is weird, and we hack it somehow
>> since we don't have a DM_CLK driver for sunxi (yet, see below). That
>> works for init, since we just set already set bits. For shutdown, we
>> *happen* to take down the AHB gate clocks and resets correctly, because
>> the order of shutdown matches the dependency (EHCI first, the OHCI). Not
>> sure if this is intentional, though. It's fragile, but works.
>>
>> What we don't (and can't easily) model is another oddity: the USB clock
>> for [OE]HCI0 is actually the parent of [OE]HCI1. So if we need to bring
>> down *two* controllers and do it in the natural order, the second one is
>> dead before it can be disabled properly.
>>
>> This was somewhat hidden before, since we had only one controller in
>> operation. b62cdbddedc3 somewhat fixed that, but the DT for the Pine64
>> (which was the test vehicle) has the controller still disabled. Enabling
>> this (what I did) or using other boards (BananaPi and NanoPi from Jagan)
>> triggered this bug though.
>> AFAICS this parent relation only affects the A64 with its two
>> controllers, so:
>> - We could just fix it for now by *not* disabling the USB clocks (and
>> only those, gates and reset still go down) at all. This is basically one
>> part of my patch from yesterday (the second part is not needed).
>> - We do more effort and skip disabling for OHCI0, but disable both
>> clocks for OHCI1. This still relies on the order, but would probably
>> shut down the controllers. A bit hackish to implement, though.
>> I will try the second solution now and let you decide on the list.
>>
>> Long term:
>> The proper solution is a DT based DM_CLK driver, which models it like
>> Linux does. Work is underway[1], but this somewhat opens a can of worms
>> (needs DM support for UART, MMC, a DM_RESET driver, pinctrl ...), so it
>> takes a bit of time.
> 
> I tried enabling DM for MMC on A64 recently and unfortunately it results in
> SPL exceeding 32kb size limit.

Mmh, worked for me with this preliminary branch:
[1] https://github.com/apritzel/u-boot/commits/sunxi-dm-WIP
(which I forgot to link in my original email).
Did you enable SPL_DM_MMC or SPL_DM as well? I think there is not much
benefit in doing so, especially given the code size issue.

> So I guess we'll have to find a workaround for this issue as well...

I recently made a patch that shrinks the ARMv8 exception table code by
250 bytes. Not much, but might help. How much did you overflow?
I think eventually we should generate the exception table and put it
somewhere else than in SRAM A1. That has the potential of freeing up
about 2KB or so. I started rewriting the vectors to make this easier,
but it's not very high on my TODO list.

Cheers,
Andre.


>> Fixing the current ad-hoc solution somewhat properly with ref-counting
>> is not easy (two driver files) and not really worthwhile, I believe, but
>> we can make it work like described above until the proper solution comes
>> into play.
>>
>> Cheers,
>> Andre.

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Vasily Khoruzhick]

On Tue, Jul 3, 2018 at 5:45 PM, André Przywara <andre.przywara@arm.com> wrote:

>>
>> I tried enabling DM for MMC on A64 recently and unfortunately it results in
>> SPL exceeding 32kb size limit.
>
> Mmh, worked for me with this preliminary branch:
> [1] https://github.com/apritzel/u-boot/commits/sunxi-dm-WIP
> (which I forgot to link in my original email).
> Did you enable SPL_DM_MMC or SPL_DM as well? I think there is not much
> benefit in doing so, especially given the code size issue.

Yes, I did. We'll need it to handle all the controller quirks
eventually (new clock mode, calibration, delays).
Currently we have a number of ifdefs in the driver.

>> So I guess we'll have to find a workaround for this issue as well...
>
> I recently made a patch that shrinks the ARMv8 exception table code by
> 250 bytes. Not much, but might help. How much did you overflow?
> I think eventually we should generate the exception table and put it
> somewhere else than in SRAM A1. That has the potential of freeing up
> about 2KB or so. I started rewriting the vectors to make this easier,
> but it's not very high on my TODO list.

That won't be enough:

aarch64-linux-gnu-ld.bfd: region `.sram' overflowed by 10584 bytes

>
> Cheers,
> Andre.

[U-Boot] [PATCH] arm64: allwinner: a64: Disable ehci1 and ohci1 for bananapi, nanopi

[Andre Przywara]

Hi,

On 04/07/18 01:50, Vasily Khoruzhick wrote:
> On Tue, Jul 3, 2018 at 5:45 PM, André Przywara <andre.przywara@arm.com> wrote:
> 
>>>
>>> I tried enabling DM for MMC on A64 recently and unfortunately it results in
>>> SPL exceeding 32kb size limit.
>>
>> Mmh, worked for me with this preliminary branch:
>> [1] https://github.com/apritzel/u-boot/commits/sunxi-dm-WIP
>> (which I forgot to link in my original email).
>> Did you enable SPL_DM_MMC or SPL_DM as well? I think there is not much
>> benefit in doing so, especially given the code size issue.
> 
> Yes, I did. We'll need it to handle all the controller quirks
> eventually (new clock mode, calibration, delays).
> Currently we have a number of ifdefs in the driver.

Yes, and they somewhat have to stay because of that, and we might have
to add some more, even. That's admittedly not pretty, and we were
discussing that already (the other thread that Jagan pointed you to).
But for the SPL specifically we just need some basic functionality to
load 600KB or so of data.
Or did you experience any problems with that? In general I think we can
get away with less performance for the sake of a simpler driver. For
instance I believe we will probably never need support for HS modes.

So yes: the usefulness of the DM_MMC conversion is somewhat questionable
because of this. As you have shown below ("overflowed by 10K") DM_MMC is
not really an option for the SPL, so we need to keep the old code
around. Or we follow the idea of having a much simpler, separate driver
just for the SPL.

>>> So I guess we'll have to find a workaround for this issue as well...
>>
>> I recently made a patch that shrinks the ARMv8 exception table code by
>> 250 bytes. Not much, but might help. How much did you overflow?
>> I think eventually we should generate the exception table and put it
>> somewhere else than in SRAM A1. That has the potential of freeing up
>> about 2KB or so. I started rewriting the vectors to make this easier,
>> but it's not very high on my TODO list.
> 
> That won't be enough:
> 
> aarch64-linux-gnu-ld.bfd: region `.sram' overflowed by 10584 bytes

Well, I don't see a way of achieving this, really. I think adding
SPL_DM_MMC would even overflow a 32-bit (Thumb2) SPL. And I am not sure
that adding a TPL is the right answer to this problem of using the
driver model. It's a boot loader, after all.

Cheers,
Andre.