| Message ID | 20210118024318.9530-1-nava.manne@xilinx.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] drivers: firmware: Add Pdi load API support | expand |
Hi Nava, On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote: > This patch adds driver for versal fpga manager. Nit: Add support for Xilinx Versal FPGA manager > > PDI source type can be DDR, OCM, QSPI flash etc.. No idea what PDI is :) > 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> > --- > drivers/fpga/Kconfig | 8 ++ > drivers/fpga/Makefile | 1 + > drivers/fpga/versal-fpga.c | 149 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 158 insertions(+) > create mode 100644 drivers/fpga/versal-fpga.c > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > index 5645226ca3ce..9f779c3a6739 100644 > --- a/drivers/fpga/Kconfig > +++ b/drivers/fpga/Kconfig > @@ -216,4 +216,12 @@ 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 versal soc firmware > + interface to load programmable logic(PL) images > + on versal soc. > endif # FPGA > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile > index d8e21dfc6778..40c9adb6a644 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..2a42aa78b182 > --- /dev/null > +++ b/drivers/fpga/versal-fpga.c > @@ -0,0 +1,149 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 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> > + > +/* Constant Definitions */ > +#define PDI_SOURCE_TYPE 0xF > + > +/** > + * struct versal_fpga_priv - Private data structure > + * @dev: Device data structure > + * @flags: flags which is used to identify the PL Image type > + */ > +struct versal_fpga_priv { > + struct device *dev; > + u32 flags; This seems unused ... please introduce them when/if you start using them. > +}; > + > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr, > + struct fpga_image_info *info, > + const char *buf, size_t size) > +{ > + struct versal_fpga_priv *priv; > + > + priv = mgr->priv; > + priv->flags = info->flags; ? What uses this ? It seems this function could just be 'return 0' right now. > + > + 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_SOURCE_TYPE, 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_OPERATING; Is that always the case? Shouldn't that be 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 err, ret; Please pick one, err or ret. 'err' seems unused? > + > + 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"); Nit: "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; > + > + platform_set_drvdata(pdev, mgr); > + Replace this part: > + err = fpga_mgr_register(mgr); > + if (err) { > + dev_err(dev, "unable to register FPGA manager"); > + fpga_mgr_free(mgr); > + return err; > + } with: return devm_fpga_mgr_register(mgr); I tried to get rid of the boilerplate, since every driver repeats it (and above calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create() created FPGA manager is wrong?) :) > + > + return 0; > +} > + Then > +static int versal_fpga_remove(struct platform_device *pdev) > +{ > + struct fpga_manager *mgr = platform_get_drvdata(pdev); > + > + fpga_mgr_unregister(mgr); > + fpga_mgr_free(mgr); > + > + return 0; > +} drop this since cleanup is now automatic. > + > +static const struct of_device_id versal_fpga_of_match[] = { > + { .compatible = "xlnx,versal-fpga", }, > + {}, > +}; > + Nit: Drop the newline > +MODULE_DEVICE_TABLE(of, versal_fpga_of_match); > + > +static struct platform_driver versal_fpga_driver = { > + .probe = versal_fpga_probe, > + .remove = versal_fpga_remove, > + .driver = { > + .name = "versal_fpga_manager", > + .of_match_table = of_match_ptr(versal_fpga_of_match), > + }, > +}; > + Nit: Drop the newline > +module_platform_driver(versal_fpga_driver); > + > +MODULE_AUTHOR("Nava kishore Manne <nava.manne@xilinx.com>"); > +MODULE_AUTHOR("Appana Durga Kedareswara rao <appanad.durga.rao@xilinx.com>"); > +MODULE_DESCRIPTION("Xilinx Versal FPGA Manager"); > +MODULE_LICENSE("GPL"); > -- > 2.18.0 > Thanks, Moritz
Hi Moritz, Thanks for the review. Please find my response inline. > -----Original Message----- > From: Moritz Fischer <mdf@kernel.org> > Sent: Tuesday, January 19, 2021 6:03 AM > To: Nava kishore Manne <navam@xilinx.com> > Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek > <michals@xilinx.com>; linux-fpga@vger.kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; git <git@xilinx.com>; chinnikishore369@gmail.com; > Appana Durga Kedareswara Rao <appanad@xilinx.com> > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver > > Hi Nava, > > On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote: > > This patch adds driver for versal fpga manager. > Nit: Add support for Xilinx Versal FPGA manager Will fix in v2. > > > > PDI source type can be DDR, OCM, QSPI flash etc.. > No idea what PDI is :) Programmable device image (PDI). This file is generated by Xilinx Vivado tool and it contains configuration data objects. > > 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> > > --- > > drivers/fpga/Kconfig | 8 ++ > > drivers/fpga/Makefile | 1 + > > drivers/fpga/versal-fpga.c | 149 > > +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 158 insertions(+) > > create mode 100644 drivers/fpga/versal-fpga.c > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index > > 5645226ca3ce..9f779c3a6739 100644 > > --- a/drivers/fpga/Kconfig > > +++ b/drivers/fpga/Kconfig > > @@ -216,4 +216,12 @@ 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 versal soc firmware > > + interface to load programmable logic(PL) images > > + on versal soc. > > endif # FPGA > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index > > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182 > > --- /dev/null > > +++ b/drivers/fpga/versal-fpga.c > > @@ -0,0 +1,149 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 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> > > + > > +/* Constant Definitions */ > > +#define PDI_SOURCE_TYPE 0xF > > + > > +/** > > + * struct versal_fpga_priv - Private data structure > > + * @dev: Device data structure > > + * @flags: flags which is used to identify the PL Image type > > + */ > > +struct versal_fpga_priv { > > + struct device *dev; > > + u32 flags; > This seems unused ... please introduce them when/if you start using them. Will fix in v2. > > +}; > > + > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr, > > + struct fpga_image_info *info, > > + const char *buf, size_t size) { > > + struct versal_fpga_priv *priv; > > + > > + priv = mgr->priv; > > + priv->flags = info->flags; > ? What uses this ? It seems this function could just be 'return 0' right now. Will fix in v2. > > + > > + 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_SOURCE_TYPE, 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_OPERATING; > Is that always the case? Shouldn't that be FPGA_MGR_STATE_UNKNOWN? For Versal SoC base PDI is always configured prior to Linux boot up. So I make the fpga state as OPERATING. Please let know if it is not a proper implementation will think about the alternate solution. > > +} > > + > > +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 err, ret; > Please pick one, err or ret. 'err' seems unused? Will fix in v2. > > + > > + 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"); > Nit: "no usable DMA configuration\n" Will fix in v2. > > + return ret; > > + } > > + > > + mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager", > > + &versal_fpga_ops, priv); > > + if (!mgr) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, mgr); > > + > > Replace this part: > > + err = fpga_mgr_register(mgr); > > + if (err) { > > + dev_err(dev, "unable to register FPGA manager"); > > + fpga_mgr_free(mgr); > > + return err; > > + } > > with: > return devm_fpga_mgr_register(mgr); > > I tried to get rid of the boilerplate, since every driver repeats it (and above > calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create() created FPGA > manager is wrong?) :) Thanks for pointing it. Will fix in v2. > > + > > + return 0; > > +} > > + > > Then > > +static int versal_fpga_remove(struct platform_device *pdev) { > > + struct fpga_manager *mgr = platform_get_drvdata(pdev); > > + > > + fpga_mgr_unregister(mgr); > > + fpga_mgr_free(mgr); > > + > > + return 0; > > +} > drop this since cleanup is now automatic. Thanks for pointing it. Will fix in v2. > > + > > +static const struct of_device_id versal_fpga_of_match[] = { > > + { .compatible = "xlnx,versal-fpga", }, > > + {}, > > +}; > > + > Nit: Drop the newline Will fix in v2. > > +MODULE_DEVICE_TABLE(of, versal_fpga_of_match); > > + > > +static struct platform_driver versal_fpga_driver = { > > + .probe = versal_fpga_probe, > > + .remove = versal_fpga_remove, > > + .driver = { > > + .name = "versal_fpga_manager", > > + .of_match_table = of_match_ptr(versal_fpga_of_match), > > + }, > > +}; > > + > Nit: Drop the newline Will fix in v2. Regards, Navakishore.
Hi Nava, On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote: > Hi Moritz, > > Thanks for the review. > Please find my response inline. > > > -----Original Message----- > > From: Moritz Fischer <mdf@kernel.org> > > Sent: Tuesday, January 19, 2021 6:03 AM > > To: Nava kishore Manne <navam@xilinx.com> > > Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek > > <michals@xilinx.com>; linux-fpga@vger.kernel.org; > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > > kernel@vger.kernel.org; git <git@xilinx.com>; chinnikishore369@gmail.com; > > Appana Durga Kedareswara Rao <appanad@xilinx.com> > > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver > > > > Hi Nava, > > > > On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote: > > > This patch adds driver for versal fpga manager. > > Nit: Add support for Xilinx Versal FPGA manager > > Will fix in v2. > > > > > > > PDI source type can be DDR, OCM, QSPI flash etc.. > > No idea what PDI is :) > > Programmable device image (PDI). > This file is generated by Xilinx Vivado tool and it contains configuration data objects. > > > > 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> > > > --- > > > drivers/fpga/Kconfig | 8 ++ > > > drivers/fpga/Makefile | 1 + > > > drivers/fpga/versal-fpga.c | 149 > > > +++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 158 insertions(+) > > > create mode 100644 drivers/fpga/versal-fpga.c > > > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index > > > 5645226ca3ce..9f779c3a6739 100644 > > > --- a/drivers/fpga/Kconfig > > > +++ b/drivers/fpga/Kconfig > > > @@ -216,4 +216,12 @@ 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 versal soc firmware > > > + interface to load programmable logic(PL) images > > > + on versal soc. > > > endif # FPGA > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index > > > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182 > > > --- /dev/null > > > +++ b/drivers/fpga/versal-fpga.c > > > @@ -0,0 +1,149 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Copyright (C) 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> > > > + > > > +/* Constant Definitions */ > > > +#define PDI_SOURCE_TYPE 0xF > > > + > > > +/** > > > + * struct versal_fpga_priv - Private data structure > > > + * @dev: Device data structure > > > + * @flags: flags which is used to identify the PL Image type > > > + */ > > > +struct versal_fpga_priv { > > > + struct device *dev; > > > + u32 flags; > > This seems unused ... please introduce them when/if you start using them. > > Will fix in v2. > > > > +}; > > > + > > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr, > > > + struct fpga_image_info *info, > > > + const char *buf, size_t size) { > > > + struct versal_fpga_priv *priv; > > > + > > > + priv = mgr->priv; > > > + priv->flags = info->flags; > > ? What uses this ? It seems this function could just be 'return 0' right now. > > Will fix in v2. > > > > + > > > + 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_SOURCE_TYPE, 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_OPERATING; > > Is that always the case? Shouldn't that be FPGA_MGR_STATE_UNKNOWN? > > For Versal SoC base PDI is always configured prior to Linux boot up. So I make the fpga state as OPERATING. > Please let know if it is not a proper implementation will think about the alternate solution. So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded? Interesting :) > > > > +} > > > + > > > +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 err, ret; > > Please pick one, err or ret. 'err' seems unused? > > Will fix in v2. > > > > + > > > + 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"); > > Nit: "no usable DMA configuration\n" > > Will fix in v2. > > > > + return ret; > > > + } > > > + > > > + mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager", > > > + &versal_fpga_ops, priv); > > > + if (!mgr) > > > + return -ENOMEM; > > > + > > > + platform_set_drvdata(pdev, mgr); > > > + > > > > Replace this part: > > > + err = fpga_mgr_register(mgr); > > > + if (err) { > > > + dev_err(dev, "unable to register FPGA manager"); > > > + fpga_mgr_free(mgr); > > > + return err; > > > + } > > > > with: > > return devm_fpga_mgr_register(mgr); > > > > I tried to get rid of the boilerplate, since every driver repeats it (and above > > calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create() created FPGA > > manager is wrong?) :) > > Thanks for pointing it. Will fix in v2. > > > > + > > > + return 0; > > > +} > > > + > > > > Then > > > +static int versal_fpga_remove(struct platform_device *pdev) { > > > + struct fpga_manager *mgr = platform_get_drvdata(pdev); > > > + > > > + fpga_mgr_unregister(mgr); > > > + fpga_mgr_free(mgr); > > > + > > > + return 0; > > > +} > > drop this since cleanup is now automatic. > > Thanks for pointing it. Will fix in v2. > > > > + > > > +static const struct of_device_id versal_fpga_of_match[] = { > > > + { .compatible = "xlnx,versal-fpga", }, > > > + {}, > > > +}; > > > + > > Nit: Drop the newline > > Will fix in v2. > > > > +MODULE_DEVICE_TABLE(of, versal_fpga_of_match); > > > + > > > +static struct platform_driver versal_fpga_driver = { > > > + .probe = versal_fpga_probe, > > > + .remove = versal_fpga_remove, > > > + .driver = { > > > + .name = "versal_fpga_manager", > > > + .of_match_table = of_match_ptr(versal_fpga_of_match), > > > + }, > > > +}; > > > + > > Nit: Drop the newline > > Will fix in v2. > > Regards, > Navakishore. - Moritz
On Mon, Jan 18, 2021 at 08:13:16AM +0530, Nava kishore Manne wrote: > This patch adds load pdi api support to enable pdi/partial loading from > linux. Programmable Device Image (PDI) is combination of headers, images > and bitstream files to be loaded. Partial PDI is partial set of image/ > images to be loaded. > > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com> > --- > drivers/firmware/xilinx/zynqmp.c | 17 +++++++++++++++++ > include/linux/firmware/xlnx-zynqmp.h | 9 +++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c > index 7eb9958662dd..a466225b9f9e 100644 > --- a/drivers/firmware/xilinx/zynqmp.c > +++ b/drivers/firmware/xilinx/zynqmp.c > @@ -897,6 +897,23 @@ int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities, > } > EXPORT_SYMBOL_GPL(zynqmp_pm_set_requirement); > > +/** > + * zynqmp_pm_load_pdi - Load and process pdi Nit: Is it pdi or PDI. Pick one :) > + * @src: Source device where PDI is located > + * @address: Pdi src address > + * > + * This function provides support to load pdi from linux > + * > + * Return: Returns status, either success or error+reason > + */ > +int zynqmp_pm_load_pdi(const u32 src, const u64 address) > +{ > + return zynqmp_pm_invoke_fn(PM_LOAD_PDI, src, > + lower_32_bits(address), > + upper_32_bits(address), 0, NULL); > +} > +EXPORT_SYMBOL_GPL(zynqmp_pm_load_pdi); > + > /** > * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using > * AES-GCM core. > diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h > index 2a0da841c942..87114ee645b1 100644 > --- a/include/linux/firmware/xlnx-zynqmp.h > +++ b/include/linux/firmware/xlnx-zynqmp.h > @@ -52,6 +52,9 @@ > #define ZYNQMP_PM_CAPABILITY_WAKEUP 0x4U > #define ZYNQMP_PM_CAPABILITY_UNUSABLE 0x8U > > +/* Loader commands */ > +#define PM_LOAD_PDI 0x701 > + > /* > * Firmware FPGA Manager flags > * XILINX_ZYNQMP_PM_FPGA_FULL: FPGA full reconfiguration > @@ -354,6 +357,7 @@ int zynqmp_pm_write_pggs(u32 index, u32 value); > int zynqmp_pm_read_pggs(u32 index, u32 *value); > int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype); > int zynqmp_pm_set_boot_health_status(u32 value); > +int zynqmp_pm_load_pdi(const u32 src, const u64 address); > #else > static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void) > { > @@ -538,6 +542,11 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value) > { > return -ENODEV; > } > + > +static inline int zynqmp_pm_load_pdi(const u32 src, const u64 address) > +{ > + return -ENODEV; > +} > #endif > > #endif /* __FIRMWARE_ZYNQMP_H__ */ > -- > 2.18.0 >
Hi Moritz, Please find my response inline. > -----Original Message----- > From: Moritz Fischer <mdf@kernel.org> > Sent: Sunday, January 24, 2021 5:04 AM > To: Nava kishore Manne <navam@xilinx.com> > Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com; > robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux- > fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara > Rao <appanad@xilinx.com> > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver > > Hi Nava, > > On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote: > > Hi Moritz, > > > > Thanks for the review. > > Please find my response inline. > > > > > -----Original Message----- > > > From: Moritz Fischer <mdf@kernel.org> > > > Sent: Tuesday, January 19, 2021 6:03 AM > > > To: Nava kishore Manne <navam@xilinx.com> > > > Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal > > > Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org; > > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > linux- kernel@vger.kernel.org; git <git@xilinx.com>; > > > chinnikishore369@gmail.com; Appana Durga Kedareswara Rao > > > <appanad@xilinx.com> > > > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager > > > driver > > > > > > Hi Nava, > > > > > > On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote: > > > > This patch adds driver for versal fpga manager. > > > Nit: Add support for Xilinx Versal FPGA manager > > > > Will fix in v2. > > > > > > > > > > PDI source type can be DDR, OCM, QSPI flash etc.. > > > No idea what PDI is :) > > > > Programmable device image (PDI). > > This file is generated by Xilinx Vivado tool and it contains configuration data > objects. > > > > > > 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> > > > > --- > > > > drivers/fpga/Kconfig | 8 ++ > > > > drivers/fpga/Makefile | 1 + > > > > drivers/fpga/versal-fpga.c | 149 > > > > +++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 158 insertions(+) create mode 100644 > > > > drivers/fpga/versal-fpga.c > > > > > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index > > > > 5645226ca3ce..9f779c3a6739 100644 > > > > --- a/drivers/fpga/Kconfig > > > > +++ b/drivers/fpga/Kconfig > > > > @@ -216,4 +216,12 @@ 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 versal soc firmware > > > > + interface to load programmable logic(PL) images > > > > + on versal soc. > > > > endif # FPGA > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index > > > > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182 > > > > --- /dev/null > > > > +++ b/drivers/fpga/versal-fpga.c > > > > @@ -0,0 +1,149 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* > > > > + * Copyright (C) 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> > > > > + > > > > +/* Constant Definitions */ > > > > +#define PDI_SOURCE_TYPE 0xF > > > > + > > > > +/** > > > > + * struct versal_fpga_priv - Private data structure > > > > + * @dev: Device data structure > > > > + * @flags: flags which is used to identify the PL Image type > > > > + */ > > > > +struct versal_fpga_priv { > > > > + struct device *dev; > > > > + u32 flags; > > > This seems unused ... please introduce them when/if you start using > them. > > > > Will fix in v2. > > > > > > +}; > > > > + > > > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr, > > > > + struct fpga_image_info *info, > > > > + const char *buf, size_t size) { > > > > + struct versal_fpga_priv *priv; > > > > + > > > > + priv = mgr->priv; > > > > + priv->flags = info->flags; > > > ? What uses this ? It seems this function could just be 'return 0' right now. > > > > Will fix in v2. > > > > > > + > > > > + 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_SOURCE_TYPE, 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_OPERATING; > > > Is that always the case? Shouldn't that be > FPGA_MGR_STATE_UNKNOWN? > > > > For Versal SoC base PDI is always configured prior to Linux boot up. So I > make the fpga state as OPERATING. > > Please let know if it is not a proper implementation will think about the > alternate solution. > > So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded? > Interesting :) > > For Versal SoC Vivado generated base PDI is always needed to bring-up the board. Regards, Navakishore.
Hi On 1/27/21 9:57 AM, Nava kishore Manne wrote: > Hi Moritz, > > Please find my response inline. > >> -----Original Message----- >> From: Moritz Fischer <mdf@kernel.org> >> Sent: Sunday, January 24, 2021 5:04 AM >> To: Nava kishore Manne <navam@xilinx.com> >> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com; >> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux- >> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git >> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara >> Rao <appanad@xilinx.com> >> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver >> >> Hi Nava, >> >> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote: >>> Hi Moritz, >>> >>> Thanks for the review. >>> Please find my response inline. >>> >>>> -----Original Message----- >>>> From: Moritz Fischer <mdf@kernel.org> >>>> Sent: Tuesday, January 19, 2021 6:03 AM >>>> To: Nava kishore Manne <navam@xilinx.com> >>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal >>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org; >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>; >>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao >>>> <appanad@xilinx.com> >>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager >>>> driver >>>> >>>> Hi Nava, >>>> >>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote: >>>>> This patch adds driver for versal fpga manager. >>>> Nit: Add support for Xilinx Versal FPGA manager >>> >>> Will fix in v2. >>> >>>>> >>>>> PDI source type can be DDR, OCM, QSPI flash etc.. >>>> No idea what PDI is :) >>> >>> Programmable device image (PDI). >>> This file is generated by Xilinx Vivado tool and it contains configuration data >> objects. >>> >>>>> 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> >>>>> --- >>>>> drivers/fpga/Kconfig | 8 ++ >>>>> drivers/fpga/Makefile | 1 + >>>>> drivers/fpga/versal-fpga.c | 149 >>>>> +++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 158 insertions(+) create mode 100644 >>>>> drivers/fpga/versal-fpga.c >>>>> >>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index >>>>> 5645226ca3ce..9f779c3a6739 100644 >>>>> --- a/drivers/fpga/Kconfig >>>>> +++ b/drivers/fpga/Kconfig >>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware >>>>> + interface to load programmable logic(PL) images >>>>> + on versal soc. >>>>> endif # FPGA >>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index >>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182 >>>>> --- /dev/null >>>>> +++ b/drivers/fpga/versal-fpga.c >>>>> @@ -0,0 +1,149 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0+ >>>>> +/* >>>>> + * Copyright (C) 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> >>>>> + >>>>> +/* Constant Definitions */ >>>>> +#define PDI_SOURCE_TYPE 0xF >>>>> + >>>>> +/** >>>>> + * struct versal_fpga_priv - Private data structure >>>>> + * @dev: Device data structure >>>>> + * @flags: flags which is used to identify the PL Image type >>>>> + */ >>>>> +struct versal_fpga_priv { >>>>> + struct device *dev; >>>>> + u32 flags; >>>> This seems unused ... please introduce them when/if you start using >> them. >>> >>> Will fix in v2. >>> >>>>> +}; >>>>> + >>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr, >>>>> + struct fpga_image_info *info, >>>>> + const char *buf, size_t size) { >>>>> + struct versal_fpga_priv *priv; >>>>> + >>>>> + priv = mgr->priv; >>>>> + priv->flags = info->flags; >>>> ? What uses this ? It seems this function could just be 'return 0' right now. >>> >>> Will fix in v2. >>> >>>>> + >>>>> + 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_SOURCE_TYPE, 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_OPERATING; >>>> Is that always the case? Shouldn't that be >> FPGA_MGR_STATE_UNKNOWN? >>> >>> For Versal SoC base PDI is always configured prior to Linux boot up. So I >> make the fpga state as OPERATING. >>> Please let know if it is not a proper implementation will think about the >> alternate solution. >> >> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded? >> Interesting :) >>> > > For Versal SoC Vivado generated base PDI is always needed to bring-up the board. Look at PDI as ps7_init/psu_init file but in different format. And bitstream is optional part of it (like a one partition). Thanks, Michal
On Wed, Jan 27, 2021 at 10:16:32AM +0100, Michal Simek wrote: > Hi > > On 1/27/21 9:57 AM, Nava kishore Manne wrote: > > Hi Moritz, > > > > Please find my response inline. > > > >> -----Original Message----- > >> From: Moritz Fischer <mdf@kernel.org> > >> Sent: Sunday, January 24, 2021 5:04 AM > >> To: Nava kishore Manne <navam@xilinx.com> > >> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com; > >> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux- > >> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > >> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara > >> Rao <appanad@xilinx.com> > >> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver > >> > >> Hi Nava, > >> > >> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote: > >>> Hi Moritz, > >>> > >>> Thanks for the review. > >>> Please find my response inline. > >>> > >>>> -----Original Message----- > >>>> From: Moritz Fischer <mdf@kernel.org> > >>>> Sent: Tuesday, January 19, 2021 6:03 AM > >>>> To: Nava kishore Manne <navam@xilinx.com> > >>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal > >>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org; > >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>; > >>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao > >>>> <appanad@xilinx.com> > >>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager > >>>> driver > >>>> > >>>> Hi Nava, > >>>> > >>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote: > >>>>> This patch adds driver for versal fpga manager. > >>>> Nit: Add support for Xilinx Versal FPGA manager > >>> > >>> Will fix in v2. > >>> > >>>>> > >>>>> PDI source type can be DDR, OCM, QSPI flash etc.. > >>>> No idea what PDI is :) > >>> > >>> Programmable device image (PDI). > >>> This file is generated by Xilinx Vivado tool and it contains configuration data > >> objects. > >>> > >>>>> 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> > >>>>> --- > >>>>> drivers/fpga/Kconfig | 8 ++ > >>>>> drivers/fpga/Makefile | 1 + > >>>>> drivers/fpga/versal-fpga.c | 149 > >>>>> +++++++++++++++++++++++++++++++++++++ > >>>>> 3 files changed, 158 insertions(+) create mode 100644 > >>>>> drivers/fpga/versal-fpga.c > >>>>> > >>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index > >>>>> 5645226ca3ce..9f779c3a6739 100644 > >>>>> --- a/drivers/fpga/Kconfig > >>>>> +++ b/drivers/fpga/Kconfig > >>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware > >>>>> + interface to load programmable logic(PL) images > >>>>> + on versal soc. > >>>>> endif # FPGA > >>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index > >>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182 > >>>>> --- /dev/null > >>>>> +++ b/drivers/fpga/versal-fpga.c > >>>>> @@ -0,0 +1,149 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0+ > >>>>> +/* > >>>>> + * Copyright (C) 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> > >>>>> + > >>>>> +/* Constant Definitions */ > >>>>> +#define PDI_SOURCE_TYPE 0xF > >>>>> + > >>>>> +/** > >>>>> + * struct versal_fpga_priv - Private data structure > >>>>> + * @dev: Device data structure > >>>>> + * @flags: flags which is used to identify the PL Image type > >>>>> + */ > >>>>> +struct versal_fpga_priv { > >>>>> + struct device *dev; > >>>>> + u32 flags; > >>>> This seems unused ... please introduce them when/if you start using > >> them. > >>> > >>> Will fix in v2. > >>> > >>>>> +}; > >>>>> + > >>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr, > >>>>> + struct fpga_image_info *info, > >>>>> + const char *buf, size_t size) { > >>>>> + struct versal_fpga_priv *priv; > >>>>> + > >>>>> + priv = mgr->priv; > >>>>> + priv->flags = info->flags; > >>>> ? What uses this ? It seems this function could just be 'return 0' right now. > >>> > >>> Will fix in v2. > >>> > >>>>> + > >>>>> + 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_SOURCE_TYPE, 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_OPERATING; > >>>> Is that always the case? Shouldn't that be > >> FPGA_MGR_STATE_UNKNOWN? > >>> > >>> For Versal SoC base PDI is always configured prior to Linux boot up. So I > >> make the fpga state as OPERATING. > >>> Please let know if it is not a proper implementation will think about the > >> alternate solution. > >> > >> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded? > >> Interesting :) > >>> > > > > For Versal SoC Vivado generated base PDI is always needed to bring-up the board. > > Look at PDI as ps7_init/psu_init file but in different format. And > bitstream is optional part of it (like a one partition). So at that point I could still have no bitstream loaded (optional), and my status would be 'unknown' not 'operating' if I cannot tell the two cases apart. What am I missing? :) Thanks, Moritz
Hi, > -----Original Message----- > From: Moritz Fischer <mdf@kernel.org> > Sent: Thursday, January 28, 2021 3:15 AM > To: Michal Simek <michals@xilinx.com> > Cc: Nava kishore Manne <navam@xilinx.com>; Moritz Fischer > <mdf@kernel.org>; trix@redhat.com; robh+dt@kernel.org; linux- > fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara > Rao <appanad@xilinx.com> > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver > > On Wed, Jan 27, 2021 at 10:16:32AM +0100, Michal Simek wrote: > > Hi > > > > On 1/27/21 9:57 AM, Nava kishore Manne wrote: > > > Hi Moritz, > > > > > > Please find my response inline. > > > > > >> -----Original Message----- > > >> From: Moritz Fischer <mdf@kernel.org> > > >> Sent: Sunday, January 24, 2021 5:04 AM > > >> To: Nava kishore Manne <navam@xilinx.com> > > >> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com; > > >> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux- > > >> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > > >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git > > >> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga > > >> Kedareswara Rao <appanad@xilinx.com> > > >> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager > > >> driver > > >> > > >> Hi Nava, > > >> > > >> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote: > > >>> Hi Moritz, > > >>> > > >>> Thanks for the review. > > >>> Please find my response inline. > > >>> > > >>>> -----Original Message----- > > >>>> From: Moritz Fischer <mdf@kernel.org> > > >>>> Sent: Tuesday, January 19, 2021 6:03 AM > > >>>> To: Nava kishore Manne <navam@xilinx.com> > > >>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal > > >>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org; > > >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > >>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>; > > >>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao > > >>>> <appanad@xilinx.com> > > >>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga > > >>>> manager driver > > >>>> > > >>>> Hi Nava, > > >>>> > > >>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne > wrote: > > >>>>> This patch adds driver for versal fpga manager. > > >>>> Nit: Add support for Xilinx Versal FPGA manager > > >>> > > >>> Will fix in v2. > > >>> > > >>>>> > > >>>>> PDI source type can be DDR, OCM, QSPI flash etc.. > > >>>> No idea what PDI is :) > > >>> > > >>> Programmable device image (PDI). > > >>> This file is generated by Xilinx Vivado tool and it contains > > >>> configuration data > > >> objects. > > >>> > > >>>>> 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> > > >>>>> --- > > >>>>> drivers/fpga/Kconfig | 8 ++ > > >>>>> drivers/fpga/Makefile | 1 + > > >>>>> drivers/fpga/versal-fpga.c | 149 > > >>>>> +++++++++++++++++++++++++++++++++++++ > > >>>>> 3 files changed, 158 insertions(+) create mode 100644 > > >>>>> drivers/fpga/versal-fpga.c > > >>>>> > > >>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index > > >>>>> 5645226ca3ce..9f779c3a6739 100644 > > >>>>> --- a/drivers/fpga/Kconfig > > >>>>> +++ b/drivers/fpga/Kconfig > > >>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware > > >>>>> + interface to load programmable logic(PL) images > > >>>>> + on versal soc. > > >>>>> endif # FPGA > > >>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index > > >>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182 > > >>>>> --- /dev/null > > >>>>> +++ b/drivers/fpga/versal-fpga.c > > >>>>> @@ -0,0 +1,149 @@ > > >>>>> +// SPDX-License-Identifier: GPL-2.0+ > > >>>>> +/* > > >>>>> + * Copyright (C) 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> > > >>>>> + > > >>>>> +/* Constant Definitions */ > > >>>>> +#define PDI_SOURCE_TYPE 0xF > > >>>>> + > > >>>>> +/** > > >>>>> + * struct versal_fpga_priv - Private data structure > > >>>>> + * @dev: Device data structure > > >>>>> + * @flags: flags which is used to identify the PL Image type > > >>>>> + */ > > >>>>> +struct versal_fpga_priv { > > >>>>> + struct device *dev; > > >>>>> + u32 flags; > > >>>> This seems unused ... please introduce them when/if you start > > >>>> using > > >> them. > > >>> > > >>> Will fix in v2. > > >>> > > >>>>> +}; > > >>>>> + > > >>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr, > > >>>>> + struct fpga_image_info *info, > > >>>>> + const char *buf, size_t size) { > > >>>>> + struct versal_fpga_priv *priv; > > >>>>> + > > >>>>> + priv = mgr->priv; > > >>>>> + priv->flags = info->flags; > > >>>> ? What uses this ? It seems this function could just be 'return 0' right > now. > > >>> > > >>> Will fix in v2. > > >>> > > >>>>> + > > >>>>> + 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_SOURCE_TYPE, 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_OPERATING; > > >>>> Is that always the case? Shouldn't that be > > >> FPGA_MGR_STATE_UNKNOWN? > > >>> > > >>> For Versal SoC base PDI is always configured prior to Linux boot > > >>> up. So I > > >> make the fpga state as OPERATING. > > >>> Please let know if it is not a proper implementation will think > > >>> about the > > >> alternate solution. > > >> > > >> So you're saying I can't boot a Versal SoC without a PDI / Bitstream > loaded? > > >> Interesting :) > > >>> > > > > > > For Versal SoC Vivado generated base PDI is always needed to bring-up > the board. > > > > Look at PDI as ps7_init/psu_init file but in different format. And > > bitstream is optional part of it (like a one partition). > > So at that point I could still have no bitstream loaded (optional), and my > status would be 'unknown' not 'operating' if I cannot tell the two cases apart. > What am I missing? :) > Agree, In few cases Bitstream partition is optional, So we can't say exactly whether the PL is having Bitstream or Not . So here the ideal default PL state should be Unknown. Will fix in v2. Regards, Navakishore.
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c index 7eb9958662dd..a466225b9f9e 100644 --- a/drivers/firmware/xilinx/zynqmp.c +++ b/drivers/firmware/xilinx/zynqmp.c @@ -897,6 +897,23 @@ int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities, } EXPORT_SYMBOL_GPL(zynqmp_pm_set_requirement); +/** + * zynqmp_pm_load_pdi - Load and process pdi + * @src: Source device where PDI is located + * @address: Pdi src address + * + * This function provides support to load pdi from linux + * + * Return: Returns status, either success or error+reason + */ +int zynqmp_pm_load_pdi(const u32 src, const u64 address) +{ + return zynqmp_pm_invoke_fn(PM_LOAD_PDI, src, + lower_32_bits(address), + upper_32_bits(address), 0, NULL); +} +EXPORT_SYMBOL_GPL(zynqmp_pm_load_pdi); + /** * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using * AES-GCM core. diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h index 2a0da841c942..87114ee645b1 100644 --- a/include/linux/firmware/xlnx-zynqmp.h +++ b/include/linux/firmware/xlnx-zynqmp.h @@ -52,6 +52,9 @@ #define ZYNQMP_PM_CAPABILITY_WAKEUP 0x4U #define ZYNQMP_PM_CAPABILITY_UNUSABLE 0x8U +/* Loader commands */ +#define PM_LOAD_PDI 0x701 + /* * Firmware FPGA Manager flags * XILINX_ZYNQMP_PM_FPGA_FULL: FPGA full reconfiguration @@ -354,6 +357,7 @@ int zynqmp_pm_write_pggs(u32 index, u32 value); int zynqmp_pm_read_pggs(u32 index, u32 *value); int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype); int zynqmp_pm_set_boot_health_status(u32 value); +int zynqmp_pm_load_pdi(const u32 src, const u64 address); #else static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void) { @@ -538,6 +542,11 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value) { return -ENODEV; } + +static inline int zynqmp_pm_load_pdi(const u32 src, const u64 address) +{ + return -ENODEV; +} #endif #endif /* __FIRMWARE_ZYNQMP_H__ */
This patch adds load pdi api support to enable pdi/partial loading from linux. Programmable Device Image (PDI) is combination of headers, images and bitstream files to be loaded. Partial PDI is partial set of image/ images to be loaded. Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com> --- drivers/firmware/xilinx/zynqmp.c | 17 +++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 9 +++++++++ 2 files changed, 26 insertions(+)