Re: [PATCH V1 RESEND 2/4] Documentation: devicetree: bindings: add binding for PCIe endpoint bus

[Lizhi Hou <lizhi.hou@xilinx.com>]

Hi Rob,


Do you have any comment on this? We are looking for guidance on the 
approaches suggested.


Thanks,

Lizhi

On 4/22/22 14:57, Lizhi Hou wrote:
> Hi Rob,
>
> On 3/7/22 06:07, Rob Herring wrote:
>> On Sun, Mar 6, 2022 at 9:37 AM Tom Rix <trix@redhat.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@xilinx.com>
>>>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
>>>> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.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
>