linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lizhi Hou <lizhi.hou@xilinx.com>
To: Rob Herring <robh@kernel.org>, Tom Rix <trix@redhat.com>
Cc: PCI <linux-pci@vger.kernel.org>, <devicetree@vger.kernel.org>,
	Xu Yilun <yilun.xu@intel.com>, Max Zhen <maxz@xilinx.com>,
	Sonal Santan <sonal.santan@xilinx.com>, Yu Liu <larryliu@amd.com>,
	Michal Simek <michal.simek@xilinx.com>,
	Stefano Stabellini <stefanos@xilinx.com>,
	Moritz Fischer <mdf@kernel.org>,
	David Woodhouse <dwmw2@infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Max Zhen <max.zhen@xilinx.com>
Subject: Re: [PATCH V1 RESEND 2/4] Documentation: devicetree: bindings: add binding for PCIe endpoint bus
Date: Fri, 13 May 2022 08:19:44 -0700	[thread overview]
Message-ID: <62e98217-e068-35fc-6809-11d75f4c7933@xilinx.com> (raw)
In-Reply-To: <79e4c876-e5a4-861f-cfbc-c75ed1a07ddd@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
>

  reply	other threads:[~2022-05-13 15:20 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-05  5:23 [PATCH V1 RESEND 0/4] Infrastructure to define apertures in a PCIe device with a flattened device tree Lizhi Hou
2022-03-05  5:23 ` [PATCH V1 RESEND 1/4] pci: add interface to create pci-ep device tree node Lizhi Hou
2022-03-10 10:02   ` Dan Carpenter
2022-03-10 19:34   ` Bjorn Helgaas
2022-06-21 15:12   ` Manivannan Sadhasivam
2022-03-05  5:23 ` [PATCH V1 RESEND 2/4] Documentation: devicetree: bindings: add binding for PCIe endpoint bus Lizhi Hou
2022-03-06 15:37   ` Tom Rix
2022-03-07 14:07     ` Rob Herring
2022-04-22 21:57       ` Lizhi Hou
2022-05-13 15:19         ` Lizhi Hou [this message]
2022-06-21 15:06   ` Manivannan Sadhasivam
2022-03-05  5:23 ` [PATCH V1 RESEND 3/4] fpga: xrt: management physical function driver Lizhi Hou
2022-06-21 15:16   ` Manivannan Sadhasivam
2022-03-05  5:23 ` [PATCH V1 RESEND 4/4] of: enhance overlay applying interface to specific target base node Lizhi Hou
2022-03-10 20:07   ` Rob Herring
2022-03-10 19:27 ` [PATCH V1 RESEND 0/4] Infrastructure to define apertures in a PCIe device with a flattened device tree Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=62e98217-e068-35fc-6809-11d75f4c7933@xilinx.com \
    --to=lizhi.hou@xilinx.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw2@infradead.org \
    --cc=larryliu@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=max.zhen@xilinx.com \
    --cc=maxz@xilinx.com \
    --cc=mdf@kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=robh@kernel.org \
    --cc=sonal.santan@xilinx.com \
    --cc=stefanos@xilinx.com \
    --cc=trix@redhat.com \
    --cc=yilun.xu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).