Message ID | c7af1603d4f18d11cb0ccee1c07708a1455ea4ed.1454600622.git.jpinto@synopsys.com (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Bjorn Helgaas |
Headers | show |
On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote: > 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@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: > +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 -- 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
Hi Bjorn, On 2/4/2016 6:19 PM, Bjorn Helgaas wrote: > On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote: >> 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@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. 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? > > 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: > >> +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. I am going to fetch your repo and get the host/pcie-synopsys to generate the next patch version. > > Bjorn > -- 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
On Thu, Feb 04, 2016 at 06:31:05PM +0000, Joao Pinto wrote: > On 2/4/2016 6:19 PM, Bjorn Helgaas wrote: > > On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote: > >> 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@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. > > 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? 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 -- 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
Hi, On 2/4/2016 11:43 PM, Bjorn Helgaas wrote: >> 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. 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? > > Bjorn > Joao -- 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
On Friday 05 February 2016 10:44:29 Joao Pinto wrote: > Hi, > > On 2/4/2016 11:43 PM, Bjorn Helgaas wrote: > >> 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. > > 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. Arnd -- 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
On 2/5/2016 2:39 PM, Arnd Bergmann wrote: > On Friday 05 February 2016 10:44:29 Joao Pinto wrote: >> Hi, >> >> On 2/4/2016 11:43 PM, Bjorn Helgaas wrote: >>>> 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. >> >> 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. > 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? > 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 > Joao -- 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
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? > > 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 -- 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
On 2/5/2016 3:43 PM, Arnd Bergmann wrote: > 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? >> >> 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, sure and I would love to be useful and improve the dw pci ecosystem, but maybe now we should submit the driver as suggested in "[PATCH] synopsys pcie rc generic platform driver update" and then make in a future intervention. Bjorn, could you please comment? Should we go now with this driver version and in the future we make the changes suggested by Arnd? > > Arnd > Joao -- 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
On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote: > On Friday 05 February 2016 10:44:29 Joao Pinto wrote: > > Hi, > > > > On 2/4/2016 11:43 PM, Bjorn Helgaas wrote: > > >> 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. > > > > 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. 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. Bjorn -- 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
On Friday 05 February 2016 17:32:48 Bjorn Helgaas wrote: > On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote: > > 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. > > > > 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. Either way is fine, we just have to be a little careful about the initialization ordering. > 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; > } This is definitely good (after checking that all existing drivers either work with the generic version, or provide their own callbacks already). > 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 -- 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
Hi Bjorn and Arnd, I agree with you in adding the platform register mechanism to the pcie-designware core driver. This feature is excellent for who wants to quickly have a taste of a pcie-designware platform drive for their Synopsys PCIe RC controller. Do you have some example for me to check in order to follow the same logic. Adding something like this in pcie-designware: #ifdef CONFIG_PCIE_DW_PLAT <add compatibility and platform register code> #endif would be acceptable? maybe the platform tweeks should be external makeing it cleaner like Arnd suggested. An example to follow would be nice. Thanks, Joao On 2/8/2016 12:31 PM, Arnd Bergmann wrote: > On Friday 05 February 2016 17:32:48 Bjorn Helgaas wrote: >> On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote: >>> 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. >>> >>> 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. > > Either way is fine, we just have to be a little careful about the > initialization ordering. > >> 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; >> } > > This is definitely good (after checking that all existing drivers > either work with the generic version, or provide their own callbacks > already). > >> 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 > -- 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
diff --git a/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt b/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt new file mode 100644 index 0000000..ee2a877 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt @@ -0,0 +1,36 @@ +Synopsys PCI RC IP Prototyping Kit +---------------------------------- + +This is the reference platform driver to be used in the Synopsys PCI Root +Complex IP Prototyping Kit. + +Required properties: +- compatible: set to "snps,dw-pcie" or "snps,ipk-pcie"; +- reg: base address and length of the pcie controller registers and +configuration address space +- reg-names: Must be "config" for the PCIe configuration space. +- #address-cells: set to <3> +- #size-cells: set to <2> +- device_type: set to "pci" +- ranges: ranges for the PCI memory and I/O regions. +- interrupts: one interrupt source for MSI interrupts, followed by interrupt + source for hardware related interrupts. +- #interrupt-cells: set to <1> +- num-lanes: set to <1>; + +Example configuration: + + pcie: pcie@0xdffff000 { + compatible = "snps,dw-pcie"; + reg = <0xdffff000 0x1000 + 0xd0000000 0x2000>; + reg-names = "csr", "config"; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000 + 0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>; + interrupts = <25>, <24>; + #interrupt-cells = <1>; + num-lanes = <1>; + }; diff --git a/MAINTAINERS b/MAINTAINERS index e9caa4b..8362189 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8230,6 +8230,13 @@ S: Maintained F: Documentation/devicetree/bindings/pci/hisilicon-pcie.txt F: drivers/pci/host/pcie-hisi.c +PCI DRIVER FOR SYNOPSYS PROTOTYPING DEVICE +M: Joao Pinto <jpinto@synopsys.com> +L: linux-pci@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt +F: drivers/pci/host/pcie-dw-pltfm.c + PCMCIA SUBSYSTEM P: Linux PCMCIA Team L: linux-pcmcia@lists.infradead.org diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index f131ba9..61cdc72 100644 --- a/drivers/pci/host/Kconfig +++ b/drivers/pci/host/Kconfig @@ -172,4 +172,12 @@ config PCI_HISI help Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC +config PCIE_DW_PLAT + bool "Platform Driver for Synopsys PCIe controller" + depends on ARC && OF + select PCIEPORTBUS + select PCIE_DW + help + Say Y here if you want to enable Synopsys PCIe controller platform + driver. endmenu diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 9d4d3c6..8c84ba4 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -20,3 +20,4 @@ obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o obj-$(CONFIG_PCI_HISI) += pcie-hisi.o +obj-$(CONFIG_PCIE_DW_PLAT) += pcie-dw-pltfm.o diff --git a/drivers/pci/host/pcie-dw-pltfm.c b/drivers/pci/host/pcie-dw-pltfm.c new file mode 100644 index 0000000..0924ec4 --- /dev/null +++ b/drivers/pci/host/pcie-dw-pltfm.c @@ -0,0 +1,244 @@ +/* + * PCIe RC driver for Synopsys Designware Core + * + * Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com) + * + * Authors: Manjunath Bettegowda <manjumb@synopsys.com>, + * Jie Deng <jiedeng@synopsys.com> + * Joao Pinto <jpinto@synopsys.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_gpio.h> +#include <linux/pci.h> +#include <linux/platform_device.h> +#include <linux/resource.h> +#include <linux/signal.h> +#include <linux/types.h> + +#include "pcie-designware.h" + +#define to_dw_pltfm_pcie(x) container_of(x, struct dw_pltfm_pcie, pp) + +struct dw_pltfm_pcie { + void __iomem *mem_base; /* Memory Base to access Core's [RC] + * Config Space Layout + */ + struct pcie_port pp; /* RC Root Port specific structure - + * DWC_PCIE_RC stuff + */ +}; + +#define PCI_EQUAL_CONTROL_PHY 0x00000707 +#define PCIE_PHY_DEBUG_R1_LINK_UP 0x00000010 + +/* PCIe Port Logic registers (memory-mapped) */ +#define PLR_OFFSET 0x700 +#define PCIE_PHY_DEBUG_R0 (PLR_OFFSET + 0x28) /* 0x728 */ +#define PCIE_PHY_DEBUG_R1 (PLR_OFFSET + 0x2c) /* 0x72c */ + +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); +} + +static void dw_pltfm_pcie_init_phy(struct pcie_port *pp) +{ + /* write Lane 0 Equalization Control fields register */ + writel(PCI_EQUAL_CONTROL_PHY, pp->dbi_base + 0x154); +} + +static int dw_pltfm_pcie_deassert_core_reset(struct pcie_port *pp) +{ + return 0; +} + +static void dw_pltfm_pcie_establish_link(struct pcie_port *pp) +{ + int retries = 0; + + /* check if the link is up or not */ + for (retries = 0; retries < 10; retries++) { + if (dw_pcie_link_up(pp)) { + dev_info(pp->dev, "Link up\n"); + return; + } + msleep(100); + } +} + +/* + * dw_pltfm_pcie_host_init() + * Platform specific host/RC initialization + * a. Assert the core reset + * b. Assert and deassert phy reset and initialize the phy + * c. De-Assert the core reset + * d. Initializet the Root Port (BARs/Memory Or IO/ Interrupt/ Commnad Reg) + * e. Initiate Link startup procedure + * + */ +static void dw_pltfm_pcie_host_init(struct pcie_port *pp) +{ + /* Initialize Phy (Reset/poweron/control-inputs ) */ + dw_pltfm_pcie_init_phy(pp); + + dw_pltfm_pcie_deassert_core_reset(pp); + + dw_pcie_setup_rc(pp); + + dw_pltfm_pcie_establish_link(pp); + + if (IS_ENABLED(CONFIG_PCI_MSI)) + dw_pcie_msi_init(pp); +} + +static int dw_pltfm_pcie_link_up(struct pcie_port *pp) +{ + u32 val; + + val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1); + return val & PCIE_PHY_DEBUG_R1_LINK_UP; +} + +/** + * This is RC operation structure + * dw_pltfm_pcie_link_up: the function which initiates the phy link up procedure + * dw_pltfm_pcie_host_init: the function which does the host/RC Root port + * initialization. + */ +static struct pcie_host_ops dw_pltfm_pcie_host_ops = { + .link_up = dw_pltfm_pcie_link_up, + .host_init = dw_pltfm_pcie_host_init, +}; + +/** + * dw_pltfm_add_pcie_port + * This function + * a. installs the interrupt handler + * b. registers host operations in the pcie_port structure + */ +static int dw_pltfm_add_pcie_port(struct pcie_port *pp, + struct platform_device *pdev) +{ + int ret; + + pp->irq = platform_get_irq(pdev, 1); + + if (pp->irq < 0) + return pp->irq; + + if (IS_ENABLED(CONFIG_PCI_MSI)) { + pp->msi_irq = platform_get_irq(pdev, 0); + + if (pp->msi_irq < 0) + return pp->msi_irq; + + ret = devm_request_irq(&pdev->dev, pp->msi_irq, + dw_pltfm_pcie_msi_irq_handler, + IRQF_SHARED, "dw-pltfm-pcie-msi", pp); + if (ret) { + dev_err(&pdev->dev, "failed to request msi irq\n"); + return ret; + } + } + + pp->root_bus_nr = -1; + pp->ops = &dw_pltfm_pcie_host_ops; + + /* Below function: + * Checks for range property from DT + * Gets the IO and MEMORY and CONFIG-Space ranges from DT + * Does IOREMAPS on the physical addresses + * Gets the num-lanes from DT + * Gets MSI capability from DT + * Calls the platform specific host initialization + * Program the correct class, BAR0, Link width, in Config space + * Then it calls pci common init routine + * Then it calls function to assign "unassigned resources" + */ + ret = dw_pcie_host_init(pp); + if (ret) { + dev_err(&pdev->dev, "failed to initialize host\n"); + return ret; + } + + return 0; +} + +/** + * dw_pltfm_pcie_probe() + * This function gets called as part of pcie registration. if the id matches + * the platform driver framework will call this function. + * + * @pdev: Pointer to the platform_device structure + * + * Returns zero on success; Negative errorno on failure + */ +static int dw_pltfm_pcie_probe(struct platform_device *pdev) +{ + struct dw_pltfm_pcie *dw_pltfm_pcie; + struct pcie_port *pp; + struct resource *res; /* Resource from DT */ + int ret; + + dw_pltfm_pcie = devm_kzalloc(&pdev->dev, sizeof(*dw_pltfm_pcie), + GFP_KERNEL); + if (!dw_pltfm_pcie) + return -ENOMEM; + + pp = &dw_pltfm_pcie->pp; + pp->dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + if (!res) + return -ENODEV; + + dw_pltfm_pcie->mem_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(dw_pltfm_pcie->mem_base)) + return PTR_ERR(dw_pltfm_pcie->mem_base); + + pp->dbi_base = dw_pltfm_pcie->mem_base; + + ret = dw_pltfm_add_pcie_port(pp, pdev); + if (ret < 0) + return ret; + + platform_set_drvdata(pdev, dw_pltfm_pcie); + + return 0; +} + +static const struct of_device_id dw_pltfm_pcie_of_match[] = { + { .compatible = "snps,dw-pcie", }, + { .compatible = "snps,ipk-pcie", }, + {}, +}; +MODULE_DEVICE_TABLE(of, dw_pltfm_pcie_of_match); + +static struct platform_driver dw_pltfm_pcie_driver = { + .driver = { + .name = "dw-pcie", + .of_match_table = dw_pltfm_pcie_of_match, + }, + .probe = dw_pltfm_pcie_probe, +}; + +module_platform_driver(dw_pltfm_pcie_driver); + +MODULE_AUTHOR("Manjunath Bettegowda <manjumb@synopsys.com>"); +MODULE_DESCRIPTION("Synopsys PCIe host controller driver"); +MODULE_LICENSE("GPL v2");
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@synopsys.com> --- Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann): - driver name was changed from pcie-synopsys to pcie-dw-pltfm - mdelay() replaced for msleep() in the new driver - Devicetree bindings for the new driver was updated (config space removed from ranges) - Unnecessary synopsys_pcie_irq_handler() was removed - Driver compatibility strings updated Change v6 -> v7 (Bjorn Helgaas): - driver name was changed from pcie-snpsdev to pcie-synopsys - driver internals (functions and certain variables) also changed name accordingly - devicetree bindings documentation also changed accordingly - quirk function dw_pcie_link_retrain() was removed (tests were made successfully without it) - driver was changed to meet pci standards (link up confirmation routine, init rc functions, etc.) - EPROBE_DEFER were removed Change v5 -> v6: - Nothing changed (just to keep up with patch set version). Change v4 -> v5: - Nothing changed (just to keep up with patch set version). Changes v3 -> v4 (Bjorn Helgaas): - ARCH dependencies were added to the drivers/pci/host/kconfig for the PCIE_SNPSDEV. Changes v2 -> v3 (Bjorn Helgaas): - link init stuff was moved to a snpsdev_pcie_establish_link() function in pcie-snpsdev - pcie-snpsdev driver declaration was changed to be more standard (Bjorn Helgaas) - pcie-designware' dw_pcie_link_retrain() now use standard registers from pci-regs.h (Bjorn Helgaas) - pcie-snpsdev.txt was complemented with more info (Mark Rutland) Changes v1 -> v2 (Bjorn Helgaas): - Fixups snpsdev_pcie_fixup_bridge() and snpsdev_pcie_fixup_res() were removed from the driver (these functions were for specific tests only and not usefull in mainline) - Driver' comments were reviewed (fix Typos and excessive comments removal) - Removed unnecessary definitions in the driver source (PCIE_PHY_CTRL and PCIE_PHY_STAT) .../devicetree/bindings/pci/pcie-dw-pltfm.txt | 36 +++ MAINTAINERS | 7 + drivers/pci/host/Kconfig | 8 + drivers/pci/host/Makefile | 1 + drivers/pci/host/pcie-dw-pltfm.c | 244 +++++++++++++++++++++ 5 files changed, 296 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt create mode 100644 drivers/pci/host/pcie-dw-pltfm.c