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

[Nishanth Menon]

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..

[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