Re: [PATCH V7 0/3] Generate device tree node for pci devices

[Frank Rowand]

On 3/6/23 02:35, clement.leger@xxxxxxxxxxx wrote:
> Le 2023-03-04 00:42, Frank Rowand a écrit :
>> On 2/27/23 04:31, Clément Léger wrote:
>>> Le Mon, 27 Feb 2023 00:51:29 -0600,
>>> Frank Rowand <frowand.list@xxxxxxxxx> a écrit :
>>>
>>>> On 1/19/23 21:02, Lizhi Hou wrote:
>>>>> This patch series introduces OF overlay support for PCI devices 
>>>>> which
>>>>> primarily addresses two use cases. First, it provides a data driven 
>>>>> method
>>>>> to describe hardware peripherals that are present in a PCI endpoint 
>>>>> and
>>>>> hence can be accessed by the PCI host. Second, it allows reuse of a 
>>>>> OF
>>>>> compatible driver -- often used in SoC platforms -- in a PCI host 
>>>>> based
>>>>> system.
>>>>>
>>>>> There are 2 series devices rely on this patch:
>>>>>
>>>>>   1) Xilinx Alveo Accelerator cards (FPGA based device)
>>>>>   2) Microchip LAN9662 Ethernet Controller
>>>>>
>>>>>      Please see: 
>>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@xxxxxxxxxxx/
>>>>>
>>>>
>>>>
>>>>> Normally, the PCI core discovers PCI devices and their BARs using 
>>>>> the
>>>>> PCI enumeration process. However, the process does not provide a way 
>>>>> to
>>>>> discover the hardware peripherals that are present in a PCI device, 
>>>>> and
>>>>> which can be accessed through the PCI BARs. Also, the enumeration 
>>>>> process
>>>>
>>>> I'm confused.  The PCI Configuration Header Registers should describe 
>>>> the
>>>> hardware on the PCI card.
>>>>
>>>> Ignoring case 1 above _for the moment_ (FPGA devices are a world unto
>>>> themselves, so I would like to analyze that case separately), does 
>>>> the
>>>> second device, "Microchip LAN9662 Ethernet Controller" properly 
>>>> implement
>>>> the PCI Configuration Header Registers?  What additional information 
>>>> is
>>>> needed that is not provided in those registers?
>>>
>>> Hi Frank,
>>>
>>> I guess Lizhi wanted to say that it does not provide a way to describe
>>> all the "platform" devices that are exposed by this PCI device. Which
>>> is of course the whole point of the work we are doing right now. But
>>> all the BARs are correctly described by the LAN9662 PCI card.
>>>
>>> Clément
>>
>> I remain confused.
>>
>> [RFC 00/10] add support for fwnode in i2c mux system and sfp
>> https://lore.kernel.org/lkml/YhQHqDJvahgriDZK@xxxxxxx/t/
>>
>>   references a PCIe driver:
>>   [2]
>> https://github.com/clementleger/linux/blob/fwnode_support/drivers/mfd/lan966x_pci_mfd.c
>>
>> So there is a PCIe driver that works.
>>
>> However, the RFC patch series was proposing adding fwnode support to
>> the driver.  My first
>> surface reading (just part of that one email, not the entire series or
>> the replies yet),
>> notes:
>>
>>   ... However, when
>>   plugged in a PCIe slot (on a x86), there is no device-tree support 
>> and
>>   the peripherals that are present must be described in some other way.
>>
>> I am assuming that the peripherals are what you mentioned above as 
>> '"platform"
>> devices'.  This is where my current confusion lies.  Are the "platform"
>> devices accessed via the PCI bus or is there some other electrical 
>> connection
>> between the host system and the PCIe card?
> 
> Hi Frank,
> 
> The platform devices exposed by this PCIe card are available via some 
> BAR using PCI memory mapped areas, so it's totally standard PCI stuff.
> 
>>
>> If the "platform" devices are accessed via the PCI bus, then I would 
>> expect them
>> to be described by PCI configuration header registers.  Are the PCI
>> configuration
>> registers to describe the "platform" devices not present?
> 
> I'm not sure to understand what you mean here. PCI configuration headers
> only provides some basic registers allowing to identify the PCI device
> (vendor/product) and some memory areas that are exposed (BAR). They do
> not provides the "list" of peripherals that are exposed by the devices,
> only some BARs that can be mapped and that allows to access.

Yes, "identify the PCI device (vendor/product) and some memory areas".
The driver for the (vendor/product) 'knows' what peripherals are exposed
by the device and where within the BAR to find the registers for each
of the devices.

A normal PCI driver would contain this information.  If I understand the
proposal of this patch series, of_pci_make_dev_node() adds a node to
the devicetree, when invoked via a PCI quirk for certain specific
vendor/product cards.  This node must exist for the flattened device
tree (FDT) overlay for the card to be loaded.  The driver for the card
will get the overlay FDT from the card and load it into the kernel.
The driver will use the information that then exists in the devicetree
describing the card, instead of using information from the PCI configuration
headers from the card.

The intent is to be able to re-use devicetree based drivers instead of
having the driver be a native PCI driver.

This goes against historical Linux practice.  The idea of taking a driver
from another environment (eg Windows, HP Unix, Sun Unix, IBM Unix, etc)
and adding a shim layer to translate between Linux and the other
environment has been rejected.  Ironically, in this case, the other
environment is Linux (more specifically the Linux OF implementation).
Even thought the other environment is Linux, this is still adding a
shim layer to translate between that other environment and the native
Linux PCI environment for which the driver would normally be written.

In other words, this is not acceptable.  Normal alternatives would be
something like (1) add the PCI awareness to the existing drivers,
(2) split the devicetree aware and PCI aware portions of the driver
to common code that would be invoked from separate devicetree and PCI
drivers, (3) write entirely separate devicetree and PCI drivers, or
(4) some other creative solution.

Am I mis-interpretting or misunderstanding anything crucial here?

-Frank

> 
> In the case of the lan9662 cnetwork controller, BAR 0 and 1 exposes
> multiples devices that are located at some subranges of this BAR. For
> instance (not accurate), we have the I2C controller located at BAR 0
> + offset 0X1000, then the flexcom controller exposed in BAR 0 at offset
>   0x20000, etc. This list of peripheral is not exposed at all by the PCI
> configuration headers (since it is not the purpose of course). All of
> these peripherals have already existing platform drivers which can then
> be reused thanks to the PCI device-tree overlay series.
> 
>>
>> I'll read through the fwnode RFC thread to add to see what happened to
>> the proposal.
> 
> You can probably read the cover letter which described the use case in 
> details. However, don't spend too much time reading the patchset, we
> discarded them for many good reason (way too much modifications in
> subsystems, no standardization of software node bindings, etc).
> 
> Clément
>