This series Adds FPGA manager driver support for Xilinx Versal SoC.
it uses the firmware interface to configure the programmable logic.
Changes for v4:
-Rebase the patch series on linux-next.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
Changes for v5:
-Updated binding doc's.
Changes for v6:
-Updated firmware binding doc.
Appana Durga Kedareswara rao (1):
dt-bindings: fpga: Add binding doc for versal fpga manager
Nava kishore Manne (3):
drivers: firmware: Add PDI load API support
dt-bindings: firmware: Add bindings for xilinx firmware
fpga: versal-fpga: Add versal fpga manager driver
.../firmware/xilinx/xlnx,zynqmp-firmware.yaml | 102 +++++++++++++++
.../bindings/fpga/xlnx,versal-fpga.yaml | 33 +++++
drivers/firmware/xilinx/zynqmp.c | 17 +++
drivers/fpga/Kconfig | 9 ++
drivers/fpga/Makefile | 1 +
drivers/fpga/versal-fpga.c | 117 ++++++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 10 ++
7 files changed, 289 insertions(+)
create mode 100644 Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-firmware.yaml
create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
create mode 100644 drivers/fpga/versal-fpga.c
--
2.17.1
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 <[email protected]>
Signed-off-by: Nava kishore Manne <[email protected]>
Reviewed-by: Moritz Fischer <[email protected]>
---
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.
drivers/fpga/Kconfig | 9 +++
drivers/fpga/Makefile | 1 +
drivers/fpga/versal-fpga.c | 117 +++++++++++++++++++++++++++++++++++++
3 files changed, 127 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..5744e44f981d
--- /dev/null
+++ b/drivers/fpga/versal-fpga.c
@@ -0,0 +1,117 @@
+// 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>
+
+/**
+ * struct versal_fpga_priv - Private data structure
+ * @dev: Device data structure
+ */
+struct versal_fpga_priv {
+ struct device *dev;
+};
+
+static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *buf, size_t size)
+{
+ return 0;
+}
+
+static int versal_fpga_ops_write(struct fpga_manager *mgr,
+ const char *buf, size_t size)
+{
+ struct versal_fpga_priv *priv;
+ dma_addr_t dma_addr = 0;
+ char *kbuf;
+ int ret;
+
+ priv = mgr->priv;
+
+ kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ memcpy(kbuf, buf, size);
+
+ wmb(); /* ensure all writes are done before initiate FW call */
+
+ ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);
+
+ dma_free_coherent(priv->dev, size, kbuf, dma_addr);
+
+ return ret;
+}
+
+static int versal_fpga_ops_write_complete(struct fpga_manager *mgr,
+ struct fpga_image_info *info)
+{
+ return 0;
+}
+
+static enum fpga_mgr_states versal_fpga_ops_state(struct fpga_manager *mgr)
+{
+ return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static const struct fpga_manager_ops versal_fpga_ops = {
+ .state = versal_fpga_ops_state,
+ .write_init = versal_fpga_ops_write_init,
+ .write = versal_fpga_ops_write,
+ .write_complete = versal_fpga_ops_write_complete,
+};
+
+static int versal_fpga_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct versal_fpga_priv *priv;
+ struct fpga_manager *mgr;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = dev;
+ ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+ if (ret < 0) {
+ dev_err(dev, "no usable DMA configuration\n");
+ return ret;
+ }
+
+ mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
+ &versal_fpga_ops, priv);
+ if (!mgr)
+ return -ENOMEM;
+
+ return devm_fpga_mgr_register(dev, mgr);
+}
+
+static const struct of_device_id versal_fpga_of_match[] = {
+ { .compatible = "xlnx,versal-fpga", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
+
+static struct platform_driver versal_fpga_driver = {
+ .probe = versal_fpga_probe,
+ .driver = {
+ .name = "versal_fpga_manager",
+ .of_match_table = of_match_ptr(versal_fpga_of_match),
+ },
+};
+module_platform_driver(versal_fpga_driver);
+
+MODULE_AUTHOR("Nava kishore Manne <[email protected]>");
+MODULE_AUTHOR("Appana Durga Kedareswara rao <[email protected]>");
+MODULE_DESCRIPTION("Xilinx Versal FPGA Manager");
+MODULE_LICENSE("GPL");
--
2.17.1
On Thu, May 20, 2021 at 01:39:54PM +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 <[email protected]>
> Signed-off-by: Nava kishore Manne <[email protected]>
> Reviewed-by: Moritz Fischer <[email protected]>
> ---
> 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.
>
> drivers/fpga/Kconfig | 9 +++
> drivers/fpga/Makefile | 1 +
> drivers/fpga/versal-fpga.c | 117 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 127 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..5744e44f981d
> --- /dev/null
> +++ b/drivers/fpga/versal-fpga.c
> @@ -0,0 +1,117 @@
> +// 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>
> +
> +/**
> + * struct versal_fpga_priv - Private data structure
> + * @dev: Device data structure
> + */
> +struct versal_fpga_priv {
> + struct device *dev;
> +};
Don't you have this pointer already? What device is this exactly and
why does it differ from the structure it currently lives in?
> +
> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t size)
> +{
> + return 0;
> +}
If you don't need this, why include it?
> +
> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> + const char *buf, size_t size)
> +{
> + struct versal_fpga_priv *priv;
> + dma_addr_t dma_addr = 0;
> + char *kbuf;
> + int ret;
> +
> + priv = mgr->priv;
> +
> + kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + memcpy(kbuf, buf, size);
> +
> + wmb(); /* ensure all writes are done before initiate FW call */
What "writes"? The memcpy above? Are you _SURE_ that really is correct
here? This feels wrong.
> +
> + ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);
If this needs some sort of barrier, shouldn't it be in this call?
> +
> + dma_free_coherent(priv->dev, size, kbuf, dma_addr);
> +
> + return ret;
> +}
> +
> +static int versal_fpga_ops_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + return 0;
> +}
Again, why have it if it does nothing?
> +
> +static enum fpga_mgr_states versal_fpga_ops_state(struct fpga_manager *mgr)
> +{
> + return FPGA_MGR_STATE_UNKNOWN;
> +}
Again, is this needed? If so, then the fpga_manager core needs to be
fixed up :)
> +static const struct fpga_manager_ops versal_fpga_ops = {
> + .state = versal_fpga_ops_state,
> + .write_init = versal_fpga_ops_write_init,
> + .write = versal_fpga_ops_write,
> + .write_complete = versal_fpga_ops_write_complete,
> +};
> +
> +static int versal_fpga_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct versal_fpga_priv *priv;
> + struct fpga_manager *mgr;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = dev;
You save a pointer to a reference counted structure, without
incrementing the reference count. What could go wrong? :)
You are getting lucky here, but as stated above, why do you need this
pointer?
thanks,
greg k-h
Hi Greg,
Thanks for providing the review comments.
Please find my response inline.
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: Thursday, May 20, 2021 2:19 PM
> To: Nava kishore Manne <[email protected]>
> Cc: [email protected]; Michal Simek <[email protected]>;
> [email protected]; [email protected]; [email protected]; Rajan Vaja
> <[email protected]>; Amit Sunil Dhamne <[email protected]>;
> Tejas Patel <[email protected]>; [email protected]; Sai Krishna
> Potthuri <[email protected]>; Ravi Patel <[email protected]>;
> [email protected]; Jiaying Liang <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; git <[email protected]>;
> [email protected]; Appana Durga Kedareswara Rao
> <[email protected]>
> Subject: Re: [PATCH v6 4/4] fpga: versal-fpga: Add versal fpga manager driver
>
> On Thu, May 20, 2021 at 01:39:54PM +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
> > <[email protected]>
> > Signed-off-by: Nava kishore Manne <[email protected]>
> > Reviewed-by: Moritz Fischer <[email protected]>
> > ---
> > 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.
> >
> > drivers/fpga/Kconfig | 9 +++
> > drivers/fpga/Makefile | 1 +
> > drivers/fpga/versal-fpga.c | 117
> > +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 127 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..5744e44f981d
> > --- /dev/null
> > +++ b/drivers/fpga/versal-fpga.c
> > @@ -0,0 +1,117 @@
> > +// 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>
> > +
> > +/**
> > + * struct versal_fpga_priv - Private data structure
> > + * @dev: Device data structure
> > + */
> > +struct versal_fpga_priv {
> > + struct device *dev;
> > +};
>
> Don't you have this pointer already? What device is this exactly and why
> does it differ from the structure it currently lives in?
>
Agree, this struct is not needed.
Will fix this issue in v7.
> > +
> > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > + struct fpga_image_info *info,
> > + const char *buf, size_t size) {
> > + return 0;
> > +}
>
> If you don't need this, why include it?
>
Agree this empty API is not needed.
It's a limitation with the framework and this needs to fixed in the fpga_manager core.
Will address this generic issue in a different series.
> > +
> > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > + const char *buf, size_t size)
> > +{
> > + struct versal_fpga_priv *priv;
> > + dma_addr_t dma_addr = 0;
> > + char *kbuf;
> > + int ret;
> > +
> > + priv = mgr->priv;
> > +
> > + kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> GFP_KERNEL);
> > + if (!kbuf)
> > + return -ENOMEM;
> > +
> > + memcpy(kbuf, buf, size);
> > +
> > + wmb(); /* ensure all writes are done before initiate FW call */
>
> What "writes"? The memcpy above? Are you _SURE_ that really is correct
> here? This feels wrong.
>
Will fix in v7.
> > +
> > + ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);
>
> If this needs some sort of barrier, shouldn't it be in this call?
>
Will fix in v7.
> > +
> > + dma_free_coherent(priv->dev, size, kbuf, dma_addr);
> > +
> > + return ret;
> > +}
> > +
> > +static int versal_fpga_ops_write_complete(struct fpga_manager *mgr,
> > + struct fpga_image_info *info)
> > +{
> > + return 0;
> > +}
>
> Again, why have it if it does nothing?
>
Same as above.
> > +
> > +static enum fpga_mgr_states versal_fpga_ops_state(struct fpga_manager
> > +*mgr) {
> > + return FPGA_MGR_STATE_UNKNOWN;
> > +}
>
> Again, is this needed? If so, then the fpga_manager core needs to be fixed
> up :)
>
Same as above.
> > +static const struct fpga_manager_ops versal_fpga_ops = {
> > + .state = versal_fpga_ops_state,
> > + .write_init = versal_fpga_ops_write_init,
> > + .write = versal_fpga_ops_write,
> > + .write_complete = versal_fpga_ops_write_complete, };
> > +
> > +static int versal_fpga_probe(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct versal_fpga_priv *priv;
> > + struct fpga_manager *mgr;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->dev = dev;
>
> You save a pointer to a reference counted structure, without incrementing
> the reference count. What could go wrong? :)
>
> You are getting lucky here, but as stated above, why do you need this
> pointer?
>
Will fix in v7.
Regards,
Navakishore.
On Tue, Jun 01, 2021 at 06:56:32AM +0000, Nava kishore Manne wrote:
> Hi Greg,
>
> Thanks for providing the review comments.
> Please find my response inline.
>
> > -----Original Message-----
> > From: Greg KH <[email protected]>
> > Sent: Thursday, May 20, 2021 2:19 PM
> > To: Nava kishore Manne <[email protected]>
> > Cc: [email protected]; Michal Simek <[email protected]>;
> > [email protected]; [email protected]; [email protected]; Rajan Vaja
> > <[email protected]>; Amit Sunil Dhamne <[email protected]>;
> > Tejas Patel <[email protected]>; [email protected]; Sai Krishna
> > Potthuri <[email protected]>; Ravi Patel <[email protected]>;
> > [email protected]; Jiaying Liang <[email protected]>;
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; git <[email protected]>;
> > [email protected]; Appana Durga Kedareswara Rao
> > <[email protected]>
> > Subject: Re: [PATCH v6 4/4] fpga: versal-fpga: Add versal fpga manager driver
> >
> > On Thu, May 20, 2021 at 01:39:54PM +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
> > > <[email protected]>
> > > Signed-off-by: Nava kishore Manne <[email protected]>
> > > Reviewed-by: Moritz Fischer <[email protected]>
> > > ---
> > > 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.
> > >
> > > drivers/fpga/Kconfig | 9 +++
> > > drivers/fpga/Makefile | 1 +
> > > drivers/fpga/versal-fpga.c | 117
> > > +++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 127 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..5744e44f981d
> > > --- /dev/null
> > > +++ b/drivers/fpga/versal-fpga.c
> > > @@ -0,0 +1,117 @@
> > > +// 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>
> > > +
> > > +/**
> > > + * struct versal_fpga_priv - Private data structure
> > > + * @dev: Device data structure
> > > + */
> > > +struct versal_fpga_priv {
> > > + struct device *dev;
> > > +};
> >
> > Don't you have this pointer already? What device is this exactly and why
> > does it differ from the structure it currently lives in?
> >
> Agree, this struct is not needed.
> Will fix this issue in v7.
>
> > > +
> > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > > + struct fpga_image_info *info,
> > > + const char *buf, size_t size) {
> > > + return 0;
> > > +}
> >
> > If you don't need this, why include it?
> >
>
> Agree this empty API is not needed.
> It's a limitation with the framework and this needs to fixed in the fpga_manager core.
> Will address this generic issue in a different series.
I was working on a series to clean this up anyways :)
>
> > > +
> > > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > > + const char *buf, size_t size)
> > > +{
> > > + struct versal_fpga_priv *priv;
> > > + dma_addr_t dma_addr = 0;
> > > + char *kbuf;
> > > + int ret;
> > > +
> > > + priv = mgr->priv;
> > > +
> > > + kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > GFP_KERNEL);
> > > + if (!kbuf)
> > > + return -ENOMEM;
> > > +
> > > + memcpy(kbuf, buf, size);
> > > +
> > > + wmb(); /* ensure all writes are done before initiate FW call */
> >
> > What "writes"? The memcpy above? Are you _SURE_ that really is correct
> > here? This feels wrong.
> >
>
> Will fix in v7.
>
> > > +
> > > + ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);
> >
> > If this needs some sort of barrier, shouldn't it be in this call?
> >
>
> Will fix in v7.
>
> > > +
> > > + dma_free_coherent(priv->dev, size, kbuf, dma_addr);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int versal_fpga_ops_write_complete(struct fpga_manager *mgr,
> > > + struct fpga_image_info *info)
> > > +{
> > > + return 0;
> > > +}
> >
> > Again, why have it if it does nothing?
> >
>
> Same as above.
>
> > > +
> > > +static enum fpga_mgr_states versal_fpga_ops_state(struct fpga_manager
> > > +*mgr) {
> > > + return FPGA_MGR_STATE_UNKNOWN;
> > > +}
> >
> > Again, is this needed? If so, then the fpga_manager core needs to be fixed
> > up :)
> >
>
> Same as above.
>
> > > +static const struct fpga_manager_ops versal_fpga_ops = {
> > > + .state = versal_fpga_ops_state,
> > > + .write_init = versal_fpga_ops_write_init,
> > > + .write = versal_fpga_ops_write,
> > > + .write_complete = versal_fpga_ops_write_complete, };
> > > +
> > > +static int versal_fpga_probe(struct platform_device *pdev) {
> > > + struct device *dev = &pdev->dev;
> > > + struct versal_fpga_priv *priv;
> > > + struct fpga_manager *mgr;
> > > + int ret;
> > > +
> > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > + if (!priv)
> > > + return -ENOMEM;
> > > +
> > > + priv->dev = dev;
> >
> > You save a pointer to a reference counted structure, without incrementing
> > the reference count. What could go wrong? :)
> >
> > You are getting lucky here, but as stated above, why do you need this
> > pointer?
> >
>
> Will fix in v7.
>
> Regards,
> Navakishore.
- Moritz