Date:   Tue, 24 Jan 2023 13:54:57 +0100
From:   Vincent Whitchurch <vincent.whitchurch@axis.com>
To:     Rob Herring <robh@kernel.org>
CC:     Lizhi Hou <lizhi.hou@amd.com>, Lee Jones <lee@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kernel <kernel@axis.com>, <gregkh@linuxfoundation.org>
Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver
Message-ID: <Y8/VIXPDcNcj/wxT@axis.com>
References: <20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com>
 <CAL_JsqKKJn3iuHu-Q5XTknCbAW1gt1BmF0w4Gzfcq2S5mv0gZw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAL_JsqKKJn3iuHu-Q5XTknCbAW1gt1BmF0w4Gzfcq2S5mv0gZw@mail.gmail.com>
Precedence: bulk

On Mon, Jan 23, 2023 at 05:13:06PM +0100, Rob Herring wrote:
> On Mon, Jan 23, 2023 at 8:32 AM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
> >
> > Add a PCI driver which registers all child nodes specified in the
> > devicetree.  It will allow platform devices to be used on virtual
> > systems which already support PCI and devicetree, such as UML with
> > virt-pci.
> 
> There's similar work underway for Xilinx/AMD PCIe FPGAs[1]. It's the
> same thing really. Non-discoverable things downstream of a PCI device.
> There's also a desire for that to work on non-DT (ACPI) based hosts.
> While UML supports DT, that's currently only for the unittest AFAIK.

It's possible to pass a devicetree blob to UML via a command line
argument[0].  The roadtest[1][2] framework uses this to test device
drivers.

[0] https://lore.kernel.org/lkml/20211208151123.29313-3-vincent.whitchurch@axis.com/
[1] https://lore.kernel.org/lkml/20220311162445.346685-1-vincent.whitchurch@axis.com/
[2] https://lwn.net/Articles/887974/

> So it's more like a non-DT host. How does the DT get populated for UML
> for this to work?

The dts is generated by the test framework based on the test cases being
run (see the files being patched in [3]) and is compiled and passed to
UML via the command line argument.

> Can you provide details on the actual h/w you want to use. What
> problem are you trying to solve?

There is no real hardware.  I'm using this to add support for platform
devices to roadtest.  As the commit message said, UML supports PCI but I
want to test platform devices so I just need something to allow me to
put arbitrary platform devices under the PCI device and have them get
probed.

The PCI "hardware" (in backend.c in [3]) is just enough implementation
of the BARs to keep Linux happy and forward the register accesses to the
platform hardware implementation which is in Python as part of the test
cases (eg. test_platform.py in [3]).  See my WIP patch for platform
device support to roadtest which includes a test for the goldfish UART:

[3] https://github.com/vwax/linux/commit/636f4150b086dc581fdfb464869eb98b8a22a254

(The roadtest code is placed in a kernel tree but the only patches to
the kernel proper are this one,
https://lore.kernel.org/lkml/20230120-uml-pci-of-v1-1-134fb66643d8@axis.com/,
and a couple of ongoing fixes at the top of the tree.  Roadtest is
designed to work on unpatched mainline kernels.)

> Really, what I want to see here is everyone interested in this feature
> to work together on it. Not just creating a one-off solution for their
> 1 use case that's a subset of a bigger solution.
> 
> > The driver has no id_table by default; user space needs to provide one
> > using the new_id mechanism in sysfs.
> 
> But your DT will have the id in it already. Wouldn't you rather
> everything work without userspace intervention? I can't imagine the
> list here would be too long.

I would be nice for things to work without userspace intervention (see
the change to init.sh in [3]), but I don't have real hardware or real
PCI IDs, and I don't think we would want to hardcode made-up numbers in
the ID table?

> > diff --git a/drivers/mfd/simple-mfd-pci.c b/drivers/mfd/simple-mfd-pci.c
> > new file mode 100644
> > index 000000000000..c5b2540e924a
> > --- /dev/null
> > +++ b/drivers/mfd/simple-mfd-pci.c
> > @@ -0,0 +1,21 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pci.h>
> > +
> > +static int simple_mfd_pci_probe(struct pci_dev *pdev,
> > +                               const struct pci_device_id *id)
> > +{
> > +       return devm_of_platform_populate(&pdev->dev);
> 
> Really, this could be anything in the child DT. Not just what Linux
> classifies as an MFD. So maybe drivers/mfd is not the right place.

What would be the right place?  drivers/bus?  Or perhaps something
UML-specific similar to arch/x86/kernel/devicetree.c?