[PATCH v10 3/4] firmware: ti_sci: Introduce Power Management Ops

[Markus Schneider-Pargmann]

Hi Nishanth,

On Wed, Aug 28, 2024 at 10:24:23PM GMT, Nishanth Menon wrote:
> On 21:54-20240828, Markus Schneider-Pargmann wrote:
> > Hi Nishanth,
> > 
> > thanks for your review. I will integrate fixes for all your comments
> > into the next version.
> > 
> > On Mon, Aug 26, 2024 at 11:43:46AM GMT, Nishanth Menon wrote:
> > > On 08:39-20240814, Kevin Hilman wrote:
> > > [...]
> > > > diff --git a/include/linux/soc/ti/ti_sci_protocol.h b/include/linux/soc/ti/ti_sci_protocol.h
> > > > index 1f1871e23f76..4ba9e520a28f 100644
> > > > --- a/include/linux/soc/ti/ti_sci_protocol.h
> > > > +++ b/include/linux/soc/ti/ti_sci_protocol.h
> > > > @@ -199,6 +199,47 @@ struct ti_sci_clk_ops {
> > > >  #define TISCI_MSG_VALUE_IO_ENABLE			1
> > > >  #define TISCI_MSG_VALUE_IO_DISABLE			0
> > > >  
> > > > +/* TISCI LPM wake up sources */
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_WKUP_I2C0	0x00
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_WKUP_UART0	0x10
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_MCU_GPIO0	0x20
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_WKUP_ICEMELTER0	0x30
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_WKUP_TIMER0	0x40
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_WKUP_TIMER1	0x41
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_WKUP_RTC0	0x50
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_RESET		0x60
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_USB0		0x70
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_USB1		0x71
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_MAIN_IO		0x80
> > > > +#define TISCI_MSG_VALUE_LPM_WAKE_SOURCE_MCU_IO		0x81
> > > 
> > > ^^ I assume these are daisy chain sources. - will these remain OR do we
> > > have to consider the wake sources SoC dependent? If so, the
> > > sci_protocol.h will be SoC agnostic..
> > 
> > These are all wakeup sources from LPM for the AM62 family of SoCs, not
> > only daisy chain sources. The currently defined wakeup sources are
> > relevant for am62x/a/p but will also be reused for others where
> > possible. Otherwise these can be extended in the future for other wakeup
> > sources.
> > 
> 
> OK -> I should have been clear, but, I think you also caught on it
> when you said am62x/a/p extended for future wakeup sources - sure..
> with 32 bits you can indeed reach a large range.. BUT:
> 
> MAIN_DOMAIN, MCU_DOMAIN are specific to a SoC part architecture, it is
> not a generic K3 SoC family architecture concept - the power domains
> will vary depending on device - same with the list of peripherals used
> as wakeup source - is WKUP_I2C0 always present in all devices with
> wakeup, Do all K3 devices have USB0 and 1 always? We should not bet on
> that.
> 
> ti_sci_protocol.h is meant as a generic SoC family level header. I see
> these as possibly hardware specific description.
> 
> Lastly, I do not see the macros used either in the patch series.. I
> understand the ti_sci_protocol.h is meant to standardize stuff in
> other driver (I tried searching to see if some other series used
> it[1], but I could not find a reference)..
> 
> So, wondering: Is DT the right place for that - With a DT header ABI
> header that is shared between driver and dt? I don't understand the
> modelling of how wakeup_reason is getting used from this series, so I
> cannot comment better..

Thanks for explaining. So should I add the header already with this
series although it is unused right now, or should we add it together
with the first actual user later on, so there is no unused header in the
meantime?

Best
Markus

> 
> [1] https://lore.kernel.org/all/?q=TISCI_MSG_VALUE_LPM_WAKE_SOURCE_MAIN_IO
> -- 
> Regards,
> Nishanth Menon
> Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D