Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Rob Herring <robh@kernel.org>
To: Baruch Siach <baruch@tkos.co.il>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	 Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>,
	Kathiravan T <kathirav@codeaurora.org>,
	 Bjorn Helgaas <bhelgaas@google.com>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Vinod Koul <vkoul@kernel.org>,
	 devicetree@vger.kernel.org, linux-phy@lists.infradead.org,
	 PCI <linux-pci@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/5] PCI: qcom: add support for IPQ60xx PCIe controller
Date: Wed, 21 Apr 2021 13:45:26 -0500
Message-ID: <CAL_JsqKP1hZ6UGEuevjcMC_ZLbojemZ7-X-koyaxViRHQOoN4g@mail.gmail.com> (raw)
In-Reply-To: <87r1j4kzzm.fsf@tarshish>

On Tue, Apr 20, 2021 at 11:45 PM Baruch Siach <baruch@tkos.co.il> wrote:
>
> Hi Rob,
>
> Thanks for your review.
>
> I have a few comments below.
>
> On Tue, Apr 20 2021, Rob Herring wrote:
> > On Tue, Apr 20, 2021 at 02:21:36PM +0300, Baruch Siach wrote:
> >> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> >>
> >> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> >> platform.
> >>
> >> The code is based on downstream Codeaurora kernel v5.4. Split out the
> >> registers access part from .init into .post_init. Registers are only
> >> accessible after phy_power_on().
> >>
> >> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> >> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >> ---
> >>  drivers/pci/controller/dwc/pcie-qcom.c | 279 +++++++++++++++++++++++++
> >>  1 file changed, 279 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >> index 8a7a300163e5..3e27de744738 100644
> >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >> @@ -41,6 +41,31 @@
> >>  #define L23_CLK_RMV_DIS                             BIT(2)
> >>  #define L1_CLK_RMV_DIS                              BIT(1)
> >>
> >> +#define PCIE_ATU_CR1_OUTBOUND_6_GEN3                0xC00
> >> +#define PCIE_ATU_CR2_OUTBOUND_6_GEN3                0xC04
> >> +#define PCIE_ATU_LOWER_BASE_OUTBOUND_6_GEN3 0xC08
> >> +#define PCIE_ATU_UPPER_BASE_OUTBOUND_6_GEN3 0xC0C
> >> +#define PCIE_ATU_LIMIT_OUTBOUND_6_GEN3              0xC10
> >> +#define PCIE_ATU_LOWER_TARGET_OUTBOUND_6_GEN3       0xC14
> >> +#define PCIE_ATU_UPPER_TARGET_OUTBOUND_6_GEN3       0xC18
> >> +
> >> +#define PCIE_ATU_CR1_OUTBOUND_7_GEN3                0xE00
> >> +#define PCIE_ATU_CR2_OUTBOUND_7_GEN3                0xE04
> >> +#define PCIE_ATU_LOWER_BASE_OUTBOUND_7_GEN3 0xE08
> >> +#define PCIE_ATU_UPPER_BASE_OUTBOUND_7_GEN3 0xE0C
> >> +#define PCIE_ATU_LIMIT_OUTBOUND_7_GEN3              0xE10
> >> +#define PCIE_ATU_LOWER_TARGET_OUTBOUND_7_GEN3       0xE14
> >> +#define PCIE_ATU_UPPER_TARGET_OUTBOUND_7_GEN3       0xE18
> >
> > ATU registers are standard DWC registers. Plus upstream now dynamically
> > detects how many ATU regions there are.
> >
> >> +#define PCIE20_COMMAND_STATUS                       0x04
> >> +#define BUS_MASTER_EN                               0x7
> >> +#define PCIE20_DEVICE_CONTROL2_STATUS2              0x98
> >> +#define PCIE_CAP_CPL_TIMEOUT_DISABLE                0x10
> >
> > All PCI standard registers.
>
> PCIE20_COMMAND_STATUS is indeed the common PCI_COMMAND. I could not find
> anything that matches PCIE20_DEVICE_CONTROL2_STATUS2. Where should I
> look?

Looks like PCI_EXP_DEVCTL2 and PCI_EXP_DEVSTA2 to me. The register bit
looks like PCI_EXP_DEVCTL2_COMP_TMOUT_DIS. Those are extended config
registers so their offset is variable.

>
> >
> >> +#define PCIE30_GEN3_RELATED_OFF                     0x890
> >
> > Looks like a DWC port logic register. The define at a minimum goes in
> > the common code. We probably already have one. Code touching the
> > register should ideally be there too (hint: look at the other drivers).
>
> pcie-tegra194.c uses the equivalent GEN3_RELATED_OFF. So I can move the
> definition to a common header. As for the code, I don't know. The tegra
> configuration sequence involves other registers as well.

I'm sure the Tegra folks will be happy to tell you if anything breaks.

>
> [snip]
>
> >> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +    struct dw_pcie *pci = pcie->pci;
> >> +    u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >> +    u32 val;
> >> +    int i;
> >> +
> >> +    writel(SLV_ADDR_SPACE_SZ,
> >> +            pcie->parf + PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE);
> >> +
> >> +    val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> >> +    val &= ~BIT(0);
> >
> > What's BIT(0)?
>
> I have no idea. I have no access to hardware documentation. I'm just
> porting working code from the Codeaurora tree.

Based on the 7 other existing modifications to that bit, it's 'enable
PCIe clocks and resets'. Looks like we need some refactoring at least
so there's not yet another copy.

> >> +    writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> >> +
> >> +    writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> >> +
> >> +    writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> >> +    writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
> >> +            pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> >> +    writel(RXEQ_RGRDLESS_RXTS | GEN3_ZRXDC_NONCOMPL,
> >> +            pci->dbi_base + PCIE30_GEN3_RELATED_OFF);
> >> +
> >> +    writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> >> +            | SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> >> +            AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
> >> +            pcie->parf + PCIE20_PARF_SYS_CTRL);
> >> +
> >> +    writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
> >> +
> >> +    writel(BUS_MASTER_EN, pci->dbi_base + PCIE20_COMMAND_STATUS);
> >
> > Pretty sure the DWC core or PCI core does this already.
> >
> >> +
> >> +    writel(DBI_RO_WR_EN, pci->dbi_base + PCIE20_MISC_CONTROL_1_REG);
> >> +    writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);

I have to wonder if all the bits being set here are really true.
Hotplug is really supported? There's an attention button? Power
indicator? If anything, that's all board specific and would need to
come from firmware (DT).

> >> +
> >> +    /* Configure PCIe link capabilities for ASPM */
> >> +    val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
> >> +    val &= ~PCI_EXP_LNKCAP_ASPMS;
> >> +    writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
> >> +
> >> +    writel(PCIE_CAP_CPL_TIMEOUT_DISABLE, pci->dbi_base +
> >> +            PCIE20_DEVICE_CONTROL2_STATUS2);
> >> +
> >> +    writel(PCIE_CAP_CURR_DEEMPHASIS | SPEED_GEN3,

SPEED_GEN3 does not look right for PCI_EXP_DEVCTL2.

> >> +                    pci->dbi_base + offset + PCI_EXP_DEVCTL2);
> >
> > This all looks like stuff that should be in the DWC core code. Maybe we
> > need an ASPM disable quirk or something? That's probably somewhat
> > common.
>
> Where in common code should that be?

If these registers are initialized elsewhere, in the same place.
Otherwise, probably in dw_pcie_setup_rc().

> Which part is quirky?

Disabling ASPM. It's a bit strange that some of this is needed at all
considering no other platform using the same IP needs it.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-20 11:21 [PATCH 0/5] arm64: IPQ6018 PCIe support Baruch Siach
2021-04-20 11:21 ` [PATCH 1/5] PCI: qcom: add support for IPQ60xx PCIe controller Baruch Siach
2021-04-20 16:18   ` Rob Herring
2021-04-21  4:45     ` Baruch Siach
2021-04-21 18:45       ` Rob Herring [this message]
2021-04-20 11:21 ` [PATCH 2/5] phy: qcom-qmp: add QMP V2 PCIe PHY support for ipq60xx Baruch Siach
2021-04-20 11:21 ` [PATCH 3/5] arm64: dts: ipq6018: Add pcie support Baruch Siach
2021-04-20 11:21 ` [PATCH 4/5] dt-bindings: phy: qcom, qmp: Add IPQ60xx PCIe PHY bindings Baruch Siach
2021-04-30 15:04   ` Rob Herring
2021-04-20 11:21 ` [PATCH 5/5] dt-bindings: pci: qcom: Document PCIe bindings for IPQ6018 SoC Baruch Siach
2021-04-30 15:05   ` Rob Herring

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=CAL_JsqKP1hZ6UGEuevjcMC_ZLbojemZ7-X-koyaxViRHQOoN4g@mail.gmail.com \
    --to=robh@kernel.org \
    --cc=agross@kernel.org \
    --cc=baruch@tkos.co.il \
    --cc=bhelgaas@google.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=kathirav@codeaurora.org \
    --cc=kishon@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-phy@lists.infradead.org \
    --cc=speriaka@codeaurora.org \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

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

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

Linux-ARM-Kernel Archive on lore.kernel.org

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

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

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


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