Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 5 Jul 2023 10:49:27 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     "Verma, Achal" <a-verma1@ti.com>
Cc:     Vignesh Raghavendra <vigneshr@ti.com>,
        Tom Joseph <tjoseph@cadence.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Krzysztof Wilczy_ski <kw@linux.com>,
        Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        linux-omap@vger.kernel.org, linux-pci@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [EXTERNAL] Re: [PATCH] PCI: j721e: Fix delay before PERST#
 deassert
Message-ID: <20230705154927.GA62511@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a245938e-2140-5f77-ab91-6cfbd3ddf66c@ti.com>
Precedence: bulk

On Tue, Jul 04, 2023 at 09:36:43PM +0530, Verma, Achal wrote:
> On 7/3/2023 9:51 PM, Bjorn Helgaas wrote:
> > In subject, "Fix" doesn't convey much information.  Does it increase?
> > Decrease?  How much time are we talking about?  PERST# deassert is at
> > one end of the delay; what event is at the other end?
> 
> How about "Increase delay to 100ms for PERST# deassert from moment
> power-rails achieve operating limits"

Maybe something like "Delay 100ms T_PVPERL from power stable to PERST#
inactive" to match the language in the spec?

> > Is this delay for the benefit of the Root Port or for the attached
> > Endpoint?  If the latter, my guess is that some Endpoints might
> > tolerate the current shorter delay, while others might require
> > more, and it doesn't sound like "TI's K3 SoC" would be relevant
> > here.
>
> Its for the endpoints, TI's EVB doesn't exhibit any issues with
> 100us delay but some customer reported the issue with shorter delay.

I wouldn't bother mentioning "some custom platform implemented using
TI's K3 SOCs" then, because the problem is that the driver didn't
observe T_PVPERL, so the problem will happen with some endpoints but
not others.

> > Numbers like 100ms that come from the PCIe specs should have #defines
> > for them.  If we don't have one already, can you add one, please?
>
> Sure, will do it in next revision but should this go in some generic PCI
> header file or just pci-j721e.c

I think it should be in drivers/pci/pci.h so all the controller
drivers can use the same thing.  Obviously none of them *currently*
use it, although there are a bunch of "msleep(100)" and a few comments
that mention T_PVPERL.

Bjorn