[4/4] fpga: zynqmp: Add afi config driver

Commit Message

Manne, Nava kishore Aug. 24, 2022, 3:55 a.m. UTC

Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for the
configuration of the PS-PL interface on Zynq US+ MPSoC platform.

Signed-off-by: Nava kishore Manne <[e-mail]>
---
 MAINTAINERS               |   6 ++
 drivers/fpga/Kconfig      |  13 +++
 drivers/fpga/Makefile     |   1 +
 drivers/fpga/zynqmp-afi.c | 211 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 drivers/fpga/zynqmp-afi.c

Comments

Krzysztof Kozlowski Aug. 24, 2022, 12:59 p.m. UTC | #1

On 24/08/2022 06:55, Nava kishore Manne wrote:
> Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for the
> configuration of the PS-PL interface on Zynq US+ MPSoC platform.
> 
> Signed-off-by: Nava kishore Manne <[e-mail]>
> ---

(...)

> +
> +static struct platform_driver zynqmp_afi_driver = {
> +	.probe = zynqmp_afi_probe,
> +	.remove = zynqmp_afi_remove,
> +	.driver = {
> +		.name = "zynqmp_afi",
> +		.of_match_table = of_match_ptr(zynqmp_afi_of_match),

It should generate warnings... test compile your driver.

Drop of_match_ptr() or use __maybe_unused

Best regards,
Krzysztof

Xu Yilun Aug. 27, 2022, 12:26 p.m. UTC | #2

On 2022-08-24 at 09:25:42 +0530, Nava kishore Manne wrote:
> Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for the
> configuration of the PS-PL interface on Zynq US+ MPSoC platform.

Please help illustrate how to use the device for FPGA reprogramming, why
it should be implemented as an FPGA bridge.

From the code I actually didn't see any operation that gates the
fpga-region from other part of the machine.

> 
> Signed-off-by: Nava kishore Manne <[e-mail]>
> ---
>  MAINTAINERS               |   6 ++
>  drivers/fpga/Kconfig      |  13 +++
>  drivers/fpga/Makefile     |   1 +
>  drivers/fpga/zynqmp-afi.c | 211 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 231 insertions(+)
>  create mode 100644 drivers/fpga/zynqmp-afi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 20ffac651214..957e753e6406 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8032,6 +8032,12 @@ F:	Documentation/fpga/
>  F:	drivers/fpga/
>  F:	include/linux/fpga/
>  
> +FPGA ZYNQMP PS-PL BRIDGE DRIVER
> +M:	Nava kishore Manne <[e-mail]>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/fpga/xlnx,zynqmp-afi-fpga.yaml
> +F:	drivers/fpga/zynqmp-afi.c
> +
>  INTEL MAX10 BMC SECURE UPDATES
>  M:	Russ Weight <[e-mail]>
>  L:	[e-mail]
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 6c416955da53..c08794d30fb5 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -130,6 +130,19 @@ config XILINX_PR_DECOUPLER
>  	  reconfiguration, preventing the system deadlock that can
>  	  occur if AXI transactions are interrupted by DFX.
>  
> +config ZYNQMP_AFI
> +	tristate "Xilinx ZYNQMP AFI support"
> +	depends on FPGA_BRIDGE
> +	help
> +	  Say Y to enable drivers to handle the PS-PL clocks configurations
> +	  and PS-PL Bus-width. Xilinx Zynq US+ MPSoC connect the PS to the
> +	  programmable logic (PL) through the AXI port. This AXI port helps
> +	  to establish the data path between the PS and PL.
> +	  In-order to establish the proper communication path between PS and PL,
> +	  the AXI port data path should be configured with the proper Bus-width
> +	  values and it will also handles the PS-PL reset signals to reset the
> +	  PL domain.

Same concern, please describe its relationship to FPGA reprogramming.

> +
>  config FPGA_REGION
>  	tristate "FPGA Region"
>  	depends on FPGA_BRIDGE
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 42ae8b58abce..94cfe60972db 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>  obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)	+= altera-freeze-bridge.o
>  obj-$(CONFIG_XILINX_PR_DECOUPLER)	+= xilinx-pr-decoupler.o
> +obj-$(CONFIG_ZYNQMP_AFI)		+= zynqmp-afi.o
>  
>  # High Level Interfaces
>  obj-$(CONFIG_FPGA_REGION)		+= fpga-region.o
> diff --git a/drivers/fpga/zynqmp-afi.c b/drivers/fpga/zynqmp-afi.c
> new file mode 100644
> index 000000000000..bc975d304039
> --- /dev/null
> +++ b/drivers/fpga/zynqmp-afi.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Xilinx, Inc.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/fpga/fpga-bridge.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +/* Registers and special values for doing register-based operations */
> +#define AFI_RDCHAN_CTRL_OFFSET	0x00
> +#define AFI_WRCHAN_CTRL_OFFSET	0x14
> +#define AFI_BUSWIDTH_MASK	BIT(0)
> +
> +/**
> + * struct zynqmp_afi - AFI register description.
> + * @dev: device that owns this.
> + * @of_node: Device Tree overlay.
> + * @resets: Pointer to the reset control for ps-pl resets.
> + */
> +struct zynqmp_afi {
> +	struct device *dev;
> +	struct device_node *of_node;
> +	struct reset_control *resets;
> +};
> +
> +/**
> + * struct zynqmp_afi_configreg - AFI configuration registers info.
> + * @reg:	Name of the AFI configuration register.
> + * @id:		Register index value.
> + */
> +struct zynqmp_afi_configreg {
> +	char *reg;
> +	u32 id;
> +};
> +
> +static struct zynqmp_afi_configreg afi_cfgreg[] = {
> +	{.reg = "xlnx,afi-fm0-rd-bus-width",	.id = AFIFM0_RDCTRL},
> +	{.reg = "xlnx,afi-fm1-rd-bus-width",	.id = AFIFM1_RDCTRL},
> +	{.reg = "xlnx,afi-fm2-rd-bus-width",	.id = AFIFM2_RDCTRL},
> +	{.reg = "xlnx,afi-fm3-rd-bus-width",	.id = AFIFM3_RDCTRL},
> +	{.reg = "xlnx,afi-fm4-rd-bus-width",	.id = AFIFM4_RDCTRL},
> +	{.reg = "xlnx,afi-fm5-rd-bus-width",	.id = AFIFM5_RDCTRL},
> +	{.reg = "xlnx,afi-fm6-rd-bus-width",	.id = AFIFM6_RDCTRL},
> +	{.reg = "xlnx,afi-fm0-wr-bus-width",	.id = AFIFM0_WRCTRL},
> +	{.reg = "xlnx,afi-fm1-wr-bus-width",	.id = AFIFM1_WRCTRL},
> +	{.reg = "xlnx,afi-fm2-wr-bus-width",	.id = AFIFM2_WRCTRL},
> +	{.reg = "xlnx,afi-fm3-wr-bus-width",	.id = AFIFM3_WRCTRL},
> +	{.reg = "xlnx,afi-fm4-wr-bus-width",	.id = AFIFM4_WRCTRL},
> +	{.reg = "xlnx,afi-fm5-wr-bus-width",	.id = AFIFM5_WRCTRL},
> +	{.reg = "xlnx,afi-fm6-wr-bus-width",	.id = AFIFM6_WRCTRL},
> +	{.reg = "xlnx,afi-fs-ss0-bus-width",	.id = AFIFS},
> +	{.reg = "xlnx,afi-fs-ss2-bus-width",	.id = AFIFS_SS2},
> +	{}
> +};
> +
> +static int zynqmp_afi_config(struct zynqmp_afi *afi_data)
> +{
> +	struct zynqmp_afi_configreg *cfgptr = afi_cfgreg;
> +	struct device_node *np = afi_data->of_node;
> +	u32 afi_ss0_val, afi_ss1_val, bus_width;
> +	int ret;
> +
> +	while (cfgptr->reg) {
> +		ret = of_property_read_u32(np, cfgptr->reg, &bus_width);
> +		if (!ret) {
> +			if (cfgptr->id == AFIFS_SS2) {
> +				if (bus_width == 32)
> +					ret = zynqmp_pm_afi(AFIFS_SS2,
> +							    AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL);
> +				else if (bus_width == 64)
> +					ret = zynqmp_pm_afi(AFIFS_SS2,
> +							    AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL);
> +				else if (bus_width == 128)
> +					ret = zynqmp_pm_afi(AFIFS_SS2,
> +							    AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL);
> +				else
> +					return -EINVAL;
> +			} else if (cfgptr->id == AFIFS) {
> +				if (bus_width == 32)
> +					afi_ss0_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> +				else if (bus_width == 64)
> +					afi_ss0_val = AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL;
> +				else if (bus_width == 128)
> +					afi_ss0_val = AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL;
> +				else
> +					return -EINVAL;
> +
> +				ret = of_property_read_u32(np, "xlnx,afi-fs-ss1-bus-width",
> +							   &bus_width);
> +				if (!ret) {
> +					if (bus_width == 32)
> +						afi_ss1_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> +					else if (bus_width == 64)
> +						afi_ss1_val = AFIFS_SS1_BUS_WIDTH_64_CONFIG_VAL;
> +					else if (bus_width == 128)
> +						afi_ss1_val = AFIFS_SS1_BUS_WIDTH_128_CONFIG_VAL;
> +					else
> +						return -EINVAL;
> +
> +					ret = zynqmp_pm_afi(AFIFS, afi_ss1_val | afi_ss0_val);
> +				}
> +			} else {
> +				if (bus_width == 32)
> +					ret = zynqmp_pm_afi(cfgptr->id,
> +							    AFIFM_BUS_WIDTH_32_CONFIG_VAL);
> +				else if (bus_width == 64)
> +					ret = zynqmp_pm_afi(cfgptr->id,
> +							    AFIFM_BUS_WIDTH_64_CONFIG_VAL);
> +				else if (bus_width == 128)
> +					ret = zynqmp_pm_afi(cfgptr->id,
> +							    AFIFM_BUS_WIDTH_128_CONFIG_VAL);
> +				else
> +					return -EINVAL;
> +			}
> +		}
> +		cfgptr++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int zynqmp_afi_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> +	struct device_node *overlay = bridge->info->overlay;
> +	struct zynqmp_afi *priv = bridge->priv;
> +	int ret = 0;
> +
> +	if (enable) {
> +		reset_control_reset(priv->resets);
> +		return 0;
> +	}
> +
> +	of_node_get(overlay);
> +	priv->of_node =
> +	of_find_node_with_property(overlay,
> +				   "xlnx,afi-fm0-rd-bus-width");

I'm a little confused here. The fpga_image_info.overlay is for
fpga-region, but from your binding doc this property is for this afi
device. You want to add another overlay targeting afi dev node during
reprograming?

> +	if (priv->of_node)
> +		ret = zynqmp_afi_config(priv);
> +	of_node_put(priv->of_node);
> +
> +	return ret;
> +}
> +
> +static const struct fpga_bridge_ops zynqmp_afi_br_ops = {
> +	.enable_set = zynqmp_afi_enable_set,
> +};
> +
> +static const struct of_device_id zynqmp_afi_of_match[] = {
> +	{ .compatible =  "xlnx,zynqmp-afi-fpga" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, zynqmp_afi_of_match);
> +
> +static int zynqmp_afi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct zynqmp_afi *priv;
> +	struct fpga_bridge *br;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +
> +	priv->resets = devm_reset_control_array_get_optional_exclusive(&pdev->dev);
> +	if (IS_ERR(priv->resets))
> +		return PTR_ERR(priv->resets);
> +
> +	br = fpga_bridge_register(dev, "Xilinx ZynqMP AFI",
> +				  &zynqmp_afi_br_ops, priv);
> +	if (IS_ERR(br)) {
> +		dev_err(dev, "unable to register Xilinx ZynqMP AFI");

Need a "\n" at the end?

Thanks,
Yilun

> +		return PTR_ERR(br);
> +	}
> +
> +	platform_set_drvdata(pdev, br);
> +
> +	return 0;
> +}
> +
> +static int zynqmp_afi_remove(struct platform_device *pdev)
> +{
> +	struct fpga_bridge *bridge = platform_get_drvdata(pdev);
> +
> +	fpga_bridge_unregister(bridge);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver zynqmp_afi_driver = {
> +	.probe = zynqmp_afi_probe,
> +	.remove = zynqmp_afi_remove,
> +	.driver = {
> +		.name = "zynqmp_afi",
> +		.of_match_table = of_match_ptr(zynqmp_afi_of_match),
> +	},
> +};
> +
> +module_platform_driver(zynqmp_afi_driver);
> +
> +MODULE_DESCRIPTION("ZYNQMP FPGA AXI FIFO interface configuration module");
> +MODULE_AUTHOR("Nava kishore Manne <[e-mail]>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
>

Manne, Nava kishore Aug. 30, 2022, 9:19 a.m. UTC | #3

Hi Yilun,

	Please find my response inline.

> -----Original Message-----
> From: Xu Yilun <[e-mail]>
> Sent: Saturday, August 27, 2022 5:56 PM
> To: Manne, Nava kishore <[e-mail]>
> Cc: git (AMD-Xilinx) <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]
> Subject: Re: [PATCH 4/4] fpga: zynqmp: Add afi config driver
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On 2022-08-24 at 09:25:42 +0530, Nava kishore Manne wrote:
> > Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for
> > the configuration of the PS-PL interface on Zynq US+ MPSoC platform.
> 
> Please help illustrate how to use the device for FPGA reprogramming, why it
> should be implemented as an FPGA bridge.
> 
> From the code I actually didn't see any operation that gates the fpga-region
> from other part of the machine.
> 

The Zynq UltraScale MPSoC family consists of a system-on-chip (SoC) style integrated processing system (PS)
and a Programmable Logic (PL) unit, providing an extensible and flexible SoC solution on a single die. 
Xilinx Zynq US+ MPSoC connect the PS to the programmable logic (PL) through the AXI port. 
This AXI port helps to establish the data path between the PS and PL (Here AXI Interface act as a Gating between PS and PL)
and this AXI port configuration vary from design to design.  In-order to establish the proper communication path between
PS and PL (Full region),  the AXI port data path should be configured with proper values priories to load the full region.

Will update the description in v2.

> >
> > Signed-off-by: Nava kishore Manne <[e-mail]>
> > ---
> >  MAINTAINERS               |   6 ++
> >  drivers/fpga/Kconfig      |  13 +++
> >  drivers/fpga/Makefile     |   1 +
> >  drivers/fpga/zynqmp-afi.c | 211
> > ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 231 insertions(+)
> >  create mode 100644 drivers/fpga/zynqmp-afi.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 20ffac651214..957e753e6406 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8032,6 +8032,12 @@ F:     Documentation/fpga/
> >  F:   drivers/fpga/
> >  F:   include/linux/fpga/
> >
> > +FPGA ZYNQMP PS-PL BRIDGE DRIVER
> > +M:   Nava kishore Manne <[e-mail]>
> > +S:   Supported
> > +F:   Documentation/devicetree/bindings/fpga/xlnx,zynqmp-afi-fpga.yaml
> > +F:   drivers/fpga/zynqmp-afi.c
> > +
> >  INTEL MAX10 BMC SECURE UPDATES
> >  M:   Russ Weight <[e-mail]>
> >  L:   [e-mail]
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 6c416955da53..c08794d30fb5 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -130,6 +130,19 @@ config XILINX_PR_DECOUPLER
> >         reconfiguration, preventing the system deadlock that can
> >         occur if AXI transactions are interrupted by DFX.
> >
> > +config ZYNQMP_AFI
> > +     tristate "Xilinx ZYNQMP AFI support"
> > +     depends on FPGA_BRIDGE
> > +     help
> > +       Say Y to enable drivers to handle the PS-PL clocks configurations
> > +       and PS-PL Bus-width. Xilinx Zynq US+ MPSoC connect the PS to the
> > +       programmable logic (PL) through the AXI port. This AXI port helps
> > +       to establish the data path between the PS and PL.
> > +       In-order to establish the proper communication path between PS and
> PL,
> > +       the AXI port data path should be configured with the proper Bus-
> width
> > +       values and it will also handles the PS-PL reset signals to reset the
> > +       PL domain.
> 
> Same concern, please describe its relationship to FPGA reprogramming.
> 

Same as above.
Will update the description in v2.

> > +
> >  config FPGA_REGION
> >       tristate "FPGA Region"
> >       depends on FPGA_BRIDGE
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > 42ae8b58abce..94cfe60972db 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -31,6 +31,7 @@ obj-$(CONFIG_FPGA_BRIDGE)           += fpga-bridge.o
> >  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)    += altera-hps2fpga.o altera-
> fpga2sdram.o
> >  obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)   += altera-freeze-bridge.o
> >  obj-$(CONFIG_XILINX_PR_DECOUPLER)    += xilinx-pr-decoupler.o
> > +obj-$(CONFIG_ZYNQMP_AFI)             += zynqmp-afi.o
> >
> >  # High Level Interfaces
> >  obj-$(CONFIG_FPGA_REGION)            += fpga-region.o
> > diff --git a/drivers/fpga/zynqmp-afi.c b/drivers/fpga/zynqmp-afi.c new
> > file mode 100644 index 000000000000..bc975d304039
> > --- /dev/null
> > +++ b/drivers/fpga/zynqmp-afi.c
> > @@ -0,0 +1,211 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2022 Xilinx, Inc.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/firmware/xlnx-zynqmp.h> #include
> > +<linux/fpga/fpga-bridge.h> #include <linux/io.h> #include
> > +<linux/module.h> #include <linux/of.h> #include
> > +<linux/platform_device.h> #include <linux/reset.h> #include
> > +<linux/slab.h>
> > +
> > +/* Registers and special values for doing register-based operations */
> > +#define AFI_RDCHAN_CTRL_OFFSET       0x00
> > +#define AFI_WRCHAN_CTRL_OFFSET       0x14
> > +#define AFI_BUSWIDTH_MASK    BIT(0)
> > +
> > +/**
> > + * struct zynqmp_afi - AFI register description.
> > + * @dev: device that owns this.
> > + * @of_node: Device Tree overlay.
> > + * @resets: Pointer to the reset control for ps-pl resets.
> > + */
> > +struct zynqmp_afi {
> > +     struct device *dev;
> > +     struct device_node *of_node;
> > +     struct reset_control *resets;
> > +};
> > +
> > +/**
> > + * struct zynqmp_afi_configreg - AFI configuration registers info.
> > + * @reg:     Name of the AFI configuration register.
> > + * @id:              Register index value.
> > + */
> > +struct zynqmp_afi_configreg {
> > +     char *reg;
> > +     u32 id;
> > +};
> > +
> > +static struct zynqmp_afi_configreg afi_cfgreg[] = {
> > +     {.reg = "xlnx,afi-fm0-rd-bus-width",    .id = AFIFM0_RDCTRL},
> > +     {.reg = "xlnx,afi-fm1-rd-bus-width",    .id = AFIFM1_RDCTRL},
> > +     {.reg = "xlnx,afi-fm2-rd-bus-width",    .id = AFIFM2_RDCTRL},
> > +     {.reg = "xlnx,afi-fm3-rd-bus-width",    .id = AFIFM3_RDCTRL},
> > +     {.reg = "xlnx,afi-fm4-rd-bus-width",    .id = AFIFM4_RDCTRL},
> > +     {.reg = "xlnx,afi-fm5-rd-bus-width",    .id = AFIFM5_RDCTRL},
> > +     {.reg = "xlnx,afi-fm6-rd-bus-width",    .id = AFIFM6_RDCTRL},
> > +     {.reg = "xlnx,afi-fm0-wr-bus-width",    .id = AFIFM0_WRCTRL},
> > +     {.reg = "xlnx,afi-fm1-wr-bus-width",    .id = AFIFM1_WRCTRL},
> > +     {.reg = "xlnx,afi-fm2-wr-bus-width",    .id = AFIFM2_WRCTRL},
> > +     {.reg = "xlnx,afi-fm3-wr-bus-width",    .id = AFIFM3_WRCTRL},
> > +     {.reg = "xlnx,afi-fm4-wr-bus-width",    .id = AFIFM4_WRCTRL},
> > +     {.reg = "xlnx,afi-fm5-wr-bus-width",    .id = AFIFM5_WRCTRL},
> > +     {.reg = "xlnx,afi-fm6-wr-bus-width",    .id = AFIFM6_WRCTRL},
> > +     {.reg = "xlnx,afi-fs-ss0-bus-width",    .id = AFIFS},
> > +     {.reg = "xlnx,afi-fs-ss2-bus-width",    .id = AFIFS_SS2},
> > +     {}
> > +};
> > +
> > +static int zynqmp_afi_config(struct zynqmp_afi *afi_data) {
> > +     struct zynqmp_afi_configreg *cfgptr = afi_cfgreg;
> > +     struct device_node *np = afi_data->of_node;
> > +     u32 afi_ss0_val, afi_ss1_val, bus_width;
> > +     int ret;
> > +
> > +     while (cfgptr->reg) {
> > +             ret = of_property_read_u32(np, cfgptr->reg, &bus_width);
> > +             if (!ret) {
> > +                     if (cfgptr->id == AFIFS_SS2) {
> > +                             if (bus_width == 32)
> > +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> > +                                                         AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL);
> > +                             else if (bus_width == 64)
> > +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> > +
> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL);
> > +                             else if (bus_width == 128)
> > +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> > +
> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL);
> > +                             else
> > +                                     return -EINVAL;
> > +                     } else if (cfgptr->id == AFIFS) {
> > +                             if (bus_width == 32)
> > +                                     afi_ss0_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> > +                             else if (bus_width == 64)
> > +                                     afi_ss0_val =
> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL;
> > +                             else if (bus_width == 128)
> > +                                     afi_ss0_val =
> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL;
> > +                             else
> > +                                     return -EINVAL;
> > +
> > +                             ret = of_property_read_u32(np, "xlnx,afi-fs-ss1-bus-
> width",
> > +                                                        &bus_width);
> > +                             if (!ret) {
> > +                                     if (bus_width == 32)
> > +                                             afi_ss1_val =
> AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> > +                                     else if (bus_width == 64)
> > +                                             afi_ss1_val =
> AFIFS_SS1_BUS_WIDTH_64_CONFIG_VAL;
> > +                                     else if (bus_width == 128)
> > +                                             afi_ss1_val =
> AFIFS_SS1_BUS_WIDTH_128_CONFIG_VAL;
> > +                                     else
> > +                                             return -EINVAL;
> > +
> > +                                     ret = zynqmp_pm_afi(AFIFS, afi_ss1_val |
> afi_ss0_val);
> > +                             }
> > +                     } else {
> > +                             if (bus_width == 32)
> > +                                     ret = zynqmp_pm_afi(cfgptr->id,
> > +                                                         AFIFM_BUS_WIDTH_32_CONFIG_VAL);
> > +                             else if (bus_width == 64)
> > +                                     ret = zynqmp_pm_afi(cfgptr->id,
> > +                                                         AFIFM_BUS_WIDTH_64_CONFIG_VAL);
> > +                             else if (bus_width == 128)
> > +                                     ret = zynqmp_pm_afi(cfgptr->id,
> > +                                                         AFIFM_BUS_WIDTH_128_CONFIG_VAL);
> > +                             else
> > +                                     return -EINVAL;
> > +                     }
> > +             }
> > +             cfgptr++;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int zynqmp_afi_enable_set(struct fpga_bridge *bridge, bool
> > +enable) {
> > +     struct device_node *overlay = bridge->info->overlay;
> > +     struct zynqmp_afi *priv = bridge->priv;
> > +     int ret = 0;
> > +
> > +     if (enable) {
> > +             reset_control_reset(priv->resets);
> > +             return 0;
> > +     }
> > +
> > +     of_node_get(overlay);
> > +     priv->of_node =
> > +     of_find_node_with_property(overlay,
> > +                                "xlnx,afi-fm0-rd-bus-width");
> 
> I'm a little confused here. The fpga_image_info.overlay is for fpga-region, but
> from your binding doc this property is for this afi device. You want to add
> another overlay targeting afi dev node during reprograming?
> 

Yes, it uses Overlay's(overlay targeting afi dev node). As I said above for design-to-design AFI configs
relevant to the full region may change and these configs must be set prior to program the full region
once this overlay integrated to the live tree the existing afi config values will be replace with the new one's

> > +     if (priv->of_node)
> > +             ret = zynqmp_afi_config(priv);
> > +     of_node_put(priv->of_node);
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct fpga_bridge_ops zynqmp_afi_br_ops = {
> > +     .enable_set = zynqmp_afi_enable_set, };
> > +
> > +static const struct of_device_id zynqmp_afi_of_match[] = {
> > +     { .compatible =  "xlnx,zynqmp-afi-fpga" },
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(of, zynqmp_afi_of_match);
> > +
> > +static int zynqmp_afi_probe(struct platform_device *pdev) {
> > +     struct device *dev = &pdev->dev;
> > +     struct zynqmp_afi *priv;
> > +     struct fpga_bridge *br;
> > +
> > +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->dev = dev;
> > +
> > +     priv->resets =
> devm_reset_control_array_get_optional_exclusive(&pdev->dev);
> > +     if (IS_ERR(priv->resets))
> > +             return PTR_ERR(priv->resets);
> > +
> > +     br = fpga_bridge_register(dev, "Xilinx ZynqMP AFI",
> > +                               &zynqmp_afi_br_ops, priv);
> > +     if (IS_ERR(br)) {
> > +             dev_err(dev, "unable to register Xilinx ZynqMP AFI");
> 
> Need a "\n" at the end?
> 

Will fix.

Regards,
Navakishore.

Marco Pagani Sept. 1, 2022, 9:53 a.m. UTC | #4

Hi Nava,

On 2022-08-30 11:19, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> 	Please find my response inline.
> 
>> -----Original Message-----
>> From: Xu Yilun <[e-mail]>
>> Sent: Saturday, August 27, 2022 5:56 PM
>> To: Manne, Nava kishore <[e-mail]>
>> Cc: git (AMD-Xilinx) <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH 4/4] fpga: zynqmp: Add afi config driver
>>
>> CAUTION: This message has originated from an External Source. Please use
>> proper judgment and caution when opening attachments, clicking links, or
>> responding to this email.
>>
>>
>> On 2022-08-24 at 09:25:42 +0530, Nava kishore Manne wrote:
>>> Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for
>>> the configuration of the PS-PL interface on Zynq US+ MPSoC platform.
>>
>> Please help illustrate how to use the device for FPGA reprogramming, why it
>> should be implemented as an FPGA bridge.
>>
>> From the code I actually didn't see any operation that gates the fpga-region
>> from other part of the machine.
>>
> 
> The Zynq UltraScale MPSoC family consists of a system-on-chip (SoC) style integrated processing system (PS)
> and a Programmable Logic (PL) unit, providing an extensible and flexible SoC solution on a single die. 
> Xilinx Zynq US+ MPSoC connect the PS to the programmable logic (PL) through the AXI port. 
> This AXI port helps to establish the data path between the PS and PL (Here AXI Interface act as a Gating between PS and PL)
> and this AXI port configuration vary from design to design.  In-order to establish the proper communication path between
> PS and PL (Full region),  the AXI port data path should be configured with proper values priories to load the full region.
> 
> Will update the description in v2.

Are you implementing your AFI driver as a Bridge because you want to
change the configuration of the AXI HP interfaces exported by the PS
every time you reconfigure the full fabric?

I understand the need to change the configuration of the PS-PL
interface at each full reconfiguration to ensure that the parameters
of the AXI ports exported by the PS (like the widths of data read
and write channels) matches the configuration of the master AXI ports
exported by the PL design. However, it seems to me that performing this
configuration doesn't fit the role of the FPGA Bridge, whose sole
responsibility should be gating the Region's interface during
reconfiguration.

I think this need could be addressed by adding a new component to the
FPGA subsystem called "FPGA Adapter". The Adapter will be an optional
component associated with a Region like a Bridge. It will be responsible
for changing the configuration of its Region's static interface
(or PS/HPS/DRAM interface for the full FPGA) to match the interface
exported by the specific reconfigurable module (or full static design).
Moreover, besides bus-level properties like address/data width and
protocol conversion, the Adapter could also be used to configure
system-level properties. For instance, the designer may use it to change
the memory access policy for the specific reconfigurable module if it
uses bus mastering.

> 
>>>
>>> Signed-off-by: Nava kishore Manne <[e-mail]>
>>> ---
>>>  MAINTAINERS               |   6 ++
>>>  drivers/fpga/Kconfig      |  13 +++
>>>  drivers/fpga/Makefile     |   1 +
>>>  drivers/fpga/zynqmp-afi.c | 211
>>> ++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 231 insertions(+)
>>>  create mode 100644 drivers/fpga/zynqmp-afi.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS index
>>> 20ffac651214..957e753e6406 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -8032,6 +8032,12 @@ F:     Documentation/fpga/
>>>  F:   drivers/fpga/
>>>  F:   include/linux/fpga/
>>>
>>> +FPGA ZYNQMP PS-PL BRIDGE DRIVER
>>> +M:   Nava kishore Manne <[e-mail]>
>>> +S:   Supported
>>> +F:   Documentation/devicetree/bindings/fpga/xlnx,zynqmp-afi-fpga.yaml
>>> +F:   drivers/fpga/zynqmp-afi.c
>>> +
>>>  INTEL MAX10 BMC SECURE UPDATES
>>>  M:   Russ Weight <[e-mail]>
>>>  L:   [e-mail]
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
>>> 6c416955da53..c08794d30fb5 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -130,6 +130,19 @@ config XILINX_PR_DECOUPLER
>>>         reconfiguration, preventing the system deadlock that can
>>>         occur if AXI transactions are interrupted by DFX.
>>>
>>> +config ZYNQMP_AFI
>>> +     tristate "Xilinx ZYNQMP AFI support"
>>> +     depends on FPGA_BRIDGE
>>> +     help
>>> +       Say Y to enable drivers to handle the PS-PL clocks configurations
>>> +       and PS-PL Bus-width. Xilinx Zynq US+ MPSoC connect the PS to the
>>> +       programmable logic (PL) through the AXI port. This AXI port helps
>>> +       to establish the data path between the PS and PL.
>>> +       In-order to establish the proper communication path between PS and
>> PL,
>>> +       the AXI port data path should be configured with the proper Bus-
>> width
>>> +       values and it will also handles the PS-PL reset signals to reset the
>>> +       PL domain.
>>
>> Same concern, please describe its relationship to FPGA reprogramming.
>>
> 
> Same as above.
> Will update the description in v2.
> 
>>> +
>>>  config FPGA_REGION
>>>       tristate "FPGA Region"
>>>       depends on FPGA_BRIDGE
>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
>>> 42ae8b58abce..94cfe60972db 100644
>>> --- a/drivers/fpga/Makefile
>>> +++ b/drivers/fpga/Makefile
>>> @@ -31,6 +31,7 @@ obj-$(CONFIG_FPGA_BRIDGE)           += fpga-bridge.o
>>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)    += altera-hps2fpga.o altera-
>> fpga2sdram.o
>>>  obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)   += altera-freeze-bridge.o
>>>  obj-$(CONFIG_XILINX_PR_DECOUPLER)    += xilinx-pr-decoupler.o
>>> +obj-$(CONFIG_ZYNQMP_AFI)             += zynqmp-afi.o
>>>
>>>  # High Level Interfaces
>>>  obj-$(CONFIG_FPGA_REGION)            += fpga-region.o
>>> diff --git a/drivers/fpga/zynqmp-afi.c b/drivers/fpga/zynqmp-afi.c new
>>> file mode 100644 index 000000000000..bc975d304039
>>> --- /dev/null
>>> +++ b/drivers/fpga/zynqmp-afi.c
>>> @@ -0,0 +1,211 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2022 Xilinx, Inc.
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/firmware/xlnx-zynqmp.h> #include
>>> +<linux/fpga/fpga-bridge.h> #include <linux/io.h> #include
>>> +<linux/module.h> #include <linux/of.h> #include
>>> +<linux/platform_device.h> #include <linux/reset.h> #include
>>> +<linux/slab.h>
>>> +
>>> +/* Registers and special values for doing register-based operations */
>>> +#define AFI_RDCHAN_CTRL_OFFSET       0x00
>>> +#define AFI_WRCHAN_CTRL_OFFSET       0x14
>>> +#define AFI_BUSWIDTH_MASK    BIT(0)
>>> +
>>> +/**
>>> + * struct zynqmp_afi - AFI register description.
>>> + * @dev: device that owns this.
>>> + * @of_node: Device Tree overlay.
>>> + * @resets: Pointer to the reset control for ps-pl resets.
>>> + */
>>> +struct zynqmp_afi {
>>> +     struct device *dev;
>>> +     struct device_node *of_node;
>>> +     struct reset_control *resets;
>>> +};
>>> +
>>> +/**
>>> + * struct zynqmp_afi_configreg - AFI configuration registers info.
>>> + * @reg:     Name of the AFI configuration register.
>>> + * @id:              Register index value.
>>> + */
>>> +struct zynqmp_afi_configreg {
>>> +     char *reg;
>>> +     u32 id;
>>> +};
>>> +
>>> +static struct zynqmp_afi_configreg afi_cfgreg[] = {
>>> +     {.reg = "xlnx,afi-fm0-rd-bus-width",    .id = AFIFM0_RDCTRL},
>>> +     {.reg = "xlnx,afi-fm1-rd-bus-width",    .id = AFIFM1_RDCTRL},
>>> +     {.reg = "xlnx,afi-fm2-rd-bus-width",    .id = AFIFM2_RDCTRL},
>>> +     {.reg = "xlnx,afi-fm3-rd-bus-width",    .id = AFIFM3_RDCTRL},
>>> +     {.reg = "xlnx,afi-fm4-rd-bus-width",    .id = AFIFM4_RDCTRL},
>>> +     {.reg = "xlnx,afi-fm5-rd-bus-width",    .id = AFIFM5_RDCTRL},
>>> +     {.reg = "xlnx,afi-fm6-rd-bus-width",    .id = AFIFM6_RDCTRL},
>>> +     {.reg = "xlnx,afi-fm0-wr-bus-width",    .id = AFIFM0_WRCTRL},
>>> +     {.reg = "xlnx,afi-fm1-wr-bus-width",    .id = AFIFM1_WRCTRL},
>>> +     {.reg = "xlnx,afi-fm2-wr-bus-width",    .id = AFIFM2_WRCTRL},
>>> +     {.reg = "xlnx,afi-fm3-wr-bus-width",    .id = AFIFM3_WRCTRL},
>>> +     {.reg = "xlnx,afi-fm4-wr-bus-width",    .id = AFIFM4_WRCTRL},
>>> +     {.reg = "xlnx,afi-fm5-wr-bus-width",    .id = AFIFM5_WRCTRL},
>>> +     {.reg = "xlnx,afi-fm6-wr-bus-width",    .id = AFIFM6_WRCTRL},
>>> +     {.reg = "xlnx,afi-fs-ss0-bus-width",    .id = AFIFS},
>>> +     {.reg = "xlnx,afi-fs-ss2-bus-width",    .id = AFIFS_SS2},
>>> +     {}
>>> +};
>>> +
>>> +static int zynqmp_afi_config(struct zynqmp_afi *afi_data) {
>>> +     struct zynqmp_afi_configreg *cfgptr = afi_cfgreg;
>>> +     struct device_node *np = afi_data->of_node;
>>> +     u32 afi_ss0_val, afi_ss1_val, bus_width;
>>> +     int ret;
>>> +
>>> +     while (cfgptr->reg) {
>>> +             ret = of_property_read_u32(np, cfgptr->reg, &bus_width);
>>> +             if (!ret) {
>>> +                     if (cfgptr->id == AFIFS_SS2) {
>>> +                             if (bus_width == 32)
>>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
>>> +                                                         AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL);
>>> +                             else if (bus_width == 64)
>>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
>>> +
>> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL);
>>> +                             else if (bus_width == 128)
>>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
>>> +
>> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL);
>>> +                             else
>>> +                                     return -EINVAL;
>>> +                     } else if (cfgptr->id == AFIFS) {
>>> +                             if (bus_width == 32)
>>> +                                     afi_ss0_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
>>> +                             else if (bus_width == 64)
>>> +                                     afi_ss0_val =
>> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL;
>>> +                             else if (bus_width == 128)
>>> +                                     afi_ss0_val =
>> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL;
>>> +                             else
>>> +                                     return -EINVAL;
>>> +
>>> +                             ret = of_property_read_u32(np, "xlnx,afi-fs-ss1-bus-
>> width",
>>> +                                                        &bus_width);
>>> +                             if (!ret) {
>>> +                                     if (bus_width == 32)
>>> +                                             afi_ss1_val =
>> AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
>>> +                                     else if (bus_width == 64)
>>> +                                             afi_ss1_val =
>> AFIFS_SS1_BUS_WIDTH_64_CONFIG_VAL;
>>> +                                     else if (bus_width == 128)
>>> +                                             afi_ss1_val =
>> AFIFS_SS1_BUS_WIDTH_128_CONFIG_VAL;
>>> +                                     else
>>> +                                             return -EINVAL;
>>> +
>>> +                                     ret = zynqmp_pm_afi(AFIFS, afi_ss1_val |
>> afi_ss0_val);
>>> +                             }
>>> +                     } else {
>>> +                             if (bus_width == 32)
>>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
>>> +                                                         AFIFM_BUS_WIDTH_32_CONFIG_VAL);
>>> +                             else if (bus_width == 64)
>>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
>>> +                                                         AFIFM_BUS_WIDTH_64_CONFIG_VAL);
>>> +                             else if (bus_width == 128)
>>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
>>> +                                                         AFIFM_BUS_WIDTH_128_CONFIG_VAL);
>>> +                             else
>>> +                                     return -EINVAL;
>>> +                     }
>>> +             }
>>> +             cfgptr++;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int zynqmp_afi_enable_set(struct fpga_bridge *bridge, bool
>>> +enable) {
>>> +     struct device_node *overlay = bridge->info->overlay;
>>> +     struct zynqmp_afi *priv = bridge->priv;
>>> +     int ret = 0;
>>> +
>>> +     if (enable) {
>>> +             reset_control_reset(priv->resets);
>>> +             return 0;
>>> +     }
>>> +
>>> +     of_node_get(overlay);
>>> +     priv->of_node =
>>> +     of_find_node_with_property(overlay,
>>> +                                "xlnx,afi-fm0-rd-bus-width");
>>
>> I'm a little confused here. The fpga_image_info.overlay is for fpga-region, but
>> from your binding doc this property is for this afi device. You want to add
>> another overlay targeting afi dev node during reprograming?
>>
> 
> Yes, it uses Overlay's(overlay targeting afi dev node). As I said above for design-to-design AFI configs
> relevant to the full region may change and these configs must be set prior to program the full region
> once this overlay integrated to the live tree the existing afi config values will be replace with the new one's
> 
>>> +     if (priv->of_node)
>>> +             ret = zynqmp_afi_config(priv);
>>> +     of_node_put(priv->of_node);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static const struct fpga_bridge_ops zynqmp_afi_br_ops = {
>>> +     .enable_set = zynqmp_afi_enable_set, };
>>> +
>>> +static const struct of_device_id zynqmp_afi_of_match[] = {
>>> +     { .compatible =  "xlnx,zynqmp-afi-fpga" },
>>> +     { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, zynqmp_afi_of_match);
>>> +
>>> +static int zynqmp_afi_probe(struct platform_device *pdev) {
>>> +     struct device *dev = &pdev->dev;
>>> +     struct zynqmp_afi *priv;
>>> +     struct fpga_bridge *br;
>>> +
>>> +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>> +     if (!priv)
>>> +             return -ENOMEM;
>>> +
>>> +     priv->dev = dev;
>>> +
>>> +     priv->resets =
>> devm_reset_control_array_get_optional_exclusive(&pdev->dev);
>>> +     if (IS_ERR(priv->resets))
>>> +             return PTR_ERR(priv->resets);
>>> +
>>> +     br = fpga_bridge_register(dev, "Xilinx ZynqMP AFI",
>>> +                               &zynqmp_afi_br_ops, priv);
>>> +     if (IS_ERR(br)) {
>>> +             dev_err(dev, "unable to register Xilinx ZynqMP AFI");
>>
>> Need a "\n" at the end?
>>
> 
> Will fix.
> 
> Regards,
> Navakishore.
> 

Thanks,
Marco

Xu Yilun Sept. 3, 2022, 5:33 p.m. UTC | #5

On 2022-08-30 at 09:19:52 +0000, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> 	Please find my response inline.
> 
> > -----Original Message-----
> > From: Xu Yilun <[e-mail]>
> > Sent: Saturday, August 27, 2022 5:56 PM
> > To: Manne, Nava kishore <[e-mail]>
> > Cc: git (AMD-Xilinx) <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH 4/4] fpga: zynqmp: Add afi config driver
> > 
> > CAUTION: This message has originated from an External Source. Please use
> > proper judgment and caution when opening attachments, clicking links, or
> > responding to this email.
> > 
> > 
> > On 2022-08-24 at 09:25:42 +0530, Nava kishore Manne wrote:
> > > Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for
> > > the configuration of the PS-PL interface on Zynq US+ MPSoC platform.
> > 
> > Please help illustrate how to use the device for FPGA reprogramming, why it
> > should be implemented as an FPGA bridge.
> > 
> > From the code I actually didn't see any operation that gates the fpga-region
> > from other part of the machine.
> > 
> 
> The Zynq UltraScale MPSoC family consists of a system-on-chip (SoC) style integrated processing system (PS)
> and a Programmable Logic (PL) unit, providing an extensible and flexible SoC solution on a single die. 
> Xilinx Zynq US+ MPSoC connect the PS to the programmable logic (PL) through the AXI port. 
> This AXI port helps to establish the data path between the PS and PL (Here AXI Interface act as a Gating between PS and PL)
> and this AXI port configuration vary from design to design.  In-order to establish the proper communication path between
> PS and PL (Full region),  the AXI port data path should be configured with proper values priories to load the full region.
> 
> Will update the description in v2.
> 
> > >
> > > Signed-off-by: Nava kishore Manne <[e-mail]>
> > > ---
> > >  MAINTAINERS               |   6 ++
> > >  drivers/fpga/Kconfig      |  13 +++
> > >  drivers/fpga/Makefile     |   1 +
> > >  drivers/fpga/zynqmp-afi.c | 211
> > > ++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 231 insertions(+)
> > >  create mode 100644 drivers/fpga/zynqmp-afi.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > 20ffac651214..957e753e6406 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -8032,6 +8032,12 @@ F:     Documentation/fpga/
> > >  F:   drivers/fpga/
> > >  F:   include/linux/fpga/
> > >
> > > +FPGA ZYNQMP PS-PL BRIDGE DRIVER
> > > +M:   Nava kishore Manne <[e-mail]>
> > > +S:   Supported
> > > +F:   Documentation/devicetree/bindings/fpga/xlnx,zynqmp-afi-fpga.yaml
> > > +F:   drivers/fpga/zynqmp-afi.c
> > > +
> > >  INTEL MAX10 BMC SECURE UPDATES
> > >  M:   Russ Weight <[e-mail]>
> > >  L:   [e-mail]
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > > 6c416955da53..c08794d30fb5 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -130,6 +130,19 @@ config XILINX_PR_DECOUPLER
> > >         reconfiguration, preventing the system deadlock that can
> > >         occur if AXI transactions are interrupted by DFX.
> > >
> > > +config ZYNQMP_AFI
> > > +     tristate "Xilinx ZYNQMP AFI support"
> > > +     depends on FPGA_BRIDGE
> > > +     help
> > > +       Say Y to enable drivers to handle the PS-PL clocks configurations
> > > +       and PS-PL Bus-width. Xilinx Zynq US+ MPSoC connect the PS to the
> > > +       programmable logic (PL) through the AXI port. This AXI port helps
> > > +       to establish the data path between the PS and PL.
> > > +       In-order to establish the proper communication path between PS and
> > PL,
> > > +       the AXI port data path should be configured with the proper Bus-
> > width
> > > +       values and it will also handles the PS-PL reset signals to reset the
> > > +       PL domain.
> > 
> > Same concern, please describe its relationship to FPGA reprogramming.
> > 
> 
> Same as above.
> Will update the description in v2.
> 
> > > +
> > >  config FPGA_REGION
> > >       tristate "FPGA Region"
> > >       depends on FPGA_BRIDGE
> > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > > 42ae8b58abce..94cfe60972db 100644
> > > --- a/drivers/fpga/Makefile
> > > +++ b/drivers/fpga/Makefile
> > > @@ -31,6 +31,7 @@ obj-$(CONFIG_FPGA_BRIDGE)           += fpga-bridge.o
> > >  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)    += altera-hps2fpga.o altera-
> > fpga2sdram.o
> > >  obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)   += altera-freeze-bridge.o
> > >  obj-$(CONFIG_XILINX_PR_DECOUPLER)    += xilinx-pr-decoupler.o
> > > +obj-$(CONFIG_ZYNQMP_AFI)             += zynqmp-afi.o
> > >
> > >  # High Level Interfaces
> > >  obj-$(CONFIG_FPGA_REGION)            += fpga-region.o
> > > diff --git a/drivers/fpga/zynqmp-afi.c b/drivers/fpga/zynqmp-afi.c new
> > > file mode 100644 index 000000000000..bc975d304039
> > > --- /dev/null
> > > +++ b/drivers/fpga/zynqmp-afi.c
> > > @@ -0,0 +1,211 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2022 Xilinx, Inc.
> > > + */
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/firmware/xlnx-zynqmp.h> #include
> > > +<linux/fpga/fpga-bridge.h> #include <linux/io.h> #include
> > > +<linux/module.h> #include <linux/of.h> #include
> > > +<linux/platform_device.h> #include <linux/reset.h> #include
> > > +<linux/slab.h>
> > > +
> > > +/* Registers and special values for doing register-based operations */
> > > +#define AFI_RDCHAN_CTRL_OFFSET       0x00
> > > +#define AFI_WRCHAN_CTRL_OFFSET       0x14
> > > +#define AFI_BUSWIDTH_MASK    BIT(0)
> > > +
> > > +/**
> > > + * struct zynqmp_afi - AFI register description.
> > > + * @dev: device that owns this.
> > > + * @of_node: Device Tree overlay.
> > > + * @resets: Pointer to the reset control for ps-pl resets.
> > > + */
> > > +struct zynqmp_afi {
> > > +     struct device *dev;
> > > +     struct device_node *of_node;
> > > +     struct reset_control *resets;
> > > +};
> > > +
> > > +/**
> > > + * struct zynqmp_afi_configreg - AFI configuration registers info.
> > > + * @reg:     Name of the AFI configuration register.
> > > + * @id:              Register index value.
> > > + */
> > > +struct zynqmp_afi_configreg {
> > > +     char *reg;
> > > +     u32 id;
> > > +};
> > > +
> > > +static struct zynqmp_afi_configreg afi_cfgreg[] = {
> > > +     {.reg = "xlnx,afi-fm0-rd-bus-width",    .id = AFIFM0_RDCTRL},
> > > +     {.reg = "xlnx,afi-fm1-rd-bus-width",    .id = AFIFM1_RDCTRL},
> > > +     {.reg = "xlnx,afi-fm2-rd-bus-width",    .id = AFIFM2_RDCTRL},
> > > +     {.reg = "xlnx,afi-fm3-rd-bus-width",    .id = AFIFM3_RDCTRL},
> > > +     {.reg = "xlnx,afi-fm4-rd-bus-width",    .id = AFIFM4_RDCTRL},
> > > +     {.reg = "xlnx,afi-fm5-rd-bus-width",    .id = AFIFM5_RDCTRL},
> > > +     {.reg = "xlnx,afi-fm6-rd-bus-width",    .id = AFIFM6_RDCTRL},
> > > +     {.reg = "xlnx,afi-fm0-wr-bus-width",    .id = AFIFM0_WRCTRL},
> > > +     {.reg = "xlnx,afi-fm1-wr-bus-width",    .id = AFIFM1_WRCTRL},
> > > +     {.reg = "xlnx,afi-fm2-wr-bus-width",    .id = AFIFM2_WRCTRL},
> > > +     {.reg = "xlnx,afi-fm3-wr-bus-width",    .id = AFIFM3_WRCTRL},
> > > +     {.reg = "xlnx,afi-fm4-wr-bus-width",    .id = AFIFM4_WRCTRL},
> > > +     {.reg = "xlnx,afi-fm5-wr-bus-width",    .id = AFIFM5_WRCTRL},
> > > +     {.reg = "xlnx,afi-fm6-wr-bus-width",    .id = AFIFM6_WRCTRL},
> > > +     {.reg = "xlnx,afi-fs-ss0-bus-width",    .id = AFIFS},
> > > +     {.reg = "xlnx,afi-fs-ss2-bus-width",    .id = AFIFS_SS2},
> > > +     {}
> > > +};
> > > +
> > > +static int zynqmp_afi_config(struct zynqmp_afi *afi_data) {
> > > +     struct zynqmp_afi_configreg *cfgptr = afi_cfgreg;
> > > +     struct device_node *np = afi_data->of_node;
> > > +     u32 afi_ss0_val, afi_ss1_val, bus_width;
> > > +     int ret;
> > > +
> > > +     while (cfgptr->reg) {
> > > +             ret = of_property_read_u32(np, cfgptr->reg, &bus_width);
> > > +             if (!ret) {
> > > +                     if (cfgptr->id == AFIFS_SS2) {
> > > +                             if (bus_width == 32)
> > > +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> > > +                                                         AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL);
> > > +                             else if (bus_width == 64)
> > > +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> > > +
> > AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL);
> > > +                             else if (bus_width == 128)
> > > +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> > > +
> > AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL);
> > > +                             else
> > > +                                     return -EINVAL;
> > > +                     } else if (cfgptr->id == AFIFS) {
> > > +                             if (bus_width == 32)
> > > +                                     afi_ss0_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> > > +                             else if (bus_width == 64)
> > > +                                     afi_ss0_val =
> > AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL;
> > > +                             else if (bus_width == 128)
> > > +                                     afi_ss0_val =
> > AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL;
> > > +                             else
> > > +                                     return -EINVAL;
> > > +
> > > +                             ret = of_property_read_u32(np, "xlnx,afi-fs-ss1-bus-
> > width",
> > > +                                                        &bus_width);
> > > +                             if (!ret) {
> > > +                                     if (bus_width == 32)
> > > +                                             afi_ss1_val =
> > AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> > > +                                     else if (bus_width == 64)
> > > +                                             afi_ss1_val =
> > AFIFS_SS1_BUS_WIDTH_64_CONFIG_VAL;
> > > +                                     else if (bus_width == 128)
> > > +                                             afi_ss1_val =
> > AFIFS_SS1_BUS_WIDTH_128_CONFIG_VAL;
> > > +                                     else
> > > +                                             return -EINVAL;
> > > +
> > > +                                     ret = zynqmp_pm_afi(AFIFS, afi_ss1_val |
> > afi_ss0_val);
> > > +                             }
> > > +                     } else {
> > > +                             if (bus_width == 32)
> > > +                                     ret = zynqmp_pm_afi(cfgptr->id,
> > > +                                                         AFIFM_BUS_WIDTH_32_CONFIG_VAL);
> > > +                             else if (bus_width == 64)
> > > +                                     ret = zynqmp_pm_afi(cfgptr->id,
> > > +                                                         AFIFM_BUS_WIDTH_64_CONFIG_VAL);
> > > +                             else if (bus_width == 128)
> > > +                                     ret = zynqmp_pm_afi(cfgptr->id,
> > > +                                                         AFIFM_BUS_WIDTH_128_CONFIG_VAL);
> > > +                             else
> > > +                                     return -EINVAL;
> > > +                     }
> > > +             }
> > > +             cfgptr++;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int zynqmp_afi_enable_set(struct fpga_bridge *bridge, bool
> > > +enable) {
> > > +     struct device_node *overlay = bridge->info->overlay;
> > > +     struct zynqmp_afi *priv = bridge->priv;
> > > +     int ret = 0;
> > > +
> > > +     if (enable) {
> > > +             reset_control_reset(priv->resets);
> > > +             return 0;
> > > +     }
> > > +
> > > +     of_node_get(overlay);
> > > +     priv->of_node =
> > > +     of_find_node_with_property(overlay,
> > > +                                "xlnx,afi-fm0-rd-bus-width");
> > 
> > I'm a little confused here. The fpga_image_info.overlay is for fpga-region, but
> > from your binding doc this property is for this afi device. You want to add
> > another overlay targeting afi dev node during reprograming?
> > 
> 
> Yes, it uses Overlay's(overlay targeting afi dev node). As I said above for design-to-design AFI configs

I have concern on it. We should limit the overlay usage only on
fpga_region, this is because we are changing the hardware in this
region during reprograming. Adding overlays for other nodes could be
easily abused, any changes could be made even not related to the fpga
region. So if any cross device configurations should be made during
reprograming, it should be initiated by fpga region driver through
normal kernel APIs.

Thanks,
Yilun

> relevant to the full region may change and these configs must be set prior to program the full region
> once this overlay integrated to the live tree the existing afi config values will be replace with the new one's
> 
> > > +     if (priv->of_node)
> > > +             ret = zynqmp_afi_config(priv);
> > > +     of_node_put(priv->of_node);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static const struct fpga_bridge_ops zynqmp_afi_br_ops = {
> > > +     .enable_set = zynqmp_afi_enable_set, };
> > > +
> > > +static const struct of_device_id zynqmp_afi_of_match[] = {
> > > +     { .compatible =  "xlnx,zynqmp-afi-fpga" },
> > > +     { },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, zynqmp_afi_of_match);
> > > +
> > > +static int zynqmp_afi_probe(struct platform_device *pdev) {
> > > +     struct device *dev = &pdev->dev;
> > > +     struct zynqmp_afi *priv;
> > > +     struct fpga_bridge *br;
> > > +
> > > +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > > +     if (!priv)
> > > +             return -ENOMEM;
> > > +
> > > +     priv->dev = dev;
> > > +
> > > +     priv->resets =
> > devm_reset_control_array_get_optional_exclusive(&pdev->dev);
> > > +     if (IS_ERR(priv->resets))
> > > +             return PTR_ERR(priv->resets);
> > > +
> > > +     br = fpga_bridge_register(dev, "Xilinx ZynqMP AFI",
> > > +                               &zynqmp_afi_br_ops, priv);
> > > +     if (IS_ERR(br)) {
> > > +             dev_err(dev, "unable to register Xilinx ZynqMP AFI");
> > 
> > Need a "\n" at the end?
> > 
> 
> Will fix.
> 
> Regards,
> Navakishore.
>

Xu Yilun Sept. 3, 2022, 5:44 p.m. UTC | #6

On 2022-09-01 at 11:53:29 +0200, Marco Pagani wrote:
> Hi Nava,
> 
> On 2022-08-30 11:19, Manne, Nava kishore wrote:
> > Hi Yilun,
> > 
> > 	Please find my response inline.
> > 
> >> -----Original Message-----
> >> From: Xu Yilun <[e-mail]>
> >> Sent: Saturday, August 27, 2022 5:56 PM
> >> To: Manne, Nava kishore <[e-mail]>
> >> Cc: git (AMD-Xilinx) <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH 4/4] fpga: zynqmp: Add afi config driver
> >>
> >> CAUTION: This message has originated from an External Source. Please use
> >> proper judgment and caution when opening attachments, clicking links, or
> >> responding to this email.
> >>
> >>
> >> On 2022-08-24 at 09:25:42 +0530, Nava kishore Manne wrote:
> >>> Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for
> >>> the configuration of the PS-PL interface on Zynq US+ MPSoC platform.
> >>
> >> Please help illustrate how to use the device for FPGA reprogramming, why it
> >> should be implemented as an FPGA bridge.
> >>
> >> From the code I actually didn't see any operation that gates the fpga-region
> >> from other part of the machine.
> >>
> > 
> > The Zynq UltraScale MPSoC family consists of a system-on-chip (SoC) style integrated processing system (PS)
> > and a Programmable Logic (PL) unit, providing an extensible and flexible SoC solution on a single die. 
> > Xilinx Zynq US+ MPSoC connect the PS to the programmable logic (PL) through the AXI port. 
> > This AXI port helps to establish the data path between the PS and PL (Here AXI Interface act as a Gating between PS and PL)
> > and this AXI port configuration vary from design to design.  In-order to establish the proper communication path between
> > PS and PL (Full region),  the AXI port data path should be configured with proper values priories to load the full region.
> > 
> > Will update the description in v2.
> 
> Are you implementing your AFI driver as a Bridge because you want to
> change the configuration of the AXI HP interfaces exported by the PS
> every time you reconfigure the full fabric?
> 
> I understand the need to change the configuration of the PS-PL
> interface at each full reconfiguration to ensure that the parameters
> of the AXI ports exported by the PS (like the widths of data read
> and write channels) matches the configuration of the master AXI ports
> exported by the PL design. However, it seems to me that performing this
> configuration doesn't fit the role of the FPGA Bridge, whose sole
> responsibility should be gating the Region's interface during
> reconfiguration.
> 
> I think this need could be addressed by adding a new component to the
> FPGA subsystem called "FPGA Adapter". The Adapter will be an optional
> component associated with a Region like a Bridge. It will be responsible
> for changing the configuration of its Region's static interface
> (or PS/HPS/DRAM interface for the full FPGA) to match the interface
> exported by the specific reconfigurable module (or full static design).

I saw the Documentation/driver_api/interconnect.rst, but didn't look deep
into it. For this AFI case, it sets the bus width, is it OK to use it?

Thanks,
Yilun

> Moreover, besides bus-level properties like address/data width and
> protocol conversion, the Adapter could also be used to configure
> system-level properties. For instance, the designer may use it to change
> the memory access policy for the specific reconfigurable module if it
> uses bus mastering.
> 
> > 
> >>>
> >>> Signed-off-by: Nava kishore Manne <[e-mail]>
> >>> ---
> >>>  MAINTAINERS               |   6 ++
> >>>  drivers/fpga/Kconfig      |  13 +++
> >>>  drivers/fpga/Makefile     |   1 +
> >>>  drivers/fpga/zynqmp-afi.c | 211
> >>> ++++++++++++++++++++++++++++++++++++++
> >>>  4 files changed, 231 insertions(+)
> >>>  create mode 100644 drivers/fpga/zynqmp-afi.c
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS index
> >>> 20ffac651214..957e753e6406 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -8032,6 +8032,12 @@ F:     Documentation/fpga/
> >>>  F:   drivers/fpga/
> >>>  F:   include/linux/fpga/
> >>>
> >>> +FPGA ZYNQMP PS-PL BRIDGE DRIVER
> >>> +M:   Nava kishore Manne <[e-mail]>
> >>> +S:   Supported
> >>> +F:   Documentation/devicetree/bindings/fpga/xlnx,zynqmp-afi-fpga.yaml
> >>> +F:   drivers/fpga/zynqmp-afi.c
> >>> +
> >>>  INTEL MAX10 BMC SECURE UPDATES
> >>>  M:   Russ Weight <[e-mail]>
> >>>  L:   [e-mail]
> >>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> >>> 6c416955da53..c08794d30fb5 100644
> >>> --- a/drivers/fpga/Kconfig
> >>> +++ b/drivers/fpga/Kconfig
> >>> @@ -130,6 +130,19 @@ config XILINX_PR_DECOUPLER
> >>>         reconfiguration, preventing the system deadlock that can
> >>>         occur if AXI transactions are interrupted by DFX.
> >>>
> >>> +config ZYNQMP_AFI
> >>> +     tristate "Xilinx ZYNQMP AFI support"
> >>> +     depends on FPGA_BRIDGE
> >>> +     help
> >>> +       Say Y to enable drivers to handle the PS-PL clocks configurations
> >>> +       and PS-PL Bus-width. Xilinx Zynq US+ MPSoC connect the PS to the
> >>> +       programmable logic (PL) through the AXI port. This AXI port helps
> >>> +       to establish the data path between the PS and PL.
> >>> +       In-order to establish the proper communication path between PS and
> >> PL,
> >>> +       the AXI port data path should be configured with the proper Bus-
> >> width
> >>> +       values and it will also handles the PS-PL reset signals to reset the
> >>> +       PL domain.
> >>
> >> Same concern, please describe its relationship to FPGA reprogramming.
> >>
> > 
> > Same as above.
> > Will update the description in v2.
> > 
> >>> +
> >>>  config FPGA_REGION
> >>>       tristate "FPGA Region"
> >>>       depends on FPGA_BRIDGE
> >>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> >>> 42ae8b58abce..94cfe60972db 100644
> >>> --- a/drivers/fpga/Makefile
> >>> +++ b/drivers/fpga/Makefile
> >>> @@ -31,6 +31,7 @@ obj-$(CONFIG_FPGA_BRIDGE)           += fpga-bridge.o
> >>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)    += altera-hps2fpga.o altera-
> >> fpga2sdram.o
> >>>  obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)   += altera-freeze-bridge.o
> >>>  obj-$(CONFIG_XILINX_PR_DECOUPLER)    += xilinx-pr-decoupler.o
> >>> +obj-$(CONFIG_ZYNQMP_AFI)             += zynqmp-afi.o
> >>>
> >>>  # High Level Interfaces
> >>>  obj-$(CONFIG_FPGA_REGION)            += fpga-region.o
> >>> diff --git a/drivers/fpga/zynqmp-afi.c b/drivers/fpga/zynqmp-afi.c new
> >>> file mode 100644 index 000000000000..bc975d304039
> >>> --- /dev/null
> >>> +++ b/drivers/fpga/zynqmp-afi.c
> >>> @@ -0,0 +1,211 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Copyright (C) 2022 Xilinx, Inc.
> >>> + */
> >>> +
> >>> +#include <linux/err.h>
> >>> +#include <linux/firmware/xlnx-zynqmp.h> #include
> >>> +<linux/fpga/fpga-bridge.h> #include <linux/io.h> #include
> >>> +<linux/module.h> #include <linux/of.h> #include
> >>> +<linux/platform_device.h> #include <linux/reset.h> #include
> >>> +<linux/slab.h>
> >>> +
> >>> +/* Registers and special values for doing register-based operations */
> >>> +#define AFI_RDCHAN_CTRL_OFFSET       0x00
> >>> +#define AFI_WRCHAN_CTRL_OFFSET       0x14
> >>> +#define AFI_BUSWIDTH_MASK    BIT(0)
> >>> +
> >>> +/**
> >>> + * struct zynqmp_afi - AFI register description.
> >>> + * @dev: device that owns this.
> >>> + * @of_node: Device Tree overlay.
> >>> + * @resets: Pointer to the reset control for ps-pl resets.
> >>> + */
> >>> +struct zynqmp_afi {
> >>> +     struct device *dev;
> >>> +     struct device_node *of_node;
> >>> +     struct reset_control *resets;
> >>> +};
> >>> +
> >>> +/**
> >>> + * struct zynqmp_afi_configreg - AFI configuration registers info.
> >>> + * @reg:     Name of the AFI configuration register.
> >>> + * @id:              Register index value.
> >>> + */
> >>> +struct zynqmp_afi_configreg {
> >>> +     char *reg;
> >>> +     u32 id;
> >>> +};
> >>> +
> >>> +static struct zynqmp_afi_configreg afi_cfgreg[] = {
> >>> +     {.reg = "xlnx,afi-fm0-rd-bus-width",    .id = AFIFM0_RDCTRL},
> >>> +     {.reg = "xlnx,afi-fm1-rd-bus-width",    .id = AFIFM1_RDCTRL},
> >>> +     {.reg = "xlnx,afi-fm2-rd-bus-width",    .id = AFIFM2_RDCTRL},
> >>> +     {.reg = "xlnx,afi-fm3-rd-bus-width",    .id = AFIFM3_RDCTRL},
> >>> +     {.reg = "xlnx,afi-fm4-rd-bus-width",    .id = AFIFM4_RDCTRL},
> >>> +     {.reg = "xlnx,afi-fm5-rd-bus-width",    .id = AFIFM5_RDCTRL},
> >>> +     {.reg = "xlnx,afi-fm6-rd-bus-width",    .id = AFIFM6_RDCTRL},
> >>> +     {.reg = "xlnx,afi-fm0-wr-bus-width",    .id = AFIFM0_WRCTRL},
> >>> +     {.reg = "xlnx,afi-fm1-wr-bus-width",    .id = AFIFM1_WRCTRL},
> >>> +     {.reg = "xlnx,afi-fm2-wr-bus-width",    .id = AFIFM2_WRCTRL},
> >>> +     {.reg = "xlnx,afi-fm3-wr-bus-width",    .id = AFIFM3_WRCTRL},
> >>> +     {.reg = "xlnx,afi-fm4-wr-bus-width",    .id = AFIFM4_WRCTRL},
> >>> +     {.reg = "xlnx,afi-fm5-wr-bus-width",    .id = AFIFM5_WRCTRL},
> >>> +     {.reg = "xlnx,afi-fm6-wr-bus-width",    .id = AFIFM6_WRCTRL},
> >>> +     {.reg = "xlnx,afi-fs-ss0-bus-width",    .id = AFIFS},
> >>> +     {.reg = "xlnx,afi-fs-ss2-bus-width",    .id = AFIFS_SS2},
> >>> +     {}
> >>> +};
> >>> +
> >>> +static int zynqmp_afi_config(struct zynqmp_afi *afi_data) {
> >>> +     struct zynqmp_afi_configreg *cfgptr = afi_cfgreg;
> >>> +     struct device_node *np = afi_data->of_node;
> >>> +     u32 afi_ss0_val, afi_ss1_val, bus_width;
> >>> +     int ret;
> >>> +
> >>> +     while (cfgptr->reg) {
> >>> +             ret = of_property_read_u32(np, cfgptr->reg, &bus_width);
> >>> +             if (!ret) {
> >>> +                     if (cfgptr->id == AFIFS_SS2) {
> >>> +                             if (bus_width == 32)
> >>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> >>> +                                                         AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL);
> >>> +                             else if (bus_width == 64)
> >>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> >>> +
> >> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL);
> >>> +                             else if (bus_width == 128)
> >>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
> >>> +
> >> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL);
> >>> +                             else
> >>> +                                     return -EINVAL;
> >>> +                     } else if (cfgptr->id == AFIFS) {
> >>> +                             if (bus_width == 32)
> >>> +                                     afi_ss0_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> >>> +                             else if (bus_width == 64)
> >>> +                                     afi_ss0_val =
> >> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL;
> >>> +                             else if (bus_width == 128)
> >>> +                                     afi_ss0_val =
> >> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL;
> >>> +                             else
> >>> +                                     return -EINVAL;
> >>> +
> >>> +                             ret = of_property_read_u32(np, "xlnx,afi-fs-ss1-bus-
> >> width",
> >>> +                                                        &bus_width);
> >>> +                             if (!ret) {
> >>> +                                     if (bus_width == 32)
> >>> +                                             afi_ss1_val =
> >> AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
> >>> +                                     else if (bus_width == 64)
> >>> +                                             afi_ss1_val =
> >> AFIFS_SS1_BUS_WIDTH_64_CONFIG_VAL;
> >>> +                                     else if (bus_width == 128)
> >>> +                                             afi_ss1_val =
> >> AFIFS_SS1_BUS_WIDTH_128_CONFIG_VAL;
> >>> +                                     else
> >>> +                                             return -EINVAL;
> >>> +
> >>> +                                     ret = zynqmp_pm_afi(AFIFS, afi_ss1_val |
> >> afi_ss0_val);
> >>> +                             }
> >>> +                     } else {
> >>> +                             if (bus_width == 32)
> >>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
> >>> +                                                         AFIFM_BUS_WIDTH_32_CONFIG_VAL);
> >>> +                             else if (bus_width == 64)
> >>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
> >>> +                                                         AFIFM_BUS_WIDTH_64_CONFIG_VAL);
> >>> +                             else if (bus_width == 128)
> >>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
> >>> +                                                         AFIFM_BUS_WIDTH_128_CONFIG_VAL);
> >>> +                             else
> >>> +                                     return -EINVAL;
> >>> +                     }
> >>> +             }
> >>> +             cfgptr++;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int zynqmp_afi_enable_set(struct fpga_bridge *bridge, bool
> >>> +enable) {
> >>> +     struct device_node *overlay = bridge->info->overlay;
> >>> +     struct zynqmp_afi *priv = bridge->priv;
> >>> +     int ret = 0;
> >>> +
> >>> +     if (enable) {
> >>> +             reset_control_reset(priv->resets);
> >>> +             return 0;
> >>> +     }
> >>> +
> >>> +     of_node_get(overlay);
> >>> +     priv->of_node =
> >>> +     of_find_node_with_property(overlay,
> >>> +                                "xlnx,afi-fm0-rd-bus-width");
> >>
> >> I'm a little confused here. The fpga_image_info.overlay is for fpga-region, but
> >> from your binding doc this property is for this afi device. You want to add
> >> another overlay targeting afi dev node during reprograming?
> >>
> > 
> > Yes, it uses Overlay's(overlay targeting afi dev node). As I said above for design-to-design AFI configs
> > relevant to the full region may change and these configs must be set prior to program the full region
> > once this overlay integrated to the live tree the existing afi config values will be replace with the new one's
> > 
> >>> +     if (priv->of_node)
> >>> +             ret = zynqmp_afi_config(priv);
> >>> +     of_node_put(priv->of_node);
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static const struct fpga_bridge_ops zynqmp_afi_br_ops = {
> >>> +     .enable_set = zynqmp_afi_enable_set, };
> >>> +
> >>> +static const struct of_device_id zynqmp_afi_of_match[] = {
> >>> +     { .compatible =  "xlnx,zynqmp-afi-fpga" },
> >>> +     { },
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, zynqmp_afi_of_match);
> >>> +
> >>> +static int zynqmp_afi_probe(struct platform_device *pdev) {
> >>> +     struct device *dev = &pdev->dev;
> >>> +     struct zynqmp_afi *priv;
> >>> +     struct fpga_bridge *br;
> >>> +
> >>> +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> >>> +     if (!priv)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     priv->dev = dev;
> >>> +
> >>> +     priv->resets =
> >> devm_reset_control_array_get_optional_exclusive(&pdev->dev);
> >>> +     if (IS_ERR(priv->resets))
> >>> +             return PTR_ERR(priv->resets);
> >>> +
> >>> +     br = fpga_bridge_register(dev, "Xilinx ZynqMP AFI",
> >>> +                               &zynqmp_afi_br_ops, priv);
> >>> +     if (IS_ERR(br)) {
> >>> +             dev_err(dev, "unable to register Xilinx ZynqMP AFI");
> >>
> >> Need a "\n" at the end?
> >>
> > 
> > Will fix.
> > 
> > Regards,
> > Navakishore.
> > 
> 
> Thanks,
> Marco
>

Marco Pagani Sept. 12, 2022, 8:02 p.m. UTC | #7

On 2022-09-03 19:44, Xu Yilun wrote:
> On 2022-09-01 at 11:53:29 +0200, Marco Pagani wrote:
>> Hi Nava,
>>
>> On 2022-08-30 11:19, Manne, Nava kishore wrote:
>>> Hi Yilun,
>>>
>>> 	Please find my response inline.
>>>
>>>> -----Original Message-----
>>>> From: Xu Yilun <[e-mail]>
>>>> Sent: Saturday, August 27, 2022 5:56 PM
>>>> To: Manne, Nava kishore <[e-mail]>
>>>> Cc: git (AMD-Xilinx) <[e-mail]>; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; linux-
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [PATCH 4/4] fpga: zynqmp: Add afi config driver
>>>>
>>>> CAUTION: This message has originated from an External Source. Please use
>>>> proper judgment and caution when opening attachments, clicking links, or
>>>> responding to this email.
>>>>
>>>>
>>>> On 2022-08-24 at 09:25:42 +0530, Nava kishore Manne wrote:
>>>>> Add zynqmp AXI FIFO interface(AFI) config driver. This is useful for
>>>>> the configuration of the PS-PL interface on Zynq US+ MPSoC platform.
>>>>
>>>> Please help illustrate how to use the device for FPGA reprogramming, why it
>>>> should be implemented as an FPGA bridge.
>>>>
>>>> From the code I actually didn't see any operation that gates the fpga-region
>>>> from other part of the machine.
>>>>
>>>
>>> The Zynq UltraScale MPSoC family consists of a system-on-chip (SoC) style integrated processing system (PS)
>>> and a Programmable Logic (PL) unit, providing an extensible and flexible SoC solution on a single die. 
>>> Xilinx Zynq US+ MPSoC connect the PS to the programmable logic (PL) through the AXI port. 
>>> This AXI port helps to establish the data path between the PS and PL (Here AXI Interface act as a Gating between PS and PL)
>>> and this AXI port configuration vary from design to design.  In-order to establish the proper communication path between
>>> PS and PL (Full region),  the AXI port data path should be configured with proper values priories to load the full region.
>>>
>>> Will update the description in v2.
>>
>> Are you implementing your AFI driver as a Bridge because you want to
>> change the configuration of the AXI HP interfaces exported by the PS
>> every time you reconfigure the full fabric?
>>
>> I understand the need to change the configuration of the PS-PL
>> interface at each full reconfiguration to ensure that the parameters
>> of the AXI ports exported by the PS (like the widths of data read
>> and write channels) matches the configuration of the master AXI ports
>> exported by the PL design. However, it seems to me that performing this
>> configuration doesn't fit the role of the FPGA Bridge, whose sole
>> responsibility should be gating the Region's interface during
>> reconfiguration.
>>
>> I think this need could be addressed by adding a new component to the
>> FPGA subsystem called "FPGA Adapter". The Adapter will be an optional
>> component associated with a Region like a Bridge. It will be responsible
>> for changing the configuration of its Region's static interface
>> (or PS/HPS/DRAM interface for the full FPGA) to match the interface
>> exported by the specific reconfigurable module (or full static design).
> 
> I saw the Documentation/driver_api/interconnect.rst, but didn't look deep
> into it. For this AFI case, it sets the bus width, is it OK to use it?
> 
> Thanks,
> Yilun

Thanks for pointing me to the interconnect subsystem. I'm currently
investigating if it's possible to implement a driver for the AFI
(and possibly other PS interconnects with their QoS regulators)
using the subsystem.
Thanks,
Marco

> 
>> Moreover, besides bus-level properties like address/data width and
>> protocol conversion, the Adapter could also be used to configure
>> system-level properties. For instance, the designer may use it to change
>> the memory access policy for the specific reconfigurable module if it
>> uses bus mastering.
>>
>>>
>>>>>
>>>>> Signed-off-by: Nava kishore Manne <[e-mail]>
>>>>> ---
>>>>>  MAINTAINERS               |   6 ++
>>>>>  drivers/fpga/Kconfig      |  13 +++
>>>>>  drivers/fpga/Makefile     |   1 +
>>>>>  drivers/fpga/zynqmp-afi.c | 211
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>  4 files changed, 231 insertions(+)
>>>>>  create mode 100644 drivers/fpga/zynqmp-afi.c
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS index
>>>>> 20ffac651214..957e753e6406 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -8032,6 +8032,12 @@ F:     Documentation/fpga/
>>>>>  F:   drivers/fpga/
>>>>>  F:   include/linux/fpga/
>>>>>
>>>>> +FPGA ZYNQMP PS-PL BRIDGE DRIVER
>>>>> +M:   Nava kishore Manne <[e-mail]>
>>>>> +S:   Supported
>>>>> +F:   Documentation/devicetree/bindings/fpga/xlnx,zynqmp-afi-fpga.yaml
>>>>> +F:   drivers/fpga/zynqmp-afi.c
>>>>> +
>>>>>  INTEL MAX10 BMC SECURE UPDATES
>>>>>  M:   Russ Weight <[e-mail]>
>>>>>  L:   [e-mail]
>>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
>>>>> 6c416955da53..c08794d30fb5 100644
>>>>> --- a/drivers/fpga/Kconfig
>>>>> +++ b/drivers/fpga/Kconfig
>>>>> @@ -130,6 +130,19 @@ config XILINX_PR_DECOUPLER
>>>>>         reconfiguration, preventing the system deadlock that can
>>>>>         occur if AXI transactions are interrupted by DFX.
>>>>>
>>>>> +config ZYNQMP_AFI
>>>>> +     tristate "Xilinx ZYNQMP AFI support"
>>>>> +     depends on FPGA_BRIDGE
>>>>> +     help
>>>>> +       Say Y to enable drivers to handle the PS-PL clocks configurations
>>>>> +       and PS-PL Bus-width. Xilinx Zynq US+ MPSoC connect the PS to the
>>>>> +       programmable logic (PL) through the AXI port. This AXI port helps
>>>>> +       to establish the data path between the PS and PL.
>>>>> +       In-order to establish the proper communication path between PS and
>>>> PL,
>>>>> +       the AXI port data path should be configured with the proper Bus-
>>>> width
>>>>> +       values and it will also handles the PS-PL reset signals to reset the
>>>>> +       PL domain.
>>>>
>>>> Same concern, please describe its relationship to FPGA reprogramming.
>>>>
>>>
>>> Same as above.
>>> Will update the description in v2.
>>>
>>>>> +
>>>>>  config FPGA_REGION
>>>>>       tristate "FPGA Region"
>>>>>       depends on FPGA_BRIDGE
>>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
>>>>> 42ae8b58abce..94cfe60972db 100644
>>>>> --- a/drivers/fpga/Makefile
>>>>> +++ b/drivers/fpga/Makefile
>>>>> @@ -31,6 +31,7 @@ obj-$(CONFIG_FPGA_BRIDGE)           += fpga-bridge.o
>>>>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)    += altera-hps2fpga.o altera-
>>>> fpga2sdram.o
>>>>>  obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)   += altera-freeze-bridge.o
>>>>>  obj-$(CONFIG_XILINX_PR_DECOUPLER)    += xilinx-pr-decoupler.o
>>>>> +obj-$(CONFIG_ZYNQMP_AFI)             += zynqmp-afi.o
>>>>>
>>>>>  # High Level Interfaces
>>>>>  obj-$(CONFIG_FPGA_REGION)            += fpga-region.o
>>>>> diff --git a/drivers/fpga/zynqmp-afi.c b/drivers/fpga/zynqmp-afi.c new
>>>>> file mode 100644 index 000000000000..bc975d304039
>>>>> --- /dev/null
>>>>> +++ b/drivers/fpga/zynqmp-afi.c
>>>>> @@ -0,0 +1,211 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright (C) 2022 Xilinx, Inc.
>>>>> + */
>>>>> +
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/firmware/xlnx-zynqmp.h> #include
>>>>> +<linux/fpga/fpga-bridge.h> #include <linux/io.h> #include
>>>>> +<linux/module.h> #include <linux/of.h> #include
>>>>> +<linux/platform_device.h> #include <linux/reset.h> #include
>>>>> +<linux/slab.h>
>>>>> +
>>>>> +/* Registers and special values for doing register-based operations */
>>>>> +#define AFI_RDCHAN_CTRL_OFFSET       0x00
>>>>> +#define AFI_WRCHAN_CTRL_OFFSET       0x14
>>>>> +#define AFI_BUSWIDTH_MASK    BIT(0)
>>>>> +
>>>>> +/**
>>>>> + * struct zynqmp_afi - AFI register description.
>>>>> + * @dev: device that owns this.
>>>>> + * @of_node: Device Tree overlay.
>>>>> + * @resets: Pointer to the reset control for ps-pl resets.
>>>>> + */
>>>>> +struct zynqmp_afi {
>>>>> +     struct device *dev;
>>>>> +     struct device_node *of_node;
>>>>> +     struct reset_control *resets;
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * struct zynqmp_afi_configreg - AFI configuration registers info.
>>>>> + * @reg:     Name of the AFI configuration register.
>>>>> + * @id:              Register index value.
>>>>> + */
>>>>> +struct zynqmp_afi_configreg {
>>>>> +     char *reg;
>>>>> +     u32 id;
>>>>> +};
>>>>> +
>>>>> +static struct zynqmp_afi_configreg afi_cfgreg[] = {
>>>>> +     {.reg = "xlnx,afi-fm0-rd-bus-width",    .id = AFIFM0_RDCTRL},
>>>>> +     {.reg = "xlnx,afi-fm1-rd-bus-width",    .id = AFIFM1_RDCTRL},
>>>>> +     {.reg = "xlnx,afi-fm2-rd-bus-width",    .id = AFIFM2_RDCTRL},
>>>>> +     {.reg = "xlnx,afi-fm3-rd-bus-width",    .id = AFIFM3_RDCTRL},
>>>>> +     {.reg = "xlnx,afi-fm4-rd-bus-width",    .id = AFIFM4_RDCTRL},
>>>>> +     {.reg = "xlnx,afi-fm5-rd-bus-width",    .id = AFIFM5_RDCTRL},
>>>>> +     {.reg = "xlnx,afi-fm6-rd-bus-width",    .id = AFIFM6_RDCTRL},
>>>>> +     {.reg = "xlnx,afi-fm0-wr-bus-width",    .id = AFIFM0_WRCTRL},
>>>>> +     {.reg = "xlnx,afi-fm1-wr-bus-width",    .id = AFIFM1_WRCTRL},
>>>>> +     {.reg = "xlnx,afi-fm2-wr-bus-width",    .id = AFIFM2_WRCTRL},
>>>>> +     {.reg = "xlnx,afi-fm3-wr-bus-width",    .id = AFIFM3_WRCTRL},
>>>>> +     {.reg = "xlnx,afi-fm4-wr-bus-width",    .id = AFIFM4_WRCTRL},
>>>>> +     {.reg = "xlnx,afi-fm5-wr-bus-width",    .id = AFIFM5_WRCTRL},
>>>>> +     {.reg = "xlnx,afi-fm6-wr-bus-width",    .id = AFIFM6_WRCTRL},
>>>>> +     {.reg = "xlnx,afi-fs-ss0-bus-width",    .id = AFIFS},
>>>>> +     {.reg = "xlnx,afi-fs-ss2-bus-width",    .id = AFIFS_SS2},
>>>>> +     {}
>>>>> +};
>>>>> +
>>>>> +static int zynqmp_afi_config(struct zynqmp_afi *afi_data) {
>>>>> +     struct zynqmp_afi_configreg *cfgptr = afi_cfgreg;
>>>>> +     struct device_node *np = afi_data->of_node;
>>>>> +     u32 afi_ss0_val, afi_ss1_val, bus_width;
>>>>> +     int ret;
>>>>> +
>>>>> +     while (cfgptr->reg) {
>>>>> +             ret = of_property_read_u32(np, cfgptr->reg, &bus_width);
>>>>> +             if (!ret) {
>>>>> +                     if (cfgptr->id == AFIFS_SS2) {
>>>>> +                             if (bus_width == 32)
>>>>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
>>>>> +                                                         AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL);
>>>>> +                             else if (bus_width == 64)
>>>>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
>>>>> +
>>>> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL);
>>>>> +                             else if (bus_width == 128)
>>>>> +                                     ret = zynqmp_pm_afi(AFIFS_SS2,
>>>>> +
>>>> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL);
>>>>> +                             else
>>>>> +                                     return -EINVAL;
>>>>> +                     } else if (cfgptr->id == AFIFS) {
>>>>> +                             if (bus_width == 32)
>>>>> +                                     afi_ss0_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
>>>>> +                             else if (bus_width == 64)
>>>>> +                                     afi_ss0_val =
>>>> AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL;
>>>>> +                             else if (bus_width == 128)
>>>>> +                                     afi_ss0_val =
>>>> AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL;
>>>>> +                             else
>>>>> +                                     return -EINVAL;
>>>>> +
>>>>> +                             ret = of_property_read_u32(np, "xlnx,afi-fs-ss1-bus-
>>>> width",
>>>>> +                                                        &bus_width);
>>>>> +                             if (!ret) {
>>>>> +                                     if (bus_width == 32)
>>>>> +                                             afi_ss1_val =
>>>> AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
>>>>> +                                     else if (bus_width == 64)
>>>>> +                                             afi_ss1_val =
>>>> AFIFS_SS1_BUS_WIDTH_64_CONFIG_VAL;
>>>>> +                                     else if (bus_width == 128)
>>>>> +                                             afi_ss1_val =
>>>> AFIFS_SS1_BUS_WIDTH_128_CONFIG_VAL;
>>>>> +                                     else
>>>>> +                                             return -EINVAL;
>>>>> +
>>>>> +                                     ret = zynqmp_pm_afi(AFIFS, afi_ss1_val |
>>>> afi_ss0_val);
>>>>> +                             }
>>>>> +                     } else {
>>>>> +                             if (bus_width == 32)
>>>>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
>>>>> +                                                         AFIFM_BUS_WIDTH_32_CONFIG_VAL);
>>>>> +                             else if (bus_width == 64)
>>>>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
>>>>> +                                                         AFIFM_BUS_WIDTH_64_CONFIG_VAL);
>>>>> +                             else if (bus_width == 128)
>>>>> +                                     ret = zynqmp_pm_afi(cfgptr->id,
>>>>> +                                                         AFIFM_BUS_WIDTH_128_CONFIG_VAL);
>>>>> +                             else
>>>>> +                                     return -EINVAL;
>>>>> +                     }
>>>>> +             }
>>>>> +             cfgptr++;
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int zynqmp_afi_enable_set(struct fpga_bridge *bridge, bool
>>>>> +enable) {
>>>>> +     struct device_node *overlay = bridge->info->overlay;
>>>>> +     struct zynqmp_afi *priv = bridge->priv;
>>>>> +     int ret = 0;
>>>>> +
>>>>> +     if (enable) {
>>>>> +             reset_control_reset(priv->resets);
>>>>> +             return 0;
>>>>> +     }
>>>>> +
>>>>> +     of_node_get(overlay);
>>>>> +     priv->of_node =
>>>>> +     of_find_node_with_property(overlay,
>>>>> +                                "xlnx,afi-fm0-rd-bus-width");
>>>>
>>>> I'm a little confused here. The fpga_image_info.overlay is for fpga-region, but
>>>> from your binding doc this property is for this afi device. You want to add
>>>> another overlay targeting afi dev node during reprograming?
>>>>
>>>
>>> Yes, it uses Overlay's(overlay targeting afi dev node). As I said above for design-to-design AFI configs
>>> relevant to the full region may change and these configs must be set prior to program the full region
>>> once this overlay integrated to the live tree the existing afi config values will be replace with the new one's
>>>
>>>>> +     if (priv->of_node)
>>>>> +             ret = zynqmp_afi_config(priv);
>>>>> +     of_node_put(priv->of_node);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct fpga_bridge_ops zynqmp_afi_br_ops = {
>>>>> +     .enable_set = zynqmp_afi_enable_set, };
>>>>> +
>>>>> +static const struct of_device_id zynqmp_afi_of_match[] = {
>>>>> +     { .compatible =  "xlnx,zynqmp-afi-fpga" },
>>>>> +     { },
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, zynqmp_afi_of_match);
>>>>> +
>>>>> +static int zynqmp_afi_probe(struct platform_device *pdev) {
>>>>> +     struct device *dev = &pdev->dev;
>>>>> +     struct zynqmp_afi *priv;
>>>>> +     struct fpga_bridge *br;
>>>>> +
>>>>> +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>>> +     if (!priv)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     priv->dev = dev;
>>>>> +
>>>>> +     priv->resets =
>>>> devm_reset_control_array_get_optional_exclusive(&pdev->dev);
>>>>> +     if (IS_ERR(priv->resets))
>>>>> +             return PTR_ERR(priv->resets);
>>>>> +
>>>>> +     br = fpga_bridge_register(dev, "Xilinx ZynqMP AFI",
>>>>> +                               &zynqmp_afi_br_ops, priv);
>>>>> +     if (IS_ERR(br)) {
>>>>> +             dev_err(dev, "unable to register Xilinx ZynqMP AFI");
>>>>
>>>> Need a "\n" at the end?
>>>>
>>>
>>> Will fix.
>>>
>>> Regards,
>>> Navakishore.
>>>
>>
>> Thanks,
>> Marco
>>
>

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 20ffac651214..957e753e6406 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8032,6 +8032,12 @@  F:	Documentation/fpga/
 F:	drivers/fpga/
 F:	include/linux/fpga/
 
+FPGA ZYNQMP PS-PL BRIDGE DRIVER
+M:	Nava kishore Manne <[e-mail]>
+S:	Supported
+F:	Documentation/devicetree/bindings/fpga/xlnx,zynqmp-afi-fpga.yaml
+F:	drivers/fpga/zynqmp-afi.c
+
 INTEL MAX10 BMC SECURE UPDATES
 M:	Russ Weight <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 6c416955da53..c08794d30fb5 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -130,6 +130,19 @@  config XILINX_PR_DECOUPLER
 	  reconfiguration, preventing the system deadlock that can
 	  occur if AXI transactions are interrupted by DFX.
 
+config ZYNQMP_AFI
+	tristate "Xilinx ZYNQMP AFI support"
+	depends on FPGA_BRIDGE
+	help
+	  Say Y to enable drivers to handle the PS-PL clocks configurations
+	  and PS-PL Bus-width. Xilinx Zynq US+ MPSoC connect the PS to the
+	  programmable logic (PL) through the AXI port. This AXI port helps
+	  to establish the data path between the PS and PL.
+	  In-order to establish the proper communication path between PS and PL,
+	  the AXI port data path should be configured with the proper Bus-width
+	  values and it will also handles the PS-PL reset signals to reset the
+	  PL domain.
+
 config FPGA_REGION
 	tristate "FPGA Region"
 	depends on FPGA_BRIDGE
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 42ae8b58abce..94cfe60972db 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -31,6 +31,7 @@  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
 obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
 obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)	+= altera-freeze-bridge.o
 obj-$(CONFIG_XILINX_PR_DECOUPLER)	+= xilinx-pr-decoupler.o
+obj-$(CONFIG_ZYNQMP_AFI)		+= zynqmp-afi.o
 
 # High Level Interfaces
 obj-$(CONFIG_FPGA_REGION)		+= fpga-region.o
diff --git a/drivers/fpga/zynqmp-afi.c b/drivers/fpga/zynqmp-afi.c
new file mode 100644
index 000000000000..bc975d304039
--- /dev/null
+++ b/drivers/fpga/zynqmp-afi.c
@@ -0,0 +1,211 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Xilinx, Inc.
+ */
+
+#include <linux/err.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+/* Registers and special values for doing register-based operations */
+#define AFI_RDCHAN_CTRL_OFFSET	0x00
+#define AFI_WRCHAN_CTRL_OFFSET	0x14
+#define AFI_BUSWIDTH_MASK	BIT(0)
+
+/**
+ * struct zynqmp_afi - AFI register description.
+ * @dev: device that owns this.
+ * @of_node: Device Tree overlay.
+ * @resets: Pointer to the reset control for ps-pl resets.
+ */
+struct zynqmp_afi {
+	struct device *dev;
+	struct device_node *of_node;
+	struct reset_control *resets;
+};
+
+/**
+ * struct zynqmp_afi_configreg - AFI configuration registers info.
+ * @reg:	Name of the AFI configuration register.
+ * @id:		Register index value.
+ */
+struct zynqmp_afi_configreg {
+	char *reg;
+	u32 id;
+};
+
+static struct zynqmp_afi_configreg afi_cfgreg[] = {
+	{.reg = "xlnx,afi-fm0-rd-bus-width",	.id = AFIFM0_RDCTRL},
+	{.reg = "xlnx,afi-fm1-rd-bus-width",	.id = AFIFM1_RDCTRL},
+	{.reg = "xlnx,afi-fm2-rd-bus-width",	.id = AFIFM2_RDCTRL},
+	{.reg = "xlnx,afi-fm3-rd-bus-width",	.id = AFIFM3_RDCTRL},
+	{.reg = "xlnx,afi-fm4-rd-bus-width",	.id = AFIFM4_RDCTRL},
+	{.reg = "xlnx,afi-fm5-rd-bus-width",	.id = AFIFM5_RDCTRL},
+	{.reg = "xlnx,afi-fm6-rd-bus-width",	.id = AFIFM6_RDCTRL},
+	{.reg = "xlnx,afi-fm0-wr-bus-width",	.id = AFIFM0_WRCTRL},
+	{.reg = "xlnx,afi-fm1-wr-bus-width",	.id = AFIFM1_WRCTRL},
+	{.reg = "xlnx,afi-fm2-wr-bus-width",	.id = AFIFM2_WRCTRL},
+	{.reg = "xlnx,afi-fm3-wr-bus-width",	.id = AFIFM3_WRCTRL},
+	{.reg = "xlnx,afi-fm4-wr-bus-width",	.id = AFIFM4_WRCTRL},
+	{.reg = "xlnx,afi-fm5-wr-bus-width",	.id = AFIFM5_WRCTRL},
+	{.reg = "xlnx,afi-fm6-wr-bus-width",	.id = AFIFM6_WRCTRL},
+	{.reg = "xlnx,afi-fs-ss0-bus-width",	.id = AFIFS},
+	{.reg = "xlnx,afi-fs-ss2-bus-width",	.id = AFIFS_SS2},
+	{}
+};
+
+static int zynqmp_afi_config(struct zynqmp_afi *afi_data)
+{
+	struct zynqmp_afi_configreg *cfgptr = afi_cfgreg;
+	struct device_node *np = afi_data->of_node;
+	u32 afi_ss0_val, afi_ss1_val, bus_width;
+	int ret;
+
+	while (cfgptr->reg) {
+		ret = of_property_read_u32(np, cfgptr->reg, &bus_width);
+		if (!ret) {
+			if (cfgptr->id == AFIFS_SS2) {
+				if (bus_width == 32)
+					ret = zynqmp_pm_afi(AFIFS_SS2,
+							    AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL);
+				else if (bus_width == 64)
+					ret = zynqmp_pm_afi(AFIFS_SS2,
+							    AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL);
+				else if (bus_width == 128)
+					ret = zynqmp_pm_afi(AFIFS_SS2,
+							    AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL);
+				else
+					return -EINVAL;
+			} else if (cfgptr->id == AFIFS) {
+				if (bus_width == 32)
+					afi_ss0_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
+				else if (bus_width == 64)
+					afi_ss0_val = AFIFS_SS0_SS2_BUS_WIDTH_64_CONFIG_VAL;
+				else if (bus_width == 128)
+					afi_ss0_val = AFIFS_SS0_SS2_BUS_WIDTH_128_CONFIG_VAL;
+				else
+					return -EINVAL;
+
+				ret = of_property_read_u32(np, "xlnx,afi-fs-ss1-bus-width",
+							   &bus_width);
+				if (!ret) {
+					if (bus_width == 32)
+						afi_ss1_val = AFIFS_SS_BUS_WIDTH_32_CONFIG_VAL;
+					else if (bus_width == 64)
+						afi_ss1_val = AFIFS_SS1_BUS_WIDTH_64_CONFIG_VAL;
+					else if (bus_width == 128)
+						afi_ss1_val = AFIFS_SS1_BUS_WIDTH_128_CONFIG_VAL;
+					else
+						return -EINVAL;
+
+					ret = zynqmp_pm_afi(AFIFS, afi_ss1_val | afi_ss0_val);
+				}
+			} else {
+				if (bus_width == 32)
+					ret = zynqmp_pm_afi(cfgptr->id,
+							    AFIFM_BUS_WIDTH_32_CONFIG_VAL);
+				else if (bus_width == 64)
+					ret = zynqmp_pm_afi(cfgptr->id,
+							    AFIFM_BUS_WIDTH_64_CONFIG_VAL);
+				else if (bus_width == 128)
+					ret = zynqmp_pm_afi(cfgptr->id,
+							    AFIFM_BUS_WIDTH_128_CONFIG_VAL);
+				else
+					return -EINVAL;
+			}
+		}
+		cfgptr++;
+	}
+
+	return 0;
+}
+
+static int zynqmp_afi_enable_set(struct fpga_bridge *bridge, bool enable)
+{
+	struct device_node *overlay = bridge->info->overlay;
+	struct zynqmp_afi *priv = bridge->priv;
+	int ret = 0;
+
+	if (enable) {
+		reset_control_reset(priv->resets);
+		return 0;
+	}
+
+	of_node_get(overlay);
+	priv->of_node =
+	of_find_node_with_property(overlay,
+				   "xlnx,afi-fm0-rd-bus-width");
+	if (priv->of_node)
+		ret = zynqmp_afi_config(priv);
+	of_node_put(priv->of_node);
+
+	return ret;
+}
+
+static const struct fpga_bridge_ops zynqmp_afi_br_ops = {
+	.enable_set = zynqmp_afi_enable_set,
+};
+
+static const struct of_device_id zynqmp_afi_of_match[] = {
+	{ .compatible =  "xlnx,zynqmp-afi-fpga" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, zynqmp_afi_of_match);
+
+static int zynqmp_afi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct zynqmp_afi *priv;
+	struct fpga_bridge *br;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	priv->resets = devm_reset_control_array_get_optional_exclusive(&pdev->dev);
+	if (IS_ERR(priv->resets))
+		return PTR_ERR(priv->resets);
+
+	br = fpga_bridge_register(dev, "Xilinx ZynqMP AFI",
+				  &zynqmp_afi_br_ops, priv);
+	if (IS_ERR(br)) {
+		dev_err(dev, "unable to register Xilinx ZynqMP AFI");
+		return PTR_ERR(br);
+	}
+
+	platform_set_drvdata(pdev, br);
+
+	return 0;
+}
+
+static int zynqmp_afi_remove(struct platform_device *pdev)
+{
+	struct fpga_bridge *bridge = platform_get_drvdata(pdev);
+
+	fpga_bridge_unregister(bridge);
+
+	return 0;
+}
+
+static struct platform_driver zynqmp_afi_driver = {
+	.probe = zynqmp_afi_probe,
+	.remove = zynqmp_afi_remove,
+	.driver = {
+		.name = "zynqmp_afi",
+		.of_match_table = of_match_ptr(zynqmp_afi_of_match),
+	},
+};
+
+module_platform_driver(zynqmp_afi_driver);
+
+MODULE_DESCRIPTION("ZYNQMP FPGA AXI FIFO interface configuration module");
+MODULE_AUTHOR("Nava kishore Manne <[e-mail]>");
+MODULE_LICENSE("GPL");