Re: [PATCH v2 3/3] PCI: imx6: Add code to support i.MX7D

From: Andrey Smirnov
Date: Thu Feb 02 2017 - 12:00:12 EST

On Wed, Feb 1, 2017 at 9:57 AM, Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> Am Mittwoch, den 01.02.2017, 08:55 -0800 schrieb Andrey Smirnov:
>> Add various bits of code needed to support i.MX7D variant of the IP.
>>
>> Cc: yurovsky@xxxxxxxxx
>> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: devicetree@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
>> ---
>> .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 6 +-
>> drivers/pci/host/pci-imx6.c | 136 +++++++++++++++++----
>> include/linux/mfd/syscon/imx7-gpc.h | 18 +++
>> include/linux/mfd/syscon/imx7-iomuxc-gpr.h | 4 +
>> include/linux/mfd/syscon/imx7-src.h | 18 +++
>> 5 files changed, 156 insertions(+), 26 deletions(-)
>> create mode 100644 include/linux/mfd/syscon/imx7-gpc.h
>> create mode 100644 include/linux/mfd/syscon/imx7-src.h
>>
>> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> index 83aeb1f..9f57759 100644
>> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> @@ -4,7 +4,11 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
>> and thus inherits all the common properties defined in designware-pcie.txt.
>>
>> Required properties:
>> -- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie", "fsl,imx6qp-pcie"
>> +- compatible:
>> + - "fsl,imx6q-pcie"
>> + - "fsl,imx6sx-pcie",
>> + - "fsl,imx6qp-pcie"
>> + - "fsl,imx7d-pcie"
>> - reg: base address and length of the PCIe controller
>> - interrupts: A list of interrupt outputs of the controller. Must contain an
>> entry for each entry in the interrupt-names property.
>> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
>> index 3ef8093..e64ddd9 100644
>> --- a/drivers/pci/host/pci-imx6.c
>> +++ b/drivers/pci/host/pci-imx6.c
>> @@ -17,6 +17,8 @@
>> #include <linux/kernel.h>
>> #include <linux/mfd/syscon.h>
>> #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
>> +#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
>> +#include <linux/mfd/syscon/imx7-src.h>
>> #include <linux/module.h>
>> #include <linux/of_gpio.h>
>> #include <linux/of_device.h>
>> @@ -27,6 +29,7 @@
>> #include <linux/signal.h>
>> #include <linux/types.h>
>> #include <linux/interrupt.h>
>> +#include <linux/pm_domain.h>
>
> I don't think you need this include. PM domain handling should be done
> by the linux driver core if the pcie node is placed in the proper
> power-domain.
>
> The pcie driver should only indirectly handle pm domains via the runtime
> pm kernel facilities.
>

You are right, I have no reason to have this include. It's just a
experimentation leftover I forgot to remove. Will get rid of it in v3.

>>
>> #include "pcie-designware.h"
>>
>> @@ -36,6 +39,7 @@ enum imx6_pcie_variants {
>> IMX6Q,
>> IMX6SX,
>> IMX6QP,
>> + IMX7D,
>> };
>>
>> struct imx6_pcie {
>> @@ -47,6 +51,7 @@ struct imx6_pcie {
>> struct clk *pcie_inbound_axi;
>> struct clk *pcie;
>> struct regmap *iomuxc_gpr;
>> + struct regmap *src;
>> enum imx6_pcie_variants variant;
>> u32 tx_deemph_gen1;
>> u32 tx_deemph_gen2_3p5db;
>> @@ -56,6 +61,11 @@ struct imx6_pcie {
>> int link_gen;
>> };
>>
>> +/* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
>> +#define PHY_PLL_LOCK_WAIT_MAX_RETRIES 2000
>> +#define PHY_PLL_LOCK_WAIT_USLEEP_MIN 50
>> +#define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200
>> +
>> /* PCIe Root Complex registers (memory-mapped) */
>> #define PCIE_RC_LCR 0x7c
>> #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1
>> @@ -251,6 +261,16 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>> u32 val, gpr1, gpr12;
>>
>> switch (imx6_pcie->variant) {
>> + case IMX7D:
>> + regmap_update_bits(imx6_pcie->src,
>> + SRC_PCIEPHY_RCR,
>> + IMX7D_SRC_PCIEPHY_RCR_PCIEPHY_G_RST,
>> + IMX7D_SRC_PCIEPHY_RCR_PCIEPHY_G_RST);
>> + regmap_update_bits(imx6_pcie->src,
>> + SRC_PCIEPHY_RCR,
>> + IMX7D_SRC_PCIEPHY_RCR_PCIEPHY_BTN,
>> + IMX7D_SRC_PCIEPHY_RCR_PCIEPHY_BTN);
>> + break;
>
> I suppose those are just some random bits tossed together, that can't
> reasonably be abstracted by a reset controller, right? In that case I'm
> fine with the code as is.

Hmm, good point, I didn't think of that. SRC is a system reset
controller, after all, so I think it actually might be possible to do
what you describe. I'll investigate the possibility and convert this
if I can in v3.

>
>> case IMX6SX:
>> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
>> IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
>> @@ -333,11 +353,33 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
>> IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
>> break;
>> + case IMX7D:
>> + break;
>> }
>>
>> return ret;
>> }
>>
> [...]
>
>> diff --git a/include/linux/mfd/syscon/imx7-gpc.h b/include/linux/mfd/syscon/imx7-gpc.h
>> new file mode 100644
>> index 0000000..38e1c9a9
>> --- /dev/null
>> +++ b/include/linux/mfd/syscon/imx7-gpc.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * Copyright (C) 2017 Impinj
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __LINUX_IMX7_GPC_H
>> +#define __LINUX_IMX7_GPC_H
>> +
>> +#define GPC_PGC_CPU_MAPPING 0xec
>> +#define GPC_PU_PGC_SW_PUP_REQ 0xf8
>> +
>> +#define IMX7D_PCIE_PHY_A7_DOMAIN BIT(3)
>> +#define IMX7D_PCIE_PHY_SW_PUP_REQ BIT(1)
>> +
>> +#endif
>
> This file isn't referenced in the patch anymore. Please remove.

More leftover code that I missed. Will remove in v3 and sorry for this noise.

Thanks,
Andrey