* Re: [RESEND PATCH 1/2] PCI: rockchip: cleanup bit definition for PCIE_RC_CONFIG_LCS
From: Bjorn Helgaas @ 2016-11-14 22:26 UTC (permalink / raw)
To: Shawn Lin
Cc: Bjorn Helgaas, Rob Herring, linux-pci, linux-rockchip, Wenrui Li,
Brian Norris, Jeffy Chen, devicetree
In-Reply-To: <1479096666-112668-1-git-send-email-shawn.lin@rock-chips.com>
On Mon, Nov 14, 2016 at 12:11:05PM +0800, Shawn Lin wrote:
> PCIE_RC_CONFIG_LCS contains control and status bits specific
> to the PCIe link. The layout for this register looks the same
> as the existed PCI_EXP_LNKCTL and PCI_EXP_LNKSTA. So let's
> reuse them.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Did something change since the version you posted yesterday?
Resending a patch with no changes or with no hint about what changed
doesn't speed things up; in fact, it slows things down.
> ---
>
> drivers/pci/host/pcie-rockchip.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 7f238af..1dba698 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -141,12 +141,6 @@
> #define PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff
> #define PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26
> #define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0)
> -#define PCIE_RC_CONFIG_LCS_RETRAIN_LINK BIT(5)
> -#define PCIE_RC_CONFIG_LCS_CCC BIT(6)
> -#define PCIE_RC_CONFIG_LCS_LBMIE BIT(10)
> -#define PCIE_RC_CONFIG_LCS_LABIE BIT(11)
> -#define PCIE_RC_CONFIG_LCS_LBMS BIT(30)
> -#define PCIE_RC_CONFIG_LCS_LAMS BIT(31)
> #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
> #define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE + 0x274)
> #define PCIE_RC_CONFIG_THP_CAP_NEXT_MASK GENMASK(31, 20)
> @@ -229,7 +223,7 @@ static void rockchip_pcie_enable_bw_int(struct rockchip_pcie *rockchip)
> u32 status;
>
> status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> - status |= (PCIE_RC_CONFIG_LCS_LBMIE | PCIE_RC_CONFIG_LCS_LABIE);
> + status |= (PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE);
> rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> }
>
> @@ -238,7 +232,7 @@ static void rockchip_pcie_clr_bw_int(struct rockchip_pcie *rockchip)
> u32 status;
>
> status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> - status |= (PCIE_RC_CONFIG_LCS_LBMS | PCIE_RC_CONFIG_LCS_LAMS);
> + status |= (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS) << 16;
> rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> }
>
> @@ -540,7 +534,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>
> /* Set RC's clock architecture as common clock */
> status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> - status |= PCIE_RC_CONFIG_LCS_CCC;
> + status |= PCI_EXP_LNKCTL_CCC;
> rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
>
> /* Enable Gen1 training */
> @@ -575,7 +569,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> * gen1 finished.
> */
> status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> - status |= PCIE_RC_CONFIG_LCS_RETRAIN_LINK;
> + status |= PCI_EXP_LNKCTL_RL;
> rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
>
> timeout = jiffies + msecs_to_jiffies(500);
> --
> 1.9.1
>
>
> --
> 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 v4 2/3] PCI: qcom: add support to msm8996 PCIE controller
From: Bjorn Helgaas @ 2016-11-14 22:23 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: svarbanov, linux-pci, bhelgaas, robh+dt, linux-arm-msm,
devicetree
In-Reply-To: <1479122155-13393-3-git-send-email-srinivas.kandagatla@linaro.org>
On Mon, Nov 14, 2016 at 11:15:54AM +0000, Srinivas Kandagatla 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>
Will need ack from Stanimir before I can apply it.
> ---
> .../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
>
> --
> 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 v4 1/3] bus: simple-pm: add support to pm clocks
From: Bjorn Helgaas @ 2016-11-14 22:14 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: svarbanov, linux-pci, bhelgaas, robh+dt, linux-arm-msm,
devicetree, Geert Uytterhoeven, Kevin Hilman, Simon Horman
In-Reply-To: <1479122155-13393-2-git-send-email-srinivas.kandagatla@linaro.org>
[+cc Geert, Kevin, Simon]
On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
> 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>
I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
like an ack or at least a review from Geert or Simon.
> ---
> 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
>
> --
> 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 v2 00/12] Additional iProc PCIe host support/fixes
From: Bjorn Helgaas @ 2016-11-14 22:08 UTC (permalink / raw)
To: Ray Jui
Cc: Bjorn Helgaas, linux-kernel, bcm-kernel-feedback-list, linux-pci,
Alex Barba, Oza Oza
In-Reply-To: <1477960721-17649-1-git-send-email-ray.jui@broadcom.com>
On Mon, Oct 31, 2016 at 05:38:29PM -0700, Ray Jui wrote:
> This patch series contains various changes and fixes to the iProc PCIe
> host driver. It also adds support for the next generation of PAXB and
> PAXC based host controllers
>
> This patch series was developed based on v4.9-rc1 and tested on both NS2
> SVK and Cygnus wireless audio platform
>
> repo: https://github.com/Broadcom/arm64-linux.git
> branch: iproc-pcie-v2
>
> Changes from v1:
> - Fixed varioues typos in the commit message
> - Improved various DT binding patch titles with more specific information
>
> Ray Jui (12):
> PCI: iproc: Improve core register population
> PCI: iproc: Do not reset PAXC when initializing the driver
> PCI: iproc: Add BCMA type
> PCI: iproc: Fix exception with multi-function devices
> PCI: iproc: Added PAXCv2 related binding
> PCI: iproc: Add PAXC v2 support
> PCI: iproc: Remove redundant outbound properties
> PCI: iproc: Making outbound mapping code more generic
> PCI: iproc: Add optional dma-ranges
> PCI: iproc: Add inbound DMA mapping support
> PCI: iproc: Add PAXBv2 binding info
> PCI: iproc: Add support for the next-gen PAXB controller
>
> .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 43 +-
> drivers/pci/host/pcie-iproc-bcma.c | 1 +
> drivers/pci/host/pcie-iproc-msi.c | 1 +
> drivers/pci/host/pcie-iproc-platform.c | 19 +-
> drivers/pci/host/pcie-iproc.c | 942 ++++++++++++++++++---
> drivers/pci/host/pcie-iproc.h | 45 +-
> 6 files changed, 916 insertions(+), 135 deletions(-)
Applied to pci/host-iproc for v4.10, thanks!
^ permalink raw reply
* Re: [PATCH v2 04/12] PCI: iproc: Fix exception with multi-function devices
From: Bjorn Helgaas @ 2016-11-14 21:52 UTC (permalink / raw)
To: Ray Jui
Cc: Bjorn Helgaas, linux-kernel, bcm-kernel-feedback-list, linux-pci,
Alex Barba, Oza Oza
In-Reply-To: <1477960721-17649-5-git-send-email-ray.jui@broadcom.com>
On Mon, Oct 31, 2016 at 05:38:33PM -0700, Ray Jui wrote:
> During enumeration with multi-function EP devices, access to the
> configuration space of a non-exist function results in an unsupported
> request being returned as expected. By default the PAXB based iProc
> PCIe controller forwards this as an APB error to the host system and
> that causes an exception, which is undesired
>
> This patch disables this undesired behaviour and lets the kernel PCI
> stack deals with an access to the non-exist function, in which case
> a vendor ID of 0xffff is returned and handled gracefully
>
> Reported-by: JD Zheng <jiandong.zheng@broadcom.com>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: JD Zheng <jiandong.zheng@broadcom.com>
> Reviewed-by: Oza Oza <oza.oza@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
> drivers/pci/host/pcie-iproc.c | 58 +++++++++++++++++++++++++++++++++++++++++--
> drivers/pci/host/pcie-iproc.h | 3 +++
> 2 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index f3b3340..07ec478 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -58,6 +58,9 @@
> #define PCIE_DL_ACTIVE_SHIFT 2
> #define PCIE_DL_ACTIVE BIT(PCIE_DL_ACTIVE_SHIFT)
>
> +#define APB_ERR_EN_SHIFT 0
> +#define APB_ERR_EN BIT(APB_ERR_EN_SHIFT)
> +
> #define OARR_VALID_SHIFT 0
> #define OARR_VALID BIT(OARR_VALID_SHIFT)
> #define OARR_SIZE_CFG_SHIFT 1
> @@ -96,6 +99,9 @@ enum iproc_pcie_reg {
> /* link status */
> IPROC_PCIE_LINK_STATUS,
>
> + /* enable APB error for unsupported requests */
> + IPROC_PCIE_APB_ERR_EN,
> +
> /* total number of core registers */
> IPROC_PCIE_MAX_NUM_REG,
> };
> @@ -124,6 +130,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
> [IPROC_PCIE_OMAP_LO] = 0xd40,
> [IPROC_PCIE_OMAP_HI] = 0xd44,
> [IPROC_PCIE_LINK_STATUS] = 0xf0c,
> + [IPROC_PCIE_APB_ERR_EN] = 0xf40,
> };
>
> /* iProc PCIe PAXC v1 registers */
> @@ -181,6 +188,28 @@ static inline void iproc_pcie_write_reg(struct iproc_pcie *pcie,
> writel(val, pcie->base + offset);
> }
>
> +/**
> + * APB error forwarding can be disabled during access of configuration
> + * registers of the endpoint device, to prevent unsupported requests
> + * (typically seen during enumeration with multi-function devices) from
> + * triggering a system exception
> + */
> +static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
> + bool disable)
> +{
> + struct iproc_pcie *pcie = iproc_data(bus);
> + u32 val;
> +
> + if (bus->number && pcie->has_apb_err_disable) {
> + val = iproc_pcie_read_reg(pcie, IPROC_PCIE_APB_ERR_EN);
> + if (disable)
> + val &= ~APB_ERR_EN;
> + else
> + val |= APB_ERR_EN;
> + iproc_pcie_write_reg(pcie, IPROC_PCIE_APB_ERR_EN, val);
> + }
> +}
> +
> static inline void iproc_pcie_ob_write(struct iproc_pcie *pcie,
> enum iproc_pcie_reg reg,
> unsigned window, u32 val)
> @@ -244,10 +273,34 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
> return (pcie->base + offset);
> }
>
> +static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 *val)
> +{
> + int ret;
> +
> + iproc_pcie_apb_err_disable(bus, true);
> + ret = pci_generic_config_read32(bus, devfn, where, size, val);
> + iproc_pcie_apb_err_disable(bus, false);
I guess this means that any other APB errors (unrelated to this config
access, e.g., a driver doing MMIo to a device at the same time the user is
doing "lspci") that happen to occur during this window will also be
ignored.
> +
> + return ret;
> +}
> +
> +static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 val)
> +{
> + int ret;
> +
> + iproc_pcie_apb_err_disable(bus, true);
> + ret = pci_generic_config_write32(bus, devfn, where, size, val);
> + iproc_pcie_apb_err_disable(bus, false);
> +
> + return ret;
> +}
> +
> static struct pci_ops iproc_pcie_ops = {
> .map_bus = iproc_pcie_map_cfg_bus,
> - .read = pci_generic_config_read32,
> - .write = pci_generic_config_write32,
> + .read = iproc_pcie_config_read32,
> + .write = iproc_pcie_config_write32,
> };
>
> static void iproc_pcie_reset(struct iproc_pcie *pcie)
> @@ -485,6 +538,7 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
> break;
> case IPROC_PCIE_PAXB:
> regs = iproc_pcie_reg_paxb;
> + pcie->has_apb_err_disable = true;
> break;
> case IPROC_PCIE_PAXC:
> regs = iproc_pcie_reg_paxc;
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index 768be05..711dd3a 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -57,6 +57,8 @@ struct iproc_msi;
> * @phy: optional PHY device that controls the Serdes
> * @map_irq: function callback to map interrupts
> * @ep_is_internal: indicates an internal emulated endpoint device is connected
> + * @has_apb_err_disable: indicates the controller can be configured to prevent
> + * unsupported request from being forwarded as an APB bus error
> * @need_ob_cfg: indicates SW needs to configure the outbound mapping window
> * @ob: outbound mapping parameters
> * @msi: MSI data
> @@ -74,6 +76,7 @@ struct iproc_pcie {
> struct phy *phy;
> int (*map_irq)(const struct pci_dev *, u8, u8);
> bool ep_is_internal;
> + bool has_apb_err_disable;
> bool need_ob_cfg;
> struct iproc_pcie_ob ob;
> struct iproc_msi *msi;
> --
> 2.1.4
>
^ permalink raw reply
* Re: [PATCH 13/15] PCI/ASPM: use permission-specific DEVICE_ATTR variants
From: Julia Lawall @ 2016-11-14 21:52 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Bjorn Helgaas, kernel-janitors, linux-pci, linux-kernel
In-Reply-To: <20161114214037.GE9868@bhelgaas-glaptop.roam.corp.google.com>
On Mon, 14 Nov 2016, Bjorn Helgaas wrote:
> 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.
I think that the point is the latter. There were also a couple of cases
where the permissions didn't match with the set of provided functions.
julia
>
> > ---
> > 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 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
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