lkml.org 
[lkml]   [2021]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 4/4] fpga: versal-fpga: Add versal fpga manager driver
From
Date

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
>

\
 
 \ /
  Last update: 2021-06-04 15:53    [W:0.051 / U:2.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site