Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Florian Fainelli]

On 4/7/2021 4:27 AM, Mark Brown wrote:
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> 
>> I'm a little confused -- here is how I remember the chronology of the
>> "DT bindings" commit reviews, please correct me if I'm wrong:
> 
>> o JimQ submitted a pullreq for using voltage regulators in the same
>> style as the existing "rockport" PCIe driver.
>> o After some deliberation, RobH preferred that the voltage regulators
>> should go into the PCIe subnode device's DT node.
>> o JimQ put the voltage regulators in the subnode device's DT node.
>> o MarkB didn't like the fact that the code did a global search for the
>> regulator since it could not provide the owning struct device* handle.
>> o RobH relented, and said that if it is just two specific and standard
>> voltage regulators, perhaps they can go in the parent DT node after
>> all.
>> o JimQ put the regulators back in the PCIe node.
>> o MarkB now wants the regulators to go back into the child node again?
> 
> ...having pointed out a couple of times now that there's no physical
> requirement that the supplies be shared between slots never mind with
> the controller.  Also note that as I've said depending on what the
> actual requirements of the controller node are you might want to have
> the regulators in both places, and further note that the driver does not
> have to actively use everything in the binding document (although if
> it's not using something that turns out to be a requirement it's likely
> to run into hardware where that causes bugs at some point).
> 
> Frankly I'm not clear why you're trying to handle powering on PCI slots
> in a specific driver, surely PCI devices are PCI devices regardless of
> the controller?

There is no currently a way to deal with that situation since you have a
chicken and egg problem to solve: there is no pci_device created until
you enumerate the PCI bus, and you cannot enumerate the bus and create
those pci_devices unless you power on the slots/PCIe end-points attached
to the root complex. There are precedents like the rockchip PCIe RC
driver, and yes, we should not be cargo culting this, which is why we
are trying to understand what is it that should be done here and there
has been conflicting advice, or rather our interpretation has led to
perceiving it as a conflicting.

If the regulator had a variation where it supported passing a
device_node reference to look up regulator properties, we could solve
this generically for all devices, that does not exist, and you stated
you will not accept changes like those, fair enough.

When you suggested to look at soundwire for an example of "software
devices", we did that but it was not clear where the sdw_slave would be
created prior to sdw_slave_add(), but this does not matter too much.

Let us say we try to solve this generically using the same idea that we
would pre-populate pci_device prior to calling pci_scan_child_bus(). We
could do something along these lines:

- pci_scan_child_bus() would attempt to walk the list of device_node
children from the PCIe root complex's device_node and call
pci_alloc_dev() for each of these devices that it finds, along with
calling device_initialize() and ensuring that pci_dev::device::of_node
is set correctly by calling pci_set_of_node()/set_dev_node(). Finally we
call list_add_tail() with the pci_bus_sem semaphore held to add that
pci_device to bus->devices such that we can later find them

- from there on we try to get the regulators of those pci_device objects
we just allocated and we try to enable their regulators, either based on
a specific list of supplies or just trying to enable all supplied declared.

- now pci_scan_child_bus() will attempt to scan the bus for real by
reading the pci_device objects ID but we already have such objects, so
we need to update pci_scan_device() to search bus::devices and if
nothing is found we allocate one

Is that roughly what you have in mind as to what should be done?
-- 
Florian