Messages in this thread | | | Subject | Re: [PATCH v7 4/4] fpga: versal-fpga: Add versal fpga manager driver | From | Tom Rix <> | Date | Fri, 4 Jun 2021 06:53:06 -0700 |
| |
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 >
| |