Re: [PATCH v2 2/3] firmware: xilinx: Add pm api function for PL readback

From: Peter Korsgaard
Date: Wed Jun 22 2022 - 08:15:33 EST


>>>>> "Nava" == Nava kishore Manne <nava.manne@xxxxxxxxxx> writes:

> Adds PM API for performing PL configuration readback.
> It provides an interface to the pmufw to readback the
> FPGA configuration registers as well as configuration
> data.

> For more detailed info related to the configuration
> registers and configuration data refer ug570.

> Signed-off-by: Nava kishore Manne <nava.manne@xxxxxxxxxx>
> ---
> Changes for v2:
> - None.

> drivers/firmware/xilinx/zynqmp.c | 33 ++++++++++++++++++++++++++++
> include/linux/firmware/xlnx-zynqmp.h | 14 ++++++++++++
> 2 files changed, 47 insertions(+)

> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index 7977a494a651..40b99299b662 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -927,6 +927,39 @@ int zynqmp_pm_fpga_get_status(u32 *value)
> }
> EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_get_status);

> +/**
> + * zynqmp_pm_fpga_read - Perform the fpga configuration readback
> + * @reg_numframes: Configuration register offset (or) Number of frames to read

An offset OR a length? That sounds odd.


> + * @phys_address: Physical Address of the buffer
> + * @readback_type: Type of fpga readback operation
> + * 0 - FPGA configuration register readback
> + * 1 - FPGA configuration data readback

readback_type is a boolean, so how about calling it `bool data` or
something like that?


> + * @value: Value to read

what is the relation between phys_address and this value output
argument?

> + *
> + * This function provides access to xilfpga library to perform
> + * fpga configuration readback.
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +int zynqmp_pm_fpga_read(const u32 reg_numframes, const phys_addr_t phys_address,
> + bool readback_type, u32 *value)
> +{
> + u32 ret_payload[PAYLOAD_ARG_CNT];
> + int ret;
> +
> + if (!value)
> + return -EINVAL;
> +
> + ret = zynqmp_pm_invoke_fn(PM_FPGA_READ, reg_numframes,
> + lower_32_bits(phys_address),
> + upper_32_bits(phys_address), readback_type,

You are adding PM_FPGA_READ_CONFIG_ defines, so how about using them,
E.G.

data ? PM_FPGA_READ_CONFIG_DATA : PM_FPGA_READ_CONFIG_REG

--
Bye, Peter Korsgaard