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

Rob Herring
2021-11-04T15:15:55Z

On Wed, Nov 3, 2021 at 1:49 PM Jim Quinlan <jim2101024@gmail.com> 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@gmail.com>
> ---
> 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