From: Jim Quinlan <james.quinlan@broadcom.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: "Jim Quinlan" <jim2101024@gmail.com>, "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" <linux-pci@vger.kernel.org>, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Cyril Brulebois" <kibi@debian.org>, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.com>, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Rob Herring" <robh@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Date: Fri, 8 Jul 2022 09:29:27 -0400 [thread overview] Message-ID: <CA+-6iNzQZVi8MbyeZNcBzE0hGjGiYSUk3riSXxTALtFyC00aPQ@mail.gmail.com> (raw) In-Reply-To: <20220706215603.GA221278@bhelgaas> [-- Attachment #1: Type: text/plain, Size: 6569 bytes --] On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote: > > We need to take some code in brcm_pcie_setup() and put it in a new function > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will > > be called indirectly by pci_host_probe() as opposed to the host driver > > invoking it directly. > > > > 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(). > > This says we need to move some code from brcm_pcie_setup() to > brcm_pcie_linkup(), but not *why* we need to do that. I will elaborate in the commit message. > > In brcm_pcie_resume(), they're called together: > > brcm_pcie_resume > brcm_pcie_setup > brcm_pcie_linkup > > In the probe path, they're not called together, but they're in the > same order: > > brcm_pcie_probe > brcm_pcie_setup > pci_host_probe > ... > brcm_pcie_add_bus # bus->ops->add_bus > brcm_pcie_linkup > > Is there something that must happen *between* them in the probe path? Yes. In the probe() case, we must do things in this order: 1. brcm_pcie_setup() 2. Turn on regulators 3. brcm_pcie_linkup() Since the voltage regulators are turned on during enumeration, pci_host_probe() must be invoked prior to 3. Before regulators, we did not care. In the resume case, there is no enumeration of course but our driver has a handle to the regulators and can turn them on/off w/o help. Regards, Jim Quinlan Broradcom STB > > > 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(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index e61058e13818..2bf5cc399fd0 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -926,16 +926,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 */ > > @@ -1025,6 +1018,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_linkup(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); > > > > @@ -1075,24 +1102,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) > > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev) > > if (ret) > > goto err_reset; > > > > + ret = brcm_pcie_linkup(pcie); > > + if (ret) > > + goto err_reset; > > + > > if (pcie->msi) > > brcm_msi_set_regs(pcie->msi); > > > > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > if (ret) > > goto fail; > > > > + ret = brcm_pcie_linkup(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"); > > -- > > 2.17.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4210 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Jim Quinlan <james.quinlan@broadcom.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: "Jim Quinlan" <jim2101024@gmail.com>, "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" <linux-pci@vger.kernel.org>, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Cyril Brulebois" <kibi@debian.org>, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.com>, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Rob Herring" <robh@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Date: Fri, 8 Jul 2022 09:29:27 -0400 [thread overview] Message-ID: <CA+-6iNzQZVi8MbyeZNcBzE0hGjGiYSUk3riSXxTALtFyC00aPQ@mail.gmail.com> (raw) In-Reply-To: <20220706215603.GA221278@bhelgaas> [-- Attachment #1.1: Type: text/plain, Size: 6569 bytes --] On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote: > > We need to take some code in brcm_pcie_setup() and put it in a new function > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will > > be called indirectly by pci_host_probe() as opposed to the host driver > > invoking it directly. > > > > 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(). > > This says we need to move some code from brcm_pcie_setup() to > brcm_pcie_linkup(), but not *why* we need to do that. I will elaborate in the commit message. > > In brcm_pcie_resume(), they're called together: > > brcm_pcie_resume > brcm_pcie_setup > brcm_pcie_linkup > > In the probe path, they're not called together, but they're in the > same order: > > brcm_pcie_probe > brcm_pcie_setup > pci_host_probe > ... > brcm_pcie_add_bus # bus->ops->add_bus > brcm_pcie_linkup > > Is there something that must happen *between* them in the probe path? Yes. In the probe() case, we must do things in this order: 1. brcm_pcie_setup() 2. Turn on regulators 3. brcm_pcie_linkup() Since the voltage regulators are turned on during enumeration, pci_host_probe() must be invoked prior to 3. Before regulators, we did not care. In the resume case, there is no enumeration of course but our driver has a handle to the regulators and can turn them on/off w/o help. Regards, Jim Quinlan Broradcom STB > > > 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(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index e61058e13818..2bf5cc399fd0 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -926,16 +926,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 */ > > @@ -1025,6 +1018,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_linkup(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); > > > > @@ -1075,24 +1102,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) > > @@ -1281,6 +1290,10 @@ static int brcm_pcie_resume(struct device *dev) > > if (ret) > > goto err_reset; > > > > + ret = brcm_pcie_linkup(pcie); > > + if (ret) > > + goto err_reset; > > + > > if (pcie->msi) > > brcm_msi_set_regs(pcie->msi); > > > > @@ -1398,6 +1411,10 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > if (ret) > > goto fail; > > > > + ret = brcm_pcie_linkup(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"); > > -- > > 2.17.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel [-- Attachment #1.2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4210 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-07-08 13:29 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-01 16:27 [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-01 16:27 ` [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-06 21:56 ` Bjorn Helgaas 2022-07-06 21:56 ` Bjorn Helgaas 2022-07-08 13:29 ` Jim Quinlan [this message] 2022-07-08 13:29 ` Jim Quinlan 2022-07-08 19:04 ` Bjorn Helgaas 2022-07-08 19:04 ` Bjorn Helgaas 2022-07-08 19:40 ` Jim Quinlan 2022-07-08 19:40 ` Jim Quinlan 2022-07-08 19:59 ` Bjorn Helgaas 2022-07-08 19:59 ` Bjorn Helgaas 2022-07-08 20:38 ` Jim Quinlan 2022-07-08 20:38 ` Jim Quinlan 2022-07-08 22:27 ` Bjorn Helgaas 2022-07-08 22:27 ` Bjorn Helgaas 2022-07-01 16:27 ` [PATCH v1 2/4] PCI: brcmstb: Add mechanism to turn on subdev regulators Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-06 23:12 ` Bjorn Helgaas 2022-07-06 23:12 ` Bjorn Helgaas 2022-07-08 14:14 ` Jim Quinlan 2022-07-08 14:14 ` Jim Quinlan 2022-07-08 19:07 ` Bjorn Helgaas 2022-07-08 19:07 ` Bjorn Helgaas 2022-07-01 16:27 ` [PATCH v1 3/4] PCI: brcmstb: Add control of subdevice voltage regulators Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-06 23:13 ` Bjorn Helgaas 2022-07-06 23:13 ` Bjorn Helgaas 2022-07-08 15:16 ` Jim Quinlan 2022-07-08 15:16 ` Jim Quinlan 2022-07-08 19:09 ` Bjorn Helgaas 2022-07-08 19:09 ` Bjorn Helgaas 2022-07-01 16:27 ` [PATCH v1 4/4] PCI: brcmstb: Do not turn off WOL regulators on suspend Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-01 20:58 ` [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset Florian Fainelli 2022-07-01 20:58 ` Florian Fainelli 2022-07-05 20:55 ` Cyril Brulebois 2022-07-05 20:55 ` Cyril Brulebois 2022-07-05 21:00 ` Florian Fainelli 2022-07-05 21:00 ` Florian Fainelli 2022-07-05 21:28 ` Cyril Brulebois 2022-07-05 21:28 ` Cyril Brulebois 2022-07-05 22:06 ` Jim Quinlan 2022-07-05 22:06 ` Jim Quinlan 2022-07-15 18:27 ` Bjorn Helgaas 2022-07-15 18:27 ` Bjorn Helgaas 2022-07-15 18:30 ` Jim Quinlan 2022-07-15 18:30 ` Jim Quinlan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CA+-6iNzQZVi8MbyeZNcBzE0hGjGiYSUk3riSXxTALtFyC00aPQ@mail.gmail.com \ --to=james.quinlan@broadcom.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=bhelgaas@google.com \ --cc=f.fainelli@gmail.com \ --cc=helgaas@kernel.org \ --cc=jim2101024@gmail.com \ --cc=kibi@debian.org \ --cc=kw@linux.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=lpieralisi@kernel.org \ --cc=nsaenz@kernel.org \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.