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

[Rob Herring]

On Tue, Nov 2, 2021 at 5:36 PM Jim Quinlan <james.quinlan@xxxxxxxxxxxx> wrote:
>
> 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?

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

> > 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