Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC

[Rob Herring <robh@kernel.org>]

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