Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

[Rob Herring]

On Wed, Nov 3, 2021 at 1:49 PM Jim Quinlan <jim2101024@xxxxxxxxx> 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.
>
> Signed-off-by: Jim Quinlan <jim2101024@xxxxxxxxx>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 156 +++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index ba4d6daf312c..aaf6a4cbeb78 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -24,6 +24,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/printk.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> @@ -191,6 +192,15 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
>  static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
>  static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
>  static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
> +static int brcm_pcie_add_bus(struct pci_bus *bus);
> +static void brcm_pcie_remove_bus(struct pci_bus *bus);
> +static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
> +
> +static const char * const supplies[] = {
> +       "vpcie3v3",
> +       "vpcie3v3aux",
> +       "vpcie12v",

Common DT properties, so they should be in common DT code.

> +};
>
>  enum {
>         RGR1_SW_INIT_1,
> @@ -295,8 +305,38 @@ struct brcm_pcie {
>         u32                     hw_rev;
>         void                    (*perst_set)(struct brcm_pcie *pcie, u32 val);
>         void                    (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> +       struct regulator_bulk_data supplies[ARRAY_SIZE(supplies)];
> +       unsigned int            num_supplies;

Humm, this will need to be stored somewhere, but the host bridge is
not the right place. That doesn't scale to more than 1 bridge/bus. I'm
not exactly sure where though. pci_bus.self->sys_data,
pci_bus.self->dev.driver_data, pci_bus->sysdata,
pci_bus->dev.driver_data are possible options. Bjorn?

However, given suspend/resume hooks are also needed, maybe
portdrv_pci.c driver is the better spot for all this? The host bridge
wouldn't be in control of opting in, but presence of a DT node ptr for
the port device may be sufficient.

Rob