| Message ID | 20220818135140.5996-4-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: aardvark controller changes BATCH 6 | expand |
[+Marc, Thomas - I can't merge this code without them reviewing it, I am not sure at all you can mix the timer/IRQ code the way you do] On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > Add support for Data Link Layer State Change in the emulated slot > registers and hotplug interrupt via the emulated root bridge. > > This is mainly useful for when an error causes link down event. With > this change, drivers can try recovery. > > Link down state change can be implemented because Aardvark supports Link > Down event interrupt. Use it for signaling that Data Link Layer Link is > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > Link up interrupt is not available on Aardvark, but we check for whether > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > Interrupt from this function we achieve Link up event, so long as the > function is called (which it is after probe and when rescanning). > Although it is not ideal, it is better than nothing. So before even coming to the code review: this patch does two things. 1) It adds support for handling the Link down state 2) It adds some code to emulate a Link-up event Now, for (2). IIUC you are adding code to make sure that an HP event is triggered if advk_pcie_link_up() is called and it detects a Link-down->Link-up transition, that has to be notified through an HP event. If that's correct, you have to explain to me please what this is actually achieving and a specific scenario where we want this to be implemented, in fine details; then we add it to the commit log. That aside, the interaction of the timer and the IRQ domain code must be reviewed by Marc and Thomas to make sure this is not a gross violation of the respective subsystems usage. Lorenzo > Since advk_pcie_link_up() is not called from interrupt handler, we > cannot call generic_handle_domain_irq() from it directly. Instead create > a TIMER_IRQSAFE timer and trigger it from advk_pcie_link_up(). > > (We haven't been able to find any documentation for a Link Up interrupt > on Aardvark, but it is possible there is one, in some undocumented > register. If we manage to find this information, this can be > rewritten.) > > Signed-off-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > Change since batch 5: > - changed commit message (add paragraph about why the change is needed) > - select hotplug and pcieportbus in Kconfig > --- > drivers/pci/controller/Kconfig | 3 + > drivers/pci/controller/pci-aardvark.c | 101 ++++++++++++++++++++++++-- > 2 files changed, 99 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index d1c5fcf00a8a..5e8a84f5c654 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -21,6 +21,9 @@ config PCI_AARDVARK > depends on OF > depends on PCI_MSI_IRQ_DOMAIN > select PCI_BRIDGE_EMUL > + select PCIEPORTBUS > + select HOTPLUG_PCI > + select HOTPLUG_PCI_PCIE > help > Add support for Aardvark 64bit PCIe Host Controller. This > controller is part of the South Bridge of the Marvel Armada > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > index 966c8b48bd96..31da28ebc5d1 100644 > --- a/drivers/pci/controller/pci-aardvark.c > +++ b/drivers/pci/controller/pci-aardvark.c > @@ -25,6 +25,7 @@ > #include <linux/of_address.h> > #include <linux/of_gpio.h> > #include <linux/of_pci.h> > +#include <linux/timer.h> > > #include "../pci.h" > #include "../pci-bridge-emul.h" > @@ -100,6 +101,7 @@ > #define PCIE_MSG_PM_PME_MASK BIT(7) > #define PCIE_ISR0_MASK_REG (CONTROL_BASE_ADDR + 0x44) > #define PCIE_ISR0_MSI_INT_PENDING BIT(24) > +#define PCIE_ISR0_LINK_DOWN BIT(1) > #define PCIE_ISR0_CORR_ERR BIT(11) > #define PCIE_ISR0_NFAT_ERR BIT(12) > #define PCIE_ISR0_FAT_ERR BIT(13) > @@ -284,6 +286,8 @@ struct advk_pcie { > DECLARE_BITMAP(msi_used, MSI_IRQ_NUM); > struct mutex msi_used_lock; > int link_gen; > + bool link_was_up; > + struct timer_list link_irq_timer; > struct pci_bridge_emul bridge; > struct gpio_desc *reset_gpio; > struct phy *phy; > @@ -313,7 +317,24 @@ static inline bool advk_pcie_link_up(struct advk_pcie *pcie) > { > /* check if LTSSM is in normal operation - some L* state */ > u8 ltssm_state = advk_pcie_ltssm_state(pcie); > - return ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED; > + bool link_is_up; > + u16 slotsta; > + > + link_is_up = ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED; > + > + if (link_is_up && !pcie->link_was_up) { > + dev_info(&pcie->pdev->dev, "link up\n"); > + > + pcie->link_was_up = true; > + > + slotsta = le16_to_cpu(pcie->bridge.pcie_conf.slotsta); > + slotsta |= PCI_EXP_SLTSTA_DLLSC; > + pcie->bridge.pcie_conf.slotsta = cpu_to_le16(slotsta); > + > + mod_timer(&pcie->link_irq_timer, jiffies + 1); > + } > + > + return link_is_up; > } > > static inline bool advk_pcie_link_active(struct advk_pcie *pcie) > @@ -442,8 +463,6 @@ static void advk_pcie_train_link(struct advk_pcie *pcie) > ret = advk_pcie_wait_for_link(pcie); > if (ret < 0) > dev_err(dev, "link never came up\n"); > - else > - dev_info(dev, "link up\n"); > } > > /* > @@ -592,6 +611,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) > reg &= ~PCIE_ISR0_MSI_INT_PENDING; > advk_writel(pcie, reg, PCIE_ISR0_MASK_REG); > > + /* Unmask Link Down interrupt */ > + reg = advk_readl(pcie, PCIE_ISR0_MASK_REG); > + reg &= ~PCIE_ISR0_LINK_DOWN; > + advk_writel(pcie, reg, PCIE_ISR0_MASK_REG); > + > /* Unmask PME interrupt for processing of PME requester */ > reg = advk_readl(pcie, PCIE_ISR0_MASK_REG); > reg &= ~PCIE_MSG_PM_PME_MASK; > @@ -918,6 +942,14 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, > advk_pcie_wait_for_retrain(pcie); > break; > > + case PCI_EXP_SLTCTL: { > + u16 slotctl = le16_to_cpu(bridge->pcie_conf.slotctl); > + /* Only emulation of HPIE and DLLSCE bits is provided */ > + slotctl &= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE; > + bridge->pcie_conf.slotctl = cpu_to_le16(slotctl); > + break; > + } > + > case PCI_EXP_RTCTL: { > u16 rootctl = le16_to_cpu(bridge->pcie_conf.rootctl); > /* Only emulation of PMEIE and CRSSVE bits is provided */ > @@ -1035,6 +1067,7 @@ static const struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = { > static int advk_sw_pci_bridge_init(struct advk_pcie *pcie) > { > struct pci_bridge_emul *bridge = &pcie->bridge; > + u32 slotcap; > > bridge->conf.vendor = > cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff); > @@ -1061,6 +1094,13 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie) > bridge->pcie_conf.cap = cpu_to_le16(2 | PCI_EXP_FLAGS_SLOT); > > /* > + * Mark bridge as Hot Plug Capable since this is the way how to enable > + * delivering of Data Link Layer State Change interrupts. > + * > + * Set No Command Completed Support because bridge does not support > + * Command Completed Interrupt. Every command is executed immediately > + * without any delay. > + * > * Set Presence Detect State bit permanently since there is no support > * for unplugging the card nor detecting whether it is plugged. (If a > * platform exists in the future that supports it, via a GPIO for > @@ -1070,8 +1110,9 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie) > * value is reserved for ports within the same silicon as Root Port > * which is not our case. > */ > - bridge->pcie_conf.slotcap = cpu_to_le32(FIELD_PREP(PCI_EXP_SLTCAP_PSN, > - 1)); > + slotcap = PCI_EXP_SLTCAP_NCCS | PCI_EXP_SLTCAP_HPC | > + FIELD_PREP(PCI_EXP_SLTCAP_PSN, 1); > + bridge->pcie_conf.slotcap = cpu_to_le32(slotcap); > bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS); > > /* Indicates supports for Completion Retry Status */ > @@ -1568,6 +1609,24 @@ static void advk_pcie_remove_rp_irq_domain(struct advk_pcie *pcie) > irq_domain_remove(pcie->rp_irq_domain); > } > > +static void advk_pcie_link_irq_handler(struct timer_list *timer) > +{ > + struct advk_pcie *pcie = from_timer(pcie, timer, link_irq_timer); > + u16 slotctl; > + > + slotctl = le16_to_cpu(pcie->bridge.pcie_conf.slotctl); > + if (!(slotctl & PCI_EXP_SLTCTL_DLLSCE) || > + !(slotctl & PCI_EXP_SLTCTL_HPIE)) > + return; > + > + /* > + * Aardvark HW returns zero for PCI_EXP_FLAGS_IRQ, so use PCIe > + * interrupt 0 > + */ > + if (generic_handle_domain_irq(pcie->rp_irq_domain, 0) == -EINVAL) > + dev_err_ratelimited(&pcie->pdev->dev, "unhandled HP IRQ\n"); > +} > + > static void advk_pcie_handle_pme(struct advk_pcie *pcie) > { > u32 requester = advk_readl(pcie, PCIE_MSG_LOG_REG) >> 16; > @@ -1619,6 +1678,7 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie) > { > u32 isr0_val, isr0_mask, isr0_status; > u32 isr1_val, isr1_mask, isr1_status; > + u16 slotsta; > int i; > > isr0_val = advk_readl(pcie, PCIE_ISR0_REG); > @@ -1645,6 +1705,26 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie) > dev_err_ratelimited(&pcie->pdev->dev, "unhandled ERR IRQ\n"); > } > > + /* Process Link Down interrupt as HP IRQ */ > + if (isr0_status & PCIE_ISR0_LINK_DOWN) { > + advk_writel(pcie, PCIE_ISR0_LINK_DOWN, PCIE_ISR0_REG); > + > + dev_info(&pcie->pdev->dev, "link down\n"); > + > + pcie->link_was_up = false; > + > + slotsta = le16_to_cpu(pcie->bridge.pcie_conf.slotsta); > + slotsta |= PCI_EXP_SLTSTA_DLLSC; > + pcie->bridge.pcie_conf.slotsta = cpu_to_le16(slotsta); > + > + /* > + * Deactivate timer and call advk_pcie_link_irq_handler() > + * function directly as we are in the interrupt context. > + */ > + del_timer_sync(&pcie->link_irq_timer); > + advk_pcie_link_irq_handler(&pcie->link_irq_timer); > + } > + > /* Process MSI interrupts */ > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING) > advk_pcie_handle_msi(pcie); > @@ -1881,6 +1961,14 @@ static int advk_pcie_probe(struct platform_device *pdev) > if (ret) > return ret; > > + /* > + * generic_handle_domain_irq() expects local IRQs to be disabled since > + * normally it is called from interrupt context, so use TIMER_IRQSAFE > + * flag for this link_irq_timer. > + */ > + timer_setup(&pcie->link_irq_timer, advk_pcie_link_irq_handler, > + TIMER_IRQSAFE); > + > advk_pcie_setup_hw(pcie); > > ret = advk_sw_pci_bridge_init(pcie); > @@ -1969,6 +2057,9 @@ static int advk_pcie_remove(struct platform_device *pdev) > advk_pcie_remove_msi_irq_domain(pcie); > advk_pcie_remove_irq_domain(pcie); > > + /* Deactivate link event timer */ > + del_timer_sync(&pcie->link_irq_timer); > + > /* Free config space for emulated root bridge */ > pci_bridge_emul_cleanup(&pcie->bridge); > > -- > 2.35.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, 9 Sep 2022 16:57:11 +0200 Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > [+Marc, Thomas - I can't merge this code without them reviewing it, > I am not sure at all you can mix the timer/IRQ code the way you do] > > On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > > From: Pali Rohár <pali@kernel.org> > > > > Add support for Data Link Layer State Change in the emulated slot > > registers and hotplug interrupt via the emulated root bridge. > > > > This is mainly useful for when an error causes link down event. With > > this change, drivers can try recovery. > > > > Link down state change can be implemented because Aardvark supports Link > > Down event interrupt. Use it for signaling that Data Link Layer Link is > > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > > > Link up interrupt is not available on Aardvark, but we check for whether > > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > > Interrupt from this function we achieve Link up event, so long as the > > function is called (which it is after probe and when rescanning). > > Although it is not ideal, it is better than nothing. > > So before even coming to the code review: this patch does two things. > > 1) It adds support for handling the Link down state > 2) It adds some code to emulate a Link-up event > > Now, for (2). IIUC you are adding code to make sure that an HP > event is triggered if advk_pcie_link_up() is called and it > detects a Link-down->Link-up transition, that has to be notified > through an HP event. > > If that's correct, you have to explain to me please what this is > actually achieving and a specific scenario where we want this to be > implemented, in fine details; then we add it to the commit log. Hello Lorenzo, sorry for not replying earlier. Would something like this be sufficient? DLLSC is needed by the pciehp driver, which handles hotplug, but also link state change events. AFAIK no Aardvark devices support hotplug, but link state change events are required for graceful driver unbinding in case of link down event. So with this change we achieve graceful driver unbind for example when WiFi card PCIe link goes down (we've seen this with ath10k and mt76 WiFi cards). Before the WiFi driver started spitting out errors, or even taking the whole system down. Since after link goes down, it can come back up if the WiFi card can recover (or if reset pin is used to reset the card), we need to be able to recognize link up event. Since AFAIK Aardvark does not have interrupt for link up event, the best thing we can do is simulate it - whenever we read the link state, find it is up, and have cached value saying it is down, we trigger the link up event. We read link state whenever the configuration space is read, for example by writing 1 to /sys/bus/pci/rescan. Marek
Hi Lorenzo, On Fri, 09 Sep 2022 15:57:11 +0100, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > [+Marc, Thomas - I can't merge this code without them reviewing it, > I am not sure at all you can mix the timer/IRQ code the way you do] > > On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > > From: Pali Rohár <pali@kernel.org> > > > > Add support for Data Link Layer State Change in the emulated slot > > registers and hotplug interrupt via the emulated root bridge. > > > > This is mainly useful for when an error causes link down event. With > > this change, drivers can try recovery. > > > > Link down state change can be implemented because Aardvark supports Link > > Down event interrupt. Use it for signaling that Data Link Layer Link is > > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > > > Link up interrupt is not available on Aardvark, but we check for whether > > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > > Interrupt from this function we achieve Link up event, so long as the > > function is called (which it is after probe and when rescanning). > > Although it is not ideal, it is better than nothing. > > So before even coming to the code review: this patch does two things. > > 1) It adds support for handling the Link down state > 2) It adds some code to emulate a Link-up event > > Now, for (2). IIUC you are adding code to make sure that an HP > event is triggered if advk_pcie_link_up() is called and it > detects a Link-down->Link-up transition, that has to be notified > through an HP event. > > If that's correct, you have to explain to me please what this is > actually achieving and a specific scenario where we want this to be > implemented, in fine details; then we add it to the commit log. > > That aside, the interaction of the timer and the IRQ domain code > must be reviewed by Marc and Thomas to make sure this is not > a gross violation of the respective subsystems usage. I don't see anything being a "gross violation" here, at least from an interrupt subsystem perspective. In a way, this is synthesising an interrupt on the back of some other event, and as long as the context is somehow appropriate (something that looks like an interrupt when pretending there is one), this should be OK. Other subsystems such as i2c GPIO expanders do similar things. The one thing I'm dubious about is the frequency of the timer. Asking for a poll of the link every jiffy is bound to be expensive, and it would be good to relax this as much as possible, specially on low-end HW such as this, where every cycle counts. It is always going to be a "best effort" thing, and the commit message doesn't say what's the actual grace period to handle this (the spec probably has one). I guess this patch could do with being split between handling link down and link up events, but that's for you to decide. Thanks, M.
On Sat, Sep 17, 2022 at 10:05:59AM +0100, Marc Zyngier wrote: > Hi Lorenzo, > > On Fri, 09 Sep 2022 15:57:11 +0100, > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > [+Marc, Thomas - I can't merge this code without them reviewing it, > > I am not sure at all you can mix the timer/IRQ code the way you do] > > > > On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > > > From: Pali Rohár <pali@kernel.org> > > > > > > Add support for Data Link Layer State Change in the emulated slot > > > registers and hotplug interrupt via the emulated root bridge. > > > > > > This is mainly useful for when an error causes link down event. With > > > this change, drivers can try recovery. > > > > > > Link down state change can be implemented because Aardvark supports Link > > > Down event interrupt. Use it for signaling that Data Link Layer Link is > > > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > > > > > Link up interrupt is not available on Aardvark, but we check for whether > > > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > > > Interrupt from this function we achieve Link up event, so long as the > > > function is called (which it is after probe and when rescanning). > > > Although it is not ideal, it is better than nothing. > > > > So before even coming to the code review: this patch does two things. > > > > 1) It adds support for handling the Link down state > > 2) It adds some code to emulate a Link-up event > > > > Now, for (2). IIUC you are adding code to make sure that an HP > > event is triggered if advk_pcie_link_up() is called and it > > detects a Link-down->Link-up transition, that has to be notified > > through an HP event. > > > > If that's correct, you have to explain to me please what this is > > actually achieving and a specific scenario where we want this to be > > implemented, in fine details; then we add it to the commit log. > > > > That aside, the interaction of the timer and the IRQ domain code > > must be reviewed by Marc and Thomas to make sure this is not > > a gross violation of the respective subsystems usage. > > I don't see anything being a "gross violation" here, at least from an > interrupt subsystem perspective. In a way, this is synthesising an > interrupt on the back of some other event, and as long as the context > is somehow appropriate (something that looks like an interrupt when > pretending there is one), this should be OK. Other subsystems such as > i2c GPIO expanders do similar things. Right, thanks. > The one thing I'm dubious about is the frequency of the timer. Asking > for a poll of the link every jiffy is bound to be expensive, and it > would be good to relax this as much as possible, specially on low-end > HW such as this, where every cycle counts. It is always going to be a > "best effort" thing, and the commit message doesn't say what's the > actual grace period to handle this (the spec probably has one). AFAICS, the code does not poll the link. It sets a timer only if the link is checked (eg upon PCI bus forced rescan or config access) the link is up and it was down, to emulate a HP IRQ. > I guess this patch could do with being split between handling link > down and link up events, but that's for you to decide. It is fine for me as-is even though its logic could be simplified by the split. Thanks, Lorenzo > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 26 Sep 2022 07:49:55 -0400, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > On Sat, Sep 17, 2022 at 10:05:59AM +0100, Marc Zyngier wrote: > > Hi Lorenzo, > > > > On Fri, 09 Sep 2022 15:57:11 +0100, > > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > > > [+Marc, Thomas - I can't merge this code without them reviewing it, > > > I am not sure at all you can mix the timer/IRQ code the way you do] > > > > > > On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > > > > From: Pali Rohár <pali@kernel.org> > > > > > > > > Add support for Data Link Layer State Change in the emulated slot > > > > registers and hotplug interrupt via the emulated root bridge. > > > > > > > > This is mainly useful for when an error causes link down event. With > > > > this change, drivers can try recovery. > > > > > > > > Link down state change can be implemented because Aardvark supports Link > > > > Down event interrupt. Use it for signaling that Data Link Layer Link is > > > > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > > > > > > > Link up interrupt is not available on Aardvark, but we check for whether > > > > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > > > > Interrupt from this function we achieve Link up event, so long as the > > > > function is called (which it is after probe and when rescanning). > > > > Although it is not ideal, it is better than nothing. > > > > > > So before even coming to the code review: this patch does two things. > > > > > > 1) It adds support for handling the Link down state > > > 2) It adds some code to emulate a Link-up event > > > > > > Now, for (2). IIUC you are adding code to make sure that an HP > > > event is triggered if advk_pcie_link_up() is called and it > > > detects a Link-down->Link-up transition, that has to be notified > > > through an HP event. > > > > > > If that's correct, you have to explain to me please what this is > > > actually achieving and a specific scenario where we want this to be > > > implemented, in fine details; then we add it to the commit log. > > > > > > That aside, the interaction of the timer and the IRQ domain code > > > must be reviewed by Marc and Thomas to make sure this is not > > > a gross violation of the respective subsystems usage. > > > > I don't see anything being a "gross violation" here, at least from an > > interrupt subsystem perspective. In a way, this is synthesising an > > interrupt on the back of some other event, and as long as the context > > is somehow appropriate (something that looks like an interrupt when > > pretending there is one), this should be OK. Other subsystems such as > > i2c GPIO expanders do similar things. > > Right, thanks. > > > The one thing I'm dubious about is the frequency of the timer. Asking > > for a poll of the link every jiffy is bound to be expensive, and it > > would be good to relax this as much as possible, specially on low-end > > HW such as this, where every cycle counts. It is always going to be a > > "best effort" thing, and the commit message doesn't say what's the > > actual grace period to handle this (the spec probably has one). > > AFAICS, the code does not poll the link. It sets a timer only if > the link is checked (eg upon PCI bus forced rescan or config access) > the link is up and it was down, to emulate a HP IRQ. I still find the timer frequency pretty high, but surely the authors of the code have worked out that this wasn't a problem. Thanks, M.
On Mon, Sep 26, 2022 at 08:35:51AM -0400, Marc Zyngier wrote: > On Mon, 26 Sep 2022 07:49:55 -0400, > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > On Sat, Sep 17, 2022 at 10:05:59AM +0100, Marc Zyngier wrote: > > > Hi Lorenzo, > > > > > > On Fri, 09 Sep 2022 15:57:11 +0100, > > > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > > > > > [+Marc, Thomas - I can't merge this code without them reviewing it, > > > > I am not sure at all you can mix the timer/IRQ code the way you do] > > > > > > > > On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > > > > > From: Pali Rohár <pali@kernel.org> > > > > > > > > > > Add support for Data Link Layer State Change in the emulated slot > > > > > registers and hotplug interrupt via the emulated root bridge. > > > > > > > > > > This is mainly useful for when an error causes link down event. With > > > > > this change, drivers can try recovery. > > > > > > > > > > Link down state change can be implemented because Aardvark supports Link > > > > > Down event interrupt. Use it for signaling that Data Link Layer Link is > > > > > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > > > > > > > > > Link up interrupt is not available on Aardvark, but we check for whether > > > > > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > > > > > Interrupt from this function we achieve Link up event, so long as the > > > > > function is called (which it is after probe and when rescanning). > > > > > Although it is not ideal, it is better than nothing. > > > > > > > > So before even coming to the code review: this patch does two things. > > > > > > > > 1) It adds support for handling the Link down state > > > > 2) It adds some code to emulate a Link-up event > > > > > > > > Now, for (2). IIUC you are adding code to make sure that an HP > > > > event is triggered if advk_pcie_link_up() is called and it > > > > detects a Link-down->Link-up transition, that has to be notified > > > > through an HP event. > > > > > > > > If that's correct, you have to explain to me please what this is > > > > actually achieving and a specific scenario where we want this to be > > > > implemented, in fine details; then we add it to the commit log. > > > > > > > > That aside, the interaction of the timer and the IRQ domain code > > > > must be reviewed by Marc and Thomas to make sure this is not > > > > a gross violation of the respective subsystems usage. > > > > > > I don't see anything being a "gross violation" here, at least from an > > > interrupt subsystem perspective. In a way, this is synthesising an > > > interrupt on the back of some other event, and as long as the context > > > is somehow appropriate (something that looks like an interrupt when > > > pretending there is one), this should be OK. Other subsystems such as > > > i2c GPIO expanders do similar things. > > > > Right, thanks. > > > > > The one thing I'm dubious about is the frequency of the timer. Asking > > > for a poll of the link every jiffy is bound to be expensive, and it > > > would be good to relax this as much as possible, specially on low-end > > > HW such as this, where every cycle counts. It is always going to be a > > > "best effort" thing, and the commit message doesn't say what's the > > > actual grace period to handle this (the spec probably has one). > > > > AFAICS, the code does not poll the link. It sets a timer only if > > the link is checked (eg upon PCI bus forced rescan or config access) > > the link is up and it was down, to emulate a HP IRQ. > > I still find the timer frequency pretty high, but surely the authors > of the code have worked out that this wasn't a problem. Please correct me if I am wrong but with mod_timer() all they want to do is emulating/firing an (hotplug) IRQ as soon as possible - a one-off. It is not a periodic timer - at least that's what I understand from the code and that's the reason why such a short interval was chosen but it should not be me who comment on that :) Again - that's just my understanding of this patch (the link-up portion). Lorenzo
On Fri, Sep 16, 2022 at 06:23:02PM +0200, Marek Behún wrote: > On Fri, 9 Sep 2022 16:57:11 +0200 > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > [+Marc, Thomas - I can't merge this code without them reviewing it, > > I am not sure at all you can mix the timer/IRQ code the way you do] > > > > On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > > > From: Pali Rohár <pali@kernel.org> > > > > > > Add support for Data Link Layer State Change in the emulated slot > > > registers and hotplug interrupt via the emulated root bridge. > > > > > > This is mainly useful for when an error causes link down event. With > > > this change, drivers can try recovery. > > > > > > Link down state change can be implemented because Aardvark supports Link > > > Down event interrupt. Use it for signaling that Data Link Layer Link is > > > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > > > > > Link up interrupt is not available on Aardvark, but we check for whether > > > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > > > Interrupt from this function we achieve Link up event, so long as the > > > function is called (which it is after probe and when rescanning). > > > Although it is not ideal, it is better than nothing. > > > > So before even coming to the code review: this patch does two things. > > > > 1) It adds support for handling the Link down state > > 2) It adds some code to emulate a Link-up event > > > > Now, for (2). IIUC you are adding code to make sure that an HP > > event is triggered if advk_pcie_link_up() is called and it > > detects a Link-down->Link-up transition, that has to be notified > > through an HP event. > > > > If that's correct, you have to explain to me please what this is > > actually achieving and a specific scenario where we want this to be > > implemented, in fine details; then we add it to the commit log. > > Hello Lorenzo, sorry for not replying earlier. > > Would something like this be sufficient? > > DLLSC is needed by the pciehp driver, which handles hotplug, but also > link state change events. AFAIK no Aardvark devices support hotplug, > but link state change events are required for graceful driver > unbinding in case of link down event. > > So with this change we achieve graceful driver unbind for example > when WiFi card PCIe link goes down (we've seen this with ath10k and > mt76 WiFi cards). Before the WiFi driver started spitting out errors, > or even taking the whole system down. > > Since after link goes down, it can come back up if the WiFi card can > recover (or if reset pin is used to reset the card), we need to be > able to recognize link up event. Since AFAIK Aardvark does not have > interrupt for link up event, the best thing we can do is simulate it > - whenever we read the link state, find it is up, and have cached > value saying it is down, we trigger the link up event. We read link > state whenever the configuration space is read, for example by > writing 1 to /sys/bus/pci/rescan. Better, certainly. Question, also related to Marc's query. Do you rely on the hotplug (emulated IRQ) to be run _before_ carrying on with PCI config space accesses following a link-up detection ? How was the jiffies + 1 expiration time determined ? I assume you want to run the emulated HP IRQ asap - the question though is how fast should it be ? Lorenzo > > Marek > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello! Just briefly from my side, but Marek would probably answer it better. On Tuesday 27 September 2022 10:29:26 Lorenzo Pieralisi wrote: > Better, certainly. Question, also related to Marc's query. Do you > rely on the hotplug (emulated IRQ) to be run _before_ carrying on > with PCI config space accesses following a link-up detection ? During PCI config space access is PCI core code holding atomic raw spin lock, so link-up check from PCI config space can throw emulated HP IRQ only _after_ config space is finished (when IRQs are unmasked again). So it happens after (not before). > How was the jiffies + 1 expiration time determined ? jiffies + 1 was chosen as the earliest possible time when HP IRQ can be thrown. Somebody said to me (year or more ago, no remember who and when) that I cannot use just "jiffies", I have to use "jiffies + 1", so timer would be scheduled after my call finish, which is after PCI config space access finish. jiffies + 1 should be the earliest possible time with the highest priority. > I assume you > want to run the emulated HP IRQ asap - the question though is > how fast should it be ? HP IRQ should be thrown _ASAP_ when we know that link is up, so PCIe HP driver can handle it and do its job. Just like for hardware which fully and correctly supports link up HP IRQs.
On Mon, 26 Sep 2022 16:00:13 +0200 Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > On Mon, Sep 26, 2022 at 08:35:51AM -0400, Marc Zyngier wrote: > > On Mon, 26 Sep 2022 07:49:55 -0400, > > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > > > On Sat, Sep 17, 2022 at 10:05:59AM +0100, Marc Zyngier wrote: > > > > Hi Lorenzo, > > > > > > > > On Fri, 09 Sep 2022 15:57:11 +0100, > > > > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > > > > > > > [+Marc, Thomas - I can't merge this code without them reviewing it, > > > > > I am not sure at all you can mix the timer/IRQ code the way you do] > > > > > > > > > > On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote: > > > > > > From: Pali Rohár <pali@kernel.org> > > > > > > > > > > > > Add support for Data Link Layer State Change in the emulated slot > > > > > > registers and hotplug interrupt via the emulated root bridge. > > > > > > > > > > > > This is mainly useful for when an error causes link down event. With > > > > > > this change, drivers can try recovery. > > > > > > > > > > > > Link down state change can be implemented because Aardvark supports Link > > > > > > Down event interrupt. Use it for signaling that Data Link Layer Link is > > > > > > not active anymore via Hot-Plug Interrupt on emulated root bridge. > > > > > > > > > > > > Link up interrupt is not available on Aardvark, but we check for whether > > > > > > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug > > > > > > Interrupt from this function we achieve Link up event, so long as the > > > > > > function is called (which it is after probe and when rescanning). > > > > > > Although it is not ideal, it is better than nothing. > > > > > > > > > > So before even coming to the code review: this patch does two things. > > > > > > > > > > 1) It adds support for handling the Link down state > > > > > 2) It adds some code to emulate a Link-up event > > > > > > > > > > Now, for (2). IIUC you are adding code to make sure that an HP > > > > > event is triggered if advk_pcie_link_up() is called and it > > > > > detects a Link-down->Link-up transition, that has to be notified > > > > > through an HP event. > > > > > > > > > > If that's correct, you have to explain to me please what this is > > > > > actually achieving and a specific scenario where we want this to be > > > > > implemented, in fine details; then we add it to the commit log. > > > > > > > > > > That aside, the interaction of the timer and the IRQ domain code > > > > > must be reviewed by Marc and Thomas to make sure this is not > > > > > a gross violation of the respective subsystems usage. > > > > > > > > I don't see anything being a "gross violation" here, at least from an > > > > interrupt subsystem perspective. In a way, this is synthesising an > > > > interrupt on the back of some other event, and as long as the context > > > > is somehow appropriate (something that looks like an interrupt when > > > > pretending there is one), this should be OK. Other subsystems such as > > > > i2c GPIO expanders do similar things. > > > > > > Right, thanks. > > > > > > > The one thing I'm dubious about is the frequency of the timer. Asking > > > > for a poll of the link every jiffy is bound to be expensive, and it > > > > would be good to relax this as much as possible, specially on low-end > > > > HW such as this, where every cycle counts. It is always going to be a > > > > "best effort" thing, and the commit message doesn't say what's the > > > > actual grace period to handle this (the spec probably has one). > > > > > > AFAICS, the code does not poll the link. It sets a timer only if > > > the link is checked (eg upon PCI bus forced rescan or config access) > > > the link is up and it was down, to emulate a HP IRQ. > > > > I still find the timer frequency pretty high, but surely the authors > > of the code have worked out that this wasn't a problem. > > Please correct me if I am wrong but with mod_timer() all they want to do > is emulating/firing an (hotplug) IRQ as soon as possible - a one-off. > > It is not a periodic timer - at least that's what I understand from the > code and that's the reason why such a short interval was chosen but > it should not be me who comment on that :) This is true, it is not a periodic timer. We are just using an IRQSAFE timer to call generic_handle_domain_irq() from it's handler, because we can't do it during the config space access. Marek
On Tue, Sep 27, 2022 at 01:13:57PM +0200, Pali Rohár wrote: > Hello! Just briefly from my side, but Marek would probably answer it > better. > > On Tuesday 27 September 2022 10:29:26 Lorenzo Pieralisi wrote: > > Better, certainly. Question, also related to Marc's query. Do you > > rely on the hotplug (emulated IRQ) to be run _before_ carrying on > > with PCI config space accesses following a link-up detection ? > > During PCI config space access is PCI core code holding atomic raw spin > lock, so link-up check from PCI config space can throw emulated HP IRQ > only _after_ config space is finished (when IRQs are unmasked again). So > it happens after (not before). > > > How was the jiffies + 1 expiration time determined ? > > jiffies + 1 was chosen as the earliest possible time when HP IRQ can be > thrown. Somebody said to me (year or more ago, no remember who and > when) that I cannot use just "jiffies", I have to use "jiffies + 1", so > timer would be scheduled after my call finish, which is after PCI config > space access finish. jiffies + 1 should be the earliest possible time > with the highest priority. > > > I assume you > > want to run the emulated HP IRQ asap - the question though is > > how fast should it be ? > > HP IRQ should be thrown _ASAP_ when we know that link is up, so PCIe HP > driver can handle it and do its job. Just like for hardware which fully > and correctly supports link up HP IRQs. My question really is - what are we expecting the HP core code to fix up ? And related, is it safe to carry on with the PCI config space access till the IRQ is actually emulated to carry out those actions ? Lorenzo
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index d1c5fcf00a8a..5e8a84f5c654 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -21,6 +21,9 @@ config PCI_AARDVARK depends on OF depends on PCI_MSI_IRQ_DOMAIN select PCI_BRIDGE_EMUL + select PCIEPORTBUS + select HOTPLUG_PCI + select HOTPLUG_PCI_PCIE help Add support for Aardvark 64bit PCIe Host Controller. This controller is part of the South Bridge of the Marvel Armada diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 966c8b48bd96..31da28ebc5d1 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -25,6 +25,7 @@ #include <linux/of_address.h> #include <linux/of_gpio.h> #include <linux/of_pci.h> +#include <linux/timer.h> #include "../pci.h" #include "../pci-bridge-emul.h" @@ -100,6 +101,7 @@ #define PCIE_MSG_PM_PME_MASK BIT(7) #define PCIE_ISR0_MASK_REG (CONTROL_BASE_ADDR + 0x44) #define PCIE_ISR0_MSI_INT_PENDING BIT(24) +#define PCIE_ISR0_LINK_DOWN BIT(1) #define PCIE_ISR0_CORR_ERR BIT(11) #define PCIE_ISR0_NFAT_ERR BIT(12) #define PCIE_ISR0_FAT_ERR BIT(13) @@ -284,6 +286,8 @@ struct advk_pcie { DECLARE_BITMAP(msi_used, MSI_IRQ_NUM); struct mutex msi_used_lock; int link_gen; + bool link_was_up; + struct timer_list link_irq_timer; struct pci_bridge_emul bridge; struct gpio_desc *reset_gpio; struct phy *phy; @@ -313,7 +317,24 @@ static inline bool advk_pcie_link_up(struct advk_pcie *pcie) { /* check if LTSSM is in normal operation - some L* state */ u8 ltssm_state = advk_pcie_ltssm_state(pcie); - return ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED; + bool link_is_up; + u16 slotsta; + + link_is_up = ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED; + + if (link_is_up && !pcie->link_was_up) { + dev_info(&pcie->pdev->dev, "link up\n"); + + pcie->link_was_up = true; + + slotsta = le16_to_cpu(pcie->bridge.pcie_conf.slotsta); + slotsta |= PCI_EXP_SLTSTA_DLLSC; + pcie->bridge.pcie_conf.slotsta = cpu_to_le16(slotsta); + + mod_timer(&pcie->link_irq_timer, jiffies + 1); + } + + return link_is_up; } static inline bool advk_pcie_link_active(struct advk_pcie *pcie) @@ -442,8 +463,6 @@ static void advk_pcie_train_link(struct advk_pcie *pcie) ret = advk_pcie_wait_for_link(pcie); if (ret < 0) dev_err(dev, "link never came up\n"); - else - dev_info(dev, "link up\n"); } /* @@ -592,6 +611,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) reg &= ~PCIE_ISR0_MSI_INT_PENDING; advk_writel(pcie, reg, PCIE_ISR0_MASK_REG); + /* Unmask Link Down interrupt */ + reg = advk_readl(pcie, PCIE_ISR0_MASK_REG); + reg &= ~PCIE_ISR0_LINK_DOWN; + advk_writel(pcie, reg, PCIE_ISR0_MASK_REG); + /* Unmask PME interrupt for processing of PME requester */ reg = advk_readl(pcie, PCIE_ISR0_MASK_REG); reg &= ~PCIE_MSG_PM_PME_MASK; @@ -918,6 +942,14 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, advk_pcie_wait_for_retrain(pcie); break; + case PCI_EXP_SLTCTL: { + u16 slotctl = le16_to_cpu(bridge->pcie_conf.slotctl); + /* Only emulation of HPIE and DLLSCE bits is provided */ + slotctl &= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE; + bridge->pcie_conf.slotctl = cpu_to_le16(slotctl); + break; + } + case PCI_EXP_RTCTL: { u16 rootctl = le16_to_cpu(bridge->pcie_conf.rootctl); /* Only emulation of PMEIE and CRSSVE bits is provided */ @@ -1035,6 +1067,7 @@ static const struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = { static int advk_sw_pci_bridge_init(struct advk_pcie *pcie) { struct pci_bridge_emul *bridge = &pcie->bridge; + u32 slotcap; bridge->conf.vendor = cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff); @@ -1061,6 +1094,13 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie) bridge->pcie_conf.cap = cpu_to_le16(2 | PCI_EXP_FLAGS_SLOT); /* + * Mark bridge as Hot Plug Capable since this is the way how to enable + * delivering of Data Link Layer State Change interrupts. + * + * Set No Command Completed Support because bridge does not support + * Command Completed Interrupt. Every command is executed immediately + * without any delay. + * * Set Presence Detect State bit permanently since there is no support * for unplugging the card nor detecting whether it is plugged. (If a * platform exists in the future that supports it, via a GPIO for @@ -1070,8 +1110,9 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie) * value is reserved for ports within the same silicon as Root Port * which is not our case. */ - bridge->pcie_conf.slotcap = cpu_to_le32(FIELD_PREP(PCI_EXP_SLTCAP_PSN, - 1)); + slotcap = PCI_EXP_SLTCAP_NCCS | PCI_EXP_SLTCAP_HPC | + FIELD_PREP(PCI_EXP_SLTCAP_PSN, 1); + bridge->pcie_conf.slotcap = cpu_to_le32(slotcap); bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS); /* Indicates supports for Completion Retry Status */ @@ -1568,6 +1609,24 @@ static void advk_pcie_remove_rp_irq_domain(struct advk_pcie *pcie) irq_domain_remove(pcie->rp_irq_domain); } +static void advk_pcie_link_irq_handler(struct timer_list *timer) +{ + struct advk_pcie *pcie = from_timer(pcie, timer, link_irq_timer); + u16 slotctl; + + slotctl = le16_to_cpu(pcie->bridge.pcie_conf.slotctl); + if (!(slotctl & PCI_EXP_SLTCTL_DLLSCE) || + !(slotctl & PCI_EXP_SLTCTL_HPIE)) + return; + + /* + * Aardvark HW returns zero for PCI_EXP_FLAGS_IRQ, so use PCIe + * interrupt 0 + */ + if (generic_handle_domain_irq(pcie->rp_irq_domain, 0) == -EINVAL) + dev_err_ratelimited(&pcie->pdev->dev, "unhandled HP IRQ\n"); +} + static void advk_pcie_handle_pme(struct advk_pcie *pcie) { u32 requester = advk_readl(pcie, PCIE_MSG_LOG_REG) >> 16; @@ -1619,6 +1678,7 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie) { u32 isr0_val, isr0_mask, isr0_status; u32 isr1_val, isr1_mask, isr1_status; + u16 slotsta; int i; isr0_val = advk_readl(pcie, PCIE_ISR0_REG); @@ -1645,6 +1705,26 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie) dev_err_ratelimited(&pcie->pdev->dev, "unhandled ERR IRQ\n"); } + /* Process Link Down interrupt as HP IRQ */ + if (isr0_status & PCIE_ISR0_LINK_DOWN) { + advk_writel(pcie, PCIE_ISR0_LINK_DOWN, PCIE_ISR0_REG); + + dev_info(&pcie->pdev->dev, "link down\n"); + + pcie->link_was_up = false; + + slotsta = le16_to_cpu(pcie->bridge.pcie_conf.slotsta); + slotsta |= PCI_EXP_SLTSTA_DLLSC; + pcie->bridge.pcie_conf.slotsta = cpu_to_le16(slotsta); + + /* + * Deactivate timer and call advk_pcie_link_irq_handler() + * function directly as we are in the interrupt context. + */ + del_timer_sync(&pcie->link_irq_timer); + advk_pcie_link_irq_handler(&pcie->link_irq_timer); + } + /* Process MSI interrupts */ if (isr0_status & PCIE_ISR0_MSI_INT_PENDING) advk_pcie_handle_msi(pcie); @@ -1881,6 +1961,14 @@ static int advk_pcie_probe(struct platform_device *pdev) if (ret) return ret; + /* + * generic_handle_domain_irq() expects local IRQs to be disabled since + * normally it is called from interrupt context, so use TIMER_IRQSAFE + * flag for this link_irq_timer. + */ + timer_setup(&pcie->link_irq_timer, advk_pcie_link_irq_handler, + TIMER_IRQSAFE); + advk_pcie_setup_hw(pcie); ret = advk_sw_pci_bridge_init(pcie); @@ -1969,6 +2057,9 @@ static int advk_pcie_remove(struct platform_device *pdev) advk_pcie_remove_msi_irq_domain(pcie); advk_pcie_remove_irq_domain(pcie); + /* Deactivate link event timer */ + del_timer_sync(&pcie->link_irq_timer); + /* Free config space for emulated root bridge */ pci_bridge_emul_cleanup(&pcie->bridge);