| Message ID | 20210630034653.10260-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Add Qualcomm PCIe Endpoint driver support | expand |
On Tue, Jun 29, 2021 at 9:47 PM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > Hello, > > This series adds support for Qualcomm PCIe Endpoint controller found > in platforms like SDX55. The Endpoint controller is based on the designware > core with additional Qualcomm wrappers around the core. > > The driver is added separately unlike other Designware based drivers that > combine RC and EP in a single driver. This is done to avoid complexity and > to maintain this driver autonomously. > > The driver has been validated with an out of tree MHI function driver on > SDX55 based Telit FN980 EVB connected to x86 host machine over PCIe. > > Thanks, > Mani > > Changes in v5: > > * Removed the DBI register settings that are not needed > * Used the standard definitions available in pci_regs.h > * Added defines for all the register fields > * Removed the left over code from previous iteration > > Changes in v4: > > * Removed the active_config settings needed for IPA integration > * Switched to writel for couple of relaxed versions that sneaked in I thought we resolved this discussion. Use _relaxed variants unless you need the stronger ones. Rob > > Changes in v3: > > * Lot of minor cleanups to the driver patch based on review from Bjorn and Stan. > * Noticeable changes are: > - Got rid of _relaxed calls and used readl/writel > - Got rid of separate TCSR memory region and used syscon for getting the > register offsets for Perst registers > - Changed the wake gpio handling logic > - Added remove() callback and removed "suppress_bind_attrs" > - stop_link() callback now just disables PERST IRQ > * Added MMIO region and doorbell interrupt to the binding > * Added logic to write MMIO physicall address to MHI base address as it is > for the function driver to work > > Changes in v2: > > * Addressed the comments from Rob on bindings patch > * Modified the driver as per binding change > * Fixed the warnings reported by Kbuild bot > * Removed the PERST# "enable_irq" call from probe() > > Manivannan Sadhasivam (3): > dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP > controller > PCI: dwc: Add Qualcomm PCIe Endpoint controller driver > MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding > > .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 160 ++++ > MAINTAINERS | 10 +- > drivers/pci/controller/dwc/Kconfig | 10 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-qcom-ep.c | 742 ++++++++++++++++++ > 5 files changed, 922 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml > create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c > > -- > 2.25.1 >
On Thu, Jul 01, 2021 at 09:25:01AM -0600, Rob Herring wrote: > On Tue, Jun 29, 2021 at 9:47 PM Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > > > Hello, > > > > This series adds support for Qualcomm PCIe Endpoint controller found > > in platforms like SDX55. The Endpoint controller is based on the designware > > core with additional Qualcomm wrappers around the core. > > > > The driver is added separately unlike other Designware based drivers that > > combine RC and EP in a single driver. This is done to avoid complexity and > > to maintain this driver autonomously. > > > > The driver has been validated with an out of tree MHI function driver on > > SDX55 based Telit FN980 EVB connected to x86 host machine over PCIe. > > > > Thanks, > > Mani > > > > Changes in v5: > > > > * Removed the DBI register settings that are not needed > > * Used the standard definitions available in pci_regs.h > > * Added defines for all the register fields > > * Removed the left over code from previous iteration > > > > Changes in v4: > > > > * Removed the active_config settings needed for IPA integration > > * Switched to writel for couple of relaxed versions that sneaked in > > I thought we resolved this discussion. Use _relaxed variants unless > you need the stronger ones. > I thought the discussion was resolved in favor of using read/writel. Here is the last reply from Bjorn: "I think we came to the conclusion that writel() was better than incorrect use of writel_relaxed() followed by wmb(). And in this particular case it's definitely not happening in a hot code path..." IMO, it is safer to use readl/writel calls than the relaxed variants. And so far the un-written rule I assumed is, only consider using the relaxed variants if the code is in hot path (but somehow I used the relaxed version in v1 :P ) Thanks, Mani > Rob > > > > > Changes in v3: > > > > * Lot of minor cleanups to the driver patch based on review from Bjorn and Stan. > > * Noticeable changes are: > > - Got rid of _relaxed calls and used readl/writel > > - Got rid of separate TCSR memory region and used syscon for getting the > > register offsets for Perst registers > > - Changed the wake gpio handling logic > > - Added remove() callback and removed "suppress_bind_attrs" > > - stop_link() callback now just disables PERST IRQ > > * Added MMIO region and doorbell interrupt to the binding > > * Added logic to write MMIO physicall address to MHI base address as it is > > for the function driver to work > > > > Changes in v2: > > > > * Addressed the comments from Rob on bindings patch > > * Modified the driver as per binding change > > * Fixed the warnings reported by Kbuild bot > > * Removed the PERST# "enable_irq" call from probe() > > > > Manivannan Sadhasivam (3): > > dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP > > controller > > PCI: dwc: Add Qualcomm PCIe Endpoint controller driver > > MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding > > > > .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 160 ++++ > > MAINTAINERS | 10 +- > > drivers/pci/controller/dwc/Kconfig | 10 + > > drivers/pci/controller/dwc/Makefile | 1 + > > drivers/pci/controller/dwc/pcie-qcom-ep.c | 742 ++++++++++++++++++ > > 5 files changed, 922 insertions(+), 1 deletion(-) > > create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml > > create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c > > > > -- > > 2.25.1 > >
On Mon, Jul 12, 2021 at 1:53 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Thu, Jul 01, 2021 at 09:25:01AM -0600, Rob Herring wrote: > > On Tue, Jun 29, 2021 at 9:47 PM Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> wrote: > > > > > > Hello, > > > > > > This series adds support for Qualcomm PCIe Endpoint controller found > > > in platforms like SDX55. The Endpoint controller is based on the designware > > > core with additional Qualcomm wrappers around the core. > > > > > > The driver is added separately unlike other Designware based drivers that > > > combine RC and EP in a single driver. This is done to avoid complexity and > > > to maintain this driver autonomously. > > > > > > The driver has been validated with an out of tree MHI function driver on > > > SDX55 based Telit FN980 EVB connected to x86 host machine over PCIe. > > > > > > Thanks, > > > Mani > > > > > > Changes in v5: > > > > > > * Removed the DBI register settings that are not needed > > > * Used the standard definitions available in pci_regs.h > > > * Added defines for all the register fields > > > * Removed the left over code from previous iteration > > > > > > Changes in v4: > > > > > > * Removed the active_config settings needed for IPA integration > > > * Switched to writel for couple of relaxed versions that sneaked in > > > > I thought we resolved this discussion. Use _relaxed variants unless > > you need the stronger ones. > > > > I thought the discussion was resolved in favor of using read/writel. Here > is the last reply from Bjorn: > > "I think we came to the conclusion that writel() was better > than incorrect use of writel_relaxed() followed by wmb(). And in this > particular case it's definitely not happening in a hot code path..." Certainly if you're needing wmb(), then you shouldn't be using _relaxed() variants. > IMO, it is safer to use readl/writel calls than the relaxed variants. Sure, and it's safer to have one big lock than it is to have no locks or lots of small locks. > And so far the un-written rule I assumed is, only consider using the > relaxed variants if the code is in hot path (but somehow I used the > relaxed version in v1 :P ) If you want any real conclusion, then best to fix the 'un-written' part. But what I say for every PCI review, is use the _relaxed() variants. Rob
Hello, This series adds support for Qualcomm PCIe Endpoint controller found in platforms like SDX55. The Endpoint controller is based on the designware core with additional Qualcomm wrappers around the core. The driver is added separately unlike other Designware based drivers that combine RC and EP in a single driver. This is done to avoid complexity and to maintain this driver autonomously. The driver has been validated with an out of tree MHI function driver on SDX55 based Telit FN980 EVB connected to x86 host machine over PCIe. Thanks, Mani Changes in v5: * Removed the DBI register settings that are not needed * Used the standard definitions available in pci_regs.h * Added defines for all the register fields * Removed the left over code from previous iteration Changes in v4: * Removed the active_config settings needed for IPA integration * Switched to writel for couple of relaxed versions that sneaked in Changes in v3: * Lot of minor cleanups to the driver patch based on review from Bjorn and Stan. * Noticeable changes are: - Got rid of _relaxed calls and used readl/writel - Got rid of separate TCSR memory region and used syscon for getting the register offsets for Perst registers - Changed the wake gpio handling logic - Added remove() callback and removed "suppress_bind_attrs" - stop_link() callback now just disables PERST IRQ * Added MMIO region and doorbell interrupt to the binding * Added logic to write MMIO physicall address to MHI base address as it is for the function driver to work Changes in v2: * Addressed the comments from Rob on bindings patch * Modified the driver as per binding change * Fixed the warnings reported by Kbuild bot * Removed the PERST# "enable_irq" call from probe() Manivannan Sadhasivam (3): dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller PCI: dwc: Add Qualcomm PCIe Endpoint controller driver MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 160 ++++ MAINTAINERS | 10 +- drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-qcom-ep.c | 742 ++++++++++++++++++ 5 files changed, 922 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c