MIME-Version: 1.0
References: <20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com>
 <Y86op9oh5ldrZQyG@google.com> <Y86vm0zDOeb1G3zh@axis.com>
In-Reply-To: <Y86vm0zDOeb1G3zh@axis.com>
From:   Rob Herring <robh@kernel.org>
Date:   Mon, 23 Jan 2023 10:36:06 -0600
Message-ID: <CAL_JsqLHi_1QJ4s28uM1b_Z2f+Nsu_CusAsqsx6vVOCCAPLMgw@mail.gmail.com>
Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver
To:     Vincent Whitchurch <vincent.whitchurch@axis.com>,
        Lizhi Hou <lizhi.hou@amd.com>
Cc:     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>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> On Mon, Jan 23, 2023 at 04:32:55PM +0100, Lee Jones wrote:
> > On Mon, 23 Jan 2023, Vincent Whitchurch 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.
> > >
> > > The driver has no id_table by default; user space needs to provide one
> > > using the new_id mechanism in sysfs.
> >
> > This feels wrong for several reasons.
> >
> > Firstly, I think Greg (Cc:ed) will have something to say about this.
> >
> > Secondly, this driver does literally nothing.
>
> Well, it does do what the commit message says.  If there's another way
> of accomplishing that, I'm all ears.
>
> > Why can't you use of of the other, pre-existing "also register my
> > children" compatibles?
> >
> > See: drivers/bus/simple-pm-bus.c
> >      drivers/of/platform.c
>
> simple-pm-bus registers a platform driver, and drivers/of/platform.c
> works on the platform bus.  The driver added by this patch is a PCI
> driver.  So I don't understand how the files you mention could be used
> here?
>
> In case it helps, the relevant nodes in my UML devicetree look something
> like this:
>
>     virtio@2 {

dtc should complain about this...

>         compatible = "virtio,uml";

Binding?

>         virtio-device-id = <1234>;
>         ranges;
>
>         pci {
>                 #address-cells = <3>;
>                 #size-cells = <2>;
>                 ranges = <0x0000000 0 0 0 0xf0000000 0 0x20000>;
>                 compatible = "virtio,device4d2", "pci";

"pci" is not a valid compatible string.

>                 device_type = "pci";
>                 bus-range = <0 0>;
>
>                 platform_parent: device@0,0 {
>                         compatible = "pci494f,dc8";
>                         reg = <0x00000 0 0 0x0 0x10000>;
>                         ranges;
>
>                         uart@10000 {
>                                 compatible = "google,goldfish-tty";
>                                 reg = <0x00000 0 0x10000 0 0x10000>;

This is not a PCI device, so it shouldn't be using PCI addressing.
'ranges' needs an entry (for each BAR) to translate to just a normal
MMIO bus with 1 or 2 address/size cells. Maybe we want a 'simple-bus'
node for each BAR. The FPGA series needs the same things, but that
aspect hasn't really been addressed as the first issue is populating
the PCI devices dynamically.

The DT address translation code should support all this
(MMIO->PCI->MMIO), but I don't think there's any existing examples. An
example (that I can test) would be great. If the unittest had that
example, I'd be thrilled.

Rob