Message ID | 20221116181307.198209-1-g-vlaev@ti.com |
---|---|
Headers | show |
Series | firmware: ti_sci: Introduce system suspend support | expand |
On 16/11/2022 19:13, Georgi Vlaev wrote: > From: Dave Gerlach <d-gerlach@ti.com> > > Add documentation for the lpm region which tells the ti-sci driver where > to load the FS Stub low power mode firmware and also the firmware-name > which tells the driver which binary to load. Both of these are optional > for normal system operation but required to enabled suspend-to-mem usage > of Deep Sleep state. > I think you got here Rob's tag after sending v4. Reviewed-by: Rob Herring <robh@kernel.org> Best regards, Krzysztof
On 20:13-20221116, Georgi Vlaev wrote: [...] > +static int ti_sci_init_suspend(struct platform_device *pdev, > + struct ti_sci_info *info) > +{ > + struct device *dev = &pdev->dev; > + int ret; > + > + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); > + info->ctx_mem_buf = dma_alloc_coherent(info->dev, LPM_CTX_MEM_SIZE, > + &info->ctx_mem_addr, > + GFP_KERNEL); > + if (!info->ctx_mem_buf) { > + dev_err(info->dev, "Failed to allocate LPM context memory\n"); > + return -ENOMEM; > + } > + > + /* > + * Attempt to call prepare_sleep, this will be NAK'd if suspend is not > + * supported by firmware in use, in which case we will not attempt to > + * init suspend. > + */ > + ret = ti_sci_cmd_prepare_sleep(&info->handle, 0, > + (u32)(info->ctx_mem_addr & 0xffffffff), > + (u32)((u64)info->ctx_mem_addr >> 32), 0); > + > + if (ret) > + goto err; > + > + return 0; > +err: > + dma_free_coherent(info->dev, LPM_CTX_MEM_SIZE, > + info->ctx_mem_buf, > + info->ctx_mem_addr); > + return ret; > +} > + > /* Description for K2G */ > static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = { > .default_host_id = 2, > @@ -3639,6 +3682,14 @@ static int ti_sci_probe(struct platform_device *pdev) > } > } > > + ret = ti_sci_init_suspend(pdev, info); > + if (ret) > + dev_warn(dev, > + "ti_sci_init_suspend failed, mem suspend will be non-functional.\n"); > + > + /* Suspend is an optional feature, reset return value and continue. */ > + ret = 0; We end up getting this warning on all platforms with TISCI - even if LPM sequence is capable or not - what does the message mean? firmware is not capable of supporting sleep or it is a firmware capable of supporting, but failed to allocate LPM context memory? If it is optional (since it is probing to see if it has functionality), then do we need a dev_warn - maybe a softer of form? [...]
On 20:13-20221116, Georgi Vlaev wrote: > + /* > + * Attempt to call prepare_sleep, this will be NAK'd if suspend is not > + * supported by firmware in use, in which case we will not attempt to > + * init suspend. > + */ > + ret = ti_sci_cmd_prepare_sleep(&info->handle, 0, > + (u32)(info->ctx_mem_addr & 0xffffffff), > + (u32)((u64)info->ctx_mem_addr >> 32), 0); > + https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/pm/lpm.html#tisci-msg-prepare-sleep "Prepare the SOC for entering into a low power mode." But we are in the init process here. From the documentation, firmware does'nt seem to guarantee it would do something unexpected (like setup io daisy chain or something like that which normal LP entry state would have to do) - How is it safe to use it as a discovery of capability API?
Hi Nishanth, On 11/21/22 20:44, Nishanth Menon wrote: > On 20:13-20221116, Georgi Vlaev wrote: > [...] > >> +static int ti_sci_init_suspend(struct platform_device *pdev, >> + struct ti_sci_info *info) >> +{ >> + struct device *dev = &pdev->dev; >> + int ret; >> + >> + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); >> + info->ctx_mem_buf = dma_alloc_coherent(info->dev, LPM_CTX_MEM_SIZE, >> + &info->ctx_mem_addr, >> + GFP_KERNEL); >> + if (!info->ctx_mem_buf) { >> + dev_err(info->dev, "Failed to allocate LPM context memory\n"); >> + return -ENOMEM; >> + } >> + >> + /* >> + * Attempt to call prepare_sleep, this will be NAK'd if suspend is not >> + * supported by firmware in use, in which case we will not attempt to >> + * init suspend. >> + */ >> + ret = ti_sci_cmd_prepare_sleep(&info->handle, 0, >> + (u32)(info->ctx_mem_addr & 0xffffffff), >> + (u32)((u64)info->ctx_mem_addr >> 32), 0); >> + >> + if (ret) >> + goto err; >> + >> + return 0; >> +err: >> + dma_free_coherent(info->dev, LPM_CTX_MEM_SIZE, >> + info->ctx_mem_buf, >> + info->ctx_mem_addr); >> + return ret; >> +} >> + >> /* Description for K2G */ >> static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = { >> .default_host_id = 2, >> @@ -3639,6 +3682,14 @@ static int ti_sci_probe(struct platform_device *pdev) >> } >> } >> >> + ret = ti_sci_init_suspend(pdev, info); >> + if (ret) >> + dev_warn(dev, >> + "ti_sci_init_suspend failed, mem suspend will be non-functional.\n"); >> + >> + /* Suspend is an optional feature, reset return value and continue. */ >> + ret = 0; > > We end up getting this warning on all platforms with TISCI - even if > LPM sequence is capable or not - what does the message mean? firmware is > not capable of supporting sleep or it is a firmware capable of > supporting, but failed to allocate LPM context memory? > > If it is optional (since it is probing to see if it has functionality), > then do we need a dev_warn - maybe a softer of form? > Yeah, I agree, the message looks confusing. In both cases we can't enter suspend-to-ram, but we consider that an optional feature, so a softer message will be more appropriate. > [...] >
Hi Nishanth, On 11/21/22 20:56, Nishanth Menon wrote: > On 20:13-20221116, Georgi Vlaev wrote: >> + /* >> + * Attempt to call prepare_sleep, this will be NAK'd if suspend is not >> + * supported by firmware in use, in which case we will not attempt to >> + * init suspend. >> + */ >> + ret = ti_sci_cmd_prepare_sleep(&info->handle, 0, >> + (u32)(info->ctx_mem_addr & 0xffffffff), >> + (u32)((u64)info->ctx_mem_addr >> 32), 0); >> + > > https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/pm/lpm.html#tisci-msg-prepare-sleep > "Prepare the SOC for entering into a low power mode." > > But we are in the init process here. From the documentation, firmware > does'nt seem to guarantee it would do something unexpected (like setup > io daisy chain or something like that which normal LP entry state > would have to do) - How is it safe to use it as a discovery of > capability API? > > Well, you're correct, there's no guarantee. It is safe to call it now on AM62x in both places we actually use it. However, this may change in the future and it's not a good idea to misuse that API. We'll switch the detection part to a more appropriate message, that's better suited for this purpose on all K3 platforms.
Hi, On 11/18/22 14:59, Krzysztof Kozlowski wrote: > On 16/11/2022 19:13, Georgi Vlaev wrote: >> From: Dave Gerlach <d-gerlach@ti.com> >> >> Add documentation for the lpm region which tells the ti-sci driver where >> to load the FS Stub low power mode firmware and also the firmware-name >> which tells the driver which binary to load. Both of these are optional >> for normal system operation but required to enabled suspend-to-mem usage >> of Deep Sleep state. >> > > I think you got here Rob's tag after sending v4. > > Reviewed-by: Rob Herring <robh@kernel.org> > I will pick it up in v5. Thanks. > Best regards, > Krzysztof >