linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rob Herring <robh@kernel.org>
To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>,
	Kishon Vijay Abraham I <kishon@ti.com>
Cc: Vidya Sagar <vidyas@nvidia.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Andrew Murray <amurray@thegoodpenguin.co.uk>,
	Thierry Reding <treding@nvidia.com>,
	Jon Hunter <jonathanh@nvidia.com>,
	PCI <linux-pci@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kthota@nvidia.com" <kthota@nvidia.com>,
	Manikanta Maddireddy <mmaddireddy@nvidia.com>,
	"sagar.tv@gmail.com" <sagar.tv@gmail.com>
Subject: Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC
Date: Tue, 3 Nov 2020 13:48:06 -0600	[thread overview]
Message-ID: <CAL_JsqK-0vKJMCzTGa-1LJbgAA9m7dNaie_=dG9kX3jQxGmB+w@mail.gmail.com> (raw)
In-Reply-To: <DM5PR12MB1835187E112C8854D4483E42DA100@DM5PR12MB1835.namprd12.prod.outlook.com>

On Mon, Nov 2, 2020 at 4:38 PM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
>
> On Mon, Nov 2, 2020 at 21:16:52, Rob Herring <robh@kernel.org> wrote:
>
> > On Mon, Nov 2, 2020 at 9:12 AM Gustavo Pimentel
> > <Gustavo.Pimentel@synopsys.com> wrote:
> > >
> > > On Mon, Nov 2, 2020 at 14:27:9, Vidya Sagar <vidyas@nvidia.com> wrote:
> > >
> > > >
> > > >
> > > > On 11/2/2020 7:45 PM, Rob Herring wrote:
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar <vidyas@nvidia.com> wrote:
> > > > >>
> > > > >> DesignWare core has a TLP digest (TD) override bit in one of the control
> > > > >> registers of ATU. This bit also needs to be programmed for proper ECRC
> > > > >> functionality. This is currently identified as an issue with DesignWare
> > > > >> IP version 4.90a. This patch does the required programming in ATU upon
> > > > >> querying the system policy for ECRC.
> > > > >>
> > > > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > >> Reviewed-by: Jingoo Han <jingoohan1@gmail.com>
> > > > >> ---
> > > > >> V3:
> > > > >> * Added 'Reviewed-by: Jingoo Han <jingoohan1@gmail.com>'
> > > > >>
> > > > >> V2:
> > > > >> * Addressed Jingoo's review comment
> > > > >> * Removed saving 'td' bit information in 'dw_pcie' structure
> > > > >>
> > > > >>   drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++--
> > > > >>   drivers/pci/controller/dwc/pcie-designware.h | 1 +
> > > > >>   2 files changed, 7 insertions(+), 2 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > >> index b5e438b70cd5..cbd651b219d2 100644
> > > > >> --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > >> @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
> > > > >>          dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
> > > > >>                                   upper_32_bits(pci_addr));
> > > > >>          val = type | PCIE_ATU_FUNC_NUM(func_no);
> > > > >> +       if (pci->version == 0x490A)
> > > > >> +               val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT;
> > > > >>          val = upper_32_bits(size - 1) ?
> > > > >>                  val | PCIE_ATU_INCREASE_REGION_SIZE : val;
> > > > >>          dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
> > > > >> @@ -294,8 +296,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > > >>                             lower_32_bits(pci_addr));
> > > > >>          dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
> > > > >>                             upper_32_bits(pci_addr));
> > > > >> -       dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
> > > > >> -                          PCIE_ATU_FUNC_NUM(func_no));
> > > > >> +       val = type | PCIE_ATU_FUNC_NUM(func_no);
> > > > >> +       if (pci->version == 0x490A)
> > > > >
> > > > > Is this even possible? Are the non-unroll ATU registers available post 4.80?
> > > > I'm not sure. Gustavo might have information about this. I made this
> > > > change so that it is taken care off even if they available.
> > >
> > > The Synopsys DesignWare PCIe IP is highly configurable, therefore is
> > > dependable on what the design team has configured for their solution.
> > > Although Synopsys doesn't recommend the use of non-unroll ATU, the
> > > customers are free to select what they want for their design.

Then given the feature is not really tied to the IP version, using
version wasn't really a good choice. A better choice would have been a
quirk flag that platforms could set. Perhaps called
'iatu_unroll_enabled'...

> > Okay, then there's a bug in the driver if the version is set to 0x480A
> > or later and non-unroll is used:
> >
> > if (pci->version >= 0x480A || (!pci->version &&
> >        dw_pcie_iatu_unroll_enabled(pci))) {
> >
> > Probably can just drop the version checking. The detection should always work.
>
> Hi Rob,
>
> The "detection" is based on the assumption that the read value on
> PCIE_ATU_VIEWPORT register is 0xffffffff (which is a hard-coded value by
> design), if it's true then the iATU is unrolled and the function returns
> 1, otherwise is non-unrolled returns 0. So like you said it should always
> work, however, this code behavior was changed by Kishon on the following
> patch 2aadcb0cd39 ("PCI: dwc: Fix ATU identification for designware
> version >= 4.80"). His patch makes me believe that on keystone platform
> the read operation on that register causes some unpredicted behavior
> leads his platform to crash/abort, that's why he created this alternative
> version approach to avoid the "detection" algorithm.

Ah, h/w designers and their love for bus aborts...

> From what I'm seeing the only drivers that use this "version" approach
> are the keystone and intel-gw (which probably doesn't need it).
>
> To summarize, this is a workaround so that the keystone driver doesn't
> break independent of the controller IP version.

Keystone is also broken in another way. The dts files claim 16 in and
out regions, but the ATU region is 4KB. It would need 8KB for 16
regions as unroll has a stride of 512bytes for each region.

Rob

  reply	other threads:[~2020-11-03 19:48 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-29  5:39 [PATCH V3 0/2] Add support to configure DWC " Vidya Sagar
2020-10-29  5:39 ` [PATCH V3 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API Vidya Sagar
2020-10-29  5:39 ` [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC Vidya Sagar
2020-10-29 22:03   ` Jingoo Han
2020-10-30  6:45     ` Vidya Sagar
2020-11-02 14:15   ` Rob Herring
2020-11-02 14:27     ` Vidya Sagar
2020-11-02 15:11       ` Gustavo Pimentel
2020-11-02 21:16         ` Rob Herring
2020-11-02 22:38           ` Gustavo Pimentel
2020-11-03 19:48             ` Rob Herring [this message]
2020-11-02 23:02   ` Bjorn Helgaas
2020-11-03  3:27     ` Vidya Sagar
2020-11-03 21:07       ` Bjorn Helgaas
2020-11-04 11:43         ` Vidya Sagar
2020-11-04 16:22           ` Bjorn Helgaas
2020-11-04 17:40             ` Vidya Sagar

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_JsqK-0vKJMCzTGa-1LJbgAA9m7dNaie_=dG9kX3jQxGmB+w@mail.gmail.com' \
    --to=robh@kernel.org \
    --cc=Gustavo.Pimentel@synopsys.com \
    --cc=amurray@thegoodpenguin.co.uk \
    --cc=bhelgaas@google.com \
    --cc=jingoohan1@gmail.com \
    --cc=jonathanh@nvidia.com \
    --cc=kishon@ti.com \
    --cc=kthota@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=sagar.tv@gmail.com \
    --cc=treding@nvidia.com \
    --cc=vidyas@nvidia.com \
    --subject='Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).