Message ID | 20190614101059.1664-1-repk@triplefau.lt |
---|---|
State | Accepted |
Delegated to: | Lorenzo Pieralisi |
Headers | show |
Series | [v3] PCI: aardvark: Fix PCI_EXP_RTCTL register configuration | expand |
On Fri, Jun 14, 2019 at 12:10:59PM +0200, Remi Pommarel wrote: > PCI_EXP_RTCTL is used to activate PME interrupt only, so writing into it > should not modify other interrupts' mask. The ISR mask polarity was also > inverted, when PCI_EXP_RTCTL_PMEIE is set PCIE_MSG_PM_PME_MASK mask bit > should actually be cleared. > > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space") > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > --- > Changes since v1: > * Improve code readability > * Fix mask polarity > * PME_MASK shift was off by one > Changes since v2: > * Modify patch title > * Change Fixes tag to commit that actually introduces the bug > --- > drivers/pci/controller/pci-aardvark.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) I need Thomas' ACK to apply it, thanks. Lorenzo > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > index 134e0306ff00..f6e55c4597b1 100644 > --- a/drivers/pci/controller/pci-aardvark.c > +++ b/drivers/pci/controller/pci-aardvark.c > @@ -415,7 +415,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge, > > case PCI_EXP_RTCTL: { > u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG); > - *value = (val & PCIE_MSG_PM_PME_MASK) ? PCI_EXP_RTCTL_PMEIE : 0; > + *value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE; > return PCI_BRIDGE_EMUL_HANDLED; > } > > @@ -451,10 +451,15 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, > advk_writel(pcie, new, PCIE_CORE_PCIEXP_CAP + reg); > break; > > - case PCI_EXP_RTCTL: > - new = (new & PCI_EXP_RTCTL_PMEIE) << 3; > - advk_writel(pcie, new, PCIE_ISR0_MASK_REG); > + case PCI_EXP_RTCTL: { > + /* Only mask/unmask PME interrupt */ > + u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG) & > + ~PCIE_MSG_PM_PME_MASK; > + if ((new & PCI_EXP_RTCTL_PMEIE) == 0) > + val |= PCIE_MSG_PM_PME_MASK; > + advk_writel(pcie, val, PCIE_ISR0_MASK_REG); > break; > + } > > case PCI_EXP_RTSTA: > new = (new & PCI_EXP_RTSTA_PME) >> 9; > -- > 2.20.1 >
On Mon, Jun 17, 2019 at 01:43:36PM +0100, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 12:10:59PM +0200, Remi Pommarel wrote: > > PCI_EXP_RTCTL is used to activate PME interrupt only, so writing into it > > should not modify other interrupts' mask. The ISR mask polarity was also > > inverted, when PCI_EXP_RTCTL_PMEIE is set PCIE_MSG_PM_PME_MASK mask bit > > should actually be cleared. > > > > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space") > > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > > --- > > Changes since v1: > > * Improve code readability > > * Fix mask polarity > > * PME_MASK shift was off by one > > Changes since v2: > > * Modify patch title > > * Change Fixes tag to commit that actually introduces the bug > > --- > > drivers/pci/controller/pci-aardvark.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > I need Thomas' ACK to apply it, thanks. I still need it :), thanks. Lorenzo > Lorenzo > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > index 134e0306ff00..f6e55c4597b1 100644 > > --- a/drivers/pci/controller/pci-aardvark.c > > +++ b/drivers/pci/controller/pci-aardvark.c > > @@ -415,7 +415,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge, > > > > case PCI_EXP_RTCTL: { > > u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG); > > - *value = (val & PCIE_MSG_PM_PME_MASK) ? PCI_EXP_RTCTL_PMEIE : 0; > > + *value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE; > > return PCI_BRIDGE_EMUL_HANDLED; > > } > > > > @@ -451,10 +451,15 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, > > advk_writel(pcie, new, PCIE_CORE_PCIEXP_CAP + reg); > > break; > > > > - case PCI_EXP_RTCTL: > > - new = (new & PCI_EXP_RTCTL_PMEIE) << 3; > > - advk_writel(pcie, new, PCIE_ISR0_MASK_REG); > > + case PCI_EXP_RTCTL: { > > + /* Only mask/unmask PME interrupt */ > > + u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG) & > > + ~PCIE_MSG_PM_PME_MASK; > > + if ((new & PCI_EXP_RTCTL_PMEIE) == 0) > > + val |= PCIE_MSG_PM_PME_MASK; > > + advk_writel(pcie, val, PCIE_ISR0_MASK_REG); > > break; > > + } > > > > case PCI_EXP_RTSTA: > > new = (new & PCI_EXP_RTSTA_PME) >> 9; > > -- > > 2.20.1 > >
On Fri, Jun 14, 2019 at 12:10:59PM +0200, Remi Pommarel wrote: > PCI_EXP_RTCTL is used to activate PME interrupt only, so writing into it > should not modify other interrupts' mask. The ISR mask polarity was also > inverted, when PCI_EXP_RTCTL_PMEIE is set PCIE_MSG_PM_PME_MASK mask bit > should actually be cleared. > > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space") > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > --- > Changes since v1: > * Improve code readability > * Fix mask polarity > * PME_MASK shift was off by one > Changes since v2: > * Modify patch title > * Change Fixes tag to commit that actually introduces the bug > --- > drivers/pci/controller/pci-aardvark.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) Thomas, are you OK with this patch ? Thanks, Lorenzo > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > index 134e0306ff00..f6e55c4597b1 100644 > --- a/drivers/pci/controller/pci-aardvark.c > +++ b/drivers/pci/controller/pci-aardvark.c > @@ -415,7 +415,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge, > > case PCI_EXP_RTCTL: { > u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG); > - *value = (val & PCIE_MSG_PM_PME_MASK) ? PCI_EXP_RTCTL_PMEIE : 0; > + *value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE; > return PCI_BRIDGE_EMUL_HANDLED; > } > > @@ -451,10 +451,15 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, > advk_writel(pcie, new, PCIE_CORE_PCIEXP_CAP + reg); > break; > > - case PCI_EXP_RTCTL: > - new = (new & PCI_EXP_RTCTL_PMEIE) << 3; > - advk_writel(pcie, new, PCIE_ISR0_MASK_REG); > + case PCI_EXP_RTCTL: { > + /* Only mask/unmask PME interrupt */ > + u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG) & > + ~PCIE_MSG_PM_PME_MASK; > + if ((new & PCI_EXP_RTCTL_PMEIE) == 0) > + val |= PCIE_MSG_PM_PME_MASK; > + advk_writel(pcie, val, PCIE_ISR0_MASK_REG); > break; > + } > > case PCI_EXP_RTSTA: > new = (new & PCI_EXP_RTSTA_PME) >> 9; > -- > 2.20.1 >
On Fri, 14 Jun 2019 12:10:59 +0200 Remi Pommarel <repk@triplefau.lt> wrote: > PCI_EXP_RTCTL is used to activate PME interrupt only, so writing into it > should not modify other interrupts' mask. The ISR mask polarity was also > inverted, when PCI_EXP_RTCTL_PMEIE is set PCIE_MSG_PM_PME_MASK mask bit > should actually be cleared. > > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space") > Signed-off-by: Remi Pommarel <repk@triplefau.lt> Sorry for the long delay, but: Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> I did verify that indeed the polarity of the PME interrupt bit is different between the standard PCI_EXP_RTCTL register and the Aardvark-specific ISR0 mask register. And obviously, we shouldn't clobber other bits of the ISR0 mask register when changing the PME interrupt enable/disable state. I did a quick test with a E1000E NIC and it worked fine. Thanks! Thomas
On Fri, Jun 14, 2019 at 12:10:59PM +0200, Remi Pommarel wrote: > PCI_EXP_RTCTL is used to activate PME interrupt only, so writing into it > should not modify other interrupts' mask. The ISR mask polarity was also > inverted, when PCI_EXP_RTCTL_PMEIE is set PCIE_MSG_PM_PME_MASK mask bit > should actually be cleared. > > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space") > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > --- > Changes since v1: > * Improve code readability > * Fix mask polarity > * PME_MASK shift was off by one > Changes since v2: > * Modify patch title > * Change Fixes tag to commit that actually introduces the bug > --- > drivers/pci/controller/pci-aardvark.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) Applied to pci/aardvark, thanks. Lorenzo > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > index 134e0306ff00..f6e55c4597b1 100644 > --- a/drivers/pci/controller/pci-aardvark.c > +++ b/drivers/pci/controller/pci-aardvark.c > @@ -415,7 +415,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge, > > case PCI_EXP_RTCTL: { > u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG); > - *value = (val & PCIE_MSG_PM_PME_MASK) ? PCI_EXP_RTCTL_PMEIE : 0; > + *value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE; > return PCI_BRIDGE_EMUL_HANDLED; > } > > @@ -451,10 +451,15 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, > advk_writel(pcie, new, PCIE_CORE_PCIEXP_CAP + reg); > break; > > - case PCI_EXP_RTCTL: > - new = (new & PCI_EXP_RTCTL_PMEIE) << 3; > - advk_writel(pcie, new, PCIE_ISR0_MASK_REG); > + case PCI_EXP_RTCTL: { > + /* Only mask/unmask PME interrupt */ > + u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG) & > + ~PCIE_MSG_PM_PME_MASK; > + if ((new & PCI_EXP_RTCTL_PMEIE) == 0) > + val |= PCIE_MSG_PM_PME_MASK; > + advk_writel(pcie, val, PCIE_ISR0_MASK_REG); > break; > + } > > case PCI_EXP_RTSTA: > new = (new & PCI_EXP_RTSTA_PME) >> 9; > -- > 2.20.1 >
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 134e0306ff00..f6e55c4597b1 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -415,7 +415,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge, case PCI_EXP_RTCTL: { u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG); - *value = (val & PCIE_MSG_PM_PME_MASK) ? PCI_EXP_RTCTL_PMEIE : 0; + *value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE; return PCI_BRIDGE_EMUL_HANDLED; } @@ -451,10 +451,15 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, advk_writel(pcie, new, PCIE_CORE_PCIEXP_CAP + reg); break; - case PCI_EXP_RTCTL: - new = (new & PCI_EXP_RTCTL_PMEIE) << 3; - advk_writel(pcie, new, PCIE_ISR0_MASK_REG); + case PCI_EXP_RTCTL: { + /* Only mask/unmask PME interrupt */ + u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG) & + ~PCIE_MSG_PM_PME_MASK; + if ((new & PCI_EXP_RTCTL_PMEIE) == 0) + val |= PCIE_MSG_PM_PME_MASK; + advk_writel(pcie, val, PCIE_ISR0_MASK_REG); break; + } case PCI_EXP_RTSTA: new = (new & PCI_EXP_RTSTA_PME) >> 9;
PCI_EXP_RTCTL is used to activate PME interrupt only, so writing into it should not modify other interrupts' mask. The ISR mask polarity was also inverted, when PCI_EXP_RTCTL_PMEIE is set PCIE_MSG_PM_PME_MASK mask bit should actually be cleared. Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space") Signed-off-by: Remi Pommarel <repk@triplefau.lt> --- Changes since v1: * Improve code readability * Fix mask polarity * PME_MASK shift was off by one Changes since v2: * Modify patch title * Change Fixes tag to commit that actually introduces the bug --- drivers/pci/controller/pci-aardvark.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)