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

From: Peter Korsgaard <peter@korsgaard.com>
To: Nava kishore Manne <nava.manne@xilinx.com>
Cc: <michal.simek@xilinx.com>,  <hao.wu@intel.com>,
	 <trix@redhat.com>, <mdf@kernel.org>,  <yilun.xu@intel.com>,
	 <gregkh@linuxfoundation.org>, <ronak.jain@xilinx.com>,
	 <rajan.vaja@xilinx.com>, <abhyuday.godhasara@xilinx.com>,
	 <piyush.mehta@xilinx.com>, <harsha.harsha@xilinx.com>,
	 <lakshmi.sai.krishna.potthuri@xilinx.com>,
	<linux-arm-kernel@lists.infradead.org>,
	 <linux-kernel@vger.kernel.org>, <linux-fpga@vger.kernel.org>,
	 <git@xilinx.com>
Subject: Re: [PATCH v2 2/3] firmware: xilinx: Add pm api function for PL readback
Date: Wed, 22 Jun 2022 14:14:06 +0200	[thread overview]
Message-ID: <87ilos520h.fsf@dell.be.48ers.dk> (raw)
In-Reply-To: <20220621092833.1057408-3-nava.manne@xilinx.com> (Nava kishore Manne's message of "Tue, 21 Jun 2022 14:58:32 +0530")

>>>>> "Nava" == Nava kishore Manne <nava.manne@xilinx.com> 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@xilinx.com>
 > ---
 > 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel