| Message ID | 20220305052304.726050-3-lizhi.hou@xilinx.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Infrastructure to define apertures in a PCIe device with a flattened device tree | expand |
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 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. > + > +allOf: > + - $ref: /schemas/simple-bus.yaml# > + > +maintainers: > + - Lizhi Hou <lizhi.hou@xilinx.com> > + > +properties: > + compatible: > + contains: > + const: pci-ep-bus > + > + "#address-cells": > + const: 2 > + > + "#size-cells": > + const: 2 > + > + ranges: true > + > +patternProperties: > + "^.*@[0-9a-f]+$": > + description: hardware apertures belong to this device. > + type: object > + > +required: > + - compatible > + - "#address-cells" > + - "#size-cells" > + - ranges > + > +additionalProperties: false > + > +examples: > + - | > + bus { > + #address-cells = <2>; > + #size-cells = <2>; > + pci-ep-bus@e0000000 { > + compatible = "pci-ep-bus", "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges = <0x0 0x0 0x0 0xe0000000 0x0 0x2000000 > + 0x20000000 0x0 0x0 0xe4200000 0x0 0x40000>; > + }; > + };
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
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 >
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. + +allOf: + - $ref: /schemas/simple-bus.yaml# + +maintainers: + - Lizhi Hou <lizhi.hou@xilinx.com> + +properties: + compatible: + contains: + const: pci-ep-bus + + "#address-cells": + const: 2 + + "#size-cells": + const: 2 + + ranges: true + +patternProperties: + "^.*@[0-9a-f]+$": + description: hardware apertures belong to this device. + type: object + +required: + - compatible + - "#address-cells" + - "#size-cells" + - ranges + +additionalProperties: false + +examples: + - | + bus { + #address-cells = <2>; + #size-cells = <2>; + pci-ep-bus@e0000000 { + compatible = "pci-ep-bus", "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges = <0x0 0x0 0x0 0xe0000000 0x0 0x2000000 + 0x20000000 0x0 0x0 0xe4200000 0x0 0x40000>; + }; + };