adding PCI support to AXS10x

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.

Signed-off-by Joao Pinto <jpinto at synopsys.com>

---
Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
- driver name was changed from pcie-synopsys to pcie-dw-pltfm

Quoted 7 lines · Joao Pinto

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.

Signed-off-by Joao Pinto <jpinto at synopsys.com>

---
Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
- driver name was changed from pcie-synopsys to pcie-dw-pltfm

"pcie-dw-pltfm" seems worse to me. We have eight existing drivers that call dw_pcie_host_init(), and they're all platform_drivers. "pcie-dw-pltfm" could apply equally well to any of them.

I think I see what happened: I wrote "It doesn't seem necessary to me to include both 'synopsys' and 'ipk' in the filename and the driver name." I meant that using one of them should be sufficient, not that *both* should be removed.

I don't know the SoC landscape, but from Arnd's comment, it sounds like "synopsys" might be too generic because many of the other drivers are connected with Synopsys. I don't know what "ipk" means, but maybe that could work. It's convenient if the name *means* something, and if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic. Is "haps" or "haps_dx" a name people would associate with this hardware? I guess it'd be nice if the driver name were related to the DT compat strings, so "ipk" is better from that perspective.

My pci/host-synopsys branch (which is still published even though I removed it from for-linus) contains your v7 series plus some fixes. This v8 series lost the fixes. For example:

drivers/pci/host/pcie-dw-pltfm.c line 56

Quoted 8 lines

drivers/pci/host/pcie-dw-pltfm.c new file +8 -0

+static irqreturn_t dw_pltfm_pcie_msi_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+
+	dw_handle_msi_irq(pp);
+
+	return dw_handle_msi_irq(pp);
+}

This is a bug. You should call dw_handle_msi_irq() *once* and return the value it returns. I already fixed this in my pci/host-synopsys branch, so you should start with that, and then apply your v7-v8 changes.

Bjorn

Quoted 18 lines · Bjorn Helgaas

Quoted 7 lines · Joao Pinto

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.

Signed-off-by Joao Pinto <jpinto at synopsys.com>

---
Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
- driver name was changed from pcie-synopsys to pcie-dw-pltfm

"pcie-dw-pltfm" seems worse to me. We have eight existing drivers that call dw_pcie_host_init(), and they're all platform_drivers. "pcie-dw-pltfm" could apply equally well to any of them.

I think I see what happened: I wrote "It doesn't seem necessary to me to include both 'synopsys' and 'ipk' in the filename and the driver name." I meant that using one of them should be sufficient, not that *both* should be removed.

Quoted 8 lines

I don't know the SoC landscape, but from Arnd's comment, it sounds like "synopsys" might be too generic because many of the other drivers are connected with Synopsys. I don't know what "ipk" means, but maybe that could work. It's convenient if the name *means* something, and if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic. Is "haps" or "haps_dx" a name people would associate with this hardware? I guess it'd be nice if the driver name were related to the DT compat strings, so "ipk" is better from that perspective.

Synopsys has a product called IP Prototyping Kit which is a bundle of HAPSDX + PCIE RC IP + drivers + Development Board. This driver was implemented originally to serve this IPK but it can be used by SoC that use the Synopsys PCIe RC IP. "ipk" would say that the driver is usable only in the IP Prototyping Kits which is not 100% true, it is usable in any SoC with Synopsys IP or in limit serve as a base for specific SoC drivers. Suggestions: "pcie-dw-soc-agnostic", "pcie-dw-ipk", "pcie-dw-haps-prototyping"

What do you think?

Quoted 1 line

What do you think?

I don't think the "dw" part is relevant (none of the other DesignWare-based drivers includes it in the driver or file name).

How do people typically refer to this board?

I really like "synopsys" because it fits the pattern of being recognizable and pronounceable like "altera", "designware", "qcom", "keystone", "layerscape", "tegra", etc. But I can't tell whether it's too generic.

"ipk" or "haps" would be fine with me. I think it's OK if it doesn't cover 100% of the possible systems.

Bjorn

Hi,

Quoted 7 lines · Bjorn Helgaas

Quoted 1 line

What do you think?

I don't think the "dw" part is relevant (none of the other DesignWare-based drivers includes it in the driver or file name).

How do people typically refer to this board?

Quoted 7 lines

I really like "synopsys" because it fits the pattern of being recognizable and pronounceable like "altera", "designware", "qcom", "keystone", "layerscape", "tegra", etc. But I can't tell whether it's too generic.

"ipk" or "haps" would be fine with me. I think it's OK if it doesn't cover 100% of the possible systems.

I think we should follow the iproc example: pcie-iproc-platform.c In this case we would have pcie-designware-platform.c
I think this would be the best name because the driver is a non soc specific designware platform driver.

Arnd and Bjorn agree on this name?

Quoted 23 lines · On Friday 05 February 2016 10:44:29 Joao Pinto wrote:

Hi,

Quoted 7 lines · Bjorn Helgaas

Quoted 1 line

What do you think?

I don't think the "dw" part is relevant (none of the other DesignWare-based drivers includes it in the driver or file name).

How do people typically refer to this board?

Quoted 7 lines

I really like "synopsys" because it fits the pattern of being recognizable and pronounceable like "altera", "designware", "qcom", "keystone", "layerscape", "tegra", etc. But I can't tell whether it's too generic.

"ipk" or "haps" would be fine with me. I think it's OK if it doesn't cover 100% of the possible systems.

I think we should follow the iproc example: pcie-iproc-platform.c In this case we would have pcie-designware-platform.c
I think this would be the best name because the driver is a non soc specific designware platform driver.

Quoted 1 line

Arnd and Bjorn agree on this name?

Sorry, I did not realize that your submission was for the generic dw-pcie implementation rather than a particular product integrating it.

I think in this case, we should do this completely differently:

How about putting all the new code into drivers/pci/host/pcie-designware.c as functions that can be used by the other drivers in absence of a chip specific handler?

Instead of providing a new instance of struct pcie_host_ops, maybe add it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init() for drivers that don't provide their own. "hisi_pcie_host_ops" currently provides no host_init() callback function, so you will have to change the hisi frontend to a provide nop-function.

For all other drivers, check if they can be changed to use your generic implementation and remove their private callbacks if possible.

I think the MSI implementation should be split out into a separate file though, as not everyone uses this.

Arnd

It is a driver that is useful for PCIe RC prototyping and to be a reference platform driver for DesignWare PCIe RC, I don't know if merging some of the driver's code into pcie-designware is really necessary depends on the usefulness of it. I would suggest that bigger step to be done in a 2nd stage since I will be around to maintain what's necessary. Agree?

I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to match your suggestions and Bjorn's. Could you please comment check it?

Quoted 7 lines · On Friday 05 February 2016 14:51:39 Joao Pinto wrote:

It is a driver that is useful for PCIe RC prototyping and to be a reference platform driver for DesignWare PCIe RC, I don't know if merging some of the driver's code into pcie-designware is really necessary depends on the usefulness of it. I would suggest that bigger step to be done in a 2nd stage since I will be around to maintain what's necessary. Agree?

Quoted 2 lines

I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to match your suggestions and Bjorn's. Could you please comment check it?

I think it would be useful to do this as generic as possible, and you seem to be the right person to integrate it into the generic driver as you have access to the hardware documents. Normally the folks writing the drivers are just guessing based on what little information they get out of manuals or vendor-provided drivers, but I'm sure that not all of that makes sense.

Arnd

Quoted 18 lines · On Friday 05 February 2016 10:44:29 Joao Pinto wrote:

Hi,

Quoted 7 lines · Bjorn Helgaas

Quoted 1 line

What do you think?

I don't think the "dw" part is relevant (none of the other DesignWare-based drivers includes it in the driver or file name).

How do people typically refer to this board?

Quoted 7 lines

I really like "synopsys" because it fits the pattern of being recognizable and pronounceable like "altera", "designware", "qcom", "keystone", "layerscape", "tegra", etc. But I can't tell whether it's too generic.

"ipk" or "haps" would be fine with me. I think it's OK if it doesn't cover 100% of the possible systems.

Quoted 6 lines

I think we should follow the iproc example: pcie-iproc-platform.c In this case we would have pcie-designware-platform.c
I think this would be the best name because the driver is a non soc specific designware platform driver.

Arnd and Bjorn agree on this name?

Sorry, I did not realize that your submission was for the generic dw-pcie implementation rather than a particular product integrating it.

I think in this case, we should do this completely differently:

How about putting all the new code into drivers/pci/host/pcie-designware.c as functions that can be used by the other drivers in absence of a chip specific handler?

Instead of providing a new instance of struct pcie_host_ops, maybe add it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init() for drivers that don't provide their own. "hisi_pcie_host_ops" currently provides no host_init() callback function, so you will have to change the hisi frontend to a provide nop-function.

For all other drivers, check if they can be changed to use your generic implementation and remove their private callbacks if possible.

I think the MSI implementation should be split out into a separate file though, as not everyone uses this.

Quoted 14 lines · Arnd Bergmann

On Friday 05 February 2016 10:44:29 Joao Pinto wrote:

I think in this case, we should do this completely differently:

How about putting all the new code into drivers/pci/host/pcie-designware.c as functions that can be used by the other drivers in absence of a chip specific handler?

Instead of providing a new instance of struct pcie_host_ops, maybe add it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init() for drivers that don't provide their own. "hisi_pcie_host_ops" currently provides no host_init() callback function, so you will have to change the hisi frontend to a provide nop-function.

Quoted 5 lines

For all other drivers, check if they can be changed to use your generic implementation and remove their private callbacks if possible.

I think the MSI implementation should be split out into a separate file though, as not everyone uses this.

I'm not sure I understand what you're proposing, Arnd, so let me ramble and you can direct me back on course.

Currently drivers/pci/host/pcie-designware.c is not usable by itself; it doesn't register a platform_driver.

There's hardly any code in Joao's patches; it looks like they add a minimal wrapper around the functionality in pcie-designware.c and register it as a platform_driver.

Are you suggesting that we should just add that functionality directly in pcie-designware.c so that file could both be a minimal driver with the functionality of Joao's patches, *and* continue to provide the shared code used by all the existing DesignWare-based drivers? Maybe the platform_driver registration part could be controlled by its own separate Kconfig option.

For example, he could make dw_pcie_link_up() look like:

  int dw_pcie_link_up(struct pcie_port *pp)
  {
    u32 val;

    if (pp->ops->link_up)
      return pp->ops->link_up(pp);

    val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
    return val & PCIE_PHY_DEBUG_R1_LINK_UP;

}

That seems like it would make sense to me. It would resolve the filename question, since there wouldn't be a new file. And if this is merely a driver for the generic DesignWare core without any extensions, I'm happy with some sort of "dw"-based driver name and compatibility string.

Quoted 31 lines · On Friday 05 February 2016 17:32:48 Bjorn Helgaas wrote:

Quoted 14 lines · Arnd Bergmann

On Friday 05 February 2016 10:44:29 Joao Pinto wrote:

I think in this case, we should do this completely differently:

How about putting all the new code into drivers/pci/host/pcie-designware.c as functions that can be used by the other drivers in absence of a chip specific handler?

Instead of providing a new instance of struct pcie_host_ops, maybe add it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init() for drivers that don't provide their own. "hisi_pcie_host_ops" currently provides no host_init() callback function, so you will have to change the hisi frontend to a provide nop-function.

Quoted 5 lines

For all other drivers, check if they can be changed to use your generic implementation and remove their private callbacks if possible.

I think the MSI implementation should be split out into a separate file though, as not everyone uses this.

I'm not sure I understand what you're proposing, Arnd, so let me ramble and you can direct me back on course.

Currently drivers/pci/host/pcie-designware.c is not usable by itself; it doesn't register a platform_driver.

There's hardly any code in Joao's patches; it looks like they add a minimal wrapper around the functionality in pcie-designware.c and register it as a platform_driver.

Quoted 6 lines

Are you suggesting that we should just add that functionality directly in pcie-designware.c so that file could both be a minimal driver with the functionality of Joao's patches, *and* continue to provide the shared code used by all the existing DesignWare-based drivers? Maybe the platform_driver registration part could be controlled by its own separate Kconfig option.

Either way is fine, we just have to be a little careful about the initialization ordering.

Quoted 6 lines

For example, he could make dw_pcie_link_up() look like:

  int dw_pcie_link_up(struct pcie_port *pp)
  {
    u32 val;

Quoted 6 lines

    if (pp->ops->link_up)
      return pp->ops->link_up(pp);

    val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
    return val & PCIE_PHY_DEBUG_R1_LINK_UP;

}

This is definitely good (after checking that all existing drivers either work with the generic version, or provide their own callbacks already).

Quoted 5 lines

That seems like it would make sense to me. It would resolve the filename question, since there wouldn't be a new file. And if this is merely a driver for the generic DesignWare core without any extensions, I'm happy with some sort of "dw"-based driver name and compatibility string.

The important part I think is that the new driver should not require and code that is seen as soc-specific: If it works with any implementation of pci-dw rather than a specific system, the driver should know how to do the right thing.

It may be helpful to move the actual matching on the compatible string and calling of the generic probe function into another module, if we are going forward with loadable PCI host drivers as posted by Paul Gortmaker today. Otherwise we end up with a device being bound to the generic driver when a more specific one exists and both are loadable modules, because the generic driver is always loaded first. As long as both drivers are built-in, it works fine because we first look for a driver matching the most specific compatible string.

Arnd