* Re: [PATCH 13/15] PCI/ASPM: use permission-specific DEVICE_ATTR variants
From: Bjorn Helgaas @ 2016-11-14 21:40 UTC (permalink / raw)
To: Julia Lawall; +Cc: Bjorn Helgaas, kernel-janitors, linux-pci, linux-kernel
In-Reply-To: <1477769829-22230-14-git-send-email-Julia.Lawall@lip6.fr>
On Sat, Oct 29, 2016 at 09:37:07PM +0200, Julia Lawall wrote:
> Use DEVICE_ATTR_RW for read-write attributes. This simplifies the
> source code, improves readbility, and reduces the chance of
> inconsistencies.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @rw@
> declarer name DEVICE_ATTR;
> identifier x,x_show,x_store;
> @@
>
> DEVICE_ATTR(x, \(0644\|S_IRUGO|S_IWUSR\), x_show, x_store);
>
> @script:ocaml@
> x << rw.x;
> x_show << rw.x_show;
> x_store << rw.x_store;
> @@
>
> if not (x^"_show" = x_show && x^"_store" = x_store)
> then Coccilib.include_match false
>
> @@
> declarer name DEVICE_ATTR_RW;
> identifier rw.x,rw.x_show,rw.x_store;
> @@
>
> - DEVICE_ATTR(x, \(0644\|S_IRUGO|S_IWUSR\), x_show, x_store);
> + DEVICE_ATTR_RW(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
I applied this to pci/aspm to follow the herd, although it looks
pretty similar to the ill-fated "Replace numeric parameter like 0444
with macro" series (http://lwn.net/Articles/696229/). Maybe this is
different because everybody except me knows what ATTR_RW means? To
me, "0644" contained more information than "_RW" does.
I do certainly like the removal of the "_show" and "_store"
redundancy.
> ---
> drivers/pci/pcie/aspm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 0ec649d..3b14d9e 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -886,8 +886,8 @@ static ssize_t clk_ctl_store(struct device *dev,
> return n;
> }
>
> -static DEVICE_ATTR(link_state, 0644, link_state_show, link_state_store);
> -static DEVICE_ATTR(clk_ctl, 0644, clk_ctl_show, clk_ctl_store);
> +static DEVICE_ATTR_RW(link_state);
> +static DEVICE_ATTR_RW(clk_ctl);
>
> static char power_group[] = "power";
> void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev)
>
^ permalink raw reply
* Re: [PATCH -next] PCI: dra7xx: Add missing of_node_put() in dra7xx_pcie_init_irq_domain()
From: Heiko Stuebner @ 2016-11-14 21:37 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Kishon Vijay Abraham I, Wei Yongjun, Bjorn Helgaas, Wei Yongjun,
linux-omap, linux-pci, Shawn Lin, michal.simek, soren.brinkmann,
bharat.kumar.gogada, robh, Frank Rowand, devicetree
In-Reply-To: <20161114211928.GD9868@bhelgaas-glaptop.roam.corp.google.com>
Am Montag, 14. November 2016, 15:19:28 CET schrieb Bjorn Helgaas:
> [+cc Shawn, Heiko, Michal, Soren, Bharat, Rob H, Frank, devicetree@vger]
>
> On Sat, Nov 12, 2016 at 12:39:01PM +0530, Kishon Vijay Abraham I wrote:
> > Hi,
> >
> > On Saturday 12 November 2016 03:08 AM, Bjorn Helgaas wrote:
> > > On Mon, Oct 17, 2016 at 02:54:37PM +0000, Wei Yongjun wrote:
> > >> From: Wei Yongjun <weiyongjun1@huawei.com>
> > >>
> > >> This node pointer is returned by of_get_next_child() with refcount
> > >> incremented in this function. of_node_put() on it before exitting
> > >> this function on error.
> > >>
> > >> This is detected by Coccinelle semantic patch.
> > >>
> > >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > >
> > > Kishon, this looks correct to me, so I applied it to pci/host-dra7xx for
> > > v4.10. Let me know if you have any issue with it.
> > >
> > >> ---
> > >>
> > >> drivers/pci/host/pci-dra7xx.c | 1 +
> > >> 1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/drivers/pci/host/pci-dra7xx.c
> > >> b/drivers/pci/host/pci-dra7xx.c
> > >> index 9595fad..79297e9 100644
> > >> --- a/drivers/pci/host/pci-dra7xx.c
> > >> +++ b/drivers/pci/host/pci-dra7xx.c
> > >> @@ -177,6 +177,7 @@ static int dra7xx_pcie_init_irq_domain(struct
> > >> pcie_port *pp)> >>
> > >> &intx_domain_ops, pp);
> > >>
> > >> if (!pp->irq_domain) {
> > >>
> > >> dev_err(dev, "Failed to get a INTx IRQ domain\n");
> > >>
> > >> + of_node_put(pcie_intc_node);
> >
> > I think of_node_put should be used for both the error case and non-error
> > case.
> Hmm, OK. I don't know what the rules are. Certainly if we made these
> drivers modular, I don't think we'd want to leak these references
> every time we unload/reload the module. Should we do the put
> immediately here, or in the module remove path, or ...?
>
> Adding other driver and DT folks for comment.
I think the function above (dra7xx_pcie_init_irq_domain) can do the
of_node_put at its end in all cases as suggested by Kishon.
of_get_next_child() will increment the refcount
irq_domain_add_linear()
__irq_domain_add() also increments the refcount
irq_domain_remove() will decrement the refcount
So it should be safe to decrement the refcount in
dra7xx_pcie_init_irq_domain() in all cases as the irq-domain internals will
always keep it above 1 as long as the node is used.
Heiko
^ permalink raw reply
* Re: [PATCH 3/4] PCI: dra7xx: Add support to force RC to work in GEN1 mode
From: Bjorn Helgaas @ 2016-11-14 21:24 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Bjorn Helgaas, Rob Herring, linux-omap, linux-pci, devicetree,
linux-kernel, nsekhar, Shawn Lin
In-Reply-To: <8ecc1325-365d-bdec-d435-729e0ea49d20@ti.com>
[+cc Shawn]
On Sat, Nov 12, 2016 at 12:40:10PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Saturday 12 November 2016 02:45 AM, Bjorn Helgaas wrote:
> > Hi Kishon,
> >
> > On Tue, Oct 11, 2016 at 06:28:34PM +0530, Kishon Vijay Abraham I wrote:
> >> PCIe in AM57x/DRA7x devices is by default
> >> configured to work in GEN2 mode. However there
> >> may be situations when working in GEN1 mode is
> >> desired. One example is limitation i925 (PCIe GEN2
> >> mode not supported at junction temperatures < 0C).
> >>
> >> Add support to force Root Complex to work in GEN1
> >> mode if so desired, but don't force GEN1 mode on
> >> any board just yet.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> >> ---
> >> Documentation/devicetree/bindings/pci/ti-pci.txt | 1 +
> >> drivers/pci/host/pci-dra7xx.c | 27 ++++++++++++++++++++++
> >> 2 files changed, 28 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> index 60e2516..a3d6ca3 100644
> >> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> @@ -25,6 +25,7 @@ PCIe Designware Controller
> >>
> >> Optional Property:
> >> - gpios : Should be added if a gpio line is required to drive PERST# line
> >> + - ti,pcie-is-gen1 : Force the PCIe controller to work in GEN1 (2.5 GT/s).
> >
> > Can we use "max-link-speed" so it's similar to imx6?
>
> yeah, maybe we should make it a generic PCI property?
I forgot that Shawn has already done this! I had already merged those
patches on pci/host-rockchip, but I moved them to pci/host since
they're not Rockchip-specific. Can you take a look at that and see if
you can do what you need based on that pci/host branch?
Bjorn
^ permalink raw reply
* Re: [PATCH 5/5] PCI: Balance ports to avoid ACS errata on Pericom switches
From: Alex Williamson @ 2016-11-14 21:21 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, bhelgaas, iommu, linux-kernel
In-Reply-To: <20161114210319.GC9868@bhelgaas-glaptop.roam.corp.google.com>
On Mon, 14 Nov 2016 15:03:19 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Oct 26, 2016 at 12:01:40PM -0600, Alex Williamson wrote:
> > As described in the included code comment, this quirk is intended to
> > work around an errata in a variety of Pericom 4-lane, 3 and 4 port
> > PCIe 2.0 switches. The switches advertise ACS capabilities, but the
> > P2P Request Redirection support includes an errata that PCI_ACS_RR
> > effectively doesn't work and results in transactions being queued and
> > not delivered within the PCIe switch. The errata has no planned
> > hardware fix.
>
> Is there a published erratum we can reference here? It'd be really
> nice to have a URL.
Unfortunately only the product briefs seem to be public. I was sent an
errata, but it's marked confidential, so I don't think I'll risk adding
it to the bz. I haven't even been granted access to the datasheet.
I'm only guessing at the affected devices IDs based on my sample of one.
One thing I've thought of since I posted this series is that it's
possible to have a configuration where the downstream ports don't all
match. If the upstream port is running at 5GT/s, the first downstream
port is also running 5GT/s, but another downstream port is running
2.5GT/s, this code will retrain the upstream port to 2.5GT/s w/o
revisiting that first port. I should fix that, but I likely won't have
time for v4.10. If you want to de-queue this, I'll try to look at it
again for v4.11 and take your other suggestions into account. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH -next] PCI: dra7xx: Add missing of_node_put() in dra7xx_pcie_init_irq_domain()
From: Bjorn Helgaas @ 2016-11-14 21:19 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Wei Yongjun, Bjorn Helgaas, Wei Yongjun, linux-omap, linux-pci,
Shawn Lin, Heiko Stuebner, michal.simek, soren.brinkmann,
bharat.kumar.gogada, robh, Frank Rowand, devicetree
In-Reply-To: <1d95b915-ddc0-b48f-270b-fffb60ecfe5e@ti.com>
[+cc Shawn, Heiko, Michal, Soren, Bharat, Rob H, Frank, devicetree@vger]
On Sat, Nov 12, 2016 at 12:39:01PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Saturday 12 November 2016 03:08 AM, Bjorn Helgaas wrote:
> > On Mon, Oct 17, 2016 at 02:54:37PM +0000, Wei Yongjun wrote:
> >> From: Wei Yongjun <weiyongjun1@huawei.com>
> >>
> >> This node pointer is returned by of_get_next_child() with refcount
> >> incremented in this function. of_node_put() on it before exitting
> >> this function on error.
> >>
> >> This is detected by Coccinelle semantic patch.
> >>
> >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> >
> > Kishon, this looks correct to me, so I applied it to pci/host-dra7xx for
> > v4.10. Let me know if you have any issue with it.
> >
> >> ---
> >> drivers/pci/host/pci-dra7xx.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> >> index 9595fad..79297e9 100644
> >> --- a/drivers/pci/host/pci-dra7xx.c
> >> +++ b/drivers/pci/host/pci-dra7xx.c
> >> @@ -177,6 +177,7 @@ static int dra7xx_pcie_init_irq_domain(struct pcie_port *pp)
> >> &intx_domain_ops, pp);
> >> if (!pp->irq_domain) {
> >> dev_err(dev, "Failed to get a INTx IRQ domain\n");
> >> + of_node_put(pcie_intc_node);
>
> I think of_node_put should be used for both the error case and non-error case.
Hmm, OK. I don't know what the rules are. Certainly if we made these
drivers modular, I don't think we'd want to leak these references
every time we unload/reload the module. Should we do the put
immediately here, or in the module remove path, or ...?
Adding other driver and DT folks for comment.
I dropped these patches for now (dra7xx, rockchip, xilinx-nwl,
xilinx).
^ permalink raw reply
* Re: [PATCH 5/5] PCI: Balance ports to avoid ACS errata on Pericom switches
From: Bjorn Helgaas @ 2016-11-14 21:03 UTC (permalink / raw)
To: Alex Williamson; +Cc: linux-pci, bhelgaas, iommu, linux-kernel
In-Reply-To: <20161026180140.23495.27388.stgit@gimli.home>
On Wed, Oct 26, 2016 at 12:01:40PM -0600, Alex Williamson wrote:
> As described in the included code comment, this quirk is intended to
> work around an errata in a variety of Pericom 4-lane, 3 and 4 port
> PCIe 2.0 switches. The switches advertise ACS capabilities, but the
> P2P Request Redirection support includes an errata that PCI_ACS_RR
> effectively doesn't work and results in transactions being queued and
> not delivered within the PCIe switch. The errata has no planned
> hardware fix.
Is there a published erratum we can reference here? It'd be really
nice to have a URL.
^ permalink raw reply
* Re: [PATCH 2/5] PCI: Extract link speed & width retrieval from pcie_get_minimum_link()
From: Bjorn Helgaas @ 2016-11-14 21:02 UTC (permalink / raw)
To: Alex Williamson; +Cc: linux-pci, bhelgaas, iommu, linux-kernel
In-Reply-To: <20161026180122.23495.26258.stgit@gimli.home>
On Wed, Oct 26, 2016 at 12:01:22PM -0600, Alex Williamson wrote:
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> drivers/pci/pci.c | 26 ++++++++++++++++++++------
> include/linux/pci.h | 2 ++
> 2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b901ee7..6d6cf89 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4729,6 +4729,25 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
> }
> EXPORT_SYMBOL(pcie_set_mps);
>
> +int pcie_get_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> + enum pcie_link_width *width)
Seems like "pcie_get_link" is missing a word. I know
pcie_get_minimum_link() exists already and is similar.
pcie_get_link_speed(), maybe? I know it also gets the width, so maybe
there's a more inclusive term that would be better.
> +{
> + int ret;
> + u16 lnksta;
> +
> + ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
> + if (ret)
> + return ret;
> +
> + if (speed)
> + *speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
> + if (width)
> + *width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
> + PCI_EXP_LNKSTA_NLW_SHIFT;
> +
> + return 0;
> +}
> +
> /**
> * pcie_get_minimum_link - determine minimum link settings of a PCI device
> * @dev: PCI device to query
> @@ -4747,18 +4766,13 @@ int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> *width = PCIE_LNK_WIDTH_UNKNOWN;
>
> while (dev) {
> - u16 lnksta;
> enum pci_bus_speed next_speed;
> enum pcie_link_width next_width;
>
> - ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
> + ret = pcie_get_link(dev, &next_speed, &next_width);
> if (ret)
> return ret;
>
> - next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
> - next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
> - PCI_EXP_LNKSTA_NLW_SHIFT;
> -
> if (next_speed < *speed)
> *speed = next_speed;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c3248d5..fbfbb40 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1026,6 +1026,8 @@ static inline int pci_is_managed(struct pci_dev *pdev)
> int pcie_set_mps(struct pci_dev *dev, int mps);
> int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> enum pcie_link_width *width);
> +int pcie_get_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> + enum pcie_link_width *width);
> int __pci_reset_function(struct pci_dev *dev);
> int __pci_reset_function_locked(struct pci_dev *dev);
> int pci_reset_function(struct pci_dev *dev);
>
> --
> 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
^ permalink raw reply
* Re: [PATCH 1/5] PCI: Make pci_std_enable_acs() non-static
From: Bjorn Helgaas @ 2016-11-14 20:59 UTC (permalink / raw)
To: Alex Williamson; +Cc: linux-pci, bhelgaas, iommu, linux-kernel
In-Reply-To: <20161026180116.23495.77322.stgit@gimli.home>
On Wed, Oct 26, 2016 at 12:01:16PM -0600, Alex Williamson wrote:
> For use by quirks.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> drivers/pci/pci.c | 2 +-
> include/linux/pci.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..b901ee7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2728,7 +2728,7 @@ void pci_request_acs(void)
> * pci_std_enable_acs - enable ACS on devices using standard ACS capabilites
> * @dev: the PCI device
> */
> -static void pci_std_enable_acs(struct pci_dev *dev)
> +void pci_std_enable_acs(struct pci_dev *dev)
> {
> int pos;
> u16 cap;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0ab8359..c3248d5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1900,6 +1900,7 @@ static inline int pci_pcie_type(const struct pci_dev *dev)
> bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
> bool pci_acs_path_enabled(struct pci_dev *start,
> struct pci_dev *end, u16 acs_flags);
> +void pci_std_enable_acs(struct pci_dev *dev);
I think putting this in drivers/pci/pci.h would be sufficient for what
you need, wouldn't it? Same for pcie_get_link() and pcie_retrain_link().
> #define PCI_VPD_LRDT 0x80 /* Large Resource Data Type */
> #define PCI_VPD_LRDT_ID(x) ((x) | PCI_VPD_LRDT)
>
^ permalink raw reply
* Re: [PATCH] PCI: rockchip: correct the use of FTS mask
From: Bjorn Helgaas @ 2016-11-14 20:17 UTC (permalink / raw)
To: Brian Norris
Cc: Bjorn Helgaas, linux-kernel, Brian Norris, Shawn Lin, Wenrui Li,
Heiko Stuebner, linux-pci, linux-rockchip, Rajat Jain
In-Reply-To: <1476832384-10215-1-git-send-email-briannorris@chromium.org>
On Tue, Oct 18, 2016 at 04:13:04PM -0700, Brian Norris wrote:
> We're trying to mask out bits[23:8] while retaining [32:24, 7:0], but
> we're doing the inverse. That doesn't have too much effect, since we're
> setting all the [23:8] bits to 1, and the other bits are only relevant
> for modes we're currently not using. But we should get this right.
>
> Fixes: ca1989084054 ("PCI: rockchip: Fix wrong transmitted FTS count")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
Applied with Shawn's ack to pci/host-rockchip, thanks!
> ---
> drivers/pci/host/pcie-rockchip.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index e0b22dab9b7a..5c2e3297a3ff 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -492,7 +492,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>
> /* Fix the transmitted FTS count desired to exit from L0s. */
> status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL_PLC1);
> - status = (status & PCIE_CORE_CTRL_PLC1_FTS_MASK) |
> + status = (status & ~PCIE_CORE_CTRL_PLC1_FTS_MASK) |
> (PCIE_CORE_CTRL_PLC1_FTS_CNT << PCIE_CORE_CTRL_PLC1_FTS_SHIFT);
> rockchip_pcie_write(rockchip, status, PCIE_CORE_CTRL_PLC1);
>
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: [PATCH v3] PCI: create revision file in sysfs
From: Daniel Vetter @ 2016-11-14 18:40 UTC (permalink / raw)
To: Emil Velikov; +Cc: dri-devel, Bjorn Helgaas, linux-pci, Greg KH
In-Reply-To: <20161111143723.5818-1-emil.l.velikov@gmail.com>
On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
>
> Currently the revision isn't available via sysfs/libudev thus if one
> wants to know the value they need to read through the config file.
>
> This in itself wakes/powers up the device, causing unwanted delay
> since it can be quite costly.
>
> There are at least two userspace components which could make use the new
> file libpciaccess and libdrm. The former [used in various places] wakes
> up _every_ PCI device, which can be observed via glxinfo [when using
> Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0]
> can lead to 2-3 second delays while starting firefox, thunderbird or
> chromium.
>
> Expose the revision as a separate file, just like we do for the device,
> vendor, their subsystem version and class.
>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Tested-by: Mauro Santos <registo.mailling@gmail.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Given that waking a gpu can take somewhere between ages and forever, and
that we read the pci revisions everytime we launch a new gl app I think
this is the correct approach. Of course we could just patch libdrm and
everyone to not look at the pci revision, but that just leads to every
pci-based driver having a driver-private ioctl/getparam thing to expose
it. Which also doesn't make much sense.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Björn, if you're all ok with this we'd like to start landing at least
libdrm patches before this patch hits a released kernel, just to shorten
the pain window for users waiting for upgrades.
Thanks, Daniel
> ---
> v2: Add r-b/t-b tags, slim down CC list, add note about userspace.
>
> v3: Add Documentation/ bits (Greg KH)
>
> Gents, please keep me in the CC list.
>
> Thanks
> Emil
> ---
> Documentation/ABI/testing/sysfs-bus-pci | 7 +++++++
> Documentation/filesystems/sysfs-pci.txt | 2 ++
> drivers/pci/pci-sysfs.c | 2 ++
> 3 files changed, 11 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index b3bc50f..5a1732b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -294,3 +294,10 @@ Description:
> a firmware bug to the system vendor. Writing to this file
> taints the kernel with TAINT_FIRMWARE_WORKAROUND, which
> reduces the supportability of your system.
> +
> +What: /sys/bus/pci/devices/.../revision
> +Date: November 2016
> +Contact: Emil Velikov <emil.l.velikov@gmail.com>
> +Description:
> + This file contains the revision field of the the PCI device.
> + The value comes from device config space. The file is read only.
> diff --git a/Documentation/filesystems/sysfs-pci.txt b/Documentation/filesystems/sysfs-pci.txt
> index 74eaac2..6ea1ced 100644
> --- a/Documentation/filesystems/sysfs-pci.txt
> +++ b/Documentation/filesystems/sysfs-pci.txt
> @@ -17,6 +17,7 @@ that support it. For example, a given bus might look like this:
> | |-- resource0
> | |-- resource1
> | |-- resource2
> + | |-- revision
> | |-- rom
> | |-- subsystem_device
> | |-- subsystem_vendor
> @@ -41,6 +42,7 @@ files, each with their own function.
> resource PCI resource host addresses (ascii, ro)
> resource0..N PCI resource N, if present (binary, mmap, rw[1])
> resource0_wc..N_wc PCI WC map resource N, if prefetchable (binary, mmap)
> + revision PCI revision (ascii, ro)
> rom PCI ROM resource, if present (binary, ro)
> subsystem_device PCI subsystem device (ascii, ro)
> subsystem_vendor PCI subsystem vendor (ascii, ro)
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index bcd10c7..0666287 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n");
> pci_config_attr(device, "0x%04x\n");
> pci_config_attr(subsystem_vendor, "0x%04x\n");
> pci_config_attr(subsystem_device, "0x%04x\n");
> +pci_config_attr(revision, "0x%02x\n");
> pci_config_attr(class, "0x%06x\n");
> pci_config_attr(irq, "%u\n");
>
> @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = {
> &dev_attr_device.attr,
> &dev_attr_subsystem_vendor.attr,
> &dev_attr_subsystem_device.attr,
> + &dev_attr_revision.attr,
> &dev_attr_class.attr,
> &dev_attr_irq.attr,
> &dev_attr_local_cpus.attr,
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic
From: Robin Murphy @ 2016-11-14 18:25 UTC (permalink / raw)
To: Joerg Roedel
Cc: Lorenzo Pieralisi, iommu, Will Deacon, Hanjun Guo, Marc Zyngier,
Rafael J. Wysocki, Tomasz Nowicki, Jon Masters, Eric Auger,
Sinan Kaya, Nate Watterson, Prem Mallappa, Dennis Chen,
linux-acpi, linux-pci, linux-kernel, linux-arm-kernel
In-Reply-To: <20161114155222.GZ2078@8bytes.org>
On 14/11/16 15:52, Joerg Roedel wrote:
> On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
>> If we've already made the decision to move away from bus ops, I don't
>> see that it makes sense to deliberately introduce new dependencies on
>> them. Besides, as it stands, this patch literally implements "tell the
>> iommu-core which hardware-iommus exist in the system and a seperate
>> iommu_ops ptr for each of them" straight off.
>
> Not sure which code you are looking at, but as I see it we have only
> per-device iommu-ops now (with this patch). That is different from
> having core-visible hardware-iommu instances where devices could link
> to.
The per-device IOMMU ops are already there since 57f98d2f61e1. This
patch generalises the other end, moving the "registering an IOMMU
instance" (i.e. iommu_fwentry) bit into the IOMMU core, from being
OF-specific. I'd be perfectly happy if we rename iommu_fwentry to
iommu_instance, fwnode_iommu_set_ops() to iommu_register_instance(), and
such if that makes the design intent clearer.
If you'd also prefer to replace iommu_fwspec::ops with an opaque
iommu_fwspec::iommu_instance pointer so that things are a bit more
centralised (and users are forced to go through the API rather then call
ops directly), I'd have no major objection either. My main point is that
we've been deliberately putting the relevant building blocks in place -
the of_iommu_{get,set}_ops stuff was designed from the start to
accommodate per-instance ops, via the ops pointer *being* the instance
token; the iommu_fwspec stuff is deliberately intended to provide
per-device ops on top of that. The raw functionality is either there in
iommu.c already, or moving there in patches already written, so if it
doesn't look right all we need to focus on is making it look right.
> Also the rest of iommu-core code still makes use of the per-bus ops. The
> per-device ops are only used for the of_xlate fn-ptr.
Hence my aforementioned patches intended for 4.10, directly following on
from introducing iommu_fwspec in 4.9:
http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html
...the purpose being to provide a smooth transition from per-bus ops to
per-device, per-instance ops. Apply those and we're 90% of the way there
for OF-based IOMMU drivers (not that any of those actually need
per-instance ops, admittedly; I did prototype it for the ARM SMMU ages
ago, but it didn't seem worth the bother). Lorenzo's series broadens the
scope to ACPI-based systems and moves the generically-useful parts into
the core where we can easily build on them further if necessary. The
major remaining work is to convert external callers of the current
bus-dependent functions like iommu_domain_alloc(), iommu_present(), etc.
to device-based alternatives.
Robin.
^ permalink raw reply
* Re: [PATCH v2] PCI: create revision file in sysfs
From: Bjorn Helgaas @ 2016-11-14 17:20 UTC (permalink / raw)
To: Emil Velikov; +Cc: Greg KH, ML dri-devel, Bjorn Helgaas, Linux PCI
In-Reply-To: <CACvgo50V+wcibUHkpdsFJa_q+K2=k1T2UzsJ2gmdc3NGo+7qUQ@mail.gmail.com>
On Fri, Nov 11, 2016 at 06:56:51PM +0000, Emil Velikov wrote:
> Hi Bjorn,
>
> On 11 November 2016 at 14:49, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Nov 11, 2016 at 12:31:47AM +0000, Emil Velikov wrote:
> >> On 10 November 2016 at 23:59, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> > Hi Emil,
> >> >
> >> > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote:
> >> >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >> > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote:
> >> >> >> From: Emil Velikov <emil.velikov@collabora.com>
> >> >> >>
> >> >> >> Currently the revision isn't available via sysfs/libudev thus if one
> >> >> >> wants to know the value they need to read through the config file.
> >> >> >>
> >> >> >> This in itself wakes/powers up the device, causing unwanted delays.
> >> >> >>
> >> >> >> There are at least two userspace components which could make use the new
> >> >> >> file - libpciaccess and libdrm. At the moment the former will wake up
> >> >> >> _every_ PCI device for simple invocation of glxinfo [when using Mesa
> >> >> >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can
> >> >> >> lead to 2-3 second delays while starting firefox, thunderbird or
> >> >> >> chromium.
> >> >
> >> > I agree, these unwanted delays are completely unacceptable. My
> >> > question is whether we should fix them by exporting more information
> >> > from the kernel, or by changing the way the userspace components work.
> >> >
> >> > It should not take anywhere near 2 seconds to wake up a PCI device.
> >> > That makes me think there's a more serious problem than just a lack of
> >> > caching for the revision field, e.g., maybe we're looking at far more
> >> > PCI devices than we need to, or we're doing it many times to the same
> >> > device, or ...
> >> >
> >> > If I understand correctly, the delay was bisected to
> >> > https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which
> >> > removed a bunch of code that looked up the vendor and device IDs, and
> >> > replaced it with drmGetDevice(). And apparently drmGetDevice(), in
> >> > this path:
> >> >
> >> > drmGetDevice
> >> > drmProcessPciDevice
> >> > drmParsePciDeviceInfo
> >> >
> >> > is a little more thorough in that it looks up the *revision* in
> >> > addition to the vendor and device IDs. So we pay the cost for the
> >> > revision even though in this instance we don't care about the revision
> >> > at all.
> >> >
> >> Above all, apologies for all the "lovely" code that you had to go
> >> through for these.
> >> And yes, you've got it spot on.
> >>
> >> > drmParsePciDeviceInfo() currently reads the whole config header from
> >> > sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949),
> >> > but I think you're extending that to try the vendor, device,
> >> > subsystem_vendor, subsystem_device, and (if present) revision sysfs
> >> > files first (http://www.spinics.net/lists/dri-devel/msg122319.html).
> >> >
> >> Yes, making the revision file optional and "faking it" was my first
> >> thought, esp. since we don't have any users of it (yet).
> >> Although people are not too keen on it, so we'll likely opt for
> >> revision-less API.
> >>
> >> > Bottom line, I guess I'm not super opposed to this, but I do feel like
> >> > we're making a kernel change to cover up a userspace problem, and I
> >> > think it would be better to push on that userspace problem a little
> >> > more.
> >> >
> >> Yes, definitely we can beat some sense into userspace. Yet that
> >> shouldn't be a deterrent for exposing the revision.
> >
> > Maybe. If we speed things up by extending this kernel ABI, there's
> > much less incentive to optimize the userspace stuff. I feel a little
> > bit like an enabler for undesirable userspace behavior :)
> >
> Yes, fixing userspace to not do silly things is the goal. But at the
> same time even if userspace is perfect, there is no reason to power on
> the device just to get the revision field, is it ?
> Especially since everything else is readily available.
>
> >> As hinted before the other prominent user libpciaccess wakes up probes
> >> _every_ pci device.
> >
> > Is it really necessary to probe *every* PCI device? That doesn't
> > sound like a scalable design.
> >
> > As you can tell, the argument that "we should add this kernel ABI to
> > make suboptimal userspace algorithms go faster" doesn't feel very
> > compelling to me.
> >
> "Don't shoot the messenger" comes to mind. I'm just the stupid^Wnice
> person who's trying to untangle unfortunate design decisions - don't
> force me to rewrite more than a dozen pieces of software, please ?
> Even then, I wonder how long it'll take for those to hit end users.
Pre-be239326aa4f, you had:
int libudev_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
int sysfs_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
int drm_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
None of them returned the revision. There was some duplicated code,
but it was apparently functional and fast.
be239326aa4f removed libudev_get_pci_id_for_fd() and
sysfs_get_pci_id_for_fd(), which made the code prettier. It also
changed drm_get_pci_id_for_fd() to use drmGetDevice() instead of the
awful hard-coding of vendor/device IDs based on drmGetVersion()->name.
But drmGetDevice() also returns the revision, which we don't need.
If you applied http://www.spinics.net/lists/dri-devel/msg122319.html,
you'd have code that's fast but unreliable (as you pointed out, it
returns the revision on new kernels, but 0 on old kernels, with no
hint to the caller about whether the revision is accurate).
If the caller can say "I don't care about the revision", e.g.,
http://www.spinics.net/lists/dri-devel/msg123013.html, you can make
drm_get_pci_id_for_fd() fast again. But it will be fast and
functional even if the kernel doesn't export a "revision" sysfs file.
So what's the benefit of adding it? This seems like a long circular
chain of making things simpler in one area but having to add new
complications in another to compensate.
Bjorn
^ permalink raw reply
* Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic
From: Lorenzo Pieralisi @ 2016-11-14 16:42 UTC (permalink / raw)
To: Joerg Roedel
Cc: Robin Murphy, iommu, Will Deacon, Hanjun Guo, Marc Zyngier,
Rafael J. Wysocki, Tomasz Nowicki, Jon Masters, Eric Auger,
Sinan Kaya, Nate Watterson, Prem Mallappa, Dennis Chen,
linux-acpi, linux-pci, linux-kernel, linux-arm-kernel
In-Reply-To: <20161114155222.GZ2078@8bytes.org>
On Mon, Nov 14, 2016 at 04:52:23PM +0100, Joerg Roedel wrote:
> On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
> > If we've already made the decision to move away from bus ops, I don't
> > see that it makes sense to deliberately introduce new dependencies on
> > them. Besides, as it stands, this patch literally implements "tell the
> > iommu-core which hardware-iommus exist in the system and a seperate
> > iommu_ops ptr for each of them" straight off.
>
> Not sure which code you are looking at, but as I see it we have only
> per-device iommu-ops now (with this patch). That is different from
> having core-visible hardware-iommu instances where devices could link
> to.
This patch enables the IOMMU-OF-node<->device look-up on non-OF (ie
ACPI) systems by "converting" the of_node to a generic fwnode_handle,
that's all it does (and move the related look-up code from
drivers/iommu/of_iommu.c to drivers/iommu/iommu.c so that it does
not depend on OF_IOMMU any longer).
> Also the rest of iommu-core code still makes use of the per-bus ops. The
> per-device ops are only used for the of_xlate fn-ptr.
I can put this patch on the backburner and retrieve the iommu_ops
through the dev->bus path in the IORT xlate function (iort_iommu_xlate()
introduced in the last patch), the change is trivial and should work
just fine but it deserves a v8 to give everyone a chance to test it.
We would end-up handling the device->iommu_ops look-up differently in DT
and ACPI for streamid translations though, I am not sure I see a reason
why.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH 2/2] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't
From: Don Dutile @ 2016-11-14 16:16 UTC (permalink / raw)
To: Johannes Thumshirn, Bjorn Helgaas
Cc: Bjorn Helgaas, linux-pci, linux-kernel, Alexander Graf,
Hannes Reinecke
In-Reply-To: <20161114115604.gzxjstjj7vb4ytno@linux-x5ow.site>
On 11/14/2016 06:56 AM, Johannes Thumshirn wrote:
> On Wed, Nov 09, 2016 at 11:11:40AM -0600, Bjorn Helgaas wrote:
>> Hi Johannes,
>>
>> On Wed, Nov 02, 2016 at 04:35:52PM -0600, Johannes Thumshirn wrote:
>>> The Read Completion Boundary (RCB) bit must only be set on a device or
>>> endpoint if it is set on the root complex.
>>>
>>> Certain BIOSes erroneously set the RCB Bit in their ACPI _HPX Tables
>>> even if it is not set on the root port. This is a violation to the PCIe
>>> Specification and is known to bring some Mellanox Connect-X 3 HCAs into
>>> a state where they can't map their firmware and go into error recovery.
>>>
>>> BIOS Information
>>> Vendor: IBM
>>> Version: -[A8E120CUS-1.30]-
>>> Release Date: 08/22/2016
>>
>> This seems like a pretty serious problem (sounds like maybe the HCA is
>> completely useless?)
>
> Correct.
>
>>
>> Can you point us at a bugzilla or other problem report? It's nice to
>> have details of what this looks like to a user, so people who trip
>> over this problem have a little more chance of finding the solution.
>
> As we already said, our bugzilla entry for this is not accessible from the
> outside, but I know Red Hat does have a bugzilla entry for the same issue as
> well. Maybe this is reachable from the outside (adding Don for this, as I know
> he has worked on this problem as well).
>
RHEL bz's are not accessible from the outside.
I suggest capturing the content of the RH bz issue and creating a k.o. bz
with the information.
>>
>> 7a1562d4f2d0 ("PCI: Apply _HPX Link Control settings to all devices
>> with a link") appeared in v3.18, so it's probably not a *new* problem,
>> so my guess is that this is v4.10 material.
>
> Yes 4.10 sounds good to me. I personally think, this problem hasn't
> materialized yet, as this is the kind of hardware you run on a rather /stable/
> kernel either you built on your own or get from an enterprise distribution and
> until recently these kernels haven't been updated to something newer than
> 3.18.
>
> Thanks,
> Johannes
>
^ permalink raw reply
* Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic
From: Joerg Roedel @ 2016-11-14 15:52 UTC (permalink / raw)
To: Robin Murphy
Cc: Lorenzo Pieralisi, iommu, Will Deacon, Hanjun Guo, Marc Zyngier,
Rafael J. Wysocki, Tomasz Nowicki, Jon Masters, Eric Auger,
Sinan Kaya, Nate Watterson, Prem Mallappa, Dennis Chen,
linux-acpi, linux-pci, linux-kernel, linux-arm-kernel
In-Reply-To: <41e3eff1-9ce6-bcfb-5716-c65ef38add63@arm.com>
On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
> If we've already made the decision to move away from bus ops, I don't
> see that it makes sense to deliberately introduce new dependencies on
> them. Besides, as it stands, this patch literally implements "tell the
> iommu-core which hardware-iommus exist in the system and a seperate
> iommu_ops ptr for each of them" straight off.
Not sure which code you are looking at, but as I see it we have only
per-device iommu-ops now (with this patch). That is different from
having core-visible hardware-iommu instances where devices could link
to.
Also the rest of iommu-core code still makes use of the per-bus ops. The
per-device ops are only used for the of_xlate fn-ptr.
Joerg
^ permalink raw reply
* Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller
From: Vivek Gautam @ 2016-11-14 14:04 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: svarbanov, linux-pci, Bjorn Helgaas, robh+dt, linux-arm-msm,
devicetree@vger.kernel.org
In-Reply-To: <1479122155-13393-3-git-send-email-srinivas.kandagatla@linaro.org>
On Mon, Nov 14, 2016 at 4:45 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks are only setup after the phy is powered on.
> It also adds ltssm_enable callback as it is very much different to other
> supported SOCs in the driver.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Thanks
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH 4/5] iommu: Move REQ_ACS_FLAGS out to header and rename
From: Joerg Roedel @ 2016-11-14 12:48 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Alex Williamson, linux-pci, bhelgaas, iommu, linux-kernel
In-Reply-To: <20161111225740.GX9868@bhelgaas-glaptop.roam.corp.google.com>
On Fri, Nov 11, 2016 at 04:57:40PM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 10, 2016 at 01:27:13PM +0100, Joerg Roedel wrote:
> > On Wed, Oct 26, 2016 at 12:01:34PM -0600, Alex Williamson wrote:
> > > Allow other parts of the kernel to see which PCI ACS flags the IOMMU
> > > layer considers necessary for isolation.
> > >
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > Cc: Joerg Roedel <joro@8bytes.org>
> > > ---
> > > drivers/iommu/iommu.c | 18 +++++-------------
> > > include/linux/iommu.h | 11 +++++++++++
> > > 2 files changed, 16 insertions(+), 13 deletions(-)
> >
> > Applied, thanks Alex.
>
> Hi Joerg, did you apply just this one (4/5), or the whole series? If the
> former, is it safe for me to apply 1, 2, 3, and 5? I assumed these were
> meant to go as a group.
Ah yes, I missed that this patch was part of a series and applied it. I
can still remove it, as the tree is not yet pushed. Will you take the
while series? Then you can add my
Acked-by: Joerg Roedel <jroedel@suse.de>
to this patch.
Thanks,
Joerg
^ permalink raw reply
* Re: [PATCH] PCI: pciehp: Check for PCIe capabilities change after resume
From: Lukas Wunner @ 2016-11-14 12:10 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Rajat Jain, Yinghai Lu, Len Brown, Chen Yu
In-Reply-To: <20161111182831.GA9868@bhelgaas-glaptop.roam.corp.google.com>
On Fri, Nov 11, 2016 at 12:28:31PM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 11, 2016 at 04:49:19PM +0100, Lukas Wunner wrote:
> I'd love to be proven wrong, but I don't believe it's a silicon bug.
> All we have is Yinghai's vague assertion with no erratum and no
> details to back it up.
>
> As I read it, the response Len got from the validation team (comment
> #22) does not confirm a silicon bug. It merely restates the fact that
> the PCIe spec requires that Presence Detect State be hardwired to 1b
> if Slot Implemented is 0b (PCIe spec r3.0, sec 7.8.11). It also
> quotes this language from an Intel spec:
>
> Slot Implemented (SI) - R/WO. Indicates whether the root port is
> connected to a slot. Slot support is platform specific. BIOS
> programs this field, and it is maintained until a platform reset.
>
> I found this in the "Intel 8 Series/C220 Series Chipset Family
> Platform Controller Hub (PCH) Datasheet", May 2014, sec 19.1.24.
> Technically this spec actually covers the Dell [8086:9c10] device, not
> the MacBook Pro [8086:8c10] device, but the Intel validation folks say
> it applies to the Dell as well.
>
> That suggests to me that it's a Dell BIOS bug: BIOS should have
> programmed Slot Implemented the same way for initial boot and for
> resume, but it did not.
Hm, sounds plausible.
> We could do a quirk for [8086:9c10] as long as it was qualified by
> some sort of DMI check. I don't think we could turn off hotplug for
> all [8086:9c10] root ports. The data I see says the hardware is
> working per spec, and it's consistent with the PCIe spec.
>
> I do like the idea of a quirk much more than mucking around in pciehp.
> However, I think we still should account for the PCI_EXP_FLAGS_SLOT
> change somehow. If we do nothing, the accessors will still assume the
> slot registers exist after resume, but the hardware will return
> different results when we read them (PCIe sec 7.8 says that except for
> Presence Detect State, the slot registers should be hardwired to zero
> if Slot Implemented is zero).
>
> Slot Implemented is defined as "R/WO". The Intel spec (sec 9) says it
> becomes read-only after the first write. If the BIOS didn't write it,
> I wonder if an OS quirk that runs after resume could still write it,
> or if there's some other locking mechanism involved. If an OS quirk
> could set Slot Implemented, the way it was at initial boot, everything
> should just work. Presence Detect State (sec 19.1.33) should then be
> 0b, indicating the slot is empty, so pciehp wouldn't try to bring up
> the link.
Len could try "setpci -s 00:1c.0 42.w=142" after resume to set the
Slot Implemented bit.
Then use "setpci -s 00:1c.0 42.w" to test if the bit was written.
If this works, it could go into a DECLARE_PCI_FIXUP_RESUME_EARLY() quirk.
If this doesn't work, the DECLARE_PCI_FIXUP_HEADER() would have to clear
not just the is_hotplug_bridge bit (to prevent pciehp from binding) but
also the Slot Implemented bit in the cached pcie_flags_reg word.
Thanks,
Lukas
^ permalink raw reply
* Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic
From: Robin Murphy @ 2016-11-14 12:00 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Joerg Roedel, iommu, Will Deacon, Hanjun Guo, Marc Zyngier,
Rafael J. Wysocki, Tomasz Nowicki, Jon Masters, Eric Auger,
Sinan Kaya, Nate Watterson, Prem Mallappa, Dennis Chen,
linux-acpi, linux-pci, linux-kernel, linux-arm-kernel
In-Reply-To: <20161114102654.GA1677@red-moon>
On 14/11/16 10:26, Lorenzo Pieralisi wrote:
> Hi Robin, Joerg,
>
> On Fri, Nov 11, 2016 at 05:43:39PM +0000, Robin Murphy wrote:
>> On 11/11/16 16:27, Joerg Roedel wrote:
>>> On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote:
>>>> In the original of_iommu_configure design, the thought was that an ops
>>>> structure could be IOMMU-instance-specific (hence the later-removed
>>>> "priv" member), so I suppose right now it is mostly a hangover from
>>>> that. However, it's also what we initialise a device's fwspec with, so
>>>> becomes important again if we're ever going to get past the limitations
>>>> of buses-which-are-not-actually-buses[1].
>>>
>>> Yeah, I discussed this with a few others at LPC. My current idea is to
>>> tell the iommu-core which hardware-iommus exist in the system and a
>>> seperate iommu_ops ptr for each of them. Then every struct device can
>>> link to the iommu-instance it is translated by.
>>
>> Er, that sounds very much like a description of what we already have in
>> 4.9-rc. Every struct device now has an iommu_fwspec which encapsulates
>> both an iommu_ops pointer (which can perfectly well be per-instance if
>> the IOMMU driver wants) and a place for the IOMMU-private data to
>> replace the mess of archdata.iommu and driver-internal globals.
>>
>>> We are not there yet, but this will give you the same per-device
>>> iommu-ops as implemented here.
>>
>> With those two patches I linked to, which make the bulk of the IOMMU
>> core code per-device-ops-aware off the bat, I'd say we *are* already
>> pretty much there. It's only iommu_domain_alloc() which needs a
>> device-based alternative, and the non-of_xlate-based IOMMU drivers to
>> either call iommu_fwspec_init() for themselves, or perhaps for x86
>> plumbing in DMAR/IVRS equivalents of the IORT parsing to the
>> infrastructure provided by this series.
>
> I think it all boils down to how we end up implementing the per-device
> iommu_ops look-up/binding, question is what do you want me to do with
> this patch, it should be fine to drop it and use dev->bus->iommu_ops
> for the look-up but I should know sooner rather than later to make
> sure the series get another good round of testing.
If we've already made the decision to move away from bus ops, I don't
see that it makes sense to deliberately introduce new dependencies on
them. Besides, as it stands, this patch literally implements "tell the
iommu-core which hardware-iommus exist in the system and a seperate
iommu_ops ptr for each of them" straight off.
Robin.
>
> Please let me know, thank you very much.
>
> Lorenzo
>
^ permalink raw reply
* Re: [PATCH 2/2] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't
From: Johannes Thumshirn @ 2016-11-14 11:56 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, linux-pci, linux-kernel, Alexander Graf,
Hannes Reinecke, Don Dutile
In-Reply-To: <20161109171140.GK14322@bhelgaas-glaptop.roam.corp.google.com>
On Wed, Nov 09, 2016 at 11:11:40AM -0600, Bjorn Helgaas wrote:
> Hi Johannes,
>
> On Wed, Nov 02, 2016 at 04:35:52PM -0600, Johannes Thumshirn wrote:
> > The Read Completion Boundary (RCB) bit must only be set on a device or
> > endpoint if it is set on the root complex.
> >
> > Certain BIOSes erroneously set the RCB Bit in their ACPI _HPX Tables
> > even if it is not set on the root port. This is a violation to the PCIe
> > Specification and is known to bring some Mellanox Connect-X 3 HCAs into
> > a state where they can't map their firmware and go into error recovery.
> >
> > BIOS Information
> > Vendor: IBM
> > Version: -[A8E120CUS-1.30]-
> > Release Date: 08/22/2016
>
> This seems like a pretty serious problem (sounds like maybe the HCA is
> completely useless?)
Correct.
>
> Can you point us at a bugzilla or other problem report? It's nice to
> have details of what this looks like to a user, so people who trip
> over this problem have a little more chance of finding the solution.
As we already said, our bugzilla entry for this is not accessible from the
outside, but I know Red Hat does have a bugzilla entry for the same issue as
well. Maybe this is reachable from the outside (adding Don for this, as I know
he has worked on this problem as well).
>
> 7a1562d4f2d0 ("PCI: Apply _HPX Link Control settings to all devices
> with a link") appeared in v3.18, so it's probably not a *new* problem,
> so my guess is that this is v4.10 material.
Yes 4.10 sounds good to me. I personally think, this problem hasn't
materialized yet, as this is the kind of hardware you run on a rather /stable/
kernel either you built on your own or get from an enterprise distribution and
until recently these kernels haven't been updated to something newer than
3.18.
Thanks,
Johannes
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06
From: liviu.dudau @ 2016-11-14 11:26 UTC (permalink / raw)
To: Gabriele Paoloni
Cc: Arnd Bergmann, linux-arm-kernel@lists.infradead.org, Yuanzhichang,
mark.rutland@arm.com, devicetree@vger.kernel.org,
lorenzo.pieralisi@arm.com, minyard@acm.org,
linux-pci@vger.kernel.org, benh@kernel.crashing.org, John Garry,
will.deacon@arm.com, linux-kernel@vger.kernel.org, xuwei (O),
Linuxarm, zourongrong@gmail.com, robh+dt@kernel.org,
kantyzc@163.com, linux-serial@vger.kernel.org,
catalin.marinas@arm.com, olof@lixom.net, bhelgaas@googl e.com,
zhichang.yuan02@gmail.com
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F90EA45@lhreml507-mbx>
On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote:
> Hi Liviu
>
[snip]
> > > >
> > > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and
> > you
> > > > actually need another variable for "reserving" an area in the I/O
> > space
> > > > that can be used for physical addresses rather than I/O tokens.
> > > >
> > > > The one good example for using PCIBIOS_MIN_IO is when your
> > > > platform/architecture
> > > > does not support legacy ISA operations *at all*. In that case
> > someone
> > > > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O
> > range
> > > > so that it doesn't get used. With Zhichang's patch you now start
> > > > forcing
> > > > those platforms to have a valid address below PCIBIOS_MIN_IO.
> > >
> > > But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be
> > used
> > > by PCI controllers only...
> >
> > Nope, that is not what it means. It means that PCI devices can see I/O
> > addresses
> > on the bus that start from 0. There never was any usage for non-PCI
> > controllers
>
> So I am a bit confused...
> From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> It seems that ISA buses operate on cpu I/O address range [0, 0xFFF].
> I thought that was the reason why for most architectures we have
> PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers
> usually use [0, PCIBIOS_MIN_IO - 1] )
First of all, cpu I/O addresses is an x86-ism. ARM architectures and others
have no separate address space for I/O, it is all merged into one unified
address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 could mean
that we don't care about ISA I/O because the platform does not support having
an ISA bus (e.g.).
>
> For those architectures whose PCIBIOS_MIN_IO != 0x1000 probably
> they are not fully compliant or they cannot fully support an ISA
> controller...?
Exactly. Not fully compliant is a bit strong, as ISA is a legacy feature and
when it comes to PCI-e you are allowed to ignore it. Having PCIBIOS_MIN_IO != 0x1000
is a way to signal that you don't fully support ISA.
>
> As said before this series forbid IO tokens to be in [0, PCIBIOS_MIN_IO)
> to allow special ISA controllers to use that range with special
> accessors.
> Having a variable threshold would make life much more difficult
> as there would be a probe dependency between the PCI controller and
> the special ISA one (PCI to wait for the special ISA device to be
> probed and set the right threshold value from DT or ACPI table).
>
> Instead using PCIBIOS_MIN_IO is easier and should not impose much
> constraint as [PCIBIOS_MIN_IO, IO_SPACE_LIMIT] is available to
> the PCI controller for I/O tokens...
What I am suggesting is to leave PCIBIOS_MIN_IO alone which still reserves
space for ISA controller and add a PCIBIOS_MIN_DIRECT_IO that will reserve
space for your direct address I/O on top of PCIBIOS_MIN_IO.
Best regards,
Liviu
>
> Thanks
>
> Gab
>
> > when PCIBIOS_MIN_IO != 0. That is what Zhichang is trying to do now and
> > what
> > I think is not the right thing (and not enough anyway).
> >
> > > so if you have a special bus device using
> > > an I/O range in this case should be a PCI controller...
> >
> > That has always been the case. It is this series that wants to
> > introduce the
> > new meaning.
> >
> > > i.e. I would
> > > expect it to fall back into the case of I/O tokens redirection rather
> > than
> > > physical addresses redirection (as mentioned below from my previous
> > reply).
> > > What do you think?
> >
> > I think you have looked too much at the code *with* Zhichang's patches
> > applied.
> > Take a step back and look at how PCIBIOS_MIN_IO is used now, before you
> > apply
> > the patches. It is all about PCI addresses and there is no notion of
> > non-PCI
> > busses using PCI framework. Only platforms and architectures that try
> > to work
> > around some legacy standards (ISA) or HW restrictions.
> >
> > Best regards,
> > Liviu
> >
> > >
> > > Thanks
> > >
> > > Gab
> > >
> > >
> > > >
> > > > For the general case you also have to bear in mind that
> > PCIBIOS_MIN_IO
> > > > could
> > > > be zero. In that case, what is your "forbidden" range? [0, 0) ? So
> > it
> > > > makes
> > > > sense to add a new #define that should only be defined by those
> > > > architectures/
> > > > platforms that want to reserve on top of PCIBIOS_MIN_IO another
> > region
> > > > where I/O tokens can't be generated for.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > > >
> > > > > > > > Your current version has
> > > > > > > >
> > > > > > > > if (arm64_extio_ops->pfout)
> > > > \
> > > > > > > > arm64_extio_ops->pfout(arm64_extio_ops-
> > > > >devpara,\
> > > > > > > > addr, value, sizeof(type));
> > > > \
> > > > > > > >
> > > > > > > > Instead, just subtract the start of the range from the
> > logical
> > > > > > > > port number to transform it back into a bus-local port
> > number:
> > > > > > >
> > > > > > > These accessors do not operate on IO tokens:
> > > > > > >
> > > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end <
> > addr)
> > > > > > > addr is not going to be an I/O token; in fact patch 2/3
> > imposes
> > > > that
> > > > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> > > > > > PCIBIOS_MIN_IO
> > > > > > > we have free physical addresses that the accessors can
> > operate
> > > > on.
> > > > > >
> > > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to
> > refer
> > > > to
> > > > > > the logical I/O tokens, the purpose of that macro is really
> > meant
> > > > > > for allocating PCI I/O port numbers within the address space of
> > > > > > one bus.
> > > > >
> > > > > As I mentioned above, special devices operate on CPU addresses
> > > > directly,
> > > > > not I/O tokens. For them there is no way to distinguish....
> > > > >
> > > > > >
> > > > > > Note that it's equally likely that whichever next platform
> > needs
> > > > > > non-mapped I/O access like this actually needs them for PCI I/O
> > > > space,
> > > > > > and that will use it on addresses registered to a PCI host
> > bridge.
> > > > >
> > > > > Ok so here you are talking about a platform that has got an I/O
> > range
> > > > > under the PCI host controller, right?
> > > > > And this I/O range cannot be directly memory mapped but needs
> > special
> > > > > redirections for the I/O tokens, right?
> > > > >
> > > > > In this scenario registering the I/O ranges with the forbidden
> > range
> > > > > implemented by the current patch would still allow to redirect
> > I/O
> > > > > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO
> > > > >
> > > > > So effectively the special PCI host controller
> > > > > 1) knows the physical range that needs special redirection
> > > > > 2) register such range
> > > > > 3) uses pci_pio_to_address() to retrieve the IO tokens for the
> > > > > special accessors
> > > > > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in
> > 3)
> > > > >
> > > > > So to be honest I think this patch can fit well both with
> > > > > special PCI controllers that need I/O tokens redirection and with
> > > > > special non-PCI controllers that need non-PCI I/O physical
> > > > > address redirection...
> > > > >
> > > > > Thanks (and sorry for the long reply but I didn't know how
> > > > > to make the explanation shorter :) )
> > > > >
> > > > > Gab
> > > > >
> > > > > >
> > > > > > If we separate the two steps:
> > > > > >
> > > > > > a) assign a range of logical I/O port numbers to a bus
> > > > > > b) register a set of helpers for redirecting logical I/O
> > > > > > port to a helper function
> > > > > >
> > > > > > then I think the code will get cleaner and more flexible.
> > > > > > It should actually then be able to replace the powerpc
> > > > > > specific implementation.
> > > > > >
> > > > > > Arnd
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply
* [PATCH v4 3/3] PCI: qcom: add runtime pm support to pcie_port
From: Srinivas Kandagatla @ 2016-11-14 11:15 UTC (permalink / raw)
To: svarbanov, linux-pci, bhelgaas
Cc: robh+dt, linux-arm-msm, srinivas.kandagatla, devicetree
In-Reply-To: <1479122155-13393-1-git-send-email-srinivas.kandagatla@linaro.org>
This patch is required when the pcie controller sits on a bus with
its own power domain and clocks which are controlled via a bus driver
like simple pm bus. As these bus driver have runtime pm enabled, it makes
sense to update the usage counter so that the runtime pm does not suspend
the clks or power domain associated with the bus driver.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/pci/host/pcie-qcom.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index 03ba6b1..c2ca848 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -587,6 +587,8 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
struct qcom_pcie *pcie = to_qcom_pcie(pp);
int ret;
+ pm_runtime_get_sync(pp->dev);
+
qcom_ep_reset_assert(pcie);
ret = pcie->ops->init(pcie);
@@ -617,6 +619,7 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
phy_power_off(pcie->phy);
err_deinit:
pcie->ops->deinit(pcie);
+ pm_runtime_put_sync(pp->dev);
}
static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
@@ -673,6 +676,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
if (!pcie)
return -ENOMEM;
+ pm_runtime_enable(dev);
pp = &pcie->pp;
pcie->ops = (struct qcom_pcie_ops *)of_device_get_match_data(dev);
--
2.10.1
^ permalink raw reply related
* [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller
From: Srinivas Kandagatla @ 2016-11-14 11:15 UTC (permalink / raw)
To: svarbanov, linux-pci, bhelgaas
Cc: robh+dt, linux-arm-msm, srinivas.kandagatla, devicetree
In-Reply-To: <1479122155-13393-1-git-send-email-srinivas.kandagatla@linaro.org>
This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
legacy interrupts and it conforms to PCI Express Base 2.1 specification.
This patch adds post_init callback to qcom_pcie_ops, as this is pcie
pipe clocks are only setup after the phy is powered on.
It also adds ltssm_enable callback as it is very much different to other
supported SOCs in the driver.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
.../devicetree/bindings/pci/qcom,pcie.txt | 67 +++++++-
drivers/pci/host/pcie-qcom.c | 177 ++++++++++++++++++++-
2 files changed, 238 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 4059a6f..141d8c3 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -7,6 +7,7 @@
- "qcom,pcie-ipq8064" for ipq8064
- "qcom,pcie-apq8064" for apq8064
- "qcom,pcie-apq8084" for apq8084
+ - "qcom,pcie-msm8996" for msm8996 or apq8096
- reg:
Usage: required
@@ -92,6 +93,17 @@
- "aux" Auxiliary (AUX) clock
- "bus_master" Master AXI clock
- "bus_slave" Slave AXI clock
+
+- clock-names:
+ Usage: required for msm8996/apq8096
+ Value type: <stringlist>
+ Definition: Should contain the following entries
+ - "pipe" Pipe Clock driving internal logic.
+ - "aux" Auxiliary (AUX) clock.
+ - "cfg" Configuration clk.
+ - "bus_master" Master AXI clock.
+ - "bus_slave" Slave AXI clock.
+
- resets:
Usage: required
Value type: <prop-encoded-array>
@@ -115,7 +127,7 @@
- "core" Core reset
- power-domains:
- Usage: required for apq8084
+ Usage: required for apq8084 and msm8996/apq8096
Value type: <prop-encoded-array>
Definition: A phandle and power domain specifier pair to the
power domain which is responsible for collapsing
@@ -231,3 +243,56 @@
pinctrl-0 = <&pcie0_pins_default>;
pinctrl-names = "default";
};
+
+* Example for apq8096:
+
+ pcie@608000{
+ compatible = "qcom,pcie-msm8996", "snps,dw-pcie";
+ power-domains = <&gcc PCIE1_GDSC>;
+ bus-range = <0x00 0xff>;
+ num-lanes = <1>;
+
+ reg = <0x00608000 0x2000>,
+ <0x0d000000 0xf1d>,
+ <0x0d000f20 0xa8>,
+ <0x0d100000 0x100000>;
+
+ reg-names = "parf", "dbi", "elbi", "config";
+
+ phys = <&pcie_phy 1>;
+ phy-names = "pciephy";
+
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges = <0x01000000 0x0 0x0d200000 0x0d200000 0x0 0x100000>,
+ <0x02000000 0x0 0x0d300000 0x0d300000 0x0 0xd00000>;
+
+ interrupts = <GIC_SPI 413 IRQ_TYPE_NONE>;
+ interrupt-names = "msi";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 &intc 0 272 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+ <0 0 0 2 &intc 0 273 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+ <0 0 0 3 &intc 0 274 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+ <0 0 0 4 &intc 0 275 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&pcie1_clkreq_default &pcie1_perst_default &pcie1_wake_default>;
+ pinctrl-1 = <&pcie1_clkreq_sleep &pcie1_perst_default &pcie1_wake_sleep>;
+
+ vdda-1p8-supply = <&pm8994_l12>;
+ vdda-supply = <&pm8994_l28>;
+ linux,pci-domain = <1>;
+
+ clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
+ <&gcc GCC_PCIE_1_AUX_CLK>,
+ <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
+ <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
+ <&gcc GCC_PCIE_1_SLV_AXI_CLK>;
+
+ clock-names = "pipe",
+ "aux",
+ "cfg",
+ "bus_master",
+ "bus_slave";
+ };
diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index 3593640..03ba6b1 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -36,11 +36,19 @@
#include "pcie-designware.h"
+#define PCIE20_PARF_DBI_BASE_ADDR 0x168
+
+#define PCIE20_PARF_SYS_CTRL 0x00
#define PCIE20_PARF_PHY_CTRL 0x40
#define PCIE20_PARF_PHY_REFCLK 0x4C
#define PCIE20_PARF_DBI_BASE_ADDR 0x168
#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16c
+#define PCIE20_PARF_MHI_CLOCK_RESET_CTRL 0x174
#define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT 0x178
+#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT 0x1A8
+#define PCIE20_PARF_LTSSM 0x1B0
+#define PCIE20_PARF_SID_OFFSET 0x234
+#define PCIE20_PARF_BDF_TRANSLATE_CFG 0x24C
#define PCIE20_ELBI_SYS_CTRL 0x04
#define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0)
@@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
struct regulator *vdda;
};
+struct qcom_pcie_resources_v2 {
+ struct clk *aux_clk;
+ struct clk *master_clk;
+ struct clk *slave_clk;
+ struct clk *cfg_clk;
+ struct clk *pipe_clk;
+};
+
union qcom_pcie_resources {
struct qcom_pcie_resources_v0 v0;
struct qcom_pcie_resources_v1 v1;
+ struct qcom_pcie_resources_v2 v2;
};
struct qcom_pcie;
@@ -82,7 +99,9 @@ struct qcom_pcie;
struct qcom_pcie_ops {
int (*get_resources)(struct qcom_pcie *pcie);
int (*init)(struct qcom_pcie *pcie);
+ int (*post_init)(struct qcom_pcie *pcie);
void (*deinit)(struct qcom_pcie *pcie);
+ void (*ltssm_enable)(struct qcom_pcie *pcie);
};
struct qcom_pcie {
@@ -116,17 +135,33 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
return dw_handle_msi_irq(pp);
}
-static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
+static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
{
u32 val;
-
- if (dw_pcie_link_up(&pcie->pp))
- return 0;
-
/* enable link training */
val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
val |= PCIE20_ELBI_SYS_CTRL_LT_ENABLE;
writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+}
+
+static void qcom_pcie_v2_ltssm_enable(struct qcom_pcie *pcie)
+{
+ u32 val;
+ /* enable link training */
+ val = readl(pcie->parf + PCIE20_PARF_LTSSM);
+ val |= BIT(8);
+ writel(val, pcie->parf + PCIE20_PARF_LTSSM);
+}
+
+static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
+{
+
+ if (dw_pcie_link_up(&pcie->pp))
+ return 0;
+
+ /* Enable Link Training state machine */
+ if (pcie->ops->ltssm_enable)
+ pcie->ops->ltssm_enable(pcie);
return dw_pcie_wait_for_link(&pcie->pp);
}
@@ -421,6 +456,113 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
return ret;
}
+static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+ struct device *dev = pcie->pp.dev;
+
+ res->aux_clk = devm_clk_get(dev, "aux");
+ if (IS_ERR(res->aux_clk))
+ return PTR_ERR(res->aux_clk);
+
+ res->cfg_clk = devm_clk_get(dev, "cfg");
+ if (IS_ERR(res->cfg_clk))
+ return PTR_ERR(res->cfg_clk);
+
+ res->master_clk = devm_clk_get(dev, "bus_master");
+ if (IS_ERR(res->master_clk))
+ return PTR_ERR(res->master_clk);
+
+ res->slave_clk = devm_clk_get(dev, "bus_slave");
+ if (IS_ERR(res->slave_clk))
+ return PTR_ERR(res->slave_clk);
+
+ res->pipe_clk = devm_clk_get(dev, "pipe");
+ if (IS_ERR(res->pipe_clk))
+ return PTR_ERR(res->pipe_clk);
+
+ return 0;
+}
+
+static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+ struct device *dev = pcie->pp.dev;
+ u32 val;
+ int ret;
+
+ ret = clk_prepare_enable(res->aux_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable aux clock\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(res->cfg_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable cfg clock\n");
+ goto err_cfg_clk;
+ }
+
+ ret = clk_prepare_enable(res->master_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable master clock\n");
+ goto err_master_clk;
+ }
+
+ ret = clk_prepare_enable(res->slave_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable slave clock\n");
+ goto err_slave_clk;
+ }
+
+ /* enable PCIe clocks and resets */
+ val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
+ val &= ~BIT(0);
+ writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
+ /* change DBI base address */
+ writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+ /* MAC PHY_POWERDOWN MUX DISABLE */
+ val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
+ val &= ~BIT(29);
+ writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
+
+ val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+ val |= BIT(4);
+ writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+
+ val = readl(pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+ val |= BIT(31);
+ writel(val, pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+
+ return 0;
+
+err_slave_clk:
+ clk_disable_unprepare(res->master_clk);
+err_master_clk:
+ clk_disable_unprepare(res->cfg_clk);
+err_cfg_clk:
+ clk_disable_unprepare(res->aux_clk);
+
+ return ret;
+}
+
+static int qcom_pcie_post_init_v2(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+ struct device *dev = pcie->pp.dev;
+ int ret;
+
+ ret = clk_prepare_enable(res->pipe_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable pipe clock\n");
+ return ret;
+ }
+
+ return 0;
+}
+
static int qcom_pcie_link_up(struct pcie_port *pp)
{
struct qcom_pcie *pcie = to_qcom_pcie(pp);
@@ -429,6 +571,17 @@ static int qcom_pcie_link_up(struct pcie_port *pp)
return !!(val & PCI_EXP_LNKSTA_DLLLA);
}
+static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+
+ clk_disable_unprepare(res->pipe_clk);
+ clk_disable_unprepare(res->slave_clk);
+ clk_disable_unprepare(res->master_clk);
+ clk_disable_unprepare(res->cfg_clk);
+ clk_disable_unprepare(res->aux_clk);
+}
+
static void qcom_pcie_host_init(struct pcie_port *pp)
{
struct qcom_pcie *pcie = to_qcom_pcie(pp);
@@ -444,6 +597,9 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
if (ret)
goto err_deinit;
+ if (pcie->ops->post_init)
+ pcie->ops->post_init(pcie);
+
dw_pcie_setup_rc(pp);
if (IS_ENABLED(CONFIG_PCI_MSI))
@@ -487,12 +643,22 @@ static const struct qcom_pcie_ops ops_v0 = {
.get_resources = qcom_pcie_get_resources_v0,
.init = qcom_pcie_init_v0,
.deinit = qcom_pcie_deinit_v0,
+ .ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
};
static const struct qcom_pcie_ops ops_v1 = {
.get_resources = qcom_pcie_get_resources_v1,
.init = qcom_pcie_init_v1,
.deinit = qcom_pcie_deinit_v1,
+ .ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
+};
+
+static const struct qcom_pcie_ops ops_v2 = {
+ .get_resources = qcom_pcie_get_resources_v2,
+ .init = qcom_pcie_init_v2,
+ .post_init = qcom_pcie_post_init_v2,
+ .deinit = qcom_pcie_deinit_v2,
+ .ltssm_enable = qcom_pcie_v2_ltssm_enable,
};
static int qcom_pcie_probe(struct platform_device *pdev)
@@ -572,6 +738,7 @@ static const struct of_device_id qcom_pcie_match[] = {
{ .compatible = "qcom,pcie-ipq8064", .data = &ops_v0 },
{ .compatible = "qcom,pcie-apq8064", .data = &ops_v0 },
{ .compatible = "qcom,pcie-apq8084", .data = &ops_v1 },
+ { .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },
{ }
};
--
2.10.1
^ permalink raw reply related
* [PATCH v4 1/3] bus: simple-pm: add support to pm clocks
From: Srinivas Kandagatla @ 2016-11-14 11:15 UTC (permalink / raw)
To: svarbanov, linux-pci, bhelgaas
Cc: robh+dt, linux-arm-msm, srinivas.kandagatla, devicetree
In-Reply-To: <1479122155-13393-1-git-send-email-srinivas.kandagatla@linaro.org>
This patch adds support to pm clocks via device tree, so that the clocks
can be turned on and off during runtime pm. This patch is required for
Qualcomm msm8996 pcie controller which sits on a bus with its own
power-domain and clocks.
Without this patch the clock associated with the bus are never turned on.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index c5eb46c..63b7e8c 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
#include <linux/pm_runtime.h>
@@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
- if (np)
+ if (np) {
+ of_pm_clk_add_clks(&pdev->dev);
of_platform_populate(np, NULL, NULL, &pdev->dev);
+ }
return 0;
}
+static const struct dev_pm_ops simple_pm_bus_pm_ops = {
+ SET_RUNTIME_PM_OPS(pm_clk_suspend,
+ pm_clk_resume, NULL)
+};
+
static int simple_pm_bus_remove(struct platform_device *pdev)
{
dev_dbg(&pdev->dev, "%s\n", __func__);
pm_runtime_disable(&pdev->dev);
+ pm_clk_destroy(&pdev->dev);
+
return 0;
}
@@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = {
.driver = {
.name = "simple-pm-bus",
.of_match_table = simple_pm_bus_of_match,
+ .pm = &simple_pm_bus_pm_ops,
},
};
--
2.10.1
^ permalink raw reply related
* [PATCH v4 0/3] PCI: qcom: Add support to msm8996 pcie controller.
From: Srinivas Kandagatla @ 2016-11-14 11:15 UTC (permalink / raw)
To: svarbanov, linux-pci, bhelgaas
Cc: robh+dt, linux-arm-msm, srinivas.kandagatla, devicetree
This patchset adds support to msm8996 pcie controller. I tested this patch on
v4.9-rc2 along with phy driver patch [1] and
"PCI: designware: check for iATU unroll support after initializing host"
fix [2] on DB820c APQ8096 board on port B and port C using sata and
ethernet controller.
Changes since v3:
- remove unnesessary variable initialization spotted by vivek.
- moved pipe clk disable before other clocks suggested by vivek.
- fixed dt example suggested by Rob.
Changes since v2:
- Removed regulators that belong to phy, spotted by Stephen
- Removed clocks in to simple pm bus driver, spotted by Stephen
- renamed msm8996 ops to v2 ops as suggested by Stephen.
- cleanups as suggested by Stephen.
- Add runtime pm support to driver.
- Added pm clk support to simple pm bus driver.
Changes since v1:
- Fixed dt example as suggested by Rob
- added smmu bus clk dependency as smmu sits in between
system NOC and PCIe.
- Removed smmu configuration from bindings and driver as
the smmu Level2 translation on this SOC is controlled by
the secure world, and level 1 translation is disabled,
so there is one-to-one mapping of the address space.
Thanks,
srini
[1] https://patchwork.kernel.org/patch/9384711/
[2] https://patchwork.kernel.org/patch/9377557/
Srinivas Kandagatla (3):
bus: simple-pm: add support to pm clocks
PCI: qcom: add support to msm8996 PCIE controller
PCI: qcom: add runtime pm support to pcie_port
.../devicetree/bindings/pci/qcom,pcie.txt | 67 +++++++-
drivers/bus/simple-pm-bus.c | 13 +-
drivers/pci/host/pcie-qcom.c | 181 ++++++++++++++++++++-
3 files changed, 254 insertions(+), 7 deletions(-)
--
2.10.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox