On Tue, Nov 2, 2021 at 5:36 PM Jim Quinlan [off-list ref] wrote:

On Tue, Nov 2, 2021 at 12:00 PM Rob Herring [off-list ref] wrote:

quoted

On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote:

quoted

This Broadcom STB PCIe RC driver has one port and connects directly to one
device, be it a switch or an endpoint.  We want to be able to turn on/off
any regulators for that device.  Control of regulators is needed because of
the chicken-and-egg situation: although the regulator is "owned" by the
device and would be best handled by its driver, the device cannot be
discovered and probed unless its regulator is already turned on.

I think this can be done in a much more simple way that avoids the
prior patches using the pci_ops.add_bus() (and remove_bus()) hook.
add_bus is called before the core scans a child bus. In the handler, you
just need to get the bridge device, then the bridge DT node, and then
get the regulators and enable.

Hi Rob,
In reply to my bindings commit you wanted to put the "xxx-supply"
property(s) under the
bridge node rather than under the pci-ep node.   This not only makes
sense but also removes
the burden of prematurely creating the struct device *ptr as the
bridge device has
already been created.

However, there is still an issue:  if  the pcie-link is not
successful, we want the bus enumeration
to stop and not read the vendor/dev id of the EP.  Our controller has
the disadvantage of causing
an abort when accessing config space when the link is not established.  Other
controllers kindly return 0xffffffff as the data.

Doing something like this gets around the issue:

static struct pci_bus *pci_alloc_child_bus(...)
{
        /* ... */
add_dev:
        /* ... */
        if (child->ops->add_bus) {
                ret = child->ops->add_bus(child);
+               if (ret == -ENOLINK)
+                       return NULL;
                if (WARN_ON(ret < 0))
                        dev_err(&child->dev, "failed to add bus: %d\n", ret);
        }

Is this acceptable?  Other suggestions?

Acceptable yes once we agree on error code to return. I'd just do -ENODEV.

quoted

Given we're talking about standard properties in a standard (bridge)
node, I think the implementation for .add_bus should be common
(drivers/pci/of.c). It doesn't scale to be doing this in every host
bridge driver.

Are you saying that the bridge DT node  should have a property such as
"get-and-turn-on-subdev-regulators;" which would invoke what I'm now
calling brcm_pcie_add_bus()?

No! Define a common function that host drivers can opt in to by
setting their .add_bus() hook to or calling from their own add_bus
function.

Ideally, it would work on Rockchip too as it's the same supplies.
However, that would require some reworking of the link initialization
and PERST handling. As Bjorn has mentioned, all that should be per RP,
not per host bridge anyways. I'm taking it one step further and saying
it should be per PCI bridge. Hikey for example needs PERST handling on
bridges behind a switch.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel