Hi,
On 31/10/18 8:03 AM, Xiaowei Bao wrote:
>
>
> -----Original Message-----
> From: Xiaowei Bao
> Sent: 2018年10月26日 17:19
> To: 'Kishon Vijay Abraham I' <kis...@ti.com>; bhelg...@google.com;
> robh...@kernel.org; mark.rutl...@arm.com; shawn...@kernel.org; Leo Li
> <leoyang...@nxp.com>; lorenzo.pieral...@arm.com; a...@arndb.de;
> gre...@linuxfoundation.org; M.h. Lian <minghuan.l...@nxp.com>; Mingkai Hu
> <mingkai...@nxp.com>; Roy Zang <roy.z...@nxp.com>;
> kstew...@linuxfoundation.org; cyrille.pitc...@free-electrons.com;
> pombreda...@nexb.com; shawn....@rock-chips.com; niklas.cas...@axis.com;
> linux-...@vger.kernel.org; devicet...@vger.kernel.org;
> linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linuxppc-dev@lists.ozlabs.org
> Cc: Jiafei Pan <jiafei....@nxp.com>
> Subject: RE: [PATCH 5/6] pci: layerscape: Add the EP mode support.
>
>
>
> -----Original Message-----
> From: Kishon Vijay Abraham I <kis...@ti.com>
> Sent: 2018年10月26日 13:29
> To: Xiaowei Bao <xiaowei....@nxp.com>; bhelg...@google.com;
> robh...@kernel.org; mark.rutl...@arm.com; shawn...@kernel.org; Leo Li
> <leoyang...@nxp.com>; lorenzo.pieral...@arm.com; a...@arndb.de;
> gre...@linuxfoundation.org; M.h. Lian <minghuan.l...@nxp.com>; Mingkai Hu
> <mingkai...@nxp.com>; Roy Zang <roy.z...@nxp.com>;
> kstew...@linuxfoundation.org; cyrille.pitc...@free-electrons.com;
> pombreda...@nexb.com; shawn....@rock-chips.com; niklas.cas...@axis.com;
> linux-...@vger.kernel.org; devicet...@vger.kernel.org;
> linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 5/6] pci: layerscape: Add the EP mode support.
>
> Hi,
>
> On Thursday 25 October 2018 04:39 PM, Xiaowei Bao wrote:
>> Add the PCIe EP mode support for layerscape platform.
>>
>> Signed-off-by: Xiaowei Bao <xiaowei....@nxp.com>
>> ---
>> drivers/pci/controller/dwc/Makefile | 2 +-
>> drivers/pci/controller/dwc/pci-layerscape-ep.c | 161
>> ++++++++++++++++++++++++
>> 2 files changed, 162 insertions(+), 1 deletions(-) create mode
>> 100644 drivers/pci/controller/dwc/pci-layerscape-ep.c
>>
>> diff --git a/drivers/pci/controller/dwc/Makefile
>> b/drivers/pci/controller/dwc/Makefile
>> index 5d2ce72..b26d617 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -8,7 +8,7 @@ obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
>> obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
>> obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>> obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>> -obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>> +obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o pci-layerscape-ep.o
>> obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>> obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>> obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o diff --git
>> a/drivers/pci/controller/dwc/pci-layerscape-ep.c
>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>> new file mode 100644
>> index 0000000..3b33bbc
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>> @@ -0,0 +1,161 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PCIe controller EP driver for Freescale Layerscape SoCs
>> + *
>> + * Copyright (C) 2018 NXP Semiconductor.
>> + *
>> + * Author: Xiaowei Bao <xiaowei....@nxp.com> */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/resource.h>
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define PCIE_DBI2_OFFSET 0x1000 /* DBI2 base address*/
>
> The base address should come from dt.
>> +
>> +struct ls_pcie_ep {
>> + struct dw_pcie *pci;
>> +};
>> +
>> +#define to_ls_pcie_ep(x) dev_get_drvdata((x)->dev)
>> +
>> +static bool ls_pcie_is_bridge(struct ls_pcie_ep *pcie) {
>> + struct dw_pcie *pci = pcie->pci;
>> + u32 header_type;
>> +
>> + header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
>> + header_type &= 0x7f;
>> +
>> + return header_type == PCI_HEADER_TYPE_BRIDGE; }
>> +
>> +static int ls_pcie_establish_link(struct dw_pcie *pci) {
>> + return 0;
>> +}
>
> There should be some way by which EP should tell RC that it is not configured
> yet. Are there no bits to control LTSSM state initialization or Configuration
> retry status enabling?
> [Xiaowei Bao] There have not bits to control LTSSM state to tell the RC it is
> configured. The start link is auto completed.
> [Xiaowei Bao] Hi Kishon, is there any advice?
If there is no HW support, I don't think anything could be done here. This
could result in RC reading configuration space even before EP is fully
initialized.
>> +
>> +static const struct dw_pcie_ops ls_pcie_ep_ops = {
>> + .start_link = ls_pcie_establish_link, };
>> +
>> +static const struct of_device_id ls_pcie_ep_of_match[] = {
>> + { .compatible = "fsl,ls-pcie-ep",},
>> + { },
>> +};
>> +
>> +static void ls_pcie_ep_init(struct dw_pcie_ep *ep) {
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + struct pci_epc *epc = ep->epc;
>> + enum pci_barno bar;
>> +
>> + for (bar = BAR_0; bar <= BAR_5; bar++)
>> + dw_pcie_ep_reset_bar(pci, bar);
>> +
>> + epc->features |= EPC_FEATURE_NO_LINKUP_NOTIFIER; }
>> +
>> +static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> + enum pci_epc_irq_type type, u16
>> interrupt_num) {
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +
>> + switch (type) {
>> + case PCI_EPC_IRQ_LEGACY:
>> + return dw_pcie_ep_raise_legacy_irq(ep, func_no);
>> + case PCI_EPC_IRQ_MSI:
>> + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>> + case PCI_EPC_IRQ_MSIX:
>> + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
>> + default:
>> + dev_err(pci->dev, "UNKNOWN IRQ type\n");
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct dw_pcie_ep_ops pcie_ep_ops = {
>> + .ep_init = ls_pcie_ep_init,
>> + .raise_irq = ls_pcie_ep_raise_irq,
>> +};
>> +
>> +static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
>> + struct platform_device *pdev)
>> +{
>> + struct dw_pcie *pci = pcie->pci;
>> + struct device *dev = pci->dev;
>> + struct dw_pcie_ep *ep;
>> + struct resource *res;
>> + int ret;
>> +
>> + ep = &pci->ep;
>> + ep->ops = &pcie_ep_ops;
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>> + if (!res)
>> + return -EINVAL;
>> +
>> + ep->phys_base = res->start;
>> + ep->addr_size = resource_size(res);
>> +
>> + ret = dw_pcie_ep_init(ep);
>> + if (ret) {
>> + dev_err(dev, "failed to initialize endpoint\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __init ls_pcie_ep_probe(struct platform_device *pdev) {
>> + struct device *dev = &pdev->dev;
>> + struct dw_pcie *pci;
>> + struct ls_pcie_ep *pcie;
>> + struct resource *dbi_base;
>> + int ret;
>> +
>> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>> + if (!pcie)
>> + return -ENOMEM;
>> +
>> + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
>> + if (!pci)
>> + return -ENOMEM;
>> +
>> + dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
>> + pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
>> + if (IS_ERR(pci->dbi_base))
>> + return PTR_ERR(pci->dbi_base);
>> +
>> + pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
>> + pci->dev = dev;
>> + pci->ops = &ls_pcie_ep_ops;
>> + pcie->pci = pci;
>> +
>> + if (ls_pcie_is_bridge(pcie))
>> + return -ENODEV;
>
> For an endpoint this condition should never occur. This should only mean, a
> wrong compatible has been used in dt.
> [Xiaowei Bao] This function is a way that can check the PCI controller
> whether work in EP mode, I think it is more safer. Of course, it can be
> removed.
> [Xiaowei Bao] Hi Kishon, is there any advice?
IMHO this check is not required.
Thanks
Kishon
