Re: [PATCH V6 3/4] firmware: ti_sci: Allocate memory for Low Power Modes

[Dhruva Gole]

On Aug 03, 2023 at 10:23:47 -0500, Andrew Davis wrote:
> On 8/3/23 1:42 AM, Dhruva Gole wrote:
> > From: Dave Gerlach <d-gerlach@xxxxxx>
> > 
> > A region of memory in DDR must be used during Deep Sleep for saving
> > of some system context when using the ti_sci firmware. From DM's point
> > of view, this can be any contiguous region in the DDR, so can allocate
> > 512KB of DMA reserved memory in probe(), instead of another carveout.
> > 
> > Also send a TISCI_MSG_QUERY_FW_CAPS message to the firmware during
> > probe to determine if any low power modes are supported and if
> > ti_sci_init_suspend should be called based on the response received.
> > 
> > Signed-off-by: Dave Gerlach <d-gerlach@xxxxxx>
> > Signed-off-by: Vibhore Vardhan <vibhore@xxxxxx>
> > Signed-off-by: Georgi Vlaev <g-vlaev@xxxxxx>
> > Tested-by: Roger Quadros <rogerq@xxxxxxxxxx>
> > [d-gole@xxxxxx: Use dma_alloc_attrs instead of dma_alloc_coherent]
> > Signed-off-by: Dhruva Gole <d-gole@xxxxxx>
> > ---
> >   drivers/firmware/ti_sci.c | 42 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> > 
[..snip..]
> >   static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
> >   	.default_host_id = 2,
> > @@ -3661,6 +3690,15 @@ static int ti_sci_probe(struct platform_device *pdev)
> >   		}
> >   	}
> > +	/*
> > +	 * Check if the firmware supports any optional low power modes
> > +	 * and initialize them if present. Old revisions of TIFS (< 08.04)
> > +	 * will NACK the request.
> > +	 */
> > +	ret = ti_sci_msg_cmd_query_fw_caps(&info->handle, &info->fw_caps);
> > +	if (!ret && (info->fw_caps & MSG_MASK_CAPS_LPM))
> > +		ti_sci_init_suspend(pdev, info);
> > +
> >   	dev_info(dev, "ABI: %d.%d (firmware rev 0x%04x '%s')\n",
> >   		 info->handle.version.abi_major, info->handle.version.abi_minor,
> >   		 info->handle.version.firmware_revision,
> > @@ -3708,6 +3746,10 @@ static int ti_sci_remove(struct platform_device *pdev)
> >   		mbox_free_channel(info->chan_rx);
> >   	}
> > +	if (info->ctx_mem_buf)
> > +		dma_free_coherent(info->dev, LPM_CTX_MEM_SIZE,
> 
> You allocated with dma_alloc_attrs() you should free with dma_free_attrs().

Good catch, will fix this in next revision

> 
> Andrew
> 
> > +				  info->ctx_mem_buf,
> > +				  info->ctx_mem_addr);
> >   	return ret;
> >   }

-- 
Best regards,
Dhruva Gole <d-gole@xxxxxx>