Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Vidya Sagar <vidyas@nvidia.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: bhelgaas@google.com, lorenzo.pieralisi@arm.com,
	rjw@rjwysocki.net, lenb@kernel.org, andrew.murray@arm.com,
	treding@nvidia.com, jonathanh@nvidia.com,
	linux-tegra@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	kthota@nvidia.com, mmaddireddy@nvidia.com, sagar.tv@gmail.com,
	"Krzysztof Wilczyński" <kw@linux.com>
Subject: Re: [PATCH V3 2/2] PCI: Add MCFG quirks for Tegra194 host controllers
Date: Fri, 16 Apr 2021 19:15:42 +0530
Message-ID: <3b9d99cc-fa20-1906-f200-c993a8e4d9c9@nvidia.com> (raw)
In-Reply-To: <20210305215749.GA1117765@bjorn-Precision-5520>



On 3/6/2021 3:27 AM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> [+cc Krzysztof for .bus_shift below]
> 
> This is [2/2] but I don't see a [1/2].  Is there something missing?
> 
> On Sat, Jan 11, 2020 at 12:45:00AM +0530, Vidya Sagar wrote:
>> The PCIe controller in Tegra194 SoC is not completely ECAM-compliant.
>> With the current hardware design limitations in place, ECAM can be enabled
>> only for one controller (C5 controller to be precise) with bus numbers
>> starting from 160 instead of 0. A different approach is taken to avoid this
>> abnormal way of enabling ECAM for just one controller but to enable
>> configuration space access for all the other controllers. In this approach,
>> ops are added through MCFG quirk mechanism which access the configuration
>> spaces by dynamically programming iATU (internal AddressTranslation Unit)
>> to generate respective configuration accesses just like the way it is
>> done in DesignWare core sub-system.
> 
> Is this a published erratum in the device?  The purpose of specs is so
> we can run existing code on new platforms without having to add quirks
> like this, so I'm looking for some acknowledgement that this is an
> issue that will be fixed in future designs.
Yes. This would be fixed in the following SoC.

> 
> Ideally this would be a URL to published errata, and we would include
> the text or a synopsis here in the commit log.
> 
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> Reported-by: kbuild test robot <lkp@intel.com>
> 
> What is this "Reported-by" telling me?  Normally this would be a
> person who could supply more information about a defect we're fixing
> and might be able to test the fix.
I'll remove this.

> 
>> ---
>> V3:
>> * Removed MCFG address hardcoding in pci_mcfg.c file
>> * Started using 'dbi_base' for accessing root port's own config space
>> * and using 'config_base' for accessing config space of downstream hierarchy
>>
>> V2:
>> * Fixed build issues reported by kbuild test bot
> 
> Ah, I see this is probably where the "Reported-by" came from.  To me,
> it would make sense to add the tag if the commit *only* fixes the
> build problem so it's obvious what the robot reported.
> 
> But here, the build fix got squashed in before merging, so it's more
> like a general review comment and I think the robot's response on the
> mailing list is probably enough.
> 
>>   drivers/acpi/pci_mcfg.c                    |   7 ++
>>   drivers/pci/controller/dwc/Kconfig         |   3 +-
>>   drivers/pci/controller/dwc/Makefile        |   2 +-
>>   drivers/pci/controller/dwc/pcie-tegra194.c | 102 +++++++++++++++++++++
>>   include/linux/pci-ecam.h                   |   1 +
>>   5 files changed, 113 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index 6b347d9920cc..707181408173 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -116,6 +116,13 @@ static struct mcfg_fixup mcfg_quirks[] = {
>>        THUNDER_ECAM_QUIRK(2, 12),
>>        THUNDER_ECAM_QUIRK(2, 13),
>>
>> +     { "NVIDIA", "TEGRA194", 1, 0, MCFG_BUS_ANY, &tegra194_pcie_ops},
>> +     { "NVIDIA", "TEGRA194", 1, 1, MCFG_BUS_ANY, &tegra194_pcie_ops},
>> +     { "NVIDIA", "TEGRA194", 1, 2, MCFG_BUS_ANY, &tegra194_pcie_ops},
>> +     { "NVIDIA", "TEGRA194", 1, 3, MCFG_BUS_ANY, &tegra194_pcie_ops},
>> +     { "NVIDIA", "TEGRA194", 1, 4, MCFG_BUS_ANY, &tegra194_pcie_ops},
>> +     { "NVIDIA", "TEGRA194", 1, 5, MCFG_BUS_ANY, &tegra194_pcie_ops},
>> +
>>   #define XGENE_V1_ECAM_MCFG(rev, seg) \
>>        {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
>>                &xgene_v1_pcie_ecam_ops }
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index 0830dfcfa43a..f5b9e75aceed 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -255,7 +255,8 @@ config PCIE_TEGRA194
>>        select PHY_TEGRA194_P2U
>>        help
>>          Say Y here if you want support for DesignWare core based PCIe host
>> -       controller found in NVIDIA Tegra194 SoC.
>> +       controller found in NVIDIA Tegra194 SoC. ACPI platforms with Tegra194
>> +       don't need to enable this.
>>
>>   config PCIE_UNIPHIER
>>        bool "Socionext UniPhier PCIe controllers"
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index 8a637cfcf6e9..76a6c52b8500 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -17,7 +17,6 @@ obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
>>   obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>>   obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
>>   obj-$(CONFIG_PCI_MESON) += pci-meson.o
>> -obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
>>   obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>>
>>   # The following drivers are for devices that use the generic ACPI
>> @@ -33,4 +32,5 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>>   ifdef CONFIG_PCI
>>   obj-$(CONFIG_ARM64) += pcie-al.o
>>   obj-$(CONFIG_ARM64) += pcie-hisi.o
>> +obj-$(CONFIG_ARM64) += pcie-tegra194.o
>>   endif
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index cbe95f0ea0ca..660f55caa8be 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -21,6 +21,8 @@
>>   #include <linux/of_irq.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/pci.h>
>> +#include <linux/pci-acpi.h>
>> +#include <linux/pci-ecam.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>> @@ -285,6 +287,103 @@ struct tegra_pcie_dw {
>>        struct dentry *debugfs;
>>   };
>>
>> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
>> +struct tegra194_pcie_acpi  {
>> +     void __iomem *config_base;
>> +     void __iomem *iatu_base;
>> +     void __iomem *dbi_base;
>> +};
>> +
>> +static int tegra194_acpi_init(struct pci_config_window *cfg)
>> +{
>> +     struct device *dev = cfg->parent;
>> +     struct tegra194_pcie_acpi *pcie;
> 
> "pcie" doesn't seem like the best name for this since everywhere else
> in this driver, "pcie" is a "struct tegra_pcie_dw *".  Maybe "mcfg"
> or similar?
> 
> With some rename of tegra194_pcie_acpi along the same lines, since it
> really isn't ACPI-specific.  It's just that the ACPI MCFG table
> happens to be the way to discover the ECAM address space.
Understood. I'll take care of it in the next patch.

> 
>> +     pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>> +     if (!pcie)
>> +             return -ENOMEM;
>> +
>> +     pcie->config_base = cfg->win;
>> +     pcie->iatu_base = cfg->win + SZ_256K;
>> +     pcie->dbi_base = cfg->win + SZ_512K;
>> +     cfg->priv = pcie;
>> +
>> +     return 0;
>> +}
>> +
>> +static inline void atu_reg_write(struct tegra194_pcie_acpi *pcie, int index,
>> +                              u32 val, u32 reg)
> 
> "inline" is pointless, I think, since this isn't a performance path
> and the compiler will probably inline it by itself.
Ok. I'll remove it in the next patch.

> 
>> +{
>> +     u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
>> +
>> +     writel(val, pcie->iatu_base + offset + reg);
>> +}
>> +
>> +static void program_outbound_atu(struct tegra194_pcie_acpi *pcie, int index,
>> +                              int type, u64 cpu_addr, u64 pci_addr, u64 size)
>> +{
>> +     atu_reg_write(pcie, index, lower_32_bits(cpu_addr),
>> +                   PCIE_ATU_LOWER_BASE);
>> +     atu_reg_write(pcie, index, upper_32_bits(cpu_addr),
>> +                   PCIE_ATU_UPPER_BASE);
>> +     atu_reg_write(pcie, index, lower_32_bits(pci_addr),
>> +                   PCIE_ATU_LOWER_TARGET);
>> +     atu_reg_write(pcie, index, lower_32_bits(cpu_addr + size - 1),
>> +                   PCIE_ATU_LIMIT);
>> +     atu_reg_write(pcie, index, upper_32_bits(pci_addr),
>> +                   PCIE_ATU_UPPER_TARGET);
>> +     atu_reg_write(pcie, index, type, PCIE_ATU_CR1);
>> +     atu_reg_write(pcie, index, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>> +}
>> +
>> +static void __iomem *tegra194_map_bus(struct pci_bus *bus,
>> +                                   unsigned int devfn, int where)
>> +{
>> +     struct pci_config_window *cfg = bus->sysdata;
>> +     struct tegra194_pcie_acpi *pcie = cfg->priv;
>> +     u32 busdev;
>> +     int type;
>> +
>> +     if (bus->number < cfg->busr.start || bus->number > cfg->busr.end)
>> +             return NULL;
>> +
>> +     if (bus->number == cfg->busr.start) {
>> +             if (PCI_SLOT(devfn) == 0)
>> +                     return pcie->dbi_base + where;
>> +             else
>> +                     return NULL;
>> +     }
>> +
>> +     busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
>> +              PCIE_ATU_FUNC(PCI_FUNC(devfn));
>> +
>> +     if (bus->parent->number == cfg->busr.start) {
>> +             if (PCI_SLOT(devfn) == 0)
>> +                     type = PCIE_ATU_TYPE_CFG0;
>> +             else
>> +                     return NULL;
>> +     } else {
>> +             type = PCIE_ATU_TYPE_CFG1;
>> +     }
>> +
>> +     program_outbound_atu(pcie, PCIE_ATU_REGION_INDEX0, type,
>> +                          cfg->res.start, busdev, SZ_256K);
> 
> I don't see a PCIE_ATU_REGION_INDEX0 definition.  Maybe that's what's
> in the [1/2] patch?  I guess there's some way to be sure this ATU
> isn't used for other purposes?
> 
>> +     return (void __iomem *)(pcie->config_base + where);
> 
> Isn't the type correct even without the cast, same as above for
> "pcie->dbi_base + where"?
Correct. I'll address it in the next patch.

> 
>> +}
>> +
>> +struct pci_ecam_ops tegra194_pcie_ops = {
>> +     .bus_shift      = 20,
> 
> I think e7708f5b10e2 ("PCI: Unify ECAM constants in native PCI Express
> drivers") means you don't need this .bus_shift.
Correct. I'll remove this in the next patch.

> 
>> +     .init           = tegra194_acpi_init,
>> +     .pci_ops        = {
>> +             .map_bus        = tegra194_map_bus,
>> +             .read           = pci_generic_config_read,
>> +             .write          = pci_generic_config_write,
>> +     }
>> +};
>> +#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
>> +
>> +#ifdef CONFIG_PCIE_TEGRA194
>> +
>>   static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)
>>   {
>>        return container_of(pci, struct tegra_pcie_dw, pci);
>> @@ -1728,3 +1827,6 @@ MODULE_DEVICE_TABLE(of, tegra_pcie_dw_of_match);
>>   MODULE_AUTHOR("Vidya Sagar <vidyas@nvidia.com>");
>>   MODULE_DESCRIPTION("NVIDIA PCIe host controller driver");
>>   MODULE_LICENSE("GPL v2");
>> +
>> +#endif /* CONFIG_PCIE_TEGRA194 */
>> +
>> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
>> index a73164c85e78..6156140dcbb6 100644
>> --- a/include/linux/pci-ecam.h
>> +++ b/include/linux/pci-ecam.h
>> @@ -57,6 +57,7 @@ extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
>>   extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
>>   extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
>>   extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
>> +extern struct pci_ecam_ops tegra194_pcie_ops; /* Tegra194 PCIe */
>>   #endif
>>
>>   #ifdef CONFIG_PCI_HOST_COMMON
>> --
>> 2.17.1
>>

  parent reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-03 17:49 [PATCH] " Vidya Sagar
2020-01-03 18:04 ` Bjorn Helgaas
2020-01-04  3:44   ` Vidya Sagar
2020-01-17 12:17     ` Lorenzo Pieralisi
2020-01-20 11:10       ` Thierry Reding
2020-01-20 15:18         ` Lorenzo Pieralisi
2020-01-21 13:44           ` Thierry Reding
2020-01-23 10:49             ` Lorenzo Pieralisi
2020-02-06 16:46               ` Thierry Reding
2020-02-07 14:50                 ` Bjorn Helgaas
2020-02-07 16:51                   ` Thierry Reding
2020-02-07 18:34                     ` Bjorn Helgaas
2020-01-04 21:53 ` kbuild test robot
2020-01-06  8:27 ` [PATCH V2] " Vidya Sagar
2020-01-10 19:14   ` [PATCH V3 0/2] " Vidya Sagar
2020-01-10 19:14     ` [PATCH V3 1/2] arm64: tegra: Re-order PCIe aperture mappings to support ACPI boot Vidya Sagar
2020-06-29 13:31       ` Jon Hunter
2020-06-30 10:52         ` Vidya Sagar
2020-01-10 19:15     ` [PATCH V3 2/2] PCI: Add MCFG quirks for Tegra194 host controllers Vidya Sagar
2020-01-17 11:42       ` Thierry Reding
2021-03-05 21:57       ` Bjorn Helgaas
2021-03-05 23:04         ` Krzysztof Wilczyński
2021-04-16 13:45         ` Vidya Sagar [this message]
2021-04-16 13:45       ` [PATCH V4] " Vidya Sagar
2021-04-16 19:06         ` Bjorn Helgaas
2021-05-13  9:40       ` [PATCH V3 2/2] " Qu Wenruo
2021-05-13 13:05         ` Vidya Sagar
2020-01-16 17:18     ` [PATCH V3 0/2] " Vidya Sagar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3b9d99cc-fa20-1906-f200-c993a8e4d9c9@nvidia.com \
    --to=vidyas@nvidia.com \
    --cc=andrew.murray@arm.com \
    --cc=bhelgaas@google.com \
    --cc=helgaas@kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=kthota@nvidia.com \
    --cc=kw@linux.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=rjw@rjwysocki.net \
    --cc=sagar.tv@gmail.com \
    --cc=treding@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git