Re: [PATCH 5/5] misc: zynqmp: Add afi config driver

[Greg KH]

On Tue, Apr 20, 2021 at 01:47:17PM +0000, Nava kishore Manne wrote:
> Hi Greg,
> 
> 	Please find my response inline.
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent: Tuesday, April 20, 2021 2:21 PM
> > To: Nava kishore Manne <navam@xxxxxxxxxx>
> > Cc: robh+dt@xxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Derek Kiernan
> > <dkiernan@xxxxxxxxxx>; Dragan Cvetic <draganc@xxxxxxxxxx>;
> > arnd@xxxxxxxx; Rajan Vaja <RAJANV@xxxxxxxxxx>; Jolly Shah
> > <JOLLYS@xxxxxxxxxx>; Tejas Patel <tejasp@xxxxxxxxxxxxxxx>; Amit Sunil
> > Dhamne <amitsuni@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > chinnikishore369@xxxxxxxxx; git <git@xxxxxxxxxx>
> > Subject: Re: [PATCH 5/5] misc: zynqmp: Add afi config driver
> > 
> > On Tue, Apr 20, 2021 at 01:41:53PM +0530, Nava kishore Manne wrote:
> > > This patch adds zynqmp 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 <nava.manne@xxxxxxxxxx>
> > > ---
> > >  drivers/misc/Kconfig      | 11 ++++++
> > >  drivers/misc/Makefile     |  1 +
> > >  drivers/misc/zynqmp-afi.c | 83
> > > +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 95 insertions(+)
> > >  create mode 100644 drivers/misc/zynqmp-afi.c
> > >
> > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> > > 877b43b3377d..d1ea1eeb3ac1 100644
> > > --- a/drivers/misc/Kconfig
> > > +++ b/drivers/misc/Kconfig
> > > @@ -456,6 +456,17 @@ config ZYNQ_AFI
> > >  	  between PS and PL, the AXI port data path should be configured
> > >  	  with the proper Bus-width values
> > >
> > > +config ZYNQMP_AFI
> > > +        tristate "Xilinx ZYNQMP AFI support"
> > > +        help
> > > +	  ZynqMP AFI driver support for writing to the AFI registers for
> > > +	  configuring 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
> > > +
> > >  source "drivers/misc/c2port/Kconfig"
> > >  source "drivers/misc/eeprom/Kconfig"
> > >  source "drivers/misc/cb710/Kconfig"
> > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
> > > e9b03843100f..54bd0edc511e 100644
> > > --- a/drivers/misc/Makefile
> > > +++ b/drivers/misc/Makefile
> > > @@ -57,3 +57,4 @@ obj-$(CONFIG_UACCE)		+= uacce/
> > >  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
> > >  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
> > >  obj-$(CONFIG_ZYNQ_AFI)		+= zynq-afi.o
> > > +obj-$(CONFIG_ZYNQMP_AFI)	+= zynqmp-afi.o
> > > diff --git a/drivers/misc/zynqmp-afi.c b/drivers/misc/zynqmp-afi.c new
> > > file mode 100644 index 000000000000..a318652576d2
> > > --- /dev/null
> > > +++ b/drivers/misc/zynqmp-afi.c
> > > @@ -0,0 +1,83 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Xilinx FPGA AFI bridge.
> > > + * Copyright (c) 2018-2021 Xilinx Inc.
> > > + */
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/firmware/xlnx-zynqmp.h> #include <linux/io.h>
> > > +#include <linux/module.h> #include <linux/of.h> #include
> > > +<linux/platform_device.h> #include <linux/slab.h>
> > > +
> > > +/**
> > > + * struct zynqmp_afi_fpga - AFI register description
> > > + * @value: value to be written to the register
> > > + * @regid: Register id for the register to be written
> > > + */
> > > +struct zynqmp_afi_fpga {
> > > +	u32 value;
> > > +	u32 regid;
> > > +};
> > > +
> > > +static int zynqmp_afi_fpga_probe(struct platform_device *pdev)
> > > +{
> > > +	struct zynqmp_afi_fpga *zynqmp_afi_fpga;
> > > +	struct device_node *np = pdev->dev.of_node;
> > > +	int i, entries, ret;
> > > +	u32 reg, val;
> > > +
> > > +	zynqmp_afi_fpga = devm_kzalloc(&pdev->dev,
> > > +				       sizeof(*zynqmp_afi_fpga), GFP_KERNEL);
> > > +	if (!zynqmp_afi_fpga)
> > > +		return -ENOMEM;
> > > +	platform_set_drvdata(pdev, zynqmp_afi_fpga);
> > > +
> > > +	entries = of_property_count_u32_elems(np, "config-afi");
> > > +	if (!entries || (entries % 2)) {
> > > +		dev_err(&pdev->dev, "Invalid number of registers\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	for (i = 0; i < entries / 2; i++) {
> > > +		ret = of_property_read_u32_index(np, "config-afi", i * 2,
> > &reg);
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev, "failed to read register\n");
> > > +			return -EINVAL;
> > > +		}
> > > +		ret = of_property_read_u32_index(np, "config-afi", i * 2 + 1,
> > > +						 &val);
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev, "failed to read value\n");
> > > +			return -EINVAL;
> > > +		}
> > > +		ret = zynqmp_pm_afi(reg, val);
> > > +		if (ret < 0) {
> > > +			dev_err(&pdev->dev, "AFI register write error %d\n",
> > > +				ret);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > 
> > Again, why does this have to be in the kernel?  All it does is make a
> > single call to the hardware based on some values read from the device
> > tree.  Can't you do this from userspace?
> > 
> For every PL design has its own PS-PL configuration.

What is a "PL design", and what is a "PS-PL configuration"?  :)

> This driver will be used by the overlay framework for configuring the interface after programming the FPGA and before probing the drivers that are present in the PL.

Again, I have no idea what this means at all.

And again, why does this have to be done in the kernel?  All you are
doing is sending some random values read in DT down to a hardware
device.   What requires a kernel driver for this?

thanks,

greg k-h