Re: [PATCH 2/9] PCI: host: brcmstb: add DT docs for Brcmstb PCIe device

[Rob Herring]

On Fri, Oct 20, 2017 at 12:27 PM, Brian Norris
<computersforpeace@xxxxxxxxx> wrote:
> Hi,
>
> On Thu, Oct 19, 2017 at 02:58:55PM -0700, Florian Fainelli wrote:
>> On 10/19/2017 02:49 PM, Rob Herring wrote:
>> > On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024@xxxxxxxxx> wrote:
>> >> On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
>> >>> This is not the regulator binding. Use the standard binding.
>> >>>
>> >> The reason we do it this way is because the PCIe controller does not
>> >> know or care what the names of the supplies are, or how many there
>> >> will be.  The list of regulators can be different for each board we
>> >> support, as these regulators turn on/off the downstream EP device.
>> >> All the PCIe controller does is turn on/off this list of regulators
>> >> when booting,resuming/suspending.
>> >>
>> >> An alternative would have the node specifying the standard properties
>> >>
>> >> xyz-supply = <&xyz_reg>;
>> >> abc-supply = <&abc_reg>;
>> >> pdq-supply = <&pdq_reg>;
>> >> ...
>> >>
>> >> and then have this driver search all of the properties in the PCIe
>> >> node for names matching /-supply$/, and then create a list of phandles
>> >> from that.  Is that what you would like?
>> >
>> > Really, you should have child nodes of the PCIe devices and have the
>> > supplies in there.
>>
>> While that would be technically more correct this poses a number of issues:
>>
>> - there is precedence in that area, and you already Acked binding
>> documents proposing the same thing:
>>       * Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt (commit
>> c26ebe98a103479dae9284fe0a86a95af4a5cd46)
>>       * Documentation/devicetree/bindings/pci/rockchip-pcie.txt (commit
>> 828bdcfbdb98eeb97facb05fe6c96ba0aed65c4a and before)
>
> I can actually speak to the Rockchip binding a bit :)
>
> It actually has a mixture of true controller regulators (e.g., the 1.8V
> and 0.9V regulators power analog portions of the SoC's PCIe logic) and
> controls for the PCIe endpoints (e.g., 12V). Additionally, some of these
> have been misused to represent a little of both, since the regulator
> framework is actually quite flexible ;)
>
> That may or may not help, but they are at least partially correct.
>
> The Freescale one does look like it's plainly *not* about the root
> controller.

Maybe not. I don't find that to be obvious reading the binding.

> Also, those rails *are* all fairly well defined by the various PCIe
> electromechanical specifications, so isn't it reasonable to expect that
> a controller can optionally control power for 1 of each of the standard
> power types?

That seems okay. The MMC binding is done that way.

I never said you can't just put things in the host node. That's just
not an ideal match to the h/w and there's a limit to what makes sense.

> Or do we really want to represent the flexibility that
> there can be up to N of each type for N endpoints?

No, then you need to describe the topology.

> As a side note: it's also been pretty tough to get the power sequencing
> requirements represented correctly for some PCIe endpoints. I've tried
> to make use of this previously, but the series took so long (>1 year and
> counting!) my team lost interest:
>
> [PATCH v16 2/7] power: add power sequence library
> https://www.spinics.net/lists/linux-usb/msg158452.html
>
> It doesn't really solve all of this problem, but it could be worth
> looking at.
>
>> (which may indicate those should also be corrected, at the possible
>> expense of creating incompatibilities)
>
> If a better way is developed, we can always augment the bindings. The
> current bindings aren't that hard to maintain as "deprecated backups."
>
>> - there is a chicken and egg problem, you can't detect the EP without
>> turning its regulator on, and if you can't create a pci_device, you
>> certainly won't be able to associate it with an device_node counterpart
>
> That's definitely a major problem driving some of the current bindings.
> We're just not set up to walk the child devices if we can't probe them.

It's common to all the probeable buses that still need DT additions.

>> - PCIe being dynamic by nature, can you have "wildcard" PCIE EP DT node
>> that would be guaranteed to match any physically attached PCIE EP which
>> is discovered by the PCI bus layer (and then back to issue #2)
>
> Technically, you *can* walk the DT (i.e., 'device_node's) without having
> pci_dev's for each yet. Something like of_pci_find_child_device() would
> do it. But that still seems kind of wonky, and it's currently neither
> precise enough nor flexible enough for this, I don't think.

That's essentially what the generic power seq stuff tries to do. I
said early on we need some sort of pre-probe hook for drivers. Trying
to handle things generically only gets you so far. There will always
be that special case that needs specific handling.

Rob