Re: [PATCH v6 7/9] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

On Tue, Nov 2, 2021 at 12:00 PM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Fri, Oct 29, 2021 at 04:03:15PM -0400, Jim Quinlan wrote:
> > 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?


>
> 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()?   The problem with this is that our host
bridge needs to be the agent freeing the regulators.  IIRC correctly, when
the regulators were freed by the EP device -- or now the bridge in this case
-- we got panics when doing unbinds.  I will go back and get the details
on this, but I'm wondering if our controller has arcane but necessary
requirements
outside of what a general mechanism could provide.

Thanks,
Jim

>
> Rob