[PATCH 00/11] PCI: aardvark controller changes BATCH 6

[PATCH 00/11] PCI: aardvark controller changes BATCH 6

[Marek Behún]

Hello Lorenzo,

here is continuation of Aardvark patches.

Some patches come from those not applied in batch 5, and were rebased.

Patches 1 and 2 touch the pciehp driver. We have changed commit
messages, since originally we thought the patches are also needed
to fix a bug, but this turns out not to be true [1].

Patch 3 was changed to also select the hotplug support in Kconfig.

Patch 7 (suspend support) was changed to use new macro
NOIRQ_SYSTEM_SLEEP_PM_OPS, and also changed commit message.

Patches 8-11 are new.

[1] https://lore.kernel.org/linux-pci/20220818142243.4c046c59@dellmb/T/#u

Marek Behún (2):
  PCI: aardvark: Don't write read-only bits explicitly in PCI_ERR_CAP
    register
  PCI: aardvark: Explicitly disable Marvell strict ordering

Miquel Raynal (2):
  PCI: aardvark: Add clock support
  PCI: aardvark: Add suspend to RAM support

Pali Rohár (7):
  PCI: pciehp: Enable DLLSC interrupt only if supported
  PCI: pciehp: Enable Command Completed Interrupt only if supported
  PCI: aardvark: Add support for DLLSC and hotplug interrupt
  PCI: aardvark: Send Set_Slot_Power_Limit message
  arm64: dts: armada-3720-turris-mox: Define slot-power-limit-milliwatt
    for PCIe
  PCI: aardvark: Replace custom PCIE_CORE_ERR_CAPCTL_* macros by
    linux/pci_regs.h macros
  PCI: aardvark: Cleanup some register macros

 .../dts/marvell/armada-3720-turris-mox.dts    |   1 +
 drivers/pci/controller/pci-aardvark.c         | 258 +++++++++++++++---
 drivers/pci/hotplug/pciehp_hpc.c              |  34 ++-
 drivers/pci/hotplug/pnv_php.c                 |  13 +-
 4 files changed, 261 insertions(+), 45 deletions(-)

-- 
2.35.1

[PATCH 01/11] PCI: pciehp: Enable DLLSC interrupt only if supported

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Don't enable Data Link Layer State Changed interrupt if it isn't
supported.

Data Link Layer Link Active Reporting Capable bit in Link Capabilities
register indicates if Data Link Layer State Changed Enable is supported.

Although Lukas Wunner says [1]
  According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
  Downstream Port [...], this bit must be hardwired to 1b."
the reason we want this is because of the pci-bridge-emul driver, which
emulates a bridge, but does not support asynchronous operations (since
implementing them is unneeded and would require massive changes to the
whole driver). Therefore enabling DLLSC unconditionally makes the
corresponding bit set only in the emulated configuration space of the
pci-bridge-emul driver, which
- results in confusing information when dumping the config space (it
  says that the interrupt is not supported but enabled), which may
  confuse developers when debugging PCIe issues,
- may cause bugs in the future if someone adds code that checks whether
  DLLSC is enabled and then waits for the interrupt.

[1] https://www.spinics.net/lists/linux-pci/msg124727.html

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
Changes since batch 5:
- changed commit message, previously we wrote that the change is needed
  to fix a bug where kernel was waiting for an event which did not
  come. This turns out to be false. See
  https://lore.kernel.org/linux-pci/20220818142243.4c046c59@dellmb/T/#u
---
 drivers/pci/hotplug/pciehp_hpc.c | 30 +++++++++++++++++++++++-------
 drivers/pci/hotplug/pnv_php.c    | 13 +++++++++----
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 040ae076ec0e..373bb396fe22 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -788,6 +788,7 @@ static int pciehp_poll(void *data)
 static void pcie_enable_notification(struct controller *ctrl)
 {
 	u16 cmd, mask;
+	u32 link_cap;
 
 	/*
 	 * TBD: Power fault detected software notification support.
@@ -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;
 	if (ATTN_BUTTN(ctrl))
 		cmd |= PCI_EXP_SLTCTL_ABPE;
 	else
@@ -844,9 +850,14 @@ void pcie_clear_hotplug_events(struct controller *ctrl)
 
 void pcie_enable_interrupt(struct controller *ctrl)
 {
+	u32 link_cap;
 	u16 mask;
 
-	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+	pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap);
+
+	mask = PCI_EXP_SLTCTL_HPIE;
+	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
+		mask |= PCI_EXP_SLTCTL_DLLSCE;
 	pcie_write_cmd(ctrl, mask, mask);
 }
 
@@ -904,19 +915,24 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, bool probe)
 	struct controller *ctrl = to_ctrl(hotplug_slot);
 	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 stat_mask = 0, ctrl_mask = 0;
+	u32 link_cap;
 	int rc;
 
 	if (probe)
 		return 0;
 
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
+
 	down_write_nested(&ctrl->reset_lock, ctrl->depth);
 
 	if (!ATTN_BUTTN(ctrl)) {
 		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
 		stat_mask |= PCI_EXP_SLTSTA_PDC;
 	}
-	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
-	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
+	if (link_cap & PCI_EXP_LNKCAP_DLLLARC) {
+		ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
+		stat_mask |= PCI_EXP_SLTSTA_DLLSC;
+	}
 
 	pcie_write_cmd(ctrl, 0, ctrl_mask);
 	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index 881d420637bf..118b514f66b9 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -841,6 +841,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 */
@@ -874,17 +875,21 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
 		return;
 	}
 
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
+
 	/* Enable the interrupts */
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
 	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
 		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
-		ctrl |= (PCI_EXP_SLTCTL_HPIE |
-			 PCI_EXP_SLTCTL_DLLSCE);
+		ctrl |= PCI_EXP_SLTCTL_HPIE;
 	} else {
 		ctrl |= (PCI_EXP_SLTCTL_HPIE |
-			 PCI_EXP_SLTCTL_PDCE |
-			 PCI_EXP_SLTCTL_DLLSCE);
+			 PCI_EXP_SLTCTL_PDCE);
 	}
+	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
+		ctrl |= PCI_EXP_SLTCTL_DLLSCE;
+	else
+		ctrl &= ~PCI_EXP_SLTCTL_DLLSCE;
 	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);
 
 	/* The interrupt is initialized successfully when @irq is valid */
-- 
2.35.1

Re: [PATCH 01/11] PCI: pciehp: Enable DLLSC interrupt only if supported

[Lukas Wunner]

On Thu, Aug 18, 2022 at 03:51:30PM +0200, Marek Behún wrote:
> Don't enable Data Link Layer State Changed interrupt if it isn't
> supported.
> 
> Data Link Layer Link Active Reporting Capable bit in Link Capabilities
> register indicates if Data Link Layer State Changed Enable is supported.
> 
> Although Lukas Wunner says [1]
>   According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
>   Downstream Port [...], this bit must be hardwired to 1b."
> the reason we want this is because of the pci-bridge-emul driver, which
> emulates a bridge, but does not support asynchronous operations (since
> implementing them is unneeded and would require massive changes to the
> whole driver). Therefore enabling DLLSC unconditionally makes the
> corresponding bit set only in the emulated configuration space of the
> pci-bridge-emul driver, which
> - results in confusing information when dumping the config space (it
>   says that the interrupt is not supported but enabled), which may
>   confuse developers when debugging PCIe issues,
> - may cause bugs in the future if someone adds code that checks whether
>   DLLSC is enabled and then waits for the interrupt.

Honestly I'm not sure this change adds value or is necessary:

advk_pci_bridge_emul_pcie_conf_read() unconditionally sets the DLLLARC
bit, so the change doesn't have any effect for aardvark.

Same for mvebu_pci_bridge_emul_pcie_conf_read().
There are no other drivers using pci-bridge-emul.

Apart from that, it is legal to set the DLLSCE bit even on PCIe r1.0,
which did not define Data Link Layer Link Active Reporting yet.
(It defined the bit RsvdP.)  Thus there's no reason for developers
to be confused.

We're also never depending *exclusively* on DLLSC events in pciehp,
we always react to either of PDC or DLLSC, whichever comes first.
So I don't see enabling DLLSCE on unsupporting hardware as a
potential source of error.


> [1] https://www.spinics.net/lists/linux-pci/msg124727.html

Please always use lore.kernel.org links as they will likely outlast
3rd party archives:

https://lore.kernel.org/linux-pci/20220509034216.GA26780@wunner.de/


> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
[...]
> +	pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap);
> +

Unfortunately this new version of the patch does not address
one of my comments on the previous version:

"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."

(Verbatim quote from the above-linked e-mail.)


> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -841,6 +841,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 */
> @@ -874,17 +875,21 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
>  		return;
>  	}
>  
> +	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
> +
>  	/* Enable the interrupts */
>  	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
>  	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
>  		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
> -		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> -			 PCI_EXP_SLTCTL_DLLSCE);
> +		ctrl |= PCI_EXP_SLTCTL_HPIE;
>  	} else {
>  		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> -			 PCI_EXP_SLTCTL_PDCE |
> -			 PCI_EXP_SLTCTL_DLLSCE);
> +			 PCI_EXP_SLTCTL_PDCE);
>  	}
> +	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
> +		ctrl |= PCI_EXP_SLTCTL_DLLSCE;
> +	else
> +		ctrl &= ~PCI_EXP_SLTCTL_DLLSCE;
>  	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);

Note that the pnv_php.c driver is relying on DLLSC here if PDC
is broken (see PNV_PHP_FLAG_BROKEN_PDC).  By not enabling DLLSC,
you may break hotplug altogether.

pnv_php.c is a PowerPC-specific hotplug controller, but you're not
cc'ing the driver's maintainers, which are:

$ scripts/get_maintainer.pl drivers/pci/hotplug/pnv_php.c
Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
Nicholas Piggin <npiggin@gmail.com> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
Christophe Leroy <christophe.leroy@csgroup.eu> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC (32-BIT AND 64-BIT))

Thanks,

Lukas

Re: [PATCH 01/11] PCI: pciehp: Enable DLLSC interrupt only if supported

[Marek Behún]

On Sun, 21 Aug 2022 14:46:21 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Thu, Aug 18, 2022 at 03:51:30PM +0200, Marek Behún wrote:
> > Don't enable Data Link Layer State Changed interrupt if it isn't
> > supported.
> > 
> > Data Link Layer Link Active Reporting Capable bit in Link Capabilities
> > register indicates if Data Link Layer State Changed Enable is supported.
> > 
> > Although Lukas Wunner says [1]
> >   According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
> >   Downstream Port [...], this bit must be hardwired to 1b."
> > the reason we want this is because of the pci-bridge-emul driver, which
> > emulates a bridge, but does not support asynchronous operations (since
> > implementing them is unneeded and would require massive changes to the
> > whole driver). Therefore enabling DLLSC unconditionally makes the
> > corresponding bit set only in the emulated configuration space of the
> > pci-bridge-emul driver, which
> > - results in confusing information when dumping the config space (it
> >   says that the interrupt is not supported but enabled), which may
> >   confuse developers when debugging PCIe issues,
> > - may cause bugs in the future if someone adds code that checks whether
> >   DLLSC is enabled and then waits for the interrupt.  
> 
> Honestly I'm not sure this change adds value or is necessary:
> 
> advk_pci_bridge_emul_pcie_conf_read() unconditionally sets the DLLLARC
> bit, so the change doesn't have any effect for aardvark.

This is the status now, but it wasn't always so. The support for
that was added in one of the previous batches of aardvark's changes.

> Same for mvebu_pci_bridge_emul_pcie_conf_read().
> There are no other drivers using pci-bridge-emul.

But there may be future users which won't support it and we were just
thinking this could spare some confusion to the developers, since Pali
spent nontrivial time on this when developing/debuggin aardvark last
year.

> Apart from that, it is legal to set the DLLSCE bit even on PCIe r1.0,
> which did not define Data Link Layer Link Active Reporting yet.
> (It defined the bit RsvdP.)  Thus there's no reason for developers
> to be confused.
> 
> We're also never depending *exclusively* on DLLSC events in pciehp,
> we always react to either of PDC or DLLSC, whichever comes first.
> So I don't see enabling DLLSCE on unsupporting hardware as a
> potential source of error.
> 
> 
> > [1] https://www.spinics.net/lists/linux-pci/msg124727.html  
> 
> Please always use lore.kernel.org links as they will likely outlast
> 3rd party archives:
> 
> https://lore.kernel.org/linux-pci/20220509034216.GA26780@wunner.de/

Will do.

> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c  
> [...]
> > +	pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap);
> > +  
> 
> Unfortunately this new version of the patch does not address
> one of my comments on the previous version:
> 
> "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."
> 
> (Verbatim quote from the above-linked e-mail.)

Sorry, I forgot abbout this one.

> > --- a/drivers/pci/hotplug/pnv_php.c
> > +++ b/drivers/pci/hotplug/pnv_php.c
> > @@ -841,6 +841,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 */
> > @@ -874,17 +875,21 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
> >  		return;
> >  	}
> >  
> > +	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
> > +
> >  	/* Enable the interrupts */
> >  	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
> >  	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
> >  		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
> > -		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> > -			 PCI_EXP_SLTCTL_DLLSCE);
> > +		ctrl |= PCI_EXP_SLTCTL_HPIE;
> >  	} else {
> >  		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> > -			 PCI_EXP_SLTCTL_PDCE |
> > -			 PCI_EXP_SLTCTL_DLLSCE);
> > +			 PCI_EXP_SLTCTL_PDCE);
> >  	}
> > +	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
> > +		ctrl |= PCI_EXP_SLTCTL_DLLSCE;
> > +	else
> > +		ctrl &= ~PCI_EXP_SLTCTL_DLLSCE;
> >  	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);  
> 
> Note that the pnv_php.c driver is relying on DLLSC here if PDC
> is broken (see PNV_PHP_FLAG_BROKEN_PDC).  By not enabling DLLSC,
> you may break hotplug altogether.

But it would only break if those controllers did not report DLLLARC,
right?

> pnv_php.c is a PowerPC-specific hotplug controller, but you're not
> cc'ing the driver's maintainers, which are:
> 
> $ scripts/get_maintainer.pl drivers/pci/hotplug/pnv_php.c
> Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Nicholas Piggin <npiggin@gmail.com> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Christophe Leroy <christophe.leroy@csgroup.eu> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC (32-BIT AND 64-BIT))

Will do.

Anyway, Lorenzo, this patch can be skipped now, the other patches do
not depend on it.

Marek

[PATCH 02/11] PCI: pciehp: Enable Command Completed Interrupt only if supported

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

The No Command Completed Support bit in the Slot Capabilities register
indicates whether Command Completed Interrupt Enable is unsupported.

We already check whether No Command Completed Support bit is set in
pcie_wait_cmd(), and do not wait in this case.

Let's not enable this Command Completed Interrupt at all if NCCS is set,
so that when users dump configuration space from userspace, the dump
does not confuse them by saying that Command Completed Interrupt is not
supported, but it is enabled.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
Changes since batch 5:
- changed commit message, previously we wrote that the change is needed
  to fix a bug where kernel was waiting for an event which did not
  come. This turns out to be false. See
  https://lore.kernel.org/linux-pci/20220818142243.4c046c59@dellmb/T/#u
---
 drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 373bb396fe22..838eb6cc3ec7 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -817,7 +817,9 @@ static void pcie_enable_notification(struct controller *ctrl)
 	else
 		cmd |= PCI_EXP_SLTCTL_PDCE;
 	if (!pciehp_poll_mode)
-		cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE;
+		cmd |= PCI_EXP_SLTCTL_HPIE;
+	if (!pciehp_poll_mode && !NO_CMD_CMPL(ctrl))
+		cmd |= PCI_EXP_SLTCTL_CCIE;
 
 	mask = (PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE |
 		PCI_EXP_SLTCTL_PFDE |
-- 
2.35.1

Re: [PATCH 02/11] PCI: pciehp: Enable Command Completed Interrupt only if supported

[Lukas Wunner]

On Thu, Aug 18, 2022 at 03:51:31PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> The No Command Completed Support bit in the Slot Capabilities register
> indicates whether Command Completed Interrupt Enable is unsupported.
> 
> We already check whether No Command Completed Support bit is set in
> pcie_wait_cmd(), and do not wait in this case.
> 
> Let's not enable this Command Completed Interrupt at all if NCCS is set,
> so that when users dump configuration space from userspace, the dump
> does not confuse them by saying that Command Completed Interrupt is not
> supported, but it is enabled.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

I note however that this change isn't really necessary because CCIE
"must be hardwired to 0b" "If Command Completed notification is not
supported" per PCIe r6.0 sec 7.5.3.10.  It's purely a cosmetic issue.

Thanks,

Lukas

Re: [PATCH 02/11] PCI: pciehp: Enable Command Completed Interrupt only if supported

[Lorenzo Pieralisi]

On Thu, Aug 18, 2022 at 03:51:31PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> The No Command Completed Support bit in the Slot Capabilities register
> indicates whether Command Completed Interrupt Enable is unsupported.
> 
> We already check whether No Command Completed Support bit is set in
> pcie_wait_cmd(), and do not wait in this case.
> 
> Let's not enable this Command Completed Interrupt at all if NCCS is set,
> so that when users dump configuration space from userspace, the dump
> does not confuse them by saying that Command Completed Interrupt is not
> supported, but it is enabled.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> Changes since batch 5:
> - changed commit message, previously we wrote that the change is needed
>   to fix a bug where kernel was waiting for an event which did not
>   come. This turns out to be false. See
>   https://lore.kernel.org/linux-pci/20220818142243.4c046c59@dellmb/T/#u
> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Hi Bjorn,

this patch is mixed with aardvark specific changes,
please let me know if it is fine for you to merge it.

Thanks,
Lorenzo

> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 373bb396fe22..838eb6cc3ec7 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -817,7 +817,9 @@ static void pcie_enable_notification(struct controller *ctrl)
>  	else
>  		cmd |= PCI_EXP_SLTCTL_PDCE;
>  	if (!pciehp_poll_mode)
> -		cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE;
> +		cmd |= PCI_EXP_SLTCTL_HPIE;
> +	if (!pciehp_poll_mode && !NO_CMD_CMPL(ctrl))
> +		cmd |= PCI_EXP_SLTCTL_CCIE;
>  
>  	mask = (PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE |
>  		PCI_EXP_SLTCTL_PFDE |
> -- 
> 2.35.1
> 

[PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Marek Behún]

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.

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

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Lorenzo Pieralisi]

[+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

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Marek Behún]

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

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Lorenzo Pieralisi]

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

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Pali Rohár]

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.

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Lorenzo Pieralisi]

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

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Lorenzo Pieralisi]

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

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Lorenzo Pieralisi]

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

Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

[Marek Behún]

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

[PATCH 04/11] PCI: aardvark: Send Set_Slot_Power_Limit message

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Emulate Slot PowerLimit Scale and Value bits in the Slot Capabilities
register of the emulated bridge and if slot power limit value is
defined, send that Set_Slot_Power_Limit message via Message Generation
Control Register in Link Up handler on link up event.

Slot power limit value is read from device-tree property
'slot-power-limit-milliwatt'. If this property is not specified, we
treat it as "Slot Capabilities register has not yet been initialized".

According to PCIe Base specification 3.0, when transitioning from a
non-DL_Up Status to a DL_Up Status, the Port must initiate the
transmission of a Set_Slot_Power_Limit Message to the other component
on the Link to convey the value programmed in the Slot Power Limit
Scale and Value fields of the Slot Capabilities register. This
transmission is optional if the Slot Capabilities register has not
yet been initialized.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 51 ++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 31da28ebc5d1..03e318bc171f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -213,6 +213,11 @@ enum {
 };
 
 #define VENDOR_ID_REG				(LMI_BASE_ADDR + 0x44)
+#define PME_MSG_GEN_CTRL			(LMI_BASE_ADDR + 0x220)
+#define     SEND_SET_SLOT_POWER_LIMIT		BIT(13)
+#define     SEND_PME_TURN_OFF			BIT(14)
+#define     SLOT_POWER_LIMIT_DATA_SHIFT		16
+#define     SLOT_POWER_LIMIT_DATA_MASK		GENMASK(25, 16)
 
 /* PCIe core controller registers */
 #define CTRL_CORE_BASE_ADDR			0x18000
@@ -285,6 +290,8 @@ struct advk_pcie {
 	raw_spinlock_t msi_irq_lock;
 	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
 	struct mutex msi_used_lock;
+	u8 slot_power_limit_value;
+	u8 slot_power_limit_scale;
 	int link_gen;
 	bool link_was_up;
 	struct timer_list link_irq_timer;
@@ -317,8 +324,9 @@ 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);
+	u16 slotsta, slotctl;
+	u32 slotpwr, val;
 	bool link_is_up;
-	u16 slotsta;
 
 	link_is_up = ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED;
 
@@ -332,6 +340,27 @@ static inline bool advk_pcie_link_up(struct advk_pcie *pcie)
 		pcie->bridge.pcie_conf.slotsta = cpu_to_le16(slotsta);
 
 		mod_timer(&pcie->link_irq_timer, jiffies + 1);
+
+		/*
+		 * According to PCIe Base specification 3.0, when transitioning
+		 * from a non-DL_Up Status to a DL_Up Status, the Port must
+		 * initiate the transmission of a Set_Slot_Power_Limit Message
+		 * to the other component on the Link to convey the value
+		 * programmed in the Slot Power Limit Scale and Value fields of
+		 * the Slot Capabilities register. This transmission is optional
+		 * if the Slot Capabilities register has not yet been
+		 * initialized.
+		 */
+		slotctl = le16_to_cpu(pcie->bridge.pcie_conf.slotctl);
+		slotpwr = FIELD_GET(PCI_EXP_SLTCAP_SPLV | PCI_EXP_SLTCAP_SPLS,
+				    le32_to_cpu(pcie->bridge.pcie_conf.slotcap));
+		if (!(slotctl & PCI_EXP_SLTCTL_ASPL_DISABLE) && slotpwr) {
+			val = advk_readl(pcie, PME_MSG_GEN_CTRL);
+			val &= ~SLOT_POWER_LIMIT_DATA_MASK;
+			val |= slotpwr << SLOT_POWER_LIMIT_DATA_SHIFT;
+			val |= SEND_SET_SLOT_POWER_LIMIT;
+			advk_writel(pcie, val, PME_MSG_GEN_CTRL);
+		}
 	}
 
 	return link_is_up;
@@ -944,8 +973,9 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 
 	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;
+		/* Only emulation of HPIE, DLLSCE and ASPLD bits is provided */
+		slotctl &= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE |
+			   PCI_EXP_SLTCTL_ASPL_DISABLE;
 		bridge->pcie_conf.slotctl = cpu_to_le16(slotctl);
 		break;
 	}
@@ -1109,9 +1139,13 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 	 * Set physical slot number to 1 since there is only one port and zero
 	 * value is reserved for ports within the same silicon as Root Port
 	 * which is not our case.
+	 *
+	 * Set slot power limit.
 	 */
 	slotcap = PCI_EXP_SLTCAP_NCCS | PCI_EXP_SLTCAP_HPC |
-		  FIELD_PREP(PCI_EXP_SLTCAP_PSN, 1);
+		  FIELD_PREP(PCI_EXP_SLTCAP_PSN, 1) |
+		  FIELD_PREP(PCI_EXP_SLTCAP_SPLV, pcie->slot_power_limit_value) |
+		  FIELD_PREP(PCI_EXP_SLTCAP_SPLS, pcie->slot_power_limit_scale);
 	bridge->pcie_conf.slotcap = cpu_to_le32(slotcap);
 	bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS);
 
@@ -1837,6 +1871,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	struct advk_pcie *pcie;
 	struct pci_host_bridge *bridge;
 	struct resource_entry *entry;
+	u32 slot_power_limit;
 	int ret, irq;
 
 	bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct advk_pcie));
@@ -1957,6 +1992,14 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	else
 		pcie->link_gen = ret;
 
+	slot_power_limit = of_pci_get_slot_power_limit(dev->of_node,
+						       &pcie->slot_power_limit_value,
+						       &pcie->slot_power_limit_scale);
+	if (slot_power_limit)
+		dev_info(dev, "Slot Power Limit: %u.%uW\n",
+			 slot_power_limit / 1000,
+			 (slot_power_limit / 100) % 10);
+
 	ret = advk_pcie_setup_phy(pcie);
 	if (ret)
 		return ret;
-- 
2.35.1

[PATCH 05/11] arm64: dts: armada-3720-turris-mox: Define slot-power-limit-milliwatt for PCIe

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

PCIe Slot Power Limit on Turris Mox is 10W.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index ada164d423f3..5d2b221dbd96 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -136,6 +136,7 @@ &pcie0 {
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
 	status = "okay";
 	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
+	slot-power-limit-milliwatt = <10000>;
 	/*
 	 * U-Boot port for Turris Mox has a bug which always expects that "ranges" DT property
 	 * contains exactly 2 ranges with 3 (child) address cells, 2 (parent) address cells and
-- 
2.35.1

[PATCH 06/11] PCI: aardvark: Add clock support

[Marek Behún]

From: Miquel Raynal <miquel.raynal@bootlin.com>

The IP relies on a gated clock. When we will add S2RAM support, this
clock will need to be resumed before any PCIe registers are
accessed. Add support for this clock.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 03e318bc171f..3beafc893969 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
@@ -297,6 +298,7 @@ struct advk_pcie {
 	struct timer_list link_irq_timer;
 	struct pci_bridge_emul bridge;
 	struct gpio_desc *reset_gpio;
+	struct clk *clk;
 	struct phy *phy;
 };
 
@@ -1809,6 +1811,29 @@ static int advk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 		return of_irq_parse_and_map_pci(dev, slot, pin);
 }
 
+static int advk_pcie_setup_clk(struct advk_pcie *pcie)
+{
+	struct device *dev = &pcie->pdev->dev;
+	int ret;
+
+	pcie->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pcie->clk) && (PTR_ERR(pcie->clk) == -EPROBE_DEFER))
+		return PTR_ERR(pcie->clk);
+
+	/* Old bindings miss the clock handle */
+	if (IS_ERR(pcie->clk)) {
+		dev_warn(dev, "Clock unavailable (%ld)\n", PTR_ERR(pcie->clk));
+		pcie->clk = NULL;
+		return 0;
+	}
+
+	ret = clk_prepare_enable(pcie->clk);
+	if (ret)
+		dev_err(dev, "Clock initialization failed (%d)\n", ret);
+
+	return ret;
+}
+
 static void advk_pcie_disable_phy(struct advk_pcie *pcie)
 {
 	phy_power_off(pcie->phy);
@@ -2000,6 +2025,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
 			 slot_power_limit / 1000,
 			 (slot_power_limit / 100) % 10);
 
+	ret = advk_pcie_setup_clk(pcie);
+	if (ret)
+		return ret;
+
 	ret = advk_pcie_setup_phy(pcie);
 	if (ret)
 		return ret;
@@ -2122,6 +2151,9 @@ static int advk_pcie_remove(struct platform_device *pdev)
 	/* Disable phy */
 	advk_pcie_disable_phy(pcie);
 
+	/* Disable clock */
+	clk_disable_unprepare(pcie->clk);
+
 	return 0;
 }
 
-- 
2.35.1

[PATCH 07/11] PCI: aardvark: Add suspend to RAM support

[Marek Behún]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Add suspend and resume callbacks. We need to use the NOIRQ variants to
ensure the controller's IRQ handlers are not run during suspend() /
resume(), which could cause races.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
Changes since batch 5:
- clarified commit message
- changed to new macro NOIRQ_SYSTEM_SLEEP_PM_OPS, as was done for many
  PCI controller drivers with commit 19b7858c3357 ("PCI: Convert to new
  *_PM_OPS macros")
---
 drivers/pci/controller/pci-aardvark.c | 34 +++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 3beafc893969..e30a33a4ecc6 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1890,6 +1890,39 @@ static int advk_pcie_setup_phy(struct advk_pcie *pcie)
 	return ret;
 }
 
+static int advk_pcie_suspend(struct device *dev)
+{
+	struct advk_pcie *pcie = dev_get_drvdata(dev);
+
+	advk_pcie_disable_phy(pcie);
+
+	clk_disable_unprepare(pcie->clk);
+
+	return 0;
+}
+
+static int advk_pcie_resume(struct device *dev)
+{
+	struct advk_pcie *pcie = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(pcie->clk);
+	if (ret)
+		return ret;
+
+	ret = advk_pcie_enable_phy(pcie);
+	if (ret)
+		return ret;
+
+	advk_pcie_setup_hw(pcie);
+
+	return 0;
+}
+
+static const struct dev_pm_ops advk_pcie_dev_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(advk_pcie_suspend, advk_pcie_resume)
+};
+
 static int advk_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -2167,6 +2200,7 @@ static struct platform_driver advk_pcie_driver = {
 	.driver = {
 		.name = "advk-pcie",
 		.of_match_table = advk_pcie_of_match_table,
+		.pm = &advk_pcie_dev_pm_ops,
 	},
 	.probe = advk_pcie_probe,
 	.remove = advk_pcie_remove,
-- 
2.35.1

Re: [PATCH 07/11] PCI: aardvark: Add suspend to RAM support

[Lorenzo Pieralisi]

On Thu, Aug 18, 2022 at 03:51:36PM +0200, Marek Behún wrote:
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Add suspend and resume callbacks. We need to use the NOIRQ variants to
> ensure the controller's IRQ handlers are not run during suspend() /
> resume(), which could cause races.

Be more explicit please, which races, which IRQ handlers. This is useful
information for future testers/reviewers in case something has to be
changed, thanks.

Lorenzo

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> Changes since batch 5:
> - clarified commit message
> - changed to new macro NOIRQ_SYSTEM_SLEEP_PM_OPS, as was done for many
>   PCI controller drivers with commit 19b7858c3357 ("PCI: Convert to new
>   *_PM_OPS macros")
> ---
>  drivers/pci/controller/pci-aardvark.c | 34 +++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 3beafc893969..e30a33a4ecc6 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1890,6 +1890,39 @@ static int advk_pcie_setup_phy(struct advk_pcie *pcie)
>  	return ret;
>  }
>  
> +static int advk_pcie_suspend(struct device *dev)
> +{
> +	struct advk_pcie *pcie = dev_get_drvdata(dev);
> +
> +	advk_pcie_disable_phy(pcie);
> +
> +	clk_disable_unprepare(pcie->clk);
> +
> +	return 0;
> +}
> +
> +static int advk_pcie_resume(struct device *dev)
> +{
> +	struct advk_pcie *pcie = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(pcie->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = advk_pcie_enable_phy(pcie);
> +	if (ret)
> +		return ret;
> +
> +	advk_pcie_setup_hw(pcie);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops advk_pcie_dev_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(advk_pcie_suspend, advk_pcie_resume)
> +};
> +
>  static int advk_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -2167,6 +2200,7 @@ static struct platform_driver advk_pcie_driver = {
>  	.driver = {
>  		.name = "advk-pcie",
>  		.of_match_table = advk_pcie_of_match_table,
> +		.pm = &advk_pcie_dev_pm_ops,
>  	},
>  	.probe = advk_pcie_probe,
>  	.remove = advk_pcie_remove,
> -- 
> 2.35.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] PCI: aardvark: Add suspend to RAM support

[Lorenzo Pieralisi]

On Fri, Sep 09, 2022 at 12:33:01PM +0200, Lorenzo Pieralisi wrote:
> On Thu, Aug 18, 2022 at 03:51:36PM +0200, Marek Behún wrote:
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > 
> > Add suspend and resume callbacks. We need to use the NOIRQ variants to
> > ensure the controller's IRQ handlers are not run during suspend() /
> > resume(), which could cause races.
> 
> Be more explicit please, which races, which IRQ handlers. This is useful
> information for future testers/reviewers in case something has to be
> changed, thanks.

Can we update the log please ?

Thanks,
Lorenzo

> Lorenzo
> 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> > Changes since batch 5:
> > - clarified commit message
> > - changed to new macro NOIRQ_SYSTEM_SLEEP_PM_OPS, as was done for many
> >   PCI controller drivers with commit 19b7858c3357 ("PCI: Convert to new
> >   *_PM_OPS macros")
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 34 +++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 3beafc893969..e30a33a4ecc6 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1890,6 +1890,39 @@ static int advk_pcie_setup_phy(struct advk_pcie *pcie)
> >  	return ret;
> >  }
> >  
> > +static int advk_pcie_suspend(struct device *dev)
> > +{
> > +	struct advk_pcie *pcie = dev_get_drvdata(dev);
> > +
> > +	advk_pcie_disable_phy(pcie);
> > +
> > +	clk_disable_unprepare(pcie->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int advk_pcie_resume(struct device *dev)
> > +{
> > +	struct advk_pcie *pcie = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(pcie->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = advk_pcie_enable_phy(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	advk_pcie_setup_hw(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops advk_pcie_dev_pm_ops = {
> > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(advk_pcie_suspend, advk_pcie_resume)
> > +};
> > +
> >  static int advk_pcie_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -2167,6 +2200,7 @@ static struct platform_driver advk_pcie_driver = {
> >  	.driver = {
> >  		.name = "advk-pcie",
> >  		.of_match_table = advk_pcie_of_match_table,
> > +		.pm = &advk_pcie_dev_pm_ops,
> >  	},
> >  	.probe = advk_pcie_probe,
> >  	.remove = advk_pcie_remove,
> > -- 
> > 2.35.1
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/11] PCI: aardvark: Replace custom PCIE_CORE_ERR_CAPCTL_* macros by linux/pci_regs.h macros

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Kernel already has these macros defined under different names.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e30a33a4ecc6..4855ac733484 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -37,11 +37,7 @@
 #define PCIE_CORE_DEV_REV_REG					0x8
 #define PCIE_CORE_PCIEXP_CAP					0xc0
 #define PCIE_CORE_PCIERR_CAP					0x100
-#define PCIE_CORE_ERR_CAPCTL_REG				0x118
-#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX			BIT(5)
-#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN			BIT(6)
-#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK			BIT(7)
-#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV			BIT(8)
+
 /* PIO registers base address and register offsets */
 #define PIO_BASE_ADDR				0x4000
 #define PIO_CTRL				(PIO_BASE_ADDR + 0x0)
@@ -589,11 +585,9 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
 
 	/* Set Advanced Error Capabilities and Control PF0 register */
-	reg = PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX |
-		PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN |
-		PCIE_CORE_ERR_CAPCTL_ECRC_CHCK |
-		PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV;
-	advk_writel(pcie, reg, PCIE_CORE_ERR_CAPCTL_REG);
+	reg = PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_GENE |
+	      PCI_ERR_CAP_ECRC_CHKC | PCI_ERR_CAP_ECRC_CHKE;
+	advk_writel(pcie, reg, PCIE_CORE_PCIERR_CAP + PCI_ERR_CAP);
 
 	/* Set PCIe Device Control register */
 	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
-- 
2.35.1

[PATCH 09/11] PCI: aardvark: Don't write read-only bits explicitly in PCI_ERR_CAP register

[Marek Behún]

The bits PCI_ERR_CAP_ECRC_GENC and PCI_ERR_CAP_ECRC_CHKC are read only,
reporting the capability of ECRC. Don't write them explicitly, instead
read the register (where they are set), and add the bits that enable
these features.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 4855ac733484..e816ab726f66 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -584,9 +584,9 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
 	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
 
-	/* Set Advanced Error Capabilities and Control PF0 register */
-	reg = PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_GENE |
-	      PCI_ERR_CAP_ECRC_CHKC | PCI_ERR_CAP_ECRC_CHKE;
+	/* Enable generation and checking of ECRC on Root Bridge */
+	reg = advk_readl(pcie, PCIE_CORE_PCIERR_CAP + PCI_ERR_CAP);
+	reg |= PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE;
 	advk_writel(pcie, reg, PCIE_CORE_PCIERR_CAP + PCI_ERR_CAP);
 
 	/* Set PCIe Device Control register */
-- 
2.35.1

[PATCH 10/11] PCI: aardvark: Explicitly disable Marvell strict ordering

[Marek Behún]

Instead of implicitly disabling BIT(5) (STRICT_ORDER_ENABLE bit) of the
CORE_CTRL2 by writing PCIE_CORE_CTRL2_RESERVED |
PCIE_CORE_CTRL2_TD_ENABLE to it, disable it explicitly, with
read-modify-write.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e816ab726f66..73a604f70f06 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -600,8 +600,8 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
 
 	/* Program PCIe Control 2 to disable strict ordering */
-	reg = PCIE_CORE_CTRL2_RESERVED |
-		PCIE_CORE_CTRL2_TD_ENABLE;
+	reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG);
+	reg &= ~PCIE_CORE_CTRL2_STRICT_ORDER_ENABLE;
 	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
 
 	/* Set lane X1 */
-- 
2.35.1

[PATCH 11/11] PCI: aardvark: Cleanup some register macros

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Define SPEED_GEN_* macros with correct PCIE_GEN_SEL_SHIFT.

Simplify macro for setting root complex mode (use BIT instead of
MSK + SHIFT).

Rename PCIE_MSG_PM_PME_MASK to PCIE_ISR0_MSG_PM_PME to match existing
naming convention, rename PCIE_ISR0_MSI_INT_PENDING to PCIE_ISR0_MSI_INT
as it is used for both interrupt mask and pending bit.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 28 +++++++++++++--------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 73a604f70f06..11afafe71e3d 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -66,11 +66,10 @@
 #define PCIE_CORE_CTRL0_REG			(CONTROL_BASE_ADDR + 0x0)
 #define     PCIE_GEN_SEL_MSK			0x3
 #define     PCIE_GEN_SEL_SHIFT			0x0
-#define     SPEED_GEN_1				0
-#define     SPEED_GEN_2				1
-#define     SPEED_GEN_3				2
-#define     IS_RC_MSK				1
-#define     IS_RC_SHIFT				2
+#define     SPEED_GEN_1				(0 << PCIE_GEN_SEL_SHIFT)
+#define     SPEED_GEN_2				(1 << PCIE_GEN_SEL_SHIFT)
+#define     SPEED_GEN_3				(2 << PCIE_GEN_SEL_SHIFT)
+#define     IS_RC				BIT(2)
 #define     LANE_CNT_MSK			0x18
 #define     LANE_CNT_SHIFT			0x3
 #define     LANE_COUNT_1			(0 << LANE_CNT_SHIFT)
@@ -95,16 +94,16 @@
 #define     PCIE_CORE_REF_CLK_RX_ENABLE		BIT(2)
 #define PCIE_MSG_LOG_REG			(CONTROL_BASE_ADDR + 0x30)
 #define PCIE_ISR0_REG				(CONTROL_BASE_ADDR + 0x40)
-#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_MSG_PM_PME		BIT(7)
 #define     PCIE_ISR0_CORR_ERR			BIT(11)
 #define     PCIE_ISR0_NFAT_ERR			BIT(12)
 #define     PCIE_ISR0_FAT_ERR			BIT(13)
 #define     PCIE_ISR0_ERR_MASK			GENMASK(13, 11)
 #define     PCIE_ISR0_INTX_ASSERT(val)		BIT(16 + (val))
 #define     PCIE_ISR0_INTX_DEASSERT(val)	BIT(20 + (val))
+#define     PCIE_ISR0_MSI_INT			BIT(24)
 #define     PCIE_ISR0_ALL_MASK			GENMASK(31, 0)
 #define PCIE_ISR1_REG				(CONTROL_BASE_ADDR + 0x48)
 #define PCIE_ISR1_MASK_REG			(CONTROL_BASE_ADDR + 0x4C)
@@ -546,7 +545,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	/* Set PCI global control register to RC mode */
 	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
-	reg |= (IS_RC_MSK << IS_RC_SHIFT);
+	reg |= IS_RC;
 	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
 
 	/*
@@ -633,7 +632,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	/* Unmask summary MSI interrupt */
 	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
-	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
+	reg &= ~PCIE_ISR0_MSI_INT;
 	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
 
 	/* Unmask Link Down interrupt */
@@ -643,7 +642,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	/* Unmask PME interrupt for processing of PME requester */
 	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
-	reg &= ~PCIE_MSG_PM_PME_MASK;
+	reg &= ~PCIE_ISR0_MSG_PM_PME;
 	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
 
 	/* Enable summary interrupt for GIC SPI source */
@@ -1661,7 +1660,7 @@ static void advk_pcie_handle_pme(struct advk_pcie *pcie)
 {
 	u32 requester = advk_readl(pcie, PCIE_MSG_LOG_REG) >> 16;
 
-	advk_writel(pcie, PCIE_MSG_PM_PME_MASK, PCIE_ISR0_REG);
+	advk_writel(pcie, PCIE_ISR0_MSG_PM_PME, PCIE_ISR0_REG);
 
 	/*
 	 * PCIE_MSG_LOG_REG contains the last inbound message, so store
@@ -1700,8 +1699,7 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
 	}
 
-	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
-		    PCIE_ISR0_REG);
+	advk_writel(pcie, PCIE_ISR0_MSI_INT, PCIE_ISR0_REG);
 }
 
 static void advk_pcie_handle_int(struct advk_pcie *pcie)
@@ -1720,7 +1718,7 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 	isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
 
 	/* Process PME interrupt as the first one to do not miss PME requester id */
-	if (isr0_status & PCIE_MSG_PM_PME_MASK)
+	if (isr0_status & PCIE_ISR0_MSG_PM_PME)
 		advk_pcie_handle_pme(pcie);
 
 	/* Process ERR interrupt */
@@ -1756,7 +1754,7 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 	}
 
 	/* Process MSI interrupts */
-	if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
+	if (isr0_status & PCIE_ISR0_MSI_INT)
 		advk_pcie_handle_msi(pcie);
 
 	/* Process legacy interrupts */
-- 
2.35.1