Re: [PATCH v2 2/6] PCI: brcmstb: Split brcm_pcie_setup() into two funcs

[Florian Fainelli]

On 7/20/22 09:18, Rob Herring wrote:
> On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <james.quinlan@xxxxxxxxxxxx> wrote:
>>
>> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>>>
>>> On Tue, Jul 19, 2022 at 09:08:48AM -0400, Jim Quinlan wrote:
>>>> On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>>>>> On Mon, Jul 18, 2022 at 02:56:03PM -0400, Jim Quinlan wrote:
>>>>>> On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>>>>>>> On Sat, Jul 16, 2022 at 06:24:49PM -0400, Jim Quinlan wrote:
>>>>>>>> Currently, the function does the setup for establishing PCIe link-up
>>>>>>>> with the downstream device, and it does the actual link-up as well.
>>>>>>>> The calling sequence is (roughly) the following in the probe:
>>>>>>>>
>>>>>>>> -> brcm_pcie_probe()
>>>>>>>>     -> brcm_pcie_setup();                       /* Set-up and link-up */
>>>>>>>>     -> pci_host_probe(bridge);
>>>>>>>>
>>>>>>>> This commit splits the setup function in two: brcm_pcie_setup(), which only
>>>>>>>> does the set-up, and brcm_pcie_start_link(), which only does the link-up.
>>>>>>>> The reason why we are doing this is to lay a foundation for subsequent
>>>>>>>> commits so that we can turn on any power regulators, as described in the
>>>>>>>> root port's DT node, prior to doing link-up.
>>>>>>>
>>>>>>> All drivers that care about power regulators turn them on before
>>>>>>> link-up, but typically those regulators are described directly under
>>>>>>> the host bridge itself.
>>>>>>
>>>>>> Actually, what you describe is what I proposed with my v1 back in Nov 2020.
>>>>>> The binding commit message said,
>>>>>>
>>>>>>     "Quite similar to the regulator bindings found in
>>>>>>     "rockchip-pcie-host.txt", this allows optional regulators to be
>>>>>>     attached and controlled by the PCIe RC driver."
>>>>>>
>>>>>>> IIUC the difference here is that you have regulators described under
>>>>>>> Root Ports (not the host bridge/Root Complex itself), so you don't
>>>>>>> know about them until you've enumerated the Root Ports.
>>>>>>> brcm_pcie_probe() can't turn them on directly because it doesn't know
>>>>>>> what Root Ports are present and doesn't know about regulators below
>>>>>>> them.
>>>>>>
>>>>>> The reviewer's requested me to move the regulator node(s)
>>>>>> elsewhere, and at some point later it was requested to be placed
>>>>>> under the Root Port driver.  I would love to return them under the
>>>>>> host bridge, just say the word!
>>>>>
>>>>> Actually, I think my understanding is wrong.  Even though the PCI core
>>>>> hasn't enumerated the Root Port as a pci_dev, brcm_pcie_setup() knows
>>>>> about it and should be able to look up the regulators and turn them
>>>>> on.
>>>>
>>>> One can do this with
>>>>         regulator_bulk_get(NULL, ...);
>>>>
>>>> However, MarkB did not like the idea of a driver getting the
>>>> regulator from the global DT namespace [1].
>>>>
>>>> For the RC driver, one  cannot invoke  regulator_bulk_get(dev, ...)
>>>> if there is not a direct child regulator node.  One needs to use the
>>>> Port driver device.  The Port driver device does not exist at this
>>>> point unless one tries to prematurely create it; I tried this and it
>>>> was a mess to say the least.
>>>>
>>>>> Can you dig up the previous discussion about why the regulators need
>>>>> to be under the Root Port and why they can't be turned on before
>>>>> calling pci_host_probe()?
>>>>
>>>> RobH did not want the regulators to be under the RC as he said their
>>>> DT location should resemble the HW [2].  The consensus evolved to
>>>> place it under the port driver, which can provide a general
>>>> mechanism for turning on regulators anywhere in the PCIe tree.
>>>
>>> I don't want to redesign this whole thing.  I just want a crisp
>>> rationale for the commit log.
>>>
>>> All other drivers (exynos, imx6, rw-rockchip, histb, qcom, tegra194,
>>> tegra, rockchip-host) have regulators for downstream PCIe power
>>> directly under the RC.  If putting the regulators under an RP instead
>>> is the direction of the future, I guess that might be OK, and I guess
>>> the reasons are:
>>>
>>>   1) Slot or device power regulators that are logically below the RP
>>>      should be described that way in the DT.
>>>
>>>   2) Associating regulators with a RP allows the possibility of
>>>      selectively controlling power to slots/devices below the RP,
>>>      e.g., to power down devices below RP A when suspending while
>>>      leaving wakeup devices below RP B powered up.
>>>
>>> I think in your case the motivating reason is 2).
>>>
>>> Your commit log for "Add mechanism to turn on subdev regulators"
>>> suggests that you want some user control of endpoint power, e.g., via
>>> sysfs, but I don't see that implemented yet except possibly via a
>>> "remove" file that would unbind the driver and remove the entire
>>> device.
>> Hi Bjorn,
>>
>> Initially we wanted to (a) turn on any regulator found under the RC
>> node and (b) allow the possibility of the regulator to control the
>> EP's power. From the feedback, we realized early on that neither of
>> these were going to fly, so we conceded both requests and just wanted
>> to turn on standard PCIe regulators.  Upon reading the aforementioned
>> commit message I realize that there are a couple of leftover sentences
>> from these early days that must be removed.
>>
>> I think when I submitted v1 of the original series that only the
>> rockchip driver had regulators under the RC.   And my recollection was
>> that this was accepted but there was apprehension of this turning into
>> the "standard" way of turning on such regulators,  as the location of
>> the regulator nodes was in question.
>>
>> In short, I would be quite content  to follow the existing examples.
> 
> The existing examples are, in general, incomplete and only work for
> the simplest cases.
> 
> There's 2 cases to consider here. The first is standard slots with
> standard PCIe signals (e.g. PERST#) and voltage rails. The 2nd is
> either non-standard slots or just soldered down devices which could
> have any number of device specific resources. In the latter case,
> those resources need to go into the node for the device. For the
> former case (which we are discussing here), putting the resources in
> the upstream (side of the link) node is fine. That's the root port
> node(s) or switch port nodes. However, since most host bridges are a
> single RP and don't put the RP node in DT, we've ended up with these
> properties in host bridge nodes. That's fine as long as it's a single
> RP and device. When it is not, we need to do something different. The
> only way this scales is putting resources in the port nodes as those
> are what have a 1:1 relationship to slots. If that's supported, then
> the simple cases are also easily supported with if the resources are
> not found in the port node/device, then look for them in the parent
> node. That's also the path for how we get the handling of PERST out of
> every host bridge driver.

This has me confused now, are you suggesting that we pursue what Jim has put together here as a series which describes the regulators in the PCIe end-point device DT node, or that given that we have a single RC single RP configuration it is somewhat acceptable to describe regulators in the PCIe RC node?
-- 
Florian