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

[Gupta, Nipun]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, October 14, 2022 12:48 PM
> To: Gupta, Nipun <Nipun.Gupta@xxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; rafael@xxxxxxxxxx;
> eric.auger@xxxxxxxxxx; alex.williamson@xxxxxxxxxx; cohuck@xxxxxxxxxx;
> Gupta, Puneet (DCG-ENG) <puneet.gupta@xxxxxxx>;
> song.bao.hua@xxxxxxxxxxxxx; mchehab+huawei@xxxxxxxxxx; maz@xxxxxxxxxx;
> f.fainelli@xxxxxxxxx; jeffrey.l.hugo@xxxxxxxxx; saravanak@xxxxxxxxxx;
> Michael.Srba@xxxxxxxxx; mani@xxxxxxxxxx; yishaih@xxxxxxxxxx;
> jgg@xxxxxxxx; jgg@xxxxxxxxxx; robin.murphy@xxxxxxx; will@xxxxxxxxxx;
> joro@xxxxxxxxxx; masahiroy@xxxxxxxxxx; ndesaulniers@xxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kbuild@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> okaya@xxxxxxxxxx; Anand, Harpreet <harpreet.anand@xxxxxxx>; Agarwal,
> Nikhil <nikhil.agarwal@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>;
> Radovanovic, Aleksandar <aleksandar.radovanovic@xxxxxxx>; git (AMD-Xilinx)
> <git@xxxxxxx>
> Subject: Re: [RFC PATCH v4 2/8] bus/cdx: add the cdx bus driver
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> 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 :)

Sure, will remove.

> 
> > +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.

Okay. We see that most of other busses like amba, fslmc, pci are bool only.
As we have MSI domain as part of this bus, we need to export multiple
symbols to make it work as a module.

> 
> 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.

We will remove this static device here.

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

Sure. Will remove from other places too.

> 
> 
> 
> > +
> > +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.

The 'data' parameter is here because the same reset_cdx_device() API is later used in
device attributes patch where it is passed as an argument to bus_for_each_dev API,
which needs the function to have these two arguments. Agree, that this parameter
should be added as part of the device attribute patch, will be updated for next spin.

Thanks,
Nipun

> 
> 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