Re: ofw_clock: reset_deassert and clock_enable order of use Qs

[Mark Kettenis]

> Date: Sun, 12 Nov 2017 23:53:10 +0200
> From: Artturi Alm 
> 
> On Sat, Oct 07, 2017 at 10:16:05AM +0300, Artturi Alm wrote:
> > Hi,
> > 
> > 
> > what was the cause of these delays? i just spotted this, so untested on
> > likely more affected HW(sunxi A64/H3 and rockchips), but just for discussion
> > about the delays/ordering? are they really needed like that?
> > 
> > i don't remember having read anything about the order of these from
> > devicetree-binding .txt's i have read so far from u-boot/linux.
> > 
> > "Make sure that the reset signal has been released
> > before the release of module clock gating;"
> > 
> > above can be found from under the short CCU "x.3.6 Programming 
> > Guidelines"[0].
> > maybe this ordering should be verified from somewhere else, as atleast
> > for my sorry non-nativeNlazy grammar i could understand wrong, what is being
> > meant w/"the release" above.. but even w/above discarded, my intuition does
> > favor deasserting reset before gating sclki, hence the diff.
> > 
> > the diff below booted faster with no observable changes in dmesg on cubie2,
> > and otherwise succesfully on panda and wandboard.
> > 
> > -Artturi
> > 
> 
> Ping?
> 
> i think i've passed by sources in both fbsd since email above
> suggesting the order should be like in the diff.
> does anyone want me to provide more on my findings about this,
> or anything else?

As far as I can tell, the Linux code always enables the clock before
deasserting the reset.  If the ordering is SoC-specific, I guess the
clock/reset controller driver should handle it as some of these
drivers are not SoC-specific.

For that reason I'm going to commit the sxitwi diff with the
"canonical" ordering.

Cheers,

Mark

> > [0] sources:
> > A64 User Manual(Revision 1.0):
> > "3.3.6.4. Gating and reset" Page 147
> > H3 Datasheet(Revision1.2):
> > "4.3.6.4. Gating and reset" Page 142
> > 
> > 
> > 
> > diff --git a/sys/dev/fdt/ehci_fdt.c b/sys/dev/fdt/ehci_fdt.c
> > index 55fe75f1a9c..fae54ac11ee 100644
> > --- a/sys/dev/fdt/ehci_fdt.c
> > +++ b/sys/dev/fdt/ehci_fdt.c
> > @@ -91,9 +91,10 @@ ehci_fdt_attach(struct device *parent, struct device 
> > *self, void *aux)
> >  
> > pinctrl_byname(sc->sc_node, "default");
> >  
> > -   clock_enable_all(sc->sc_node);
> > reset_deassert_all(sc->sc_node);
> >  
> > +   clock_enable_all(sc->sc_node);
> > +
> > /* Disable interrupts, so we don't get any spurious ones. */
> > sc->sc.sc_offs = EREAD1(>sc, EHCI_CAPLENGTH);
> > EOWRITE2(>sc, EHCI_USBINTR, 0);
> > diff --git a/sys/dev/fdt/if_dwge_fdt.c b/sys/dev/fdt/if_dwge_fdt.c
> > index edfe5acb992..00668980f4a 100644
> > --- a/sys/dev/fdt/if_dwge_fdt.c
> > +++ b/sys/dev/fdt/if_dwge_fdt.c
> > @@ -125,10 +125,10 @@ dwge_fdt_attach(struct device *parent, struct device 
> > *self, void *aux)
> > else if (OF_is_compatible(faa->fa_node, "rockchip,rk3399-gmac"))
> > dwge_fdt_attach_rockchip(fsc);
> >  
> > +   reset_deassert(faa->fa_node, "stmmaceth");
> > +
> > /* Enable clock. */
> > clock_enable(faa->fa_node, "stmmaceth");
> > -   reset_deassert(faa->fa_node, "stmmaceth");
> > -   delay(5000);
> >  
> > /* Power up PHY. */
> > phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0);
> > diff --git a/sys/dev/fdt/if_dwxe.c b/sys/dev/fdt/if_dwxe.c
> > index 22a383c06ef..6e303745ec3 100644
> > --- a/sys/dev/fdt/if_dwxe.c
> > +++ b/sys/dev/fdt/if_dwxe.c
> > @@ -376,10 +376,10 @@ dwxe_attach(struct device *parent, struct device 
> > *self, void *aux)
> >  
> > pinctrl_byname(faa->fa_node, "default");
> >  
> > +   reset_deassert(faa->fa_node, "stmmaceth");
> > +
> > /* Enable clock. */
> > clock_enable(faa->fa_node, "stmmaceth");
> > -   reset_deassert(faa->fa_node, "stmmaceth");
> > -   delay(5000);
> >  
> > /* Power up PHY. */
> > phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0);
> > diff --git a/sys/dev/fdt/sximmc.c b/sys/dev/fdt/sximmc.c
> > index 7acb8f55d56..9ed2ae09fe4 100644
> > --- a/sys/dev/fdt/sximmc.c
> > +++ b/sys/dev/fdt/sximmc.c
> > @@ -366,11 +366,10 @@ sximmc_attach(struct device *parent, struct device 
> > *self, void *aux)
> >  
> > pinctrl_byname(faa->fa_node, "default");
> >  
> > +   reset_deassert_all(faa->fa_node);
> > +
> > /* enable clock */
> > clock_enable(faa->fa_node, NULL);
> > -   delay(5000);
> > -
> > -   reset_deassert_all(faa->fa_node);
> >  
> > /*
> >  * The FIFO register is in a different location on the
> > diff --git a/sys/dev/fdt/sxitwi.c b/sys/dev/fdt/sxitwi.c
> > index f53f2bfd594..c80bdc2ab06 100644
> > --- a/sys/dev/fdt/sxitwi.c
> > +++ b/sys/dev/fdt/sxitwi.c
> > @@ -218,6 +218,8 @@ sxitwi_attach(struct device *parent, struct device 
> > *self, void *aux)
> >  
> > pinctrl_byname(faa->fa_node, "default");
> >  
> > +   reset_deassert_all(faa->fa_node);
> > +
> > /* Enable clock */
> > clock_enable(faa->fa_node, NULL);
> >  
> 

Re: ofw_clock: reset_deassert and clock_enable order of use Qs

[Artturi Alm]

On Sat, Oct 07, 2017 at 10:16:05AM +0300, Artturi Alm wrote:
> Hi,
> 
> 
> what was the cause of these delays? i just spotted this, so untested on
> likely more affected HW(sunxi A64/H3 and rockchips), but just for discussion
> about the delays/ordering? are they really needed like that?
> 
> i don't remember having read anything about the order of these from
> devicetree-binding .txt's i have read so far from u-boot/linux.
> 
> "Make sure that the reset signal has been released
> before the release of module clock gating;"
> 
> above can be found from under the short CCU "x.3.6 Programming Guidelines"[0].
> maybe this ordering should be verified from somewhere else, as atleast
> for my sorry non-nativeNlazy grammar i could understand wrong, what is being
> meant w/"the release" above.. but even w/above discarded, my intuition does
> favor deasserting reset before gating sclki, hence the diff.
> 
> the diff below booted faster with no observable changes in dmesg on cubie2,
> and otherwise succesfully on panda and wandboard.
> 
> -Artturi
> 

Ping?

i think i've passed by sources in both fbsd since email above
suggesting the order should be like in the diff.
does anyone want me to provide more on my findings about this,
or anything else?

-Artturi

> [0] sources:
> A64 User Manual(Revision 1.0):
> "3.3.6.4. Gating and reset" Page 147
> H3 Datasheet(Revision1.2):
> "4.3.6.4. Gating and reset" Page 142
> 
> 
> 
> diff --git a/sys/dev/fdt/ehci_fdt.c b/sys/dev/fdt/ehci_fdt.c
> index 55fe75f1a9c..fae54ac11ee 100644
> --- a/sys/dev/fdt/ehci_fdt.c
> +++ b/sys/dev/fdt/ehci_fdt.c
> @@ -91,9 +91,10 @@ ehci_fdt_attach(struct device *parent, struct device 
> *self, void *aux)
>  
>   pinctrl_byname(sc->sc_node, "default");
>  
> - clock_enable_all(sc->sc_node);
>   reset_deassert_all(sc->sc_node);
>  
> + clock_enable_all(sc->sc_node);
> +
>   /* Disable interrupts, so we don't get any spurious ones. */
>   sc->sc.sc_offs = EREAD1(>sc, EHCI_CAPLENGTH);
>   EOWRITE2(>sc, EHCI_USBINTR, 0);
> diff --git a/sys/dev/fdt/if_dwge_fdt.c b/sys/dev/fdt/if_dwge_fdt.c
> index edfe5acb992..00668980f4a 100644
> --- a/sys/dev/fdt/if_dwge_fdt.c
> +++ b/sys/dev/fdt/if_dwge_fdt.c
> @@ -125,10 +125,10 @@ dwge_fdt_attach(struct device *parent, struct device 
> *self, void *aux)
>   else if (OF_is_compatible(faa->fa_node, "rockchip,rk3399-gmac"))
>   dwge_fdt_attach_rockchip(fsc);
>  
> + reset_deassert(faa->fa_node, "stmmaceth");
> +
>   /* Enable clock. */
>   clock_enable(faa->fa_node, "stmmaceth");
> - reset_deassert(faa->fa_node, "stmmaceth");
> - delay(5000);
>  
>   /* Power up PHY. */
>   phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0);
> diff --git a/sys/dev/fdt/if_dwxe.c b/sys/dev/fdt/if_dwxe.c
> index 22a383c06ef..6e303745ec3 100644
> --- a/sys/dev/fdt/if_dwxe.c
> +++ b/sys/dev/fdt/if_dwxe.c
> @@ -376,10 +376,10 @@ dwxe_attach(struct device *parent, struct device *self, 
> void *aux)
>  
>   pinctrl_byname(faa->fa_node, "default");
>  
> + reset_deassert(faa->fa_node, "stmmaceth");
> +
>   /* Enable clock. */
>   clock_enable(faa->fa_node, "stmmaceth");
> - reset_deassert(faa->fa_node, "stmmaceth");
> - delay(5000);
>  
>   /* Power up PHY. */
>   phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0);
> diff --git a/sys/dev/fdt/sximmc.c b/sys/dev/fdt/sximmc.c
> index 7acb8f55d56..9ed2ae09fe4 100644
> --- a/sys/dev/fdt/sximmc.c
> +++ b/sys/dev/fdt/sximmc.c
> @@ -366,11 +366,10 @@ sximmc_attach(struct device *parent, struct device 
> *self, void *aux)
>  
>   pinctrl_byname(faa->fa_node, "default");
>  
> + reset_deassert_all(faa->fa_node);
> +
>   /* enable clock */
>   clock_enable(faa->fa_node, NULL);
> - delay(5000);
> -
> - reset_deassert_all(faa->fa_node);
>  
>   /*
>* The FIFO register is in a different location on the
> diff --git a/sys/dev/fdt/sxitwi.c b/sys/dev/fdt/sxitwi.c
> index f53f2bfd594..c80bdc2ab06 100644
> --- a/sys/dev/fdt/sxitwi.c
> +++ b/sys/dev/fdt/sxitwi.c
> @@ -218,6 +218,8 @@ sxitwi_attach(struct device *parent, struct device *self, 
> void *aux)
>  
>   pinctrl_byname(faa->fa_node, "default");
>  
> + reset_deassert_all(faa->fa_node);
> +
>   /* Enable clock */
>   clock_enable(faa->fa_node, NULL);
>