Messages in this thread | | | Date | Fri, 13 May 2022 08:19:44 -0700 | Subject | Re: [PATCH V1 RESEND 2/4] Documentation: devicetree: bindings: add binding for PCIe endpoint bus | From | Lizhi Hou <> |
| |
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 >
| |