Hi Dhruva, On Fri, Dec 20, 2024 at 05:20:43PM +0530, Dhruva Gole wrote: > Hi Markus, > > On Dec 19, 2024 at 21:02:13 +0100, Markus Schneider-Pargmann wrote: > > Add support for Partial-IO poweroff. In Partial-IO pins of a few > > Maybe add a comma after the In partial-IO, a few pins in this SOC can generate.... > > > hardware units can generate system wakeups while DDR memory is not > > powered resulting in a fresh boot of the system. These hardware units in > > the SoC are always powered so that some logic can detect pin activity. > > > > If the system supports Partial-IO as described in the fw capabilities, a > > sys_off handler is added. This sys_off handler decides if the poweroff > > is executed by entering normal poweroff or Partial-IO instead. The > > decision is made by checking if wakeup is enabled on all devices that > > may wake up the SoC from Partial-IO. > > > > The possible wakeup devices are found by checking which devices have the > > "poweroff" in the list of wakeup-source power states. Only devices that > > are actually enabled by the user will be considered as an active wakeup > > source. If none of the wakeup sources is enabled the system will do a > > normal poweroff. If at least one wakeup source is enabled it will > > instead send a TI_SCI_MSG_PREPARE_SLEEP message from the sys_off > > handler. Sending this message will result in an immediate shutdown of > > the system. No execution is expected after this point. The code will > > wait for 5s and do an emergency_restart afterwards if Partial-IO wasn't > > entered at that point. > > > > A short documentation about Partial-IO can be found in section 6.2.4.5 > > of the TRM at > > https://www.ti.com/lit/pdf/spruiv7 > > > > Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx> > > --- > > drivers/firmware/ti_sci.c | 115 +++++++++++++++++++++++++++++++++++++++++++++- > > drivers/firmware/ti_sci.h | 5 ++ > > 2 files changed, 119 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c > > index ec0c54935ac0d667323d98b86ac9d288b73be6aa..693ac816f8ba3941a9156bd39524099ca476d712 100644 > > --- a/drivers/firmware/ti_sci.c > > +++ b/drivers/firmware/ti_sci.c > > @@ -3746,6 +3746,100 @@ static const struct dev_pm_ops ti_sci_pm_ops = { > > #endif > > }; > > > > +/* > > + * Enter Partial-IO, which disables everything including DDR with only a small > > + * logic being active for wakeup. > > + */ > > +static int tisci_enter_partial_io(struct ti_sci_info *info) > > Isn't the function naming style in the driver ti_sci_XXX ? You're > missing one `_` I guess > > > +{ > > + struct ti_sci_msg_req_prepare_sleep *req; > > + struct ti_sci_xfer *xfer; > > + struct device *dev = info->dev; > > + int ret = 0; > > + > > + xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_PREPARE_SLEEP, > > + TI_SCI_FLAG_REQ_GENERIC_NORESPONSE, > > + sizeof(*req), sizeof(struct ti_sci_msg_hdr)); > > + if (IS_ERR(xfer)) { > > + ret = PTR_ERR(xfer); > > + dev_err(dev, "Message alloc failed(%d)\n", ret); > > + return ret; > > + } > > + > > + req = (struct ti_sci_msg_req_prepare_sleep *)xfer->xfer_buf; > > + req->mode = TISCI_MSG_VALUE_SLEEP_MODE_PARTIAL_IO; > > + req->ctx_lo = 0; > > + req->ctx_hi = 0; > > + req->debug_flags = 0; > > + > > + dev_info(dev, "Entering Partial-IO because a powered wakeup-enabled device was found.\n"); > > + > > + ret = ti_sci_do_xfer(info, xfer); > > + if (ret) { > > + dev_err(dev, "Mbox send fail %d\n", ret); > > + goto fail; > > + } > > + > > +fail: > > + ti_sci_put_one_xfer(&info->minfo, xfer); > > + > > + return ret; > > +} > > + > > +static bool tisci_canuart_wakeup_enabled(struct ti_sci_info *info) > > Add some documentation around this please. > > > +{ > > + struct device_node *wakeup_node = NULL; > > + > > + for (wakeup_node = of_find_node_with_property(NULL, "wakeup-source"); > > + wakeup_node; > > + wakeup_node = of_find_node_with_property(wakeup_node, "wakeup-source")) { > > + struct platform_device *pdev; > > + int index; > > + > > + index = of_property_match_string(wakeup_node, "wakeup-source", "poweroff"); > > + if (index < 0) > > + continue; > > Doesn't the fact that we're inside the for loop already ensure this is > 0? No, this can return several error codes if the property doesn't have a value or the string is not found in the property array. > > > + > > + pdev = of_find_device_by_node(wakeup_node); > > + if (!pdev) > > + break; > > Same here? Would we otherwise be in the loop? > Just having a quick look here, I could be wrong, please just check once. The devicetree node doesn't necessarily have a device associated with it. So I need to check if there is a device for this devicetree node. I fixed the rest of your comments. Thanks for your review. Best Markus
Attachment:
signature.asc
Description: PGP signature
