* Re: [PATCH v2 06/22] PCI/MSI: Introduce weak arch_find_msi_chip() to find MSI chip
From: Thomas Gleixner @ 2014-09-26 10:38 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, Thierry Reding,
sparclinux, linux-arch, linux-s390, Russell King, Joerg Roedel,
x86, Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
linux-arm-kernel, Thomas Petazzoni, Xinwei Hu, Tony Luck,
Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu, David Vrabel,
Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <5424D30A.6040900@huawei.com>
On Fri, 26 Sep 2014, Yijing Wang wrote:
> On 2014/9/25 18:38, Thomas Gleixner wrote:
> > On Thu, 25 Sep 2014, Yijing Wang wrote:
> >
> >> Introduce weak arch_find_msi_chip() to find the match msi_chip.
> >> Currently, MSI chip associates pci bus to msi_chip. Because in
> >> ARM platform, there may be more than one MSI controller in system.
> >> Associate pci bus to msi_chip help pci device to find the match
> >> msi_chip and setup MSI/MSI-X irq correctly. But in other platform,
> >> like in x86. we only need one MSI chip, because all device use
> >> the same MSI address/data and irq etc. So it's no need to associate
> >> pci bus to MSI chip, just use a arch function, arch_find_msi_chip()
> >> to return the MSI chip for simplicity. The default weak
> >> arch_find_msi_chip() used in ARM platform, find the MSI chip
> >> by pci bus.
> >
> > This is really backwards. On one hand you try to get rid of the weak
> > arch functions zoo and then you invent new ones for no good
> > reason. Why can't x86 store the chip in the pci bus?
> >
> > Looking deeper, I'm questioning the whole notion of different
> > msi_chips. Are this really different MSI controllers with a different
> > feature set or are this defacto multiple instances of the same
> > controller which just need a different data set?
>
> MSI chip in this series is to setup MSI irq, including IRQ allocation, Map,
> compose MSI msg ..., in different platform, many arch specific MSI irq details in it.
> It's difficult to extract the common data and code.
>
> I have a plan to rework MSI related irq_chips in kernel, PCI and Non-PCI, currently,
> HPET, DMAR and PCI have their own irq_chip and MSI related functions, write_msi_msg(),
> mask_msi_irq(), etc... I want to extract the common data set for that, so we can
> remove lots of unnecessary MSI code.
That makes sense. Can you please make sure that this does not conflict
with the ongoing work Jiang is doing in the x86 irq area with
hierarchical irqdomains to distangle layered levels like MSI from the
underlying vector/irqremap mechanics.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v2 00/22] Use MSI chip framework to configure MSI/MSI-X in all platforms
From: Thierry Reding @ 2014-09-26 9:05 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-mips, linux-ia64, linux-pci, Xinwei Hu, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, Bharat.Bhushan, xen-devel, arnab.basu,
Arnd Bergmann, Konrad Rzeszutek Wilk, Chris Metcalf,
Bjorn Helgaas, Thomas Gleixner, linux-arm-kernel,
Thomas Petazzoni, Liviu Dudau, Tony Luck, Sergei Shtylyov,
linux-kernel, Ralf Baechle, iommu, David Vrabel, Wuyun,
linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140926085430.GG31106@ulmo>
[-- Attachment #1.1: Type: text/plain, Size: 214 bytes --]
On Fri, Sep 26, 2014 at 10:54:32AM +0200, Thierry Reding wrote:
[...]
> At least for Tegra it's trivial to just hook it up in tegra_pcie_scan_bus()
> directly (patch attached).
Really attached this time.
Thierry
[-- Attachment #1.2: 0001-PCI-tegra-Remove-.add_bus-callback.patch --]
[-- Type: text/x-diff, Size: 1945 bytes --]
From 2cedfcf38cdfe21688d1363659f28e271ce43358 Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Fri, 26 Sep 2014 10:35:47 +0200
Subject: [PATCH] PCI: tegra: Remove .add_bus() callback
The .add_bus() callback is called for every bus and used to associate an
MSI chip with each bus. However the PCI core code already propagates the
root bus' MSI chip to child busses, so it is enough to associate the MSI
chip with the root bus upon creation. Conveniently the Tegra PCIe host
bridge driver creates the root bus directly, so the association can be
done at the same time.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
drivers/pci/host/pci-tegra.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 3d43874319be..d314e549ac0c 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -694,15 +694,6 @@ static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
return irq;
}
-static void tegra_pcie_add_bus(struct pci_bus *bus)
-{
- if (IS_ENABLED(CONFIG_PCI_MSI)) {
- struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
-
- bus->msi = &pcie->msi.chip;
- }
-}
-
static struct pci_bus *tegra_pcie_scan_bus(int nr, struct pci_sys_data *sys)
{
struct tegra_pcie *pcie = sys_to_pcie(sys);
@@ -713,6 +704,9 @@ static struct pci_bus *tegra_pcie_scan_bus(int nr, struct pci_sys_data *sys)
if (!bus)
return NULL;
+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ bus->msi = &pcie->msi.chip;
+
pci_scan_child_bus(bus);
return bus;
@@ -1885,7 +1879,6 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
hw.private_data = (void **)&pcie;
hw.setup = tegra_pcie_setup;
hw.map_irq = tegra_pcie_map_irq;
- hw.add_bus = tegra_pcie_add_bus;
hw.scan = tegra_pcie_scan_bus;
hw.ops = &tegra_pcie_ops;
--
2.1.0
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply related
* Re: [PATCH v2 00/22] Use MSI chip framework to configure MSI/MSI-X in all platforms
From: Thierry Reding @ 2014-09-26 8:54 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-mips, linux-ia64, linux-pci, Xinwei Hu, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, Bharat.Bhushan, xen-devel, arnab.basu,
Arnd Bergmann, Konrad Rzeszutek Wilk, Chris Metcalf,
Bjorn Helgaas, Thomas Gleixner, linux-arm-kernel,
Thomas Petazzoni, Liviu Dudau, Tony Luck, Sergei Shtylyov,
linux-kernel, Ralf Baechle, iommu, David Vrabel, Wuyun,
linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <542505B3.7040208@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 3627 bytes --]
On Fri, Sep 26, 2014 at 02:20:35PM +0800, Yijing Wang wrote:
> >> The PCI core can already deal with that. An MSI chip can be set per bus
> >> and the weak pcibios_add_bus() can be used to set that. Often it might
> >> not even be necessary to do it via pcibios_add_bus() if you create the
> >> root bus directly, since you can attach the MSI chip at that time.
> >
> > But I'm thinking that we need to move away from pcibios_add_bus() interface to do
> > something that should be generic. You don't need to be called for every bus when all
> > you want is just the root bus in order to add the MSI chip. Also, from looking at
> > the current patchset, a lot of architectures would set the MSI chip to a global
> > variable, which means you don't have an option to choose the MSI chip based on the
> > bus.
>
> I also agree to remove the pcibios_add_bus() in arm which call .add_bus() to associate msi_chip
> and PCI bus.
>
> In my opinions, all PCI devices under the same PCI hostbridge must share same msi chip, right ?
> So if we can associate msi chip and PCI hostbridge, every PCI device can find correct msi chip.
> PCI hostbridge private attributes can be saved in PCI sysdata, and this data will be propagate to
> PCI root bus and its child buses.
struct pci_sys_data is architecture specific, so the code won't become
any more generic than it is now.
> >>> What I would like to see is a way of creating the pci_host_bridge structure outside
> >>> the pci_create_root_bus(). That would then allow us to pass this sort of platform
> >>> details like associated msi_chip into the host bridge and the child busses will
> >>> have an easy way of finding the information needed by finding the root bus and then
> >>> the host bridge structure. Then the generic pci_scan_root_bus() can be used by (mostly)
> >>> everyone and the drivers can remove their kludges that try to work around the
> >>> current limitations.
> >>
> >> I think both issues are orthogonal. Last time I checked a lot of work
> >> was still necessary to unify host bridges enough so that it could be
> >> shared across architectures. But perhaps some of that work has
> >> happened in the meantime.
> >
> > Breaking out the host bridge creation from root bus creation is not difficult, just not
> > agree upon. That would be the first step in making the generic host brige structure
> > useful for sharing, specially if used as a sort of "parent" structure that you can
> > wrap with your actual host bridge structure.
>
> Breaking out the host bridge creation is a good idea, but there need a lot of changes, we can
> do it in another series.
I agree, this can be done step by step.
> And if we save msi chip in pci sysdata now, it will be easy to
> move it to generic pci host bridge. We can also move the pci domain number and other common info to it.
But like I said above, we wouldn't gain anything by moving the MSI chip
into struct pci_sys_data, since the core code couldn't access it from
there. The code wouldn't become more generic than the current approach
of using pcibios_add_bus(). At least for Tegra it's trivial to just hook
it up in tegra_pcie_scan_bus() directly (patch attached). So I think a
generic solution would be to allow it to be easily associated with a
bus.
Perhaps it would be as simple as adding a pci_scan_root_bus_with_msi()
or something with a less awkward name, or extending the existing
pci_scan_root_bus() with an MSI chip parameter. The function already has
too many arguments for my taste, though, so I'd like to avoid the
latter.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 00/22] Use MSI chip framework to configure MSI/MSI-X in all platforms
From: Liviu Dudau @ 2014-09-26 8:50 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, Thierry Reding,
sparclinux, linux-arch, linux-s390, Russell King, Joerg Roedel,
x86, Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <5424E09F.50701@huawei.com>
On Fri, Sep 26, 2014 at 11:42:23AM +0800, Yijing Wang wrote:
> >> I am actually in disagreement with you, Thierry. I don't like the general direction
> >> of the patches, or at least I don't like the fact that we don't have a portable
> >> way of setting up the msi_chip without having to rely on weak architectural hooks.
> >
> > Oh, good. That's actually one of the things I said I wasn't happy with
> > either. =)
>
> Hm, I decide to drop weak arch_find_msi_chip(), no one likes it.
> Let's find a better solution :)
>
> >
> >> I'm surprised no one considers the case of a platform having more than one host
> >> bridge and possibly more than one MSI unit. With the current proposed patchset I
> >> can't see how that would work.
> >
> > The PCI core can already deal with that. An MSI chip can be set per bus
> > and the weak pcibios_add_bus() can be used to set that. Often it might
> > not even be necessary to do it via pcibios_add_bus() if you create the
> > root bus directly, since you can attach the MSI chip at that time.
>
> Yes, PCI hostbridge driver find the matched msi chip during its initialization,
> and assign the msi chip to PCI bus in pcibios_add_bus().
>
> >
> >> What I would like to see is a way of creating the pci_host_bridge structure outside
> >> the pci_create_root_bus(). That would then allow us to pass this sort of platform
> >> details like associated msi_chip into the host bridge and the child busses will
> >> have an easy way of finding the information needed by finding the root bus and then
> >> the host bridge structure. Then the generic pci_scan_root_bus() can be used by (mostly)
> >> everyone and the drivers can remove their kludges that try to work around the
> >> current limitations.
>
> So I think maybe save msi chip in PCI arch sysdata is a good candidate.
Except that arch sysdata at the moment is an opaque pointer. I am all in favour in
changing the type of sysdata from void* into pci_host_bridge* and arches can wrap their old
sysdata around the pci_host_bridge*.
Best regards,
Liviu
>
> >
> > I think both issues are orthogonal. Last time I checked a lot of work
> > was still necessary to unify host bridges enough so that it could be
> > shared across architectures. But perhaps some of that work has
> > happened in the meantime.
> >
> > But like I said, when you create the root bus, you can easily attach the
> > MSI chip to it.
> >
> > Thierry
> >
>
>
> --
> Thanks!
> Yijing
>
>
--
-------------------
.oooO
( )
\ ( Oooo.
\_) ( )
) /
(_/
One small step
for me ...
^ permalink raw reply
* Re: [PATCH v2 03/22] MSI: Remove the redundant irq_set_chip_data()
From: Thierry Reding @ 2014-09-26 8:09 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <1411614872-4009-4-git-send-email-wangyijing@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]
On Thu, Sep 25, 2014 at 11:14:13AM +0800, Yijing Wang wrote:
> Currently, pcie-designware, pcie-rcar, pci-tegra drivers
> use irq chip_data to save the msi_chip pointer. They
> already call irq_set_chip_data() in their own MSI irq map
> functions. So irq_set_chip_data() in arch_setup_msi_irq()
> is useless.
>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
> drivers/pci/msi.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Thierry Reding <treding@nvidia.com>
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 51d7e62..50f67a3 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -41,14 +41,13 @@ int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> if (err < 0)
> return err;
>
> - irq_set_chip_data(desc->irq, chip);
> -
> return 0;
> }
>
> void __weak arch_teardown_msi_irq(unsigned int irq)
> {
> - struct msi_chip *chip = irq_get_chip_data(irq);
> + struct msi_desc *entry = irq_get_msi_desc(irq);
> + struct msi_chip *chip = entry->dev->bus->msi;
>
> if (!chip || !chip->teardown_irq)
> return;
> --
> 1.7.1
>
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 03/22] MSI: Remove the redundant irq_set_chip_data()
From: Thierry Reding @ 2014-09-26 8:09 UTC (permalink / raw)
To: Yijing Wang
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <5424C9BD.3040506@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]
On Fri, Sep 26, 2014 at 10:04:45AM +0800, Yijing Wang wrote:
> On 2014/9/25 15:19, Thierry Reding wrote:
> > On Thu, Sep 25, 2014 at 11:14:13AM +0800, Yijing Wang wrote:
> >> Currently, pcie-designware, pcie-rcar, pci-tegra drivers
> >> use irq chip_data to save the msi_chip pointer. They
> >> already call irq_set_chip_data() in their own MSI irq map
> >> functions. So irq_set_chip_data() in arch_setup_msi_irq()
> >> is useless.
> >
> > Again, I think this should be the other way around. If drivers do
> > something that's already handled by the core, then the duplicate code
> > should be dropped from the drivers.
>
> Hi Thierry, this is different thing, because chip_data is specific to IRQ
> controller, and in other platform, like in x86, chip_data is used to save irq_cfg.
> So we can not call irq_set_chip_data() in core code.
>
> x86 irq piece code
>
> int arch_setup_hwirq(unsigned int irq, int node)
> {
> struct irq_cfg *cfg;
> unsigned long flags;
> int ret;
>
> cfg = alloc_irq_cfg(irq, node);
> if (!cfg)
> return -ENOMEM;
>
> raw_spin_lock_irqsave(&vector_lock, flags);
> ret = __assign_irq_vector(irq, cfg, apic->target_cpus());
> raw_spin_unlock_irqrestore(&vector_lock, flags);
>
> if (!ret)
> irq_set_chip_data(irq, cfg); ------------->Save irq_cfg
> else
> free_irq_cfg(irq, cfg);
> return ret;
> }
Okay, makes sense to keep irq_set_chip_data() for driver-specific data
then.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [1/4] powerpc/powernv: Sync header with firmware
From: Gavin Shan @ 2014-09-26 6:48 UTC (permalink / raw)
To: Michael Ellerman; +Cc: qiudayu, linuxppc-dev, Gavin Shan
In-Reply-To: <20140925042747.8E2AE14016A@ozlabs.org>
On Thu, Sep 25, 2014 at 02:27:47PM +1000, Michael Ellerman wrote:
>On Tue, 2014-26-08 at 07:56:16 UTC, Gavin Shan wrote:
>> From: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>>
>> The patch synchronizes firmware header file (opal.h) for PCI error
>> injection.
>>
>> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
>> index 4593a93..9113653 100644
>> --- a/arch/powerpc/include/asm/opal.h
>> +++ b/arch/powerpc/include/asm/opal.h
>> @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity {
>> OPAL_EEH_SEV_INF = 5
>> };
>>
>> +enum OpalErrinjctType {
>> + OpalErrinjctTypeIoaBusError = 0,
>> + OpalErrinjctTypeIoaBusError64 = 1,
>> +
>> + /* IoaBusError & IoaBusError64 */
>> + OpalEjtIoaLoadMemAddr = 0,
>> + OpalEjtIoaLoadMemData = 1,
>> + OpalEjtIoaLoadIoAddr = 2,
>> + OpalEjtIoaLoadIoData = 3,
>> + OpalEjtIoaLoadConfigAddr = 4,
>> + OpalEjtIoaLoadConfigData = 5,
>> + OpalEjtIoaStoreMemAddr = 6,
>> + OpalEjtIoaStoreMemData = 7,
>> + OpalEjtIoaStoreIoAddr = 8,
>> + OpalEjtIoaStoreIoData = 9,
>> + OpalEjtIoaStoreConfigAddr = 10,
>> + OpalEjtIoaStoreConfigData = 11,
>> + OpalEjtIoaDmaReadMemAddr = 12,
>> + OpalEjtIoaDmaReadMemData = 13,
>> + OpalEjtIoaDmaReadMemMaster = 14,
>> + OpalEjtIoaDmaReadMemTarget = 15,
>> + OpalEjtIoaDmaWriteMemAddr = 16,
>> + OpalEjtIoaDmaWriteMemData = 17,
>> + OpalEjtIoaDmaWriteMemMaster = 18,
>> + OpalEjtIoaDmaWriteMemTarget = 19,
>> +};
>
>I realise these come from the skiboot source, but they're just too ugly.
>
>Please use kernel style naming, like most of the rest of the file, eg:
>
> OPAL_ERR_INJECT_IOA_BUS_ERR
>
After thinking it for more, I decided to take this way because the specfic
function types will potentially be used when we're going to support error
injection from userland (QEMU).
The new revision has folded all your comments (for other patches as well).
Michael, I'm not sure it's good way to send all my patches for 3.18, or I
just need send revised ones?
Thanks,
Gavin
>Also this enum seems to contain two separate types, the first two values are
>the "type", and the rest are "functions".
>
>The only usage I see is:
>
> /* Sanity check on error type */
> if (type < OpalErrinjctTypeIoaBusError ||
> type > OpalErrinjctTypeIoaBusError64 ||
> function < OpalEjtIoaLoadMemAddr ||
> function > OpalEjtIoaDmaWriteMemTarget) {
> pr_warn("%s: Invalid error type %d-%d\n",
> __func__, type, function);
> return -ERANGE;
> }
>
>So we could also just do:
>
># define OPAL_ERR_INJECT_TYPE_MIN 0
># define OPAL_ERR_INJECT_TYPE_MAX 1
>
># define OPAL_ERR_INJECT_FUNC_MIN 0
># define OPAL_ERR_INJECT_FUNC_MAX 19
>
> if (type < OPAL_ERR_INJECT_TYPE_MIN ||
> type > OPAL_ERR_INJECT_TYPE_MAX ||
> function < OPAL_ERR_INJECT_FUNC_MIN ||
> function > OPAL_ERR_INJECT_FUNC_MIN)
> {
> pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function);
> return -ERANGE;
> }
>
^ permalink raw reply
* Re: [PATCH v2 00/22] Use MSI chip framework to configure MSI/MSI-X in all platforms
From: Yijing Wang @ 2014-09-26 6:20 UTC (permalink / raw)
To: Liviu Dudau, Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925171612.GC31157@bart.dudau.co.uk>
>> The PCI core can already deal with that. An MSI chip can be set per bus
>> and the weak pcibios_add_bus() can be used to set that. Often it might
>> not even be necessary to do it via pcibios_add_bus() if you create the
>> root bus directly, since you can attach the MSI chip at that time.
>
> But I'm thinking that we need to move away from pcibios_add_bus() interface to do
> something that should be generic. You don't need to be called for every bus when all
> you want is just the root bus in order to add the MSI chip. Also, from looking at
> the current patchset, a lot of architectures would set the MSI chip to a global
> variable, which means you don't have an option to choose the MSI chip based on the
> bus.
I also agree to remove the pcibios_add_bus() in arm which call .add_bus() to associate msi_chip
and PCI bus.
In my opinions, all PCI devices under the same PCI hostbridge must share same msi chip, right ?
So if we can associate msi chip and PCI hostbridge, every PCI device can find correct msi chip.
PCI hostbridge private attributes can be saved in PCI sysdata, and this data will be propagate to
PCI root bus and its child buses.
>
>>
>>> What I would like to see is a way of creating the pci_host_bridge structure outside
>>> the pci_create_root_bus(). That would then allow us to pass this sort of platform
>>> details like associated msi_chip into the host bridge and the child busses will
>>> have an easy way of finding the information needed by finding the root bus and then
>>> the host bridge structure. Then the generic pci_scan_root_bus() can be used by (mostly)
>>> everyone and the drivers can remove their kludges that try to work around the
>>> current limitations.
>>
>> I think both issues are orthogonal. Last time I checked a lot of work
>> was still necessary to unify host bridges enough so that it could be
>> shared across architectures. But perhaps some of that work has
>> happened in the meantime.
>
> Breaking out the host bridge creation from root bus creation is not difficult, just not
> agree upon. That would be the first step in making the generic host brige structure
> useful for sharing, specially if used as a sort of "parent" structure that you can
> wrap with your actual host bridge structure.
Breaking out the host bridge creation is a good idea, but there need a lot of changes, we can
do it in another series. And if we save msi chip in pci sysdata now, it will be easy to
move it to generic pci host bridge. We can also move the pci domain number and other common info to it.
Thanks!
Yijing.
>
>>
>> But like I said, when you create the root bus, you can easily attach the
>> MSI chip to it.
>
> Not if you want to use the generic pci_scan_root_bus() function. One needs to copy the code
> and add their own needed modifications. Which makes it hard to fix bugs and prevents code
> reuse.
>
> Best regards,
> Liviu
>
>>
>> Thierry
>
>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH 09/15] powerpc/opal: Add PHB to cxl mode call
From: Anton Blanchard @ 2014-09-26 4:35 UTC (permalink / raw)
To: Michael Neuling
Cc: cbe-oss-dev, arnd, greg, linux-kernel, imunsie, linuxppc-dev, jk
In-Reply-To: <1411028820-29933-10-git-send-email-mikey@neuling.org>
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This adds the OPAL call to change a PHB into cxl mode.
>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
Reviewed-by: Anton Blanchard <anton@samba.org>
^ permalink raw reply
* Re: [PATCH 10/15] powerpc/mm: Add hooks for cxl
From: Anton Blanchard @ 2014-09-26 4:33 UTC (permalink / raw)
To: Michael Neuling
Cc: cbe-oss-dev, arnd, greg, linux-kernel, imunsie, linuxppc-dev, jk
In-Reply-To: <1411028820-29933-11-git-send-email-mikey@neuling.org>
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This add a hook into tlbie() so that we use global invalidations when
> there are cxl contexts active.
>
> Normally cxl snoops broadcast tlbie. cxl can have TLB entries
> invalidated via MMIO, but we aren't doing that yet. So for now we
> are just disabling local tlbies when cxl contexts are active. In
> future we can make tlbie() local mode smarter so that it invalidates
> cxl contexts explicitly when it needs to.
>
> This also adds a hooks for when SLBs are invalidated to ensure any
> corresponding SLBs in cxl are also invalidated at the same time.
>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> + use_local = local && mmu_has_feature(MMU_FTR_TLBIEL) && !cxl_ctx_in_use();
Seems reasonable until we can get the MMIO based optimisation in.
Will all CAPI cached translations be invalidated before we finish using
a CAPI context? And conversely, could CAPI cache any translations when a
context isn't active? I'm mostly concerned that we can't have a
situation where badly behaving userspace could result in a stale
translation.
> spu_flush_all_slbs(mm);
> #endif
> + cxl_slbia(mm);
> spu_flush_all_slbs(mm);
> #endif
> + cxl_slbia(mm);
> spu_flush_all_slbs(mm);
> #endif
> + cxl_slbia(mm);
> spu_flush_all_slbs(mm);
> #endif
> + cxl_slbia(mm);
Should we combine the SPU vs CXL callouts into something common -
perhaps copro_flush_all_slbs()?
Anton
^ permalink raw reply
* Re: [PATCH 02/15] powerpc/cell: Move data segment faulting code out of cell platform
From: Anton Blanchard @ 2014-09-26 4:05 UTC (permalink / raw)
To: Michael Neuling
Cc: cbe-oss-dev, arnd, greg, linux-kernel, imunsie, linuxppc-dev, jk
In-Reply-To: <1411028820-29933-3-git-send-email-mikey@neuling.org>
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> __spu_trap_data_seg() currently contains code to determine the VSID
> and ESID required for a particular EA and mm struct.
>
> This code is generically useful for other co-processors. This moves
> the code of the cell platform so it can be used by other powerpc code.
Could we also mention:
and adds 1TB segment support.
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
Reviewed-by: Anton Blanchard <anton@samba.org>
Anton
^ permalink raw reply
* Re: [PATCH 01/15] powerpc/cell: Move spu_handle_mm_fault() out of cell platform
From: Anton Blanchard @ 2014-09-26 3:57 UTC (permalink / raw)
To: Michael Neuling
Cc: cbe-oss-dev, arnd, greg, linux-kernel, imunsie, linuxppc-dev, jk
In-Reply-To: <1411028820-29933-2-git-send-email-mikey@neuling.org>
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> Currently spu_handle_mm_fault() is in the cell platform.
>
> This code is generically useful for other non-cell co-processors on
> powerpc.
>
> This patch moves this function out of the cell platform into
> arch/powerpc/mm so that others may use it.
>
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
Reviewed-by: Anton Blanchard <anton@samba.org>
Anton
^ permalink raw reply
* Re: [PATCH v2 00/22] Use MSI chip framework to configure MSI/MSI-X in all platforms
From: Yijing Wang @ 2014-09-26 3:42 UTC (permalink / raw)
To: Thierry Reding, Liviu Dudau
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925164937.GB30382@ulmo>
>> I am actually in disagreement with you, Thierry. I don't like the general direction
>> of the patches, or at least I don't like the fact that we don't have a portable
>> way of setting up the msi_chip without having to rely on weak architectural hooks.
>
> Oh, good. That's actually one of the things I said I wasn't happy with
> either. =)
Hm, I decide to drop weak arch_find_msi_chip(), no one likes it.
Let's find a better solution :)
>
>> I'm surprised no one considers the case of a platform having more than one host
>> bridge and possibly more than one MSI unit. With the current proposed patchset I
>> can't see how that would work.
>
> The PCI core can already deal with that. An MSI chip can be set per bus
> and the weak pcibios_add_bus() can be used to set that. Often it might
> not even be necessary to do it via pcibios_add_bus() if you create the
> root bus directly, since you can attach the MSI chip at that time.
Yes, PCI hostbridge driver find the matched msi chip during its initialization,
and assign the msi chip to PCI bus in pcibios_add_bus().
>
>> What I would like to see is a way of creating the pci_host_bridge structure outside
>> the pci_create_root_bus(). That would then allow us to pass this sort of platform
>> details like associated msi_chip into the host bridge and the child busses will
>> have an easy way of finding the information needed by finding the root bus and then
>> the host bridge structure. Then the generic pci_scan_root_bus() can be used by (mostly)
>> everyone and the drivers can remove their kludges that try to work around the
>> current limitations.
So I think maybe save msi chip in PCI arch sysdata is a good candidate.
>
> I think both issues are orthogonal. Last time I checked a lot of work
> was still necessary to unify host bridges enough so that it could be
> shared across architectures. But perhaps some of that work has
> happened in the meantime.
>
> But like I said, when you create the root bus, you can easily attach the
> MSI chip to it.
>
> Thierry
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 04/22] x86/xen/MSI: Eliminate arch_msix_mask_irq() and arch_msi_mask_irq()
From: Yijing Wang @ 2014-09-26 3:12 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, Thierry Reding,
sparclinux, linux-arch, linux-s390, Russell King, Joerg Roedel,
x86, Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Chris Metcalf, Bjorn Helgaas, Thomas Gleixner, linux-arm-kernel,
Thomas Petazzoni, Xinwei Hu, Tony Luck, Sergei Shtylyov,
linux-kernel, Ralf Baechle, iommu, David Vrabel, Wuyun,
linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925143346.GF20089@laptop.dumpdata.com>
On 2014/9/25 22:33, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 25, 2014 at 11:14:14AM +0800, Yijing Wang wrote:
>> Commit 0e4ccb150 added two __weak arch functions arch_msix_mask_irq()
>> and arch_msi_mask_irq() to fix a bug found when running xen in x86.
>> Introduced these two funcntions make MSI code complex. And mask/unmask
>
> "These two functions made the MSI code more complex."
OK, will update, thanks.
>> is the irq actions related to interrupt controller, should not use
>> weak arch functions to override mask/unmask interfaces. This patch
>
> I am bit baffled of what you are saying.
Sorry for my poor English. The meaning is that I think override irq_chip
mask/unmask irq is better than introduced weak functions.
>> reverted commit 0e4ccb150 and export struct irq_chip msi_chip, modify
>> msi_chip->irq_mask/irq_unmask() in xen init functions to fix this
>> bug for simplicity. Also this is preparation for using struct
>> msi_chip instead of weak arch MSI functions in all platforms.
>> Keep default_msi_mask_irq() and default_msix_mask_irq() in
>> linux/msi.h to make s390 MSI code compile happy, they wiil be removed
>
> s/wiil/will.
Will update, thanks.
>
>> in the later patch.
>>
>> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> I don't even remember testing it - I guess I did the earlier version.
Yes, I added your tested-by because in last version, you help to test the whole series in xen.
And I didn't change something in xen part patches in this new version.
>
> So a couple of questions since I've totally forgotten about this:
>
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index 50f67a3..5f8f3af 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
...
>> static void msi_set_mask_bit(struct irq_data *data, u32 flag)
>> @@ -852,7 +842,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
>> /* Return the device with MSI unmasked as initial states */
>> mask = msi_mask(desc->msi_attrib.multi_cap);
>> /* Keep cached state to be restored */
>> - arch_msi_mask_irq(desc, mask, ~mask);
>> + __msi_mask_irq(desc, mask, ~mask);
>
> If I am reading this right, it will call the old 'default_msi_mask_irq'
> which is exactly what we don't want to do under Xen. We want to call
> the NOP code.
Good catch. I missed this one, it will also be called in xen.
I need to rework this patch.
>>
>> /* Restore dev->irq to its default pin-assertion irq */
>> dev->irq = desc->msi_attrib.default_irq;
>> @@ -950,7 +940,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
>> /* Return the device with MSI-X masked as initial states */
>> list_for_each_entry(entry, &dev->msi_list, list) {
>> /* Keep cached states to be restored */
>> - arch_msix_mask_irq(entry, 1);
>> + __msix_mask_irq(entry, 1);
>
> Ditto here.
>
> Looking more at this code I have to retract my Reviewed-by and Tested-by
> on the whole series.
OK, because this patch still need some enhancement.
>
> Is it possible to get a git tree for this please?
I will provide a git tree as soon as possible.
Thanks!
Yijing.
>
>> }
>>
>> msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>> index 45ef8cb..cc46a62 100644
>> --- a/include/linux/msi.h
>> +++ b/include/linux/msi.h
>> @@ -19,6 +19,8 @@ void read_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>> void get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>> void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>> void write_msi_msg(unsigned int irq, struct msi_msg *msg);
>> +u32 __msix_mask_irq(struct msi_desc *desc, u32 flag);
>> +u32 __msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag);
>>
>> struct msi_desc {
>> struct {
>> @@ -59,8 +61,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
>>
>> void default_teardown_msi_irqs(struct pci_dev *dev);
>> void default_restore_msi_irqs(struct pci_dev *dev);
>> -u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag);
>> -u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag);
>> +#define default_msi_mask_irq __msi_mask_irq
>> +#define default_msix_mask_irq __msix_mask_irq
>>
>> struct msi_chip {
>> struct module *owner;
>> --
>> 1.7.1
>>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 00/22] Use MSI chip framework to configure MSI/MSI-X in all platforms
From: Yijing Wang @ 2014-09-26 2:47 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, Thierry Reding,
sparclinux, linux-arch, linux-s390, Russell King, Joerg Roedel,
x86, Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Chris Metcalf, Bjorn Helgaas, Thomas Gleixner, linux-arm-kernel,
Thomas Petazzoni, Xinwei Hu, Tony Luck, Sergei Shtylyov,
linux-kernel, Ralf Baechle, iommu, David Vrabel, Wuyun,
linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925142340.GE20089@laptop.dumpdata.com>
On 2014/9/25 22:23, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 25, 2014 at 11:14:10AM +0800, Yijing Wang wrote:
>> This series is based Bjorn's pci/msi branch
>> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/msi
>
> Is there a git tree for these patches?
Hi Konrad, my git tree in company can not be pulled from outside.
I will try to update this series to github these days.
>>
>> Currently, there are a lot of weak arch functions in MSI code.
>> Thierry Reding Introduced MSI chip framework to configure MSI/MSI-X in arm.
>> This series use MSI chip framework to refactor MSI code across all platforms
>> to eliminate weak arch functions. Then all MSI irqs will be managed in a
>> unified framework. Because this series changed a lot of ARCH MSI code,
>> so tests in the platforms which MSI code modified are warmly welcomed!
>>
>> v1->v2:
>> Add a patch to make s390 MSI code build happy between patch "x86/xen/MSI: E.."
>> and "s390/MSI: Use MSI..". Fix several typo problems found by Lucas.
>>
>> RFC->v1:
>> Updated "[patch 4/21] x86/xen/MSI: Eliminate...", export msi_chip instead
>> of #ifdef to fix MSI bug in xen running in x86.
>> Rename arch_get_match_msi_chip() to arch_find_msi_chip().
>> Drop use struct device as the msi_chip argument, we will do that
>> later in another patchset.
>>
>> Yijing Wang (22):
>> PCI/MSI: Clean up struct msi_chip argument
>> PCI/MSI: Remove useless bus->msi assignment
>> MSI: Remove the redundant irq_set_chip_data()
>> x86/xen/MSI: Eliminate arch_msix_mask_irq() and arch_msi_mask_irq()
>> s390/MSI: Use __msi_mask_irq() instead of default_msi_mask_irq()
>> PCI/MSI: Introduce weak arch_find_msi_chip() to find MSI chip
>> PCI/MSI: Refactor struct msi_chip to make it become more common
>> x86/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> x86/xen/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> Irq_remapping/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> x86/MSI: Remove unused MSI weak arch functions
>> MIPS/Octeon/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> MIPS/Xlp: Remove the dead function destroy_irq() to fix build error
>> MIPS/Xlp/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> MIPS/Xlr/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> Powerpc/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> s390/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> arm/iop13xx/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> IA64/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> Sparc/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> tile/MSI: Use MSI chip framework to configure MSI/MSI-X irq
>> PCI/MSI: Clean up unused MSI arch functions
>>
>> arch/arm/mach-iop13xx/include/mach/pci.h | 2 +
>> arch/arm/mach-iop13xx/iq81340mc.c | 1 +
>> arch/arm/mach-iop13xx/iq81340sc.c | 1 +
>> arch/arm/mach-iop13xx/msi.c | 9 ++-
>> arch/arm/mach-iop13xx/pci.c | 6 ++
>> arch/ia64/kernel/msi_ia64.c | 18 ++++-
>> arch/mips/pci/msi-octeon.c | 35 ++++++----
>> arch/mips/pci/msi-xlp.c | 18 ++++--
>> arch/mips/pci/pci-xlr.c | 15 ++++-
>> arch/powerpc/kernel/msi.c | 14 +++-
>> arch/s390/pci/pci.c | 18 ++++-
>> arch/sparc/kernel/pci.c | 14 +++-
>> arch/tile/kernel/pci_gx.c | 14 +++-
>> arch/x86/include/asm/apic.h | 4 +
>> arch/x86/include/asm/pci.h | 4 +-
>> arch/x86/include/asm/x86_init.h | 7 --
>> arch/x86/kernel/apic/io_apic.c | 16 ++++-
>> arch/x86/kernel/x86_init.c | 34 ---------
>> arch/x86/pci/xen.c | 60 +++++++++-------
>> drivers/iommu/irq_remapping.c | 9 ++-
>> drivers/irqchip/irq-armada-370-xp.c | 8 +--
>> drivers/pci/host/pci-tegra.c | 8 ++-
>> drivers/pci/host/pcie-designware.c | 4 +-
>> drivers/pci/host/pcie-rcar.c | 8 ++-
>> drivers/pci/msi.c | 114 ++++++++++++++----------------
>> drivers/pci/probe.c | 1 -
>> include/linux/msi.h | 26 ++-----
>> 27 files changed, 266 insertions(+), 202 deletions(-)
>>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 06/22] PCI/MSI: Introduce weak arch_find_msi_chip() to find MSI chip
From: Yijing Wang @ 2014-09-26 2:44 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, Thierry Reding,
sparclinux, linux-arch, linux-s390, Russell King, Joerg Roedel,
x86, Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
linux-arm-kernel, Thomas Petazzoni, Xinwei Hu, Tony Luck,
Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu, David Vrabel,
Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <alpine.DEB.2.10.1409251220570.4604@nanos>
On 2014/9/25 18:38, Thomas Gleixner wrote:
> On Thu, 25 Sep 2014, Yijing Wang wrote:
>
>> Introduce weak arch_find_msi_chip() to find the match msi_chip.
>> Currently, MSI chip associates pci bus to msi_chip. Because in
>> ARM platform, there may be more than one MSI controller in system.
>> Associate pci bus to msi_chip help pci device to find the match
>> msi_chip and setup MSI/MSI-X irq correctly. But in other platform,
>> like in x86. we only need one MSI chip, because all device use
>> the same MSI address/data and irq etc. So it's no need to associate
>> pci bus to MSI chip, just use a arch function, arch_find_msi_chip()
>> to return the MSI chip for simplicity. The default weak
>> arch_find_msi_chip() used in ARM platform, find the MSI chip
>> by pci bus.
>
> This is really backwards. On one hand you try to get rid of the weak
> arch functions zoo and then you invent new ones for no good
> reason. Why can't x86 store the chip in the pci bus?
>
> Looking deeper, I'm questioning the whole notion of different
> msi_chips. Are this really different MSI controllers with a different
> feature set or are this defacto multiple instances of the same
> controller which just need a different data set?
MSI chip in this series is to setup MSI irq, including IRQ allocation, Map,
compose MSI msg ..., in different platform, many arch specific MSI irq details in it.
It's difficult to extract the common data and code.
I have a plan to rework MSI related irq_chips in kernel, PCI and Non-PCI, currently,
HPET, DMAR and PCI have their own irq_chip and MSI related functions, write_msi_msg(),
mask_msi_irq(), etc... I want to extract the common data set for that, so we can
remove lots of unnecessary MSI code.
Thanks!
Yijing.
>
> Thanks,
>
> tglx
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 06/22] PCI/MSI: Introduce weak arch_find_msi_chip() to find MSI chip
From: Yijing Wang @ 2014-09-26 2:33 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, Thierry Reding,
sparclinux, linux-arch, linux-s390, Russell King, Joerg Roedel,
x86, Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
linux-arm-kernel, Thomas Petazzoni, Xinwei Hu, Tony Luck,
Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu, David Vrabel,
Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <alpine.DEB.2.10.1409251220570.4604@nanos>
On 2014/9/25 18:38, Thomas Gleixner wrote:
> On Thu, 25 Sep 2014, Yijing Wang wrote:
>
>> Introduce weak arch_find_msi_chip() to find the match msi_chip.
>> Currently, MSI chip associates pci bus to msi_chip. Because in
>> ARM platform, there may be more than one MSI controller in system.
>> Associate pci bus to msi_chip help pci device to find the match
>> msi_chip and setup MSI/MSI-X irq correctly. But in other platform,
>> like in x86. we only need one MSI chip, because all device use
>> the same MSI address/data and irq etc. So it's no need to associate
>> pci bus to MSI chip, just use a arch function, arch_find_msi_chip()
>> to return the MSI chip for simplicity. The default weak
>> arch_find_msi_chip() used in ARM platform, find the MSI chip
>> by pci bus.
>
> This is really backwards. On one hand you try to get rid of the weak
> arch functions zoo and then you invent new ones for no good
> reason. Why can't x86 store the chip in the pci bus?
Hi Thomas, I introduced this weak function , because I thought all platforms
except arm always have only one msi chip, and I hoped to provide a simplest
solution, less code changes. I consider several solutions to associate msi chip
and PCI device. In my reply to Thierry in first reply,
http://marc.info/?l=linux-pci&m=141169658208255&w=2
Could you give me some advices ?
Thanks!
Yijing.
>
> Looking deeper, I'm questioning the whole notion of different
> msi_chips. Are this really different MSI controllers with a different
> feature set or are this defacto multiple instances of the same
> controller which just need a different data set?
>
> Thanks,
>
> tglx
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v4 01/10] tools/perf: support parsing parameterized events
From: Sukadev Bhattiprolu @ 2014-09-26 2:25 UTC (permalink / raw)
To: Jiri Olsa
Cc: ak, Michael Ellerman, peterz, linux-kernel, eranian, dev,
Arnaldo Carvalho de Melo, Paul Mackerras, linuxppc-dev,
Anshuman Khandual
In-Reply-To: <20140925085858.GA20915@krava.brq.redhat.com>
Jiri Olsa [jolsa@redhat.com] wrote:
| On Wed, Sep 24, 2014 at 12:27:15PM -0700, Sukadev Bhattiprolu wrote:
| > From: Cody P Schafer <cody@linux.vnet.ibm.com>
| >
| > Enable event specification like:
| >
| > pmu/event_name,param1=0x1,param2=0x4/
| >
| > Assuming that
| >
| > /sys/bus/event_source/devices/pmu/events/event_name
| >
| > Contains something like
| >
| > param2=foo,bar=1,param1=baz
|
| hum, so what happened to the '?' ... AFAIU from out last discussion,
| you wanted to mark terms which are mandatory and user must provide
| values for them.. and I thought the decision was to have following
| alias record:
|
| $ cat /sys/bus/event_source/devices/pmu/events/event_name
| param2=?,bar=1,param1=?
|
| while perf would scream if any of param1/2 wasnt filled like for:
| pmu/event_name,param1=0x1/
Sorry, I meant to make perf list consistent with sysfs.
Consider these two sysfs entries:
$ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE
domain=0x2,offset=0xe0,starting_index=core,lpar=0x0
$ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE
domain=0x3,offset=0xe0,starting_index=vcpu,lpar=sibling_guest_id
In the first one, starting_index refers to a 'core' while in the second
it refers to a vcpu. This serves as a "hint" for the parameter's meaning.
By replacing both with 'starting_index=?' we lose that hint.
Should we fix both sysfs and 'perf list' to say
starting_index=?core
Sukadev
^ permalink raw reply
* Re: [PATCH v2 15/22] MIPS/Xlr/MSI: Use MSI chip framework to configure MSI/MSI-X irq
From: Yijing Wang @ 2014-09-26 2:13 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925073700.GL12423@ulmo>
On 2014/9/25 15:37, Thierry Reding wrote:
> On Thu, Sep 25, 2014 at 11:14:25AM +0800, Yijing Wang wrote:
> [...]
>> diff --git a/arch/mips/pci/pci-xlr.c b/arch/mips/pci/pci-xlr.c
> [...]
>> @@ -214,11 +214,11 @@ static int get_irq_vector(const struct pci_dev *dev)
>> }
>>
>> #ifdef CONFIG_PCI_MSI
>> -void arch_teardown_msi_irq(unsigned int irq)
>> +void xlr_teardown_msi_irq(unsigned int irq)
>> {
>> }
>>
>> -int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>> +int xlr_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>
> Can both of these now be static?
Yes, Will update.
>
> Thierry
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 01/22] PCI/MSI: Clean up struct msi_chip argument
From: Yijing Wang @ 2014-09-26 2:15 UTC (permalink / raw)
To: Thomas Gleixner, Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
linux-arm-kernel, Thomas Petazzoni, Xinwei Hu, Tony Luck,
Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu, David Vrabel,
Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <alpine.DEB.2.10.1409251208420.4604@nanos>
On 2014/9/25 18:20, Thomas Gleixner wrote:
> On Thu, 25 Sep 2014, Thierry Reding wrote:
>
>> On Thu, Sep 25, 2014 at 11:14:11AM +0800, Yijing Wang wrote:
>>> Msi_chip functions setup_irq/teardown_irq rarely use msi_chip
>>> argument.
>>
>> That's not true. Out of the four drivers that you modify two use the
>> parameter. And the two that don't probably should be using it too.
>>
>> 50% is not "rarely". =)
>>
>>> We can look up msi_chip pointer by the device pointer
>>> or irq number, so clean up msi_chip argument.
>>
>> I don't like this particular change. The idea was to keep the API object
>> oriented so that drivers wouldn't have to know where to get the MSI chip
>> from. It also makes it more resilient against code reorganizations since
>> the core code is the only place that needs to know where to get the chip
>> from.
>
> Right. We have the same thing in the irq_chip callbacks. All of them
> take "struct irq_data", because it's already available in the core
> code and it gives easy access to all information (chip, chipdata ...)
> which is necessary for the callback implementations.
OK, I will drop this change, tglx, thanks for your review and comments!
Thanks!
Yijing.
>
> Thanks,
>
> tglx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 17/22] s390/MSI: Use MSI chip framework to configure MSI/MSI-X irq
From: Yijing Wang @ 2014-09-26 2:14 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925073800.GM12423@ulmo>
On 2014/9/25 15:38, Thierry Reding wrote:
> On Thu, Sep 25, 2014 at 11:14:27AM +0800, Yijing Wang wrote:
> [...]
>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> [...]
>> @@ -358,7 +358,7 @@ static void zpci_irq_handler(struct airq_struct *airq)
>> }
>> }
>>
>> -int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
>> +int zpci_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
>
> static?
Yes, Will update.
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 14/22] MIPS/Xlp/MSI: Use MSI chip framework to configure MSI/MSI-X irq
From: Yijing Wang @ 2014-09-26 2:13 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925073608.GK12423@ulmo>
On 2014/9/25 15:36, Thierry Reding wrote:
> On Thu, Sep 25, 2014 at 11:14:24AM +0800, Yijing Wang wrote:
>> Use MSI chip framework instead of arch MSI functions to configure
>> MSI/MSI-X irq. So we can manage MSI/MSI-X irq in a unified framework.
>
> Nit: s/irq/IRQ/ in the above.
>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>> arch/mips/pci/msi-xlp.c | 14 ++++++++++++--
>> 1 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/pci/msi-xlp.c b/arch/mips/pci/msi-xlp.c
>> index e469dc7..6b791ef 100644
>> --- a/arch/mips/pci/msi-xlp.c
>> +++ b/arch/mips/pci/msi-xlp.c
>> @@ -245,7 +245,7 @@ static struct irq_chip xlp_msix_chip = {
>> .irq_unmask = unmask_msi_irq,
>> };
>>
>> -void arch_teardown_msi_irq(unsigned int irq)
>> +void xlp_teardown_msi_irq(unsigned int irq)
>
> Should this not be static now as well?
Yes, Will update.
>
> Thierry
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 12/22] MIPS/Octeon/MSI: Use MSI chip framework to configure MSI/MSI-X irq
From: Yijing Wang @ 2014-09-26 2:12 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925073435.GJ12423@ulmo>
On 2014/9/25 15:34, Thierry Reding wrote:
> On Thu, Sep 25, 2014 at 11:14:22AM +0800, Yijing Wang wrote:
> [...]
>> diff --git a/arch/mips/pci/msi-octeon.c b/arch/mips/pci/msi-octeon.c
> [...]
>> @@ -132,12 +132,12 @@ msi_irq_allocated:
>> /* Make sure the search for available interrupts didn't fail */
>> if (irq >= 64) {
>> if (request_private_bits) {
>> - pr_err("arch_setup_msi_irq: Unable to find %d free interrupts, trying just one",
>> + pr_err("octeon_setup_msi_irq: Unable to find %d free interrupts, trying just one",
>> 1 << request_private_bits);
>
> Perhaps while at it make this (and other similar changes in this patch):
>
> pr_err("%s(): Unable to ...", __func__, ...);
Will update it, thanks!
>
> So that it becomes more resilient against this kind of rename?
>
>> request_private_bits = 0;
>> goto try_only_one;
>> } else
>> - panic("arch_setup_msi_irq: Unable to find a free MSI interrupt");
>> + panic("octeon_setup_msi_irq: Unable to find a free MSI interrupt");
>
>> @@ -210,14 +210,13 @@ int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>>
>> return 0;
>> }
>> -
>
> This...
OK
>
>> @@ -240,7 +239,7 @@ void arch_teardown_msi_irq(unsigned int irq)
>> */
>> number_irqs = 0;
>> while ((irq0 + number_irqs < 64) &&
>> - (msi_multiple_irq_bitmask[index]
>> + (msi_multiple_irq_bitmask[index]
>
> ... and this seem like unrelated whitespace changes.
OK
>
>> & (1ull << (irq0 + number_irqs))))
>> number_irqs++;
>> number_irqs++;
>> @@ -249,8 +248,8 @@ void arch_teardown_msi_irq(unsigned int irq)
>> /* Shift the mask to the correct bit location */
>> bitmask <<= irq0;
>> if ((msi_free_irq_bitmask[index] & bitmask) != bitmask)
>> - panic("arch_teardown_msi_irq: Attempted to teardown MSI "
>> - "interrupt (%d) not in use", irq);
>> + panic("octeon_teardown_msi_irq: Attempted to teardown MSI "
>> + "interrupt (%d) not in use", irq);
>
> And the second line here also needlessly changes the indentation.
OK
>
> Thierry
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 06/22] PCI/MSI: Introduce weak arch_find_msi_chip() to find MSI chip
From: Yijing Wang @ 2014-09-26 2:10 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925072619.GI12423@ulmo>
On 2014/9/25 15:26, Thierry Reding wrote:
> On Thu, Sep 25, 2014 at 11:14:16AM +0800, Yijing Wang wrote:
>> Introduce weak arch_find_msi_chip() to find the match msi_chip.
>> Currently, MSI chip associates pci bus to msi_chip. Because in
>> ARM platform, there may be more than one MSI controller in system.
>> Associate pci bus to msi_chip help pci device to find the match
>> msi_chip and setup MSI/MSI-X irq correctly. But in other platform,
>> like in x86. we only need one MSI chip, because all device use
>> the same MSI address/data and irq etc. So it's no need to associate
>> pci bus to MSI chip, just use a arch function, arch_find_msi_chip()
>> to return the MSI chip for simplicity. The default weak
>> arch_find_msi_chip() used in ARM platform, find the MSI chip
>> by pci bus.
>
> Can't x86 simply set the bus' .msi field anyway? It would seem to be
> easy to do and unifies the code rather than driving it further apart
> using even more of the __weak functions.
As mentioned in the first reply, I will rework this one when we find a better solution.
Thanks!
Yijing.
>
> Thierry
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 03/22] MSI: Remove the redundant irq_set_chip_data()
From: Yijing Wang @ 2014-09-26 2:04 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-mips, linux-ia64, linux-pci, Bharat.Bhushan, sparclinux,
linux-arch, linux-s390, Russell King, Joerg Roedel, x86,
Sebastian Ott, xen-devel, arnab.basu, Arnd Bergmann,
Konrad Rzeszutek Wilk, Chris Metcalf, Bjorn Helgaas,
Thomas Gleixner, linux-arm-kernel, Thomas Petazzoni, Xinwei Hu,
Tony Luck, Sergei Shtylyov, linux-kernel, Ralf Baechle, iommu,
David Vrabel, Wuyun, linuxppc-dev, David S. Miller, Lucas Stach
In-Reply-To: <20140925071919.GH12423@ulmo>
On 2014/9/25 15:19, Thierry Reding wrote:
> On Thu, Sep 25, 2014 at 11:14:13AM +0800, Yijing Wang wrote:
>> Currently, pcie-designware, pcie-rcar, pci-tegra drivers
>> use irq chip_data to save the msi_chip pointer. They
>> already call irq_set_chip_data() in their own MSI irq map
>> functions. So irq_set_chip_data() in arch_setup_msi_irq()
>> is useless.
>
> Again, I think this should be the other way around. If drivers do
> something that's already handled by the core, then the duplicate code
> should be dropped from the drivers.
Hi Thierry, this is different thing, because chip_data is specific to IRQ
controller, and in other platform, like in x86, chip_data is used to save irq_cfg.
So we can not call irq_set_chip_data() in core code.
x86 irq piece code
int arch_setup_hwirq(unsigned int irq, int node)
{
struct irq_cfg *cfg;
unsigned long flags;
int ret;
cfg = alloc_irq_cfg(irq, node);
if (!cfg)
return -ENOMEM;
raw_spin_lock_irqsave(&vector_lock, flags);
ret = __assign_irq_vector(irq, cfg, apic->target_cpus());
raw_spin_unlock_irqrestore(&vector_lock, flags);
if (!ret)
irq_set_chip_data(irq, cfg); ------------->Save irq_cfg
else
free_irq_cfg(irq, cfg);
return ret;
}
Thanks!
Yijing.
>
> Thierry
>
--
Thanks!
Yijing
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox