Re: [RFC PATCH v4 2/8] bus/cdx: add the cdx bus driver

[Greg KH]

On Fri, Oct 14, 2022 at 10:10:43AM +0530, Nipun Gupta wrote:
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -20,6 +20,9 @@ obj-$(CONFIG_INTEL_IXP4XX_EB)	+= intel-ixp4xx-eb.o
>  obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
>  obj-$(CONFIG_MVEBU_MBUS) 	+= mvebu-mbus.o
>  
> +#CDX bus

No need for a comment like this :)

> +obj-$(CONFIG_CDX_BUS)		+= cdx/
> +
>  # Interconnect bus driver for OMAP SoCs.
>  obj-$(CONFIG_OMAP_INTERCONNECT)	+= omap_l3_smx.o omap_l3_noc.o
>  
> diff --git a/drivers/bus/cdx/Kconfig b/drivers/bus/cdx/Kconfig
> new file mode 100644
> index 000000000000..98ec05ad708d
> --- /dev/null
> +++ b/drivers/bus/cdx/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# CDX bus configuration
> +#
> +# Copyright (C) 2022, Advanced Micro Devices, Inc.
> +#
> +
> +config CDX_BUS
> +	bool "CDX Bus driver"
> +	help
> +		Driver to enable CDX Bus infrastructure. CDX bus uses
> +		CDX controller and firmware to scan the FPGA based
> +		devices.

Why bool?  Not as a module?  That's broken.

And "FPGA based devices" means nothing to me, please expand on what this
all is as it is not descriptive at all.

Also list the module name please.



> diff --git a/drivers/bus/cdx/Makefile b/drivers/bus/cdx/Makefile
> new file mode 100644
> index 000000000000..2e8f42611dfc
> --- /dev/null
> +++ b/drivers/bus/cdx/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for CDX
> +#
> +# Copyright (C) 2022, Advanced Micro Devices, Inc.
> +#
> +
> +obj-$(CONFIG_CDX_BUS) += cdx.o
> diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
> new file mode 100644
> index 000000000000..5a366f4ae69c
> --- /dev/null
> +++ b/drivers/bus/cdx/cdx.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CDX bus driver.
> + *
> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> + */
> +
> +/*
> + * Architecture Overview
> + * =====================
> + * CDX is a Hardware Architecture designed for AMD FPGA devices. It
> + * consists of sophisticated mechanism for interaction between FPGA,
> + * Firmware and the APUs (Application CPUs).
> + *
> + * Firmware resides on RPU (Realtime CPUs) which interacts with
> + * the FPGA program manager and the APUs. The RPU provides memory-mapped
> + * interface (RPU if) which is used to communicate with APUs.
> + *
> + * The diagram below shows an overview of the CDX architecture:
> + *
> + *          +--------------------------------------+
> + *          |    Application CPUs (APU)            |
> + *          |                                      |
> + *          |                    CDX device drivers|
> + *          |     Linux OS                |        |
> + *          |                        CDX bus       |
> + *          |                             |        |
> + *          |                     CDX controller   |
> + *          |                             |        |
> + *          +-----------------------------|--------+
> + *                                        | (discover, config,
> + *                                        |  reset, rescan)
> + *                                        |
> + *          +------------------------| RPU if |----+
> + *          |                             |        |
> + *          |                             V        |
> + *          |          Realtime CPUs (RPU)         |
> + *          |                                      |
> + *          +--------------------------------------+
> + *                                |
> + *          +---------------------|----------------+
> + *          |  FPGA               |                |
> + *          |      +-----------------------+       |
> + *          |      |           |           |       |
> + *          | +-------+    +-------+   +-------+   |
> + *          | | dev 1 |    | dev 2 |   | dev 3 |   |
> + *          | +-------+    +-------+   +-------+   |
> + *          +--------------------------------------+
> + *
> + * The RPU firmware extracts the device information from the loaded FPGA
> + * image and implements a mechanism that allows the APU drivers to
> + * enumerate such devices (device personality and resource details) via
> + * a dedicated communication channel. RPU mediates operations such as
> + * discover, reset and rescan of the FPGA devices for the APU. This is
> + * done using memory mapped interface provided by the RPU to APU.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/cdx/cdx_bus.h>
> +
> +#include "cdx.h"
> +
> +/*
> + * Default DMA mask for devices on a CDX bus
> + */
> +#define CDX_DEFAULT_DMA_MASK	(~0ULL)
> +
> +static struct cdx_controller_t *cdx_controller;

You don't get a static device, sorry, everything must be dynamic or your
design is broken.

And remove the crazy "_t" from your structure name, checkpatch should
have complained about that.



> +
> +static int reset_cdx_device(struct device *dev, void * __always_unused data)

__always_unused is never needed.

But the funny thing is, you added that variable, why are you not using
it?  If it's not used, then don't have it.

Try to get others at AMD to review your code first, before sending your
next batch out, this is really really odd...

greg k-h