Date:   Fri, 22 Apr 2022 14:57:16 -0700
From:   Lizhi Hou <lizhi.hou@...inx.com>
To:     Rob Herring <robh@...nel.org>, Tom Rix <trix@...hat.com>
CC:     PCI <linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Xu Yilun <yilun.xu@...el.com>, Max Zhen <maxz@...inx.com>,
        Sonal Santan <sonal.santan@...inx.com>,
        Yu Liu <larryliu@....com>,
        Michal Simek <michal.simek@...inx.com>,
        Stefano Stabellini <stefanos@...inx.com>,
        Moritz Fischer <mdf@...nel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Max Zhen <max.zhen@...inx.com>
Subject: Re: [PATCH V1 RESEND 2/4] Documentation: devicetree: bindings: add
 binding for PCIe endpoint bus

Hi Rob,

On 3/7/22 06:07, Rob Herring wrote:
> On Sun, Mar 6, 2022 at 9:37 AM Tom Rix <trix@...hat.com> wrote:
>> Lizhi,
>>
>> Sorry for the delay, I am fighting with checking this with 'make
>> dt_binding_check'
>>
>> There is a recent failure in linux-next around display/mediatek,*
>> between next-20220301 and next-20220302 that I am bisecting.
> There's already patches for that posted.
>
> Just use 'make -k'.
>
>> There are a couple of checkpatch --strict warnings for this set, the
>> obvious one is adding to the MAINTAINERS for new files.
>>
>> Tom
>>
>> On 3/4/22 9:23 PM, Lizhi Hou wrote:
>>> Create device tree binding document for PCIe endpoint bus.
>>>
>>> Signed-off-by: Sonal Santan <sonal.santan@...inx.com>
>>> Signed-off-by: Max Zhen <max.zhen@...inx.com>
>>> Signed-off-by: Lizhi Hou <lizhi.hou@...inx.com>
>>> ---
>>>    .../devicetree/bindings/bus/pci-ep-bus.yaml   | 72 +++++++++++++++++++
>>>    1 file changed, 72 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/bus/pci-ep-bus.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/bus/pci-ep-bus.yaml b/Documentation/devicetree/bindings/bus/pci-ep-bus.yaml
>>> new file mode 100644
>>> index 000000000000..0ca96298db6f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/pci-ep-bus.yaml
>>> @@ -0,0 +1,72 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/bus/pci-ep-bus.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: PCIe Endpoint Bus binding
>>> +
>>> +description: |
>>> +  PCIe device may use flattened device tree to describe apertures in its
>>> +  PCIe BARs. The Bus PCIe endpoint node is created and attached under the
>>> +  device tree root node for this kind of device. Then the flatten device
>>> +  tree overlay for this device is attached under the endpoint node.
>>> +
>>> +  The aperture address which is under the endpoint node consists of BAR
>>> +  index and offset. It uses the following encoding:
>>> +
>>> +    0xIooooooo 0xoooooooo
>>> +
>>> +  Where:
>>> +
>>> +    I = BAR index
>>> +    oooooo oooooooo = BAR offset
>>> +
>>> +  The endpoint is compatible with 'simple-bus' and contains 'ranges'
>>> +  property for translating aperture address to CPU address.
>
> This binding is completely confusing because 'PCIe endpoint' is
> generally used (in context of bindings and Linux) for describing the
> endpoint's system (i.e. the internal structure of a PCIe device (e.g.
> add-in card) from the view of its own processor (not the host
> system)). This binding seems to be describing the host system's view
> of a PCIe device. We already have that! That's just the PCI bus
> binding[1] which has existed for ~25 years.
>
> For a non-DT system, what you are going to need here is some way to
> create DT nodes of the PCI bus hierarchy or at least from your device
> back up to the host bridge. I would suggest you solve that problem
> separately from implementing the FPGA driver by making it work first
> on a DT based system.
>
> Rob
>
> [1] https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
>
>
I investigated the implementation detail for adding device tree node for
PCIe devices. Based on my findings this is quite involved and so would
like to bounce off my approach with you before I start making changes.

We will start with DT-Base system which already has device node for PCIe
controller in base device tree. And we will focus on:

- Adding functions to generate device tree node for all PCIe devices.
- Linking dynamically generated DT nodes to the PCIe to the PCIe devices.

So the first question to resolve is when the PCIe DT node will be created
and destroyed. Here are the different options:

- Option #1: Add functions in pci_bus_add_device()/pci_stop_dev()
   - The same place for creating/destroying device sysfs node. This should
     be able to handle different cases: SR-IOV vf, hotplug, virtual device
     etc.
   - Leverage existing PCI enumeration and get the device information
     directly from pci_dev structure.

-  Option #2: Enumerate PCIe devices and create DT node without relying
    on PCI subsystem enumeration.
   - E.g. Enumerating and creating PCIe dt node in an init callback
     pure_initcall()
   - Linking dt node to PCIe device is already supported in
     pci_setup_device()
   - Eventually need to handle hotplug case separately.

Option #1 looks an easier and cleaner way to implement.

The second question is for linking the dynamic generated dt node to PCIe
device through pci_dev->dev.of_node. The biggest concern is that current
kernel and driver code may check of_node pointer and run complete different
code path if of_node is not NULL. Here are some examples.

  - of_irq_parse_pci(): 
https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/pci/of.c#L492
  - pci_msi_domain_get_msi_rid(): 
https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/pci/msi/irqdomain.c#L233
  - pci_dma_configure(): 
https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/pci/pci-driver.c#L1610

It needs to identify all the places which use of_node and make sure using
dynamic generated of_node is equivalent with the code without using
of_node. Considering there are so many hardware and virtualization Linux
support, the risk might high for changing the code which has been existing
for long time. And how to validate the change could be another challenge.
Introducing compiling option (e.g. CONFIG_PCI_DT_NODE) may lower the risk.

There are other approaches for linking dt node to PCIe device.
- Option #a: Add a special flag or property to dynamic generated PCIe DT
   node. Then convert the code.

       if (pdev->dev.of_node)
           funcA();
       else
           funcB();

    to
       struct device_node *pci_get_of_node(pdev)
       {
           if (pdev->dev.of_node is dynamic generated)
               return NULL;
           return pdev->dev.of_node;
       }

       if (pci_get_of_node(pdev))
           funcA ();
       else
           funcB ();

- Option #b: Introduce a new data member "dyn_of_node" in struct device
   to link the dynamic generated PCIe dt node.

       struct device {
           ...
           of_node: Associated device tree node.
           fwnode: Associated device node supplied by platform firmware.
           dyn_of_node: Associated dynamic generated device tree node.
           ...

Could we implement Option #b for now because it is lower risk?

The last question is about the properties for each dynamic generated
PCIe dt node. To keep the generated device node lightweight, what will be
the desired (minimum) set of properties to generate? Besides the properties
defined in IEEE Std 1275, it looks more properties are needed. E.g.
     interrupt-map, interrupt-map-mask, ...

Thanks,
Lizhi

Powered by blists - more mailing lists