| Message ID | 20160602125247.17566-1-sr@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Thu, 2 Jun 2016 14:52:47 +0200, Stefan Roese wrote: > Add a check to mvebu_pcie_add_windows() to detect, if an MBus window is > already configured. If this is the case (base address, size, target and > attribute are identical), then this window is not created. This fixes > a problem I'm currently seeing on a custom Armada XP based board, which > generates this error upon PCI rescanning (in this case via sysfs): > > $ echo 1 > /sys/bus/pci/rescan > mvebu_mbus: cannot add window '4:e8', conflicts with another window > mvebu-pcie soc:pcie-controller: Could not create MBus window at [mem 0x9e000000-0x9e0fffff]: -22 Thanks for the patch. I am not familiar with what happens during a PCI rescan, but shouldn't the MBus windows be deleted and then re-added? The pci-mvebu driver deletes the MBus windows for a given PCI device when invalid memory base/limits are set in the emulated PCI bridge. Shouldn't this happen over a rescan? If it doesn't, then the other question is whether the check you're adding should be done in the PCI driver or in the MBus driver. In commit b566e782be32145664d96ada3e389f17d32742e5, we already relaxed the checks done by the MBus driver, and since this commit we allow different windows to have the same target/attribute. Should the MBus driver also allow re-creating a window that already exists, if all its properties are the same? Note that I am not asking you to rework your patch in this direction for the moment, I'm only opening the question. I haven't made up my mind yet on which solution is the most correct one. Thanks! Thomas
Hi Thomas, On 02.06.2016 15:18, Thomas Petazzoni wrote: > On Thu, 2 Jun 2016 14:52:47 +0200, Stefan Roese wrote: >> Add a check to mvebu_pcie_add_windows() to detect, if an MBus window is >> already configured. If this is the case (base address, size, target and >> attribute are identical), then this window is not created. This fixes >> a problem I'm currently seeing on a custom Armada XP based board, which >> generates this error upon PCI rescanning (in this case via sysfs): >> >> $ echo 1 > /sys/bus/pci/rescan >> mvebu_mbus: cannot add window '4:e8', conflicts with another window >> mvebu-pcie soc:pcie-controller: Could not create MBus window at [mem 0x9e000000-0x9e0fffff]: -22 > > Thanks for the patch. I am not familiar with what happens during a PCI > rescan, but shouldn't the MBus windows be deleted and then re-added? It does not happen in this rescan case. > The pci-mvebu driver deletes the MBus windows for a given PCI device > when invalid memory base/limits are set in the emulated PCI bridge. > Shouldn't this happen over a rescan? I'm not sure what *should* happen upon rescan. But testing shows, that no MBus window is removed upon rescan. I just tested what happens, if the PCI devices are "removed" via sysfs. And this also does not delete any of the MBus windows. > If it doesn't, then the other question is whether the check you're > adding should be done in the PCI driver or in the MBus driver. > > In commit b566e782be32145664d96ada3e389f17d32742e5, we already relaxed > the checks done by the MBus driver, and since this commit we allow > different windows to have the same target/attribute. > > Should the MBus driver also allow re-creating a window that > already exists, if all its properties are the same? This sounds like a good idea to me. To move this detection / decision one layer up. So that it will work for other drivers using this MBus interface as well. > Note that I am not asking you to rework your patch in this direction > for the moment, I'm only opening the question. I haven't made up my > mind yet on which solution is the most correct one. Understood. Thanks for your comments. Just let me know, if you want me to rework this patch to move this check into the MBus driver instead. And I will try to come up with such a patch shortly. Thanks, Stefan
Hello, On Fri, 3 Jun 2016 07:57:34 +0200, Stefan Roese wrote: > > The pci-mvebu driver deletes the MBus windows for a given PCI device > > when invalid memory base/limits are set in the emulated PCI bridge. > > Shouldn't this happen over a rescan? > > I'm not sure what *should* happen upon rescan. But testing shows, that > no MBus window is removed upon rescan. I just tested what happens, > if the PCI devices are "removed" via sysfs. And this also does not > delete any of the MBus windows. Then maybe that's a question for the PCI folks. Is it normal that when a PCI device gets removed, the configuration of the PCI bridge does not get adjusted accordingly? Thanks, Thomas
On Fri, Jun 03, 2016 at 09:30:42AM +0200, Thomas Petazzoni wrote: > Hello, > > On Fri, 3 Jun 2016 07:57:34 +0200, Stefan Roese wrote: > > > > The pci-mvebu driver deletes the MBus windows for a given PCI device > > > when invalid memory base/limits are set in the emulated PCI bridge. > > > Shouldn't this happen over a rescan? > > > > I'm not sure what *should* happen upon rescan. But testing shows, that > > no MBus window is removed upon rescan. I just tested what happens, > > if the PCI devices are "removed" via sysfs. And this also does not > > delete any of the MBus windows. > > Then maybe that's a question for the PCI folks. Is it normal that when > a PCI device gets removed, the configuration of the PCI bridge does not > get adjusted accordingly? I'm not sure I completely understand the question. When a PCI device is removed, I don't think we touch any upstream devices. The device is gone; there's no reason to change any bridge windows leading to it. When we rescan, we should try to assign resources to BARs of any new devices we find. Theoretically that could involve configuring windows of upstream bridges, but I don't think we actually do that, because we'd have to be careful not to break any active devices, and I don't think we're smart enough for that. Bjorn
On Fri, Jun 03, 2016 at 07:57:34AM +0200, Stefan Roese wrote: > Hi Thomas, > > On 02.06.2016 15:18, Thomas Petazzoni wrote: > >On Thu, 2 Jun 2016 14:52:47 +0200, Stefan Roese wrote: > >>Add a check to mvebu_pcie_add_windows() to detect, if an MBus window is > >>already configured. If this is the case (base address, size, target and > >>attribute are identical), then this window is not created. This fixes > >>a problem I'm currently seeing on a custom Armada XP based board, which > >>generates this error upon PCI rescanning (in this case via sysfs): > >> > >>$ echo 1 > /sys/bus/pci/rescan > >>mvebu_mbus: cannot add window '4:e8', conflicts with another window > >>mvebu-pcie soc:pcie-controller: Could not create MBus window at [mem 0x9e000000-0x9e0fffff]: -22 > > > >Thanks for the patch. I am not familiar with what happens during a PCI > >rescan, but shouldn't the MBus windows be deleted and then re-added? > > It does not happen in this rescan case. > > >The pci-mvebu driver deletes the MBus windows for a given PCI device > >when invalid memory base/limits are set in the emulated PCI bridge. > >Shouldn't this happen over a rescan? > > I'm not sure what *should* happen upon rescan. But testing shows, that > no MBus window is removed upon rescan. I just tested what happens, > if the PCI devices are "removed" via sysfs. And this also does not > delete any of the MBus windows. > > >If it doesn't, then the other question is whether the check you're > >adding should be done in the PCI driver or in the MBus driver. > > > >In commit b566e782be32145664d96ada3e389f17d32742e5, we already relaxed > >the checks done by the MBus driver, and since this commit we allow > >different windows to have the same target/attribute. > > > >Should the MBus driver also allow re-creating a window that > >already exists, if all its properties are the same? > > This sounds like a good idea to me. To move this detection / decision > one layer up. So that it will work for other drivers using this > MBus interface as well. I don't know how you want to solve it, but it definitely should be safe to write the bridge window registers multiple times, either with the same values, a new invalid base/limit pair, or a new valid base/limit pair. A complete emulation should handle all those, including disposing of an old valid window and replacing it with a new, different, valid window. And of course, it should handle individual 16-bit writes for the base/limit of the 32-bit windows. Bjorn
Hi Bjorn, Hi Thomas, On 18.06.2016 01:46, Bjorn Helgaas wrote: > On Fri, Jun 03, 2016 at 07:57:34AM +0200, Stefan Roese wrote: >> Hi Thomas, >> >> On 02.06.2016 15:18, Thomas Petazzoni wrote: >>> On Thu, 2 Jun 2016 14:52:47 +0200, Stefan Roese wrote: >>>> Add a check to mvebu_pcie_add_windows() to detect, if an MBus window is >>>> already configured. If this is the case (base address, size, target and >>>> attribute are identical), then this window is not created. This fixes >>>> a problem I'm currently seeing on a custom Armada XP based board, which >>>> generates this error upon PCI rescanning (in this case via sysfs): >>>> >>>> $ echo 1 > /sys/bus/pci/rescan >>>> mvebu_mbus: cannot add window '4:e8', conflicts with another window >>>> mvebu-pcie soc:pcie-controller: Could not create MBus window at [mem 0x9e000000-0x9e0fffff]: -22 >>> >>> Thanks for the patch. I am not familiar with what happens during a PCI >>> rescan, but shouldn't the MBus windows be deleted and then re-added? >> >> It does not happen in this rescan case. >> >>> The pci-mvebu driver deletes the MBus windows for a given PCI device >>> when invalid memory base/limits are set in the emulated PCI bridge. >>> Shouldn't this happen over a rescan? >> >> I'm not sure what *should* happen upon rescan. But testing shows, that >> no MBus window is removed upon rescan. I just tested what happens, >> if the PCI devices are "removed" via sysfs. And this also does not >> delete any of the MBus windows. >> >>> If it doesn't, then the other question is whether the check you're >>> adding should be done in the PCI driver or in the MBus driver. >>> >>> In commit b566e782be32145664d96ada3e389f17d32742e5, we already relaxed >>> the checks done by the MBus driver, and since this commit we allow >>> different windows to have the same target/attribute. >>> >>> Should the MBus driver also allow re-creating a window that >>> already exists, if all its properties are the same? >> >> This sounds like a good idea to me. To move this detection / decision >> one layer up. So that it will work for other drivers using this >> MBus interface as well. > > I don't know how you want to solve it, but it definitely should be > safe to write the bridge window registers multiple times, either with > the same values, a new invalid base/limit pair, or a new valid > base/limit pair. > > A complete emulation should handle all those, including disposing of > an old valid window and replacing it with a new, different, valid > window. > > And of course, it should handle individual 16-bit writes for the > base/limit of the 32-bit windows. So this patch under discussion allows the re-creation of the identical MBus window (a Marvell special internal bus). Which is a result of a PCI re-scan with unchanged PCIe devices. For a "complete emulation" as mentioned above, we would need to remove all MBus windows matching the target and attribute for this PCIe port and lane. This is usually one window - I can't currently think of a situation with multiple MBus windows with the same properties here. Thomas, should I go this way and rework this patch to first remove the MBus window(s) and then create the new one? Or what is your preferred solution here? Thanks, Stefan
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index 6b451df..a85b2b4 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -355,18 +355,31 @@ static void mvebu_pcie_add_windows(struct mvebu_pcie_port *port, while (size) { size_t sz = 1 << (fls(size) - 1); int ret; + u32 wsize; + u8 wtarget, wattr; - ret = mvebu_mbus_add_window_remap_by_id(target, attribute, base, - sz, remap); - if (ret) { - phys_addr_t end = base + sz - 1; - - dev_err(&port->pcie->pdev->dev, - "Could not create MBus window at [mem %pa-%pa]: %d\n", - &base, &end, ret); - mvebu_pcie_del_windows(port, base - size_mapped, - size_mapped); - return; + /* + * Only add this MBus window when it does not yet exist. + * Otherwise an error may occur upon PCI rescanning (e.g. + * via sysfs). + */ + ret = mvebu_mbus_get_io_win_info(base, &wsize, &wtarget, + &wattr); + if (ret < 0 || wsize != sz || wtarget != target || + wattr != attribute) { + ret = mvebu_mbus_add_window_remap_by_id(target, + attribute, base, + sz, remap); + if (ret) { + phys_addr_t end = base + sz - 1; + + dev_err(&port->pcie->pdev->dev, + "Could not create MBus window at [mem %pa-%pa]: %d\n", + &base, &end, ret); + mvebu_pcie_del_windows(port, base - size_mapped, + size_mapped); + return; + } } size -= sz;
Add a check to mvebu_pcie_add_windows() to detect, if an MBus window is already configured. If this is the case (base address, size, target and attribute are identical), then this window is not created. This fixes a problem I'm currently seeing on a custom Armada XP based board, which generates this error upon PCI rescanning (in this case via sysfs): $ echo 1 > /sys/bus/pci/rescan mvebu_mbus: cannot add window '4:e8', conflicts with another window mvebu-pcie soc:pcie-controller: Could not create MBus window at [mem 0x9e000000-0x9e0fffff]: -22 Signed-off-by: Stefan Roese <sr@denx.de> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Russell King <rmk+kernel@arm.linux.org.uk> Cc: Jisheng Zhang <jszhang@marvell.com> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/host/pci-mvebu.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)