Re: [PATCH V4 XRT Alveo 20/20] fpga: xrt: Kconfig and Makefile updates for XRT drivers

[Lizhi Hou <lizhi.hou@xxxxxxxxxx>]

Hi Tom,


On 04/06/2021 02:00 PM, Tom Rix wrote:
> On 3/23/21 10:29 PM, Lizhi Hou wrote:
> > Update fpga Kconfig/Makefile and add Kconfig/Makefile for new drivers.
> >
> > Signed-off-by: Sonal Santan <sonal.santan@xxxxxxxxxx>
> > Signed-off-by: Max Zhen <max.zhen@xxxxxxxxxx>
> > Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxxxxx>
> > ---
> >   MAINTAINERS                        | 11 +++++++++++
> >   drivers/Makefile                   |  1 +
> >   drivers/fpga/Kconfig               |  2 ++
> >   drivers/fpga/Makefile              |  5 +++++
> >   drivers/fpga/xrt/Kconfig           |  8 ++++++++
> >   drivers/fpga/xrt/lib/Kconfig       | 17 +++++++++++++++++
> >   drivers/fpga/xrt/lib/Makefile      | 30 ++++++++++++++++++++++++++++++
> >   drivers/fpga/xrt/metadata/Kconfig  | 12 ++++++++++++
> >   drivers/fpga/xrt/metadata/Makefile | 16 ++++++++++++++++
> >   drivers/fpga/xrt/mgmt/Kconfig      | 15 +++++++++++++++
> >   drivers/fpga/xrt/mgmt/Makefile     | 19 +++++++++++++++++++
> >   11 files changed, 136 insertions(+)
> >   create mode 100644 drivers/fpga/xrt/Kconfig
> >   create mode 100644 drivers/fpga/xrt/lib/Kconfig
> >   create mode 100644 drivers/fpga/xrt/lib/Makefile
> >   create mode 100644 drivers/fpga/xrt/metadata/Kconfig
> >   create mode 100644 drivers/fpga/xrt/metadata/Makefile
> >   create mode 100644 drivers/fpga/xrt/mgmt/Kconfig
> >   create mode 100644 drivers/fpga/xrt/mgmt/Makefile
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index aa84121c5611..44ccc52987ac 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7009,6 +7009,17 @@ F:     Documentation/fpga/
> >   F:  drivers/fpga/
> >   F:  include/linux/fpga/
> >
> > +FPGA XRT DRIVERS
> > +M:   Lizhi Hou <lizhi.hou@xxxxxxxxxx>
> > +R:   Max Zhen <max.zhen@xxxxxxxxxx>
> > +R:   Sonal Santan <sonal.santan@xxxxxxxxxx>
> > +L:   linux-fpga@xxxxxxxxxxxxxxx
> > +S:   Maintained
> Should this be 'Supported' ?
Sure.
> > +W:   https://github.com/Xilinx/XRT
> > +F:   Documentation/fpga/xrt.rst
> > +F:   drivers/fpga/xrt/
> > +F:   include/uapi/linux/xrt/
> > +
> >   FPU EMULATOR
> >   M:  Bill Metzenthen <billm@xxxxxxxxxxxxx>
> >   S:  Maintained
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 6fba7daba591..dbb3b727fc7a 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -179,6 +179,7 @@ obj-$(CONFIG_STM)         += hwtracing/stm/
> >   obj-$(CONFIG_ANDROID)               += android/
> >   obj-$(CONFIG_NVMEM)         += nvmem/
> >   obj-$(CONFIG_FPGA)          += fpga/
> > +obj-$(CONFIG_FPGA_XRT_METADATA) += fpga/
> CONFIG_FPGA_XRT_METADATA is only defined when CONFIG_FPGA is, so i don't
> think this line is needed.
CONFIG_FPGA could be 'm'.
And as we discussed before, CONFIG_FPGA_XRT_METADATA extending fdt_* can 
be only build in kernel ('y'). Maybe it can not rely on CONFIG_FPGA?
> >   obj-$(CONFIG_FSI)           += fsi/
> >   obj-$(CONFIG_TEE)           += tee/
> >   obj-$(CONFIG_MULTIPLEXER)   += mux/
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 5ff9438b7b46..01410ff000b9 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -227,4 +227,6 @@ config FPGA_MGR_ZYNQMP_FPGA
> >         to configure the programmable logic(PL) through PS
> >         on ZynqMP SoC.
> >
> > +source "drivers/fpga/xrt/Kconfig"
> > +
> >   endif # FPGA
> This is where it is defined..
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 18dc9885883a..4b887bf95cb3 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -48,3 +48,8 @@ obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000) += 
> > dfl-n3000-nios.o
> >
> >   # Drivers for FPGAs which implement DFL
> >   obj-$(CONFIG_FPGA_DFL_PCI)          += dfl-pci.o
> > +
> > +# XRT drivers for Alveo
> > +obj-$(CONFIG_FPGA_XRT_METADATA)              += xrt/metadata/
> > +obj-$(CONFIG_FPGA_XRT_LIB)           += xrt/lib/
> > +obj-$(CONFIG_FPGA_XRT_XMGMT)         += xrt/mgmt/
> > diff --git a/drivers/fpga/xrt/Kconfig b/drivers/fpga/xrt/Kconfig
> > new file mode 100644
> > index 000000000000..0e2c59589ddd
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/Kconfig
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# Xilinx Alveo FPGA device configuration
> > +#
> > +
> > +source "drivers/fpga/xrt/metadata/Kconfig"
> > +source "drivers/fpga/xrt/lib/Kconfig"
> > +source "drivers/fpga/xrt/mgmt/Kconfig"
> > diff --git a/drivers/fpga/xrt/lib/Kconfig b/drivers/fpga/xrt/lib/Kconfig
> > new file mode 100644
> > index 000000000000..935369fad570
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/lib/Kconfig
> > @@ -0,0 +1,17 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# XRT Alveo FPGA device configuration
> > +#
> > +
> > +config FPGA_XRT_LIB
> > +     tristate "XRT Alveo Driver Library"
> > +     depends on HWMON && PCI && HAS_IOMEM
> > +     select FPGA_XRT_METADATA
> > +     select REGMAP_MMIO
> > +     help
> > +       Select this option to enable Xilinx XRT Alveo driver library. 
> > This
> > +       library is core infrastructure of XRT Alveo FPGA drivers which
> > +       provides functions for working with device nodes, iteration and
> > +       lookup of platform devices, common interfaces for platform 
> > devices,
> > +       plumbing of function call and ioctls between platform devices 
> > and
> > +       parent partitions.
> > diff --git a/drivers/fpga/xrt/lib/Makefile 
> > b/drivers/fpga/xrt/lib/Makefile
> > new file mode 100644
> > index 000000000000..58563416efbf
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/lib/Makefile
> > @@ -0,0 +1,30 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright (C) 2020-2021 Xilinx, Inc. All rights reserved.
> > +#
> > +# Authors: Sonal.Santan@xxxxxxxxxx
> > +#
> > +
> > +FULL_XRT_PATH=$(srctree)/$(src)/..
> > +FULL_DTC_PATH=$(srctree)/scripts/dtc/libfdt
> > +
> > +obj-$(CONFIG_FPGA_XRT_LIB) += xrt-lib.o
> > +
> > +xrt-lib-objs :=                      \
> > +     lib-drv.o               \
> > +     xroot.o                 \
> > +     xclbin.o                \
> > +     subdev.o                \
> > +     cdev.o                  \
> > +     group.o                 \
> > +     xleaf/vsec.o            \
> > +     xleaf/axigate.o         \
> > +     xleaf/devctl.o          \
> > +     xleaf/icap.o            \
> > +     xleaf/clock.o           \
> > +     xleaf/clkfreq.o         \
> > +     xleaf/ucs.o             \
> > +     xleaf/ddr_calibration.o
> > +
> > +ccflags-y := -I$(FULL_XRT_PATH)/include       \
> > +     -I$(FULL_DTC_PATH)
> > diff --git a/drivers/fpga/xrt/metadata/Kconfig 
> > b/drivers/fpga/xrt/metadata/Kconfig
> > new file mode 100644
> > index 000000000000..129adda47e94
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/metadata/Kconfig
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# XRT Alveo FPGA device configuration
> > +#
> > +
> > +config FPGA_XRT_METADATA
> > +     bool "XRT Alveo Driver Metadata Parser"
> > +     select LIBFDT
> > +     help
> > +       This option provides helper functions to parse Xilinx Alveo FPGA
> > +       firmware metadata. The metadata is in device tree format and the
> > +       XRT driver uses it to discover the HW subsystems behind PCIe 
> > BAR.
> > diff --git a/drivers/fpga/xrt/metadata/Makefile 
> > b/drivers/fpga/xrt/metadata/Makefile
> > new file mode 100644
> > index 000000000000..14f65ef1595c
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/metadata/Makefile
> > @@ -0,0 +1,16 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright (C) 2020-2021 Xilinx, Inc. All rights reserved.
> > +#
> > +# Authors: Sonal.Santan@xxxxxxxxxx
> > +#
> > +
> > +FULL_XRT_PATH=$(srctree)/$(src)/..
> > +FULL_DTC_PATH=$(srctree)/scripts/dtc/libfdt
> > +
> > +obj-$(CONFIG_FPGA_XRT_METADATA) += xrt-md.o
> > +
> > +xrt-md-objs := metadata.o
> > +
> > +ccflags-y := -I$(FULL_XRT_PATH)/include      \
> > +     -I$(FULL_DTC_PATH)
> > diff --git a/drivers/fpga/xrt/mgmt/Kconfig 
> > b/drivers/fpga/xrt/mgmt/Kconfig
> > new file mode 100644
> > index 000000000000..31e9e19fffb8
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/mgmt/Kconfig
> > @@ -0,0 +1,15 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# Xilinx XRT FPGA device configuration
> > +#
> > +
> > +config FPGA_XRT_XMGMT
> > +     tristate "Xilinx Alveo Management Driver"
> > +     depends on FPGA_XRT_LIB
> > +     select FPGA_XRT_METADATA
>
> If the XRT driver depends on these other two configs and it does not
> make sense to build these two seperately, could you remove these configs
> and just use something like FPGA_XRT ?
This is similar reason with above. CONFIG_FPGA_XRT_METADATA can be only 
built in. And FPGA_XRT_LIB can be built as module.  They might not be 
built together.

Thanks,
Lizhi
> Tom
>
> > +     select FPGA_BRIDGE
> > +     select FPGA_REGION
> > +     help
> > +       Select this option to enable XRT PCIe driver for Xilinx Alveo 
> > FPGA.
> > +       This driver provides interfaces for userspace application to 
> > access
> > +       Alveo FPGA device.
> > diff --git a/drivers/fpga/xrt/mgmt/Makefile 
> > b/drivers/fpga/xrt/mgmt/Makefile
> > new file mode 100644
> > index 000000000000..acabd811f3fd
> > --- /dev/null
> > +++ b/drivers/fpga/xrt/mgmt/Makefile
> > @@ -0,0 +1,19 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright (C) 2020-2021 Xilinx, Inc. All rights reserved.
> > +#
> > +# Authors: Sonal.Santan@xxxxxxxxxx
> > +#
> > +
> > +FULL_XRT_PATH=$(srctree)/$(src)/..
> > +FULL_DTC_PATH=$(srctree)/scripts/dtc/libfdt
> > +
> > +obj-$(CONFIG_FPGA_XRT_XMGMT) += xrt-mgmt.o
> > +
> > +xrt-mgmt-objs := root.o              \
> > +        main.o               \
> > +        fmgr-drv.o           \
> > +        main-region.o
> > +
> > +ccflags-y := -I$(FULL_XRT_PATH)/include              \
> > +     -I$(FULL_DTC_PATH)