From: "Pali Rohár" <pali@kernel.org> To: Lukas Wunner <lukas@wunner.de> Cc: "Marek Behún" <kabel@kernel.org>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Bjorn Helgaas" <helgaas@kernel.org>, "Krzysztof Wilczy??ski" <kw@linux.com>, "Marc Zyngier" <maz@kernel.org>, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "Gregory CLEMENT" <gregory.clement@bootlin.com> Subject: Re: [PATCH 06/18] PCI: pciehp: Enable DLLSC interrupt only if supported Date: Fri, 13 May 2022 18:57:29 +0200 [thread overview] Message-ID: <20220513165729.wuaatfr2drsjwoos@pali> (raw) In-Reply-To: <20220509034216.GA26780@wunner.de> On Monday 09 May 2022 05:42:16 Lukas Wunner wrote: > On Sun, Feb 20, 2022 at 08:33:34PM +0100, Marek Behún wrote: > > --- a/drivers/pci/hotplug/pciehp_hpc.c > > +++ b/drivers/pci/hotplug/pciehp_hpc.c > > @@ -788,6 +788,7 @@ static int pciehp_poll(void *data) > > @@ -800,12 +801,17 @@ static void pcie_enable_notification(struct controller *ctrl) > > * next power fault detected interrupt was notified again. > > */ > > > > + pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap); > > /* > > - * Always enable link events: thus link-up and link-down shall > > - * always be treated as hotplug and unplug respectively. Enable > > - * presence detect only if Attention Button is not present. > > + * Enable link events if their support is indicated in Link Capability > > + * register: thus link-up and link-down shall always be treated as > > + * hotplug and unplug respectively. Enable presence detect only if > > + * Attention Button is not present. > > */ > > - cmd = PCI_EXP_SLTCTL_DLLSCE; > > + cmd = 0; > > + if (link_cap & PCI_EXP_LNKCAP_DLLLARC) > > + cmd |= PCI_EXP_SLTCTL_DLLSCE; > > The Data Link Layer Link Active Reporting Capable bit is cached > in ctrl_dev(ctrl)->link_active_reporting. Please use that > instead of re-reading it from the register. > > According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable > Downstream Port [...], this bit must be hardwired to 1b." > > That has been part of the spec since PCIe r1.1, sec. 7.8.6. > > PCIe r1.0 did not contain the sentence because it did not support > DLLLARC (it defined bit 20 as RsvdP). > > In other words, what you're doing here is add support for PCIe r1.0. > I'm not opposed to that, but I'd assume that aardvark supports a > more recent spec version. More likely it doesn't comply with the spec? > > What is the user-visible issue that you're experiencing without this > commit? Does aardvark somehow misbehave if the DLLLARC bit is set to 1? > Since the bit is RsvdP, setting it shouldn't have any negative side > effects. I will let fixing those issues to Marek. To answer your questions: Config space of Aardvark Root Port does not conform to PCIe base spec. It does not implement DLLLARC, nor DLLSCE and lot of other bits. Plus it has Type 0 header (not Type 1). And due to these reasons, pci-aardvark.c driver implements "emulation" of the Root Port and implements some of the functionality via custom aardvark registers. So Root Port would be presented to kernel and also to userspace as PCI Bridge device with Type 1 header and with PCIe registers required by linux kernel. During my testing of kernel hotplug code last year, I figured out that kernel was waiting for event which never happened. And so it was needed to "fix" kernel to not try to enable DLLSCE because it did nothing. I asked more times Marvell for Aardvark documentation, so I could fix these issues, but I have never received any response for it. > > > --- a/drivers/pci/hotplug/pnv_php.c > > +++ b/drivers/pci/hotplug/pnv_php.c > > @@ -840,6 +840,7 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq) > > struct pci_dev *pdev = php_slot->pdev; > > u32 broken_pdc = 0; > > u16 sts, ctrl; > > + u32 link_cap; > > int ret; > > > > /* Allocate workqueue */ > > pnv_php.c is a driver for PowerNV, yet this patch is for a series > targeting an ARM PCIe controller. That doesn't make sense, > changes to pnv_php.c seem wrong here. > > Thanks, > > Lukas At time when I prepared this patch (year ago) I changed that DLLSCE pattern in all places because it looked like copy+paste code which should be fixed too.
WARNING: multiple messages have this Message-ID (diff)
From: "Pali Rohár" <pali@kernel.org> To: Lukas Wunner <lukas@wunner.de> Cc: "Marek Behún" <kabel@kernel.org>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Bjorn Helgaas" <helgaas@kernel.org>, "Krzysztof Wilczy??ski" <kw@linux.com>, "Marc Zyngier" <maz@kernel.org>, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "Gregory CLEMENT" <gregory.clement@bootlin.com> Subject: Re: [PATCH 06/18] PCI: pciehp: Enable DLLSC interrupt only if supported Date: Fri, 13 May 2022 18:57:29 +0200 [thread overview] Message-ID: <20220513165729.wuaatfr2drsjwoos@pali> (raw) In-Reply-To: <20220509034216.GA26780@wunner.de> On Monday 09 May 2022 05:42:16 Lukas Wunner wrote: > On Sun, Feb 20, 2022 at 08:33:34PM +0100, Marek Behún wrote: > > --- a/drivers/pci/hotplug/pciehp_hpc.c > > +++ b/drivers/pci/hotplug/pciehp_hpc.c > > @@ -788,6 +788,7 @@ static int pciehp_poll(void *data) > > @@ -800,12 +801,17 @@ static void pcie_enable_notification(struct controller *ctrl) > > * next power fault detected interrupt was notified again. > > */ > > > > + pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap); > > /* > > - * Always enable link events: thus link-up and link-down shall > > - * always be treated as hotplug and unplug respectively. Enable > > - * presence detect only if Attention Button is not present. > > + * Enable link events if their support is indicated in Link Capability > > + * register: thus link-up and link-down shall always be treated as > > + * hotplug and unplug respectively. Enable presence detect only if > > + * Attention Button is not present. > > */ > > - cmd = PCI_EXP_SLTCTL_DLLSCE; > > + cmd = 0; > > + if (link_cap & PCI_EXP_LNKCAP_DLLLARC) > > + cmd |= PCI_EXP_SLTCTL_DLLSCE; > > The Data Link Layer Link Active Reporting Capable bit is cached > in ctrl_dev(ctrl)->link_active_reporting. Please use that > instead of re-reading it from the register. > > According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable > Downstream Port [...], this bit must be hardwired to 1b." > > That has been part of the spec since PCIe r1.1, sec. 7.8.6. > > PCIe r1.0 did not contain the sentence because it did not support > DLLLARC (it defined bit 20 as RsvdP). > > In other words, what you're doing here is add support for PCIe r1.0. > I'm not opposed to that, but I'd assume that aardvark supports a > more recent spec version. More likely it doesn't comply with the spec? > > What is the user-visible issue that you're experiencing without this > commit? Does aardvark somehow misbehave if the DLLLARC bit is set to 1? > Since the bit is RsvdP, setting it shouldn't have any negative side > effects. I will let fixing those issues to Marek. To answer your questions: Config space of Aardvark Root Port does not conform to PCIe base spec. It does not implement DLLLARC, nor DLLSCE and lot of other bits. Plus it has Type 0 header (not Type 1). And due to these reasons, pci-aardvark.c driver implements "emulation" of the Root Port and implements some of the functionality via custom aardvark registers. So Root Port would be presented to kernel and also to userspace as PCI Bridge device with Type 1 header and with PCIe registers required by linux kernel. During my testing of kernel hotplug code last year, I figured out that kernel was waiting for event which never happened. And so it was needed to "fix" kernel to not try to enable DLLSCE because it did nothing. I asked more times Marvell for Aardvark documentation, so I could fix these issues, but I have never received any response for it. > > > --- a/drivers/pci/hotplug/pnv_php.c > > +++ b/drivers/pci/hotplug/pnv_php.c > > @@ -840,6 +840,7 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq) > > struct pci_dev *pdev = php_slot->pdev; > > u32 broken_pdc = 0; > > u16 sts, ctrl; > > + u32 link_cap; > > int ret; > > > > /* Allocate workqueue */ > > pnv_php.c is a driver for PowerNV, yet this patch is for a series > targeting an ARM PCIe controller. That doesn't make sense, > changes to pnv_php.c seem wrong here. > > Thanks, > > Lukas At time when I prepared this patch (year ago) I changed that DLLSCE pattern in all places because it looked like copy+paste code which should be fixed too. _______________________________________________ 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-05-13 16:57 UTC|newest] Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-20 19:33 [PATCH 00/18] PCI: aardvark controller changes BATCH 5 Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 01/18] PCI: pci-bridge-emul: Re-arrange register tests Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 02/18] PCI: pci-bridge-emul: Add support for PCIe extended capabilities Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 03/18] PCI: aardvark: Add support for AER registers on emulated bridge Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 04/18] PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-04-28 11:09 ` Lorenzo Pieralisi 2022-04-28 11:09 ` Lorenzo Pieralisi 2022-04-28 11:16 ` Pali Rohár 2022-04-28 11:16 ` Pali Rohár 2022-05-18 19:23 ` Bjorn Helgaas 2022-05-18 19:23 ` Bjorn Helgaas 2022-05-18 19:26 ` Pali Rohár 2022-05-18 19:26 ` Pali Rohár 2022-05-18 20:05 ` Marek Behún 2022-05-18 20:05 ` Marek Behún 2022-05-18 20:27 ` Bjorn Helgaas 2022-05-18 20:27 ` Bjorn Helgaas 2022-02-20 19:33 ` [PATCH 05/18] PCI: aardvark: Fix reporting Slot capabilities on emulated bridge Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 06/18] PCI: pciehp: Enable DLLSC interrupt only if supported Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-05-09 3:42 ` Lukas Wunner 2022-05-13 16:57 ` Pali Rohár [this message] 2022-05-13 16:57 ` Pali Rohár 2022-05-14 9:14 ` Lukas Wunner 2022-08-18 12:22 ` Marek Behún 2022-08-18 12:22 ` Marek Behún 2022-02-20 19:33 ` [PATCH 07/18] PCI: pciehp: Enable Command Completed Interrupt " Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-05-09 4:01 ` Lukas Wunner 2022-05-13 16:59 ` Pali Rohár 2022-05-13 16:59 ` Pali Rohár 2022-02-20 19:33 ` [PATCH 08/18] PCI: aardvark: Add support for DLLSC and hotplug interrupt Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 09/18] PCI: Add PCI_EXP_SLTCTL_ASPL_DISABLE macro Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 10/18] PCI: Add function for parsing `slot-power-limit-milliwatt` DT property Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 11/18] dt-bindings: PCI: aardvark: Describe slot-power-limit-milliwatt Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 12/18] PCI: aardvark: Send Set_Slot_Power_Limit message Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 13/18] arm64: dts: armada-3720-turris-mox: Define slot-power-limit-milliwatt for PCIe Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 14/18] PCI: aardvark: Add clock support Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-20 19:33 ` [PATCH 15/18] arm64: dts: marvell: armada-37xx: Add clock to PCIe node Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-02-28 15:52 ` Gregory CLEMENT 2022-02-28 15:52 ` Gregory CLEMENT 2022-02-20 19:33 ` [PATCH 16/18] PCI: aardvark: Add suspend to RAM support Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-04-12 11:14 ` Lorenzo Pieralisi 2022-04-12 11:14 ` Lorenzo Pieralisi 2022-02-20 19:33 ` [PATCH 17/18] PCI: aardvark: Run link training in separate worker Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-04-12 15:25 ` Lorenzo Pieralisi 2022-04-12 15:25 ` Lorenzo Pieralisi 2022-04-12 17:55 ` Pali Rohár 2022-04-12 17:55 ` Pali Rohár 2022-04-13 9:16 ` Lorenzo Pieralisi 2022-04-13 9:16 ` Lorenzo Pieralisi 2022-05-04 14:02 ` Marek Behún 2022-05-04 14:02 ` Marek Behún 2022-02-20 19:33 ` [PATCH 18/18] PCI: aardvark: Optimize PCIe card reset via GPIO Marek Behún 2022-02-20 19:33 ` Marek Behún 2022-04-11 15:36 ` [PATCH 00/18] PCI: aardvark controller changes BATCH 5 Lorenzo Pieralisi 2022-04-11 15:36 ` Lorenzo Pieralisi 2022-04-11 16:53 ` Pali Rohár 2022-04-11 16:53 ` Pali Rohár 2022-05-13 10:33 ` Lorenzo Pieralisi 2022-05-13 10:33 ` Lorenzo Pieralisi 2022-05-13 16:48 ` Pali Rohár 2022-05-13 16:48 ` Pali Rohár 2022-05-18 15:54 ` (subset) " Lorenzo Pieralisi 2022-05-18 15:54 ` Lorenzo Pieralisi 2022-08-16 16:25 ` Lorenzo Pieralisi 2022-08-16 16:25 ` Lorenzo Pieralisi 2022-08-18 13:56 ` Marek Behún 2022-08-18 13:56 ` Marek Behún
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=20220513165729.wuaatfr2drsjwoos@pali \ --to=pali@kernel.org \ --cc=gregory.clement@bootlin.com \ --cc=helgaas@kernel.org \ --cc=kabel@kernel.org \ --cc=kw@linux.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=lukas@wunner.de \ --cc=maz@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.