| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <06301910-10a1-0e62-45a0-d28ab5a787ed@redhat.com>
Date: Fri, 4 Jun 2021 06:53:06 -0700
From: Tom Rix <trix@...hat.com>
To: Greg KH <gregkh@...uxfoundation.org>,
Nava kishore Manne <nava.manne@...inx.com>
Cc: robh+dt@...nel.org, michal.simek@...inx.com, mdf@...nel.org,
arnd@...db.de, rajan.vaja@...inx.com, amit.sunil.dhamne@...inx.com,
tejas.patel@...inx.com, zou_wei@...wei.com,
lakshmi.sai.krishna.potthuri@...inx.com, ravi.patel@...inx.com,
iwamatsu@...auri.org, wendy.liang@...inx.com,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-fpga@...r.kernel.org,
git@...inx.com, chinnikishore369@...il.com,
Appana Durga Kedareswara rao <appana.durga.rao@...inx.com>
Subject: Re: [PATCH v7 4/4] fpga: versal-fpga: Add versal fpga manager driver
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@...inx.com>
>> Signed-off-by: Nava kishore Manne <nava.manne@...inx.com>
>> Reviewed-by: Moritz Fischer <mdf@...nel.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
>
Powered by blists - more mailing lists