| Message ID | 20220716222454.29914-3-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: brcmstb: Re-submit reverted patchset | expand |
Hello! On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote: > @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > if (pcie->gen) > brcm_pcie_set_gen(pcie, pcie->gen); > > + /* Don't advertise L0s capability if 'aspm-no-l0s' */ > + aspm_support = PCIE_LINK_STATE_L1; > + if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) > + aspm_support |= PCIE_LINK_STATE_L0S; > + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > + u32p_replace_bits(&tmp, aspm_support, > + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); > + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > + > + /* > + * For config space accesses on the RC, show the right class for > + * a PCIe-PCIe bridge (the default setting is to be EP mode). > + */ > + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); > + u32p_replace_bits(&tmp, 0x060400, There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it instead of magic constant. I introduced it recently in commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004 > + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); > + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); > + > + return 0; > +}
On Mon, Jul 18, 2022 at 9:11 AM Pali Rohár <pali@kernel.org> wrote: > > Hello! > > On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote: > > @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > if (pcie->gen) > > brcm_pcie_set_gen(pcie, pcie->gen); > > > > + /* Don't advertise L0s capability if 'aspm-no-l0s' */ > > + aspm_support = PCIE_LINK_STATE_L1; > > + if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) > > + aspm_support |= PCIE_LINK_STATE_L0S; > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > > + u32p_replace_bits(&tmp, aspm_support, > > + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > > + > > + /* > > + * For config space accesses on the RC, show the right class for > > + * a PCIe-PCIe bridge (the default setting is to be EP mode). > > + */ > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); > > + u32p_replace_bits(&tmp, 0x060400, > > There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it > instead of magic constant. Will do, thanks. Jim Quinlan Broadcom STB > > I introduced it recently in commit: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004 > > > + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); > > + > > + return 0; > > +}
On Mon, Jul 18, 2022 at 09:37:08AM -0400, Jim Quinlan wrote: > On Mon, Jul 18, 2022 at 9:11 AM Pali Rohár <pali@kernel.org> wrote: > > > > Hello! > > > > On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote: > > > @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > > if (pcie->gen) > > > brcm_pcie_set_gen(pcie, pcie->gen); > > > > > > + /* Don't advertise L0s capability if 'aspm-no-l0s' */ > > > + aspm_support = PCIE_LINK_STATE_L1; > > > + if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) > > > + aspm_support |= PCIE_LINK_STATE_L0S; > > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > > > + u32p_replace_bits(&tmp, aspm_support, > > > + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); > > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > > > + > > > + /* > > > + * For config space accesses on the RC, show the right class for > > > + * a PCIe-PCIe bridge (the default setting is to be EP mode). > > > + */ > > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); > > > + u32p_replace_bits(&tmp, 0x060400, > > > > There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it > > instead of magic constant. > > Will do, thanks. I can fix that up locally. > > I introduced it recently in commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004 > > > > > + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); > > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); > > > + > > > + return 0; > > > +} > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Monday 18 July 2022 12:05:28 Bjorn Helgaas wrote: > On Mon, Jul 18, 2022 at 09:37:08AM -0400, Jim Quinlan wrote: > > On Mon, Jul 18, 2022 at 9:11 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > Hello! > > > > > > On Saturday 16 July 2022 18:24:49 Jim Quinlan wrote: > > > > @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > > > if (pcie->gen) > > > > brcm_pcie_set_gen(pcie, pcie->gen); > > > > > > > > + /* Don't advertise L0s capability if 'aspm-no-l0s' */ > > > > + aspm_support = PCIE_LINK_STATE_L1; > > > > + if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) > > > > + aspm_support |= PCIE_LINK_STATE_L0S; > > > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > > > > + u32p_replace_bits(&tmp, aspm_support, > > > > + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); > > > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); > > > > + > > > > + /* > > > > + * For config space accesses on the RC, show the right class for > > > > + * a PCIe-PCIe bridge (the default setting is to be EP mode). > > > > + */ > > > > + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); > > > > + u32p_replace_bits(&tmp, 0x060400, > > > > > > There is already macro PCI_CLASS_BRIDGE_PCI_NORMAL, so please use it > > > instead of magic constant. > > > > Will do, thanks. > > I can fix that up locally. Great! I did git grep on recent master branch and found another candidates with magic numbers which refers to PCI_CLASS_BRIDGE_PCI_NORMAL: arch/mips/pci/pci-mt7620.c: pcie_w32(0x06040001, RALINK_PCI0_CLASS); arch/mips/pci/pci-rt3883.c: rt3883_pci_w32(rpc, 0x06040001, RT3883_PCI_REG_CLASS(1)); arch/powerpc/platforms/4xx/pci.c: out_le32(mbase + 0x208, 0x06040001); drivers/pci/controller/pcie-brcmstb.c: u32p_replace_bits(&tmp, 0x060400, (class code is stored in upper 24 bits of 32-bit register, so it makes sense that on lowest 8 bits is something more - 0x01) What do you think? Does it make sense to send patch which replace above hex numbers by macros? > > > I introduced it recently in commit: > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=904b10fb189cc15376e9bfce1ef0282e68b0b004 > > > > > > > + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); > > > > + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); > > > > + > > > > + return 0; > > > > +} > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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. 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. So I think brcm_pcie_setup() does initialization that doesn't depend on the link or any downstream devices, and brcm_pcie_start_link() does things that depend on the link being up. Right? If so, "start_link" might be a slight misnomer since AFAICT brcm_pcie_start_link() doesn't do anything to initiate link-up except maybe deasserting fundamental reset. Some drivers start the LTSSM or explicitly enable link training, but brcm_pcie_start_link() doesn't seem to do anything like that. brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does that really depend on the link being up? If that only affects the Root Port, maybe it could be done before link-up? > We do this by defining an > add_bus() callback which is invoked during enumeraion. At the end of this > patchset the probe function trace will look something like this: > > -> brcm_pcie_probe() > -> brcm_pcie_setup(); /* Set-up only */ > -> pci_host_probe(bridge); > -> [enumeration] > -> pci_alloc_child_bus() > -> bus->ops->add_bus(bus); /* We've set this op */ > -> brcm_pcie_add_bus() /* Our callback */ > -> [turn on regulators] /* Main objective! */ > -> brcm_pcie_start_link() /* Link-up */
On Mon, Jul 18, 2022 at 2:14 PM Bjorn Helgaas <helgaas@kernel.org> 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. Hi Bjorn, 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! > > So I think brcm_pcie_setup() does initialization that doesn't depend > on the link or any downstream devices, and brcm_pcie_start_link() does > things that depend on the link being up. Right? Yes. > > If so, "start_link" might be a slight misnomer since AFAICT > brcm_pcie_start_link() doesn't do anything to initiate link-up except > maybe deasserting fundamental reset. Some drivers start the LTSSM or > explicitly enable link training, but brcm_pcie_start_link() doesn't > seem to do anything like that. > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does > that really depend on the link being up? If that only affects the > Root Port, maybe it could be done before link-up? Some of the registers cannot be accessed until after linkup but these do not have that issue. I will change this. Jim Quinlan Broadcom STB > > > We do this by defining an > > add_bus() callback which is invoked during enumeraion. At the end of this > > patchset the probe function trace will look something like this: > > > > -> brcm_pcie_probe() > > -> brcm_pcie_setup(); /* Set-up only */ > > -> pci_host_probe(bridge); > > -> [enumeration] > > -> pci_alloc_child_bus() > > -> bus->ops->add_bus(bus); /* We've set this op */ > > -> brcm_pcie_add_bus() /* Our callback */ > > -> [turn on regulators] /* Main objective! */ > > -> brcm_pcie_start_link() /* Link-up */
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@kernel.org> 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! I'm not suggesting a change in that design; I'm only trying to understand and clarify the commit log. I looked briefly for the suggestion to put the regulators under the Root Port instead of the host bridge, but didn't find it. I don't know enough to have an opinion yet. > > So I think brcm_pcie_setup() does initialization that doesn't depend > > on the link or any downstream devices, and brcm_pcie_start_link() does > > things that depend on the link being up. Right? > > Yes. > > > If so, "start_link" might be a slight misnomer since AFAICT > > brcm_pcie_start_link() doesn't do anything to initiate link-up except > > maybe deasserting fundamental reset. Some drivers start the LTSSM or > > explicitly enable link training, but brcm_pcie_start_link() doesn't > > seem to do anything like that. > > > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does > > that really depend on the link being up? If that only affects the > > Root Port, maybe it could be done before link-up? > > Some of the registers cannot be accessed until after linkup but these do > not have that issue. I will change this. Here's my attempt (assuming we don't change the DT regulator design): diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index bd88a0a46c63..70cad1cbcbb4 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -852,14 +852,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie); u64 rc_bar2_offset, rc_bar2_size; void __iomem *base = pcie->base; - struct device *dev = pcie->dev; + int ret, memc; + u32 tmp, burst, aspm_support; struct resource_entry *entry; - bool ssc_good = false; struct resource *res; int num_out_wins = 0; - u16 nlw, cls, lnksta; - int i, ret, memc; - u32 tmp, burst, aspm_support; /* Reset the bridge */ pcie->bridge_sw_init_set(pcie, 1); @@ -948,25 +945,23 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) if (pcie->gen) brcm_pcie_set_gen(pcie, pcie->gen); - /* Unassert the fundamental reset */ - pcie->perst_set(pcie, 0); + /* Don't advertise L0s capability if 'aspm-no-l0s' */ + aspm_support = PCIE_LINK_STATE_L1; + if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) + aspm_support |= PCIE_LINK_STATE_L0S; + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); + u32p_replace_bits(&tmp, aspm_support, + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); /* - * Give the RC/EP time to wake up, before trying to configure RC. - * Intermittently check status for link-up, up to a total of 100ms. + * For config space accesses on the RC, show the right class for + * a PCIe-PCIe bridge (the default setting is to be EP mode). */ - for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5) - msleep(5); - - if (!brcm_pcie_link_up(pcie)) { - dev_err(dev, "link down\n"); - return -ENODEV; - } - - if (!brcm_pcie_rc_mode(pcie)) { - dev_err(dev, "PCIe misconfigured; is in EP mode\n"); - return -EINVAL; - } + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); + u32p_replace_bits(&tmp, PCI_CLASS_BRIDGE_PCI_NORMAL, + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); resource_list_for_each_entry(entry, &bridge->windows) { res = entry->res; @@ -998,23 +993,37 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) num_out_wins++; } - /* Don't advertise L0s capability if 'aspm-no-l0s' */ - aspm_support = PCIE_LINK_STATE_L1; - if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) - aspm_support |= PCIE_LINK_STATE_L0S; - tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); - u32p_replace_bits(&tmp, aspm_support, - PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); - writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); + return 0; +} + +static int brcm_pcie_start_link(struct brcm_pcie *pcie) +{ + struct device *dev = pcie->dev; + void __iomem *base = pcie->base; + u16 nlw, cls, lnksta; + bool ssc_good = false; + u32 tmp; + int ret, i; + + /* Unassert the fundamental reset */ + pcie->perst_set(pcie, 0); /* - * For config space accesses on the RC, show the right class for - * a PCIe-PCIe bridge (the default setting is to be EP mode). + * Give the RC/EP time to wake up, before trying to configure RC. + * Intermittently check status for link-up, up to a total of 100ms. */ - tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); - u32p_replace_bits(&tmp, 0x060400, - PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); - writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); + for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5) + msleep(5); + + if (!brcm_pcie_link_up(pcie)) { + dev_err(dev, "link down\n"); + return -ENODEV; + } + + if (!brcm_pcie_rc_mode(pcie)) { + dev_err(dev, "PCIe misconfigured; is in EP mode\n"); + return -EINVAL; + } if (pcie->ssc) { ret = brcm_pcie_set_ssc(pcie); @@ -1204,6 +1213,10 @@ static int brcm_pcie_resume(struct device *dev) if (ret) goto err_reset; + ret = brcm_pcie_start_link(pcie); + if (ret) + goto err_reset; + if (pcie->msi) brcm_msi_set_regs(pcie->msi); @@ -1393,6 +1406,10 @@ static int brcm_pcie_probe(struct platform_device *pdev) if (ret) goto fail; + ret = brcm_pcie_start_link(pcie); + if (ret) + goto fail; + pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION); if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) { dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
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@kernel.org> 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. 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()? Bjorn
On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote: > ... > So I think brcm_pcie_setup() does initialization that doesn't depend > on the link or any downstream devices, and brcm_pcie_start_link() does > things that depend on the link being up. Right? > > If so, "start_link" might be a slight misnomer since AFAICT > brcm_pcie_start_link() doesn't do anything to initiate link-up except > maybe deasserting fundamental reset. Some drivers start the LTSSM or > explicitly enable link training, but brcm_pcie_start_link() doesn't > seem to do anything like that. > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does > that really depend on the link being up? If that only affects the > Root Port, maybe it could be done before link-up? What about the /* PCIe->SCB endian mode for BAR */ thing? Does that depend on the link being up? And the "Refclk from RC should be gated with CLKREQ#" part? Does that depend on the link being up? It seems obvious that brcm_pcie_set_ssc() and reading the negotiated link speed and width depend on the link being up.
On Mon, Jul 18, 2022 at 6:40 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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. Regards, Jim Quinlan Broadcom STB [1] https://lore.kernel.org/linux-pci/20210329162539.GG5166@sirena.org.uk/ [2] https://lore.kernel.org/linux-pci/CAL_JsqKPKk3cPO8DG3FQVSHrKnO+Zed1R=PV7n7iAC+qJKgHcw@mail.gmail.com/ > > > Bjorn
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@kernel.org> 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@kernel.org> 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. > [1] https://lore.kernel.org/linux-pci/20210329162539.GG5166@sirena.org.uk/ > [2] https://lore.kernel.org/linux-pci/CAL_JsqKPKk3cPO8DG3FQVSHrKnO+Zed1R=PV7n7iAC+qJKgHcw@mail.gmail.com/
On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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@kernel.org> 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. Regards, Jim Quinlan Broadcom STB > > > [1] https://lore.kernel.org/linux-pci/20210329162539.GG5166@sirena.org.uk/ > > [2] https://lore.kernel.org/linux-pci/CAL_JsqKPKk3cPO8DG3FQVSHrKnO+Zed1R=PV7n7iAC+qJKgHcw@mail.gmail.com/
On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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@kernel.org> 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. Rob
On Mon, Jul 18, 2022 at 05:40:33PM -0500, Bjorn Helgaas wrote: > On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote: > > ... > > > So I think brcm_pcie_setup() does initialization that doesn't depend > > on the link or any downstream devices, and brcm_pcie_start_link() does > > things that depend on the link being up. Right? > > > > If so, "start_link" might be a slight misnomer since AFAICT > > brcm_pcie_start_link() doesn't do anything to initiate link-up except > > maybe deasserting fundamental reset. Some drivers start the LTSSM or > > explicitly enable link training, but brcm_pcie_start_link() doesn't > > seem to do anything like that. > > > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does > > that really depend on the link being up? If that only affects the > > Root Port, maybe it could be done before link-up? > > What about the /* PCIe->SCB endian mode for BAR */ thing? Does that > depend on the link being up? > > And the "Refclk from RC should be gated with CLKREQ#" part? Does that > depend on the link being up? > > It seems obvious that brcm_pcie_set_ssc() and reading the negotiated > link speed and width depend on the link being up. Can we close on this? Splitting into (a) stuff that can be initialized before the link is available and (b) stuff that depends on the link makes good sense, but then (b) should only contain stuff that actually depends on the link. The "PCIe->SCB endian mode for BAR" *sounds* like something related to the primary side of the RP, not the link. Not sure about "Refclk from RC". RC would certainly be primary side, but ASPM has to do with secondary (link) side. Bjorn
On 7/20/22 09:18, Rob Herring wrote: > On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: >> >> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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@kernel.org> 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?
On Wed, Jul 20, 2022 at 02:34:06PM -0700, Florian Fainelli wrote: > On 7/20/22 09:18, Rob Herring wrote: > > On Wed, Jul 20, 2022 at 8:53 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > >> > >> On Tue, Jul 19, 2022 at 4:03 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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@kernel.org> 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? (Need to fix your mailer to wrap lines) We should not continue with the same mistake of putting per slot properties in the RC node when they belong in the RP node. I was just pointing out that we could still handle those existing cases by checking the parent node. Rob
On Wed, Jul 20, 2022 at 4:37 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Jul 18, 2022 at 05:40:33PM -0500, Bjorn Helgaas wrote: > > On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote: > > > ... > > > > > So I think brcm_pcie_setup() does initialization that doesn't depend > > > on the link or any downstream devices, and brcm_pcie_start_link() does > > > things that depend on the link being up. Right? > > > > > > If so, "start_link" might be a slight misnomer since AFAICT > > > brcm_pcie_start_link() doesn't do anything to initiate link-up except > > > maybe deasserting fundamental reset. Some drivers start the LTSSM or > > > explicitly enable link training, but brcm_pcie_start_link() doesn't > > > seem to do anything like that. > > > > > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does > > > that really depend on the link being up? If that only affects the > > > Root Port, maybe it could be done before link-up? > > > > What about the /* PCIe->SCB endian mode for BAR */ thing? Does that > > depend on the link being up? > > > > And the "Refclk from RC should be gated with CLKREQ#" part? Does that > > depend on the link being up? > > > > It seems obvious that brcm_pcie_set_ssc() and reading the negotiated > > link speed and width depend on the link being up. > > Can we close on this? Splitting into Absolutely. > > (a) stuff that can be initialized before the link is available and > (b) stuff that depends on the link > > makes good sense, but then (b) should only contain stuff that actually > depends on the link. > > The "PCIe->SCB endian mode for BAR" *sounds* like something related to > the primary side of the RP, not the link. > > Not sure about "Refclk from RC". RC would certainly be primary side, > but ASPM has to do with secondary (link) side. I get the feedback, submission coming soon -- I was waiting for the email thread to conclude. Regards, Jim Quinlan Broadcom STB > > Bjorn
On Thu, Jul 21, 2022 at 10:56:53AM -0400, Jim Quinlan wrote: > On Wed, Jul 20, 2022 at 4:37 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Jul 18, 2022 at 05:40:33PM -0500, Bjorn Helgaas wrote: > > > On Mon, Jul 18, 2022 at 01:14:25PM -0500, Bjorn Helgaas wrote: > > > > ... > > > > > > > So I think brcm_pcie_setup() does initialization that doesn't depend > > > > on the link or any downstream devices, and brcm_pcie_start_link() does > > > > things that depend on the link being up. Right? > > > > > > > > If so, "start_link" might be a slight misnomer since AFAICT > > > > brcm_pcie_start_link() doesn't do anything to initiate link-up except > > > > maybe deasserting fundamental reset. Some drivers start the LTSSM or > > > > explicitly enable link training, but brcm_pcie_start_link() doesn't > > > > seem to do anything like that. > > > > > > > > brcm_pcie_start_link() still does brcm_pcie_set_outbound_win(). Does > > > > that really depend on the link being up? If that only affects the > > > > Root Port, maybe it could be done before link-up? > > > > > > What about the /* PCIe->SCB endian mode for BAR */ thing? Does that > > > depend on the link being up? > > > > > > And the "Refclk from RC should be gated with CLKREQ#" part? Does that > > > depend on the link being up? > > > > > > It seems obvious that brcm_pcie_set_ssc() and reading the negotiated > > > link speed and width depend on the link being up. > > > > Can we close on this? Splitting into > > Absolutely. > > > (a) stuff that can be initialized before the link is available and > > (b) stuff that depends on the link > > > > makes good sense, but then (b) should only contain stuff that actually > > depends on the link. > > > > The "PCIe->SCB endian mode for BAR" *sounds* like something related to > > the primary side of the RP, not the link. > > > > Not sure about "Refclk from RC". RC would certainly be primary side, > > but ASPM has to do with secondary (link) side. > > I get the feedback, submission coming soon -- I was waiting for the > email thread to conclude. I don't expect a new posting of the patches in response to every question, but hopefully this is a conversation that goes both ways, and there's no need to slow down the conversation. It's more than acceptable to simply ask and answer questions and post updated patches later. We're running out of runway and I want to make sure we get this thing done this cycle. Bjorn
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index bd88a0a46c63..c026446d5830 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -849,16 +849,9 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie, static int brcm_pcie_setup(struct brcm_pcie *pcie) { - struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie); u64 rc_bar2_offset, rc_bar2_size; void __iomem *base = pcie->base; - struct device *dev = pcie->dev; - struct resource_entry *entry; - bool ssc_good = false; - struct resource *res; - int num_out_wins = 0; - u16 nlw, cls, lnksta; - int i, ret, memc; + int ret, memc; u32 tmp, burst, aspm_support; /* Reset the bridge */ @@ -948,6 +941,40 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) if (pcie->gen) brcm_pcie_set_gen(pcie, pcie->gen); + /* Don't advertise L0s capability if 'aspm-no-l0s' */ + aspm_support = PCIE_LINK_STATE_L1; + if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) + aspm_support |= PCIE_LINK_STATE_L0S; + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); + u32p_replace_bits(&tmp, aspm_support, + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); + + /* + * For config space accesses on the RC, show the right class for + * a PCIe-PCIe bridge (the default setting is to be EP mode). + */ + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); + u32p_replace_bits(&tmp, 0x060400, + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); + + return 0; +} + +static int brcm_pcie_start_link(struct brcm_pcie *pcie) +{ + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie); + struct device *dev = pcie->dev; + void __iomem *base = pcie->base; + struct resource_entry *entry; + struct resource *res; + int num_out_wins = 0; + u16 nlw, cls, lnksta; + bool ssc_good = false; + u32 tmp; + int ret, i; + /* Unassert the fundamental reset */ pcie->perst_set(pcie, 0); @@ -998,24 +1025,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) num_out_wins++; } - /* Don't advertise L0s capability if 'aspm-no-l0s' */ - aspm_support = PCIE_LINK_STATE_L1; - if (!of_property_read_bool(pcie->np, "aspm-no-l0s")) - aspm_support |= PCIE_LINK_STATE_L0S; - tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); - u32p_replace_bits(&tmp, aspm_support, - PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); - writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY); - - /* - * For config space accesses on the RC, show the right class for - * a PCIe-PCIe bridge (the default setting is to be EP mode). - */ - tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3); - u32p_replace_bits(&tmp, 0x060400, - PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK); - writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3); - if (pcie->ssc) { ret = brcm_pcie_set_ssc(pcie); if (ret == 0) @@ -1204,6 +1213,10 @@ static int brcm_pcie_resume(struct device *dev) if (ret) goto err_reset; + ret = brcm_pcie_start_link(pcie); + if (ret) + goto err_reset; + if (pcie->msi) brcm_msi_set_regs(pcie->msi); @@ -1393,6 +1406,10 @@ static int brcm_pcie_probe(struct platform_device *pdev) if (ret) goto fail; + ret = brcm_pcie_start_link(pcie); + if (ret) + goto fail; + pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION); if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) { dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
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. We do this by defining an add_bus() callback which is invoked during enumeraion. At the end of this patchset the probe function trace will look something like this: -> brcm_pcie_probe() -> brcm_pcie_setup(); /* Set-up only */ -> pci_host_probe(bridge); -> [enumeration] -> pci_alloc_child_bus() -> bus->ops->add_bus(bus); /* We've set this op */ -> brcm_pcie_add_bus() /* Our callback */ -> [turn on regulators] /* Main objective! */ -> brcm_pcie_start_link() /* Link-up */ One final note: some code that was executed after the PCIe linkup is now placed so that it executes prior to linkup, since this code has to run prior to the invocation of pci_host_probe(). Link: https://lore.kernel.org/r/20220106160332.2143-5-jim2101024@gmail.com Signed-off-by: Jim Quinlan <jim2101024@gmail.com> --- drivers/pci/controller/pcie-brcmstb.c | 69 +++++++++++++++++---------- 1 file changed, 43 insertions(+), 26 deletions(-)