Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
Dear Grant Likely, On Wed, 13 Feb 2013 22:59:50 +, Grant Likely wrote: > There isn't a whole lot of value in this patch without another user. > I'll need to see the other patches that make use of this. See the proposed Marvell PCIe driver and Tegra PCIe driver: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/149232.html http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/140649.html Both Thierry (doing the Tegra PCIe driver) and myself (doing the Marvell PCIe driver) already have fairly long patch series to add support for the PCIe interfaces in our respective SoC. In order to ease the process of getting those merged, we'd like to get some of the base functions we depend on to be merged first, so that our patch series become a bit smaller and therefore more manageable. My Marvell PCIe patch series already has 32 patches (including the ones we are currently discussing), and I will need even more patches for the upcoming fourth version (due to additional comments made during the review of the third version of the patch set). So, it would really be helpful if this base infrastructure, for which users already exist in the form of submitted patches, that have already gone through multiple iterations, could be merged. Thanks, Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Tue, Jan 22, 2013 at 07:29:01PM +, Jason Gunthorpe wrote: > On Thu, Jan 17, 2013 at 04:22:18PM +, Andrew Murray wrote: > > > > In either of those cases, does it make sense to use the MSI support > > > outside the scope of the PCI infrastructure? That is, would devices > > > other than PCI devices be able to generate an MSI? > > > > I've come around to your way of thinking. Your approach sounds good for > > registration of MSI ops - let the RC host driver do it (it probably has its > > own), or use a helper for following a phandle to get ops that are not part > > of the driver. MSIs won't be used outside of PCI devices. > > Here is a bit of additional info on some MSI stuff.. > > This can be pretty complex. For instance on hyper transport systems > the PCI to HT bridge has an MSI controller that maps between PCI and > HT MSI formats, that mapping is configurable, so technically each > brige could be considered a MSI controller. Typically the mapping > controllers are all setup the same so there is not much problem with > this. However *native* HT devices can (which are super rare) can use a > different MSI format than PCI devices. From a linux perspective HT is > just a variant of PCI. > > On x86 the MSI is delivered to the CPU APIC complex which converts it > into a vectored interrupt - part of the value of MSI is that the MSI > data can vector the interrupt to a specific CPU, or group of CPUs or > whatever. > > Presumably SMP ARMs will evolve similar MSI based interrupt vectoring > capabilities, and presumably on-chip, non-PCI peripherals will evolve > options to use MSI as well (ie multi-queue ethernet). So it might be > worth giving some thought to how things could migrate in that > direction someday. > > I have a bit hacky MSI driver for Kirkwood, this work you have to > generalize the interface could let me actually upstream it :) The MSI > is built using the Host2CPU doorbell registers, so it is entirely > unrelated to the PCI-E RC driver. > > However, my use of the MSI driver on kirkwood is to assign MSIs to a > PCI-E device via non-standard registers, more like an on chip > peripheral. This is because the Host2CPU doorbell doesn't fit 100% > perfectly with the standard PCI MSI stuff, and the hardware has funny > needs.. So an 'allocate a MSI interrupt' API would be snazzy too :) Thanks for this. I believe Thierry may be working on improving the MSI API - so perhaps we can see where that takes us. Andrew Murray ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Jan 17, 2013 at 04:22:18PM +, Andrew Murray wrote: > > In either of those cases, does it make sense to use the MSI support > > outside the scope of the PCI infrastructure? That is, would devices > > other than PCI devices be able to generate an MSI? > > I've come around to your way of thinking. Your approach sounds good for > registration of MSI ops - let the RC host driver do it (it probably has its > own), or use a helper for following a phandle to get ops that are not part > of the driver. MSIs won't be used outside of PCI devices. Here is a bit of additional info on some MSI stuff.. This can be pretty complex. For instance on hyper transport systems the PCI to HT bridge has an MSI controller that maps between PCI and HT MSI formats, that mapping is configurable, so technically each brige could be considered a MSI controller. Typically the mapping controllers are all setup the same so there is not much problem with this. However *native* HT devices can (which are super rare) can use a different MSI format than PCI devices. From a linux perspective HT is just a variant of PCI. On x86 the MSI is delivered to the CPU APIC complex which converts it into a vectored interrupt - part of the value of MSI is that the MSI data can vector the interrupt to a specific CPU, or group of CPUs or whatever. Presumably SMP ARMs will evolve similar MSI based interrupt vectoring capabilities, and presumably on-chip, non-PCI peripherals will evolve options to use MSI as well (ie multi-queue ethernet). So it might be worth giving some thought to how things could migrate in that direction someday. I have a bit hacky MSI driver for Kirkwood, this work you have to generalize the interface could let me actually upstream it :) The MSI is built using the Host2CPU doorbell registers, so it is entirely unrelated to the PCI-E RC driver. However, my use of the MSI driver on kirkwood is to assign MSIs to a PCI-E device via non-standard registers, more like an on chip peripheral. This is because the Host2CPU doorbell doesn't fit 100% perfectly with the standard PCI MSI stuff, and the hardware has funny needs.. So an 'allocate a MSI interrupt' API would be snazzy too :) Jason ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Fri, Jan 18, 2013 at 09:56:20AM +, Andrew Murray wrote:
> On Wed, Jan 09, 2013 at 08:43:10PM +, Thierry Reding wrote:
> > Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> > directory. The motivation is to collect various host controller drivers
> > in the same location in order to facilitate refactoring.
> >
> > The Tegra PCIe driver has been largely rewritten, both in order to turn
> > it into a proper platform driver and to add MSI (based on code by
> > Krishna Kishore ) as well as device tree support.
> >
> > Signed-off-by: Thierry Reding
>
> [snip]
>
> > +static int tegra_pcie_enable(struct tegra_pcie *pcie)
> > +{
> > + struct hw_pci hw;
> > +
> > + memset(&hw, 0, sizeof(hw));
> > +
> > + hw.nr_controllers = 1;
> > + hw.private_data = (void **)&pcie;
> > + hw.setup = tegra_pcie_setup;
> > + hw.scan = tegra_pcie_scan_bus;
> > + hw.map_irq = tegra_pcie_map_irq;
> > +
> > + pci_common_init(&hw);
> > +
> > + return 0;
> > +}
>
> [snip]
>
> > +static int tegra_pcie_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *port;
> > + struct tegra_pcie *pcie;
> > + int err;
> > +
> > + pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> > + if (!pcie)
> > + return -ENOMEM;
> > +
> > + INIT_LIST_HEAD(&pcie->ports);
> > + pcie->dev = &pdev->dev;
> > +
> > + err = tegra_pcie_parse_dt(pcie);
> > + if (err < 0)
> > + return err;
> > +
> > + pcibios_min_mem = 0;
> > +
> > + err = tegra_pcie_get_resources(pcie);
> > + if (err < 0) {
> > + dev_err(&pdev->dev, "failed to request resources: %d\n",
> > err);
> > + return err;
> > + }
> > +
> > + err = tegra_pcie_enable_controller(pcie);
> > + if (err)
> > + goto put_resources;
> > +
> > + /* probe root ports */
> > + for_each_child_of_node(pdev->dev.of_node, port) {
> > + if (!of_device_is_available(port))
> > + continue;
> > +
> > + err = tegra_pcie_add_port(pcie, port);
> > + if (err < 0) {
> > + dev_err(&pdev->dev, "failed to add port %s: %d\n",
> > + port->name, err);
> > + }
> > + }
> > +
> > + /* setup the AFI address translations */
> > + tegra_pcie_setup_translations(pcie);
> > +
> > + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > + err = tegra_pcie_enable_msi(pcie);
> > + if (err < 0) {
> > + dev_err(&pdev->dev,
> > + "failed to enable MSI support: %d\n",
> > + err);
> > + goto put_resources;
> > + }
> > + }
> > +
> > + err = tegra_pcie_enable(pcie);
> > + if (err < 0) {
> > + dev_err(&pdev->dev, "failed to enable PCIe ports: %d\n",
> > err);
> > + goto disable_msi;
> > + }
> > +
> > + platform_set_drvdata(pdev, pcie);
> > + return 0;
> > +
> > +disable_msi:
> > + if (IS_ENABLED(CONFIG_PCI_MSI))
> > + tegra_pcie_disable_msi(pcie);
> > +put_resources:
> > + tegra_pcie_put_resources(pcie);
> > + return err;
> > +}
> > +
>
> [snip]
>
> > +
> > +static const struct of_device_id tegra_pcie_of_match[] = {
> > + { .compatible = "nvidia,tegra20-pcie", },
> > + { },
> > +};
> > +
> > +static struct platform_driver tegra_pcie_driver = {
> > + .driver = {
> > + .name = "tegra-pcie",
> > + .owner = THIS_MODULE,
> > + .of_match_table = tegra_pcie_of_match,
> > + },
> > + .probe = tegra_pcie_probe,
> > + .remove = tegra_pcie_remove,
> > +};
> > +module_platform_driver(tegra_pcie_driver);
>
> If you have multiple 'nvidia,tegra20-pcie's in your DT then you will end up
> with multiple calls to tegra_pcie_probe/tegra_pcie_enable/pci_common_init.
>
> However pci_common_init/pcibios_init_hw assumes it will only ever be called
> once, and will thus result in trying to create multiple busses with the same
> bus number. (The first root bus it creates is always zero provided you haven't
> implemented hw->scan).
Right, I hadn't noticed. There's currently no hardware that actually has
two PCIe host bridges but I wanted to keep the driver properly prepared
in case this ever happened.
Actually I've reimplemented hw->scan, but it still forwards the bus
number setup by pcibios_init_hw() (sys->busnr) to pci_create_root_bus()
so it will still break. I wonder, though, if a better approach would be
to take this number from the bus-range property in DT instead.
> I have a patch for this if you want to fold it into your series? (I see you've
> made changes to bios32 for per-controller data).
I would certainly like to take a look at it.
Thierr
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wed, Jan 09, 2013 at 08:43:10PM +, Thierry Reding wrote:
> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> directory. The motivation is to collect various host controller drivers
> in the same location in order to facilitate refactoring.
>
> The Tegra PCIe driver has been largely rewritten, both in order to turn
> it into a proper platform driver and to add MSI (based on code by
> Krishna Kishore ) as well as device tree support.
>
> Signed-off-by: Thierry Reding
[snip]
> +static int tegra_pcie_enable(struct tegra_pcie *pcie)
> +{
> + struct hw_pci hw;
> +
> + memset(&hw, 0, sizeof(hw));
> +
> + hw.nr_controllers = 1;
> + hw.private_data = (void **)&pcie;
> + hw.setup = tegra_pcie_setup;
> + hw.scan = tegra_pcie_scan_bus;
> + hw.map_irq = tegra_pcie_map_irq;
> +
> + pci_common_init(&hw);
> +
> + return 0;
> +}
[snip]
> +static int tegra_pcie_probe(struct platform_device *pdev)
> +{
> + struct device_node *port;
> + struct tegra_pcie *pcie;
> + int err;
> +
> + pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> + if (!pcie)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&pcie->ports);
> + pcie->dev = &pdev->dev;
> +
> + err = tegra_pcie_parse_dt(pcie);
> + if (err < 0)
> + return err;
> +
> + pcibios_min_mem = 0;
> +
> + err = tegra_pcie_get_resources(pcie);
> + if (err < 0) {
> + dev_err(&pdev->dev, "failed to request resources: %d\n", err);
> + return err;
> + }
> +
> + err = tegra_pcie_enable_controller(pcie);
> + if (err)
> + goto put_resources;
> +
> + /* probe root ports */
> + for_each_child_of_node(pdev->dev.of_node, port) {
> + if (!of_device_is_available(port))
> + continue;
> +
> + err = tegra_pcie_add_port(pcie, port);
> + if (err < 0) {
> + dev_err(&pdev->dev, "failed to add port %s: %d\n",
> + port->name, err);
> + }
> + }
> +
> + /* setup the AFI address translations */
> + tegra_pcie_setup_translations(pcie);
> +
> + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> + err = tegra_pcie_enable_msi(pcie);
> + if (err < 0) {
> + dev_err(&pdev->dev,
> + "failed to enable MSI support: %d\n",
> + err);
> + goto put_resources;
> + }
> + }
> +
> + err = tegra_pcie_enable(pcie);
> + if (err < 0) {
> + dev_err(&pdev->dev, "failed to enable PCIe ports: %d\n", err);
> + goto disable_msi;
> + }
> +
> + platform_set_drvdata(pdev, pcie);
> + return 0;
> +
> +disable_msi:
> + if (IS_ENABLED(CONFIG_PCI_MSI))
> + tegra_pcie_disable_msi(pcie);
> +put_resources:
> + tegra_pcie_put_resources(pcie);
> + return err;
> +}
> +
[snip]
> +
> +static const struct of_device_id tegra_pcie_of_match[] = {
> + { .compatible = "nvidia,tegra20-pcie", },
> + { },
> +};
> +
> +static struct platform_driver tegra_pcie_driver = {
> + .driver = {
> + .name = "tegra-pcie",
> + .owner = THIS_MODULE,
> + .of_match_table = tegra_pcie_of_match,
> + },
> + .probe = tegra_pcie_probe,
> + .remove = tegra_pcie_remove,
> +};
> +module_platform_driver(tegra_pcie_driver);
If you have multiple 'nvidia,tegra20-pcie's in your DT then you will end up
with multiple calls to tegra_pcie_probe/tegra_pcie_enable/pci_common_init.
However pci_common_init/pcibios_init_hw assumes it will only ever be called
once, and will thus result in trying to create multiple busses with the same
bus number. (The first root bus it creates is always zero provided you haven't
implemented hw->scan).
I have a patch for this if you want to fold it into your series? (I see you've
made changes to bios32 for per-controller data).
Andrew Murray
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Jan 17, 2013 at 08:30:10PM +, Thierry Reding wrote: > On Thu, Jan 17, 2013 at 04:22:18PM +, Andrew Murray wrote: > > On Thu, Jan 17, 2013 at 04:05:02PM +, Thierry Reding wrote: > > > On Thu, Jan 17, 2013 at 03:42:36PM +, Andrew Murray wrote: > > > > On Wed, Jan 16, 2013 at 06:31:01PM +, Thierry Reding wrote: > > > > > Alright, putting the functions into pci_ops doesn't sound like a very > > > > > good idea then. Or perhaps it would make sense for hardware where the > > > > > root complex and the MSI controller are handled by the same driver. > > > > > Basically it could be done as a shortcut and if those are not filled > > > > > in, the drivers could still opt to look up an MSI controller from a > > > > > phandle specified in DT. > > > > > > > > > > Even another alternative would be to keep the functions within the > > > > > struct pci_ops and use generic ones if an external MSI controller is > > > > > used. Just tossing around ideas. > > > > > > > > I think an ideal solution would be for additional logic in drivers/msi.c > > > > (e.g. in functions like msi_capability_init) to determine (based on the > > > > passed in pci_dev) which MSI controller ops to use. I'm not sure the > > > > best > > > > way to implement an association between an MSI controller and PCI busses > > > > (I believe arch/sparc does something like this - perhaps there will be > > > > inspiration there). > > > > > > > > As you've pointed out, most RCs will have their own MSI controllers - so > > > > it should be easy to register and associate both together. > > > > > > > > I've submitted my previous work on MSI controller registration, but it > > > > doesn't quite solve this problem - perhaps it can be a starting point? > > > > > > We basically have two cases: > > > > > > - The PCI host bridge contains registers for MSI support. In that case > > > it makes little sense to uncouple the MSI implementation from the > > > host bridge driver. > > > > > > - An MSI controller exists outside of the PCI host bridge. The PCI > > > host bridge would in that case have to lookup an MSI controller (via > > > DT phandle or some other method). > > > > > > In either of those cases, does it make sense to use the MSI support > > > outside the scope of the PCI infrastructure? That is, would devices > > > other than PCI devices be able to generate an MSI? > > > > I've come around to your way of thinking. Your approach sounds good for > > registration of MSI ops - let the RC host driver do it (it probably has its > > own), or use a helper for following a phandle to get ops that are not part > > of the driver. MSIs won't be used outside of PCI devices. > > > > Though existing drivers will use MSI framework functions to request MSIs, > > that > > will result in callbacks to the arch_setup_msi_irqs type functions. These > > functions would need to be updated to find these new ops if they exist, > > i.e. by > > traversing the pci_dev structure up to the RC and finding a suitable > > structure. > > > > Perhaps the msi ops could live alongside pci_ops in the pci_bus structure. > > This > > way when traversing up the buses from the provided pci_dev - the first bus > > with > > msi ops populated would be used? > > > > If no ops are found, the standard arch callbacks can be called - thus > > preserving > > exiting functionality. > > Yes, what you describe is exactly what I had in mind. I've been thinking > about a possible implementation and there may be some details that could > prove difficult to resolve. For instance, we likely need to pass context > around for the MSI ops, or else make sure that they can find the context > from the struct pci_dev or by traversing upwards from it. > > I think for the case where the MSI hardware is controlled by the same > driver as the PCI host bridge, doing this is easy because the context > could be part of the PCI host bridge context, which in case of Tegra is > stored in struct pci_bus' sysdata field (which is actually an ARM struct > pci_sys_data and in turn stores a pointer to the struct tegra_pcie in > the .private_data field). Other drivers often just use a global variable > assuming that there will only ever be a single instance of the PCI host > bridge. Yes. > If the MSI controller is external to the PCI host bridge, things get a > little more complicated. The easiest way would probably be to store the > context along with the PCI host bridge context and use simple wrappers > around the actual implementations to retrieve the PHB context and pass > the attached MSI context. This would be nice, but may not be necessary. The MSI controller could use a global (file-scope) variable to hold context gained from its probe and assume it will be the only instance of that MSI controller. > > Maybe this could even be made more generic by adding a struct msi_ops * > along with a struct msi_chip * in struct pci_bus. Perhaps I should try > and code something up to
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Jan 17, 2013 at 04:22:18PM +, Andrew Murray wrote: > On Thu, Jan 17, 2013 at 04:05:02PM +, Thierry Reding wrote: > > On Thu, Jan 17, 2013 at 03:42:36PM +, Andrew Murray wrote: > > > On Wed, Jan 16, 2013 at 06:31:01PM +, Thierry Reding wrote: > > > > Alright, putting the functions into pci_ops doesn't sound like a very > > > > good idea then. Or perhaps it would make sense for hardware where the > > > > root complex and the MSI controller are handled by the same driver. > > > > Basically it could be done as a shortcut and if those are not filled > > > > in, the drivers could still opt to look up an MSI controller from a > > > > phandle specified in DT. > > > > > > > > Even another alternative would be to keep the functions within the > > > > struct pci_ops and use generic ones if an external MSI controller is > > > > used. Just tossing around ideas. > > > > > > I think an ideal solution would be for additional logic in drivers/msi.c > > > (e.g. in functions like msi_capability_init) to determine (based on the > > > passed in pci_dev) which MSI controller ops to use. I'm not sure the best > > > way to implement an association between an MSI controller and PCI busses > > > (I believe arch/sparc does something like this - perhaps there will be > > > inspiration there). > > > > > > As you've pointed out, most RCs will have their own MSI controllers - so > > > it should be easy to register and associate both together. > > > > > > I've submitted my previous work on MSI controller registration, but it > > > doesn't quite solve this problem - perhaps it can be a starting point? > > > > We basically have two cases: > > > > - The PCI host bridge contains registers for MSI support. In that case > > it makes little sense to uncouple the MSI implementation from the > > host bridge driver. > > > > - An MSI controller exists outside of the PCI host bridge. The PCI > > host bridge would in that case have to lookup an MSI controller (via > > DT phandle or some other method). > > > > In either of those cases, does it make sense to use the MSI support > > outside the scope of the PCI infrastructure? That is, would devices > > other than PCI devices be able to generate an MSI? > > I've come around to your way of thinking. Your approach sounds good for > registration of MSI ops - let the RC host driver do it (it probably has its > own), or use a helper for following a phandle to get ops that are not part > of the driver. MSIs won't be used outside of PCI devices. > > Though existing drivers will use MSI framework functions to request MSIs, that > will result in callbacks to the arch_setup_msi_irqs type functions. These > functions would need to be updated to find these new ops if they exist, i.e. > by > traversing the pci_dev structure up to the RC and finding a suitable > structure. > > Perhaps the msi ops could live alongside pci_ops in the pci_bus structure. > This > way when traversing up the buses from the provided pci_dev - the first bus > with > msi ops populated would be used? > > If no ops are found, the standard arch callbacks can be called - thus > preserving > exiting functionality. Yes, what you describe is exactly what I had in mind. I've been thinking about a possible implementation and there may be some details that could prove difficult to resolve. For instance, we likely need to pass context around for the MSI ops, or else make sure that they can find the context from the struct pci_dev or by traversing upwards from it. I think for the case where the MSI hardware is controlled by the same driver as the PCI host bridge, doing this is easy because the context could be part of the PCI host bridge context, which in case of Tegra is stored in struct pci_bus' sysdata field (which is actually an ARM struct pci_sys_data and in turn stores a pointer to the struct tegra_pcie in the .private_data field). Other drivers often just use a global variable assuming that there will only ever be a single instance of the PCI host bridge. If the MSI controller is external to the PCI host bridge, things get a little more complicated. The easiest way would probably be to store the context along with the PCI host bridge context and use simple wrappers around the actual implementations to retrieve the PHB context and pass the attached MSI context. Maybe this could even be made more generic by adding a struct msi_ops * along with a struct msi_chip * in struct pci_bus. Perhaps I should try and code something up to make things more concrete. Thierry pgp9kp5qHXb9N.pgp Description: PGP signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Jan 17, 2013 at 04:05:02PM +, Thierry Reding wrote: > On Thu, Jan 17, 2013 at 03:42:36PM +, Andrew Murray wrote: > > On Wed, Jan 16, 2013 at 06:31:01PM +, Thierry Reding wrote: > > > Alright, putting the functions into pci_ops doesn't sound like a very > > > good idea then. Or perhaps it would make sense for hardware where the > > > root complex and the MSI controller are handled by the same driver. > > > Basically it could be done as a shortcut and if those are not filled > > > in, the drivers could still opt to look up an MSI controller from a > > > phandle specified in DT. > > > > > > Even another alternative would be to keep the functions within the > > > struct pci_ops and use generic ones if an external MSI controller is > > > used. Just tossing around ideas. > > > > I think an ideal solution would be for additional logic in drivers/msi.c > > (e.g. in functions like msi_capability_init) to determine (based on the > > passed in pci_dev) which MSI controller ops to use. I'm not sure the best > > way to implement an association between an MSI controller and PCI busses > > (I believe arch/sparc does something like this - perhaps there will be > > inspiration there). > > > > As you've pointed out, most RCs will have their own MSI controllers - so > > it should be easy to register and associate both together. > > > > I've submitted my previous work on MSI controller registration, but it > > doesn't quite solve this problem - perhaps it can be a starting point? > > We basically have two cases: > > - The PCI host bridge contains registers for MSI support. In that case > it makes little sense to uncouple the MSI implementation from the > host bridge driver. > > - An MSI controller exists outside of the PCI host bridge. The PCI > host bridge would in that case have to lookup an MSI controller (via > DT phandle or some other method). > > In either of those cases, does it make sense to use the MSI support > outside the scope of the PCI infrastructure? That is, would devices > other than PCI devices be able to generate an MSI? I've come around to your way of thinking. Your approach sounds good for registration of MSI ops - let the RC host driver do it (it probably has its own), or use a helper for following a phandle to get ops that are not part of the driver. MSIs won't be used outside of PCI devices. Though existing drivers will use MSI framework functions to request MSIs, that will result in callbacks to the arch_setup_msi_irqs type functions. These functions would need to be updated to find these new ops if they exist, i.e. by traversing the pci_dev structure up to the RC and finding a suitable structure. Perhaps the msi ops could live alongside pci_ops in the pci_bus structure. This way when traversing up the buses from the provided pci_dev - the first bus with msi ops populated would be used? If no ops are found, the standard arch callbacks can be called - thus preserving exiting functionality. Andrew Murray ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Jan 17, 2013 at 03:42:36PM +, Andrew Murray wrote: > On Wed, Jan 16, 2013 at 06:31:01PM +, Thierry Reding wrote: > > Alright, putting the functions into pci_ops doesn't sound like a very > > good idea then. Or perhaps it would make sense for hardware where the > > root complex and the MSI controller are handled by the same driver. > > Basically it could be done as a shortcut and if those are not filled > > in, the drivers could still opt to look up an MSI controller from a > > phandle specified in DT. > > > > Even another alternative would be to keep the functions within the > > struct pci_ops and use generic ones if an external MSI controller is > > used. Just tossing around ideas. > > I think an ideal solution would be for additional logic in drivers/msi.c > (e.g. in functions like msi_capability_init) to determine (based on the > passed in pci_dev) which MSI controller ops to use. I'm not sure the best > way to implement an association between an MSI controller and PCI busses > (I believe arch/sparc does something like this - perhaps there will be > inspiration there). > > As you've pointed out, most RCs will have their own MSI controllers - so > it should be easy to register and associate both together. > > I've submitted my previous work on MSI controller registration, but it > doesn't quite solve this problem - perhaps it can be a starting point? We basically have two cases: - The PCI host bridge contains registers for MSI support. In that case it makes little sense to uncouple the MSI implementation from the host bridge driver. - An MSI controller exists outside of the PCI host bridge. The PCI host bridge would in that case have to lookup an MSI controller (via DT phandle or some other method). In either of those cases, does it make sense to use the MSI support outside the scope of the PCI infrastructure? That is, would devices other than PCI devices be able to generate an MSI? Thierry pgpoAAWY_tZXl.pgp Description: PGP signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wed, Jan 16, 2013 at 06:31:01PM +, Thierry Reding wrote: > Alright, putting the functions into pci_ops doesn't sound like a very > good idea then. Or perhaps it would make sense for hardware where the > root complex and the MSI controller are handled by the same driver. > Basically it could be done as a shortcut and if those are not filled > in, the drivers could still opt to look up an MSI controller from a > phandle specified in DT. > > Even another alternative would be to keep the functions within the > struct pci_ops and use generic ones if an external MSI controller is > used. Just tossing around ideas. I think an ideal solution would be for additional logic in drivers/msi.c (e.g. in functions like msi_capability_init) to determine (based on the passed in pci_dev) which MSI controller ops to use. I'm not sure the best way to implement an association between an MSI controller and PCI busses (I believe arch/sparc does something like this - perhaps there will be inspiration there). As you've pointed out, most RCs will have their own MSI controllers - so it should be easy to register and associate both together. I've submitted my previous work on MSI controller registration, but it doesn't quite solve this problem - perhaps it can be a starting point? Andrew Murray ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wed, Jan 16, 2013 at 04:17:16PM +, Andrew Murray wrote: > On Wed, Jan 16, 2013 at 02:00:26PM +, Arnd Bergmann wrote: > > On Tuesday 15 January 2013, Thierry Reding wrote: > > > Is there actually hardware that supports this? I assumed that the MSI > > > controller would have to be tightly coupled to the PCI host bridge in > > > order to raise an interrupt when an MSI is received via PCI. > > > > No, as long as it's guaranteed that the MSI notification won't arrive > > at the CPU before any inbound DMA data before it, the MSI controller > > can be anywhere. Typically, the MSI controller is actually closer to > > the CPU core than to the PCI bridge. On X86, I believe the MSI address > > is on normally on the the "local APIC" on each CPU. > > MSIs are indistinguishable from other memory-write transactions originating > from the RC other than the address they target. Anything that can capture > that write in the address space (even a page fault) could be an MSI controller > and call interrupt handlers. And so the RC / MSI controllers don't need to > be aware of each other. Alright, putting the functions into pci_ops doesn't sound like a very good idea then. Or perhaps it would make sense for hardware where the root complex and the MSI controller are handled by the same driver. Basically it could be done as a shortcut and if those are not filled in, the drivers could still opt to look up an MSI controller from a phandle specified in DT. Even another alternative would be to keep the functions within the struct pci_ops and use generic ones if an external MSI controller is used. Just tossing around ideas. Thierry pgpJDz2PnS4fl.pgp Description: PGP signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wed, Jan 16, 2013 at 02:00:26PM +, Arnd Bergmann wrote: > On Tuesday 15 January 2013, Thierry Reding wrote: > > Is there actually hardware that supports this? I assumed that the MSI > > controller would have to be tightly coupled to the PCI host bridge in > > order to raise an interrupt when an MSI is received via PCI. > > No, as long as it's guaranteed that the MSI notification won't arrive > at the CPU before any inbound DMA data before it, the MSI controller > can be anywhere. Typically, the MSI controller is actually closer to > the CPU core than to the PCI bridge. On X86, I believe the MSI address > is on normally on the the "local APIC" on each CPU. MSIs are indistinguishable from other memory-write transactions originating from the RC other than the address they target. Anything that can capture that write in the address space (even a page fault) could be an MSI controller and call interrupt handlers. And so the RC / MSI controllers don't need to be aware of each other. Andrew Murray ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Tuesday 15 January 2013, Thierry Reding wrote: > Is there actually hardware that supports this? I assumed that the MSI > controller would have to be tightly coupled to the PCI host bridge in > order to raise an interrupt when an MSI is received via PCI. No, as long as it's guaranteed that the MSI notification won't arrive at the CPU before any inbound DMA data before it, the MSI controller can be anywhere. Typically, the MSI controller is actually closer to the CPU core than to the PCI bridge. On X86, I believe the MSI address is on normally on the the "local APIC" on each CPU. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Tue, Jan 15, 2013 at 03:40:38PM +, Andrew Murray wrote: > On Tue, Jan 15, 2013 at 12:44:12PM +, Arnd Bergmann wrote: > > On Tuesday 15 January 2013, Thierry Reding wrote: > > > I'm not sure I follow you're reasoning here. Is it possible to use MSIs > > > without PCI? If not then I think there's little sense in keeping the > > > implementations separate. > > > > Conceptually, you can use MSI for any device, but the Linux interfaces > > for MSI are tied to PCI. If you use an MSI controller for a non-PCI > > device, it would probably just appear as a regular interrupt controller. > > > > > Furthermore, if MSI controller and PCI host bridge are separate entities > > > how do you look up the MSI controller given a PCI device? > > > > The host bridge can contain a pointer ot the MSI controller. You can > > have multiple host bridges sharing a single MSI controller or you > > can have separate ones for each host. > > Yes and I hoped this relationship would be described by a device tree phandle > as is done for relating devices to their interrupt-parent (where device trees > are used). This would provide (arguably unnecessarily) greater flexibility, > e.g. if you have two PCI/MSI controller pairs, the MSIs only offer limited > MSIs > and you only use one PCI fabric - you could service different parts of the > fabric by different MSI controllers (assuming you relate MSI controllers to > part of the fabric and that you'd want to). Perhaps there would be benefits > for > virtualisation as well? Is there actually hardware that supports this? I assumed that the MSI controller would have to be tightly coupled to the PCI host bridge in order to raise an interrupt when an MSI is received via PCI. Thierry pgpoVxvB5AWnA.pgp Description: PGP signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Tue, Jan 15, 2013 at 12:44:12PM +, Arnd Bergmann wrote: > On Tuesday 15 January 2013, Thierry Reding wrote: > > I'm not sure I follow you're reasoning here. Is it possible to use MSIs > > without PCI? If not then I think there's little sense in keeping the > > implementations separate. > > Conceptually, you can use MSI for any device, but the Linux interfaces > for MSI are tied to PCI. If you use an MSI controller for a non-PCI > device, it would probably just appear as a regular interrupt controller. > > > Furthermore, if MSI controller and PCI host bridge are separate entities > > how do you look up the MSI controller given a PCI device? > > The host bridge can contain a pointer ot the MSI controller. You can > have multiple host bridges sharing a single MSI controller or you > can have separate ones for each host. Yes and I hoped this relationship would be described by a device tree phandle as is done for relating devices to their interrupt-parent (where device trees are used). This would provide (arguably unnecessarily) greater flexibility, e.g. if you have two PCI/MSI controller pairs, the MSIs only offer limited MSIs and you only use one PCI fabric - you could service different parts of the fabric by different MSI controllers (assuming you relate MSI controllers to part of the fabric and that you'd want to). Perhaps there would be benefits for virtualisation as well? Andrew Murray ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Tuesday 15 January 2013, Thierry Reding wrote: > I'm not sure I follow you're reasoning here. Is it possible to use MSIs > without PCI? If not then I think there's little sense in keeping the > implementations separate. Conceptually, you can use MSI for any device, but the Linux interfaces for MSI are tied to PCI. If you use an MSI controller for a non-PCI device, it would probably just appear as a regular interrupt controller. > Furthermore, if MSI controller and PCI host bridge are separate entities > how do you look up the MSI controller given a PCI device? The host bridge can contain a pointer ot the MSI controller. You can have multiple host bridges sharing a single MSI controller or you can have separate ones for each host. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Mon, Jan 14, 2013 at 09:57:07AM +, Andrew Murray wrote: > On Sun, Jan 13, 2013 at 09:58:06AM +, Thierry Reding wrote: > > On Sat, Jan 12, 2013 at 09:12:25PM +, Arnd Bergmann wrote: > > > On Saturday 12 January 2013, Thierry Reding wrote: > > > > > I already hinted at that in one of the other subthreads. Having such a > > > > > multiplex would also allow the driver to be built as a module. I had > > > > > already thought about this when I was working on an earlier version of > > > > > these patches. Basically these would be two ops attached to the host > > > > > bridge, and the generic arch_setup_msi_irq() could then look that up > > > > > given the struct pci_dev that is passed to it and call this new per- > > > > > host bridge .setup_msi_irq(). > > > > > > > > struct pci_ops looks like a good place to put these. They'll be > > > > available from each struct pci_bus, so should be easy to call from > > > > arch_setup_msi_irq(). > > > > > > > > Any objections? > > > > > > > > > > struct pci_ops has a long history of being specifically about > > > config space read/write operations, so on the one hand it does > > > not feel like the right place to put interrupt specific operations, > > > but on the other hand, the name sounds appropriate and I cannot > > > think of any other place to put this, so it's fine with me. > > > > > > The only alternative I can think of is to introduce a new > > > structure next to it in struct pci_bus, but that feels a bit > > > pointless. Maybe Bjorn has a preference one way or the other. > > > > The name pci_ops is certainly generic enough. Also the comment above the > > structure declaration says "Low-level architecture-dependent routines", > > which applies to the MSI functions as well. > > I've previously looked into this. It seems that architectures handle this > in different ways, some use vector tables, others use a multiplex and others > just let the end user implement the callback directly. > > I've made an attempt to find a more common way. Though my implementation, > which > I will try to share later today for reference provides a registration function > in drivers/pci/msi.c to provide implementations of the > (setup|teardown)_msi_irq(s) ops. This seems slightly better than the current > approach and doesn't break existing users - but is still ugly. > > At present the PCI and MSI frameworks are largely uncoupled from each other > and > so I was keen to not pollute PCI structures (e.g. pci_ops) with MSI ops. Just > because most PCI host bridges also provide MSI support I don't think there is > a > reason why they should always come as a pair or be provided by the same chip. > > Perhaps the solution is to support MSI controller drivers and a means to > associate them with PCI host controller drivers? I'm not sure I follow you're reasoning here. Is it possible to use MSIs without PCI? If not then I think there's little sense in keeping the implementations separate. Furthermore, if MSI controller and PCI host bridge are separate entities how do you look up the MSI controller given a PCI device? Thierry pgpn8UtP7YhQY.pgp Description: PGP signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Sun, Jan 13, 2013 at 09:58:06AM +, Thierry Reding wrote: > On Sat, Jan 12, 2013 at 09:12:25PM +, Arnd Bergmann wrote: > > On Saturday 12 January 2013, Thierry Reding wrote: > > > > I already hinted at that in one of the other subthreads. Having such a > > > > multiplex would also allow the driver to be built as a module. I had > > > > already thought about this when I was working on an earlier version of > > > > these patches. Basically these would be two ops attached to the host > > > > bridge, and the generic arch_setup_msi_irq() could then look that up > > > > given the struct pci_dev that is passed to it and call this new per- > > > > host bridge .setup_msi_irq(). > > > > > > struct pci_ops looks like a good place to put these. They'll be > > > available from each struct pci_bus, so should be easy to call from > > > arch_setup_msi_irq(). > > > > > > Any objections? > > > > > > > struct pci_ops has a long history of being specifically about > > config space read/write operations, so on the one hand it does > > not feel like the right place to put interrupt specific operations, > > but on the other hand, the name sounds appropriate and I cannot > > think of any other place to put this, so it's fine with me. > > > > The only alternative I can think of is to introduce a new > > structure next to it in struct pci_bus, but that feels a bit > > pointless. Maybe Bjorn has a preference one way or the other. > > The name pci_ops is certainly generic enough. Also the comment above the > structure declaration says "Low-level architecture-dependent routines", > which applies to the MSI functions as well. I've previously looked into this. It seems that architectures handle this in different ways, some use vector tables, others use a multiplex and others just let the end user implement the callback directly. I've made an attempt to find a more common way. Though my implementation, which I will try to share later today for reference provides a registration function in drivers/pci/msi.c to provide implementations of the (setup|teardown)_msi_irq(s) ops. This seems slightly better than the current approach and doesn't break existing users - but is still ugly. At present the PCI and MSI frameworks are largely uncoupled from each other and so I was keen to not pollute PCI structures (e.g. pci_ops) with MSI ops. Just because most PCI host bridges also provide MSI support I don't think there is a reason why they should always come as a pair or be provided by the same chip. Perhaps the solution is to support MSI controller drivers and a means to associate them with PCI host controller drivers? Andrew Murray ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Sat, Jan 12, 2013 at 09:12:25PM +, Arnd Bergmann wrote: > On Saturday 12 January 2013, Thierry Reding wrote: > > > I already hinted at that in one of the other subthreads. Having such a > > > multiplex would also allow the driver to be built as a module. I had > > > already thought about this when I was working on an earlier version of > > > these patches. Basically these would be two ops attached to the host > > > bridge, and the generic arch_setup_msi_irq() could then look that up > > > given the struct pci_dev that is passed to it and call this new per- > > > host bridge .setup_msi_irq(). > > > > struct pci_ops looks like a good place to put these. They'll be > > available from each struct pci_bus, so should be easy to call from > > arch_setup_msi_irq(). > > > > Any objections? > > > > struct pci_ops has a long history of being specifically about > config space read/write operations, so on the one hand it does > not feel like the right place to put interrupt specific operations, > but on the other hand, the name sounds appropriate and I cannot > think of any other place to put this, so it's fine with me. > > The only alternative I can think of is to introduce a new > structure next to it in struct pci_bus, but that feels a bit > pointless. Maybe Bjorn has a preference one way or the other. The name pci_ops is certainly generic enough. Also the comment above the structure declaration says "Low-level architecture-dependent routines", which applies to the MSI functions as well. Thierry pgpz2r8CjfG4N.pgp Description: PGP signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Saturday 12 January 2013, Thierry Reding wrote: > > I already hinted at that in one of the other subthreads. Having such a > > multiplex would also allow the driver to be built as a module. I had > > already thought about this when I was working on an earlier version of > > these patches. Basically these would be two ops attached to the host > > bridge, and the generic arch_setup_msi_irq() could then look that up > > given the struct pci_dev that is passed to it and call this new per- > > host bridge .setup_msi_irq(). > > struct pci_ops looks like a good place to put these. They'll be > available from each struct pci_bus, so should be easy to call from > arch_setup_msi_irq(). > > Any objections? > struct pci_ops has a long history of being specifically about config space read/write operations, so on the one hand it does not feel like the right place to put interrupt specific operations, but on the other hand, the name sounds appropriate and I cannot think of any other place to put this, so it's fine with me. The only alternative I can think of is to introduce a new structure next to it in struct pci_bus, but that feels a bit pointless. Maybe Bjorn has a preference one way or the other. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Fri, Jan 11, 2013 at 04:45:16PM +0100, Thierry Reding wrote:
> On Fri, Jan 11, 2013 at 03:36:14PM +, Arnd Bergmann wrote:
> > On Friday 11 January 2013, Thierry Reding wrote:
> > > Right, it'll need #ifdefs around the arch_{setup,teardown}_msi_irq(). Or
> > > select PCI_MSI unconditionally. Once this is merged I was going to post
> > > a patch that enables PCI_MSI in tegra_defconfig anyway. But it might be
> > > better to keep it optional anyway since the remainder of the code copes
> > > with it properly.
> > >
> > Actually, we need something better than that. You cannot define
> > arch_setup_msi_irq in a tegra specific pci host driver, because that
> > will seriously mess up other platforms in multiplatform configurations
> > by giving a link error when they also define this function, or with a
> > run-time error when they don't support it.
> >
> > I think what we should do here is fix it the right way by adding
> > a pci host specific callback rather than an architecture specific
> > callback in drivers/pci/msi.c. There is already a default version
> > of arch_setup_msi_irqs (with s), and we can probably do the
> > same for arch_setup_msi_irq (without s) to fall back to the
> > arch version for most architectures.
> > Most architectures (at least powerpc, sparc, ia64 and x86) already
> > multiplex the msi handlers internally, but ARM does not because
> > there is only one implementation (iop33x) at the moment.
> >
> > We can add a generix multiplex and then move architectures over to
> > use it.
>
> I already hinted at that in one of the other subthreads. Having such a
> multiplex would also allow the driver to be built as a module. I had
> already thought about this when I was working on an earlier version of
> these patches. Basically these would be two ops attached to the host
> bridge, and the generic arch_setup_msi_irq() could then look that up
> given the struct pci_dev that is passed to it and call this new per-
> host bridge .setup_msi_irq().
struct pci_ops looks like a good place to put these. They'll be
available from each struct pci_bus, so should be easy to call from
arch_setup_msi_irq().
Any objections?
Thierry
pgpvzISuDJ6VJ.pgp
Description: PGP signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On 01/10/2013 08:52 PM, Thierry Reding wrote:
> On Thu, Jan 10, 2013 at 05:48:46PM -0700, Stephen Warren wrote:
>> On 01/09/2013 01:43 PM, Thierry Reding wrote:
>>> Move the PCIe driver from arch/arm/mach-tegra into the
>>> drivers/pci/host directory. The motivation is to collect
>>> various host controller drivers in the same location in order
>>> to facilitate refactoring.
>>>
>>> The Tegra PCIe driver has been largely rewritten, both in order
>>> to turn it into a proper platform driver and to add MSI (based
>>> on code by Krishna Kishore ) as well as
>>> device tree support.
>>
>>> diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c
>>> b/arch/arm/mach-tegra/board-dt-tegra20.c
>>> +static int tegra_pcie_enable_controller(struct tegra_pcie
>>> *pcie) +{ + unsigned int timeout; + unsigned long value; + +/*
>>> enable dual controller and both ports */ + value =
>>> afi_readl(pcie, AFI_PCIE_CONFIG); + value &=
>>> ~(AFI_PCIE_CONFIG_PCIEC0_DISABLE_DEVICE | +
>>> AFI_PCIE_CONFIG_PCIEC1_DISABLE_DEVICE | +
>>> AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_MASK); +value |=
>>> AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_DUAL; + afi_writel(pcie,
>>> value, AFI_PCIE_CONFIG);
>>
>> Eventually, we should probably derive the port enables from the
>> state of the root port DT nodes, so that we can disable some and
>> presumably save a little power. Also, I notice that the
>> nvidia,num-lanes property isn't implemented yet. Still, we can
>> probably take care of this later.
>
> Yes, the plan was to eventually derive the disable bits from the
> port status and setup the XBAR_CONFIG field based on the
> combination of nvidia,num-lanes properties.
>
> I assume we should simply fail if the configuration specified by
> nvidia,num-lanes is invalid?
Makes sense to me.
>>> +static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>
>>> + pcie->vdd_supply = devm_regulator_get(pcie->dev, "vdd"); + if
>>> (IS_ERR(pcie->vdd_supply)) +return
>>> PTR_ERR(pcie->vdd_supply); + + pcie->pex_clk_supply =
>>> devm_regulator_get(pcie->dev, "pex-clk"); + if
>>> (IS_ERR(pcie->pex_clk_supply)) +return
>>> PTR_ERR(pcie->pex_clk_supply);
>>
>> Oh, I guess the regulator_get() calls are already strict.
>
> Yeah, I think they can't return NULL, right? In that case I can
> just drop the extra checks in tegra_pcie_power_{on,off}().
It looks like NULL can be returned if !CONFIG_REGULATOR. The comment
in the dummy implementation implies that drivers should treat a NULL
value as valid regulator in most cases.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Fri, Jan 11, 2013 at 03:36:14PM +, Arnd Bergmann wrote:
> On Friday 11 January 2013, Thierry Reding wrote:
> > Right, it'll need #ifdefs around the arch_{setup,teardown}_msi_irq(). Or
> > select PCI_MSI unconditionally. Once this is merged I was going to post
> > a patch that enables PCI_MSI in tegra_defconfig anyway. But it might be
> > better to keep it optional anyway since the remainder of the code copes
> > with it properly.
> >
> Actually, we need something better than that. You cannot define
> arch_setup_msi_irq in a tegra specific pci host driver, because that
> will seriously mess up other platforms in multiplatform configurations
> by giving a link error when they also define this function, or with a
> run-time error when they don't support it.
>
> I think what we should do here is fix it the right way by adding
> a pci host specific callback rather than an architecture specific
> callback in drivers/pci/msi.c. There is already a default version
> of arch_setup_msi_irqs (with s), and we can probably do the
> same for arch_setup_msi_irq (without s) to fall back to the
> arch version for most architectures.
> Most architectures (at least powerpc, sparc, ia64 and x86) already
> multiplex the msi handlers internally, but ARM does not because
> there is only one implementation (iop33x) at the moment.
>
> We can add a generix multiplex and then move architectures over to
> use it.
I already hinted at that in one of the other subthreads. Having such a
multiplex would also allow the driver to be built as a module. I had
already thought about this when I was working on an earlier version of
these patches. Basically these would be two ops attached to the host
bridge, and the generic arch_setup_msi_irq() could then look that up
given the struct pci_dev that is passed to it and call this new per-
host bridge .setup_msi_irq().
Thierry
pgpvQSQh2H9TU.pgp
Description: PGP signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Friday 11 January 2013, Thierry Reding wrote:
> Right, it'll need #ifdefs around the arch_{setup,teardown}_msi_irq(). Or
> select PCI_MSI unconditionally. Once this is merged I was going to post
> a patch that enables PCI_MSI in tegra_defconfig anyway. But it might be
> better to keep it optional anyway since the remainder of the code copes
> with it properly.
>
Actually, we need something better than that. You cannot define
arch_setup_msi_irq in a tegra specific pci host driver, because that
will seriously mess up other platforms in multiplatform configurations
by giving a link error when they also define this function, or with a
run-time error when they don't support it.
I think what we should do here is fix it the right way by adding
a pci host specific callback rather than an architecture specific
callback in drivers/pci/msi.c. There is already a default version
of arch_setup_msi_irqs (with s), and we can probably do the
same for arch_setup_msi_irq (without s) to fall back to the
arch version for most architectures.
Most architectures (at least powerpc, sparc, ia64 and x86) already
multiplex the msi handlers internally, but ARM does not because
there is only one implementation (iop33x) at the moment.
We can add a generix multiplex and then move architectures over to
use it.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Jan 10, 2013 at 05:48:46PM -0700, Stephen Warren wrote:
> On 01/09/2013 01:43 PM, Thierry Reding wrote:
> > Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> > directory. The motivation is to collect various host controller drivers
> > in the same location in order to facilitate refactoring.
> >
> > The Tegra PCIe driver has been largely rewritten, both in order to turn
> > it into a proper platform driver and to add MSI (based on code by
> > Krishna Kishore ) as well as device tree support.
>
> > diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c
> > b/arch/arm/mach-tegra/board-dt-tegra20.c
>
> > static void __init trimslice_init(void)
> > {
> > #ifdef CONFIG_TEGRA_PCI
> > - int ret;
> > -
> > - ret = tegra_pcie_init(true, true);
> > - if (ret)
> > - pr_err("tegra_pci_init() failed: %d\n", ret);
> > + platform_device_register(&tegra_pcie_device);
>
> That struct doesn't actually exist anywhere; only an extern definition
> is added (and that extern definition isn't removed by patch 14 either).
Right, this shouldn't be there. In fact TEGRA_PCI is removed by this
patch, so I should go over the code more carefully again.
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>
> > +config PCI_TEGRA
> > + bool "NVIDIA Tegra PCIe controller"
> > + depends on ARCH_TEGRA_2x_SOC
>
> Perhaps depend on ARCH_TEGRA; that will save churn once this is ported
> to Tegra30, and shouldn't cause any problems before then.
Okay, I can do that.
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>
> > +#define AFI_INTR_CODE 0xb8
> > +#define AFI_INTR_CODE_MASK0xf
> > +#define AFI_INTR_MASTER_ABORT 4
> > +#define AFI_INTR_LEGACY 6
>
> Adding defines for at least some other codes here, would help further
> below ...
>
> > +static irqreturn_t tegra_pcie_isr(int irq, void *arg)
>
> > + if (code == AFI_INTR_MASTER_ABORT) {
> > + dev_dbg(pcie->dev, "%s, signature: %08x\n", err_msg[code],
> > + signature);
> > + } else
> > + dev_err(pcie->dev, "%s, signature: %08x\n", err_msg[code],
> > + signature);
> > +
> > + if (code == 3 || code == 4 || code == 7) {
>
> ... i.e. here.
Will do.
>
> > + u32 fpci = afi_readl(pcie, AFI_UPPER_FPCI_ADDRESS) & 0xff;
> > + u64 address = (u64)fpci << 32 | (signature & 0xfffc);
> > + dev_dbg(pcie->dev, " FPCI address: %10llx\n", address);
>
> I'd suggest making that dev_err(), or at least something higher than
> debug, since the message indicating the error happened is dev_err(), so
> the complete details may as well be available since they're small.
I can make it conditional on !AFI_INTR_MASTER_ABORT to match the
previous output. Or rather move it into the branches above.
> > +static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> > +{
> > + unsigned int timeout;
> > + unsigned long value;
> > +
> > + /* enable dual controller and both ports */
> > + value = afi_readl(pcie, AFI_PCIE_CONFIG);
> > + value &= ~(AFI_PCIE_CONFIG_PCIEC0_DISABLE_DEVICE |
> > + AFI_PCIE_CONFIG_PCIEC1_DISABLE_DEVICE |
> > + AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_MASK);
> > + value |= AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_DUAL;
> > + afi_writel(pcie, value, AFI_PCIE_CONFIG);
>
> Eventually, we should probably derive the port enables from the state of
> the root port DT nodes, so that we can disable some and presumably save
> a little power. Also, I notice that the nvidia,num-lanes property isn't
> implemented yet. Still, we can probably take care of this later.
Yes, the plan was to eventually derive the disable bits from the port
status and setup the XBAR_CONFIG field based on the combination of
nvidia,num-lanes properties.
I assume we should simply fail if the configuration specified by
nvidia,num-lanes is invalid?
> > +static void tegra_pcie_power_off(struct tegra_pcie *pcie)
>
> > + if (!IS_ERR_OR_NULL(pcie->pex_clk_supply)) {
>
> Hmm. I think we should make supplies mandatory; it doesn't make sense
> for regulator support to be disabled on Tegra, and where a specific
> board doesn't actually have a regulator, you're supposed to provide a
> dummy fixed regulator so the driver doesn't have to care.
>
> The same comment obviously applies to tegra_pcie_power_on() and wherever
> regulator_get() happens.
Okay, I'll fix that.
> > +static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>
> > + pcie->vdd_supply = devm_regulator_get(pcie->dev, "vdd");
> > + if (IS_ERR(pcie->vdd_supply))
> > + return PTR_ERR(pcie->vdd_supply);
> > +
> > + pcie->pex_clk_supply = devm_regulator_get(pcie->dev, "pex-clk");
> > + if (IS_ERR(pcie->pex_clk_supply))
> > + return PTR_ERR(pcie->pex_clk_supply);
>
> Oh, I guess the regulator_get() calls are already strict.
Yeah, I think they can't return NULL, right? In that case I can
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Jan 10, 2013 at 04:54:30PM -0700, Stephen Warren wrote:
> On 01/09/2013 01:43 PM, Thierry Reding wrote:
> > Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> > directory. The motivation is to collect various host controller drivers
> > in the same location in order to facilitate refactoring.
> >
> > The Tegra PCIe driver has been largely rewritten, both in order to turn
> > it into a proper platform driver and to add MSI (based on code by
> > Krishna Kishore ) as well as device tree support.
>
> This driver doesn't compile unless CONFIG_PCI_MSI is also enabled.
> Should it select that, or contain a few ifdefs?
>
> drivers/pci/host/pci-tegra.c:900: undefined reference to `write_msi_msg'
Right, it'll need #ifdefs around the arch_{setup,teardown}_msi_irq(). Or
select PCI_MSI unconditionally. Once this is merged I was going to post
a patch that enables PCI_MSI in tegra_defconfig anyway. But it might be
better to keep it optional anyway since the remainder of the code copes
with it properly.
Thierry
pgpWbIsfKJRKt.pgp
Description: PGP signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On 01/09/2013 01:43 PM, Thierry Reding wrote:
> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> directory. The motivation is to collect various host controller drivers
> in the same location in order to facilitate refactoring.
>
> The Tegra PCIe driver has been largely rewritten, both in order to turn
> it into a proper platform driver and to add MSI (based on code by
> Krishna Kishore ) as well as device tree support.
> diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c
> b/arch/arm/mach-tegra/board-dt-tegra20.c
> static void __init trimslice_init(void)
> {
> #ifdef CONFIG_TEGRA_PCI
> - int ret;
> -
> - ret = tegra_pcie_init(true, true);
> - if (ret)
> - pr_err("tegra_pci_init() failed: %d\n", ret);
> + platform_device_register(&tegra_pcie_device);
That struct doesn't actually exist anywhere; only an extern definition
is added (and that extern definition isn't removed by patch 14 either).
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> +config PCI_TEGRA
> + bool "NVIDIA Tegra PCIe controller"
> + depends on ARCH_TEGRA_2x_SOC
Perhaps depend on ARCH_TEGRA; that will save churn once this is ported
to Tegra30, and shouldn't cause any problems before then.
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> +#define AFI_INTR_CODE0xb8
> +#define AFI_INTR_CODE_MASK 0xf
> +#define AFI_INTR_MASTER_ABORT 4
> +#define AFI_INTR_LEGACY 6
Adding defines for at least some other codes here, would help further
below ...
> +static irqreturn_t tegra_pcie_isr(int irq, void *arg)
> + if (code == AFI_INTR_MASTER_ABORT) {
> + dev_dbg(pcie->dev, "%s, signature: %08x\n", err_msg[code],
> + signature);
> + } else
> + dev_err(pcie->dev, "%s, signature: %08x\n", err_msg[code],
> + signature);
> +
> + if (code == 3 || code == 4 || code == 7) {
... i.e. here.
> + u32 fpci = afi_readl(pcie, AFI_UPPER_FPCI_ADDRESS) & 0xff;
> + u64 address = (u64)fpci << 32 | (signature & 0xfffc);
> + dev_dbg(pcie->dev, " FPCI address: %10llx\n", address);
I'd suggest making that dev_err(), or at least something higher than
debug, since the message indicating the error happened is dev_err(), so
the complete details may as well be available since they're small.
> +static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> +{
> + unsigned int timeout;
> + unsigned long value;
> +
> + /* enable dual controller and both ports */
> + value = afi_readl(pcie, AFI_PCIE_CONFIG);
> + value &= ~(AFI_PCIE_CONFIG_PCIEC0_DISABLE_DEVICE |
> +AFI_PCIE_CONFIG_PCIEC1_DISABLE_DEVICE |
> +AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_MASK);
> + value |= AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_DUAL;
> + afi_writel(pcie, value, AFI_PCIE_CONFIG);
Eventually, we should probably derive the port enables from the state of
the root port DT nodes, so that we can disable some and presumably save
a little power. Also, I notice that the nvidia,num-lanes property isn't
implemented yet. Still, we can probably take care of this later.
> +static void tegra_pcie_power_off(struct tegra_pcie *pcie)
> + if (!IS_ERR_OR_NULL(pcie->pex_clk_supply)) {
Hmm. I think we should make supplies mandatory; it doesn't make sense
for regulator support to be disabled on Tegra, and where a specific
board doesn't actually have a regulator, you're supposed to provide a
dummy fixed regulator so the driver doesn't have to care.
The same comment obviously applies to tegra_pcie_power_on() and wherever
regulator_get() happens.
> +static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
> + pcie->vdd_supply = devm_regulator_get(pcie->dev, "vdd");
> + if (IS_ERR(pcie->vdd_supply))
> + return PTR_ERR(pcie->vdd_supply);
> +
> + pcie->pex_clk_supply = devm_regulator_get(pcie->dev, "pex-clk");
> + if (IS_ERR(pcie->pex_clk_supply))
> + return PTR_ERR(pcie->pex_clk_supply);
Oh, I guess the regulator_get() calls are already strict.
> +static int tegra_pcie_add_port(struct tegra_pcie *pcie, struct device_node
> *np)
> + port = devm_kzalloc(pcie->dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&port->list);
> + port->index = index;
> + port->pcie = pcie;
> +
> + port->base = devm_request_and_ioremap(pcie->dev, ®s);
> + if (!port->base)
> + return -EADDRNOTAVAIL;
> +
> + if (!tegra_pcie_port_check_link(port)) {
> + dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
Perhaps devm_kfree(port)? Not a big leak, but equally if you don't, it's
an unreferenced memory block.
> + return -ENODEV;
> + }
> +
> + list_add_tail(&port->list, &pcie->ports);
> +
> + return 0;
> +}
___
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On 01/09/2013 01:43 PM, Thierry Reding wrote: > Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host > directory. The motivation is to collect various host controller drivers > in the same location in order to facilitate refactoring. > > The Tegra PCIe driver has been largely rewritten, both in order to turn > it into a proper platform driver and to add MSI (based on code by > Krishna Kishore ) as well as device tree support. This driver doesn't compile unless CONFIG_PCI_MSI is also enabled. Should it select that, or contain a few ifdefs? drivers/pci/host/pci-tegra.c:900: undefined reference to `write_msi_msg' ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wednesday 09 January 2013, Thierry Reding wrote: > Stephen suggested I post this as removal/addition because pretty much > everything changed. Reviewing the individual changes would be more > confusing than actually reviewing a new driver. > Ok, fair enough. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wed, Jan 09, 2013 at 09:22:07PM +, Arnd Bergmann wrote: > On Wednesday 09 January 2013, Thierry Reding wrote: > > Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host > > directory. The motivation is to collect various host controller drivers > > in the same location in order to facilitate refactoring. > > > > The Tegra PCIe driver has been largely rewritten, both in order to turn > > it into a proper platform driver and to add MSI (based on code by > > Krishna Kishore ) as well as device tree support. > > > > Signed-off-by: Thierry Reding > > Can you split this patch into two patches, one that moves the file, > and another one that does all the changes? It's a little hard to > review when I can't easily see which code is actually new here. Stephen suggested I post this as removal/addition because pretty much everything changed. Reviewing the individual changes would be more confusing than actually reviewing a new driver. Thierry pgpxz1q61gzFb.pgp Description: PGP signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wednesday 09 January 2013, Thierry Reding wrote: > Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host > directory. The motivation is to collect various host controller drivers > in the same location in order to facilitate refactoring. > > The Tegra PCIe driver has been largely rewritten, both in order to turn > it into a proper platform driver and to add MSI (based on code by > Krishna Kishore ) as well as device tree support. > > Signed-off-by: Thierry Reding Can you split this patch into two patches, one that moves the file, and another one that does all the changes? It's a little hard to review when I can't easily see which code is actually new here. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
