From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 47A03C433FE for ; Fri, 22 Oct 2021 16:55:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 276BB611BD for ; Fri, 22 Oct 2021 16:55:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233627AbhJVQ5b (ORCPT ); Fri, 22 Oct 2021 12:57:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59424 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233413AbhJVQ5a (ORCPT ); Fri, 22 Oct 2021 12:57:30 -0400 Received: from mail-pg1-x533.google.com (mail-pg1-x533.google.com [IPv6:2607:f8b0:4864:20::533]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 82E9AC061766 for ; Fri, 22 Oct 2021 09:55:12 -0700 (PDT) Received: by mail-pg1-x533.google.com with SMTP id 75so3867249pga.3 for ; Fri, 22 Oct 2021 09:55:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gateworks-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XKk5S6zzVsLpJki6KUTEWM/bL8H8tlEpxuGdUAGLl0U=; b=ZoDtpaZS4cpVQQOKeR1r9y3j/BXLAxVmG1e5e0/Fgp6xIHmssCQBlBIwARimP9oy47 ntqMdiGiscx3uY2O31gZfk2+xLzd4uWEEnmg6K+pWUXT8DDGQRq00C+esyx9vv2laA/p 82QOLbwdZ3vdRqRY5Pm8XP4RQkTOyefm9NiwZyqE1wFY+EhcFBsyqfSjOonQ/152hAXp efmQOxvZUlr9mTsvN/hq2wvxecDZPr06xNX8DUuCtxYwlybn7QJREfbVRiVTZGYyBZbg fRq/vLw+yWaYQ/CzrBWolnvIr0X0W9vtvOk5UtTjaXbf3ik2tYn6c2jMbNb+5LHAFotZ /SZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XKk5S6zzVsLpJki6KUTEWM/bL8H8tlEpxuGdUAGLl0U=; b=qNzV8rgX4mA5fuvyX6yBInAt3vQT8QuLs1H1RgR9sbuDz77gjN30Dajiik40MOrGAo vzU/c2j4h3JpHYkiEPFXU3HVfVUj2yPIjnyXmV5S2Z3i6ToRXz5m6q3nTDTcw8OFCCVp rELoppOLQ1PXZuogWcF4wVaV3Mt9pMtvDLN2/tcScbmIksB3+jegOH2BlNyMGXj3o8VY qvNpnZ1od6SrdcKZLHkLd3WpFU27OQoC9eDrM+cfNV/4m0Y2G7Fgd9i51uF3vAb+w4tC zXNj+f6slg+0H7tVPf0sAjM6LtyMogJngZBW5EXP7nMCM0YNYaHw/kWDywn/V5yNbkWk kDXw== X-Gm-Message-State: AOAM530EXJMcYM4gDDah9vCj0FTM5Kk4O6zzHWB2hgnj25CXjH2LrhKS GSaIEVALpSMH4Terhh1cPWk2ijfSylLvQH2+aOBdVg== X-Google-Smtp-Source: ABdhPJzoKPJA+Npi33fozucBU+egiBMqOwtCUaF7Au9zs2T9f9BTF6ZYR/4JNltrZhWa99tRXVV72Zt8wSf2CHsQIJw= X-Received: by 2002:a63:788e:: with SMTP id t136mr660555pgc.432.1634921711960; Fri, 22 Oct 2021 09:55:11 -0700 (PDT) MIME-Version: 1.0 References: <1634028078-2387-1-git-send-email-hongxing.zhu@nxp.com> In-Reply-To: From: Tim Harvey Date: Fri, 22 Oct 2021 09:55:00 -0700 Message-ID: Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie support To: Richard Zhu Cc: Lucas Stach , Kishon Vijay Abraham I , "vkoul@kernel.org" , Rob Herring , "galak@kernel.crashing.org" , Shawn Guo , "linux-phy@lists.infradead.org" , Device Tree Mailing List , Linux ARM Mailing List , open list , Sascha Hauer , dl-linux-imx Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 22, 2021 at 8:59 AM Tim Harvey wrote: > > On Thu, Oct 21, 2021 at 5:43 PM Richard Zhu wrote: > > > > > -----Original Message----- > > > From: Tim Harvey > > > Sent: Friday, October 22, 2021 12:25 AM > > > To: Richard Zhu > > > Cc: Lucas Stach ; Kishon Vijay Abraham I > > > ; vkoul@kernel.org; Rob Herring ; > > > galak@kernel.crashing.org; Shawn Guo ; > > > linux-phy@lists.infradead.org; Device Tree Mailing List > > > ; Linux ARM Mailing List > > > ; open list > > > ; Sascha Hauer ; > > > dl-linux-imx > > > Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie > > > support > > > > > > On Wed, Oct 20, 2021 at 8:32 PM Richard Zhu > > > wrote: > > > > > > > > > > > > > > > > > > Richard, > > > > > > > > > > What is this 'invalid resource' about? I see that with my downstream > > > > > IMX8MM PCIe driver as well and have been asked about it. > > > > > > > > > [Richard Zhu] Hi Tim: > > > > This complain is caused by the following codes in pcie-designware.c driver. > > > > I'm not sure that why there is only size assignment after the res valid check, > > > and do nothing if the res is invalid. > > > > It seems that it is an expected design logic refer to the later codes. > > > > if (!pci->atu_base) { > > > > struct resource *res = > > > > > > > platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu"); > > > > if (res) > > > > pci->atu_size = resource_size(res); > > > > pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > > if (IS_ERR(pci->atu_base)) > > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > > } > > > > > > > > Since the default offset is used on i.MX8MM, the "atu" is not specified in > > > i.MX8MM PCIe DT node, so there is no real res at all. > > > > Then, devm_ioremap_resource() would complain the invalid resource. > > > > > > I think you are saying a change should be made like this: > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c > > > b/drivers/pci/controller/dwc/pcie-designware.c > > > index a945f0c0e73d..3254f60d1713 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -671,10 +671,11 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > if (!pci->atu_base) { > > > struct resource *res = > > > > > > platform_get_resource_byname(pdev, > > > IORESOURCE_MEM, "atu"); > > > - if (res) > > > + if (res) { > > > pci->atu_size = resource_size(res); > > > - pci->atu_base = devm_ioremap_resource(dev, > > > res); > > > - if (IS_ERR(pci->atu_base)) > > > + pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > + } > > > + if (!pci->atu_base || IS_ERR(pci->atu_base)) > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > } > > > > > > so that it looks like this: > > > if (!pci->atu_base) { > > > struct resource *res = > > > > > > platform_get_resource_byname(pdev, > > > IORESOURCE_MEM, "atu"); > > > if (res) { > > > pci->atu_size = resource_size(res); > > > pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > } > > > if (!pci->atu_base || IS_ERR(pci->atu_base)) > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > } > > > > > > Right? > > [Richard Zhu] Yes, it is. The res shouldn't be remapped if it is invalid resource memory. > > Ok, I will submit a patch for that. > > > > > > > > > > > > > > > > [ 1.316305] imx6q-pcie 33800000.pcie: iATU unroll: enabled > > > > > > [ 1.321799] imx6q-pcie 33800000.pcie: Detected iATU regions: 4 > > > > > outbound, 4 inbound > > > > > > [ 1.429803] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.534497] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.538870] imx6q-pcie 33800000.pcie: Link up, Gen2 > > > > > > [ 1.550364] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.550487] imx6q-pcie 33800000.pcie: PCI host bridge to bus > > > 0000:00 > > > > > > [ 1.565545] pci_bus 0000:00: root bus resource [bus 00-ff] > > > > > > [ 1.573834] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] > > > > > > [ 1.580055] pci_bus 0000:00: root bus resource [mem > > > > > 0x18000000-0x1fefffff] > > > > > > [ 1.586968] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400 > > > > > > [ 1.592997] pci 0000:00:00.0: reg 0x10: [mem > > > 0x00000000-0x000fffff] > > > > > > [ 1.599282] pci 0000:00:00.0: reg 0x38: [mem > > > 0x00000000-0x0000ffff > > > > > pref] > > > > > > [ 1.606033] pci 0000:00:00.0: supports D1 > > > > > > [ 1.610053] pci 0000:00:00.0: PME# supported from D0 D1 D3hot > > > > > D3cold > > > > > > [ 1.618206] pci 0000:01:00.0: [15b7:5002] type 00 class 0x010802 > > > > > > [ 1.624293] pci 0000:01:00.0: reg 0x10: [mem > > > 0x00000000-0x00003fff > > > > > 64bit] > > > > > > [ 1.631177] pci 0000:01:00.0: reg 0x20: [mem > > > 0x00000000-0x000000ff > > > > > 64bit] > > > > > > [ 1.638409] pci 0000:01:00.0: 4.000 Gb/s available PCIe bandwidth, > > > > > limited by 5.0 GT/s PCIe x1 link at 0000:00:00.0 (capable of 31.504 > > > > > Gb/s with > > > > > 8.0 GT/s PCIe x4 link) > > > > > > [ 1.664931] pci 0000:00:00.0: BAR 0: assigned [mem > > > > > 0x18000000-0x180fffff] > > > > > > [ 1.671745] pci 0000:00:00.0: BAR 14: assigned [mem > > > > > 0x18100000-0x181fffff] > > > > > > [ 1.678634] pci 0000:00:00.0: BAR 6: assigned [mem > > > > > 0x18200000-0x1820ffff pref] > > > > > > [ 1.685873] pci 0000:01:00.0: BAR 0: assigned [mem > > > > > 0x18100000-0x18103fff 64bit] > > > > > > [ 1.693222] pci 0000:01:00.0: BAR 4: assigned [mem > > > > > 0x18104000-0x181040ff 64bit] > > > > > > [ 1.700577] pci 0000:00:00.0: PCI bridge to [bus 01-ff] > > > > > > [ 1.705814] pci 0000:00:00.0: bridge window [mem > > > > > 0x18100000-0x181fffff] > > > > > > [ 1.712972] pcieport 0000:00:00.0: PME: Signaling with IRQ 216 > > > > > > " > > > > > > Regarding the log you pasted, it seems that the clock is not feed > > > > > > to PHY > > > > > properly. > > > > > > > > > > > > Anyway, let's waiting for the v4 series, then make a try. Thanks > > > > > > for your > > > > > great help to make the double tests. > > > > > > > > > > > > > > > > My boards do not use CLKREQ# so I do not have that defined in pinmux > > > > > and I found that if I add MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B > > > PCIe > > > > > works on my board but this isn't a solution just a work-around (I > > > > > have boards that use the only two possible pins for CLKREQ as other > > > features). > > > > > > > > > > Similarly you will find on the imx8mm-evk if you comment out the > > > > > CLKREQ (which isn't required) the imx8mmevk will end up hanging like my > > > boards: > > > > [Richard Zhu] Hi Tim: > > > > Regarding the SPEC, the CLKREQ# is mandatory required, and should be > > > configured as an open drain, active low signal. > > > > And this signal should be driven low by the PCIe M.2 device to request the > > > REF clock be available(active low). > > > > So, there is such kind of CLKREQ# pin definition on i.MX8MM EVK board. > > > > > > > > Anyway, I think the external OSC circuit should be always running if there is > > > no CLKREQ# on your HW board design. > > > > > > > > > > The way I understand it is CLKREQ# allows the host to disable the REFCLK > > > when not needed for power savings so it would seem optional to implement > > > that and if not implemented should be left unconnected on the card. > > > > > [Richard Zhu] No, not that way. Regarding the SPEC, this signal is mandatory required. > > Especially for the L1ss usages. This signal would be OD(open drain), bi-directional, and might be > > driven low/high by RC or EP automatically if L1ss modes are enabled. > > You can make reference to the "ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a", or > > the chapter 5.5 L1 PM Substates of "PCI Express Base Specification, Rev. 4.0 Version 1.0". > > > > CLKREQ is only mandatory if you wish to support clock power > management. Many boards with a PCI host controller do not support > this. > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > index 5ce43daa0c8b..f0023b48f475 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > @@ -448,7 +448,9 @@ > > > > > > > > > > pinctrl_pcie0: pcie0grp { > > > > > fsl,pins = < > > > > > +/* > > > > > > > > > > MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B 0x61 > > > > > +*/ > > > > > > > > MX8MM_IOMUXC_SAI2_RXFS_GPIO4_IO21 > > > > > 0x41 > > > > > >; > > > > > }; > > > > > > > > > > I have PCIe working with a driver that I ported from NXP's kernel > > > > > which differs from your driver in that the PCIe PHY is not > > > > > abstracted to its own driver so I think this has something to do > > > > > with the order in which the phy is reset or initialized? The configuration of > > > gpr14 bits looks correct to me. > > > > [Richard Zhu] The CLKREQ# PIN definition shouldn't be masked. > > > > In the NXP's local BSP kernel, I just force CLKREQ# low to level up the HW > > > compatibility. > > > > That's might the reason why the PCIe works on your HW board although the > > > CLKREQ# PIN is not defined. > > > > This method is a little rude and violate the SPEC, and not recommended > > > although it levels up the HW compatibility. > > > > So I drop this method in this series. > > > > > > > > > > Sorry, I don't understand what you are saying here. Is there a change you are > > > going to make to v4 that will make this work for the evk and my boards? What > > > is that change exactly? > > [Richard Zhu] No. What I said above is that the CLKREQ# is forced to be low in NXP > > local BSP kernel. I guess this might be the reason why your board works. > > > > BIT11 and BIT10 of IOMUXC_GPR14 can be used to force the CLKREQ# to be low. > > Set CLKREQ_OVERRIDE_EN(bit10) 1b1, then write one zero to CLKREQ_OVERRIDE(bit11). > > > > Ok, that makes sense. Those bits are not explained well in the > IMX8MMRM. As my board's external REFCLK is always enabled that must > gate the clock internally to the host controller block. > > I can confirm that asserting those GPR14 bits does resolve my issue: > > #define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL BIT(11) > #define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN BIT(10) > > /* > * for boards that do not connect CLKREQ#, > * override CLKREQ# and drive it low internally > */ > regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14, > IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL, 0); > regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14, > IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN, 1); > > Should this be added as a 'fsl,clkreq-unsupported' flag that needs to > be set true to implement the above code? > Richard, Sorry - spoke too soon. My test was flawed as I still was pinmuxing CLKREQ in my dt to work around the issue and after removed the above did not resolve my issue. The setting of OVERRIDE_EN was wrong above (should not be set to '1' but BIT(10) instead) but this code already exists in imx6_pcie_enable_ref_clk and is used for IMX8MM per your patch so this is not the issue. What makes my board work is to clear GPR14 bit9 (like the NXP kernel does) so I don't think this bit does what we think it does (select between internal and ext clk). I think setting it enables clock gating via CLKREQ#. This also points out that perhaps the CLKREQ_OVERRIDE logic should be moved to the new phy driver for IMX8MM. Best regards, Tim From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D5B16C433F5 for ; Fri, 22 Oct 2021 16:55:27 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9606E611BD for ; Fri, 22 Oct 2021 16:55:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9606E611BD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=gateworks.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=77fM6mkCcMdR1MZB46hBmCbatZ+WCSfw339O3uZRAgg=; b=mmWwfXaZmKORfi 7AsO3ogsTdJtvCODvCW9GZe3Z4DA9hANFlTXD1iom9zX6z07yF5Dg1sJg1VyH4O95G8w1k+fDRsB9 JGICvHYS8A7xJ4oAv24b8G3mGccsVHvU0ftYZR9/PArwIV3StIt+xXqT8MWKcKLicbPaG6W6T8y/X PBmmFVFtHYg7fD7B2wP2Bu8ko/Ewk7BpNPvO8qppz6Tqs8LJUBVWjYn8hxgIcINR7xAimF/C9a/jn Vsh2UVMIj20eA3Id6zRKevKI45aCK8xerOU9F+bDSJQATz3KLNdVQtzMCdwY3FNWMuRUErSgwoLD0 Kh0zNJTrqdJKqS5ToEHg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mdxpH-00BYpe-1R; Fri, 22 Oct 2021 16:55:27 +0000 Received: from mail-pg1-x531.google.com ([2607:f8b0:4864:20::531]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mdxp2-00BYlY-VX for linux-phy@lists.infradead.org; Fri, 22 Oct 2021 16:55:15 +0000 Received: by mail-pg1-x531.google.com with SMTP id m21so3828390pgu.13 for ; Fri, 22 Oct 2021 09:55:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gateworks-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XKk5S6zzVsLpJki6KUTEWM/bL8H8tlEpxuGdUAGLl0U=; b=ZoDtpaZS4cpVQQOKeR1r9y3j/BXLAxVmG1e5e0/Fgp6xIHmssCQBlBIwARimP9oy47 ntqMdiGiscx3uY2O31gZfk2+xLzd4uWEEnmg6K+pWUXT8DDGQRq00C+esyx9vv2laA/p 82QOLbwdZ3vdRqRY5Pm8XP4RQkTOyefm9NiwZyqE1wFY+EhcFBsyqfSjOonQ/152hAXp efmQOxvZUlr9mTsvN/hq2wvxecDZPr06xNX8DUuCtxYwlybn7QJREfbVRiVTZGYyBZbg fRq/vLw+yWaYQ/CzrBWolnvIr0X0W9vtvOk5UtTjaXbf3ik2tYn6c2jMbNb+5LHAFotZ /SZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XKk5S6zzVsLpJki6KUTEWM/bL8H8tlEpxuGdUAGLl0U=; b=ds/6sRQI9UZ1LvYJH7M5XUf9WQNyKYfYrGN0C7IePKTx1Znqj+B5+5Dc2kIEV82MhU nv/I8yfd/BDK64KlEs+mZw5pdLvtdXK9dyAgJmmj26LIxoOUPhpny5OATM7s06F1RbSV CMknLNr1Jx6o89MsQp3CSqTLm9VGhjN96DWbs4JclDfWeQCKDs6Yw5oXQU+m515dkaO5 pmNcIdL7pfLRhnmC0jrICua08tz9r/shQp5wY1y90fQFNECHMsxsZQJEMgi4mZA/akld UkzBnRCoeohVwn9Ldg+UrdPI3eV6AakCpDsujbVxzHVcARSBuLBobYfI0Ec806esO3Ny TV8A== X-Gm-Message-State: AOAM530+5GFJxqyUJ89YW2bFdu8BmLgZ6/WoMtKQBL3h1vMgRZZ0fzxX IgPXKxAGNUAoI7aiLloiulJcO69YDhT+tdubQTN8fw== X-Google-Smtp-Source: ABdhPJzoKPJA+Npi33fozucBU+egiBMqOwtCUaF7Au9zs2T9f9BTF6ZYR/4JNltrZhWa99tRXVV72Zt8wSf2CHsQIJw= X-Received: by 2002:a63:788e:: with SMTP id t136mr660555pgc.432.1634921711960; Fri, 22 Oct 2021 09:55:11 -0700 (PDT) MIME-Version: 1.0 References: <1634028078-2387-1-git-send-email-hongxing.zhu@nxp.com> In-Reply-To: From: Tim Harvey Date: Fri, 22 Oct 2021 09:55:00 -0700 Message-ID: Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie support To: Richard Zhu Cc: Lucas Stach , Kishon Vijay Abraham I , "vkoul@kernel.org" , Rob Herring , "galak@kernel.crashing.org" , Shawn Guo , "linux-phy@lists.infradead.org" , Device Tree Mailing List , Linux ARM Mailing List , open list , Sascha Hauer , dl-linux-imx X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211022_095513_041385_E0CC4838 X-CRM114-Status: GOOD ( 66.81 ) X-BeenThere: linux-phy@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux Phy Mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org On Fri, Oct 22, 2021 at 8:59 AM Tim Harvey wrote: > > On Thu, Oct 21, 2021 at 5:43 PM Richard Zhu wrote: > > > > > -----Original Message----- > > > From: Tim Harvey > > > Sent: Friday, October 22, 2021 12:25 AM > > > To: Richard Zhu > > > Cc: Lucas Stach ; Kishon Vijay Abraham I > > > ; vkoul@kernel.org; Rob Herring ; > > > galak@kernel.crashing.org; Shawn Guo ; > > > linux-phy@lists.infradead.org; Device Tree Mailing List > > > ; Linux ARM Mailing List > > > ; open list > > > ; Sascha Hauer ; > > > dl-linux-imx > > > Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie > > > support > > > > > > On Wed, Oct 20, 2021 at 8:32 PM Richard Zhu > > > wrote: > > > > > > > > > > > > > > > > > > Richard, > > > > > > > > > > What is this 'invalid resource' about? I see that with my downstream > > > > > IMX8MM PCIe driver as well and have been asked about it. > > > > > > > > > [Richard Zhu] Hi Tim: > > > > This complain is caused by the following codes in pcie-designware.c driver. > > > > I'm not sure that why there is only size assignment after the res valid check, > > > and do nothing if the res is invalid. > > > > It seems that it is an expected design logic refer to the later codes. > > > > if (!pci->atu_base) { > > > > struct resource *res = > > > > > > > platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu"); > > > > if (res) > > > > pci->atu_size = resource_size(res); > > > > pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > > if (IS_ERR(pci->atu_base)) > > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > > } > > > > > > > > Since the default offset is used on i.MX8MM, the "atu" is not specified in > > > i.MX8MM PCIe DT node, so there is no real res at all. > > > > Then, devm_ioremap_resource() would complain the invalid resource. > > > > > > I think you are saying a change should be made like this: > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c > > > b/drivers/pci/controller/dwc/pcie-designware.c > > > index a945f0c0e73d..3254f60d1713 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -671,10 +671,11 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > if (!pci->atu_base) { > > > struct resource *res = > > > > > > platform_get_resource_byname(pdev, > > > IORESOURCE_MEM, "atu"); > > > - if (res) > > > + if (res) { > > > pci->atu_size = resource_size(res); > > > - pci->atu_base = devm_ioremap_resource(dev, > > > res); > > > - if (IS_ERR(pci->atu_base)) > > > + pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > + } > > > + if (!pci->atu_base || IS_ERR(pci->atu_base)) > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > } > > > > > > so that it looks like this: > > > if (!pci->atu_base) { > > > struct resource *res = > > > > > > platform_get_resource_byname(pdev, > > > IORESOURCE_MEM, "atu"); > > > if (res) { > > > pci->atu_size = resource_size(res); > > > pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > } > > > if (!pci->atu_base || IS_ERR(pci->atu_base)) > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > } > > > > > > Right? > > [Richard Zhu] Yes, it is. The res shouldn't be remapped if it is invalid resource memory. > > Ok, I will submit a patch for that. > > > > > > > > > > > > > > > > [ 1.316305] imx6q-pcie 33800000.pcie: iATU unroll: enabled > > > > > > [ 1.321799] imx6q-pcie 33800000.pcie: Detected iATU regions: 4 > > > > > outbound, 4 inbound > > > > > > [ 1.429803] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.534497] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.538870] imx6q-pcie 33800000.pcie: Link up, Gen2 > > > > > > [ 1.550364] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.550487] imx6q-pcie 33800000.pcie: PCI host bridge to bus > > > 0000:00 > > > > > > [ 1.565545] pci_bus 0000:00: root bus resource [bus 00-ff] > > > > > > [ 1.573834] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] > > > > > > [ 1.580055] pci_bus 0000:00: root bus resource [mem > > > > > 0x18000000-0x1fefffff] > > > > > > [ 1.586968] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400 > > > > > > [ 1.592997] pci 0000:00:00.0: reg 0x10: [mem > > > 0x00000000-0x000fffff] > > > > > > [ 1.599282] pci 0000:00:00.0: reg 0x38: [mem > > > 0x00000000-0x0000ffff > > > > > pref] > > > > > > [ 1.606033] pci 0000:00:00.0: supports D1 > > > > > > [ 1.610053] pci 0000:00:00.0: PME# supported from D0 D1 D3hot > > > > > D3cold > > > > > > [ 1.618206] pci 0000:01:00.0: [15b7:5002] type 00 class 0x010802 > > > > > > [ 1.624293] pci 0000:01:00.0: reg 0x10: [mem > > > 0x00000000-0x00003fff > > > > > 64bit] > > > > > > [ 1.631177] pci 0000:01:00.0: reg 0x20: [mem > > > 0x00000000-0x000000ff > > > > > 64bit] > > > > > > [ 1.638409] pci 0000:01:00.0: 4.000 Gb/s available PCIe bandwidth, > > > > > limited by 5.0 GT/s PCIe x1 link at 0000:00:00.0 (capable of 31.504 > > > > > Gb/s with > > > > > 8.0 GT/s PCIe x4 link) > > > > > > [ 1.664931] pci 0000:00:00.0: BAR 0: assigned [mem > > > > > 0x18000000-0x180fffff] > > > > > > [ 1.671745] pci 0000:00:00.0: BAR 14: assigned [mem > > > > > 0x18100000-0x181fffff] > > > > > > [ 1.678634] pci 0000:00:00.0: BAR 6: assigned [mem > > > > > 0x18200000-0x1820ffff pref] > > > > > > [ 1.685873] pci 0000:01:00.0: BAR 0: assigned [mem > > > > > 0x18100000-0x18103fff 64bit] > > > > > > [ 1.693222] pci 0000:01:00.0: BAR 4: assigned [mem > > > > > 0x18104000-0x181040ff 64bit] > > > > > > [ 1.700577] pci 0000:00:00.0: PCI bridge to [bus 01-ff] > > > > > > [ 1.705814] pci 0000:00:00.0: bridge window [mem > > > > > 0x18100000-0x181fffff] > > > > > > [ 1.712972] pcieport 0000:00:00.0: PME: Signaling with IRQ 216 > > > > > > " > > > > > > Regarding the log you pasted, it seems that the clock is not feed > > > > > > to PHY > > > > > properly. > > > > > > > > > > > > Anyway, let's waiting for the v4 series, then make a try. Thanks > > > > > > for your > > > > > great help to make the double tests. > > > > > > > > > > > > > > > > My boards do not use CLKREQ# so I do not have that defined in pinmux > > > > > and I found that if I add MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B > > > PCIe > > > > > works on my board but this isn't a solution just a work-around (I > > > > > have boards that use the only two possible pins for CLKREQ as other > > > features). > > > > > > > > > > Similarly you will find on the imx8mm-evk if you comment out the > > > > > CLKREQ (which isn't required) the imx8mmevk will end up hanging like my > > > boards: > > > > [Richard Zhu] Hi Tim: > > > > Regarding the SPEC, the CLKREQ# is mandatory required, and should be > > > configured as an open drain, active low signal. > > > > And this signal should be driven low by the PCIe M.2 device to request the > > > REF clock be available(active low). > > > > So, there is such kind of CLKREQ# pin definition on i.MX8MM EVK board. > > > > > > > > Anyway, I think the external OSC circuit should be always running if there is > > > no CLKREQ# on your HW board design. > > > > > > > > > > The way I understand it is CLKREQ# allows the host to disable the REFCLK > > > when not needed for power savings so it would seem optional to implement > > > that and if not implemented should be left unconnected on the card. > > > > > [Richard Zhu] No, not that way. Regarding the SPEC, this signal is mandatory required. > > Especially for the L1ss usages. This signal would be OD(open drain), bi-directional, and might be > > driven low/high by RC or EP automatically if L1ss modes are enabled. > > You can make reference to the "ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a", or > > the chapter 5.5 L1 PM Substates of "PCI Express Base Specification, Rev. 4.0 Version 1.0". > > > > CLKREQ is only mandatory if you wish to support clock power > management. Many boards with a PCI host controller do not support > this. > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > index 5ce43daa0c8b..f0023b48f475 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > @@ -448,7 +448,9 @@ > > > > > > > > > > pinctrl_pcie0: pcie0grp { > > > > > fsl,pins = < > > > > > +/* > > > > > > > > > > MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B 0x61 > > > > > +*/ > > > > > > > > MX8MM_IOMUXC_SAI2_RXFS_GPIO4_IO21 > > > > > 0x41 > > > > > >; > > > > > }; > > > > > > > > > > I have PCIe working with a driver that I ported from NXP's kernel > > > > > which differs from your driver in that the PCIe PHY is not > > > > > abstracted to its own driver so I think this has something to do > > > > > with the order in which the phy is reset or initialized? The configuration of > > > gpr14 bits looks correct to me. > > > > [Richard Zhu] The CLKREQ# PIN definition shouldn't be masked. > > > > In the NXP's local BSP kernel, I just force CLKREQ# low to level up the HW > > > compatibility. > > > > That's might the reason why the PCIe works on your HW board although the > > > CLKREQ# PIN is not defined. > > > > This method is a little rude and violate the SPEC, and not recommended > > > although it levels up the HW compatibility. > > > > So I drop this method in this series. > > > > > > > > > > Sorry, I don't understand what you are saying here. Is there a change you are > > > going to make to v4 that will make this work for the evk and my boards? What > > > is that change exactly? > > [Richard Zhu] No. What I said above is that the CLKREQ# is forced to be low in NXP > > local BSP kernel. I guess this might be the reason why your board works. > > > > BIT11 and BIT10 of IOMUXC_GPR14 can be used to force the CLKREQ# to be low. > > Set CLKREQ_OVERRIDE_EN(bit10) 1b1, then write one zero to CLKREQ_OVERRIDE(bit11). > > > > Ok, that makes sense. Those bits are not explained well in the > IMX8MMRM. As my board's external REFCLK is always enabled that must > gate the clock internally to the host controller block. > > I can confirm that asserting those GPR14 bits does resolve my issue: > > #define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL BIT(11) > #define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN BIT(10) > > /* > * for boards that do not connect CLKREQ#, > * override CLKREQ# and drive it low internally > */ > regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14, > IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL, 0); > regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14, > IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN, 1); > > Should this be added as a 'fsl,clkreq-unsupported' flag that needs to > be set true to implement the above code? > Richard, Sorry - spoke too soon. My test was flawed as I still was pinmuxing CLKREQ in my dt to work around the issue and after removed the above did not resolve my issue. The setting of OVERRIDE_EN was wrong above (should not be set to '1' but BIT(10) instead) but this code already exists in imx6_pcie_enable_ref_clk and is used for IMX8MM per your patch so this is not the issue. What makes my board work is to clear GPR14 bit9 (like the NXP kernel does) so I don't think this bit does what we think it does (select between internal and ext clk). I think setting it enables clock gating via CLKREQ#. This also points out that perhaps the CLKREQ_OVERRIDE logic should be moved to the new phy driver for IMX8MM. Best regards, Tim -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E5BF9C433EF for ; Fri, 22 Oct 2021 16:56:30 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id AB684611BD for ; Fri, 22 Oct 2021 16:56:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org AB684611BD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=gateworks.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ysmAGV2pUIEQGl8lt1LFi4v/d1hR4qDRDqqopnzKNvs=; b=uAnWg3vXaN4jL8 LrjdRLl4qGEYeZZE3y3gqVdzyMazVku7PYxbRsX433LpuzDC8eZ5lc9drzoSDcNjYrTGNEMis4Njn mH/21Z4e39vobq1QjpCOuD8Hu6fo7jsfh9M275pRTaUAbbktVz+pB97/gif35INhwuI3y1sJLjSTM Gv8tHwN6AzBRWiM6wqZCPVX/3DnsSN1bVnh5j/t7ealCc2xSDxDJQEBs5CZqOxmFLHFPusFK/NLR+ /vCqKUDVIeoY5dn/sjE09m+yRD4AsG/yy4ipN8M0n84yQ5BoYOZ0sqrUaPe82U4I+WSD7pDLFuAjd tSDSR4CnvlVgzZCkG5dQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mdxp7-00BYnU-7J; Fri, 22 Oct 2021 16:55:17 +0000 Received: from mail-pg1-x52b.google.com ([2607:f8b0:4864:20::52b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mdxp2-00BYlX-Jm for linux-arm-kernel@lists.infradead.org; Fri, 22 Oct 2021 16:55:14 +0000 Received: by mail-pg1-x52b.google.com with SMTP id q187so3871620pgq.2 for ; Fri, 22 Oct 2021 09:55:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gateworks-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XKk5S6zzVsLpJki6KUTEWM/bL8H8tlEpxuGdUAGLl0U=; b=ZoDtpaZS4cpVQQOKeR1r9y3j/BXLAxVmG1e5e0/Fgp6xIHmssCQBlBIwARimP9oy47 ntqMdiGiscx3uY2O31gZfk2+xLzd4uWEEnmg6K+pWUXT8DDGQRq00C+esyx9vv2laA/p 82QOLbwdZ3vdRqRY5Pm8XP4RQkTOyefm9NiwZyqE1wFY+EhcFBsyqfSjOonQ/152hAXp efmQOxvZUlr9mTsvN/hq2wvxecDZPr06xNX8DUuCtxYwlybn7QJREfbVRiVTZGYyBZbg fRq/vLw+yWaYQ/CzrBWolnvIr0X0W9vtvOk5UtTjaXbf3ik2tYn6c2jMbNb+5LHAFotZ /SZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XKk5S6zzVsLpJki6KUTEWM/bL8H8tlEpxuGdUAGLl0U=; b=FW71uLgR4OqjrZ8gvCeKHJQHw6ijA2jLUdQdy558BMbBM2Kn+Cd353mHDAck/Yx4hX 8RGMAGi0TeqhsgM5JFjTH2p1zeXWNgFC9IKyffXpHbHULrOd1m/ZS180ps68nFRJQbT7 r1zbUDm7PPcPFJj6ONoY3KLyGQmpc/vcgGu9PLncyKEVwg7w9Q7ik6mc0vI+pIM1Erk2 ozUUiBk49f/lKYT0AeWQbOvozH9uDS7WhS60P8KOHwbVeyIAYj5uf3LZvEUoNLhui51l o08/y8xvcI5Y/EBULNmzFK/3X75WKI8Taa7JISZiAfXRd9QiOF4zhlEPQgLmX4TaYQsS rrtg== X-Gm-Message-State: AOAM5335dIf+G3N7aTnQugTIiLV7Iuhq6tmtPdEiXVuoTGjZdSB5lDt5 j/uyviXGfVz1BP0QpZ7s6J9ys+bmOf0x4up6RezxPg== X-Google-Smtp-Source: ABdhPJzoKPJA+Npi33fozucBU+egiBMqOwtCUaF7Au9zs2T9f9BTF6ZYR/4JNltrZhWa99tRXVV72Zt8wSf2CHsQIJw= X-Received: by 2002:a63:788e:: with SMTP id t136mr660555pgc.432.1634921711960; Fri, 22 Oct 2021 09:55:11 -0700 (PDT) MIME-Version: 1.0 References: <1634028078-2387-1-git-send-email-hongxing.zhu@nxp.com> In-Reply-To: From: Tim Harvey Date: Fri, 22 Oct 2021 09:55:00 -0700 Message-ID: Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie support To: Richard Zhu Cc: Lucas Stach , Kishon Vijay Abraham I , "vkoul@kernel.org" , Rob Herring , "galak@kernel.crashing.org" , Shawn Guo , "linux-phy@lists.infradead.org" , Device Tree Mailing List , Linux ARM Mailing List , open list , Sascha Hauer , dl-linux-imx X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211022_095512_692903_9B3C6655 X-CRM114-Status: GOOD ( 68.16 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Oct 22, 2021 at 8:59 AM Tim Harvey wrote: > > On Thu, Oct 21, 2021 at 5:43 PM Richard Zhu wrote: > > > > > -----Original Message----- > > > From: Tim Harvey > > > Sent: Friday, October 22, 2021 12:25 AM > > > To: Richard Zhu > > > Cc: Lucas Stach ; Kishon Vijay Abraham I > > > ; vkoul@kernel.org; Rob Herring ; > > > galak@kernel.crashing.org; Shawn Guo ; > > > linux-phy@lists.infradead.org; Device Tree Mailing List > > > ; Linux ARM Mailing List > > > ; open list > > > ; Sascha Hauer ; > > > dl-linux-imx > > > Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie > > > support > > > > > > On Wed, Oct 20, 2021 at 8:32 PM Richard Zhu > > > wrote: > > > > > > > > > > > > > > > > > > Richard, > > > > > > > > > > What is this 'invalid resource' about? I see that with my downstream > > > > > IMX8MM PCIe driver as well and have been asked about it. > > > > > > > > > [Richard Zhu] Hi Tim: > > > > This complain is caused by the following codes in pcie-designware.c driver. > > > > I'm not sure that why there is only size assignment after the res valid check, > > > and do nothing if the res is invalid. > > > > It seems that it is an expected design logic refer to the later codes. > > > > if (!pci->atu_base) { > > > > struct resource *res = > > > > > > > platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu"); > > > > if (res) > > > > pci->atu_size = resource_size(res); > > > > pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > > if (IS_ERR(pci->atu_base)) > > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > > } > > > > > > > > Since the default offset is used on i.MX8MM, the "atu" is not specified in > > > i.MX8MM PCIe DT node, so there is no real res at all. > > > > Then, devm_ioremap_resource() would complain the invalid resource. > > > > > > I think you are saying a change should be made like this: > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c > > > b/drivers/pci/controller/dwc/pcie-designware.c > > > index a945f0c0e73d..3254f60d1713 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -671,10 +671,11 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > if (!pci->atu_base) { > > > struct resource *res = > > > > > > platform_get_resource_byname(pdev, > > > IORESOURCE_MEM, "atu"); > > > - if (res) > > > + if (res) { > > > pci->atu_size = resource_size(res); > > > - pci->atu_base = devm_ioremap_resource(dev, > > > res); > > > - if (IS_ERR(pci->atu_base)) > > > + pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > + } > > > + if (!pci->atu_base || IS_ERR(pci->atu_base)) > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > } > > > > > > so that it looks like this: > > > if (!pci->atu_base) { > > > struct resource *res = > > > > > > platform_get_resource_byname(pdev, > > > IORESOURCE_MEM, "atu"); > > > if (res) { > > > pci->atu_size = resource_size(res); > > > pci->atu_base = > > > devm_ioremap_resource(dev, res); > > > } > > > if (!pci->atu_base || IS_ERR(pci->atu_base)) > > > pci->atu_base = pci->dbi_base + > > > DEFAULT_DBI_ATU_OFFSET; > > > } > > > > > > Right? > > [Richard Zhu] Yes, it is. The res shouldn't be remapped if it is invalid resource memory. > > Ok, I will submit a patch for that. > > > > > > > > > > > > > > > > [ 1.316305] imx6q-pcie 33800000.pcie: iATU unroll: enabled > > > > > > [ 1.321799] imx6q-pcie 33800000.pcie: Detected iATU regions: 4 > > > > > outbound, 4 inbound > > > > > > [ 1.429803] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.534497] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.538870] imx6q-pcie 33800000.pcie: Link up, Gen2 > > > > > > [ 1.550364] imx6q-pcie 33800000.pcie: Link up > > > > > > [ 1.550487] imx6q-pcie 33800000.pcie: PCI host bridge to bus > > > 0000:00 > > > > > > [ 1.565545] pci_bus 0000:00: root bus resource [bus 00-ff] > > > > > > [ 1.573834] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] > > > > > > [ 1.580055] pci_bus 0000:00: root bus resource [mem > > > > > 0x18000000-0x1fefffff] > > > > > > [ 1.586968] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400 > > > > > > [ 1.592997] pci 0000:00:00.0: reg 0x10: [mem > > > 0x00000000-0x000fffff] > > > > > > [ 1.599282] pci 0000:00:00.0: reg 0x38: [mem > > > 0x00000000-0x0000ffff > > > > > pref] > > > > > > [ 1.606033] pci 0000:00:00.0: supports D1 > > > > > > [ 1.610053] pci 0000:00:00.0: PME# supported from D0 D1 D3hot > > > > > D3cold > > > > > > [ 1.618206] pci 0000:01:00.0: [15b7:5002] type 00 class 0x010802 > > > > > > [ 1.624293] pci 0000:01:00.0: reg 0x10: [mem > > > 0x00000000-0x00003fff > > > > > 64bit] > > > > > > [ 1.631177] pci 0000:01:00.0: reg 0x20: [mem > > > 0x00000000-0x000000ff > > > > > 64bit] > > > > > > [ 1.638409] pci 0000:01:00.0: 4.000 Gb/s available PCIe bandwidth, > > > > > limited by 5.0 GT/s PCIe x1 link at 0000:00:00.0 (capable of 31.504 > > > > > Gb/s with > > > > > 8.0 GT/s PCIe x4 link) > > > > > > [ 1.664931] pci 0000:00:00.0: BAR 0: assigned [mem > > > > > 0x18000000-0x180fffff] > > > > > > [ 1.671745] pci 0000:00:00.0: BAR 14: assigned [mem > > > > > 0x18100000-0x181fffff] > > > > > > [ 1.678634] pci 0000:00:00.0: BAR 6: assigned [mem > > > > > 0x18200000-0x1820ffff pref] > > > > > > [ 1.685873] pci 0000:01:00.0: BAR 0: assigned [mem > > > > > 0x18100000-0x18103fff 64bit] > > > > > > [ 1.693222] pci 0000:01:00.0: BAR 4: assigned [mem > > > > > 0x18104000-0x181040ff 64bit] > > > > > > [ 1.700577] pci 0000:00:00.0: PCI bridge to [bus 01-ff] > > > > > > [ 1.705814] pci 0000:00:00.0: bridge window [mem > > > > > 0x18100000-0x181fffff] > > > > > > [ 1.712972] pcieport 0000:00:00.0: PME: Signaling with IRQ 216 > > > > > > " > > > > > > Regarding the log you pasted, it seems that the clock is not feed > > > > > > to PHY > > > > > properly. > > > > > > > > > > > > Anyway, let's waiting for the v4 series, then make a try. Thanks > > > > > > for your > > > > > great help to make the double tests. > > > > > > > > > > > > > > > > My boards do not use CLKREQ# so I do not have that defined in pinmux > > > > > and I found that if I add MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B > > > PCIe > > > > > works on my board but this isn't a solution just a work-around (I > > > > > have boards that use the only two possible pins for CLKREQ as other > > > features). > > > > > > > > > > Similarly you will find on the imx8mm-evk if you comment out the > > > > > CLKREQ (which isn't required) the imx8mmevk will end up hanging like my > > > boards: > > > > [Richard Zhu] Hi Tim: > > > > Regarding the SPEC, the CLKREQ# is mandatory required, and should be > > > configured as an open drain, active low signal. > > > > And this signal should be driven low by the PCIe M.2 device to request the > > > REF clock be available(active low). > > > > So, there is such kind of CLKREQ# pin definition on i.MX8MM EVK board. > > > > > > > > Anyway, I think the external OSC circuit should be always running if there is > > > no CLKREQ# on your HW board design. > > > > > > > > > > The way I understand it is CLKREQ# allows the host to disable the REFCLK > > > when not needed for power savings so it would seem optional to implement > > > that and if not implemented should be left unconnected on the card. > > > > > [Richard Zhu] No, not that way. Regarding the SPEC, this signal is mandatory required. > > Especially for the L1ss usages. This signal would be OD(open drain), bi-directional, and might be > > driven low/high by RC or EP automatically if L1ss modes are enabled. > > You can make reference to the "ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a", or > > the chapter 5.5 L1 PM Substates of "PCI Express Base Specification, Rev. 4.0 Version 1.0". > > > > CLKREQ is only mandatory if you wish to support clock power > management. Many boards with a PCI host controller do not support > this. > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > index 5ce43daa0c8b..f0023b48f475 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi > > > > > @@ -448,7 +448,9 @@ > > > > > > > > > > pinctrl_pcie0: pcie0grp { > > > > > fsl,pins = < > > > > > +/* > > > > > > > > > > MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B 0x61 > > > > > +*/ > > > > > > > > MX8MM_IOMUXC_SAI2_RXFS_GPIO4_IO21 > > > > > 0x41 > > > > > >; > > > > > }; > > > > > > > > > > I have PCIe working with a driver that I ported from NXP's kernel > > > > > which differs from your driver in that the PCIe PHY is not > > > > > abstracted to its own driver so I think this has something to do > > > > > with the order in which the phy is reset or initialized? The configuration of > > > gpr14 bits looks correct to me. > > > > [Richard Zhu] The CLKREQ# PIN definition shouldn't be masked. > > > > In the NXP's local BSP kernel, I just force CLKREQ# low to level up the HW > > > compatibility. > > > > That's might the reason why the PCIe works on your HW board although the > > > CLKREQ# PIN is not defined. > > > > This method is a little rude and violate the SPEC, and not recommended > > > although it levels up the HW compatibility. > > > > So I drop this method in this series. > > > > > > > > > > Sorry, I don't understand what you are saying here. Is there a change you are > > > going to make to v4 that will make this work for the evk and my boards? What > > > is that change exactly? > > [Richard Zhu] No. What I said above is that the CLKREQ# is forced to be low in NXP > > local BSP kernel. I guess this might be the reason why your board works. > > > > BIT11 and BIT10 of IOMUXC_GPR14 can be used to force the CLKREQ# to be low. > > Set CLKREQ_OVERRIDE_EN(bit10) 1b1, then write one zero to CLKREQ_OVERRIDE(bit11). > > > > Ok, that makes sense. Those bits are not explained well in the > IMX8MMRM. As my board's external REFCLK is always enabled that must > gate the clock internally to the host controller block. > > I can confirm that asserting those GPR14 bits does resolve my issue: > > #define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL BIT(11) > #define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN BIT(10) > > /* > * for boards that do not connect CLKREQ#, > * override CLKREQ# and drive it low internally > */ > regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14, > IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL, 0); > regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14, > IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN, 1); > > Should this be added as a 'fsl,clkreq-unsupported' flag that needs to > be set true to implement the above code? > Richard, Sorry - spoke too soon. My test was flawed as I still was pinmuxing CLKREQ in my dt to work around the issue and after removed the above did not resolve my issue. The setting of OVERRIDE_EN was wrong above (should not be set to '1' but BIT(10) instead) but this code already exists in imx6_pcie_enable_ref_clk and is used for IMX8MM per your patch so this is not the issue. What makes my board work is to clear GPR14 bit9 (like the NXP kernel does) so I don't think this bit does what we think it does (select between internal and ext clk). I think setting it enables clock gating via CLKREQ#. This also points out that perhaps the CLKREQ_OVERRIDE logic should be moved to the new phy driver for IMX8MM. Best regards, Tim _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel