Re: [PATCH v7 4/4] fpga: versal-fpga: Add versal fpga manager driver

[Tom Rix <trix@redhat.com>]

On 6/4/21 4:48 AM, Greg KH wrote:
> On Fri, Jun 04, 2021 at 05:03:32PM +0530, Nava kishore Manne wrote:
>> Add support for Xilinx Versal FPGA manager.
>>
>> PDI source type can be DDR, OCM, QSPI flash etc..
>> But driver allocates memory always from DDR, Since driver supports only
>> DDR source type.
>>
>> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
>> Reviewed-by: Moritz Fischer <mdf@kernel.org>
>> ---
>> Changes for v2:
>>                -Updated the Fpga Mgr registrations call's
>>                 to 5.11
>>                -Fixed some minor coding issues as suggested by
>>                 Moritz.
>>
>> Changes for v3:
>>                -Rewritten the Versal fpga Kconfig contents.
>>
>> Changes for v4:
>>                -Rebased the changes on linux-next.
>>                 No functional changes.
>>
>> Changes for v5:
>>                -None.
>>
>> Changes for v6:
>>                -None.
>>
>> Changes for v7:
>>                -Updated driver to remove unwated priv struct dependency.
>>
>>   drivers/fpga/Kconfig       |  9 ++++
>>   drivers/fpga/Makefile      |  1 +
>>   drivers/fpga/versal-fpga.c | 96 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 106 insertions(+)
>>   create mode 100644 drivers/fpga/versal-fpga.c
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 33e15058d0dc..92c20b92357a 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -234,4 +234,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>>   	  to configure the programmable logic(PL) through PS
>>   	  on ZynqMP SoC.
>>   
>> +config FPGA_MGR_VERSAL_FPGA
>> +	tristate "Xilinx Versal FPGA"
>> +	depends on ARCH_ZYNQMP || COMPILE_TEST
>> +	help
>> +	  Select this option to enable FPGA manager driver support for
>> +	  Xilinx Versal SoC. This driver uses the firmware interface to
>> +	  configure the programmable logic(PL).
>> +
>> +	  To compile this as a module, choose M here.
>>   endif # FPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 18dc9885883a..0bff783d1b61 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_FPGA_MGR_TS73XX)		+= ts73xx-fpga.o
>>   obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
>>   obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>>   obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>> +obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)      += versal-fpga.o
>>   obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>>   obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>   
>> diff --git a/drivers/fpga/versal-fpga.c b/drivers/fpga/versal-fpga.c
>> new file mode 100644
>> index 000000000000..1bd312a31b23
>> --- /dev/null
>> +++ b/drivers/fpga/versal-fpga.c
>> @@ -0,0 +1,96 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019-2021 Xilinx, Inc.
>> + */
>> +
>> +#include <linux/dma-mapping.h>
>> +#include <linux/fpga/fpga-mgr.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/string.h>
>> +#include <linux/firmware/xlnx-zynqmp.h>
>> +
>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
>> +				      struct fpga_image_info *info,
>> +				      const char *buf, size_t size)
>> +{
>> +	return 0;
> Why have this if it does nothing?
>
>> +}
>> +
>> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
>> +				 const char *buf, size_t size)
>> +{
>> +	dma_addr_t dma_addr = 0;
>> +	char *kbuf;
>> +	int ret;
>> +
>> +	kbuf = dma_alloc_coherent(mgr->dev.parent, size, &dma_addr, GFP_KERNEL);
>> +	if (!kbuf)
>> +		return -ENOMEM;
>> +
>> +	memcpy(kbuf, buf, size);
>> +	ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);
>> +	dma_free_coherent(mgr->dev.parent, size, kbuf, dma_addr);
>> +
>> +	return ret;
>> +}
>> +
>> +static int versal_fpga_ops_write_complete(struct fpga_manager *mgr,
>> +					  struct fpga_image_info *info)
>> +{
>> +	return 0;
> Same here, why have this at all?
>
>> +}
>> +
>> +static enum fpga_mgr_states versal_fpga_ops_state(struct fpga_manager *mgr)
>> +{
>> +	return FPGA_MGR_STATE_UNKNOWN;
> Shouln't that be the default state of the fpga manager if there is no
> state function callback?
>
> This driver should just need a write and probe function, and that's it,
> why make it more complex?

These empty functions are needed by each board because of an aggressive 
check of ops early in the bringup of the fpga mgr.

I did some deck chair shuffling here recently

https://lore.kernel.org/linux-fpga/20210524162820.2221474-1-trix@redhat.com/

that shows location of check.

A finer grained handling of the ops would be better.

I'll add that to the next spin.

Tom

> thanks,
>
> greg k-h
>