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

[a0230503]

On 13/08/24 09:19, Dhruva Gole wrote:
> On Aug 12, 2024 at 14:16:45 -0700, Kevin Hilman wrote:
> [...]
> > > > +/**
> > > > + * struct ti_sci_msg_resp_lpm_wake_reason - Response for TI_SCI_MSG_LPM_WAKE_REASON.
> > > > + *
> > > > + * @hdr:		Generic header.
> > > > + * @wake_source:	The wake up source that woke soc from LPM.
> > > > + * @wake_timestamp:	Timestamp at which soc woke.
> > > > + *
> > > > + * Response to a generic message with message type TI_SCI_MSG_LPM_WAKE_REASON,
> > > > + * used to query the wake up source from low power mode.
> > > > + */
> > > > +struct ti_sci_msg_resp_lpm_wake_reason {
> > > > +	struct ti_sci_msg_hdr hdr;
> > > > +	u32 wake_source;
> > > > +	u64 wake_timestamp;
> > > > +} __packed;
> > > > +
> > >
> > > It looks like we forgot to update the parameters in this API.
> > > See [1]:
> > >
> > > struct tisci_msg_lpm_wake_reason_req
> > >
> > > We're missing here the wake_pin, mode and 2 rsvd fields as well.
> > >
> > > [1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/pm/lpm.html#tisci-msg-lpm-wake-reason
> >
> > The docs mention how to interpret the pin, but not the mode.  This
> > driver should translate this mode number to some human-readable state
> > for better debug messages.
> >
> > Kevin
>
> Agreed, we would all benefit from a logic that prints the human-readable
> state. The way to interpret the mode would be just as:
> https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/pm/lpm.html#supported-low-power-modes
>
> So for eg. 0x0 = TISCI_MSG_VALUE_SLEEP_MODE_DEEP_SLEEP

Missing the call to ti_sci_msg_cmd_lpm_wake_reason in resume path.

Regards,
Akashdeep Kaur