| Message ID | 20190325215849.13182-10-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ti-sysc changes to probe devices with dts data only | expand |
* Tony Lindgren <tony@atomide.com> [190325 22:00]: > We should not do the reset until the clocks are enabled. Let's only init > restctrl in sysc_init_resets() and do the reset later on in sysc_reset(). ... > static int sysc_reset(struct sysc *ddata) > { > int offset = ddata->offsets[SYSC_SYSCONFIG]; > - int val; > + int error, val; > > if (ddata->legacy_mode || offset < 0 || > ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) > - return 0; > + return sysc_rstctrl_reset_deassert(ddata, false); > + > + error = sysc_rstctrl_reset_deassert(ddata, true); > + if (error) > + return error; This change is wrong, we need to deassert rstctrl reset before we enable clocks, not after. Updated version below. Regards, Tony 8< -------------------- From tony Mon Sep 17 00:00:00 2001 From: Tony Lindgren <tony@atomide.com> Date: Thu, 21 Mar 2019 11:00:21 -0700 Subject: [PATCH] bus: ti-sysc: Move rstctrl reset to happen later We can do the rsstctrl a bit later, but need to deassert rstctrl reset before the clocks are enabled if asserted. Let's only init restctrl in sysc_init_resets() and do the reset later on just before we enable the device clocks. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/bus/ti-sysc.c | 61 +++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -339,38 +339,18 @@ static void sysc_disable_opt_clocks(struct sysc *ddata) } /** - * sysc_init_resets - reset module on init + * sysc_init_resets - init rstctrl reset line if configured * @ddata: device driver data * - * A module can have both OCP softreset control and external rstctrl. - * If more complicated rstctrl resets are needed, please handle these - * directly from the child device driver and map only the module reset - * for the parent interconnect target module device. - * - * Automatic reset of the module on init can be skipped with the - * "ti,no-reset-on-init" device tree property. + * See sysc_rstctrl_reset_deassert(). */ static int sysc_init_resets(struct sysc *ddata) { - int error; - ddata->rsts = devm_reset_control_array_get_optional_exclusive(ddata->dev); if (IS_ERR(ddata->rsts)) return PTR_ERR(ddata->rsts); - if (ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) - goto deassert; - - error = reset_control_assert(ddata->rsts); - if (error) - return error; - -deassert: - error = reset_control_deassert(ddata->rsts); - if (error) - return error; - return 0; } @@ -1031,6 +1011,35 @@ static int sysc_legacy_init(struct sysc *ddata) return error; } +/** + * sysc_rstctrl_reset_deassert - deassert rstctrl reset + * @ddata: device driver data + * @reset: reset before deassert + * + * A module can have both OCP softreset control and external rstctrl. + * If more complicated rstctrl resets are needed, please handle these + * directly from the child device driver and map only the module reset + * for the parent interconnect target module device. + * + * Automatic reset of the module on init can be skipped with the + * "ti,no-reset-on-init" device tree property. + */ +static int sysc_rstctrl_reset_deassert(struct sysc *ddata, bool reset) +{ + int error; + + if (!ddata->rsts) + return 0; + + if (reset) { + error = reset_control_assert(ddata->rsts); + if (error) + return error; + } + + return reset_control_deassert(ddata->rsts); +} + static int sysc_reset(struct sysc *ddata) { int offset = ddata->offsets[SYSC_SYSCONFIG]; @@ -1071,6 +1080,14 @@ static int sysc_init_module(struct sysc *ddata) { int error = 0; bool manage_clocks = true; + bool reset = true; + + if (ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) + reset = false; + + error = sysc_rstctrl_reset_deassert(ddata, reset); + if (error) + return error; if (ddata->cfg.quirks & (SYSC_QUIRK_NO_IDLE | SYSC_QUIRK_NO_IDLE_ON_INIT))
Hi Tony, On 3/26/19 6:13 PM, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [190325 22:00]: >> We should not do the reset until the clocks are enabled. Let's only init >> restctrl in sysc_init_resets() and do the reset later on in sysc_reset(). > ... > >> static int sysc_reset(struct sysc *ddata) >> { >> int offset = ddata->offsets[SYSC_SYSCONFIG]; >> - int val; >> + int error, val; >> >> if (ddata->legacy_mode || offset < 0 || >> ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) >> - return 0; >> + return sysc_rstctrl_reset_deassert(ddata, false); >> + >> + error = sysc_rstctrl_reset_deassert(ddata, true); >> + if (error) >> + return error; > > This change is wrong, we need to deassert rstctrl reset before > we enable clocks, not after. Updated version below. Hmm, are you envisioning the SYSC reset (OCP SoftReset) here or the PRCM RSTCTRL hardresets here? The latter in general requires the clocks to be running first (module won't be in ready status until you deassert the hardresets with clocks running). You can look up the Warm-reset or Cold-reset sequences in the TRMs for any of the processors. I am working on preparing the next version of PRUSS patches with ti-sysc on AM33xx/AM437x/AM57xx platforms, so will pick up these patches for my testing. regards Suman > > Regards, > > Tony > > 8< -------------------- > From tony Mon Sep 17 00:00:00 2001 > From: Tony Lindgren <tony@atomide.com> > Date: Thu, 21 Mar 2019 11:00:21 -0700 > Subject: [PATCH] bus: ti-sysc: Move rstctrl reset to happen later > > We can do the rsstctrl a bit later, but need to deassert rstctrl reset > before the clocks are enabled if asserted. Let's only init restctrl > in sysc_init_resets() and do the reset later on just before we enable > the device clocks. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/bus/ti-sysc.c | 61 +++++++++++++++++++++++++++---------------- > 1 file changed, 39 insertions(+), 22 deletions(-) > > diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c > --- a/drivers/bus/ti-sysc.c > +++ b/drivers/bus/ti-sysc.c > @@ -339,38 +339,18 @@ static void sysc_disable_opt_clocks(struct sysc *ddata) > } > > /** > - * sysc_init_resets - reset module on init > + * sysc_init_resets - init rstctrl reset line if configured > * @ddata: device driver data > * > - * A module can have both OCP softreset control and external rstctrl. > - * If more complicated rstctrl resets are needed, please handle these > - * directly from the child device driver and map only the module reset > - * for the parent interconnect target module device. > - * > - * Automatic reset of the module on init can be skipped with the > - * "ti,no-reset-on-init" device tree property. > + * See sysc_rstctrl_reset_deassert(). > */ > static int sysc_init_resets(struct sysc *ddata) > { > - int error; > - > ddata->rsts = > devm_reset_control_array_get_optional_exclusive(ddata->dev); > if (IS_ERR(ddata->rsts)) > return PTR_ERR(ddata->rsts); > > - if (ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) > - goto deassert; > - > - error = reset_control_assert(ddata->rsts); > - if (error) > - return error; > - > -deassert: > - error = reset_control_deassert(ddata->rsts); > - if (error) > - return error; > - > return 0; > } > > @@ -1031,6 +1011,35 @@ static int sysc_legacy_init(struct sysc *ddata) > return error; > } > > +/** > + * sysc_rstctrl_reset_deassert - deassert rstctrl reset > + * @ddata: device driver data > + * @reset: reset before deassert > + * > + * A module can have both OCP softreset control and external rstctrl. > + * If more complicated rstctrl resets are needed, please handle these > + * directly from the child device driver and map only the module reset > + * for the parent interconnect target module device. > + * > + * Automatic reset of the module on init can be skipped with the > + * "ti,no-reset-on-init" device tree property. > + */ > +static int sysc_rstctrl_reset_deassert(struct sysc *ddata, bool reset) > +{ > + int error; > + > + if (!ddata->rsts) > + return 0; > + > + if (reset) { > + error = reset_control_assert(ddata->rsts); > + if (error) > + return error; > + } > + > + return reset_control_deassert(ddata->rsts); > +} > + > static int sysc_reset(struct sysc *ddata) > { > int offset = ddata->offsets[SYSC_SYSCONFIG]; > @@ -1071,6 +1080,14 @@ static int sysc_init_module(struct sysc *ddata) > { > int error = 0; > bool manage_clocks = true; > + bool reset = true; > + > + if (ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) > + reset = false; > + > + error = sysc_rstctrl_reset_deassert(ddata, reset); > + if (error) > + return error; > > if (ddata->cfg.quirks & > (SYSC_QUIRK_NO_IDLE | SYSC_QUIRK_NO_IDLE_ON_INIT)) >
Hi, * Suman Anna <s-anna@ti.com> [190326 23:22]: > On 3/26/19 6:13 PM, Tony Lindgren wrote: > Hmm, are you envisioning the SYSC reset (OCP SoftReset) here or the PRCM > RSTCTRL hardresets here? The latter in general requires the clocks to be > running first (module won't be in ready status until you deassert the > hardresets with clocks running). You can look up the Warm-reset or > Cold-reset sequences in the TRMs for any of the processors. That's for rstctrl. I just did a quick test with my earlier reset-simple patch and I noticed sgx on am33xx produces a clock error unless we deassert it's rstrctrl before enabling clocks first: gfx-l3-clkctrl:0004:0: failed to enable > I am working on preparing the next version of PRUSS patches with ti-sysc > on AM33xx/AM437x/AM57xx platforms, so will pick up these patches for my > testing. OK great, yes please check and test with your rstctrl use case. I guess you still need to use the reset-simple patch for now until we have a proper prm rstctrl driver. Note that you probably also want to leave out the struct omap_hwmod data from omap_hwmod_*_data.c files with rstctrl entries. Regards, Tony
Hi Tony, On 3/26/19 6:40 PM, Tony Lindgren wrote: > Hi, > > * Suman Anna <s-anna@ti.com> [190326 23:22]: >> On 3/26/19 6:13 PM, Tony Lindgren wrote: >> Hmm, are you envisioning the SYSC reset (OCP SoftReset) here or the PRCM >> RSTCTRL hardresets here? The latter in general requires the clocks to be >> running first (module won't be in ready status until you deassert the >> hardresets with clocks running). You can look up the Warm-reset or >> Cold-reset sequences in the TRMs for any of the processors. > > That's for rstctrl. I just did a quick test with my earlier > reset-simple patch and I noticed sgx on am33xx produces a > clock error unless we deassert it's rstrctrl before enabling > clocks first: > > gfx-l3-clkctrl:0004:0: failed to enable Yeah, and I see a similar one across the other modules controlled by RSTCTRL bits for me as well - MMUs, PRUSS etc. This is because you can only check the module ready status in _omap4_clkctrl_clk_enable() only both after the clocks are turned on and resets are deasserted. That check will always fail with rstctrl asserted. The omap_hwmod code does use the reset status checks for bailing out, but that stuff is not present in clkctrl code and can only be achieved by adding a CLKF_NO_IDLEST (somewhat misnamed) to the corresponding clkctrl atm. See [1] for AM33xx SGX. I will be posting some of these once I check the behavior. > >> I am working on preparing the next version of PRUSS patches with ti-sysc >> on AM33xx/AM437x/AM57xx platforms, so will pick up these patches for my >> testing. > > OK great, yes please check and test with your rstctrl use case. > I guess you still need to use the reset-simple patch for now > until we have a proper prm rstctrl driver. > > Note that you probably also want to leave out the struct > omap_hwmod data from omap_hwmod_*_data.c files with rstctrl > entries. You mean no hwmod entries at all, or hwmod entries with no rstctrl data? regards Suman [1] http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=commitdiff;h=536d660714e98bdb7f96e5990a095283e52e4d8a
* Suman Anna <s-anna@ti.com> [190327 16:27]: > On 3/26/19 6:40 PM, Tony Lindgren wrote: > > That's for rstctrl. I just did a quick test with my earlier > > reset-simple patch and I noticed sgx on am33xx produces a > > clock error unless we deassert it's rstrctrl before enabling > > clocks first: > > > > gfx-l3-clkctrl:0004:0: failed to enable > > Yeah, and I see a similar one across the other modules controlled by > RSTCTRL bits for me as well - MMUs, PRUSS etc. This is because you can > only check the module ready status in _omap4_clkctrl_clk_enable() only > both after the clocks are turned on and resets are deasserted. That > check will always fail with rstctrl asserted. The omap_hwmod code does > use the reset status checks for bailing out, but that stuff is not > present in clkctrl code and can only be achieved by adding a > CLKF_NO_IDLEST (somewhat misnamed) to the corresponding clkctrl atm. Sounds like on ti-sysc init we should just deassert the rstctrl if asserted, then enable clocks, and then read the revision. Then if we actually need to toggle rstctrl reset, that can be added with later patches. But with reset driver, the device IP handling device driver(s) should probably manage the rstctrl bits directly. > See [1] for AM33xx SGX. I will be posting some of these once I check the > behavior. Yeah OK sounds like we can avoid those issues by deasserting the module related rstctrl bit before enabling the clocks. Then deal with resets later if needed. > > Note that you probably also want to leave out the struct > > omap_hwmod data from omap_hwmod_*_data.c files with rstctrl > > entries. > > You mean no hwmod entries at all, or hwmod entries with no rstctrl data? Except for the lack of rstctrl reset driver, struct hwmod_data entries should only be needed for the few cases where we're not yet handling some oh->flags quirks. I think most of the remaining unhandled quirks are for omap2 and 3. Regards, Tony > [1] > http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=commitdiff;h=536d660714e98bdb7f96e5990a095283e52e4d8a > >
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -339,38 +339,18 @@ static void sysc_disable_opt_clocks(struct sysc *ddata) } /** - * sysc_init_resets - reset module on init + * sysc_init_resets - init rstctrl reset line if configured * @ddata: device driver data * - * A module can have both OCP softreset control and external rstctrl. - * If more complicated rstctrl resets are needed, please handle these - * directly from the child device driver and map only the module reset - * for the parent interconnect target module device. - * - * Automatic reset of the module on init can be skipped with the - * "ti,no-reset-on-init" device tree property. + * See sysc_rstctrl_reset_deassert(). */ static int sysc_init_resets(struct sysc *ddata) { - int error; - ddata->rsts = devm_reset_control_array_get_optional_exclusive(ddata->dev); if (IS_ERR(ddata->rsts)) return PTR_ERR(ddata->rsts); - if (ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) - goto deassert; - - error = reset_control_assert(ddata->rsts); - if (error) - return error; - -deassert: - error = reset_control_deassert(ddata->rsts); - if (error) - return error; - return 0; } @@ -1031,14 +1011,47 @@ static int sysc_legacy_init(struct sysc *ddata) return error; } +/** + * sysc_rstctrl_reset_deassert - deassert rstctrl reset + * @ddata: device driver data + * @reset: reset before deassert + * + * A module can have both OCP softreset control and external rstctrl. + * If more complicated rstctrl resets are needed, please handle these + * directly from the child device driver and map only the module reset + * for the parent interconnect target module device. + * + * Automatic reset of the module on init can be skipped with the + * "ti,no-reset-on-init" device tree property. + */ +static int sysc_rstctrl_reset_deassert(struct sysc *ddata, bool reset) +{ + int error; + + if (!ddata->rsts) + return 0; + + if (reset) { + error = reset_control_assert(ddata->rsts); + if (error) + return error; + } + + return reset_control_deassert(ddata->rsts); +} + static int sysc_reset(struct sysc *ddata) { int offset = ddata->offsets[SYSC_SYSCONFIG]; - int val; + int error, val; if (ddata->legacy_mode || offset < 0 || ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) - return 0; + return sysc_rstctrl_reset_deassert(ddata, false); + + error = sysc_rstctrl_reset_deassert(ddata, true); + if (error) + return error; /* * Currently only support reset status in sysstatus.
We should not do the reset until the clocks are enabled. Let's only init restctrl in sysc_init_resets() and do the reset later on in sysc_reset(). Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/bus/ti-sysc.c | 61 ++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 24 deletions(-)