| Message ID | 1549295637-24890-1-git-send-email-rogerq@ti.com |
|---|---|
| Headers | show |
| Series | AM57xx: PRU ICSS Support | expand |
* Roger Quadros <rogerq@ti.com> [190204 15:54]: > +static int sysc_enable_pruss(struct sysc *sysc) > +{ > + int i; > + u32 reg; > + bool ready; > + > + /* configure for Smart Idle & Smart Standby */ > + reg = sysc_read(sysc, sysc->offsets[SYSC_SYSCONFIG]); > + reg &= ~(SYSC_PRUSS_STANDBY_MASK | SYSC_PRUSS_IDLE_MASK); > + reg |= SYSC_PRUSS_STANDBY_SMART | SYSC_IDLE_SMART; > + sysc_write(sysc, sysc->offsets[SYSC_SYSCONFIG], reg); I think you can get rid of the SYSC_PRUSS_ defines here if you define the bits for it in struct sysc_regbits. The idle modes are SYSC_IDLE_* defines we already have in include/dt-bindings/bus/ti-sysc.h. My guess is these will just become generic sysc_enable() and sysc_disable() functions :) If you need module specific handling, you could add function pointers for enable and disable to struct sysc_capabilities. > @@ -649,6 +693,9 @@ static int __maybe_unused sysc_runtime_suspend(struct device *dev) > goto idled; > } > > + if (ddata->cap->type == TI_SYSC_PRUSS) > + sysc_disable_pruss(ddata); > + > for (i = 0; i < ddata->nr_clocks; i++) { > if (IS_ERR_OR_NULL(ddata->clocks[i])) > continue; Ideally this would be just unconditional call to generic sysc_disable() here for non-legacy mode. Then if module specific enable and disable are there, sysc_enable() and disable() can call them. > +static const struct sysc_regbits sysc_regbits_pruss = { > + .midle_shift = -ENODEV, > + .clkact_shift = -ENODEV, > + .sidle_shift = -ENODEV, > + .enwkup_shift = -ENODEV, > + .srst_shift = -ENODEV, > + .autoidle_shift = -ENODEV, > + .dmadisable_shift = -ENODEV, > + .emufree_shift = -ENODEV, > +}; So it seems you should populate at least midle_shift and sidle_shift bits here as in PRUSS_SYSCFG. I think STANDBY_MODE offset should go into the .midle_shift as it mentions initiator in TRM, and IDLE_MODE offset should go into .sidle_shift. So this might be really just using sysc_regbits_omap4_simple except it has an additional STANDBY_INIT bit which you could add for struct sysc_regbits if we don't have something similar already. > @@ -1702,6 +1772,10 @@ static int sysc_probe(struct platform_device *pdev) > > INIT_DELAYED_WORK(&ddata->idle_work, ti_sysc_idle); > > + /* FIXME: how to ensure PRUSS stays enabled? */ > + if (ddata->cap->type == TI_SYSC_PRUSS) > + goto skip_pm_put; > + > /* At least earlycon won't survive without deferred idle */ > if (ddata->cfg.quirks & (SYSC_QUIRK_NO_IDLE_ON_INIT | > SYSC_QUIRK_NO_RESET_ON_INIT)) { Hmm so do you need to specify ti,no-idle-on-init or what's the logic needed here? > diff --git a/include/dt-bindings/bus/ti-sysc.h b/include/dt-bindings/bus/ti-sysc.h > index 8ec78e8..7138384 100644 > --- a/include/dt-bindings/bus/ti-sysc.h > +++ b/include/dt-bindings/bus/ti-sysc.h > @@ -17,17 +17,6 @@ > > #define SYSC_DRA7_MCAN_ENAWAKEUP (1 << 4) > > -/* SYSCONFIG specific to PRUSS */ > -#define SYSC_PRUSS_SUB_MWAIT (1 << 5) > -#define SYSC_PRUSS_STANDBY_INIT (1 << 4) > - > -#define SYSC_PRUSS_STANDBY_FORCE (0 << 2) > -#define SYSC_PRUSS_STANDBY_NO (1 << 2) > -#define SYSC_PRUSS_STANDBY_SMART (2 << 2) > -#define SYSC_PRUSS_STANDBY_MASK (3 << 2) > - > -#define SYSC_PRUSS_IDLE_MASK 3 > - > /* SYSCONFIG STANDBYMODE/MIDLEMODE/SIDLEMODE supported by hardware */ > #define SYSC_IDLE_FORCE 0 > #define SYSC_IDLE_NO 1 I suggest you make this series independent of the rest of the PRUSS patches as we can add this separately. So no need to define these bits at all AFAIK. Regards, Tony
* Roger Quadros <rogerq@ti.com> [190204 15:54]: > --- a/arch/arm/boot/dts/dra7.dtsi > +++ b/arch/arm/boot/dts/dra7.dtsi > @@ -167,6 +167,200 @@ > l4_per3: interconnect@48800000 { > }; > > + pru_icss1: target-module@4b200000 { I suggest you add these into dra7-pruss.dtsi as they have internal interconnects. And eventually some of the *pruss.dtsi files might become shared :) And we avoid moving these around when we define the l3 interconnects and avoid a lot of extra long dtsi line lengths with the added nestedness later on. Regards, Tony
Tony, On 04/02/19 20:00, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [190204 15:54]: >> +static int sysc_enable_pruss(struct sysc *sysc) >> +{ >> + int i; >> + u32 reg; >> + bool ready; >> + >> + /* configure for Smart Idle & Smart Standby */ >> + reg = sysc_read(sysc, sysc->offsets[SYSC_SYSCONFIG]); >> + reg &= ~(SYSC_PRUSS_STANDBY_MASK | SYSC_PRUSS_IDLE_MASK); >> + reg |= SYSC_PRUSS_STANDBY_SMART | SYSC_IDLE_SMART; >> + sysc_write(sysc, sysc->offsets[SYSC_SYSCONFIG], reg); > > I think you can get rid of the SYSC_PRUSS_ defines here > if you define the bits for it in struct sysc_regbits. The > idle modes are SYSC_IDLE_* defines we already have in > include/dt-bindings/bus/ti-sysc.h. > > My guess is these will just become generic sysc_enable() > and sysc_disable() functions :) > > If you need module specific handling, you could add function > pointers for enable and disable to struct sysc_capabilities. OK. I'll move all this to a generic handler then. > >> @@ -649,6 +693,9 @@ static int __maybe_unused sysc_runtime_suspend(struct device *dev) >> goto idled; >> } >> >> + if (ddata->cap->type == TI_SYSC_PRUSS) >> + sysc_disable_pruss(ddata); >> + >> for (i = 0; i < ddata->nr_clocks; i++) { >> if (IS_ERR_OR_NULL(ddata->clocks[i])) >> continue; > > Ideally this would be just unconditional call to generic > sysc_disable() here for non-legacy mode. Then if module > specific enable and disable are there, sysc_enable() and > disable() can call them. OK. > >> +static const struct sysc_regbits sysc_regbits_pruss = { >> + .midle_shift = -ENODEV, >> + .clkact_shift = -ENODEV, >> + .sidle_shift = -ENODEV, >> + .enwkup_shift = -ENODEV, >> + .srst_shift = -ENODEV, >> + .autoidle_shift = -ENODEV, >> + .dmadisable_shift = -ENODEV, >> + .emufree_shift = -ENODEV, >> +}; > > So it seems you should populate at least midle_shift and sidle_shift > bits here as in PRUSS_SYSCFG. I think STANDBY_MODE offset should go > into the .midle_shift as it mentions initiator in TRM, and IDLE_MODE > offset should go into .sidle_shift. So this might be really just using > sysc_regbits_omap4_simple except it has an additional STANDBY_INIT > bit which you could add for struct sysc_regbits if we don't have > something similar already. Got it. > >> @@ -1702,6 +1772,10 @@ static int sysc_probe(struct platform_device *pdev) >> >> INIT_DELAYED_WORK(&ddata->idle_work, ti_sysc_idle); >> >> + /* FIXME: how to ensure PRUSS stays enabled? */ >> + if (ddata->cap->type == TI_SYSC_PRUSS) >> + goto skip_pm_put; >> + This was my bad. I forgot to move the pm_runtime_enable/get from the old pruss_soc_bus.c to pruss.c :). It work after than and this hack is not required. >> /* At least earlycon won't survive without deferred idle */ >> if (ddata->cfg.quirks & (SYSC_QUIRK_NO_IDLE_ON_INIT | >> SYSC_QUIRK_NO_RESET_ON_INIT)) { > > Hmm so do you need to specify ti,no-idle-on-init or what's > the logic needed here? I was using "ti,no-reset-on-init" but that was because sysc_reset() was returning error due to missing syss mask. That can be fixed like so. diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c index 5b9c81a..f5f2000 100644 --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -941,6 +941,7 @@ static int sysc_reset(struct sysc *ddata) int val; if (ddata->legacy_mode || offset < 0 || + ddata->cap->regbits->srst_shift == -ENODEV || ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT) return 0; > >> diff --git a/include/dt-bindings/bus/ti-sysc.h b/include/dt-bindings/bus/ti-sysc.h >> index 8ec78e8..7138384 100644 >> --- a/include/dt-bindings/bus/ti-sysc.h >> +++ b/include/dt-bindings/bus/ti-sysc.h >> @@ -17,17 +17,6 @@ >> >> #define SYSC_DRA7_MCAN_ENAWAKEUP (1 << 4) >> >> -/* SYSCONFIG specific to PRUSS */ >> -#define SYSC_PRUSS_SUB_MWAIT (1 << 5) >> -#define SYSC_PRUSS_STANDBY_INIT (1 << 4) >> - >> -#define SYSC_PRUSS_STANDBY_FORCE (0 << 2) >> -#define SYSC_PRUSS_STANDBY_NO (1 << 2) >> -#define SYSC_PRUSS_STANDBY_SMART (2 << 2) >> -#define SYSC_PRUSS_STANDBY_MASK (3 << 2) >> - >> -#define SYSC_PRUSS_IDLE_MASK 3 >> - >> /* SYSCONFIG STANDBYMODE/MIDLEMODE/SIDLEMODE supported by hardware */ >> #define SYSC_IDLE_FORCE 0 >> #define SYSC_IDLE_NO 1 > > I suggest you make this series independent of the > rest of the PRUSS patches as we can add this > separately. So no need to define these bits at all > AFAIK. OK. Thanks. cheers, -roger > > Regards, > > Tony > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 25/02/2019 23:26, Rob Herring wrote: > On Mon, Feb 04, 2019 at 05:53:55PM +0200, Roger Quadros wrote: >> The PRUSS module has a SYSCFG which is unique. Add >> support for it. >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/bus/ti-sysc.c | 77 +++++++++++++++++++++++++++++++++++ >> include/dt-bindings/bus/ti-sysc.h | 11 ----- > > Did you intend to remove what you just added? Yes, it was a mistake. > >> include/linux/platform_data/ti-sysc.h | 1 + >> 3 files changed, 78 insertions(+), 11 deletions(-) cheers, -roger -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
* Roger Quadros <rogerq@ti.com> [190329 14:02]: > Hi Tony, > > On 04/02/2019 20:03, Tony Lindgren wrote: > > * Roger Quadros <rogerq@ti.com> [190204 15:54]: > >> --- a/arch/arm/boot/dts/dra7.dtsi > >> +++ b/arch/arm/boot/dts/dra7.dtsi > >> @@ -167,6 +167,200 @@ > >> l4_per3: interconnect@48800000 { > >> }; > >> > >> + pru_icss1: target-module@4b200000 { > > > > I suggest you add these into dra7-pruss.dtsi as they > > have internal interconnects. And eventually some of > > the *pruss.dtsi files might become shared :) And we > > avoid moving these around when we define the l3 > > interconnects and avoid a lot of extra long dtsi > > line lengths with the added nestedness later on. > > PRUSS is only present on AM57x variants. > I will move this to am57x-pruss.dtsi. > > This means all AM57x boards will have to include it. > > Is this OK? Sure sounds good to me. Regards, Tony